Skip to content

test(BottomSheet): name the failure when a mouse release does not ask the sheet to close - #10823

Merged
cixzhang merged 5 commits into
mainfrom
test/bottomsheet-reduced-motion-mouse-close
Nov 9, 2026
Merged

cixzhang merged 5 commits into
mainfrom
test/bottomsheet-reduced-motion-mouse-close

Conversation

@cixzhang

@cixzhang cixzhang commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to #10756 (co-authored by astracat-bot), which made the reduced-motion mouse close use timestamped CDP events.

Maintainer impact

When the reduced-motion mouse close fails, the test now says why. Before, a release read as still (its projection short of the dismiss line) surfaced only as a generic toBeHidden timeout: "Expected hidden, Received visible".

Change

  • New assertion: right after the drag's frame check, before toBeHidden, the test asserts that the release asked the sheet to close. The sheet renders aria-modal only while open, so it drops the attribute when the release requests the close. The check polls the DOM (dialog[open]'s aria-modal must become null; closed or no longer modal both count), not a role locator, because a closing dialog leaves the accessibility tree. If it doesn't, the test fails with "the release did not ask the sheet to close: the drag read as a still release (N ms from the last move to the lift), so its projection stayed short of the dismiss line".
  • The gap: it comes from the events' own timeStamp, since CDP delivers them back to back. upAt stays on performance.now() for the frame filter.
  • Unchanged: every timeout, and every other assertion.

Evidence

