[Portal] Prepare deprecation of onRendered (#16597)

* [Portal] Prepare deprecation of onRendered

* review

* breakdown the tests

* remove onRendered usage in the Popper component

docs/pages/api/modal.md

@@ -44,7 +44,7 @@ This component shares many concepts with [react-overlays](https://react-bootstra
 | <span class="prop-name">onBackdropClick</span> | <span class="prop-type">func</span> |  | Callback fired when the backdrop is clicked. |
 | <span class="prop-name">onClose</span> | <span class="prop-type">func</span> |  | Callback fired when the component requests to be closed. The `reason` parameter can optionally be used to control the response to `onClose`.<br><br>**Signature:**<br>`function(event: object, reason: string) => void`<br>*event:* The event source of the callback<br>*reason:* Can be:`"escapeKeyDown"`, `"backdropClick"` |
 | <span class="prop-name">onEscapeKeyDown</span> | <span class="prop-type">func</span> |  | Callback fired when the escape key is pressed, `disableEscapeKeyDown` is false and the modal is in focus. |
-| <span class="prop-name">onRendered</span> | <span class="prop-type">func</span> |  | Callback fired once the children has been mounted into the `container`. It signals that the `open={true}` prop took effect. |
+| <span class="prop-name">onRendered</span> | <span class="prop-type">func</span> |  | Callback fired once the children has been mounted into the `container`. It signals that the `open={true}` prop took effect.<br>This prop will be deprecated and removed in v5, the ref can be used instead. |
 | <span class="prop-name required">open&nbsp;*</span> | <span class="prop-type">bool</span> |  | If `true`, the modal is open. |
 
 The `ref` is forwarded to the root element.

docs/pages/api/portal.md

@@ -22,7 +22,7 @@ that exists outside the DOM hierarchy of the parent component.
 | <span class="prop-name required">children&nbsp;*</span> | <span class="prop-type">node</span> |  | The children to render into the `container`. |
 | <span class="prop-name">container</span> | <span class="prop-type">union:&nbsp;object&nbsp;&#124;<br>&nbsp;func<br></span> |  | A node, component instance, or function that returns either. The `container` will have the portal children appended to it. By default, it uses the body of the top-level document object, so it's simply `document.body` most of the time. |
 | <span class="prop-name">disablePortal</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | Disable the portal behavior. The children stay within it's parent DOM hierarchy. |
-| <span class="prop-name">onRendered</span> | <span class="prop-type">func</span> |  | Callback fired once the children has been mounted into the `container`. |
+| <span class="prop-name">onRendered</span> | <span class="prop-type">func</span> |  | Callback fired once the children has been mounted into the `container`.<br>This prop will be deprecated and removed in v5, the ref can be used instead. |
 
 The component cannot hold a ref.
 

packages/material-ui/src/Modal/Modal.js

@@ -112,7 +112,13 @@ const Modal = React.forwardRef(function Modal(props, ref) {
     }
   });
 
-  const handleRendered = useEventCallback(() => {
+  const handlePortalRef = useEventCallback(node => {
+    mountNodeRef.current = node;
+
+    if (!node) {
+      return;
+    }
+
     if (onRendered) {
       onRendered();
     }
@@ -216,12 +222,7 @@ const Modal = React.forwardRef(function Modal(props, ref) {
   }
 
   return (
-    <Portal
-      ref={mountNodeRef}
-      container={container}
-      disablePortal={disablePortal}
-      onRendered={handleRendered}
-    >
+    <Portal ref={handlePortalRef} container={container} disablePortal={disablePortal}>
       {/*
           Marking an element with the role presentation indicates to assistive technology
           that this element should be ignored; it exists to support the web application and
@@ -327,8 +328,7 @@ Modal.propTypes = {
   /**
    * @ignore
    *
-   * A modal manager used to track and manage the state of open
-   * Modals. This enables customizing how modals interact within a container.
+   * A modal manager used to track and manage the state of open Modals.
    */
   manager: PropTypes.object,
   /**
@@ -351,6 +351,8 @@ Modal.propTypes = {
   /**
    * Callback fired once the children has been mounted into the `container`.
    * It signals that the `open={true}` prop took effect.
+   *
+   * This prop will be deprecated and removed in v5, the ref can be used instead.
    */
   onRendered: PropTypes.func,
   /**

packages/material-ui/src/Popper/Popper.js

@@ -4,7 +4,7 @@ import PopperJS from 'popper.js';
 import { chainPropTypes } from '@material-ui/utils';
 import Portal from '../Portal';
 import { createChainedFunction } from '../utils/helpers';
-import { useForkRef } from '../utils/reactHelpers';
+import { setRef, useForkRef } from '../utils/reactHelpers';
 
 /**
  * Flips placement if in <body dir="rtl" />
@@ -58,7 +58,7 @@ const Popper = React.forwardRef(function Popper(props, ref) {
     ...other
   } = props;
   const tooltipRef = React.useRef(null);
-  const handleRef = useForkRef(tooltipRef, ref);
+  const ownRef = useForkRef(tooltipRef, ref);
 
   const popperRef = React.useRef(null);
   const handlePopperRefRef = React.useRef();
@@ -81,10 +81,6 @@ const Popper = React.forwardRef(function Popper(props, ref) {
   }
 
   const handleOpen = React.useCallback(() => {
-    const handlePopperUpdate = data => {
-      setPlacement(data.placement);
-    };
-
     const popperNode = tooltipRef.current;
 
     if (!popperNode || !anchorEl || !open) {
@@ -96,6 +92,10 @@ const Popper = React.forwardRef(function Popper(props, ref) {
       handlePopperRefRef.current(null);
     }
 
+    const handlePopperUpdate = data => {
+      setPlacement(data.placement);
+    };
+
     const popper = new PopperJS(getAnchorEl(anchorEl), popperNode, {
       placement: rtlPlacement,
       ...popperOptions,
@@ -118,6 +118,14 @@ const Popper = React.forwardRef(function Popper(props, ref) {
     handlePopperRefRef.current(popper);
   }, [anchorEl, disablePortal, modifiers, open, rtlPlacement, popperOptions]);
 
+  const handleRef = React.useCallback(
+    node => {
+      setRef(ownRef, node);
+      handleOpen();
+    },
+    [ownRef, handleOpen],
+  );
+
   const handleEnter = () => {
     setExited(false);
   };
@@ -169,7 +177,7 @@ const Popper = React.forwardRef(function Popper(props, ref) {
   }
 
   return (
-    <Portal onRendered={handleOpen} disablePortal={disablePortal} container={container}>
+    <Portal disablePortal={disablePortal} container={container}>
       <div
         ref={handleRef}
         role="tooltip"

packages/material-ui/src/Portal/Portal.js

@@ -2,7 +2,7 @@ import React from 'react';
 import ReactDOM from 'react-dom';
 import PropTypes from 'prop-types';
 import { exactProp } from '@material-ui/utils';
-import { useForkRef } from '../utils/reactHelpers';
+import { setRef, useForkRef } from '../utils/reactHelpers';
 
 function getContainer(container) {
   container = typeof container === 'function' ? container() : container;
@@ -19,22 +19,30 @@ const useEnhancedEffect = typeof window !== 'undefined' ? React.useLayoutEffect
 const Portal = React.forwardRef(function Portal(props, ref) {
   const { children, container, disablePortal = false, onRendered } = props;
   const [mountNode, setMountNode] = React.useState(null);
-  const childRef = React.useRef(null);
-  const handleRef = useForkRef(children.ref, childRef);
+  const handleRef = useForkRef(children.ref, ref);
 
   useEnhancedEffect(() => {
     if (!disablePortal) {
       setMountNode(getContainer(container) || document.body);
     }
   }, [container, disablePortal]);
 
-  React.useImperativeHandle(ref, () => mountNode || childRef.current, [mountNode]);
+  useEnhancedEffect(() => {
+    if (mountNode && !disablePortal) {
+      setRef(ref, mountNode);
+      return () => {
+        setRef(ref, null);
+      };
+    }
+
+    return undefined;
+  }, [ref, mountNode, disablePortal]);
 
   useEnhancedEffect(() => {
-    if (onRendered && mountNode) {
+    if (onRendered && (mountNode || disablePortal)) {
       onRendered();
     }
-  }, [mountNode, onRendered]);
+  }, [onRendered, mountNode, disablePortal]);
 
   if (disablePortal) {
     React.Children.only(children);
@@ -65,6 +73,8 @@ Portal.propTypes = {
   disablePortal: PropTypes.bool,
   /**
    * Callback fired once the children has been mounted into the `container`.
+   *
+   * This prop will be deprecated and removed in v5, the ref can be used instead.
    */
   onRendered: PropTypes.func,
 };

packages/material-ui/src/Portal/Portal.test.js

@@ -47,21 +47,48 @@ describe('<Portal />', () => {
     });
   });
 
-  it('should have access to the mountNode', () => {
-    const refSpy1 = spy();
-    mount(
-      <Portal ref={refSpy1}>
-        <h1>Foo</h1>
-      </Portal>,
-    );
-    assert.deepEqual(refSpy1.args, [[null], [null], [document.body]]);
-    const refSpy2 = spy();
-    mount(
-      <Portal disablePortal ref={refSpy2}>
-        <h1 className="woofPortal">Foo</h1>
-      </Portal>,
-    );
-    assert.deepEqual(refSpy2.args, [[document.querySelector('.woofPortal')]]);
+  describe('ref', () => {
+    it('should have access to the mountNode when disabledPortal={false}', () => {
+      const refSpy = spy();
+      const wrapper = mount(
+        <Portal ref={refSpy}>
+          <h1>Foo</h1>
+        </Portal>,
+      );
+      assert.deepEqual(refSpy.args, [[document.body]]);
+      wrapper.unmount();
+      assert.deepEqual(refSpy.args, [[document.body], [null]]);
+    });
+
+    it('should have access to the mountNode when disabledPortal={true}', () => {
+      const refSpy = spy();
+      const wrapper = mount(
+        <Portal disablePortal ref={refSpy}>
+          <h1 className="woofPortal">Foo</h1>
+        </Portal>,
+      );
+      const mountNode = document.querySelector('.woofPortal');
+      assert.deepEqual(refSpy.args, [[mountNode]]);
+      wrapper.unmount();
+      assert.deepEqual(refSpy.args, [[mountNode], [null]]);
+    });
+
+    it('should have access to the mountNode when switching disabledPortal', () => {
+      const refSpy = spy();
+      const wrapper = mount(
+        <Portal disablePortal ref={refSpy}>
+          <h1 className="woofPortal">Foo</h1>
+        </Portal>,
+      );
+      const mountNode = document.querySelector('.woofPortal');
+      assert.deepEqual(refSpy.args, [[mountNode]]);
+      wrapper.setProps({
+        disablePortal: false,
+      });
+      assert.deepEqual(refSpy.args, [[mountNode], [null], [document.body]]);
+      wrapper.unmount();
+      assert.deepEqual(refSpy.args, [[mountNode], [null], [document.body], [null]]);
+    });
   });
 
   it('should render in a different node', () => {