Improve menu/menu item a11y

docs/getting-started/changelog.md

@@ -12,6 +12,10 @@ _During the beta period, these restrictions may be relaxed in the event of a mis
 - Fixed a bug where hoisted dropdowns would render in the wrong position when place inside an `sl-dialog`
 - Improved `sl-dropdown` accessibility by attaching `aria-haspopup` and `aria-expanded` to the slotted trigger
 - Removed `console.log` from modal utility
+- 🚨 BREAKING CHANGE: Refactored `sl-menu` and `sl-menu-item` to improve accessibility by using proper focus states
+  - Moved `tabindex` from `sl-menu` to `sl-menu-item`
+  - Removed the `active` prop from `sl-menu-item` because synthetic focus states are bad for accessibility
+  - Removed the `sl-activate` and `sl-deactivate` events from `sl-menu-item` (listen for `focus` and `blur` instead)
 
 ## 2.0.0-beta.21
 

src/components.d.ts

@@ -686,10 +686,6 @@ export namespace Components {
     interface SlMenuDivider {
     }
     interface SlMenuItem {
-        /**
-          * Draws the menu in an active (i.e. or hover/focus), state to indicate the current menu selection. This is used in lieu of standard :hover and :focus states to prevent concurrent interactions from different devices, such as focusing with the keyboard and hovering with the mouse.
-         */
-        "active": boolean;
         /**
           * Set to true to draw the item in a checked state.
          */
@@ -698,6 +694,14 @@ export namespace Components {
           * Set to true to draw the menu item in a disabled state.
          */
         "disabled": boolean;
+        /**
+          * Removes focus from the button.
+         */
+        "removeFocus": () => Promise<void>;
+        /**
+          * Sets focus on the button.
+         */
+        "setFocus": () => Promise<void>;
         /**
           * A unique value to store in the menu item.
          */
@@ -2195,10 +2199,6 @@ declare namespace LocalJSX {
     interface SlMenuDivider {
     }
     interface SlMenuItem {
-        /**
-          * Draws the menu in an active (i.e. or hover/focus), state to indicate the current menu selection. This is used in lieu of standard :hover and :focus states to prevent concurrent interactions from different devices, such as focusing with the keyboard and hovering with the mouse.
-         */
-        "active"?: boolean;
         /**
           * Set to true to draw the item in a checked state.
          */
@@ -2207,14 +2207,6 @@ declare namespace LocalJSX {
           * Set to true to draw the menu item in a disabled state.
          */
         "disabled"?: boolean;
-        /**
-          * Emitted when the menu item becomes active.
-         */
-        "onSl-activate"?: (event: CustomEvent<any>) => void;
-        /**
-          * Emitted when the menu item becomes inactive.
-         */
-        "onSl-deactivate"?: (event: CustomEvent<any>) => void;
         /**
           * A unique value to store in the menu item.
          */

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

@@ -21,12 +21,14 @@
   white-space: nowrap;
   cursor: pointer;
 
-  &.menu-item--active:not(.menu-item--disabled) {
+  &.menu-item--focused:not(.menu-item--disabled) {
+    outline: none;
     background-color: var(--sl-color-primary-95);
     color: var(--sl-color-primary-50);
   }
 
   &.menu-item--disabled {
+    outline: none;
     color: var(--sl-color-gray-70);
     cursor: not-allowed;
   }

src/components/menu-item/menu-item.tsx

@@ -1,4 +1,4 @@
-import { Component, Event, EventEmitter, Prop, Watch, h } from '@stencil/core';
+import { Component, Method, Prop, State, h } from '@stencil/core';
 
 /**
  * @since 2.0
@@ -21,46 +21,73 @@ import { Component, Event, EventEmitter, Prop, Watch, h } from '@stencil/core';
   shadow: true
 })
 export class MenuItem {
+  menuItem: HTMLElement;
+
+  @State() hasFocus = false;
+
   /** Set to true to draw the item in a checked state. */
   @Prop({ reflect: true }) checked = false;
 
-  /**
-   * Draws the menu in an active (i.e. or hover/focus), state to indicate the current menu selection. This is used in
-   * lieu of standard :hover and :focus states to prevent concurrent interactions from different devices, such as
-   * focusing with the keyboard and hovering with the mouse.
-   */
-  @Prop({ reflect: true }) active = false;
-
   /** A unique value to store in the menu item. */
   @Prop({ reflect: true }) value = '';
 
   /** Set to true to draw the menu item in a disabled state. */
   @Prop({ reflect: true }) disabled = false;
 
-  @Watch('active')
-  handleActiveChange() {
-    this.active ? this.slActivate.emit() : this.slDeactivate.emit();
+  connectedCallback() {
+    this.handleBlur = this.handleBlur.bind(this);
+    this.handleFocus = this.handleFocus.bind(this);
+    this.handleMouseOver = this.handleMouseOver.bind(this);
+    this.handleMouseOut = this.handleMouseOut.bind(this);
   }
 
-  /** Emitted when the menu item becomes active. */
-  @Event({ eventName: 'sl-activate' }) slActivate: EventEmitter;
+  /** Sets focus on the button. */
+  @Method()
+  async setFocus() {
+    this.menuItem.focus();
+  }
 
-  /** Emitted when the menu item becomes inactive. */
-  @Event({ eventName: 'sl-deactivate' }) slDeactivate: EventEmitter;
+  /** Removes focus from the button. */
+  @Method()
+  async removeFocus() {
+    this.menuItem.blur();
+  }
+
+  handleBlur() {
+    this.hasFocus = false;
+  }
+
+  handleFocus() {
+    this.hasFocus = true;
+  }
+
+  handleMouseOver() {
+    this.setFocus();
+  }
+
+  handleMouseOut() {
+    this.removeFocus();
+  }
 
   render() {
     return (
       <div
+        ref={el => (this.menuItem = el)}
         part="base"
         class={{
           'menu-item': true,
           'menu-item--checked': this.checked,
-          'menu-item--active': this.active,
-          'menu-item--disabled': this.disabled
+          'menu-item--disabled': this.disabled,
+          'menu-item--focused': this.hasFocus
         }}
         role="menuitem"
         aria-disabled={this.disabled}
         aria-selected={this.checked}
+        tabIndex={!this.disabled ? 0 : null}
+        onFocus={this.handleFocus}
+        onBlur={this.handleBlur}
+        onMouseOver={this.handleMouseOver}
+        onMouseOut={this.handleMouseOut}
       >
         <span part="checked-icon" class="menu-item__check">
           <sl-icon name="check2" />

src/components/menu/menu.tsx

@@ -1,4 +1,4 @@
-import { Component, Event, EventEmitter, Method, State, h } from '@stencil/core';
+import { Component, Element, Event, EventEmitter, Method, State, h } from '@stencil/core';
 import { getTextContent } from '../../utilities/slot';
 
 /**
@@ -20,6 +20,8 @@ export class Menu {
   typeToSelectString = '';
   typeToSelectTimeout: any;
 
+  @Element() host: HTMLSlMenuElement;
+
   @State() hasFocus = false;
 
   /** Emitted when the menu gains focus. */
@@ -32,12 +34,8 @@ export class Menu {
   @Event({ eventName: 'sl-select' }) slSelect: EventEmitter<{ item: HTMLSlMenuItemElement }>;
 
   connectedCallback() {
-    this.handleBlur = this.handleBlur.bind(this);
     this.handleClick = this.handleClick.bind(this);
-    this.handleFocus = this.handleFocus.bind(this);
     this.handleKeyDown = this.handleKeyDown.bind(this);
-    this.handleMouseOver = this.handleMouseOver.bind(this);
-    this.handleMouseOut = this.handleMouseOut.bind(this);
   }
 
   /** Sets focus on the menu. */
@@ -65,13 +63,12 @@ export class Menu {
     clearTimeout(this.typeToSelectTimeout);
     this.typeToSelectTimeout = setTimeout(() => (this.typeToSelectString = ''), 750);
     this.typeToSelectString += key.toLowerCase();
-
     const items = this.getItems();
     for (const item of items) {
       const slot = item.shadowRoot.querySelector('slot:not([name])') as HTMLSlotElement;
       const label = getTextContent(slot).toLowerCase().trim();
       if (label.substring(0, this.typeToSelectString.length) === this.typeToSelectString) {
-        items.map(i => (i.active = i === item));
+        item.setFocus();
         break;
       }
     }
@@ -85,27 +82,11 @@ export class Menu {
   }
 
   getActiveItem() {
-    return this.getItems().find(i => i.active);
+    return this.getItems().find(i => i === document.activeElement);
   }
 
-  setActiveItem(item?: HTMLSlMenuItemElement) {
-    this.getItems().map(i => (i.active = i === item));
-  }
-
-  handleFocus() {
-    this.slFocus.emit();
-
-    // Activate the first item if no other item is active
-    const activeItem = this.getActiveItem();
-    if (!activeItem) {
-      const items = this.getItems();
-      this.setActiveItem(items[0]);
-    }
-  }
-
-  handleBlur() {
-    this.setActiveItem();
-    this.slBlur.emit();
+  setActiveItem(item: HTMLSlMenuItemElement) {
+    item.setFocus();
   }
 
   handleClick(event: MouseEvent) {
@@ -164,17 +145,6 @@ export class Menu {
     this.typeToSelect(event.key);
   }
 
-  handleMouseOver(event: MouseEvent) {
-    const target = event.target as HTMLElement;
-    const item = target.closest('sl-menu-item');
-
-    this.setActiveItem(item);
-  }
-
-  handleMouseOut() {
-    this.setActiveItem(null);
-  }
-
   render() {
     return (
       <div
@@ -184,14 +154,9 @@ export class Menu {
           menu: true,
           'menu--has-focus': this.hasFocus
         }}
-        tabIndex={0}
         role="menu"
         onClick={this.handleClick}
-        onFocus={this.handleFocus}
-        onBlur={this.handleBlur}
         onKeyDown={this.handleKeyDown}
-        onMouseOver={this.handleMouseOver}
-        onMouseOut={this.handleMouseOut}
       >
         <slot />
       </div>