fix(MobileMenuToggle): trap keyboard focus inside the mobile menu - #3911
Conversation
When the mobile menu is open, Tab and Shift+Tab now wrap focus between the first and last visible focusable elements inside the surrounding `<nav>` instead of escaping to the underlying page content. Closes withastro#2697
🦋 Changeset detectedLatest commit: e1b4be9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for astro-starlight ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hello! Thank you for opening your first PR to Starlight! ✨ Here’s what will happen next:
|
HiDeoo
left a comment
There was a problem hiding this comment.
Thanks for the contribution 🙌
At first glance, I notice a few potential issues with the current approach:
- The current code would exclude the close button from the focus loop, which feels a bit odd to me when quickly testing it out.
- The manually maintained list of interactive elements could be difficult to maintain. For example, right now it misses
<summary>that we are using for sidebar groups. I think it would also need to be extended to include all possible interactive elements as users may add new ones in their own project. - If we change the viewport width to a point where the mobile menu becomes hidden, e.g. changing the orientation on a tablet or resizing the window, the focus trap would still be active and prevent users from accessing the rest of the page.
Thinking outside of the box, I wonder if we could use the inert attribute to achieve a similar effect in this case. I think we would only need to set inert on the .main-frame and potentially the "Skip to content" link to achieve a similar effect without needing to maintain a list of interactive elements. This could end up being a tiny method called from setExpanded() and a media query listener to handle the viewport width changes.
What do you think? Would you be open to exploring this approach so we can discuss the pros and cons of each approach in more detail?
… menu Replaces the hand-rolled Tab/Shift+Tab focus trap with the native `inert` attribute applied to `.main-frame` and the skip link while the mobile menu is expanded. Addresses HiDeoo's review feedback on withastro#3911: - The browser now enumerates focusable elements, so sidebar `<summary>` group toggles (and any user-added interactive elements) are covered automatically. - A `matchMedia('(min-width: 50em)')` listener collapses the menu when the viewport grows past the mobile breakpoint, so rotating or resizing a device no longer leaves the page locked behind a stale trap. - Removes `packages/starlight/__e2e__/mobile-menu.test.ts` per the maintainer's request to defer dedicated e2e coverage; future tests belong in basics.test.ts to share the basics fixture. SkipLink now carries a `.sl-skip-link` class so the toggle can mark it inert alongside the main content.
|
Rewrote the focus containment around the
|
This comment was marked as spam.
This comment was marked as spam.
|
Thanks for your interest @farrosfr With Astro v7 around the corner, I did not get the time yet to prioritize this PR since it was last updated, my apologies for that. As commented here, I think we will first want to agree on the approach, make sure it properly fixes the issue, does not have any regressions, etc. before moving forward with any tests. Once we have a clear direction, we can then evaluate the best way to implement tests for this change, if any. Considering the cost of E2E tests, we will definitely want to focus tests that really provide value to us and our users, and we would probably want to avoid adding too many tests and rather focus on a few high-value tests instead if needed. In the meantime, the most helpful feedback could be to actually manually test the current implementation on many devices and browsers, making sure it works as expected, and report such tests, e.g. which device, which browser, which browser version, etc. have been tested, and what the results were. Thanks everyone for your patience and help in making sure we get this right. |
This comment was marked as spam.
This comment was marked as spam.
delucis
left a comment
There was a problem hiding this comment.
Thanks very much for the PR @timothyjordan! I think the implementation is basically sound 🎉
I did a quick test in the following browsers on macOS:
- Firefox 152
- Safari 26.2
- Chrome 149
- Edge 148
In each, it seems to work as expected: in the current production deployment of the Starlight docs, tabbing would continue into page content; in the preview deployment for this PR, tabbing would end at the end of the menu either moving to browser chrome or back to the top of the page (behaviour varies slightly here depending on the browser, but in each case I believe its the standard behaviour and working correctly).
I would obviously love more tests on different browsers and OSes, but as a baseline it gives me some confidence that this approach works.
I left a few suggestions to clean up the code — I’d also be happy to apply them myself directly if that would be helpful.
|
Came across this while working on a similar fix (#4029). The inert approach is definitely cleaner — tested the deploy preview on mobile and focus containment works correctly. Happy to help with any additional testing if needed. |
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
|
Thanks for the refactor! One thing that looks off to me in the latest commit: the media query handler was changed from setExpanded(false) to setExpanded(e.matches). For (min-width: 50em), e.matches is true when the viewport crosses into desktop territory — so rotating from mobile to desktop now calls setExpanded(true), which sets .main-frame and .sl-skip-link to inert on desktop and cuts off keyboard access to the main content. I think the original alwaysfalse was correct: regardless of which direction the viewport changes, we want to reset to a closed state. Should this be reverted to setExpanded(false)? |
There was a problem hiding this comment.
Thanks again for the PR @timothyjordan! I took the liberty of applying my review suggestions (and refactoring a little more) so we can get this released. I also added some end-to-end tests that check the expected behaviour using our basic fixture to help avoid regressions in the future.
@FrancoKaddour I think you may have been looking at an isolated intermediary commit? The code is still using setExpanded(false) when the viewport changes.
Co-authored-by: HiDeoo <494699+HiDeoo@users.noreply.github.com>
Co-authored-by: HiDeoo <494699+HiDeoo@users.noreply.github.com>
This PR contains the following updates: | Package | Change | [Age](https://docs.renovatebot.com/merge-confidence/) | [Confidence](https://docs.renovatebot.com/merge-confidence/) | |---|---|---|---| | [@astrojs/starlight](https://starlight.astro.build) ([source](https://github.com/withastro/starlight/tree/HEAD/packages/starlight)) | [`^0.40.0` → `^0.41.0`](https://renovatebot.com/diffs/npm/@astrojs%2fstarlight/0.40.0/0.41.3) |  |  | | [sharp](https://sharp.pixelplumbing.com) ([source](https://github.com/lovell/sharp), [changelog](https://github.com/lovell/sharp/blob/main/docs/src/content/docs/changelog.md)) | [`0.35.2` → `0.35.3`](https://renovatebot.com/diffs/npm/sharp/0.35.2/0.35.3) |  |  | --- ### Release Notes <details> <summary>withastro/starlight (@​astrojs/starlight)</summary> ### [`v0.41.3`](https://github.com/withastro/starlight/blob/HEAD/packages/starlight/CHANGELOG.md#0413) [Compare Source](https://github.com/withastro/starlight/compare/@astrojs/starlight@0.41.2...@astrojs/starlight@0.41.3) ##### Patch Changes - [#​3911](withastro/starlight#3911) [`1686ecc`](withastro/starlight@1686ecc) Thanks [@​timothyjordan](https://github.com/timothyjordan)! - Keeps keyboard focus inside the mobile menu while it is open, preventing focus moving to hidden interactive elements in page content. ### [`v0.41.2`](https://github.com/withastro/starlight/blob/HEAD/packages/starlight/CHANGELOG.md#0412) [Compare Source](https://github.com/withastro/starlight/compare/@astrojs/starlight@0.41.1...@astrojs/starlight@0.41.2) ##### Patch Changes - [#​4008](withastro/starlight#4008) [`58a3520`](withastro/starlight@58a3520) Thanks [@​FrancoKaddour](https://github.com/FrancoKaddour)! - Fixes the table of contents overflowing the right edge of the viewport when a custom `--sl-content-width` value exceeds available space - [#​4015](withastro/starlight#4015) [`bdbfffc`](withastro/starlight@bdbfffc) Thanks [@​delucis](https://github.com/delucis)! - Fixes an issue where aside icons were rendered incorrectly in projects where Astro’s MDX integration had optimization disabled ### [`v0.41.1`](https://github.com/withastro/starlight/blob/HEAD/packages/starlight/CHANGELOG.md#0411) [Compare Source](https://github.com/withastro/starlight/compare/@astrojs/starlight@0.41.0...@astrojs/starlight@0.41.1) ##### Patch Changes - [#​3967](withastro/starlight#3967) [`72e63dc`](withastro/starlight@72e63dc) Thanks [@​HiDeoo](https://github.com/HiDeoo)! - Adds 2 new icons: `link` and `link-alt`. - [#​3988](withastro/starlight#3988) [`ac55cfa`](withastro/starlight@ac55cfa) Thanks [@​delucis](https://github.com/delucis)! - Fixes a dependency resolution issue introduced in Starlight v0.41 - [#​3967](withastro/starlight#3967) [`72e63dc`](withastro/starlight@72e63dc) Thanks [@​HiDeoo](https://github.com/HiDeoo)! - Optimizes the icons of Markdown asides. ### [`v0.41.0`](https://github.com/withastro/starlight/blob/HEAD/packages/starlight/CHANGELOG.md#0410) [Compare Source](https://github.com/withastro/starlight/compare/@astrojs/starlight@0.40.0...@astrojs/starlight@0.41.0) ##### Minor Changes - [#​3951](withastro/starlight#3951) [`1202dd4`](withastro/starlight@1202dd4) Thanks [@​HiDeoo](https://github.com/HiDeoo)! - Adds support for Astro v7, drops support for Astro v6. ##### Upgrade Astro and dependencies⚠️ **BREAKING CHANGE:** Astro v6 is no longer supported. Make sure you [update Astro](https://docs.astro.build/en/guides/upgrade-to/v7/) and any other official integrations at the same time as updating Starlight: ```sh npx @​astrojs/upgrade ``` *Community Starlight plugins and Astro integrations may also need to be manually updated to work with Astro v7. If you encounter any issues, please reach out to the plugin or integration author to see if it is a known issue or if an updated version is being worked on.*⚠️ **BREAKING CHANGE:** This release drops official support for Chromium-based browsers prior to version 111 (released 07 March 2023) and Safari-based browsers prior to version 16.4 (released 27 March 2023). You can find a list of currently supported browsers and their versions using this [browserslist query](https://browsersl.ist/#q=%3E+0.5%25%2C+not+dead%2C+Chrome+%3E%3D+111%2C+Edge+%3E%3D+111%2C+Firefox+%3E%3D+121%2C+Safari+%3E%3D+16.4%2C+iOS+%3E%3D+16.4%2C+not+op_mini+all). ##### Patch Changes - [#​3953](withastro/starlight#3953) [`a935d33`](withastro/starlight@a935d33) Thanks [@​HiDeoo](https://github.com/HiDeoo)! - Fixes Starlight Markdown processing being potentially applied to files that should not be processed. </details> <details> <summary>lovell/sharp (sharp)</summary> ### [`v0.35.3`](https://github.com/lovell/sharp/releases/tag/v0.35.3) [Compare Source](lovell/sharp@v0.35.2...v0.35.3) - Tighten verification of `text` dimensions, TIFF tile dimensions and `extend` values. - Improve code bundler support by resolving path to libvips binary. - Increase default concurrency when use of `MALLOC_ARENA_MAX` is detected. - Emit warning about binaries provided by Electron for use on Linux. - Add `hasAlpha` property to output `info`. [#​4500](lovell/sharp#4500) - TypeScript: Return more precise `Buffer<ArrayBuffer>` from `toBuffer`. [#​4520](lovell/sharp#4520) [@​Andarist](https://github.com/Andarist) - Bound `clahe` width and height to avoid signed overflow. [#​4551](lovell/sharp#4551) [@​metsw24-max](https://github.com/metsw24-max) - Bound `trim` margin to avoid signed overflow. [#​4552](lovell/sharp#4552) [@​metsw24-max](https://github.com/metsw24-max) - Reject infinite values when validating numbers. [#​4553](lovell/sharp#4553) [@​metsw24-max](https://github.com/metsw24-max) - Bound extract region to libvips coordinate limit. [#​4555](lovell/sharp#4555) [@​metsw24-max](https://github.com/metsw24-max) - Verify background colour values are numbers. [#​4556](lovell/sharp#4556) [@​metsw24-max](https://github.com/metsw24-max) - Bound create and raw input dimensions to coordinate limit. [#​4558](lovell/sharp#4558) [@​metsw24-max](https://github.com/metsw24-max) - Tighten recomb and affine matrix verification. [#​4560](lovell/sharp#4560) [@​chatman-media](https://github.com/chatman-media) - Verify cache memory limit to avoid overflow. [#​4561](lovell/sharp#4561) [@​metsw24-max](https://github.com/metsw24-max) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My44Mi4wIiwidXBkYXRlZEluVmVyIjoiNDMuODIuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiZGVwZW5kZW5jaWVzIl19--> --------- Co-authored-by: Paul Tibbetts <code@paultibbetts.uk> Reviewed-on: https://gitea.cloud.paultibbetts.uk/paul/dev/pulls/139 Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-committed-by: Renovate Bot <bot@renovateapp.com>
This PR contains the following updates: | Package | Change | [Age](https://docs.renovatebot.com/merge-confidence/) | [Confidence](https://docs.renovatebot.com/merge-confidence/) | |---|---|---|---| | [@astrojs/starlight](https://starlight.astro.build) ([source](https://github.com/withastro/starlight/tree/HEAD/packages/starlight)) | [`0.41.2` → `0.41.3`](https://renovatebot.com/diffs/npm/@astrojs%2fstarlight/0.41.2/0.41.3) |  |  | --- ### Release Notes <details> <summary>withastro/starlight (@​astrojs/starlight)</summary> ### [`v0.41.3`](https://github.com/withastro/starlight/blob/HEAD/packages/starlight/CHANGELOG.md#0413) [Compare Source](https://github.com/withastro/starlight/compare/@astrojs/starlight@0.41.2...@astrojs/starlight@0.41.3) ##### Patch Changes - [#​3911](withastro/starlight#3911) [`1686ecc`](withastro/starlight@1686ecc) Thanks [@​timothyjordan](https://github.com/timothyjordan)! - Keeps keyboard focus inside the mobile menu while it is open, preventing focus moving to hidden interactive elements in page content. </details> --- ### Configuration 📅 **Schedule**: (UTC) - Branch creation - At any time (no schedule defined) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4yNTIuMSIsInVwZGF0ZWRJblZlciI6IjQzLjI1Mi4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->
Description
Fixes #2697.
When the mobile menu is open on narrow viewports, Tab navigation currently escapes the menu after the last focusable element (the theme switcher) and lands on the underlying page content.
This PR sets the native
inertattribute on.main-frameand the skip-to-content link while the mobile menu is expanded, so the browser keeps keyboard focus inside the header and sidebar without a hand-maintained list of focusable selectors. AmatchMedia('(min-width: 50em)')listener collapses the menu and clearsinertautomatically if the viewport grows past the mobile breakpoint — so rotating or resizing a device cannot leave the page locked behind a stale trap.Test plan
Verified manually against
pnpm --filter starlight-docs devat mobile (600×900) and desktop (1024×900) widths:aria-expanded="true",inertset on both.main-frameand.sl-skip-link.<summary>group toggles (e.g. "Start Here", "Guides") and never lands inside.main-frame. Shift+Tab from the first sidebar item returns to the toggle/close button..focus()on a link inside.main-frameis refused by the browser while inert.aria-expandeddrops to"false",inertis cleared, article links are interactive again.Per maintainer request, no dedicated e2e tests are added in this PR — if we add coverage later, it should live in
__e2e__/basics.test.tsso it shares the basics fixture build.pnpm lint,pnpm typecheck, andpnpm --filter @astrojs/starlight test(519/519) all pass.Changeset
Patch bump for
@astrojs/starlightincluded.