Repository navigation
feat(core): BottomSheetSwitcher renders a dialog per sheet (prototype) - #7706
imdreamrunner wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
the
needs:design-review
|
AI review status for this pull request.
|
PR Analysis Report
No new or modified components detected. Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Visual Regression40 of 482 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 20 more. Generated by PR Enrichment workflow | View full report |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on a maintainer decision: this prototype rewrites the draft BottomSheetSwitcher requirements FR2, FR8, FR9 and PR1 from one shared dialog to one native dialog per sheet, and a maintainer needs to approve or reject that direction (the PR itself recommends keeping the shared dialog). People using a flow would see no difference: frames match the base in Chromium, WebKit differs only by a hover tint, and the scrim never dips. If the direction goes ahead, these need fixing first:
- The switcher calls consumer refs itself, so a React 19 callback ref receives
nulland its returned cleanup never runs. - Dialog props are copied onto every sheet dialog, so an
idpassed to the switcher appears once per sheet. zIndex: 1001on the non-modal path depends on a page-wide band, which AST-027 FR4 rules out.ref.currentis nownullwhile the flow is closed, which breaks released usage, so the Changeset needs[breaking]and a minor bump rather thanpatch.
[Automated review]
| return stableRef.current; | ||
| } | ||
|
|
||
| function assignRef<T>(ref: React.Ref<T> | undefined, value: T | null): void { |
There was a problem hiding this comment.
assignRef calls a callback ref, discards what it returns, and later calls ref(null). Under React 19, a callback ref may return a cleanup that runs instead of ref(null) (public-component-api INV8), and utils/mergeRefs.ts already handles this. A cleanup-returning ref receives ref(null) six times in one open, handoff, close and unmount cycle, and its cleanup never runs.
| )} | ||
| hidden={!isPresented} | ||
| <dialog | ||
| {...switcherDialogProps} |
There was a problem hiding this comment.
Every sheet dialog spreads the switcher's rest props, so an id or data-testid given to BottomSheetSwitcher renders once per sheet. With two sheets, getElementById returns the first sheet's dialog even after it has closed. Could single-instance attributes go only on the host sheet's dialog?
| // would decide which sheet paints in front during a handoff. The interactive | ||
| // sheet's dialog takes the next value in the same overlay band instead. | ||
| dialogNonModalTop: { | ||
| zIndex: 1001, |
There was a problem hiding this comment.
This orders the interactive non-modal sheet by a page-level value in the 1000 band, which AST-027 FR4 rules out ("MUST NOT … depend on page-wide bands such as … 'overlays at 1000.'"). With no switcher element, nothing provides an isolation owner for this relationship.
| @@ -0,0 +1,9 @@ | |||
| --- | |||
| '@astryxdesign/core': patch | |||
There was a problem hiding this comment.
ref.current is now null while the flow is closed and moves between sheet dialogs. Code that reads or listens on the released "one shared native dialog" ref must change, which AST-017 FR1 and FR7 classify as [breaking] in the minor tier, not patch.
| (visibleTransition.retainedSheet == null | ||
| ? undefined | ||
| : sheetLabels.get(visibleTransition.retainedSheet)); | ||
| scrimOpacityRef.current = activeSheet == null ? 0 : 1; |
There was a problem hiding this comment.
This effect now only copies activeSheet into a ref and synchronizes no external system (react-component-runtime INV3). Folding the assignment into applyScrimOpacity keeps the backdrop write in one effect.
Primary intent
Evaluate whether
BottomSheetSwitchershould coordinate one native<dialog>per child sheet (the way a standalone<BottomSheet isOpen>and the open-order stacking candidate in #7648 work) instead of hosting every child as a panel inside one switcher-owned dialog. The public API is unchanged; the structure, the native call pattern, and two observable details change, and they are measured below.The other templates do not fit: nothing is broken (
bug-fix), no capability is added (new-feature), and pixels are unchanged at rest (visual-update).Why the dialog was on the switcher
The switcher has had one shared dialog since it was merged in #5023 (
fb0b7a99f1). The commits inside that PR show it was a deliberate mid-PR move, and the surrounding text states the reason.Before the move — a dialog per sheet, non-modal, under a portaled scrim. The first commit,
9dac3f6792("feat(lab): add multi-step BottomSheet orchestrator"), kept a native<dialog>on every child, but opened each withshow()and had the orchestrator render its own<div>scrim with a JS focus trap and scroll lock. ItsBottomSheet.tsxheader (line 23) reads: "Inside a BottomSheetOrchestrator, the parent owns one shared scrim/focus trap/scroll lock and every child usesshow()beneath it." Becauseshow()does not enter the top layer, the orchestrator had to portal the whole visual layer todocument.body. At5890d633aa,BottomSheetSwitcher.tsxline 214 explains: "Switcher-managed sheets use show(), so they do not enter the native top layer. Portal the complete visual layer to body to keep transformed, contained, or overflow-clipped application ancestors from displacing the viewport scrim and sheets. Waiting until layout keeps server and initial client markup aligned while still mounting the portal before paint." The doc of the same revision summarised it as: "The switcher portals one shared visual layer to document.body and renders one scrim for the whole flow, so containing blocks cannot clip it and handoffs never stack backdrops."The move —
fbdc197ef7"refactor(lab): share dialog across BottomSheet flow". That commit replaced the per-sheetshow()dialogs, the portal, and the<div>scrim with one switcher-owned dialog opened once withshowModal(). The reason is in the header it added (BottomSheetSwitcher.tsxlines 18–21, unchanged onmaintoday): "All child sheets render as panels inside one switcher-owned<dialog>. A scrim flow calls showModal() once and keeps that native top-layer dialog open across every handoff. A no-scrim flow calls show() on the same inline shell. This keeps one modal boundary and one native ::backdrop without a portal." The PR description says the same from the outside: "WithhasScrim={true}, the switcher usesshowModal()to provide one top-layer boundary and one native::backdropacross every handoff", and lists as validation "Verified exactly one open dialog across handoffs" and "Verified modal mode remains viewport-sized under a transformed and clipped ancestor".So the stated reasons were, in order:
::backdropacross every handoff — no stacked or flickering backdrops while one sheet replaces another, and native modality (inert page, native focus containment,cancel) from oneshowModal()instead of a JS trap plus a painted<div>.show()for the children (twoshowModal()calls would have stacked two backdrops), andshow()dialogs stay in their containing block, so the layer had to be portaled out of transformed or clipping ancestors, with an SSR/hydration timing cost. OneshowModal()dialog sits in the top layer wherever it is rendered.BottomSheetPanel(61aa914d5b), and describes children as "panel layers inside it". The currentBottomSheetSwitcher.doc.mjsanatomy onmainnames them "Sheet panels", and the draftBottomSheetSwitcher.spec.mdrecords it as FR2 ("All child sheets share one native dialog") and PR1 ("One shared host"). AsheetIdchild is a step of one sheet, not a sheet of its own.No reviewer discussion on #5023 argues the point (the PR has no review comments), and the draft spec's decision log is empty, so items 1–2 are the stated engineering reasons and item 3 is the model they serve.
Before / after structure
Before (
main):After (this PR): the switcher renders no element; each child renders what a standalone sheet renders.
The switcher keeps the transition state machine, the focus trap, the scroll lock, the layer-dismissal registration, the opener capture and focus return, and the
purpose-driven dismissal rules. It gains three coordination duties the shared dialog never needed:::backdroppaints above the retained dialog, so it would dim the previous panel and double-dim the page. The retained dialog therefore keeps the flow's scrim while it is visible; the entering dialog is presented with a transparent backdrop (--_sheet-scrim-opacity: 0); when the retained dialog closes after its fade, the new owner takes the value with its backdrop transition disabled for that one style flush (--_sheet-scrim-duration: 0s, inline, then removed). A0.01spin was tried first and still painted one frame at the start value — a visible one-frame dip — which is why the swap disables the transition outright.showModal()records the focused element andclose()returns focus to it. For a sheet presented from a Back/Continue button inside another sheet, that return would pull focus out of the active sheet when the retained dialog closes after its fade (measured as a focus jump before the fix). The switcher blurs such a control before presenting, so the browser records nothing; the panel focus the switcher sets stands.Behaviour deltas
mainshowModal()for the flow)showModal(next)on entry,close(previous)after its fadearia-hidden)dipFrames=0<body>show()focusing steps)onActiveSheetChange(null)perpurposeclose()+showModal()of the returning sheetrefnullwhile closedonCancel, dialog propsaria-labelstill wins<dialog><dialog>per sheetopenattribute server-side)Dialogopened from a stepshowModal()orderNon-modal flows (
hasScrim={false}) still useshow()per sheet; the interactive sheet's dialog takesz-index: 1001so DOM order cannot put a retained sheet in front. This stays inside the non-modal path that AST-027 already lists as a conformance gap.Test edits
Existing suites were run unedited first. Every failure was an assertion about the structure this PR deliberately changes; no behaviour assertion was weakened.
BottomSheetSwitcher.test.tsx)getSharedDialog()getActiveDialog(); addedgetSheetDialog(testId)andopenDialogs()aria-hidden)dialog[open]count 1 →[details, confirm]during the handoff,[confirm]after; shared-dialog identity → active-dialog identity; retained dialog assertedinertdialog[open]count 1 → 2; asserts a thirdshowModal()(re-present)0Nine tests were added under "one native dialog per sheet": per-sheet dialog structure, the native call order through a handoff and close,
refmovement, dialog-prop forwarding to every sheet dialog, one scrim with an instant takeover, focus into the entering sheet with the retained dialog inert, blur-before-present (no focused control for the browser to return to),purposeafter a handoff, non-modalshow()per sheet, and re-presenting a still-fading sheet on reversal.Other suites in the validation set (Dialog, Layer, Selector, DateInput, Tokenizer, MobileTokenizer, …) pass unedited: 2458 tests on this branch vs 2448 on
main(+10 new).Records touched
BottomSheetSwitcher.spec.mdandBottomSheet.spec.mdare bothdraftrecords. They were edited only where they describe the shared host (Intent, Ownership, FR2, FR8, FR9, representative states, PR1, the design-relationships rows, and the theming-anatomy keyShared dialog→Sheet dialogs, whichcheck:knowledgerequires to match the.doc.mjsanatomy). No decision was added; the decision log stays "None".Writing rubric results
draftstaysdraft; no claim was promoted.BottomSheetSwitcher.test.tsxcovers FR1–FR9).Authority
component:BottomSheetSwitcher(draft) FR1–FR9, PR1, AR1–AR3 — preserved except the host structure noted above.family:overlay-dismissal— the switcher keeps one registration for the flow and routes nativecancelthroughshouldDismissOnCloseRequest().architecture:layer-runtimeandspec:AST-027— nativeshowModal()hosting; the non-modalshow()+z-indexpath remains the recorded gap.spec:AST-002/architecture:public-component-api— props, defaults, and exports unchanged;reftarget semantics documented in the prop doc and the Changeset.Change and evidence
Files:
BottomSheetSwitcher.tsx(coordinator, no element),BottomSheet.tsx(the switcher item renders its own dialog; standalone host untouched;scrimtransition duration reads an unset custom property with the token as fallback; one newdialogNonModalTopstyle),BottomSheetSwitcherContext.ts(host props and scrim/presentation hooks),BottomSheetSwitcher.doc.mjs,BottomSheet.doc.mjs, the two draft specs, tests, and a[feat]Changeset.API audit. Core built on
origin/mainand on this branch;.d.tsfiles and theexportsmap diffed.exportsmap identical.BottomSheet/index.d.tsand the rootindex.d.tsare byte-identical.BottomSheetSwitcher.d.tsdiffers only in doc comments and the destructuring pattern in the declared signature (no type change).BottomSheetSwitcherContext.d.tschanges but is not reachable from any public entry.Browser (Chromium, 390×844, touch, static Storybook,
mainvs branch, storieshug-content,capped,no-scrim). Harness instrumentsshow/showModal/close, samples every open dialog's computed::backdropopacity per animation frame through each switch and derives the combined page dimming, recordsdocument.activeElementper frame, hit-tests the interactive panel, and reads a page pixel's luminance.showModal(next)at click,close(previous)~640 ms later after its fade.main: none. Open/close/open flashes: none; a dialog is never closed and re-shown within a switch except the rapid-reversal case (oneclose+showModal).dim 0.5 [A:1.00]→0.5 [A:1.00 B:0.00]→0.5 [B:1.00];dipFrames=0 overFrames=0; pixel luminance 127 mid-handoff and settled on both builds.main:0.5constant.<body>onmain.no-scrimon both.showModal()dialog opened during a handoff hit-tests on top on both builds.scrollWidth 390 = clientWidth 390), no console errors or warnings on either build.What it buys and breaks under "the switcher is the sheet"
If the switcher is one sheet and its
sheetIdchildren are steps, this PR turns every step into a sheet: each gets its own modal boundary, top-layer entry, backdrop, and native focus record. The measurements show the user-visible result can be made identical, but only by actively undoing what the browser does for a second sheet — hiding the second backdrop and handing the scrim over without a fade, re-presenting on reversal to fix top-layer order, and blurring beforeshowModal()soclose()cannot return focus into the active step. The shared dialog needs none of that because the replace animation happens inside one host.What per-step dialogs buy: one DOM shape for standalone sheets, switcher steps, and the open-order stacking candidate (#7648); each step carries its own accessible name and
alertdialogrole without a precedence rule; non-modal steps receive focus natively.What they cost: two native calls per switch, two open modal dialogs through every handoff, a moving
ref, and three coordination mechanisms that exist only to make two sheets look like one.Recommendation
Keep the switcher as the single sheet. The shared dialog is the direct expression of the model (one sheet, N steps), it is the only structure that gets the replace handoff, one scrim, and one focus boundary for free, and the stacking candidate in #7648 does not need this change: a switcher with one dialog is already one level in an open-order stack. This prototype is useful as evidence of the exact cost of the alternative, and the blur-before-present finding is worth keeping in mind for any design that presents a second modal dialog from inside a first.
Scope
Testing
Browser evidence (call logs, per-frame backdrop samples, focus traces, screenshots) was captured against the static Storybook of both builds at 390×844 with touch, as summarised above.