Fix useElementShouldClose so it may work on preact components (#1683)

Fix useElementShouldClose so it may work on preact components

src/sidebar/components/hooks/test/use-element-should-close-test.js

@@ -31,62 +31,99 @@ describe('hooks.useElementShouldClose', () => {
     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();
   });
 
-  events.forEach(event => {
-    it(`should invoke close callback once for events outside of element (${event.type})`, () => {
-      const wrapper = createComponent();
-
-      act(() => {
-        document.body.dispatchEvent(event);
+  // 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);
+        });
       });
-      wrapper.update();
 
-      assert.calledOnce(handleClose);
+      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 });
 
-      // Update the component to change it and re-execute the hook
-      wrapper.setProps({ isOpen: false });
+          act(() => {
+            document.body.dispatchEvent(event);
+          });
+          wrapper.update();
 
-      act(() => {
-        document.body.dispatchEvent(event);
+          assert.equal(handleClose.callCount, 0);
+        });
       });
 
-      // Cleanup of hook should have removed eventListeners, so the callback
-      // is not called again
-      assert.calledOnce(handleClose);
-    });
-  });
+      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');
 
-  events.forEach(event => {
-    it(`should not invoke close callback on events outside of element if element closed (${event.type})`, () => {
-      const wrapper = createComponent({ isOpen: false });
+          act(() => {
+            button.getDOMNode().dispatchEvent(event);
+          });
+          wrapper.update();
 
-      act(() => {
-        document.body.dispatchEvent(event);
+          assert.equal(handleClose.callCount, 0);
+        });
       });
-      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);
     });
   });
 });

src/sidebar/components/hooks/use-element-should-close.js

@@ -2,6 +2,20 @@ import { useEffect } from 'preact/hooks';
 
 import { listen } from '../../util/dom';
 
+/**
+ * @typedef Ref
+ * @prop current {Node} - HTML node
+ *
+ * A ref object attached to a HTML node.
+ */
+
+/**
+ * @typedef PreactRef
+ * @prop current {Object} - preact component object
+ *
+ * A ref object attached to a custom preact component.
+ */
+
 /**
  * This hook adds appropriate `eventListener`s to the document when a target
  * element (`closeableEl`) is open. Events such as `click` and `focus` on
@@ -10,10 +24,13 @@ import { listen } from '../../util/dom';
  * to indicate that `closeableEl` should be closed. This hook also performs
  * cleanup to remove `eventListener`s when appropriate.
  *
- * @param {Object} closeableEl - Preact ref object:
- *                                Reference to a DOM element that should be
- *                                closed when DOM elements external to it are
- *                                interacted with or `Esc` is pressed
+ * Limitation: This will not work when attached to a custom component that has
+ * more than one element nested under a root <Fragment>
+ *
+ * @param {Ref|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 {bool} 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
@@ -24,6 +41,25 @@ export default function useElementShouldClose(
   isOpen,
   handleClose
 ) {
+  /**
+   *  Helper to return the underlying node object whether
+   *  `closeableEl` is attached to an HTMLNode or Preact component.
+   *
+   *  @param {Preact ref} closeableEl
+   *  @returns {Node}
+   */
+
+  const getCurrentNode = closeableEl => {
+    // if base is present, assume its a preact component
+    const node = closeableEl.current.base
+      ? closeableEl.current.base
+      : closeableEl.current;
+    if (typeof node !== 'object') {
+      throw new Error('useElementShouldClose can not find a node reference');
+    }
+    return node;
+  };
+
   useEffect(() => {
     if (!isOpen) {
       return () => {};
@@ -46,7 +82,8 @@ export default function useElementShouldClose(
       document.body,
       'focus',
       event => {
-        if (!closeableEl.current.contains(event.target)) {
+        const current = getCurrentNode(closeableEl);
+        if (!current.contains(event.target)) {
           handleClose();
         }
       },
@@ -59,7 +96,8 @@ export default function useElementShouldClose(
       document.body,
       ['mousedown', 'click'],
       event => {
-        if (!closeableEl.current.contains(event.target)) {
+        const current = getCurrentNode(closeableEl);
+        if (!current.contains(event.target)) {
           handleClose();
         }
       },