fix: Stale closure preventing tooltip from closing on mouseleave

[tjosepo]

Closes #1141

Summary

Seems like tooltips remaining open was related to performance, as it mostly appeared in performance intensive areas like virtualized tables.

My guess was that the issue was caused by calling the onmouseenter and onmouseleave callbacks in a single render.

My guess seems to have been confirmed after simulating a slow render by adding these lines to the TriggerTooltip

// We simulate a render that takes a least 500ms to happen.
// This gives us enough time to trigger both onmouseenter and onmouseleave events in a single render.
const start = performance.now();
while(performance.now() - 500 < start) {};

What I did

After looking into the code, I noticed that we had an if-statement that looked at the current value of isOpen before updating the state. isOpen can become stale if onmouseenter and onmouseleave happen in a single render. I modified the setter function to accept an updater function, which always passes the most up-to-date state as the parameter, which fixes the stale closure issue.

How to test

No idea if we can make a test for this.

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: bf36b19dd935968ffc7f7aa9daac0b31bd47c2f1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

| Name | Type | | -------------------- | ----- | | @orbit-ui/components | Patch | | @sharegate/orbit-ui | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[netlify[bot]]

✅ Deploy Preview for sg-storybook ready!

Name	Link
🔨 Latest commit	bf36b19dd935968ffc7f7aa9daac0b31bd47c2f1
🔍 Latest deploy log	https://app.netlify.com/sites/sg-storybook/deploys/645c04de27f38a00081d55f4
😎 Deploy Preview	https://deploy-preview-1212--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.

[netlify[bot]]

✅ Deploy Preview for sg-orbit ready!

Name	Link
🔨 Latest commit	bf36b19dd935968ffc7f7aa9daac0b31bd47c2f1
🔍 Latest deploy log	https://app.netlify.com/sites/sg-orbit/deploys/645c04de359eb000082a50ad
😎 Deploy Preview	https://deploy-preview-1212--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.

[patricklafrance]

Does it still work if TooltipTrigger is controlled by externally managing the isOpen state value?

[tjosepo]

Good catch. I think you might be right. I'll add a test to check what happens when the tooltip is controlled and onOpenChange is triggered.

I think I might just turn the ref into a simple mutable variable that gets reset on every render.

[patricklafrance]

An old school trick to fix those kind of issue is to add a setTimeout() of 0. It usually hide a deeper issue thought but it might be fine in this case.

Using setTimeout, you can try something like the following:

    const updateIsOpen = useCallback((event: SyntheticEvent, newValue: boolean) => {
        setTimeout(() => {
                if (isOpen !== newValue) {
                    setIsOpen(newValue);

                    if (!isNil(onOpenChange)) {
                        onOpenChange(event, newValue);
                   }
                }
        }, 0);
    }, [onOpenChange, isOpen, setIsOpen]);

I think the setTimeout would opt out everything inside from the current React event "transaction" thought. Might not be that much of a big deal in this case, not sure.

I wonder if https://react.dev/reference/react/useTransition could help here? Not sure how it would integrate with Orbit controlled/uncontrolled support thought. Might not worth it if it requires to add an alternate hook to useControllableState

[patricklafrance]

Good catch. I think you might be right. I'll add a test to check what happens when the tooltip is controlled and onOpenChange is triggered.

I think I might just turn the ref into a simple mutable variable that gets reset on every render.

There's already a hook for this: https://github.com/gsoft-inc/sg-orbit/blob/master/packages/components/src/shared/src/useCommittedRef.ts

Another thing you could do is use the useControllableState optional onChange callback to update your ref value when the state value change.

That being said, that ref does looks more like a patch than fixing the underlying issue.

[tjosepo]

I decided to change approach and modify the setter of useControllableState to accept an updater function.

It works just like useState's updater function.

I added a test to make sure the onOpenChange fires properly when the Tooltip component is controlled.

I also added tests to verify that the new updater function works as intended.

Do you think this is the right approach?

[tjosepo]

I wonder if https://react.dev/reference/react/useTransition could help here? Not sure how it would integrate with Orbit controlled/uncontrolled support thought. Might not worth it if it requires to add an alternate hook to useControllableState

I think transitions might help hide the problem by making the renders faster, but not solve the underlying issue.

I also wonder how useTransition and controlled/uncontrolled components would play out. 🤔

The new React documentation has a very vague warning about using ref.current during rendering:

Do not write or read ref.current during rendering, except for initialization. This makes your component’s behavior unpredictable.

No idea what it means by "makes your component’s behavior unpredictable", but I think it might be linked to supense & transitions. This may cause issues with hooks like useControllableState which rely a lot on refs.

A hook like useSyncExternalStore, might be a good alternative to using refs, since it also lets us sync renders with a non-React state. I believe the hook also has some built-in support for Transitions and Suspense that makes it return the previous state when the component is being suspended (vs. the ref which always returns the current state.)

[patricklafrance]

It's an interesting change.

The updater function idea kind of make sense. There's an issue thought in the current implementation because it doesn't take into account that a provided onChange function could return an updated state value.

The onChange function is kind of a mess on its own, the type of issues that it solves should usually be adressed with a state machine / reducer but I never really understood how to integrate those with the components controlled/uncontrolled behavior.

Anyway, that's an issue for another time :)

Do you think it's a good idea that an updater function calls an handler? I feel like an updater function should be pure.

[tjosepo]

Do you think it's a good idea that an updater function calls an handler? I feel like an updater function should be pure.

I verified, and you're right. React throws a warning when you set another component's state inside a useState updater function. It works, but it triggers two full renders. :/

I'll look into it some more, but I think I'll go for the onChange function.

[tjosepo]

I'm having issues determining what should happen some issues with the behavior of onOpenChange when the tooltip is controlled.

Assuming we only want to fire onOpenChange when the value changes (e.g. when the tooltip is controlled and open, we only want to signal closing changes), but when the state changes twice in a single render, we don't know if we should fire the onOpenChange event again, because won't know if the open prop changed until the next render.

I looked at all our codebases with the RegEx /<TooltipTrigger.*onOpenChange.*>/ to see if there was any usage of the TooltipTrigger component in one of our projects, but I couldn't find any.

I'm considering changing the behavior of onOpenChange events to always fire on mouseenter and mouseleave, regardless of the current open state.

[patricklafrance]

I'm having issues determining what should happen some issues with the behavior of onOpenChange when the tooltip is controlled.

Assuming we only want to fire onOpenChange when the value changes (e.g. when the tooltip is controlled and open, we only want to signal closing changes), but when the state changes twice in a single render, we don't know if we should fire the onOpenChange event again, because won't know if the open prop changed until the next render.

I looked at all our codebases with the RegEx /<TooltipTrigger.*onOpenChange.*>/ to see if there was any usage of the TooltipTrigger component in one of our projects, but I couldn't find any.

I'm considering changing the behavior of onOpenChange events to always fire on mouseenter and mouseleave, regardless of the current open state.

I am not sure why you would want to do this. As a consumer I don't want the onOpenChange handler to be called if the open state didn't change.

By the way, this check to prevent accidental fire is something that has been standardized to every component handler.

[tjosepo]

If we don't fire both change events, we might not call onOpenChange for mouseleave, causing the tooltip to remain open indefinitely.

[patricklafrance]

Hey @tjosepo 👋🏻

Assuming we only want to fire onOpenChange when the value changes (e.g. when the tooltip is controlled and open, we only want to signal closing changes)

Is it an assumption you are making to explain your proposition or are you considering it's always the case? I can imagine scenarios in which an handler is listening to open/close state changes to track the state of an uncontrolled tooltip component and react accordingly.

but when the state changes twice in a single render

AFAIK it shoudn't be possible, whenever a state prop change, a re-render is happening, the next state change is handled by a subsequent re-render. Maybe you are not refering to actual React state change here?

[tjosepo]

AFAIK it shoudn't be possible, whenever a state prop change, a re-render is happening, the next state change is handled by a subsequent re-render. Maybe you are not refering to actual React state change here?

Right, but because the callback functions of mouseenter and mouseleave compare the current state of isOpen with the new value before update the state, it's possible to skip updating the state when onmouseenter and onmouseleave happen in quick succession, faster than React has time to complete a full re-render and update the value of isOpen.

This is why I think the best approach is to change the state without doing a comparison with isOpen beforehand, as the value might be stale and there is no way to know if the state prop open will change value in the middle of a re-render.

In my latest commit, I did just that:

const updateIsOpen = useCallback((event: SyntheticEvent, newValue: boolean) => {
- if (isOpen !== newValue) {
    setIsOpen(newValue);

    if (!isNil(onOpenChange)) {
        onOpenChange(event, newValue);
    }
- }
}

[patricklafrance]

Sounds reasonable to me, we can try it.

Could you somehow document your fix so there's a trace in the code? Not sure how thought since the fix is to remove code 😆

Also, to accept this PR, could you remove changes to useControllableState ? It should be added in a distinct PR if we want to go this way.

[tjosepo]

I updated the PR!