Add keyboard navigation to this sim

[jessegreenberg]

We would like to add keyboard navigation to this sim. When I use the ?accessibility query parameter, it looks like everything is already working with the keyboard, though focus order and focus highlights could be adjusted. Pinging @pixelzoom because he is the responsible dev, but since since everything seems to be already working with keyboard nav, I am going to assume we can proceed.

@mbarlow12 could you please choose an intuitive focus order (as long as reset all button is last) and maybe adjust focus highlights where it is occluding text and deploy a dev version for the design team to review?

[pixelzoom]

@jessegreenberg said:

Pinging @pixelzoom because he is the responsible dev, but since since everything seems to be already working with keyboard nav, I am going to assume we can proceed.

I'm not super familiar with this sim, since I wasn't involved with it for very long. I inherited it and took it to the finish line. If it looks like keyboard navigation is working, great!

[mbarlow12]

Completed in above commits. Ready for review.

[pixelzoom]

@mbarlow12 Who would you like to review this?

[amanda-phet]

I'm looking at it now and can comment here when I have some feedback.

[pixelzoom]

Since I'm the responsible developer for this sim, I skimmed the code changed. Looks good.

[jessegreenberg]

Thanks @mbarlow12. One thing I noticed is that when the play/pause buttons in the lab screen are pressed, focus is lost when the pressed button becomes invisible. A fix might be one of

• Focus the visible button in the sim, adding sim specific code.
• Use Node.replaceChild() instead of toggling visibility.
• Some other new way for scenery to handle this?

I would recommend using Node.replaceChild() in this case. @pixelzoom @mbarlow12 do you have a preference?

[pixelzoom]

I don't understand what "when the pressed button becomes invisible" is referring to.

We typically prefer to use visible instead of replaceChild for performance reasons. And this sounds like it could be a general issue. So perhaps you should check in with @jonathanolson to see if there's a way to handle this in scenery.

[jessegreenberg]

Sounds good, thanks, we will brainstorm in scenery.

[amanda-phet]

One thing I noticed is that when the play/pause buttons in the lab screen are pressed, focus is lost when the pressed button becomes invisible.

@jessegreenberg I noticed this too, and agree that the focus shouldn't be lost. I recall a lot of trouble around that button's implementation, because we were using the button in a non-standard way- in the case of one ball the play doesn't become a pause button. Perhaps that is relevant.

[jessegreenberg]

Thanks @amanda-phet. In phetsims/scenery#694 we added a function to Node called swapVisibility that will swap the visibility of two child nodes and manage the keyboard focus. The above commit uses it, @pixelzoom @mbarlow12 could you please review?

[pixelzoom]

I'm on the fence about swapVisibility.

Node is huge (5116 lines) so we should think carefully about adding more stuff to it. Is swapVisibility really necessary? Should it be in the Accessibility mixin? Is this functionality that's going to be needed in other places? What would LabPlayPanel look like if it handled focus itself?

The name swapVisibility is also a bit misleading. The function is not responsible solely for visibility, it's also responsible for focus. And I would expect a function named swapVisibility to simply swap the visibility of its 2 arguments, not require that the first argument be visible.

[pixelzoom]

Btw... ToggleNode and the set of sun buttons that use it (BooleanRoundToggleButton,...) also have this same problem with focus. So let's make sure that whatever we do is generally applicable.

And why isn't this sim using ToggleNode in the first place?

[jessegreenberg]

Should it be in the Accessibility mixin? Is this functionality that's going to be needed in other places?...The name swapVisibility is also a bit misleading.

There is an issue in scenery for swapVisibility, thanks, Ill move these comments there.

Is swapVisibility really necessary?...What would LabPlayPanel look like if it handled focus itself?

I think so, the implementation of LabPlayPanel.togglePlayPauseButtonVisibility would look similar to the implementation of Node.swapVisibility:

togglePlayPauseButtonVisibility: function() {
  assert && assert( this.playButton.visible !== this.pauseButton.visible, 'the visibility of the play and pause buttons should alternate' );

  var visibleButton;
  var invisibleButton;
  if ( this.playButton.visible ) {
    visibleButton = this.playButton;
    invisibleButton = this.pauseButton;
  }
  else {
    visibleButton = this.pauseButton;
    invisibleButton = this.playButton;
  }

  var buttonWasFocused = visibleButton.focused;

  // swap visibility
  visibleButton.visible = false;
  invisibleButton.visible = true;

  // the button that is now visible should receive keyboard focus if it is focusable and the invisible
  // button had focus
  if ( buttonWasFocused && invisibleButton.focusable ) {
    invisibleButton.focus();
  }
}

[pixelzoom]

Since there's a precedent for Node methods affecting focus as a side effect, then I guess leave the name as swapVisibility. Up to @jonathanolson whether to change the requirement on ordering of the inputs. Up to @jessegreenberg and @jonathanolson to decide if this is really generally useful and is worth more Node bloat.

[pixelzoom]

Linking to the swapVisibility issue that @jessegreenberg mentioned above: phetsims/scenery#694

[jonathanolson]

Up to @jonathanolson whether to change the requirement on ordering of the inputs. Up to @jessegreenberg and @jonathanolson to decide if this is really generally useful and is worth more Node bloat.

I think it's generally useful, particularly since we should use it (when it makes sense) to future support a11y.

The order of parameters in swapVisibility is exactly what I hoped for, so I wouldn't change it unless there's significant disagreement.

[pixelzoom]

So back to the specific focus issue in this sim... As I asked in #104 (comment), why isn't this sim using ToggleNode. Shouldn't we prefer to add a11y to common UI components instead of sim-specific code? Or is the icon change in this case just too much of a mismatch with ToggleNode?

[jessegreenberg]

I forgot about ToggleNode, I will look into seeing if it can be used here. Definitely prefer to add a11y to common components.

[jessegreenberg]

ToggleNode is now being used in LabPlayPanel. I found another issue, buttons are firing for as long as 'enter' is held down because buttons fire with the click event. Pressing and holding down 'enter' on the play button creates way too many balls. I made a note of this in a sun issue where we will work on a fix: phetsims/sun#323

[mbarlow12]

Going through old, open issues and found this.

@jessegreenberg it appears that the 'enter' interaction has been handled for this sim. However, I noticed one other item. If the PlayPause button is focused with the keyboard, all mouse interactions appear to be disabled until you move focus to another element with either the mouse or keyboard.

Is this related to phetsims/sun#323?

[jessegreenberg]

Thanks @mbarlow12, yes related to that issue. Should be fixed in the above commit can you please verify?

[pixelzoom]

@mbarlow12 @jessegreenberg what is the status? Assigned to @mbarlow12 to review on 4/24.

[mbarlow12]

@jessegreenberg should focus shift to the slider on mouse interaction with the thumb? I'm seeing that an extra tab is required to regain focus after moving the slider (not the tweakers, though), and the focus is on the next element (e.g. the radio buttons after the play/pause button).

I saw this also in Coulomb's Law, so I think this is in either NumberControl since this doesn't happen in RIAW or Ohm's Law. I'll make a separate issue to investigate, but I think this one can be closed.