instrument ComboBox better

[zepumph]

While working on phetsims/tandem#62, we found that there was weird buggyness in how the buttonNode was being instrumented, but the tandem set to be "enabled: false". This was the only place in the project where tandem.enabled was being used.

We should make sure that we are getting the appropriate events/customizations fro ComboBox.

[samreid]

I'm working on #396 and found that ComboBox should probably be using one FireListener per button rather than batching them all into a single listener.

[samreid]

Not used in graphing quadratics or faradays-law.

[samreid]

the tandem set to be "enabled: false". This was the only place in the project where tandem.enabled was being used.

tandem.enabled was removed in phetsims/tandem#62

Hence the remaining work for this issue is:

• weird buggyness in how the buttonNode was being instrumented (may have been addressed by removing tandem.enabled)
• We should make sure that we are getting the appropriate events/customizations fro ComboBox.
• ComboBox should probably be using one FireListener per button rather than batching them all into a single listener.
• Finish conversion to ES6.

[samreid]

In discussion with @chrisklus, we concluded that it seems best to schedule a PhET-iO design for ComboBox before deciding how to approach the remaining work for this issue. For instance, if we decide that PhET-iO clients should not be able to disable (make unpickable/transparent) individual elements in the popup box, we may prefer a single FireListener for the entire ComboBox. Conversely, if we keep customization of individual elements, it may make more sense for each of them to have their own FireListener.

Perhaps next time we PhET-iO design a simulation with ComboBox it would be a good idea to discuss this issue.

[samreid]

"meeting:phet-io" label is to make sure other team members are aware of the need for ComboBox PhET-iO design, probably the next time we have a PhET-iO design for a sim with a ComboBox.

[samreid]

@Denz1994 and I worked on a commit to get ComboBox working for phetsims/scenery-phet#439, we are not planning on it as a long term feature, but just as something to get things working for now.

[pixelzoom]

FYI, things are pretty broken right now. f75f530 is passing tandem to the wrong constructor (ComboBoxItemNode instead of ButtonNode). ButtonNode is being passed all of ComboBox's options, which is bad news when those are phetio options. ButtonNode was blowing away any tandem that it was passed (fixed in e0d1a3d).

Marking as high priority and "blocks-publication", because this is in no shape for production.

[pixelzoom]

I see that "Finish conversion to ES6" was check off in #405 (comment). Not even close, I'm going to uncheck it. There's no use of class, and it's still using inherit, var....

[samreid]

Today we discussed ComboBox PhET-iO API design:

• set the default value (provided by Property)
• make it unpickable (provided by Node)
• make it possible to remove elements from ComboBox (provided by ComboBoxListItemNode.visibleProperty)
• change the text name of elements, like "mystery object" or "object 1"
• Put it in a mode where you get rid of the triangle and line. Completed in Add "display mode" to ComboBox #451

Brainstorming:

• nice, but would be more tricky => reorder things
• add new things?

We can write up a proposal for how to set initial or default values for when the sim is starting up.
@kathy-phet said we can put that as a maybe for future work, when a client needs it.

[pixelzoom]

Re:

• change the text name of elements, like "mystery object" or "object 1"

It's important to note that this operation is generally not specific to, or performed on, ComboBox. ComboBox displays ComboBoxItems, with include the visual representations (Nodes) that corresponds to values. In the case where those visual representations contain text, we want to change the text at the source - NOT after the Nodes have been assembled, and most definitely NOT after they have been provided to the ComboBox.

[samreid]

We previously experimented with a PhET-iO mode where objects were customized as they were created, but it was ultimately removed it because it was incompatible with subtyping. We could open a new investigation for this, or look at using query parameters (or another upstream mechanism) to accomplish this.

@jonathanolson previously promoted making our UI components more mutable, and we have done that in some cases, such as changing colors. @pixelzoom said often it is simpler to dispose an entire UI component and create a new one, but @jonathanolson pointed out that can damage the state of interaction with an in-use component.

we want to change the text at the source - NOT after the Nodes have been assembled, and most definitely NOT after they have been provided to the ComboBox.

@pixelzoom also pointed out that it may be possible to change text after UI component construction as long as we correctly set maxWidth on individual components so the layout isn't disturbed.

we want to change the text at the source

One way to change the values at the source and have the changes reflected downstream would be to model them as axon Properties. This is what we typically do for other cases.

[pixelzoom]

But again, since I'm not sure everyone understands... Regardless of whether UI components are mutable, text strings should not be changed at the UI component. We use MVC, so in general there may be multiple views of one text string. So the correct place to make the change is at that one text string, not in each UI component.

[pixelzoom]

@pixelzoom also pointed out that it may be possible to change text after UI component construction as long as we correctly set maxWidth on individual components so the layout isn't disturbed.

If we're going to rely on maxWidth to solve this problem, then we should consider that we're typically using maxWidth only for translated string. If the PhET-iO client is allowed to change non-translated text (as was proposed for the formula in Beer's Law Lab), then we'll potentially need to start setting maxWidth for all text. And we'll need a way to test, since stringTest won't expose problems with non-translated strings.

[pixelzoom]

FYI, due to the "listbox" terminology that we decided on in #440, methods and event names in ComboBox and ComboBoxIO have been changed as follows:

showList -> showListbox
hideList -> hideListBox
'popupShown' -> 'listboxShown'
'popupHidden' -> 'listboxHidden'

[samreid]

4/5 of the main (non-brainstorm) requests in #405 (comment) are complete. I recommend to close this issue and open an issue for the remaining part. @pixelzoom or @zepumph anything else for this issue?

[pixelzoom]

I count 3/5 that are completed. These 2 are not done:

• How should PhET-iO customize text in components like ComboBoxes? #458, change the text name of elements, like "mystery object" or "object 1"
• Add "display mode" to ComboBox #451, Put it in a mode where you get rid of the triangle and line

But I'm OK with closing this and tracking the remaining tasks in their respective issues.

A general question... If issues like this one are the way that we're going to develop PhET-iO API requirements for common-code UI components, then how will we refer to this documentation in the future? Do I need to find this issue and then try to synthesize the decisions from the comments? Should the decisions made herein be documented in ComboBox.js? ComboBox.md? somewhere else?

[pixelzoom]

This issue is not ready to be closed. There are 2 TODO items that reference this issue in ComboBoxListBox - search for "TODO sun#405". They were preserved during (and pre-existed) the ComboBox refactor #430.

[pixelzoom]

@zepumph, @samreid, and I address the remaining TODO comments. So I'm OK with closing this if @zepumph and @samreid agree.

[zepumph]

Sounds great. Thanks all.