diff --git a/docs/components/menu.md b/docs/components/menu.md index af7f8fbd6..e0de3199b 100644 --- a/docs/components/menu.md +++ b/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 diff --git a/docs/resources/changelog.md b/docs/resources/changelog.md index 507f13e6b..414624fe4 100644 --- a/docs/resources/changelog.md +++ b/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 Enter 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 diff --git a/src/components/dropdown/dropdown.ts b/src/components/dropdown/dropdown.ts index ce6497833..8d7870e84 100644 --- a/src/components/dropdown/dropdown.ts +++ b/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; } diff --git a/src/components/menu-item/menu-item.scss b/src/components/menu-item/menu-item.scss index 1ad3c6b60..05caa7942 100644 --- a/src/components/menu-item/menu-item.scss +++ b/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; diff --git a/src/components/menu-item/menu-item.ts b/src/components/menu-item/menu-item.ts index ca38f3089..f6cf7b7f9 100644 --- a/src/components/menu-item/menu-item.ts +++ b/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} > diff --git a/src/components/menu/menu.ts b/src/components/menu/menu.ts index 8352e7396..ebe687d65 100644 --- a/src/components/menu/menu.ts +++ b/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` -