Skip to content

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

Open
rubencarvalho wants to merge 116 commits into
ruben/popover-migrationfrom
ruben/feat-popover-a11y-styling-swc-1993
Open

feat(popover): lifecycle, positioning, a11y + styling (phase 4+5)#6357
rubencarvalho wants to merge 116 commits into
ruben/popover-migrationfrom
ruben/feat-popover-a11y-styling-swc-1993

Conversation

@rubencarvalho

@rubencarvalho rubencarvalho commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Description

Phase 4 (a11y) + Phase 5 (styling) of the Popover migration, plus the open/show lifecycle deferred from Phase 3 (without it the component was invisible). Hardened through review. Scoped to Popover; later-phase items tracked below.

You can play with it here: https://swcpreviews.z13.web.core.windows.net/pr-6357/docs/second-gen-storybook/?path=/story/components-popover--playground&args=open:!true;actual-placement:bottom

What's in it

  • Lifecycleopen drives showPopover() (default <div popover="auto">) / showModal() (modal <dialog>); swc-open / -after-open / -close / -after-close events; swc-close.detail.source is escape / outside / programmatic.
  • PositioningPlacementController anchored to the resolved trigger, tip via the arrow middleware; surface is position: absolute to match the controller (fixes scroll drift). Flip-resolved side reflected as an internal actual-placement attribute (CSS-only, not a public property).
  • a11yaria-controls via the element-reference IDL, aria-expanded, modal aria-haspopup="dialog" + native focus trap; Escape coordinated through a dismissible stack; focus returns to the trigger on close; modal page scroll-lock.
  • CSS — S2 surface + tip, modal ::backdrop (no dimming), forced-colors.
  • Shared core/utilsresolve-trigger, dismissible-stack, transition, deep-contains (checked: no existing equivalent to reuse).
  • Fixes [Bug]: Overlay in focus region dismissed when click into its content #5731 (overlay in a focus region dismissed on content click) via native light-dismiss, plus broad tests (lifecycle, modal, focus, scroll-lock, nested layering, placement).

Later-phase work

  • Changeset (publish step); Phase 6 test depth + utils unit tests + screen-reader passes; Phase 7 docs MDX.
  • Tooltip popover="manual" follow-up so a tooltip doesn't light-dismiss an open popover (Tooltip is reverted here to keep this PR Popover-only).
  • Tip pixel/forced-colors polish; VRT.

Related

Notes

Accessibility testing checklist

  • Keyboard — default: Escape + click-outside dismiss, no trap. Modal: native focus trap + Escape (cancel), backdrop-click closes, focus returns to the trigger. Full manual pass in Phase 6.
  • Screen reader — trigger announces the controls relationship + expanded state; modal announces role="dialog". Pending 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
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
…hase 4+5)

Combine a11y + styling and pull in the deferred lifecycle so the component
is visible and functional (work in progress; polish remains):

- utils: real dismissible-stack (LIFO) and resolve-trigger (for= /
  trigger-element + open-shadow inner-button discovery)
- lifecycle: open -> showPopover()/showModal() and hidePopover()/close()
  with a syncing guard; swc-open/after-open/close/after-close dispatch
  (0-duration transition guard) via beforetoggle (default) and cancel/close
  (modal); modal backdrop-click; swc-close.detail.source detection
- positioning: lazy PlacementController start/stop, tip arrow wiring,
  actualPlacement + reactive .swc-Popover--<placement> class
- a11y: durable ariaControlsElements, aria-expanded, aria-haspopup (modal);
  dismissibleStack Escape coordination; Q4 a11y-analysis amendment
- css: S2 surface + UA-chrome resets, content padding, modal ::backdrop,
  tip orientation per side group, forced-colors
- stories: trigger + anchored open popover with tip
- plan/status: completion notes; status table through Render & Style

Deferred polish (todo in popover.css): pixel-accurate tip geometry vs Figma
and RTL logical-placement tip parity (SWC-917).
@changeset-bot

changeset-bot Bot commented Jun 1, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 0059788

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

github-actions Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

📚 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-6357

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.

