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

[pixelzoom]

Discovered in phetsims/my-solar-system#306 for phetsims/qa#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 phetsims/my-solar-system#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]

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]

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

this.visibleListItemNodes = listItemNodes.slice();

[pixelzoom]

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]

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]

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

• density
• geometric-optics
• geometric-optics-basics
• greenhouse-effect

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.

• beers-law-lab
• buoyancy
• center-and-variability
• density
• energy-skate-park
• fourier-making-waves
• gas-properties
• gases-intro
• geometric-optics
• geometric-optics-basics
• greenhouse-effect
• keplers-laws
• models-of-the-hydrogen-atom
• molarity
• molecule-shapes
• molecule-shapes-basics
• my-solar-system
• ph-scale
• ph-scale-basics
• projectile-motion
• states-of-matter
• states-of-matter-basics

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

• beers-law-lab
• center-and-variability
• density
• geometric-optics
• geometric-optics-basics
• greenhouse-effect
• molecule-shapes
• molecule-shapes-basics
• ph-scale
• ph-scale-basics

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

• center-and-variability
• density
• geometric-optics
• geometric-optics-basics
• greenhouse-effect

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.

• density
• geometric-optics
• geometric-optics-basics
• greenhouse-effect

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: a9815df.

[pixelzoom]

@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]

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]

There is another serious problem with ComboBoxListBox, reported in phetsims/my-solar-system#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 #859 (comment), probably this list of 10:

• beers-law-lab
• center-and-variability
• density
• geometric-optics
• geometric-optics-basics
• greenhouse-effect
• molecule-shapes
• molecule-shapes-basics
• ph-scale
• ph-scale-basics

@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]

Thanks for looking into this, your change looks good. But for #859 (comment), 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.

[zepumph]

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