feat(popover): gen2 popover migration#6356
Merged
Merged
Conversation
Adds sidebar entries for accessibility migration analysis docs on action-button, button-group, close-button, grid, and infield-button, plus the link migration plan, so they appear under their components in the Storybook navigation.
Extracts Floating UI-based positioning from 1st-gen overlay's PlacementController into a standalone reactive controller under core/controllers. Adds start/stop/recompute API, hyphenated Placement union aligned with Floating UI and swc-popover, opt-in constrainSize for picker/menu scroll cases, and VirtualTrigger support for virtual anchors. Wires the floating-ui/dom dependency and package.json export for the new controller subpath. Also lands the supporting Storybook ApiTable block plus the ConditionalAPIReference template change, and migrates the focusgroup-navigation-controller stories to the new controllerApi parameter pattern. SWC-1996
Address correctness bugs, plan-deviating defaults, an iOS regression
risk, and minor structural issues surfaced during code review.
Correctness
- toFloatingPlacement: logical-side + physical-alignment placements
(start-top, start-bottom, end-top, end-bottom) previously returned
invalid Floating UI strings (left-top, left-bottom, right-top,
right-bottom) because the logical-side branch did not consult
PHYSICAL_ALIGNMENT_TO_FLOATING. They now resolve correctly to
left-start / left-end / right-start / right-end.
- fromFloatingPlacement: Floating UI's left-start, left-end,
right-start, right-end are not in the SWC Placement union; the
function now maps them back to the physical equivalents
(left-top / left-bottom / right-top / right-bottom). The
previously unchecked cast is gone.
Defaults aligned with the popover migration plan
- DEFAULT_OFFSET: 8 to 0 (controller is now neutral; each consuming
component sets its own pattern-specific default).
- DEFAULT_CONTAINER_PADDING: 12 to 8 (matches 1st-gen
REQUIRED_DISTANCE_TO_EDGE and Spectrum guidance).
- Storybook args and argType defaults updated to match.
Behavior
- Restored an iOS WebKit visualViewport listener (passive,
rAF-coalesced) so positioning stays correct when the URL bar,
pinch-zoom, or virtual keyboard shifts the visual viewport without
triggering events that Floating UI's autoUpdate observes. The
matching offset compensation in computePlacement was already
present; only the trigger was missing.
- Comment on the single autoUpdate channel explaining the
intentional difference from 1st-gen, which closed the overlay on
ancestor scroll rather than repositioning.
Structure
- Added a reserved PlacementHostConfig interface and updated the
constructor to accept an optional config argument, so future
integration hooks (e.g. tip-element resolver for arrow middleware)
can be added without a constructor signature change.
- Cached isWebKit() per session so computePlacement no longer runs a
UA regex per autoUpdate tick.
- Removed the internal-only getFallbackPlacements re-export from
src/index.ts (it is not part of the public surface).
- Expanded JSDoc on actualPlacement (initial-value semantics),
fromFloatingPlacement (invariants), and toPlacementClassSuffix
(why the indirection exists).
Tests
- Added play-function stories covering the placement-conversion fix
end to end, the logical-side placements computing valid
coordinates, constrainSize applying max-height, shouldFlip: false
preserving the requested side, and rapid start() calls replacing
the prior session.
SWC-1996
- Drop toPlacementClassSuffix. It was a pass-through that just returned
its input; consumers can do the BEM string interpolation themselves.
- Strip {@link Foo} JSDoc annotations across placement-controller
files; they don't render in this environment. Replaced with plain
backtick references.
- Remove popover-specific phrasing from controller comments and JSDoc
so the controller stays host-agnostic. Generic mentions of popover,
picker, menu, etc. as example use cases in story descriptions are
retained where they help the reader.
- Revert the focusgroup-navigation-controller stories file. The
controllerApi parameter pattern was not required for the
placement-controller API table to render; the focusgroup keeps its
prior inline-JSDoc API documentation.
Revert the controllerApi infrastructure added to ApiTable.tsx and DocumentTemplate.mdx. Document the placement-controller's methods, readonly properties, options, and types as inline markdown tables in JSDoc on a description-only API story — matching the focusgroup-navigation-controller pattern. - ApiTable.tsx: drop the ControllerApiReference type, five controller-specific table components, and the tag-based branching added for controllers. Component CEM rendering is unchanged. - DocumentTemplate.mdx: restore ConditionalAPISection (Primary + Controls + optional stories tagged 'api') and drop the ConditionalAPIReference / ApiTable wiring. - placement-controller stories: remove parameters.controllerApi, add an API story with inline JSDoc tables tagged ['api', 'description-only'].
Replace the two side-by-side flip demos (demo-placement-flip and demo-placement-no-flip) with a single demo-placement-should-flip that has a shouldFlip checkbox. One trigger + one floating panel + one toggle isolates the feature so readers can verify the placement only moves because of the flip middleware. - demo-hosts.ts: delete DemoPlacementFlip and DemoPlacementNoFlip; add DemoPlacementShouldFlip with a should-flip attribute, a checkbox in the demo template, and an updated() hook that rebinds the controller when the toggle flips. - placement-controller.stories.ts: ShouldFlip story now renders a single demo-placement-should-flip element. Story description gained a sentence explaining the toggle. - placement-controller.test.ts: FlipReorients reads from the new element (default shouldFlip=true). NoFlipKeepsRequestedSide reuses the same element, sets shouldFlip=false, then asserts actualPlacement stays at 'bottom'.
- Virtual trigger demo: surface is a square (aspect-ratio 1, max 320px) so the click area reads as a single anchor canvas. The 'Click to move anchor' label uses Body sizeL emphasized so it reads as the demo's call to action. Added a 12px offset so the floating panel sits visibly clear of the clicked point. - Constrain-size demo: dropped the dashed outline around the surface. It implied the floating element was bounded by the surface, but the size middleware uses the viewport / clipping ancestors, so the scaffolding was misleading. - Story docs: removed prescriptive lines that named specific consumer components (picker / menu / combobox / tooltip / popover) in the description for shouldFlip, constrainSize, the meta JSDoc, and the subtitle. The controller is host-agnostic. - Documentation template: replaced the bare <ApiTable /> with a ConditionalApi helper. Controllers (tag 'controller') document their API via inline JSDoc stories tagged 'api', so they no longer trigger the 'Component not found in manifest' fallback. Components are unchanged.
Switch the virtual-trigger demo surface from a responsive square (inline-size: 100%; max-inline-size: 320px; aspect-ratio: 1) to a fixed 320x320px so the click target is the same size at any container width.
- Drop the dashed outline on the surface; replace with a subtle tinted background and a rounded border for a calmer visual. - Add 'resize: vertical' so the user can drag the bottom edge of the box to grow it. Pushing the trigger toward the bottom of the viewport makes the requested 'bottom' placement no longer fit, so toggling shouldFlip becomes meaningful — the user can watch the panel flip above when enabled and stay below (overflowing) when disabled. - Added a short hint paragraph above the surface explaining what to do.
The interactive resize-to-flip demo didn't reliably demonstrate the feature — flip depends on the trigger's viewport position, which is fragile across canvas sizes. Documenting the behavior in the story description is clearer than a demo that may not flip. - Story: ShouldFlip becomes a description-only story (no render). - Demo hosts: drop DemoPlacementShouldFlip, its tagname map entry, the FLIP_DEMO_ITEMS items, and the flipDemoStyles sheet that was only used by it. - Tests: drop FlipReorients and NoFlipKeepsRequestedSide, plus the ShouldFlip / DemoPlacementShouldFlip imports they depended on. Pure-function and constrainSize tests still exercise the controller surface.
The docs referenced `shift` in seven places as if it were an opt-in option. It isn't — `shift` runs on every compute and there's no opt-out (same as 1st-gen). Spell that out in the middleware stack section so the existing prose references read correctly.
shift is not a configurable option, so listing it alongside flip and
size in option JSDoc and story prose just makes those references read
like undocumented options. Replace the lists with neutral phrasing
("used for collision detection" / "inset from the overflow
boundary") and keep a single shift mention in the middleware-stack
section, where the always-on note belongs.
Adds six tests against a new DemoPlacementTestFixture host, which pins the trigger to a configurable viewport edge / center and can optionally swap in a 600px-tall floating panel. The fixture isn't referenced from any docs story; the test file is the only consumer. New tests: - FlipReorients — bottom placement reorients when the panel can't fit below a trigger anchored to the viewport bottom. - NoFlipKeepsRequestedSide — same setup with shouldFlip: false keeps the requested side, overflow and all. - OffsetMovesAlongPlacementAxis — offset: 40 shifts translateY by ~40 px for a 'bottom' placement. - CrossOffsetMovesAlongTriggerEdge — crossOffset: 40 shifts translateX without materially affecting translateY. - ContainerPaddingMovesPanelInward — with the trigger near the right edge, a larger containerPadding pulls the panel further inside the boundary. - OnPlacementChangeFiresOnChangeOnly — callback is silent when the computed placement matches the requested one and fires once when flip reorients. DemoPlacementTestFixture also exposes placementChanges (records each onPlacementChange invocation since the last rebind) and the controller field directly, so tests can read firing semantics or call recompute() without going through indirection.
…ways-on size Reconcile two behavioural deviations from 1st-gen that the migration plan didn't justify against the cost of breaking consumer parity. Middleware order — restored to 1st-gen: offset → shift → flip → size. The 2nd-gen had switched to offset → flip → shift → size on the rationale that it was 'Floating UI's canonical recommended order'; Floating UI doesn't actually prescribe one, and the two orders produce different positions in edge cases (trigger near a corner, panel close to viewport edge). The 1st-gen pattern is what downstream consumers have been seeing for years — no upside to changing it. size middleware — now always installed. Dropped the constrainSize option. The apply callback uses 1st-gen semantics: it tracks initialHeight from the first un-constrained compute as the baseline, writes max-width on every compute, and writes max-height only when isConstrained is true (content overflows the available space). stop() now unconditionally clears max-height / max-width and resets initialHeight. The ConstrainSize story was renamed to SizeAlwaysClamps and reframed as documentation of the always-on behaviour rather than an opt-in toggle. PlacementOptions.constrainSize is removed; the Playground demo's constrainSize property + attribute + bind entry are gone; the API table no longer lists constrainSize; the test that previously asserted 'constrainSize applies max-height' was rewritten to use the test fixture with tallFloating + triggerPosition='bottom-center' + shouldFlip=false so the overflow scenario is deterministic.
…-gen) The controller now installs Floating UI's arrow middleware when a tipElement is passed in PlacementOptions, matching 1st-gen behaviour. After every compute the controller writes inline translate on the tip element using the same pattern as 1st-gen: - top:0 reset for left/right placements (arrow on a vertical edge) - left:0 reset for top/bottom placements (arrow on a horizontal edge) - translate carries the arrow.x / arrow.y from middlewareData CSS positions the tip element relative to the floating element's edge (typically with a negative offset for the half-size of the arrow); the controller only slides it along that edge so it stays pointing at the trigger's center as shift moves the floating panel. API additions: - PlacementOptions.tipElement?: HTMLElement - PlacementOptions.tipPadding?: number (default 8) - stop() now clears the tip element's inline translate/top/left Demo + tests: - New demo-placement-arrow: trigger + floating panel with a CSS triangle tip. The host passes the tip to the controller. - New Arrow story in the docs page (section-order: 8) under Behaviors. - New ArrowMiddlewarePositionsTip test asserts the tip receives a numeric inline translate after the first compute. - Middleware-stack docs and API options table updated.
…ed trigger - Trigger button: drop the fixed 48x48 sizing and use min-block-size + inline padding so 'Trigger' fits naturally without overflowing. - Default placement: switch from 'bottom' to 'bottom-end' so the tip's computed offset from the floating panel's center is clearly visible (with 'bottom', the tip would sit at the center and there'd be nothing visually distinct from a CSS-centered tip).
Renames the references in code comments and story prose only — no behaviour change. Affects the middleware-stack JSDoc, the controller's inline comments about the order match and tip-positioning pattern, the SizeAlwaysClamps story's 'matches gen1 behaviour' note, and the appendix 'Relationship to gen1 PlacementController' section.
…ghten assertions Move the behavioral tests off the interactive demo-placement-playground host onto the lean, property-driven demo-placement-test-fixture so they no longer depend on the demo's controls, placement picker, or layout. Tighten loose assertions to the actual expected geometry: start/end alignment gap, below-trigger position, offset and cross-offset deltas, container-padding inward shift, and the flip-reorients placement value.
Replace the loose regex check on the tip's serialized translate with direct style and geometry assertions: the controller pins the tip to the floating edge (left: 0) and the arrow middleware centers it on the trigger. Read the tip and trigger rects directly and assert their centers align.
The size middleware's apply callback writes baseline state (isConstrained, initialHeight) during computePosition. A rapid start()/stop() replacement could let a stale in-flight compute bleed that state into the new session, so bail when the session has been replaced.
|
Contributor
📚 Branch Preview Links🔍 Gen1 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
Addresses PR review: the size middleware no longer writes max-width / max-height inline (which overrode a component's intended CSS max-size, e.g. Tooltip wouldn't wrap until viewport width). Instead it exposes --swc-placement-available-width / --swc-placement-available-height on the floating element; components opt in via min() so their intended size wins and overflow stays their concern (resolving the box-sizing overflow too). - stop() removes the custom properties; isConstrained kept as an informational flag (no longer drives an inline max-height) - document the contract + consumer pattern on the controller and in stories - demo-placement-constrain-size consumes the props so it clamps and scrolls - tighten the size test to assert real bounded values; no inline max-* written
Addresses PR review: logical primary sides must resolve to the correct physical side here, not in CSS, or the panel lands on the wrong side in RTL. - toFloatingPlacement takes a direction arg; start->right / end->left in RTL (and start-top -> right-start, etc.) - the controller reads getComputedStyle(trigger).direction (falling back to the floating element for a VirtualTrigger), honoring a scoped dir, not document.dir - logical alignment suffixes (bottom-start, top-end) pass through unchanged so Floating UI's own RTL handling flips them without double-flipping - actualPlacement stays physical; add RTL conversion tests
rubencarvalho
added
Component:Popover
2.0.0
Status:Ready for review
nikkimk
requested changes
Jul 8, 2026
nikkimk
left a comment
nikkimk
left a comment
Contributor
There was a problem hiding this comment.
See my documentation suggestions.
Comment on lines
+43
to
+44
|
|
||
| ### States |
Contributor
There was a problem hiding this comment.
This is a duplicate heading.
Suggested change
| ### States |
Contributor
Author
There was a problem hiding this comment.
Done. Removed the duplicate ### States heading.
Comment on lines
+64
to
+72
| ### Modal | ||
|
|
||
| Setting `modal` switches the surface to a `<dialog>` opened with `showModal()`: focus is trapped inside, the page behind is inert and its scroll is locked, and the surface exposes `role="dialog"`. Dismiss with `Escape` or a click on the backdrop. Name the dialog with `accessible-label` (see [Accessibility](#accessibility)). | ||
|
|
||
| <Canvas of={Stories.Modal} /> | ||
|
|
||
| #### Default-mode dismissal | ||
|
|
||
| In the default (non-modal) mode the surface is a `<div popover="auto">`: the browser dismisses it on `Escape` or a click outside its own content, and the page behind stays interactive and scrollable. Clicking or focusing content inside the popover does not dismiss it. |
Contributor
There was a problem hiding this comment.
Suggested change
| ### Modal | |
| Setting `modal` switches the surface to a `<dialog>` opened with `showModal()`: focus is trapped inside, the page behind is inert and its scroll is locked, and the surface exposes `role="dialog"`. Dismiss with `Escape` or a click on the backdrop. Name the dialog with `accessible-label` (see [Accessibility](#accessibility)). | |
| <Canvas of={Stories.Modal} /> | |
| #### Default-mode dismissal | |
| In the default (non-modal) mode the surface is a `<div popover="auto">`: the browser dismisses it on `Escape` or a click outside its own content, and the page behind stays interactive and scrollable. Clicking or focusing content inside the popover does not dismiss it. | |
| ### Focus trapping and dismissal | |
| #### Default-mode dismissal | |
| In the default (non-modal) mode the surface is a `<div popover="auto">`: the browser dismisses it on `Escape` or a click outside its own content, and the page behind stays interactive and scrollable. Clicking or focusing content inside the popover does not dismiss it. | |
| #### Modal | |
| Setting `modal` switches the surface to a `<dialog>` opened with `showModal()`: focus is trapped inside, the page behind is inert and its scroll is locked, and the surface exposes `role="dialog"`. Dismiss with `Escape` or a click on the backdrop. Name the dialog with `accessible-label` (see [Accessibility](#accessibility)). | |
| <Canvas of={Stories.Modal} /> |
Contributor
Author
There was a problem hiding this comment.
Done. Applied your structure: ### Focus trapping and dismissal with #### Default-mode dismissal and #### Modal subsections.
| - The popover needs an **accessible name**: set `accessible-label` and its value is applied to the surface. The name is required because the surface is exposed with `role="dialog"` internally, and the component warns in development when a popover opens without one. | ||
| - Opening the popover moves focus into it and closing returns focus to the trigger. | ||
| - The resolved trigger receives `aria-expanded`, a durable `aria-controls` relationship wired through the element-reference IDL (so it works across shadow boundaries), and `aria-haspopup="dialog"`. For an SWC component trigger with an open shadow root, the inner `<button>` receives the ARIA. | ||
|
|
Contributor
There was a problem hiding this comment.
Can you create Keyboard navigation section with list similar to the one in Tabs?
Contributor
Author
There was a problem hiding this comment.
Done. Added a #### Keyboard navigation list (Tab/Shift+Tab, Enter/Space, Escape) under Accessibility, matching the Tabs pattern.
5t3ph
reviewed
Jul 8, 2026
5t3ph
left a comment
5t3ph
left a comment
Contributor
There was a problem hiding this comment.
Exciting to see this go in! Mostly documentation suggestions.
| @@ -0,0 +1,496 @@ | |||
| /** | |||
Contributor
There was a problem hiding this comment.
Play functions are needed to show the popover for purposes of VRT
Contributor
Author
There was a problem hiding this comment.
Yes, I was hoping the VRT PR would come in soon, which means I'd need to refactor this, but for this pass I'll figure this out!
- align Placement and Events prose to Tooltip's structure/wording - consolidate the overlapping Trigger element / Custom anchor sections into one Trigger element section; rename CustomAnchor story to TriggerElement - regroup dismissal under 'Focus trapping and dismissal'; add Keyboard navigation - correct focus-on-open/close behavior (surface focus; conditional restore) and the Tab dismissal claim; note the surface is a role=dialog in both modes - drop menu/card slot wording; remove duplicate States heading; add scroll-region tabindex best practice; trim the accessible-label JSDoc - css: reset user-agent margins on first/last slotted children (Anatomy slots a plain paragraph); a11y snapshot expects a paragraph node - migration-guide: use the shared yellow callout for the built-in-component note
rubencarvalho
added
Status:Ready for re-review
…s stories Chromatic snapshots every story in the file. The docs stories render the popover closed (just a trigger), so disable their snapshots at the meta level and add hidden (global !autodocs/!dev) VRT-only stories that open a single popover so the surface, tip orientation, sizes, arrowless, and modal states are captured. These stay out of the docs/sidebar and are not used by the a11y spec.
Opens the outer and inner popovers together so Chromatic captures the stacked nested state (the inner forms an ancestor chain, so the outer stays open beneath it). Hidden from docs/sidebar and separate from the a11y nested fixture.
5t3ph
approved these changes
Jul 10, 2026
rubencarvalho
enabled auto-merge (squash)
July 13, 2026 20:51
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.
Description
Adds the 2nd-gen
<swc-popover>and completes the 1st-gen → 2nd-gen Popover migration (Epic SWC-1993). One component, two modes, plus the shared overlay primitives it needs.popover="auto"): light-dismiss on Escape or outside click; the page stays interactive and scrollable.modal):<dialog>.showModal()with a focus trap, inert page, scroll lock, androle="dialog".What's included
core/components/popover(lifecycle, positioning, dismissal, ARIA) andswc/components/popover(render, styles, element registration).for/triggerElement,open,modal,placement(logical:top/bottom/start/endand their variants),size,hide-arrow,offset,cross-offset,should-flip,manual. Events:swc-open/swc-after-open/swc-close(withdetail.source:escape,outside, orprogrammatic) /swc-after-close. Trigger ARIA:aria-expanded,aria-controls, andaria-haspopup="dialog"in modal mode.PageScrollLockcontroller and theresolve-trigger,dismissible-stack,transition,deep-contains, andactual-placementutils, each with unit tests. Positioning uses the existingPlacementController.<sp-popover>+<sp-overlay>.Breaking changes
None for existing consumers; this is a net-new 2nd-gen component. Consumers upgrading from 1st-gen
<sp-popover>/<sp-overlay>should follow the migration guide (swc/components/popover/migration-guide.mdx).Related issue(s)
Accessibility testing checklist
aria-expanded; default mode dismisses on Escape and outside click; modal mode traps focus and closes on Escape (nativecancel); focus returns to the trigger on close. Covered by the Playwright a11y spec (trusted input) and play functions.role="dialog"in both modes with its accessible name fromaccessible-label; the trigger exposesaria-haspopup="dialog"(modal) and thearia-controlsrelationship. ARIA-tree snapshots assert the closed trigger and the open dialog in both modes.