Repository navigation
feat(core): BottomSheet locks page scroll by CSS keyed off its modal dialog - #7719
imdreamrunner wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
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. Generated by PR Enrichment workflow | View full report |
1a42428 to
a84ca11
Compare
![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 scroll lock rests on component:BottomSheet FR5 and DEC-2, which exist only in the draft spec PR #7744, so a maintainer needs to approve or amend that record, or name other governing spec for a document-lifetime CSS scroll lock, before the change can be judged. Whatever that decision is, three things need work:
- On a page whose
<body>has a non-visibleoverflowand a constrained height (for examplehtml, body { height: 100% }plusbody { overflow-x: hidden }), opening a modal sheet jumps the page to its top and the scroll offset is lost on close, becauseoverflow: hiddenon the root stops<body>'s overflow from propagating to the viewport; locking<body>in that case keeps the offset. - Desktop Safari 17.0–18.1 (Tier 2) has no
scrollbar-gutter, so the gutter hold needs the shared hook's padding fallback or a documented reduced fallback (spec:AST-013FR7). spec:AST-013FR10 needs real-device iOS Safari evidence for the new lock, which Playwright WebKit cannot stand in for.
[Automated review]
| /** The lock, as CSS text. */ | ||
| export function sheetScrollLockCss(): string { | ||
| return ( | ||
| `${SHEET_SCROLL_LOCK_SELECTOR}{overflow:hidden}` + |
There was a problem hiding this comment.
Setting overflow: hidden on the root changes overflow propagation. CSS applies <body>'s overflow to the viewport only while the root's is visible, so on a page with html, body { height: 100% } and body { overflow-x: hidden } (or body { height: 100vh; overflow-y: auto }), <body> becomes a viewport-tall scroller when a sheet opens. scrollY then clamps to 0 and the page stays at the top after close; in Chromium it went from 300 to 0, where the base keeps 300. When, at present time, the root's computed overflow is visible and <body>'s is not, locking <body> instead keeps the offset.
| export function sheetScrollLockCss(): string { | ||
| return ( | ||
| `${SHEET_SCROLL_LOCK_SELECTOR}{overflow:hidden}` + | ||
| `${SHEET_SCROLLBAR_GUTTER_SELECTOR}{scrollbar-gutter:stable}` |
There was a problem hiding this comment.
scrollbar-gutter reached Safari only in 18.2 (see hooks/scrollbarGutter.ts), and the Browser Support guide's Tier 2 includes Safari 17+. On those versions the removed useScrollLock path padded <body> by its measured growth; this rule has no fallback, so the page reflows by the scrollbar's width there. spec:AST-013 FR7 asks for equivalent behaviour or a documented reduced fallback.
| * modal, their once-per-document installer, and the per-present scrollbar | ||
| * gutter measurement | ||
| * @position Core tests for sheetScrollLock.ts; the engines' reading of the | ||
| * rules is BottomSheetNativeModal.a11y.browser.spec.ts |
There was a problem hiding this comment.
BottomSheetNativeModal.a11y.browser.spec.ts does not exist at this head; the engine evidence here is __tests__/BottomSheetScrollLock.a11y.browser.spec.ts.
7777e5e to
fd6a8d4
Compare
a84ca11 to
5c35da2
Compare
There was a problem hiding this comment.
Approval waits on a maintainer decision: this scroll lock rests on component:BottomSheet FR5 and DEC-2, which exist only in the draft spec PR #7744, so a maintainer needs to approve or amend that record (the CSS lock keyed off :modal, the scrollY and nested-scroller changes, the document-lifetime stylesheet, and whether FR5's "no inline styles on <body>" allows the Safari 17.0-18.1 padding fallback) before this can land. The offset jump on body-scrolled pages from the earlier revision is fixed. Independently of the spec decision, two things are still needed:
- On engines without
scrollbar-gutter, a sheet that unmounts while presented never releases the<body>padding, because the hold is left only when the dialog'sopenattribute changes (sheetScrollLock.ts:188-192); release it on unmount and add that case tosheetScrollLock.test.ts. spec:AST-013FR10 still needs the real-device iOS Safari check described in the PR body.
[Automated review]
| if (!observedDialogs.has(dialog) && typeof MutationObserver === 'function') { | ||
| observedDialogs.add(dialog); | ||
| new MutationObserver(() => { | ||
| if (!dialog.open) { | ||
| leaveGutterFallback(dialog); | ||
| } | ||
| }).observe(dialog, {attributes: true, attributeFilter: ['open']}); |
There was a problem hiding this comment.
On engines without scrollbar-gutter (Safari 17.0-18.1), this observer is the only way a dialog leaves the shared padding hold, and it never fires for a sheet that unmounts while presented: neither host closes its dialog on unmount, and removing an open <dialog> from the document keeps its open attribute. The detached dialog then stays in the member set, so <body> keeps its padding-right for the rest of the page's life and later sheets cannot release it (reproduced for both BottomSheet and BottomSheetSwitcher; the useScrollLock path released it). Releasing the membership and disconnecting the observer when the host unmounts, for example from a layout-effect cleanup in both hosts, would fix it, and an unmount-while-presented case in sheetScrollLock.test.ts would cover it.
5c35da2 to
6657ea4
Compare
fd6a8d4 to
0beae80
Compare
There was a problem hiding this comment.
Approval waits on a maintainer decision: this scroll lock rests on component:BottomSheet FR5 and DEC-2, which exist only in the draft spec PR #7744, so a maintainer needs to approve or amend that record (the CSS lock keyed off :modal, the scrollY and nested-scroller changes, and the document-lifetime stylesheet) before this can land. The padding leak on unmount from the previous revision is fixed. Independently of that decision, these still need work:
- Without
scrollbar-gutter, a sheet presented while another is still closing never joins the padding hold, so the page shifts 15px when the first sheet finishes its exit (sheetScrollLock.ts:219-228); keep the hold for every modal sheet dialog and test a second present made after the scrollbar is already hidden. - The new unmount-only release effects end the hold under StrictMode replay without the presentation effects re-joining it, and the per-dialog
MutationObserveris never disconnected, so the release should live with the acquisition. spec:AST-013FR10 still needs the real-device iOS Safari check described in the PR body.
[Automated review]
| } else { | ||
| dialog.removeAttribute(SHEET_HOLDS_GUTTER_ATTRIBUTE); | ||
| } | ||
| if (!takesSpace || supportsScrollbarGutter(doc)) { |
There was a problem hiding this comment.
On an engine without scrollbar-gutter, a dialog presented while another sheet dialog already holds the lock measures no scrollbar here (the first lock hid it, so clientWidth equals innerWidth), returns early and never joins the hold. When the first dialog closes, leaveGutterFallback releases the padding while this one is still modal, and the page shifts 15px. Once a document's hold exists, every modal sheet dialog should join it whatever it measures.
| settleFirst(); | ||
| expect(document.body.style.paddingRight).toBe('15px'); | ||
|
|
||
| // A second modal sheet dialog (a handoff) joins the same hold. |
There was a problem hiding this comment.
This second present still sees clientWidth 1265, but once the first lock has hidden the scrollbar on an engine without scrollbar-gutter the layout viewport reports 1280. With clientWidth at 1280 for this call the second dialog does not join, and the padding is gone after the first dialog closes.
| // A sheet unmounted while presented takes its dialog out of the document | ||
| // with it; the CSS lock releases on its own (the dialog is no longer | ||
| // `:modal`), but the scrollbar-gutter fallback is script and must be told. | ||
| useLayoutEffect( |
There was a problem hiding this comment.
Under StrictMode replay this mount-only cleanup ends the padding hold, but the presentation effect does not re-join because dialog.open is already true, so a sheet mounted open loses its padding (architecture:react-component-runtime INV3). Releasing from the effect that acquired the hold avoids that; BottomSheetSwitcher.tsx:302 has the same shape.
…al; each BottomSheetSwitcher step owns its native dialog Records two owner decisions in the BottomSheet and BottomSheetSwitcher component records. BottomSheet FR5/DEC-2: while a sheet dialog is modal the page does not scroll, <body> gains no inline styles, the scroll offset is unchanged on release, and a classic scrollbar's gutter is held only when it takes layout space; the lock follows the dialog's own modal state. The mechanism is the implementation's, not a public contract. BottomSheetSwitcher DEC-1: each step owns its own native dialog; the switcher coordinates presentation, retention, the one-scrim handoff, re-present on reversal, flow-level focus return, and purpose-gated cancel, with native modal focus containment and a switcher-owned isolated wrapper for non-modal ordering. DEC-2: the host dialog's identity is implementation, not public contract; the hosting change is nonbreaking. Rewrites the claims that described the shared host and the body-pin lock (Switcher FR2, FR5, FR8, FR9, PR1, the Dialog surface concept and states; BottomSheet ownership, states, and a new FR5) as decided requirements. The anatomy keeps its shipped name until the implementation lands.
…dialog Every sheet dialog carries data-sheet-dialog and one stylesheet, installed once per document, freezes the page's scroller exactly while a sheet dialog is modal: through a switcher handoff, beneath a Dialog opened from a sheet, and released with the last modal sheet dialog. No counter, no saved scroll offset, no inline styles on <body>. The rule freezes the root, or <body> on a page whose <body> owns the viewport scroll, so the page keeps its offset either way. The scrollbar gutter is held open only when a classic scrollbar took layout space when the sheet was presented, so a non-scrolling page gets no undimmed strip; where the engine has no scrollbar-gutter the shared gutter helper pads <body> by the width it grew, joined by every presented modal sheet dialog and released, through paired effect cleanups, with the last one.
0beae80 to
b578bbd
Compare
6657ea4 to
5d75a18
Compare
There was a problem hiding this comment.
Approval waits on a maintainer decision: this scroll lock rests on component:BottomSheet FR5 and DEC-2, which exist only in the draft spec PR #7744, so a maintainer needs to approve or amend that record (the CSS lock keyed off :modal, the scrollY and nested-scroller changes, the <body> padding fallback and the document-lifetime stylesheet) or name other governing spec. Whatever that decision, two things remain before this can be approved:
- Where
scrollbar-gutteris unsupported (Safari 17.0–18.1), closing a standalone sheet paints one frame with the scrollbar back and the 15px<body>padding still applied, because the padding hold is left only in a passive effect afterdialog.close(); releasing it where the dialog closes, as the switcher's layout effects already do, removes the flash. spec:AST-013FR10 needs a run on Safari on the latest stable iOS, which the PR's own iOS check steps already describe.
[Automated review]
| useEffect(() => { | ||
| const dialog = dialogRef.current; | ||
| if (dialog == null || !(shouldPresent && hasScrim)) { | ||
| return; | ||
| } | ||
| return acquireSheetScrollLock(dialog); | ||
| }, [hasScrim, shouldPresent]); |
There was a problem hiding this comment.
Where scrollbar-gutter is unsupported, this cleanup is the only way a standalone sheet leaves the padding hold, and it runs only after React commits isPresented: false (then a microtask). handleMotionComplete has already called dialog.close() by then, so the CSS lock has let the classic scrollbar back while <body> still carries the 15px padding. In Chromium with classic scrollbars and scrollbar-gutter disabled, the frame after the close shows both, and the page content narrows from 1265px to 1250px for that frame; the switcher avoids it by closing and leaving in layout effects. Calling the release right after dialog.close() (keeping this cleanup as the backstop, since a second leave is a no-op) closes the gap.
b578bbd to
e79eac5
Compare
Primary intent
Lock page scroll for a modal
BottomSheetwith CSS keyed off the browser's own modal-dialog state instead of pinning<body>from script. One stylesheet, installed once per document, freezes the page's scroller while any sheet dialog is modal — a standalone sheet, aBottomSheetSwitcherflow through its handoffs, a sheet beneath aDialogopened from it, and however many sheet dialogs a future stacking model keeps open — with no counter, no saved scroll offset, and no inline styles on<body>. It freezes the root, or<body>on a page whose<body>owns the viewport scroll, so the page keeps its offset either way. It is independent of where the switcher keeps its dialog: it applies to the standalone sheet and to the switcher's existing shared dialog alike.The other templates do not fit: nothing is broken (
bug-fix), no capability is added (new-feature), pixels are unchanged at rest (visual-update).User or maintainer impact
Consumers see the same sheets: page scroll locked while a modal sheet is open, released after its exit motion. Two observable details change (see Behaviour deltas):
window.scrollYkeeps reading the page's offset while a sheet is open, and a page that scrolls a nested element rather than the viewport is no longer pinned by the sheet. Maintainers loseuseScrollLockfrom both sheet hosts and gain a ~120-line module (sheetScrollLock.ts) whose only state is one<style>element.Problem and solution fit
Why change the lock?
showModal()makes the page inert but does not stop it scrolling; today the sheet answers that by pinning<body>(position: fixed; top: -scrollY), keeping a lock counter across overlays, measuring the scrollbar gutter, and callingscrollToon release. Why does that matter? Each of those is state to keep balanced: a second open sheet, a sheet beneath aDialog, a flow that re-presents a dialog all have to count correctly, and the body pin makesscrollYread0for anything else on the page. Why does that matter? Every design that keeps more than one sheet dialog open multiplies the bookkeeping, while the browser already knows exactly when a sheet dialog is modal —dialog:modal— and:root:has(dialog[data-sheet-dialog]:modal) { overflow: hidden }is true in precisely those moments.Every delta maps to that:
sheetScrollLock.ts: the rules, the once-per-document installer, andprepareSheetScrollLock(dialog)called by each host beforeshowModal()data-sheet-dialogon every sheet dialog (standalone and switcher)data-sheet-locks-bodyset on the dialog when, at present time, the root's computed overflow isvisibleand<body>'s is not; the lock rule then freezes<body>instead of the root<body>'s overflow to the viewport only while the root's isvisible. Freezing the root on such a page (html, body { height: 100% }+body { overflow-x: hidden }, orbody { height: 100vh; overflow-y: auto }) stops that propagation, makes<body>a viewport-tall scroller, and clampsscrollY300 → 0 (measured on the previous head); freezing<body>propagateshiddento the viewport and keeps 300data-sheet-holds-gutterset on the dialog only whenwindow.innerWidth > documentElement.clientWidthat present time; a rule appliesscrollbar-gutter: stableon the root only then::backdrop; reserving one on a page that had no scrollbar opens an undimmed 15px strip and shifts the sheet 7.5px (measured). Same condition asuseScrollLock's gutter hold. The root is the right element in both lock modes:scrollbar-gutteron the root reaches the viewport whichever element's overflow it uses (measured:scrollbar-gutteron<body>does not)CSS.supports('scrollbar-gutter: stable')is false (Safari 17.0–18.1, Tier 2), the sharedholdScrollbarGutterhelper pads<body>by the width it actually grew, held by the first modal sheet dialog that needs it and released when the last one'sopenattribute goes (aMutationObserver, so release runs as a microtask before the next paint)spec:AST-013FR7: equivalent behaviour on the supported floor, reusing the exact fallbackuseScrollLockships rather than a new mechanism.prepareSheetScrollLock(dialog)now returns thesettlecall the host makes right aftershowModal(), which is when the helper measures growthacquireSheetScrollLock(dialog)returns the release; each host calls it in the effect that keeps its dialog a presented modal sheet and returns the release as that effect's cleanup (shouldPresent && hasScrimfor the standalone sheet,isModalfor the switcher)MutationObserver(removed, so nothing is left connected)clientWidth === innerWidth); previously it returned early and never joined, so closing the first released the padding under the second — a 15px shift mid-flowuseScrollLockremoved fromStandaloneBottomSheetandBottomSheetSwitcherDialog,Drawer,Lightboxkeep the hookRemovable tagalongs: none. The
.doc.mjsedits only describe the mechanism that changed.Authority
component:BottomSheetFR5 and DEC-2 (spec PR spec(core): each BottomSheetSwitcher step owns its native dialog #7744) — FR5 states the effects (no page scroll while a sheet dialog is modal; the sheet's lock sets no inline positioning or overflow on<body>; offset unchanged at release; gutter held only when a classic scrollbar takes space, as a<body>padding wherescrollbar-gutteris unsupported, released with the lock); DEC-2 binds the lock to the dialog's modal state and leaves the mechanism to the implementation.component:BottomSheetSwitcherFR5 and PR1 reference it.spec:AST-013FR10 — browser-owned behaviour needs real-browser evidence: Chromium and WebKit evidence is attached; iOS Safari touch scrolling is not verified here (see Open risk).architecture:layer-runtime— nativeshowModal()hosting unchanged.spec:AST-002/architecture:public-component-api— props, defaults, exports unchanged.Behaviour deltas
mainposition: fixed; top: -scrollY; overflow: hiddenon<body>, lock counter,scrollToon release:root { overflow: hidden }while a sheet dialog is modal;<body>untouchedwindow.scrollYwhile a sheet is open0(restored by script on close)scrollbar-gutter: stableon root; padding fallback where unsupported<body>owns the viewport scroll (html, body { height: 100% }+body { overflow-x: hidden };body { height: 100vh; overflow-y: auto })<body>frozen (itshiddenpropagates to the viewport), offset kept — the previous head of this PR clamped to 0Dialogfrom a sheet / through the exit motion:modalthroughout)scrollbar-gutterscrollbar-gutter<StrictMode>, sheet mounted open, engine withoutscrollbar-gutteroverflow: hiddenon the root — unverified on hardwareTest edits
Existing suites ran unedited against the new code first: 2 failures, both assertions on the removed body pin (
document.body.style.position === 'fixed') in "keeps a standalone modal sheet presented through its exit animation" and "dismisses the flow from the one shared scrim". Both now assert the lock's precondition from the DOM (isScrollLockEngaged(): an opendialog[data-sheet-dialog]the stubs recorded as modal), since jsdom evaluates neither:modalnor:has(); the behaviour asserted (lock held through the exit, released after it) is unchanged. Two more assertions in the switcher suite that checked the body was not pinned now check the lock is not engaged.New:
sheetScrollLock.test.ts(8: rule text — root rule, body rule, gutter rule on the root only; once-per-document install; iframe copy; the gutter mark set and cleared by the measured viewport; the body-lock mark set for both page shapes and cleared for a self-scrolling root; the padding fallback engages wherescrollbar-gutteris unsupported; a second dialog that measures no scrollbar (the first lock hid it:clientWidth1280) joins the hold anyway; the first's leave keeps the padding, the last's releases it; no padding where the property is supported or no scrollbar takes space; a leave-then-join in one task (a StrictMode replay) keeps the hold, and a presented dialog removed from the document releases through its paired cleanup),BottomSheet.test.tsx"native modal host" (8: attribute + stylesheet installed once before presenting with<body>untouched; both hosts give up the padding fallback when unmounted while presented; sheet B opened while sheet A holds the padding — A closes through its exit, padding stays15pxuntil B closes — fails on the previous head (padding released with A);<StrictMode>with a sheet mounted open: padding held after the replay, released on close and on unmount — fails on the previous head (padding lost to the replay); gutter held only when a classic scrollbar took space at present time; lock held under a CoreDialogand released only with the sheet; nativecancelbypurpose; backdrop click bypurpose),BottomSheetSwitcher.test.tsx(1: the flow's one modal dialog locks by CSS across handoffs, no body pin, released after the last exit), and__tests__/BottomSheetScrollLock.a11y.browser.spec.ts(4, Chromium + WebKit: the lock through a handoff, under a probeshowModal()dialog, released after the exit withscrollYunchanged and<body>untouched, no chaining from the sheet's content; the offset kept on both body-owned-scroller page shapes,<body>frozen and the root leftvisible,<body>never becoming its own scroller — both fail on the previous head withscrollY300 → 0 and pass now; a non-modal flow locks nothing). Against themainbuild the first browser test fails as expected (rootoverflowstaysvisible).Change and evidence
Files:
sheetScrollLock.ts(new),BottomSheet.tsx,BottomSheetSwitcher.tsx, the two.doc.mjs, the three test files, the browser spec, a[feat]Changeset (patch: no public API delta). No knowledge record is touched; the record this implements is #7744.API audit. Core built on
origin/mainand on this branch:exportsmap identical;BottomSheet/index.d.tsand the rootindex.d.tsbyte-identical;BottomSheet.d.tsandBottomSheetSwitcher.d.tsdiffer only in doc comments;sheetScrollLock.d.tsis new and unreachable from any public entry.Browser (Chromium 149 and WebKit under Playwright 1.61, static Storybook of
mainand of this branch).hug-content/capped/no-scrim/ standaloneshowcase: page atscrollY300, flow opened from an off-screen opener; wheel 400 over the backdrop → 300; real finger drag 300px (Chromium, CDP) → 300; wheel 3000 and drag 400px inside the sheet content → 300 (no chaining); mid-handoff → 300; after close → 300 with rootoverflowback tovisible;<body>carries no inline style at any point; viewport width 390 → 390.main: body pinned,scrollYreads 0, restored to 300 by script. Both engines.--hide-scrollbarsremoved), six page shapes (root scroller;html, body { height: 100% }+body { overflow-x: hidden };body { height: 100vh; overflow-y: auto }— each non-scrolling and scrolling at offset 300): non-scrolling — layout viewport 1280 → 1280, no undimmed strip, no padding,<body>untouched; scrolling (15px scrollbar before) —<body>width 1265 → 1265 (gutter held on the root in both lock modes),scrollY300 → 300 through wheel and after close,<body>untouched. WithCSS.supports('scrollbar-gutter')forced false and the property neutralised (html { scrollbar-gutter: auto !important }, simulating Safari 17.0–18.1):<body>padded15pxinline while the sheet is open and''after close, on every scrolling shape; nothing on non-scrolling ones. An unconditionalscrollbar-gutter: stableon a non-scrolling page shows the strip (255 vs 127) and the sheet at 632.5. WebKit under Playwright has overlay scrollbars (0px) and so cannot show the gutter case.mainin both engines; settled screenshots pixel-identical (36 of 36 pairs); no console errors or warnings.iOS check (owner-owed)
spec:AST-013FR10 asks for the named browser. iOS Safari's handling of a frozen scroller (overflow: hiddenon the root, or on<body>when it owns the viewport scroll) against a finger could not be measured here: desktop WebKit under Playwright cannot be driven by native touch and is not iOS.component:BottomSheetFR5 (#7744) names that device check as owed. Steps, on an iPhone running the oldest supported iOS Safari and the current one, against this branch's static Storybook served over the LAN:iframe.html?id=core-bottomsheetswitcher--hug-content&viewMode=story; dismiss the flow (tap the scrim). In the Web Inspector console rundocument.body.appendChild(Object.assign(document.createElement('div'), {style: 'height:3000px'})); scrollTo(0, 300).scrollYstays 300, the sheet's content scrolls and does not chain. Rubber-banding at the viewport edge is acceptable; a page that scrolls is a failure.scrollYstill 300 after the exit motion, and the page scrolls again.html, body { height: 100% } body { overflow-x: hidden }to the page (step 1 console:document.head.appendChild(Object.assign(document.createElement('style'), {textContent: 'html,body{height:100%}body{overflow-x:hidden}'}))).iframe.html?id=core-bottomsheet--showcase&viewMode=story(a standalone sheet) and with the phone rotated.If any step fails, this PR is not merged: nothing else depends on it, and the sheets keep
useScrollLock.Scope
Testing