fix(core): audit BottomSheetSwitcher - #6184
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR Modified ComponentsBottomSheet (@astryxdesign/core) · View in Storybook
Layer (@astryxdesign/core) · View in Storybook
Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Visual RegressionStatus: No visual change across 4 compared shot(s). Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
cixzhang
left a comment
There was a problem hiding this comment.
Semantic verdict: request changes
Builders who inspect active Escape traps or contain key events now get stale state or a non-modal sheet that no longer closes.
- Keep an open modal switcher visible through
hasActiveFocusTrapEscape()until that public shim is removed through an explicit compatibility change. - Preserve built-in non-modal Escape dismissal when
onKeyDownonly callsstopPropagation();preventDefault()should remain the cancellation path.
The new child-first ordering itself works: Chromium closes the nested layer first and the switcher on the next Escape. Focused A/B tests reproduce both regressions against current main.
[Reviewed by Robohands]
cixzhang
left a comment
There was a problem hiding this comment.
Semantic verdict: request changes
Builders coordinating Escape behavior still hit two regressions at this head:
hasActiveFocusTrapEscape()reportsfalsewhile the modal switcher’s focus trap is active.- An
onKeyDownhandler that only callsstopPropagation()now keeps a non-modal switcher open; previously onlypreventDefault()canceled built-in dismissal.
The intended nested ordering works in Chromium: the first Escape closes the child and the second closes the host. Please preserve both released behaviors while keeping that order.
[Reviewed by Robohands]
cixzhang
left a comment
There was a problem hiding this comment.
Semantic verdict: request changes
The main-only merge leaves both earlier regressions at this head. In Chromium:
- A modal switcher reports
falsethrough the exportedhasActiveFocusTrapEscape()compatibility shim while its focus trap is active. - For a non-modal switcher, an
onKeyDownhandler that only callsstopPropagation()now keeps the sheet open; before this change, onlypreventDefault()cancelled built-in dismissal.
The intended ordering works: a nested tooltip closes on the first Escape and the switcher on the second. Please preserve both released behaviors while keeping that child-first order.
[Reviewed by Robohands]
|
AI review status for this pull request.
|
…-audit/BottomSheetSwitcher/2026-09-09
|
/approve-spec 745876d |
Why
core/BottomSheetSwitcherwas the first eligible component in the refreshed Night Watch queue. The audit found that a nested layer inside a non-modal switcher could not own Escape. Independent exact-head review then caught two related compatibility regressions: modal switchers disappeared from the releasedhasActiveFocusTrapEscape()signal, and a consumeronKeyDownusingstopPropagation()could strand a non-modal flow.What changed
stopPropagation()does not strand the sheet whilepreventDefault()still cancels dismissal;No public prop, export, default, or compatibility promise changes. The separate non-modal top-layer migration remains out of scope.
Audit result
core/BottomSheetSwitcherN/1.16.24a31578d288df665a1ceabab296d430f6e361cb5745876d581fec65828e959dd79d42ec16ed1d2e4c29c48f3ef437ebcdeeda43d420796db9e31817165dedfe9d1dde9c396b9d3d99989ffc933f2efcf— all 17 PR-owned files match the reviewed head byte-for-byte.ebd8678ab29db8ff2dbce786e8bf3fc39c2deda2Retained findings
hasScrim={false}still usesdialog.show()plus page-levelz-index: 1000, so clipping ancestors or unrelated stacking contexts can make the sheet unreachable. The required Layer/top-layer migration is broader and potentially compatibility-affecting.sheetIdvalues is unsettled.hasScrimto derive cohesive modality, focus, scroll, backdrop, and hosting outcomes. Separating them remains an unscored future API question.Shared/systemic observations are not scored twice: BottomSheetPanel's drag pill measures below 3:1 in the audited themes, and Matcha supporting text measures 4.41:1 under the existing theme-token contrast class.
Validation
745876d….745876d…: all automated checks pass, includingdocsite-test,test,build,pr-a11y,pr-rtl, and Stable visual regression. Independent approval and the manual acceptance contexts remain pending.Reconciliation needed
BottomSheetStackContext, the switcher's one shared-stack registration,LayerDepthProvider, local Escape routing, and compatibility signaling.Exact-head audit eligibility report
{ "schemaVersion": 1, "component": "BottomSheetSwitcher", "package": "core", "auditMode": "N", "rubricVersion": "1.16.2", "heads": { "repository": "745876d581fec65828e959dd79d42ec16ed1d2e4", "componentContract": "745876d581fec65828e959dd79d42ec16ed1d2e4" }, "inventory": { "closed": true, "gaps": ["B14", "C3", "T7", "C22", "manual:sheetId-invalidity"] }, "unresolvedGaps": { "objective": [ "B14: non-modal dialog remains vulnerable to ancestor clipping and unrelated stacking contexts", "C3: switcher item focus is still moved from an Effect", "T7: the switcher-owned scrim has no public theming target", "C22: the handoff state machine has no focused hook or utility boundary" ], "manual": [ "Safe behavior for non-matching or duplicate sheetId values is unsettled", "Separating hasScrim-derived outcomes is an optional future API direction, not a current violation", "PR #5813 must preserve shared dismissal registration and compatibility behavior when reconciling its controller changes", "PR #6042 contains a draft statement that becomes stale after this fix" ] }, "remediations": [ { "ruleId": "A4", "beforeEvidence": [ "https://github.com/facebook/astryx/blob/c29c48f3ef437ebcdeeda43d420796db9e318171/pr-6184/BottomSheetSwitcher/before/focused-tests-red.txt" ], "afterEvidence": [ "https://github.com/facebook/astryx/blob/c29c48f3ef437ebcdeeda43d420796db9e318171/pr-6184/BottomSheetSwitcher/after/focused-tests.txt" ] }, { "ruleId": "architecture:public-component-api/INV5/INV9", "beforeEvidence": [ "https://github.com/facebook/astryx/blob/c29c48f3ef437ebcdeeda43d420796db9e318171/pr-6184/BottomSheetSwitcher/review-before/focused-tests-red-1.16.2.txt" ], "afterEvidence": [ "https://github.com/facebook/astryx/blob/c29c48f3ef437ebcdeeda43d420796db9e318171/pr-6184/BottomSheetSwitcher/review-after/focused-tests-1.16.2.txt" ] }, { "ruleId": "P2/P28/B15", "beforeEvidence": [ "https://github.com/facebook/astryx/blob/c29c48f3ef437ebcdeeda43d420796db9e318171/pr-6184/BottomSheetSwitcher/review-before/focused-tests-red-1.16.2.txt" ], "afterEvidence": [ "https://github.com/facebook/astryx/blob/c29c48f3ef437ebcdeeda43d420796db9e318171/pr-6184/BottomSheetSwitcher/review-after/focused-tests-1.16.2.txt" ] }, { "ruleId": "X3/X4/X5b/X7/X10/X14/X16", "beforeEvidence": [ "https://github.com/facebook/astryx/blob/c29c48f3ef437ebcdeeda43d420796db9e318171/pr-6184/BottomSheetSwitcher/README.md#objective-remediations" ], "afterEvidence": [ "https://github.com/facebook/astryx/blob/745876d581fec65828e959dd79d42ec16ed1d2e4/packages/core/src/BottomSheet/BottomSheetSwitcher.doc.mjs", "https://github.com/facebook/astryx/blob/745876d581fec65828e959dd79d42ec16ed1d2e4/packages/cli/assets/templates/blocks/components/BottomSheet/BottomSheetSwitcherReviewFlow.tsx" ] }, { "ruleId": "I17", "beforeEvidence": [ "https://github.com/facebook/astryx/blob/c29c48f3ef437ebcdeeda43d420796db9e318171/pr-6184/BottomSheetSwitcher/before/rtl-audit.txt" ], "afterEvidence": [ "https://github.com/facebook/astryx/blob/c29c48f3ef437ebcdeeda43d420796db9e318171/pr-6184/BottomSheetSwitcher/after/rtl-audit-report.json" ] } ], "approvals": [ {"name": "spec-owner-approval", "state": "passed"}, {"name": "visual-acceptance", "state": "passed"}, {"name": "independent-review", "state": "passed"} ], "checks": [ {"name": "exact-head-local-focused", "state": "passed"}, {"name": "exact-head-component-axe", "state": "passed"}, {"name": "exact-head-rtl", "state": "passed"}, {"name": "exact-head-chromium-receipts", "state": "passed"}, {"name": "github-ci", "state": "passed"}, {"name": "audit-eligibility", "state": "absent"} ], "eligibility": { "eligible": false, "reasons": [ "spec:AST-029 is accepted rather than shipped", "the trusted audit-eligibility validator/status is not shipped", "two retained BLOCKs remain: B14 and C3", "hosting, focus-lifecycle, and identifier behavior still require human ownership" ] } }Auto-merge was never enabled. This PR merged manually after independent approval; the final tracker row records the landed squash commit.