Skip to content

Add callbacks for detecting tap up events to TapRegion #156110

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
auto-submit merged 6 commits into from
Oct 21, 2024
Merged

Add callbacks for detecting tap up events to TapRegion #156110

auto-submit merged 6 commits into from
Oct 21, 2024

Conversation

@Hannnes1
Copy link
Contributor

@Hannnes1 Hannnes1 commented Oct 2, 2024 •
edited
Loading

Adds onTapUpOutside and onTapUpInside callbacks to TapRegion, and onTapUpOutside to EditableText text fields. This allows users to detect PointerUpEvents, which in turn can be used to differentiate between a tap and pan / scroll gesture (for example by comparing the distance between the down and up events).

Fixes #140271 and potentially #138190

Pre-launch Checklist

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. f: cupertino flutter/packages/flutter/cupertino repository labels Oct 2, 2024
@Hannnes1
Copy link
Contributor Author

Hannnes1 commented Oct 2, 2024

I did consider renaming onTapOutside and onTapInside to onTapDownOutside and onTapDownInside (deprecating the old names), but that would probably be a breaking change (it doesn't break flutter/tests because that doesn't use any of those callbacks).
A renaming would improve the API in my opinion, but is it worth the breaking change?

@Hannnes1 Hannnes1 changed the title Tap region up Add callbacks for detecting tap up events to TapRegion Oct 2, 2024
@justinmc justinmc requested a review from gspencergoog October 3, 2024 21:47
@justinmc
Copy link
Contributor

justinmc commented Oct 3, 2024

@gspencergoog Do you have any thoughts on this since you wrote TapRegion? I think it seems reasonable but I wonder if this goes with what you were imagining when you wrote it.

@gspencergoog
Copy link
Contributor

gspencergoog commented Oct 3, 2024

This seems fine, and is along the lines of other events we have. It does make me wish I'd named it onTapDownOutside, though, for symmetry. Too late to change that now, though.

@Hannnes1
Copy link
Contributor Author

Hannnes1 commented Oct 9, 2024

I'll leave the names as they are then.

I just noticed that #155297 was merged recently. I should probably add the same isCurrent check to onTapUpOutside as well, right?

@Piinks Piinks requested a review from justinmc October 16, 2024 18:13
justinmc
justinmc reviewed Oct 16, 2024
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

Yes please add the check for isCurrent. Good catch!

Just a few nits and questions but I think this PR looks good overall. Thanks for your work on it!

justinmc
justinmc approved these changes Oct 17, 2024
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍 . Thanks for the quick changes.

justinmc
justinmc reviewed Oct 17, 2024
View reviewed changes
packages/flutter/lib/src/widgets/tap_region.dart Outdated
///
/// The event is the pointer event that caused the callback to be called.
/// Signature for a callback called for a [PointerDownEvent] relative to a [TapRegion].
///
Copy link
Contributor

@justinmc justinmc Oct 17, 2024

Choose a reason for hiding this comment

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

Oh I think the analyzer check is failing because here and below there is a trailing space at the end of the line.

Copy link
Contributor Author

@Hannnes1 Hannnes1 Oct 18, 2024

Choose a reason for hiding this comment

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

Thanks! Fixed that now

@justinmc
Copy link
Contributor

justinmc commented Oct 17, 2024

@gspencergoog Would you mind being the secondary reviewer here since you already took a brief look before?

gspencergoog
gspencergoog approved these changes Oct 21, 2024
View reviewed changes
Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

@gspencergoog gspencergoog added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 21, 2024
@auto-submit auto-submit bot merged commit 8621694 into flutter:master Oct 21, 2024
@justinmc
Copy link
Contributor

justinmc commented Oct 21, 2024

Congrats on the merge @Hannnes1, thank you for contributing!

@Hannnes1 Hannnes1 deleted the tap_region_up branch October 21, 2024 17:21
@Hannnes1
Copy link
Contributor Author

Hannnes1 commented Oct 21, 2024

Thank you! And thanks for reviewing.

@Hannnes1 Hannnes1 mentioned this pull request Oct 21, 2024
Open
2 tasks
@Piinks Piinks mentioned this pull request Oct 25, 2024
Closed
@Hannnes1 Hannnes1 mentioned this pull request Jan 25, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 12, 2025
@engine-flutter-autoroll
8621694 Add callbacks for detecting tap up events to TapRegion (flutt…
83347b6 
…er/flutter#156110)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
@engine-flutter-autoroll
8621694 Add callbacks for detecting tap up events to TapRegion (flutt…
4739ec7 
…er/flutter#156110)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
@engine-flutter-autoroll
8621694 Add callbacks for detecting tap up events to TapRegion (flutt…
5fe340e 
…er/flutter#156110)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025
@engine-flutter-autoroll
8621694 Add callbacks for detecting tap up events to TapRegion (flutt…
5792626 
…er/flutter#156110)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025
@engine-flutter-autoroll
8621694 Add callbacks for detecting tap up events to TapRegion (flutt…
d3b8a6d 
…er/flutter#156110)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@justinmc justinmc justinmc approved these changes

@gspencergoog gspencergoog gspencergoog approved these changes

Assignees

No one assigned

Labels

a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. 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.

TapRegion has no clear way to differentiate between different gestures

3 participants

@Hannnes1 @justinmc @gspencergoog