Repository navigation
Conversation
A browser resolves touch-action through DOM ancestors, and a descendant cannot allow back what an ancestor blocks. The drawer's dialog declared touch-action: pinch-zoom (and its content pan-y pinch-zoom), so a Dialog opened inside the drawer could not be scrolled with a finger. The gesture block moves to the scrim: a new element beside the drawer blocks pans and allows pinch, as the dialog did, and so does the header, which does not scroll. The dialog, the drawer and its content declare no touch-action, so nested layers keep their own. A tap on the scrim closes the nav, as a tap on the dialog did. The drawer stays the dialog's first child (z-index above the scrim), so existing selectors still find it. A Chromium spec opens a Dialog from inside the drawer: no ancestor of its content up to the drawer restricts panning, a finger drag scrolls its results, and the scrim and header still block pans and allow pinch. A story shows the nested Dialog.
|
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.
|
github-actions
Bot
added
the
needs:design-review
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
…o they scroll; changeset
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested. The scrim keeps tap-to-close and pinch zoom working, but the change breaks existing browser tests and does not yet show the failure it fixes.
MobileNavWidth.a11y.chromium.spec.ts(line 81),MobileNavPinchZoom.a11y.chromium.spec.ts(line 141) andMobileNavTouchPresentation.a11y.chromium.spec.ts(line 161, used by the tests at lines 291 and 353) still expect the<dialog>to be the hit target beside the drawer, so they fail now that the scrim takes those hits; treat any non-drawer element inside the dialog as backdrop there.- The new
MobileNavNestedTouch.a11y.chromium.spec.tsfails at line 53 because the Nested Dialog story's 40 inlineTextresults fit inside the 60dvh dialog, so the results region never scrolls and never takes itsgrouprole; render one block row per result. - In Chromium with real touch input, a Dialog inside the drawer whose results overflow already scrolls on
main(251 px for a 200 px drag, the same as this branch), so the title and description claim a failure the tests do not reproduce; reproduce it onmainin the browser where it fails, name that browser and the cause, and add a check that fails onmain, or drop the fix claim. @astryxdesign/coresource changes without a Changeset, which the changeset coverage check rejects; add one withpnpm changeset:new --packages @astryxdesign/core --category fix, worded to match what the previous point settles.
The comment above styles.dialog (MobileNav.tsx lines 121–123) repeats the ancestor-blocking cause, so keep only the reason that holds once the real cause is known. CI has not run on this draft yet, and each failure above reproduces with the repository's own Chromium specs.
[Automated review]
| <div | ||
| ref={scrimRef} | ||
| aria-hidden="true" | ||
| {...stylex.props(styles.scrim)} | ||
| /> |
There was a problem hiding this comment.
This scrim now takes every hit beside the drawer, so MobileNavWidth.a11y.chromium.spec.ts:81 (expects dialog, gets div), MobileNavPinchZoom.a11y.chromium.spec.ts:141 and MobileNavTouchPresentation.a11y.chromium.spec.ts:161 fail, though all pass on main. Treat any non-drawer element inside the dialog as backdrop in those checks, for example hit.closest('dialog') === dialog && !hit.closest('[tabindex="-1"]'). With that change the backdrop pinch passes here.
| // No touch-action here: a browser resolves it through DOM ancestors, so a | ||
| // block on the dialog would also stop touch scrolling in a layer opened | ||
| // inside the drawer. The scrim and the header block pans instead. |
There was a problem hiding this comment.
Non-blocking (INV29): on main in Chromium, a nested results scroller pans under a finger despite this dialog's pinch-zoom, because the touch-action intersection stops at the nearest scroll container. Keep only the reason that holds, for example: "The scrim and the header block pans. The dialog sets no touch-action, so a layer in the drawer keeps its own."
| content={ | ||
| <LayoutContent label="Search results"> | ||
| {SEARCH_RESULTS.map(result => ( | ||
| <Text key={result} type="body"> |
There was a problem hiding this comment.
These Text results render inline, so all 40 fit in about 400 px inside the 60dvh dialog. The region never overflows, so it takes no group role or name (LayoutContent FR4), and nothing scrolls. Wrap each result in a block row so the story shows the scrolling its doc comment describes.
| await page.waitForTimeout(600); // the slide-in | ||
| await nav.getByRole('button', {name: 'Search'}).tap(); | ||
| const results = page.getByRole('group', {name: 'Search results'}); | ||
| await expect(results).toBeVisible(); |
There was a problem hiding this comment.
This fails at this head with element(s) not found: the story's results region fits, so it never gets the group role (see the story comment). Once the story overflows, the drag check at line 100 passes on main too: the same 200 px finger drag scrolls 251 px on both, so this spec does not yet guard the fix.
There was a problem hiding this comment.
Changes requested. The story and changeset points from the last review are fixed; two blocking points remain.
MobileNavWidth.a11y.chromium.spec.ts(line 81),MobileNavPinchZoom.a11y.chromium.spec.ts(line 141) andMobileNavTouchPresentation.a11y.chromium.spec.ts(line 161, used by the tests at lines 291 and 353) still treat the<dialog>as the hit target beside the drawer, so all four tests fail now that the scrim takes those hits; count any non-drawer element inside the dialog as backdrop there.- The title, description and changeset say a Dialog inside the drawer could not scroll with a finger, but in Chromium with real touch input the Nested Dialog story's results already scroll on
main(250 px for a 200 px swipe, 252 px on this branch) and the new spec's drag check passes there too; reproduce the failure onmainin the browser where it happens, with a check that fails there by behavior, or reword the title, description and changeset to what this branch changes.
The comment above styles.dialog (lines 121–123) still gives the ancestor cause; keep only the reason that holds. CI has not run on this draft, and each point reproduces with the repository's own Chromium specs.
[Automated review]
| ref={scrimRef} | ||
| aria-hidden="true" | ||
| {...stylex.props(styles.scrim)} | ||
| /> |
There was a problem hiding this comment.
This scrim takes every hit beside the drawer, so MobileNavWidth.a11y.chromium.spec.ts:81 (expects dialog, gets div), MobileNavPinchZoom.a11y.chromium.spec.ts:141 and MobileNavTouchPresentation.a11y.chromium.spec.ts:161 (tests at lines 291 and 353) fail, though all pass on main. Count any non-drawer element inside the dialog as backdrop in those checks, for example hit.closest('dialog') === dialog && !hit.closest('[tabindex="-1"]'). A pinch on the scrim does zoom the page, so only the checks are stale.
| // No touch-action here: a browser resolves it through DOM ancestors, so a | ||
| // block on the dialog would also stop touch scrolling in a layer opened | ||
| // inside the drawer. The scrim and the header block pans instead. |
There was a problem hiding this comment.
Non-blocking (INV29): on main in Chromium a nested results scroller pans under a finger despite this dialog's pinch-zoom, because the effective touch-action stops at the nearest scroll container. Keep only the reason that holds, for example: "The scrim and the header block pans. The dialog sets no touch-action, so a layer in the drawer keeps its own."
| touchPoints: [], | ||
| }); | ||
| await sleep(600); | ||
| expect(await results.evaluate(element => element.scrollTop)).toBeGreaterThan( |
There was a problem hiding this comment.
This drag check also passes on main: with the Nested Dialog story in place there and only line 75 removed, the same 200 px drag scrolls the results (250 px, against 252 px here). Line 75 fails on main only because getComputedStyle reports the declared touch-action, not whether a pan is allowed. So the spec does not yet guard the fix; it needs a step that fails on main by behavior.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] A Dialog or other layer opened inside the open `MobileNav` drawer scrolls with a finger again. The drawer blocks background pans on its scrim and header instead of on the whole dialog, so a nested layer keeps its own touch scrolling. Pinch-zoom still works over the nav. |
There was a problem hiding this comment.
"scrolls with a finger again" claims a failure that does not reproduce on main in Chromium, where a Dialog inside the drawer already scrolls under a finger, and the released drawer already ships these declarations. Reword this to the change this branch shows, or name the browser where it fails once that is reproduced.
Per review: MobileNavWidth, MobileNavPinchZoom and MobileNavTouchPresentation treated the dialog element as the only hit target beside the drawer; any element inside the dialog and outside the drawer is backdrop now. The dialog comment, the changeset and the nested spec's header say what this change does: Chromium already scrolls a nested results list (it stops resolving touch-action at the nearest scroll container), so the ancestor walk is the guard and the drag a regression check.
cixzhang
changed the title
There was a problem hiding this comment.
Changes requested. The backdrop checks now count the scrim and every MobileNav browser test passes; two points about what the change claims to fix remain.
- The changeset still calls this a fix for nested touch scrolling "in a browser that resolves
touch-actionthrough DOM ancestors", but nothing in the pull request reproduces that failure (in Chromium the Nested Dialog story's results scroll the same onmainand here, and the phone check the description mentions is not attached), so attach the phone before and after naming the browser and version, or describe only the change, for example that no drawer element restrictstouch-actionfor a layer rendered in its content. - Miss: the changeset covers "a Dialog or other layer opened inside" the drawer, but the header now declares
touch-action: pinch-zoom(MobileNav.tsxline 253) over the caller's header content, so under the premise in the description a layer rendered in the header keeps that restriction; scope the wording to layers in the drawer's content, or take the block off the header.
Two comments in MobileNav.tsx (lines 171–174 and 703–704) could be shorter; see the inline notes. CI has not run on this draft.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] The open `MobileNav` drawer blocks background pans on its scrim and header instead of on the whole drawer, so a Dialog or other layer opened inside it keeps its own touch scrolling in a browser that resolves `touch-action` through DOM ancestors. Pinch-zoom still works over the nav. |
There was a problem hiding this comment.
Nothing in this pull request reproduces the failure this fixes: in Chromium the Nested Dialog story's results scroll 251 px on main and 249 px here under the same 200 px swipe, and the Pointer Events resolution stops at the nearest scroll container. Attach the phone before and after, naming the browser and version, or describe the change itself, for example: "…so no drawer element restricts touch-action for a layer rendered in its content."
| paddingInline: spacingVars['--spacing-2'], | ||
| flexShrink: 0, | ||
| // The header does not scroll, so a pan on it would reach the page. | ||
| touchAction: 'pinch-zoom', |
There was a problem hiding this comment.
Miss: the caller's header content renders inside this element, so under the premise in the description a layer rendered in the header (results for the search field the Tall Header story puts here) keeps a pinch-zoom ancestor, as Content-region layers did on main. Scope the changeset to layers in the drawer's content, or take this block off the header (FR3's scroll lock already holds the page).
| // The area beside the drawer. Stop pans passing through (pull-to-refresh, | ||
| // background scroll), but leave pinch to the browser: the open nav covers | ||
| // the whole viewport, and pinch-zoom is how phone readers resize text | ||
| // (WCAG 1.4.4). |
There was a problem hiding this comment.
Non-blocking (INV29): the second sentence runs 30 words over three topics. For example: "Pans here would reach the page, so block them. Leave pinch to the browser. The nav covers the viewport, and pinch is how readers enlarge text."
| // Only close on the overlay beside the drawer (the dialog itself or its | ||
| // scrim), not on the drawer or its children. |
There was a problem hiding this comment.
Non-blocking (INV29): this restates the condition below it. Drop it, or keep only a reason the code cannot show.
…hanges Per review: the header declared pinch-zoom over the caller's header content, so a layer rendered there would keep the restriction. FR3's scroll lock already holds the page, so only the scrim blocks pans now. The changeset describes the change (no drawer element restricts touch-action for a layer inside it) rather than a failure Chromium does not show. Shorter comments.
There was a problem hiding this comment.
Changes requested. The changeset now describes the change, the header no longer restricts touch-action, and every MobileNav browser and unit test passes; only the title is left.
- The title says the scrim and header block background pans, but this push removed the header's block (the new spec expects the header to compute
auto), so retitle to the change, for example "fix(MobileNav): block background pans only on the scrim, so no drawer element restricts touch-action for a layer inside it". - Miss: the title's "so a layer in the drawer keeps its own touch scrolling" still claims the restoration the changeset dropped, which nothing in the pull request reproduces (in Chromium the Nested Dialog story's results scroll the same on
mainand here), and the same retitle fixes it.
The description's User impact still points to a phone check "below" that is not attached. CI has not run on this draft.
[Automated review]
cixzhang
changed the title
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet because the pull request is a draft, and it runs once the pull request is marked ready for review.
The title now matches the change, the scrim keeps tap-to-close and pinch zoom working, and every MobileNav browser and unit test passes. One optional fix: the Testing section says the new spec "checks three things" and then lists four.
[Automated review]
User impact
A Dialog opened from inside the open mobile navigation drawer, such as a search dialog, was reported as not scrolling with a finger on a phone, while the wheel and the keyboard still scrolled it.
touch-action: pinch-zoom, and its content setspan-y pinch-zoom. A Dialog rendered inside the drawer is their DOM descendant, even though it paints in the top layer.touch-actionthrough every DOM ancestor applies the drawer's block to the nested Dialog, and a descendant cannot allow back what an ancestor blocks. Chromium stops at the nearest scroll container, so it already scrolls the nested results; a phone check on the device the report came from is pending; until it reports, this pull request claims only the change below, not a restored behavior.The fix
The gesture block moves to the scrim, the area beside the drawer.
touch-action, so a layer rendered anywhere inside the drawer keeps its own. FR3's scroll lock already holds the page under the drawer.Testing
MobileNavNestedTouch.a11y.chromium.spec.ts(new): opens a Dialog from inside the drawer on a touch phone viewport. It checks three things: no ancestor of the Dialog's content up to the drawer restricts panning, a finger drag scrolls the results, the scrim computespinch-zoom, and the header computesauto.MobileNavNestedTouch.test.tsx(new): a tap on the scrim closes the nav; a tap on drawer content doesn't.MobileNavWidth,MobileNavPinchZoomandMobileNavTouchPresentationspecs: their backdrop checks now count any element inside the dialog and outside the drawer, which includes the scrim.Chromium 154 with a touch phone viewport, against Storybooks built by Vercel from this head (after) and from main plus only this story (before):
pan-y pinch-zoom, dialogpinch-zoomtouch-actionauto/pinch-zoompinch-zoom/auto/autoChromium stops resolving
touch-actionat the nearest scroll container, so it already scrolled the nested results before the change. The browser where an ancestor's block reaches the nested layer is what the report hit; that phone check is pending. The ancestor walk in the spec fails on main and passes here.