Add buttons to gestures #30339

dkwingsmt · 2019-04-01T21:50:43Z

Overview

This PR add support for buttons to tap, long press, drag, and double tap gestures.

Their existing callbacks now only respond to primary button. For example, onLongPressStart will only respond when the long press is done by touch, stylus-touch, or LMB. This is supposed to cover most daily uses (without changing existing code).

Tap also has a "secondary" version of each callback, such as onSecondaryTapDown.

The following is a summary table for callbacks after the PR:

Recognizer	`on*` (for primary)	`onSecondary*`
Tap	✔️	✔️
LongPress	✔️
Drag	✔️
DoubleTap	✔️

Design reasoning

This chapter explains the reason for choosing this design using an example of TapGestureRecognizer.onTap as we try to fix the issue where buttons incorrectly respond to RMB clicks.

First, the existing callbacks of GestureDetector must only respond to primary events. Most (if not all) existing widgets use callbacks while assuming the gestures received are all primary. For example, a Button assigns GestureDetector with a onTap that triggers its action without reassuring the buttons of the incoming gesture. Unless we limit the existing callbacks to primary events, we will have to change the code of Button, as well as countless external code.

It also means that we need separate onSecondaryTap* callbacks to handle secondary taps.

Second, because RawGestureDetector receives a map from recognizer type to recognizer factory, there can only be one TapGR instance. It means that TapGR can not be configurable, which leaves us with three options:

There is one TapGR class, whose callbacks respond to all tap events, and dispatching is done by GestureDetector.
There is one TapGR class, whose callbacks are already post-dispatch (i.e. it has callbacks for primary as well as callbacks for secondary).
There is another SecondaryTapGR, who has callback onTap which responds to secondary taps.

Option 1 is infeasible, because the recognizer must filter events in isAllowedPointer instead of deferring it to callbacks. To explain this, say we have a component which only listens to primary drag and secondary tap. The two kinds of events are exclusive from each other and should not cause arena competition, thus both of them should be accepted immediately (for example, onTapDown should be fired immediately at Down events). However, now that the exclusiveness is only known by callbacks, both gestures will try to compete upon every Down events, which is a behavioral degradation.

Between the remaining two options, we preferred option 2 over 3 for two reasons:

It uses fewer classes.
It makes TapGR a centralized class to handle "any kinds of tap gestures", which is an easier concept to understand.

Major changes

Existing gestures require primary button

All existing callbacks of the following gesture recognizers now require "primary button rule":

TapGestureRecognizer
LongPressGestureRecognizer
DragGestureRecognizer (and its variants Pan, HorizontalDrag and VerticalDrag)
DoubleTapGestureRecognizer

The same change is applied to their counterparts in GestureDetector.

New "onSecondary" tap gesture callbacks

TapGestureRecognizer also have a series of "onSecondary" callbacks:

TapGestureRecognizer: onSecondaryTap{Down,Up,Cancel}

Recognizers only compete on events if there are any related callbacks

A recognizer will compete on primary events only if there are non-null primary callbacks. Similarly, TapGR will compete on secondary events only if there are non-null secondary callbacks.

This should not affect real-world code, but some tests used to create recognizers with no callbacks in order to form competence to the test target. These tests have been updated to assign an empty callback.

Minor changes

New utility functions

smallestButton: basically rightmost set bit
isSingleButton: returns true if the target buttons has one and only one set bit

New constant

kSecondaryButton: an alias to kSecondaryMouseButton and kPrimaryStylusButton

TestPointer can configure buttons

TestPointer now has a modifiable property int buttons, which will be assigned to PointerDownEvents and PointerMoveEvents that it generates.

Fix: Semantic long press dispatches all callbacks

RawGestureDetector._handleSemanticsLongPress used to only dispatch onLongPress. It now dispatches onLongPress{Start,End,Up} as well.

Related Issues

Tests

New tests: each of tap, long press, drag, and double tap gesture recognizers has the following new tests:

Recognizer status is correctly reset when consistent button restriction is violated.
Gestures are correctly dispatched to related callbacks.
Gestures will not compete in events which they have no related callbacks to.

New tests: the following new utility functions work property:

smallestButton
isSingleButton

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

Hixie · 2019-04-01T23:29:42Z

If I'm reading this correctly, this treats a left-click the same as a right-click. Is that what we want?
(Is the idea to add logic to distinguish left from right in a later PR?)

Hixie · 2019-04-01T23:30:04Z

packages/flutter/lib/src/gestures/tap.dart

-  TapDownDetails({ this.globalPosition = Offset.zero })
-    : assert(globalPosition != null);
+  /// Arguments [globalPosition] and [buttons] must not be null.
+  TapDownDetails({ this.globalPosition = Offset.zero, this.buttons = 0 })


I think before we make this change we should update the Android and iOS logic to treat fingers-touching-surface and stylus-touching-surface as button=1.

For now the number of set bits is not checked, so a buttons = 0 will also work as long as it stays 0.

Well you're right, for it to work without the change, the callback also needs device.

@Hixie The plan is to apply it to long tap, drag and double click later.

Since they share a common superclass, does it make sense to fix them all at once?

