Skip to content

Add onTertiaryTap* to TapGestureRecognizer and GestureDetector - #62788

Merged
fluttergithubbot merged 11 commits into
flutter:masterfrom
simpleclub-extended:tertiary-tap
Aug 8, 2020
Merged

Add onTertiaryTap* to TapGestureRecognizer and GestureDetector#62788
fluttergithubbot merged 11 commits into
flutter:masterfrom
simpleclub-extended:tertiary-tap

Conversation

@creativecreatorormaybenot

@creativecreatorormaybenot creativecreatorormaybenot commented Aug 3, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

This PR aims to support the middle mouse button in Flutter (which is currently the only tertiary button).
The naming decision and the decision to omit the tap event were derived from #62488 (comment).

cc @gspencergoog

Related Issues

Tests

I added the following tests:

  • Tap gesture tests for the tertiary handlers.
  • GestureDetector tests in all relevant files.
  • Regression test for the bug fixed.

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 read the Tree Hygiene wiki page, which explains my responsibilities.
  • 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

  • No, no existing tests failed, so this is not a breaking change.

@flutter-dashboard flutter-dashboard Bot added a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) framework flutter/packages/flutter repository. See also f: labels. labels Aug 3, 2020
@gspencergoog

gspencergoog commented Aug 3, 2020

Copy link
Copy Markdown
Contributor

@dkwingsmt Can you review this, since I'm out this week?

@goderbauer

goderbauer commented Aug 3, 2020

Copy link
Copy Markdown
Member

Fly-by comment: It looks like one of the new tests is failing on cirrus.

@creativecreatorormaybenot

creativecreatorormaybenot commented Aug 3, 2020

Copy link
Copy Markdown
Contributor Author

@goderbauer Thank you! Sorry that I was not thorough enough.

However, I do not understand why the Tap down occurs after kPressTimeout (tertiary) test is failing.
Can someone help me with that?

@creativecreatorormaybenot

creativecreatorormaybenot commented Aug 3, 2020

Copy link
Copy Markdown
Contributor Author

(I am aware that tests-widgets are failing, however, I do not understand why that one test is failing)

@dkwingsmt dkwingsmt self-assigned this Aug 3, 2020
dkwingsmt
dkwingsmt requested changes Aug 3, 2020
View reviewed changes

@dkwingsmt dkwingsmt left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  1. Since mouse is not the only device that can use a tertiary button, but also stylus, using kMiddleMouseButton is not accurate. Can you create a new kTertiaryButton, and replace all uses of kMiddleMouseButton in your new code to that?
  2. We probably want to also add a onTertiaryTap callback just like the primary and secondary button.
  3. The reason why the test fails is because the tertiary tap has no competition, which means the tertiary tap is accepted immediately on the down event. A primary or secondary tap, however, is competed by long press (thus the 2 long press callbacks) and must wait kPressTimeout before being accepted, which is interrupted by the drag. The solution is probably adding a tertiary long press callback. Also since you're confused by it, it'd be best if you can add a comment to explain this at the 2 long press callbacks.

Everything else looks good.

@creativecreatorormaybenot

creativecreatorormaybenot commented Aug 4, 2020

Copy link
Copy Markdown
Contributor Author

@dkwingsmt Thank you so much for the review!

About 2. and 3.: I think that @gspencergoog pointed out that we might not want to have the other callbacks for tertiary pointers, #62488 (comment).

What you said is logical to me - I just want to make sure that we are clear on that 👍🏼

@dkwingsmt

dkwingsmt commented Aug 4, 2020

Copy link
Copy Markdown
Contributor

Sounds good :)

@creativecreatorormaybenot

creativecreatorormaybenot commented Aug 4, 2020

Copy link
Copy Markdown
Contributor Author

@dkwingsmt So I think it is just fixing 1. now and adjusting the tests to address that there are no long presses?

@dkwingsmt

dkwingsmt commented Aug 4, 2020

Copy link
Copy Markdown
Contributor

I think he didn't say we shouldn't add the tertiary version of long press. But I don't think we NEED to add that either, at least before real demands come; but then to use the same logic of variants you might still need to create a private one just for this test. I can't think of other ways if we want too reuse the variant logic.

@creativecreatorormaybenot

creativecreatorormaybenot commented Aug 4, 2020

Copy link
Copy Markdown
Contributor Author

@dkwingsmt that sounds good 👍🏽

@creativecreatorormaybenot

creativecreatorormaybenot commented Aug 4, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@dkwingsmt I think I addressed everything.

I only added the tertiary long press callbacks to the LongPressGestureRecognizer but not to GestureDetector because they are not needed there.
Consequently, I used RawGestureDetector instead in the tests that used long presses with variants.

@creativecreatorormaybenot creativecreatorormaybenot changed the title Add onTertiaryTap* to TapGestureRecognizer and GestureDetector Add onTertiaryTap* to TapGestureRecognizer and GestureDetector + fix long press callbacks in GestureDetector Aug 4, 2020
@creativecreatorormaybenot creativecreatorormaybenot changed the title Add onTertiaryTap* to TapGestureRecognizer and GestureDetector + fix long press callbacks in GestureDetector Add onTertiaryTap* to TapGestureRecognizer and GestureDetector Aug 4, 2020
@creativecreatorormaybenot

creativecreatorormaybenot commented Aug 4, 2020

Copy link
Copy Markdown
Contributor Author

(I decided to move a fix I did to another PR, see #62897)

@creativecreatorormaybenot

creativecreatorormaybenot commented Aug 5, 2020

Copy link
Copy Markdown
Contributor Author

@dkwingsmt Seems like the tests are all fine now.

If I got the implementation right, this should be done 🙂

dkwingsmt
dkwingsmt approved these changes Aug 5, 2020
View reviewed changes

@dkwingsmt dkwingsmt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, great job!

@dkwingsmt

dkwingsmt commented Aug 7, 2020

Copy link
Copy Markdown
Contributor

Seems a conflict is blocking the merge.

@creativecreatorormaybenot

creativecreatorormaybenot commented Aug 8, 2020

Copy link
Copy Markdown
Contributor Author

@dkwingsmt resolved.

@fluttergithubbot
fluttergithubbot merged commit 96b6aaf into flutter:master Aug 8, 2020
@creativecreatorormaybenot
creativecreatorormaybenot deleted the tertiary-tap branch August 9, 2020 10:51
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@creativecreatorormaybenot @mingwandroid
Add onTertiaryTap* to TapGestureRecognizer and GestureDetector (flutt…
c5182f5 
…er#62788)
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dkwingsmt dkwingsmt dkwingsmt approved these changes

Assignees

@dkwingsmt dkwingsmt

Labels

a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Gesture detect Middle Mouse Button

6 participants

@creativecreatorormaybenot @gspencergoog @goderbauer @dkwingsmt @googlebot @fluttergithubbot