radix-ui / primitives

Radix Primitives is an open-source UI component library for building high-quality, accessible design systems and web apps. Maintained by @workos.
https://radix-ui.com/primitives
MIT License
15.82k stars 821 forks source link

[Tooltip] Wrapping the app in TooltipProvider causes every single tooltip to re-render on hover #2375

Open lauri865 opened 1 year ago

lauri865 commented 1 year ago

Bug report

Current Behavior

When wrapping the app in Tooltip provider, every single component that implements a tooltip gets re-rendered on hovering any of the components that implement a tooltip, even if nothing changes, causing excessive re-renders.

Expected behavior

Only components with an active tooltip should be affected or when default settings change.

Reproducible example

Wrap an app in TooltipProvider and turn on Highlight updates when components render in React devtools, and then hover any component with a tooltip to notice that every component that implements a tooltip gets re-rendered.

Suggested solution

Additional context

Your environment

Software Name(s) Version
Radix Package(s) @radix-ui/react-tooltip 1.0.6
React react 18.2.0
Browser Chrome Version 116.0.5845.140 (Official Build) (arm64)
Assistive tech
Node n/a
npm/yarn
Operating System
CarlosVergikosk commented 2 months ago

bump?

divmgl commented 1 month ago

Oh wow, we also just ran into this. Does this mean that we need to be wrapping every single Tooltip in TooltipProvider? I thought the opposite was supposed to be true.

aboveyunhai commented 1 month ago

Based on the Profiler highlight,

  1. It seems like anything under the Tooltip Provider will re-render upon the first detection (mouse-wise, keyboard-wise)
  2. after that, only the target item will be re-render when you hover them, rest wouldn't.
  3. then the last detection when you leave the Provider "gray area", will trig the re-render of all, like the first detection.

Hence, the Provider is pretty similar to how React.Context works, Where your issue might be from the first and last detection as far as I can tell, can't tell about the detection algorithm without digging though the source code. So you were claiming might not be true.

That also means, the following case can cause excessive re-renders, where you have a local Tooltip Provider that wraps lots of small items. For example, a calendar panel. That the total size of the Tooltip Provider (panel) in page is also small enough and your mouse is constantly in and out across the Provider detection area (panel edge). So you constantly trig the first(in)/last(out) detection. If you also have some heavy computations in each date button, without some sort of useMemo or useCallback. You might experience lag.

But you can't always make the conclusion based on the dev-tools highlight, always need to measure, the render might be cheap.

hichemfantar commented 1 month ago

ran into the same thing, this should be mentioned in the docs. would having multiple providers cause issues with render priority?

misha-erm commented 3 weeks ago

Here is a patch that fixes the issue

The trick is to use useRef for isOpenDelayed instead of useState. Seems like a safe change since the variable is read inside event callback and not inside render

Our app has tons of elements with tooltips on screen so the issue was quite critical for us. After the change only one element re-renders on hover

diff --git a/dist/index.js b/dist/index.js
index a805b534d2f34981afe9270bfbf0ad9e9a115d21..17f07a96ec77e4b1dcefa8554ee14fab602ba95b 100644
--- a/dist/index.js
+++ b/dist/index.js
@@ -79,7 +79,7 @@ var TooltipProvider = (props) => {
     disableHoverableContent = false,
     children
   } = props;
-  const [isOpenDelayed, setIsOpenDelayed] = React.useState(true);
+  const isOpenDelayedRef = React.useRef(true);
   const isPointerInTransitRef = React.useRef(false);
   const skipDelayTimerRef = React.useRef(0);
   React.useEffect(() => {
@@ -90,16 +90,16 @@ var TooltipProvider = (props) => {
     TooltipProviderContextProvider,
     {
       scope: __scopeTooltip,
-      isOpenDelayed,
+      isOpenDelayedRef,
       delayDuration,
       onOpen: React.useCallback(() => {
         window.clearTimeout(skipDelayTimerRef.current);
-        setIsOpenDelayed(false);
+        isOpenDelayedRef.current = (false);
       }, []),
       onClose: React.useCallback(() => {
         window.clearTimeout(skipDelayTimerRef.current);
         skipDelayTimerRef.current = window.setTimeout(
-          () => setIsOpenDelayed(true),
+          () => isOpenDelayedRef.current = (true),
           skipDelayDuration
         );
       }, [skipDelayDuration]),
@@ -178,9 +178,9 @@ var Tooltip = (props) => {
       trigger,
       onTriggerChange: setTrigger,
       onTriggerEnter: React.useCallback(() => {
-        if (providerContext.isOpenDelayed) handleDelayedOpen();
+        if (providerContext.isOpenDelayedRef.current) handleDelayedOpen();
         else handleOpen();
-      }, [providerContext.isOpenDelayed, handleDelayedOpen, handleOpen]),
+      }, [providerContext.isOpenDelayedRef, handleDelayedOpen, handleOpen]),
       onTriggerLeave: React.useCallback(() => {
         if (disableHoverableContent) {
           handleClose();
diff --git a/dist/index.mjs b/dist/index.mjs
index a6ac5447b420e89a93415510e2a6fc4b11ab96bd..065f5845826afdee9e01aa3191d6a48eeeed936d 100644
--- a/dist/index.mjs
+++ b/dist/index.mjs
@@ -32,7 +32,7 @@ var TooltipProvider = (props) => {
     disableHoverableContent = false,
     children
   } = props;
-  const [isOpenDelayed, setIsOpenDelayed] = React.useState(true);
+  const isOpenDelayedRef = React.useRef(true);
   const isPointerInTransitRef = React.useRef(false);
   const skipDelayTimerRef = React.useRef(0);
   React.useEffect(() => {
@@ -43,16 +43,16 @@ var TooltipProvider = (props) => {
     TooltipProviderContextProvider,
     {
       scope: __scopeTooltip,
-      isOpenDelayed,
+      isOpenDelayedRef,
       delayDuration,
       onOpen: React.useCallback(() => {
         window.clearTimeout(skipDelayTimerRef.current);
-        setIsOpenDelayed(false);
+        isOpenDelayedRef.current = false;
       }, []),
       onClose: React.useCallback(() => {
         window.clearTimeout(skipDelayTimerRef.current);
         skipDelayTimerRef.current = window.setTimeout(
-          () => setIsOpenDelayed(true),
+          () => isOpenDelayedRef.current = true,
           skipDelayDuration
         );
       }, [skipDelayDuration]),
@@ -131,9 +131,9 @@ var Tooltip = (props) => {
       trigger,
       onTriggerChange: setTrigger,
       onTriggerEnter: React.useCallback(() => {
-        if (providerContext.isOpenDelayed) handleDelayedOpen();
+        if (providerContext.isOpenDelayedRef.current) handleDelayedOpen();
         else handleOpen();
-      }, [providerContext.isOpenDelayed, handleDelayedOpen, handleOpen]),
+      }, [providerContext.isOpenDelayedRef, handleDelayedOpen, handleOpen]),
       onTriggerLeave: React.useCallback(() => {
         if (disableHoverableContent) {
           handleClose();
suitux commented 19 hours ago

Do you have plan to merge this? Still happening