Fix tooltips don't dismiss when using TooltipTriggerMode.tap #103960

bleroux · 2022-05-17T08:01:33Z

Description

Tooltips lifecycle is managed using three sources of pointer events :

a MouseRegion to manage hover events.
a listener registered to GestureBinding.instance.pointerRouter.
a GestureDetector to react to long press or tap gestures (depending on the tooltip trigger mode).

The GestureDetector handlers (onLongPress and onTap) call _handlePress method to display the Tooltip.
The tooltip is dismissed either by :

the MouseRegion onHoverExit handler when the mouse is moved outside the targeted widget.
the listener to PointerRouter after a PointerUpEvent or a pointerCancelEvent.

For TooltipTriggerMode.onTap, the tooltip might not be disposed (see #98854). It happens when there is one or more competing GestureDetectors. In this case (see #98854), the GestureArena disambiguation process takes more time and the PointerRouter events are received before the tooltip is shown.

This PR updates the Tooltip GestureDetector onTap callback to show the Tooltip and dismisses it after showDuration expired.

Related Issue

Fixes #98854

Tests

Add 3 test in material/tooltip_test.dart

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.

chunhtai

Based on the pr description, the issue is that the global pointer arrive faster than the ontap call back which makes the _handleMouseExit called before the _handleTap is called?

If so it seems like we should not use the globalpointer route in the first place, why don't we just use the onLongPressUp/onLongPressCancel and onTapUp/onTapCancel to call the _handleMouseExit?

bleroux · 2022-05-17T18:57:19Z

@chunhtai Thanks for the review.

Based on the pr description, the issue is that the global pointer arrive faster than the ontap call back which makes the _handleMouseExit called before the _handleTap is called?

Yes, that's the issue.

If so it seems like we should not use the globalpointer route in the first place, why don't we just use the onLongPressUp/onLongPressCancel and onTapUp/onTapCancel to call the _handleMouseExit?

I think the global pointer route was used for the main following reason : from the material spec, "If the user takes another action before that time ends, the tooltip will disappear.". For instance, if the showDuration is set to 3 seconds, the user can dismiss the tooltip before this delay with a tap anywhere, this 'action' will be catch by the PointerRouter.
It's also true for Tooltips shown after onMouseHover, a tap (or a click) anywhere will hide them even if it is in the widget bounds (it's a way to dismiss the tooltip without requiring onMouseExit).
It was introduced a long time ago by this PR #4284 :-) and the expected behavior is well described in #4182.

chunhtai · 2022-05-17T20:35:09Z

Thanks for clarifying, based on the original issue it seems like only this is required

flutter/packages/flutter/lib/src/material/tooltip.dart