rubencarvalho and others added 23 commits June 1, 2026 11:51
The component now opens/closes on trigger click instead of being purely
property-driven:

- attach a click listener to the resolved trigger that toggles open; a
  #lastDismissAt guard prevents the native light-dismiss from immediately
  reopening when clicking an open popover's trigger
- new 'manual' property (reflected, mirrors tooltip) suppresses the auto
  wiring so Picker/Menu drive open themselves; ARIA wiring still applies
- listener lifecycle managed on trigger/manual change and disconnect
- Playground story starts closed with a Toggle button to demo the interaction
- ClickToggleTest verifies open-on-click and close-on-click (aria-expanded)
- plan: Invocation pattern rewritten, manual added to the property table and
  TL;DR additions, completion note updated
Remove end-user-facing references to internal consumers (Picker/Menu) and
to the popover API; `open` is the public surface for controlling visibility.
Applies to the manual property JSDoc and the migration plan.
- public API mirrors the S2 Popover: add `size` (s/m/l), `offset` default 8,
  `should-flip` public, and replace `tip` with `hide-arrow` (arrow shown by
  default). `container-padding`/`tip-padding` stay @internal.
- styling: fixed size widths, viewport max-width, and a 200ms opacity fade via
  @starting-style (tokenized duration + easing) with a reduced-motion guard.
- fix: defer PlacementController teardown until the close transition ends so
  the arrow no longer snaps to the edge during the close fade.
- convention: convert #private members to the private _underscore form.
- remove code comments that referenced React; tokenize transition timings.
- plan: property table, internal-set note, B7, and architecture/checklist
  references updated to the S2-aligned surface.
