Repository navigation
test(DropdownMenu): a footer stays reachable by Tab when the React root is the document - #9815
Conversation
…andled, so a DropdownMenu footer is reachable in a Next.js app
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This looks ready to merge. The early return in useFocusTrap restores the DropdownMenu header and footer Tab walk that FR15 describes for apps whose React root is the document, and the new tests fail without it and pass with it.
One optional, non-blocking suggestion: the comment at DropdownMenu.tsx:1192-1194 packs three ideas into one sentence, and the inline note offers a shorter version.
[Automated review]
There was a problem hiding this comment.
This looks ready to merge. The early return in useFocusTrap restores the DropdownMenu header and footer Tab walk that FR15 describes for apps whose React root is the document: the new tests fail without it and pass with it, and on a document-root page in Chromium every Tab and Shift+Tab lands where FR15 says, while an unhandled Tab still wraps.
[Automated review]
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| SideNavItem | 55.3 kB (+12 B) · 10.5 kB CSS (0) | — | — · — · — |
| DropdownMenu | 74.1 kB (+11 B) · 15.5 kB CSS (0) | — | ✓ · ✓ · ✓ |
| TabMenu | 56.3 kB (+11 B) · 10.5 kB CSS (0) | — | — · — · — |
45 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| TopNavHeading | 59.1 kB (+11 B) · 10.4 kB CSS (0) | — | — · — · — |
| ChatComposerInput | 59.9 kB (+10 B) · 10.7 kB CSS (0) | — | — · — · — |
| ChatComposerTokenElement | 59.9 kB (+10 B) · 10.7 kB CSS (0) | — | — · — · — |
| SideNavHeading | 59.7 kB (+10 B) · 10.6 kB CSS (0) | — | — · — · — |
| TopNavMegaMenu | 54.8 kB (+10 B) · 11.2 kB CSS (0) | — | — · — · — |
| Typeahead | 63.6 kB (+10 B) · 11.8 kB CSS (0) | — | — · — · — |
| BaseTypeahead | 55.9 kB (+9 B) · 10.3 kB CSS (0) | — | — · — · — |
| Popover | 53.3 kB (+9 B) · 12.7 kB CSS (0) | — | — · — · — |
| TopNavMenu | 58.9 kB (+9 B) · 10.5 kB CSS (0) | — | — · — · — |
| lab/ChatEmojiPicker | 66.9 kB (+9 B) · 773 B CSS (0) | — | — · — · — |
| lab/ChatReactionBar | 67.5 kB (+9 B) · 1.1 kB CSS (0) | — | — · — · — |
| lab/TourStep | 58.6 kB (+8 B) · 590 B CSS (0) | — | — · — · — |
| DateRangeInput | 72.1 kB (+7 B) · 11.9 kB CSS (0) | — | — · — · — |
| Tokenizer | 71.3 kB (+7 B) · 17.1 kB CSS (0) | — | — · — · — |
| Pagination | 87.3 kB (+6 B) · 16.1 kB CSS (0) | — | — · — · — |
| lab/CommentThread | 254.2 kB (+6 B) · 2.4 kB CSS (0) | — | — · — · — |
| ChatComposer | 62.0 kB (+5 B) · 11.2 kB CSS (0) | — | — · — · — |
| Drawer | 39.3 kB (+5 B) · 8.2 kB CSS (0) | — | — · — · — |
| MultiSelector | 73.7 kB (+5 B) · 15.3 kB CSS (0) | — | — · — · — |
| Selector | 78.4 kB (+5 B) · 15.7 kB CSS (0) | — | — · — · — |
| BreadcrumbItem | 68.4 kB (+4 B) · 11.5 kB CSS (0) | — | — · — · — |
| DateInput | 106.0 kB (+4 B) · 15.4 kB CSS (0) | — | — · — · — |
| PowerSearch | 194.3 kB (+4 B) · 21.6 kB CSS (0) | — | — · — · — |
| ComplexSelector | 56.3 kB (+3 B) · 11.1 kB CSS (0) | — | — · — · — |
| DateTimeInput | 162.7 kB (+3 B) · 17.1 kB CSS (0) | — | — · — · — |
| lab/CommentComposer | 105.5 kB (+3 B) · 1.9 kB CSS (0) | — | — · — · — |
| lab/TransferListSelector | 83.2 kB (+3 B) · 1.9 kB CSS (0) | — | — · — · — |
| MoreMenu | 74.4 kB (+2 B) · 15.5 kB CSS (0) | — | — · — · — |
| richtext/RichTextEditorToolbar | 225.6 kB (+2 B) · 454 B CSS (0) | — | — · — · — |
| lab/Comments | 259.4 kB (+1 B) · 2.5 kB CSS (0) | — | — · — · — |
| BottomSheetSwitcher | 17.0 kB (0) · 3.4 kB CSS (0) | — | — · — · — |
| ContextMenu | 75.9 kB (0) · 15.4 kB CSS (0) | — | — · — · — |
| ContextMenuItem | 26.2 kB (0) · 5.0 kB CSS (0) | — | — · — · — |
| DropdownMenuCheckboxItem | 18.3 kB (0) · 5.9 kB CSS (0) | — | ✓ · ✓ · ✓ |
| DropdownMenuDivider | 5.2 kB (0) · 548 B CSS (0) | — | ✓ · ✓ · — |
| DropdownMenuGroup | 6.2 kB (0) · 3.4 kB CSS (0) | — | ✓ · ✓ · — |
| DropdownMenuItem | 17.5 kB (0) · 5.5 kB CSS (0) | — | ✓ · ✓ · — |
| DropdownMenuRadioGroup | 6.1 kB (0) · 3.3 kB CSS (0) | — | ✓ · ✓ · — |
| DropdownMenuRadioItem | 18.3 kB (0) · 5.9 kB CSS (0) | — | ✓ · ✓ · — |
| DropdownMenuSubMenu | 56.5 kB (0) · 10.0 kB CSS (0) | — | ✓ · ✓ · ✓ |
| Markdown | 146.3 kB (0) · 18.4 kB CSS (0) | — | — · — · — |
| SideNav | 68.8 kB (0) · 11.2 kB CSS (0) | — | — · — · — |
| Table | 85.3 kB (0) · 16.1 kB CSS (0) | — | — · — · — |
| TableCell | 77.1 kB (0) · 15.9 kB CSS (0) | — | — · — · — |
| TableHeaderCell | 76.7 kB (0) · 15.6 kB CSS (0) | — | — · — · — |
Package size: @astryxdesign/core 6.24 MB packed (+326 B); no new dependencies.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
main's focus-region containment (#8883) already leaves a Tab whose default is prevented to the content that handled it. Resolve useFocusTrap.ts to main's handler and keep this branch's regression tests: the DropdownMenu document-root test and the useFocusTrap tests. Drop the changeset, the hook doc sentence and the source comment edits, so the branch changes tests only.
cixzhang
changed the title
There was a problem hiding this comment.
Approved. The new tests lock in the DropdownMenu footer Tab walk for apps whose React root is the document, which #8883 fixed. All 38 tests in the two files pass, and removing the focus trap's early return for a prevented Tab makes 5 of the 7 new tests fail.
Optional follow-up, not blocking: the comments at DropdownMenu.tsx:1207-1209 and menuRegionStops.ts:120-121 still say the document-level trap never reads the menu's Tab a second time, which the new test file shows is not true when the React root is the document.
[Automated review]
User impact
In an app whose React root is the document (every Next.js App Router app), a keyboard user could not reach the controls in a
DropdownMenufooter: the popover's focus trap undid the menu's own Tab move. The fix shipped in #8883, which makes the focus trap leave a Tab whose default is already prevented to the content that handled it. This pull request adds the regression tests for that case, so the defect cannot come back unnoticed. It changes tests only; nothing ships to consumers.What the tests prove
DropdownMenuDocumentRoot.test.tsx(new file; React attaches its root listeners only for the first root in a test environment, so the document root needs its own file): with a footer holding two controls, Tab from the last row goes through both footer controls; Shift+Tab from the last footer control goes back through the footer to the menu; Tab past the last footer control wraps to the menu, and Shift+Tab wraps back.useFocusTrap.test.tsx› "a Tab that content inside has already handled": content moves focus to the last control (Tab) or the first (Shift+Tab) and the trap keeps it there; after a handled Tab, a later move outside is not pulled back in; an unhandled Tab still wraps at either end, which guards against a guard that is too broad.Authority
component:DropdownMenuFR15 / AR5 / DEC-10 (current): with a header or footer, Tab and Shift+Tab move through the header's controls, the menu, then the footer's controls, and wrap at either end.component:PopoverAR3: Tab inside the open popup wraps, except where a child role's own record states its own Tab model, as DropdownMenu FR15 does.Testing
main:npx vitest run packages/core/src/DropdownMenu/DropdownMenuDocumentRoot.test.tsx packages/core/src/hooks/useFocusTrap.test.tsx: 38 passed.event.defaultPreventedearly return from fix(Dialog, AlertDialog, CommandPalette, BottomSheet, Lightbox): Tab wraps inside the open modal through the shared focus-region module #8883 removed fromuseFocusTrap: 5 of the new tests fail (3 inuseFocusTrap.test.tsx, 2 inDropdownMenuDocumentRoot.test.tsx). In jsdom the forward footer case passes either way, because jsdom returns the trap's focusable-selector matches grouped by selector rather than in document order.pnpm check:changesets; changeset coverage: nothing here ships to consumers, so no Changeset.Scope