Skip to content

Add an example and update GestureDetector documentation #102360

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.

Sign up for GitHub

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

Jump to bottom
Merged
fluttergithubbot merged 1 commit into from
May 24, 2022
Merged

Add an example and update GestureDetector documentation #102360

fluttergithubbot merged 1 commit into from
May 24, 2022

Conversation

@bleroux
Copy link
Contributor

@bleroux bleroux commented Apr 22, 2022 •
edited
Loading

Description

This PR is a first step towards fixing #74733.
This first step is to add a troubleshooting section and an example to GestureDetector documentation.
It focuses on nested GestureDetector as it is a common use case that leads to misunderstanding (#92103).
The provided example is simple and aims to help users to investigate gesture issues, for instance it introduces debugPrintGestureArenaDiagnostics.
It doesn’t explain why HitTestBehavior.transparent and HitTestBehavior.opaque have no impact in this case.
I will submit another PR to update HitTestBehavior documentation using @Hixie diagrams and explanations from #74733

Related Issue

Fixes #92103

Tests

Add a test for the new example

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos documentation framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Apr 22, 2022
@bleroux bleroux requested a review from Hixie April 22, 2022 09:13
@bleroux bleroux linked an issue Apr 22, 2022 that may be closed by this pull request
GestureDetector.onTap not being hit when behind GestureDetector with HitTestBehavior.translucent #92103
Closed
@bleroux bleroux force-pushed the nested_gesture_detectors branch from aee2167 to 90b9f3b Compare April 25, 2022 04:27
goderbauer
goderbauer reviewed Apr 29, 2022
View reviewed changes
packages/flutter/lib/src/widgets/gesture_detector.dart Outdated
Copy link
Member

@goderbauer goderbauer Apr 29, 2022

Choose a reason for hiding this comment

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

This section is a little confusing/misleading. In particular: onTapDown is always called (not might be called), I think, and if the detector looses in the arena onTapCancel will be called after the arena is already resolved.

goderbauer
goderbauer reviewed Apr 29, 2022
View reviewed changes
examples/api/lib/widgets/gesture_detector/gesture_detector.2.dart Outdated
Comment on lines 51 to 52
Copy link
Member

@goderbauer goderbauer Apr 29, 2022

Choose a reason for hiding this comment

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

Should this (and the other handler) actually only set one variable? Right now, this doesn't actually illustrate that only one handler gets called. They could be called both as one handler would just override the behavior of the previous one.

If we do this, maybe also include a reset button in the sample to remove the boarders.

Copy link
Contributor Author

@bleroux bleroux May 13, 2022

Choose a reason for hiding this comment

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

In my last commit, I replaced this with an enum and used it to set only one variable. I also added a reset button.

packages/flutter/lib/src/widgets/gesture_detector.dart Outdated
Copy link
Member

@goderbauer goderbauer Apr 29, 2022

Choose a reason for hiding this comment

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

Maybe explicitly state here that because the child wins, its onTap is called and the parent's onTap is not called.

Piinks
Piinks reviewed Apr 29, 2022
View reviewed changes
Hixie
Hixie reviewed Apr 29, 2022
View reviewed changes
Hixie
Hixie reviewed Apr 29, 2022
View reviewed changes
Hixie
Hixie reviewed Apr 29, 2022
View reviewed changes
packages/flutter/lib/src/widgets/gesture_detector.dart Outdated
Copy link
Contributor

@Hixie Hixie Apr 29, 2022

Choose a reason for hiding this comment

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

s/Given/Consider/ to make it a full sentence.

Hixie
Hixie reviewed Apr 29, 2022
View reviewed changes
@goderbauer
Copy link
Member

goderbauer commented May 12, 2022

@bleroux Do you still have plans to come back to this one to address the feedback given above?

@bleroux bleroux force-pushed the nested_gesture_detectors branch 3 times, most recently from c560943 to 5d8cbf4 Compare May 13, 2022 12:45
@bleroux bleroux requested a review from goderbauer May 13, 2022 13:18
@goderbauer goderbauer requested a review from Piinks May 18, 2022 21:50
Piinks
Piinks reviewed May 19, 2022
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Thank you for the updates! There are a few grammar and spelling nits below, once those are resolved this should be good to land. :)

packages/flutter/lib/src/widgets/gesture_detector.dart Outdated
Copy link
Contributor

@Piinks Piinks May 19, 2022

Choose a reason for hiding this comment

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

Suggested change
/// its callbacks even though it looses in the arena.
/// its callbacks even though it loses in the arena.

packages/flutter/lib/src/widgets/gesture_detector.dart Outdated
Comment on lines 190 to 191
Copy link
Contributor

@Piinks Piinks May 19, 2022

Choose a reason for hiding this comment

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

It looks like there is a typo here somewhere, or copy-pasta. Does this sound right?

Suggested change
/// wins because it was the first detector to enter the arena and it got first
/// because it is in front.
/// wins because it was the first detector to enter the arena.

@bleroux bleroux force-pushed the nested_gesture_detectors branch from 5d8cbf4 to fc3f36e Compare May 20, 2022 08:46
@bleroux bleroux force-pushed the nested_gesture_detectors branch from fc3f36e to d65e017 Compare May 20, 2022 08:58
@bleroux bleroux requested a review from Piinks May 20, 2022 14:02
Piinks
Piinks approved these changes May 24, 2022
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Flutter_LGTM

Thanks!

@fluttergithubbot fluttergithubbot merged commit 1e0a1a2 into flutter:master May 24, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 24, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 24, 2022
@engine-flutter-autoroll
1e0a1a2 Add an example and update GestureDetector documentation (fl…
4054c57 
…utter/flutter#102360)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 25, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request May 25, 2022
@engine-flutter-autoroll
1e0a1a2 Add an example and update GestureDetector documentation (fl…
1a06420 
…utter/flutter#102360)
@bleroux bleroux deleted the nested_gesture_detectors branch May 25, 2022 07:29
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
@bleroux @camsim99
Add an example and update GestureDetector documentation (flutter#10…
db9adc1 
…2360)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022
@engine-flutter-autoroll
1e0a1a2 Add an example and update GestureDetector documentation (fl…
f046a68 
…utter/flutter#102360)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022
@engine-flutter-autoroll
1e0a1a2 Add an example and update GestureDetector documentation (fl…
6fc82a8 
…utter/flutter#102360)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Hixie Hixie Hixie left review comments

@Piinks Piinks Piinks approved these changes

@goderbauer goderbauer Awaiting requested review from goderbauer

Assignees

No one assigned

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos 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.

GestureDetector.onTap not being hit when behind GestureDetector with HitTestBehavior.translucent

5 participants

@bleroux @goderbauer @Hixie @Piinks @fluttergithubbot