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

fix(theme-classic): hamburger menu control navigation by keyboard #8207

Merged
merged 2 commits into from
Oct 19, 2022

Conversation

jeferson-sb
Copy link
Contributor

@jeferson-sb jeferson-sb commented Oct 12, 2022

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

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

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 12, 2022
@netlify
Copy link

netlify bot commented Oct 12, 2022

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 14f4f89
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/6348b4d4f1a3d10009bcab69
😎 Deploy Preview https://deploy-preview-8207--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Oct 12, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 75 🟢 97 🟢 100 🟢 100 🟠 80 Report
/docs/installation 🟠 85 🟢 100 🟢 100 🟢 100 🟢 90 Report

@slorber slorber added pr: bug fix This PR fixes a bug in a past release. domain: a11y Related to accessibility concerns of the default theme labels Oct 13, 2022
@slorber
Copy link
Collaborator

slorber commented Oct 13, 2022

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?

@jeferson-sb
Copy link
Contributor Author

jeferson-sb commented Oct 14, 2022

@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.

@slorber
Copy link
Collaborator

slorber commented Oct 19, 2022

@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 <dialog> and inert. We are also not doing great regarding navigating the mobile drawer with JS disabled. Looks like it's not so easy to achieve, but we'll look into it.

Some useful refs:

In the meantime, this PR looks good enough to merge 👍

@slorber slorber merged commit f668b3b into facebook:main Oct 19, 2022
@slorber slorber added the to backport This PR is planned to be backported to a stable version of Docusaurus label Oct 19, 2022
@slorber slorber added backported This PR has been backported to a stable version of Docusaurus and removed to backport This PR is planned to be backported to a stable version of Docusaurus labels Nov 2, 2022
@timveld
Copy link

timveld commented Dec 8, 2022

After reviewing the discussion in this and related issues, I have some comments:

  • On the question of whether we should invest time in making the hamburger menu keyboard accessible: Absolutely! The present hamburger menu implementation may not be WCAG compliant and will certainly cause issues for users with visual or motor impairments who use a mobile device or their browser's zoom functionality.
  • The menu should close when escape is being pressed. Though not a (WCAG) requirement, it is definitely best practice and behavior that routine keyboard users would expect.
  • If the menu is being collapsed, focus should return to the hamburger button. This ensures predictable navigation and implicitly confirms the close action to a screen reader user.
  • When the menu is expanded, in the DOM the (invisible) site logo and search button are between the hamburger button and expanded menu. I suggest placing the expanded menu immediately after the hamburger button in the DOM, to ensure the relation between the hamburger and the content it collapses / expands is clear in all environments and to avoit potential virtual / keyboard focus related issues.

Then, we need to trap keyboard focus such that it cannot reach the visually hidden elements. There are two approaches:

  1. Putting the menu in a modal dialog as suggested in this discussion. This involves the following steps:
    • Use the html dialog element (or aria role=dialog) and ensure the dialog has an accessible name (could be "menu")
    • Trap keyboard focus such that it cannot move out of this dialog
  2. Follow the approach of the microsoft.com website, which was referred to in this discussion:
    • When the menu opens, move keyboard focus to the first item
    • When keyboard focus leaves the menu, collapse the menu (returning focus to the hamburger button)

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
Copy link
Collaborator

slorber commented Dec 8, 2022

Thanks @timveld , added a note to myself on #3030 to be sure to take into account your comment once we refactor this drawer menu. Ideally the accessibility should also stay decent when JS is disabled, which will likely not be easy to achieve completely 😅

@timveld
Copy link

timveld commented Dec 12, 2022

@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:

  • When the menu is being expanded, set the role=dialog on the menu (the advantage of doing that from javascript is that non-javascript users are not confronted with a nonfunctioning aria dialog)
  • Implement focus trap
  • Implement close menu on pressing escape
  • Place aria-expanded on the hamburger button and make sure it is synchronized with menu open/closed status

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR has been backported to a stable version of Docusaurus CLA Signed Signed Facebook CLA domain: a11y Related to accessibility concerns of the default theme pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants