Repository navigation
fix(gestures): a pan on its own scroller takes a trackpad's scroll, as a finger's pan (spec:AST-094 FR43) - #10212
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🔎 Preview ·
|
| Component | Size | LoC |
|---|---|---|
| DateInput | 165.9 kB (+171 B) · 15.6 kB CSS (0) | — |
| lab/CommentThread | 273.4 kB (+165 B) · 2.4 kB CSS (0) | — |
| Switch | 94.0 kB (+148 B) · 9.5 kB CSS (0) | — |
7 more components
| Component | Size | LoC |
|---|---|---|
| SegmentedControl | 88.3 kB (+147 B) · 6.8 kB CSS (0) | — |
| Slider | 96.4 kB (+142 B) · 8.3 kB CSS (0) | — |
| AlertDialog | 94.2 kB (+132 B) · 13.2 kB CSS (0) | — |
| PowerSearch | 262.6 kB (+131 B) · 22.7 kB CSS (0) | — |
| TimeInput | 142.7 kB (+128 B) · 14.6 kB CSS (0) | — |
| lab/Comments | 279.1 kB (+122 B) · 2.5 kB CSS (0) | — |
| DateTimeInput | 188.4 kB (+113 B) · 17.3 kB CSS (0) | — |
Package size: @astryxdesign/core 6.61 MB packed (+1.2 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 19 rows, core 23 rows; run pnpm api:report.
Not run on this PR: add ci:run-visual, ci:run-a11y or ci:run-rtl to run them.
Generated by PR Enrichment workflow | View full report
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested: the gesture-host fix looks right and both new tests fail on main and pass here, but the module docs still describe the old wheel rule.
packages/core/src/gestures/README.md(the "Wheel and trackpad" paragraph) and theallowedScrollTypesJSDoc inGestureRecognizer.tsandPanGestureRecognizer.tsstill say a pan takes a trackpad scroll only after every scroller under the pointer, so please add the new exception (a pan's own scroller is skipped on the axis itstouch-actionkeeps, and an inner scroller still goes first), as the SYNC line inGestureHost.tsasks.
Optional: a host test that attaches the long press before the pan would cover the dropped touch-action the description names; on main the current test fails only at its final detach check. CI has not run on this head yet.
[Automated review]
| node != null; | ||
| node = parentAcrossShadow(node) | ||
| ) { | ||
| if (node === boundary && keepsBoundary) { |
There was a problem hiding this comment.
This skips the pan's own scroller when its touch-action keeps the axis, but README.md's "Wheel and trackpad" paragraph and the allowedScrollTypes JSDoc in GestureRecognizer.ts and PanGestureRecognizer.ts still say a pan moves only after every scroller under the pointer. Please state the exception there, and that an inner scroller still goes first; the SYNC line at the top of this file names the README.
There was a problem hiding this comment.
Done in 581f5e9: the README's "Wheel and trackpad" paragraph and the allowedScrollTypes JSDoc in GestureRecognizer.ts and PanGestureRecognizer.ts state the own-scroller exception, and that an inner scroller still goes first.
| detachers.push(coarsePrimaryPointer()); | ||
| const {row} = mount('<div id="row">A row a pan and a catch share</div>'); | ||
| const lock = vi.fn(() => () => {}); | ||
| const detachPan = host.attach( |
There was a problem hiding this comment.
Non-blocking: on main this case fails only at the final hasAttribute check (the mark left after both detach). The dropped touch-action the description names happens when the unmanaged recognizer attaches first, or when the pan is enabled after it; a case that attaches the long press before the pan would cover it.
There was a problem hiding this comment.
Done in 581f5e9: the case runs in both attach orders; with the hold attached first, main drops the pan's touch-action at the first check.
…s a finger's pan (spec:AST-094 FR43) On the axis that a pan's derived touch-action keeps from the browser, the pan's own element is the pan's for a trackpad's scroll too, as it is for a finger. A scroller inside it still takes the wheel first, and a mouse wheel's notches stay native. The mechanism this needs: manageStyle: false applies to its own recognizer, so a pager's catch on the same element no longer drops the pan's touch-action. The gesture README and the allowedScrollTypes JSDoc state the own-scroller rule.
076b10a to
581f5e9
Compare
vjeux
changed the title
|
/approve-spec 581f5e9 |
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 from a fork; mark it ready for review, and a maintainer adds the ci:run label so CI runs.
The own-scroller trackpad rule and the per-recognizer manageStyle: false follow AST-094 FR43, FR18 and FR41, the module docs now state the exception, and the new wheel, host and WebKit trackpad cases fail on main and pass here. One optional test-cleanup note is inline.
[Automated review]
| {manageStyle: false}, | ||
| ); | ||
| const detachEarly = holdFirst ? attachHold() : null; | ||
| const detachPan = host.attach( |
There was a problem hiding this comment.
Non-blocking: these attachments sit outside detachers, so a failed assertion above the explicit detaches leaves the row attached. With main's GestureHost.ts, the two cases fail and seven unrelated listener-count cases fail with them. Pushing detachPan and detachHold onto detachers, as the neighbouring cases do, keeps a future failure to this case.
There was a problem hiding this comment.
Agreed. It is test-only, so it rides this pull request's next push rather than moving the stamped head.
cixzhang
added
the
ci:run
…ative snap's stops (#8153) * fix(gestures): a pan on its own scroller takes a trackpad's scroll, as a finger's pan (spec:AST-094 FR43) On the axis that a pan's derived touch-action keeps from the browser, the pan's own element is the pan's for a trackpad's scroll too, as it is for a finger. A scroller inside it still takes the wheel first, and a mouse wheel's notches stay native. The mechanism this needs: manageStyle: false applies to its own recognizer, so a pager's catch on the same element no longer drops the pan's touch-action. The gesture README and the allowedScrollTypes JSDoc state the own-scroller rule. * feat(core): Carousel's hasSnap takes useSnapPaging(), iOS paging on native snap's stops hasSnap keeps native CSS snap. hasSnap={useSnapPaging()} pages the row under a finger and on a trackpad swipe, for every pointer: a flick moves one view, or one item with {pages: 'item'}, landing on an item's start edge. A mouse wheel and a mouse drag keep native scrolling. Only a carousel given the hook's value bundles the pager, and the hook has no non-React equivalent. The hook has its doc page and a runnable example block, and Carousel's record no longer lists snap paging as unbuilt. * Merge main into the Carousel snap paging branch (#10212 landed) * fix(SnapPaging): a held row's moves stop where the row ends While a flick holds the row, its children's translate widens the row's overflow, so its live scroll width runs past its last item. The press, Shift+wheel and scrollTo moves now clamp to the travel the row was taken over with, so no space shows past the last item. Carousel's record states that it also asks the value to move a row it holds. * Merge main into the Carousel snap paging branch Co-authored-by: Cindy Zhang <cindyxz@meta.com>
Stack: #10212 → #8153
User impact
On a desktop, a trackpad swipe does not move a pan whose element is also its own scroller, though a finger's pan there does: the host hands the wheel to that scroller, so the row scrolls natively. Carousel's snap paging (#8153, stacked on this) is such a row, and
spec:AST-094FR43 has a pager follow a trackpad and a finger alike.Problem and solution fit
touch-actionalready keeps that axis from the browser, so the two pointers disagree on the same element.touch-actionkeeps, the pan's own element is the pan's for a wheel too. A scroller inside it still takes the wheel first, and a mouse wheel's notches stay native.manageStyle: falseapplies to its own recognizer. A pager whose viewport is its track attaches its catch to the same element withmanageStyle: false, which before dropped the pan'stouch-actionfor the whole element.Expected behavior and authority
spec:AST-094FR43: a pager follows a trackpad and a finger alike, and a pan takes a trackpad's two-finger scroll where its record allows it, never a mouse wheel's notches.allowedScrollTypesJSDoc state the own-scroller rule.Smallest restoration
touch-actionkeeps.manageStyle: falseleaves the element's styles, and the hold's lock (FR41), to the caller for that recognizer only.Evidence
GestureHost.tsand pass here.Scope
Testing
GestureHost.ts: the host cases fail (the trackpad case, and the per-recognizer styling in both attach orders); the browser case fails in WebKit at the trackpad step, where the pan never began and the row scrolled natively. Its Chromium mouse-wheel step passes before and after.pnpm review:pre, 215 authored lines, no findings.GestureHost.ts: jsdom overgestures,hooksandCarousel, 1,853 passed;CI=true pnpm lintpasses; Storybook builds; the gestures browser spec, Chromium on a pinned pair of 2 cores 69 passed, 4 skipped, and WebKit in the CI job's profile (podman--cpus=4,CI=1,LP_NUM_THREADS=4, 1 worker) 61 passed, 12 skipped.GestureHostmodule bundled with the size report's esbuild options (minified, production): 72,022 B → 72,461 B (+439 B); gzip 21,180 B → 21,324 B (+144 B). Nothing lazy-loaded; the API report is unchanged.