Add "display mode" to ComboBox

[pixelzoom]

1/17/19 phet-io meeting, requested by @kathy-phet:

Add {BooleanProperty} singleItemDisplayProperty, default false. Hides the arrow and vertical separator on the ComboBoxButton, and set's pickable: false on the ComboBox.

[pixelzoom]

This will involve instrumenting ComboBoxButton, which is currently not implemented.

[pixelzoom]

Implemented in the above commits.

While implementing this, I decided that displayOnlyProperty was a better name. Here it is in ComboBoxButton:

      this.displayOnlyProperty = new BooleanProperty( false, {
        tandem: options.tandem.createTandem( 'displayOnlyProperty' ),
        phetioDocumentation: 'disables interaction with the ComboBox and makes it appear like value display'
      } );

Here's how it looks in Studio with Beer's Law Lab. First with false (the normal state):

Here's how it looks where set to true. The arrow and vertical separator are hidden, and the combo box is not interactive.

@ariel-phet please assigning someone to review.

[pixelzoom]

phetioDocumentation was revised to:

phetioDocumentation: 'disables interaction with the ComboBox button ' +
                     'and makes it appear like a display that shows the current selection'

[samreid]

@chrisklus and I reviewed and it looks great, we made minor modifications above. Back to @pixelzoom to sign off on the minor changes. Close if all is well.

[pixelzoom]

👍

@samreid FYI, it looks like a bunch of commit above are for some other issue with title "Remove whitespace from tandem strings". Up to you whether to tag those commits with the correct issue number.

Closing.

[pixelzoom]

Reopening. I think this Property may be misplaced. It should probably be a Property of ComboBox, not ComboBoxButton. And in addition to changing the look of ComboBoxButton, it should hide the listBox.

@samreid your opinion?

[samreid]

I agree with your recommendations. @zepumph does that sound right to you? I recall we discussed whether this property should be in ComboBoxButton or ComboBox, but I don't recall our reasoning.

[pixelzoom]

displayOnlyProperty has been moved from ComboBoxButton to ComboBox. Please re-review.

Here's how it looks in Studio with Beer's Law Lab when set to false (normal ComboBox operation):

Here's how it looks where set to true. Note that the listbox has been hidden, the button's arrow and vertical separator are hidden, and the entire ComboBox is pickable:false.

[zepumph]

Everything looks good to me. I like putting it directly on the ComboBox.

I wasn't quite sure why we aren't resetting this property on reset? It makes sense to me why we may think that it is nice to not reset so that a client doesn't have to invoke a call again on reset to turn the combo box to display mode. I am worried that this is a new convention, that we don't do anywhere else in the project. Currently our "solution" is explained at the bottom of the wrapper index:

Simulation "Reset All" buttons can overwrite certain kinds of customizations when restoring the simulation to its initial state. If you don't need the Reset All button, then you can hide it using the API with NodeIO.visibleProperty.setValue(). If the ResetAllButton is made invisible, invoking customizations on the simulation in the Client.onSimInitialized() listener is enough. Otherwise, you will need to add a listener to the Reset All button to apply the customizations on the button fire event. This can also be done successfully in Client.onSimInitialized(). Note that it is recommended that you add Client callbacks (like onSimInitialized and onPhetioInitialized) from the launchSim options.

While this isn't the greatest solution, it is general. I would recommend making displayOnlyProperty reset with the rest of combo box. If there are disagreements about this, then I think we should open another issue to discuss this topic in general. Referencing https://github.com/phetsims/phet-io/issues/584 here is probably worth while too.

[pixelzoom]

Zoom discussion with @zepumph and @samreid:

displayOnlyProperty is not part of the user experience, it's not something that the user can change, and it therefore shouldn't be changeable via Reset All.

There's no advantage to wiring displayOnlyProperty into Reset All, although that would work OK since the PhET-iO wrapper client uses the customization callback is used on startup and Reset All. In order to wire displayOnlyProperty into Reset All, we'd need to add a reset method to all UI components between the ResetAllButton and the ComboBox.

This is the same policy that we've had for many years for (e.g.) customizing text elements - they do not reset on Reset All.

So in general, Properties that exist solely for PhET-iO customization should not be wired into Reset All. Assigning to @zepumph to make a note about this in PhET-iO Instrumentation doc. Then please close this issue.

[zepumph]

Over in #914, @samreid and I realized that this "displayOnly" characteristic was not implemented with respect to the PDOM.