Repository navigation
fix(useScrollLock): keep the page where it is while an overlay is open - #8098
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
needs:design-review
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
The new lock keeps the page at its offset under a Dialog on an ordinary page. However, it now shares the root's overflow with MobileNav and changes how pages that style <body> scroll, so this needs changes before it can land. Please fix the following, each with a regression test:
MobileNavstill writesdocument.documentElement.style.overflowdirectly, so a BottomSheet opened from an open MobileNav and closed together with it leaves the root atoverflow: clipand the page unscrollable, and closing or unmounting MobileNav drops another overlay's lock; moving MobileNav ontoclaimRootScrollStylewould fix both.- On a page styled
html, body { height: 100% }andbody { overflow-x: hidden }, opening a Dialog moves the page to the top and it stays there after close, because the root'soverflow: hiddenstops<body>'s overflow from propagating to the viewport. - The touch guard starts its walk from
event.target, which adocumentlistener sees as the shadow host, so pans inside a scroller in a shadow root are cancelled; walkingevent.composedPath()fixes it.
[Automated review]
PR Analysis Report🔎 Preview ·
|
|
Posted by Cindy's agent on her behalf. Parts of this PR re-implement behavior that already exists, or is being specified, as a shared part of core. Converging keeps one implementation per behavior:
This is about where the behavior lives, not about the PR's intent. |
|
Agreed: one implementation.
|
There was a problem hiding this comment.
The new lock keeps the page at its offset on ordinary pages and on pages whose <body> sets its own overflow, and the shadow-root and stuck-clip cases are fixed. One regression remains: MobileNav still writes document.documentElement.style.overflow itself, so when it closes or unmounts while another overlay is open, for example on a click that closes it and opens a Dialog rendered before it, it clears the lock and lets wheel and keyboard scroll the page behind that overlay, which main prevents today. Please move MobileNav onto claimRootScrollStyle in this PR, or land that part of #8119 with it, with tests for a close and an unmount while another lock holds.
- The touch guard fixes a pan's axis from its first
touchmove, so a horizontal swipe that starts with a tie or a vertical lean (for example 2 px by 2 px) is cancelled even when the strip under the finger can scroll; deciding after a small slop would avoid that.
[Automated review]
…held while a Dialog is open; URL text filled)
6eee897 to
0455794
Compare
|
Done in 55ad273: the guard's own document listeners are gone. useScrollLock holds the page through the gesture host's |
github-actions
Bot
removed
the
needs:design-review
|
Folded in the four points: MobileNav FR3 has its clip follow the page's scroller (the root, or AlertDialog's layout tests stub the page hold, whose pointer read is the lock's, so their guard still sees only the layout's queries. |
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the three current-record amendments this change makes: MobileNav FR3 would have the drawer clip <body> instead of the root on pages whose <body> sets its own overflow, and BottomSheetSwitcher FR5 and Drawer FR5 would add that pull-to-refresh and the page's bounce stop behind a modal overlay, which current spec does not state. An engineering owner records that decision by commenting /approve-spec 4721503acd64d4a362cfe8c8a027036f5763a499 on this pull request (the author may do so as an engineering owner), or asks for the root clip to stay and pull-to-refresh to be left alone. Whatever is decided, before approval the change still needs:
- a rebase onto a landed #8360, so the diff carries only the scroll-lock commit;
- real-device recordings for the iOS Safari and Android Chrome claims (an iPhone below iOS 26.4 dragging the scrim right after an overlay opens, a drag that becomes a pinch, and an Android phone pulling down at the top), which AST-094's platform support asks for.
[Automated review]
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the three current-record amendments this change makes: MobileNav FR3 would have the drawer clip <body> instead of the root on pages whose <body> sets its own overflow, and BottomSheetSwitcher FR5 and Drawer FR5 would add that pull-to-refresh and the page's bounce stop behind a modal overlay, which current spec does not state. An engineering owner records that decision by commenting /approve-spec 423aee10cdc73339f4d081587540632fdc6e1700 on this pull request (the author may do so as an engineering owner), or asks for the root clip to stay and pull-to-refresh to be left alone. Whatever is decided, before approval the change still needs:
- a rebase onto a landed #8360, so the diff carries only the scroll-lock commit and merges cleanly again (it now conflicts with the base in
packages/core/src/gestures/GestureHost.ts); - real-device recordings for the iOS Safari and Android Chrome claims (an iPhone below iOS 26.4 dragging the scrim right after an overlay opens, a drag that becomes a pinch, and an Android phone pulling down at the top), which AST-094's platform support asks for.
[Automated review]
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the three current-record amendments this change makes: MobileNav FR3 would have the drawer clip <body> instead of the root on pages whose <body> sets its own overflow, and BottomSheetSwitcher FR5 and Drawer FR5 would add that pull-to-refresh and the page's bounce stop behind a modal overlay, which current spec does not state. An engineering owner records that decision by commenting /approve-spec 3e964de1701e0abd7cdae882ea4688957bc55a08 on this pull request (the author may do so as an engineering owner), or asks for the root clip to stay and pull-to-refresh to be left alone. Whatever is decided, before approval the change still needs:
- a rebase onto a landed #8360, so the diff carries only the scroll-lock commit;
- real-device recordings for the iOS Safari and Android Chrome claims (an iPhone below iOS 26.4 dragging the scrim right after an overlay opens, a drag that becomes a pinch, and an Android phone pulling down at the top), which AST-094's platform support asks for.
[Automated review]
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the three current-record amendments this change makes: MobileNav FR3 would have the drawer clip <body> instead of the root on pages whose <body> sets its own overflow, and BottomSheetSwitcher FR5 and Drawer FR5 would add that pull-to-refresh and the page's bounce stop behind a modal overlay, which current spec does not state. An engineering owner records that decision by commenting /approve-spec 9821890c91955bbe26f7ab6642e811ea95903bfd on this pull request (the author may do so as an engineering owner), or asks for the root clip to stay and pull-to-refresh to be left alone. Whatever is decided, before approval the change still needs:
- a rebase onto a landed #8360, so the diff carries only the scroll-lock commit;
- real-device recordings for the iOS Safari and Android Chrome claims (an iPhone below iOS 26.4 dragging the scrim right after an overlay opens, a drag that becomes a pinch, and an Android phone pulling down at the top), which AST-094's platform support asks for.
[Automated review]
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on five current-record edits at this head: MobileNav FR3 (the drawer clips <body> on pages whose <body> sets its own overflow), BottomSheetSwitcher FR5 and Drawer FR5 (pull-to-refresh and the page's bounce stop behind a modal overlay), and the carried #8360 additions to AST-094 FR11 and FR19 and react-component-runtime INV13; an engineering owner records it by commenting /approve-spec 1e22e47861575b299c184427d5261b5e540d20fc on this pull request, which the author may do as an engineering owner. Whatever is decided, these changes are still needed:
- Land #8360 and rebase, so this pull request carries only the scroll-lock commit.
- Move MobileNav onto
useScrollLock, or say why it cannot use it: its own copy of the lock gets neither the page hold nor the overscroll stop. - Reword the changeset's MobileNav sentence, since on the released version closing MobileNav never released another overlay's lock.
- Add real iPhone and Android recordings for the iOS Safari and Android Chrome claims, as AST-094 asks.
[Automated review]
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on five current-record edits at this head: MobileNav FR3 (the drawer clips <body> on pages whose <body> sets its own overflow), BottomSheetSwitcher FR5 and Drawer FR5 (pull-to-refresh and the page's bounce stop behind a modal overlay), and the carried #8360 additions to AST-094 FR11 and FR19 and react-component-runtime INV13. An engineering owner records it by commenting /approve-spec 5adb879f877d1963740a0e0a313716db3f32ee61 on this pull request, which the author may do as an engineering owner. Whatever is decided, these changes are still needed:
- Land #8360 and rebase, so this pull request carries only the scroll-lock commit.
- Move MobileNav onto
useScrollLock, or say why it cannot use it: its own copy of the lock gets neither the page hold nor the overscroll stop. - Reword the changeset's MobileNav sentence, since on the released version closing MobileNav never released another overlay's lock.
- Add real iPhone and Android recordings for the iOS Safari and Android Chrome claims, as AST-094 asks.
[Automated review]
There was a problem hiding this comment.
Approval of this pull request waits on an engineering owner: it amends current MobileNav FR3 (the drawer clips <body> on pages whose <body> sets its own overflow), BottomSheetSwitcher FR5 and Drawer FR5 (pull-to-refresh and the page's bounce stop behind a modal overlay), and the carried #8360 commits add a MUST to AST-094 FR11, a MAY to FR19 and new INV13 text; an engineering owner records the decision by commenting /approve-spec 4b839fd4a06df9ec991e7ad4099136bd0ae90103 on the pull request, and the author may do so as an engineering owner. Whatever that decision, these still need changes before approval:
- Land #8360 and rebase, so this pull request carries only its own scroll-lock commit.
- Have MobileNav call
useScrollLockinstead of keeping its own gutter hold and root claim (MobileNav.tsx:583-619), or say why the shared lock cannot serve it. - Reword the changeset's last sentence: the released MobileNav never released another overlay's lock.
- Add real-device recordings (an iPhone below iOS 26.4 and an Android phone) for the iOS Safari and Android Chrome claims, as AST-094 Platform support asks.
[Automated review]
There was a problem hiding this comment.
Approval of this pull request waits on an engineering owner: it amends current MobileNav FR3 so the drawer clips <body> on pages whose <body> sets its own overflow, and it adds a pull-to-refresh and bounce stop to BottomSheetSwitcher FR5 and Drawer FR5 that no current spec states (MobileNav PR1 should follow whatever FR3 says). An engineering owner records that decision by commenting /approve-spec d8a610632ded3d8fe360a3de07315ab2937e7100 on the pull request (the author may do so as an engineering owner), or asks for MobileNav to keep clipping the root and the lock to leave pull-to-refresh alone. Two fixes are needed either way:
- The changeset's last sentence says MobileNav no longer releases another overlay's lock, but the released lock's
<body>pin was never touched by MobileNav, and the part it does release, the lock's scrollbar gutter, it still releases under an open lock, leavingscrollbar-gutter: stableon the root afterwards; reword it or route the gutter through the shared lock. spec:AST-094's platform section asks for physical-device evidence for the iOS Safari and Android Chrome claims, which are recorded only on a simulator and an emulator.
[Automated review]
|
/approve-spec 22f1909 |
|
Threads at 22f1909:
The fixed, answered and moot threads are resolved; the owner-decision threads stay open. |
There was a problem hiding this comment.
This needs an engineering owner's decision and a few changes before it can be approved. The scroll-lock fix itself keeps the page in place as the records ask, and the MobileNav, BottomSheetSwitcher and Drawer record edits are approved at this head.
The open decision: the pull-to-refresh and bounce stop also reaches Dialog, AlertDialog, Lightbox, a standalone modal BottomSheet and anyone calling useScrollLock directly, but none of their records states it. Approval needs an engineering owner to provide or identify the record that covers it (for example one sentence in the touch-presentations spec's FR19, or in each component record, recorded by commenting /approve-spec followed by the full head SHA; the author may do this as an engineering owner), or to say that the approved BottomSheetSwitcher and Drawer wording covers those overlays.
Required either way:
- MobileNav still builds its own page lock beside
useScrollLock, and this change copies the overscroll stop into it without the one-finger hold the other modals get, so please have MobileNav calluseScrollLock(isOpen)and remove its own claim and gutter hold (MobileNav.tsx:589-610). - The changeset, the hook's JSDoc and its doc say the hold works "on touch screens", but it runs only where the primary pointer is coarse, as the new test pins, so please say "where the primary pointer is coarse (phones and tablets)".
- The iOS Safari and Android Chrome claims rest on the simulator and the emulator, and the touch-presentations spec asks for real-device evidence for them, so please attach recordings from a physical iPhone and Android phone.
- The branch conflicts with main in
docs/specs/components/Drawer/Drawer.spec.mdandscripts/touch-support-bundle.test.mjs, so please rebase (the new head will need a fresh/approve-spec).
[Automated review]
|
/approve-spec bb54740 |
|
At bb54740:
|
There was a problem hiding this comment.
The scroll-lock change is close: MobileNav now uses the shared useScrollLock, the changeset names the coarse pointer, and an engineering owner has approved the record edits at this head. Two things are needed before approval:
- Rebase onto
main:docs/specs/components/Drawer/Drawer.spec.mdanddocs/specs/components/MobileNav/MobileNav.spec.mdconflict with main's new Drawer FR20 and MobileNav FR13 rows (keep them), and the rebased head needs a fresh/approve-spec <full head SHA>from an engineering owner (the author may record it as one). - Attach physical-device recordings for the iOS Safari and Android Chrome claims (the toolbar staying put, the scrim drag holding, pull-to-refresh stopping), which
spec:AST-094Platform support requires beyond the simulator and emulator recordings.
[Automated review]
|
/approve-spec c1d9903 |
There was a problem hiding this comment.
Close, but two things need fixing before this can merge. The record edits are approved at this head, and the iOS claims have a real-iPhone pass.
- The changeset and the hook docs say the lock turns off pull-to-refresh in Chrome on Android, but the only checks are emulators, which also report no pull-to-refresh on
main; check on a real Android phone in Chrome onmainand on this branch, and drop that sentence ifmaindoes not reload either (spec:AST-094Platform support). - The branch now conflicts with
maininAlertDialog.test.tsx, and since #8083 AlertDialog reaches the gesture host through its own press tracking, so the AlertDialog case inuseScrollLockImports.test.tsfails once rebased; rebase, keep both test setups, and drop AlertDialog from that guard's list.
After the rebase, the new head needs its own /approve-spec from an engineering owner.
[Automated review]
| gutterRef.current = null; | ||
| } | ||
| }, []); | ||
| // The same page lock as every other modal. Its effect runs before the |
There was a problem hiding this comment.
Non-blocking (comment style): "The same page lock as every other modal." restates the call below. Keeping only the reason reads better: "This Effect runs before the open/close Effect below. So it measures the scrollbar gutter before showModal() can hide the scrollbar."
There was a problem hiding this comment.
Kept: the first sentence says why MobileNav takes the shared lock (it is the one every other modal takes), which the call alone doesn't say; the second gives the ordering it relies on.
| /** The root scroll styles a caller can hold. */ | ||
| export interface RootScrollStyle { | ||
| /** `hidden` or `clip` stops wheel, keyboard and scrollbar scrolling of the page. */ | ||
| overflow?: 'hidden' | 'clip'; |
There was a problem hiding this comment.
Non-blocking: no caller passes 'clip' now that MobileNav takes useScrollLock, so overflow could be 'hidden' only.
There was a problem hiding this comment.
Kept: the claim stack is per value, and its latest-wins and restore-order tests need two overflow values; with 'hidden' alone that path could only be tested through a cast.
|
The real-device evidence is now in the description, under Evidence: the owner's hand pass on a real iPhone at c1d9903 (Dialog keeps the page and Safari's toolbar put, the BottomSheet backdrop drag holds the page, the Drawer's top pull doesn't bounce, the MobileNav strip drag holds the page), which the spec owners accept for AST-094 Platform support. The Android pull-to-refresh line follows with the CI owners' emulator check. |
There was a problem hiding this comment.
Approval waits on an engineering owner: this head rebases the previously approved head onto main, so the five record edits (spec:AST-094 FR19's overscroll sentence, MobileNav FR3 and PR1, BottomSheetSwitcher FR5 and Drawer FR5) need a fresh decision, recorded by commenting /approve-spec 3bba29a0936342a11b4b5aff443f05b01bc919c4 on this pull request. The author may record it as an engineering owner. One fix is still needed after that:
- The changeset and the
useScrollLockdocs say the lock turns off pull-to-refresh in Chrome on Android, but the only checks are emulators, which also report no pull-to-refresh onmain; check on a real Android phone in Chrome onmainand on this branch, and drop that sentence ifmaindoes not reload either (spec:AST-094Platform support).
The rebase cleared the earlier AlertDialog test conflict and import-guard case.
[Automated review]
The scroll lock no longer pins <body>, which reset the page's offset to 0 (the page jumped and Safari's toolbar came back). It holds `overflow: hidden` on the page's scroller and, through the gesture host's page hold, cancels only a one-finger pan that nothing inside the overlay takes: a scroller, a range input, a pinch or a zoomed page's pan still goes through. The root's scroll styles become claim stacks (rootScrollStyles), so a lock and a pull to refresh compose, and MobileNav's clip moves onto them, with the same stop of pull-to-refresh and the page's bounce. The scrollbar gutter is a claim on the same stacks, so closing MobileNav under an open modal keeps the gutter the modal still needs, and the page's own gutter comes back after both. The top-layer check asks CSS.supports for `:modal` and `:popover-open` instead of catching errors from `matches`. The touch module bundle test names useScrollLock and the imports that reach it as carrying the page hold. The records state the clip on the page's scroller (MobileNav FR3 and PR1) and that pull-to-refresh and the page's bounce stop behind a modal overlay (BottomSheetSwitcher FR5, Drawer FR5, MobileNav FR3), since a scroll lock under a modal holds the page. AlertDialog's layout tests stub the page hold, whose pointer read belongs to the lock, so their guard still sees only the layout's own media queries.
|
/approve-spec f03f66e |
There was a problem hiding this comment.
Recommend approval. The five record edits (spec:AST-094 FR19's overscroll sentence, MobileNav FR3 and PR1, BottomSheetSwitcher FR5 and Drawer FR5) are approved by an engineering owner at this head, and the scroll lock now keeps the page at its offset under every modal overlay, with unit tests, browser checks and a real-iPhone pass behind it.
This head also settles both points from the last review: the changeset and the useScrollLock docs no longer claim an Android pull-to-refresh change, and the touch-support bundle list names each import once.
Optional follow-ups (non-blocking):
architecture:react-component-runtimeINV16 still lists MobileNav's own page scroll lock as a known gap; this change closes it.- The
holdScrollbarGutterJSDoc example still writes the root'soverflowdirectly;claimRootScrollStyle({overflow: 'hidden'})is the supported path now. - No record states that a one-finger drag on a pinch-zoomed page goes through while the lock holds; one clause in AST-094 FR19 would keep that behavior from being dropped later.
[Automated review]
| * const hold = holdScrollbarGutter(document.body); | ||
| * body.style.position = 'fixed'; | ||
| * const hold = holdScrollbarGutter(document.documentElement); | ||
| * document.documentElement.style.overflow = 'hidden'; |
There was a problem hiding this comment.
Non-blocking: this example writes the root's overflow directly, which rootScrollStyles now owns. const release = claimRootScrollStyle({overflow: 'hidden'}); would show the supported path.
Behavior-module PR: scroll lock, now MobileNav's too.
Why
Opening a Dialog, BottomSheet, Drawer or Lightbox on a scrolled page moved the page: the lock pinned
<body>, soscrollYread 0, the page jumped and Safari's toolbar came back.What led to it
The pin exists because iOS Safari scrolls an
overflow: hiddenpage under a finger.spec:AST-094FR19 lets a modal lock cancel a lone finger's page pan.Options and tradeoffs
overflow: hiddenand cancel the one-finger pans that would reach the page: keeps the offset and toolbar; those pans wait on script.Why this choice
The second keeps the page still, and the overlay's scrollers, sliders and pinch still get their pans. AST-017 FR3 covers function and semantics, not the pin. AST-094 FR19 gains one sentence: a modal overlay holding the page scroll lock also stops pull-to-refresh and the overscroll bounce. No API change. MobileNav takes the same lock, so its drawer's strip holds too.
How to check it
On a phone, scroll and open a Dialog: the page and Safari's toolbar stay put, a scrim drag holds the page, the dialog's text still scrolls.
Evidence
iOS 26.2 simulator, main and this PR:
Writing rubric results