Tooltip bonanza bug

Question

Tooltip bonanza bug

Closed this issue 5 years ago · 18 comments

bvaughn commented 5 years ago

Noticed this issue testing the React profiler:

Can be reproduced by going here and importing this profiling.json export.

I am working on a smaller repro but haven't yet figured it out. 😅

Note that this bug occurs in tooltip versions 0.1 and 0.2.

Answer 1 · 2019-05-07T22:13:59.000Z

Here's a smaller repro:
https://qkyvl35j09.codesandbox.io/

Answer 2 · 2019-05-21T18:52:54.000Z

I've seen this issue a few times in my project as well, though I can't figure out the steps to reproduce it as consistently as your example ^

Answer 3 · 2019-05-22T14:30:26.000Z

@bvaughn I noticed that the tooltip bug doesn't happen if you remove the Tooltip from the initial repro button. Possibly a timing/subscriptions issue?

https://codesandbox.io/s/reachtooltip-bonanza-bug-repro-k8fwg

Answer 4 · 2019-05-22T15:24:38.000Z

Fair enough, although that's not really an option in my project (DevTools) since that button needs a tooltip as well :)

Answer 5 · 2019-05-22T15:38:14.000Z

Totally understand. I was just hinting towards a possible cause for the issue, our team is in the same boat!

Answer 6 · 2019-06-11T22:23:38.000Z

I found a possible solution. Though, I want to make sure that it does not have an unintended side effects before I post a PR.

Answer 7 · 2019-06-11T23:14:08.000Z

@bvaughn, I was able to correct all the tooltips showing upon transition with my WIP solution. However, your demo does shift focus away from and back to the button that was originally clicked (default browser behavior, AFAIK). I added a red outline to the focused element in the GIF below demoing my solution. Is this your desired behavior?

Answer 8 · 2019-06-12T14:29:13.000Z

I'm not really sure what you're asking. I don't explicitly set focus, so... it seems like if the browser's default behavior causes tooltips to break, that's not good. (There are a few cases where this happens in DevTools. This was just one repro I was able to isolate.)

Answer 9 · 2019-06-12T15:06:51.000Z

I'm asking what you're expected behavior is. I can see that there is something off, but I'm not sure what you're expected behavior is. Given the patch I've written I would say that it is almost there. As only the focused element is given a tooltip. However, I do think that the element ID assignment could be better as previously used IDs are automatically used again upon the removal of a node with a reserved ID.

So, as it stands right now. I'm not sure what should be done, @ryanflorence.

Answer 10 · 2019-06-12T15:11:12.000Z

Gotcha.

I'm asking what you're expected behavior is.

My expectation is just that every tooltip won't show at once. I don't actually have much of an expectation about whether the single tooltip would show (or not) on return focus. Slight preference for "not" but I can understand if this would be difficult to achieve.

Answer 11 · 2019-06-12T15:33:41.000Z

My expectation is just that every tooltip won't show at once.

If that is the case, then I believe my patch will address that. I suppose I'll just post a PR and we can continue from there.

Answer 12 · 2019-06-13T15:24:45.000Z

Thanks for digging in @SavePointSam! This is a bug that would prevent us from using it in production, but we love it otherwise. Hopefully we can hear back soon from Ryan if he's ok with your solution.

Answer 13 · 2019-06-14T15:24:37.000Z

I'll take a look at this today, thanks everybody :)

Answer 14 · 2019-06-14T15:26:48.000Z

Also, just noticed the name of this issue, @bvaughn you crack me up 😂

Answer 15 · 2019-06-18T20:05:31.000Z

@ryanflorence any plans to merge my fix soon?

Answer 16 · 2019-06-19T16:04:44.000Z

Looks like the fix is in 0.2.2! Thanks so much @ryanflorence and @SavePointSam , this will be going out to some very happy users (and devs) in the next month. Our previous tooltip implementation was a mess.

Answer 17 · 2019-06-19T16:05:14.000Z

Thanks Ryan and Sam!

Answer 18 · 2019-06-19T22:03:36.000Z

I believe this can be closed now. 😄🚀