Skip to content

fix(ui): make mobile nav toggle keyboard accessible #8801

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.

Sign up for GitHub

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

Open
abhijeetnardele24-hash wants to merge 1 commit into
base: main
Choose a base branch
from
+68 −24
Open

fix(ui): make mobile nav toggle keyboard accessible #8801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions packages/ui-components/src/Containers/NavBar/__tests__/index.test.jsx
View file
Open in desktop
Copy link
Copy Markdown
Member

@avivkeller avivkeller Apr 5, 2026

Choose a reason for hiding this comment

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

This test seems unnecessary

Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import { describe, it } from 'node:test';
import assert from 'node:assert/strict';

import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';

import NavBar from '..';

const Logo = () => <span>Node.js</span>;

describe('NavBar', () => {
it('uses a keyboard-accessible button to toggle the mobile navigation', async () => {
const user = userEvent.setup();

render(
<NavBar
as="a"
Logo={Logo}
pathname="/"
sidebarItemTogglerAriaLabel="Toggle navigation menu"
navItems={[
{ text: 'Learn', link: '/learn' },
{ text: 'About', link: '/about' },
]}
>
<a href="/search">Search</a>
</NavBar>
);

const menuButton = screen.getByRole('button', {
name: 'Toggle navigation menu',
});
const navigation = screen
.getByRole('navigation')
.querySelector('#navbar-navigation');

assert.ok(menuButton);
assert.ok(navigation);
assert.equal(menuButton.tagName, 'BUTTON');
assert.equal(menuButton.getAttribute('tabindex'), null);
assert.equal(menuButton.getAttribute('aria-expanded'), 'false');
assert.match(navigation.className, /\bhidden\b/);

await user.click(menuButton);

assert.equal(menuButton.getAttribute('aria-expanded'), 'true');
assert.match(navigation.className, /\bflex\b/);

await user.click(menuButton);
Comment on lines +44 to +49
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
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(' ');

Copilot uses AI. Check for mistakes.

assert.equal(menuButton.getAttribute('aria-expanded'), 'false');
assert.match(navigation.className, /\bhidden\b/);
});
});
14 changes: 4 additions & 10 deletions packages/ui-components/src/Containers/NavBar/index.module.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@
.sidebarItemTogglerLabel {
@apply block
cursor-pointer
appearance-none
border-0
bg-transparent
p-0
xl:hidden;

.navInteractionIcon {
Expand All @@ -49,16 +53,6 @@
}
}

.sidebarItemToggler {
@apply absolute
right-4
-z-10
size-6
-translate-y-[200%]
appearance-none
opacity-0;
}

.main {
@apply flex-1
flex-col
Expand Down
24 changes: 10 additions & 14 deletions packages/ui-components/src/Containers/NavBar/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import Hamburger from '@heroicons/react/24/solid/Bars3Icon';
import XMark from '@heroicons/react/24/solid/XMarkIcon';
import * as Label from '@radix-ui/react-label';
import classNames from 'classnames';
import { useState } from 'react';

Expand Down Expand Up @@ -55,25 +54,22 @@ const NavBar: FC<PropsWithChildren<NavbarProps>> = ({
<Logo />
</Component>

<Label.Root
<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>
Comment on lines +57 to +66
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

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

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

Copilot uses AI. Check for mistakes.
</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')}
>
Comment on lines +57 to +72
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
{navItems && navItems.length > 0 && (
<div className={styles.navItems}>
{navItems.map(({ text, link, target }) => (
Expand Down
Loading