fix(FloatingMenu): lazy evaluate trigger button position (#5881)

This change switches the position calculation logic for the trigger
buttons of `<OverflowMenu>` and `<Tooltip>` from eager-evaluation on
mount to lazy-evaluation on rendering `<FloatingMenu>`.

This change relegates such logic from `<OverflowMenu>` and `<Tooltip>`
to `<FloatingMenu>` to do that.

Such change has two merits:

1. Avoids stale trigger position data, which happened when `<Tooltip>`
   is opened programmatically
2. Reduces `getBoundingClientRect()` calls when application is
   initialized

Also, swiches `<FloatingMenu>`'s prop for the DOM element ref of
trigger button, as well as the corresponding internal properties of
`<OverflowMenu>`/`<Tooltip>` from raw element to React ref.

Fixes #5305.

Author: asudoh

packages/react/src/components/OverflowMenu/OverflowMenu.js

@@ -15,7 +15,6 @@ import FloatingMenu, {
   DIRECTION_TOP,
   DIRECTION_BOTTOM,
 } from '../../internal/FloatingMenu';
-import OptimizedResize from '../../internal/OptimizedResize';
 import { OverflowMenuVertical16 } from '@carbon/icons-react';
 import { keys, matches as keyCodeMatches } from '../../internal/keyboard';
 import mergeRefs from '../../tools/mergeRefs';
@@ -240,29 +239,17 @@ class OverflowMenu extends Component {
    */
   _hBlurTimeout;
 
-  shouldComponentUpdate(nextProps, nextState) {
-    if (nextState.open && !this.state.open) {
-      requestAnimationFrame(() => {
-        this.getMenuPosition();
-      });
-      return false; // Let `.getMenuPosition()` cause render
-    }
-
-    return true;
-  }
-
-  componentDidMount() {
-    requestAnimationFrame(() => {
-      this.getMenuPosition();
-    });
-    this.hResize = OptimizedResize.add(() => {
-      this.getMenuPosition();
-    });
-  }
+  /**
+   * The element ref of the tooltip's trigger button.
+   * @type {React.RefObject<Element>}
+   * @private
+   */
+  _triggerRef = React.createRef();
 
   getPrimaryFocusableElement = () => {
-    if (this.menuEl) {
-      const primaryFocusPropEl = this.menuEl.querySelector(
+    const { current: triggerEl } = this._triggerRef;
+    if (triggerEl) {
+      const primaryFocusPropEl = triggerEl.querySelector(
         '[data-floating-menu-primary-focus]'
       );
       if (primaryFocusPropEl) {
@@ -301,16 +288,8 @@ class OverflowMenu extends Component {
       clearTimeout(this._hBlurTimeout);
       this._hBlurTimeout = undefined;
     }
-    this.hResize.release();
   }
 
-  getMenuPosition = () => {
-    if (this.menuEl) {
-      const menuPosition = this.menuEl.getBoundingClientRect();
-      this.setState({ menuPosition });
-    }
-  };
-
   handleClick = evt => {
     if (!this._menuBody || !this._menuBody.contains(evt.target)) {
       this.setState({ open: !this.state.open });
@@ -365,13 +344,10 @@ class OverflowMenu extends Component {
     });
   };
 
-  bindMenuEl = menuEl => {
-    this.menuEl = menuEl;
-  };
-
   focusMenuEl = () => {
-    if (this.menuEl) {
-      this.menuEl.focus();
+    const { current: triggerEl } = this._triggerRef;
+    if (triggerEl) {
+      triggerEl.focus();
     }
   };
 
@@ -428,9 +404,10 @@ class OverflowMenu extends Component {
         focusinEventName,
         event => {
           const { target } = event;
+          const { current: triggerEl } = this._triggerRef;
           if (
             !menuBody.contains(target) &&
-            this.menuEl &&
+            triggerEl &&
             !target.matches(
               `.${prefix}--overflow-menu,.${prefix}--overflow-menu-options`
             )
@@ -448,8 +425,9 @@ class OverflowMenu extends Component {
    * @returns {Element} The DOM element where the floating menu is placed in.
    */
   _getTarget = () => {
+    const { current: triggerEl } = this._triggerRef;
     return (
-      (this.menuEl && this.menuEl.closest('[data-floating-menu-container]')) ||
+      (triggerEl && triggerEl.closest('[data-floating-menu-container]')) ||
       document.body
     );
   };
@@ -525,11 +503,10 @@ class OverflowMenu extends Component {
 
     const wrappedMenuBody = (
       <FloatingMenu
-        menuPosition={this.state.menuPosition}
+        triggerRef={this._triggerRef}
         menuDirection={direction}
         menuOffset={flipped ? menuOffsetFlip : menuOffset}
         menuRef={this._bindMenuBody}
-        menuEl={this.menuEl}
         flipped={this.props.flipped}
         target={this._getTarget}
         onPlace={this._handlePlace}>
@@ -559,7 +536,7 @@ class OverflowMenu extends Component {
           aria-label={ariaLabel}
           id={id}
           tabIndex={tabIndex}
-          ref={mergeRefs(ref, this.bindMenuEl)}>
+          ref={mergeRefs(this._triggerRef, ref)}>
           <IconElement {...iconProps}>
             {iconDescription && <title>{iconDescription}</title>}
           </IconElement>

packages/react/src/components/Tooltip/Tooltip-test.js

@@ -198,54 +198,4 @@ describe('Tooltip', () => {
       expect(rootWrapper.find('Tooltip').instance().state.open).toEqual(false);
     });
   });
-
-  describe('getTriggerPosition', () => {
-    it('sets triggerPosition when triggerEl is set', () => {
-      const rootWrapper = mount(<Tooltip triggerText="Tooltip" />);
-      // Enzyme doesn't seem to allow setState() in a forwardRef-wrapped class component
-      rootWrapper
-        .find('Tooltip')
-        .instance()
-        .setState({
-          triggerPosition: { left: 0, top: 0, right: 0, bottom: 0 },
-        });
-      rootWrapper.update();
-      rootWrapper
-        .find('Tooltip')
-        .instance()
-        .getTriggerPosition();
-      // Enzyme doesn't seem to allow state() in a forwardRef-wrapped class component
-      expect(rootWrapper.find('Tooltip').state.triggerPosition).not.toEqual({
-        left: 0,
-        top: 0,
-        right: 0,
-        bottom: 0,
-      });
-    });
-    it('does not set triggerPosition when triggerEl is not set', () => {
-      const rootWrapper = mount(<Tooltip triggerText="Tooltip" />);
-      // Enzyme doesn't seem to allow setState() in a forwardRef-wrapped class component
-      rootWrapper
-        .find('Tooltip')
-        .instance()
-        .setState({
-          triggerPosition: { left: 0, top: 0, right: 0, bottom: 0 },
-        });
-      rootWrapper.update();
-      delete rootWrapper.find('Tooltip').instance().triggerEl;
-      rootWrapper
-        .find('Tooltip')
-        .instance()
-        .getTriggerPosition();
-      // Enzyme doesn't seem to allow state() in a forwardRef-wrapped class component
-      expect(
-        rootWrapper.find('Tooltip').instance().state.triggerPosition
-      ).toEqual({
-        left: 0,
-        top: 0,
-        right: 0,
-        bottom: 0,
-      });
-    });
-  });
 });

packages/react/src/components/Tooltip/Tooltip.js

@@ -210,13 +210,17 @@ class Tooltip extends Component {
    */
   _tooltipEl = null;
 
+  /**
+   * The element ref of the tooltip's trigger button.
+   * @type {React.RefObject<Element>}
+   * @private
+   */
+  _triggerRef = React.createRef();
+
   componentDidMount() {
     if (!this._debouncedHandleFocus) {
       this._debouncedHandleFocus = debounce(this._handleFocus, 200);
     }
-    requestAnimationFrame(() => {
-      this.getTriggerPosition();
-    });
 
     document.addEventListener('keydown', this.handleEscKeyPress, false);
   }
@@ -251,13 +255,6 @@ class Tooltip extends Component {
     });
   };
 
-  getTriggerPosition = () => {
-    if (this.triggerEl) {
-      const triggerPosition = this.triggerEl.getBoundingClientRect();
-      this.setState({ triggerPosition });
-    }
-  };
-
   /**
    * Handles `focus`/`blur` event.
    * @param {string} state `over` to show the tooltip, `out` to hide the tooltip.
@@ -266,15 +263,15 @@ class Tooltip extends Component {
   _handleFocus = (state, evt) => {
     const { relatedTarget } = evt;
     if (state === 'over') {
-      this.getTriggerPosition();
       this._handleUserInputOpenClose(evt, { open: true });
     } else {
       // Note: SVGElement in IE11 does not have `.contains()`
+      const { current: triggerEl } = this._triggerRef;
       const shouldPreventClose =
         relatedTarget &&
-        ((this.triggerEl &&
-          this.triggerEl.contains &&
-          this.triggerEl.contains(relatedTarget)) ||
+        ((triggerEl &&
+          triggerEl.contains &&
+          triggerEl.contains(relatedTarget)) ||
           (this._tooltipEl && this._tooltipEl.contains(relatedTarget)));
       if (!shouldPreventClose) {
         this._handleUserInputOpenClose(evt, { open: false });
@@ -292,10 +289,13 @@ class Tooltip extends Component {
   /**
    * @returns {Element} The DOM element where the floating menu is placed in.
    */
-  _getTarget = () =>
-    (this.triggerEl &&
-      this.triggerEl.closest('[data-floating-menu-container]')) ||
-    document.body;
+  _getTarget = () => {
+    const { current: triggerEl } = this._triggerRef;
+    return (
+      (triggerEl && triggerEl.closest('[data-floating-menu-container]')) ||
+      document.body
+    );
+  };
 
   handleMouse = evt => {
     evt.persist();
@@ -312,9 +312,6 @@ class Tooltip extends Component {
       const shouldOpen = this.isControlled
         ? !this.props.open
         : !this.state.open;
-      if (shouldOpen) {
-        this.getTriggerPosition();
-      }
       this._handleUserInputOpenClose(evt, { open: shouldOpen });
     } else if (
       state &&
@@ -348,9 +345,6 @@ class Tooltip extends Component {
       const shouldOpen = this.isControlled
         ? !this.props.open
         : !this.state.open;
-      if (shouldOpen) {
-        this.getTriggerPosition();
-      }
       this._handleUserInputOpenClose(event, { open: shouldOpen });
     }
   };
@@ -403,10 +397,7 @@ class Tooltip extends Component {
       triggerClassName
     );
 
-    const refProp = mergeRefs(ref, node => {
-      this.triggerEl = node;
-    });
-
+    const refProp = mergeRefs(this._triggerRef, ref);
     const iconProperties = { name: iconName, role: null, description: null };
 
     const properties = {
@@ -457,7 +448,7 @@ class Tooltip extends Component {
         {open && (
           <FloatingMenu
             target={this._getTarget}
-            menuPosition={this.state.triggerPosition}
+            triggerRef={this._triggerRef}
             menuDirection={direction}
             menuOffset={menuOffset}
             menuRef={node => {

packages/react/src/internal/FloatingMenu.js

@@ -11,6 +11,7 @@ import React from 'react';
 import ReactDOM from 'react-dom';
 import window from 'window-or-global';
 import { settings } from 'carbon-components';
+import OptimizedResize from './OptimizedResize';
 
 const { prefix } = settings;
 
@@ -84,7 +85,7 @@ const hasChangeInOffset = (oldMenuOffset = {}, menuOffset = {}) => {
  */
 const getFloatingPosition = ({
   menuSize,
-  refPosition,
+  refPosition = {},
   offset = {},
   direction = DIRECTION_BOTTOM,
   scrollX = 0,
@@ -153,16 +154,6 @@ class FloatingMenu extends React.Component {
      */
     target: PropTypes.func,
 
-    /**
-     * The position in the viewport of the trigger button.
-     */
-    menuPosition: PropTypes.shape({
-      top: PropTypes.number,
-      right: PropTypes.number,
-      bottom: PropTypes.number,
-      left: PropTypes.number,
-    }),
-
     /**
      * Where to put the tooltip, relative to the trigger button.
      */
@@ -201,7 +192,6 @@ class FloatingMenu extends React.Component {
   };
 
   static defaultProps = {
-    menuPosition: {},
     menuOffset: {},
     menuDirection: DIRECTION_BOTTOM,
   };
@@ -239,7 +229,6 @@ class FloatingMenu extends React.Component {
    * Calculates the position in the viewport of floating menu,
    * once this component is mounted or updated upon change in the following props:
    *
-   * * `menuPosition` (The position in the viewport of the trigger button)
    * * `menuOffset` (The adjustment that should be applied to the calculated floating menu's position)
    * * `menuDirection` (Where the floating menu menu should be placed relative to the trigger button)
    *
@@ -256,30 +245,23 @@ class FloatingMenu extends React.Component {
     }
 
     const {
-      menuPosition: oldRefPosition = {},
       menuOffset: oldMenuOffset = {},
       menuDirection: oldMenuDirection,
     } = prevProps;
-    const {
-      menuPosition: refPosition = {},
-      menuOffset = {},
-      menuDirection,
-    } = this.props;
+    const { menuOffset = {}, menuDirection } = this.props;
 
     if (
-      oldRefPosition.top !== refPosition.top ||
-      oldRefPosition.right !== refPosition.right ||
-      oldRefPosition.bottom !== refPosition.bottom ||
-      oldRefPosition.left !== refPosition.left ||
       hasChangeInOffset(oldMenuOffset, menuOffset) ||
       oldMenuDirection !== menuDirection
     ) {
+      const { flipped, triggerRef } = this.props;
+      const { current: triggerEl } = triggerRef;
       const menuSize = menuBody.getBoundingClientRect();
-      const { menuEl, flipped } = this.props;
+      const refPosition = triggerEl && triggerEl.getBoundingClientRect();
       const offset =
         typeof menuOffset !== 'function'
           ? menuOffset
-          : menuOffset(menuBody, menuDirection, menuEl, flipped);
+          : menuOffset(menuBody, menuDirection, triggerEl, flipped);
       // Skips if either in the following condition:
       // a) Menu body has `display:none`
       // b) `menuOffset` as a callback returns `undefined` (The callback saw that it couldn't calculate the value)
@@ -302,6 +284,16 @@ class FloatingMenu extends React.Component {
     }
   };
 
+  componentWillUnmount() {
+    this.hResize.release();
+  }
+
+  componentDidMount() {
+    this.hResize = OptimizedResize.add(() => {
+      this._updateMenuSize();
+    });
+  }
+
   componentDidUpdate(prevProps) {
     this._updateMenuSize(prevProps);
     const { onPlace } = this.props;