test(popover): phase 6 — behavior, util, and a11y coverage#6426
Open
rubencarvalho wants to merge 90 commits into
Open
test(popover): phase 6 — behavior, util, and a11y coverage#6426rubencarvalho wants to merge 90 commits into
rubencarvalho wants to merge 90 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.
Adds NestedLayersTest: a popover inside a popover, a tooltip on the inner trigger, and a standalone button + tooltip outside the popovers. The play function asserts the nesting invariant (opening the inner auto popover keeps the outer open via the native ancestor chain, resolved across the shadow/slot boundary). Tooltips are hover/focus driven and do not participate in the popover dismissible stack, so they layer independently. Native Escape/click light-dismiss needs trusted events, so dismissal is left to manual/visual verification.
Modal mode restores focus natively via <dialog>, but the popover API drops focus to <body> when focused content inside an open popover="auto" is hidden. On close, if focus is within the popover's content, move it back to the trigger. Gated on focus actually being inside (composed ancestor walk from the deepest focused element, so focus in a slotted custom element's shadow is detected) so an outside click that already moved focus elsewhere is left where the user put it. Adds a regression test for both cases.
Toggling `modal` while open re-shows the swapped internal element, but the re-show had three side effects: - Double positioning: `_wireTrigger` (run because `modal` changed) re-started the controller, then `_show` started it again. Re-anchoring on a trigger/positioning change now lives solely in `updated()` (which also covers `for`/`triggerElement` changes); `_wireTrigger` no longer repositions. - Duplicate `swc-open`: `_show` re-dispatched the open event for a popover that never closed. `_show(reShow)` now suppresses that one dispatch (modal: skip the manual dispatch; default: a one-shot guard consumed by the beforetoggle path). - Leaked Escape listener: switching default->modal left the default-mode document keydown listener attached. `_show`'s modal branch now removes it. Also dedupes `aria-expanded`: it is written by `updated()` on every `open` change and by `_wireTrigger` on wiring, so the redundant writes in `_show`/`_closeTeardown` are removed. Extends ModalToggleWhileOpenTest to assert swc-open fires once.
… outside the dialog box A modal backdrop click targets the `<dialog>` element, but so does a pointerdown on the dialog's own border (the surface has a border outside its padded content). The handler closed on any `target === dialog`, so pressing the 1px border dismissed the modal like a backdrop click. Confirm the pointer coordinates fall outside the dialog's bounding box before closing. Adds a border-pointerdown regression (stays open) alongside the backdrop case (closes).
…ontainment `_isFocusWithin` hand-rolled a composed-tree ancestor walk (crossing shadow boundaries via `ShadowRoot.host`). Extract it as `deepContains(ancestor, node)` in core/utils so the one tricky invariant (the host hop) lives in one place and can be reused (e.g. the tooltip follow-up). `_isFocusWithin` is now `deepContains(this, getActiveElement())`. Behavior unchanged; covered by FocusRestoreOnCloseTest (focus inside a slotted control).
…ariants Document two behaviors surfaced in review: - The surface/tip reveal depends on the PlacementController completing a compute (which sets actual-placement). The controller skips it on a 0x0 floating element, but the surface always has border + padded content, so it is never zero-sized while open. Note where to gate the reveal independently if that assumption ever changes. - Focus restoration runs from the close lifecycle (default via _onBeforeToggle, modal natively via <dialog>), covering Escape/outside/programmatic closes. Removing an open popover from the DOM is not covered: focus has already moved to <body> before disconnectedCallback runs.
…rule text-formatting.md forbids Jira ticket numbers in code comments. Remove the SWC-1336 references from the focus-region regression test comments; the GitHub issue reference (#5731) is kept. The behavior is described factually.
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).
|
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 |
2 tasks
rubencarvalho
changed the base branch from
ruben/feat-popover-a11y-styling-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)
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 6 (testing) of the Popover migration. Targets the
ruben/popover-migrationintegration branch; stacks on #6357 (Phase 4+5). Until #6357 merges, this diff also includes its commits, so review/merge in order: #6357 → #6426.What's in it
escape/outside/programmatic), dismissal coordination (nested-dismissible Escape, modalcancel), focus restoration, modal scroll-lock, sequential popovers (one light-dismisses the prior), failedshowPopover()/showModal()handling, base-class default getters. RaisesPopover.base.tsfrom ~90% to ~97% lines (funcs 100%).core/utilsregression tests —resolve-trigger,dismissible-stack,transition,deep-contains(excluded from the coverage metric, but now directly guarded).aria-expanded,aria-haspopup="dialog"(modal), and the element-referencearia-controlsrelationship.Notes
toMatchAriaSnapshotbaselines need the a11y runner (storybook server) to lock; trigger/dialog ARIA is covered by play functions and the test-runner axe scan in the meantime.Related issue(s)
Accessibility testing checklist
aria-expandedtoggles, modal Escape (cancel) deferral when not topmost, default/programmatic close-source labeling, focus returns to the trigger on close. Full manual pass tracked in the docs/review phase.aria-haspopup="dialog"(modal) and thearia-controlsrelationship are asserted; dialog accessible name fromaccessible-label. Manual SR verification pending.