Focus is not handled properly for dialogs.

[Nancy-Salpepi]

Test device
MacBook Air (m1 chip)

Operating System
11.6

Browser
chrome

Problem description
phetsims/qa#711

When using keyboard nav, the close (X) button is already highlighted when the Keyboard Shortcuts dialog is opened. However, when the Info and Expand Sum dialogs are opened, it is not. This prevents the user from being able to hit "esc" to exit. In those instances, the "tab" button must first be pressed and then either "esc" or "space bar" to exit.

Visuals

Troubleshooting information:
!!!!! DO NOT EDIT !!!!!
Name: ‪Fourier: Making Waves‬
URL: https://phet-dev.colorado.edu/html/fourier-making-waves/1.0.0-rc.1/phet/fourier-making-waves_all_phet.html
Version: 1.0.0-rc.1 2021-09-28 15:44:23 UTC
Features missing: applicationcache, applicationcache, touch
Flags: pixelRatioScaling
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/94.0.4606.61 Safari/537.36
Language: en-US
Window: 1391x690
Pixel Ratio: 2/1
WebGL: WebGL 1.0 (OpenGL ES 2.0 Chromium)
GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.0 Chromium)
Vendor: WebKit (WebKit WebGL)
Vertex: attribs: 16 varying: 31 uniform: 1024
Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 80)
Max viewport: 16384x16384
OES_texture_float: true
Dependencies JSON: {}

[pixelzoom]

In addition to not setting focus on the close button, we're also not restoring focus to the push button when the dialog is closed.

@jessegreenberg please provide guidance here. Is there something that needs to be done to set focus when a Dialog is opened? Why doesn't Dialog handle that automatically?

[pixelzoom]

Here's the relevant code related to KeyboardHelpDialog, in KeyboardHelpButton.js:

    keyboardHelpDialogCapsule = new PhetioCapsule( tandem => {
      const keyboardHelpDialog = new KeyboardHelpDialog( content, {
        tandem: tandem
      } );
      keyboardHelpDialog.setFocusOnCloseNode( this );
      return keyboardHelpDialog;
    }, [], {
      tandem: tandem.createTandem( 'keyboardHelpDialogCapsule' ),
      phetioType: PhetioCapsule.PhetioCapsuleIO( Dialog.DialogIO )
    } );

...

    options.listener = () => {
      const keyboardHelpDialog = keyboardHelpDialogCapsule.getElement();
      keyboardHelpDialog.show();

      // if listener was fired because of accessibility
      if ( this.isPDOMClicking() ) {

        // focus the close button if the dialog is open with a keyboard
        keyboardHelpDialog.focusCloseButton();
      }
    };

So is every client that creates an accessible Dialog expected to call setFocusOnCloseNode after instantiation? Then check isPDOMClicking and call focusCloseButton after show? There's a lot of boilerplate here that I'll need to copy to make this work, for 3 instances of Dialog.

[pixelzoom]

I've added support for this in the above commits, in master and 1.0 branches.

Here's the boilerplate that I had to add in 3 places to make this work:

    const dialog = new SomeDialog(...);

    const button = new SomeButton( {
      listener: () => {
        dialog.show();
        if ( button.isPDOMClicking() ) {
          dialog.focusCloseButton();
        }
      },
      ...
    } );

    dialog.setFocusOnCloseNode( button );

@jessegreenberg is this the correct pattern?

[jessegreenberg]

The change you made will work and matches what is currently supported by Dialog. And that should fly for 1.0 release.

The pattern can generally be improved though and this can be handled in Dialog now. Checking the source of input with isPDOMClicking or SceneryEvent.isFromPDOM used to be required because we didn't want the highlight to appear from mouse or touch. But the highlight implementation has been improved so that this isn't necessary. And setFocusOnCloseNode shouldn't be needed, it is only needed if focus should return to something other than the Node that opened the Dialog.

The right way forward is to focus the close button by default when the Dialog is opened but offer an option to focus something else instead.

[pixelzoom]

Thanks @jessegreenberg. I removed the unnecessary calls to setFocusOnCloseNode in the above commits to master and 1.0.

Oh rats... I found another dialog that I need to handle, the Info dialog for Wave Game screen.

[pixelzoom]

Wave Game info dialog is handled in the above commits, for master and 1.0.

[pixelzoom]

Slack conversation with @jessegreenberg

Chris Malley 3:13 PM
Why is it necessary to do this:

        if ( infoButton.isPDOMClicking() ) {
          infoDialog.focusCloseButton();
        }

… instead of simply calling focusCloseButton.

Jesse Greenberg 3:16 PM
It was historically necessary but isn't anymore. I tried to describe in #194 (comment) - it used to be that the focus highlight would appear as soon as focus() was called so we wanted to prevent that for mouse and touch input. But that is not the case anymore so isPDOMClicking checks shouldn't be needed either.

Chris Malley 3:18 PM
OK thanks, I think I’ll remove them then. Trying to pare it down to only what’s necessary, which seems to be calling dialog.focusCloseButton().

[Nancy-Salpepi]

@pixelzoom should the change be made for each amplitude box as well?

?

[pixelzoom]

Absolutely, good catch @Nancy-Salpepi.

[pixelzoom]

Oh boy... here's another. This one isn't opened by a user action, it's opened when the sim detects a specific state.

[pixelzoom]

And another, weeeee! For this one, it's common code, the button that opened it is then disabled, so it will be interesting to see where focus goes after closing it. And it has content buttons, so I'm not sure where the initial focus should be.

