feat(popover): lifecycle, positioning, a11y + styling (phase 4+5, WIP)#6357
Draft
rubencarvalho wants to merge 46 commits into
Draft
feat(popover): lifecycle, positioning, a11y + styling (phase 4+5, WIP)#6357rubencarvalho wants to merge 46 commits into
rubencarvalho wants to merge 46 commits into
Conversation
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).
|
5 tasks
Contributor
📚 Branch Preview Links🔍 First Generation 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 |
2 tasks
…1y-styling-swc-1993
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).
…r-a11y-styling-swc-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.
…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
- 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.
… into ruben/feat-popover-a11y-styling-swc-1993 # Conflicts: # 2nd-gen/packages/core/utils/dismissible-stack.ts # 2nd-gen/packages/core/utils/resolve-trigger.ts # 2nd-gen/packages/swc/components/popover/Popover.ts # 2nd-gen/packages/swc/components/popover/stories/popover.stories.ts # 2nd-gen/packages/swc/components/popover/test/popover.test.ts # CONTRIBUTOR-DOCS/03_project-planning/03_components/popover/migration-plan.md
- Re-enable the Playwright a11y suite (this branch implements the lifecycle, so the overview story mounts a real popover). - `hide-arrow` is the decided attribute name; drop the "under active discussion" / "name may change" notes from the migration plan (B7 row and the hide-arrow property row).
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.
The PlacementController computes coordinates with Floating UI strategy 'absolute', but the surface was 'position: fixed'. Applying absolute (document-relative) coordinates to a fixed element made the popover drift with page scroll instead of staying anchored. Align with the landed tooltip convention (position: absolute) so the strategy and CSS position match.
_dispatchAfter built its event as new CustomEvent(type, ...) from a parameter, so the manifest analyzer recorded a spurious 'type' event. Callers now build the CustomEvent with a string literal and pass it in, so the manifest records swc-after-open / swc-after-close and no 'type' row.
Demonstrate decoupling the toggle control from the positioning anchor: a button drives open while the popover anchors to a link via triggerElement, with manual set so the link's own click does not toggle.
…r-a11y-styling-swc-1993 Brings in origin/main (tooltip, button-group) and the API-branch change that drops the public actualPlacement property. Resolving the latter, the popover now adopts Tooltip's strategy: the computed placement is reflected as an internal `actual-placement` host attribute (set via setAttribute, removed on close) and styled via :host([actual-placement]), replacing the .swc-Popover--<placement> modifier classes. Adds an actual-placement test and the Storybook argTypes guard, and updates the migration plan B8 row accordingly.
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
force-pushed
the
ruben/feat-popover-a11y-styling-swc-1993
branch
from
6569866 to
b59a396
Compare
Move the docs page to a per-component popover.mdx (Phase 7 start) referencing the existing stories: Overview via DocsHeader and the Custom anchor behaviors story via Canvas. Anatomy / Options / States / Accessibility sections are rough scaffolds to be authored and polished later. Drop 'autodocs' from the Playground tags so the MDX page is the single Docs entry.
…1y-styling-swc-1993
…ben/feat-popover-a11y-styling-swc-1993 # Conflicts: # 2nd-gen/packages/core/utils/dismissible-stack.ts # 2nd-gen/packages/core/utils/resolve-trigger.ts # 2nd-gen/packages/swc/components/popover/Popover.ts # 2nd-gen/packages/swc/components/popover/popover.css # 2nd-gen/packages/swc/components/popover/stories/popover.stories.ts # 2nd-gen/packages/swc/components/popover/test/popover.a11y.spec.ts # 2nd-gen/packages/swc/components/popover/test/popover.test.ts # CONTRIBUTOR-DOCS/03_project-planning/02_workstreams/02_2nd-gen-component-migration/01_status.md # CONTRIBUTOR-DOCS/03_project-planning/03_components/popover/migration-plan.md
The popover had its dialog lifecycle, trigger/ARIA wiring, PlacementController integration, dismissal coordination, and event dispatch in the SWC concrete class — the inverse of the base-vs-concrete style guide and of Tooltip (whose base owns the behavior). Move all of it into PopoverBase; the concrete class now supplies only styles, the render template, and the internalElement/tipElement getter overrides the base reads (mirroring Tooltip's tipElement). Public API (attributes, events) is unchanged. Updates the migration plan's core/SWC split, which had prescribed the behavior-in-concrete layout.
rubencarvalho
force-pushed
the
ruben/feat-popover-a11y-styling-swc-1993
branch
from
c822e2b to
37365e6
Compare
transitionDuration is comma-separated when multiple properties transition (opacity, overlay, display). The previous parseFloat(...) === 0 check only read the first entry, so a zero first value with a non-zero later value (e.g. "0s, 0.2s") would dispatch the after-event early. Match Tooltip: split on comma and require every value to be 0s before dispatching immediately.
…ning teardown - Add a shared core/utils `hasActiveTransition(element)` helper that does the comma-split, every-zero transition-duration check in one tested place, so the logic is consistent across components (tooltip can adopt in a follow-up, like resolve-trigger / dismissible-stack). - Replace the event-aware `_dispatchAfter(event)` with a generic `_afterTransition(callback)` that runs after transitionend (or immediately when there is no transition). The after-open/after-close events and the `_stopPositioningWhenClosed()` teardown now live in the callers: teardown is an explicit peer of the after-close dispatch rather than being gated inside the helper by `event.type`. It stays tied to transition completion (tearing down mid-fade would snap the surface to 0,0).
…rationMs Use the new core/utils transition helpers in Tooltip's toggle handler so the comma-separated transition-duration parsing lives in one place across components. Adds maxTransitionDurationMs to the util for Tooltip's allow-discrete fallback timer, removing its inline parsing. Also tidies the popover teardown comment to describe the code rather than the change.
Gen1 (#5731 / SWC-1336) dismissed an auto overlay when clicking into its own content if it sat inside a focus region (an ancestor with tabindex=0). Add a regression test placing the popover inside such a region and asserting that clicking and focusing content inside the popover keeps it open — Gen2 relies on native popover=auto light-dismiss, which excludes the popover's own flat-tree (including slotted) content from 'outside', so the bug does not reproduce.
…comment The trigger-click guard is unchanged in behavior — a short time window is the robust, browser-agnostic way to read a light-dismiss + reopen as a single click gesture (native popovertarget correlation can't reach the shadow-DOM surface across roots). Extract the 200ms magic number to a named constant and rewrite the comment to explain the platform gap rather than narrate the mechanism.
The trigger-click reopen guard was stamped on every close, so an Escape or programmatic close followed by a quick trigger click was wrongly suppressed. Pass the close source through to _closeTeardown and stamp _lastDismissAt only when source is 'outside' — the only close a trigger click can have caused. Adds a regression test that a programmatic close still reopens on click.
The token-based arrow gap was a CSS margin keyed off actual-placement, but a margin only shifts the absolutely-positioned surface on its block-start / inline-start sides — so top and left placements got no gap (the surface sat flush against the trigger). Read the token-derived --_swc-popover-tip-height and add it to the PlacementController's main-axis offset instead, which is applied direction-aware on every side. Keeps the gap token-synced; removes the margin rules.
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
1 participant
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
Combined Phase 4 (a11y) + Phase 5 (styling) of the Popover migration (Epic SWC-1993), plus the runtime lifecycle that was deferred out of Phase 3. Combining them was deliberate: the component was invisible because the open/show lifecycle wasn't implemented (a
<div popover="auto">that never getsshowPopover()), and CSS alone can't reveal it — a11y, positioning, and CSS are all coupled through that lifecycle.What's implemented
open→showPopover()(default) /showModal()(modal), andhidePopover()/close(), with a_syncingOpenguard against setter↔listener loops. Eventsswc-open/swc-after-open/swc-close/swc-after-close(0-duration transition guard) viabeforetoggle(default) andcancel/close(modal). Modal backdrop-click viapointerdown+event.target === dialog.swc-close.detail.sourcereportsescape/outside/programmatic.PlacementControllerstart/stop anchored to the resolved trigger; tip wired through thearrowmiddleware. The surface isposition: absoluteto match the controller's Floating UIstrategy: 'absolute'— a mismatch (fixed+ absolute coordinates) previously made the popover drift on page scroll.actual-placementhost attribute (set viasetAttribute, removed on close) and styled via:host([actual-placement]). It is not a public property — the readonlyactualPlacementproperty was dropped during review on feat(popover): scaffold + API contract (phases 2+3) #6354 (a readonly property is still writable at runtime and could desync from the controller).dismissible-stack(LIFO) andresolve-trigger(for=/trigger-element+ open-shadow inner-<button>discovery).ariaControlsElements,aria-expanded,aria-haspopup="dialog"(modal);dismissibleStackEscape coordination. The Q4 accessibility-analysis amendment is included.::backdrop; tip orientation per physical side via:host([actual-placement]); forced-colors.open; popover anchors to a link viatriggerElementwithmanual).CustomEvents with string-literal types, so the CEM no longer records a spurioustypeevent.popover="auto"light-dismiss, which treats the popover's own flat-tree (including slotted) content as "inside," so clicking or focusing content no longer dismisses it — even when the popover sits inside an ancestor focus region (tabindex="0"). Covered by theFocusRegionContentClickTestregression test.Review changes carried in from #6354
This branch merges the API branch (#6354), so it includes the review responses there: the dropped public
actualPlacementproperty, and the migration-plan B8 / computed-placement updates describing theactual-placementattribute strategy.Remaining work (not exhaustive)
dismissible-stack/resolve-triggerunit tests. Current tests cover the API contract, render shape, and theactual-placementreflection.popover.mdxdocs page and full Anatomy / Options / States / Behaviors / Accessibility stories.@todoinpopover.css.swc-close.detail.sourceescape-vs-outside detection in default mode uses a keydown heuristic — revisit.Motivation and context
Phase 3 left the component API-complete but invisible (no lifecycle). This PR makes it functional and visible, lands the shared positioning/dismissal utilities, and brings the a11y and styling surfaces to a reviewable baseline for the remaining phases.
Related issue(s)
Notes
ruben/popover-migrationintegration branch (now up to date withmain). The API/Setup work appears in this diff until feat(popover): scaffold + API contract (phases 2+3) #6354 merges into the integration branch.Accessibility testing checklist
popover="auto"); no focus trap. Modal mode: native<dialog>focus trap + Escape (cancel); backdrop-click closes. Trigger receivesaria-expanded. Manual keyboard + screen-reader passes pending in Phase 6.ariaControlsElements) and expanded state; modal announcesrole="dialog"witharia-haspopup="dialog"on the trigger. Pending verification in Phase 6.