[Popper] Reposition when rerender

[cmeeren]

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

When a tooltip's label is updated, the label is not repositioned. Furthermore, this can cause the tooltip to overflow, causing scrollbars.

Expected Behavior 🤔

When the tooltip's label is updated, the label should be re-positioned, just like it is after moving the mouse away and over again.

Steps to Reproduce 🕹

Unfortunately I'm not a JS dev and can't easily create a JS/TS repro (I use MUI through F#/Fable), but it should be easy to repro. Here's a gif (the tooltip is the only relevant part). Note the horizontal scrollbar when the label overflows.

Your Environment 🌎

Tech	Version
Material-UI	v4.6.0
React	16.11.0
Browser	Electron (Chromium)

[oliviertassinari]

We really need a reproduction, we can't know what's wrong without.

[cmeeren]

Try just making a button with a tooltip, where clicking the button toggles between a short and a long tooltip label. That should be sufficient to repro it. Sorry I don't know how to do that in JS/TS outside of an F#/Elmish context.

[oliviertassinari]

@cmeeren Ok thanks, I could reproduce in https://codesandbox.io/s/material-demo-04kmy.

We have workaround this very problem in the past with this logic:

https://github.com/mui-org/material-ui/blob/575776f3004c6ac655b128fbdb30bd4b35115ab7/packages/material-ui-lab/src/Autocomplete/Autocomplete.js#L208-L213

https://github.com/mui-org/material-ui/blob/575776f3004c6ac655b128fbdb30bd4b35115ab7/docs/src/pages/components/slider/CustomizedSlider.tsx#L30-L35

Now, my question would be, is this something we can safely (performance) move into the Popper component?

[oliviertassinari]

I remember having a performance issue in the past (~1 year ago) with the Tooltip, we solved them with: #12085. It was related to React rerendering of hidden elements rather than popper.js itself. I suspect it's safe to move forward; I think that we can give it a try, see and learn.

Note that react-overlay doesn't update the position at each render.

What do you think of this diff?

diff --git a/docs/src/pages/components/slider/CustomizedSlider.tsx b/docs/src/pages/components/slider/CustomizedSlider.tsx
index e141773f7..63812a99f 100644
--- a/docs/src/pages/components/slider/CustomizedSlider.tsx
+++ b/docs/src/pages/components/slider/CustomizedSlider.tsx
@@ -27,18 +27,8 @@ interface Props {
 function ValueLabelComponent(props: Props) {
   const { children, open, value } = props;

-  const popperRef = React.useRef<PopperJs | null>(null);
-  React.useEffect(() => {
-    if (popperRef.current) {
-      popperRef.current.update();
-    }
-  });
-
   return (
     <Tooltip
-      PopperProps={{
-        popperRef,
-      }}
       open={open}
       enterTouchDelay={0}
       placement="top"
diff --git a/packages/material-ui-lab/src/Autocomplete/Autocomplete.js b/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
index ef98fbd61..ed37f078f 100644
--- a/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
+++ b/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
@@ -203,12 +203,6 @@ const Autocomplete = React.forwardRef(function Autocomplete(props, ref) {
   } = props;
   /* eslint-enable no-unused-vars */

-  const popperRef = React.useRef(null);
-  React.useEffect(() => {
-    if (popperRef.current) {
-      popperRef.current.update();
-    }
-  });
   const PopperComponent = disablePortal ? DisablePortal : PopperComponentProp;

   const {

diff --git a/packages/material-ui/src/Popper/Popper.js b/packages/material-ui/src/Popper/Popper.js
index 80f5f9909..8d506041d 100644
--- a/packages/material-ui/src/Popper/Popper.js
+++ b/packages/material-ui/src/Popper/Popper.js
@@ -186,6 +186,12 @@ const Popper = React.forwardRef(function Popper(props, ref) {
     }
   }, [open, transition]);

+  React.useEffect(() => {
+    if (open && popperRef.current) {
+      popperRef.current.update();
+    }
+  });
+
   if (!keepMounted && !open && (!transition || exited)) {
     return null;
   }

[cmeeren]

What do you think of this diff?

In case that was aimed at me instead of other maintainers: I have no idea. I can't see anything immediately wrong, but I have never used useEffect and know little about MUI internals or React performance in a quantitative sense.

[oliviertassinari]

@cmeeren Ok, let's leave this issue here in hibernation, we have a workaround, and a documented potential solution, if a developer (myself, a core contributor or an external contributor), in the future is hit by the problem and find it important enough to dedicate time to it, he will find a good starting point.

[eps1lon]

To be fair changing the label of an action is discouraged. A button should include every action associated with it in a label.

While some might consider it a bug it nudges users to not use (potential) inaccessible practices such as changing the label.

[oliviertassinari]

@eps1lon Thanks for sharing this a11y warning on the tooltip use case.

[cmeeren]

@eps1lon thanks for the tip. How would you then do the button/tooltip for the use-case I showed? (Toggling theme between light / dark / follow system).

I just read the part you linked to, and it seems that what's discouraged is changing the button enabled/disabled state together with the label, which is not what I'm doing (the button is always enabled). They give an example of "how to do things" which seems like an (enabled) play button toggling to an (enabled) pause button, with a label change. So that seems like exactly what I'm doing here.

[eps1lon]

(Toggling theme between light / dark / follow system).

I wouldn't use a toggle button for switching themes but a simple action button (toggling a tri-state isn't a toggle button).

For the play/pause use case I would follow the old casette player UI if you want toggle buttons.

I'd have to look more into this issue. We might talk past each other by using "toggle button".

[cmeeren]

We might talk past each other by using "toggle button".

Yes, I think we might be. The button I am using is never "toggled", i.e. it is never in a "pressed" state or "disabled" state. It is always active (just a normal, enabled button), and when clicked, will switch to the next theme (Light -> Dark -> System). The gif at the top shows this.

[MichaelFoss]

@cmeeren Ok thanks, I could reproduce in https://codesandbox.io/s/material-demo-04kmy.

We have workaround this very problem in the past with this logic:

https://github.com/mui-org/material-ui/blob/575776f3004c6ac655b128fbdb30bd4b35115ab7/packages/material-ui-lab/src/Autocomplete/Autocomplete.js#L208-L213

https://github.com/mui-org/material-ui/blob/575776f3004c6ac655b128fbdb30bd4b35115ab7/docs/src/pages/components/slider/CustomizedSlider.tsx#L30-L35

@oliviertassinari, this handles functional components, but how would you be able to call update from a ref within a class component?

Consider a class-based component that renders a Tooltip, but the Tooltip's popper contains an image that loads after the popper shows. In this instance, the popper could be positioned incorrectly. How could you hook into the React lifecycle to update the position of the popper after React updates the popper's contents?

To see an example of this, I've forked your original example and updated it to show exactly what's going on.

[oliviertassinari]

@MichaelFoss You can use ref in class components. I don't get the issue. We might want to ask on StackOverflow for support. If you can migrate your class component to a function + hook, even better.

[MichaelFoss]

@MichaelFoss You can use ref in class components. I don't get the issue. We might want to ask on StackOverflow for support. If you can migrate your class component to a function + hook, even better.

You're absolutely right.

My updated fork shows an example of how this is broken, and another example implementing your solution, as well as thorough documentation of what's going on (in case anyone else has the same question).

Thanks for the quick reply!

[sanchitbansal10]

@MichaelFoss, thanks for sharing the codesandbox soln.
I tried that but for some reason the popperRef is not getting attached to the ref I am creating in the class based component.

The value of ref.current is always coming as null.

I checked your sandbox also, and it seems it have the same issue
Can u pls let me know if its working as expected for you