Line 631 in 10087a3

} else if (event is PointerDownEvent) {

the PointerUpEvent, PointerCancelEvent should be handled by the gesture detector instead?

chunhtai · 2022-05-17T20:37:18Z

The reason I brought this up is this pr create two different ways to dismiss the tooltip for longpress and tap, one from the global route for long press and one from the ontap handler for single tap, It will be easier to maintain if they are more unified.

bleroux · 2022-05-18T05:55:32Z

Unification would be great. Unfortunately, I don’t think we can get rid of PointerRouter reacting to PointerUpEvent/PointerCancelEvent.

I tried and quickly found one issue on Desktop : hold down the mouse button outside the widget, move above the widget, the tooltip is shown (onMouseOver), release the button the tooltip should be dismissed, it is not when removing this line :

flutter/packages/flutter/lib/src/material/tooltip.dart

Line 629 in 10087a3

if (event is PointerUpEvent || event is PointerCancelEvent) {

and we can't rely on GestureDetector for this scenario because the gesture started elsewhere.

As TooltipTriggerMode.tap was introduced recently (in 2021) and is not covered by Material spec, I tried to find a fix that doesn’t interfere with the main use cases (longPress and Hover) to avoid subtle breaking changes.

chunhtai · 2022-05-18T21:43:36Z

I tried and quickly found one issue on Desktop : hold down the mouse button outside the widget, move above the widget, the tooltip is shown (onMouseOver), release the button the tooltip should be dismissed.

Is this a expected behavior? I tried on web https://www.w3schools.com/css/tryit.asp?filename=trycss_tooltip
and the tooltip is not dismissed when lifted the mouse click even if the tooltip started outside. I thought the tooltip only relies on the mouse hover when mouse is connected.

bleroux · 2022-05-19T08:03:28Z

Is this a expected behavior?

As per material spec, I would say yes
"Display the tooltip for 1.5 seconds. If the user takes another action before that time ends, the tooltip will disappear.".
But it depends on how 'action' is interpreted 😄 and one can argue this is not user-friendly (see this comment).

I thought the tooltip only relies on the mouse hover when mouse is connected.

By default no. I too expected that it will, for instance on desktop. That's why I filed #103703.
I closed #103073, because I recently figured out that with TooltipTriggerMode.manual we can show Tooltip only when hovered.

What you suggest is smarter : using mouse hover when a mouse is connected and relying on gestures when there are no mouse. That seems very interesting for computer with a touch screen or tablets with a mouse. If it makes sense, filing a new issue will be more actionnable than updating this PR.

I would like to keep this PR simple, as TooltipTriggerMode.tap is a edge case and updating Tooltip lifecycle code might be risky. Once this one is merged, it would be interesting to consider creating two new issues :

one about the Material spec definition of 'action', directly related to your question 'is this an expected behavior?'
another about relying only on the mouse hover when mouse is connected.

chunhtai

LGTM, make sense. seem like the best thing to do at the moment

chunhtai · 2022-05-19T17:42:55Z

packages/flutter/lib/src/material/tooltip.dart

can you add some comments around globalpointer route may be called faster than the the gesture recognizer and why this is here.

fluttergithubbot · 2022-05-20T09:28:08Z

This pull request is not suitable for automatic merging in its current state.

This commit is not mergeable and has conflicts. Please rebase your PR and fix all the conflicts.

…lutter/flutter#103960)

…#103960)

…lutter/flutter#103960)

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels May 17, 2022

bleroux force-pushed the fix_tooltip_not_dismissed_on_tap branch from a0e7e6c to bfa3023 Compare May 17, 2022 08:48

bleroux requested a review from chunhtai May 17, 2022 09:23

chunhtai reviewed May 17, 2022

View reviewed changes

bleroux requested a review from chunhtai May 19, 2022 08:04

bleroux mentioned this pull request May 19, 2022

Wrapping GestureDetector in a Tooltip widget causes onTapCancel #84531

Closed

chunhtai approved these changes May 19, 2022

View reviewed changes

bleroux force-pushed the fix_tooltip_not_dismissed_on_tap branch 2 times, most recently from d01fcf4 to a4b7329 Compare May 20, 2022 08:20

bleroux added the waiting for tree to go green label May 20, 2022

fluttergithubbot removed the waiting for tree to go green label May 20, 2022

fix Tooltip doesn't dismiss when using TooltipTriggerMode.tap

89bada8

bleroux force-pushed the fix_tooltip_not_dismissed_on_tap branch from a4b7329 to 89bada8 Compare May 20, 2022 09:43

bleroux added the waiting for tree to go green label May 20, 2022

fluttergithubbot merged commit afdfc56 into flutter:master May 20, 2022

bleroux deleted the fix_tooltip_not_dismissed_on_tap branch May 20, 2022 11:59

engine-flutter-autoroll mentioned this pull request May 20, 2022

Roll Flutter from 0428f421312e to afdfc56bbb63 (1 revision) flutter/packages#2015

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2022

afdfc56 Fix tooltips don't dismiss when using TooltipTriggerMode.tap (f…

5561247

…lutter/flutter#103960)

engine-flutter-autoroll mentioned this pull request May 21, 2022

Roll Flutter from a4a8e73bce15 to 1e1f4bcfb561 (65 revisions) flutter/plugins#5795

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request May 21, 2022

afdfc56 Fix tooltips don't dismiss when using TooltipTriggerMode.tap (f…

1e00136

…lutter/flutter#103960)

srihariash999 mentioned this pull request Jun 16, 2022

Nested tooltip does not get dismissed #106090

Closed

camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022

Fix tooltips don't dismiss when using TooltipTriggerMode.tap (flutter…

9b1a791

…#103960)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022

afdfc56 Fix tooltips don't dismiss when using TooltipTriggerMode.tap (f…

7d2c939

…lutter/flutter#103960)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022

afdfc56 Fix tooltips don't dismiss when using TooltipTriggerMode.tap (f…

e275180

…lutter/flutter#103960)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix tooltips don't dismiss when using TooltipTriggerMode.tap #103960

Fix tooltips don't dismiss when using TooltipTriggerMode.tap #103960

Uh oh!

bleroux commented May 17, 2022 •

edited

Loading

Uh oh!

chunhtai left a comment

Uh oh!

bleroux commented May 17, 2022

Uh oh!

chunhtai commented May 17, 2022

Uh oh!

chunhtai commented May 17, 2022

Uh oh!

bleroux commented May 18, 2022

Uh oh!

chunhtai commented May 18, 2022

Uh oh!

bleroux commented May 19, 2022 •

edited

Loading

Uh oh!

chunhtai left a comment

Uh oh!

chunhtai May 19, 2022

Uh oh!

fluttergithubbot commented May 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fix tooltips don't dismiss when using TooltipTriggerMode.tap #103960

Fix tooltips don't dismiss when using TooltipTriggerMode.tap #103960

Uh oh!

Conversation

bleroux commented May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Tests

Pre-launch Checklist

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

bleroux commented May 17, 2022

Uh oh!

chunhtai commented May 17, 2022

Uh oh!

chunhtai commented May 17, 2022

Uh oh!

bleroux commented May 18, 2022

Uh oh!

chunhtai commented May 18, 2022

Uh oh!

bleroux commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai May 19, 2022

Choose a reason for hiding this comment

Uh oh!

fluttergithubbot commented May 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bleroux commented May 17, 2022 •

edited

Loading

bleroux commented May 19, 2022 •

edited

Loading