[pixelzoom]

@zepumph please let me know when phetsims/sun#719 is closed, so that I can proceed with this issue.

[pixelzoom]

Focus is not behaving as desired for RewardDialog, see phetsims/vegas#96 (comment). So unchecking those checklist items in #194 (comment), and on-hold until that issue is closed.

[zepumph]

I reviewed the common code issue over in phetsims/sun#719 (comment). Unassigning.

[pixelzoom]

Looks like there's still significant work to be done in phetsims/sun#719, so still on-hold until that is completed.

In the meantime, I'm not going to attempt to cherry-pick anything related to phetsims/sun#719.

[pixelzoom]

I discussed the RewardDialog with @arouinfar via Zoom. She's OK with publishing with the current behavior, so we'll consider the issue closed for Fourier 1.0. And I'll check off the related items in #194 (comment).

The current implementation of RewardDialog is using the new focusOnShowNode option that was added to Dialog in phetsims/sun#719. That work has been cherry-picked to Fourier 1.0. But nothing related to phetsims/sun#719 has been cherry-picked to Fourier 1.0, so the RewardDialog is not working correctly in the 1.0 branch. It's setting options. focusOnShowNode, but Dialog ignores it. So phetsims/sun#719 is most definitely blocking the next RC, because there is now code in Fourier 1.0 that relies on new Dialog functionality that is not yet present in Fourier 1.0.

[jessegreenberg]

phetsims/sun#719 has been reviewed and I collected a list of changes to cherry-pick in phetsims/sun#719 (comment) so this is no longer on hold.

[pixelzoom]

Cherry-picks identified in phetsims/sun#719 (comment) have been completed and patched into Fourier 1.0 branch.

This is ready for verification in the next RC. This has been a long, complicated issue, so I'll need to summarize what and how to verify.

[pixelzoom]

To verify in the next RC, test each of these scenarios. You may want to reload the sim for each scenario.

• In the Discrete screen:

1. Tab to the Info button in the control panel.
2. Press space to open the dialog. The close (X) button should have focus and be highlighted.
3. Press escape or space to close the dialog. Focus and highlight should return to the info button

• In the Discrete screen:

1. Set “Waveform” combo box to “sawtooth”
2. Tab to the Series radio button group, and choose “cos”
3. An “Oops” dialog opens. The close (X) button should have focus and be highlighted.
4. Press escape or space to close the dialog. Focus and highlight should return to the Series radio button group

• In the Discrete screen:

1. Set “Waveform” combo box to “sinusoid”
2. Set the Series radio button group to “cos”
3. Tab to the “Waveform” combo box and select “sawtooth”
4. An “Oops” dialog opens. The close (X) button should have focus and be highlighted.
5. Press escape or space to close the dialog. Focus and highlight should return to the “Waveform” combo box.

• In the Discrete screen:

1. Click on one of the amplitude value displays with the mouse
2. Press tab to move focus to the keypad dialog
3. Press escape or space to close the dialog. Verify that nothing has focus highlight in the UI.

• In the Wave Game screen:

1. Tab to the info button on the level-selection UI.
2. Press space to open the dialog. The close (X) button should have focus and be highlighted.
3. Press escape or space to close the dialog. Focus and highlight should return to the info button

• In the Wave Game screen:

1. Run the sim with ?showAnswers&showReward. This will show you the answers and show the Reward dialog after every correct answer.
2. Press the button for level 1.
3. Set amplitude sliders to the correct values.
4. Tab to the “Check Answer” button and press space
5. Reward Dialog will open, and the “New Level” button should have focus
6. Tab repeatedly to confirm that the traversal order is New Level > Keep Going > browser URL textfield > close (X) button.
7. Exit the dialog by pressing space while on one of the buttons, or escape at any time.

• In the Wave Packet screen:

1. Tab to the info button in the control panel.
2. Press space to open the dialog. The close (X) button should have focus and be highlighted.
3. Press escape or space to close the dialog. Focus and highlight should return to the info button

[Nancy-Salpepi]

@pixelzoom
To verify traversal order, can you clarify what you mean by "browser URL textfield?"

When I tab the traversal order is: New Level > Keep Going > first open tab in browser > tab repeatedly through everything at the top of the browser > close (X) button

If that is what you meant, then this issue is ready to be closed.

[arouinfar]

To verify traversal order, can you clarify what you mean by "browser URL textfield?"

@Nancy-Salpepi this is the browser's address bar, where you type in URLs.

When I tab the traversal order is: New Level > Keep Going > first open tab in browser > tab repeatedly through everything at the top of the browser > close (X) button

That's a bit unexpected. When I test in Chrome and Safari, the only part of the browser that gets focus is the address bar. I'm not forced to tab through any other browser controls. I'll assign to @pixelzoom to investigate.

[Nancy-Salpepi]

Here is a video I took when testing (I only had 1 tab open to make it shorter):

[jessegreenberg]

Sorry for the confusion by saying "browser url textfield", that specific location isn't important. What is important is that after the last focusable sim component focus goes to browser controls (whatever those may be) before going back to the first focusable element in the sim. Firefox has decided that all tabs and bookmarks are in their traversal order and that is not something that we can control.

[Nancy-Salpepi]

Thanks @jessegreenberg. I was actually on Mac + chrome not Firefox.
But yes, the focus order is as you described so I am closing.