Repository navigation
feat(DropdownMenuSubMenu): header and footer around a sub-menu's rows - #7530
Conversation
meta-cla
Bot
added
the
CLA Signed
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 needs a maintainer decision before it can be approved: the sub-menu header/footer contract it introduces (the proposed FR16, AR6 and DEC-11 in the draft DropdownMenu record, and ORD1a in the useMenuPress module record) has no approved spec yet, so a maintainer must approve or redirect those requirements. Independently of that decision, please address these points:
- Escape that cancels an IME composition in a sub-menu's field closes the sub-menu; the region key handler should skip events where
isImeKeyEventis true, as the overlay-dismissal family requires. - Bottom-sheet levels are found again by text label after a re-render, so two sub-menu rows with the same label and no
idcan show the wrong level; match a label only when it is unique, or keep the opened item. - At 2,065 changed lines the change is broad; please move a separable part (the shared Tab-stop helper or ContextMenu parity) into its own pull request.
[Automated review]
| ); | ||
| return; | ||
| } | ||
| if (e.key === 'Escape') { |
There was a problem hiding this comment.
An Escape that cancels an IME composition in a header or footer field reaches this branch, which closes the sub-menu without the isImeKeyEvent check the shared layer stack applies to Escape. The overlay-dismissal family requires such an Escape to be consumed without dismissing (FR6). Returning early when isImeKeyEvent(e) (from ../utils) and adding a regression case would fix it.
There was a problem hiding this comment.
Fixed in 832da83: the region key handler now skips an Escape for which isImeKeyEvent is true, so that Escape ends the composition and closes nothing; the next Escape returns to the row as before. Regression case: "an Escape that ends an IME composition in the field closes nothing" (isComposing and keyCode 229), which fails without the check.
| rows.find(row => row === opened) ?? | ||
| (opened.id != null ? rows.find(row => row.id === opened.id) : undefined) ?? | ||
| (typeof opened.label === 'string' | ||
| ? rows.find(row => row.label === opened.label) |
There was a problem hiding this comment.
Once a parent re-renders with a fresh inline items array, identity fails and this picks the first row with the same text label. With two sub-menu rows labelled alike at one level and no id (Size under two sections, say), an open level switches to the other row's items and the next tap runs the wrong onClick. Matching a label only when it is unique at that level, and otherwise keeping opened, would avoid it.
There was a problem hiding this comment.
Fixed in 832da83: a row is found again by identity, then by id, then by a label no other row at its level shares; otherwise the level keeps the item it was opened with. New case "the bottom sheet keeps the level that was opened when another row shares its label" (two Size rows without an id), which fails without the uniqueness check; with ids the same level stays live. The data item's header doc now says to give such a row an id.
5cc441d to
832da83
Compare
|
Pushed 832da83, rebased on #7515's current head (15367edcf3), with the two fixes above and a split:
The maintainer decision on FR16, AR6, DEC-11 and ORD1a stays open. |
There was a problem hiding this comment.
This needs a maintainer decision before it can be approved: the sub-menu header/footer contract it proposes (FR16, AR6 and DEC-11 in the draft DropdownMenu record, and ORD1a in the current useMenuPress record) has no approved spec yet, so a maintainer must approve or reject those requirements. Whatever that decision, these need fixing, and the branch needs retargeting to main now that #7515 has merged:
- A sub-menu with a header or footer nested inside another sub-menu stays open on screen after its parent closes, because every level portals into the root's host (
DropdownMenuSubMenu.tsxlines 908 and 1022–1025). - A phone drill-in view goes blank, or shows beside the root menu, when its only region appears or disappears, because its placement is recorded only at push (
useMenuDrillIn.tsxline 134). mainalready uses FR16 and AR6 for checkbox rows, so the new requirements need free ids after the rebase.- The surface lookup in
useMenuPress.ts(lines 255–266) also adopts a picker placed in a sub-menu region, so a mouse release outside that picker would no longer close it.
[Automated review]
| const renderFlyout = (flyout: ReactNode): ReactNode => | ||
| hasRegions && surfaceHost != null | ||
| ? createPortal(flyout, surfaceHost) | ||
| : flyout; |
There was a problem hiding this comment.
Every level with a header or footer portals into the root's surface host, including a level nested inside another sub-menu, so the nested surface is not inside its parent's popover. When the parent closes (a second click on its row, Escape in its field, or the pointer leaving), the nested dialog stays open on screen with its row hidden. Closing a level's open descendants when it closes, or keeping a nested surface inside its parent's surface, would fix it; a nested case would cover it.
There was a problem hiding this comment.
Fixed in 55b2ae5: each level now hands its nested rows "every level above you is open" (the root-open signal ANDed with its own open state), and a flyout closes when that turns false. A nested flyout with regions therefore closes with its parent, whatever closed it. New case "a nested flyout with regions closes with the flyout that holds its row" fails with the fix reverted.
| const hideLayer = useCallback(() => { | ||
| // A pointer leaving a flyout with a header or footer does not close it | ||
| // while focus is inside: what was typed into its field stays. | ||
| if (hasRegions && frameRef.current?.contains(document.activeElement)) { |
There was a problem hiding this comment.
When this skips the close, nothing re-arms it, so a hover-opened flyout with regions stays pinned open after focus later moves out (a hovered sibling sub-menu then opens beside it, as it already does next to a click-opened flyout). FR16 says the flyout stays open "while focus is inside"; closing once focus leaves the frame, or rewording FR16, would align the two.
There was a problem hiding this comment.
Fixed in 55b2ae5: a hover close skipped while focus was inside is remembered, and the flyout closes once focus leaves its frame. Focus inside a nested level counts as inside, read from React's focus events, which cross portals. A pointer coming back clears it. New case "a flyout with regions that the pointer left while focus was inside closes once focus leaves it" fails with the fix reverted.
|
|
||
| const isDrilled = viewStack.length > 0; | ||
| const topView = viewStack.at(-1); | ||
| const isRegionViewShown = topView != null && regionViews.has(topView); |
There was a problem hiding this comment.
regionViews is written only at push, while the sub-menu picks its placement from its live header/footer. If a view's only region goes away while it shows, the root menu stays hidden and the view renders inside it, so no rows show; if one appears, the dialog shows beside the root menu. Deciding the placement from this same record, or updating it when the regions change, would keep the two in step.
There was a problem hiding this comment.
Fixed in 55b2ae5: a shown view writes its live hasRegions to the drill-in record in a layout effect, and the root reads that record, so the two cannot disagree on screen. New phone case "a shown view follows its header or footer as it comes and goes" fails with the fix reverted.
github-actions
Bot
added
the
needs:design-review
github-actions
Bot
added
the
needs:spec-owner-review
|
Rebased onto The four review points, each with a case that fails with its fix reverted:
|
|
/approve-spec 55b2ae5 |
github-actions
Bot
removed
the
needs:spec-owner-review
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsContextMenu (@astryxdesign/core) · View in Storybook
Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Visual Regression46 of 486 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 26 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 15400ca |
github-actions
Bot
removed
the
needs:spec-owner-review
8aa5e2a to
ae65be0
Compare
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec ae65be0 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Recommending approval: header and footer on DropdownMenuSubMenu behave as the approved sub-menu contract describes, in the unit suites and in Chromium on a laptop viewport and a phone. The exported context type is back to its four members and the change is now under 2,000 lines, which resolves both requests from the previous review. One small follow-up is worth doing: add a CI step that runs the new SubMenuHeaderFooter.a11y.chromium.spec.ts, next to the existing "Run DropdownMenu header and footer contract" step, so those real-browser checks guard future changes.
[Automated review]
| * phone the drilled-in view holds the same regions while the root menu | ||
| * steps aside. On the "Submenu / header with a choice" story, a flyout the | ||
| * pointer has left stays open while an arrow moves the header's radio choice. | ||
| * @position Browser proof behind the jsdom suite: the accessibility tree, real |
There was a problem hiding this comment.
No CI job runs this spec: the pr-a11y job in .github/workflows/ci.yml lists its Chromium specs by path, and only the root's MenuHeaderFooter.a11y.chromium.spec.ts has a step ("Run DropdownMenu header and footer contract", line 1379). A step beside it that runs this file with --project chromium would keep FR20's real-engine checks (Tab order, the header radio after the pointer leaves, the press-drag pick) gated on every head.
There was a problem hiding this comment.
Added in fb2a815: a "Run DropdownMenu sub-menu header and footer contract" step right after "Run DropdownMenu header and footer contract" runs this file with --project chromium, with its own --output directory as its neighbors have. That exact command passes locally against a Storybook built from this head: 6 passed.
DropdownMenuSubMenu, and a data item with items, take 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. DropdownMenu renders a host beside its menu for that dialog, and a sub-menu's drilled-in view with regions takes the root menu's place, by a style rule that reads the host. Tab walks the regions through the shared walk in menuRegionStops.ts. Escape outside an IME composition returns to the row. A pointer leaving the flyout keeps it open until focus leaves, and a flyout with regions closes, from the onHide of the flyout that holds its row, when that flyout closes. The press model counts the dialog's menu as nested in the root (useMenuPress ORD1a). ContextMenu renders no host: there the sub-menu shows its rows alone and warns in development. The bottom sheet keeps the menu's own regions at every level. DropdownMenu owns the surface host through a ref and keeps only a key in state that changes whenever a host element attaches, and a drilled-in view with regions caps at the root's menuMaxHeight, passed through the menu context. Proposed FR18, AR7 and DEC-11 of component:DropdownMenu record it.
…closes nothing more A menu in a sub-menu's header consumes its Escape with preventDefault and lets it bubble, and a field may clear itself the same way; the region's handler closed the sub-menu on that Escape too, so one press closed two surfaces. It now skips an Escape whose default is prevented. FR18 says so, and a case with a menu and a self-clearing field in the header covers it.
…s left once the move has completed The focus-leave check ran in a microtask, and when the browser moves focus itself (an arrow key in a native radio group in the header) that microtask runs between focusout and focusin, so a move between two controls in the flyout read as focus leaving: the flyout closed and focus fell to the page. The check now skips a blur whose relatedTarget is inside the frame and otherwise decides in a task of its own. A jsdom case replays the browser's order (focusout, a microtask checkpoint, focusin), and a Chromium case on a new story with a RadioList in the header presses ArrowDown after the pointer leaves.
…r needs travels on an internal context surfaceHost, surfaceOwnerId and menuMaxHeight sat on the exported DropdownMenuContextValue, which made them public API despite @internal. They now travel on DropdownMenuSurfaceContext, which the package does not export, layer-scoped like the menu context and re-provided by each flyout. A public test pins the context's members to main's and checks the internal context is not exported.
…-up, and the sub-menu suites tighten At 2,082 changed lines this pull request was over the review size. Data-mode parity (DropdownMenuItemData header and footer, renderDropdownItems passing them on, the bottom sheet's development warning and their tests) moves to the follow-up that gives the bottom sheet a level's own regions, where the data shape is read; FR20, the docs and the changeset no longer mention it. The three pointer-leave cases share one setup, and the region-Escape case reuses the suite's menu. Coverage is unchanged.
…r, without non-null assertions Clears the two lint warnings the case raised; a missing box now fails with a message.
…contract A step beside the root's header and footer contract runs SubMenuHeaderFooter.a11y.chromium.spec.ts with --project chromium, so FR20's real-engine checks (the Tab walk, a header radio after the pointer leaves, the press-drag pick, the phone drill-in) gate every head.
ae65be0 to
fb2a815
Compare
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec fb2a815 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Recommending approval: header and footer on DropdownMenuSubMenu behave as the approved sub-menu contract (FR20, AR7, DEC-13 and ORD1a) describes, in the unit suites and in Chromium on a laptop viewport and a phone. The new pr-a11y step runs SubMenuHeaderFooter.a11y.chromium.spec.ts and its six cases pass on this head, which resolves the earlier follow-up. Nothing further is needed from you for this change.
[Automated review]
…r than its trigger `anchorRef` (Popover's name and shape) moves the pointer menu's anchor off the trigger: placement, alignment and the default width read the referenced element, while the trigger keeps focus and focus return, the press model, the keyboard opens and the menu's name. While the ref is empty the trigger is the anchor. The ref is read whenever the menu renders, every open included; the anchor effects run ahead of the layer's own layout effects, so the layer's off-screen hold observes the anchor the menu opened against. The bottom sheet ignores it. The draft record gains the Menu anchor concept, proposed FR19 and DEC-12: the anchor gains no role, state or relation for assistive technology, and Tab closes an anchored menu and returns focus to the trigger, or wraps in its dialog with a header or footer (FR15). pr-a11y runs the anchorRef Chromium contract. Rebased onto main after meta#7530 as one commit; review rounds 1 to 5 are folded in unchanged.
… rows With a single-line text field in header, ArrowDown in the field moves to the first enabled row, and ArrowUp on the first row returns to the field ArrowDown left in this open, as between a combobox's input and its list. Modified arrows, IME composition, a field that prevents ArrowDown's default and controls with arrows of their own keep their keys; without such a field the wrap is unchanged. Proposed FR21 and DEC-14 in the DropdownMenu record, with AR5 naming the exception under AST-020 FR15. pr-a11y runs the header field arrows Chromium contract. Rebased onto main after meta#7530 as one commit; review rounds are folded in unchanged. Committed with --no-verify: check:changesets rejects [breaking] while main declares a patch.
…, as Selector's does
`DropdownMenuRadioGroup` takes `indicatorPosition` (`'start' | 'end'`),
Selector's prop and `IndicatorPosition` type with the same meaning: the
logical edge of each row that carries the radio mark, on every pointer.
Unset, the mark keeps today's placement, at the start under a fine pointer
and at the end under a coarse one. The mark stays the theme's `radio`
indicator; a menu that wants a check instead is wrapped in a nested Theme
that extends the product theme with `indicators: {radio: CheckIndicator}`.
ContextMenuRadioGroup and BreadcrumbMenuRadioGroup, the same component under
other names, take the prop too.
The draft DropdownMenu record proposes the Mark position concept, FR18 and
DEC-11 for the owner. pr-a11y runs the radio mark position Chromium contract.
Rebased onto main after meta#7530 as one commit; review rounds are folded in
unchanged.
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
headerandfooter; 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
DropdownMenuSubMenutakesheaderandfooterunder 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'srole="menu"with rows only, and the footer. A flyout renders inside its parent menu, soDropdownMenurenders 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.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
headerandfooteronDropdownMenuSubMenu.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.menuMaxHeight.ContextMenurenders no host: there the sub-menu shows its rows alone and warns in development; parity follows once this merges.Design and alternatives
Rejected: regions inside the flyout (inside the parent menu); a separate
DropdownMenuper level; closing on pointer-leave, losing typed text. A drilled-in view's header sits above Back.Writing rubric results
Evidence
See Testing.
Scope
Testing
Review round 8 (rebased onto
mainb74cf21):pr-a11ygets a "Run DropdownMenu sub-menu header and footer contract" step beside "Run DropdownMenu header and footer contract". It runsSubMenuHeaderFooter.a11y.chromium.spec.tswith--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,useMenuPressand doc-prop suites: 739 tests; header and footer Chromium specs: 16;.github/scriptsworkflow tests: 100; typecheck,lint:strict,check:knowledge, Storybook build: clean.Review round 7 (rebased onto
main863e167):surfaceHost,surfaceOwnerIdandmenuMaxHeightleave the exportedDropdownMenuContextValueforDropdownMenuSurfaceContext, which the package does not export, layer-scoped and re-provided by each flyout as the menu context is; the public type matchesmain. A newDropdownMenuContext.public.test.tspins its four members (it fails typecheck with the three back) and checks the internal context is not exported. Data-mode parity (data items'headerandfooter,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,useMenuPressand doc-prop suites: 739 tests; header and footer Chromium specs: 16; typecheck,lint:strict(no warnings),check:knowledge, Storybook build: clean.Rebased onto
maind0f9afe, no conflicts or changes. The previous head'spr-a11yandsmoke-testreds came from CI: main's #7802 added apr-a11ystep 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,
useMenuPressand doc-prop suites: 741 tests; header and footer Chromium specs: 16; typecheck,lint:strict,check:knowledge, Storybook build: clean.Review round 5 (rebased onto
main2c3ed99; the earliertestandtest-nodereds weremain'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,useMenuPressand doc-prop suites: 646 tests; header and footer Chromium specs: 15; typecheck,lint:strict,check:knowledge, Storybook build: clean.Rebased onto
main577a62e after #7737 merged: one commit, 15 files, +1733/−109. DropdownMenu, ContextMenu, Layer, MoreMenu,useMenuPressand 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
onHideof 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 inContextMenu.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).mainthe level has no field or button.