Focus is not handled properly for dialogs.

Nancy-Salpepi commented 3 years ago

Test device MacBook Air (m1 chip)

Operating System 11.6

Browser chrome

Problem description https://github.com/phetsims/qa/issues/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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

pixelzoom commented 3 years ago

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 https://github.com/phetsims/fourier-making-waves/issues/194#issuecomment-929620651 - 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 commented 3 years ago

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

?

pixelzoom commented 3 years ago

Absolutely, good catch @Nancy-Salpepi.

pixelzoom commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

A couple of problems here. Questions in italics for @jessegreenberg:

(1) This dialog is opened by the sim when it detects a bad state:

Adding focusOnCloseButton works fine, and the close button gets focus when the dialog is opened. But when the dialog is closed, there's a focus problem. Nothing in the UI gets focus, and pressing tab sets focus to the browser's URL textfield. @jessegreenberg how should this be handled?

(2) The RewardDialog has 2 button in addition to the close (X) button:

There is currently no support in vegas.RewardDialog for setting focus to those 2 buttons, so I've set it to the close button. It seems kind of odd to default to the close button when the dialog has content that you can interact with. What is typical here - should focus start with some part of the dialog's interactive content?

When you close the RewardDialog, it's also odd that the "Check Answer" button gets focus, because that button becomes disabled after the dialog opens. Is it OK to return focus to a disabled button?

jessegreenberg commented 3 years ago

Nothing in the UI gets focus, and pressing tab sets focus to the browser's URL textfield.

This is expected. When the dialog is open, focus moves right to the close button even though highlights are still invisible. Pressing tab will then move focus to the next focusable element. In this case there is nothing available to focus so the browser moves focus to your URL textfield. I don't feel strongly but if you find it odd, feel free to make an issue in joist and we can potentially improve it.

What is typical here - should focus start with some part of the dialog's interactive content?

Intuition tells me focus should be placed on the interactive content instead of the close button. In this case "New Level" makes sense to me. Master now has a focusOnOpenNode option and setter in Dialog, though that is not available in Fourier 1.0 SHAs. If this is something that needs to be in Fourier 1.0 you could focus the newLevelButton in the Dialog's showCallback. Or you could cherry pick changes from https://github.com/phetsims/sun/issues/719 once it is done to get focusOnOpenNode and use it in RewardDialog.

Is it OK to return focus to a disabled button?

Yes, I think that is OK because PhET has decided to leave disabled components in the traversal order.

pixelzoom commented 3 years ago

Thanks for the answers @jessegreenberg.

Re (1)... I'll do nothing, if this is expected behavior.

Re (2)... Since this involves modifying vegas.RewardNode either way, and that's common-code, it seems like I should do this correctly. So I'll wait to cherry-pick phetsims/sun#719. That should also allow me to remove all the calls to focusCloseButton that I added, since that is now the default - correct?

On hold until phetsims/sun#719 is completed.

pixelzoom commented 3 years ago

Back to (1) - there's something really strange going on here.

To demonstrate, step through these 2 scenarios, both of which open the same "Oops" dialog, shown in https://github.com/phetsims/fourier-making-waves/issues/194#issuecomment-929641125. Recall that this is a dialog that is opened by the sim, when it detects an invalid state. That invalid state is an attempt to create a sawtooth waveform using cosines.

Scenario 1:

Start the sim, go to Discrete screen.
Set "Series" radio button group to "cos"
Tab to "Waveform" combo box, select "sawtooth" using the keyboard
The sim sets "Series" to "sin", then opens an "Oops" dialog.
The dialog's close button has focus. Close the dialog using the keyboard, by pressing Esc or Space.
Note that there is now no focus. Pressing tab moves focus to the browser's URL textfield. The user must repeated press Tab to get back to where they were, the "Waveform" combo box.

Scenario 2:

Start the sim, go to Discrete screen.
Set "Waveform" combo box to "sawtooth"
Tab to the "Series" radio button group and select "cos" using the keyboard
The sim sets "Series" to "sin", then opens an "Oops" dialog.
The dialog's close button has focus. Close the dialog using the keyboard, by pressing Esc or Space.
Note that focus is returned to "cos" button in the "Series" radio button group. The user is where they were when the invalid state was detected.

