Navigation block: Don't close submenu when it has focus #52177
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
luisherranz
added
[Focus] Accessibility (a11y)
luisherranz
commented
Jun 30, 2023
| // If focus is outside modal, and in the document, close menu | ||
| // event.target === The element losing focus | ||
| // event.relatedTarget === The element receiving focus (if any) | ||
| // When focusout is outsite the document, | ||
| // `window.document.activeElement` doesn't change. | ||
| if ( | ||
| selectors.core.navigation.menuOpenBy( store ).click && |
There was a problem hiding this comment.
We shouldn't be careful about not calling closeMenu multiple times even if the menu is closed. If anything, we should make the side effects that happen when the menu is closed (inside the closeMenu function) more declarative so they only trigger when the status changes.
There was a problem hiding this comment.
If I am not mistaken, I added that check because the focusout event was triggered in the children, so it is being called each time we change focus over the elements of the submenu.
There was a problem hiding this comment.
I think that works fine now with the new implementation, doesn't it?
|
Size Change: +25 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
|
This may have already been present, but I found an odd bug.
Screen.Recording.2023-06-30.at.3.32.03.PM.mov |
|
You're right. It feels weird not to flush the "Opened because it has focus" status when you are actively trying to close the menu. I've pushed a fix, let me know if it feels better now! |
SantosGuillamot
requested changes
Jul 5, 2023
| if ( $w->next_tag( | ||
| array( | ||
| 'tag_name' => 'UL', | ||
| 'class_name' => 'wp-block-navigation-submenu', |
There was a problem hiding this comment.
Right now, the fix doesn't work with the Page list block. It seems it doesn't have the wp-block-navigation-submenu. I believe we should either check the class wp-block-navigation__submenu-container or don't check any class. But I guess the first one should be safer.
| // If focus is outside modal, and in the document, close menu | ||
| // event.target === The element losing focus | ||
| // event.relatedTarget === The element receiving focus (if any) | ||
| // When focusout is outsite the document, | ||
| // `window.document.activeElement` doesn't change. | ||
| if ( | ||
| selectors.core.navigation.menuOpenBy( store ).click && |
There was a problem hiding this comment.
If I am not mistaken, I added that check because the focusout event was triggered in the children, so it is being called each time we change focus over the elements of the submenu.
| 'class_name' => 'wp-block-navigation-submenu', | ||
| ) | ||
| ) ) { | ||
| $w->set_attribute( 'data-wp-on--focusin', 'actions.core.navigation.openMenuOnFocus' ); |
There was a problem hiding this comment.
The same as focusout, the focusin event is propagated to all the focusable children. This means that the action is called each time the focus is changed between the submenu elements. Not sure if it is a problem, but we are calling that action more times than needed. Should we add a check in the action or is it okay?
There was a problem hiding this comment.
I think it's ok. I think openMenu and closeMenu should be more declarative, and calling them multiple times should be ok.
Base automatically changed from
fix/navigation-block-remove-aria-expanded-on-overlay
to
trunk
July 5, 2023 10:35
luisherranz
force-pushed
the
fix/navigation-block-close-submenu-when-in-focus
branch
from
9ea3bc9
to
93bc1e2
Compare
SantosGuillamot
approved these changes
Jul 5, 2023
|
Awesome. Thank you, Mario 🎉 @jeryj, I'm going to merge this now but feel free to get back to me if you think something can still be improved. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR builds on top of #52170 because it includes changes that only work with the modified logic of that PR.
Don't close the submenu when it has been focused because the mouse was hovering.
Why?
It's an edge case, but it shouldn't happen.
Spotted by @jeryj in Slack.
How?
Adding a new
focusinhandler that adds a"focus"prop to the"submenuOpenedBy"object and prevents the menu from closing until"focus"is removed on thefocusouthandler.Testing Instructions
tabkeypressesScreenshots or screencast
Before
Screen.Capture.on.2023-06-30.at.15-44-56.mp4
After
Screen.Capture.on.2023-06-30.at.15-42-52.mp4