-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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
fix(theme-classic): hamburger menu control navigation by keyboard #8207
fix(theme-classic): hamburger menu control navigation by keyboard #8207
Conversation
✅ [V2]Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the deploy preview of this PR
|
Thanks I've always found this "keyDown" suspicious but it's here for a while 😅 For me I was able to open the mobile nav even if indeed the experience looked weird, particularly when tabbing would open it AND focus the next item at the same time. While we are fixing this, was wondering if our focus behavior is correct? Shouldn't we focus something inside the mobile nav after opening it? |
packages/docusaurus-theme-classic/src/theme/Navbar/MobileSidebar/Header/index.tsx
Outdated
Show resolved
Hide resolved
@slorber I believe the focus should be trapped inside in the sidebar when is it opened and moved directly to the first item initially, similar to a behavior in a Modal so you would add a little more JS to handle that. |
Looks like a good use-case for using Some useful refs:
In the meantime, this PR looks good enough to merge 👍 |
After reviewing the discussion in this and related issues, I have some comments:
Then, we need to trap keyboard focus such that it cannot reach the visually hidden elements. There are two approaches:
Both approaches (and perhaps others) are an appropriate fix with their respective pro's and con's. My preference, and (if I read you correctly) the preference of other participants in this discussion so far, is option 1 'modal dialog'. I like this approach because it is predictable, avoids the 'automatic collapse on losing focus' and best matches the visual appearance of this menu. |
@slorber it is actually possible to make a WCAG compliant pure CSS hamburger menu. See Pure css menu without javascript on codepen. The author is concerned that screen readers may not properly notify the user when the hamburger menu is opened, but I think this is not much of an issue because focus is being placed on the close button when the menu is opened. This has the effect of making my screen reader say, "main menu navigation landmark link close menu". That being said, I miss the focus trap and ability to close the menu with the escape key. But both functions can be added using Javascript without having any effect on non-javascript users. If non-javascript support is important, I suggest that we refactor the hamburger menu first along the lines of the codepen example. This will already result in a WCAG compliant solution. Then we can add javascript with the following functionality:
|
Pre-flight checklist
Motivation
Below certain viewport, the hamburger menu is shown for the left-side navigation on the site which can be used to open the navigation when clicked, but not when using via keyboard due to the use of
keyDown
event for toggling causing a strange behavior on any key press. I know the case is for a small number of users that use a keyword with tablet/smartphone, but if that same hamburger button were used on a laptop screen that would be a problem as well.Test Plan
Manual testing on docs site using keyboard only
Test links
Deploy preview: https://deploy-preview-8207--docusaurus-2.netlify.app/
Related issues/PRs
See #8095