[ios] not calling activation callback on inactive gestures#3986
[ios] not calling activation callback on inactive gestures#3986akwasniewski wants to merge 15 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where gestures were always calling the activation callback upon finalizing, regardless of their previous state. The fix ensures that manually handled gestures don't trigger the activation callback, unifying behavior across iOS and other platforms.
Changes:
- Added a
manualActivationparameter throughout the gesture handling call chain to distinguish manual state changes - Modified
sendEventsInStateto check both the parameter and instance variable before sending activation callbacks - Updated all gesture recognizer subclasses that override
handleGesture:fromReset:to support the new parameter
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| RNGestureHandlerModule.mm | Pass manualActivation:YES when manually setting gesture state via setGestureStateSync |
| RNGestureHandler.mm | Add manualActivation parameter to gesture handling methods and update activation callback logic |
| RNGestureHandler.h | Declare new method signatures with manualActivation parameter |
| RNRotationHandler.m | Add overloaded handleGesture:fromReset:manualActivation: method |
| RNPinchHandler.m | Add overloaded handleGesture:fromReset:manualActivation: method |
| RNLongPressHandler.m | Add overloaded handleGesture:fromReset:manualActivation: method with platform-specific declarations |
| RNFlingHandler.m | Add overloaded handleGesture:fromReset:manualActivation: method for macOS version |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/react-native-gesture-handler/apple/RNGestureHandler.mm
Outdated
Show resolved
Hide resolved
packages/react-native-gesture-handler/apple/RNGestureHandler.mm
Outdated
Show resolved
Hide resolved
packages/react-native-gesture-handler/apple/Handlers/RNFlingHandler.m
Outdated
Show resolved
Hide resolved
| [_gestureHandler handleGesture:self fromReset:fromReset manualActivation:NO]; | ||
| } |
There was a problem hiding this comment.
Is this override needed at all? It doesn't change anything compared to the base implementation
There was a problem hiding this comment.
Yes we need the default value one so that the selector works correctly (as in #3983) and we need to overload the one with all the arguements so that when called from stateManager it initializes the handler specific values (like previousTime in LongPress or scale in pinch).
There was a problem hiding this comment.
In the PR you linked, the base one ((void)handleGesture:(nonnull id)recognizer;) is implemented once in the base class. Here, handlers have both - the base one, and the extended one - defined. The base one is exactly the same as the implementation in GestureHandler. Hover doesn't seem to override either, and I assume the selector works there, no?
There was a problem hiding this comment.
ah right, I assume the override in Fling was used at some, but not anumore. Removed altogether in 1246b9d. The other overrides are needed though.
There was a problem hiding this comment.
Why are they needed? They don't differ from the default implementation.
| { | ||
| previousTime = CACurrentMediaTime(); | ||
| [_gestureHandler handleGesture:recognizer fromReset:NO]; | ||
| [self handleGesture:recognizer fromReset:fromReset manualActivation:NO]; | ||
| } |
| { | ||
| [self handleGesture:recognizer fromReset:fromReset manualActivation:NO]; | ||
| } |
| { | ||
| [self handleGesture:recognizer fromReset:fromReset manualActivation:NO]; | ||
| } |
| } else if ( | ||
| state == RNGestureHandlerStateEnd && _lastState != RNGestureHandlerStateActive && !manualActivation && | ||
| !_manualActivation) { |
There was a problem hiding this comment.
What's the end result here? onFinalize is invoked with didSucceed: true without going through activate/deactivate?
There was a problem hiding this comment.
Yes, this unifies the behaviour across platforms. Previously we forced calling activation callback. Do we want to handle it differently?
There was a problem hiding this comment.
I think if the user called onFinalize, rather than onFail he wants to have the onSuccess flag set to true (for whaterver reason), but if he wanted the activation callback he would have called StateManager.activate directly.
There was a problem hiding this comment.
What do you mean by
I think if the user called
onFinalize, rather thanonFail
I don't recall having onFail in that context.
There was a problem hiding this comment.
Sorry, I meant when they called GestureStateManager.finalize() rather than GestureStateManager,fail()
There was a problem hiding this comment.
Is didSucceed: true consistent across platforms here?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/react-native-gesture-handler/apple/Handlers/RNFlingHandler.m
Outdated
Show resolved
Hide resolved
packages/react-native-gesture-handler/apple/Handlers/RNFlingHandler.m
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| previousTime = CACurrentMediaTime(); | ||
| [_gestureHandler handleGesture:recognizer fromReset:NO]; | ||
| [_gestureHandler handleGesture:recognizer fromReset:NO manualActivation:NO]; |
There was a problem hiding this comment.
previousTime is no longer updated in the handleGesture: action method, but getDuration relies on previousTime - startTime. Since handleGesture: is the selector registered as the recognizer's action, duration may stay near 0 or stale for the whole gesture. Consider updating previousTime here as well (or route handleGesture: through the fromReset:manualActivation: path that sets it).
| [_gestureHandler handleGesture:recognizer fromReset:NO manualActivation:NO]; | |
| [self handleGesture:recognizer fromReset:NO manualActivation:NO]; |
packages/react-native-gesture-handler/apple/Handlers/RNFlingHandler.m
Outdated
Show resolved
Hide resolved
…ndler.m Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return self; | ||
| } | ||
|
|
||
| - (void)handleGesture:(NSPanGestureRecognizer *)gestureRecognizer | ||
| { | ||
| [_gestureHandler handleGesture:self]; | ||
| } | ||
|
|
||
| - (void)mouseDown:(NSEvent *)event | ||
| { | ||
| [super mouseDown:event]; |
There was a problem hiding this comment.
In the macOS implementation of RNBetterSwipeGestureRecognizer, the recognizer is still created with initWithTarget:self action:@selector(handleGesture:), but handleGesture: is no longer present (it was removed in this change). When AppKit fires the action, this will crash with an unrecognized selector. Please re-add -handleGesture: to forward to _gestureHandler, or change the target/action to _gestureHandler like the iOS version.
| - (void)handleGesture:(nonnull id)recognizer fromReset:(BOOL)fromReset; | ||
| - (void)handleGesture:(nonnull id)recognizer fromReset:(BOOL)fromReset fromManual:(BOOL)fromManual; | ||
| - (void)handleGesture:(nonnull id)recognizer inState:(RNGestureHandlerState)state; | ||
| - (void)handleGesture:(nonnull id)recognizer inState:(RNGestureHandlerState)state fromManual:(BOOL)fromManual; |
There was a problem hiding this comment.
The new selector label fromManual: is ambiguous given the existing manualActivation flag on RNGestureHandler. It’s hard to tell whether this refers to “manual activation” vs “manual state changes via setGestureState”. Consider renaming this parameter/selector to something more specific (e.g. fromManualStateChange: or fromStateManager:) to avoid API confusion.
There was a problem hiding this comment.
I agree with @copilot , my first thought was fromManualStateChange, but second one is also ok.
…dler API (#3996) The `fromManual:` selector label was ambiguous — it could be confused with the existing `manualActivation` flag on `RNGestureHandler`. Rename it to `fromManualStateChange:` to clearly convey that this parameter tracks state changes triggered via `setGestureState` (the manual state manager path), not manual activation. - **`RNGestureHandler.h`** — updated all three method declarations - **`RNGestureHandler.mm`** — updated implementations and all call sites - **`RNGestureHandlerModule.mm`** — updated `setGestureState` call sites (the only places passing `YES`) - **All handler files** (`RNFlingHandler`, `RNHoverHandler`, `RNLongPressHandler`, `RNPanHandler`, `RNPinchHandler`, `RNRotationHandler`, `RNTapHandler`) — updated forwarding declarations and call sites <!-- START COPILOT CODING AGENT TIPS --> --- 🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. [Learn more about Advanced Security.](https://gh.io/cca-advanced-security) --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: m-bert <63123542+m-bert@users.noreply.github.com>
Description
Gestures always call activation callback upon finalizing, regardless of the previous state. This is intended for normal gestures, however for manually handled gestures we don't want to call it. This PR unifies the behaviour across platforms, GestureManager.finalize() won't call activation callback.
Test plan
Tested on the state manager example