Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PhetMenu shouldn't need to use getNextFocusable() #748

Open
zepumph opened this issue Sep 23, 2021 · 2 comments
Open

PhetMenu shouldn't need to use getNextFocusable() #748

zepumph opened this issue Sep 23, 2021 · 2 comments
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Sep 23, 2021

While working on https://github.com/phetsims/phet-io/issues/1748, which took us to phetsims/scenery#1284 with @jessegreenberg, we realized that using document.activeElement most likely won't play nicely with PhET-iO. Furthermore, using PDOMUtils.getNextFocusable() in general for the PhetMenu and MenuItem seemed like a crutch, something that we don't really need to do.

@jessegreenberg if I misremember our conversation from last Friday, please fill in the gaps. I'd like to explore actively focusing the elements that we are supposed to be using. We can potentially use the snapshot-comparison tool to make sure that we don't have any regressions.

I will work on this as I work on phetsims/scenery#1284.

@jessegreenberg
Copy link
Contributor

That is right, thanks. I don't know why PDOMUtils.getNextFocusable and PDOMUtils.getPreviousFocusable are being used here. Maybe this (untested) patch could work.

Index: js/PhetMenu.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/PhetMenu.js	(revision d428469a77688d6ff6a0da092e4447f55b07d25d)
+++ js/PhetMenu.js	(date 1632416499826)
@@ -426,6 +426,8 @@
 
         const firstItem = this.items[ 0 ];
         const lastItem = this.items[ this.items.length - 1 ];
+        const indexOfFocus = this.items.indexOf( FocusManager.pdomFocusedNode );
+        assert && assert( indexOfFocus > -1, 'how did we receive a keydown while focus is not in the menu?' );
 
         const key = KeyboardUtils.getEventCode( domEvent );
 
@@ -437,13 +439,13 @@
         if ( key === KeyboardUtils.KEY_DOWN_ARROW ) {
 
           // On down arrow, focus next item in the list, or wrap up to the first item if focus is at the end
-          const nextFocusable = lastItem.focused ? firstItem : PDOMUtils.getNextFocusable();
+          const nextFocusable = lastItem.focused ? firstItem : this.items[ indexOfFocus + 1 ];
           nextFocusable.focus();
         }
         else if ( key === KeyboardUtils.KEY_UP_ARROW ) {
 
           // On up arrow, focus previous item in the list, or wrap back to the last item if focus is on first item
-          const previousFocusable = firstItem.focused ? lastItem : PDOMUtils.getPreviousFocusable();
+          const previousFocusable = firstItem.focused ? lastItem : this.items[ indexOfFocus - 1 ];
           previousFocusable.focus();
         }
         else if ( key === KeyboardUtils.KEY_ESCAPE || key === KeyboardUtils.KEY_TAB ) {

Or maybe it could be even better getting rid of firstItem and lastItem since we are calculating the index anyway.

@zepumph
Copy link
Member Author

zepumph commented Nov 12, 2021

This work pertains to phetsims/scenery#1298.

@zepumph zepumph removed their assignment Mar 3, 2023
@jessegreenberg jessegreenberg self-assigned this Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants