feat(popover): scaffold + API contract (phases 2+3) by rubencarvalho · Pull Request #6354 · adobe/spectrum-web-components

rubencarvalho · 2026-06-01T08:59:16Z

Description

Foundation for the Popover migration (Epic SWC-1993): Phase 2 (Setup) + Phase 3 (API contract). No runtime behavior — the lifecycle, positioning, ARIA wiring, and styling land in #6357.

Phase 2 — Setup

core: PopoverBase (property surface), Popover.types, and the resolve-trigger / dismissible-stack util stubs wired into utils/index + package.json.
swc: Popover render stub (branches <div popover="auto"> vs <dialog> on modal), popover.css stub, swc-popover registration, and empty-but-valid stories + tests.

Phase 3 — API contract (aligned with React Spectrum S2 `Popover`)

Public surface: placement, size (s/m/l), offset (default 8), cross-offset, should-flip, hide-arrow (arrow shown by default), plus the web-component-specific open, modal, for, trigger-element, manual, and the readonly actualPlacement.

types: Placement (re-exported from the PlacementController), POPOVER_VALID_PLACEMENTS, POPOVER_VALID_SIZES, PopoverSize, PopoverCloseSource, PopoverCloseEventDetail.
base: full property surface; container-padding / tip-padding marked @internal (positioning details, excluded from the public API table); dev-only placement validation via the VALID_PLACEMENTS static.
swc: @fires for swc-open / swc-after-open / swc-close / swc-after-close (surfaced in the CEM; @internal props excluded).

Related issue(s)

Epic SWC-1993

Notes

Targets the ruben/popover-migration integration branch. Behavior is in feat(popover): lifecycle, positioning, a11y + styling (phase 4+5) #6357; the changeset is added once at the integration→main boundary (feat(popover): gen2 popover migration #6356), not per phase.

Accessibility testing checklist

Keyboard — no behavior in these phases (scaffolding + contract); keyboard handling is implemented in feat(popover): lifecycle, positioning, a11y + styling (phase 4+5) #6357 / verified in Phase 6.
Screen reader — event and type contract defined here; ARIA wiring is implemented in feat(popover): lifecycle, positioning, a11y + styling (phase 4+5) #6357 and verified in Phase 6.

Set up the core and SWC structure for the <swc-popover> migration: - core: PopoverBase property-surface stub, Popover.types (re-exports Placement from the placement-controller), plus resolve-trigger and dismissible-stack util stubs wired into utils/index and package.json - swc: Popover render stub branching <div popover="auto"> (non-modal) vs <dialog> (modal), popover.css stub, swc-popover registration, and empty-but-valid stories + test scaffolds (3/3 vitest storybook pass) - plan: record div/dialog render decision, mark container-padding, should-flip and tip-padding @internal, arrow positioning in v1 scope, and check off the Setup phase; update workstream status table

@fires

Define the typed public API contract for <swc-popover> (contract only; runtime lifecycle deferred to phases 4-5): - types: add PopoverCloseSource docs and PopoverCloseEventDetail for the swc-close event - base: runtime placement validation in update() via the VALID_PLACEMENTS static (dev-only window.__swc.warn, supports the proxy pattern); fix @internal JSDoc format on container-padding / should-flip / tip-padding - swc: @fires JSDoc for swc-open / swc-after-open / swc-close / swc-after-close (verified surfaced in the CEM; @internal props excluded) - plan: check off API contract items, note lifecycle deferral; status table

changeset-bot · 2026-06-01T08:59:25Z

⚠️ No Changeset found

Latest commit: 1718f78

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2026-06-01T09:05:16Z

📚 Branch Preview Links

🔍 First Generation Visual Regression Test Results

When 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: pr-6354

If the changes are expected, update the current_golden_images_cache hash in the circleci config to accept the new images. Instructions are included in that file.
If the changes are unexpected, you can investigate the cause of the differences and update the code accordingly.

…c-1993

Bring the foundation PR's public surface up to the final S2-aligned contract so reviewers see what actually ships: add `size` and `manual`, replace `tip` with `hide-arrow` (arrow shown by default), default `offset` to 8, and make `should-flip` public. `container-padding` / `tip-padding` stay @internal. Types gain PopoverSize and PopoverCloseEventDetail. Runtime behavior still lands in the styling/a11y PR.

Rajdeepc

Some of my findings:

tip → hide-arrow rename is undocumented in the migration plan API table (B7 breaking change needs updating)
offset default is 8 in code but 0 in the plan. This needs reconciliation
size and manual are new properties not yet in the plan API table
Stub utilities (dismissible-stack, resolve-trigger) are exported but non-functional
actualPlacement is writable by consumers despite being documented as readonly

Rajdeepc · 2026-06-03T11:58:10Z

+   * Readonly; not reflected as an attribute.
+   */
+  @property({ attribute: false })
+  public actualPlacement: Placement | null = null;


The migration plan says this is readonly updated by the controller via onPlacementChange callback, but @property + public means consumers can freely write to it:

popover.actualPlacement = 'top'; // silently desyncs from controller

Would it be ok to add a @readonly JSDoc so that the CEM communicates the intent.

For consideration - tooltip ended up setting this via regular attribute methods and not exposing here. Maybe @readonly accomplishes the same thing, but it felt like it should be more protected. I may have had the wrong train of thought, though!

Rajdeepc · 2026-06-03T11:58:10Z

+   * @default false
+   */
+  @property({ type: Boolean, reflect: true, attribute: 'hide-arrow' })
+  public hideArrow = false;


The migration plan API table still documents tip as a boolean (opt-in, default false). Your code implements hideArrow with attribute hide-arrow. You need to document this as breaking change and update the API table.

For consideration: Tooltip was tentatively planning a no-tip property (additive, not yet implemented). Either way, it would be good to have consistent naming.

I have an active thread with Nadeen on this, because design calls it "tip" and React an arrow. Trying to figure out which way to go. We called "tip" on Gen1 too.

Rajdeepc · 2026-06-03T11:58:10Z

+   * @default 8
+   */
+  @property({ type: Number })
+  public offset = 8;


The migration plan API table says:

offset | number | 0 | ... we default to 0 to make the controller-host contract neutral

But the code defaults to 8. If 8 is correct kindly document it.

You will likely have to take a cue from Tooltip as far as styling is concerned where I adjusted it to use the offset via a custom property in the CSS with a token fallback, that way the distance stays in sync and the CSS didn't add on top.

Rajdeepc · 2026-06-03T11:58:10Z

+   * @default true
+   */
+  @property({ type: Boolean, reflect: true, attribute: 'should-flip' })
+  public shouldFlip = true;


The migration plan marks should-flip as Internal (@internal) but the JSDoc here has no @internal tag, unlike containerPadding and tipPadding which do.
Without the tag, the CEM will surface shouldFlip in the public API table.

I recommend keeping this public - the flipping behavior is rather aggressive and may be unwanted in cases such as long menus or pickers (unless we will be blocking page scroll while open).

Yes, 1) I tried aligning with React here and 2) Steph's comment regarding aggressive flipping. I also have an active thread with Nadeen regarding this "aggressive flipping" concern.

Rajdeepc · 2026-06-03T11:58:10Z

+   * (`s` → 336px, `m` → 416px, `l` → 576px); when unset, it fits its contents.
+   */
+  @property({ type: String, reflect: true })
+  public size?: PopoverSize;


Kindly add size as is not documented in the migration plan API table

Rajdeepc · 2026-06-03T11:58:10Z

+ * Resolve the trigger for a popover-like host and discover the AT-facing inner
+ * button across shadow boundaries.
+ *
+ * @todo Phase 3 (API): implement `for=` resolution via


@todo Phase 3 (API): implement for= resolution...

This PR is Phase 3. The todo should reference Phase 4/5 (lifecycle implementation).

Also: the function returns { trigger: null, interactiveElement: null } unconditionally. Any code importing and calling it will get a null trigger with no warning. Since the types and interface are published to core/utils/index.ts, this could be consumed by a parallel work.

