Merge pull request #1188 from hypothesis/menu-usability-fixes

Menu usability improvements

src/sidebar/components/menu.js

@@ -82,32 +82,72 @@ function Menu({
       return () => {};
     }
 
-    const removeListeners = listen(
+    // Close menu when user presses Escape key, regardless of focus.
+    const removeKeypressListener = listen(
       document.body,
-      ['keypress', 'click', 'mousedown'],
+      ['keypress'],
       event => {
-        if (event.type === 'keypress' && event.key !== 'Escape') {
-          return;
+        if (event.key === 'Escape') {
+          closeMenu();
         }
-        if (
-          event.type === 'mousedown' &&
-          menuRef.current &&
-          menuRef.current.contains(event.target)
-        ) {
-          // Close the menu as soon as the user _presses_ the mouse outside the
-          // menu, but only when they _release_ the mouse if they click inside
-          // the menu.
-          return;
-        }
-        closeMenu();
       }
     );
 
-    return removeListeners;
+    // 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]);
 
+  const stopPropagation = e => e.stopPropagation();
+
+  // Close menu if user presses a key which activates menu items.
+  const handleMenuKeyPress = event => {
+    if (event.key === 'Enter' || event.key === ' ') {
+      closeMenu();
+    }
+  };
+
   return (
-    <div className="menu" ref={menuRef}>
+    <div
+      className="menu"
+      ref={menuRef}
+      // Don't close the menu if the mouse is released over one of the menu
+      // elements outside the content area (eg. the arrow at the top of the
+      // content).
+      onClick={stopPropagation}
+      // Don't close the menu if the user presses the mouse down on menu elements
+      // except for the toggle button.
+      onMouseDown={stopPropagation}
+    >
       <button
         aria-expanded={isOpen ? 'true' : 'false'}
         aria-haspopup={true}
@@ -135,6 +175,8 @@ function Menu({
               contentClass
             )}
             role="menu"
+            onClick={closeMenu}
+            onKeyPress={handleMenuKeyPress}
           >
             {children}
           </div>

src/sidebar/components/test/menu-test.js

@@ -83,6 +83,7 @@ describe('Menu', () => {
     new Event('mousedown'),
     new Event('click'),
     ((e = new Event('keypress')), (e.key = 'Escape'), e),
+    new Event('focus'),
   ].forEach(event => {
     it(`closes when the user clicks or presses the mouse outside (${event.type})`, () => {
       const wrapper = createMenu({ defaultOpen: true });
@@ -122,6 +123,51 @@ describe('Menu', () => {
     assert.isTrue(isOpen(wrapper));
   });
 
+  [
+    {
+      eventType: 'click',
+      key: null,
+      shouldClose: true,
+    },
+    {
+      eventType: 'keypress',
+      key: 'Enter',
+      shouldClose: true,
+    },
+    {
+      eventType: 'keypress',
+      key: ' ',
+      shouldClose: true,
+    },
+    {
+      eventType: 'keypress',
+      key: 'a',
+      shouldClose: false,
+    },
+    {
+      eventType: 'focus',
+      key: null,
+      shouldClose: false,
+    },
+  ].forEach(({ eventType, key, shouldClose }) => {
+    it(`${
+      shouldClose ? 'closes' : "doesn't close"
+    } when user performs a "${eventType}" (key: "${key}") on menu content`, () => {
+      const wrapper = createMenu({ defaultOpen: true });
+      wrapper.find('.menu__content').simulate(eventType, { key });
+      assert.equal(isOpen(wrapper), !shouldClose);
+    });
+  });
+
+  it("doesn't close when user presses on a menu element outside the toggle button or content", () => {
+    const wrapper = createMenu({ defaultOpen: true });
+
+    // The event may be received either by the top `<div>` or the arrow element
+    // itself.
+    wrapper.find('.menu').simulate('mousedown');
+    wrapper.find('.menu__arrow').simulate('mousedown');
+  });
+
   it('aligns menu content depending on `align` prop', () => {
     const wrapper = createMenu({ defaultOpen: true });
     assert.isTrue(wrapper.exists('.menu__content--align-left'));

src/sidebar/util/dom.js

 'use strict';
 
 /**
- * Attach `handler` as an event listener for `events` on `element`.
+ * Attach listeners for one or multiple events to an element and return a
+ * function that removes the listeners.
  *
+ * @param {Element}
+ * @param {string[]} events
+ * @param {(event: Event) => any} listener
+ * @param {boolean} [options.useCapture]
  * @return {function} Function which removes the event listeners.
  */
-function listen(element, events, handler) {
+function listen(element, events, listener, { useCapture = false } = {}) {
   if (!Array.isArray(events)) {
     events = [events];
   }
-  events.forEach(event => element.addEventListener(event, handler));
+  events.forEach(event =>
+    element.addEventListener(event, listener, useCapture)
+  );
   return () => {
-    events.forEach(event => element.removeEventListener(event, handler));
+    events.forEach(event =>
+      element.removeEventListener(event, listener, useCapture)
+    );
   };
 }
 

src/sidebar/util/test/dom-test.js

+'use strict';
+
+const { listen } = require('../dom');
+
+describe('sidebar/util/dom', () => {
+  const createElement = () => ({
+    addEventListener: sinon.stub(),
+    removeEventListener: sinon.stub(),
+  });
+
+  describe('listen', () => {
+    [true, false].forEach(useCapture => {
+      it('adds listeners for specified events', () => {
+        const element = createElement();
+        const handler = sinon.stub();
+
+        listen(element, ['click', 'mousedown'], handler, { useCapture });
+
+        assert.calledWith(
+          element.addEventListener,
+          'click',
+          handler,
+          useCapture
+        );
+        assert.calledWith(
+          element.addEventListener,
+          'mousedown',
+          handler,
+          useCapture
+        );
+      });
+    });
+
+    [true, false].forEach(useCapture => {
+      it('removes listeners when returned function is invoked', () => {
+        const element = createElement();
+        const handler = sinon.stub();
+
+        const removeListeners = listen(
+          element,
+          ['click', 'mousedown'],
+          handler,
+          { useCapture }
+        );
+        removeListeners();
+
+        assert.calledWith(
+          element.removeEventListener,
+          'click',
+          handler,
+          useCapture
+        );
+        assert.calledWith(
+          element.removeEventListener,
+          'mousedown',
+          handler,
+          useCapture
+        );
+      });
+    });
+  });
+});

src/styles/sidebar/components/menu-item.scss

 $menu-item-padding: 10px;
 
 .menu-item {
+  @include outline-on-keyboard-focus;
+
   $item-padding: $menu-item-padding;
   $item-height: 40px;