Dialogs should focus the close button by default when shown

[jessegreenberg]

From phetsims/fourier-making-waves#194. Focus behavior when opening a dialog is inconsistent because you currently need to call focusCloseButton explicitly after calling show().

This is because we used to only focus the button when the dialog was opened from alternative input. Calling focus would immediately show the focus highlight and we did not want to do that for mouse and touch input. The general pattern was

if ( button.isPDOMClicking() ) {
  dialog.show();
  dialog.focusCloseButton();
}

That is not necessary anymore since highlights have been improved to only be shown when the sim receives some form of alternative input.

Instead of a proliferation of focusCloseButton calls, lets make focusing the close button the default behavior for Dialog.

[jessegreenberg]

This was done in the above commit. In some spot checking I that Dialogs being opened from the PhET menu had focus going to the second interactive element. This is because of handleFocusCallback of MenuItem. After 214150e this default behavior isn't necessary and it is an improvement to have this done in Dialog.

This was fixed in 3440aab, which also gets rid of a usage of getNextFocusable and getGlobal.

@zepumph can you please review this so far? If this direction looks OK we can remove focusCloseButton and usages.

[jessegreenberg]

I also considered moving this focusOnOpenNode and focusOnCloseNode to popupable. But I decided not to because there isn't a reasonable default there. The default might involve getNextFocusable. Or the default might be null indicating no behavior and Dialog could set the default to be the close node.

@zepumph do you think this should be moved to popupable?

[zepumph]

I think that popupable is the place for this. I would highly recommend again using getNextFocusable (as I feel like you probably agree with), and instead just no-op when null. The reason is that it will be so very easy to do this now (at least I think so from the outside), and I could see us easily using this in whatever we popup next.

Good work with removing focusCloseButton.

[jessegreenberg]

Thanks @zepumph, Ill move these options into Popupable and set the close button as the default in Dialog.

[jessegreenberg]

I am having a hard time putting this in Popupable because of when Dialog calls this.sim.setAccessibleViewsVisible( true );. Popupable's listener on the isShowingProperty is called before Dialog's so it is trying to set focus on focusOnCloseNode before the Node to restore focus to has become visible (in the PDOM) again.

[zepumph]

What about instead of putting it in the listener, you put it directly in show/hide, after you set the Property? Would that work?

[pixelzoom]

Raising priority to high, since this is blocking the next Fourier RC, as noted in phetsims/fourier-making-waves#194 (comment).

[jessegreenberg]

Yes @zepumph, thanks! That worked really well. These are now in Popupable. I renamed the options because Popupable uses show/hide terminology instead of Dialog's open/close. Can you please review this change?

[jessegreenberg]

Wait, focusCloseButton still exists, back to me to remove that.

[arouinfar]

(1) Over in phetsims/vegas#96 (comment), @arouinfar's feedback for RewardDialog is that she'd like to see the traversal order be content -> closeButton -> browser URL textfield, instead of the current content -> browser URL textfield -> closeButton. I don't think this is specific to RewardDialog, and seems to make sense for any Dialog.

To clarify, @jessegreenberg mentioned that the current traversal order was more optimal for screen readers, so I am fine with leaving this as-is, as I said in phetsims/vegas#96 (comment). Dialogs can be closed with the esc hotkey, so users don't need to tab to the close button, anyway.

[pixelzoom]

Summary of Zoom discussion with @arouinfar about how this issue affects the next Fourier RC, whether it's blocking, etc.

From phetsims/fourier-making-waves#194 (comment):

The current implementation of RewardDialog is using the new focusOnShowNode option that was added to Dialog in #719. That work has been cherry-picked to Fourier 1.0. But nothing related to #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 #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.

If we can unblock this by Thursday 10/7 morning, I should be able to publish the next Fourier RC. Otherwise we'll be waiting until Wednesday 10/13.

[zepumph]

@jessegreenberg your commits are aligned well with what we talked about. Good work here and in #722

[jessegreenberg]

Thanks @zepumph. Here are the commits to cherry-pick, in order. I tried cherry-picking to test branches off of fourier 1.0 SHAs and saw things working OK. Let me know if you encounter a problem @pixelzoom.

sun:

• 214150e
• 3440aab
• 4a56833
• 1908da5
• abd1315
• f626442
• 620d973

joist:

• phetsims/joist@da02403
• phetsims/joist@9023172
• phetsims/joist@d28d370

[pixelzoom]

Thanks @jessegreenberg and @zepumph. Cherry-picks listed in #719 (comment) have been completed for Fourier, no problems experienced, and a test build seems to be working as expected.

[pixelzoom]

I'm not sure if there's more to do here, so back to @jessegreenberg.

[jessegreenberg]

We are ready to close this.