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 PreviewNo new or modified components detected. Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Visual Regression44 of 448 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 24 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]
| useIsomorphicLayoutEffect(() => { | ||
| if (anchorRef == null) { | ||
| return; | ||
| } | ||
| setAnchor(anchorRef.current); | ||
| return () => { | ||
| setAnchor(null); | ||
| }; | ||
| }, [anchorRef, setAnchor]); |
There was a problem hiding this comment.
With anchorRef, this cleanup runs on every open. popover.triggerRef gets a new identity whenever the layer's isOpen changes, so React runs setAnchor(null) before the layer's layout effects re-run.
The layer's anchor-visibility effect is declared earlier in this component. It runs before this effect re-attaches the anchor, finds no anchor, and skips both the pre-paint read and the IntersectionObserver. As a result, spec:AST-059 FR5/FR8 (an off-screen anchor holds) never apply to anchored menus.
Suggested fix:
- Attach the
anchorRefelement without clearing it on these re-runs, and detach only whenanchorRefgoes away or on unmount. - Resolve the element after mount: an ancestor's ref is still
nullwhen this layout effect first runs. - Add a case asserting that the observer watches the
anchorRefelement.
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]
| | 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. 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 (`spec:AST-059` FR5) follows the anchor the menu opened against until it closes. 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.
This now matches the code. One consequence for whoever approves it: when an anchor rendered outside the menu unmounts during an open without re-rendering the menu, the layer has no connected anchor until that next render. On this revision, in Chromium, a memoized menu with a sibling anchor painted one frame at the viewport's top-left (x 16, y 4), then moved under the trigger with the off-screen hold (justify-self: unsafe self-end) set while the trigger was in view. Current spec:AST-059 FR8 ("A layer MUST NOT paint at a geometry it is about to correct") and FR4/FR5 reach every usePopover consumer through FR7 "with no record change", so accepting this is the maintainer's decision. The new memoized-menu test covers only the mount case.
| 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.
…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, FR16 and proposed DEC-10 (pending owner review). Tests: an anchorRef suite in DropdownMenu.test.tsx and DropdownMenuAnchorRef.a11y.chromium.spec.ts; story AnchorRef.
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. |
…nmounted mid-open The anchor is also the popover's invoker: the menu's popover nesting follows it, while Tab still closes the menu to the trigger first, so the focus order runs from the trigger (new Chromium case: Shift+Tab from the menu lands on the field before the trigger). An anchor that unmounts while the menu is open leaves it with no anchor until its next render, then on the trigger, with the layer's visibility read still on the removed element, so the menu stays held until it closes (new unit case). DEC-10 names the departures from AST-055 FR2 and AST-059 FR4/FR5/FR8, notes that Popover's anchorRef is also its control, and adds the shared-layer re-observe alternative to the rejected list. The prop docs carry the unmount sentence.
53609d9 to
4e0593f
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, FR16 and DEC-10, including the three departures they now state. Without anchorRef nothing changes and the new tests pass; whether anything else must change depends on that decision.
- Whether the menu may accept that an anchor rendered outside it which unmounts while it is open leaves 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]
| // An `anchorRef` (as on Popover) moves the menu's anchor off the trigger; | ||
| // the trigger keeps focus, the press model and the menu's name, and only | ||
| // positioning reads the anchor. These effects are declared before the | ||
| // popover so they run ahead of its layer's own layout effects in each | ||
| // commit: the layer reads the anchor as it opens (its first-frame | ||
| // visibility read and its observer, spec:AST-059 FR5/FR8), so the anchor | ||
| // must already be set then and must not be cleared and re-set around an | ||
| // open. The first follows the caller's element once it is attached (an | ||
| // ancestor's ref attaches after this component's effects) and whenever it | ||
| // changes; the second clears it when the menu unmounts or drops the ref. | ||
| // While the caller's element is absent (not yet attached, rendered only in | ||
| // some layouts, or a component that does not forward its ref), the trigger | ||
| // 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 comment still says "only positioning reads the anchor" and that "the menu is never placed against nothing". On this revision FR16 and DEC-10 say the anchor is also the popover's invoker and the layer's visibility target, and that an anchor unmounting while the menu is open leaves it with no anchor until its next render; updating these lines would keep the code comment in step with the proposal.
This branch was successfully deployed
1 participant
User need
A menu sometimes lines up with a box wider than its control: a filter menu under a whole search row, at its width.
DropdownMenuanchors only to its trigger (renderTrigger's one ref is anchor, focus and press target), so callers change the design or host rows in aPopover, losing the press model and keyboard opens.Problem and solution fit
DropdownMenutakesanchorRef, Popover's name for an external anchor. The pointer menu is placed against that element (placement,alignment, default width); the trigger keeps focus and focus return, the press model, the keyboard opens and the menu's name. The bottom sheet ignores it.Current specification
component:DropdownMenuis a draft; its proposed Menu anchor concept, FR16 and DEC-10 record the shape for the owner. DEC-3 (decided) sets what the trigger carries;component:PopoverdefinesanchorRef. FR16 and DEC-10 also state what the owner rules on: the anchor is the popover's invoker too, the handed-back props carry no anchor (spec:AST-055FR2), and an anchor unmounted mid-open leaves the menu unanchored until it renders and held until it closes (spec:AST-059FR4, FR5, FR8).Public delta and caller burden
anchorRef?: React.RefObject<HTMLElement | null>; nothing changes without it.menuWidth's default is the anchor's width. While the ref is empty, the trigger is the anchor.Design and alternatives
DropdownMenuTriggerProps(the anchor is usually an ancestor the render prop does not render), an offset from the trigger, and Popover-hosted rows.Evidence
spec:AST-059FR5) watches the row; empty, late and mid-open-unmounted anchors behave as FR16 states. Seven of nine cases fail onmain.main.Writing rubric results
R1 narration: the concept, FR16 and DEC-10 are present tense.
R2 research: none.
R3 evidence: FR16's verification row names the tests.
R4 rejected alternatives: DEC-10 lists five.
R5 change references: none.
R6 open questions: none.
Scope
Testing