use roving tab index for menu

docs/components/menu.md

@@ -4,7 +4,7 @@
 
 Menus provide a list of options for the user to choose from.
 
-You can use [menu items](/components/menu-item), [menu dividers](/components/menu-divider), and [menu labels](/components/menu-label) to compose a menu.
+You can use [menu items](/components/menu-item), [menu dividers](/components/menu-divider), and [menu labels](/components/menu-label) to compose a menu. Menus support keyboard interactions, including type-to-select an option.
 
 ```html preview
 <sl-menu style="max-width: 200px; border: solid 1px var(--sl-panel-border-color); border-radius: var(--sl-border-radius-medium);">

docs/resources/changelog.md

@@ -17,6 +17,8 @@ This is a lot more intuitive and makes it easier to activate animations imperati
 - 🚨 BREAKING: removed `closeOnSelect` prop from `sl-dropdown` (use `stayOpenOnSelect` instead)
 - Added `currentTime` to `sl-animation` to control the current time without methods
 - Fixed a bug in `sl-range` where the tooltip wasn't showing in Safari [#477](https://github.com/shoelace-style/shoelace/issues/477)
+- Fixed a bug in `sl-menu` where pressing <kbd>Enter</kbd> in a menu didn't work with click handlers
+- Reworked `sl-menu` and `sl-menu-item` to use a roving tab index and improve keyboard accessibility
 - Reworked tabbable logic to be more performant [#466](https://github.com/shoelace-style/shoelace/issues/466)
 
 ## 2.0.0-beta.45

src/components/dropdown/dropdown.ts

@@ -272,11 +272,14 @@ export default class SlDropdown extends LitElement {
 
       // Focus on a menu item
       if (event.key === 'ArrowDown' && firstMenuItem) {
+        const menu = this.getMenu();
+        menu.setCurrentItem(firstMenuItem);
         firstMenuItem.focus();
         return;
       }
 
       if (event.key === 'ArrowUp' && lastMenuItem) {
+        menu.setCurrentItem(lastMenuItem);
         lastMenuItem.focus();
         return;
       }

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

@@ -21,12 +21,6 @@
   white-space: nowrap;
   cursor: pointer;
 
-  &.menu-item--focused:not(.menu-item--disabled) {
-    outline: none;
-    background-color: var(--sl-color-primary-500);
-    color: var(--sl-color-white);
-  }
-
   &.menu-item--disabled {
     outline: none;
     color: var(--sl-color-gray-400);
@@ -58,6 +52,17 @@
   }
 }
 
+:host(:focus) {
+  outline: none;
+}
+
+:host(:hover:not([aria-disabled='true'])) .menu-item,
+:host(:focus:not([aria-disabled='true'])) .menu-item {
+  outline: none;
+  background-color: var(--sl-color-primary-500);
+  color: var(--sl-color-white);
+}
+
 .menu-item .menu-item__check {
   display: flex;
   position: absolute;

src/components/menu-item/menu-item.ts

@@ -1,7 +1,7 @@
 import { LitElement, html, unsafeCSS } from 'lit';
-import { customElement, property, query, state } from 'lit/decorators.js';
+import { customElement, property, query } from 'lit/decorators.js';
 import { classMap } from 'lit-html/directives/class-map';
-import { ifDefined } from 'lit-html/directives/if-defined';
+import { watch } from '../../internal/watch';
 import styles from 'sass:./menu-item.scss';
 
 /**
@@ -26,8 +26,6 @@ export default class SlMenuItem extends LitElement {
 
   @query('.menu-item') menuItem: HTMLElement;
 
-  @state() private hasFocus = false;
-
   /** Draws the item in a checked state. */
   @property({ type: Boolean, reflect: true }) checked = false;
 
@@ -37,30 +35,18 @@ export default class SlMenuItem extends LitElement {
   /** Draws the menu item in a disabled state. */
   @property({ type: Boolean, reflect: true }) disabled = false;
 
-  /** Sets focus on the button. */
-  focus(options?: FocusOptions) {
-    this.menuItem.focus(options);
+  firstUpdated() {
+    this.setAttribute('role', 'menuitem');
   }
 
-  /** Removes focus from the button. */
-  blur() {
-    this.menuItem.blur();
+  @watch('checked')
+  handleCheckedChange() {
+    this.setAttribute('aria-checked', String(this.checked));
   }
 
-  handleBlur() {
-    this.hasFocus = false;
-  }
-
-  handleFocus() {
-    this.hasFocus = true;
-  }
-
-  handleMouseEnter() {
-    this.focus();
-  }
-
-  handleMouseLeave() {
-    this.blur();
+  @watch('disabled')
+  handleDisabledChange() {
+    this.setAttribute('aria-disabled', String(this.disabled));
   }
 
   render() {
@@ -70,17 +56,8 @@ export default class SlMenuItem extends LitElement {
         class=${classMap({
           'menu-item': true,
           'menu-item--checked': this.checked,
-          'menu-item--disabled': this.disabled,
-          'menu-item--focused': this.hasFocus
+          'menu-item--disabled': this.disabled
         })}
-        role="menuitem"
-        aria-disabled=${this.disabled ? 'true' : 'false'}
-        aria-checked=${this.checked ? 'true' : 'false'}
-        tabindex=${ifDefined(!this.disabled ? '0' : undefined)}
-        @focus=${this.handleFocus}
-        @blur=${this.handleBlur}
-        @mouseenter=${this.handleMouseEnter}
-        @mouseleave=${this.handleMouseLeave}
       >
         <span part="checked-icon" class="menu-item__check">
           <sl-icon name="check" library="system" aria-hidden="true"></sl-icon>

src/components/menu/menu.ts

@@ -22,10 +22,43 @@ export default class SlMenu extends LitElement {
   @query('.menu') menu: HTMLElement;
   @query('slot') defaultSlot: HTMLSlotElement;
 
-  private items: SlMenuItem[] = [];
   private typeToSelectString = '';
   private typeToSelectTimeout: any;
 
+  getAllItems(options: { includeDisabled: boolean } = { includeDisabled: true }) {
+    return [...this.defaultSlot.assignedElements({ flatten: true })].filter((el: HTMLElement) => {
+      if (el.getAttribute('role') !== 'menuitem') {
+        return false;
+      }
+
+      if (!options?.includeDisabled && (el as SlMenuItem).disabled) {
+        return false;
+      }
+
+      return true;
+    }) as SlMenuItem[];
+  }
+
+  /**
+   * @internal Gets the current menu item, which is the menu item that has `tabindex="0"` within the roving tab index.
+   * The menu item may or may not have focus, but for keyboard interaction purposes it's considered the "active" item.
+   */
+  getCurrentItem() {
+    return this.getAllItems({ includeDisabled: false }).find(i => i.getAttribute('tabindex') === '0');
+  }
+
+  /**
+   * @internal Sets the current menu item to the specified element. This sets `tabindex="0"` on the target element and
+   * `tabindex="-1"` to all other items. This method must be called prior to setting focus on a menu item.
+   */
+  setCurrentItem(item: SlMenuItem) {
+    const items = this.getAllItems({ includeDisabled: false });
+    let activeItem = item.disabled ? items[0] : item;
+
+    // Update tab indexes
+    items.map(i => i.setAttribute('tabindex', i === activeItem ? '0' : '-1'));
+  }
+
   /**
    * Initiates type-to-select logic, which automatically selects an option based on what the user is currently typing.
    * The key passed will be appended to the internal query and the selection will be updated. After a brief period, the
@@ -33,10 +66,11 @@ export default class SlMenu extends LitElement {
    * enabling type-to-select when the menu doesn't have focus.
    */
   typeToSelect(key: string) {
+    const items = this.getAllItems({ includeDisabled: false });
     clearTimeout(this.typeToSelectTimeout);
     this.typeToSelectTimeout = setTimeout(() => (this.typeToSelectString = ''), 750);
     this.typeToSelectString += key.toLowerCase();
-    for (const item of this.items) {
+    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) {
@@ -46,20 +80,6 @@ export default class SlMenu extends LitElement {
     }
   }
 
-  syncItems() {
-    this.items = [...this.defaultSlot.assignedElements({ flatten: true })].filter(
-      (el: any) => el.tagName.toLowerCase() === 'sl-menu-item' && !el.disabled
-    ) as [SlMenuItem];
-  }
-
-  getActiveItem() {
-    return this.items.filter(i => i.shadowRoot!.querySelector('.menu-item--focused'))[0];
-  }
-
-  setActiveItem(item: SlMenuItem) {
-    item.focus();
-  }
-
   handleClick(event: MouseEvent) {
     const target = event.target as HTMLElement;
     const item = target.closest('sl-menu-item') as SlMenuItem;
@@ -72,11 +92,12 @@ export default class SlMenu extends LitElement {
   handleKeyDown(event: KeyboardEvent) {
     // Make a selection when pressing enter
     if (event.key === 'Enter') {
-      const item = this.getActiveItem();
+      const item = this.getCurrentItem();
       event.preventDefault();
 
       if (item) {
-        emit(this, 'sl-select', { detail: { item } });
+        // Simulate a click to support @click handlers on menu items that also work with the keyboard
+        item.click();
       }
     }
 
@@ -87,10 +108,11 @@ export default class SlMenu extends LitElement {
 
     // Move the selection when pressing down or up
     if (['ArrowDown', 'ArrowUp', 'Home', 'End'].includes(event.key)) {
-      const selectedItem = this.getActiveItem();
-      let index = selectedItem ? this.items.indexOf(selectedItem) : 0;
+      const items = this.getAllItems({ includeDisabled: false });
+      const activeItem = this.getCurrentItem();
+      let index = activeItem ? items.indexOf(activeItem) : 0;
 
-      if (this.items.length) {
+      if (items.length) {
         event.preventDefault();
 
         if (event.key === 'ArrowDown') {
@@ -100,13 +122,14 @@ export default class SlMenu extends LitElement {
         } else if (event.key === 'Home') {
           index = 0;
         } else if (event.key === 'End') {
-          index = this.items.length - 1;
+          index = items.length - 1;
         }
 
         if (index < 0) index = 0;
-        if (index > this.items.length - 1) index = this.items.length - 1;
+        if (index > items.length - 1) index = items.length - 1;
 
-        this.setActiveItem(this.items[index]);
+        this.setCurrentItem(items[index]);
+        items[index].focus();
 
         return;
       }
@@ -115,13 +138,34 @@ export default class SlMenu extends LitElement {
     this.typeToSelect(event.key);
   }
 
+  handleMouseDown(event: MouseEvent) {
+    const target = event.target as HTMLElement;
+
+    if (target.getAttribute('role') === 'menuitem') {
+      this.setCurrentItem(target as SlMenuItem);
+      target.focus();
+    }
+  }
+
   handleSlotChange() {
-    this.syncItems();
+    const items = this.getAllItems({ includeDisabled: false });
+
+    // Reset the roving tab index when the slotted items change
+    if (items.length) {
+      this.setCurrentItem(items[0]);
+    }
   }
 
   render() {
     return html`