It's a good idea, but technically the concept of PrimaryPointerGR does not necessarily imply "restricted to single button". Do you think I should implement it in PrimaryPointerGR or a new intermediate class?

Hixie · 2019-04-01T23:30:49Z

packages/flutter/lib/src/gestures/tap.dart

  final Offset globalPosition;
+
+  /// The buttons pressed when the pointer contacted the screen.
+  final int buttons;


I think for this to make sense we should require that it be a single button, not multiple buttons. Otherwise pressing left+right on a mouse will fail 99% of the time (because one will be detected a split second before the other) and work 1% of the time.

I can limit it to "zero or one button" for #30339 (comment). What do you think?

I think it should be one button, and we should never have an event with zero buttons.

dkwingsmt · 2019-04-01T23:32:54Z

@Hixie The idea is, to put it simpler,

Events	Interpretation
Left down, left up	a tap gesture that sends "left button"
Right down, right up	a tap gesture that sends "right button"
Left down, right down, right up	illegal tap

So that both "left click" and "right click" will trigger the same TapGR, but the callback can distinguish with details provided (which doesn't help onTap unfortunately)

Hixie · 2019-04-01T23:51:09Z

Should this be specific to tap, or should it apply to other primary-pointer recognizers like drag, or long press, or double tap?

dkwingsmt · 2019-04-01T23:57:11Z

@Hixie The plan is to apply it to long tap, drag and double click later.

Hixie · 2019-04-02T00:26:26Z

I don't think it makes sense for us to fire onTap when you right-click. Nobody is going to be checking whether it's a right mouse button or not.

IMHO, GestureDetector's onTap/onTapDown/etc should fire just for left clicks and taps.

GestureDetector can then have an additional onRightClick that fires for right clicks. (Maybe call it onSecondary or something more general.)

We can easily implement this by having the gesture recognizer take a constructor argument that says what button it supports.

Hixie · 2019-04-02T01:14:55Z

We talked about this in person. RawGestureDetector's API makes it awkward to do what I suggested.
Instead we considered various ideas and @dkwingsmt came up with the idea of "forking" the recognizer's onTapUp callback so that after calling the given onTapUp, it would look at the button and call onTap if it's 1 and onSecondaryTap if it's 2. (And then we don't actually hook into onTap directly at all, we "fake" it.)

Not clear if it'll work or not, we'll have to try it and see how it comes out.

packages/flutter/lib/src/gestures/long_press.dart

dkwingsmt · 2019-04-24T21:21:58Z

@Hixie I wrote a chapter of "design reasoning" in PR description, which explained why other approaches (AFAIW) won't work except for two. The other viable design is to use different gesture classes on top of configurable super class,

class BaseTapGestureRecognizer {
  BaseTapGestureRecognizer({ this.button, });
}

class TapGestureRecognizer extends BaseTapGestureRecognizer {
  TapGestureRecognizer() : super(button: kPrimaryButton);
}

class SecondaryTapGestureRecognizer extends BaseTapGestureRecognizer {
  SecondaryTapGestureRecognizer() : super(button: kSecondaryButton);
}

It will reduce the number of callbacks per class, but it will likely cause the increase to number of classes, especially if we're to support secondary button to drag gestures, then we'll need to add a secondary series to v-drag, h-drag, and pan.

gspencergoog

gspencergoog · 2019-04-26T16:04:52Z

packages/flutter/lib/src/gestures/events.dart

-///  * [kPrimaryMouseButton]: an alias of this constant when used by mouse.
+///  * [kSecondaryButton], which describes a cross-device behavior of
+///    "secondary operation".
 const int kPrimaryButton = 0x01;


It's too bad about all of these global constants. I wish they were in a class. Probably that ship has sailed since it would be a massive breaking change, but I thought I'd just put that out there.

gspencergoog · 2019-04-26T16:08:57Z

packages/flutter/lib/src/gestures/tap.dart

-        resolve(GestureDisposition.accepted);
-        _checkUp();
-      }
+      _checkUp();


Got it, makes sense. Thanks for the explanation.

gspencergoog · 2019-04-26T16:11:05Z

packages/flutter/lib/src/gestures/tap.dart

    properties.add(DiagnosticsProperty<Offset>('finalPosition', _finalPosition, defaultValue: null));
    properties.add(FlagProperty('sentTapDown', value: _sentTapDown, ifTrue: 'sent tap down'));
+    // TODO(tongmu): Uncomment this line and update related tests.
+    // properties.add(IntProperty('initialButtons', _initialButtons));


Just delete these two lines: don't check in commented out code:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-abandonware

This reverts commit 727e7e8.

This reverts commit 8fd7fa4.

…)" This reverts commit 8fd7fa4.

