Skip to content

Support ignoring pointer events on tooltip overlay (#142465) #161363

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.

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

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

BenjiFarquhar
Copy link
Contributor

@BenjiFarquhar BenjiFarquhar commented Jan 9, 2025

As #142465 states, tooltips often interrupt widget interactivity by not allowing events to pass through to the Tooltip child, which is especially poor UX when hovering interact-able widgets on web when the mouse happens to land on the tooltip.

I've gone with defaulting ignorePointer to true when a simple message is supplied, since there won't ever be anything interact-able on the Tooltip, and defaulting to false when richMessage is supplied, so it doesn't break anyone's code that has interact-able widgets in the Tooltip.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jan 9, 2025
@BenjiFarquhar BenjiFarquhar changed the title Support ignoring pointer events on overlay (#142465) Support ignoring pointer events on tooltip overlay (#142465) Jan 9, 2025
@BenjiFarquhar BenjiFarquhar force-pushed the tooltip-ignore-pointer branch 3 times, most recently from ccee1e1 to 18fac21 Compare January 10, 2025 05:02
@BenjiFarquhar
Copy link
Contributor Author

The function 'ensureAndroidOrIosDevice' isn't defined

Anyone know what these build pipeline errors are? I think not related to my PR?

@Piinks Piinks requested a review from nate-thegrate January 15, 2025 19:23
@nate-thegrate
Copy link
Contributor

The function 'ensureAndroidOrIosDevice' isn't defined

Anyone know what these build pipeline errors are? I think not related to my PR?

I saw this same error in another recent PR, so I believe you're right that it's unrelated to this change. At this point we can probably fix it with a rebase.

Copy link
Contributor

@nate-thegrate nate-thegrate left a comment

Choose a reason for hiding this comment

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

As far as the implementation, this LGTM 👍
It's great to see #142465 being solved in a simple & effective way.

As far as backward-compatibility: let's get all the other checks passing and then see what Google Testing does!

@BenjiFarquhar BenjiFarquhar force-pushed the tooltip-ignore-pointer branch 7 times, most recently from c318d4b to ade4fa3 Compare January 21, 2025 05:21
@BenjiFarquhar
Copy link
Contributor Author

@nate-thegrate Thanks for the review! Looks like everything has passed 🙂

Copy link
Contributor

@nate-thegrate nate-thegrate left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

This approval should kick off Google Testing; I guess we'll see how it goes 😃

@BenjiFarquhar BenjiFarquhar force-pushed the tooltip-ignore-pointer branch 2 times, most recently from b73e45d to 2a0b150 Compare January 22, 2025 05:54
@BenjiFarquhar
Copy link
Contributor Author

@nate-thegrate Cheers! Looks like it's ready to merge

@dkwingsmt dkwingsmt self-requested a review January 22, 2025 19:34
@BenjiFarquhar
Copy link
Contributor Author

@dkwingsmt Hi. Looking like you will get this review done this week?

Copy link
Contributor

@dkwingsmt dkwingsmt 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 change! I'm ok with this default behavior change.

(And sorry for the delayed review!)

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 with a nit 👍

@BenjiFarquhar BenjiFarquhar force-pushed the tooltip-ignore-pointer branch 2 times, most recently from 5eedec1 to b791d47 Compare February 4, 2025 01:21
@BenjiFarquhar BenjiFarquhar force-pushed the tooltip-ignore-pointer branch from b791d47 to 53e3c29 Compare February 4, 2025 01:21
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 8, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 8, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 9, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants