fix(ui): make mobile nav toggle keyboard accessible#8801
fix(ui): make mobile nav toggle keyboard accessible#8801abhijeetnardele24-hash wants to merge 1 commit intonodejs:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryLow Risk Overview Removes the now-unused hidden checkbox styling and adds a focused test to assert correct button semantics and open/close behavior of the mobile navigation. Reviewed by Cursor Bugbot for commit f4e9cb3. Bugbot is set up for automated code reviews on this repo. Configure here. |
👋 Codeowner Review RequestThe following codeowners have been identified for the changed files: Team reviewers: @nodejs/nodejs-website Please review the changes when you have a chance. Thank you! 🙏 |
There was a problem hiding this comment.
Pull request overview
This PR updates the NavBar component in packages/ui-components to make the mobile navigation toggle properly keyboard accessible and semantically correct by replacing the hidden checkbox + label approach with a native <button> and adding ARIA state.
Changes:
- Replace the label/checkbox toggle with a native button that controls menu open/close state via React state.
- Add
aria-controlsandaria-expandedto reflect the controlled region and current open/closed state. - Remove the now-unused hidden checkbox styling and add a new NavBar test for toggle state.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/ui-components/src/Containers/NavBar/index.tsx | Swap hidden-checkbox toggle for a real button and toggle menu visibility via state + ARIA. |
| packages/ui-components/src/Containers/NavBar/index.module.css | Remove checkbox styles and adjust toggle styling for a button element. |
| packages/ui-components/src/Containers/NavBar/tests/index.test.jsx | Add a test asserting aria-expanded and visibility classes during toggling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <button | ||
| className={styles.sidebarItemTogglerLabel} | ||
| htmlFor="sidebarItemToggler" | ||
| role="button" | ||
| type="button" | ||
| aria-label={sidebarItemTogglerAriaLabel} | ||
| aria-controls="navbar-navigation" | ||
| aria-expanded={isMenuOpen} | ||
| onClick={() => setIsMenuOpen(previousState => !previousState)} | ||
| > | ||
| {navInteractionIcons[isMenuOpen ? 'close' : 'show']} | ||
| </Label.Root> | ||
| </button> | ||
| </div> | ||
|
|
||
| <input | ||
| className={classNames('peer', styles.sidebarItemToggler)} | ||
| id="sidebarItemToggler" | ||
| type="checkbox" | ||
| onChange={e => setIsMenuOpen(() => e.target.checked)} | ||
| aria-label={sidebarItemTogglerAriaLabel} | ||
| tabIndex={-1} | ||
| /> | ||
| <div className={classNames(styles.main, `hidden peer-checked:flex`)}> | ||
| <div | ||
| id="navbar-navigation" | ||
| className={classNames(styles.main, isMenuOpen ? 'flex' : 'hidden')} | ||
| > |
There was a problem hiding this comment.
aria-controls points at a hard-coded id (navbar-navigation). If multiple NavBar instances render on the same page (e.g., in Storybook/docs), this creates duplicate IDs and breaks the aria-controls relationship. Consider generating a unique id per component instance (e.g., via React useId) and wiring both aria-controls and the controlled element's id to that value.
| <button | ||
| className={styles.sidebarItemTogglerLabel} | ||
| htmlFor="sidebarItemToggler" | ||
| role="button" | ||
| type="button" | ||
| aria-label={sidebarItemTogglerAriaLabel} | ||
| aria-controls="navbar-navigation" | ||
| aria-expanded={isMenuOpen} | ||
| onClick={() => setIsMenuOpen(previousState => !previousState)} | ||
| > | ||
| {navInteractionIcons[isMenuOpen ? 'close' : 'show']} | ||
| </Label.Root> | ||
| </button> |
There was a problem hiding this comment.
The CSS module class name sidebarItemTogglerLabel is now applied to a <button>, which makes the naming misleading and harder to maintain (it’s no longer a label). Consider renaming the class (and corresponding usages) to reflect the new semantics (e.g., sidebarItemTogglerButton).
| await user.click(menuButton); | ||
|
|
||
| assert.equal(menuButton.getAttribute('aria-expanded'), 'true'); | ||
| assert.match(navigation.className, /\bflex\b/); | ||
|
|
||
| await user.click(menuButton); |
There was a problem hiding this comment.
This test claims to validate keyboard accessibility, but it only uses user.click(). To actually cover the reported issue, exercise keyboard interaction (e.g., user.tab() to focus the toggle, then user.keyboard('{Enter}') / '{Space}') and assert aria-expanded / visibility changes.
| await user.click(menuButton); | |
| assert.equal(menuButton.getAttribute('aria-expanded'), 'true'); | |
| assert.match(navigation.className, /\bflex\b/); | |
| await user.click(menuButton); | |
| await user.tab(); | |
| assert.equal(document.activeElement, menuButton); | |
| await user.keyboard('{Enter}'); | |
| assert.equal(menuButton.getAttribute('aria-expanded'), 'true'); | |
| assert.match(navigation.className, /\bflex\b/); | |
| await user.keyboard(' '); |
Description
Replaces the mobile navigation toggle's hidden-checkbox + label pattern with a real button so the control is focusable and exposed with the correct button semantics to keyboard and assistive technology users.
Changes
Label-driven mobile toggle with a nativebuttonaria-controlsandaria-expandedto the toggleValidation
node --enable-source-maps --import=global-jsdom/register --import=tsx --import=../../tests/setup.mjs --test src/Containers/NavBar/__tests__/index.test.jsxcorepack pnpm exec prettier --check packages/ui-components/src/Containers/NavBar/index.tsx packages/ui-components/src/Containers/NavBar/index.module.css packages/ui-components/src/Containers/NavBar/__tests__/index.test.jsxFixes #7895