Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
63b9cb4 to
f0a9bab
Compare
|
AI review status for this pull request.
|
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewComponent Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Visual Regression55 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 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 needs a maintainer decision before it can be approved: DropdownMenu.spec.md is still a draft, so the new anchorRef prop and the proposed FR15 and DEC-8 have no approved spec behind them yet. A maintainer needs to approve or reject FR15 and DEC-8 as the governing spec for this addition, and settle the two points below. Separately, please fix one defect: with anchorRef, the new layout effect clears the anchor on every open before the layer checks whether it is in view, so anchored menus never get the off-screen-anchor hold of spec:AST-059 FR5/FR8, and a test asserting that the visibility observer watches the anchorRef element would lock the fix in.
- Whether
spec:AST-055FR2, which says the trigger props carry the anchor the overlay positions against, allows an anchor supplied outside those props. - Whether
anchorRefmay mean positioning-only here, when Popover's referenced anchor also supplies the trigger.
[Automated review]
f0a9bab to
0c2fae6
Compare
There was a problem hiding this comment.
This needs a maintainer decision before it can be approved: DropdownMenu.spec.md is still a draft, so the new anchorRef prop and the proposed FR16, DEC-9 and Menu anchor concept have no approved spec behind them yet, and a maintainer needs to approve or reject them and settle the two points below. Separately, please fix one defect whatever that decision is: the new layout effect clears the menu's anchor on every open before the layer checks whether the anchor is in view, so anchored menus never get the off-screen-anchor hold of spec:AST-059 FR5/FR8, and a test asserting that the visibility observer watches the anchorRef element would lock the fix in. The PR description still calls the proposal FR15 and DEC-8, which on this branch are the header and footer rows, so please update it.
- Whether
spec:AST-055FR2, which says the trigger props carry the anchor the overlay positions against, admits an anchor supplied outside those props. - Whether
anchorRefmay mean positioning-only here, when Popover's and Tooltip's referenced anchor is also the control.
[Automated review]
|
Thanks. Fixed in 1b66fd2.
|
There was a problem hiding this comment.
This needs a maintainer decision before it can be approved: DropdownMenu.spec.md is still a draft, so approval needs a maintainer to provide the governing spec for the new anchorRef prop, by approving or rejecting the proposed FR16, DEC-9 and Menu anchor concept, and to settle the two points below. The off-screen-anchor issue from the last round is fixed. Separately, please handle an anchorRef that is still empty when the menu opens: today the menu opens attached to nothing (at the page's top-left in Chromium) with no warning, so falling back to the trigger, as BaseTypeahead falls back to its input, or warning in development, plus a test, would close it.
- Whether
spec:AST-055FR2, which says the trigger props carry the anchor the overlay positions against, admits an anchor supplied outside those props. - Whether
anchorRefmay mean positioning-only here, when Popover's and Tooltip's referenced anchor is also the control.
[Automated review]
| if (anchorRef == null) { | ||
| return; | ||
| } | ||
| const element = anchorRef.current; |
There was a problem hiding this comment.
When anchorRef.current is null here (for example, a ref on a component that doesn't forward it, or a conditionally rendered anchor), nothing is anchored: attachTrigger skips the trigger whenever anchorRef is set, so the layer's position-anchor names an anchor no element carries. In Chromium the menu then opens at the page's top-left, over other content, with no warning.
Falling back to the trigger while the element is absent, as BaseTypeahead falls back to its input, or warning in development, would avoid a silently broken state (spec:AST-002 FR15). A test for the empty-ref case would lock it in.
There was a problem hiding this comment.
Done in 165c041. While anchorRef.current is null, the trigger is now the anchor, as BaseTypeahead falls back to its input. The menu moves to the caller's element once it's attached, so it's never placed against nothing. I chose the documented fallback over a development warning, per spec:AST-002 FR15: an anchor rendered only in some layouts is legitimate composition, and a warning would flag it. The prop doc, the changeset, FR16 and DEC-9 now say what an empty ref does. Two new unit cases: an empty ref keeps the menu on its trigger through an open, and an anchor that appears later takes over from the trigger. Both fail on 1b66fd2 and pass now. The DropdownMenu, ContextMenu, Popover, Layer and Selector suites, typecheck, lint, knowledge, the Storybook build and the Chromium anchor, header/footer, press and drill-in specs pass.
There was a problem hiding this comment.
This needs a maintainer decision before it can be approved: DropdownMenu.spec.md is still a draft, so approval needs a maintainer to provide the governing spec for the new anchorRef prop, by approving or rejecting the proposed FR16, DEC-9 and Menu anchor concept, and to settle the two points below. The empty-ref issue from the last round is fixed: an unattached anchorRef now leaves the menu on its trigger, and the new tests fail without that fallback. One optional follow-up: if the anchor element unmounts while the menu is open, the menu moves to its trigger but keeps the old element's off-screen hold until it closes, so either re-subscribing the layer's visibility observer when its anchor changes or noting this in FR16 would close it.
- Whether
spec:AST-055FR2, which says the trigger props carry the anchor the overlay positions against, admits an anchor supplied outside those props. - Whether
anchorRefmay mean positioning-only here, when Popover's and Tooltip's referenced anchor is also the control.
[Automated review]
| if (anchorRef == null) { | ||
| return; | ||
| } | ||
| const element = anchorRef.current ?? buttonRef.current; |
There was a problem hiding this comment.
Non-blocking: when this re-points the anchor during an open (the referenced element unmounts or mounts while the menu is open), the layer's anchor-visibility observer keeps watching the element it subscribed to when the menu opened, because that effect only re-subscribes when the open state changes (useLayer.tsx, the anchor-visibility effect). In Chromium, after a sibling anchor unmounted mid-open, the menu correctly moved under the trigger but kept justify-self: unsafe self-end (the off-screen hold) while the trigger was in view, so the slide stays withdrawn until the menu closes. Narrow and self-correcting at the next open; re-subscribing the observer when the layer's anchor changes, or saying in FR16 that an anchor change during an open takes full effect at the next open, would close it.
There was a problem hiding this comment.
Noted in FR16 in 86a921a: an anchor change during an open moves the menu at once, and the off-screen hold (spec:AST-059 FR5) follows the new anchor from the next open. I kept the observer change out of this PR. The layer's visibility effect is shared by every layer, and re-subscribing it when the anchor changes is a separate fix to useLayer.
There was a problem hiding this comment.
This needs a maintainer decision before it can be approved: DropdownMenu.spec.md is still a draft, so approval needs a maintainer to provide the governing spec for the new anchorRef prop, by approving or rejecting the proposed FR16, DEC-9 and Menu anchor concept, and to settle the points below. Separately, please fix one small issue whatever that decision is: FR16's new sentence says an anchor change during an open moves the menu at once, but the menu only re-reads anchorRef when it renders, so with a sibling anchor beside a memoized menu it stays on its trigger when that anchor mounts mid-open and is left for a frame with no anchor, at the viewport's top-left, when it unmounts; narrowing FR16 and DEC-9 to what the code guarantees, or following the element without a menu render, plus a test, would close it.
- Whether the off-screen hold may follow the previous anchor for the rest of an open under
spec:AST-059FR4, FR5 and FR7, or the layer must re-observe a changed anchor first. - Whether
spec:AST-055FR2, which says the trigger props carry the anchor the overlay positions against, admits an anchor supplied outside those props. - Whether
anchorRefmay mean positioning-only here, when Popover's and Tooltip's referenced anchor is also the control.
[Automated review]
| | FR13 | On a mouse, a nested flyout stays open while the pointer is inside the triangle from where it left its row to the flyout's near edge — including while the pointer is paused there — and closes after the existing delay once the pointer has left both the row and that triangle. The row's click toggle and its guard window are unchanged. | Proposed in this change; `DropdownMenuSubMenu.test.tsx` safe-triangle suite, `useMenuHover.test.tsx` | Proposed; verified in jsdom, pending owner review | | ||
| | FR14 | A compound `DropdownMenuGroup` is one `role="group"` named by its heading through `aria-labelledby`; the heading carries `dropdown-menu-section-heading`, is plain text rather than a menu item, and is skipped by roving focus and typeahead. An untitled group is an unnamed `role="group"`. | DEC-1, docs, and tests | Proposed; awaiting owner approval | | ||
| | FR15 | With `header` or `footer`, the pointer popover holds the header, then the `role="menu"` element, then the footer, and the menu element holds only its rows. A region keeps its own keys and presses: neither the menu's key handling nor its press model claims an event that begins there. The menu is one Tab stop between the regions. Tab from the menu moves to the footer, and Shift+Tab to the header, when that region holds a control Tab can reach; otherwise Tab closes the menu as before. Tab past the footer's last control or Shift+Tab before the header's first closes the menu, and focus returns to the trigger. From a row of an open sub-menu, Tab and Shift+Tab close every open flyout and continue as they do from a row of the root menu. Escape closes from anywhere in the popover. The bottom sheet shows the header above the actions and the footer below them. Without either, the menu renders and behaves exactly as it does without the props. | DEC-8, docs, and tests | Proposed; awaiting owner approval | | ||
| | FR16 | With `anchorRef`, the pointer menu MUST be placed against the referenced element, with `placement` and `alignment` read against it and its default minimum width that element's width, still capped by the viewport, while the Trigger button keeps focus and focus return (FR6), the press model (FR8), the keyboard opens and the menu's name (FR9); a press on that element outside the trigger is a press outside the menu and dismisses it. Without it, or while its ref is empty, the trigger is the anchor. An anchor change during an open moves the menu at once; the off-screen hold (`spec:AST-059` FR5) follows it from the next open. The touch sheet ignores it. | Proposed DEC-9; `component:Popover` `anchorRef` | Proposed; verified in jsdom and Chromium, pending owner review | |
There was a problem hiding this comment.
The first half of the new sentence holds only when the anchor change also re-renders the menu: the anchor effect in DropdownMenu.tsx runs on menu renders. With a sibling anchor beside a memoized menu, the menu stayed on its trigger in Chromium after the anchor mounted mid-open, and when the anchor unmounted it sat for one frame at the viewport's top-left, with no element carrying its anchor name, before the layer's observer re-rendered it onto the trigger. DEC-9's "never placed against nothing" has the same gap. Stating what the code guarantees (the menu reads anchorRef whenever it renders, including at every open), or following the element without depending on a menu render, would close it; a memoized-menu case would lock it in.
The second half (the hold follows the element observed at open) is a question for the maintainer under spec:AST-059 FR4, FR5 and FR7.
There was a problem hiding this comment.
Narrowed in 37516c6. FR16 now says the menu reads anchorRef whenever it renders, every open included, rather than observing it, as Popover and BaseTypeahead read theirs. An anchor that mounts or unmounts between renders of the menu is followed at its next render, and the off-screen hold follows the anchor the menu opened against until it closes. DEC-9 drops "never placed against nothing": it now covers an empty ref when the menu renders, states the same read-on-render rule, and rejects observing the document for anchor changes. A new memoized-menu case puts a sibling anchor beside the menu and mounts it mid-open. The menu stays open and keeps an anchor, then follows the row at its next render. That case fails against main's DropdownMenu and passes now; the whole anchorRef suite is 8/8. The off-screen-hold question stays with the maintainer, as you noted.
| if (anchorRef == null) { | ||
| return; | ||
| } | ||
| const element = anchorRef.current ?? buttonRef.current; |
There was a problem hiding this comment.
This read happens only when DropdownMenuPopover renders. A caller whose anchor mounts or unmounts during an open without re-rendering the menu (for example, a sibling anchor beside a memo-wrapped menu) gets no update: the trigger keeps the anchor after a mount, and after an unmount the layer's position-anchor names an element that is no longer connected until something re-renders the menu (in Chromium the layer's IntersectionObserver does, a frame later). See the comment on FR16.
There was a problem hiding this comment.
Addressed by narrowing FR16 and DEC-9 to this read-on-render behavior, in 37516c6 (see the FR16 thread). The code is unchanged: following the element without a render would mean a document-wide DOM observer on every open menu, which DEC-9 now lists as rejected. The memoized-menu case pins the guarantee.
There was a problem hiding this comment.
This needs a maintainer decision before it can be approved: DropdownMenu.spec.md is still a draft, so approval needs a maintainer to provide the governing spec for the new anchorRef prop, by approving or rejecting the proposed FR16, DEC-9 and Menu anchor concept, and to settle the points below. The narrowed FR16 and DEC-9 now match the code; if the maintainer holds spec:AST-059 FR8 for this case, the menu must not paint without an anchor when such an anchor unmounts mid-open, and if FR16 is accepted as written, the prop docs should carry its at-render wording too.
- Whether FR16 may accept that, when an anchor rendered outside the menu unmounts during an open, the menu paints one frame at the viewport's top-left before moving to its trigger and keeps the old anchor's off-screen hold until it closes (
spec:AST-059FR4, FR5, FR7, FR8). - Whether
spec:AST-055FR2, which says the trigger props carry the anchor the overlay positions against, admits an anchor supplied outside those props. - Whether
anchorRefmay mean positioning-only here, when Popover's and Tooltip's referenced anchor is also the control.
[Automated review]
| name: 'anchorRef', | ||
| type: 'React.RefObject<HTMLElement | null>', | ||
| description: | ||
| "External ref to use as the menu's anchor, as on Popover. The popover presentation is placed against this element, with placement and alignment read against it, and takes its width by default, instead of the trigger's: a menu that lines up with a toolbar or a search row wider than its control. The trigger keeps focus and focus return, the press model, the keyboard opens and the menu's name. While the ref is empty (an element rendered only in some layouts, or a component that does not forward its ref), the trigger is the anchor. Ignored by bottom-sheet presentation.", |
There was a problem hiding this comment.
If FR16's read-on-render rule is accepted, this sentence should carry it too: an anchor that unmounts while the menu is open empties the ref, but the trigger only becomes the anchor at the menu's next render.
There was a problem hiding this comment.
Done in 768353a: the prop's JSDoc, this doc entry and the changeset now carry FR16's read-on-render rule. The ref is read whenever the menu renders, every open included, as on Popover. While it is empty the trigger is the anchor, and an element that mounts or unmounts while the menu is open is followed at the menu's next render. The one-frame paint after an outside anchor unmounts mid-open stays with the maintainer, under spec:AST-059 FR8.
There was a problem hiding this comment.
This needs a maintainer decision before it can be approved: DropdownMenu.spec.md is still a draft, so approval needs a maintainer to provide the governing spec for the new anchorRef prop, by approving or rejecting the proposed FR16, DEC-9 and Menu anchor concept, and to settle the points below. The prop docs now carry FR16's read-on-render rule; any further change depends on that decision.
- Whether FR16 may accept that, when an anchor rendered outside the menu unmounts while the menu is open, the menu has no anchor until its next render (one painted frame at the viewport's top-left in Chromium) and keeps the removed anchor's off-screen hold until it closes (
spec:AST-059FR4, FR5, FR7, FR8). - Whether
spec:AST-055FR2, which says the trigger props carry the anchor the overlay positions against, admits an anchor supplied outside those props. - Whether
anchorRefmay mean positioning-only here, when Popover's and Tooltip's referenced anchor is also the element that opens the overlay.
[Automated review]
| interaction fact. A menu that lines up with a box wider than its control, a | ||
| toolbar or a search row, needs the first without moving the second: the | ||
| trigger keeps focus and focus return, the press model, the keyboard opens and | ||
| the menu's name (DEC-3), and only placement, alignment and the default width |
There was a problem hiding this comment.
With anchorRef applied, more than placement, alignment and the default width read the anchor: the layer also passes it to the browser as the popover's invoker source (useLayer.tsx, where the comment says the invoker sets the layer's sequential focus order and popover nesting), and its visibility observation, which FR16's off-screen-hold clause relies on, watches it too. If DEC-9 is accepted, this sentence should say so, so the approver sees the whole effect.
There was a problem hiding this comment.
Done in 4e0593f (amended from 53609d9 to add the visibility read). DEC-10 now says that placement, alignment, the default width and the layer's anchor-visibility read (spec:AST-059 FR5) follow the anchor. It also says the anchor is the popover's invoker (showPopover({source})), so popover nesting follows it. The invoker's sequential focus order never reaches the menu, because Tab closes it and returns focus to the trigger before the browser moves focus. A new Chromium case covers this: Shift+Tab lands on the field before the trigger. FR16 says the same in its contract clause. The other two points are in the PR comment above.
768353a to
67a11b0
Compare
|
Rebased onto main 250fd9a as one commit (67a11b0). #7459 recorded DEC-9 (the Menu Button trigger), so this PR's proposed decision is now DEC-10; the concept row and FR16 are unchanged, and the verification row sits in #7459's wider table. The code merged cleanly past #7382 and #7475. DropdownMenu, MoreMenu, Popover, Layer and ContextMenu suites pass (600 tests), as do 28 Chromium tests, typecheck, lint:strict, check:knowledge and the Storybook build. |
There was a problem hiding this comment.
This needs a maintainer decision before it can be approved: DropdownMenu.spec.md is still a draft, so a maintainer needs to provide the governing spec for the new anchorRef prop by approving or rejecting the proposed Menu anchor concept, FR16 and DEC-10, and to settle the points below. Without anchorRef nothing changes and the new tests pass; whether anything else must change depends on that decision.
- Whether FR16 may accept that, when an anchor rendered outside the menu unmounts while the menu is open, the menu has no anchor until its next render and keeps the removed anchor's off-screen hold until it closes (
spec:AST-059FR4, FR5, FR7, FR8). - Whether
spec:AST-055FR2, which says the trigger props carry the anchor the overlay positions against, admits an anchor supplied outside those props. - Whether
anchorRefmay mean positioning-only here, when Popover's referenced anchor is also the control, and the element also becomes the popover's invoker, which DEC-10 does not mention.
[Automated review]
|
Thanks. 53609d9 writes the three points into the proposed text, so the ruling is on complete wording. No runtime code changed.
The prop docs carry the unmount sentence. The anchorRef suite (7 of 9 fail on main), 601 unit tests, 29 Chromium tests, typecheck, lint:strict, check:knowledge and the Storybook build all pass. |
There was a problem hiding this comment.
This needs a maintainer decision before it can be approved: DropdownMenu.spec.md is still a draft, so a maintainer needs to provide the governing spec for the new anchorRef prop by approving or rejecting the proposed Menu anchor concept, FR19 and DEC-12, including the departures they state. Placement, width, keyboard opens, focus return, dismissal and the empty-ref fallback check out on this revision; optionally, the block comment above the anchor effects in DropdownMenu.tsx could be brought in line with FR19 and DEC-12.
- Whether an anchor rendered outside the menu that unmounts while it is open may leave it unanchored until its next render and held in place until it closes, against
spec:AST-059FR4, FR5 and FR8. - Whether
spec:AST-055FR2, which says the trigger props carry the anchor the overlay positions against, admits an anchor supplied outside those props. - Whether
anchorRefmay name an element that anchors the menu and becomes its popover invoker while the trigger stays the control, when Popover's referenced anchor is also the control.
[Automated review]
3b49c7a to
1585e48
Compare
There was a problem hiding this comment.
This needs a maintainer decision before it can be approved: DropdownMenu.spec.md is still a draft, so a maintainer needs to provide the governing spec for the new anchorRef prop by approving or rejecting the proposed Menu anchor concept, FR19 and DEC-12, including the departures they state. Placement, width, keyboard opens, focus return, dismissal, the empty-ref fallback and the new accessibility-tree case check out on this revision; one optional touch remains: the comment above the anchor effects in DropdownMenu.tsx still ends with "so the menu is never placed against nothing", which FR19 no longer says.
- Whether an anchor rendered outside the menu that unmounts while it is open may leave it unanchored until its next render and held in place until it closes, against
spec:AST-059FR4, FR5 and FR8. - Whether
spec:AST-055FR2, which says the trigger props carry the anchor the overlay positions against, admits an anchor supplied outside those props. - Whether
anchorRefmay name an element that anchors the menu and becomes its popover invoker while the trigger stays the control, when Popover's referenced anchor is also the control.
[Automated review]
| // is the anchor, as BaseTypeahead falls back to its input, so the menu is | ||
| // never placed against nothing. |
There was a problem hiding this comment.
Non-blocking: this clause predates FR19. With an anchor rendered outside the menu that unmounts while the menu is open, the menu has no anchor until its next render (FR19, DEC-12 and the prop docs say so), so "never placed against nothing" no longer holds. Dropping the clause, and rewrapping line 1092, keeps the comment in step with the proposal.
There was a problem hiding this comment.
Dropped in 37933d4, and the comment is rewrapped; it now ends "…the trigger is the anchor, as BaseTypeahead falls back to its input." The same push adds a "Run DropdownMenu anchorRef contract" step to pr-a11y, so this file's real-engine checks gate every head (6 passed locally against a Storybook built from it).
1585e48 to
37933d4
Compare
There was a problem hiding this comment.
This needs a maintainer decision before it can be approved: DropdownMenu.spec.md is still a draft, so a maintainer needs to provide the governing spec for the new anchorRef prop by approving or rejecting the proposed Menu anchor concept, FR19 and DEC-12, including the departures they state. Placement, width, keyboard opens, focus return, dismissal, the empty-ref fallback and the new pr-a11y step check out on this revision, so nothing else is required from you; one optional touch is the comment over the default width style in DropdownMenu.tsx, which still says the menu prefers the trigger's width.
- Whether an anchor rendered outside the menu that unmounts while it is open may leave it unanchored until its next render and held in place until it closes, against
spec:AST-059FR4, FR5 and FR8. - Whether
spec:AST-055FR2, which says the trigger props carry the anchor the overlay positions against, admits an anchor supplied outside those props. - Whether
anchorRefmay name an element that anchors the menu and becomes its popover invoker while the trigger stays the control, when Popover's referenced anchor is also the control.
[Automated review]
…#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.
37933d4 to
d8f960d
Compare
There was a problem hiding this comment.
This needs a maintainer decision before it can be approved: DropdownMenu.spec.md is still a draft, so a maintainer needs to provide the governing spec for the new anchorRef prop by approving or rejecting the proposed Menu anchor concept, FR19 and DEC-12, including the departures they state. Placement, width, keyboard opens, focus return, dismissal and the empty-ref fallback check out on this revision; the red pr-a11y job comes from a step main added after this branch's base, so a rebase should clear it, and the comment over the default width style in DropdownMenu.tsx still says the menu prefers the trigger's width.
- Whether an anchor rendered outside the menu that unmounts while it is open may leave it unanchored until its next render and held in place until it closes, against
spec:AST-059FR4, FR5 and FR8. - Whether
spec:AST-055FR2, which says the trigger props carry the anchor the overlay positions against, admits an anchor supplied outside those props. - Whether
anchorRefmay name an element that anchors the menu and becomes its popover invoker while the trigger stays the control, when Popover's referenced anchor is also the control.
[Automated review]
d8f960d to
b8ebd17
Compare
There was a problem hiding this comment.
This can't be approved until a maintainer settles the spec: DropdownMenu.spec.md is a draft, so the governing spec for anchorRef has to come from the record owner approving or rejecting the proposed Menu anchor concept, FR19 and DEC-12, departures included. On this revision placement, width, keyboard opening, focus return and dismissal behave as FR19 describes, so nothing else is needed from you before that ruling; optionally, the comment above popoverMatchTrigger in DropdownMenu.tsx still says the menu prefers the trigger's width.
- May an anchor that lives outside the menu and unmounts mid-open leave the menu without an anchor until it re-renders, and keep that anchor's off-screen hold until it closes (
spec:AST-059FR4, FR5, FR8)? - Does
spec:AST-055FR2, which puts the anchor in the handed-back trigger props, allow an anchor supplied outside them, or should it be amended? - Can
anchorRefname an element that only positions the menu and becomes its popover invoker, when Popover uses the name for an element that is also the control?
[Automated review]
|
Six older review threads, each addressed in this branch's commit (b8ebd17):
Resolving these six. |
…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.
b8ebd17 to
955f58e
Compare
There was a problem hiding this comment.
This can't be approved until the spec is settled: DropdownMenu.spec.md is a draft, so the governing spec for anchorRef has to come from an engineering owner approving or amending the proposed Menu anchor concept, FR19 and DEC-12, including the departures they state. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. On this revision placement, width, keyboard opening, focus return and dismissal behave as FR19 describes, so nothing else is needed from you before that ruling.
- May an anchor outside the menu that unmounts mid-open leave the menu unanchored until it re-renders, and keep that anchor's off-screen hold until it closes (
spec:AST-059FR4, FR5, FR8)? - Does
spec:AST-055FR2, which puts the anchor in the handed-back trigger props, admit an anchor supplied outside them, or should it be amended? - Can
anchorRefname an element that positions the menu and becomes its popover invoker, when Popover uses the name for an element that is also the control?
[Automated review]
|
I'm pausing this work for now to reduce contention on the repo. Thank you for the reviews and the spec guidance. |
User need
What led to it: this comes from moving every menu and picker in our app onto Astryx, which began with a press, slide and lift bug in its phone menus. Some of our menus, such as a filter beside a search field, should open under the whole row at its width, but
DropdownMenucan only anchor to its own trigger.Why: callers either change the design or host rows in a
Popover, which loses the menu's press model and keyboard opens.Problem and solution fit
DropdownMenutakesanchorRef,Popover's name for an external anchor. The menu is placed against that element (placement, alignment, default width), while the trigger keeps focus, focus return, the press model, keyboard opens and the menu's name. The bottom sheet ignores it.Current specification
component:DropdownMenu(draft): this PR proposes a Menu anchor concept, FR19 and DEC-12, pending owner review.component:PopoverdefinesanchorRef.Public delta and caller burden
anchorRef; nothing changes without it.menuWidthdefaults to the anchor's width; an empty ref anchors on the trigger.Design and alternatives
Popover. Works, but drops the menu's press model and keyboard behaviour.anchorRef, as onPopoverandTooltip(chosen). Same name and shape callers already know; the trigger keeps every menu behaviour. The cost: a caller who sets it must keep the anchor mounted while the menu is open, or the menu falls back to the trigger.Evidence
Seven of nine new unit cases fail on
main: the row carries the menu's anchor and the trigger none, for the default and a custom trigger, and empty, late and unmounted anchors behave as FR19 says. A Chromium test checks placement under the row at its width, dismissal, keyboard opens and focus return; placement fails onmain.Writing rubric results
R1 narration: the concept, FR19 and DEC-12 are present tense.
R2 research: none.
R3 evidence: FR19's verification row names the tests.
R4 rejected alternatives: DEC-12 lists five.
R5 change references: none.
R6 open questions: none.
Scope
Testing
In Storybook, open Core/DropdownMenu → Anchored to a row (anchorRef).