Menu key navigation

- Allow keyboard navigation of menus
- Fix various menu a11y issues
- Fix one underlying issue where executing a link in the menu with keyboard (Enter or Space) never worked
- Add MenuKeyboardNavigation helper component

src/sidebar/components/group-list-item.js

@@ -50,6 +50,11 @@ function GroupListItem({
     }
   };
 
+  /**
+   * Opens or closes the submenu.
+   *
+   * @param {MouseEvent|KeyboardEvent} event
+   */
   const toggleSubmenu = event => {
     event.stopPropagation();
 

src/sidebar/components/menu-item.js

 import classnames from 'classnames';
 import { Fragment, createElement } from 'preact';
+import { useEffect, useRef } from 'preact/hooks';
 import propTypes from 'prop-types';
 
-import { onActivate } from '../util/on-activate';
+import { normalizeKeyName } from '../../shared/browser-compatibility-utils';
 
-import Slider from './slider';
 import SvgIcon from '../../shared/components/svg-icon';
+import MenuKeyboardNavigation from './menu-keyboard-navigation';
+import Slider from './slider';
 
 /**
  * An item in a dropdown menu.
@@ -21,6 +23,7 @@ import SvgIcon from '../../shared/components/svg-icon';
  *
  * For items that have submenus, the `MenuItem` will call the `renderSubmenu`
  * prop to render the content of the submenu, when the submenu is visible.
+ * Note that the `submenu` is not supported for link (`href`) items.
  */
 export default function MenuItem({
   href,
@@ -38,13 +41,13 @@ export default function MenuItem({
 }) {
   const iconClass = 'menu-item__icon';
   const iconIsUrl = icon && icon.indexOf('/') !== -1;
-  const labelClass = classnames('menu-item__label', {
-    'menu-item__label--submenu': isSubmenuItem,
-  });
 
   const hasLeftIcon = icon || isSubmenuItem;
   const hasRightIcon = icon && isSubmenuItem;
 
+  const menuItemRef = useRef(null);
+  let focusTimer = null;
+
   let renderedIcon = null;
   if (icon !== 'blank') {
     renderedIcon = iconIsUrl ? (
@@ -56,49 +59,111 @@ export default function MenuItem({
   const leftIcon = isSubmenuItem ? null : renderedIcon;
   const rightIcon = isSubmenuItem ? renderedIcon : null;
 
-  return (
-    <Fragment>
-      {/* FIXME-A11Y */}
-      {/* eslint-disable-next-line jsx-a11y/role-supports-aria-props */}
+  // menuItem can be either a link or a button
+  let menuItem;
+  const hasSubmenuVisible = typeof isSubmenuVisible === 'boolean';
+  const isRadioButtonType = typeof isSelected === 'boolean';
+
+  useEffect(() => {
+    return () => {
+      // unmount
+      clearTimeout(focusTimer);
+    };
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, []);
+
+  const onCloseSubmenu = event => {
+    if (onToggleSubmenu) {
+      onToggleSubmenu(event);
+    }
+    // The focus won't work without delaying rendering.
+    focusTimer = setTimeout(() => {
+      menuItemRef.current.focus();
+    });
+  };
+
+  const onKeyDown = event => {
+    switch (normalizeKeyName(event.key)) {
+      case 'ArrowRight':
+        if (onToggleSubmenu) {
+          event.stopPropagation();
+          event.preventDefault();
+          onToggleSubmenu(event);
+        }
+        break;
+      case 'Enter':
+      case ' ':
+        if (onClick) {
+          // Let event propagate so the menu closes
+          onClick(event);
+        }
+    }
+  };
+  if (href) {
+    // The menu item is a link
+    menuItem = (
+      <a
+        ref={menuItemRef}
+        className={classnames('menu-item', {
+          'is-submenu': isSubmenuItem,
+          'is-disabled': isDisabled,
+        })}
+        href={href}
+        target="_blank"
+        tabIndex="-1"
+        rel="noopener noreferrer"
+        role="menuitem"
+        onKeyDown={onKeyDown}
+      >
+        {hasLeftIcon && (
+          <div className="menu-item__icon-container">{leftIcon}</div>
+        )}
+        <span className="menu-item__label">{label}</span>
+        {hasRightIcon && (
+          <div className="menu-item__icon-container">{rightIcon}</div>
+        )}
+      </a>
+    );
+  } else {
+    // The menu item is a clickable button or radio button.
+    // In either case there may be an optional submenu.
+
+    menuItem = (
       <div
-        aria-checked={isSelected}
+        ref={menuItemRef}
         className={classnames('menu-item', {
-          'menu-item--submenu': isSubmenuItem,
+          'is-submenu': isSubmenuItem,
           'is-disabled': isDisabled,
           'is-expanded': isExpanded,
           'is-selected': isSelected,
         })}
-        role="menuitem"
-        {...(onClick && onActivate('menuitem', onClick))}
+        tabIndex="-1"
+        onKeyDown={onKeyDown}
+        onClick={onClick}
+        role={isRadioButtonType ? 'menuitemradio' : 'menuitem'}
+        aria-checked={isRadioButtonType ? isSelected : undefined}
+        aria-haspopup={hasSubmenuVisible}
+        aria-expanded={hasSubmenuVisible ? isSubmenuVisible : undefined}
       >
-        <div className="menu-item__action">
-          {hasLeftIcon && (
-            <div className="menu-item__icon-container">{leftIcon}</div>
-          )}
-          {href && (
-            <a
-              className={labelClass}
-              href={href}
-              target="_blank"
-              rel="noopener noreferrer"
-            >
-              {label}
-            </a>
-          )}
-          {!href && <span className={labelClass}>{label}</span>}
-          {hasRightIcon && (
-            <div className="menu-item__icon-container">{rightIcon}</div>
-          )}
-        </div>
-        {typeof isSubmenuVisible === 'boolean' && (
+        {hasLeftIcon && (
+          <div className="menu-item__icon-container">{leftIcon}</div>
+        )}
+        <span className="menu-item__label">{label}</span>
+        {hasRightIcon && (
+          <div className="menu-item__icon-container">{rightIcon}</div>
+        )}
+
+        {hasSubmenuVisible && (
           <div
+            // We should not have a <button> inside of the menu item itself
+            // but we have a non-standard mechanism with the toggle control
+            // requiring an onClick event nested inside a "menuitemradio|menuitem".
+            // Therefore, a static element with a role="none" is necessary here.
+            role="none"
+            icon={isSubmenuVisible ? 'collapse-menu' : 'expand-menu'}
             className="menu-item__toggle"
-            // We need to pass strings here rather than just the boolean attribute
-            // because otherwise the attribute will be omitted entirely when
-            // `isSubmenuVisible` is false.
-            aria-expanded={isSubmenuVisible ? 'true' : 'false'}
-            aria-label={`Show actions for ${label}`}
-            {...onActivate('button', onToggleSubmenu)}
+            onClick={onToggleSubmenu}
+            title={`Show actions for ${label}`}
           >
             <SvgIcon
               name={isSubmenuVisible ? 'collapse-menu' : 'expand-menu'}
@@ -107,9 +172,20 @@ export default function MenuItem({
           </div>
         )}
       </div>
-      {typeof isSubmenuVisible === 'boolean' && (
+    );
+  }
+  return (
+    <Fragment>
+      {menuItem}
+      {hasSubmenuVisible && (
         <Slider visible={isSubmenuVisible}>
-          <div className="menu-item__submenu">{submenu}</div>
+          <MenuKeyboardNavigation
+            closeMenu={onCloseSubmenu}
+            visible={isSubmenuVisible}
+            className="menu-item__submenu"
+          >
+            {submenu}
+          </MenuKeyboardNavigation>
         </Slider>
       )}
     </Fragment>
@@ -165,6 +241,7 @@ MenuItem.propTypes = {
   /**
    * If present, display a button to toggle the sub-menu associated with this
    * item and indicate the current state; `true` if the submenu is visible.
+   * Note. Omit this prop, or set it to null, if there is no `submenu`.
    */
   isSubmenuVisible: propTypes.bool,
 

src/sidebar/components/menu-keyboard-navigation.js

+import { createElement } from 'preact';
+import { useEffect, useRef } from 'preact/hooks';
+import propTypes from 'prop-types';
+
+import { normalizeKeyName } from '../../shared/browser-compatibility-utils';
+
+function isElementVisible(element) {
+  return element.offsetParent !== null;
+}
+
+/**
+ * Helper component used by Menu and MenuItem to facilitate keyboard navigation of a
+ * list of <MenuItem> components. This component should not be used directly.
+ *
+ * Note that `ArrowRight` shall be handled by the parent <MenuItem> directly and
+ * all other focus() related navigation is handled here.
+ */
+export default function MenuKeyboardNavigation({
+  className,
+  closeMenu,
+  children,
+  visible,
+}) {
+  const menuRef = useRef(null);
+
+  useEffect(() => {
+    let focusTimer = null;
+    if (visible) {
+      focusTimer = setTimeout(() => {
+        // The focus won't work without delaying rendering.
+        const firstItem = menuRef.current.querySelector('[role^="menuitem"]');
+        if (firstItem) {
+          firstItem.focus();
+        }
+      });
+    }
+    return () => {
+      // unmount
+      clearTimeout(focusTimer);
+    };
+  }, [visible]);
+
+  const onKeyDown = event => {
+    const menuItems = Array.from(
+      menuRef.current.querySelectorAll('[role^="menuitem"]')
+    ).filter(isElementVisible);
+
+    let focusedIndex = menuItems.findIndex(el =>
+      el.contains(document.activeElement)
+    );
+
+    let handled = false;
+
+    switch (normalizeKeyName(event.key)) {
+      case 'ArrowLeft':
+      case 'Escape':
+        if (closeMenu) {
+          closeMenu(event);
+          handled = true;
+        }
+        break;
+      case 'ArrowUp':
+        focusedIndex -= 1;
+        if (focusedIndex < 0) {
+          focusedIndex = menuItems.length - 1;
+        }
+        handled = true;
+        break;
+      case 'ArrowDown':
+        focusedIndex += 1;
+        if (focusedIndex === menuItems.length) {
+          focusedIndex = 0;
+        }
+        handled = true;
+        break;
+      case 'Home':
+        focusedIndex = 0;
+        handled = true;
+        break;
+      case 'End':
+        focusedIndex = menuItems.length - 1;
+        handled = true;
+        break;
+    }
+
+    if (handled && focusedIndex >= 0) {
+      event.stopPropagation();
+      event.preventDefault();
+      menuItems[focusedIndex].focus();
+    }
+  };
+
+  return (
+    // This element needs to have role="menu" to facilitate readers
+    // correctly enumerating discrete submenu items, but it also needs
+    // to facilitate keydown events for navigation. Disable the linter
+    // error so it can do both.
+    // eslint-disable-next-line jsx-a11y/interactive-supports-focus
+    <div role="menu" className={className} ref={menuRef} onKeyDown={onKeyDown}>
+      {children}
+    </div>
+  );
+}
+
+MenuKeyboardNavigation.propTypes = {
+  className: propTypes.string,
+
+  // Callback when the menu is closed via keyboard input
+  closeMenu: propTypes.func,
+
+  // When  true`, sets focus on the first item in the list
+  // which has a role="menuitem" attribute.
+  visible: propTypes.bool,
+
+  // Array of nodes which may contain <MenuItems> or any nodes
+  // that have role set to 'menuitem', 'menuitemradio', or 'menuitemcheckbox'
+  children: propTypes.any.isRequired,
+};

src/sidebar/components/menu.js

@@ -4,7 +4,10 @@ import { useCallback, useEffect, useRef, useState } from 'preact/hooks';
 import propTypes from 'prop-types';
 
 import useElementShouldClose from './hooks/use-element-should-close';
+import { normalizeKeyName } from '../../shared/browser-compatibility-utils';
+
 import SvgIcon from '../../shared/components/svg-icon';
+import MenuKeyboardNavigation from './menu-keyboard-navigation';
 
 // The triangular indicator below the menu toggle button that visually links it
 // to the menu content.
@@ -52,6 +55,7 @@ export default function Menu({
   title,
 }) {
   const [isOpen, setOpen] = useState(defaultOpen);
+  const [openedByKeyboard, setOpenedByKeyboard] = useState(false);
 
   // Notify parent when menu is opened or closed.
   const wasOpen = useRef(isOpen);
@@ -71,11 +75,23 @@ export default function Menu({
     if (event.type === 'mousedown') {
       ignoreNextClick = true;
     } else if (event.type === 'click' && ignoreNextClick) {
+      // Ignore "click" event triggered from the mouse up action.
       ignoreNextClick = false;
       event.stopPropagation();
       event.preventDefault();
       return;
     }
+    // State variable so we know to set focus() on the first item when opened
+    // via the keyboard. Note, when opening the menu via keyboard by pressing
+    // enter or space, a simulated MouseEvent is created with a type value of
+    // "click". We also know this is not a mouseup event because that condition
+    // is checked above.
+    if (!isOpen && event.type === 'click') {
+      setOpenedByKeyboard(true);
+    } else {
+      setOpenedByKeyboard(false);
+    }
+
     setOpen(!isOpen);
   };
   const closeMenu = useCallback(() => setOpen(false), [setOpen]);
@@ -95,8 +111,14 @@ export default function Menu({
 
   // It should also close if the user presses a key which activates menu items.
   const handleMenuKeyDown = event => {
-    if (event.key === 'Enter' || event.key === ' ') {
-      closeMenu();
+    const key = normalizeKeyName(event.key);
+    if (key === 'Enter' || key === ' ') {
+      // The browser will not open the link if the link element is removed
+      // from within the keypress event that triggers it. Add a little
+      // delay to work around that.
+      setTimeout(() => {
+        closeMenu();
+      });
     }
   };
 
@@ -156,7 +178,9 @@ export default function Menu({
             onClick={closeMenu}
             onKeyDown={handleMenuKeyDown}
           >
-            {children}
+            <MenuKeyboardNavigation visible={openedByKeyboard}>
+              {children}
+            </MenuKeyboardNavigation>
           </div>
         </Fragment>
       )}

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

+import { mount } from 'enzyme';
+import { createElement } from 'preact';
+
+import MenuKeyboardNavigation from '../menu-keyboard-navigation';
+import { $imports } from '../menu-keyboard-navigation';
+
+import { checkAccessibility } from '../../../test-util/accessibility';
+import mockImportedComponents from '../../../test-util/mock-imported-components';
+
+describe('MenuKeyboardNavigation', () => {
+  let fakeCloseMenu;
+  let clock;
+  let containers = [];
+
+  const createMenuItem = props => {
+    let newContainer = document.createElement('div');
+    containers.push(newContainer);
+    document.body.appendChild(newContainer);
+    return mount(
+      <MenuKeyboardNavigation
+        closeMenu={fakeCloseMenu}
+        className="test-nav"
+        {...props}
+      >
+        <button>Item 0</button>
+        <button role="menuitem">Item 1</button>
+        <button role="menuitem">Item 2</button>
+        <button role="menuitem">Item 3</button>
+      </MenuKeyboardNavigation>,
+      {
+        attachTo: newContainer,
+      }
+    );
+  };
+
+  beforeEach(() => {
+    fakeCloseMenu = sinon.stub();
+    $imports.$mock(mockImportedComponents());
+  });
+
+  afterEach(() => {
+    $imports.$restore();
+    containers.forEach(container => {
+      container.remove();
+    });
+    containers = [];
+  });
+
+  it('renders the provided class name', () => {
+    const wrapper = createMenuItem({ className: 'test' });
+    assert.isTrue(wrapper.find('.test').exists());
+  });
+
+  ['ArrowLeft', 'Escape'].forEach(key => {
+    it(`calls \`closeMenu\` callback when the '${key}' is pressed`, () => {
+      const wrapper = createMenuItem({ visible: true }).find('div.test-nav');
+      wrapper.simulate('keydown', { key });
+      assert.called(fakeCloseMenu);
+    });
+  });
+
+  // useFakeTimers does not work with checkAccessibility
+  // so wrap these tests in their own describe block
+  describe('keyboard navigation', () => {
+    beforeEach(() => {
+      clock = sinon.useFakeTimers();
+    });
+
+    afterEach(() => {
+      clock.restore();
+    });
+
+    it('sets focus to the first `menuitem` child when `visible` is true', () => {
+      const clock = sinon.useFakeTimers();
+      createMenuItem({ visible: true });
+      clock.tick(1);
+      assert.equal(document.activeElement.innerText, 'Item 1');
+    });
+
+    it('changes focus circularly when down arrow is pressed', () => {
+      const wrapper = createMenuItem({ visible: true }).find('div.test-nav');
+      clock.tick(1);
+      wrapper.simulate('keydown', { key: 'ArrowDown' });
+      assert.equal(document.activeElement.innerText, 'Item 2');
+      wrapper.simulate('keydown', { key: 'ArrowDown' });
+      assert.equal(document.activeElement.innerText, 'Item 3');
+      wrapper.simulate('keydown', { key: 'ArrowDown' });
+      assert.equal(document.activeElement.innerText, 'Item 1');
+    });
+
+    it('changes focus circularly when up arrow is pressed', () => {
+      const wrapper = createMenuItem({ visible: true }).find('div.test-nav');
+      clock.tick(1);
+      wrapper.simulate('keydown', { key: 'ArrowUp' });
+      assert.equal(document.activeElement.innerText, 'Item 3');
+      wrapper.simulate('keydown', { key: 'ArrowUp' });
+      assert.equal(document.activeElement.innerText, 'Item 2');
+      wrapper.simulate('keydown', { key: 'ArrowUp' });
+      assert.equal(document.activeElement.innerText, 'Item 1');
+    });
+
+    it('changes focus to the last item when up `End` is pressed', () => {
+      const wrapper = createMenuItem({ visible: true }).find('div.test-nav');
+      clock.tick(1);
+      wrapper.simulate('keydown', { key: 'End' });
+      assert.equal(document.activeElement.innerText, 'Item 3');
+    });
+
+    it('changes focus to the first item when up `Home` is pressed', () => {
+      const wrapper = createMenuItem({ visible: true }).find('div.test-nav');
+      clock.tick(1);
+      wrapper.simulate('keydown', { key: 'End' }); // move focus off first item
+      wrapper.simulate('keydown', { key: 'Home' });
+      assert.equal(document.activeElement.innerText, 'Item 1');
+    });
+
+    it('does not throw an error when unmounting the component before the focus timeout finishes', () => {
+      const wrapper = createMenuItem({ visible: true });
+      wrapper.unmount();
+      clock.tick(1);
+      // no assert needed
+    });
+  });
+
+  it(
+    'should pass a11y checks',
+    checkAccessibility({
+      // eslint-disable-next-line react/display-name
+      content: () => (
+        <div>
+          <MenuKeyboardNavigation>
+            <button role="menuitem">Item 1</button>
+          </MenuKeyboardNavigation>
+        </div>
+      ),
+    })
+  );
+});

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

@@ -10,6 +10,7 @@ import { checkAccessibility } from '../../../test-util/accessibility';
 
 describe('Menu', () => {
   let container;
+  let clock;
 
   const TestLabel = () => 'Test label';
   const TestMenuItem = () => 'Test item';
@@ -39,12 +40,17 @@ describe('Menu', () => {
   afterEach(() => {
     $imports.$restore();
     container.remove();
+    if (clock) {
+      clock.restore();
+      clock = null;
+    }
   });
 
   it('opens and closes when the toggle button is clicked', () => {
     const wrapper = createMenu();
     assert.isFalse(isOpen(wrapper));
     wrapper.find('button').simulate('click');
+    assert.isTrue(wrapper.find('MenuKeyboardNavigation').prop('visible'));
     assert.isTrue(isOpen(wrapper));
     wrapper.find('button').simulate('click');
     assert.isFalse(isOpen(wrapper));
@@ -54,6 +60,7 @@ describe('Menu', () => {
     const onOpenChanged = sinon.stub();
     const wrapper = createMenu({ onOpenChanged });
     wrapper.find('button').simulate('click');
+    assert.isTrue(wrapper.find('MenuKeyboardNavigation').prop('visible'));
     assert.calledWith(onOpenChanged, true);
     wrapper.find('button').simulate('click');
     assert.calledWith(onOpenChanged, false);
@@ -64,6 +71,7 @@ describe('Menu', () => {
     assert.isFalse(isOpen(wrapper));
 
     wrapper.find('button').simulate('mousedown');
+    assert.isFalse(wrapper.find('MenuKeyboardNavigation').prop('visible'));
     // Make sure the follow-up click doesn't close the menu.
     wrapper.find('button').simulate('click');
 
@@ -161,8 +169,15 @@ describe('Menu', () => {
     it(`${
       shouldClose ? 'closes' : "doesn't close"
     } when user performs a "${eventType}" (key: "${key}") on menu content`, () => {
+      clock = sinon.useFakeTimers();
       const wrapper = createMenu({ defaultOpen: true });
       wrapper.find('.menu__content').simulate(eventType, { key });
+      // The close event is delayed by a minimal amount of time in
+      // order to allow links to say in the DOM long enough to be
+      // followed on a click. Therefore, this test must simulate
+      // time passing in order for the menu to close.
+      clock.tick(1);
+      wrapper.update();
       assert.equal(isOpen(wrapper), !shouldClose);
     });
   });

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

@@ -3,96 +3,91 @@
 @use "../../variables" as var;
 
 $menu-item-padding: 10px;
+$item-height: 40px;
 
-.menu-item {
-  @include focus.outline-on-keyboard-focus;
+// The container for the clickable area of the menu item which triggers an
+// action or link. For menu items without submenus, this covers the full area of
+// the menu item. For menu items with submenus, this covers the full area
+// except for the toggle button that opens the submenu.
 
-  $item-height: 40px;
+a.menu-item:hover {
+  color: var.$link-color-hover;
+}
 
-  color: var.$grey-6;
+.menu-item {
+  @include focus.outline-on-keyboard-focus;
+  align-self: stretch;
   display: flex;
-  flex-direction: row;
   flex-grow: 1;
-  font-weight: 500;
+  appearance: none;
   align-items: center;
+  padding-left: $menu-item-padding;
+  color: var.$grey-6;
+  border: none;
+  padding-right: 0;
+  font-weight: 500;
+  min-height: 40px;
+  width: 100%;
   min-width: 150px;
-  min-height: $item-height;
-
-  // TODO - Make the link fill the full available vertical space.
-  cursor: pointer;
-
+  // Prevent height changes when .is-expanded border is rendered.
+  border-bottom: solid 1px transparent;
   // Prevent menu item text from being selected when user toggles menu.
   user-select: none;
 
-  // An item in a submenu associated with a top-level item.
-  &--submenu {
+  &:hover {
+    background-color: var.$grey-1;
+    .menu-item__toggle {
+      // Make it a bit darker when its a submenu item.
+      background-color: var.$grey-3;
+    }
+  }
+
+  &.is-submenu {
     min-height: $item-height - 10px;
     background: var.$grey-1;
     color: color.mix(var.$grey-5, var.$grey-6, $weight: 50%);
     font-weight: normal;
-  }
-
-  // Animate the background color transition of menu items with expanding submenus.
-  // The timing should match the expansion of the submenu.
-  transition: background 0.15s ease-in;
-
-  &.is-expanded {
-    // Set the background color of menu items with submenus to match the
-    // submenu items.
-    background: var.$grey-1;
-  }
-}
-
-// The container for the clickable area of the menu item which triggers its
-// main action. For menu items without submenus, this covers the full area of
-// the menu item. For menu items with submenus, this covers the full area
-// except for the toggle that opens the submenu.
-.menu-item__action {
-  align-self: stretch;
-  display: flex;
-  flex-grow: 1;
-  align-items: center;
-  padding-left: $menu-item-padding;
-
-  &:hover {
-    background: var.$grey-1;
-
-    .menu-item.is-expanded & {
-      // Since expanded items already have a light grey background, we need to
-      // make the hover state a little darker. This should match submenu items'
-      // hover state.
-      background: var.$grey-3;
-    }
-
-    // Since submenu items already have a light grey background, we need to
-    // use a slightly darker grey as the hover state.
-    .menu-item--submenu & {
-      background: var.$grey-3;
-      color: var.$grey-6;
+    &:hover {
+      background-color: var.$grey-3;
     }
   }
 
-  .menu-item.is-selected & {
+  &.is-selected {
     $border-width: 4px;
     border-left: $border-width solid var.$brand;
     padding-left: $menu-item-padding - $border-width;
   }
-}
 
-.menu-item__icon-container {
-  margin-right: 10px;
-  width: 15px;
-  height: 15px;
+  &.is-expanded {
+    background-color: var.$grey-1;
+    color: var.$grey-6;
+    border-bottom: solid 1px var.$grey-3;
+    &:hover {
+      background-color: var.$grey-3;
+    }
+    .menu-item__toggle {
+      // Make it a bit darker when its expanded. This is needed for
+      // a color darker than hover while expanded.
+      background-color: var.$grey-4;
+    }
+  }
+  &.is-disabled {
+    color: var.$grey-4;
+  }
 }
-
 .menu-item__icon {
   color: inherit;
   display: inline-block;
   margin-right: 4px;
   position: relative;
-
   height: 15px;
   width: 15px;
+
+  &-container {
+    margin-right: 10px;
+    width: 15px;
+    height: 15px;
+  }
 }
 
 .menu-item__label {
@@ -113,24 +108,20 @@ $menu-item-padding: 10px;
   &--submenu {
     font-weight: normal;
   }
-
-  .menu-item.is-disabled & {
-    color: var.$grey-4;
-  }
 }
 
 // Toggle button used to expand or collapse the submenu associated with a menu
 // item.
 .menu-item__toggle {
-  @include focus.outline-on-keyboard-focus;
-
-  color: var.$grey-4;
+  color: var.$grey-5;
   display: flex;
   flex-direction: column;
   justify-content: center;
   align-items: center;
   align-self: stretch;
   width: 40px;
+  padding: 0;
+  height: 40px;
 
   // Add a wide transparent border to provide a large-enough hit target (~40px),
   // larger than the visual size of the button (~20px).
@@ -143,8 +134,8 @@ $menu-item-padding: 10px;
   border-radius: 12px;
 
   &:hover {
-    background-color: var.$grey-3;
-    color: var.$grey-5;
+    background-color: var.$grey-4;
+    color: var.$grey-6;
   }
 
   &-icon {
@@ -155,5 +146,9 @@ $menu-item-padding: 10px;
 
 // The container for open submenus
 .menu-item__submenu {
-  border-bottom: solid 1px var.$grey-2;
+  border-bottom: solid 1px var.$grey-3;
+  &:hover {
+    // Make it a bit darker on hover.
+    background-color: var.$grey-3;
+  }
 }