phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

ComboBox has bad interaction between enabledProperty and displayOnlyProperty #617

Open pixelzoom opened 4 years ago

pixelzoom commented 4 years ago

Reported in https://github.com/phetsims/states-of-matter/issues/343.

The is a general problem with ComboBox, occurring on all platforms.

Steps to reproduce in SOM:

  1. Run the sim in Studio
  2. Find statesOfMatter.statesScreen.view.particleContainerNode.compositeThermometerNode.temperatureComboBox.enabledProperty and set it to false.
  3. Find statesOfMatter.statesScreen.view.particleContainerNode.compositeThermometerNode.temperatureComboBox.displayOnlyProperty (just above enabledProperty) and toggle it twice, on then off.
  4. Now try to interact with the combo box. Even though it looks disabled, the listbox will pop up.

This is occurring because enabledProperty and displayOnlyProperty are both changing pickableProperty, which determines whether the combo box is interactive.

Relevant code in ComboBox.js:

    const enabledObserver = enabled => {
      this.pickable = enabled;
      this.opacity = enabled ? 1.0 : options.disabledOpacity;
      this.button.setAccessibleAttribute( 'aria-disabled', !enabled );
    };
    this.enabledProperty.link( enabledObserver );
...
    this.displayOnlyProperty.link( displayOnly => {
      this.hideListBox();
      this.button.setDisplayOnly( displayOnly );
      this.pickable = !displayOnly;
    } );

Labeling as "dev:phet-io" because this displayOnlyProperty is specific to PhET-iO, and sim configuration via Studio.

Assigned to @ariel-phet for prioritization, assignment, and to decide whether it's blocking for any sims that are (or will be) in RC.

pixelzoom commented 4 years ago

Tracking for ph-scale in https://github.com/phetsims/ph-scale/issues/204.

kathy-phet commented 4 years ago

This is not blocking for RC.

arouinfar commented 4 years ago

The behavior described in https://github.com/phetsims/sun/issues/617#issue-686636169 is definitely buggy, but it's the result of an unrealistic set of customizations. I don't think we'd need to worry about including this in a maintenance release later, either.

zepumph commented 5 months ago

Noting here that when combo box's displayOnlyProperty is true, you can still focus it, though you cannot interact with it at all. This differs from mouse input to me, because the mouse cursor is not "pointer" when displayOnlyProperty:true is set.

zepumph commented 5 months ago

@marlitas @kathy-phet and I just had a conversation brainstorming this exact problem in regards to spinners over in https://github.com/phetsims/sun/issues/881 (from an MSAB design conversation). I have an idea about how to solve this generally.

To support enabled and displayOnly (both instrumented) in your common code component, for example ComboBox, you will need to create your own inputEnabledProperty that is a DerivedProperty based on the value of enabledProperty and displayOnlyProperty. I believe that it will be a pretty simple and nice pattern generally, but currently will not work because Node's implementation of enabled is setting the inputEnabled targetForwardingProperty directly, without a simple way to set the value of the target property instead, so I will need to keep brainstorming the best way to handle this generally.

https://github.com/phetsims/scenery/blob/b3e6c15d2ab4d2db60371ec1729da840395bfaa8/js/nodes/Node.ts#L4477-L4479