fix(react-headless-components-preview): let the consumer popover prop win on the PopoverSurface, MenuPopover and Listbox slots - #36665
Merged
Dmytro Kirpa (dmytrokirpa) merged 3 commits intoSep 1, 2026
Conversation
… win on the PopoverSurface slot The surface slot spelled its popover attribute after the consumer props spread, so the prop was silently discarded — popover exclusivity then capped a page at one open Popover where eight should show, and a consumer switching to manual dismiss was ignored. It moves into the slot's defaultProps, the placement the Tooltip hook already uses: the hook supplies the default, the consumer wins. The id spelled alongside it deliberately stays put. Popover.types documents `<Popover id>` as the surface's id channel, and usePopoverTrigger points aria-details at that same value, so letting a surface-level id win would leave the trigger referencing an element that no longer exists. PopoverSurface.test.tsx already guards that linkage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013wmpBCYJpDJCLXcScCWz1i
… win on the MenuPopover and Listbox slots Both surfaces pinned their popover attribute where a consumer's prop could not reach it. useMenuPopover spread `popover: 'auto'` over the base state's root after the base hook had already merged props, and useListbox assigned `state.root.popover = 'auto'` outright after its hook returned. Because every auto popover on a page closes the others, that capped a page at one open Menu and one open listbox: measured, three listboxes give one open surface, and three Menus give one. The default now merges before the base state rather than over it, which is the order slot.always already uses for defaultProps — the hook supplies 'auto', a consumer spelling popover="manual" wins, and the same three-cell pages then show three open surfaces. Nothing else rides either assignment. Unlike PopoverSurface, where an id was spelled alongside the attribute and had to stay put, these two sites set the popover attribute alone: the listbox's id and its aria-labelledby linkage are owned by useListboxSlot, which this does not touch. Light dismiss under the default is unchanged, and a closed listbox still stays mounted at display:none. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013wmpBCYJpDJCLXcScCWz1i
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aj9uA3rCVgosnh2zNn8qkc
Dmytro Kirpa (dmytrokirpa)
requested review from
Dmytro Kirpa (dmytrokirpa)
and
a balanced review from Copilot
September 1, 2026 07:46
Copilot started reviewing on behalf of
Dmytro Kirpa (dmytrokirpa)
September 1, 2026 07:47
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes headless popover slots so consumer-provided popover values override the default. Confidence: 100/100.
Changes:
- Makes
popover="auto"an overridable default for PopoverSurface, MenuPopover, and Listbox. - Adds regression tests for default and overridden values.
- Adds a patch change file.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
usePopoverSurface.ts |
Moves popover into slot defaults. |
PopoverSurface.test.tsx |
Tests consumer override. |
useMenuPopover.ts |
Gives base root props precedence. |
Menu.test.tsx |
Tests default and override behavior. |
useListbox.ts |
Gives listbox root props precedence. |
Dropdown.test.tsx |
Tests listbox override. |
change/...json |
Records the patch release. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
📊 Bundle size report
🤖 This report was generated against 49c0291821bfa7e2ef29514685a666198001ac73 |
|
Pull request demo site: URL |
Dmytro Kirpa (dmytrokirpa)
approved these changes
Sep 1, 2026
Contributor
|
Thanks for the contribution Ray Knight (@ArrayKnight)! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three hooks in the headless package set the native
popoverattribute on their surface slot on the wrong side of the props spread, so a consumer's ownpopovervalue can never win:usePopoverSurfacepins it after...props,useMenuPopoverspreadspopover: 'auto'over the base state's root, anduseListboxassignsstate.root.popover = 'auto'after the hook returns. Sincepopover="auto"is the browser's light-dismiss mode — opening one auto popover closes every other — an application that needs two surfaces open at once (a popover inside a popover, a persistent side surface) has no way to reachpopover="manual". Measured on an 8-cell probe: 1/8 surfaces open before, 8/8 after.The fix moves the attribute to the default side of the merge at all three sites, the position the package's own slot-default convention already provides, so the consumer's value merges over it. A regression test is added at each site; each was verified to fail without the fix. (The surface
idandrole, which are pinned deliberately for the trigger'saria-detailscontract, are intentionally not touched.)Fixes #36647.
Extracted from #36656 per maintainer request — each in-tree fix from that PR as an isolated change.