Material-ui: [Popper] Reposition when rerender

[cmeeren]

• [x] The issue is present in the latest release.
• [x] 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.