fix(react-headless-components-preview): share anchor-name across positioned popovers#36347
Open
micahgodbolt wants to merge 1 commit into
Open
Conversation
…tioned popovers usePositioning overwrote the trigger's anchor-name with a unique name per instance, so two popovers sharing one trigger (e.g. a hover Tooltip and a click Menu on the same button) clobbered each other — only the last writer stayed anchored and the other fell back to default placement. Append this instance's anchor name to the trigger's comma-separated anchor-name list and remove only our own on cleanup, preserving any author-set or sibling anchors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📊 Bundle size report
🤖 This report was generated against 19d82d85bda458b0af3fa4e0814bf3232fc78594 |
|
Pull request demo site: URL |
Contributor
Confidence Score: 85/100The code changes look correct and well-covered by tests, but merge readiness is currently blocked by failing CI checks. FindingsBlockers (must fix before merge)
Warnings (should address)
Info (consider)
Category Breakdown
RecommendationREQUEST_CHANGES Code quality is solid and the fix is well-tested, but CI must be green before merge. Posted via the /review-pr skill. |
dmytrokirpa
reviewed
Jun 25, 2026
| expect(node.style.getPropertyValue('anchor-name')).toMatch(/^--popover-anchor-/); | ||
| }); | ||
|
|
||
| it('appends to anchor-name so multiple instances can share one trigger', () => { |
Contributor
There was a problem hiding this comment.
we should also add a e2e test that covers the Menu + Tooltip scenario mentioned in the issue
spmonahan
reviewed
Jul 6, 2026
| // single trigger (e.g. a Tooltip on hover and a Menu on click attached to | ||
| // the same button) without clobbering each other's anchor. On cleanup we | ||
| // remove only our own name, preserving any others still in use. | ||
| const readAnchorNames = () => |
Contributor
There was a problem hiding this comment.
nit: define this function in module scope rather than in the closure.
spmonahan
reviewed
Jul 6, 2026
| .filter(Boolean); | ||
|
|
||
| const names = readAnchorNames(); | ||
| if (!names.includes(anchorName)) { |
Contributor
There was a problem hiding this comment.
This check could be included in readAnchorNames. Rather than call .filter you can call .find and save an iteration of the array. You can short circuit if anchorName is falsey.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes
Part of #36346.
What
usePositioningwrote a uniqueanchor-nameonto the trigger element and overwrote anything already there:When two positioned popovers share one trigger — e.g. a hover Tooltip + a click Menu on the same button — the two
usePositioninginstances clobber each other'sanchor-name.anchor-nameis single-valued here, so the last writer wins; the other popover'sposition-anchorpoints at a name that no longer exists and falls back to default (static / top‑left) placement.Rendered proof (menu closed, tooltip visible)
How
anchor-nameaccepts a comma-separated list. EachusePositioninginstance now appends its own name and removes only its own on cleanup, preserving any author-set or sibling anchors:Single-popover triggers are unchanged; N popovers per trigger now each resolve their own
position-anchor.Tests
Added two unit tests to
usePositioning.test.tsx:anchor-namecontains both distinct namesanchor-nameis preservedAll 19
usePositioningtests pass; Tooltip suite (16) unaffected.