Repository navigation
refactor(DropdownMenu): the header and footer Tab walk moves to a module of its own - #7737
Merged
Merged
Conversation
…ule of its own The region Tab stops (tabbablesIn, the hidden-wrapper test, the radio-group collapse, focusRegionControl) and the walk itself move unchanged from DropdownMenu.tsx into menuRegionStops.ts, so a sub-menu with a header or footer can share them. No behavior change.
vjeux
requested review from
cixzhang,
imdreamrunner and
josephfarina
as code owners
October 20, 2026 17:03
|
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.
|
4 tasks done
Contributor
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewNo new or modified components detected. Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Visual Regression40 of 486 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 20 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
![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 header and footer Tab walk moves into menuRegionStops.ts with no change in behavior. The four helpers match main byte for byte apart from export, and the existing DropdownMenu header and footer tests, unchanged, still exercise the walk in its new module. One optional tidy-up: tabbablesIn and focusRegionControl are exported, but nothing imports them, here or in #7530, so dropping export would keep the module's surface to moveRegionsTab.
[Automated review]
Contributor
Author
|
On the optional note: I'm keeping the two exports. Once this lands, #7532's header-field arrows (ArrowUp on the first row returns to the header's last text field) call |
4 tasks done
cixzhang
pushed a commit
that referenced
this pull request
Oct 21, 2026
…#7530) > Data items' `header` and `footer`, and the bottom sheet's level regions, follow once this merges. ## User need A sub-menu picking from a long list needs a filter field above its rows, often a "Create" button below. #7344 gave the root menu `header` and `footer`; a sub-menu has neither, so the field goes among the rows (DEC-8 forbids it) or the picker leaves the menu. ## Problem and solution fit `DropdownMenuSubMenu` takes `header` and `footer` under the root's rules (DEC-8, DEC-10). With either, the sub-menu's surface (the flyout, or the drilled-in view on a phone) is a modal dialog named by its row, holding the header, the sub-menu's `role="menu"` with rows only, and the footer. A flyout renders inside its parent menu, so `DropdownMenu` renders a host beside its menu for the dialog. A drilled-in view with regions replaces the root's menu and regions until popped, by a style rule that reads the host. ```tsx <DropdownMenuSubMenu label="Labels" header={<TextInput label="Filter labels" … />} footer={<Button label="Create label" … />}> {visibleLabelRows} </DropdownMenuSubMenu> ``` ## Current specification `component:DropdownMenu` (draft) gains a concept row, FR20, AR7 and DEC-13, all proposed and pending owner review. Other proposals in open pull requests: #7319 FR18/DEC-11, #7352 FR19/DEC-12, #7532 FR21/DEC-14, #7541 FR22/DEC-15. `module:DropdownMenu/useMenuPress` (current) gains a proposed ORD1a: a press released on a row of that dialog's menu acts, as FR1 requires for any flyout; a picker placed in its header or footer stays its own press root. ## Public delta and caller burden - `header` and `footer` on `DropdownMenuSubMenu`. - With either, the row says `aria-haspopup="dialog"`; Tab walks header, rows as one stop, footer, and wraps; an Escape nothing inside handled returns to the row; a pointer leaving the flyout keeps it open until focus leaves. Rendered beside the root's menu, such a flyout closes with the flyout holding its row. - Phone drill-in: a shown view follows its regions and caps at the root's `menuMaxHeight`. - `ContextMenu` renders no host: there the sub-menu shows its rows alone and warns in development; parity follows once this merges. - Without them, nothing changes. ## Design and alternatives Rejected: regions inside the flyout (inside the parent menu); a separate `DropdownMenu` per level; closing on pointer-leave, losing typed text. A drilled-in view's header sits above Back. ## Writing rubric results - R1 narration: none. - R2 research in a record: none. - R3 evidence naming a change: none. - R4 rejected alternatives over one line or outside a decision: none. - R5 pull request or issue references: none. - R6 ruled open questions listing options: none. ## Evidence See Testing. ## Scope - [x] One capability is added. - [x] Unrelated fixes, cleanup, and policy changes were removed or split. - [x] Consumer docs, focused tests, a story and a changeset ship with the feature. - [x] Public text and artifacts contain no internal Meta context. ## Testing Review round 8 (rebased onto `main` b74cf21): `pr-a11y` gets a "Run DropdownMenu sub-menu header and footer contract" step beside "Run DropdownMenu header and footer contract". It runs `SubMenuHeaderFooter.a11y.chromium.spec.ts` with `--project chromium`, so FR20's real-engine checks gate every head. That exact command passes locally against a Storybook built from this head (6 cases). 15 files, +1867/−121 (1,988 lines). DropdownMenu, ContextMenu, Layer, MoreMenu, RadioList, `useMenuPress` and doc-prop suites: 739 tests; header and footer Chromium specs: 16; `.github/scripts` workflow tests: 100; typecheck, `lint:strict`, `check:knowledge`, Storybook build: clean. Review round 7 (rebased onto `main` 863e167): `surfaceHost`, `surfaceOwnerId` and `menuMaxHeight` leave the exported `DropdownMenuContextValue` for `DropdownMenuSurfaceContext`, which the package does not export, layer-scoped and re-provided by each flyout as the menu context is; the public type matches `main`. A new `DropdownMenuContext.public.test.ts` pins its four members (it fails typecheck with the three back) and checks the internal context is not exported. Data-mode parity (data items' `header` and `footer`, `renderDropdownItems`, the bottom sheet's warning and their tests) moves to the follow-up, and the three pointer-leave cases share one setup: 14 files, +1856/−121 (1,977 lines). DropdownMenu, ContextMenu, Layer, MoreMenu, RadioList, `useMenuPress` and doc-prop suites: 739 tests; header and footer Chromium specs: 16; typecheck, `lint:strict` (no warnings), `check:knowledge`, Storybook build: clean. Rebased onto `main` d0f9afe, no conflicts or changes. The previous head's `pr-a11y` and `smoke-test` reds came from CI: main's #7802 added a `pr-a11y` step whose spec file the older head lacked, and jsdelivr was still propagating core@0.7.2. DropdownMenu, ContextMenu, RadioList, docProp* suites: 477 tests; typecheck, `check:knowledge`: clean. Review round 6: a flyout the pointer has left now decides that focus left once the move has completed, so an arrow key in a header radio group, where the browser moves focus itself, no longer closes it. A jsdom case replays the browser's order (focusout, a microtask checkpoint, focusin) and fails on the previous head; a Chromium case on the new "Submenu / header with a choice" story presses ArrowDown after the pointer leaves, and fails against a Storybook built from the previous head, where the flyout closes. DropdownMenu, ContextMenu, Layer, MoreMenu, RadioList, `useMenuPress` and doc-prop suites: 741 tests; header and footer Chromium specs: 16; typecheck, `lint:strict`, `check:knowledge`, Storybook build: clean. Review round 5 (rebased onto `main` 2c3ed99; the earlier `test` and `test-node` reds were `main`'s own, a PostCSS fixture that #7797 fixed): an Escape a control in a region already handled (a header menu closing, a field clearing itself) closes nothing more, and the new case fails on the previous head. The contract takes FR20, AR7 and DEC-13 so open proposals in this record stay distinct. DropdownMenu, ContextMenu, Layer, MoreMenu, `useMenuPress` and doc-prop suites: 646 tests; header and footer Chromium specs: 15; typecheck, `lint:strict`, `check:knowledge`, Storybook build: clean. Rebased onto `main` 577a62e after #7737 merged: one commit, 15 files, +1733/−109. DropdownMenu, ContextMenu, Layer, MoreMenu, `useMenuPress` and doc-prop suites: 645 tests; header and footer Chromium specs: 15; typecheck, `lint:strict`, `check:knowledge`, Storybook build: clean. Review round 4 (amended into the last commit): the surface host is owned through a ref, with only a key in state that changes when a host attaches (it moves when the root's header or footer comes or goes); a drilled-in view with regions caps at the root's `menuMaxHeight`. New cases: the view cap (fails on the previous head) and the host move (a guard). Gates below are for this head. Round 3, then stacked on #7737: the root steps aside for a view by a style rule over its surface host instead of a drill-in record kept in an Effect; a flyout with regions closes from the `onHide` of the flyout holding its row instead of an Effect; the bottom sheet's level regions moved to a follow-up. Each change has a case that fails with it reverted. - `DropdownMenuSubMenu.test.tsx` "header and footer" suite: 20 cases, plus one in `ContextMenu.test.tsx` (rows alone, with the warning). They cover the dialog's role, modality, name and row wiring; regions outside every menu; the Tab walk and wrap from a field, a row and a deeper flyout; field keys; Escape, an IME Escape and Escapes a region handled; a press from a menu row released on a dialog row (acts) or its field (outside); a menu placed in a region keeping its own press; pointer-leave with focus inside, focus moving within (a header radio's arrow included), then focus leaving; a nested flyout closing with its parent; the phone drill-in, the root stepping aside, a shown view following its regions and its height cap. - `__tests__/SubMenuHeaderFooter.a11y.chromium.spec.ts`, real Chromium: six cases (laptop structure, keyboard walk, picks, press-drag pick, a header radio after the pointer leaves; phone drill-in with the root stepped aside). - iOS 26.2 simulator (Mobile Safari), earlier head: phone drill-in as described above; on `main` the level has no field or button.
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Maintainer impact
A sub-menu with a header or footer (#7530) needs the Tab walk
DropdownMenualready runs through its own header, menu and footer. The walk and its helpers sit insideDropdownMenu.tsx, which a sub-menu cannot import without a cycle (DropdownMenu.tsxrenders data items throughDropdownMenuSubMenu). Review of #7530 asked for this move as a pull request of its own, so that one stays reviewable.Intended invariant
component:DropdownMenuFR15 and AR5 stay as they are: with a header or footer, Tab and Shift+Tab walk the header's controls, the menu as one stop, then the footer's controls, and wrap; a named radio group is one stop; a hidden control, or one under a hidden wrapper, is not a stop; from a row of an open flyout the walk goes on from the menu's stop.Change
tabbablesIn, the hidden-wrapper test, the radio-group collapse andfocusRegionControlmove fromDropdownMenu.tsxintomenuRegionStops.ts, byte for byte. The steps insidemoveDialogTabbecomemoveRegionsTab(event, menu, header, footer, isFromFlyout)there, in the same order with the same reads;moveDialogTabcalls it with its refs.DropdownMenu.tsxno longer importsgetFocusableElements. No export, prop, default, style or record changes. Package source changed, so a[chore]changeset says nothing reaches consumers.Evidence
MenuHeaderFooter.a11y.chromium.spec.tspasses in real Chromium.main's, but forexport; the walk ismain's, with itsactiveElementread once before the cycle instead of twice.Scope
Testing