feat: cancel contradictory gesture events by MCurran16 · Pull Request #7296 · NativeScript/NativeScript

MCurran16 · 2019-06-02T16:46:09Z

PR Checklist

The PR title follows our guidelines: https://github.com/NativeScript/NativeScript/blob/master/CONTRIBUTING.md#commit-messages.
There is an issue for the bug/feature this PR is for. To avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it.
You have signed the CLA.
All existing tests are passing: https://github.com/NativeScript/NativeScript/blob/master/DevelopmentWorkflow.md#running-unit-tests.
Tests for the changes are included - https://github.com/NativeScript/NativeScript/blob/master/WritingUnitTests.md.

What is the current behavior?

When you have a tap and double tap gesture recognizer on a view and you attempt to double tap, the tap handler fires. The tap gesture event should not fire if you're attempting a double tap.

What is the new behavior?

For both platforms, the tap event will not be fired when you're attempting a doubleTap gesture.

Implements #7281

Additional Context

@MartoYankov

For android, I decided to not use onSingleTapConfirmed due to the delay that is described in this SO answer: https://stackoverflow.com/a/48844866 . I did try using onSingleTapConfirmed and there was consistently a noticeable delay and sometimes it wasn't even fired.

For iOS, the solution in the attached issue was implemented.

I also tried to get those files I changed to follow the NS code convention a bit more... hoping that's okay!

BREAKING CHANGES:

Old behavior:

iOS/Android:
- double tap: child tap -> parent tap -> child double tap -> parent double tap
- tap: child tap -> parent tap

New behavior:

iOS
- double tap: child double tap
- tap: child tap
Android
- double tap: child double tap -> parent double tap
- tap: child tap -> parent tap

Migration steps:
Move event handlers accordingly.

…e target

… gestures-fix * 'gestures-fix' of github.com:MCurran16/NativeScript: Reset tns-<android/ios> for test app refact(iOS): code convention for iOS gestures fix(gestures-android): Handle tap and double tap listeners on the same target Fix simultaneous tap gestures for iOS # Conflicts: # tns-core-modules/ui/gestures/gestures.android.ts

tns-core-modules/ui/gestures/gestures.android.ts

MartoYankov · 2019-06-10T07:54:52Z

Also, I think this should be considered as behavior breaking change.

…st event up and second event on down

edusperoni · 2019-06-10T14:39:33Z

tns-core-modules/ui/gestures/gestures.ios.ts

