#1141 Tooltips often stay open when hovered fast

[alexasselin008]

Summary

Tooltips now open one at the time, never multiple at once. Also fixed an issue where mouseLeave event would not be detected anymore when hovering the overlay

What I did

The condition

if (!isTargetParent((event as FocusEvent).relatedTarget, overlayRef)) {

really was the issue. It was intended for blur events, but was preventing mouseLeave event from firing if the mouseleave was on the overlay.

added a check to make sure only blur event were taking into consideration for this logic

[netlify[bot]]

✅ Deploy Preview for sg-orbit ready!

Name	Link
🔨 Latest commit	719c57497c8cb63431be733e69a1d77429aae8a9
🔍 Latest deploy log	https://app.netlify.com/sites/sg-orbit/deploys/63e4076d05441a00081ec1da
😎 Deploy Preview	https://deploy-preview-1142--sg-orbit.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[netlify[bot]]

✅ Deploy Preview for sg-storybook ready!

Name	Link
🔨 Latest commit	719c57497c8cb63431be733e69a1d77429aae8a9
🔍 Latest deploy log	https://app.netlify.com/sites/sg-storybook/deploys/63e4076d47a3ab0008650a63
😎 Deploy Preview	https://deploy-preview-1142--sg-storybook.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[patricklafrance]

Sorry for commenting on a closed PR.

I am glad that you are working on ithis issue and seems to have found a fix.

However, I am not sure this is the right one.

For a tooltip, there's a use case where it could be opened by focusing a trigger element with a tab keypress. When it happens, since the tooltip is not activated with the mouse, the mouse can hover something else. If the mouse hover another tooltip trigger element, with this fix, it would actually closed the currently tab focused trigger element. I would personally consider this as a bug.

Here's an example:

While this use case is not frequent, I believe it highlights that it might not be the right fix for this issue.

For a design system component, I usually wouldn't recommend tracking other components state. I think components should only know about theirself or sometimes, when in a parent/child relationship, integrates with an opt-in context mecanism.

I was personally expecting the fix to rather involve reacting to specific DOM events. Doing it this way rather seems like the underlying cause of the issue is not well understood.

Anyway, I wouldn't do it this way but if you still want to, here's a few other things I noticed:

• You should use a WeakMap rather than an object literal to store the close functions. Kind of weird, but I think that with a WeakMap, the key could even be the setIsOpen instead of the id.

• A tooltip already have a unique id, you could potentially use this instead of a generating a new one. (const tooltipId = useId(tooltip.props.id, "o-ui-tooltip");

• closeOpenTooltips -> closeOpenedTooltips

• ensureTooltipEntry, the word "ensure", usually means to validate that something already exist or is in a specific state. In this case, you are adding a new entry to the tooltip bucket. I would rather go with something like add or set

• It might be something that you wish to remove from the DS, but at the moment those if conditions are usually using the isNil function.

if (!isNil(tooltip))

[alexasselin008]

Hi @patricklafrance ,

Thanks for your insights on this PR, all comments are welcome, even on closed PR.

This PR contains 2 things:

• The "right" fix on where we adress the issue when reacting to a specific DOM event.

-if (!isTargetParent((event as FocusEvent).relatedTarget, overlayRef)) {
+if (event.type !== "blur" || !isTargetParent((event as FocusEvent).relatedTarget, overlayRef)) {

• The rest of the code is more "future proofing" errors that could occur with tooltips. ~Most of~ All the the time, we only want one tooltip opened at a time. The gif in the issue shows multiple tooltip being opened at once (my fix should fix the issue) but should an other bug happen, at least it will prevent multiple tooltip to be shown.

For a design system component, I usually wouldn't recommend tracking other components state. I think components should only know about theirself or sometimes, when in a parent/child relationship, integrates with an opt-in context mecanism.

This code was directly taken from @react-stately/tooltip. The only reason i didnt take the package directly is because they have a notion of "warm up" and "delay" before showing tooltips, and we don't necessary want that concept.

So React-Spectrum do think it's recommended to track other component state.

For the other concerns:

• Using a weakmap here doesn't bring any value, since there is a useEffect that makes sure that the tooltip is removed from the map, and when a tooltip open, it deletes all other entries. There is no possibility of this object litteral to be the only one having a reference on the tooltip
• There is no need for the tooltipID to be passed, since we don't use this map to retreive specific tooltips.
• I could do the renaming, but since they are named this way in stately, i would prefer to keep their original name, since they are scoped inside a method and not public.
• Good catch for the isNil, i'll fix it in another PR

[patricklafrance]

Thanks for the clarifications!

Glag to know that you found the underlying issue, I misread the change and read it as you were "removing the "blur" check rather than adding it.

I would recommend to update the comment on the previous line accordingly to remember why the blur check has been added. That's the kind of fixes that most people tend to forget after a few days.

I would also recommend adding interaction tests to ensure the issue isn't reintroduced accidently later on.

The rest of the code is more "future proofing" errors that could occur with tooltips. Most of All the the time, we only want one tooltip opened at a time. The gif in the issue should multiple tooltip being opened at once (my fix should fix the issue) but should an other bug happen, at least it will prevent multiple tooltip to be shown.

I dont understand what you mean here. Are you inferring that the use case in the gif isn't valid?

Furthermore, if adding the "blur" check fixed the issue, why adding code that you don't need? Adding code only for "future proofing" IMO is more like adding complexity and futur code to maintain.

So React-Spectrum do think it's recommended to track other component state.

Well, as far as I know you are not aware of the context and the reasons behind the addition of this code to react-spectrum. So you can't say for sure that they think it's "recommended". I personally still think it's a bad idea to track other components state directly in DS land.

Using a weakmap here doesn't bring any value, since there is a useEffect that makes sure that the tooltip is removed from the map, and when a tooltip open, it deletes all other entries. There is no possibility of this object litteral to be the only one having a reference on the tooltip

Granted, I was preaching for the safe side on this one but you're right it's useless since there's a useEffect. I wonder how it works thought with SSR? Since userEffect is a no-op on the server, I guess the dispose function won't be called? Is it? Not sure how SSR manage memory.

There is no need for the tooltipID to be passed, since we don't use this map to retreive specific tooltips.

The idea here is that you already have a unique id for a tooltip, why use a new one. But yeah anyway it’s was a bad proposition and it leaks encapsulation. The consume shouldn’t be aware about the impementation details of the hook, even when it’s a 1:1 hook.

I could do the renaming, but since they are named this way in stately, i would prefer to keep their original name, since they are scoped inside a method and not public.

I am not sure why it matters that stately is already using this name. Anyway, up to you!