[Android] Fix buttons firing press events when a scroll takes over the touch - #4441
[Android] Fix buttons firing press events when a scroll takes over the touch#4441m-bert wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 5 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Android touch flow now tracks native dispatch, defers native touch-grab cancellation until dispatch ends, and cancels eligible native view handlers. Existing legacy-handler cancellation keeps its previous filters. ChangesNative touch-grab cancellation
Sequence Diagram(s)sequenceDiagram
participant NativeView
participant RNGestureHandlerRootHelper
participant RNGestureHandlerRootView
participant GestureHandlerOrchestrator
participant ButtonViewGroup
NativeView->>RNGestureHandlerRootHelper: requestDisallowInterceptTouchEvent
RNGestureHandlerRootHelper->>GestureHandlerOrchestrator: cancelAllLegacyHandlers
NativeView->>RNGestureHandlerRootView: dispatchTouchEvent
RNGestureHandlerRootView->>RNGestureHandlerRootHelper: onNativeDispatchEnd
RNGestureHandlerRootHelper->>GestureHandlerOrchestrator: cancelHandlersOnNativeTouchGrab
GestureHandlerOrchestrator->>ButtonViewGroup: evaluate native touch-grab cancellation
Merge Risk: ⚪ Minimal · up to The change prevents press events during native scrolling while preserving normal taps and long presses; no actionable merge-blocking risk remains after normal checks and review. 🚥 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.
Pull request overview
This PR addresses an Android-specific v3 Pressable regression where onPress could fire when a touch only stops a fling/scroll, by deferring certain gesture cancellations until after native dispatch completes so the system can distinguish “defensive” disallow-intercept calls from real native interception.
Changes:
- Track disallow-intercept requests during native dispatch and perform a post-dispatch cancellation pass for opted-in handlers.
- Add an opt-in hook on
NativeViewGestureHandlerso specific native-backed handlers (currently the button-backedPressable) can be cancelled when nativeDOWNnever reached them. - Refactor orchestrator cancellation logic to share a common predicate-based cancellation helper.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/react-native-gesture-handler/android/src/main/java/com/swmansion/gesturehandler/react/RNGestureHandlerRootView.kt | Calls onNativeDispatchEnd() after super.dispatchTouchEvent to enable post-dispatch cancellation. |
| packages/react-native-gesture-handler/android/src/main/java/com/swmansion/gesturehandler/react/RNGestureHandlerRootHelper.kt | Tracks native touch-grab requests and triggers orchestrator cancellation after native dispatch completes. |
| packages/react-native-gesture-handler/android/src/main/java/com/swmansion/gesturehandler/react/RNGestureHandlerButtonViewManager.kt | Makes button-backed Pressable opt into post-dispatch cancellation when it didn’t receive native DOWN. |
| packages/react-native-gesture-handler/android/src/main/java/com/swmansion/gesturehandler/core/NativeViewGestureHandler.kt | Exposes a hook-driven shouldCancelOnNativeTouchGrab() decision point. |
| packages/react-native-gesture-handler/android/src/main/java/com/swmansion/gesturehandler/core/GestureHandlerOrchestrator.kt | Adds cancelHandlersOnNativeTouchGrab() and refactors cancellation into a shared predicate helper. |
Suppressed comments (1)
packages/react-native-gesture-handler/android/src/main/java/com/swmansion/gesturehandler/react/RNGestureHandlerRootHelper.kt:133
- The KDoc for
onNativeDispatchEnd()says cancellation runs here “not at request time”, butrequestDisallowInterceptTouchEvent()still cancels legacy handlers immediately. Consider clarifying that this method performs the additional opt-in cancellation after native dispatch completes.
/**
* A disallow-intercept request may mean a real interception, but it may also be just a
* defensive call from a view that lets the event through (e.g. a nested pager). The two can only
* be told apart after the native dispatch completes, so cancellation runs here, not at request time.
*/
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // If this method gets called it means that some native view is attempting to grab lock for | ||
| // touch event delivery. In that case we cancel all gesture recognizers | ||
| if (orchestrator != null && !passingTouch) { | ||
| // if we are in the process of delivering touch events via GH orchestrator, we don't want to | ||
| // treat it as a native gesture capturing the lock |
Pressable firing onPress when the touch only stops scroll|
@coderabbitai review again |
|
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/react-native-gesture-handler/android/src/main/java/com/swmansion/gesturehandler/react/RNGestureHandlerRootHelper.kt:137
ACTION_POINTER_DOWNis classified as a mid-gesture grab even when it is the first/down event delivered to a split child touch target. For example, while one pointer holds a button, a second pointer entering the nested pager can produce the pager's defensiveDOWNdisallow request while this root event isACTION_POINTER_DOWN; this passestrueand the root-wide sweep cancels the unrelated button (and any button under the second pointer) despite native dispatch continuing normally. The decision needs to be tied to the affected pointer/handler's native delivery rather than only the root event action.
orchestrator?.cancelHandlersOnNativeTouchGrab(event.actionMasked != MotionEvent.ACTION_DOWN)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/react-native-gesture-handler/android/src/main/java/com/swmansion/gesturehandler/react/RNGestureHandlerRootHelper.kt:139
ACTION_POINTER_DOWNoccurs after the gesture's initialDOWN, so treating it as an initial pass leaves a button alive if an ancestor starts intercepting when an additional pointer lands. BecausereceivedNativeDownis already true from the first pointer, the hook then declines cancellation and the handler can still reachENDand emit a press. OnlyACTION_DOWNshould be exempt fromgrabbedMidGesture.
val grabbedMidGesture = event.actionMasked != MotionEvent.ACTION_DOWN &&
event.actionMasked != MotionEvent.ACTION_POINTER_DOWN
Description
Pressablewithout relation props presses natively throughButtonViewGroup, whose managedNativeViewGestureHandleris attached withACTION_TYPE_NONE. RNGH delivers touches through the orchestrator regardless of what happens in the native dispatch, so when a nativeScrollViewtakes the gesture over, nothing stops the handler - it reachesSTATE_ENDon lift and fires a press. This shows up in three ways:ScrollViewinterceptsDOWNwhile decelerating, the button never sees any native event, yetonPressfires on lift ([Android] Pressable fires onPress when the touch only stops a fling #4432)ScrollViewintercepts onMOVEwhen the finger starts scrolling from a row, andonPressstill fires on lift (comment)BEGANfires mid-scroll (same comment)In all three the
ScrollViewcallsrequestDisallowInterceptTouchEvent(true), but the existing sweep (cancelAllLegacyHandlers) only cancels action-driven handlers, and theButtonViewGroupoverride from #4367 never runs since the request only bubbles up from theScrollView.Cancelling button handlers directly at request time (the #4433 approach) is not valid either: an eager disallow-intercept (
react-native-pager-view'sNestedScrollableHostrequests it onDOWNwhenever it's nested inside anotherViewPager, without intercepting anything) is indistinguishable from a real interception at that moment, so everyPressableinside nested pagers (e.g. material top tabs in a pager) would go dead - the regression class #4367 fixed.The two can be told apart by when the grab happened and whether the native dispatch still reached the button:
ButtonViewGrouptracksreceivedNativeDown- set indispatchTouchEvent(handler delivery bypasses it), reset onBEGAN, which the orchestrator dispatches before the native dispatch of the sameDOWN.RNGestureHandlerRootHelperrecords the disallow request, and once the root view finishessuper.dispatchTouchEventrunscancelHandlersOnNativeTouchGrab, cancelling handlers whose hook opts in.shouldCancelOnNativeTouchGrab(grabbedMidGesture) = grabbedMidGesture || !receivedNativeDown: a grab on any pass afterDOWNmeans actual dragging (cancel, matching what the legacyPressableand RN'sPressabledo), while a grab during theDOWNpass spares a button that received thatDOWN(a defensive disallow lets the event through).Only
ButtonViewGroupopts into the hook, so handlers attached to detectors, scrollables and text inputs are unaffected. The cost on passes without a disallow request is a single boolean check.Fixes #4432
Supersedes #4433
Test plan
Repro below: a
SectionListwithPressablerows (onPress+onLongPress), aPressableand a long-pressGestureDetectorinside nestedPagerViews (the eager-disallow setup from #2383), and an engine toggle (v3 /LegacyPressable/ RNPressable). All runs on the same emulator, main vs this PR:onPressonPressonLongPressonPressonPressonLongPressonLongPressPressableinside nested pagersonPressonPressLegacyPressablebehaves the same in the list scenarios; inside nested pagers it doesn't fire on main either - its handlers are cancelled on any disallow-intercept request, which is the pre-existing legacy behavior this PR doesn't change. RN'sPressabledoesn't go through RNGH and is clean everywhere.Repro