+        // If both gesture recognizers are of type UITapGestureRecognizer, do not allow
+        // simultaneous recognition.
+        if (gestureRecognizer instanceof UITapGestureRecognizer && otherGestureRecognizer instanceof UITapGestureRecognizer) {
+            return false;


I imagine this is going to change the behavior where a tap was recognized by both parent and child. This change makes it so only one tap gesture is recognized on iOS, but seems to be unchanged on Android, which may lead to inconsistent behavior that was previously patched out (for more context see #7310)

Old behavior:

iOS and Android:
double tap: child tap -> parent tap -> child double tap -> parent double tap
tap: child tap -> parent tap

New behavior (iOS):
double tap: child double tap
tap: child tap

New behavior (Android):
double tap: child double tap -> parent double tap
tap: child tap -> parent tap

@edusperoni It is already inconsistent between the platforms and this PR was not intending to fix child/parent intercepting/cancelling/bubbling.

My understanding of the iOS implementation was that the UIGestureRecognizerDelegateImpl was for a single view as the NS view is passed in which can be seen here. It sounds like that is an incorrect understanding.

Actually, the current implementation of events (and bubbling) seem to be consistent in both platforms (all events bubble and the same events trigger).

I'm aware this PR does not intend to fix child/parent bubbling, I'm pointing out that this is a behavior breaking change. The same delegate is used by all views, as seen here, so that line will affect child/parent bubbling and break consistency.

Okay that is fair. I'm understanding now.

In your mind, where do we go from here? What would you like me to do?

Maybe @MartoYankov can weigh in on next steps?

I don't believe this is a negative change, but I can think of 3 options:

if we're detecting a tap gesture, bubble this information up to views on android and prevent tap recognition up the chain

NativeScript/tns-core-modules/ui/core/view/view.android.ts

Lines 397 to 399 in cf533a7

if (this.parent instanceof View) {

this.parent.handleGestureTouch(event);

}

Compare targets and required taps in if (gestureRecognizer instanceof UITapGestureRecognizer && otherGestureRecognizer instanceof UITapGestureRecognizer) {, so events will keep bubbling

Document it as a breaking change

I believe 1 is out of scope of this PR and 2 may add unneeded complexity. Maybe the best option is really 3 until we reach a consensus on how gesture disambiguation should work in NS?

The impact of the change should be really low. The only plugin I know to handle multiple taps (nativescript-windowed-modal) only does it to prevent a parent tap when the child was tapped (using Hit Testing). My own plugin (nativescript-ng-ripple, soon to become one with nativescript-ripple) uses touch, so it probably won't be affected.

We discussed this internally and we agree that 3 is probably the best option for now.

@MCurran16 @edusperoni @MartoYankov Seems we have a problem with this specific change in UIGestureRecognizerDelegateImpl.gestureRecognizerShouldRecognizeSimultaneouslyWithGestureRecognizer(...) that breaks ActionBar usage scenario in all master/detail templates so I am reverting it for the time being -- see #7400.

MartoYankov · 2019-06-12T05:48:42Z

test

MCurran16 added 9 commits May 23, 2019 09:08

Fix simultaneous tap gestures for iOS

a76e954

fix(gestures-android): Handle tap and double tap listeners on the sam…

c7eca4e

…e target

refact(iOS): code convention for iOS gestures

4e59f9f

Reset tns-<android/ios> for test app

8f513ee

Fix simultaneous tap gestures for iOS

6c7cbdd

fix(gestures-android): Handle tap and double tap listeners on the sam…

71ff350

…e target

refact(iOS): code convention for iOS gestures

3e6588b

Reset tns-<android/ios> for test app

3c31185

cla-bot bot added the cla: yes label Jun 2, 2019

MCurran16 changed the title ~~Gestures fix~~ enhance(iOS/Android): Fix contradictory gesture events Jun 2, 2019

MCurran16 added 3 commits June 3, 2019 11:22

Merge branch 'master' into gestures-fix

fa6d322

Merge branch 'master' into gestures-fix

d5c9fa2

Merge branch 'master' into gestures-fix

054213d

MartoYankov reviewed Jun 10, 2019

View reviewed changes

tns-core-modules/ui/gestures/gestures.android.ts Outdated Show resolved Hide resolved

MCurran16 added 2 commits June 10, 2019 09:18

refactor(android): handle single tap on up and double tap between fir…

0f5b87e

…st event up and second event on down

Merge branch 'master' into gestures-fix

d58b8b7

edusperoni reviewed Jun 10, 2019

View reviewed changes

MartoYankov changed the title ~~enhance(iOS/Android): Fix contradictory gesture events~~ feat: cancel contradictory gesture events Jun 12, 2019

MartoYankov added the e2e test needed label Jun 12, 2019

MartoYankov approved these changes Jun 12, 2019

View reviewed changes

MartoYankov added the docs needed Additional documentation on this issue/PR is needed label Jun 12, 2019

MartoYankov merged commit b8a82f2 into NativeScript:master Jun 12, 2019

MCurran16 deleted the gestures-fix branch June 12, 2019 12:56

manoldonev mentioned this pull request Jun 24, 2019

fix(ios-actionbar): parent/child gesture recognition #7400

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: cancel contradictory gesture events#7296

feat: cancel contradictory gesture events#7296
MartoYankov merged 14 commits intoNativeScript:masterfrom
MCurran16:gestures-fix

MCurran16 commented Jun 2, 2019 •

edited by MartoYankov

Loading

Uh oh!

Uh oh!

MartoYankov commented Jun 10, 2019

Uh oh!

edusperoni Jun 10, 2019

Uh oh!

MCurran16 Jun 10, 2019 •

edited

Loading

Uh oh!

edusperoni Jun 10, 2019

Uh oh!

MCurran16 Jun 10, 2019

Uh oh!

edusperoni Jun 10, 2019 •

edited

Loading

Uh oh!

MartoYankov Jun 11, 2019

Uh oh!

manoldonev Jun 24, 2019

Uh oh!

MartoYankov commented Jun 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	if (this.parent instanceof View) {
	this.parent.handleGestureTouch(event);
	}

Uh oh!

Conversation

MCurran16 commented Jun 2, 2019 • edited by MartoYankov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

What is the current behavior?

What is the new behavior?

Additional Context

Uh oh!

Uh oh!

MartoYankov commented Jun 10, 2019

Uh oh!

edusperoni Jun 10, 2019

Choose a reason for hiding this comment

Uh oh!

MCurran16 Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edusperoni Jun 10, 2019

Choose a reason for hiding this comment

Uh oh!

MCurran16 Jun 10, 2019

Choose a reason for hiding this comment

Uh oh!

edusperoni Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartoYankov Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

manoldonev Jun 24, 2019

Choose a reason for hiding this comment

Uh oh!

MartoYankov commented Jun 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MCurran16 commented Jun 2, 2019 •

edited by MartoYankov

Loading

MCurran16 Jun 10, 2019 •

edited

Loading

edusperoni Jun 10, 2019 •

edited

Loading