fix(android): honor scroll releaseBehavior to stop the fling overshoot - #2372
fix(android): honor scroll releaseBehavior to stop the fling overshoot#2372thymikee wants to merge 4 commits into
Conversation
scrollAndroid ignored ScrollReleaseBehavior and always released the pan like a fling, so a controlled `scroll` overshot its requested distance by ~60% on both a plain RecyclerView and an RN ScrollView. Android's VelocityTracker treats a truly stationary release as a resampled duplicate and ignores it, so a hold alone can't stop the fling. For the default 'controlled' release, append a short tail after the pan's endpoint that holds the scroll axis exactly fixed (zero velocity there) while nudging the orthogonal axis every frame so no two consecutive samples repeat. 'inertial' (the scroll top/bottom edge passes) is unchanged. Live-measured on a Pixel 9 Pro XL emulator: displacement lands within touch-slop of the requested drag instead of overshooting it. Fixes #2371
Size Report
Startup median (7 runs, lower is better):
|
|
At fce45a3, the release tail is added after gesture duration validation. A valid 10000ms scroll therefore produces a 10160ms plan, beyond the shared total-duration limit. Please account for the tail at the owning duration boundary and add a maximum-duration regression without silently shortening the requested move. The reported Settings result is encouraging, but #2371 also requires RN ScrollView and CI Pixel 7 displacement evidence. Please add those measurements before marking this ready. The provider integration failure is related: it still expects a 350ms plan instead of the new 510ms controlled gesture. Update it to assert controlled versus inertial release. Coverage also reports a stale eager-closure approval, which looks unrelated to this change. |
… ceiling The release tail was appended after buildGesturePlan's own duration validation, so a maximum-duration (10000ms) scroll silently produced a 10160ms plan beyond GESTURE_DURATION_MAX_MS. Cap the tail so the dispatched plan never exceeds that shared ceiling, shrinking (and, at the exact maximum, dropping) the tail near the boundary instead of truncating the requested move. Add regression coverage for the exact maximum and near-maximum cases. Also fix the stale android-lifecycle provider-integration expectation: a plain scroll resolves to 'controlled' release by default, so its dispatched plan is now 510ms (350ms move + 160ms tail), not 350ms.
|
At 4a67a37, the duration cap is respected, but controlled scrolls near the maximum lose the required braking tail; at 10000ms there is no tail at all. Please preserve controlled-release behavior across the accepted duration range, or make an explicit validated duration constraint rather than silently dropping the braking step. Add a boundary regression and the RN ScrollView/Pixel 7 displacement evidence requested earlier. Coverage also fails because the new explanatory comment grows the over-budget android-lifecycle.test.ts. Keep the corrected expectation without growing that legacy test. The IME recovery persistence failure and stale closure approval look unrelated. |
… no room, instead of shrinking it The previous fix capped the tail near GESTURE_DURATION_MAX_MS, which silently dropped the fling-suppressing braking step for scrolls close to the ceiling -- defeating the point of the fix exactly where a long scroll needs it. A controlled scroll's own accepted duration range now stops CONTROLLED_RELEASE_TAIL_MS short of that shared ceiling (9840ms), rejecting anything past it with a clear INVALID_ARGS error instead of truncating the move or the tail. Every accepted controlled scroll now runs the full, unshortened tail; 'inertial' releases are unaffected and keep the full 10000ms range. Also drop the multi-line explanatory comment on the corrected android-lifecycle.test.ts expectation (a same-line comment instead) so the fix doesn't grow that file past its line-count tripwire.
|
Addressed both rounds of feedback. Duration boundary (fixed properly this time): the first fix I pushed shrank the tail near the ceiling, which — as you caught — silently dropped the braking step exactly where a long scroll needs it. Reworked at 9a3ea26: a controlled scroll's own accepted Provider integration + file size: fixed the stale RN ScrollView + Pixel 7 evidence, gathered live on a fresh
All three land at 92–94% of the requested drag (touch-slop territory), consistent with The IME recovery persistence failure and the |
|
The duration fix at 9a3ea26 is sound: every accepted controlled scroll retains the full braking tail, and inertial scrolling keeps its existing range. All checks pass, and the reported Pixel 7 measurements cover both Settings and RN ScrollView. One small acceptance item remains from #2371: update the scroll documentation to describe reduced momentum, not exact displacement, and explain the new 9840ms Android controlled-scroll limit. The current docs only contain the generic app-physics caveat. |
…roll limit The scroll docs only carried the generic app-physics caveat, not the release-behavior contract itself. State plainly that scroll releases with reduced momentum rather than an exact stop, and document Android's controlled-release durationMs ceiling (9840ms, reserved headroom for the braking tail added in #2371) and that a longer request is rejected rather than silently truncated.
|
|
Updated the scroll docs at 0214cc4 — |
|
The help and duration-limit docs at 0214cc4 resolve the remaining review finding; no further code findings. Android smoke failed because the Back diff did not add the expected Open automation lab line: the connection to this scroll change is unclear and needs a reproduction or a justified rerun before merging. The iOS xcrun timeout appears unrelated. Ready for human review, not yet CI-green. |
thymikee
added
the
ready-for-human
1 participant
Summary
scrollAndroidignoredScrollReleaseBehaviorand always released the pan like a fling, so a "controlled"scrollovershot its requested distance by ~60% on both a plainRecyclerViewand an RNScrollView— measured and root-caused in Android scroll ignores releaseBehavior: every controlled scroll flings ~60% past its drag #2371.'controlled'release, append a short (~160ms) tail after the pan's endpoint that holds the scroll axis exactly fixed at the endpoint (zero velocity there by construction) while nudging the orthogonal axis every frame, so no two consecutive samples repeat. That dodges an AOSP quirk whereInputConsumer::rewriteMessagetreats a truly stationary tail as a "resampled" duplicate thatVelocityTrackerignores, so the list still flings even with a hold.'inertial'(thescroll top/scroll bottomedge passes) is unchanged.Known trade-off (not addressed here)
scrollAndroid's touch plan extendsdurationMsby the tail length before Android's shared densification (lowerAndroidTouchPlan) re-grids the whole gesture at ~16ms cadence. Because that re-grid is computed from the total (move + tail) duration rather than per-segment, it can interleave a few extra sub-pixel interpolated samples near the tail's own 16ms-spaced points (this already happens today forbuildDragGesturePlan's hold segments — not something this PR introduces). It doesn't affect correctness (verified live below), just adds a handful of extra touch events. Fixing it properly means making the shared lowering segment-aware, which is a larger, separate change.Test plan
packages/platform-android/src/__tests__/input-actions.test.ts— 16 tests, including new coverage for: the default controlled-release tail (zigzag, never-repeating, duration composition), the duration floor composing correctly with the tail, a horizontal scroll jittering the correct (orthogonal) axis, and the'inertial'edge-pass path being unchanged.pnpm tsc --noEmit(repo-wide) and full affected-packagevitestruns (680 tests) — all green.scroll down --pixels 900 --duration-ms 300on the Settings app (matching the maintainer's original repro) now lands within touch-slop of the requested drag (828–837px measured, vs. the old fling's ~1350–1440px overshoot) across two independent runs;scroll bottom('inertial') still completes cleanly and lands on the list's actual last row.