So depending on which component had focus when the dialog was opened, the sim may or may not have focus when the dialog is closed. This inconsistency smells like unintentional or buggy behavior. And it also seems like a usability problem - shouldn't focus be returned to the UI component that the user was interacting with - so they know what they were doing when the problem was detected by the sim, and so they don't have to traversal all the way through the sim again?

@jessegreenberg please contact me on Zoom when you have a few minutes to discuss this.

pixelzoom commented 3 years ago

@arouinfar and I discuss https://github.com/phetsims/fourier-making-waves/issues/194#issuecomment-933685632 via Zoom and impacts usability. She agreed that the current behavior feels buggy. Focus should be restored to the UI component that had focus at the time that the Oops dialog was opened.

jessegreenberg commented 3 years ago

Case 1 above is happening because the Node that had focus when the Dialog was opened is no longer focusable. When the Dialog opens the ComboBox closes and makes all of the ComboBoxListItemNodes invisible.

In general, this seems like OK behavior to me. If the previously focused item is removed from the traversal order when the Dialog opens then starting the traversal order from the top of the page makes sense. But I imagine we could set the Dialog's focusOnCloseNode to be the ComboBoxButton in this case. It would be set to the button when the ComboBox is open and set back to null when the ComboBox is closed.

pixelzoom commented 3 years ago

@jessegreenberg and I talked about Case 1 in https://github.com/phetsims/fourier-making-waves/issues/194#issuecomment-933685632, and discovered that it's a problem in ComboBoxListBox. The Property is modified before focus is restored to the ComboBoxButton. That issues was addressed in phetsims/sun#721, cherry-picked to sun's "fourier-making-waves-1.0" branch, and patched into dependencies.json in the above commit.

jessegreenberg commented 3 years ago

In https://github.com/phetsims/sun/issues/719 we are removing Dialog.focusCloseButton and the last usages are the ones added to fourier for this issue. @pixelzoom can you please remove them and assign back to me when done so I can remove focusCloseButton from Dialog?

pixelzoom commented 3 years ago

@jessegreenberg I've removed calls to focusCloseButton in master and 1.0 branches in the above commit.

pixelzoom commented 3 years ago

RewardDialog has turned into a "keyboard navigation design" issue. Because it's common code (vegas), I've created https://github.com/phetsims/vegas/issues/96. This issue is on hold until that issue is addressed.

pixelzoom commented 3 years ago

Note to self.... Summary of what still needs to be done here:

[ ] cherry-pick phetsims/sun#719 to Fourier 1.0
[ ] verify that the default focus for dialogs (throughout the sim, expect for RewardDialog) is the close button
[x] complete keyboard navigation design and implementation for RewardDialog in phetsims/vegas#96
[x] cherry-pick vegas changes to Fourier 1.0

pixelzoom commented 3 years ago

@zepumph please let me know when https://github.com/phetsims/sun/issues/719 is closed, so that I can proceed with this issue.

pixelzoom commented 3 years ago

Focus is not behaving as desired for RewardDialog, see https://github.com/phetsims/vegas/issues/96#issuecomment-935091268. So unchecking those checklist items in https://github.com/phetsims/fourier-making-waves/issues/194#issuecomment-933937895, and on-hold until that issue is closed.

zepumph commented 3 years ago

I reviewed the common code issue over in https://github.com/phetsims/sun/issues/719#issuecomment-935109587. Unassigning.

pixelzoom commented 3 years ago

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

In the meantime, I'm not going to attempt to cherry-pick anything related to https://github.com/phetsims/sun/issues/719.

pixelzoom commented 3 years ago

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 https://github.com/phetsims/fourier-making-waves/issues/194#issuecomment-933937895.

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 commented 3 years ago

https://github.com/phetsims/sun/issues/719 has been reviewed and I collected a list of changes to cherry-pick in https://github.com/phetsims/sun/issues/719#issuecomment-937895180 so this is no longer on hold.

pixelzoom commented 3 years ago

Cherry-picks identified in https://github.com/phetsims/sun/issues/719#issuecomment-937895180 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 commented 3 years ago

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

[x] 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
[x] 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
[x] 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.
[x] 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.
[x] 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
[x] 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.
[x] 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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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

jessegreenberg commented 3 years ago

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 commented 3 years ago

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

phetsims / fourier-making-waves

Focus is not handled properly for dialogs. #194