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 with hover trigger not working on page with scrollbar #118

Closed
b0hdy opened this issue Feb 18, 2021 · 14 comments · Fixed by #120
Closed

Tooltip with hover trigger not working on page with scrollbar #118

b0hdy opened this issue Feb 18, 2021 · 14 comments · Fixed by #120

Comments

@b0hdy
Copy link

b0hdy commented Feb 18, 2021

Describe the bug
When mouse enters trigger element and page has vertical scrollbar and his position is not at top of page then tooltip is flickering and does not remain visible. Problem is probably with function mouseOutsideRect and comparing mouse and trigger position: (pageY || clientY) < Math.floor(top) || (pageY || clientY) > Math.ceil(bottom).

To Reproduce
Steps to reproduce the behavior:
https://codesandbox.io/s/white-leaf-094it?file=/src/index.js
https://user-images.githubusercontent.com/18634735/108426708-613e2900-723c-11eb-924b-b7b9096d97dc.mov

Expected behavior
Tooltip should remain visible after hover button.

Desktop (please complete the following information):

  • OS: macOS
  • Browser chrome
  • Version 88
@mohsinulhaq
Copy link
Owner

@czabaj I think using pageX instead of clientX might solve this? Not sure, can you check?

@b0hdy
Copy link
Author

b0hdy commented Feb 20, 2021

I found another bug with hover on trigger element, the problem is only in Chrome (FF and Safari works properly). When mouse enters trigger element from the top of the element then tooltip is flickering again. Property mouseEvent.clientY in mouseOutsideRect strangely returns mouse position about 1px less than a tooltipRect.top.

Code: https://codesandbox.io/s/objective-volhard-7hktp?file=/src/index.js
Demo: https://user-images.githubusercontent.com/18634735/108590190-73b28280-7362-11eb-8fa1-7ba8de80392c.mov

@mohsinulhaq
Copy link
Owner

@czabaj can you check this case with your fix?

@czabaj
Copy link
Contributor

czabaj commented Feb 21, 2021

@mohsinulhaq just checked it and this problem appears with top and left properties, decreasing both by 1 pixel during comparison is a quick fix. But I don't know why this happens 😕

@mohsinulhaq
Copy link
Owner

wow, pretty weird, let me check it out as well

@mohsinulhaq
Copy link
Owner

https://codesandbox.io/s/serene-voice-7cbco?file=/src/index.js
Chrome: 7px and 8px
Firefox: 9.6px and 10px
Safari: 10px and 10px
🤷‍♂️

@czabaj
Copy link
Contributor

czabaj commented Feb 22, 2021

@mohsinulhaq forked your test and re-tested https://codesandbox.io/s/sharp-zhukovsky-g65qm?file=/src/index.js

Both Firefox and Safari suffer from the rounding error originating from the fact, that DOMRect supports fractional pixels but MouseEvent not (although it seems that it shall support fractional pixels too), but only Chrome reports whole integer difference.

Does that sound like a bug in chromium? Meantime, we could exceed the DOMRect by 1 pixel to fix this issue, OK?

@czabaj
Copy link
Contributor

czabaj commented Feb 22, 2021

I created an issue in the chromium issue tracker https://bugs.chromium.org/p/chromium/issues/detail?id=1180746

@mohsinulhaq
Copy link
Owner

mohsinulhaq commented Feb 22, 2021

@czabaj thanks for filing it, I was thinking the same, but I thought first I would consult StackOverflow.
But there was no reply, so I guess, no problem.

Btw, shouldn't we add/subtract 1 only for Chrome?

@czabaj
Copy link
Contributor

czabaj commented Feb 22, 2021

@mohsinulhaq I would rather not exceed the DOMRect conditionally, it will increase the complexity, I'm not aware of any way how to test the browser vendor reliably and easily, and increasing the DOMRect by 1 pixel will only cause the Tooltip to hide after the mouse leaves "the Trigger and 1 pixel" which is negligible IMHO.

@mohsinulhaq
Copy link
Owner

hmmm makes sense 👍

@mohsinulhaq
Copy link
Owner

somehow, merging the PR auto-closed this issue

@mohsinulhaq mohsinulhaq reopened this Feb 23, 2021
@mohsinulhaq
Copy link
Owner

@b0hdy please try out v4.1.1 and let us know if it fixed your issue.

@b0hdy
Copy link
Author

b0hdy commented Feb 23, 2021

@mohsinulhaq works well, thank you.

@b0hdy b0hdy closed this as completed Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants