Remove support for passing a non-Element ref to useElementShouldClose

robertknight · robertknight · commit f7706eb91d0e · 2021-02-02T14:37:54.000Z
`useElementShouldClose` allowed passing a ref to a Preact component rather than a ref to a DOM element. Support for this relied on Preact component's having a `base` property, which is going away in future [1] All of the actual usage of `useElementShouldClose` in the app passed an Element ref, so I've simply removed the functionality and associated tests. [1] preactjs/preact#2971
diff --git a/src/sidebar/components/TagEditor.js b/src/sidebar/components/TagEditor.js
@@ -49,7 +49,6 @@ function TagEditor({
 
   // Set up callback to monitor outside click events to close the AutocompleteList
   const closeWrapperRef = useRef(/** @type {HTMLElement|null} */ (null));
-  /** @ts-ignore - TODO: fix useElementShouldClose Ref types */
   useElementShouldClose(closeWrapperRef, suggestionsListOpen, () => {
     setSuggestionsListOpen(false);
   });
diff --git a/src/sidebar/components/hooks/test/use-element-should-close-test.js b/src/sidebar/components/hooks/test/use-element-should-close-test.js
@@ -2,21 +2,27 @@ import { mount } from 'enzyme';
 import { createElement } from 'preact';
 import { useRef } from 'preact/hooks';
 import { act } from 'preact/test-utils';
-import propTypes from 'prop-types';
 
 import useElementShouldClose from '../use-element-should-close';
 
-describe('hooks.useElementShouldClose', () => {
+describe('useElementShouldClose', () => {
   let handleClose;
-  let e;
+
+  const createEvent = (name, props) => {
+    const event = new Event(name);
+    Object.assign(event, props);
+    return event;
+  };
+
   const events = [
     new Event('mousedown'),
     new Event('click'),
-    ((e = new Event('keydown')), (e.key = 'Escape'), e),
+    createEvent('keydown', { key: 'Escape' }),
     new Event('focus'),
   ];
 
   // Create a fake component to mount in tests that uses the hook
+  // eslint-disable-next-line react/prop-types
   function FakeComponent({ isOpen = true }) {
     const myRef = useRef();
     useElementShouldClose(myRef, isOpen, handleClose);
@@ -27,103 +33,62 @@ describe('hooks.useElementShouldClose', () => {
     );
   }
 
-  FakeComponent.propTypes = {
-    isOpen: propTypes.bool,
-  };
-
-  // Tests useElementShouldClose on a custom component directly
-  function FakeCompoundComponent({ isOpen = true }) {
-    function FakeCustomComponent() {
-      return (
-        <div>
-          <button>Hi</button>
-        </div>
-      );
-    }
-    const myRef = useRef();
-    useElementShouldClose(myRef, isOpen, handleClose);
-    return <FakeCustomComponent ref={myRef} />;
-  }
-
-  FakeCompoundComponent.propTypes = {
-    isOpen: propTypes.bool,
-  };
-
   function createComponent(props) {
     return mount(<FakeComponent isOpen={true} {...props} />);
   }
 
-  function createCompoundComponent(props) {
-    return mount(<FakeCompoundComponent isOpen={true} {...props} />);
-  }
-
   beforeEach(() => {
     handleClose = sinon.stub();
   });
 
-  // Run each set of tests twice, once for a regular node and a second
-  // time for a custom preact component
-  [
-    {
-      createWrapper: createComponent,
-      description: 'useElementShouldClose attached to a html node',
-    },
-    {
-      createWrapper: createCompoundComponent,
-      description: 'useElementShouldClose attached to a preact component',
-    },
-  ].forEach(test => {
-    context(test.description, () => {
-      events.forEach(event => {
-        it(`should invoke close callback once for events outside of element (${event.type})`, () => {
-          const wrapper = test.createWrapper();
-
-          act(() => {
-            document.body.dispatchEvent(event);
-          });
-          wrapper.update();
-
-          assert.calledOnce(handleClose);
-
-          // Update the component to change it and re-execute the hook
-          wrapper.setProps({ isOpen: false });
-
-          act(() => {
-            document.body.dispatchEvent(event);
-          });
-
-          // Cleanup of hook should have removed eventListeners, so the callback
-          // is not called again
-          assert.calledOnce(handleClose);
-        });
+  events.forEach(event => {
+    it(`should invoke close callback once for events outside of element (${event.type})`, () => {
+      const wrapper = createComponent();
+
+      act(() => {
+        document.body.dispatchEvent(event);
       });
+      wrapper.update();
 
-      events.forEach(event => {
-        it(`should not invoke close callback on events outside of element if element closed (${event.type})`, () => {
-          const wrapper = test.createWrapper({ isOpen: false });
+      assert.calledOnce(handleClose);
 
-          act(() => {
-            document.body.dispatchEvent(event);
-          });
-          wrapper.update();
+      // Update the component to change it and re-execute the hook
+      wrapper.setProps({ isOpen: false });
 
-          assert.equal(handleClose.callCount, 0);
-        });
+      act(() => {
+        document.body.dispatchEvent(event);
       });
 
-      events.forEach(event => {
-        it(`should not invoke close callback on events inside of element (${event.type})`, () => {
-          const wrapper = test.createWrapper();
-          const button = wrapper.find('button');
+      // Cleanup of hook should have removed eventListeners, so the callback
+      // is not called again
+      assert.calledOnce(handleClose);
+    });
+  });
 
-          act(() => {
-            button.getDOMNode().dispatchEvent(event);
-          });
-          wrapper.update();
+  events.forEach(event => {
+    it(`should not invoke close callback on events outside of element if element closed (${event.type})`, () => {
+      const wrapper = createComponent({ isOpen: false });
 
-          assert.equal(handleClose.callCount, 0);
-        });
+      act(() => {
+        document.body.dispatchEvent(event);
       });
+      wrapper.update();
+
+      assert.equal(handleClose.callCount, 0);
+    });
+  });
+
+  events.forEach(event => {
+    it(`should not invoke close callback on events inside of element (${event.type})`, () => {
+      const wrapper = createComponent();
+      const button = wrapper.find('button');
+
+      act(() => {
+        button.getDOMNode().dispatchEvent(event);
+      });
+      wrapper.update();
+
+      assert.equal(handleClose.callCount, 0);
     });
   });
 });
diff --git a/src/sidebar/components/hooks/use-element-should-close.js b/src/sidebar/components/hooks/use-element-should-close.js
@@ -8,15 +8,6 @@ import { listen } from '../../util/dom';
  * @typedef {import("preact/hooks").Ref<T>} Ref
  */
 
-/**
- * @typedef PreactElement
- * @prop {Node} base
- */
-
-/**
- * @typedef {Ref<HTMLElement> | Ref<PreactElement>} PreactRef
- */
-
 /**
  * This hook adds appropriate `eventListener`s to the document when a target
  * element (`closeableEl`) is open. Events such as `click` and `focus` on
@@ -25,13 +16,9 @@ import { listen } from '../../util/dom';
  * to indicate that `closeableEl` should be closed. This hook also performs
  * cleanup to remove `eventListener`s when appropriate.
  *
- * Limitation: This will not work when attached to a custom component that has
- * more than one element nested under a root <Fragment>
- *
- * @param {PreactRef} closeableEl - ref object:
- *                                Reference to a DOM element or preat component
- *                                that should be closed when DOM elements external
- *                                to it are interacted with or `Esc` is pressed
+ * @param {Ref<HTMLElement>} closeableEl -
+ *   Reference to a DOM element that should be closed when DOM elements external
+ *   to it are interacted with or `Esc` is pressed
  * @param {boolean} isOpen - Whether the element is currently open. This hook does
  *                        not attach event listeners/do anything if it's not.
  * @param {() => void} handleClose - A function that will do the actual closing
@@ -42,25 +29,6 @@ export default function useElementShouldClose(
   isOpen,
   handleClose
 ) {
-  /**
-   *  Helper to return the underlying node object whether
-   *  `closeableEl` is attached to an HTMLNode or Preact component.
-   *
-   *  @param {PreactRef} closeableEl
-   *  @returns {Node}
-   */
-
-  const getCurrentNode = closeableEl => {
-    // if base is present, assume its a preact component
-    const node = /** @type {PreactElement} */ (closeableEl.current).base
-      ? /** @type {PreactElement} */ (closeableEl.current).base
-      : closeableEl.current;
-    if (typeof node !== 'object') {
-      throw new Error('useElementShouldClose can not find a node reference');
-    }
-    return /** @type {Node} */ (node);
-  };
-
   useEffect(() => {
     if (!isOpen) {
       return () => {};
@@ -80,8 +48,7 @@ export default function useElementShouldClose(
       document.body,
       'focus',
       event => {
-        const current = getCurrentNode(closeableEl);
-        if (!current.contains(/** @type {Node} */ (event.target))) {
+        if (!closeableEl.current.contains(/** @type {Node} */ (event.target))) {
           handleClose();
         }
       },
@@ -94,8 +61,7 @@ export default function useElementShouldClose(
       document.body,
       ['mousedown', 'click'],
       event => {
-        const current = getCurrentNode(closeableEl);
-        if (!current.contains(/** @type {Node} */ (event.target))) {
+        if (!closeableEl.current.contains(/** @type {Node} */ (event.target))) {
           handleClose();
         }
       },
