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 flickers when hovering SVG #1067

Closed
dancormier opened this issue Aug 19, 2022 · 4 comments
Closed

Tooltip flickers when hovering SVG #1067

dancormier opened this issue Aug 19, 2022 · 4 comments
Labels
bug A reproducible problem with the Stacks code

Comments

@dancormier
Copy link
Contributor

dancormier commented Aug 19, 2022

When including an SVG within an element that renders a tooltip, the tooltip will rerender when the user hovers over the SVG. My hunch is that it has to do with popper.js but I haven't verified this. Issue demonstrated here: https://codepen.io/dc-so/pen/JjLxJxd

hover

To Reproduce
Steps to reproduce the behavior:

  1. Go to https://codepen.io/dc-so/pen/JjLxJxd
  2. Hover over the text "Shouldn't flicker" in the preview. Notice the tooltip.
  3. Now, hover the red square (which is an SVG). Notice the tooltip flash out of existence for a moment.

Expected behavior
The tooltip shouldn't flicker.

cc @KyleMit @tmcentee

@dancormier dancormier added the bug A reproducible problem with the Stacks code label Aug 19, 2022
@KyleMit
Copy link
Collaborator

KyleMit commented Aug 22, 2022

Example of repro - when coming off/on the red svg:

flicker-svg

@giamir
Copy link
Contributor

giamir commented Oct 5, 2022

This issue is not reproducible anymore from v.1.3.1 onwards.
The bug was most likely fixed by the introduction of a delay on tooltip hide:
844d5a3#diff-6dd14c2015a0c674232011f67fec0c6c43bb03c3065bbcced039136814895174R73
I don't have much context about why the delay of 100ms was introduced in the first place.
@dancormier perhaps you could double check from your end and decide if we can mark this issue as resolved or take the time to investigate further.

Additional Thoughts (beyond the scope of this issue):
One aspect that worries me a bit is that without a test suite these bugs are inevitably destined to represent themselves as the codebase evolves (e.g. tomorrow we decide to remove the 100ms delay). In an ideal world we could have added a test case that protect us exactly from such a regression before closing the issue. I think we already have a decent testing setup in the Stacks-Editor, perhaps we could take some inspiration from there. 😊

@dancormier
Copy link
Contributor Author

dancormier commented Oct 5, 2022

This issue is not reproducible anymore from v.1.3.1 onwards. The bug was most likely fixed by the introduction of a delay on tooltip hide: 844d5a3#diff-6dd14c2015a0c674232011f67fec0c6c43bb03c3065bbcced039136814895174R73 I don't have much context about why the delay of 100ms was introduced in the first place. @dancormier perhaps you could double check from your end and decide if we can mark this issue as resolved or take the time to investigate further.

Looks like you're correct! It seems this issue has been resolved since it's initial report and you're likely correct about it being remedied with the 100ms delay. I can't recall why there's two separate commits introducing the delay, but some more context could be found in this PR.

tl;dr: Tooltips need to persist when hovered and adding a small delay to the hide function will prevent it from hiding when the user moves their mouse from the triggering element to the tooltip itself. See also: "W3C: Understanding Success Criterion 1.4.13: Content on Hover or Focus" and this PR comment.

Additional Thoughts (beyond the scope of this issue): One aspect that worries me a bit is that without a test suite these bugs are inevitably destined to represent themselves as the codebase evolves (e.g. tomorrow we decide to remove the 100ms delay). In an ideal world we could have added a test case that protect us exactly from such a regression before closing the issue. I think we already have a decent testing setup in the Stacks-Editor, perhaps we could take some inspiration from there. 😊

You're right, we really should have more robust testing in Stacks. Soon we could decide to codify an approach to testing in Stacks when we have some bandwidth. Testing functionality should be relatively straightforward and I'd also like to update our approach to visual regression testing. It's something I'd be eager to explore with you!

@giamir
Copy link
Contributor

giamir commented Oct 5, 2022

Thanks @dancormier for the additional context. 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A reproducible problem with the Stacks code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants