Repository navigation
test(MobileNav): a native close of the drawer releases the page lock - #8119
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.
This pull request is on hold for an owner decision. It carries #8064's MobileNav close behavior, which blocks every tap until the slide-out ends and returns focus only then, and no current spec decides that behavior; an engineering owner needs to approve or reject it by commenting /approve-spec <full head SHA> on this pull request, and as an engineering owner you may record that yourself. Whatever the decision, the following still need fixing, each with a regression test:
- On a page styled
html, body { height: 100% }withbody { overflow-x: hidden }, opening a Dialog now jumps the page to the top and leaves it 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; starting fromevent.composedPath()[0]fixes it. - The MobileNav fix targets iOS Safari, so it needs an iOS Safari run on this head showing that a drag on the open drawer leaves the page still while the drawer's list still scrolls.
- The
mobile-nav-shared-scroll-lockchangeset says a pull-to-refresh control sees the lock, but no such control is part of this change.
[Automated review]
|
|
||
| // Taken before the root's overflow hides the scrollbar. | ||
| const gutter = holdScrollbarGutter(document.documentElement); | ||
| const releaseRootStyle = claimRootScrollStyle({overflow: 'hidden'}); |
There was a problem hiding this comment.
On a page styled html, body { height: 100% } with body { overflow-x: hidden }, <body>'s overflow propagates to the viewport only while the root's overflow is visible. This claim stops that, so <body> becomes its own 100%-tall scroller and the viewport offset clamps to 0. Releasing the claim does not bring the offset back. In Chromium a Dialog on such a page moved scrollY 600 → 0 and left it at 0 after close, where main restores 600. Detecting that case (root overflow visible, <body>'s computed overflow not visible) and claiming <body>, or saving and restoring the offset, would keep the page where it is.
| return; | ||
| } | ||
| // The browser picks the scroller once per gesture; so does the guard. | ||
| verdict = isPanAbsorbedBeforePage(event.target, dx, dy) |
There was a problem hiding this comment.
This listener sits on document, where event.target for a touch inside a shadow root is retargeted to the shadow host. The walk therefore never reaches a scroller inside the shadow tree, and the pan is cancelled. In Chromium an 80 px swipe moved a shadow-root scroller 0 px, against 76 px for a light-DOM scroller in the same open Dialog. Starting from event.composedPath()[0], or walking composedPath(), would let those scrollers take the pan.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] MobileNav locks the page through the shared scroll lock (`useScrollLock`) while it is open and until its slide-out ends. On iOS a drag on the open drawer no longer scrolls the page behind it, and other overlays and the pull-to-refresh control see the lock. |
There was a problem hiding this comment.
No pull-to-refresh control exists in this change or tree, and subscribeScrollLock has no production subscriber, so "and the pull-to-refresh control see the lock" promises something users will not find. Dropping that clause keeps the note accurate.
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| MobileNavToggle | 46.1 kB (0) · 9.8 kB CSS (0) | — | — · ✓ · ✗ |
| MobileNav | 52.5 kB (0) · 10.4 kB CSS (0) | — | — · ✓ · ✓ |
Package size: no published package changed; no new dependencies.
API changes: none in this pull request
Committed API report behind this build: core 2 rows; run pnpm api:report.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
c110de2 to
62fcf7d
Compare
…rag with the backdrop blur off for capture, and a blur-on still
There was a problem hiding this comment.
This pull request is on hold for an engineering owner's decision. The draft spec:AST-094 it adds is the only record behind the experimental gesture and motion subpaths and the theme motion roles in this diff, so an engineering owner needs to approve or reject it by commenting /approve-spec with this pull request's full head SHA; as an engineering owner, you may record that decision yourself. Whatever the decision, the following need fixing:
- The diff carries the whole open stack under #8098 (about 34,000 authored lines), so land those pull requests first and rebase until this one holds only its 96-line MobileNav commit.
- The stable press controller (
pressFeedback.ts) now turns off text selection and the touch callout under every touch press but ships under an[experimental]changeset, so keep the lock to the experimental press tracker or release it as a stable change. @astryxdesign/core/motion/reducedMotion.stylexis a new stable export outside the experimental boundary, and the theme template documents aplatformadaptation condition thatdefineThemerejects.- In
spec:AST-094, FR8's 0.5 s Reduce Motion cross-fade conflicts with the current 200 ms rule, FR4 says touch physics are not themable while this diff adds theme motion roles, and FR7 cites a spec that does not exist; three changesets also misdescribe what this head ships.
[Automated review]
| @@ -283,6 +301,9 @@ function onPointerDown(event: PointerEvent): void { | |||
| gesture: idlePressGesture(), | |||
| pointerId: event.pointerId, | |||
| timer: null, | |||
| // A held finger never starts text selection on the surface (UIKit's | |||
| // controls and cells never do), on any coarse pointer. | |||
| unlock: lockSelection([element], event.target), | |||
There was a problem hiding this comment.
This lock now runs for every touch or pen press on every usePressFeedback surface (Button, Link, Switch, Tab, Item and 26 more), writing user-select, -webkit-user-select and -webkit-touch-callout as none until the lift. That is a stable runtime change, which spec:AST-017 FR22 keeps out of an [experimental] changeset, and on iOS it should also hide the link callout on every Link. Keeping the lock to the experimental press tracker and recognizers with preventsTextSelection would match the changeset.
| * are primary-pointer precision, contrast preference, motion preference, and | ||
| * platform (`ios` for the browsers of iPhone and iPad, `other` for the rest). |
There was a problem hiding this comment.
defineTheme has no platform condition (CONDITION_AXES is width, pointer, contrast and motion), and when: {platform: 'ios'} throws "… is not supported". spec:AST-012 FR2 closes the vocabulary, so this sentence should go back to "primary-pointer precision, contrast preference, and motion preference".
| @@ -177,6 +190,13 @@ const STATIC_EXPORTS = { | |||
| types: './dist/utils/interactionOverlay.stylex.d.ts', | |||
| default: './dist/utils/interactionOverlay.stylex.js', | |||
| }, | |||
| // Proposed; pending owner review (spec:AST-094 OQ4): the Reduce Motion | |||
There was a problem hiding this comment.
spec:AST-094 has only OQ1 and OQ2, and this subpath ships with no @experimental marker, so once released it is a stable export (spec:AST-017 FR22; FR27 asks for experimental exports and names for CSS and tokens). Exporting these constants from motion/experimental, or holding the export until an owner approves it, keeps it inside the boundary.
| cross-fades over 0.5 s on the ease-in-out curve; a sheet, a full-screen | ||
| modal or an alert fades on its own presentation spring and drops its travel | ||
| and scale; menus open and close on UIKit's Reduce Motion spring. Motion that |
There was a problem hiding this comment.
The current design:reduced-motion DR2 says a Reduce Motion transition "MUST change opacity only and MUST finish in 200 ms or less", and this head's animator uses 200 ms (motion/uikit/timing.ts:151). A 0.5 s push or pop and a spring-timed sheet fade contradict it, and the Current-state impact does not name that record.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] `useScrollLock`, and so Dialog, BottomSheet, Drawer and Lightbox, keeps the page where it is while an overlay is open. The lock used to pin `<body>` with `position: fixed` at minus the scroll offset and scroll back on release: the page's offset read 0 for the whole lock, and on iOS Safari the jump to the top brought the toolbar back, where it stayed after the overlay closed. Now the page gets `overflow: hidden` (on the root, or on `<body>` when the body's own overflow is the one the viewport uses), and on touch screens a one-finger pan that would scroll the page is cancelled, which iOS Safari before 26.4 needs because it scrolls an `overflow: hidden` page under a finger. Pans that a scroller inside the overlay can take, and pinch-zoom, still work. The lock no longer pins `<body>`, and `window.scrollY` keeps its value while the lock holds. Pans are judged through the shared gesture host, after a small slop so a swipe's first wobble does not fix the wrong axis. MobileNav's own clip goes through the same page scroll-style claims, so closing or unmounting it no longer releases another overlay's lock. |
There was a problem hiding this comment.
At this head MobileNav has no clip of its own; it calls useScrollLock (see mobile-nav-shared-scroll-lock.md, released with this note). The note also leaves out that the lock writes overscroll-behavior-y: none on the root and <body>, which turns off pull-to-refresh while an overlay is open.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] `presentation="adaptive"` on DropdownMenu, ContextMenu, MoreMenu, Selector and MultiSelector, and a DropdownMenu's default sub-menu drill-in, now use the theme's compact definition with a coarse primary pointer: a viewport narrower than the theme's `md` point (768px by default) or shorter than 500px. This is a behavior change. The old default was a bottom sheet at 768px wide and below on a coarse pointer. Now a coarse-pointer screen exactly 768px wide keeps the anchored popover, and a phone in landscape (wider than 768px but shorter than 500px) gets the bottom sheet and the drilled-in sub-menu. To pin a component's presentation regardless of the rule, pass `presentation="popover"` or `presentation="bottom-sheet"`. Mouse and keyboard users on desktop see no change, because every case requires a coarse pointer. |
There was a problem hiding this comment.
COMPACT_TOUCH_QUERY is built from DEFAULT_COMPACT_DEFINITION (touchPresentation.ts:71), so a theme that overrides md does not move this edge. Either say 768px, or make the resolver read the theme's effective md as spec:AST-012 FR9 describes.
62fcf7d to
edc6cee
Compare
…ock) as a dependency
There was a problem hiding this comment.
This pull request is on hold for an engineering owner's decision: alongside the MobileNav fix, its compare view ships stable additions that no current spec decides (defineTheme touch motion roles, the press-feedback selection lock for touch and pen, and the astryxStylex undefinedVariables option) and the internal gesture and motion system behind them. An engineering owner needs to approve or reject them by commenting /approve-spec edc6cee7bf1e515195ab93322157d722ca2d0fe0 on the pull request; as an engineering owner, the author may record that decision too. Whatever the decision, these need changes before approval:
- Land or drop the #8098 stack first and rebase, so this pull request carries only its 96-line MobileNav commit instead of 34,611 authored lines.
- Keep the gesture host out of
useScrollLock's static imports, since CI's size report shows Dialog, BottomSheet, Drawer, Lightbox, MobileNav and 18 other components each growing by about 17 KB gzip for a touch-only page hold (architecture:component-style-authoringINV6). - Limit the press selection lock to touch or show pen evidence (
spec:AST-017FR59), and release it and the new build option under accurate changeset tags. - Correct
scroll-lock-keeps-the-page.md, which still describes a MobileNav clip, and makereduced-motion-record.mdan internal[chore]note.
[Automated review]
| */ | ||
|
|
||
| import {useEffect} from 'react'; | ||
| import {claimRootScrollStyle, reportScrollLock} from './rootScrollStyles'; | ||
| import {getGestureHost} from '../gestures/GestureHost'; |
There was a problem hiding this comment.
This static import puts the whole gesture host (arena, pan and tap recognizers, the UIKit gesture table) into every component that reaches useScrollLock: CI's size report shows Dialog going from 13,787 to 30,964 gzip bytes and 22 other components growing by about 17 KB each. The page hold only needs document and isTouchClaimed, which answers false while no recognizer is attached. architecture:component-style-authoring INV6 asks a touch-only feature over 2 KB to load lazily or justify the cost; reading an already-created host from a small registry module, treating no host as nothing claimed, would keep the hold and drop the weight.
| @@ -283,6 +301,9 @@ function onPointerDown(event: PointerEvent): void { | |||
| gesture: idlePressGesture(), | |||
| pointerId: event.pointerId, | |||
| timer: null, | |||
| // A held finger never starts text selection on the surface (UIKit's | |||
| // controls and cells never do), on any coarse pointer. | |||
| unlock: lockSelection([element], event.target), | |||
There was a problem hiding this comment.
onPointerDown returns early only for mouse, so a pen takes this lock as a finger does. spec:AST-017 FR59 lets touch work change what fine-pointer users do only for a defect they hit or a named cross-device reason, with before-and-after evidence, and none is in this pull request. Limiting the lock to pointerType === 'touch', or naming the reason and showing pen evidence, would settle it; the [fix] label also needs a documented contract to restore.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] `useScrollLock`, and so Dialog, BottomSheet, Drawer and Lightbox, keeps the page where it is while an overlay is open. The lock used to pin `<body>` with `position: fixed` at minus the scroll offset and scroll back on release: the page's offset read 0 for the whole lock, and on iOS Safari the jump to the top brought the toolbar back, where it stayed after the overlay closed. Now the page gets `overflow: hidden` (on the root, or on `<body>` when the body's own overflow is the one the viewport uses), and on touch screens a one-finger pan that would scroll the page is cancelled, which iOS Safari before 26.4 needs because it scrolls an `overflow: hidden` page under a finger. Pans that a scroller inside the overlay can take, and pinch-zoom, still work. The lock no longer pins `<body>`, and `window.scrollY` keeps its value while the lock holds. Pans are judged through the shared gesture host, after a small slop so a swipe's first wobble does not fix the wrong axis. MobileNav's own clip goes through the same page scroll-style claims, so closing or unmounting it no longer releases another overlay's lock. |
There was a problem hiding this comment.
At this head MobileNav has no clip of its own; it calls useScrollLock (see mobile-nav-shared-scroll-lock.md, released with this note), so the last sentence describes code that is gone. The note also leaves out that the lock writes overscroll-behavior-y: none on the root and <body>, which turns off Chrome on Android's pull-to-refresh while an overlay is open.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] The experimental touch animator follows design:reduced-motion. Under Reduce Motion every `animate` and `play` (travel or opacity, on a spring or any curve) becomes an opacity-only cross-fade of 200 ms on a coarse primary pointer, or for a call marked `touchRelease`, and completes at once on any other pointer; nothing moves, scales or springs, and travel lands with a plain style write while the element is transparent, never as an animated transform. `timing.reducedMotionCrossFadeMs` is now 200 ms and is no longer a theme role. The `reducedMotion` and `crossfade` call options are removed; `crossfadeAs` stays. `animator.reducedMotion()` reports the mode for feedback with no fade of its own, `reducedMotionFade()` gives the same fade to code beside the animator, and `reducedMotion.stylex.ts` holds its constants for StyleX transitions. |
There was a problem hiding this comment.
This head exports no touch animator (no motion entry in packages/core/package.json), so a [fix] note about its options tells consumers about an API they cannot import. Its siblings use [chore] Internal: …; this one should too.
| '@astryxdesign/build': patch | ||
| --- | ||
|
|
||
| [fix] `astryxStylex` resolves `defineConsts` values read from another file. With the `{stylexOptions}` form (the one Storybook uses), a file that read only constants from a `.stylex.ts` module lost that import once StyleX compiled it, so the constants never reached the CSS: a constant used as a media-query key failed the build ("Invalid empty selector") and one used as a value shipped as a custom property nothing declares. A constant defined on the library side of the layer split and read on the product side stayed unresolved in either form. A production build now also warns, naming the variable and the rule that reads it, when its stylesheet reads a StyleX-generated custom property that nothing declares; set `undefinedVariables: 'error'` to fail the build instead. |
There was a problem hiding this comment.
The cross-file defineConsts fix fits [fix], but the new undefinedVariables option and its default production warning are a compatible capability, which spec:AST-017 FR7 releases as [feat]. A separate changeset for the option would keep each category accurate.
There was a problem hiding this comment.
Changes requested. Switching MobileNav to the shared useScrollLock is small and matches the current MobileNav and touch-presentation requirements, but this pull request still carries the open #8098, #8320 and #8076 stack, about 29,000 authored lines beside MobileNav's 217. Please merge it after that stack lands and rebase so only the MobileNav files remain.
scripts/touch-support-bundle.test.mjscurrently fails its root-entry and hooks-entry cases becauseuseScrollLocknow importsgestures/pageHold; the head that merges needs that test's replacement or a lock that keeps the page hold out of those entries.
[Automated review]
284dbd5 to
f40ce29
Compare
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
This pull request is on hold for an engineering owner's decision, and it also needs two changes whatever that decision is.
Approval needs an engineering owner to approve the three component-record edits this head makes, because no existing spec decides them:
component:DrawerFR5 andcomponent:BottomSheetSwitcherFR5 now say pull-to-refresh and the page's bounce stop behind a modal (overscroll-behavior-y: none), which the citedspec:AST-094FR19 does not cover, since it only lets a modal lock cancel one-finger pans.component:MobileNavFR3 moves the drawer's clip from the page's root to<body>when<body>sets its own overflow.
An engineering owner records the decision by commenting /approve-spec f40ce2995f5f0c9811bf18bfc67e010e70e0a177 on this pull request; the author may do so if they are an engineering owner.
Required changes:
- 2,156 of the 2,224 changed lines are #8098's
useScrollLockrewrite, and the branch conflicts withmain, so land #8098 first and rebase until only the MobileNav commit remains (merging bothGESTURE_USERSlists inscripts/touch-support-bundle.test.mjs). .changeset/scroll-lock-keeps-the-page.mdstill says "MobileNav's own clip goes through the same page scroll-style claims", but MobileNav has no clip of its own at this head (it callsuseScrollLock), so drop that sentence.
The MobileNav switch itself is small, its unit suites pass, and on a full-height page it keeps the scroll offset that main loses.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] `useScrollLock`, and so Dialog, BottomSheet, Drawer and Lightbox, keeps the page where it is while an overlay is open. The lock used to pin `<body>` with `position: fixed` at minus the scroll offset and scroll back on release: the page's offset read 0 for the whole lock, and on iOS Safari the jump to the top brought the toolbar back, where it stayed after the overlay closed. Now the page gets `overflow: hidden` (on the root, or on `<body>` when the body's own overflow is the one the viewport uses), and on touch screens a one-finger pan that would scroll the page is cancelled, which iOS Safari before 26.4 needs because it scrolls an `overflow: hidden` page under a finger. Pans that a scroller inside the overlay can take, drags on a native range input, pinch-zoom, and one-finger drags on a pinch-zoomed page (they pan the zoomed view) still work. The lock no longer pins `<body>`, and `window.scrollY` keeps its value while the lock holds. The lock cancels those pans itself, leaving a touch a gesture recognizer owns to it, and judges each pan after a small slop so a swipe's first wobble does not fix the wrong axis. It follows the browser's scroll chain, so an overlay rendered inside a scrolling container still holds the page: a modal dialog's pans go to the page, not to that container. It also sets `overscroll-behavior-y: none` on the root and `<body>`, which turns off pull-to-refresh (Chrome on Android) while an overlay is open. MobileNav's own clip goes through the same page scroll-style claims, so closing or unmounting it no longer releases another overlay's lock. |
There was a problem hiding this comment.
The last sentence says "MobileNav's own clip goes through the same page scroll-style claims", but at this head MobileNav has no clip of its own: it calls useScrollLock(isOpen) (MobileNav.tsx:521), as mobile-nav-shared-scroll-lock.md says. Please drop that sentence here, so the release notes describe MobileNav once.
There was a problem hiding this comment.
Dropped at 36c3088: the release notes now describe MobileNav once, in mobile-nav-shared-scroll-lock.md.
f40ce29 to
36c3088
Compare
There was a problem hiding this comment.
This pull request is on hold for an engineering owner's decision, and it also needs two changes whatever that decision is.
Approval needs an engineering owner to approve the three component-record edits this head makes, because no existing spec decides them:
component:DrawerFR5 andcomponent:BottomSheetSwitcherFR5 now say pull-to-refresh and the page's bounce stop behind a modal (overscroll-behavior-y: none), which the citedspec:AST-094FR19 does not cover, since it only lets a modal lock cancel one-finger pans.component:MobileNavFR3 moves the drawer's clip from the page's root to<body>when<body>sets its own overflow.
An engineering owner records the decision by commenting /approve-spec 36c308845072b775444751b187a03c60256ff930 on this pull request; the author may do so if they are an engineering owner.
Required changes:
- 2,156 of the 2,224 changed lines are #8098's
useScrollLockrewrite, and the branch conflicts withmaininDrawer.spec.md,scrollbarGutter.tsandscripts/touch-support-bundle.test.mjs, so land #8098 first and rebase until only the MobileNav commit remains, merging bothGESTURE_USERSlists. packages/core/src/hooks/scrollLockTouchGuard.ts:36-57copies the scroll-chain walk and thecanMoveX/canMoveYchecks ofcreateNativeScrollProbeinpackages/core/src/gestures/nativeScroll.ts, and the two already differ at modal dialogs andoverscroll-behaviorboundaries, so extend that probe with what the lock needs and call it instead of keeping a second copy.
The MobileNav switch itself is small, its unit and Chromium browser suites pass at this head, and on a full-height page it keeps the scroll offset that main loses.
[Automated review]
| const EDGE_TOLERANCE = 1; | ||
|
|
||
| function canMoveY(element: Element, dy: number, range: number): boolean { |
There was a problem hiding this comment.
EDGE_TOLERANCE, canMoveY and canMoveX here repeat packages/core/src/gestures/nativeScroll.ts (lines 77 and 189-212), and the walk below repeats createNativeScrollProbe's scroll chain. The two already differ (this one stops at a modal or fixed element and treats an overscroll-behavior boundary as taking the pan; the probe reads touch-action and walks past a modal), so a fix to one misses the other. Extending the probe with the chain end and a "reaches the page" answer, and calling it from isPanAbsorbedBeforePage, keeps one path; nativeScroll.ts imports nothing of the gesture host.
| * @input The element whose width a scroll lock holds, plus live layout | ||
| * measurements | ||
| * @output Exports holdScrollbarGutter and the ScrollbarGutterHold it returns | ||
| * @position Internal hook utility; used by useScrollLock and MobileNav so that |
There was a problem hiding this comment.
Non-blocking: MobileNav no longer imports this hold at this head (it calls useScrollLock), so this line can say "used by useScrollLock".
| * (gestures/pageHold) with the scroll lock's pan decision, and the | ||
| * scrollbar gutter hold | ||
| * @output Exports useScrollLock hook for locking page scroll behind a modal | ||
| * @position Core hook; used by Dialog, BottomSheet, Drawer and Lightbox to |
There was a problem hiding this comment.
Non-blocking: MobileNav now takes this lock too, so this list can read "used by Dialog, BottomSheet, Drawer, Lightbox and MobileNav".
36c3088 to
40a4ff7
Compare
vjeux
changed the title
There was a problem hiding this comment.
This pull request is on hold for an engineering owner's decision, and it also needs three changes whatever that decision is.
Required changes:
- 2,358 of the 2,385 changed lines are #8098's
useScrollLockcommit, so please land #8098 first and mergemain, leaving only the new MobileNav case in this diff. packages/core/src/hooks/scrollLockTouchGuard.ts:25-77copies the edge test thatcanElementScrollinpackages/core/src/gestures/nativeScroll.tsalready exports (and the copies already differ onoverflow: overlay), so please callcanElementScrollfromtakesPanand keep only theoverscroll-behaviorand chain-end logic in the guard.- The new case's
scrollbarGutterassertion (MobileNavCloseEdgeCases.test.tsx:332) cannot fail, because jsdom reports a 0 px viewport and the gutter is never held, so please stub a classic scrollbar asMobileNavScrollbarGutter.test.tsxdoes and assertstablewhile open, or describe the case as covering the overflow only.
Approval also needs an engineering owner to approve the record changes this head carries from #8098, because no existing spec decides them:
spec:AST-094FR19 gains a sentence saying a modal scroll lock stops the page's pull-to-refresh and overscroll bounce, andcomponent:DrawerFR5,component:BottomSheetSwitcherFR5 andcomponent:MobileNavFR3 now say the same.component:MobileNavFR3 and PR1 move the drawer's clip from the page's root to<body>when<body>sets its own overflow.
An engineering owner records the decision by commenting /approve-spec 40a4ff7e76ddafaa7486524f317eeabd23b2fe35 on this pull request; the author may do so if they are an engineering owner.
The new case itself is a useful guard: it passes on main and on this head, and on a full-height page the carried lock keeps the scroll offset that main loses.
[Automated review]
| }); | ||
|
|
||
| expect(document.documentElement.style.overflow).toBe(''); | ||
| expect(document.documentElement.style.scrollbarGutter).toBe(''); |
There was a problem hiding this comment.
This assertion cannot fail: jsdom reports documentElement.clientWidth as 0, so holdScrollbarGutter returns its no-op and scrollbarGutter is already '' while the drawer is open. Stubbing a classic scrollbar (as stubViewport(1024, 1009) with CSS.supports('scrollbar-gutter', 'stable') in MobileNavScrollbarGutter.test.tsx) and asserting stable before the close would make the gutter half of the case real.
There was a problem hiding this comment.
Fixed at 0a17209: the case stubs a classic scrollbar, as MobileNavScrollbarGutter.test.tsx does, and asserts stable while open and '' after the native close. A control without the lock's gutter release fails here.
| const SCROLLABLE = new Set(['auto', 'scroll']); | ||
|
|
||
| /** Overflow at or below this many px is a box that fits, not a scroller. */ | ||
| const EDGE_TOLERANCE = 1; | ||
|
|
||
| function canMoveY(element: Element, dy: number, range: number): boolean { | ||
| // A finger moving down drags the content down: scrollTop decreases. | ||
| return dy > 0 | ||
| ? element.scrollTop > EDGE_TOLERANCE - 0.5 | ||
| : element.scrollTop < range - EDGE_TOLERANCE + 0.5; | ||
| } | ||
|
|
||
| function canMoveX( | ||
| element: Element, | ||
| dx: number, | ||
| range: number, | ||
| isRtl: boolean, | ||
| ): boolean { | ||
| const min = isRtl ? -range : 0; | ||
| const max = isRtl ? 0 : range; | ||
| // A finger moving right drags the content right: scrollLeft decreases. | ||
| return dx > 0 | ||
| ? element.scrollLeft > min + EDGE_TOLERANCE - 0.5 | ||
| : element.scrollLeft < max - EDGE_TOLERANCE + 0.5; | ||
| } |
There was a problem hiding this comment.
SCROLLABLE, EDGE_TOLERANCE, canMoveY and canMoveX repeat gestures/nativeScroll.ts (lines 77-78 and 192-217), whose exported canElementScroll already answers whether an element scrolls natively for a finger moving by (dx, dy); the copies already differ (overlay is missing here). Calling canElementScroll from takesPan and keeping only the overscroll-behavior containment and the chain end here keeps one edge test for the lock and the recognizers.
There was a problem hiding this comment.
This is #8098's code, now merged; this pull request no longer touches the file. The follow-up goes to that change's owner.
| // iOS Safari scrolls an `overflow: hidden` page under a finger. | ||
| // So the lock also cancels each one-finger pan that would reach the page. The | ||
| // scroll offset never changes, so unlock restores nothing, and iOS Safari's | ||
| // toolbar keeps its state. |
There was a problem hiding this comment.
Non-blocking: this comment restates the hook's JSDoc (the kept offset and the one-finger cancel). Keeping only the reason in the JSDoc is enough: "iOS Safari scrolls an overflow: hidden page under a finger, so the lock cancels a one-finger pan that would reach the page."
There was a problem hiding this comment.
This is #8098's code, now merged; this pull request no longer touches the file. The follow-up goes to that change's owner.
A `<form method="dialog">` submit, or a close() through a ref, closes the drawer's dialog before the app's isOpen follows. The case pins that the page lock MobileNav takes through useScrollLock still releases the root's overflow and scrollbar gutter once isOpen turns false. It stubs a classic scrollbar, so the lock holds the gutter while the drawer is open. Test only.
40a4ff7 to
0a17209
Compare
github-actions
Bot
removed
needs:design-review
There was a problem hiding this comment.
Recommended for approval. The new case covers MobileNav's native-close path, matches the current MobileNav contract (FR3), and fails when the page lock's release is broken. There are no blocking findings.
Non-blocking suggestion:
- The current FR3 says closing releases three things (the overflow clip, the scrollbar gutter and the overscroll stop); asserting
overscrollBehaviorYis''on the root and<body>after the close would cover the third, and the description's FR3 quote could use the current wording.
[Automated review]
| }); | ||
|
|
||
| expect(document.documentElement.style.overflow).toBe(''); | ||
| expect(document.documentElement.style.scrollbarGutter).toBe(''); |
There was a problem hiding this comment.
Non-blocking: the current component:MobileNav FR3 releases three things on close, and the lock writes overscroll-behavior-y: none on the root and <body> in the same claim as the overflow. These two lines after this one pass today and would pin the third release on the native-close path:
expect(document.documentElement.style.overscrollBehaviorY).toBe('');
expect(document.body.style.overscrollBehaviorY).toBe('');
cixzhang
added
the
ci:run
#8098 moved MobileNav onto the shared scroll lock. This pull request adds one guard its tests do not cover.
Maintainer impact
A
<form method="dialog">submit, or aclose()through a ref, closes MobileNav's dialog before the app'sisOpenfollows. Nothing pins that the page lock still releases on that path. A lock held through the slide-out and released by the delayed close would leave the page locked: a native close never starts that close's timer.Intended invariant
No behavior change.
component:MobileNavFR3 (current): "While open, the page's root clips its overflow and keeps its scrollbar gutter; closing releases both." The release must hold when the dialog closes natively first.Change
One unit case in
MobileNavCloseEdgeCases.test.tsx, "releases the page when the dialog closed natively before isOpen turned false". It stubs a classic scrollbar, asMobileNavScrollbarGutter.test.tsxdoes, so the lock holds the gutter. While the drawer is open, the page is locked (whatever value the lock writes) and the gutter readsstable. Its dialog then closes natively,isOpenturns false, and both the root's overflow and its gutter are released. Test only, so no changeset.Evidence
expected 'hidden' to be '';useScrollLockwithout its gutter release (a46263f) fails on the gutter:expected 'stable' to be ''.Scope
Testing
Local gates at 0a17209 (on main 2188aa9), after a frozen install (exit 0):
pnpm review:prereports no findings;check:repopasses; the core type check exits 0; strict lint and Prettier are clean on the changed file.test:uirun 19,550/19,550 (770 files).storybook:buildexits 0; MobileNav's three*.a11y.chromium.spec.tscontracts pass, 6/6.test:node: 86packages/clifiles hit their 30 s timeout under box load (load 43-56 on 166 cores). The node project runs nothing this pull request changes: its one changed file is a jsdom test that only the ui project runs, so the node run's inputs are byte-identical to main's.