search

phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

Dialogs should focus the close button by default when shown #719

Closed jessegreenberg closed 3 years ago

jessegreenberg commented 3 years ago

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

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 https://github.com/phetsims/sun/commit/214150ea6ba6b2b5975db9c35553b65c834a5097 this default behavior isn't necessary and it is an improvement to have this done in Dialog.

This was fixed in https://github.com/phetsims/sun/commit/3440aab18041d6f1b3e3d0b84122649f4da2ff6a, 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 commented 3 years ago

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

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

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

jessegreenberg commented 3 years ago

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

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

Raising priority to high, since this is blocking the next Fourier RC, as noted in https://github.com/phetsims/fourier-making-waves/issues/194#issuecomment-931748651.

jessegreenberg commented 3 years ago

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

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

jessegreenberg commented 3 years ago

It has been removed, back to @zepumph for https://github.com/phetsims/sun/issues/719#issuecomment-933911808. After review Ill collect the commits that are needed for a cherry-pick.

pixelzoom commented 3 years ago

Some feedback that should be addressed by this issue:

(1) Over in https://github.com/phetsims/vegas/issues/96#issuecomment-934994871, @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.

(2) Also in https://github.com/phetsims/vegas/issues/96#issuecomment-934994871, @arouinfar noted that Dialog seems to be setting focus to focusOnOpenNode regardless of how the Dialog was opened. If the Dialog was opened via the keyboard, pressing Tab therefore does not set focus to focusOnOpenNode, but to the next element in the traversal order. This seems like undesirable behavior, as demonstrated in RewardDialog.

So assigning back to @jessegreenberg.

zepumph commented 3 years ago

Thanks for the good work @jessegreenberg, let me know if you want to meet in person about this stuff.

jessegreenberg commented 3 years ago

Thanks for the thorough review @zepumph. Responding to comments in order:

Once I did phetsims/joist@d28d370 I actually could not get focusOnHideNode to work until I found a fix In f626442

Good catch, your change is correct.

I hate that there is no easy way to pass options up to Popupable.

I am not sure, is it too messy to pass all Dialog options right to the super type? Also may be better tracked in its own issue?

I saw a spot to simplify some stuff, can we get rid of

Sure, it doesn't seem like these setters are necessary any more. See https://github.com/phetsims/sun/commit/620d973bdcb877953e5e8286592447085f4e2f35

what about focusOnCloseNode and setFocusOnCloseNode in PhetMenu. Does it make sense to rename or change?

Yes, that is probably cleaner, how is https://github.com/phetsims/joist/commit/ce451e50edee0413490e9a90fcf9f8fe98265b3d?

Is it too clever to use the currently focused item by default

In my opinion this is the most reasonable default for Popupable, whenever a Popupable is dismissed I believe focus should generally return to the Node that had focus when it was open.

Shouldn't this be focusOnHideNode?

No, nodeWithFocusOnShow is a reference to the Node that had focus when the Popupable is shown so we are restoring focus to it on hide.

If we have a focusOnHideNode, shouldn't we assert that it is focusable? Can you state a case where it is expected and good to be graceful here? Same with focusOnShowNode.

It is possible that the Node has been removed from the scene graph or made invisible since the Popupable has been hidden/shown. If the Node is no longer focusable I think we should continue gracefully in this case.

Can we get rid of the setters on Popupable. I converted to use the option, and it just feels cleaner to me. Then we can move the assertions to the constructor. Otherwise, you will want to duplicate the assertions in both.

Sounds good to me, I think this is the same as the third bullet point? See the commit above.

jessegreenberg commented 3 years ago

Re https://github.com/phetsims/sun/issues/719#issuecomment-935099853, these are being tracked in different issues, 1) is being discussed in https://github.com/phetsims/vegas/issues/96 and 2) is being discussed in https://github.com/phetsims/joist/issues/750.

jessegreenberg commented 3 years ago

@zepumph and I discussed in a zoom call:

I hate that there is no easy way to pass options up to Popupable.

We made https://github.com/phetsims/sun/issues/722 to work on this part more.

Shouldn't this be focusOnHideNode?

To clarify this we would like to rename to nodeToFocusOnHide.

If we have a focusOnHideNode, shouldn't we assert that it is focusable?

After discussing this we agreed that it should assert out loudly. In these cases we always want focus to go to something else and a loud error will make sure that we specify a different Node that is focusable with focusOnHideNode.

pixelzoom commented 3 years ago

To clarify this we would like to rename to nodeToFocusOnHide

I'll just point out that most PhET options have names of the form "{{adjectives}}{{noun}}". Examples:

So focusOnHideNode seems preferrable to nodeToFocusOnHide.

zepumph commented 3 years ago

focusOnHideNode is already the name of the option, but we need another name for the property on the type that could be that provided option (focusOnHideNode), or have a default assigned each time the dialog is shown.

https://github.com/phetsims/sun/blob/620d973bdcb877953e5e8286592447085f4e2f35/js/Popupable.js#L111

I agree that that is a better name in general, but I think it would be confusing to use the same name. Mostly though my review just thought it should include the work "hide" in it.

jessegreenberg commented 3 years ago

https://github.com/phetsims/sun/issues/719#issuecomment-936461098 were done. For the assertions, I manually tested many Dialogs and didn't see the assertion hit. Local aqua has been running for a long time without error for me. @zepumph anything else/would you like to spot check? If not I will collect commits to cherry-pick before closing.

arouinfar commented 3 years ago

(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 https://github.com/phetsims/vegas/issues/96#issuecomment-935149674. Dialogs can be closed with the esc hotkey, so users don't need to tab to the close button, anyway.

pixelzoom commented 3 years ago

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

From https://github.com/phetsims/fourier-making-waves/issues/194#issuecomment-937250246:

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.

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

@jessegreenberg your commits are aligned well with what we talked about. Good work here and in https://github.com/phetsims/sun/issues/722

jessegreenberg commented 3 years ago

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:

joist:

pixelzoom commented 3 years ago

Thanks @jessegreenberg and @zepumph. Cherry-picks listed in https://github.com/phetsims/sun/issues/719#issuecomment-937895180 have been completed for Fourier, no problems experienced, and a test build seems to be working as expected.

pixelzoom commented 3 years ago

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

jessegreenberg commented 3 years ago

We are ready to close this.