Reset accumulated wheel delta in web WheelEventManager - #4484
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughChangesWheel gesture reset
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Web wheel gestures now clear prior gesture state and accumulated scroll offsets before the next gesture, preventing stale coordinates from carrying over. The covered reset behavior is ready to merge. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🟢 Approval recommended
The fix is small, targeted, and validated by focused tests that reproduce the reported failure mode.
Pull request overview
This PR fixes a web-specific coordinate drift issue in WheelEventManager by clearing its accumulated wheel delta when the manager is reset, aligning its bookkeeping behavior with other event managers and preventing stale deltas from affecting subsequent gestures.
Changes:
- Reset
wheelDeltainsideWheelEventManager.resetManager()so accumulated deltas don’t leak across gestures. - Add Jest coverage to verify deltas still accumulate within a gesture, but are cleared after
resetManager().
File summaries
| File | Description |
|---|---|
packages/react-native-gesture-handler/src/web/tools/WheelEventManager.ts |
Clears stored wheelDelta on manager reset to prevent cross-gesture coordinate offset. |
packages/react-native-gesture-handler/src/web/tools/__tests__/WheelEventManager.test.ts |
Adds regression tests covering accumulation behavior and reset behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
m-bert
left a comment
There was a problem hiding this comment.
Hi @giaBaoJS! Thank you for submitting this PR! Unfortunately seems that it is not enough. The good news is, replacing
if (this.state === State.ACTIVE) {
this.end();
this.tracker.removeFromTracker(event.pointerId);
this.state = State.UNDETERMINED;
}with
if (this.state === State.ACTIVE) {
this.end();
this.reset();
}in PanGestureHandler does the job.
The test passes as it calls manager.resetManager manually. Given that - I don't think we need this test file.
`WheelEventManager` synthesizes pointer coordinates by accumulating `deltaX`/`deltaY` on top of the wheel event's client coordinates, since a wheel does not move the cursor. Its `resetManager` override only called `super.resetManager()` and left `wheelDelta` untouched, unlike `PointerEventManager`, which clears its own bookkeeping there. `PanGestureHandler.scheduleWheelEnd` ended the gesture by removing the pointer from the tracker and assigning `State.UNDETERMINED` directly. Because the handler never reached a finished state, the orchestrator's `cleanupFinishedHandlers` microtask skipped it and `reset()` never ran, so `GestureHandlerWebDelegate.reset` never reached the event managers. Calling `reset()` covers both: `resetTracker` is a superset of the single `removeFromTracker`, and it ends with the same state assignment. The next wheel gesture on the same view then started from the previous gesture's accumulated scroll distance and reported `absoluteX`/ `absoluteY` offset by it, growing with every gesture until a `pointermove` happened to clear the delta.
41e0dd4 to
13d3fe0
Compare
|
Done, The // src/web/tools/GestureHandlerWebDelegate.ts:184
reset(): void {
this.eventManagers.forEach((manager: EventManager<unknown>) =>
manager.resetManager()
);
}With only the On why
One note on the swap itself: |
m-bert
left a comment
There was a problem hiding this comment.
The WheelEventManager.resetManager change is still needed alongside it.
I'm aware, that's why I said "it is not enough", not that it is wrong 😄
Thank you once again for this PR
Description
WheelEventManagerhas no pointer to follow, so it synthesizes coordinates by accumulatingdeltaX/deltaYon top of each wheel event's client coordinates. ItsresetManageroverride calls onlysuper.resetManager()and never clearswheelDelta, unlikePointerEventManager, which clears its own bookkeeping there.resetManagerruns on every handler reset (GestureHandler.reset->delegate.reset->manager.resetManager), which the orchestrator triggers once a gesture reachesEND. So when a trackpad pan ends, the whole scroll distance of that gesture stays inwheelDelta.To reproduce, put a
Pangesture withenableTrackpadTwoFingerGestureon a view, then do two two-finger trackpad pans in a row without moving the cursor in between. The second gesture reportsabsoluteX/absoluteYoffset by the first gesture's total scroll. A two-finger scroll does not move the cursor, so thepointermovelistener that clears the delta does not necessarily fire, and the offset keeps growing with each gesture.Fix is to clear
wheelDeltainresetManager, matching whatPointerEventManageralready does.Test plan
Added
src/web/tools/__tests__/WheelEventManager.test.tswith two cases:yis 130 after deltas of 100 and 30), so the accumulation behaviour is not lost.resetManager, the next wheel event reports only its own delta (yis 30, not 130).The second test fails on
mainwithExpected: 30, Received: 130and passes with the fix.yarn jest src/web,yarn ts-checkandeslint/prettieron the touched files all pass.