Skip to content

Introduce exitDuration to Tooltip for mouse pointer devices - #138321

Merged
auto-submit[bot] merged 12 commits into
flutter:masterfrom
JaspervanRiet:tooltip-show-duration-adjusts-mouse
Dec 7, 2023
Merged

Introduce exitDuration to Tooltip for mouse pointer devices#138321
auto-submit[bot] merged 12 commits into
flutter:masterfrom
JaspervanRiet:tooltip-show-duration-adjusts-mouse

Conversation

@JaspervanRiet

@JaspervanRiet JaspervanRiet commented Nov 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR introduces a new property exitDuration to Tooltip, the counterpart to waitDuration. The need for this is shown by #136586. This changes the behaviour of showDuration on mouse pointer devices. This is because the use cases for the current behaviour on touch screen devices vs mouse pointer devices is fundamentally different.

Demo: tooltip with showDuration set

Tooltip disappears after 100 ms when moving away the mouse. Tooltip will not disappear when hovered.

chrome_J5Wz6XoF3s.mp4

Currently, when showDuration is set, this adjusts the time it takes for the tooltip to hide after a mouse pointer has left the tooltip. This is not the same use case as its effect on touch screen devices, where it dictates how long the tooltip stays on screen after a long press. That is needed because the tooltip takes up screen space and there is not an intuitive way to hide it, whereas when using a mouse users expect to simply have to hover somewhere else. Having the tooltip stay around will look broken.

Thus, this PR splits the two use cases. showDuration no longer affects mouse pointer devices at all*. There is a precedent for such mouse pointer-only behaviour in waitDuration. Instead, I have split up the two use cases and created the new property exitDuration, which will still allow for tweaking the time it takes for the tooltip to hide after the user has moved their mouse pointer somewhere else.

*Note: Should showDuration affect this line?

Fixes #136586.

Note: I noticed that when I made the change, no tests were broken. Hopefully, the tests added here help that in the future. I also noticed that in the existing tests, the waitDuration tests contain assertions that implicate that it is the role of waitDuration to change this behaviour, but that's not currently (nor in the new behaviour) true, so I have fixed those tests.

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.

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. p: material_ui material_ui package in flutter/packages labels Nov 12, 2023
gspencergoog
gspencergoog approved these changes Nov 22, 2023
View reviewed changes

@gspencergoog gspencergoog 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.

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

Thanks for the PR (and fixing the tests!)

Comment thread packages/flutter/lib/src/material/tooltip.dart Resolved
Comment thread packages/flutter/lib/src/material/tooltip.dart Resolved
@gspencergoog

gspencergoog commented Nov 22, 2023

Copy link
Copy Markdown
Contributor

Looks like examples/api/test/material/tooltip/tooltip.1_test.dart is failing.

cbracken
cbracken approved these changes Nov 22, 2023
View reviewed changes

@cbracken cbracken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm with one small nit.

Comment thread packages/flutter/test/material/tooltip_test.dart Resolved
@LongCatIsLooong

LongCatIsLooong commented Nov 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

*Note: Should showDuration affect this line?

I think showDuration makes more sense there since that line only schedules a dismiss timer when there is no hovering pointer keeping the tooltip visible.

@github-actions github-actions Bot added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Nov 22, 2023
@JaspervanRiet

JaspervanRiet commented Nov 22, 2023

Copy link
Copy Markdown
Contributor Author

Looks like examples/api/test/material/tooltip/tooltip.1_test.dart is failing.

@gspencergoog I have updated the test. It was coupled to the old behaviour.

@gspencergoog gspencergoog added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 22, 2023
@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Dec 1, 2023
@auto-submit

auto-submit Bot commented Dec 1, 2023

Copy link
Copy Markdown
Contributor

auto label is removed for flutter/flutter/138321, due to - The status or check suite Mac customer_testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@JaspervanRiet

JaspervanRiet commented Dec 4, 2023

Copy link
Copy Markdown
Contributor Author

@gspencergoog Looks like it's failing checks now. Do I need to do anything?

@gspencergoog

gspencergoog commented Dec 4, 2023

Copy link
Copy Markdown
Contributor

I don't think it has anything to do with your change. I've merged changes from the base branch, but the tree is broken at the moment anyhow, so we'll wait until it is green and try it again.

@LongCatIsLooong LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 7, 2023
@auto-submit
auto-submit Bot merged commit 3c422dd into flutter:master Dec 7, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 8, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023
@engine-flutter-autoroll
3c422dd Introduce exitDuration to Tooltip for mouse pointer devices (…
df2a659 
…flutter/flutter#138321)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 8, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023
@engine-flutter-autoroll
3c422dd Introduce exitDuration to Tooltip for mouse pointer devices (…
a5b5f78 
…flutter/flutter#138321)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 8, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023
@engine-flutter-autoroll
3c422dd Introduce exitDuration to Tooltip for mouse pointer devices (…
125026f 
…flutter/flutter#138321)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 8, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023
@engine-flutter-autoroll
3c422dd Introduce exitDuration to Tooltip for mouse pointer devices (…
347ddf5 
…flutter/flutter#138321)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 8, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023
@engine-flutter-autoroll
3c422dd Introduce exitDuration to Tooltip for mouse pointer devices (…
60185be 
…flutter/flutter#138321)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 8, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023
@engine-flutter-autoroll
3c422dd Introduce exitDuration to Tooltip for mouse pointer devices (…
3248dbc 
…flutter/flutter#138321)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 8, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023
@engine-flutter-autoroll
3c422dd Introduce exitDuration to Tooltip for mouse pointer devices (…
dbd23a1 
…flutter/flutter#138321)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 8, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023
@engine-flutter-autoroll
3c422dd Introduce exitDuration to Tooltip for mouse pointer devices (…
525ded6 
…flutter/flutter#138321)
@JaspervanRiet
JaspervanRiet deleted the tooltip-show-duration-adjusts-mouse branch December 9, 2023 13:35
@HansMuller HansMuller mentioned this pull request Dec 15, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024
@engine-flutter-autoroll
3c422dd Introduce exitDuration to Tooltip for mouse pointer devices (…
2a004e2 
…flutter/flutter#138321)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cbracken cbracken cbracken approved these changes
@gspencergoog gspencergoog Awaiting requested review from gspencergoog

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Assigning showDuration in tooltip applies same duration to both long press and tap released and mouse pointer exits

4 participants

@JaspervanRiet @gspencergoog @LongCatIsLooong @cbracken