-      <div part="base" class="menu" role="menu" @click=${this.handleClick} @keydown=${this.handleKeyDown} tabindex="0">
+      <div
+        part="base"
+        class="menu"
+        role="menu"
+        @click=${this.handleClick}
+        @keydown=${this.handleKeyDown}
+        @mousedown=${this.handleMouseDown}
+      >
         <slot @slotchange=${this.handleSlotChange}></slot>
       </div>
     `;

src/components/select/select.ts

@@ -32,7 +32,7 @@ let id = 0;
  * @event sl-clear - Emitted when the clear button is activated.
  * @event sl-change - Emitted when the control's value changes.
  * @event sl-focus - Emitted when the control gains focus.
- * @event l-blur - Emitted when the control loses focus.
+ * @event sl-blur - Emitted when the control loses focus.
  *
  * @csspart base - The component's base wrapper.
  * @csspart clear-button - The input's clear button, exported from <sl-input>.
@@ -231,11 +231,13 @@ export default class SlSelect extends LitElement {
 
       // Focus on a menu item
       if (event.key === 'ArrowDown' && firstItem) {
+        this.menu.setCurrentItem(firstItem);
         firstItem.focus();
         return;
       }
 
       if (event.key === 'ArrowUp' && lastItem) {
+        this.menu.setCurrentItem(lastItem);
         lastItem.focus();
         return;
       }