ComboBox PhET-iO design pattern

[arouinfar]

Several sims currently undergoing PhET-iO instrumentation contain ComboBox, see:

• PhET-iO instrumentation ph-scale#92
• Instrument for PhET-iO projectile-motion#177
• Instrument for PhET-iO states-of-matter#245

In design reviews @kathy-phet and I have discussed the general pattern what we would like featured in a ComboBox. If any of these requests are better handled on a sim-by-sim basis in the overrides, please let me know.

We would like the following to be phetioFeatured: true

• comboBox.displayOnlyProperty
• comboBox.enabledProperty
• comboBox.visibleProperty
• comboBox.listBox.*.visibleProperty

If possible, make these phetioFeatured: false

• comboBox.button.enabledProperty
• comboBox.button.visibleProperty

We also have some concerns regarding the ListBox. It seems like the listBox.opacityProperty and listBox.pickableProperty should be phetioReadOnly: true. However, they don't appear to be listening to the parent comboBox.opacityProperty and comboBox.pickableProperty, so it's possible for the ComboBox and ListBox to have different opacities. Similarly, it's possible to make the ComboBox unpickable, but the ListBox is still pickable. Both of these scenarios seem odd and undesirable to us.

• comboBox.listBox.visibleProperty should be phetioReadOnly: true since the ListBox should only appear through interaction with the button.

[arouinfar]

@kathy-phet and I talked a bit more about this today, and listBox.opacityProperty and listbox.pickableProperty are not high priority since these properties are not exposed to the client.

Addressing the checkboxes in #587 (comment) should be the priority for this issue.

[arouinfar]

@zepumph @samreid @chrisklus this issue needs to be addressed before delivering a dev version of ph-scale to the client.

[zepumph]

comboBox.enabledProperty

Currently this is only supported for default enabledProperties. In all three sims above it looks like comboboxes are not being passed a premade enabledProperty. So this will be good. I don't want to build out any more logic in ComboBox for this because it will be fixed when we use EnabledNode as a trait to add the enabledProperty instead.

We also have some concerns regarding the ListBox. It seems like the listBox.opacityProperty and listBox.pickableProperty should be phetioReadOnly: true. However, they don't appear to be listening to the parent comboBox.opacityProperty and comboBox.pickableProperty, so it's possible for the ComboBox and ListBox to have different opacities. Similarly, it's possible to make the ComboBox unpickable, but the ListBox is still pickable. Both of these scenarios seem odd and undesirable to us.

I would be simple enough to link the pickability and opacity of the listbox to the value of the property for the Combobox, but I would first want to make sure that this wasn't interferring with any patterns that may already be in place for ComboBox. I don't know much about combobox and as a result I'm a bit uncertain if that would cause problems.

[zepumph]

All the checkboxes have been implemented above.

All that remains is the above discussion point about linking opacity and pickability of the listbox to the combo box.

[zepumph]

@arouinfar please review and remove the blocking label if all looks as you desire.

[arouinfar]

Thanks @zepumph!

I noticed that comboBox.button.visibleProperty is still phetioFeatured: true in ph-scale and states-of-matter (and I suspect the other sims too). Can you make it false?

Currently this is only supported for default enabledProperties. In all three sims above it looks like comboboxes are not being passed a premade enabledProperty. So this will be good. I don't want to build out any more logic in ComboBox for this because it will be fixed when we use EnabledNode as a trait to add the enabledProperty instead.

That seems reasonable to me. I'll make a note in case I run into a sim in the future where comboBox.enabledProperty needs to be featured in the overrides.

I would be simple enough to link the pickability and opacity of the listbox to the value of the property for the Combobox, but I would first want to make sure that this wasn't interferring with any patterns that may already be in place for ComboBox.

Good to know. I think this may take may merit a bigger discussion, but it's not pressing for any of the PhET-iO dev deliveries.

[zepumph]

combo box button visibleProperty phetioFeatured false bug should be fixed above.

In 217dbaa I synced opacity and pickable of the listbox to the value of the ComboBox. I think my preference from here (if this is what we like and want to go with), is to make the listbox pickable/opactity Properties phetioReadOnly. I'll go ahead and implement that since it is in the top comment.

[zepumph]

list box has read-only opacity and pickle Properties above.

@pixelzoom can you please review 217dbaa

That is one-directional, but, when coupled with the fact that on the PhET-iO API the list box's Properties are read-only, I think that it is sufficient.

[pixelzoom]

I verified that comboBox.button.visiblityProperty is phetioReadOnly: false.

I verified that changing comboBox.opacityProperty results in the same change to comboBox.listbox.opacityProperty.

Changing comboBox.pickableProperty results in the same change to comboBox.listbox.pickableProperty. I'm not sure why this was necessary or desirable, because the only way to popup the listbox is to press the button, so this feels like unnecessary complication. But in any case, I can still select items in the listbox. @arouinfar is that what was intended? My recommendation would be to remove the linkage between pickableProperty.

[arouinfar]

Changing comboBox.pickableProperty results in the same change to comboBox.listbox.pickableProperty. I'm not sure why this was necessary or desirable, because the only way to popup the listbox is to press the button, so this feels like unnecessary complication. But in any case, I can still select items in the listbox. @arouinfar is that what was intended? My recommendation would be to remove the linkage between pickableProperty.

The behavior is definitely a bit funky. Now thatcomboBox.listBox.visibleProperty is read-only, I'm less convinced that the listbox needs to listen to the parent pickableProperty. I think it would fine to remove the linkage.

[pixelzoom]

@zepumph sounds like it would be OK to remove the linkage for pickableProperty. If you don't have any objections, could you make it so?

[zepumph]

That is a very great call. Thanks you two!