Red-first: with a temporary 400 ms hold before the lift (the velocity tracker's stillness is 40 ms, so a still release), a local Chromium run failed at the new assertion with its named message, and passed without the hold. Under load, a role-locator version of the check failed a finished close (4/24 at 4 busy loops, 22/24 at 8); the DOM poll passed 24/24 at 0, 4 and 8 busy loops, and the 400 ms hold still failed by name. The hold isn't in this change. The timestamped drag from #10756 passed 24/24 at the CI accessibility profile.

Scope

  • Test-only; no product behavior, API or visual change.
  • No weakened assertions, longer waits or retries; one assertion added.
  • Public text contains no private operational context.

Co-authored-by: astracat-bot[bot] <323722387+astracat-bot[bot]@users.noreply.github.com>
@cixzhang cixzhang added the ci:run-a11y Run the accessibility suite on this PR; write access, next push (AST-030 FR12) label Nov 9, 2026
@vercel

vercel Bot commented Nov 9, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
astryx Ready Ready Preview Nov 9, 2026 6:22pm UTC

Request Review

@astracat-bot

astracat-bot Bot commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

AI review status for this pull request.

Review status Updated
🟢 Reviewed (for maintainers only) Nov 9, 2026, 6:33 PM UTC

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Nov 9, 2026
@github-actions

github-actions Bot commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

PR Analysis Report

🔎 Preview · 3077f8a

Storybook · Sandbox · built from this PR's head

Modified components

Component Size LoC Visual · A11y · RTL
BottomSheet 68.3 kB (0) · 7.7 kB CSS (0) — ✗ · ✓ · —
BottomSheetSwitcher 17.9 kB (0) · 3.4 kB CSS (0) — ✗ · ✓ · —
Visual changes: 2 snapshots in 1 component

A change is a question for the reviewer, not a failure.

Component · story Before After Diff
BottomSheet · Text Only (neutral, dark) before frame after frame diff frame
BottomSheet · Text Only (neutral, light) before frame after frame diff frame

Package size: measured; main has no measurement of the merge base to compare yet.

API changes: cli +22 · ~7 · −0 (against the merge base's committed report); theme-butter +1 · ~0 · −0 (against the merge base's committed report); theme-y2k +1 · ~0 · −0 (against the merge base's committed report)

Committed API report behind this build: cli 29 rows, core 36 rows, theme-butter 1 row, theme-y2k 1 row; run pnpm api:report.

31 API changes (advisory)
Package Where Signature Usage
+ cli ./api · Functions · function exportSearchIndex function exportSearchIndex @experimental: (options?: { cwd?: string | undefined; package?: string | undefined }) => Promise<SearchIndex> import {exportSearchIndex} from '@astryxdesign/cli/api'; exportSearchIndex(…)
+ cli ./api · Functions · function rankSearchIndex function rankSearchIndex @experimental: (query: string, indexes: SearchIndex | SearchIndex[], options?: { type?: SearchDomain | undefined; limit?: number | undefined }) => Promise<SearchResponse> import {rankSearchIndex} from '@astryxdesign/cli/api'; rankSearchIndex(…)
+ cli ./api · Types · type DocsListChild type DocsListChild = DocsNodeChild & { topic: string; description: string; children?: DocsListChild[]; childCount?: number } import type {DocsListChild} from '@astryxdesign/cli/api'
+ cli ./api · Types · type DocsListNamespace childCount?: number | undefined import type {DocsListNamespace} from '@astryxdesign/cli/api'; const x: DocsListNamespace = {childCount: …}
+ cli ./api · Types · type DocsListNamespace children?: DocsListChild[] | undefined import type {DocsListNamespace} from '@astryxdesign/cli/api'; const x: DocsListNamespace = {children: …}
+ cli ./api · Types · type SearchIndex type SearchIndex import type {SearchIndex} from '@astryxdesign/cli/api'
+ cli ./api · Types · type SearchResultEntry deprecated?: ComponentDeprecation | undefined import type {SearchResultEntry} from '@astryxdesign/cli/api'; const x: SearchResultEntry = {deprecated: …}
+ cli ./authoring · Types · interface ComponentDeprecation interface ComponentDeprecation import type {ComponentDeprecation} from '@astryxdesign/cli/authoring'
+ cli ./authoring · Types · interface ComponentIconSlotDoc interface ComponentIconSlotDoc import type {ComponentIconSlotDoc} from '@astryxdesign/cli/authoring'
+ cli ./authoring · Types · interface ComponentIconSlotStateDoc interface ComponentIconSlotStateDoc import type {ComponentIconSlotStateDoc} from '@astryxdesign/cli/authoring'
+ cli ./authoring · Types · interface MultiComponentDoc deprecated?: ComponentDeprecation import type {MultiComponentDoc} from '@astryxdesign/cli/authoring'; const x: MultiComponentDoc = {deprecated: …}
+ cli ./authoring · Types · interface SingleComponentDoc deprecated?: ComponentDeprecation import type {SingleComponentDoc} from '@astryxdesign/cli/authoring'; const x: SingleComponentDoc = {deprecated: …}
+ cli ./doc · Types · interface ComponentBaseDoc deprecated?: ComponentDeprecation import type {ComponentBaseDoc} from '@astryxdesign/cli/doc'; const x: ComponentBaseDoc = {deprecated: …}
+ cli ./doc · Types · interface SubComponentDoc deprecated?: ComponentDeprecation import type {SubComponentDoc} from '@astryxdesign/cli/doc'; const x: SubComponentDoc = {deprecated: …}
+ cli ./doc · Also exported from an earlier entry point · ComponentDeprecation ComponentDeprecation (./authoring) import {ComponentDeprecation} from '@astryxdesign/cli/doc'
+ cli ./doc · Also exported from an earlier entry point · ComponentIconSlotDoc ComponentIconSlotDoc (./authoring) import {ComponentIconSlotDoc} from '@astryxdesign/cli/doc'
+ cli ./doc · Also exported from an earlier entry point · ComponentIconSlotStateDoc ComponentIconSlotStateDoc (./authoring) import {ComponentIconSlotStateDoc} from '@astryxdesign/cli/doc'
+ cli ./json · Also exported from an earlier entry point · DocsListChild DocsListChild (./api) import {DocsListChild} from '@astryxdesign/cli/json'
+ cli ./json · Also exported from an earlier entry point · SearchIndex SearchIndex (./api) import {SearchIndex} from '@astryxdesign/cli/json'
+ cli ./template · Also exported from an earlier entry point · ComponentDeprecation ComponentDeprecation (./authoring) import {ComponentDeprecation} from '@astryxdesign/cli/template'
+ cli ./template · Also exported from an earlier entry point · ComponentIconSlotDoc ComponentIconSlotDoc (./authoring) import {ComponentIconSlotDoc} from '@astryxdesign/cli/template'
+ cli ./template · Also exported from an earlier entry point · ComponentIconSlotStateDoc ComponentIconSlotStateDoc (./authoring) import {ComponentIconSlotStateDoc} from '@astryxdesign/cli/template'
~ cli ./api · Functions · function search function search: (query: string, options?: { cwd?: string | undefined; type?: "template" | "component" | "hook" | "theme" | "doc" | undefined; limit?: number | undefined }) => Promise<SearchResponse> → function search: (query: string, options?: { cwd?: string | undefined; type?: "template" | "component" | "doc" | "theme" | "hook" | undefined; limit?: number | undefined }) => Promise<SearchResponse> import {search} from '@astryxdesign/cli/api'; search(…)
~ cli ./api · Types · type SwizzleCopyResponse data: { component: string; package: string; outputDir: string; filesCopied: number; files: string[]; usesStyleX: boolean; feedback?: SwizzleFeedback | undefined } → data: { component: string; package: string; outputDir: string; filesCopied: number; files: string[]; usesStyleX: boolean; dependencies: Record<string, string>; marker … #84d9505b import type {SwizzleCopyResponse} from '@astryxdesign/cli/api'; const x: SwizzleCopyResponse = {data: …}
~ cli ./authoring · Types · interface MultiComponentDoc theming?: { container?: boolean; targets: ComponentThemingTarget[]; vars?: ComponentThemingVar[]; derived?: ComponentThemingDerivedVar[] } → theming?: { container?: boolean; targets: ComponentThemingTarget[]; vars?: ComponentThemingVar[]; derived?: ComponentThemingDerivedVar[]; componentIcons?: ComponentIconSlotDoc[] } import type {MultiComponentDoc} from '@astryxdesign/cli/authoring'; const x: MultiComponentDoc = {theming: …}
~ cli ./authoring · Types · interface SingleComponentDoc theming?: { container?: boolean; targets: ComponentThemingTarget[]; vars?: ComponentThemingVar[]; derived?: ComponentThemingDerivedVar[] } → theming?: { container?: boolean; targets: ComponentThemingTarget[]; vars?: ComponentThemingVar[]; derived?: ComponentThemingDerivedVar[]; componentIcons?: ComponentIconSlotDoc[] } import type {SingleComponentDoc} from '@astryxdesign/cli/authoring'; const x: SingleComponentDoc = {theming: …}
~ cli ./authoring · Types · type TemplateCategory type TemplateCategory = | 'Dashboard - Analytics' | 'Dashboard - Comparison' | 'Dashboard - KPI Summary' | 'Dashboard - Monitoring' | 'Dashboard - Executive Summary' | 'Dashboard - Sco … #e7c18378 → type TemplateCategory = | 'Dashboard - Analytics' | 'Dashboard - Comparison' | 'Dashboard - KPI Summary' | 'Dashboard - Monitoring' | 'Dashboard - Executive Summary' | 'Dashboard - Sco … #a8be998f import type {TemplateCategory} from '@astryxdesign/cli/authoring'
~ cli ./doc · Types · interface ComponentBaseDoc theming?: { container?: boolean; targets: ComponentThemingTarget[]; vars?: ComponentThemingVar[]; derived?: ComponentThemingDerivedVar[] } → theming?: { container?: boolean; targets: ComponentThemingTarget[]; vars?: ComponentThemingVar[]; derived?: ComponentThemingDerivedVar[]; componentIcons?: ComponentIconSlotDoc[] } import type {ComponentBaseDoc} from '@astryxdesign/cli/doc'; const x: ComponentBaseDoc = {theming: …}
~ cli ./doc · Types · interface SubComponentDoc theming?: { container?: boolean; targets: ComponentThemingTarget[]; vars?: ComponentThemingVar[]; derived?: ComponentThemingDerivedVar[] } → theming?: { container?: boolean; targets: ComponentThemingTarget[]; vars?: ComponentThemingVar[]; derived?: ComponentThemingDerivedVar[]; componentIcons?: ComponentIconSlotDoc[] } import type {SubComponentDoc} from '@astryxdesign/cli/doc'; const x: SubComponentDoc = {theming: …}
+ theme-butter Public attributes · attr data-astryx-pressable attr data-astryx-pressable (public data attribute) [data-astryx-pressable] { … }
+ theme-y2k Public attributes · attr data-astryx-pressable attr data-astryx-pressable (public data attribute) [data-astryx-pressable] { … }

Touched components checked; add ci:run-visual or ci:run-rtl to run the full suites.


Generated by PR Enrichment workflow | View full report

…close (red check: 400 ms hold before the lift; reverted next)
@vjeux

vjeux commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

Two things at f69674a, from local runs (Chromium, main 42081ef):

  1. As committed, the test fails 3 of 3. mouseDrag(cdp, sheet, 150, 50, 400) still has the 400 ms rest before the lift, so the release reads no speed. With mouseDrag(cdp, sheet, 150, 50) it passes 3 of 3.
  2. The gap in the new message comes from performance.now() in the listeners, which is dispatch time. The release speed comes from the stamped event.timeStamp. After the 400 ms stamped rest, the message said "3 ms from the last move to the lift". Recording event.timeStamp in both listeners reports the gap that the release saw.

Supporting numbers, in a 4-CPU container with 4 workers (the accessibility shards' profile), 24 runs each:

Load main The same stamped drag (from #10756)
8 busy loops 10 fail 24 pass
4 busy loops 9 fail 24 pass
No added load 24 pass

Each main failure was a run whose lift came more than 40 ms after the last move, as in CI.

@astracat-bot astracat-bot Bot 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.

Requesting changes: the timestamped drag is the right fix for the flake, but this head still carries the red-check hold, so the targeted test fails.

  • mouseDrag(cdp, sheet, 150, 50, 400) (line 337) stamps the lift 400 ms after the last move, past the velocity tracker's 40 ms stillness interval, so the release reads as still and the test fails 3 of 3 locally; drop the 400 (with mouseDrag(cdp, sheet, 150, 50) it passed 20 of 20).
  • The new failure message takes its gap from performance.now() in the two listeners (lines 309-322, 380), which is dispatch time, so after the 400 ms stamped pause it reports "3 ms"; record event.timeStamp for the gap (keeping upAt for the frame filter at line 390) so the message reports the gap the release actually saw.

After the revert, please update the description: it still says every assertion is unchanged, while this head adds the aria-modal assertion before toBeHidden. CI was skipped on this draft head, so these results come from local Chromium runs.

[Automated review]

… the sheet to close

Merges main, which now carries the timestamped drag. The red-check hold is gone; the gap comes from the events' own times.

Co-authored-by: astracat-bot[bot] <323722387+astracat-bot[bot]@users.noreply.github.com>
@cixzhang cixzhang changed the title test(BottomSheet): timestamp the reduced-motion mouse close test(BottomSheet): name the failure when a mouse release does not ask the sheet to close Nov 9, 2026
@cixzhang
cixzhang marked this pull request as ready for review November 9, 2026 17:32
@vjeux

vjeux commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

Thanks: at e311e9d both points are fixed. The test passes 3 of 3, and with a stamped 400 ms rest the message reads "(401 ms from the last move to the lift)".

One new gap. Under load, the aria-modal check fails on runs where the close worked. dialog is getByRole('dialog', {name: 'Filters'}). Once the close starts, the sheet goes inert and the dialog closes, so the role locator finds nothing. A loaded runner finishes the close before the check runs. The check then fails with "element(s) not found", under the "did not ask the sheet to close" message.

A fix: poll the open dialog's aria-modal, which is null for a closing or closed sheet.

await expect
  .poll(
    async () =>
      page.evaluate(
        () =>
          document
            .querySelector('dialog[open]')
            ?.getAttribute('aria-modal') ?? null,
      ),
    {message: `the release did not ask the sheet to close: …`, timeout: 2_000},
  )
  .toBeNull();

Chromium in a 4-CPU container with 4 workers (the accessibility shards' profile), 24 runs each:

Load main a0f2a2d e311e9d e311e9d with the fix above
No added load not run 24 pass 24 pass
4 busy loops not run 4 fail 24 pass
8 busy loops 24 pass 22 fail 24 pass

With the fix, the 400 ms rest still fails by name, 3 of 3, with "(401 ms from the last move to the lift)".

@astracat-bot astracat-bot Bot 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.

Requesting changes: the red-check hold is gone and the gap now reads the events' own times, but the new aria-modal check can fail a sheet that closed correctly.

  • Miss: the negated aria-modal check (lines 385-388) passes only while the closing dialog is still open, which ends 216-234 ms after the lift while the check first runs at about 166 ms, so a driver stall of 60 ms or more fails a correct close with "element(s) not found" and a message that blames a still release at a 1 ms gap; record the aria-modal drop in the page with a MutationObserver and poll that record instead (with that change a 100 ms stall passes 5 of 5 and the 400 ms hold still fails with the named "401 ms" message).

The two earlier points, the 400 ms hold and the dispatch-time gap, are fixed. These results come from local Chromium runs; CI's accessibility contracts passed the test once at this head.

[Automated review]

@cixzhang
cixzhang marked this pull request as draft November 9, 2026 17:50
@cixzhang
cixzhang marked this pull request as ready for review November 9, 2026 17:51

@astracat-bot astracat-bot Bot 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.

Requesting changes: the DOM poll fixes the stalled-runner false failure and still names a still release, but it breaks the required lint check.

  • The expect.poll callback at line 389 returns page.evaluate(…) without async, so lint fails with @typescript-eslint/promise-function-async ("Functions that return promises must be async"); write async () => there.

Non-blocking: the message names a still release even at a 1 ms gap (a 40 px drag that does not project fails the same way), so consider naming that cause only when the gap passes the 40 ms stillness interval. The earlier points (the 400 ms hold, the dispatch-time gap and the role-locator check) are fixed.

[Automated review]

@cixzhang
cixzhang marked this pull request as draft November 9, 2026 18:13
@cixzhang
cixzhang marked this pull request as ready for review November 9, 2026 18:14

@astracat-bot astracat-bot Bot 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.

Approved pending CI: CI has not run on this head yet. The CI run for this push was refused admission because CI was full, so its required checks ended without running their suites; re-run CI so they report on this head.

The async poll callback clears the lint error, and the failure message now names a still release only past the tracker's 40 ms stillness interval, reporting the gap alone below it.

Non-blocking:

  • The comment at lines 385-386 says a role locator finds nothing once the close has started, but a closing sheet stays in the accessibility tree until its dialog closes; consider "the dialog leaves the tree when it closes, and a slow runner can reach that before this check."
  • The description still quotes the failure message as always naming a still release; below 40 ms it now reports the gap alone.

[Automated review]

Comment on lines +385 to +386
// Read the DOM, not the accessibility tree: a closing dialog leaves the
// tree, so a role locator finds nothing once the close has started.

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.

Non-blocking: a closing sheet that holds focus stays neither inert nor aria-hidden until the close settles (BottomSheet.tsx:932-941), so the dialog leaves the accessibility tree when it closes, not when the close starts. Suggested wording:

  // Read the DOM, not the accessibility tree: the dialog leaves the tree
  // when it closes, and a slow runner can reach that before this check.

@cixzhang
cixzhang merged commit cb0973b into main Nov 9, 2026
109 of 126 checks passed
@cixzhang
cixzhang deleted the test/bottomsheet-reduced-motion-mouse-close branch November 9, 2026 19:38

This branch was successfully deployed

1 active deployment
Preview — 3077f8a7 Deployed Nov 9, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:run-a11y Run the accessibility suite on this PR; write access, next push (AST-030 FR12) CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants