From 91b72f7b88c96fb49c50e915f398a388ebc5792d Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Thu, 16 Jan 2025 16:06:34 -0500 Subject: [PATCH 01/25] fix(menu): added rovingTabindexController and removed aria-activedescendant --- packages/menu/src/Menu.ts | 39 +++++++++++++++++++++++------------ packages/menu/src/MenuItem.ts | 16 ++++---------- 2 files changed, 30 insertions(+), 25 deletions(-) diff --git a/packages/menu/src/Menu.ts b/packages/menu/src/Menu.ts index 5a64c94aa0..d4dedcf06a 100644 --- a/packages/menu/src/Menu.ts +++ b/packages/menu/src/Menu.ts @@ -27,6 +27,7 @@ import { MenuItem } from './MenuItem.js'; import type { MenuItemAddedOrUpdatedEvent } from './MenuItem.js'; import type { Overlay } from '@spectrum-web-components/overlay'; import menuStyles from './menu.css.js'; +import { RovingTabindexController } from '@spectrum-web-components/reactive-controllers/src/RovingTabindex.js'; export interface MenuChildItem { menuItem: MenuItem; @@ -68,6 +69,25 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { return this.slot === 'submenu'; } + rovingTabindexController = new RovingTabindexController(this, { + focusInIndex: (elements: MenuItem[] | undefined) => { + let firstEnabledIndex = -1; + const firstSelectedIndex = elements?.findIndex((el, index) => { + if (!elements[firstEnabledIndex] && !el.disabled) { + firstEnabledIndex = index; + } + return el.selected && !el.disabled; + }); + return elements && + firstSelectedIndex && + elements[firstSelectedIndex] + ? firstSelectedIndex + : firstEnabledIndex; + }, + elements: () => this.cachedChildItems || [], + isFocusableElement: (el: MenuItem) => !el.disabled, + }); + @property({ type: String, reflect: true }) public label = ''; @@ -166,6 +186,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { ); } + this.rovingTabindexController.clearElementCache(); return this.cachedChildItems; } @@ -335,7 +356,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { return; } this.focusMenuItemByOffset(0); - super.focus({ preventScroll }); + this.rovingTabindexController.focus({ preventScroll }); const selectedItem = this.selectedItems[0]; if (selectedItem && !preventScroll) { selectedItem.scrollIntoView({ block: 'nearest' }); @@ -406,6 +427,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { } public handleFocusin(event: FocusEvent): void { + // ignore if a child element has a different root menu if ( this.childItems.some( (childItem) => childItem.menuData.focusRoot !== this @@ -416,9 +438,12 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { const activeElement = (this.getRootNode() as Document).activeElement as | MenuItem | Menu; + + // selected child items root menu const selectionRoot = this.childItems[this.focusedItemIndex]?.menuData.selectionRoot || this; + if (activeElement !== selectionRoot || event.target !== this) { selectionRoot.focus({ preventScroll: true }); if (activeElement && this.focusedItemIndex === 0) { @@ -428,24 +453,13 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { this.focusMenuItemByOffset(Math.max(offset, 0)); } } - this.startListeningToKeyboard(); - } - - public startListeningToKeyboard(): void { - this.addEventListener('keydown', this.handleKeydown); } public handleBlur(event: FocusEvent): void { if (elementIsOrContains(this, event.relatedTarget as Node)) { return; } - this.stopListeningToKeyboard(); this.childItems.forEach((child) => (child.focused = false)); - this.removeAttribute('aria-activedescendant'); - } - - public stopListeningToKeyboard(): void { - this.removeEventListener('keydown', this.handleKeydown); } private descendentOverlays = new Map(); @@ -822,7 +836,6 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { return child.hasVisibleFocusInTree(); }); item.focused = focused; - this.setAttribute('aria-activedescendant', item.id); if ( item.menuData.selectionRoot && item.menuData.selectionRoot !== this diff --git a/packages/menu/src/MenuItem.ts b/packages/menu/src/MenuItem.ts index 75c955c946..66b39e92dd 100644 --- a/packages/menu/src/MenuItem.ts +++ b/packages/menu/src/MenuItem.ts @@ -237,18 +237,6 @@ export class MenuItem extends LikeAnchor( @property({ type: Boolean, reflect: true }) public open = false; - public override click(): void { - if (this.disabled) { - return; - } - - if (this.shouldProxyClick()) { - return; - } - - super.click(); - } - private handleClickCapture(event: Event): void | boolean { if (this.disabled) { event.preventDefault(); @@ -256,6 +244,10 @@ export class MenuItem extends LikeAnchor( event.stopPropagation(); return false; } + + if (this.shouldProxyClick()) { + return; + } } private handleSlottableRequest = (event: SlottableRequestEvent): void => { From adc0d2693a3d5c75e3146570125c150e79a179e7 Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Thu, 16 Jan 2025 16:08:35 -0500 Subject: [PATCH 02/25] fix(menu): fixes firefox keyboard navigation and call stack issues --- tools/reactive-controllers/src/FocusGroup.ts | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/tools/reactive-controllers/src/FocusGroup.ts b/tools/reactive-controllers/src/FocusGroup.ts index ec2d726cbe..92ba71e821 100644 --- a/tools/reactive-controllers/src/FocusGroup.ts +++ b/tools/reactive-controllers/src/FocusGroup.ts @@ -189,7 +189,9 @@ export class FocusGroupController focusElement = elements[this.currentIndex]; } if (focusElement && this.isFocusableElement(focusElement)) { - elements[this.prevIndex]?.setAttribute('tabindex', '-1'); + if (elements[this.prevIndex] !== focusElement) { + elements[this.prevIndex]?.setAttribute('tabindex', '-1'); + } focusElement.tabIndex = 0; focusElement.focus(options); } @@ -298,28 +300,28 @@ export class FocusGroupController } }; - acceptsEventCode(code: string): boolean { - if (code === 'End' || code === 'Home') { + acceptsEventKey(key: string): boolean { + if (key === 'End' || key === 'Home') { return true; } switch (this.direction) { case 'horizontal': - return code === 'ArrowLeft' || code === 'ArrowRight'; + return key === 'ArrowLeft' || key === 'ArrowRight'; case 'vertical': - return code === 'ArrowUp' || code === 'ArrowDown'; + return key === 'ArrowUp' || key === 'ArrowDown'; case 'both': case 'grid': - return code.startsWith('Arrow'); + return key.startsWith('Arrow'); } } handleKeydown = (event: KeyboardEvent): void => { - if (!this.acceptsEventCode(event.code) || event.defaultPrevented) { + if (!this.acceptsEventKey(event.key) || event.defaultPrevented) { return; } let diff = 0; this.prevIndex = this.currentIndex; - switch (event.code) { + switch (event.key) { case 'ArrowRight': diff += 1; break; From 97d88c3b668beab3027cba6435229dfdcd0931f4 Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Thu, 16 Jan 2025 16:09:41 -0500 Subject: [PATCH 03/25] test(menu): updates menu test based on changes --- packages/menu/test/menu.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/menu/test/menu.test.ts b/packages/menu/test/menu.test.ts index 74d2d41396..bec103cc41 100644 --- a/packages/menu/test/menu.test.ts +++ b/packages/menu/test/menu.test.ts @@ -413,8 +413,6 @@ describe('Menu', () => { }) ); await nextFrame(); - // re-bind keyevents - el.startListeningToKeyboard(); // focus management should start again from the first item. el.dispatchEvent(arrowDownEvent()); expect(secondItem.focused, 'second').to.be.true; From d10ed4cbcf72fe6af9edcc79753c8232c254179a Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Thu, 16 Jan 2025 16:11:04 -0500 Subject: [PATCH 04/25] test(menu): adds click test for accessibility --- packages/menu/test/menu-item.test.ts | 37 +++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/packages/menu/test/menu-item.test.ts b/packages/menu/test/menu-item.test.ts index ec6ae45767..24dab4f0b1 100644 --- a/packages/menu/test/menu-item.test.ts +++ b/packages/menu/test/menu-item.test.ts @@ -122,6 +122,42 @@ describe('Menu item', () => { expect(clickTargetSpy.calledWith(anchorElement)).to.be.true; }); + it('allows link click', async () => { + let clicked = false; + const el = await fixture(html` + + + With Target + + + `); + + await elementUpdated(el); + + // prevents browser from activating link but records the proxy click + el.shadowRoot + ?.querySelector('.anchor') + ?.addEventListener('click', (event: Event) => { + event.preventDefault(); + clicked = true; + }); + const rect = el.getBoundingClientRect(); + + // tests mouse click events, and by extension VoiceOver CRTL+Option+Space click + await sendMouse({ + steps: [ + { + position: [ + rect.left + rect.width / 2, + rect.top + rect.height / 2, + ], + type: 'click', + }, + ], + }); + await elementUpdated(el); + expect(clicked).to.be.true; + }); it('value attribute', async () => { const el = await fixture(html` Selected Text @@ -193,7 +229,6 @@ describe('Menu item', () => { expect(el.hasSubmenu).to.be.false; }); - it('should not allow text-align to cascade when used inside an overlay', async () => { const element = await fixture(html`
From cb74cba06e09ac5f83472d65974870f797662c25 Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Fri, 17 Jan 2025 09:14:21 -0500 Subject: [PATCH 05/25] test(menu): menu itself should delegate focus to active item --- packages/menu/test/menu-item.test.ts | 44 ++++++++++++++++++---------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/packages/menu/test/menu-item.test.ts b/packages/menu/test/menu-item.test.ts index 24dab4f0b1..bed959601d 100644 --- a/packages/menu/test/menu-item.test.ts +++ b/packages/menu/test/menu-item.test.ts @@ -117,33 +117,45 @@ describe('Menu item', () => { ).anchorElement.dispatchEvent(new FocusEvent('focus')); await elementUpdated(item); - expect(el === document.activeElement).to.be.true; + + expect(item === document.activeElement).to.be.true; item.click(); expect(clickTargetSpy.calledWith(anchorElement)).to.be.true; }); it('allows link click', async () => { - let clicked = false; + const clickTargetSpy = spy(); const el = await fixture(html` - - - With Target + { + clickTargetSpy( + event.composedPath()[0] as HTMLAnchorElement + ); + event.stopPropagation(); + event.preventDefault(); + }} + > + + Selected Text `); - await elementUpdated(el); + const item = el.querySelector('sp-menu-item') as MenuItem; + const { anchorElement } = item as unknown as { + anchorElement: HTMLAnchorElement; + }; + ( + item as unknown as { anchorElement: HTMLAnchorElement } + ).anchorElement.dispatchEvent(new FocusEvent('focus')); - // prevents browser from activating link but records the proxy click - el.shadowRoot - ?.querySelector('.anchor') - ?.addEventListener('click', (event: Event) => { - event.preventDefault(); - clicked = true; - }); - const rect = el.getBoundingClientRect(); + await elementUpdated(item); + expect(item === document.activeElement).to.be.true; // tests mouse click events, and by extension VoiceOver CRTL+Option+Space click + const rect = el.getBoundingClientRect(); await sendMouse({ steps: [ { @@ -155,8 +167,8 @@ describe('Menu item', () => { }, ], }); - await elementUpdated(el); - expect(clicked).to.be.true; + + expect(clickTargetSpy.calledWith(anchorElement)).to.be.true; }); it('value attribute', async () => { const el = await fixture(html` From 3876c36a21067cac0560634cf98a486b8179b7b0 Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Thu, 23 Jan 2025 11:02:47 -0500 Subject: [PATCH 06/25] fix(menu): fixed menu group navigation according WAI ARIA APG --- packages/menu/src/Menu.ts | 71 ++++++++++++++++++++-------------- packages/menu/src/MenuGroup.ts | 22 ++++++----- 2 files changed, 53 insertions(+), 40 deletions(-) diff --git a/packages/menu/src/Menu.ts b/packages/menu/src/Menu.ts index d4dedcf06a..529e3c42db 100644 --- a/packages/menu/src/Menu.ts +++ b/packages/menu/src/Menu.ts @@ -46,6 +46,13 @@ function elementIsOrContains( ): boolean { return !!isOrContains && (el === isOrContains || el.contains(isOrContains)); } +export class MenuGroupConnectedEvent extends Event { + root?: SpectrumElement | undefined; + constructor({ root }: { root?: SpectrumElement | undefined }) { + super('sp-menu-group-connected', { bubbles: true, composed: true }); + this.root = root; + } +} /** * Spectrum Menu Component @@ -69,24 +76,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { return this.slot === 'submenu'; } - rovingTabindexController = new RovingTabindexController(this, { - focusInIndex: (elements: MenuItem[] | undefined) => { - let firstEnabledIndex = -1; - const firstSelectedIndex = elements?.findIndex((el, index) => { - if (!elements[firstEnabledIndex] && !el.disabled) { - firstEnabledIndex = index; - } - return el.selected && !el.disabled; - }); - return elements && - firstSelectedIndex && - elements[firstSelectedIndex] - ? firstSelectedIndex - : firstEnabledIndex; - }, - elements: () => this.cachedChildItems || [], - isFocusableElement: (el: MenuItem) => !el.disabled, - }); + protected rovingTabindexController?: RovingTabindexController; @property({ type: String, reflect: true }) public label = ''; @@ -186,7 +176,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { ); } - this.rovingTabindexController.clearElementCache(); + this.rovingTabindexController?.clearElementCache(); return this.cachedChildItems; } @@ -340,7 +330,37 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { this.addEventListener('sp-closed', this.handleSubmenuClosed); } + private initRovingTabindexController(): void { + if (!this.rovingTabindexController) { + this.rovingTabindexController = + new RovingTabindexController(this, { + focusInIndex: (elements: MenuItem[] | undefined) => { + let firstEnabledIndex = -1; + const firstSelectedIndex = elements?.findIndex( + (el, index) => { + if ( + !elements[firstEnabledIndex] && + !el.disabled + ) { + firstEnabledIndex = index; + } + return el.selected && !el.disabled; + } + ); + return elements && + firstSelectedIndex && + elements[firstSelectedIndex] + ? firstSelectedIndex + : firstEnabledIndex; + }, + elements: () => this.cachedChildItems || [], + isFocusableElement: (el: MenuItem) => !el.disabled, + }); + } + } + public override focus({ preventScroll }: FocusOptions = {}): void { + if (!this.rovingTabindexController) return; if ( !this.childItems.length || this.childItems.every((childItem) => childItem.disabled) @@ -625,7 +645,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { } public handleKeydown(event: KeyboardEvent): void { - if (event.defaultPrevented) { + if (event.defaultPrevented || !this.rovingTabindexController) { return; } const lastFocusedItem = this.childItems[this.focusedItemIndex]; @@ -678,16 +698,6 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { } return; } - if (key === 'ArrowDown' || key === 'ArrowUp') { - const childItem = this.childItems[this.focusedItemIndex]; - if ( - childItem && - childItem.menuData.selectionRoot === event.target - ) { - this.navigateWithinMenu(event); - } - return; - } this.navigateBetweenRelatedMenus(event); } @@ -933,6 +943,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { if (!this.hasAttribute('role') && !this.ignore) { this.setAttribute('role', this.ownRole); } + this.initRovingTabindexController(); this.updateComplete.then(() => this.updateItemFocus()); } diff --git a/packages/menu/src/MenuGroup.ts b/packages/menu/src/MenuGroup.ts index 69fc3a50ba..36f9836a66 100644 --- a/packages/menu/src/MenuGroup.ts +++ b/packages/menu/src/MenuGroup.ts @@ -21,7 +21,7 @@ import { } from '@spectrum-web-components/base/src/decorators.js'; import { randomID } from '@spectrum-web-components/shared/src/random-id.js'; -import { Menu } from './Menu.js'; +import { Menu, MenuGroupConnectedEvent } from './Menu.js'; // Leveraged in build systems that use aliasing to prevent multiple registrations: https://github.com/adobe/spectrum-web-components/pull/3225 import '@spectrum-web-components/menu/sp-menu.js'; import menuGroupStyles from './menu-group.css.js'; @@ -49,16 +49,11 @@ export class MenuGroup extends Menu { private headerElement?: HTMLElement; protected override get ownRole(): string { - switch (this.selects) { - case 'multiple': - case 'single': - case 'inherit': - return 'group'; - default: - return 'menu'; - } + return 'menu'; } + protected override rovingTabindexController = undefined; + protected updateLabel(): void { const headerElement = this.headerElements.length ? this.headerElements[0] @@ -87,7 +82,14 @@ export class MenuGroup extends Menu { - ${this.renderMenuItemSlot()} + ${this.renderMenuItemSlot()} `; } + public override connectedCallback(): void { + super.connectedCallback(); + this.dispatchEvent(new MenuGroupConnectedEvent({ root: this })); + } + private override initRovingTabindexController(): void { + return; + } } From 350e9ba31e0bc9231200493fff8c02663d086385 Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Thu, 23 Jan 2025 11:07:07 -0500 Subject: [PATCH 07/25] fix(reactive-controllers): complex element delegates focus to active child WAI ARIA APG --- .../src/RovingTabindex.ts | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/tools/reactive-controllers/src/RovingTabindex.ts b/tools/reactive-controllers/src/RovingTabindex.ts index 30081c719b..917066b7b3 100644 --- a/tools/reactive-controllers/src/RovingTabindex.ts +++ b/tools/reactive-controllers/src/RovingTabindex.ts @@ -45,18 +45,14 @@ export class RovingTabindexController< } manageTabindexes(): void { - if (this.focused) { - this.updateTabindexes(() => ({ tabIndex: -1 })); - } else { - this.updateTabindexes((el: HTMLElement): UpdateTabIndexes => { - return { - removeTabIndex: - el.contains(this.focusInElement) && - el !== this.focusInElement, - tabIndex: el === this.focusInElement ? 0 : -1, - }; - }); - } + this.updateTabindexes((el: HTMLElement): UpdateTabIndexes => { + return { + removeTabIndex: + el.contains(this.focusInElement) && + el !== this.focusInElement, + tabIndex: el === this.focusInElement ? 0 : -1, + }; + }); } updateTabindexes(getTabIndex: (el: HTMLElement) => UpdateTabIndexes): void { From b7c750f879c39dccf03c314befaaaae606e65be1 Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Mon, 27 Jan 2025 11:09:20 -0500 Subject: [PATCH 08/25] fix(menu): components should delegate focus --- packages/menu/src/Menu.ts | 94 ++++++++++++++++++---------------- packages/menu/src/MenuGroup.ts | 17 +++--- packages/menu/stories/index.ts | 22 ++++++-- 3 files changed, 74 insertions(+), 59 deletions(-) diff --git a/packages/menu/src/Menu.ts b/packages/menu/src/Menu.ts index 529e3c42db..565ffa4bad 100644 --- a/packages/menu/src/Menu.ts +++ b/packages/menu/src/Menu.ts @@ -46,13 +46,6 @@ function elementIsOrContains( ): boolean { return !!isOrContains && (el === isOrContains || el.contains(isOrContains)); } -export class MenuGroupConnectedEvent extends Event { - root?: SpectrumElement | undefined; - constructor({ root }: { root?: SpectrumElement | undefined }) { - super('sp-menu-group-connected', { bubbles: true, composed: true }); - this.root = root; - } -} /** * Spectrum Menu Component @@ -72,6 +65,11 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { return [menuStyles]; } + static override shadowRootOptions = { + ...SpectrumElement.shadowRootOptions, + delegatesFocus: true, + }; + private get isSubmenu(): boolean { return this.slot === 'submenu'; } @@ -134,6 +132,10 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { public focusedItemIndex = 0; public focusInItemIndex = 0; + protected get controlsRovingTabindex(): boolean { + return true; + } + private selectedItemsMap = new Map(); public get childItems(): MenuItem[] { @@ -177,6 +179,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { } this.rovingTabindexController?.clearElementCache(); + return this.cachedChildItems; } @@ -245,7 +248,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { event.item.menuData.parentMenu = event.item.menuData.parentMenu || this; if (cascadeData.hadFocusRoot && !this.ignore) { // Only have one tab stop per Menu tree - this.tabIndex = -1; + //this.tabIndex = -1; } this.addChildItem(event.item); @@ -324,41 +327,12 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { this.addEventListener('click', this.handleClick); this.addEventListener('pointerup', this.handlePointerup); - this.addEventListener('focusin', this.handleFocusin); - this.addEventListener('blur', this.handleBlur); + //this.addEventListener('focusin', this.handleFocusin); + //this.addEventListener('blur', this.handleBlur); this.addEventListener('sp-opened', this.handleSubmenuOpened); this.addEventListener('sp-closed', this.handleSubmenuClosed); } - private initRovingTabindexController(): void { - if (!this.rovingTabindexController) { - this.rovingTabindexController = - new RovingTabindexController(this, { - focusInIndex: (elements: MenuItem[] | undefined) => { - let firstEnabledIndex = -1; - const firstSelectedIndex = elements?.findIndex( - (el, index) => { - if ( - !elements[firstEnabledIndex] && - !el.disabled - ) { - firstEnabledIndex = index; - } - return el.selected && !el.disabled; - } - ); - return elements && - firstSelectedIndex && - elements[firstSelectedIndex] - ? firstSelectedIndex - : firstEnabledIndex; - }, - elements: () => this.cachedChildItems || [], - isFocusableElement: (el: MenuItem) => !el.disabled, - }); - } - } - public override focus({ preventScroll }: FocusOptions = {}): void { if (!this.rovingTabindexController) return; if ( @@ -375,8 +349,9 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { super.focus({ preventScroll }); return; } - this.focusMenuItemByOffset(0); + //this.focusMenuItemByOffset(0); this.rovingTabindexController.focus({ preventScroll }); + super.focus({ preventScroll }); const selectedItem = this.selectedItems[0]; if (selectedItem && !preventScroll) { selectedItem.scrollIntoView({ block: 'nearest' }); @@ -459,7 +434,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { | MenuItem | Menu; - // selected child items root menu + // selected child item's root menu const selectionRoot = this.childItems[this.focusedItemIndex]?.menuData.selectionRoot || this; @@ -666,7 +641,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { !(event as KeyboardEvent).shiftKey && !this.hasAttribute('tabindex') ) { - this.tabIndex = 0; + //this.tabIndex = 0; document.removeEventListener('keyup', replaceTabindex); this.removeEventListener('focusout', replaceTabindex); } @@ -812,10 +787,13 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { this.updateSelectedItemIndex(); this.updateItemFocus(); } + this._willUpdateItems = false; } private updateItemFocus(): void { + const focusInElement = this.rovingTabindexController?.focusInElement; + focusInElement?.setAttribute('tabindex', '0'); if (this.childItems.length == 0) { return; } @@ -891,14 +869,14 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { protected override firstUpdated(changed: PropertyValues): void { super.firstUpdated(changed); - if (!this.hasAttribute('tabindex') && !this.ignore) { + /*if (!this.hasAttribute('tabindex') && !this.ignore) { const role = this.getAttribute('role'); if (role === 'group') { this.tabIndex = -1; } else { this.tabIndex = 0; } - } + }*/ const updates: Promise[] = [ new Promise((res) => requestAnimationFrame(() => res(true))), ]; @@ -943,7 +921,33 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { if (!this.hasAttribute('role') && !this.ignore) { this.setAttribute('role', this.ownRole); } - this.initRovingTabindexController(); + + if (!this.rovingTabindexController && this.controlsRovingTabindex) { + this.rovingTabindexController = + new RovingTabindexController(this, { + focusInIndex: (elements: MenuItem[] | undefined) => { + let firstEnabledIndex = -1; + const firstSelectedIndex = elements?.findIndex( + (el, index) => { + if ( + !elements[firstEnabledIndex] && + !el.disabled + ) { + firstEnabledIndex = index; + } + return el.selected && !el.disabled; + } + ); + return elements && + firstSelectedIndex && + elements[firstSelectedIndex] + ? firstSelectedIndex + : firstEnabledIndex; + }, + elements: () => this.cachedChildItems || [], + isFocusableElement: (el: MenuItem) => !el.disabled, + }); + } this.updateComplete.then(() => this.updateItemFocus()); } diff --git a/packages/menu/src/MenuGroup.ts b/packages/menu/src/MenuGroup.ts index 36f9836a66..47798c67af 100644 --- a/packages/menu/src/MenuGroup.ts +++ b/packages/menu/src/MenuGroup.ts @@ -21,7 +21,7 @@ import { } from '@spectrum-web-components/base/src/decorators.js'; import { randomID } from '@spectrum-web-components/shared/src/random-id.js'; -import { Menu, MenuGroupConnectedEvent } from './Menu.js'; +import { Menu } from './Menu.js'; // Leveraged in build systems that use aliasing to prevent multiple registrations: https://github.com/adobe/spectrum-web-components/pull/3225 import '@spectrum-web-components/menu/sp-menu.js'; import menuGroupStyles from './menu-group.css.js'; @@ -48,11 +48,17 @@ export class MenuGroup extends Menu { @state() private headerElement?: HTMLElement; + /** + * a menu group must have the role `group` + * and should never function as a menu + */ protected override get ownRole(): string { return 'menu'; } - protected override rovingTabindexController = undefined; + protected override get controlsRovingTabindex(): boolean { + return false; + } protected updateLabel(): void { const headerElement = this.headerElements.length @@ -85,11 +91,4 @@ export class MenuGroup extends Menu { ${this.renderMenuItemSlot()} `; } - public override connectedCallback(): void { - super.connectedCallback(); - this.dispatchEvent(new MenuGroupConnectedEvent({ root: this })); - } - private override initRovingTabindexController(): void { - return; - } } diff --git a/packages/menu/stories/index.ts b/packages/menu/stories/index.ts index 28d99a1275..efc0b96dd3 100644 --- a/packages/menu/stories/index.ts +++ b/packages/menu/stories/index.ts @@ -53,19 +53,25 @@ export class ComplexSlottedGroup extends LitElement { get menu(): Menu { return this.renderRoot.querySelector('sp-menu') as Menu; } + + static override shadowRootOptions = { + ...LitElement.shadowRootOptions, + delegatesFocus: true, + }; + protected override render(): TemplateResult { return html` - - + + Before First - + Sibling 1 Sibling 2 - + After 1 After 2 @@ -84,9 +90,15 @@ export class ComplexSlottedMenu extends LitElement { ) as ComplexSlottedGroup ).menu; } + + static override shadowRootOptions = { + ...LitElement.shadowRootOptions, + delegatesFocus: true, + }; + protected override render(): TemplateResult { return html` - + Middle 1 Middle 2 Middle 3 From 68a298d83dfdba0689e705be2a9e1567ce524c66 Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Mon, 27 Jan 2025 16:38:41 -0500 Subject: [PATCH 09/25] fix(menu): update keyboard nav to match WAI ARIA APG --- packages/menu/src/Menu.ts | 51 +--------------------------- packages/menu/src/MenuItem.ts | 64 +++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 50 deletions(-) diff --git a/packages/menu/src/Menu.ts b/packages/menu/src/Menu.ts index 565ffa4bad..8eab880bf8 100644 --- a/packages/menu/src/Menu.ts +++ b/packages/menu/src/Menu.ts @@ -40,13 +40,6 @@ export interface MenuChildItem { type SelectsType = 'none' | 'ignore' | 'inherit' | 'multiple' | 'single'; type RoleType = 'group' | 'menu' | 'listbox' | 'none'; -function elementIsOrContains( - el: Node, - isOrContains: Node | undefined | null -): boolean { - return !!isOrContains && (el === isOrContains || el.contains(isOrContains)); -} - /** * Spectrum Menu Component * @element sp-menu @@ -246,10 +239,6 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { if (!cascadeData) return; event.item.menuData.parentMenu = event.item.menuData.parentMenu || this; - if (cascadeData.hadFocusRoot && !this.ignore) { - // Only have one tab stop per Menu tree - //this.tabIndex = -1; - } this.addChildItem(event.item); if (this.selects === 'inherit') { @@ -327,8 +316,6 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { this.addEventListener('click', this.handleClick); this.addEventListener('pointerup', this.handlePointerup); - //this.addEventListener('focusin', this.handleFocusin); - //this.addEventListener('blur', this.handleBlur); this.addEventListener('sp-opened', this.handleSubmenuOpened); this.addEventListener('sp-closed', this.handleSubmenuClosed); } @@ -421,42 +408,6 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { this.prepareToCleanUp(); } - public handleFocusin(event: FocusEvent): void { - // ignore if a child element has a different root menu - if ( - this.childItems.some( - (childItem) => childItem.menuData.focusRoot !== this - ) - ) { - return; - } - const activeElement = (this.getRootNode() as Document).activeElement as - | MenuItem - | Menu; - - // selected child item's root menu - const selectionRoot = - this.childItems[this.focusedItemIndex]?.menuData.selectionRoot || - this; - - if (activeElement !== selectionRoot || event.target !== this) { - selectionRoot.focus({ preventScroll: true }); - if (activeElement && this.focusedItemIndex === 0) { - const offset = this.childItems.findIndex( - (childItem) => childItem === activeElement - ); - this.focusMenuItemByOffset(Math.max(offset, 0)); - } - } - } - - public handleBlur(event: FocusEvent): void { - if (elementIsOrContains(this, event.relatedTarget as Node)) { - return; - } - this.childItems.forEach((child) => (child.focused = false)); - } - private descendentOverlays = new Map(); protected handleDescendentOverlayOpened(event: Event): void { @@ -657,7 +608,6 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { if (lastFocusedItem?.hasSubmenu) { // Remove focus while opening overlay from keyboard or the visible focus // will slip back to the first item in the menu. - // this.blur(); lastFocusedItem.openOverlay(); return; } @@ -925,6 +875,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { if (!this.rovingTabindexController && this.controlsRovingTabindex) { this.rovingTabindexController = new RovingTabindexController(this, { + direction: 'vertical', focusInIndex: (elements: MenuItem[] | undefined) => { let firstEnabledIndex = -1; const firstSelectedIndex = elements?.findIndex( diff --git a/packages/menu/src/MenuItem.ts b/packages/menu/src/MenuItem.ts index 66b39e92dd..e9bbc36e97 100644 --- a/packages/menu/src/MenuItem.ts +++ b/packages/menu/src/MenuItem.ts @@ -384,6 +384,7 @@ export class MenuItem extends LikeAnchor( protected override firstUpdated(changes: PropertyValues): void { super.firstUpdated(changes); this.setAttribute('tabindex', '-1'); + this.addEventListener('keydown', this.handleKeydown); this.addEventListener('pointerdown', this.handlePointerdown); this.addEventListener('pointerenter', this.closeOverlaysForRoot); if (!this.hasAttribute('id')) { @@ -391,6 +392,56 @@ export class MenuItem extends LikeAnchor( } } + handleKeydown = (event: KeyboardEvent): void => { + if (event.defaultPrevented) { + return; + } + const isLTR = this.isLTR; + let action: 'open' | 'close' | 'none' = 'none'; + switch (event.key) { + case 'Enter': + action = 'open'; + break; + case 'Escape': + action = 'close'; + break; + case 'ArrowLeft': + if (isLTR) { + action = 'close'; + } else { + action = 'open'; + } + break; + case 'ArrowRight': + if (isLTR) { + action = 'open'; + } else { + action = 'close'; + } + break; + } + if (action === 'none') { + return; + } else { + event.preventDefault(); + event.stopPropagation(); + if (action === 'open') { + const handleSubmenuOpen = (): void => { + this.submenuElement?.focus(); + }; + this.addEventListener( + 'sp-menu-submenu-opened', + handleSubmenuOpen, + { once: true } + ); + this.openOverlay(); + } else if (action === 'close') { + this.menuData.parentMenu?.parentElement?.focus(); + this.open = false; + } + } + }; + protected closeOverlaysForRoot(): void { if (this.open) return; this.menuData.parentMenu?.closeDescendentOverlays(); @@ -510,6 +561,19 @@ export class MenuItem extends LikeAnchor( this.updateAriaSelected(); } + protected override willUpdate(changes: PropertyValues): void { + super.updated(changes); + + // make sure focus returns to the anchor element when submenu is closed + if ( + changes.has('open') && + !this.open && + this.matches(':focus-within') + ) { + this.focus(); + } + } + protected override updated(changes: PropertyValues): void { super.updated(changes); if ( From 63e1dab95ebab7f6ad10019acecfc7b535ee3688 Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Tue, 28 Jan 2025 10:09:30 -0500 Subject: [PATCH 10/25] fix(menu): updated docs to reflect preferred accessible method --- packages/menu/menu-group.md | 14 ++------------ packages/menu/menu-item.md | 33 ++++++++++++++++++++++++++++----- 2 files changed, 30 insertions(+), 17 deletions(-) diff --git a/packages/menu/menu-group.md b/packages/menu/menu-group.md index 4cf9a26c4e..3c95fed752 100644 --- a/packages/menu/menu-group.md +++ b/packages/menu/menu-group.md @@ -30,22 +30,13 @@ An `` can be used to organize ``s in an `` ```html -

- Your favorite park in New York is: -

- Your favorite park in San Fransisco is: -

+ selects="single"> New York @@ -60,9 +51,8 @@ An `` can be used to organize ``s in an `` - San Fransisco + San Francisco Golden Gate Park diff --git a/packages/menu/menu-item.md b/packages/menu/menu-item.md index 752564b7b5..7cf93ac519 100644 --- a/packages/menu/menu-item.md +++ b/packages/menu/menu-item.md @@ -95,12 +95,35 @@ Content assigned to the `value` slot will be placed at the end of the `` can also accept content addressed to its `submenu` slot. Using the `` element with this slot name the options will be surfaced in flyout menu that can be activated by hovering over the root menu item with your pointer or focusing the menu item and pressing the appropriate `ArrowRight` or `ArrowLeft` key based on text direction to move into the submenu. ```html - +

+ Your favorite park in New York is: + +
+
+ Your favorite park in San Francisco is: + +

+ + + New York + + Central Park + Flushing Meadows Corona Park + Prospect Park + + - Item with submenu - - Additional options - Available on request + San Francisco + + Golden Gate Park + John McLaren Park + Lake Merced Park From b6ee246d2e11f4e88067d531878955dac0e35292 Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Wed, 29 Jan 2025 11:49:45 -0500 Subject: [PATCH 11/25] fix(menu): selection and keyboard navigation according to APG --- packages/menu/src/Menu.ts | 206 ++++++++++----------------------- packages/menu/src/MenuGroup.ts | 6 +- packages/menu/src/MenuItem.ts | 163 ++++++++++++++++++-------- 3 files changed, 183 insertions(+), 192 deletions(-) diff --git a/packages/menu/src/Menu.ts b/packages/menu/src/Menu.ts index 8eab880bf8..041c1527ad 100644 --- a/packages/menu/src/Menu.ts +++ b/packages/menu/src/Menu.ts @@ -24,7 +24,10 @@ import { } from '@spectrum-web-components/base/src/decorators.js'; import { MenuItem } from './MenuItem.js'; -import type { MenuItemAddedOrUpdatedEvent } from './MenuItem.js'; +import type { + MenuItemAddedOrUpdatedEvent, + MenuItemKeydownEvent, +} from './MenuItem.js'; import type { Overlay } from '@spectrum-web-components/overlay'; import menuStyles from './menu.css.js'; import { RovingTabindexController } from '@spectrum-web-components/reactive-controllers/src/RovingTabindex.js'; @@ -69,15 +72,31 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { protected rovingTabindexController?: RovingTabindexController; + /** + * label of the menu + */ @property({ type: String, reflect: true }) public label = ''; + /** + * whether menu should be ignored by roving tabindex controller + */ @property({ type: Boolean, reflect: true }) public ignore = false; + /** + * how the menu allows selection of its items: + * - `undefined` (default): no selection is allowed + * - `"inherit"`: the selection behavior is managed from an ancestor + * - `"single"`: only one item can be selected at a time + * - `"multiple"`: multiple items can be selected + */ @property({ type: String, reflect: true }) public selects: undefined | 'inherit' | 'single' | 'multiple'; + /** + * value of the selected item(s) + */ @property({ type: String }) public value = ''; @@ -86,7 +105,9 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { @property({ type: String, attribute: 'value-separator' }) public valueSeparator = ','; - // TODO: which of these to keep? + /** + * selected items values as string + */ @property({ attribute: false }) public get selected(): string[] { return this._selected; @@ -115,6 +136,9 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { protected _selected = [] as string[]; + /** + * array of selected menu items + */ @property({ attribute: false }) public selectedItems = [] as MenuItem[]; @@ -131,6 +155,9 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { private selectedItemsMap = new Map(); + /** + * child items managed by menu + */ public get childItems(): MenuItem[] { if (!this.cachedChildItems) { this.cachedChildItems = this.updateCachedMenuItems(); @@ -201,7 +228,14 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { return 'menu'; } + /** + * menuitem role based on selection type + */ private resolvedSelects?: SelectsType; + + /** + * menu role based on selection type + */ private resolvedRole?: RoleType; /** @@ -315,6 +349,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { ); this.addEventListener('click', this.handleClick); + this.addEventListener('sp-menu-item-keydown', this.handleKeydown); this.addEventListener('pointerup', this.handlePointerup); this.addEventListener('sp-opened', this.handleSubmenuOpened); this.addEventListener('sp-closed', this.handleSubmenuClosed); @@ -336,7 +371,6 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { super.focus({ preventScroll }); return; } - //this.focusMenuItemByOffset(0); this.rovingTabindexController.focus({ preventScroll }); super.focus({ preventScroll }); const selectedItem = this.selectedItems[0]; @@ -405,7 +439,6 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { } else { return; } - this.prepareToCleanUp(); } private descendentOverlays = new Map(); @@ -447,18 +480,12 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { composed: true, }) ); - const focusedItem = this.childItems[this.focusedItemIndex]; - if (focusedItem) { - focusedItem.focused = false; - } + const openedItem = event .composedPath() .find((el) => this.childItemSet.has(el as MenuItem)); /* c8 ignore next 1 */ if (!openedItem) return; - const openedItemIndex = this.childItems.indexOf(openedItem as MenuItem); - this.focusedItemIndex = openedItemIndex; - this.focusInItemIndex = openedItemIndex; }; public async selectOrToggleItem(targetItem: MenuItem): Promise { @@ -467,13 +494,10 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { const oldSelected = this.selected.slice(); const oldSelectedItems = this.selectedItems.slice(); const oldValue = this.value; - const focusedChild = this.childItems[this.focusedItemIndex]; - if (focusedChild) { - focusedChild.focused = false; - focusedChild.active = false; + + if (targetItem.menuData.selectionRoot !== this) { + return; } - this.focusedItemIndex = this.childItems.indexOf(targetItem); - this.forwardFocusVisibleToItem(targetItem); if (resolvedSelects === 'multiple') { if (this.selectedItemsMap.has(targetItem)) { @@ -535,34 +559,21 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { } } - protected navigateWithinMenu(event: KeyboardEvent): void { - const { key } = event; - const lastFocusedItem = this.childItems[this.focusedItemIndex]; - const direction = key === 'ArrowDown' ? 1 : -1; - const itemToFocus = this.focusMenuItemByOffset(direction); - if (itemToFocus === lastFocusedItem) { - return; - } - event.preventDefault(); - event.stopPropagation(); - itemToFocus.scrollIntoView({ block: 'nearest' }); - } - - protected navigateBetweenRelatedMenus(event: KeyboardEvent): void { - const { key } = event; + protected navigateBetweenRelatedMenus(event: MenuItemKeydownEvent): void { + const { key, root } = event; event.stopPropagation(); const shouldOpenSubmenu = (this.isLTR && key === 'ArrowRight') || (!this.isLTR && key === 'ArrowLeft'); const shouldCloseSelfAsSubmenu = (this.isLTR && key === 'ArrowLeft') || - (!this.isLTR && key === 'ArrowRight'); + (!this.isLTR && key === 'ArrowRight') || + key === 'Escape'; + const lastFocusedItem = root as MenuItem; if (shouldOpenSubmenu) { - const lastFocusedItem = this.childItems[this.focusedItemIndex]; if (lastFocusedItem?.hasSubmenu) { - // Remove focus while opening overlay from keyboard or the visible focus - // will slip back to the first item in the menu. - lastFocusedItem.openOverlay(); + //open submenu and set focus + lastFocusedItem.openOverlay(true); } } else if (shouldCloseSelfAsSubmenu && this.isSubmenu) { this.dispatchEvent(new Event('close', { bubbles: true })); @@ -570,20 +581,12 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { } } - public handleKeydown(event: KeyboardEvent): void { + public handleKeydown(event: Event): void { if (event.defaultPrevented || !this.rovingTabindexController) { return; } - const lastFocusedItem = this.childItems[this.focusedItemIndex]; - if (lastFocusedItem) { - lastFocusedItem.focused = true; - } - const { key } = event; - if ( - event.shiftKey && - event.target !== this && - this.hasAttribute('tabindex') - ) { + const { key, root, shiftKey, target } = event as MenuItemKeydownEvent; + if (shiftKey && target !== this && this.hasAttribute('tabindex')) { this.removeAttribute('tabindex'); const replaceTabindex = ( event: FocusEvent | KeyboardEvent @@ -592,7 +595,6 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { !(event as KeyboardEvent).shiftKey && !this.hasAttribute('tabindex') ) { - //this.tabIndex = 0; document.removeEventListener('keyup', replaceTabindex); this.removeEventListener('focusout', replaceTabindex); } @@ -601,73 +603,24 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { this.addEventListener('focusout', replaceTabindex); } if (key === 'Tab') { - this.prepareToCleanUp(); + this.closeDescendentOverlays(); return; } if (key === ' ') { - if (lastFocusedItem?.hasSubmenu) { + if (root?.hasSubmenu) { // Remove focus while opening overlay from keyboard or the visible focus // will slip back to the first item in the menu. - lastFocusedItem.openOverlay(); + root.openOverlay(); return; } } if (key === ' ' || key === 'Enter') { - const childItem = this.childItems[this.focusedItemIndex]; - if ( - childItem && - childItem.menuData.selectionRoot === event.target - ) { - event.preventDefault(); - childItem.click(); - } + event.preventDefault(); + root?.click(); + if (root) this.selectOrToggleItem(root); return; } - this.navigateBetweenRelatedMenus(event); - } - - public focusMenuItemByOffset(offset: number): MenuItem { - const step = offset || 1; - const focusedItem = this.childItems[this.focusedItemIndex]; - if (focusedItem) { - focusedItem.focused = false; - // Remain active while a submenu is opened. - focusedItem.active = focusedItem.open; - } - this.focusedItemIndex = - (this.childItems.length + this.focusedItemIndex + offset) % - this.childItems.length; - let itemToFocus = this.childItems[this.focusedItemIndex]; - let availableItems = this.childItems.length; - // cycle through the available items in the directions of the offset to find the next non-disabled item - while (itemToFocus?.disabled && availableItems) { - availableItems -= 1; - this.focusedItemIndex = - (this.childItems.length + this.focusedItemIndex + step) % - this.childItems.length; - itemToFocus = this.childItems[this.focusedItemIndex]; - } - // if there are no non-disabled items, skip the work to focus a child - if (!itemToFocus?.disabled) { - this.forwardFocusVisibleToItem(itemToFocus); - } - return itemToFocus; - } - - private prepareToCleanUp(): void { - document.addEventListener( - 'focusout', - () => { - requestAnimationFrame(() => { - const focusedItem = this.childItems[this.focusedItemIndex]; - if (focusedItem) { - focusedItem.focused = false; - this.updateSelectedItemIndex(); - } - }); - }, - { once: true } - ); + this.navigateBetweenRelatedMenus(event as MenuItemKeydownEvent); } private _hasUpdatedSelectedItemIndex = false; @@ -695,7 +648,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { // Remove "focused" from non-"selected" items ONLY // Preserve "focused" on index===0 when no selection if (itemIndex !== firstOrFirstSelectedIndex) { - childItem.focused = false; + //childItem.focused = false; } } } @@ -703,15 +656,13 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { // When there is more than one "selected" item, // ensure only the first one can be "focused" if (i > 0) { - item.focused = false; + //item.focused = false; } }); this.selectedItemsMap = selectedItemsMap; this._selected = selected; this.selectedItems = selectedItems; this.value = this.selected.join(this.valueSeparator); - this.focusedItemIndex = firstOrFirstSelectedIndex; - this.focusInItemIndex = firstOrFirstSelectedIndex; } private _willUpdateItems = false; @@ -747,13 +698,6 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { if (this.childItems.length == 0) { return; } - const focusInItem = this.childItems[this.focusInItemIndex]; - if ( - (this.getRootNode() as Document).activeElement === - focusInItem.menuData.focusRoot - ) { - this.forwardFocusVisibleToItem(focusInItem); - } } public closeDescendentOverlays(): void { @@ -763,25 +707,6 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { this.descendentOverlays = new Map(); } - private forwardFocusVisibleToItem(item: MenuItem): void { - if (!item || item.menuData.focusRoot !== this) { - return; - } - this.closeDescendentOverlays(); - const focused = - this.hasVisibleFocusInTree() || - !!this.childItems.find((child) => { - return child.hasVisibleFocusInTree(); - }); - item.focused = focused; - if ( - item.menuData.selectionRoot && - item.menuData.selectionRoot !== this - ) { - item.menuData.selectionRoot.focus(); - } - } - private handleSlotchange({ target, }: Event & { target: HTMLSlotElement }): void { @@ -819,14 +744,6 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { protected override firstUpdated(changed: PropertyValues): void { super.firstUpdated(changed); - /*if (!this.hasAttribute('tabindex') && !this.ignore) { - const role = this.getAttribute('role'); - if (role === 'group') { - this.tabIndex = -1; - } else { - this.tabIndex = 0; - } - }*/ const updates: Promise[] = [ new Promise((res) => requestAnimationFrame(() => res(true))), ]; @@ -872,6 +789,9 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { this.setAttribute('role', this.ownRole); } + /** + * only create an RTI if menu controls keyboard navigation and one does not already exist + */ if (!this.rovingTabindexController && this.controlsRovingTabindex) { this.rovingTabindexController = new RovingTabindexController(this, { diff --git a/packages/menu/src/MenuGroup.ts b/packages/menu/src/MenuGroup.ts index 47798c67af..7311706d24 100644 --- a/packages/menu/src/MenuGroup.ts +++ b/packages/menu/src/MenuGroup.ts @@ -53,9 +53,13 @@ export class MenuGroup extends Menu { * and should never function as a menu */ protected override get ownRole(): string { - return 'menu'; + return 'group'; } + /** + * only a menu controls roving tabindex; + * groups should defer navigation to parent menu + */ protected override get controlsRovingTabindex(): boolean { return false; } diff --git a/packages/menu/src/MenuItem.ts b/packages/menu/src/MenuItem.ts index e9bbc36e97..d77712f3ba 100644 --- a/packages/menu/src/MenuItem.ts +++ b/packages/menu/src/MenuItem.ts @@ -54,6 +54,9 @@ type MenuCascadeItem = { ancestorWithSelects?: HTMLElement; }; +/** + * Fires when a menu item is added or updated so that a parent menu can track it. + */ export class MenuItemAddedOrUpdatedEvent extends Event { constructor(item: MenuItem) { super('sp-menu-item-added-or-updated', { @@ -81,6 +84,55 @@ export class MenuItemAddedOrUpdatedEvent extends Event { currentAncestorWithSelects?: Menu; } +/** + * Fires to forward keyboard event information to parent menu. + */ +export class MenuItemKeydownEvent extends KeyboardEvent { + root?: MenuItem; + private _event?: KeyboardEvent; + constructor({ root, event }: { root?: MenuItem; event?: KeyboardEvent }) { + super('sp-menu-item-keydown', { bubbles: true, composed: true }); + this.root = root; + this._event = event; + } + + public override get altKey(): boolean { + return this._event?.altKey || false; + } + + public override get code(): string { + return this._event?.code || ''; + } + + public override get ctrlKey(): boolean { + return this._event?.ctrlKey || false; + } + + public override get isComposing(): boolean { + return this._event?.isComposing || false; + } + + public override get key(): string { + return this._event?.key || ''; + } + + public override get location(): number { + return this._event?.location || 0; + } + + public override get metaKey(): boolean { + return this._event?.metaKey || false; + } + + public override get repeat(): boolean { + return this._event?.repeat || false; + } + + public override get shiftKey(): boolean { + return this._event?.shiftKey || false; + } +} + export type MenuItemChildren = { icon: Element[]; content: Node[] }; /** @@ -108,17 +160,29 @@ export class MenuItem extends LikeAnchor( abortControllerSubmenu!: AbortController; + /** + * whether the menu item is active or has an active descendant + */ @property({ type: Boolean, reflect: true }) public active = false; private dependencyManager = new DependencyManagerController(this); + /** + * whether the menu item has keyboard focus + */ @property({ type: Boolean, reflect: true }) public focused = false; + /** + * whether the menu item is selected + */ @property({ type: Boolean, reflect: true }) public selected = false; + /** + * value of the menu item which is used for selection + */ @property({ type: String }) public get value(): string { return this._value || this.itemText; @@ -140,6 +204,7 @@ export class MenuItem extends LikeAnchor( /** * @private + * text content of the menu item minus whitespace */ public get itemText(): string { return this.itemChildren.content.reduce( @@ -148,6 +213,9 @@ export class MenuItem extends LikeAnchor( ); } + /** + * whether the menu item has a submenu + */ @property({ type: Boolean, reflect: true, attribute: 'has-submenu' }) public hasSubmenu = false; @@ -157,6 +225,9 @@ export class MenuItem extends LikeAnchor( @query('slot[name="icon"]') iconSlot!: HTMLSlotElement; + /** + * whether menu item text content should not wrap + */ @property({ type: Boolean, reflect: true, @@ -175,6 +246,9 @@ export class MenuItem extends LikeAnchor( private submenuElement?: HTMLElement; + /** + * the focusable element of the menu item + */ public override get focusElement(): HTMLElement { return this; } @@ -214,6 +288,8 @@ export class MenuItem extends LikeAnchor( this.addEventListener('click', this.handleClickCapture, { capture: true, }); + this.addEventListener('focus', this.handleFocus); + this.addEventListener('blur', this.handleBlur); new MutationController(this, { config: { @@ -234,6 +310,9 @@ export class MenuItem extends LikeAnchor( }); } + /** + * whether submenu is open + */ @property({ type: Boolean, reflect: true }) public open = false; @@ -359,6 +438,9 @@ export class MenuItem extends LikeAnchor( `; } + /** + * determines if item has a submenu and updates the `aria-haspopup` attribute + */ protected manageSubmenu(event: Event & { target: HTMLSlotElement }): void { this.submenuElement = event.target.assignedElements({ flatten: true, @@ -392,54 +474,15 @@ export class MenuItem extends LikeAnchor( } } + /** + * forward key info from keydown event to parent menu + */ handleKeydown = (event: KeyboardEvent): void => { - if (event.defaultPrevented) { - return; - } - const isLTR = this.isLTR; - let action: 'open' | 'close' | 'none' = 'none'; - switch (event.key) { - case 'Enter': - action = 'open'; - break; - case 'Escape': - action = 'close'; - break; - case 'ArrowLeft': - if (isLTR) { - action = 'close'; - } else { - action = 'open'; - } - break; - case 'ArrowRight': - if (isLTR) { - action = 'open'; - } else { - action = 'close'; - } - break; - } - if (action === 'none') { - return; - } else { - event.preventDefault(); - event.stopPropagation(); - if (action === 'open') { - const handleSubmenuOpen = (): void => { - this.submenuElement?.focus(); - }; - this.addEventListener( - 'sp-menu-submenu-opened', - handleSubmenuOpen, - { once: true } - ); - this.openOverlay(); - } else if (action === 'close') { - this.menuData.parentMenu?.parentElement?.focus(); - this.open = false; - } - } + const { target } = event; + if (target === this) + this.dispatchEvent( + new MenuItemKeydownEvent({ root: this, event: event }) + ); }; protected closeOverlaysForRoot(): void { @@ -447,6 +490,20 @@ export class MenuItem extends LikeAnchor( this.menuData.parentMenu?.closeDescendentOverlays(); } + protected handleFocus(event: FocusEvent): void { + const { target } = event; + if (target === this) { + this.focused = true; + } + } + + protected handleBlur(event: FocusEvent): void { + const { target } = event; + if (target === this) { + this.focused = false; + } + } + protected handleSubmenuClick(event: Event): void { if (event.composedPath().includes(this.overlayElement)) { return; @@ -532,10 +589,18 @@ export class MenuItem extends LikeAnchor( this.active = false; } - public async openOverlay(): Promise { + public async openOverlay(focus: boolean = false): Promise { if (!this.hasSubmenu || this.open || this.disabled) { return; } + if (focus) { + const handleSubmenuOpen = (): void => { + this.submenuElement?.focus(); + }; + this.addEventListener('sp-menu-submenu-opened', handleSubmenuOpen, { + once: true, + }); + } this.open = true; this.active = true; this.setAttribute('aria-expanded', 'true'); @@ -674,8 +739,10 @@ export class MenuItem extends LikeAnchor( selectionRoot?: Menu; cleanupSteps: ((item: MenuItem) => void)[]; } = { + // menu that controls ArrowUp/ArrowDown navigation focusRoot: undefined, parentMenu: undefined, + // menu or menu group that controls selection selectionRoot: undefined, cleanupSteps: [], }; From ef99ebbc39c263a8a32ef64fa4d0e04e312fc84a Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Wed, 29 Jan 2025 12:04:02 -0500 Subject: [PATCH 12/25] test(menu): updated to recommended a11y behavior from APG --- packages/menu/test/menu-selects.test.ts | 93 ++++++++++++------------- 1 file changed, 43 insertions(+), 50 deletions(-) diff --git a/packages/menu/test/menu-selects.test.ts b/packages/menu/test/menu-selects.test.ts index 90bbb2c68b..a6abd9794d 100644 --- a/packages/menu/test/menu-selects.test.ts +++ b/packages/menu/test/menu-selects.test.ts @@ -29,15 +29,13 @@ describe('Menu [selects]', () => { let el!: Menu; let options!: MenuItem[]; beforeEach(async () => { - el = await fixture( - html` - - Option 1 - Option 2 - Option 3 - - ` - ); + el = await fixture(html` + + Option 1 + Option 2 + Option 3 + + `); options = [...el.querySelectorAll('sp-menu-item')] as MenuItem[]; await Promise.all(options.map((option) => option.updateComplete)); await nextFrame(); @@ -151,17 +149,15 @@ describe('Menu [selects] w/ group', () => { let el!: Menu; let options!: MenuItem[]; beforeEach(async () => { - el = await fixture( - html` - - - Option 1 - Option 2 - Option 3 - - - ` - ); + el = await fixture(html` + + + Option 1 + Option 2 + Option 3 + + + `); options = [...el.querySelectorAll('sp-menu-item')] as MenuItem[]; await Promise.all(options.map((option) => option.updateComplete)); await nextFrame(); @@ -275,17 +271,15 @@ describe('Menu w/ group [selects]', () => { let group!: MenuGroup; let options!: MenuItem[]; beforeEach(async () => { - el = await fixture( - html` - - - Option 1 - Option 2 - Option 3 - - - ` - ); + el = await fixture(html` + + + Option 1 + Option 2 + Option 3 + + + `); group = el.querySelector('sp-menu-group') as MenuGroup; options = [...el.querySelectorAll('sp-menu-item')] as MenuItem[]; await Promise.all(options.map((option) => option.updateComplete)); @@ -403,22 +397,20 @@ describe('Menu w/ groups [selects]', () => { let groupB!: MenuGroup; let options!: MenuItem[]; beforeEach(async () => { - el = await fixture( - html` - - - Option 1a - Option 2a - Option 3a - - - Option 1b - Option 2b - Option 3b - - - ` - ); + el = await fixture(html` + + + Option 1a + Option 2a + Option 3a + + + Option 1b + Option 2b + Option 3b + + + `); groupA = el.querySelector('sp-menu-group:first-child') as MenuGroup; groupB = el.querySelector('sp-menu-group:last-child') as MenuGroup; options = [...el.querySelectorAll('sp-menu-item')] as MenuItem[]; @@ -638,16 +630,17 @@ describe('Menu w/ groups [selects]', () => { input.focus(); expect(document.activeElement === input).to.be.true; await sendKeys({ press: 'Shift+Tab' }); - expect(document.activeElement === groupA).to.be.true; + expect(document.activeElement === options[0]).to.be.true; await sendKeys({ press: 'ArrowDown' }); + expect(document.activeElement === options[1]).to.be.true; await sendKeys({ press: 'ArrowUp' }); await elementUpdated(el); let optionCount = 0; for (const option of options) { const parentElement = option.parentElement as Menu; - expect(document.activeElement === parentElement, 'parent focused') - .to.be.true; + expect(document.activeElement === option, 'option focused').to.be + .true; expect(option.focused, `option ${optionCount} visually focused`).to .be.true; await sendKeys({ press: 'Space' }); From eb7e539e4016b4c80889f5c7470e169b4dda88a5 Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Wed, 29 Jan 2025 12:06:15 -0500 Subject: [PATCH 13/25] docs(menu): reverted docs but fixed typos --- packages/menu/menu-group.md | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/packages/menu/menu-group.md b/packages/menu/menu-group.md index 3c95fed752..e434869e53 100644 --- a/packages/menu/menu-group.md +++ b/packages/menu/menu-group.md @@ -30,13 +30,22 @@ An `` can be used to organize ``s in an `` ```html +

+ Your favorite park in New York is: +

+ Your favorite park in San Francisco is: +

+ onchange="this.parentElement + .previousElementSibling + .querySelector(`#${arguments[0].target.id}-value`) + .textContent = arguments[0].target.value"> New York @@ -51,6 +60,7 @@ An `` can be used to organize ``s in an `` San Francisco From 36cc4a25d5f0d9cd8d4401cc3f1af6142eeb120e Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Wed, 29 Jan 2025 12:08:05 -0500 Subject: [PATCH 14/25] fix(menu): removed unused map --- packages/menu/src/Menu.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/packages/menu/src/Menu.ts b/packages/menu/src/Menu.ts index 041c1527ad..d94427ff77 100644 --- a/packages/menu/src/Menu.ts +++ b/packages/menu/src/Menu.ts @@ -652,13 +652,6 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { } } } - selectedItems.map((item, i) => { - // When there is more than one "selected" item, - // ensure only the first one can be "focused" - if (i > 0) { - //item.focused = false; - } - }); this.selectedItemsMap = selectedItemsMap; this._selected = selected; this.selectedItems = selectedItems; From 9f5f16c5a73782fb2cd13b4fa5cb7ba338b95491 Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Wed, 29 Jan 2025 13:05:53 -0500 Subject: [PATCH 15/25] test(menu): updated test to match changed group id --- packages/menu/test/menu-group.test.ts | 182 +++++++++++++------------- 1 file changed, 89 insertions(+), 93 deletions(-) diff --git a/packages/menu/test/menu-group.test.ts b/packages/menu/test/menu-group.test.ts index bf9e1a9a17..98f2fefa09 100644 --- a/packages/menu/test/menu-group.test.ts +++ b/packages/menu/test/menu-group.test.ts @@ -43,28 +43,7 @@ const focusableItems = (menu: Menu | MenuGroup): MenuItem[] => { describe('Menu group', () => { testForLitDevWarnings( async () => - await fixture( - html` - - - Section Heading - Action 1 - Action 2 - Action 3 - - - - Section Heading - Save - Download - - - ` - ) - ); - it('renders', async () => { - const el = await fixture( - html` + await fixture(html` Section Heading @@ -79,22 +58,40 @@ describe('Menu group', () => { Download - ` - ); + `) + ); + it('renders', async () => { + const el = await fixture(html` + + + Section Heading + Action 1 + Action 2 + Action 3 + + + + Section Heading + Save + Download + + + `); - await waitUntil(() => { - return managedItems(el).length === 5; - }, `expected menu group to manage 5 children, received ${managedItems(el).length} of ${el.childItems.length}`); + await waitUntil( + () => { + return managedItems(el).length === 5; + }, + `expected menu group to manage 5 children, received ${managedItems(el).length} of ${el.childItems.length}` + ); await elementUpdated(el); await expect(el).to.be.accessible(); }); it('manages [slot="header"] content', async () => { - const el = await fixture( - html` - - ` - ); + const el = await fixture(html` + + `); await elementUpdated(el); const slot = el.shadowRoot.querySelector( '[name="header"' @@ -116,46 +113,44 @@ describe('Menu group', () => { expect(header.id).to.equal(''); }); it('handles selects for nested menu groups', async () => { - const el = await fixture( - html` - - First - - Second - - - Multi1 + const el = await fixture(html` + + First + + Second + + + Multi1 + + Multi2 + + + SubInherit1 - Multi2 + SubInherit2 - - SubInherit1 - - SubInherit2 - - - - Single1 - - Single2 - - - - Inherit1 - - Inherit2 - - - - Inherit1 - - Inherit2 - - - - ` - ); + + + Single1 + + Single2 + + + + Inherit1 + + Inherit2 + + + + Inherit1 + + Inherit2 + + + + `); // 1 & 3 should be menuitemradio // 2 shouwl menuitemcheckbox @@ -317,22 +312,20 @@ describe('Menu group', () => { }); it('handles changing managed items for selects changes', async () => { - const el = await fixture( - html` - - First - Second - - Inherit1 - Inherit2 - - SubInherit1 - SubInherit2 - + const el = await fixture(html` + + First + Second + + Inherit1 + Inherit2 + + SubInherit1 + SubInherit2 - - ` - ); + + + `); await waitUntil( () => managedItems(el).length == 6, @@ -381,9 +374,12 @@ describe('Menu group', () => { await elementUpdated(inheritGroup); await elementUpdated(el); - await waitUntil(() => { - return managedItems(inheritGroup).length === 4; - }, `expected new single sub-group to manage 4 items, received ${managedItems(inheritGroup).length} because "selects === ${inheritGroup.selects}`); + await waitUntil( + () => { + return managedItems(inheritGroup).length === 4; + }, + `expected new single sub-group to manage 4 items, received ${managedItems(inheritGroup).length} because "selects === ${inheritGroup.selects}` + ); await waitUntil( () => managedItems(el).length === 2, @@ -419,13 +415,13 @@ describe('Menu group', () => { items.i6 = el.renderRoot.querySelector('#i-6') as MenuItem; items.i7 = el.renderRoot.querySelector('#i-7') as MenuItem; const group = el.renderRoot.querySelector( - '#group' + '#complex-slotted-group' ) as ComplexSlottedGroup; - items.i1 = group.renderRoot.querySelector('#i-1') as MenuItem; - items.i4 = group.renderRoot.querySelector('#i-4') as MenuItem; - items.i10 = group.renderRoot.querySelector('#i-10') as MenuItem; - items.i11 = group.renderRoot.querySelector('#i-11') as MenuItem; - items.i12 = group.renderRoot.querySelector('#i-12') as MenuItem; + items.i1 = group?.renderRoot.querySelector('#i-1') as MenuItem; + items.i4 = group?.renderRoot.querySelector('#i-4') as MenuItem; + items.i10 = group?.renderRoot.querySelector('#i-10') as MenuItem; + items.i11 = group?.renderRoot.querySelector('#i-11') as MenuItem; + items.i12 = group?.renderRoot.querySelector('#i-12') as MenuItem; const rect = items.i9.getBoundingClientRect(); await sendMouse({ From 01be995d7eb7094894fab89e64ce6d3814c35f5d Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Wed, 29 Jan 2025 13:36:40 -0500 Subject: [PATCH 16/25] test(menu): keyboard selection is fixed --- packages/menu/test/submenu.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/menu/test/submenu.test.ts b/packages/menu/test/submenu.test.ts index 4365b8d60b..8f91714c9b 100644 --- a/packages/menu/test/submenu.test.ts +++ b/packages/menu/test/submenu.test.ts @@ -552,7 +552,7 @@ describe('Submenu', () => { this.rootItem = this.el.querySelector('.root') as MenuItem; await elementUpdated(this.rootItem); }); - describe.skip('selects', () => { + describe('selects', () => { selectWithPointer(); selectsWithKeyboardData.map((testData) => { selectsWithKeyboard(testData); From c5a9514427fd166eee884b5d357410434fcf0eda Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Wed, 29 Jan 2025 15:25:27 -0500 Subject: [PATCH 17/25] test(menu): updated to reflect WAI ARIA APG --- packages/menu/test/submenu.test.ts | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/packages/menu/test/submenu.test.ts b/packages/menu/test/submenu.test.ts index 8f91714c9b..1f13318983 100644 --- a/packages/menu/test/submenu.test.ts +++ b/packages/menu/test/submenu.test.ts @@ -130,14 +130,17 @@ describe('Submenu', () => { }); await opened; + const rootItem = this.el.querySelector('.root') as MenuItem; let submenu = this.el.querySelector('[slot="submenu"]') as Menu; let submenuItem = this.el.querySelector( - '.submenu-item-2' + '.submenu-item-1' ) as MenuItem; expect(this.rootItem.open).to.be.true; + + //opening a menu via keyboard should set focus on first item expect( - submenu === document.activeElement, + submenuItem === document.activeElement, `${document.activeElement?.id}` ).to.be.true; @@ -148,8 +151,10 @@ describe('Submenu', () => { await closed; expect(this.rootItem.open).to.be.false; + + //closing a submenu via keyboard should set focus on its parent menuitem expect( - this.el === document.activeElement, + rootItem === document.activeElement, `${document.activeElement?.id}` ).to.be.true; @@ -160,9 +165,10 @@ describe('Submenu', () => { await opened; submenu = this.el.querySelector('[slot="submenu"]') as Menu; - submenuItem = this.el.querySelector('.submenu-item-2') as MenuItem; expect(this.rootItem.open).to.be.true; + expect(submenuItem.focused).to.be.true; + expect(document.activeElement === submenuItem).to.be.true; await sendKeys({ press: 'ArrowDown', @@ -170,9 +176,9 @@ describe('Submenu', () => { await elementUpdated(submenu); await elementUpdated(submenuItem); - expect(submenu.getAttribute('aria-activedescendant')).to.equal( - submenuItem.id - ); + submenuItem = this.el.querySelector('.submenu-item-2') as MenuItem; + expect(submenuItem.focused).to.be.true; + expect(document.activeElement === submenuItem).to.be.true; closed = oneEvent(this.rootItem, 'sp-closed'); await sendKeys({ From a949c615f15a9a775d78a956e46db06f051f9e7c Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Wed, 29 Jan 2025 15:35:18 -0500 Subject: [PATCH 18/25] fix(picker): picker should delegate focus --- packages/picker/src/Picker.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/picker/src/Picker.ts b/packages/picker/src/Picker.ts index 9d38aed057..60d234c0ce 100644 --- a/packages/picker/src/Picker.ts +++ b/packages/picker/src/Picker.ts @@ -70,6 +70,11 @@ const chevronClass = { export const DESCRIPTION_ID = 'option-picker'; export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { + static override shadowRootOptions = { + ...Focusable.shadowRootOptions, + delegatesFocus: true, + }; + public isMobile = new MatchMediaController(this, IS_MOBILE); public strategy!: DesktopController | MobileController; From 62147ea6ff7d712e081bb063368a97e6e5d651ed Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Wed, 29 Jan 2025 18:32:29 -0500 Subject: [PATCH 19/25] fix(action-menu): fixed a11y error message --- packages/action-menu/src/ActionMenu.ts | 28 ++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/packages/action-menu/src/ActionMenu.ts b/packages/action-menu/src/ActionMenu.ts index 24615f07dc..e167887b12 100644 --- a/packages/action-menu/src/ActionMenu.ts +++ b/packages/action-menu/src/ActionMenu.ts @@ -135,4 +135,32 @@ export class ActionMenu extends ObserveSlotPresence( } super.update(changedProperties); } + + protected override hasAccessibleLabel(): boolean { + return ( + !!this.label || + !!this.getAttribute('aria-label') || + !!this.getAttribute('aria-labelledby') || + !!this.appliedLabel || + this.hasLabel || + this.labelOnly + ); + } + + protected override warnNoLabel(): void { + window.__swc.warn( + this, + `<${this.localName}> needs one of the following to be accessible:`, + 'https://opensource.adobe.com/spectrum-web-components/components/action-menu/#accessibility', + { + type: 'accessibility', + issues: [ + `an element with a \`for\` attribute referencing the \`id\` of the \`<${this.localName}>\`, or`, + 'value supplied to the "label" attribute, which will be displayed visually as placeholder text', + 'text content supplied in a with slot="label", or, text content supplied in a with slot="label-only"', + 'which will also be displayed visually as placeholder text.', + ], + } + ); + } } From 821107747210d316cf7f1f8e2a00342a2ef01a83 Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Wed, 29 Jan 2025 19:04:46 -0500 Subject: [PATCH 20/25] fix(picker): according to APG ArrowDown should focus on first item --- packages/picker/src/Picker.ts | 73 +++++++++++++++++++++++++---------- 1 file changed, 52 insertions(+), 21 deletions(-) diff --git a/packages/picker/src/Picker.ts b/packages/picker/src/Picker.ts index 60d234c0ce..a7a7acf8d7 100644 --- a/packages/picker/src/Picker.ts +++ b/packages/picker/src/Picker.ts @@ -258,9 +258,19 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { } event.stopPropagation(); event.preventDefault(); - this.toggle(true); + this.keyboardOpen(); }; + protected async keyboardOpen(): Promise { + this.toggle(true); + // timing issue with focus and open state + await Promise.all([ + this.updateComplete, + new Promise((res) => requestAnimationFrame(() => res(true))), + ]); + this.optionsMenu.focus(); + } + protected async setValueFromItem( item: MenuItem, menuChangeEvent?: Event @@ -520,6 +530,19 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { `; } + protected override willUpdate(changes: PropertyValues): void { + super.willUpdate(changes); + if ( + changes.has('open') && + !this.open && + this.focusElement === this.optionsMenu + ) { + this.updateComplete.then(async () => { + this.button.focus(); + }); + } + } + protected override update(changes: PropertyValues): void { if (this.selects) { // Always force `selects` to "single" when set. @@ -564,31 +587,39 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { // However, `appliesLabel` is applied by external elements that must be update complete as well to be bound appropriately. await new Promise((res) => requestAnimationFrame(res)); await new Promise((res) => requestAnimationFrame(res)); - if ( - !this.label && - !this.getAttribute('aria-label') && - !this.getAttribute('aria-labelledby') && - !this.appliedLabel - ) { - window.__swc.warn( - this, - `<${this.localName}> needs one of the following to be accessible:`, - 'https://opensource.adobe.com/spectrum-web-components/components/picker/#accessibility', - { - type: 'accessibility', - issues: [ - `an element with a \`for\` attribute referencing the \`id\` of the \`<${this.localName}>\`, or`, - 'value supplied to the "label" attribute, which will be displayed visually as placeholder text, or', - 'text content supplied in a with slot="label", which will also be displayed visually as placeholder text.', - ], - } - ); + if (!this.hasAccessibleLabel) { + this.warnNoLabel(); } }); } super.update(changes); } + protected hasAccessibleLabel(): boolean { + return ( + !!this.label || + !!this.getAttribute('aria-label') || + !!this.getAttribute('aria-labelledby') || + !!this.appliedLabel + ); + } + + protected warnNoLabel(): void { + window.__swc.warn( + this, + `<${this.localName}> needs one of the following to be accessible:`, + 'https://opensource.adobe.com/spectrum-web-components/components/picker/#accessibility', + { + type: 'accessibility', + issues: [ + `an element with a \`for\` attribute referencing the \`id\` of the \`<${this.localName}>\`, or`, + 'value supplied to the "label" attribute, which will be displayed visually as placeholder text, or', + 'text content supplied in a with slot="label", which will also be displayed visually as placeholder text.', + ], + } + ); + } + protected bindButtonKeydownListener(): void { this.button.addEventListener('keydown', this.handleKeydown); } @@ -848,7 +879,7 @@ export class Picker extends PickerBase { return; } if (code === 'ArrowUp' || code === 'ArrowDown') { - this.toggle(true); + this.keyboardOpen(); event.preventDefault(); return; } From 66eb2d85cbd3828ae52aefe4d67231e0bc3bf681 Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Thu, 30 Jan 2025 10:33:24 -0500 Subject: [PATCH 21/25] test(action-menu): updated tests for focus delegation and APG keyboard nav --- packages/action-menu/test/action-menu-groups.test.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/action-menu/test/action-menu-groups.test.ts b/packages/action-menu/test/action-menu-groups.test.ts index d6f88f9934..b7a8d338e3 100644 --- a/packages/action-menu/test/action-menu-groups.test.ts +++ b/packages/action-menu/test/action-menu-groups.test.ts @@ -24,11 +24,10 @@ describe('Action Menu - Groups', () => { groupsWithSelects({ onChange: () => {} }) ); - const firstGroup = el.querySelector('sp-menu-group') as HTMLElement; const firstItem = el.querySelector('sp-menu-item') as MenuItem; expect(firstItem.focused).to.be.false; - expect(document.activeElement === firstGroup).to.be.false; + expect(document.activeElement === firstItem).to.be.false; const opened = oneEvent(el, 'sp-opened'); el.focus(); @@ -39,7 +38,7 @@ describe('Action Menu - Groups', () => { expect(firstItem.focused).to.be.true; expect( - document.activeElement === firstGroup, + document.activeElement === firstItem, document.activeElement?.localName ).to.be.true; }); From 7f6b3638c81547279f756214f46bc6c626c04f1a Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Thu, 30 Jan 2025 10:34:13 -0500 Subject: [PATCH 22/25] fix(picker): ensure overlay is open before setting focus --- packages/picker/src/Picker.ts | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/packages/picker/src/Picker.ts b/packages/picker/src/Picker.ts index a7a7acf8d7..4e7fa05ccc 100644 --- a/packages/picker/src/Picker.ts +++ b/packages/picker/src/Picker.ts @@ -262,13 +262,10 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { }; protected async keyboardOpen(): Promise { + this.addEventListener('sp-opened', () => this.optionsMenu.focus(), { + once: true, + }); this.toggle(true); - // timing issue with focus and open state - await Promise.all([ - this.updateComplete, - new Promise((res) => requestAnimationFrame(() => res(true))), - ]); - this.optionsMenu.focus(); } protected async setValueFromItem( From d4f7f9353546c12a730c657978e832961a7d8014 Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Thu, 30 Jan 2025 10:34:42 -0500 Subject: [PATCH 23/25] fix(menu): removed extra focus --- packages/menu/src/Menu.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/menu/src/Menu.ts b/packages/menu/src/Menu.ts index d94427ff77..18f62a01e9 100644 --- a/packages/menu/src/Menu.ts +++ b/packages/menu/src/Menu.ts @@ -372,7 +372,6 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { return; } this.rovingTabindexController.focus({ preventScroll }); - super.focus({ preventScroll }); const selectedItem = this.selectedItems[0]; if (selectedItem && !preventScroll) { selectedItem.scrollIntoView({ block: 'nearest' }); From 341141305a32238a5e7f097bd39ff9940e7a707c Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Thu, 30 Jan 2025 11:28:52 -0500 Subject: [PATCH 24/25] chore: added changeset --- .changeset/lemon-points-ring.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 .changeset/lemon-points-ring.md diff --git a/.changeset/lemon-points-ring.md b/.changeset/lemon-points-ring.md new file mode 100644 index 0000000000..1a3222cdc5 --- /dev/null +++ b/.changeset/lemon-points-ring.md @@ -0,0 +1,16 @@ +--- +'@spectrum-web-components/reactive-controllers': minor +'@spectrum-web-components/action-menu': minor +'@spectrum-web-components/picker': minor +'@spectrum-web-components/menu': minor +--- + +Used WAI ARIA Authoring Practices Guide (APG) to make accessibility improvements for ``, ``, and ``, including: + - Numpad keys now work with `` and `` + -``'s `` elements can now be read by a screen reader ([#4556](https://github.com/adobe/spectrum-web-components/issues/4556)) + - `` href can now be clicked by a screen reader ([#4997](https://github.com/adobe/spectrum-web-components/issues/4997)) + - Opening a ``, ``, and `` with a keyboard now sets focus on an item within the menu. ([#4557](https://github.com/adobe/spectrum-web-components/issues/4557)) + + See the following APG examples for more information: + - [Navigation Menu Example](https://www.w3.org/WAI/ARIA/apg/patterns/menubar/examples/menubar-navigation/) + - [Editor Menubar Example](https://www.w3.org/WAI/ARIA/apg/patterns/menubar/examples/menubar-editor/) From 645342d67f3ae1f8614229d2f2d4877eda659156 Mon Sep 17 00:00:00 2001 From: Nikki Massaro Date: Thu, 30 Jan 2025 17:42:11 -0500 Subject: [PATCH 25/25] fix(picker): prevents the picker button from getting focus when menu is first updated --- packages/picker/src/Picker.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/picker/src/Picker.ts b/packages/picker/src/Picker.ts index 39cc46b7ad..50054574b5 100644 --- a/packages/picker/src/Picker.ts +++ b/packages/picker/src/Picker.ts @@ -541,6 +541,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { changes.has('open') && !this.open && this.focusElement === this.optionsMenu + && this.focusElement?.matches(':focus-within') ) { this.updateComplete.then(async () => { this.button.focus();