Skip to content

test(BottomSheet): find a non-modal sheet as an open manual popover - #8647

Merged
cixzhang merged 1 commit into
meta:mainfrom
vjeux:uikit/c08-sheet-nonmodal-escape-spec
Oct 28, 2026
Merged

cixzhang merged 1 commit into
meta:mainfrom
vjeux:uikit/c08-sheet-nonmodal-escape-spec

Conversation

@vjeux

@vjeux vjeux commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Why

One of the 27 tests in BottomSheet's keyboard browser suite, "non-modal: Escape closes the nested Popover before the sheet", has failed on main in Chromium and WebKit since sheets started stacking by open order (#7648), so the suite is red on main and on every pull request that runs it.

What led to it

Found while running BottomSheet's browser tests for the keyboard work (part of bringing Astryx's touch behavior to parity with iOS's UIKit, text entry first). The test finds an open sheet with dialog[open]. Since #7648, a non-modal sheet opens as a popover, as component:BottomSheet FR11 specifies, and a popover doesn't carry the open attribute. So the test counted zero open sheets, even though the sheet was open and Escape still worked.

Options and tradeoffs

Why this choice

The last one keeps what the test is for (one Escape closes the nested Popover, the next closes the sheet) and only fixes how it finds the sheet. No product code changes.

How to check it

  1. On main, run the BottomSheet browser tests in Chromium or WebKit. Expected: the non-modal case fails, finding 0 open sheets.
  2. With this change, run them again. Expected: they pass, and the case's two Escape presses behave as described.

Evidence

  • The test's locator becomes dialog[open], dialog:popover-open, with a comment citing FR11.
  • With the change, the file's 27 tests pass in Chromium (twice) and the BottomSheet browser suite passes in WebKit (34 tests).

Since open-order stacking landed, a non-modal sheet (`hasScrim={false}`) is
presented in the top layer as a manual popover (component:BottomSheet FR11),
which carries no `open` attribute. The nested-Popover Escape case looked for
an open sheet as `dialog[open]`, so its non-modal run found none and failed in
Chromium and WebKit before reaching the Escape presses it checks.

The locator now matches an open dialog or an open popover. Escape behaves as
the case asserts: the first press closes the nested Popover and keeps the
sheet, the second closes the sheet. The modal and required cases are
unchanged.
@vercel

vercel Bot commented Oct 27, 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 Oct 27, 2026 3:51am UTC

Request Review

@astracat-bot

astracat-bot Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

AI review status for this pull request.

Review status Updated
🟢 Reviewed (for maintainers only) Oct 27, 2026, 4:06 AM UTC

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Oct 27, 2026

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Recommended for approval. The new locator matches how component:BottomSheet FR11 hosts a non-modal sheet: as a manual popover, which carries no open attribute. The non-modal Escape case therefore passes again without any product change, and its final assertion now really observes the sheet closing. Nothing more is needed from you; because this spec is release-only, the release check is where the fix will show.

[Automated review]

@github-actions

github-actions Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

PR Analysis Report

📚 Storybook Preview

View Storybook for this PR

🧪 Sandbox Preview

View Sandbox for this PR

Component Size

Component Import before Import after Delta CSS before CSS after Delta
core/BottomSheet 34.0KB 34.0KB 0B 7.9KB 7.9KB 0B
core/BottomSheetSwitcher 14.8KB 14.8KB 0B 3.3KB 3.3KB 0B

Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included.

Bundle Size Summary

Gzip figures measure each package's entry file, not the code an import pulls in.

Package Entry (ESM) Entry gzip before Entry gzip after Delta
@astryxdesign/core 4.9KB 1.2KB 1.2KB 0B
Package stylesheet Gzip before Gzip after Delta
@astryxdesign/core 34.6KB 34.6KB 0B

Accessibility Audit

Status: No accessibility violations detected.

Visual Regression

55 of 502 shot(s) changed.

A change here is a question, not a failure: check whether the after is the
picture you intended. Record that review on the PR. Baseline maintenance is an
explicit dispatch of CI; this report never rewrites the baseline or adds a release gate.

component story theme mode pixels
Selector Indicator Space Bottom Sheet Narrow End RTL neutral light 753,933
Selector Indicator Space Bottom Sheet Wide Start neutral light 740,072
RichTextEditor Markdown parity: side by side probe dark 558,840
RichTextEditor Markdown parity: side by side probe light 540,756
Chat Full AI Chat probe light 230,823
Chat Full AI Chat probe dark 229,805
AppShell With Mobile Nav probe light 217,010
AppShell With Mobile Nav probe dark 216,960
Collapsible Dividers probe dark 66,445
Collapsible Dividers probe light 66,445
AppShell Playground neutral dark 65,198
AppShell Playground neutral light 63,788
Chat Default neutral dark 36,304
Chat Default neutral light 35,767
Chat Empty State probe light 23,531
Chat Empty State probe dark 22,622
ChatAutoScroll Scroll Behavior Comparison neutral dark 15,378
ChatAutoScroll Scroll Behavior Comparison neutral light 14,107
Chat With Attachments probe light 13,406
Chat With Attachments probe dark 11,232

and 35 more.


Generated by PR Enrichment workflow | Storybook | Sandbox | View full report

@cixzhang

Copy link
Copy Markdown
Contributor

Posted by Cindy's agent on her behalf: closed and reopened only to restart this PR's CI on the current workflow after tonight's queue-wide cancel; nothing in the PR changed.

@cixzhang
cixzhang merged commit d27b545 into meta:main Oct 28, 2026
221 of 223 checks passed
@vjeux
vjeux deleted the uikit/c08-sheet-nonmodal-escape-spec branch October 28, 2026 15:31

This branch was successfully deployed

1 active deployment
Preview — 5685b632 Deployed Oct 27, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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