Navigation submenus: Allow Escape key to close the submenu and focus trigger #41774
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
alexstine
added
[Type] Bug
|
Size Change: +223 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
I tested this and it adds the maybe related to this PR
unrelated to this PR
|
|
@draganescu My thoughts.
I don't think this is really an issue, actually, one could argue it should have been working like this all along. The code that handles this detects outside focus, the only thing I changed was adding the Escape key. As a result, the focus was necessary to prevent Escape key from closing the menu and focus getting trapped. If others think it is a big UX issue, I could always split out the functions and only focus on Escape key vs. loss of focus on the menu. |
|
We discussed this Pull Request during the weekly Accessibility Team's bug-scrub: discussion happened in the accessibility channel on Slack. |
draganescu
approved these changes
Jun 20, 2022
There was a problem hiding this comment.
This works as described and fixes the problem. The one issue I found seems to be a non issue. Awesome 👏🏻
@alexstine maybe we could check if the focus is on an unrelated element (something unrelated to the menu) and not move the focus if that is true? Should be possible.
|
@draganescu I am sure we could but then it would not announce the closed state. If this was the case, users should reasonably expect to be able to Shift+Tab back to the open submenu only finding that it closed with no notification to them. I am not sure those UX would be any better. |
aristath
approved these changes
Jun 21, 2022
getdave
approved these changes
Jun 21, 2022
|
Soon to become the most approved PR on Github 😂 |
|
Thanks for the reviews everyone! If there is any further questions about UX, happy to make adjustments should they be needed. |
|
Late to the party, but I noticed a couple issues with this change. |
82 tasks
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?
Adding the ability to close open submenus via Escape key and focus the submenu trigger.
Why?
For better accessibility.
How?
Modify the JS on the front-end.
Testing Instructions
aria-expandedwas removed and the submenu trigger should not have lost focus.Screenshots or screencast