Rajdeepc · 2026-06-03T11:58:10Z

+
+    // The render shape branches on `modal`: a `<div popover="auto">` in the
+    // default (non-modal) mode, a `<dialog>`(.showModal()) in modal mode.
+    return this.modal


If modal changes at runtime while the popover is open, Lit will swap the entire internal element (<div> ↔ <dialog>) and will destroy the existing top-layer session (showPopover() / showModal() state
and all attached event listeners (beforetoggle, cancel, etc.)
Phase 4/5 should guard against this: if open is true when modal changes, you should close the current element first, let Lit re-render, then re-open with the new lifecycle. A dev-mode warning would also help.

@todo

…se ref - registerDismissible/unregisterDismissible were no-op stubs exported from core/utils; any parallel work importing them would silently get broken stack coordination - add duplicate-guard push and lastIndexOf-based removal so the stack actually tracks open dismissibles - correct @todo in resolve-trigger from Phase 3 to Phase 4/5 since this PR is Phase 3

@readonly

… guard - mark actualPlacement as @readonly so CEM and docs communicate the contract that consumers should never write to it - add @todo Phase 4/5 comment in Popover.ts documenting the need to guard against runtime modal toggles while the popover is open

- replace tip row with hide-arrow documenting the inverted semantics and breaking change; note active naming discussion with design - update offset default from 0 to 8 to match Popover.base.ts - add size and manual properties to the API table - promote should-flip from @internal to public per reviewer feedback on aggressive flipping behavior - document runtime modal toggle edge case and close/reopen guard

- property defaults: verify all 11 public properties match the documented API contract (open, modal, placement, size, offset, etc.) - default mode render shape: assert div[popover=auto] with content wrapper and arrow tip - modal mode render shape: assert dialog without popover attribute - hide-arrow: confirm tip suppression when hide-arrow is set - property reflection: placement, modal, size, and should-flip reflect to attributes after mutation - dev-mode warnings: invalid placement triggers window.__swc.warn; valid placement does not

# Conflicts: # 2nd-gen/packages/core/controllers/placement-controller/src/placement-controller.ts # 2nd-gen/packages/core/controllers/placement-controller/src/types.ts # 2nd-gen/packages/core/controllers/placement-controller/stories/demo-hosts.ts # 2nd-gen/packages/core/controllers/placement-controller/stories/placement-controller.stories.ts # 2nd-gen/packages/core/controllers/placement-controller/test/placement-controller.test.ts # 2nd-gen/packages/core/package.json # 2nd-gen/packages/swc/.storybook/DocumentTemplate.mdx # 2nd-gen/packages/swc/.storybook/main.ts # 2nd-gen/packages/swc/.storybook/preview.ts

Aligns the shared PlacementController with the swc-2017/tooltip branch's unlanded refinements, which the popover (top-layer) surface also depends on: - Use Floating UI strategy 'absolute' instead of 'fixed' for correct top-layer element placement. - stop() no longer removes inline styles (translate/top/left and the --swc-placement-available-* custom properties); cleanup is left to the caller so exit transitions can complete before the properties are removed. - Update the multi-controller test to assert on actualPlacement only, since stop() no longer clears the available-space custom property.

… into ruben/feat-popover-api-swc-1993

…ben/feat-popover-api-swc-1993 # Conflicts: # 2nd-gen/packages/core/controllers/placement-controller/src/placement-controller.ts

CEM event tags name a custom event (e.g. `swc-open`), not a JS namepath, so the hyphen tripped jsdoc/valid-types. Add a structuredTags setting so `fires` and `event` names are parsed as free text with no bracketed type, clearing the warnings repo-wide for popover (and tooltip) without altering the JSDoc.

…abel - Rename the section separator to "PLAYGROUND STORY" per the updated stories-format rule. - Drop the redundant per-story render functions that just re-called the meta render; use args directly so the default render applies. - Remove the now-unused lit `html` import.

With no open/close lifecycle yet, the overview story renders an unopened top-layer surface with no meaningful accessibility tree to assert against, so the Playwright a11y spec fails. Mark the suite `.skip` with a note to re-enable once `showPopover()`/`showModal()`, ARIA wiring, and modal semantics land in Phase 4/5.

Rajdeepc

Approving since most of the concerns are addressed.
Few nits

You can add unit tests for dismissible-stack.ts (register/unregister/isTop)
It would be great if you can document closest('[dir]') shadow boundary limitation in PlacementController JSDoc.

5t3ph

Looks good! Just some suggestions to consider for the later phases.

5t3ph · 2026-06-16T15:08:00Z

+   * Readonly; not reflected as an attribute.
+   */
+  @property({ attribute: false })
+  public actualPlacement: Placement | null = null;


For consideration - tooltip ended up setting this via regular attribute methods and not exposing here. Maybe @readonly accomplishes the same thing, but it felt like it should be more protected. I may have had the wrong train of thought, though!

5t3ph · 2026-06-16T15:09:16Z

+   * @default 8
+   */
+  @property({ type: Number })
+  public offset = 8;


You will likely have to take a cue from Tooltip as far as styling is concerned where I adjusted it to use the offset via a custom property in the CSS with a token fallback, that way the distance stays in sync and the CSS didn't add on top.

5t3ph · 2026-06-16T18:17:55Z

+
+const { events, args, argTypes, template } = getStorybookHelpers('swc-popover');
+
+/**


[nit at this stage] reminder to switch to MDX for final docs

5t3ph · 2026-06-16T18:21:06Z

+
+/* @todo Phase 5 (styling): port S2 source from spectrum-css and add placement modifiers. */
+
+* {


[FYI] You'll probably want to make the agent aware of tooltip, and copy most if not all of the :host styles onto .swc-Popover to correctly place and transition the element, plus take its cues to resolve some of the other positioning styles.

5t3ph · 2026-06-16T18:22:54Z

@@ -69,12 +69,12 @@



[nit] consider adding a decision log to capture the update to change from a div[popover] for both modal and non-modal to instead the swap to dialog

Address Steph's review on PR #6354: - Remove the public readonly `actualPlacement` property from PopoverBase. Mirroring Tooltip, the computed placement is internal state that drives the `.swc-Popover--<placement>` modifier classes; it is not exposed as a property or attribute. A public readonly property is still writable at runtime and risked desyncing the component from the controller. - Drop the corresponding default-value assertion from the popover test. - Update the migration plan: B8 row, public API table/intro, the computed placement section, and add a Decision log capturing this change and the div[popover] -> dialog swap for modal mode.

Address Steph's review on PR #6354: - Remove the public readonly `actualPlacement` property from PopoverBase. A readonly property is still writable at runtime and risked desyncing the component from the controller. The computed placement stays internal and drives the `.swc-Popover--<placement>` modifier classes for styling. - Drop the corresponding default-value assertion from the popover test. - Update the migration plan (B8 row, public API table/intro, computed placement section) to reflect the removal. Note that Tooltip reaches the same "no writable property" outcome by reflecting an `actual-placement` host attribute, whereas the popover styles its internal surface element with modifier classes.

Per Steph's review (#6354), mirror Tooltip: the arrow's clearance is now a token-based margin (popover-tip-height) on the trigger-facing side of the surface, keyed off the actual-placement host attribute, instead of a hard-coded ARROW_SPACE constant added to the controller offset. The consumer's offset is passed through unchanged, keeping a single source of truth in CSS. Also carries the no-arrow rename through Popover.ts/popover.css and reconciles the migration plan (B7/B8, API table, S2/S5, computed-placement section, checklists).

Per Steph's review (#6354), mirror Tooltip: the arrow's clearance is now a token-based margin (popover-tip-height) on the trigger-facing side of the surface, keyed off the actual-placement host attribute, instead of a hard-coded ARROW_SPACE constant added to the controller offset. The consumer's offset is passed through unchanged, keeping a single source of truth in CSS. Adds an actual-placement reflection test and reconciles the migration plan (B8, API table, S2/S5, computed-placement section, checklists) with the actual-placement attribute mechanism. The hide-arrow attribute name is unchanged.

rubencarvalho added 2 commits June 1, 2026 10:34

rubencarvalho mentioned this pull request Jun 1, 2026

feat(popover): gen2 popover migration #6356

Draft

5 tasks

rubencarvalho changed the title ~~feat(popover): API contract (phase 3)~~ feat(popover): scaffold + API contract (phases 2+3) Jun 1, 2026

rubencarvalho mentioned this pull request Jun 1, 2026

feat(popover): scaffold 2nd-gen component (phase 2 setup) #6353

Closed

2 tasks

rubencarvalho added 2 commits June 2, 2026 13:52

Merge branch 'ruben/popover-migration' into ruben/feat-popover-api-sw…

e2213fe

…c-1993

rubencarvalho marked this pull request as ready for review June 2, 2026 14:42

rubencarvalho requested a review from a team as a code owner June 2, 2026 14:42

rubencarvalho added ready-for-review Status:Ready for review PR ready for review or re-review. and removed ready-for-review labels Jun 2, 2026

Rajdeepc reviewed Jun 3, 2026

View reviewed changes

Rajdeepc self-assigned this Jun 3, 2026

Rajdeep Chandra and others added 8 commits June 10, 2026 17:18

Merge remote-tracking branch 'origin/ruben/feat-popover-api-swc-1993'…

9455762

… into ruben/feat-popover-api-swc-1993

Merge remote-tracking branch 'origin/ruben/popover-migration' into ru…

c9231b7

…ben/feat-popover-api-swc-1993 # Conflicts: # 2nd-gen/packages/core/controllers/placement-controller/src/placement-controller.ts

rubencarvalho requested review from 5t3ph and Rajdeepc June 15, 2026 21:05

rubencarvalho added 3 commits June 15, 2026 22:05

Rajdeepc approved these changes Jun 16, 2026

View reviewed changes

5t3ph approved these changes Jun 16, 2026

View reviewed changes

rubencarvalho force-pushed the ruben/feat-popover-api-swc-1993 branch from 4127ce7 to 1718f78 Compare June 17, 2026 08:59

rubencarvalho merged commit bcf04d8 into ruben/popover-migration Jun 17, 2026
17 of 26 checks passed

rubencarvalho deleted the ruben/feat-popover-api-swc-1993 branch June 17, 2026 09:09

rubencarvalho mentioned this pull request Jun 17, 2026

feat(popover): lifecycle, positioning, a11y + styling (phase 4+5) #6357

Open

2 tasks


		const { events, args, argTypes, template } = getStorybookHelpers('swc-popover');

		/**


		/* @todo Phase 5 (styling): port S2 source from spectrum-css and add placement modifiers. */

		* {

Uh oh!

Conversation

rubencarvalho commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Phase 2 — Setup

Phase 3 — API contract (aligned with React Spectrum S2 Popover)

Related issue(s)

Notes

Accessibility testing checklist

Uh oh!

changeset-bot Bot commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

github-actions Bot commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📚 Branch Preview Links

🔍 First Generation Visual Regression Test Results

Uh oh!

Rajdeepc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rajdeepc Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rajdeepc Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rajdeepc Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rajdeepc Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rajdeepc Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rajdeepc Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rajdeepc Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rajdeepc left a comment

Choose a reason for hiding this comment

Uh oh!

5t3ph left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rubencarvalho commented Jun 1, 2026 •

edited

Loading

Phase 3 — API contract (aligned with React Spectrum S2 `Popover`)

changeset-bot Bot commented Jun 1, 2026 •

edited

Loading

github-actions Bot commented Jun 1, 2026 •

edited

Loading

Rajdeepc left a comment •

edited

Loading

Rajdeepc Jun 3, 2026 •

edited

Loading

Rajdeepc Jun 3, 2026 •

edited

Loading

Rajdeepc Jun 3, 2026 •

edited

Loading

Rajdeepc Jun 3, 2026 •

edited

Loading

Rajdeepc Jun 3, 2026 •

edited

Loading

Rajdeepc Jun 3, 2026 •

edited

Loading

Rajdeepc Jun 3, 2026 •

edited

Loading