Update Popover Reference element on rerender (#2833)

* compare to prevReferenceElement

* Adds tests for useReferenceElement

* Create popover-ref-el.md

* rm referenceElement from deps

* add lodash

Author: TheSonOfThomp

.changeset/popover-ref-el.md

@@ -0,0 +1,5 @@
+---
+'@leafygreen-ui/popover': patch
+---
+
+Updates reference element positioning logic to ensure the correct element is used as a reference in cases where the element `ref` or DOM node changes in between renders.

packages/popover/package.json

@@ -29,6 +29,7 @@
     "@leafygreen-ui/portal": "workspace:^",
     "@leafygreen-ui/tokens": "workspace:^",
     "@types/react-transition-group": "^4.4.5",
+    "lodash": "^4.17.21",
     "react-transition-group": "^4.4.5"
   },
   "devDependencies": {

packages/popover/src/Popover/Popover.tsx

@@ -15,11 +15,7 @@ import {
   getWindowSafePlacementValues,
 } from '../utils/positionUtils';
 
-import {
-  useContentNode,
-  usePopoverProps,
-  useReferenceElement,
-} from './Popover.hooks';
+import { useContentNode, usePopoverProps, useReferenceElement } from './hooks';
 import {
   contentClassName,
   getPopoverStyles,

packages/popover/src/Popover/hooks/Popover.hooks.spec.ts

@@ -0,0 +1,72 @@
+import { createRef, MutableRefObject } from 'react';
+import { act, renderHook } from '@testing-library/react';
+
+import { useReferenceElement } from './Popover.hooks';
+
+describe('packages/popover/hooks', () => {
+  describe('useReferenceElement', () => {
+    test('if a `refEl` is provided, the ref value is used as the reference element', () => {
+      const testRef: MutableRefObject<HTMLDivElement | null> =
+        createRef<HTMLDivElement>();
+      testRef.current = document.createElement('div');
+
+      const { result } = renderHook(() => useReferenceElement(testRef));
+
+      expect(result.current.referenceElement).toBe(testRef.current);
+    });
+
+    test('if the `refEl` changes between renders, the reference element should update', () => {
+      const testRef: MutableRefObject<HTMLDivElement | null> =
+        createRef<HTMLDivElement>();
+      testRef.current = document.createElement('div');
+      testRef.current.setAttribute('data-testid', 'testRef');
+
+      const { result, rerender } = renderHook(
+        ({ refEl }) => useReferenceElement(refEl),
+        {
+          initialProps: { refEl: testRef },
+        },
+      );
+
+      const newRef: MutableRefObject<HTMLDivElement | null> =
+        createRef<HTMLDivElement>();
+      newRef.current = document.createElement('div');
+      newRef.current.setAttribute('data-testid', 'newRef');
+
+      rerender({ refEl: newRef });
+      expect(result.current.referenceElement).toBe(newRef.current);
+    });
+
+    test('if `refEl.current` changes between renders, the reference element should update', () => {
+      const testRef: MutableRefObject<HTMLDivElement | null> =
+        createRef<HTMLDivElement>();
+      testRef.current = document.createElement('div');
+      testRef.current.setAttribute('data-testid', 'testRef');
+
+      const { result, rerender } = renderHook(
+        ({ refEl }) => useReferenceElement(refEl),
+        {
+          initialProps: { refEl: testRef },
+        },
+      );
+      testRef.current = document.createElement('div');
+      testRef.current.setAttribute('data-testid', 'testRef2');
+
+      rerender({ refEl: testRef });
+      expect(result.current.referenceElement).toBe(testRef.current);
+    });
+
+    test('if a `refEl` is not provided, the parent element of the placeholder is used as the reference element', () => {
+      const { result } = renderHook(() => useReferenceElement());
+      const placeholderElement = document.createElement('span');
+      const parentElement = document.createElement('div');
+      parentElement.appendChild(placeholderElement);
+
+      act(() => {
+        result.current.setPlaceholderElement(placeholderElement);
+      });
+
+      expect(result.current.referenceElement).toBe(parentElement);
+    });
+  });
+});

packages/popover/src/Popover/Popover.hooks.tsx renamed to packages/popover/src/Popover/hooks/Popover.hooks.ts

@@ -1,23 +1,24 @@
 import React, { useMemo, useRef, useState } from 'react';
+import isEqual from 'lodash/isEqual';
 
 import {
   useIsomorphicLayoutEffect,
   useObjectDependency,
+  usePrevious,
 } from '@leafygreen-ui/hooks';
 import {
   useMigrationContext,
   usePopoverPortalContainer,
   usePopoverPropsContext,
 } from '@leafygreen-ui/leafygreen-provider';
 
-import { getElementDocumentPosition } from '../utils/positionUtils';
-
+import { getElementDocumentPosition } from '../../utils/positionUtils';
 import {
   PopoverProps,
   RenderMode,
   UseContentNodeReturnObj,
   UseReferenceElementReturnObj,
-} from './Popover.types';
+} from '../Popover.types';
 
 /**
  * This hook handles logic for determining what prop values are used for the `Popover`
@@ -129,9 +130,18 @@ export function useReferenceElement(
     null,
   );
 
+  const prevReferenceElement = usePrevious(refEl?.current);
+
+  // If the DOM element has changed, we need to update the reference element.
+  // Store this variable so we can trigger the useLayoutEffect
+  const didRefElementChange = !isEqual(prevReferenceElement, refEl?.current);
+
   useIsomorphicLayoutEffect(() => {
     if (refEl && refEl.current) {
-      setReferenceElement(refEl.current);
+      if (didRefElementChange) {
+        setReferenceElement(refEl.current);
+      }
+
       return;
     }
 
@@ -142,7 +152,7 @@ export function useReferenceElement(
       setReferenceElement(maybeParentEl);
       return;
     }
-  }, [placeholderElement, refEl]);
+  }, [didRefElementChange, placeholderElement, refEl]);
 
   const referenceElDocumentPos = useObjectDependency(
     useMemo(

packages/popover/src/Popover/hooks/index.ts

@@ -0,0 +1 @@
+export * from './Popover.hooks';