Merge pull request #1494 from hypothesis/use-element-should-close-hook

Add hook to tell an element to close if interactions happen outside of it

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

+'use strict';
+
+const { createElement } = require('preact');
+const { useRef } = require('preact/hooks');
+const propTypes = require('prop-types');
+
+const { act } = require('preact/test-utils');
+const { mount } = require('enzyme');
+
+const useElementShouldClose = require('../use-element-should-close');
+
+describe('hooks.useElementShouldClose', () => {
+  let handleClose;
+  let e;
+  const events = [
+    new Event('mousedown'),
+    new Event('click'),
+    ((e = new Event('keypress')), (e.key = 'Escape'), e),
+    new Event('focus'),
+  ];
+
+  // Create a fake component to mount in tests that uses the hook
+  function FakeComponent({ isOpen = true }) {
+    const myRef = useRef();
+    useElementShouldClose(myRef, isOpen, handleClose);
+    return (
+      <div ref={myRef}>
+        <button>Hi</button>
+      </div>
+    );
+  }
+
+  FakeComponent.propTypes = {
+    isOpen: propTypes.bool,
+  };
+
+  function createComponent(props) {
+    return mount(<FakeComponent 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);
+      });
+      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 not invoke close callback on events outside of element if element closed (${event.type})`, () => {
+      const wrapper = createComponent({ isOpen: false });
+
+      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);
+    });
+  });
+});

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

+'use strict';
+
+const { useEffect } = require('preact/hooks');
+
+const { listen } = require('../../util/dom');
+
+/**
+ * This hook adds appropriate `eventListener`s to the document when a target
+ * element (`closeableEl`) is open. Events such as `click` and `focus` on
+ * elements that fall outside of `closeableEl` in the document, or keypress
+ * events for the `esc` key, will invoke the provided `handleClose` function
+ * 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
+ * @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
+ *                                   of `closeableEl`
+ */
+function useElementShouldClose(closeableEl, isOpen, handleClose) {
+  useEffect(() => {
+    if (!isOpen) {
+      return () => {};
+    }
+
+    // Close element when user presses Escape key, regardless of focus.
+    const removeKeypressListener = listen(
+      document.body,
+      ['keypress'],
+      event => {
+        if (event.key === 'Escape') {
+          handleClose();
+        }
+      }
+    );
+
+    // Close element if user focuses an element outside of it via any means
+    // (key press, programmatic focus change).
+    const removeFocusListener = listen(
+      document.body,
+      'focus',
+      event => {
+        if (!closeableEl.current.contains(event.target)) {
+          handleClose();
+        }
+      },
+      { useCapture: true }
+    );
+
+    // Close element if user clicks outside of it, even if on an element which
+    // does not accept focus.
+    const removeClickListener = listen(
+      document.body,
+      ['mousedown', 'click'],
+      event => {
+        if (!closeableEl.current.contains(event.target)) {
+          handleClose();
+        }
+      },
+      { useCapture: true }
+    );
+
+    return () => {
+      removeKeypressListener();
+      removeClickListener();
+      removeFocusListener();
+    };
+  }, [closeableEl, isOpen, handleClose]);
+}
+
+module.exports = useElementShouldClose;

src/sidebar/components/menu.js

@@ -5,7 +5,7 @@ const { Fragment, createElement } = require('preact');
 const { useCallback, useEffect, useRef, useState } = require('preact/hooks');
 const propTypes = require('prop-types');
 
-const { listen } = require('../util/dom');
+const useElementShouldClose = require('./hooks/use-element-should-close');
 
 const SvgIcon = require('./svg-icon');
 
@@ -89,59 +89,14 @@ function Menu({
   // These handlers close the menu when the user taps or clicks outside the
   // menu or presses Escape.
   const menuRef = useRef();
-  useEffect(() => {
-    if (!isOpen) {
-      return () => {};
-    }
-
-    // Close menu when user presses Escape key, regardless of focus.
-    const removeKeypressListener = listen(
-      document.body,
-      ['keypress'],
-      event => {
-        if (event.key === 'Escape') {
-          closeMenu();
-        }
-      }
-    );
-
-    // Close menu if user focuses an element outside the menu via any means
-    // (key press, programmatic focus change).
-    const removeFocusListener = listen(
-      document.body,
-      'focus',
-      event => {
-        if (!menuRef.current.contains(event.target)) {
-          closeMenu();
-        }
-      },
-      { useCapture: true }
-    );
-
-    // Close menu if user clicks outside menu, even if on an element which
-    // does not accept focus.
-    const removeClickListener = listen(
-      document.body,
-      ['mousedown', 'click'],
-      event => {
-        // nb. Mouse events inside the current menu are handled elsewhere.
-        if (!menuRef.current.contains(event.target)) {
-          closeMenu();
-        }
-      },
-      { useCapture: true }
-    );
 
-    return () => {
-      removeKeypressListener();
-      removeClickListener();
-      removeFocusListener();
-    };
-  }, [closeMenu, isOpen]);
+  // Menu element should close via `closeMenu` whenever it's open and there
+  // are user interactions outside of it (e.g. clicks) in the document
+  useElementShouldClose(menuRef, isOpen, closeMenu);
 
   const stopPropagation = e => e.stopPropagation();
 
-  // Close menu if user presses a key which activates menu items.
+  // It should also close if the user presses a key which activates menu items.
   const handleMenuKeyPress = event => {
     if (event.key === 'Enter' || event.key === ' ') {
       closeMenu();