* Revert "Revert "Add buttons to gestures (#30339)" (#31801)" This reverts commit 8fd7fa4. * Synthesise kPrimaryButton for unknown devices * Change TestPointer to a better API

Add button regulation to TapGR

f7deebc

dkwingsmt requested review from Hixie and goderbauer April 1, 2019 21:50

googlebot added the cla: yes label Apr 1, 2019

dkwingsmt changed the title ~~Add button regulation to TapGestureRecognizer~~ Force consistent button by TapGestureRecognizer Apr 1, 2019

Hixie reviewed Apr 1, 2019

View reviewed changes

dkwingsmt added 4 commits April 2, 2019 14:10

Simplify logic using stopTrackingPointer

9b87849

Require single button

1607e12

Revert changes of _checkCancel because it's no longer necessary

eba810c

Revert a missing change

12ac20e

goderbauer added a: desktop Running on desktop framework flutter/packages/flutter repository. See also f: labels. labels Apr 2, 2019

dkwingsmt added the work in progress; do not review label Apr 4, 2019

dkwingsmt mentioned this pull request Apr 4, 2019

Touching the screen should be represented as 0x01 in buttons #30454

Open

Piinks changed the title ~~Force consistent button by TapGestureRecognizer~~ WIP Force consistent button by TapGestureRecognizer 🚧 Apr 4, 2019

dkwingsmt mentioned this pull request Apr 4, 2019

WIP Add buttons to all PointerMoveEvent and PointerDownEvent #30408

Closed

9 tasks

dkwingsmt added 7 commits April 4, 2019 17:08

Small refactor to TapGR so that _checkUp is a more precise name

5bba019

Merge remote-tracking branch 'upstream/master' into tap-buttons

14950f4

Long press requires button consistancy and sigular button

490a167

Merge remote-tracking branch 'upstream/master' into tap-buttons

6504723

Add buttons to drag details

91051eb

Merge remote-tracking branch 'upstream/master' into tap-buttons

af5cb92

Add button support to monodrag

8e2e3bd

dkwingsmt added 7 commits April 22, 2019 14:45

Apply "early button filtering" to tap

a9c72ba

Apply "early button filtering" to longpress

93d9be6

Apply "early button filtering" to drag

3baaa1e

Apply "early button filtering" to doubleclick

5b03985

Fix tap test

6beb96c

Remove buttons from details

93dfd75

Change terminate to cancel to reduce different terms.

6fe1658

Hixie reviewed Apr 23, 2019

View reviewed changes

packages/flutter/lib/src/gestures/long_press.dart Outdated Show resolved Hide resolved

dkwingsmt added 6 commits April 23, 2019 10:37

Improve document

bbf952b

Remove unnecessary initial button check

e8835c9

Fix test

0f8b9c3

Merge remote-tracking branch 'upstream/master' into tap-buttons

fe0d4d4

Fix tests

22b0293

Add doc for callback check

4cb90e4

dkwingsmt changed the title ~~WIP: Add buttons to gestures~~ Add buttons to gestures Apr 23, 2019

dkwingsmt removed the work in progress; do not review label Apr 23, 2019

gspencergoog approved these changes Apr 26, 2019

View reviewed changes

dkwingsmt added 2 commits April 29, 2019 09:54

Merge remote-tracking branch 'upstream/master' into tap-buttons

e78fdec

Change commented code to TODO

170e1da

dkwingsmt merged commit 727e7e8 into flutter:master Apr 29, 2019

dkwingsmt deleted the tap-buttons branch April 29, 2019 17:46

jonahwilliams pushed a commit that referenced this pull request Apr 29, 2019

Revert "Add buttons to gestures (#30339)"

fda17bc

This reverts commit 727e7e8.

jonahwilliams mentioned this pull request Apr 29, 2019

Revert "Add buttons to gestures" #31801

Merged

jonahwilliams pushed a commit that referenced this pull request Apr 29, 2019

Revert "Add buttons to gestures (#30339)" (#31801)

8fd7fa4

This reverts commit 727e7e8.

dkwingsmt added a commit that referenced this pull request Apr 29, 2019

Revert "Revert "Add buttons to gestures (#30339)" (#31801)"

f78859c

This reverts commit 8fd7fa4.

dkwingsmt added a commit to dkwingsmt/flutter that referenced this pull request Apr 30, 2019

Revert "Revert "Add buttons to gestures (flutter#30339)" (flutter#31801…

d1a22c2

…)" This reverts commit 8fd7fa4.

dkwingsmt mentioned this pull request Apr 30, 2019

Redo: Add buttons to gestures #31819

Merged

9 tasks

github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021

Add buttons to gestures #30339

Add buttons to gestures #30339

Uh oh!

Conversation

dkwingsmt commented Apr 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Design reasoning

Major changes

Existing gestures require primary button

New "onSecondary" tap gesture callbacks

Recognizers only compete on events if there are any related callbacks

Minor changes

New utility functions

New constant

TestPointer can configure buttons

Fix: Semantic long press dispatches all callbacks

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Hixie commented Apr 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt commented Apr 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hixie commented Apr 1, 2019

Uh oh!

dkwingsmt commented Apr 1, 2019

Uh oh!

Hixie commented Apr 2, 2019

Uh oh!

Hixie commented Apr 2, 2019

Uh oh!

Uh oh!

dkwingsmt commented Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dkwingsmt commented Apr 1, 2019 •

edited

Loading

dkwingsmt commented Apr 1, 2019 •

edited

Loading

dkwingsmt commented Apr 24, 2019 •

edited

Loading