Material-ui: [Tooltip/Popper]: keepMounted doesn't work when `open` updated before `Portal` updates

[Sharakai]

• [x] This is not a v0.x issue.
• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

When the PopperProps.keepMounted prop is set to false, I expect the <Tooltip /> / <Popper /> / <Portal /> component to not exist in the DOM whenever the Tooltip is closed (open prop set to false).

If the open prop changes from true to false before the <Portal /> component gets its' mount node and re-renders, I'd expect the Tooltip to never exist on the DOM (or at least immediately clean itself up after attaching to the DOM)

Current Behavior 😯

Not that!

If the open prop is changed from false to true, and then set back to false within the next update phase (or the setState callback), the Tooltip is added to the DOM and not removed, even though open is false.

Steps to Reproduce 🕹

Link: https://codesandbox.io/embed/4jjq50051x

1. Open DevTools so you can see the React DOM root (It's nested inside the iframe, so you might want to search for "Index-root- to find it faster)
2. Press the "TOGGLE TOOLTIP" button - This will set the state to open, and then set the state to false on the next update (setState callback)
3. Note that the Tooltip Portal element is now on the DOM, but not visible on the screen

If you comment out line 34, and uncomment lines 36-38, this works fine because the closing setState is now after the next update phase, allowing the react-transition-group Transition component to fully render and set its' internal state to ENTERED.
When the component updates to in: false, it will then invoke MUI's Grow and Popper's handleExited functions and remove the Portal from the DOM.

Context 🔦

We have an RxJS stream which is indirectly used in hiding/showing a component Tooltip. This can cause the Tooltip to have open: true and then open: false in the update phase immediately following, which causes this issue.

We use the fact that the Tooltip unmounts in our functional tests, when we're waiting (or testing) for the removal of the Tooltip. Therefore, this bug causes our functional tests to fail.

It obviously also causes our DOM to be littered with redundant tooltips which shouldn't exist.

Your Environment 🌎

| Tech | Version |
|--------------|---------|
| Material-UI | v3.9.3 |
| React | v16.8.6 |
| Browser | Chrome 73 |
| TypeScript | v3.4.1 |

[oliviertassinari]

@Sharakai We most likely have the same issue with the Modal component. What do you think of this change?

--- a/packages/material-ui/src/Popper/Popper.js
+++ b/packages/material-ui/src/Popper/Popper.js
@@ -66,23 +66,6 @@ class Popper extends React.Component {
     this.handleClose();
   }

-  static getDerivedStateFromProps(nextProps) {
-    if (nextProps.open) {
-      return {
-        exited: false,
-      };
-    }
-
-    if (!nextProps.transition) {
-      // Otherwise let handleExited take care of marking exited.
-      return {
-        exited: true,
-      };
-    }
-
-    return null;
-  }
-
   handleOpen = () => {
     const { anchorEl, modifiers, open, placement, popperOptions = {}, disablePortal } = this.props;
     const popperNode = this.tooltipRef.current;
@@ -126,6 +109,10 @@ class Popper extends React.Component {
     }
   };

+  handleEntered = () => {
+    this.setState({ exited: false });
+  };
+
   handleExited = () => {
     this.setState({ exited: true });
     this.handleClose();
@@ -174,6 +161,7 @@ class Popper extends React.Component {
       childProps.TransitionProps = {
         in: open,
         onExited: this.handleExited,
+        onEntered: this.handleEntered,
       };
     }

It seems to solve the problem and to help in the upcoming class -> hook migration cc @joshwooding. It would be great to add a failing test case for your codesandbox as well as migrate the Modal component if this solution is satisfactory. Do you want to give it a shot? :)

[joshwooding]

This would definitely simplify the hook migration :) Modal might be slightly more difficult though

[Sharakai]

Right you are with the Modal component. I've made a codesandbox to repro that, too: https://codesandbox.io/s/2xolvp57n0

I've added a failing test case for both Popper and Modal, as well as a few others to ensure the keepMounted prop works as expected for both components. - I'll PR them up, along with the change to fix both Popper and Modal as soon as I can.

@oliviertassinari The proposed change works fine, though I'm still trying to grok how...

@joshwooding If there's anything I can do to help with the hooks migration, I'd be more than happy to help. I'll make this change sans-hooks, of course, but happy to look at switching either/or later on.

[joshwooding]

@Sharakai More than happy for you to help. I've created an umbrella issue with the components we have left (#15231). I will warn you that some of the ones left are quite difficult