The arrow is a 45deg-rotated square, whose visible width is side × √2. It
was sized at the full tip width, rendering ~√2 too wide. Size the square at
tip-width ÷ √2 so the visible arrow is popover-tip-width (16px) wide by
popover-tip-height (8px) tall, and derive the edge insets from the same size.
Replace the single-layer drop-shadow with the elevated token's three
layers (ambient, transition, key). Arrow shown (default): filter drop-shadow
so the arrow casts a matching shadow; hide-arrow: box-shadow form of
`--swc-drop-shadow-elevated`, which composites cleanly with nested popovers.
Drive the Storybook controls from Popover.VALID_PLACEMENTS (22 values) and
Popover.VALID_SIZES (s/m/l, plus undefined to fit contents) instead of the
default text inputs.
Swap the native <button> trigger for <swc-button> in the Playground and
Overview stories, exercising the cross-shadow inner-<button> discovery in
resolveTrigger (ARIA wiring lands on the button inside swc-button's shadow root).
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.
…r-a11y-styling-swc-1993

# Conflicts:
#	2nd-gen/packages/swc/components/popover/Popover.ts
…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
… 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.
…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.
…o available height

Expose the content padding as a public custom property (documented via @cssprop
so it lands in the API table), since the popover is a blank-canvas container.
Bound the content wrapper to the PlacementController's
--swc-placement-available-height and scroll there (not on the surface, whose
overflow: visible lets the arrow render outside the box) so a tall popover stays
within the viewport without clipping the tip.
…iptions

Drop the "blank-canvas container / for content that needs a different inset"
rationale from the @cssprop API description and the CSS comment; state what the
property does and its default.
Add public --swc-popover-background-color, --swc-popover-border-color, and
--swc-popover-corner-radius (wrapping the surface tokens), each documented via
@cssprop for the API table. The arrow reuses background/border-color so theming
the surface also themes the tip. Joins --swc-popover-content-padding as the
popover's public theming surface.
POPOVER_VALID_PLACEMENTS now lists the 12 placements in the design API table:
each physical side (top/right/bottom/left) plus its two alignments. Drops the
logical start/end variants the controller can resolve but design does not
specify (and whose arrow rendering was incorrect). actual-placement still
reduces each to its physical side for tip orientation. Updates the Placement
docs prose to match.
…close

Mirror the tooltip's entry/exit motion: the surface starts offset toward the
trigger (distance from the spacing-75 token) and slides to rest. The offset uses
transform (which composes with the PlacementController's translate positioning)
and is keyed on the reflected placement attribute, known immediately, since
actual-placement is set only after async positioning. @starting-style is not
usable here for that same reason, so the slide rides the actual-placement reveal
gate alongside the opacity fade. Reduced-motion disables it via the existing
transition: none.
The earlier forced-colors border fix also swapped the tip inset side, so a
right-placed popover put its arrow on the right edge pointing away from the
trigger. The arrow now sits on the edge nearest the trigger: left placement ->
right edge, right placement -> left edge, with the border-removals kept.
…crolls the page

Root cause of the modal open scroll-shift: the surface was reset to inset: auto
(the document's 0,0 origin) immediately, so showModal()'s native focus landed
off-screen and the browser scrolled to reveal it. Instead keep the browser's
native top-layer centering (inset: 0; margin: auto, which centers in the
viewport) until the PlacementController anchors the surface; :host([actual-placement])
then switches to inset: auto; margin: 0 with the controller's inline translate.
The surface now opens in view, so showModal() focus does not scroll. Removes the
pre-showModal seed translate and the scroll-restore workaround entirely.

@5t3ph 5t3ph left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check the transforms once the build is working again.

Comment on lines 47 to 48
max-inline-size: var(--swc-placement-available-width, calc(100vw - 24px));
max-block-size: none;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use min() to have the browser consider these as dynamic options ("smallest" wins to retain some visual margin).

max-inline-size: min(var(--swc-placement-available-width), calc(100vi - 24px));
max-block-size: min(var(--swc-placement-available-height), 90vb);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Comment on lines +186 to +187
background-color: var(--swc-popover-background-color, token("background-layer-2-color"));
border: token("border-width-100") solid var(--swc-popover-border-color, token("popover-border-color"));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can simply use inheritance since these props are initially applied on the wrapper:

background-color: inherit;
border: inherit;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

*/
export const POPOVER_VALID_PLACEMENTS =
ALL_PLACEMENTS satisfies readonly Placement[];
export const POPOVER_VALID_PLACEMENTS = [

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm not interpreting this correctly, but this seems to be excluding logical placements which doesn't address the real issue of the tip alignment for something like bottom-left (arrow aligned to the left corner) vs bottom (arrow aligned in the center)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was a bad commit - it is fixed now. WDYM about th arrow alignment? From what I can see, it is working as expected in the latest commit :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at React's a bit more, their behavior matches. I was expecting the arrow to also shift, so that for example in a "bottom start" position the arrow would mimic with "top start", accounting for any cross offset as well. That is also technically what design expects, but we can follow-up on it in a different ticket, it doesn't need to be blocking.

@rubencarvalho rubencarvalho Jun 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that makes sense, but we'll need to jump into a call because I'm not sure I'm looking at the right thing 😛

On touch, the popover light-dismiss can fire off touchstart, before the
trigger's pointerdown listener runs, so the gesture window had not opened and the
close was not attributed to the press — the trailing click then reopened the
popover (tapping the trigger did not close it). Bind the press-start handler to
both pointerdown and touchstart (capture) so the window opens before the dismiss
on touch as well as mouse. No timing involved.
…the page shift

showModal() moves focus into the dialog while it is still at its 0,0 origin
(positioning is async), and has no preventScroll option, so the page scrolls a
few pixels to reveal it on open. Capture window scroll before showModal() and
restore it immediately after; the focus scroll is synchronous, so the restore is
seamless. Replaces the in-viewport seed translate, which only reduced the shift.

@nikkimk nikkimk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed in Slack, let me know when you want me to re-test for a11y.

@rubencarvalho rubencarvalho added Status:Addressing feedback PR owner is addressing review comments and will change label back to "Ready for review" when ready. and removed Status:Ready for review PR ready for review or re-review. labels Jun 23, 2026
The initial render calls _hide() for the default open=false, which set
_closeSource to 'programmatic' unconditionally. Nothing was open to hide,
so no beforetoggle fired to clear it, and the first real light-dismiss
read its source as non-'outside' and skipped arming the trigger-click
reopen guard, re-opening on the first close. Set the close source only
inside the branch that actually hides an open element.

Also stop _onTriggerPressStart from clearing _dismissedByTriggerPress: on
touch both pointerdown and touchstart fire for one press, and the second
must not reset a dismissal the light-dismiss already recorded.
_onTriggerClick clears it once per gesture.
Trim verbose inline comments across the popover base, SWC class, and
stylesheet without losing the non-obvious reasoning. Consolidate the
reopen-guard explanation that was fully restated in three places down to a
single canonical note on the press-start handler, fix a stale comment that
claimed the modal page-scroll lock was deferred (PageScrollLockController
implements it), and replace an em dash in a code comment.
Replace the meta/instructional slot content (Playground's "enable modal,
then press Tab" note, "Above the trigger.", "Small — fixed 336px width.",
etc.) with realistic popover content: a two-factor prompt, an autosave note,
a sync status, a glossary definition for the custom anchor, and an add-comment
panel for the virtual anchor. Give every story an accessible-label so each
models a properly named dialog instead of dev-warning.
Make trigger buttons state their action ("Open popover") instead of themed
labels that hid whether the button opens a popover; the realistic content
stays inside the popover. Variant stories keep their explicit labels (placement
sides, sizes, "Open modal"). Set the overview account example to size s with
12px content padding via --swc-popover-content-padding, matching the react
popover's content inset.
Keep the component content padding at its default token and add the account
card's extra 12px inset through a wrapper div in the example instead of
overriding --swc-popover-content-padding.
Narrow POPOVER_VALID_PLACEMENTS to the logical 12-value set (top/bottom plus
the logical start/end inline sides, each with two alignments) instead of the
physical left/right set. Logical sides resolve against the writing direction
via the PlacementController (start is the left in LTR, the right in RTL), so
placements stay correct under RTL. The controller already converts logical to
physical and reports the resolved physical side, so actual-placement and tip
orientation are unchanged.

Add direction-aware entry-slide rules for start/end via :dir(), reordered
after the size/hide-arrow rules to keep specificity ascending. Update the
Placement story (Start/End), the docs placement prose, and reconcile the
migration plan (the earlier 22-value/physical entries) with this session
decision.
- Cap the surface inline size and the content block size with min() against
  the placement-aware custom properties and the logical viewport (100vi/90vb)
  so both constraints apply, not just the fallback (5t3ph).
- Let the arrow tip inherit the surface background and border instead of
  re-referencing the --swc-popover-* props; the surface already resolves them
  and the orientation rules drop the inward edges (5t3ph).
- Drop the Playground content's fixed inline size and start it at size s (like
  React) so the content fills the surface at any size instead of looking narrow
  when the size control changes (5t3ph).
@rubencarvalho rubencarvalho added Status:Ready for review PR ready for review or re-review. Status:Ready for re-review PR has had its feedback addressed and is once again ready for review. and removed Status:Addressing feedback PR owner is addressing review comments and will change label back to "Ready for review" when ready. Status:Ready for review PR ready for review or re-review. labels Jun 25, 2026

@5t3ph 5t3ph left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, this item can be a ticket to investigate later if you prefer.

// the surface is still at its 0,0 origin when focus lands. Capture and
// restore the scroll position around the call; it is synchronous, so the
// restore is seamless (no paint between).
const { scrollX, scrollY } = window;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commenting here since scroll related - edge case of opening a modal when the trigger and possibly part or all of the modal is out of view. May seem odd but I can see this happening to keyboard users or in multi-pane apps where scroll is already modified.

Higher risk potential: you can also get it to happen when a trigger opens a modal on an anchor somewhere else.

Image

Scroll blocking prevents moving it into view. Should we do any sort of edge collision detection and scroll into view only in that case?

@rubencarvalho rubencarvalho force-pushed the ruben/feat-popover-a11y-styling-swc-1993 branch from 23d9d5e to 0059788 Compare June 28, 2026 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component:Popover Status:Ready for re-review PR has had its feedback addressed and is once again ready for review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants