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

A11Y improvements for DropdownNavbarItemDesktop #9439

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

srapilly
Copy link

@srapilly srapilly commented Oct 23, 2023

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

The goal of this PR is to improve the usability of the dropdown buttons, for example, the version dropdown:
image

Improvements

  • I used a button instead of a link with an ARIA Role, a button should be usable with the SPACE and ENTER key, it was only working with the ENTER key.

https://www.w3.org/TR/using-aria/#rule1

If you can use a native HTML element [HTML51] or attribute with the semantics and behavior you require already built in, instead of re-purposing an element and adding an ARIA role, state or property to make it accessible, then do so.

  • Escape key to close the dropdown

  • I removed aria-haspopup: true: When using this ARIA attributes, the dropdown should match the menu role

A popup element usually appears as a block of content that is on top of other content. Authors MUST ensure that the role of the element that serves as the container for the popup content is menu, listbox, tree, grid, or dialog, and that the value of aria-haspopup matches the role of the popup container.

See also https://adrianroselli.com/2017/10/dont-use-aria-menu-roles-for-site-nav.html

Test Plan

On https://deploy-preview-9439--docusaurus-2.netlify.app/

Keyboard:

  • should open with SPACE or ENTER
  • should close with ESCAPE

Hover

  • should work with JS disabled
  • should work with JS enabled
  • a click on the trigger should expand / close the dropdown

Test links

Deploy preview: https://deploy-preview-9439--docusaurus-2.netlify.app/

Related issues/PRs

#8478

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

netlify bot commented Oct 23, 2023

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 9861b32
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/6536d24f886be50008809c68
😎 Deploy Preview https://deploy-preview-9439--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 configuration.

Comment on lines +97 to 106
onMouseEnter={() => setShowDropdown(true)}
onMouseLeave={() => setShowDropdown(false)}
onKeyDown={onKeyDown}
className={clsx('navbar__item', 'dropdown', {
'dropdown--right': position === 'right',
'dropdown--show': showDropdown,
// When hydrated, handle hover state in JS
// It need to be kept in sync with the button onClick
'dropdown--hoverable': !isBrowser,
})}>
Copy link
Author

@srapilly srapilly Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be the same behavior as https://squareup.com navbar and https://www.radix-ui.com/primitives/docs/components/navigation-menu

The dropdown can be collapsed with a click on the button if it was open with a hover

role="button"
href={props.to ? undefined : '#'}
className={clsx('navbar__link', className)}
{...props}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this draft/test I removed from now the props spread

@github-actions
Copy link

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 51 🟢 97 🟢 92 🟢 100 🟠 89 Report
/docs/installation/ 🟠 73 🟢 98 🟢 92 🟢 100 🟠 89 Report
/docs/category/getting-started/ 🟠 83 🟢 100 🟢 92 🟢 90 🟠 89 Report
/blog/ 🟠 76 🟢 100 🟢 92 🟢 90 🟠 89 Report
/blog/preparing-your-site-for-docusaurus-v3/ 🟠 71 🟢 97 🟢 92 🟢 100 🟠 89 Report
/blog/tags/release/ 🟠 75 🟢 100 🟢 92 🟠 80 🟠 89 Report
/blog/tags/ 🟠 81 🟢 100 🟢 92 🟢 90 🟠 89 Report

Comment on lines +8 to +32
.dropdownButton {
font-size: inherit;
border: none;
background: none;
color: var(--ifm-navbar-link-color);
font-weight: var(--ifm-font-weight-semibold);
cursor: pointer;
padding: 0;
}

.dropdownButton:hover {
color: var(--ifm-navbar-link-hover-color);
}

.dropdownButton::after {
display: inline-block;
border-color: currentColor transparent;
border-style: solid;
border-width: 0.4em 0.4em 0;
content: '';
margin-left: 0.3em;
position: relative;
top: 2px;
transform: translateY(-50%);
}
Copy link
Author

@srapilly srapilly Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I used a native button, I needed to reset native styles. I guess everything from this file should be moved/refactored to this repo https://github.com/facebookincubator/infima

I didn't reuse the navbar__link in this current PR because they were some styles I didn't want, for example:

https://github.com/facebookincubator/infima/blob/79b4f86e4d161a8a61205ab0bc4676f97460f9ae/packages/core/styles/components/navbar.pcss#L135

@srapilly
Copy link
Author

srapilly commented Oct 26, 2023

Hi @Josh-Cena @slorber Still a draft, what do you think of the changes?

I could also make a smaller PR, that would still fix the issue (#8478). We could keep the link role=button, I think it would be better to use a button, but it has some styling impact.

@slorber
Copy link
Collaborator

slorber commented Oct 31, 2023

Hey

Thanks for your work on this
Now is not a good time for me to review 😅 I just had a baby
We'll review this when possible but can't promise it will be soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants