search

phetsims / sun

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

Keyboard traversal is broken when ComboBoxListBox contains invisible items or the order of items is changed. #859

Closed pixelzoom closed 7 months ago

pixelzoom commented 8 months ago

Discovered in https://github.com/phetsims/my-solar-system/issues/306 for https://github.com/phetsims/qa/issues/1008 ...

Keyboard traversal is broken when ComboBoxListBox contains invisible items. Since PhET-iO allow clients to show/hide listbox items, this is a serious problem that affects PhET-iO sims. See https://github.com/phetsims/my-solar-system/issues/306 for examples in my-solar-system and density.

Git history suggested that @jessegreenberg added keyboard input to ComboBoxListBox, so I'll start by assigning to him. This is high priority -- my-solar-system is in a dev test, and need to create the release branch as soon as possible after the dev test is completed.

There also will need to be an evaluation of which published PhET-iO sims contains this bug, and whether that an MR is needed. Density is definitely broken, and there may be others.

pixelzoom commented 8 months ago

Looking at ComboBoxListBox, this is the relevant part of it's KeyboardListener:

else if ( keysPressed === 'arrowUp' || keysPressed === 'arrowDown' ) {
          const domEvent = sceneryEvent.domEvent!;
          assert && assert( domEvent, 'domEvent is required for this listener' );

          // prevent "native" behavior so that Safari doesn't make an error sound with arrow keys in
          // full screen mode, see #210
          domEvent.preventDefault();

          // Up/down arrow keys move the focus between items in the list box
          const direction = keysPressed === 'arrowDown' ? 1 : -1;
          const focusedItemIndex = this.visibleListItemNodes.indexOf( this.getFocusedItemNode() );
          assert && assert( focusedItemIndex > -1, 'how could we receive keydown without a focused list item?' );

          const nextIndex = focusedItemIndex + direction;
          this.visibleListItemNodes[ nextIndex ] && this.visibleListItemNodes[ nextIndex ].focus();

          // reserve for drag after focus has moved, as the change in focus will clear the intent on the pointer
          sceneryEvent.pointer.reserveForKeyboardDrag();
        }

Looking at this.visibleListItemNodes, it looks like that list is modified by calling setItemVisible:

  public setItemVisible( value: T, visible: boolean ): void {
    this.getListItemNode( value ).visible = visible;
    this.visibleListItemNodes = _.filter( this.listItemNodes, itemNode => itemNode.visible );
  }

PhET-iO sims to not change the visibility of items by calling setItemVisible. They instrument each item's visibleProperty, and expect that visibleProperty to be set directly via the PhET-iO API / Studio.

I see used of setItemVisible in cck-dc and fourier-making-waves. Those uses appear unrelated to PhET-iO.

pixelzoom commented 8 months ago

This line in ComboBoxListBox construtor is also wrong; it assumes that all items are initially visible:

this.visibleListItemNodes = listItemNodes.slice();
pixelzoom commented 8 months ago

Fixed in the above commit. Tested in my-solar-system, density, and fourier-making-waves. @jessegreenberg please review.

I don't follow how to test the use of setItemVisible in cck-dc, so @samreid please verify.

Next step will be to figure out what to do about published PhET-iO sims (like density) that have this bug.

samreid commented 8 months ago

Should I take the time to get up to speed on this issue, and figure out to sufficiently test cck-dc? Or would it be ok to describe how @pixelzoom or @jessegreenberg could exercise setItemVisible in cck-dc? (Lab screen => Advanced => Add Real Bulbs checkbox). Checking and unchecking that checkbox doesn't crash the sim, and I don't see any keyboard focus highlighting in the sim (probably not supported yet?)

pixelzoom commented 8 months ago

I've identified which published sims have these problem. They are:

Here's the process that I used to identify those sims:

PhET-iO API files for these sims contain the element "listBox", and therefore potentially have this problem.

From that list, these sims have been published (have links at https://bayes.colorado.edu/dev/phet-io/latest/):

From that list, these sims have alt input enabled (supportsInteractiveDescription: true in package.json):

Finally, from that list, these sims instrument listBox.*.visibleProperty, and therefore have this problem. This was determined by searching for "listBox" in Studio, and inspecting the instrumentation of items. These are the sims that would benefit from an MR.

Assigning to @brent-phet and @kathy-phet to decided whether to do an MR. Assuming that @jessegreenberg is OK with how I've fixed this, there is one commit to cherry-pick, involving 1 file: https://github.com/phetsims/sun/commit/a9815dfe29e65229c2c3bd6608233f05c3f7691e.

pixelzoom commented 8 months ago

@samreid asked:

... Should I take the time to get up to speed on this issue, and figure out to sufficiently test cck-dc?

Happy to pair with you on this on Zoom. But FYI, some useful documentation in LabScreenView would have made your involvement and this back-and-forth unnecessary. Here's the relevant code, which says nothing about why the item is being made visible, or why it was not visible in the first place.

    // Scroll to the real bulbs if selected, but not on startup
    model.addRealBulbsProperty.link( addRealBulbs => {
      this.circuitElementToolbox.carousel.setItemVisible( realBulbItem, addRealBulbs );

      if ( addRealBulbs ) {
        this.circuitElementToolbox.carousel.scrollToItem( realBulbItem );
      }
    } );
pixelzoom commented 8 months ago

Blocks publication of sims, and creation of my-solar-system 1.3 release branch, until the code review portion of this issue is completed.

pixelzoom commented 8 months ago

There is another serious problem with ComboBoxListBox, reported in https://github.com/phetsims/my-solar-system/issues/308. PhET-iO allows you to change the order of items in the listBox. ComboBoxListBox does not adjust the order of this.visibleListItemNodes (or this.listItemNodes!) to match the new order. This affects many more PhET-iO sims than the 4 identified in https://github.com/phetsims/sun/issues/859#issuecomment-1839087778, probably this list of 10:

@jessegreenberg How do you want to proceed with this? If it were me, I'd investigate getting rid of this.visibleListItemNodes and use the order of ComboBoxListBox content: VBox to do the traversal.

jessegreenberg commented 8 months ago

Thanks for looking into this, your change looks good. But for https://github.com/phetsims/sun/issues/859#issuecomment-1839389457, using content children directly does make sense. That is done in the following commit.

Now there is another problem. If PhET-iO makes the selected item invisible, it also breaks alt input for the ComboBox because the selected item cannot receive focus. I'm working on that next.

jessegreenberg commented 8 months ago

Here is a potential fix, but thought a review before checking into main would be better.

I had to replace listItemNodes because the order of that list got stale in the same way. @pixelzoom what do you think about this change?

```patch Subject: [PATCH] focusPanDirection -> limitPanDirection, see https://github.com/phetsims/center-and-variability/issues/564 --- Index: js/ComboBoxListBox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/ComboBoxListBox.ts b/js/ComboBoxListBox.ts --- a/js/ComboBoxListBox.ts (revision 77b9e080b04df4bd783dbc092c6e67b13f820d21) +++ b/js/ComboBoxListBox.ts (date 1701730941511) @@ -40,8 +40,8 @@ export default class ComboBoxListBox extends Panel { - // Nodes that correspond to items in the list - private readonly listItemNodes: ComboBoxListItemNode[]; + // The container for list items which will be provided to the panel. + private readonly content: Node; private readonly disposeComboBoxListBox: () => void; @@ -181,6 +181,8 @@ yMargin: options.yMargin / 2 } ) ); + this.content = content; + this.voiceOnSelectionNode = voiceOnSelectionNode; // variable for tracking whether the selected value was changed by the user @@ -227,7 +229,7 @@ // Only visible items can receive focus - using content children directly because PhET-iO may change // their order - const visibleItems = content.children.filter( child => child.visible ); + const visibleItems = this.getAllListItemNodes().filter( child => child.visible ); if ( keysPressed === 'escape' || keysPressed === 'tab' ) { @@ -268,8 +270,6 @@ } ); this.addInputListener( keyboardListener ); - this.listItemNodes = listItemNodes; - this.disposeComboBoxListBox = () => { for ( let i = 0; i < listItemNodes.length; i++ ) { listItemNodes[ i ].dispose(); // to unregister tandem @@ -309,11 +309,19 @@ return this.getListItemNode( value ).visible; } + /** + * Returns all list item Nodes, as children of the list box content in the correct order which may have changed + * from PhET-iO. + */ + private getAllListItemNodes(): ComboBoxListItemNode[] { + return this.content.children as ComboBoxListItemNode[]; + } + /** * Gets the ComboBoxListItemNode that corresponds to a specified value. Assumes that values are unique. */ private getListItemNode( value: T ): ComboBoxListItemNode { - const listItemNode = _.find( this.listItemNodes, ( listItemNode: ComboBoxListItemNode ) => listItemNode.item.value === value )!; + const listItemNode = _.find( this.getAllListItemNodes(), ( listItemNode: ComboBoxListItemNode ) => listItemNode.item.value === value )!; assert && assert( listItemNode, `no item found for value: ${value}` ); assert && assert( listItemNode instanceof ComboBoxListItemNode, 'invalid listItemNode' ); // eslint-disable-line no-simple-type-checking-assertions return listItemNode; @@ -323,19 +331,28 @@ * Gets the item in the ComboBox that currently has focus. */ private getFocusedItemNode(): ComboBoxListItemNode { - const listItemNode = _.find( this.listItemNodes, ( listItemNode: ComboBoxListItemNode ) => listItemNode.focused )!; + const listItemNode = _.find( this.getAllListItemNodes(), ( listItemNode: ComboBoxListItemNode ) => listItemNode.focused )!; assert && assert( listItemNode, 'no item found that has focus' ); assert && assert( listItemNode instanceof ComboBoxListItemNode, 'invalid listItemNode' ); // eslint-disable-line no-simple-type-checking-assertions return listItemNode; } /** - * Focuses the ComboBoxListItemNode that corresponds to a specified value + * Focuses the ComboBoxListItemNode that corresponds to a specified value. If the item for that value is not + * visible, focus is placed on the first visible item. */ public focusListItemNode( value: T ): void { - const listItemNode = this.getListItemNode( value ); - listItemNode.supplyOpenResponseOnNextFocus(); - listItemNode.focus(); + let listItemNode: ComboBoxListItemNode | undefined = this.getListItemNode( value ); + + // If the item Node is not visible, just place focus on the first available item. + if ( !listItemNode.visible ) { + listItemNode = _.find( this.getAllListItemNodes(), ( listItemNode: ComboBoxListItemNode ) => listItemNode.visible ); + } + + if ( listItemNode ) { + listItemNode.supplyOpenResponseOnNextFocus(); + listItemNode.focus(); + } } /** ```
pixelzoom commented 8 months ago

In Slack, @kathy-phet pointed out that only sims that support alt input are affected. Sims that have been published, have an instrumented comboBox, and support alt input are:

pixelzoom commented 8 months ago

@jessegreenberg Your patch looks and tests good, thanks! I went ahead and committed it.

I think it would be worth having QA verify this change. @KatieWoe can you fit that in?

To test:

KatieWoe commented 8 months ago

Did a quick check on main and it looked ok

pixelzoom commented 8 months ago

This is ready for MR. Adding @zepumph to assignees in case this can be added to the PhET-iO MR https://github.com/phetsims/phet-io/issues/1974.

zepumph commented 8 months ago

This is ready for MR. Adding @zepumph to assignees in case this can be added to the PhET-iO MR https://github.com/phetsims/phet-io/issues/1974.

Can you please tell me what commits go to what sims?

pixelzoom commented 8 months ago

12/7/23 phet-io meeting:

The commits to cherry-pick are all in ComboBoxListBox, as linked about in this issue. They are:

zepumph commented 8 months ago

And this is the starting list I will go off of for the MR. https://github.com/phetsims/sun/issues/859#issuecomment-1839724735

zepumph commented 7 months ago

Excellent. This doesn't need to be patched into MSS 1.3 because this fix occurred before the RC was taken.

zepumph commented 7 months ago

I'm having a bit of a hard time feeling confident about the provided list of sims that need the fix. Furthermore, I'm worried about cherry picking selectively like that and then running into trouble in the next 2 combo box issues because some branches don't have this change in there and so cherry picking fails.

I think I'd prefer a complete list of all hydrogen sims that use a combo box.

I tried to use binder for this, but I don't think I trust it, since it doesn't have CAV. Likely it isn't being tested with ?locales=* so it isn't handling the region/culture combo box. So I think I recommend adding CAV to this list, and proceeding with this for all three combo box patches.

    "beers-law-lab",
    "concentration",
    "density",
    "geometric-optics",
    "geometric-optics-basics",
    "greenhouse-effect",
    "molecule-polarity",
    "molecule-shapes",
    "molecule-shapes-basics",
    "ph-scale",
    "ph-scale-basics"
zepumph commented 7 months ago

I have gotten stuck. The last two patches didn't apply automatically and I think it will be fastest and easiest to work with @jessegreenberg sync to try to get this working.

zepumph commented 7 months ago

This issue has been MR'd successfully. While we were working on this, @jessegreenberg and I found two other regressions in Combo box that need to be MR'd. Noted in the above issues, and the MR will occur separately in those side issues.

Next we are ready to test.

zepumph commented 7 months ago

The above commits fixed a type error in ph-scale by importing Node (it was using the global, which doesn't have children)

zepumph commented 7 months ago

This was confirmed fixed by QA for all MR sims with a Combo Box. Closing