UI: Apply user updates for mobile navigation accessibility #31401

yatishgoel · 2025-05-07T06:09:42Z

What I did

This PR significantly enhances the accessibility of the mobile navigation and addresses focus management issues, resolving the core concerns raised in #31380. It also incorporates the DOM order fix for desktop and mobile reading consistency from #31076.

Key improvements:

ARIA Attributes for Mobile Navigation:
- The mobile menu toggle button now correctly uses aria-label, aria-expanded, and aria-controls to properly communicate its state and relationship with the menu drawer (MobileNavigation.tsx).
- The addon panel toggle button also includes aria-label and aria-expanded.
Accessible Mobile Menu Drawer (MobileMenuDrawer.tsx):
- The menu drawer now has role="dialog", aria-modal="true", and an aria-label for clear anouncement to screen reader users.
- The content area within the drawer (holding the navigation links) is marked with role="navigation".
Focus Management and Trapping:
- A robust useFocusTrap hook has been implemented and applied to MobileMenuDrawer.tsx.
- When the mobile menu opens, focus is now programmatically moved into the drawer, targeting the first focusable element.
- Focus is trapped within the open menu, allowing navigation via Tab and Shift+Tab, and closing via the Escape key.
- Focus is correctly restored to the previously focused element (the menu button) when the drawer is closed.
- The main content area is marked with aria-hidden="true" and tabIndex="-1" when the mobile menu is open to prevent interaction with background content (Layout.tsx).
DOM Order for Reading Consistency (Addresses Fix Storybook accessibility order #31076):
- Desktop View: The DOM order in Layout.tsx ensures <SidebarContainer> renders before the main content (<MainContentMatcher>) for a logical reading order.
- Mobile View: The <MobileNavigation> trigger component is rendered first in the DOM within its mobile layout block, while CSS order is used for its visual placement at the bottom. This ensures screen readers encounter the navigation trigger in a logical sequence.
General Code Refinements:
- Centralized initial focus logic within useFocusTrap.
- Improved focusable element detection in useFocusTrap.
- Added id props and aria-controls for the MobileAddonsDrawer as well, preparing it for similar focus trap implementation if it contains interactive elements.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

stories
unit tests
integration tests
end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Run a sandbox for a relevant template, e.g., yarn task --task sandbox --start-from auto --template react-vite/default-ts
Open Storybook in your browser.
Desktop View:
- Using a screen reader (e.g., NVDA, VoiceOver), verify that the sidebar content is announced before the main story/docs content.
Mobile View (resize browser or use dev tools):
- Verify the mobile navigation bar appears visually at the bottom.
- Using a screen reader:
  - Check that the "Open navigation menu" button is focusable and correctly announces its purpose and state (collapsed).
  - Activate the button.
  - Verify the "Navigation menu" dialog is announced as open and modal.
  - Verify focus moves into the menu (e.g., to the first navigation item).
  - Navigate within the menu using Tab and Shift+Tab. Ensure focus stays within the menu.
  - Press Escape. Verify the menu closes and focus returns to the "Open navigation menu" button.
  - Verify the main content behind the menu was hidden from the screen reader (aria-hidden="true") when the menu was open.
- Repeat similar checks for the "Open addon panel" button and its drawer if it contains interactive elements (note: focus trap for addon drawer is suggested but might be implemented separately if not part of this direct change).

Documentation

Add or update documentation reflecting your changes
If you are deprecating/removing a feature, make sure to update
MIGRATION.MD

Checklist for Maintainers

When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts
Make sure this PR contains one of the labels below:
Available labels
- bug: Internal changes that fixes incorrect behavior.
- maintenance: User-facing maintenance tasks.
- dependencies: Upgrading (sometimes downgrading) dependencies.
- build: Internal-facing build tooling & test updates. Will not show up in release changelog.
- cleanup: Minor cleanup style change. Will not show up in release changelog.
- documentation: Documentation only changes. Will not show up in release changelog.
- feature request: Introducing a new feature.
- BREAKING CHANGE: Changes that break compatibility in some way with current major version.
- other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

Greptile Summary

Here's a concise summary of the key accessibility improvements made to the mobile navigation components:

Enhanced mobile navigation components with comprehensive accessibility features and proper DOM ordering.

Added proper ARIA attributes to navigation elements (role="dialog", aria-modal, aria-controls, aria-expanded) in MobileNavigation.tsx and MobileMenuDrawer.tsx
Implemented focus trap management with new useFocusTrap hook for modal dialogs
Fixed DOM order in Layout.tsx to ensure sidebar renders before main content for logical screen reader flow
Added aria-hidden and tabIndex controls to main content when mobile menu is open
Improved keyboard navigation with proper focus restoration and trap management within modal dialogs

The changes significantly improve screen reader compatibility and keyboard navigation while maintaining visual layout through CSS ordering rather than DOM manipulation.

…moving unnecessary comments and simplifying focus handling logic.

greptile-apps

_{6 file(s) reviewed, 6 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

code/core/src/manager/components/mobile/navigation/MobileMenuDrawer.tsx

code/core/src/manager/components/mobile/navigation/MobileAddonsDrawer.tsx

code/core/src/manager/components/mobile/navigation/MobileNavigation.tsx

code/core/src/manager/components/mobile/navigation/useFocusTrap.tsx

nx-cloud · 2025-05-07T06:14:18Z

View your CI Pipeline Execution ↗ for commit 18b9fa1.

Command	Status	Duration	Result
`nx run-many -t build --parallel=3`	✅ Succeeded	1m 17s	View ↗

☁️ Nx Cloud last updated this comment at 2025-06-28 20:33:56 UTC

…aria-controls in MobileNavigation

ndelangen · 2025-05-12T13:15:29Z

@MichaelArestad Do you have any input here?

MichaelArestad

@yatishgoel Nice!

@ndelangen These changes seem great. I am not an expert on aria attributes and I have not implemented anything like useFocusTrap though I understand the reasoning.

I believe this breaks a few Interaction tests in Navigation that were targeting elements with a specific title attribute. We shouldn't be using title attributes for this anyway. title attributes are not designed to be used for anything other than iframes.

Regarding the title attributes, they were being used as tooltips, which isn't great. In the near future, I do think we should probably:

Create new tooltips that are accessible, specifically the tooltips designed to show a short string (black background, white text).
Implement those tooltips in almost every situation where we use title.

code/core/src/manager/components/layout/Layout.tsx

MichaelArestad · 2025-05-16T15:59:28Z

@ndelangen Can you code review this when you get a chance? Thank you!

Sidnioulz

Thanks so much for undertaking this work, @yatishgoel! This is hard work so please don't take anything in my review as a criticism of your effort. I very much appreciate you giving this a go!

You applied a dialog role with an aria-modal attribute. That's great, and it's what I wrote in the issue so really this is my own fault! It's unfortunately missing a powerful element of the Dialog element: showModal. The showModal function lets you instruct the browser to open a modal and install a focus trap for you, which greatly reduces the amount of code to write and makes better use of standards.

You also used the navigation role in your code. It would be preferrable to use the nav element directly instead. Generally speaking (and as reminded on https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/navigation_role), it's preferable to use the HTML standard element rather than its inferred role and attributes.

I shouldn't have mentioned role in my issue. I sometimes refer to roles instead of elements out of habit because I don't know if the HTML markup is controlled and can be edited, but when a role can be fulfilled through the use of an element, it should. That allows for better future compatibility and easier maintenance.

To move forward, I suggest removing the focus trap implementation and using showModal instead, and switching to actual nav/dialog elements. Then, there would only be minor details left to address.

code/core/src/manager/components/layout/Layout.tsx

code/core/src/manager/components/mobile/navigation/MobileAddonsDrawer.tsx

code/core/src/manager/components/mobile/navigation/MobileMenuDrawer.tsx

code/core/src/manager/components/mobile/navigation/MobileNavigation.tsx

code/core/src/manager/components/mobile/navigation/useFocusTrap.tsx

…ate management. Updated MobileAddonsDrawer and MobileMenuDrawer to use dialog elements for better focus control. Removed unused useFocusTrap hook. Simplified MainContentMatcher by removing unnecessary props. Enhanced MobileNavigation to manage addon panel state more effectively.

Sidnioulz

A small bug left to fix and a refactor could be made to regroup the modal hooks into a single file, but this is great overall and would definitely fix the linked issue. Kudos, @yatishgoel!

code/core/src/manager/components/mobile/navigation/MobileAddonsDrawer.tsx

…ialog hook for improved dialog management. Introduced useModalDialog for better encapsulation of dialog logic and enhanced accessibility. Updated shortcut handling to skip events when a dialog is open.

code/core/src/manager-api/modules/shortcuts.ts

code/core/src/manager/components/mobile/navigation/MobileMenuDrawer.tsx

code/core/src/manager/hooks/useModalDialog.ts

…ling - Introduced `forceCloseDialog` function in MobileMenuDrawer to encapsulate dialog closing logic. - Updated onExited callback to use `forceCloseDialog` for cleaner code. - Enhanced useModalDialog to handle Escape key for closing the dialog and prevent default behavior. - Removed redundant dialog close logic from useModalDialog to streamline the component's functionality.

Sidnioulz

Congrats on fixing the animations! Can you please test the mobile addon panel again? I see a few remaining issues:

It's positioned in the middle of the screen instead of the bottom
The close button doesn't close the panel
It seems I have to press Esc 2-3 times to close the panel

- Fix MobileMenuDrawer and MobileAddonsDrawer appearing centered instead of bottom - Add full width positioning for mobile dialogs - Fix Esc key requiring double press in addon drawer - Fix close button not working properly - Ensure smooth exit animations with proper timing coordination - Override browser default modal positioning with &[open] selector

yatishgoel · 2025-06-14T10:05:38Z

Congrats on fixing the animations! Can you please test the mobile addon panel again? I see a few remaining issues:

It's positioned in the middle of the screen instead of the bottom

The close button doesn't close the panel

It seems I have to press Esc 2-3 times to close the panel

Thanks for the feedback! I actually just fixed those exact issues in both MobileAddonsDrawer.tsx and MobileMenuDrawer.tsx:

Positioning: Added position: 'fixed', bottom: 0, right: 0, left: 0, top: 'auto' with &[open] selector to override browser default modal positioning, plus maxWidth: '100vw' for full width.

Close Behavior: Converted MobileAddonsDrawer to use React Transition with the same working pattern as MobileMenuDrawer - added forceCloseDialog function that runs after exit animation completes. This should fix both the close button and single Esc key press.

Both mobile drawers now use identical patterns: React Transition + useModalDialog + forceCloseDialog for behavior, and the same CSS positioning overrides. MobileMenuDrawer was working correctly, so I used it as the reference to fix MobileAddonsDrawer. Both should now appear at bottom with full width and proper close behavior.

storybook-app-bot · 2025-06-14T10:18:02Z

Package Benchmarks

^{Commit: 18b9fa1, ran on 28 June 2025 at 20:21:54 UTC}

No significant changes detected, all good. 👏

Sidnioulz

Code does everything I'd like it to do! Now this is just a matter of getting the CI passing.

We can now turn our attention to the CI :) I haven't looked in detail at the CI reports, but if you get stuck, feel free to ask me to take over and I will have a look. It seems there are a few tests that need to be updated, maybe you could start with this? https://app.circleci.com/jobs/github/storybookjs/storybook/843273

Sidnioulz · 2025-06-23T11:17:39Z

@yatishgoel can you please update the tests affected by your change?

- Replace getByTitle() with getByLabelText() in test queries - Update MockPanel component to use aria-label instead of title - Align tests with accessibility improvements that replaced title with aria-label attributes - All MobileNavigation.stories.tsx tests now pass

- Replace 'button[title="Open navigation menu"]' with 'button[aria-label="Open navigation menu"]' - Fixes timeout issue in composition.spec.ts mobile test - Aligns e2e test with accessibility improvements that replaced title with aria-label

- Replace title attributes with aria-label in manager.spec.ts for better accessibility - Aligns with recent accessibility improvements in the codebase

- Replace title with aria-label for mobile close button in Panel.tsx - Fixes e2e test failure expecting 'aria-label="Close addon panel"' - Completes accessibility improvements for mobile addon panel - Aligns mobile panel close button with navigation accessibility standards

yatishgoel · 2025-06-28T20:20:09Z

@yatishgoel can you please update the tests affected by your change?

Hi @Sidnioulz,

I have successfully updated all tests affected by the accessibility changes and confirmed they're working locally.

MobileNavigation.stories.tsx - Updated selectors from getByTitle() to getByLabelText() (8/8 tests passing)
e2e-tests/manager.spec.ts - Updated mobile navigation selectors to use aria-label attributes
composition.spec.ts - Fixed mobile navigation selector
Panel.tsx - Added missing aria-label="Close addon panel" for mobile close button
Local testing: All 30 related tests pass, confirming accessibility fixes are production-ready

Thanks for your guidance!

yatishgoel added 2 commits May 7, 2025 11:27

Apply user updates for mobile navigation accessibility

156bf0f

Refactor useFocusTrap hook for improved clarity and performance by re…

c121938

…moving unnecessary comments and simplifying focus handling logic.

greptile-apps bot reviewed May 7, 2025

View reviewed changes

Enhance type safety in MobileMenuDrawer and useFocusTrap; streamline …

59c6166

…aria-controls in MobileNavigation

yannbf added bug accessibility ci:merged Run the CI jobs that normally run when merged. labels May 7, 2025

yatishgoel and others added 3 commits May 7, 2025 16:55

Merge branch 'next' into fix/mobile-nav-a11y-user-updates

23a3bd9

Removed duplicate MainContentMatcher and MainContentMatcher

a26d01d

Merge branch 'next' into fix/mobile-nav-a11y-user-updates

75d114c

ndelangen assigned Sidnioulz May 12, 2025

Sidnioulz added this to Accessibility Improvements May 12, 2025

Sidnioulz moved this to In review in Accessibility Improvements May 12, 2025

Sidnioulz removed this from Accessibility Improvements May 12, 2025

MichaelArestad requested changes May 13, 2025

View reviewed changes

code/core/src/manager/components/layout/Layout.tsx Outdated Show resolved Hide resolved

MichaelArestad requested a review from ndelangen May 16, 2025 15:59

Sidnioulz requested changes May 16, 2025

View reviewed changes

yatishgoel added 2 commits May 20, 2025 00:16

Merge branch 'next' into fix/mobile-nav-a11y-user-updates

22d7a24

yatishgoel requested a review from Sidnioulz May 19, 2025 18:47

Sidnioulz reviewed May 25, 2025

View reviewed changes

code/core/src/manager/components/mobile/navigation/MobileAddonsDrawer.tsx Outdated Show resolved Hide resolved

code/core/src/manager/components/mobile/navigation/MobileAddonsDrawer.tsx Outdated Show resolved Hide resolved

yatishgoel added 2 commits May 26, 2025 01:54

Refactor MobileAddonsDrawer and MobileMenuDrawer to utilize useModalD…

3c06a20

…ialog hook for improved dialog management. Introduced useModalDialog for better encapsulation of dialog logic and enhanced accessibility. Updated shortcut handling to skip events when a dialog is open.

Merge branch 'next' into fix/mobile-nav-a11y-user-updates

7a6fb43

yatishgoel requested a review from Sidnioulz May 29, 2025 17:01

Merge branch 'next' into fix/mobile-nav-a11y-user-updates

b44c5c5