docs(popover): phase 7 — per-component MDX + section stories#6427
Open
rubencarvalho wants to merge 98 commits into
Open
docs(popover): phase 7 — per-component MDX + section stories#6427rubencarvalho wants to merge 98 commits into
rubencarvalho wants to merge 98 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).
…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.
The inline id snippet wrapped a template literal in a single-backtick code span
using escaped backticks. Markdown does not honor backslash-escaped backticks
inside a code span, so the span closed early and ${++PopoverIdCounter} leaked out
as an MDX JSX expression in the generated contributor-docs .mdx, failing eslint
(no-undef on PopoverIdCounter) under lint:2nd-gen. Reword the checklist item to
describe the counter in prose instead of an inline template literal.
… anchor - _show now enters the top layer first and only registers the dismissible-stack entry, the Escape listener, and positioning once showPopover()/showModal() actually succeeds. A thrown show no longer leaves the component wired up for a popover that never opened. - _startPositioning, when open with no resolvable anchor (e.g. `for` changed to a missing id), now stops the controller and clears actual-placement instead of returning early and leaving the surface anchored to stale geometry. The gate hides it until a valid anchor returns. Adds UnresolvedTriggerWhileOpenTest.
…roll-lock Add `overscroll-behavior: contain` to the popover surface so scrolling an internal scroll region does not chain out to the page (most relevant in modal mode) — the CSS-doable part of the planned scroll behavior. The page-level scroll lock for modal mode (`overflow: hidden` on `html`) cannot be done from the component's shadow stylesheet, which cannot reach `html`; it needs JS to toggle `documentElement` overflow on open/close and is deferred. Correct the migration plan (S7 and the lifecycle step) which described it as CSS-only.
CSS overscroll-behavior only prevents scroll chaining, not page scroll behind the modal, and a shadow stylesheet cannot reach `html`. Lock scroll in JS: on modal show set `overflow: hidden` on documentElement with scrollbar-width padding compensation (no layout shift), saving the prior inline values; restore them on close, on toggle to non-modal while open, and on disconnect. Default mode never locks. Adds ModalScrollLockTest; updates migration plan S7 to mark it implemented.
… modal scroll lock Keep the modal scroll lock minimal: toggle `overflow: hidden` on documentElement and restore the prior inline value. Removes the scrollbar-width padding compensation and its saved field. Updates the migration plan accordingly.
Replace the eight chained changedProperties.has(...) checks with a named POSITIONING_PROPERTIES list iterated by .some(). Same behavior; the set of re-anchor-triggering properties is now declared in one place.
…nd failed-show paths Phase 6 coverage push for the component. New play-function tests: - unresolved `for` warns (DEBUG) - modal pointerdown on content (not the backdrop) does not close - nested-dismissible modal Escape is deferred when not topmost (cancel prevented) - default-mode Escape and programmatic close label swc-close.detail.source - modal Escape (topmost) labels source escape - sequential popovers: opening one light-dismisses the prior - base-class default rendering getters (tipElement null, arrowHeight 0) - failed showPopover()/showModal() leave no half-wired state Raises Popover.base.ts from 90% to ~97% lines (funcs 100%). Remaining uncovered lines are defensive guards (no-element, double-lock, double-add) and timing-dependent reopen paths, intentionally not contrived per the testing mindset.
Add direct regression tests for the shared core/utils introduced with Popover (they are excluded from the coverage metric but guard the shared primitives): - deep-contains: self / light-DOM / across-shadow containment, non-descendant, null - transition: hasActiveTransition, maxTransitionDurationMs (s/ms, multi-prop), runAfterTransition (sync when none, settles once, ignores descendant bubbling) - resolve-trigger: for= resolution, triggerElement override, unresolved → null, inner-<button> discovery across an open shadow root - dismissible-stack: LIFO, unregister exposes next, re-register de-dupes, empty Also adds TriggerAriaTest: modal aria-haspopup=dialog, the element-reference aria-controls relationship, and aria-haspopup removal in default mode.
…tion plan Util suites (resolve-trigger, dismissible-stack, plus deep-contains and transition) and the component behavior suite are landed; the Playwright ARIA snapshot remains deferred with a note (needs the a11y runner to lock baselines).
…e 7) Flesh out popover.mdx with prose + Canvas references for Anatomy, Options (placement, sizes, hide-arrow), States, Behaviors (custom anchor, modal, default-mode dismissal, focus/events), and Accessibility (features, modal naming, best practices); remove the WIP banner; add an Upcoming features note. Add interactive trigger-based section stories (Anatomy, Placement, Sizes, HideArrow, States, Modal, Accessibility): a top-layer popover can't be shown open inline and open auto popovers light-dismiss one another, so each example is a trigger the reader clicks to open. Updates the meta description (rendered in the docs header). No public --swc-* custom properties are exposed, so no @cssprop tags are needed; @slot and @fires are already declared on Popover.ts.
Per-component MDX (Anatomy/Options/States/Behaviors/Accessibility) and the section stories are authored; JSDoc/@slot/@fires are in place and no public --swc-* props exist (no @cssprop needed). Breaking-change items remain for the consumer migration guide.
|
rubencarvalho
added
ready-for-review
Status:Ready for review
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 |
…ies done - Q7 (open questions): the modal scroll lock is implemented in JS (overflow:hidden on documentElement, restored on close) plus CSS overscroll-behavior; corrected the stale 'CSS-only / no JS-level scroll lock' wording to match the implementation and the S7 / lifecycle-step entries. - Status table: mark the popover 'Add Stories' column done (all 7 steps complete).
rubencarvalho
changed the base branch from
ruben/test-popover-swc-1993
to
ruben/popover-migration
…FadeGateTest The opacity checks read the in-flight transition value: during a rapid close-then-reopen the close fade is still mid-flight, so getComputedStyle reports a non-zero opacity (e.g. '1') rather than the gated '0', failing intermittently in CI. The gate's deterministic contract is the absence of `actual-placement` (the stylesheet keys the fade on it), which the test already asserts; the computed opacity is just the CSS consequence and is removed.
…st-popover-swc-1993
…uthiness)
- registration: assert the element resolves to the Popover class, not just truthy
- render shape: assert the internal element / content / tip are the exact
HTMLElement subclasses (div / dialog / span) instead of toBeTruthy
- anchored translate: assert a pixel translate via toMatch(/\dpx/) instead of
not.toBe('')
- invalid-placement warning: assert exactly one warning (toBe(1)) instead of >0
…st-popover-swc-1993
- unresolved-for and nameless-modal warnings assert exactly one warning (toBe(1)) instead of >0 - TriggerAriaTest asserts ariaControlsElements equals [popover] exactly (toEqual) instead of includes(...).toBe(true)
…over Covers the breaking changes deferred from the per-component MDX: the sp-overlay + sp-popover composition collapses to a single swc-popover wired with for/open; type="modal" maps to the modal attribute; the tip attribute is replaced by the default-on arrow plus hide-arrow; sp-opened/sp-closed become swc-after-open/swc-after-close; and --mod-popover-* overrides have no public replacement.
…ts own content swc-popover applies content padding on its internal content wrapper, so the 1st-gen sp-dialog wrapper used purely for padding is no longer needed. Reframe the future swc-dialog note as structured dialog chrome rather than padding.
…to a trigger Drop the 'surface for menus, dialogs, and contextual content' framing; those are first-party patterns consumers should not rebuild on the bare popover. Align the meta JSDoc and subtitle with the React Spectrum framing.
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
Phase 7 (documentation) of the Popover migration. Targets the
ruben/popover-migrationintegration branch; stacks on #6426 / #6357. Until those merge, this diff also includes their commits, so review/merge in order: #6357 → #6426 → #6427.What's in it
popover.mdx— authored the per-component docs page: Anatomy, Options (placement, sizes, hide-arrow), States (open), Behaviors (custom anchor, modal, default-mode dismissal, focus/events), Accessibility (features, modal naming, best practices), and an Upcoming features note. WIP banner removed; meta description updated.Anatomy,Placement,Sizes,HideArrow,States,Modal,Accessibility, each section-tagged and referenced by a<Canvas>in the MDX.Notes
popover="auto"surfaces light-dismiss one another, so rows of simultaneously-open popovers aren't possible; Overview keeps one open example and the prose carries placement/size/mode detail.--swc-*custom properties are exposed, so no@cssproptags are needed;@slotand@firesare already onPopover.ts, and the public@propertyJSDoc lives on the base class.Related issue(s)
Accessibility testing checklist
aria-expanded/aria-haspopup="dialog"(modal) /aria-controls; modalrole="dialog"named byaccessible-label. Documented; manual SR verification pending.