Skip to content

Commit 3ccc421

Browse files
misama-ctclaude
andauthored
feat(popover): export Popover as a public compound component (#1936)
* docs(openspec): propose popover compound component export Plan for promoting the internal Popover wrapper to a public three-part compound component (Popover.Root/.Trigger/.Content). Corrects two inaccuracies found while researching the ticket: Popover is not currently exported at all, and its internal consumers are ComboBox and LocalizedField rather than Select. Also narrows the aspirational nimbus-popover spec, which described eight parts, size variants, a backdrop and an i18n message that were never implemented. Replaces the spec's Non-Modal Focus Behavior requirement with Focus Management: React Aria's Popover contains focus and blocks outside interaction unless isNonModal is passed, so the previous requirement described the opposite of the framework's default and would have changed LocalizedField's current behavior. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(popover): export Popover as a public compound component Promote the internal Popover wrapper to a public three-part compound component: Popover.Root, Popover.Trigger and Popover.Content, exported from @commercetools/nimbus. - Root renders RaDialogTrigger via asChild so it mounts no DOM element and cannot disturb the trigger's layout. - Content supplies its own React Aria Dialog, so the accessibility contract cannot be omitted by consumers, and forwards a function child to the dialog's render prop for programmatic close. - nimbusPopover becomes a slot recipe (root/trigger/content/dialog) and the raw `bg: "white"` is replaced with the semantic `bg` token. Both former internal consumers drop to React Aria's Popover directly, matching Select. Neither gained anything from the shared surface: each already re-declared every style in its own recipe. ComboBox keeps its inherited `zIndex: 1` explicitly, and LocalizedField gains an `infoPopover` slot carrying exactly what it previously inherited, so both render identically. 127 existing consumer story tests pass unchanged. Removes a dead `autoFocus={false}` from ComboBox's popover: it was never a React Aria Popover prop and only typechecked via the old wrapper's Chakra div props. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(popover): add documentation set and narrow the popover spec Adds the full doc set for the new public Popover: overview, guidelines, implementation guide and accessibility notes, plus consumer-facing copy-ready examples. Pages are discovered from mdx frontmatter, so Popover appears under Components > Feedback at lifecycleState Beta. Narrows openspec/specs/nimbus-popover to what actually ships. The spec previously described eight parts, size variants, a backdrop and a closePopover message, none of which were ever implemented, and its Overview and Purpose described the component as non-modal with non-trapped focus — the opposite of React Aria's default. Also corrects two ARIA and placement claims that were carried over from that spec and disproved against the implementation: - The trigger does not expose aria-haspopup. React Aria sets it only for menu and listbox overlays, deliberately, because screen readers often announce other values as "menu". The spec now records aria-expanded and aria-controls instead. - Placement alignment vocabulary is axis-dependent: vertical sides take left/right/start/end, horizontal sides take top/bottom. "right start" is not a valid value. The docs.spec omits the focus-restoration assertion by design: React Aria's focus restore does not survive JSDOM, and these examples are meant to be copied into consumers' own JSDOM suites. That behavior is covered by the browser story test instead. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(popover): forward Trigger props supplied via asChild The asChild branch returned without spreading the remaining props, so `id`, `isDisabled`, `onPress` and friends were accepted by the type but silently dropped at runtime. They now reach the supplied element, matching how Dialog.Trigger forwards them. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(popover): add Chromatic snapshots and tighten assertions Snapshots are opt-in via the global `disableSnapshot: true` default, and Popover opted into nothing, so a new public component shipped with no visual regression coverage. Adds two dedicated frames following the Menu/Dialog/Drawer pattern: - OpenPopover: the `content` surface (bg, radius, shadow, padding, zIndex), held open with `defaultOpen` and given room to render so the portal is not cropped. - FocusedTrigger: the `trigger` slot's focus ring, which does not go through the Button recipe and so is covered by no other baseline. Placement stays off-snapshot: React Aria repositions the same box with no arrow, which is behavioral rather than visual. Also strengthens three assertions: the outside-press story now checks focus returns to the trigger, the asChild story checks a forwarded prop survives, and the non-modal story drops a check that could never fail (with `isNonModal` React Aria never calls `ariaHideOutside`, so the role query above it is the real assertion). FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(popover): render the consumer tests and correct focus claims popover.dev.mdx was missing its closing sections, so the docs page had no API reference and — because the `{{docs-tests:}}` marker was absent — popover.docs.spec.tsx never rendered at all. Popover was the only component with a docs.spec that was orphaned this way. Rewrites that spec toward what the file type is for: form validation that keeps the popover open on error, async loading of popover contents, a filter workflow driving a list outside the popover, and guarded dismissal in controlled mode. Open/close, keyboard and ARIA coverage is unchanged — it already lives in the story play functions. Also corrects two claims: focus returns to the trigger on close except when the dismissing press lands on another focusable element, and the WCAG resource linked a AAA-filtered view under an AA label. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(combobox): tidy leftovers from the popover migration Merges the duplicate `react-aria-components` type import into the existing block, and drops the JSDoc paragraph narrating the removed `autoFocus` prop — that history belongs in the commit log, not in the source. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(openspec): archive the popover compound export change The live spec had its Overview rewritten to describe the three-part component while its Requirements still specified eight parts, size variants and a closePopover message, leaving the file contradicting itself. Archiving applies the change's delta (+1 added, 12 modified, 8 removed) so the capability is consistent. Two requirements the delta had not covered still specified props that do not exist. Since a MODIFIED block may not drop scenarios, their bodies now state the truth instead: Popover.Content exposes no portal-container prop (React Aria's UNSAFE_PortalProvider controls the target), and Popover has no hover trigger mode (Tooltip covers hover). The touch requirement also claimed a minimum target size the bare-button trigger does not impose. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(openspec): correct the Popover Optimized Rendering requirement The previous change narrowed this capability but its scope did not reach Optimized Rendering, whose Scroll optimization scenario still specified two things the component cannot be held to: that positioning updates use requestAnimationFrame, which is React Aria's internal scheduling rather than anything Nimbus implements or tests, and that the popover may close on scroll if configured, for which no option exists. Both scenario bodies are restated against observable behavior — overlay content stays out of the DOM while closed, and the popover stays anchored to its trigger and inside the viewport while the page moves. Scenario names are preserved, since a MODIFIED requirement may not drop them. Specification only; no source or test files change. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(popover): reject Trigger button props under asChild Supersedes 84a1123, which fixed the wrong layer. That commit forwarded the Trigger's remaining props to the asChild child so they would stop vanishing — but React Aria already hands the child everything the trigger contract needs. DialogTrigger wraps its children in a PressResponder (react-aria-components/dist/private/Dialog.mjs), which publishes the press handlers, aria-expanded, aria-controls and the trigger ref through context. Nothing has to be forwarded for the trigger to work. So the props were never missing a destination; they never had one. Under asChild the consumer authors the child element and puts its props there, and the only thing Trigger contributes is the `trigger` slot's styling. Accepting the full React Aria button surface in that mode advertised a second place to configure the element, and forwarding it made things worse: Chakra's mergeProps chains `on*` handlers, so an onPress on both the Trigger and its child fired twice. PopoverTriggerProps is now a discriminated union — style props only under asChild, the full button surface otherwise. `<Popover.Trigger asChild id="x">` becomes a compile error pointing at the child, instead of compiling and doing nothing. Free to do here because Popover is unreleased. Menu.Trigger has the same shape and needs the same treatment, but it is Stable, so narrowing its props is a compile-time break for already-broken call sites and belongs in its own PR with a changeset. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(popover): trim the changeset to release-note scope The changeset had grown into a tutorial: a usage code block, two paragraphs of when-to-use prose, and six bullets covering details that belong on the docs page. Per docs/changeset-conventions.md a changeset is release notes for consumers, and shipped entries for new components (Breadcrumbs, Item) are a lead sentence plus a few tight bullets. Reshaped to that: what the component is, then the four things a consumer acts on — the accessible name Content cannot supply, the close render prop, where trigger props go under asChild, and the positioning and focus model. The code block and the narrative go to the docs page, which now carries all of it. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(popover): ring the dialog when it holds focus React Aria focuses the first focusable child when a popover opens, and falls back to the dialog itself when the content has none. The dialog slot set `outline: none`, so a keyboard user opening a text-only popover was left with no visible focus indicator anywhere on screen. The dialog now carries `focusVisibleRing: "outside"`, gated on `:focus-visible` so a mouse press still draws nothing. Popovers whose content has its own focusable element are unaffected, since focus never lands on the dialog there. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(localized-field): adopt the public Popover component The hint popover was assembled by hand from React Aria's DialogTrigger, Popover and Dialog, alongside an `infoPopover` slot that existed only to carry the four rules the shared flat recipe used to supply before it became a slot recipe. The public compound covers all of it: Root, Trigger with `asChild` around the IconButton that was already there, and Content, whose own `content` slot provides those same four rules. So the slot, its type and its recipe entry are gone, and `infoDialog` stays a plain div inside Content, which keeps its bespoke styling in the recipe rather than spilling it into style props. Two things change for consumers. The dialog now has an accessible name; it previously had none, which React Aria warns about in development. And it plays the shared fade and scale animations, so it stays mounted briefly after a dismissing click — checkFieldDetailsDialog waits for removal instead of assuming a synchronous unmount, which is what made two hints match the same text. `focusRing` came off `infoDialog`, which is no longer the focused element; the popover's dialog supplies the ring now. The fieldset's `aria-details` association with the trigger had no test, and moving that id onto an `asChild` child is precisely what could have broken it. It is now asserted, together with the dialog's accessible name and `aria-expanded` on the trigger. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(localized-field): let the popover surface style the hint box `infoDialog` carried its own background, border, shadow and corner radius from when the hint popover was assembled by hand out of React Aria primitives. Now that Popover.Content supplies the surface, the two boxes are exactly coincident (measured: same rect, to the pixel), so those rules were painting a second surface directly over the first. The shadow was the visible cost. `boxShadow: 6` on the inner box stacked on the surface's own `boxShadow: 5`, and the surface does not clip its children, so both drew into the region outside the shared rect — a doubled shadow the hint has been rendering all along. What remains is what the shared surface does not provide: the hint's bounds and its overflow. The hint now takes the same background, radius and shadow as every other popover. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(localized-field): note the hint popover's new appearance The changeset predated dropping the hint box's own background, border and shadow. A consumer upgrading sees that change, so it belongs in the release notes alongside the accessible name and the focus outline. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * revert(popover): drop the dialog focus ring Reverts a6564cf, which rested on a false premise. That commit assumed React Aria focuses the first focusable child on open and only falls back to the dialog when the content has none, making the ring a narrow fix for text-only popovers. Measured, React Aria focuses the dialog container on every open — `activeElement` is the dialog's `section` even when the content holds a Button. So a ring keyed to the dialog outlines every popover on open rather than indicating anything. It also drew in the wrong place. `padding: 400` lives on `content` so consumers can zero it with one prop, which leaves the dialog inset inside the visible surface: a 230x53 bubble around a 199x23 focused element, so the ring hugged the text instead of the bubble. Moving it to `content` and firing on `:has(> dialog:focus-visible)` fixed the geometry and avoided double-ringing when focus moves to inner content, but could not make the ring conditional — `defaultOpen` with no interaction at all still matched. `Dialog` and `Drawer` both set `outline: 0` on their surface for the same reason, and React Aria does not ring dialog containers either. The changeset drops its keyboard-only claim to match. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(popover): stop the a11y doc crashing on an undefined Kbd The keyboard table used `<Kbd>`, which the docs MDX runtime does not provide. Its component registry (apps/docs/src/components/document-renderer/components) exposes Box, Flex, Grid, Icon, Icons, PropsTable and the token demos — not Kbd — so rendering the page threw "Expected component `Kbd` to be defined" and took the whole view down. A lowercase `<kbd>` would not have worked either. The Paragraph base tag converts `<kbd>` to Nimbus's Kbd, but only inside a paragraph, and this is a table cell. Switched to inline code, which is what every other a11y doc and this component's own dev doc already use for key names. A scan of every component .mdx now reports no unprovided component references in prose. The docs build does not catch this: the MDX is stored as a string and compiled in the browser, so a missing reference only throws at render time. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(popover): gate the trigger ring on :focus-visible The trigger slot used `focusRing`, which Chakra expands to `&:is(:focus, [data-focus])`, so the ring drew on pointer focus as well as keyboard focus. `[data-focus]` is not an attribute React Aria Components emits, whereas `[data-focus-visible]` is, and Button and IconButton already use `focusVisibleRing`. FEC-1167 * fix(popover): stop the uncontrolled example blocking the docs page The `defaultOpen` example under "Uncontrolled mode" was a `jsx live-dev` fence, so the docs runtime mounted it already open. A popover is modal from the moment it mounts: it rendered over the page, contained focus, and hid the surrounding documentation from assistive technology as soon as the reader reached that section, which took the whole Implementation tab out of use. Switched to a static `tsx` fence. `defaultOpen` is still documented, but no live example can auto-open. The prose now says why it is shown as code, so the next author does not convert it back. This was the only auto-opening example across the four popover MDX files. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(popover): trim the designer doc to overview and variables The designer tab carried implementation content and duplicated two sibling tabs. The render-prop and `asChild` examples were byte-identical to popover.dev.mdx, the five best-practice alerts repeated popover.guidelines.mdx, and the overlay table restated the fuller comparison matrix that the Guidelines tab already owns. Dropped the `## Guidelines` section, keeping its two visual examples by moving "Basic usage" and "Interactive content" under `## Variables`, and replaced the overlay table with a pointer to the Guidelines tab. The file now reads Overview, Resources, Variables, matching dialog.mdx and drawer.mdx. Of every component carrying both a designer .mdx and a .guidelines.mdx tab, only three duplicated a Guidelines section in the designer file; popover was one. The writing-designer-documentation checklist still asks for that section and is stale on this point, tracked alongside the other stale guideline notes from review. Also escaped the `offset={16}` label, which the live renderer had been consuming as a JSX expression and printing as "offset16". FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(popover): cover focus containment and placement, de-flake focus waits Four changes to the story file, all from review: `StoryObj<typeof meta>` is the type-inference anti-pattern stories.md names explicitly, at a measured 2-5s per file against 10-150ms. The worst case it calls out is a meta whose component spreads Chakra style props via HTMLChakraProps, which PopoverContentProps does. Pinned to `StoryObj<typeof Popover.Content>`. Only two of the package's 82 story files still use the meta form. Added FocusContainment, placed before NonModal as its opposite. Portal components are required to cover focus management, the existing stories asserted containment only in prose, and popover.docs.spec.tsx defers all focus coverage here, so nothing tested it. It asserts focus enters the dialog, that Tab and Shift+Tab across three focusable children never leave it, that neither outside button took focus, and that Escape restores the trigger. Placement rendered `placement="right top"` and asserted nothing. It now checks React Aria's resolved `data-placement`. Asserted as an axis rather than an exact side: a flip under a tight viewport stays on the requested axis, so landing on right or left still proves the prop took effect, since the unset default is bottom. Wrapped four bare `toHaveFocus()` calls in `waitFor`. Focus restoration lands after the exit animation unmounts the overlay, so it is not observable on the dismissal tick. Every equivalent assertion in dialog.stories.tsx already waits. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(changeset): release the LocalizedField popover change as minor The changeset was marked patch, but its own text describes appearance changes existing consumers will see: one shadow rather than two stacked, no border or background tint, a fade in and out, and no outline when opened. Changeset conventions reserve patch for a bug fix with no API change and no new behaviour, and say to prefer minor when in doubt. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(serena): apply the project config schema migration Rewritten in place by the Serena MCP tooling on activation, not by hand: `languages` becomes `language_servers`, `additional_workspace_folders` becomes `ls_additional_workspace_folders`, and `ls_workspace_folders` is added. The effective settings are unchanged, still a single typescript language server over the project root. Unrelated to the popover work on this branch, so no ticket key; it is isolated in its own commit to make it easy to drop or move. * revert(serena): drop the project config schema migration Reverts ade312b. The Serena MCP rewrote `.serena/project.yml` in place on activation — `languages` became `language_servers`, `additional_workspace_folders` became `ls_additional_workspace_folders`, and `ls_workspace_folders` was added — and that rewrite was parked here in its own commit so it could be dropped or moved later. This is the drop. The migration is unrelated to the popover work and does not belong in this PR. Reverted rather than rewritten out of history so the pushed branch stays intact. The change is not lost: the tooling rewrites the file again on its next activation, so it can be proposed on its own from a branch off `main`. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(popover): narrow PopoverRootProps to the props Root honours PopoverRootProps intersected OmitInternalProps<PopoverRootSlotProps>, so it advertised every Chakra style prop and every native div attribute on top of the four state props it actually declares. None of them did anything. Both halves died, by different routes. The DOM half died in popover.root.tsx: extractStyleProps returns [styleProps, otherProps] and the destructure took only index 0, so id, className, onClick and data-* were computed and dropped on that line. The style half was forwarded to PopoverRootSlot and still died -- that slot renders with asChild, so Chakra clones its child instead of emitting the div, and the child is RaDialogTrigger, which mounts no host element. The resolved className and style went to a component that has no such props. Nothing else consumed them either: the recipe's root slot is {} with no variants. There is no react-aria component to route them to. DialogTriggerProps extends OverlayTriggerProps and adds children -- isOpen, defaultOpen, onOpenChange, children, and nothing more. The tell is DialogProps twelve lines down in the same file, which opts into StyleProps, DOMRenderProps and GlobalDOMAttributes; DialogTriggerProps opts into none of them. Forwarding to the trigger or the content element would be wrong on API grounds, not just implementation grounds -- both parts already own that surface, and Content is portaled out of Root's tree. So the type narrows rather than the plumbing moving. PopoverRootProps is now the four props, which makes the "renders no DOM element" contract a compile error instead of prose plus one story assertion. Dropped the now-dead extractStyleProps import, call and rest element, and the @supportsStyleProps tag, which had been making the docs site render a style-props banner on a component that supports none. PopoverRootSlotProps and PopoverRootSlot stay as they are. withProvider is what installs the slot-recipe context that portaled Popover.Content reads, per openspec/specs/nimbus-popover/spec.md:305. Verified padding, id, className and onClick on Popover.Root are now TS2322 while the four real props still pass. data-testid still typechecks and still does nothing: TypeScript exempts hyphenated JSX attribute names from excess-property checking, which affects every component in the package and is not closable here. The regenerated props table drops from a wrong three entries to the correct four, and reports supportsStyleProps false. The spec needed no change. Its two Root scenarios assert no layout box and slot-recipe context across the portal, both of which this reinforces. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(popover): forward Content's DOM props to the dialog element popover.content.tsx called extractStyleProps(props) and took only index 0, so the style half reached the surface and everything else was dropped: id, className, data-* and every event handler PopoverContentProps advertises. data-testid on the overlay is about the most predictable thing a consumer reaches for, and popover.dev.mdx teaches a persistent-id pattern that happens to demonstrate it on Trigger, which does forward. Unlike Root, Content has a real destination -- it renders two elements, so the question was which. Picked the dialog (RaDialog), not the surface (RaPopover), for three reasons. RaPopover's PopoverProps does not declare id at all; the interface keeps it internal via PopoverContextValue, which RAC uses for its own aria wiring. aria-label and aria-labelledby already route to the dialog, so sending id there keeps a consumer's identifying and naming props on one element. And the dialog is the element with role="dialog" -- the semantic target. Verified against the rendered DOM rather than inferred. The trigger's aria-controls points at the dialog's generated id, so a consumer id had to be checked for breaking that link: React Aria re-points aria-controls at whatever id it receives, so the link survives. className merges with the slot's classes instead of replacing them, because Chakra's asChild appends. Style props stay on the surface. RAC's Dialog runs filterDOMProps(props, {global: true}), which passes id and data-* ungated, plus the global attrs and the mouse, pointer, touch, scroll, animation and transition event families. It does not pass title, tabIndex, or the keyboard and focus handlers -- confirmed by probe: title comes back null, tabIndex stays at RAC's -1, onKeyDown and onFocus never fire. Those still typecheck via HTMLChakraProps and still do nothing. Left the base type alone rather than shipping a denylist that would drift with RAC versions; the JSDoc now states both halves of the contract, and consumers can put those handlers on their own content element. Added an AttributeForwarding story covering data-testid reaching the DOM, id landing with aria-controls still resolving to the dialog, className being added rather than swapped in, style props staying on the surface, and onClick firing. Dialog.Content has the identical dropped-restProps shape and is left alone: it is a shipped public component, so changing its forwarding is a separate change with its own changeset. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(popover): cover Escape not reaching a parent overlay The spec's Escape scenario has three clauses -- closes, restores focus, and "SHALL NOT propagate Escape to parent overlays". EscapeToClose covered the first two, but it renders a lone popover, so the third had nothing to assert against. It was the only behavioural clause in the narrowed spec with no play function behind it. Added EscapeStopsAtThePopover: a popover inside a Dialog, where one Escape closes the popover and leaves the Dialog open, and a second closes the Dialog. Verified the claim holds rather than assuming it -- React Aria's overlay stack does stop the key at the innermost layer. Also noted in popover.dev.mdx that the default Trigger renders a bare button. Its slot adds a focus ring and nothing else, so the form used in every doc example produces browser chrome, and a reader copying the first snippet gets something that does not look like Nimbus. The existing "two things worth knowing up front" list was the natural home, and the changeset bullet now says the same thing for consumers. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(popover): make Root the compound's configuration surface Popover composes four React Aria components, but Root only exposed DialogTrigger's four props. The behavioural surface of the Popover and Dialog elements -- the ones Content renders on Root's behalf -- was 14 props short of what React Aria offers: maxHeight, shouldUpdatePosition, scrollRef, getTargetRect, arrowBoundaryOffset, arrowRef, trigger, isEntering, isExiting, shouldSkipAnimation, UNSTABLE_portalContainer, the three onFocusWithin handlers. Root now takes all of them plus Dialog's role, and it is the only place any of them can be set. Content keeps its own presentation and labelling and nothing else, so there is exactly one place to look for a given option. Content renders through a portal and Root mounts no element, so the config travels by context. Only behavioural props are hoisted. Popover and Dialog share 78 prop names, 67 with incompatible types -- className is ClassNameOrFunction<PopoverRenderProps> on one and string on the other, every DOM handler differs by element type. Those cannot be intersected into one type, and Root has no element of its own to disambiguate against, so per-element presentation stays on the part that renders the element. isOpen/defaultOpen/onOpenChange are excluded from the published config by construction: PopoverConfigContextValue is PopoverRootProps minus keyof RaDialogTriggerProps. React Aria's Popover declares the same three and derives its own state the moment isOpen or defaultOpen is set on it (Popover.mjs:53), which would leave the trigger toggling one state while the surface renders another. RootOpenStateIsNotForwarded is the guard. offset and maxHeight are the only two of the 25 that collide with Chakra style props, and on Root neither is a styling concern -- both are positioning inputs that feed the placement and flip calculation. maxHeight turned out to be worse than a name clash: useOverlayPosition assigns overlay.style.maxHeight imperatively on every position pass, so the inline value outranks the recipe class and the style prop on Content could never have capped the surface. The docs claimed otherwise; corrected in the JSDoc, the dev doc and the spec. Moved config off Content at four doc call sites. The popover.mdx ones matter: live examples are not typechecked, so they would have rendered unpositioned with a React unknown-attribute warning rather than failing the build. Amended the unreleased changeset that introduces Popover instead of adding a second one, so consumers see one coherent note. LocalizedField needed no change. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(popover): align the specs and archived proposals with Root's surface Audited every popover doc, spec and proposal against the implemented set rather than assuming the previous commit had reached them all. It had not. The live spec contradicted itself in four places: `isNonModal` and the `offset` and `crossOffset` scenarios still attributed the prop to `Popover.Content`, and the placement scenario said "Root or Content" after I widened it, which stopped being true once Content began rejecting configuration. The spec also named only 9 of the 25 accepted props, enumerating `placement`, `offset` and `crossOffset` individually while leaving their siblings covered only by a categorical clause. Added an explicit scenario listing the full set by group, so the spec is a checkable contract instead of a description. It also records the two things a reader would otherwise trip on: `arrowRef` and `arrowBoundaryOffset` are accepted but inert while no OverlayArrow is rendered, and `UNSTABLE_portalContainer` keeps its prefix on purpose. Both archived changes described the old surface. Updated the statements that describe the current API, and marked the one design decision that was actively reversed -- `isNonModal` on Content -- as superseded rather than rewriting its reasoning, since the argument about React Aria's containment default still holds and only the part it is set on changed. Verified: no JSX anywhere passes a configuration prop to `<Popover.Content>`, and all 25 props plus the three open-state props now appear in the dev doc, the spec and the changeset. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(popover): trim the changeset to consumer-visible changes The Popover changeset had grown to 323 words, most of it reference material rather than release notes: an enumeration of all 25 props Root accepts grouped by category, the `maxHeight`/`offset` name-collision explanation, the which-part-receives-which-DOM-prop split, and the note that `arrowRef`/`arrowBoundaryOffset` are accepted but inert. docs/changeset-conventions.md scopes a changeset to what a consumer sees, types or runs, with one or two short sentences per bullet and a docs link for anything longer. The prop enumeration is exactly that "anything longer", and it already lives in the JSDoc and the dev doc, so it is now a link to the Popover docs page instead. Kept the five bullets that change what a consumer writes: the labelling requirement, the `close` render prop, `asChild` on Trigger, Root as the single configuration surface, and modal-by-default with `isNonModal` as the escape hatch. Added the Beta marker, matching how the Item/ItemGroup entry flagged its lifecycle state. Bump stays minor: new component, purely additive. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(popover): correct where isNonModal and placement are set Two doc sites still described the surface from before Root became the compound's configuration surface, and both would fail if a consumer copied them. popover.tsx's namespace JSDoc told consumers `isNonModal` on `Popover.Content` relaxes focus containment, and the `Popover.Content` @example opened with `placement="bottom start"`. Content rejects both: PopoverContentProps is OmitInternalProps<PopoverContentSlotProps> plus children and the two labelling props, and PopoverContentSlotProps is HTMLChakraProps<"div">, so neither name is in the type. Verified with a throwaway probe passing both to `<Popover.Content>` — TS2322 on the object literal, naming the full accepted shape. popover.stories.tsx's Placement description said "Content accepts React Aria placement values" while the story below it correctly sets `placement="right top"` on Root. The story was right and its own description contradicted it. The preceding commit audited the spec, dev doc and archived proposals for this same drift; these three call sites were the ones it did not reach. Grepped the component's tsx and mdx for other behaviour props attributed to Content and found none left. popover.types.ts:124 mentions `Popover.Content` too but is correct — there `maxHeight` really is the Chakra CSS style prop, in contrast to Root's positioning cap. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(popover): drop comments duplicated by the types and recipe The implementation files had accumulated prose that restated the JSDoc on popover.types.ts, and in one case said the same thing in three places. popover.content.tsx carried the largest block, 42 lines: the React Aria "requires a Dialog inside a Popover" rationale, the `close` render prop, Root-owns-all-configuration, and the `maxHeight` imperative-assignment caveat. Every one of those is already on PopoverContentProps, which is the JSDoc a consumer actually sees on hover; the guideline pattern for implementation files is a summary line plus @supportsStyleProps. Kept the part that is not recoverable from the code and warns off a plausible refactor: the two-element render split, and that RaPopover rejects `id`, so routing a consumer id to the dialog is what keeps aria-controls pointing at it. "DialogTrigger mounts no DOM element, so the root slot exists only for withProvider" appeared in popover.root.tsx, popover.slots.tsx and popover.recipe.tsx. Kept it in the recipe, where an empty `root: {}` begs the question, and cut it to a clause in the other two. Root keeps its open-state rationale — why the three props are destructured out rather than spread — since that is what stops someone "simplifying" the guard that RootOpenStateIsNotForwarded covers. popover.context.tsx now points at Root for it instead of re-deriving it, and its hook JSDoc no longer narrates a `?? {}`. Trigger keeps the PressResponder note and loses two block comments that restated `if (props.asChild)`. One thing moved rather than deleted: the inventory of which DOM props survive React Aria's filter existed only in the content.tsx block, with popover.types.ts:231 pointing at it as "see the note on PopoverContent" and the dev doc not covering it. It is now inline in the types JSDoc and the dangling pointer is gone. Left popover.tsx alone — its 81 comment lines are the per-part namespace JSDoc that main-component.md mandates. Left the recipe's "Base styles applied to all instances" boilerplate, which 6 of 9 recipes share. Left the story play-function comments: they explain why each assertion is shaped as it is, including the exit-animation timing that a reader would otherwise "fix" into a flake. Comment lines: content 46 to 13, root 23 to 12, trigger 24 to 14, context 20 to 12, slots 10 to 4. typecheck:dev, eslint and prettier clean. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(popover): reject role, offset and maxHeight on Content Root is meant to be the compound's only configuration surface, and four places said so, but PopoverContentProps derives from HTMLChakraProps<"div"> and three configuration names survive that derivation: `role` via React's HTMLAttributes, `offset` and `maxHeight` as CSS properties Chakra recognises. Verified each with a throwaway probe. `role` was the one with teeth. It is not a Chakra style property, so it fell through extractStyleProps into restProps, which was spread after `role={role}` — a Content-level role silently outranked Root's, with no precedence rule documented anywhere. Its DOM type is the whole AriaRole union, so `<Popover.Content role="tooltip">` also typechecked and reached the dialog element, a role RaDialog itself rejects. `offset` and `maxHeight` were quieter: both compile to inert CSS, so `offset={12}` read as "nudge the popover" and did nothing. All three are now excluded by name, and restProps spreads before `role={role}` so Root remains authoritative even for an untyped consumer. The compile-time half is a step in the RootConfiguration play function: three @ts-expect-error literals typed as PopoverContentProps, which fail the build with TS2578 the moment a name is re-admitted. It imports the type from @commercetools/nimbus, so the prod typecheck holds the published surface to it. Docs realigned to the invariant now that it is real: the stale precedence sentence in PopoverRootProps' JSDoc (Content "may still override … the nearer value wins") is gone, the dev.mdx NOTE explains why the three names are excluded rather than merely ineffective, and the spec's colliding-names scenario names `role` as the third collision. The `maxH` shorthand and `maxBlockSize` still compile on Content and still cannot cap the surface — React Aria writes overlay.style.maxHeight inline on every position pass — so that residue is documented rather than omitted. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(popover): correct the non-modal and labelling claims in the docs Auditing every doc, spec and archived proposal against the implementation turned up three behavioural claims that the code does not support. All three were verified in headless Chromium (throwaway probe stories, since JSDOM is not authoritative for focus or pointer semantics), not by reading alone. `isNonModal` does not relax focus containment. useDialog calls useOverlayFocusContain() unconditionally, so the dialog Content always renders contains focus whether or not the popover is modal. Probed: with isNonModal, Tab cycles inside-two -> inside-one -> inside-two and never reaches an outside button, and an outside press still dismisses (usePopover keeps shouldCloseOnBlur: true even when isDismissable is off). What isNonModal actually relaxes is the page behind the overlay: modal marks it inert (ariaHideOutside with shouldUseInert: true) and locks page scroll; non-modal does neither. Six sites said "relaxes containment", including the changeset. The NonModal story asserted this vacuously. It looked up the outside button with getByRole, whose comment claimed ariaHideOutside would hide it without isNonModal. Testing Library skips aria-hidden subtrees but not inert ones, and the mechanism is inert now, so that lookup succeeded in both modes and proved nothing. It now asserts the absence of inert and of the scroll lock, plus that focus stays contained anyway, and FocusContainment asserts the modal side of both — so the pair brackets the real difference. A popover without an explicit name is not unnamed. RAC's DialogTrigger sets overlayProps['aria-labelledby'] to the trigger's id as a deliberate fallback ("Label dialog by the trigger as a fallback if there is no title slot"), so an unnamed popover inherits the trigger's accessible name. Probed both ways: a Heading with slot="title" names the dialog, a plain visible heading does not. So "give it an aria-label whenever the content has no visible heading" was wrong twice over, in the a11y doc, the types and the changeset. Two narrower corrections in the same pass. Content's prop docs claimed `aria-*` and event handlers reach the dialog; RAC builds its DOM props with filterDOMProps({global: true}), whose only wildcard is data-*, so aria-live and aria-roledescription are accepted and dropped while the labelling four arrive (probed). And the `role` rationale I added in the previous commit described pre-fix behaviour: since restProps now spreads before role={role}, an untyped role is dropped rather than outranking Root, which is the argument for excluding the name. Also corrected from the same audit: maxHeight "feeds the placement and flip calculation" (calculatePosition compares flip before applying the cap, so only placement is true within a pass), maxHeight described as the CSS property it "denotes on Popover.Content" where it is now a type error, a Root-less Popover.Content described as "working" when it throws on the missing slot context, popover.mdx omitting logical start/end as placement sides and labelling the default gap "No offset" when RAC defaults offset to 8, and Root's docgen summary never mentioning that it is the configuration surface. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(popover): resync the specs, proposals and guidelines with the code Same audit, the surfaces outside the component directory. Every item below was checked against a source line, and the ones that turned out to be already correct are left alone. openspec/specs/nimbus-popover: - The exit animation "SHALL use the same duration as the enter animation". The recipe uses `fast` to enter and `faster` to exit — deliberately shorter, copied from Menu — so the spec described an animation we never shipped. - The isNonModal scenario required that focus "SHALL NOT be contained". The opposite is true and is now asserted by two stories; rewritten around inert and the scroll lock, which is what modality actually changes here. - The accessible-name scenario assumed a visible heading names the dialog. RAC falls back to the trigger's name instead. - The maxHeight carve-out named only `maxH`; `maxBlockSize` is equally accepted and equally unable to cap the surface. openspec/specs/nimbus-localized-field: the info-box scenario still specified the surface treatment its own recipe used to carry (bg neutral.1, boxShadow 6, focusRing outside). After the migration onto the Popover compound, infoDialog carries only sizing and scrolling, the surface comes from the compound's content slot, and the only focus ring is the hint trigger's. Archived 2026-08-20-popover-compound-export. These are historical records, so only present-tense claims about the shipped API are corrected and the narrative framing is left intact: - design.md decided LocalizedField would drop to bare React Aria and gain an `infoPopover` slot, concluding the compound ships with zero internal consumers. It shipped with one: LocalizedField renders the compound, no `infoPopover` slot exists, and focusRing was removed rather than relocated. Recorded as a Superseded note in the file's existing style rather than by rewriting the decision — the ComboBox half still holds, and the paragraph above it that reads the old recipe as evidence is correctly in past tense. - proposal.md's impact table claimed the same three LocalizedField changes. - tasks.md 5.8 recorded asserting `aria-haspopup="dialog"` on the trigger; the story asserts its absence, and the change's own design.md argues for that. - tasks.md 3.1 described Root splitting recipe variants and extracting style props (it does neither, and the recipe declares no variants), and 2.2 named a recipe-props layer that popover.types.ts does not have. - The delta's portal scenario predated `UNSTABLE_portalContainer` landing on Root, and its offset/crossOffset scenarios were unqualified as to which part they are set on, which now reads as the wrong part. docs/file-type-guidelines/compound-components.md illustrated flexible child composition with `Popover.Title`, `Popover.Description` and `Popover.Actions`, none of which exist, and no trigger. Retargeted to Dialog, which really has those parts (verified against dialog.tsx: Header/Title/Body/Footer/ CloseTrigger), and added a Popover example showing the opposite case, where the consumer's own markup fills that role. Line 26 of this file had already been corrected to three parts; the sample 440 lines down had not. Untouched deliberately: the earlier serena/openspec metadata, the released CHANGELOG, and archived prose that is already framed in the past tense. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(popover): simplify Popover.Content usage instructions and clarify naming requirements * docs(popover): revert the compound-components guideline edits Out of scope for this branch, which adds the Popover component. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(popover): condense the types file comments The type-level blocks had grown into essays duplicating popover.dev.mdx and the nimbus-popover spec. Keep the actionable half — what a part rejects and where to set it instead — and link the docs site for the rest. Per-property JSDoc is untouched, so the docgen output is unchanged. Comments only; the type surface is identical. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(popover): document slot="close" for dismissing from content Popover.Content renders a React Aria Dialog, which publishes a ButtonContext with a close slot, so `<Button slot="close">` works inside it — the same way it already does in Dialog and Drawer. The docs only showed the function-child render prop, so the conventional form looked unavailable. Lead with slot="close" and keep the render prop for the case it is actually needed: an action that has to run before the popover dismisses. Add a CloseViaSlot story so the behaviour is covered rather than just claimed. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(popover): drop the non-conforming dev.mdx sections "Where configuration goes" restated the API design rather than showing usage: its prop table duplicated the generated <PropsTable>, its example duplicated the one in "Positioning" directly above it, and no other component documents configuration this way — Root taking the configuration is the house pattern. The exclusion rationale it carried is already in the nimbus-popover spec. "Styling" was one of only two such sections across 88 dev.mdx files. Usage examples is now feature sections with live examples throughout, matching the peer components and the template. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(popover): move the isNonModal mechanics to the Implementation tab The `isNonModal` paragraph in the Accessibility tab documented prop mechanics (`inert`, page scroll, why the prop exists) rather than accessibility behavior, and `popover.dev.mdx` already carried a condensed copy of the same claim in its Component requirements section. Keep the a11y-facing consequence in `popover.a11y.mdx` — containment is not lifted, and most popovers should not use the prop — and move the mechanics into the Implementation tab's Dismissal behavior section, alongside `isKeyboardDismissDisabled` and `shouldCloseOnInteractOutside` and matching the spec's "dismissal and modality" grouping. The condensed duplicate becomes a Modality bullet pointing there. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent d5ae7e0 commit 3ccc421

37 files changed

Lines changed: 4284 additions & 402 deletions

File tree

.changeset/lucky-pandas-explore.md

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
"@commercetools/nimbus": minor
3+
---
4+
5+
`Popover`: new compound component for interactive content anchored to a trigger
6+
`Popover.Root`, `Popover.Trigger` and `Popover.Content`. Reach for it for
7+
filter panels, short edit forms and context-sensitive actions; `Tooltip` still
8+
covers plain hints, and `Dialog` covers flows that should block the page.
9+
10+
Name `Popover.Content` with `aria-label` or `aria-labelledby` — a visible
11+
heading inside the popover does not name it.
12+
13+
Beta. See the
14+
[Popover docs](https://nimbus-documentation.vercel.app/components/feedback/popover)
15+
for the full prop reference.

.changeset/wise-moons-listen.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
"@commercetools/nimbus": minor
3+
---
4+
5+
`LocalizedField`: the info popover opened by the hint button now has an
6+
accessible name, so screen readers announce it instead of an unnamed dialog. It
7+
also matches the appearance of other popovers — one shadow rather than two
8+
stacked, no border or background tint of its own — and fades in and out like
9+
them. It no longer draws an outline around itself when opened, matching `Dialog`
10+
and `Drawer`.
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
schema: spec-driven
2+
created: 2026-08-20
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
# Proposal: Correct the Popover Optimized Rendering requirement
2+
3+
## Why
4+
5+
Rewrite the `Optimized Rendering` requirement in the `nimbus-popover` capability
6+
so it describes observable Popover behavior instead of React Aria internals and
7+
a configuration option that does not exist.
8+
9+
### Background
10+
11+
`popover-compound-export` narrowed this capability from an eight-part
12+
aspirational surface to the three parts actually built, and corrected the
13+
requirements it touched. `Optimized Rendering` was not among them, so its
14+
`Scroll optimization` scenario still specifies two things the component cannot
15+
be held to:
16+
17+
- **`SHALL use requestAnimationFrame for positioning updates`** — an assertion
18+
about React Aria's internals. Nimbus neither implements nor tests it, and it
19+
would silently become false if React Aria changed its scheduling.
20+
- **`MAY close popover on scroll if configured`** — there is no such option.
21+
`Popover.Root` accepts `placement`, `offset`, `crossOffset`, `shouldFlip`,
22+
`containerPadding`, `boundaryElement`, `isNonModal`,
23+
`isKeyboardDismissDisabled`, `shouldCloseOnInteractOutside` and `triggerRef`,
24+
and none of them dismisses on scroll. (At the time of this change those props
25+
were accepted on `Popover.Content`; they moved to `Popover.Root` when it
26+
became the compound's single configuration surface. The conclusion is
27+
unaffected.)
28+
29+
Both are the same class of inaccuracy as the `portalContainer` and
30+
`trigger="hover"` scenarios corrected in the previous change, and they were left
31+
behind only because that change's scope did not reach this requirement.
32+
33+
## What Changes
34+
35+
Keep both scenario names and restate their bodies against what the component
36+
actually does:
37+
38+
- `Lazy mounting` is accurate and stays, worded against the overlay content
39+
rather than "content in DOM".
40+
- `Scroll optimization` becomes a statement about the observable outcome — the
41+
popover stays anchored to its trigger and within the viewport while the page
42+
moves — and records that no scroll-dismissal option is exposed.
43+
44+
## Scope
45+
46+
- **In scope**: the `Optimized Rendering` requirement of `nimbus-popover`.
47+
- **Out of scope**: any implementation change. This corrects the specification
48+
to match shipped behavior; no source file changes.
49+
50+
## Impact
51+
52+
- **Breaking changes**: None
53+
- **Migration required**: None
54+
- **Dependencies**: None
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
## MODIFIED Requirements
2+
3+
### Requirement: Optimized Rendering
4+
5+
The component SHALL keep overlay content out of the DOM while closed, and SHALL
6+
stay anchored to its trigger while the page moves.
7+
8+
#### Scenario: Lazy mounting
9+
10+
- **WHEN** the popover is closed
11+
- **THEN** the overlay content SHALL NOT be mounted in the DOM
12+
- **AND** it SHALL mount when the popover opens
13+
- **AND** it SHALL unmount once the exit animation has finished
14+
15+
#### Scenario: Scroll optimization
16+
17+
- **WHEN** the page scrolls or the viewport resizes while the popover is open
18+
- **THEN** the popover SHALL remain anchored to its trigger
19+
- **AND** it SHALL stay within the viewport, flipping or shifting as needed
20+
- **AND** no scroll-dismissal option SHALL be exposed
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
# Tasks: Correct the Popover Optimized Rendering requirement
2+
3+
- [x] Task 1: Rewrite the `Optimized Rendering` requirement as a delta
4+
- [x] Task 2: Validate and archive
5+
6+
---
7+
8+
## Task 1: Rewrite the `Optimized Rendering` requirement as a delta
9+
10+
**File:**
11+
`openspec/changes/correct-popover-rendering-spec/specs/nimbus-popover/spec.md`
12+
13+
- Restate `Lazy mounting` against the overlay content.
14+
- Replace the `Scroll optimization` body: drop the `requestAnimationFrame`
15+
claim and the non-existent scroll-dismissal option, and state the observable
16+
anchoring behavior instead.
17+
- Keep both scenario names, since a MODIFIED requirement may not drop scenarios
18+
the current spec still has.
19+
20+
**Validation:**
21+
22+
- `pnpm exec openspec validate correct-popover-rendering-spec --strict` clean
23+
24+
## Task 2: Validate and archive
25+
26+
- Archive so the delta applies to `openspec/specs/nimbus-popover/spec.md`.
27+
28+
**Validation:**
29+
30+
- `openspec/specs/nimbus-popover/spec.md` no longer mentions
31+
`requestAnimationFrame` or closing on scroll
32+
- No source or test files change
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
schema: spec-driven
2+
created: 2026-08-19

0 commit comments

Comments
 (0)