Skip to content
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

[Tooltip] Apply enterDelay on Component base #19765

Closed
2 tasks done
Ritorna opened this issue Feb 18, 2020 · 6 comments · Fixed by #19766
Closed
2 tasks done

[Tooltip] Apply enterDelay on Component base #19765

Ritorna opened this issue Feb 18, 2020 · 6 comments · Fixed by #19766
Labels
component: tooltip This is the name of the generic UI component, not the React module! new feature New feature or request ready to take Help wanted. Guidance available. There is a high chance the change will be accepted

Comments

@Ritorna
Copy link
Contributor

Ritorna commented Feb 18, 2020

Tooltips shouldn't open immediately when another tooltip is already opened. Each component instance should respect its own delays.

Though there is no official description about the behavior, material uses the proposed behavior for its tooltips: see
https://material.io/components/tooltips/#specs

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Currently when one Tooltip is already opened and we hover above another one, the tooltip opens directly without respecting the defined enterDelay.

Expected Behavior 🤔

Tooltips should respect their own enter delays.

Steps to Reproduce 🕹

https://codesandbox.io/s/material-demo-3fxbm?fontsize=14&hidenavigation=1&theme=dark

Steps:

  1. Hover over one item and wait till the Tooltip opens
  2. Hover over the other items

Context 🔦

I'm trying to render a DataGrid with a sort of Link Column, which should render a tooltip for each cell displaying the actual href.

Your Environment 🌎

Tech Version
Material-UI v4.9.3
React 16.12.0
Browser chrome 80
TypeScript 3.7.5
@eps1lon
Copy link
Member

eps1lon commented Feb 18, 2020

Tooltips shouldn't open immediately when another tooltip is already opened. Each component instance should respect its own delays.

I disagree with this. Imagine a toolbar where you can hover over the icons. You wouldn't want to wait on each button but rather quickly scan over them.

I do agree that we should document this behavior though.

Do you have a concrete example where individual tooltips should wait even if one is already open? This would help us determine if we want to support this use case.

@eps1lon eps1lon added component: tooltip This is the name of the generic UI component, not the React module! discussion labels Feb 18, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 18, 2020

I'm 💯 with @eps1lon. For context, the behavior was introduced in #18458.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 19, 2020

Actually, looking closer at how a native title attribute prop behaves, I think that we could improve the current implementation with a new delay. on Chrome macOS, I could measure the following:

  • delay of the "first enter" (A): 1.7s (native) vs 0ms (MUI)
  • delay of the "consecutive enter" (B): 400ms (native) vs 0ms (MUI)
  • delay of the state transition from B to A: 800ms (native) vs 500ms (MUI)

(test case, 2 title attributes https://www.w3schools.com/code/tryit.asp?filename=GC32FEG17F3K)

So, in this context, I would propose the following diff:

diff --git a/packages/material-ui/src/Tooltip/Tooltip.js b/packages/material-ui/src/Tooltip/Tooltip.js
index ff299f448..00bbf14dd 100644
--- a/packages/material-ui/src/Tooltip/Tooltip.js
+++ b/packages/material-ui/src/Tooltip/Tooltip.js
@@ -185,6 +185,7 @@ const Tooltip = React.forwardRef(function Tooltip(props, ref) {
     disableTouchListener = false,
     enterDelay = 0,
     enterTouchDelay = 700,
+    consecutiveEnterDelay = enterDelay / 3,
     id: idProp,
     interactive = false,
     leaveDelay = 0,
@@ -303,11 +304,11 @@ const Tooltip = React.forwardRef(function Tooltip(props, ref) {

     clearTimeout(enterTimer.current);
     clearTimeout(leaveTimer.current);
-    if (enterDelay && !hystersisOpen) {
+    if (enterDelay || (hystersisOpen && consecutiveEnterDelay)) {
       event.persist();
       enterTimer.current = setTimeout(() => {
         handleOpen(event);
-      }, enterDelay);
+      }, hystersisOpen ? consecutiveEnterDelay : enterDelay);
     } else {
       handleOpen(event);
     }
@@ -345,8 +346,8 @@ const Tooltip = React.forwardRef(function Tooltip(props, ref) {
     clearTimeout(hystersisTimer);
     hystersisTimer = setTimeout(() => {
       hystersisOpen = false;
-    }, 500);
-    // Use 500 ms per https://github.com/reach/reach-ui/blob/3b5319027d763a3082880be887d7a29aee7d3afc/packages/tooltip/src/index.js#L214
+    }, 800 + leaveDelay);
+    // Wait x ms, about the same amount of time the Web platform do.

     setOpenState(false);

The pros of the above approach:

  • We keep the same default.
  • @Ritorna could force consecutiveEnterDelay === enterDelay
  • It would make the feature better documented
  • It would be closer to the native title attribute behavior

@oliviertassinari oliviertassinari added ready to take Help wanted. Guidance available. There is a high chance the change will be accepted new feature New feature or request and removed discussion labels Feb 19, 2020
@Ritorna
Copy link
Contributor Author

Ritorna commented Feb 19, 2020

Sorry for the late response... Thanks in advance for taking a closer look.
I'd be glad to see this as an additonal feature, as you proposed 👍

My special use case is rendering link cells in a data grid, which should provide a tooltip with the actual href when hovering:
image

@eps1lon
Copy link
Member

eps1lon commented Feb 20, 2020

Actually, looking closer at how a native title attribute prop behaves

I would not use this as the source of truth for the timings. Why do you think the native title attribute is a good candidate for tooltip timings?

@oliviertassinari
Copy link
Member

It's the worse best benchmark source I could find. Happy to consider a different one. In any case, Material Design has different timing values shouldn't we give it more priority?

I was more interested in the structural values of the title attribute implementation than in the absolute values. I mean the A/B ratio of ~3 and to wait for the tooltip to be invisible to reset the FSM state from B to A.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tooltip This is the name of the generic UI component, not the React module! new feature New feature or request ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants