Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update PinchGestureHandler behavior #1613

Open
wants to merge 7 commits into
base: fix-handler-inconsistency
Choose a base branch
from

Conversation

j-piasecki
Copy link
Member

@j-piasecki j-piasecki commented Aug 31, 2021

Description

PinchGestureHandler has been working differently on iOS and on Android.
On Android PinchGestureHandler was transitioning to BEGAN state immediately after first pointer down and transitioning to END if the pointer was pulled up.
On iOS it is transitioning to BEGAN just before it activates and was allowing to continue the gesture after one of the pointers was pulled up.

This PR modifies both of the platforms to be much more consistent:

  • (Android) add requirement for at least two active pointers on screen before PinchGH can begin
  • (Android) add condition in Orchestrator to not send state change event from UNDETERMINED state only to BEGAN state and ignore others. Internally the logic stays the same but it prevents sending UNDETERMINED -> FAILED or CANCELLED which IMO is the correct behavior (it wasn't required before because all handlers went to BEGAN state on touch start, but this PR allows PinchGH to stay in the UNDETERMINED state).
  • (Android) make PinchGH mimic the iOS behavior by default - when the gesture is active it will not end while there is a pointer on the component inside gesture handler
  • (Android) add new canActivateAlongsideAlreadyActive method to base GestureHandler class. While shouldRecognizeSimultaneously is called on handler activation to cancel other handlers that cannot be recognized at the same time, when a new pointer appears on the screen it may extract previously cancelled handlers again. This method adds option to prevent a handler from activating in that case. (Fixes a bug where ScrollView would activate when PinchGH was inside of it with endOnFingerRelease set to false)
  • (Both platforms) add a new endOnFingerRelease prop to PinchGH which changes behavior of the handler to match the original Android behavior - gesture will end when any of the fingers is released

Test plan

Tested on the Example app

Copy link
Member

@jakub-gonet jakub-gonet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change needs one or two new examples and docs update.

Comment on lines 333 to 341
// this differs from shouldRecognizeSimultaneously because this method is called to check if there
// is an already active handler that prevents this from activating, while shouldRecognizeSimultaneously
// is called to check which handlers to cancel when a handler becoming active
// main reason behind this method is the fact that when a handler activates it cancels all other
// handlers that cannot be recognized simultaneously but when another pointer is placed on screen
// while it is already active it has no influence over the ones extracted from the new pointer.
// we could just use shouldRecognizeSimultaneously in this case but that would break things like
// separate draggable objects without `simultaneousHandlers` set, when the problem it's trying to
// solve is a scrollview activating when there is active PinchGH inside
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move this comment to interface docs and use /** comment.

Suggested change
// this differs from shouldRecognizeSimultaneously because this method is called to check if there
// is an already active handler that prevents this from activating, while shouldRecognizeSimultaneously
// is called to check which handlers to cancel when a handler becoming active
// main reason behind this method is the fact that when a handler activates it cancels all other
// handlers that cannot be recognized simultaneously but when another pointer is placed on screen
// while it is already active it has no influence over the ones extracted from the new pointer.
// we could just use shouldRecognizeSimultaneously in this case but that would break things like
// separate draggable objects without `simultaneousHandlers` set, when the problem it's trying to
// solve is a scrollview activating when there is active PinchGH inside
// This differs from shouldRecognizeSimultaneously because this method is called to check if there
// is an already active handler that prevents this from activating, while shouldRecognizeSimultaneously
// is called to check which handlers to cancel when a handler becoming active.
// The main reason behind this method is the fact that when a handler activates it cancels all other
// handlers that cannot be recognized simultaneously but when another pointer is placed on the screen
// while it is already active it has no influence over the ones extracted from the new pointer.
// We could use shouldRecognizeSimultaneously in this case but that would break things like
// separate draggable objects without `simultaneousHandlers` set, when the problem it's trying to
// solve is a scrollview activating when there is active PinchGH inside

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I don't fully understand this part:

// We could use shouldRecognizeSimultaneously in this case but that would break things like
// separate draggable objects without `simultaneousHandlers` set, when the problem it's trying to
// solve is a scrollview activating when there is active PinchGH inside

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that (because of the similar behavior suggested by name) one could think that the same effect could be accomplished by additional logic built upon simultaneousHandlers, but that would break existing functionality. I renamed this method and updated logic to reflect that change and it should be much more understandable now. Also added the comment in 3dbec81.

Comment on lines 99 to 103
// when there is an active PinchGH lifting finger up and placing it back on the screen activates
// scrollview alongside PinchGH resulting in weird behavior
if (handler is PinchGestureHandler) {
return false
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why it relates only to pinch? Wouldn't pan with maxPointers=2 behave in a similar way?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the name of that method to needsToPreventOtherHandlerFromActivating and updated the logic associated with it in d3ba7a1. Now it allows to add specific check in every gesture, that allows us to update all multi-touch gestures in the future.

ios/Handlers/RNPinchHandler.m Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants