diff --git a/.changeset/lemon-points-ring.md b/.changeset/lemon-points-ring.md new file mode 100644 index 00000000000..1a3222cdc5b --- /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/) diff --git a/.circleci/config.yml b/.circleci/config.yml index 79d62830f6d..3996f8e9e0a 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -14,7 +14,7 @@ parameters: # 3. Commit this change to the PR branch where the changes exist. current_golden_images_hash: type: string - default: be83a254993526d25ab8f6dd32639eae6db1c7eb + default: 34923d02e4b68736f367f04be8d5a4f3843bc091 wireit_cache_name: type: string default: wireit diff --git a/packages/action-group/src/ActionGroup.ts b/packages/action-group/src/ActionGroup.ts index b9e6a2d22a3..0aa07964da8 100644 --- a/packages/action-group/src/ActionGroup.ts +++ b/packages/action-group/src/ActionGroup.ts @@ -22,7 +22,7 @@ import { property, query, } from '@spectrum-web-components/base/src/decorators.js'; -import type { ActionButton } from '@spectrum-web-components/action-button'; +import { ActionButton } from '@spectrum-web-components/action-button'; import { RovingTabindexController } from '@spectrum-web-components/reactive-controllers/src/RovingTabindex.js'; import { MutationController } from '@lit-labs/observers/mutation-controller.js'; @@ -40,6 +40,10 @@ export class ActionGroup extends SizedMixin(SpectrumElement, { validSizes: ['xs', 's', 'm', 'l', 'xl'], noDefaultSize: true, }) { + static override shadowRootOptions = { + ...SpectrumElement.shadowRootOptions, + delegatesFocus: true, + }; public static override get styles(): CSSResultArray { return [styles]; } @@ -90,6 +94,7 @@ export class ActionGroup extends SizedMixin(SpectrumElement, { : firstEnabledIndex; }, elements: () => this.buttons, + hostDelegatesFocus: true, isFocusableElement: (el: ActionButton) => !el.disabled, } ); @@ -245,7 +250,8 @@ export class ActionGroup extends SizedMixin(SpectrumElement, { const selections: ActionButton[] = []; const updates = options.map(async (option) => { await option.updateComplete; - option.setAttribute('role', 'radio'); + if (option instanceof ActionButton) + option.setAttribute('role', 'radio'); option.setAttribute( 'aria-checked', option.selected ? 'true' : 'false' @@ -273,7 +279,8 @@ export class ActionGroup extends SizedMixin(SpectrumElement, { const selections: ActionButton[] = []; const updates = options.map(async (option) => { await option.updateComplete; - option.setAttribute('role', 'checkbox'); + if (option instanceof ActionButton) + option.setAttribute('role', 'checkbox'); option.setAttribute( 'aria-checked', option.selected ? 'true' : 'false' @@ -297,7 +304,8 @@ export class ActionGroup extends SizedMixin(SpectrumElement, { const selections: ActionButton[] = []; const updates = options.map(async (option) => { await option.updateComplete; - option.setAttribute('role', 'button'); + if (option instanceof ActionButton) + option.setAttribute('role', 'button'); if (option.selected) { option.setAttribute('aria-pressed', 'true'); selections.push(option); @@ -315,7 +323,8 @@ export class ActionGroup extends SizedMixin(SpectrumElement, { ); } else { this.buttons.forEach((option) => { - option.setAttribute('role', 'button'); + if (option instanceof ActionButton) + option.setAttribute('role', 'button'); }); break; } diff --git a/packages/action-group/test/action-group.test.ts b/packages/action-group/test/action-group.test.ts index d7bdcb8fd40..672321a5c77 100644 --- a/packages/action-group/test/action-group.test.ts +++ b/packages/action-group/test/action-group.test.ts @@ -22,8 +22,8 @@ import { } from '@open-wc/testing'; import { ActionButton } from '@spectrum-web-components/action-button'; -import { MenuItem } from '@spectrum-web-components/menu'; import { ActionMenu } from '@spectrum-web-components/action-menu'; +import { MenuItem } from '@spectrum-web-components/menu'; import '@spectrum-web-components/action-button/sp-action-button.js'; import '@spectrum-web-components/action-menu/sp-action-menu.js'; import '@spectrum-web-components/menu/sp-menu.js'; @@ -53,7 +53,6 @@ import { spy } from 'sinon'; import { sendMouse } from '../../../test/plugins/browser.js'; import { HasActionMenuAsChild } from '../stories/action-group.stories.js'; import '../stories/action-group.stories.js'; -import { isWebKit } from '@spectrum-web-components/shared'; import sinon from 'sinon'; class QuietActionGroup extends LitElement { @@ -190,13 +189,28 @@ describe('ActionGroup', () => { await elementUpdated(el); // expect the first button to be focused - expect(document.activeElement?.id).to.equal('first'); + expect( + document.activeElement?.id, + 'should be focused on the first button' + ).to.equal('first'); // expect all the elements of the focus group to have a tabIndex of -1 except the first button because it is focused using Tab - expect((el.children[0] as ActionButton)?.tabIndex).to.equal(0); - expect((el.children[1] as ActionButton)?.tabIndex).to.equal(-1); - expect((el.children[2] as ActionButton)?.tabIndex).to.equal(-1); - expect((el.children[3] as ActionMenu)?.tabIndex).to.equal(-1); + expect( + (el.children[0] as ActionButton)?.tabIndex, + 'should be focused on the first button' + ).to.equal(0); + expect( + (el.children[1] as ActionButton)?.tabIndex, + 'should not be focused on the second button' + ).to.equal(-1); + expect( + (el.children[2] as ActionButton)?.tabIndex, + 'should not be focused on the third button' + ).to.equal(-1); + expect( + (el.children[3] as ActionMenu)?.tabIndex, + 'should not be focused on the fourth button' + ).to.equal(-1); // navigate to the action-menu using the arrow keys await sendKeys({ press: 'ArrowRight' }); @@ -206,39 +220,35 @@ describe('ActionGroup', () => { await elementUpdated(el); // expect the action-menu to be focused - expect((el.children[3] as ActionMenu)?.focused).to.be.true; + expect(el.children[3]).to.equal(document.activeElement); // press Enter to open the action-menu await sendKeys({ press: 'Enter' }); - const opened = oneEvent(el.children[3] as ActionMenu, 'sp-opened'); + let opened = oneEvent(el.children[3] as ActionMenu, 'sp-opened'); await elementUpdated(el.children[3]); await opened; // expect the first menu item to be focused - const firstMenuItem = el.querySelector('#first-menu-item') as MenuItem; - expect(firstMenuItem?.focused).to.be.true; - + const firstMenuItem = el.querySelector('#first-menu-item'); + const fourthMenuItem = el.querySelector('#fourth-menu-item'); + expect(firstMenuItem).to.equal(document.activeElement); // navigate to the fourth menu item using the arrow keys await sendKeys({ press: 'ArrowDown' }); await sendKeys({ press: 'ArrowDown' }); await sendKeys({ press: 'ArrowDown' }); + opened = oneEvent(fourthMenuItem as MenuItem, 'sp-opened'); + // press Enter to select the fourth menu item await sendKeys({ press: 'Enter' }); - - await elementUpdated(el.children[3]); - - await nextFrame(); - await nextFrame(); - await nextFrame(); - await nextFrame(); + await opened; // expect the second submenu item to be focused const secondSubMenuItem = el.querySelector( '#second-sub-menu-item' ) as MenuItem; - expect(secondSubMenuItem?.focused).to.be.true; + expect(secondSubMenuItem).to.equal(document.activeElement); // press Enter to select the second submenu item await sendKeys({ press: 'Enter' }); @@ -249,7 +259,7 @@ describe('ActionGroup', () => { await closed; // expect the action-menu to be focused - expect((el.children[3] as ActionMenu)?.focused).to.be.true; + expect(el.children[3]).to.equal(document.activeElement); }); it('action-group with action-menu manages tabIndex correctly while using mouse', async () => { @@ -280,10 +290,22 @@ describe('ActionGroup', () => { await aTimeout(500); // expect all the elements of the focus group to have a tabIndex of -1 except the first button because it is focused using mouse - expect((el.children[0] as ActionButton)?.tabIndex).to.equal(0); - expect((el.children[1] as ActionButton)?.tabIndex).to.equal(-1); - expect((el.children[2] as ActionButton)?.tabIndex).to.equal(-1); - expect((el.children[3] as ActionMenu)?.tabIndex).to.equal(-1); + expect( + (el.children[0] as ActionButton)?.tabIndex, + 'mouse1: should be focused on the first button' + ).to.equal(0); + expect( + (el.children[1] as ActionButton)?.tabIndex, + 'mouse1: should not be focused on the second button' + ).to.equal(-1); + expect( + (el.children[2] as ActionButton)?.tabIndex, + 'mouse1: should not be focused on the third button' + ).to.equal(-1); + expect( + (el.children[3] as ActionMenu)?.tabIndex, + 'mouse1: should not be focused on the fourth button' + ).to.equal(-1); // click outside the action-group and it should loose focus and update the tabIndexes sendMouse({ @@ -299,10 +321,22 @@ describe('ActionGroup', () => { await aTimeout(500); // expect the first button to have a tabIndex of 0 and everything else to have a tabIndex of -1 - expect((el.children[0] as ActionButton)?.tabIndex).to.equal(0); - expect((el.children[1] as ActionButton)?.tabIndex).to.equal(-1); - expect((el.children[2] as ActionButton)?.tabIndex).to.equal(-1); - expect((el.children[3] as ActionMenu)?.tabIndex).to.equal(-1); + expect( + (el.children[0] as ActionButton)?.tabIndex, + 'mouse2: should be focused on the first button' + ).to.equal(0); + expect( + (el.children[1] as ActionButton)?.tabIndex, + 'mouse2: should not be focused on the second button' + ).to.equal(-1); + expect( + (el.children[2] as ActionButton)?.tabIndex, + 'mouse2: should not be focused on the third button' + ).to.equal(-1); + expect( + (el.children[3] as ActionMenu)?.tabIndex, + 'mouse2: should not be focused on the fourth button' + ).to.equal(-1); // get the bounding box of the action-menu const actionMenu = el.querySelector('#action-menu') as ActionMenu; @@ -331,23 +365,23 @@ describe('ActionGroup', () => { const closed = oneEvent(el.children[3] as ActionMenu, 'sp-closed'); await closed; - if (!isWebKit()) { - sendMouse({ - steps: [ - { - position: [0, 0], - type: 'click', - }, - ], - }); - } - await aTimeout(500); - - expect((el.children[0] as ActionButton)?.tabIndex).to.equal(0); - expect((el.children[1] as ActionButton)?.tabIndex).to.equal(-1); - expect((el.children[2] as ActionButton)?.tabIndex).to.equal(-1); - expect((el.children[3] as ActionMenu)?.tabIndex).to.equal(-1); + expect( + (el.children[0] as ActionButton)?.tabIndex, + 'final: should NOT be focused on the first button' + ).to.equal(-1); + expect( + (el.children[1] as ActionButton)?.tabIndex, + 'final: should not be focused on the second button' + ).to.equal(-1); + expect( + (el.children[2] as ActionButton)?.tabIndex, + 'final: should not be focused on the third button' + ).to.equal(-1); + expect( + (el.children[3] as ActionMenu)?.tabIndex, + 'final: should be be focused on the fourth button' + ).to.equal(0); }); testForLitDevWarnings( diff --git a/packages/action-menu/src/ActionMenu.ts b/packages/action-menu/src/ActionMenu.ts index 24615f07dca..e167887b128 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.', + ], + } + ); + } } diff --git a/packages/action-menu/stories/action-menu.stories.ts b/packages/action-menu/stories/action-menu.stories.ts index 762f1483903..fdb17743550 100644 --- a/packages/action-menu/stories/action-menu.stories.ts +++ b/packages/action-menu/stories/action-menu.stories.ts @@ -406,6 +406,7 @@ export const groups = ({ onChange(value: string): void; }): TemplateResult => html` onChange(value)} open @@ -540,3 +541,24 @@ export const withScrollEvent = (): TemplateResult => { withScrollEvent.parameters = { chromatic: { disableSnapshot: true }, }; + +export const MenuItemAlerts = (): TemplateResult => html` + + More Actions + alert('Deselect')}>Deselect + alert('Select inverse')}> + Select inverse + + alert('Feather...')}> + Feather... + + alert('Select and mask...')}> + Select and mask... + + + alert('Save selection')}> + Save selection + + Make work path + +`; diff --git a/packages/action-menu/test/action-menu-directive.test.ts b/packages/action-menu/test/action-menu-directive.test.ts index 565ba568613..caac6629244 100644 --- a/packages/action-menu/test/action-menu-directive.test.ts +++ b/packages/action-menu/test/action-menu-directive.test.ts @@ -22,17 +22,20 @@ describe('Slottable Request Directive', () => { it('Action Menu requests for options rendering when opening and closing', async function () { const el = await fixture(directive()); const initialNodeLength = el.children.length; - expect(el.open).to.be.false; + + expect(el.open, 'should be closed initially').to.be.false; expect(el.children.length).to.equal(initialNodeLength); const opened = oneEvent(el, 'sp-opened'); + el.click(); await opened; - expect(el.open).to.be.true; + expect(el.open, 'should be open after clicking').to.be.true; expect(el.children.length).to.be.gt(initialNodeLength); const closed = oneEvent(el, 'sp-closed'); + await sendKeys({ press: 'Escape', }); @@ -40,7 +43,8 @@ describe('Slottable Request Directive', () => { await nextFrame(); await nextFrame(); - expect(el.open).to.be.false; + expect(el.open, 'should be closed after escape key is pressed').to.be + .false; expect(el.children.length).to.equal(initialNodeLength); }); }); diff --git a/packages/action-menu/test/action-menu-groups.test.ts b/packages/action-menu/test/action-menu-groups.test.ts index d6f88f9934b..0d3059bf0c6 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; }); @@ -57,7 +56,7 @@ describe('Action Menu - Groups', () => { 'sp-menu-item' ) as MenuItem; - expect(firstItem.selected).to.be.false; + expect(firstItem.selected, 'before opening: first item selected?').to.be.false; let opened = oneEvent(el, 'sp-opened'); el.focus(); @@ -65,7 +64,7 @@ describe('Action Menu - Groups', () => { press: 'ArrowDown', }); await opened; - expect(el.open).to.be.true; + expect(el.open, 'first opened: open?').to.be.true; await sendKeys({ press: 'ArrowUp', @@ -81,8 +80,8 @@ describe('Action Menu - Groups', () => { await elementUpdated(el); await elementUpdated(firstItem); - expect(el.open).to.be.false; - expect(firstItem.selected).to.be.true; + expect(el.open, 'first closed: open?').to.be.false; + expect(firstItem.selected, 'after select: first item selected?').to.be.true; expect(document.activeElement === el, document.activeElement?.localName) .to.be.true; @@ -91,12 +90,7 @@ describe('Action Menu - Groups', () => { press: 'ArrowDown', }); await opened; - expect(el.open).to.be.true; - - await sendKeys({ - press: 'ArrowUp', - }); - await elementUpdated(el); + expect(el.open, 'reopened: open?').to.be.true; closed = oneEvent(el, 'sp-closed'); await sendKeys({ @@ -107,7 +101,7 @@ describe('Action Menu - Groups', () => { await elementUpdated(el); await elementUpdated(firstItem); - expect(el.open).to.be.false; - expect(firstItem.selected).to.be.false; + expect(el.open, 'reclosed: open?').to.be.false; + expect(firstItem.selected, 'after deselect: first item selected?').to.be.false; }); }); diff --git a/packages/action-menu/test/index.ts b/packages/action-menu/test/index.ts index 092b213098f..9c7a0977a03 100644 --- a/packages/action-menu/test/index.ts +++ b/packages/action-menu/test/index.ts @@ -17,6 +17,7 @@ import { html, nextFrame, oneEvent, + waitUntil } from '@open-wc/testing'; import { testForLitDevWarnings } from '../../../test/testing-helpers'; @@ -38,7 +39,11 @@ import type { Tooltip } from '@spectrum-web-components/tooltip'; import { sendMouse } from '../../../test/plugins/browser.js'; import type { TestablePicker } from '../../picker/test/index.js'; import type { Overlay } from '@spectrum-web-components/overlay'; -import { sendKeys, setViewport } from '@web/test-runner-commands'; +import { + a11ySnapshot, + findAccessibilityNode, + sendKeys, + setViewport } from '@web/test-runner-commands'; import { TemplateResult } from '@spectrum-web-components/base'; import { isWebKit } from '@spectrum-web-components/shared'; import { SAFARI_FOCUS_RING_CLASS } from '@spectrum-web-components/picker/src/InteractionController.js'; @@ -142,6 +147,34 @@ export const testActionMenu = (mode: 'sync' | 'async'): void => { await expect(el).to.be.accessible(); }); + it('has menuitems in accessibility tree', async() => { + const el = await fixture(html` + + Deselect + Make Work Path + + `); + const opened = oneEvent(el, 'sp-opened'); + el.focus(); + sendKeys({ press: 'Enter' }); + await opened; + await nextFrame(); + + + type NamedNode = { name: string, role: string, disabled: boolean }; + const snapshot = (await a11ySnapshot({})) as unknown as NamedNode & { + children: NamedNode[]; + }; + const button = findAccessibilityNode(snapshot, (node) => node.name === 'More Actions'); + const menu = findAccessibilityNode(snapshot, (node) => node.role === 'menu'); + const deselect = findAccessibilityNode(snapshot, (node) => node.role === 'menuitem' && node.name === 'Deselect'); + const workPath = findAccessibilityNode(snapshot, (node) => node.role === 'menuitem' && node.name === 'Make Work Path' && node.disabled); + expect(button, 'button').to.not.be.null; + expect(menu, 'menu').to.not.be.null; + expect(deselect, 'first menuitem').to.not.be.null; + expect(workPath, 'second menuitem').to.not.be.null; + }); it('dispatches change events, no [href]', async () => { const changeSpy = spy(); @@ -404,35 +437,33 @@ export const testActionMenu = (mode: 'sync' | 'async'): void => { }) ); - await elementUpdated(el); + expect(el.open, 'open?').to.be.false; - el.focus(); - await elementUpdated(el); let opened = oneEvent(el, 'sp-opened'); - await sendKeys({ press: 'ArrowRight' }); - await sendKeys({ press: 'ArrowLeft' }); - await sendKeys({ press: 'Space' }); + el.click(); await opened; + expect(el.open, 'open?').to.be.true; + const firstRect = ( el as unknown as { overlayElement: Overlay } - ).overlayElement.dialogEl.getBoundingClientRect(); + )?.overlayElement?.dialogEl?.getBoundingClientRect(); - let closed = oneEvent(el, 'sp-closed'); - await sendKeys({ press: 'Space' }); + const closed = oneEvent(el, 'sp-closed'); + el.close(); await closed; + expect(el.open, 'open?').to.be.false; opened = oneEvent(el, 'sp-opened'); - await sendKeys({ press: 'Space' }); + el.toggle(); await opened; + expect(el.open, 'open?').to.be.true; const secondRect = ( el as unknown as { overlayElement: Overlay } - ).overlayElement.dialogEl.getBoundingClientRect(); + )?.overlayElement?.dialogEl?.getBoundingClientRect(); - closed = oneEvent(el, 'sp-closed'); - await sendKeys({ press: 'Space' }); - await closed; + el.close(); expect(firstRect).to.deep.equal(secondRect); }); @@ -486,6 +517,44 @@ export const testActionMenu = (mode: 'sync' | 'async'): void => { expect(el.open).to.be.false; expect(selected).to.equal(thirdItem.value); }); + it('returns focus on `Escape`', async () => { + const el = await actionMenuFixture(); + const thirdItem = el.querySelector( + 'sp-menu-item:nth-of-type(3)' + ) as MenuItem; + + expect(el.value).to.not.equal(thirdItem.value); + const opened = oneEvent(el, 'sp-opened'); + el.focus(); + await sendKeys({ press: 'Enter'}) + await opened; + + await sendKeys({ press: 'Escape'}); + await waitUntil( + () => document.activeElement === el, + 'focused', { timeout: 300 } + ); + expect(el.open).to.be.false; + }); + it('returns focus on select', async () => { + const el = await actionMenuFixture(); + const thirdItem = el.querySelector( + 'sp-menu-item:nth-of-type(3)' + ) as MenuItem; + + expect(el.value).to.not.equal(thirdItem.value); + const opened = oneEvent(el, 'sp-opened'); + el.focus(); + await sendKeys({ press: 'Enter'}) + await opened; + + thirdItem.click(); + await waitUntil( + () => document.activeElement === el, + 'focused', { timeout: 300 } + ); + expect(el.open).to.be.false; + }); it('has attribute aria-describedby', async () => { const name = 'sp-picker'; const description = 'Rendering a Picker'; @@ -581,94 +650,86 @@ export const testActionMenu = (mode: 'sync' | 'async'): void => { 'initially selected item should maintain selection' ).to.be.true; }); - it('allows top-level selection state to change', async () => { - let selected = true; - const handleChange = ( - event: Event & { target: ActionMenu } - ): void => { - if (event.target.value === 'test') { - selected = !selected; - - event.target.updateComplete.then(() => { - event.target.value = selected ? 'test' : ''; - }); - } - }; + it('does not alter submenu selection when top-level menu items are selected', async () => { const root = await fixture(html` - - One - - Two - - - B should be selected - - A - - B - - C + + One + + Two, with B selected + + A + B `); - const unselectedItem = root.querySelector( - 'sp-menu-item' - ) as MenuItem; - const selectedItem = root.querySelector( - '#root-selected-item' - ) as MenuItem; - - expect(unselectedItem.textContent).to.include('One'); - expect(unselectedItem.selected).to.be.false; - expect(selectedItem.textContent).to.include('Two'); - expect(selectedItem.selected).to.be.true; + const item1 = root.querySelector('#item-1') as MenuItem; + const item2 = root.querySelector('#item-2') as MenuItem; + const itemA = root.querySelector('#item-2a') as MenuItem; + const itemB = root.querySelector('#item-2b') as MenuItem; let opened = oneEvent(root, 'sp-opened'); + + expect(item1.selected, 'before opening: item1 selected?').to.be + .false; + expect(item2.selected, 'before opening: item2 selected?').to.be + .false; + expect(itemA.selected, 'before opening: itemA selected?').to.be + .true; + expect(item2.selected, 'before opening: itemB selected?').to.be + .false; root.click(); await opened; - // close by clicking selected - // (with event listener: should set selected = false) + expect(root.open, 'after clicking open: open?').to.be.true; + let closed = oneEvent(root, 'sp-closed'); - selectedItem.click(); + item1.click(); await closed; - expect(root.open).to.be.false; + expect(item1.selected, 'after clicking item1: item1 selected?').to + .be.false; + expect(itemA.selected, 'after clicking item1: itemA selected?').to + .be.true; + expect(root.open, 'after clicking item1: open?').to.be.false; + opened = oneEvent(root, 'sp-opened'); root.click(); await opened; - // close by clicking unselected - // (no event listener: should remain selected = false) + expect(root.open, 'after reopening: open?').to.be.true; + closed = oneEvent(root, 'sp-closed'); - unselectedItem.click(); + itemB.click(); + root.close(); await closed; + expect(item1.selected, 'after clicking itemB: item1 selected?').to + .be.false; + expect(item2.selected, 'after clicking itemB: item2 selected?').to + .be.false; + expect(itemA.selected, 'after clicking itemB: itemA selected?').to + .be.false; + expect(itemB.selected, 'after clicking itemB: itemB selected?').to + .be.true; + expect(root.open, 'after clicking itemB: open?').to.be.false; + opened = oneEvent(root, 'sp-opened'); root.click(); await opened; - expect(unselectedItem.textContent).to.include('One'); - expect(unselectedItem.selected).to.be.false; - expect(selectedItem.textContent).to.include('Two'); - expect(selectedItem.selected).to.be.false; + expect(root.open, 'after reopening: open?').to.be.true; - // close by clicking selected - // (with event listener: should set selected = false) closed = oneEvent(root, 'sp-closed'); - selectedItem.click(); + itemB.click(); await closed; - opened = oneEvent(root, 'sp-opened'); - root.click(); - await opened; - - expect(unselectedItem.textContent).to.include('One'); - expect(unselectedItem.selected).to.be.false; - expect(selectedItem.textContent).to.include('Two'); - expect(selectedItem.selected).to.be.true; + expect(item2.selected, 'after clicking item2: item2 selected?').to + .be.false; + expect(itemB.selected, 'after clicking item2: itemB selected?').to + .be.true; + expect(root.open, 'after clicking item2: open?').to.be.false; }); it('shows tooltip', async function () { const openSpy = spy(); diff --git a/packages/combobox/src/Combobox.ts b/packages/combobox/src/Combobox.ts index 509f475034e..b29542640a6 100644 --- a/packages/combobox/src/Combobox.ts +++ b/packages/combobox/src/Combobox.ts @@ -227,6 +227,12 @@ export class Combobox extends Textfield { if (!this.availableOptions[nextActiveIndex].disabled) { this.activeDescendant = this.availableOptions[nextActiveIndex]; } + this.optionEls.forEach((el) => + el.setAttribute( + 'aria-selected', + el.value === this.activeDescendant?.value ? 'true' : 'false' + ) + ); } public activatePreviousDescendant(): void { @@ -245,6 +251,12 @@ export class Combobox extends Textfield { if (!this.availableOptions[previousActiveIndex].disabled) { this.activeDescendant = this.availableOptions[previousActiveIndex]; } + this.optionEls.forEach((el) => + el.setAttribute( + 'aria-selected', + el.value === this.activeDescendant?.value ? 'true' : 'false' + ) + ); } public selectDescendant(): void { @@ -283,9 +295,8 @@ export class Combobox extends Textfield { protected handleMenuChange(event: PointerEvent & { target: Menu }): void { const { target } = event; - const value = target.selected[0]; const selected = (this.options || this.optionEls).find( - (item) => item.value === value + (item) => item.value === target?.value ); this.value = selected?.itemText || ''; event.preventDefault(); diff --git a/packages/combobox/test/combobox-a11y.test.ts b/packages/combobox/test/combobox-a11y.test.ts index 71501f33270..984375aa6d5 100644 --- a/packages/combobox/test/combobox-a11y.test.ts +++ b/packages/combobox/test/combobox-a11y.test.ts @@ -20,7 +20,7 @@ import { import '@spectrum-web-components/combobox/sp-combobox.js'; import { Combobox } from '@spectrum-web-components/combobox'; -import { detectOS, fixture } from '../../../test/testing-helpers.js'; +import { fixture } from '../../../test/testing-helpers.js'; import { findDescribedNode } from '../../../test/testing-helpers-a11y.js'; import { a11ySnapshot, @@ -29,7 +29,6 @@ import { } from '@web/test-runner-commands'; import type { AccessibleNamedNode } from './helpers.js'; import { comboboxFixture } from './helpers.js'; -import { isWebKit } from '@spectrum-web-components/shared'; import { withFieldLabel, withHelpText, @@ -72,9 +71,6 @@ describe('Combobox accessibility', () => {
${withFieldLabel()}
`); const el = test.querySelector('sp-combobox') as unknown as Combobox; - const name = 'Pick something'; - const webkitName = 'Pick something Banana'; - const isWebKitMacOS = isWebKit() && detectOS() === 'Mac OS'; await elementUpdated(el); await nextFrame(); @@ -89,7 +85,7 @@ describe('Combobox accessibility', () => { const a11yNode = findAccessibilityNode( snapshot, (node) => - node.name === name && !node.value && node.role === 'combobox' + node.name === 'Pick something' && !node.value && node.role === 'combobox' ); // by default, is there a combobox that has `name` as the label? expect(a11yNode, '`name` is the label text').to.not.be.null; @@ -102,55 +98,26 @@ describe('Combobox accessibility', () => { )) as unknown as AccessibleNamedNode & { children: AccessibleNamedNode[]; }; - - // WebKit doesn't currently associate the `name` via the accessibility tree. - // Instead if lists this data in the description 🤷🏻‍♂️ - // Give it an escape hatch for now. const node = findAccessibilityNode( snapshot, (node) => - (node.name === name || - (isWebKitMacOS && node.name === webkitName)) && + node.name === 'Pick something' && node.value === 'Banana' && node.role === 'combobox' ); expect( node, - `pre escape hatch node not available: ${JSON.stringify( + `node not available: ${JSON.stringify( snapshot, null, ' ' )}` ).to.not.be.null; - - if (isWebKitMacOS) { - // Retest WebKit without the escape hatch, expecting it to fail. - // This way we get notified when the results are as expected, again. - const iOSNode = findAccessibilityNode( - snapshot, - (node) => - node.name === name && - node.value === 'Banana' && - node.role === 'combobox' - ); - expect( - iOSNode, - `post escape hatch node available: ${JSON.stringify( - snapshot, - null, - ' ' - )}` - ).to.be.null; - } }); it('manages its "name" value in the accessibility tree', async () => { const el = await comboboxFixture(); - const name = 'Combobox'; - const webkitName = 'Combobox Banana'; - const isWebKitMacOS = isWebKit() && detectOS() === 'Mac OS'; - await elementUpdated(el); await nextFrame(); await nextFrame(); @@ -164,7 +131,7 @@ describe('Combobox accessibility', () => { const a11yNode = findAccessibilityNode( snapshot, (node) => - node.name === name && !node.value && node.role === 'combobox' + node.name === 'Combobox' && !node.value && node.role === 'combobox' ); // by default, is there a combobox that has `name` as the label? expect(a11yNode, '`name` is the label text').to.not.be.null; @@ -177,47 +144,22 @@ describe('Combobox accessibility', () => { )) as unknown as AccessibleNamedNode & { children: AccessibleNamedNode[]; }; - - // WebKit doesn't currently associate the `name` via the accessibility tree. - // Instead if lists this data in the description 🤷🏻‍♂️ - // Give it an escape hatch for now. const node = findAccessibilityNode( snapshot, (node) => - (node.name === name || - (isWebKitMacOS && node.name === webkitName)) && + node.name === 'Combobox' && node.value === 'Banana' && node.role === 'combobox' ); expect( node, - `pre escape hatch node not available: ${JSON.stringify( + `node not available: ${JSON.stringify( snapshot, null, ' ' )}` ).to.not.be.null; - - if (isWebKitMacOS) { - // Retest WebKit without the escape hatch, expecting it to fail. - // This way we get notified when the results are as expected, again. - const iOSNode = findAccessibilityNode( - snapshot, - (node) => - node.name === name && - node.value === 'Banana' && - node.role === 'combobox' - ); - expect( - iOSNode, - `post escape hatch node available: ${JSON.stringify( - snapshot, - null, - ' ' - )}` - ).to.be.null; - } }); it('manages its "description" value with slotted ', async () => { const test = await fixture(html` diff --git a/packages/combobox/test/combobox.test.ts b/packages/combobox/test/combobox.test.ts index 0fa53e51293..0c2fe93c709 100644 --- a/packages/combobox/test/combobox.test.ts +++ b/packages/combobox/test/combobox.test.ts @@ -488,27 +488,35 @@ describe('Combobox', () => { await elementUpdated(el); expect(el.value).to.equal(''); expect(el.activeDescendant).to.be.undefined; - expect(el.open).to.be.false; + expect(el.open, 'open?').to.be.false; el.focusElement.focus(); const opened = oneEvent(el, 'sp-opened'); el.focusElement.dispatchEvent(arrowDownEvent()); await opened; - expect(el.open).to.be.true; - - await elementUpdated(el); - el.focusElement.dispatchEvent(arrowDownEvent()); - + expect(el.open, 'open?').to.be.true; await elementUpdated(el); - testActiveElement(el, 'banana'); + expect( + el.activeDescendant?.value, + 'activeDscendant after open?' + ).to.equal('apple'); el.focusElement.dispatchEvent(enterEvent()); await elementUpdated(el); - expect(el.open).to.be.false; - expect(el.activeDescendant).to.be.undefined; - expect(el.value).to.equal('Banana'); - expect(el.focusElement.value).to.equal(el.value); + expect(el.open, 'open?').to.be.false; + expect(el.activeDescendant, 'activeDescendant after Enter?').to.be + .undefined; + expect(el.value, 'value after enter').to.equal('Apple'); + expect( + el.shadowRoot.querySelector( + 'sp-menu-item[aria-selected="true"]' + )?.id, + 'aria-selected' + ).to.equal('apple'); + expect(el.focusElement.value, 'focusElement after enter').to.equal( + el.value + ); }); it('does not set the value when `enter` is pressed and no active descendent', async () => { const el = await comboboxFixture(); @@ -538,9 +546,10 @@ describe('Combobox', () => { await elementUpdated(el); - expect(el.value).to.equal(''); - expect(el.activeDescendant).to.be.undefined; - expect(el.open).to.be.false; + expect(el.value, 'initial value').to.equal(''); + expect(el.activeDescendant, 'initial activeDescendant').to.be + .undefined; + expect(el.open, 'initially open?').to.be.false; const opened = oneEvent(el, 'sp-opened'); el.focusElement.click(); @@ -549,7 +558,7 @@ describe('Combobox', () => { const item = el.shadowRoot.querySelector('#cherry') as HTMLElement; await elementUpdated(item); - expect(el.open).to.be.true; + expect(el.open, 'open after click?').to.be.true; const itemValue = (item.textContent as string).trim(); const rect = item.getBoundingClientRect(); @@ -567,9 +576,10 @@ describe('Combobox', () => { }); await elementUpdated(el); - expect(el.value).to.equal(itemValue); - expect(el.open).to.be.false; - expect(el.activeDescendant).to.be.undefined; + expect(el.value, 'value after item click?').to.equal(itemValue); + expect(el.open, 'open after item click?').to.be.false; + expect(el.activeDescendant, 'activeDescendant after item click').to + .be.undefined; }); it('reflects the selected value in menu on reopening', async () => { const el = await comboboxFixture(); diff --git a/packages/combobox/test/helpers.ts b/packages/combobox/test/helpers.ts index bca89058020..341f6ca7507 100644 --- a/packages/combobox/test/helpers.ts +++ b/packages/combobox/test/helpers.ts @@ -87,9 +87,12 @@ export const testActiveElement = ( el: TestableCombobox, testId: string ): void => { - expect(el.activeDescendant?.value).to.equal(testId); + expect(el.activeDescendant?.value, 'active descendant').to.equal(testId); const activeElement = el.shadowRoot.querySelector( `#${el.activeDescendant.value}` ) as HTMLElement; - expect(activeElement.getAttribute('aria-selected')).to.equal('true'); + expect( + activeElement.getAttribute('aria-selected'), + 'aria-selected' + ).to.equal('true'); }; diff --git a/packages/menu/menu-group.md b/packages/menu/menu-group.md index 4cf9a26c4e4..e434869e53b 100644 --- a/packages/menu/menu-group.md +++ b/packages/menu/menu-group.md @@ -33,7 +33,7 @@ An `` can be used to organize ``s in an ``

Your favorite park in New York is:

- Your favorite park in San Fransisco is: + Your favorite park in San Francisco is:

` can be used to organize ``s in an `` id="group-2" selects="single" > - San Fransisco + San Francisco Golden Gate Park diff --git a/packages/menu/menu-item.md b/packages/menu/menu-item.md index 752564b7b55..7cf93ac519d 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 diff --git a/packages/menu/src/Menu.ts b/packages/menu/src/Menu.ts index 5a64c94aa02..4c1076253e3 100644 --- a/packages/menu/src/Menu.ts +++ b/packages/menu/src/Menu.ts @@ -24,9 +24,13 @@ 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'; export interface MenuChildItem { menuItem: MenuItem; @@ -39,13 +43,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 @@ -64,19 +61,42 @@ 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'; } + 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 = ''; @@ -85,10 +105,12 @@ 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; + return !this.selects ? [] : this._selected; } public set selected(selected: string[]) { @@ -114,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[]; @@ -124,8 +149,19 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { public focusedItemIndex = 0; public focusInItemIndex = 0; + public get focusInItem(): MenuItem | undefined { + return this.rovingTabindexController?.focusInElement; + } + + protected get controlsRovingTabindex(): boolean { + return true; + } + private selectedItemsMap = new Map(); + /** + * child items managed by menu + */ public get childItems(): MenuItem[] { if (!this.cachedChildItems) { this.cachedChildItems = this.updateCachedMenuItems(); @@ -136,12 +172,10 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { private cachedChildItems: MenuItem[] | undefined; private updateCachedMenuItems(): MenuItem[] { - this.cachedChildItems = []; - if (!this.menuSlot) { return []; } - + const itemsList = []; const slottedElements = this.menuSlot.assignedElements({ flatten: true, }) as HTMLElement[]; @@ -149,7 +183,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { for (const [i, slottedElement] of slottedElements.entries()) { if (this.childItemSet.has(slottedElement as MenuItem)) { // Assign members of the array that are in this.childItemSet to this.chachedChildItems. - this.cachedChildItems.push(slottedElement as MenuItem); + itemsList.push(slottedElement as MenuItem); continue; } const isHTMLSlotElement = slottedElement.localName === 'slot'; @@ -166,6 +200,9 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { ); } + this.cachedChildItems = [...itemsList]; + this.rovingTabindexController?.clearElementCache(); + return this.cachedChildItems; } @@ -194,7 +231,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; /** @@ -232,10 +276,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') { @@ -259,6 +299,10 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { this.resolvedRole === 'none' ? 'ignore' : 'none'; } + if (this.resolvedRole === 'none') { + return; + } + const selects = this.resolvedSelects === 'single' || this.resolvedSelects === 'multiple'; @@ -287,18 +331,48 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { } private async removeChildItem(item: MenuItem): Promise { + if (item.focused || item.hasAttribute('focused') || item.active) { + this._updateFocus = this.getNeighboringFocusableElement(item); + } this.childItemSet.delete(item); this.cachedChildItems = undefined; - if (item.focused) { - this.handleItemsChanged(); - await this.updateComplete; - this.focus(); - } } public constructor() { super(); + /** + * 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, { + direction: 'vertical', + 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.childItems, + isFocusableElement: this.isFocusableElement.bind(this), + hostDelegatesFocus: true, + }); + } + this.addEventListener( 'sp-menu-item-added-or-updated', this.onSelectableItemAddedOrUpdated @@ -312,40 +386,63 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { ); this.addEventListener('click', this.handleClick); + this.addEventListener('focusout', this.handleFocusout); + this.addEventListener('sp-menu-item-keydown', this.handleKeydown); 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); } - public override focus({ preventScroll }: FocusOptions = {}): void { - if ( - !this.childItems.length || - this.childItems.every((childItem) => childItem.disabled) - ) { - return; - } - if ( - this.childItems.some( - (childItem) => childItem.menuData.focusRoot !== this - ) - ) { - super.focus({ preventScroll }); + /** + * for picker elements, will set focus on first selected item + */ + public focusOnFirstSelectedItem({ + preventScroll, + }: FocusOptions = {}): void { + if (!this.rovingTabindexController) return; + const selectedItem = this.selectedItems.find((el) => + this.isFocusableElement(el) + ); + if (!selectedItem) { + this.focus({ preventScroll }); return; } - this.focusMenuItemByOffset(0); - super.focus({ preventScroll }); - const selectedItem = this.selectedItems[0]; + if (selectedItem && !preventScroll) { selectedItem.scrollIntoView({ block: 'nearest' }); } + this.rovingTabindexController?.focusOnItem(selectedItem); + } + + public override focus({ preventScroll }: FocusOptions = {}): void { + if (this.rovingTabindexController) { + if ( + !this.childItems.length || + this.childItems.every((childItem) => childItem.disabled) + ) { + return; + } + if ( + this.childItems.some( + (childItem) => childItem.menuData.focusRoot !== this + ) + ) { + super.focus({ preventScroll }); + return; + } + this.rovingTabindexController.focus({ preventScroll }); + } } // if the click and pointerup events are on the same target, we should not // handle the click event. private pointerUpTarget = null as EventTarget | null; + private handleFocusout(): void { + if (!this.matches(':focus-within')) + this.rovingTabindexController?.reset(); + } + private handleClick(event: Event): void { if (this.pointerUpTarget === event.target) { this.pointerUpTarget = null; @@ -405,49 +502,6 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { this.prepareToCleanUp(); } - public handleFocusin(event: FocusEvent): void { - if ( - this.childItems.some( - (childItem) => childItem.menuData.focusRoot !== this - ) - ) { - return; - } - const activeElement = (this.getRootNode() as Document).activeElement as - | MenuItem - | 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)); - } - } - 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(); protected handleDescendentOverlayOpened(event: Event): void { @@ -478,6 +532,33 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { ); }; + /** + * given a menu item, returns the next focusable menu item before or after it; + * if no menu item is provided, returns the first focusable menu item + * @param menuItem {MenuItem} + * @param before {boolean} return the item before; default is false + * @returns {MenuItem} + */ + public getNeighboringFocusableElement( + menuItem?: MenuItem, + before = false + ): MenuItem { + const diff = before ? -1 : 1; + const elements = this.rovingTabindexController?.elements || []; + const index = !!menuItem ? elements.indexOf(menuItem) : -1; + let newIndex = Math.min(Math.max(0, index + diff), elements.length - 1); + while ( + !this.isFocusableElement(elements[newIndex]) && + 0 < newIndex && + newIndex < elements.length - 1 + ) { + newIndex += diff; + } + return !!this.isFocusableElement(elements[newIndex]) + ? (elements[newIndex] as MenuItem) + : menuItem || elements[0]; + } + public handleSubmenuOpened = (event: Event): void => { event.stopPropagation(); const target = event.composedPath()[0] as Overlay; @@ -487,18 +568,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 { @@ -507,13 +582,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)) { @@ -572,58 +644,44 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { targetItem.selected = true; } else if (resolvedSelects === 'multiple') { targetItem.selected = !targetItem.selected; + } else if ( + !targetItem.hasSubmenu && + targetItem?.menuData?.focusRoot === this + ) { + this.dispatchEvent(new Event('close', { bubbles: 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; - event.stopPropagation(); + protected navigateBetweenRelatedMenus(event: MenuItemKeydownEvent): void { + const { key, root } = event; 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. + //open submenu and set focus + event.stopPropagation(); lastFocusedItem.openOverlay(); } } else if (shouldCloseSelfAsSubmenu && this.isSubmenu) { + event.stopPropagation(); this.dispatchEvent(new Event('close', { bubbles: true })); this.updateSelectedItemIndex(); } } - public handleKeydown(event: KeyboardEvent): void { - if (event.defaultPrevented) { + 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; + const openSubmenuKey = ['Enter', ' '].includes(key); + if (shiftKey && target !== this && this.hasAttribute('tabindex')) { this.removeAttribute('tabindex'); const replaceTabindex = ( event: FocusEvent | KeyboardEvent @@ -632,7 +690,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); } @@ -641,79 +698,38 @@ 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) { - // 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; - } - } - if (key === ' ' || key === 'Enter') { - const childItem = this.childItems[this.focusedItemIndex]; - if ( - childItem && - childItem.menuData.selectionRoot === event.target - ) { - event.preventDefault(); - childItem.click(); - } + if (openSubmenuKey && root?.hasSubmenu && !root.open) { + // Remove focus while opening overlay from keyboard or the visible focus + // will slip back to the first item in the menu. + event.preventDefault(); + root.openOverlay(); return; } - if (key === 'ArrowDown' || key === 'ArrowUp') { - const childItem = this.childItems[this.focusedItemIndex]; - if ( - childItem && - childItem.menuData.selectionRoot === event.target - ) { - this.navigateWithinMenu(event); - } + if (key === ' ' || key === 'Enter') { + event.preventDefault(); + root?.focusElement?.click(); + if (root) this.selectOrToggleItem(root); return; } - this.navigateBetweenRelatedMenus(event); + this.navigateBetweenRelatedMenus(event as MenuItemKeydownEvent); } - 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 _hasUpdatedSelectedItemIndex = false; + /** + * on focus, removes focus from focus styling item, and updates the selected item index + */ private prepareToCleanUp(): void { document.addEventListener( 'focusout', () => { requestAnimationFrame(() => { - const focusedItem = this.childItems[this.focusedItemIndex]; + const focusedItem = this.focusInItem; if (focusedItem) { focusedItem.focused = false; - this.updateSelectedItemIndex(); } }); }, @@ -721,8 +737,6 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { ); } - private _hasUpdatedSelectedItemIndex = false; - public updateSelectedItemIndex(): void { let firstOrFirstSelectedIndex = 0; const selectedItemsMap = new Map(); @@ -750,13 +764,7 @@ 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; @@ -766,6 +774,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { } private _willUpdateItems = false; + private _updateFocus?: MenuItem; private handleItemsChanged(): void { this.cachedChildItems = undefined; @@ -788,20 +797,15 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { this.updateSelectedItemIndex(); this.updateItemFocus(); } + this._willUpdateItems = false; } private updateItemFocus(): void { + this.focusInItem?.setAttribute('tabindex', '0'); 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 { @@ -811,26 +815,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; - this.setAttribute('aria-activedescendant', item.id); - if ( - item.menuData.selectionRoot && - item.menuData.selectionRoot !== this - ) { - item.menuData.selectionRoot.focus(); - } - } - private handleSlotchange({ target, }: Event & { target: HTMLSlotElement }): void { @@ -850,6 +834,10 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { } }); } + if (!!this._updateFocus) { + this.rovingTabindexController?.focusOnItem(this._updateFocus); + this._updateFocus = undefined; + } } protected renderMenuItemSlot(): TemplateResult { @@ -868,14 +856,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))), ]; @@ -923,6 +903,10 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { this.updateComplete.then(() => this.updateItemFocus()); } + private isFocusableElement(el: MenuItem): boolean { + return el ? !el.disabled : false; + } + public override disconnectedCallback(): void { this.cachedChildItems = undefined; this.selectedItems = []; diff --git a/packages/menu/src/MenuGroup.ts b/packages/menu/src/MenuGroup.ts index 69fc3a50ba4..7311706d243 100644 --- a/packages/menu/src/MenuGroup.ts +++ b/packages/menu/src/MenuGroup.ts @@ -48,15 +48,20 @@ 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 { - switch (this.selects) { - case 'multiple': - case 'single': - case 'inherit': - return 'group'; - default: - return 'menu'; - } + return 'group'; + } + + /** + * only a menu controls roving tabindex; + * groups should defer navigation to parent menu + */ + protected override get controlsRovingTabindex(): boolean { + return false; } protected updateLabel(): void { @@ -87,7 +92,7 @@ export class MenuGroup extends Menu { - ${this.renderMenuItemSlot()} + ${this.renderMenuItemSlot()} `; } } diff --git a/packages/menu/src/MenuItem.ts b/packages/menu/src/MenuItem.ts index a6478d6f948..f0aa2be9157 100644 --- a/packages/menu/src/MenuItem.ts +++ b/packages/menu/src/MenuItem.ts @@ -52,6 +52,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', { @@ -79,6 +82,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[] }; /** @@ -100,17 +152,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; @@ -132,6 +196,7 @@ export class MenuItem extends LikeAnchor( /** * @private + * text content of the menu item minus whitespace */ public get itemText(): string { return this.itemChildren.content.reduce( @@ -140,6 +205,9 @@ export class MenuItem extends LikeAnchor( ); } + /** + * whether the menu item has a submenu + */ @property({ type: Boolean, reflect: true, attribute: 'has-submenu' }) public hasSubmenu = false; @@ -149,6 +217,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, @@ -167,6 +238,9 @@ export class MenuItem extends LikeAnchor( private submenuElement?: HTMLElement; + /** + * the focusable element of the menu item + */ public override get focusElement(): HTMLElement { return this; } @@ -206,6 +280,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: { @@ -227,21 +303,12 @@ export class MenuItem extends LikeAnchor( }); } + /** + * whether submenu is open + */ @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(); @@ -249,6 +316,10 @@ export class MenuItem extends LikeAnchor( event.stopPropagation(); return false; } + + if (this.shouldProxyClick()) { + return; + } } private handleSlottableRequest = (event: SlottableRequestEvent): void => { @@ -360,6 +431,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, @@ -385,6 +459,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')) { @@ -392,11 +467,44 @@ export class MenuItem extends LikeAnchor( } } + /** + * forward key info from keydown event to parent menu + */ + handleKeydown = (event: KeyboardEvent): void => { + const { target, key } = event; + const openSubmenuKey = + this.hasSubmenu && !this.open && [' ', 'Enter'].includes(key); + if (target === this) { + if ( + ['ArrowLeft', 'ArrowRight', 'Escape'].includes(key) || + openSubmenuKey + ) + event.preventDefault(); + this.dispatchEvent( + new MenuItemKeydownEvent({ root: this, event: event }) + ); + } + }; + protected closeOverlaysForRoot(): void { if (this.open) return; 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; @@ -409,6 +517,7 @@ export class MenuItem extends LikeAnchor( // Wait till after `closeDescendentOverlays` has happened in Menu // to reopen (keep open) the direct descendent of this Menu Item this.overlayElement.open = this.open; + this.focused = false; }); } @@ -467,6 +576,7 @@ export class MenuItem extends LikeAnchor( } protected handleSubmenuOpen(event: Event): void { + const shouldFocus = this.matches(':focus, :focus-within') || this.focused; this.focused = false; const parentOverlay = event.composedPath().find((el) => { return ( @@ -474,10 +584,13 @@ export class MenuItem extends LikeAnchor( (el as HTMLElement).localName === 'sp-overlay' ); }) as Overlay; + if (shouldFocus) + this.submenuElement?.focus(); this.overlayElement.parentOverlayToForceClose = parentOverlay; } protected cleanup(): void { + this.setAttribute('aria-expanded', 'false'); this.open = false; this.active = false; } @@ -511,6 +624,20 @@ 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.hasSubmenu && + this.hasVisibleFocusInTree() + ) { + this.focus(); + } + } + protected override updated(changes: PropertyValues): void { super.updated(changes); if ( @@ -597,6 +724,18 @@ export class MenuItem extends LikeAnchor( this.dispatchUpdate(); } + public override focus(): void { + super.focus(); + // ensure focus event fires in Chromium for tests + this.dispatchEvent(new FocusEvent('focus')); + } + + public override blur(): void { + // ensure focus event fires in Chromium for tests + this.dispatchEvent(new FocusEvent('blur')); + super.blur(); + } + public dispatchUpdate(): void { if (!this.isConnected) { return; @@ -611,8 +750,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: [], }; diff --git a/packages/menu/stories/index.ts b/packages/menu/stories/index.ts index 28d99a12758..efc0b96dd3d 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 diff --git a/packages/menu/stories/menu.stories.ts b/packages/menu/stories/menu.stories.ts index 80fbaa45e98..f57d70d93b4 100644 --- a/packages/menu/stories/menu.stories.ts +++ b/packages/menu/stories/menu.stories.ts @@ -11,7 +11,7 @@ governing permissions and limitations under the License. */ import { html, TemplateResult } from '@spectrum-web-components/base'; -import type { Menu } from '@spectrum-web-components/menu'; +import type { Menu, MenuItem } from '@spectrum-web-components/menu'; import '@spectrum-web-components/menu/sp-menu.js'; import '@spectrum-web-components/popover/sp-popover.js'; import '@spectrum-web-components/action-menu/sp-action-menu.js'; @@ -431,3 +431,24 @@ export const menuWithValueSlots = (): TemplateResult => { `; }; + +headersAndIcons.storyName = 'Dynamic MenuItems'; + +export const dynamicRemoval = (): TemplateResult => { + const removeItem = async function (event: FocusEvent) { + await (event.target as MenuItem)?.updateComplete; + (event.target as MenuItem)?.remove(); + }; + return html` + + Deselect + Select Inverse + + Feather... + + Select and Mask... + Save Selection + Make Work Path + + `; +}; diff --git a/packages/menu/test/menu-group.test.ts b/packages/menu/test/menu-group.test.ts index bf9e1a9a17e..98f2fefa090 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({ diff --git a/packages/menu/test/menu-item.test.ts b/packages/menu/test/menu-item.test.ts index ec6ae45767b..bed959601d7 100644 --- a/packages/menu/test/menu-item.test.ts +++ b/packages/menu/test/menu-item.test.ts @@ -117,11 +117,59 @@ 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 () => { + const clickTargetSpy = spy(); + const el = await fixture(html` + { + clickTargetSpy( + event.composedPath()[0] as HTMLAnchorElement + ); + event.stopPropagation(); + event.preventDefault(); + }} + > + + Selected Text + + + `); + + 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')); + + 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: [ + { + position: [ + rect.left + rect.width / 2, + rect.top + rect.height / 2, + ], + type: 'click', + }, + ], + }); + + expect(clickTargetSpy.calledWith(anchorElement)).to.be.true; + }); it('value attribute', async () => { const el = await fixture(html` Selected Text @@ -193,7 +241,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`
diff --git a/packages/menu/test/menu-selects.test.ts b/packages/menu/test/menu-selects.test.ts index 90bbb2c68b7..a6abd9794db 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' }); diff --git a/packages/menu/test/menu.test.ts b/packages/menu/test/menu.test.ts index 74d2d41396d..4e783563b6e 100644 --- a/packages/menu/test/menu.test.ts +++ b/packages/menu/test/menu.test.ts @@ -9,11 +9,7 @@ the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTA OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. */ -import '@spectrum-web-components/menu/sp-menu.js'; -import '@spectrum-web-components/menu/sp-menu-divider.js'; -import '@spectrum-web-components/menu/sp-menu-group.js'; -import '@spectrum-web-components/menu/sp-menu-item.js'; -import { Menu, MenuItem } from '@spectrum-web-components/menu'; + import { aTimeout, elementUpdated, @@ -22,6 +18,15 @@ import { nextFrame, waitUntil, } from '@open-wc/testing'; +import { Menu, MenuItem } from '@spectrum-web-components/menu'; +import '@spectrum-web-components/menu/sp-menu-divider.js'; +import '@spectrum-web-components/menu/sp-menu-group.js'; +import '@spectrum-web-components/menu/sp-menu-item.js'; +import '@spectrum-web-components/menu/sp-menu.js'; +import { isFirefox, isWebKit } from '@spectrum-web-components/shared'; +import { sendKeys } from '@web/test-runner-commands'; +import { spy } from 'sinon'; +import { sendMouse } from '../../../test/plugins/browser.js'; import { arrowDownEvent, arrowUpEvent, @@ -30,10 +35,6 @@ import { testForLitDevWarnings, tEvent, } from '../../../test/testing-helpers.js'; -import { sendMouse } from '../../../test/plugins/browser.js'; -import { spy } from 'sinon'; -import { sendKeys } from '@web/test-runner-commands'; -import { isWebKit } from '@spectrum-web-components/shared'; describe('Menu', () => { it('renders empty', async () => { @@ -119,16 +120,9 @@ describe('Menu', () => { `); - await waitUntil( - () => el.childItems.length == 6, - 'expected menu to manage 6 menu items' - ); await elementUpdated(el); - const inTabindexElement = el.querySelector( - '[tabindex]:not([tabindex="-1"])' - ); - expect(inTabindexElement).to.be.null; + expect(el.childItems.length).to.equal(6); await expect(el).to.be.accessible(); }); @@ -154,11 +148,16 @@ describe('Menu', () => { await elementUpdated(el); + const selectedItem = el.querySelector( + 'sp-menu-item[selected]' + ) as MenuItem; + + expect(selectedItem.selected).to.be.true; await expect(el).to.be.accessible(); }); it('has a "value" that can be copied during "change" events', async function () { - if (isWebKit()) { + if (isWebKit() || isFirefox()) { this.skip(); } const el = await fixture(html` @@ -178,8 +177,12 @@ describe('Menu', () => { await elementUpdated(el); - const otherItem = el.querySelector('#other') as MenuItem; - otherItem.focus(); + const selectedItem = el.querySelector( + 'sp-menu-item[selected]' + ) as MenuItem; + + selectedItem.focus(); + await elementUpdated(el); await sendKeys({ press: 'ArrowDown', @@ -189,14 +192,14 @@ describe('Menu', () => { press: 'Enter', }); + const clipboardText = await navigator.clipboard.readText(); await elementUpdated(el); - const clipboardText = await navigator.clipboard.readText(); expect(clipboardText).to.equal('Other'); }); it('accepts Numpad keys', async function () { - if (isWebKit()) { + if (isWebKit() || isFirefox()) { this.skip(); } const el = await fixture(html` @@ -215,9 +218,10 @@ describe('Menu', () => { `); await elementUpdated(el); - - const otherItem = el.querySelector('#other') as MenuItem; - otherItem.focus(); + const selectedItem = el.querySelector( + 'sp-menu-item[selected]' + ) as MenuItem; + selectedItem.focus(); await elementUpdated(el); await sendKeys({ press: 'ArrowDown', @@ -286,32 +290,45 @@ describe('Menu', () => { el.focus(); await elementUpdated(el); - // Activate :focus-visible - await sendKeys({ press: 'ArrowDown' }); - await sendKeys({ press: 'ArrowUp' }); - expect(document.activeElement === el).to.be.true; - expect(firstItem.focused).to.be.true; + expect(document.activeElement === firstItem, 'active element').to.be + .true; + expect(firstItem.focused, 'first item focused').to.be.true; + expect(firstItem.textContent, 'focused item text').to.equal('Deselect'); el.dispatchEvent(arrowUpEvent()); el.dispatchEvent(arrowUpEvent()); el.dispatchEvent(tEvent()); - expect(document.activeElement === el).to.be.true; - expect(thirdToLastItem.focused).to.be.true; + expect( + document.activeElement === thirdToLastItem, + 'active element after arrow up' + ).to.be.true; + expect(thirdToLastItem.focused, 'third to last item focused').to.be + .true; + expect(thirdToLastItem.textContent, 'focused item text').to.equal( + 'Select and Mask...' + ); el.dispatchEvent(arrowDownEvent()); - expect(document.activeElement === el).to.be.true; - expect(secondToLastItem.focused).to.be.true; + expect( + document.activeElement === secondToLastItem, + 'active element after arrow down' + ).to.be.true; + expect(secondToLastItem.focused, 'second to last item focused').to.be + .true; + expect(secondToLastItem.textContent, 'focused item text').to.equal( + 'Save Selection' + ); }); - it('handle focus and late descendent additions', async () => { + it('handle focus and late descendant additions', async () => { const el = await fixture(html` Options - Deselect + Deselect `); @@ -322,27 +339,29 @@ describe('Menu', () => { ); await elementUpdated(el); - const firstItem = el.querySelector( - 'sp-menu-item:nth-of-type(1)' - ) as MenuItem; + const initialLoadedItem = el.querySelector('#deselect') as MenuItem; el.focus(); await elementUpdated(el); - // Activate :focus-visible - await sendKeys({ press: 'ArrowDown' }); - await sendKeys({ press: 'ArrowUp' }); - expect(document.activeElement === el, 'active element').to.be.true; - expect(firstItem.focused, 'visually focused').to.be.true; + expect(document.activeElement === initialLoadedItem, 'active element') + .to.be.true; + expect(initialLoadedItem.focused, 'visually focused').to.be.true; + expect(initialLoadedItem.textContent, 'focused item text').to.equal( + 'Deselect' + ); el.blur(); const group = el.querySelector('sp-menu-group') as HTMLElement; + const prependedItem = document.createElement('sp-menu-item'); prependedItem.textContent = 'Prepended Item'; + const appendedItem = document.createElement('sp-menu-item'); appendedItem.textContent = 'Appended Item'; + group.prepend(prependedItem); group.append(appendedItem); await elementUpdated(el); @@ -350,62 +369,57 @@ describe('Menu', () => { await waitUntil(() => { return el.childItems.length == 3; }, 'expected menu to manage 3 items'); - await elementUpdated(el); - expect(document.activeElement === el).to.be.false; - expect(firstItem.focused).to.be.false; - expect(prependedItem.focused).to.be.false; + await elementUpdated(el); + expect(el.childItems.length).to.equal(3); el.focus(); - // Activate :focus-visible - await sendKeys({ press: 'ArrowDown' }); - await sendKeys({ press: 'ArrowUp' }); - expect(document.activeElement === el, 'another active element').to.be + expect( + document.activeElement === prependedItem, + 'prepended item is active element?' + ).to.be.true; + expect(prependedItem.focused, 'prepended item visibly focused').to.be .true; - expect(prependedItem.focused, 'another visibly focused').to.be.true; - el.dispatchEvent(arrowUpEvent()); + await sendKeys({ press: 'ArrowUp' }); - expect(document.activeElement === el, 'last active element').to.be.true; - expect(appendedItem.focused, 'last visibly focused').to.be.true; + expect( + document.activeElement === appendedItem, + 'appended item is active element' + ).to.be.true; + expect(appendedItem.focused, 'appended visibly focused').to.be.true; }); + it('cleans up when tabbing away', async () => { const el = await fixture(html` - + Deselect Select Inverse Third Item `); - - await waitUntil( - () => el.childItems.length == 3, - 'expected menu to manage 3 items' - ); await elementUpdated(el); const firstItem = el.querySelector( 'sp-menu-item:nth-of-type(1)' ) as MenuItem; - const secondItem = el.querySelector( - 'sp-menu-item:nth-of-type(2)' - ) as MenuItem; const thirdItem = el.querySelector( 'sp-menu-item:nth-of-type(3)' ) as MenuItem; el.focus(); - // Activate :focus-visible - await sendKeys({ press: 'ArrowDown' }); - await sendKeys({ press: 'ArrowUp' }); - expect(document.activeElement === el).to.be.true; - expect(firstItem.focused, 'first').to.be.true; + + expect( + document.activeElement === firstItem, + 'first item is active element' + ).to.be.true; + expect(firstItem.focused, 'first item focused').to.be.true; el.dispatchEvent(arrowDownEvent()); el.dispatchEvent(arrowDownEvent()); - expect(thirdItem.focused, 'third').to.be.true; + expect(thirdItem.focused, 'third item focused').to.be.true; // imitate tabbing away - el.dispatchEvent(tabEvent()); + thirdItem.dispatchEvent(tabEvent()); el.dispatchEvent( new CustomEvent('focusout', { composed: true, @@ -413,47 +427,57 @@ describe('Menu', () => { }) ); await nextFrame(); - // re-bind keyevents - el.startListeningToKeyboard(); + + el.focus(); // focus management should start again from the first item. - el.dispatchEvent(arrowDownEvent()); - expect(secondItem.focused, 'second').to.be.true; + await sendKeys({ press: 'ArrowDown' }); + expect(firstItem.focused, 'first item focused again').to.be.true; }); + it('handles focus across focused MenuItem removals', async () => { const el = await fixture(html` - - Deselect - Invert Selection - Feather... - Select and Mask... - - Save Selection - + + Deselect + Select Inverse + Third Item `); - const firstItem = el.querySelector('.first') as MenuItem; - const selectedItem = el.querySelector('.selected') as MenuItem; + await waitUntil( + () => el.childItems.length == 3, + 'expected menu to manage 3 items', + { timeout: 100 } + ); + + expect(el.children.length).to.equal(el.childItems.length); - await elementUpdated(el); - await nextFrame(); el.focus(); - expect(document.activeElement).to.equal(el); - // Enforce visible focus - await sendKeys({ - press: 'ArrowUp', - }); - await sendKeys({ - press: 'ArrowDown', - }); - expect(selectedItem.focused).to.be.true; + const children = [...el.children]; - selectedItem.remove(); - await elementUpdated(el); + expect(children[1], 'selected element is focused').to.equal( + document.activeElement + ); - expect(document.activeElement).to.equal(el); - expect(firstItem.focused).to.be.true; + await sendKeys({ press: 'ArrowUp' }); + + expect(children[0], 'first element is focused').to.equal( + document.activeElement + ); + //@todo this test fails on Chromium + if (isFirefox() || isWebKit()) { + children[0].remove(); + await elementUpdated(el); + expect(children[1], 'selected element is focused').to.equal( + document.activeElement + ); + + await sendKeys({ press: 'ArrowUp' }); + expect(children[2], 'last element is focused').to.equal( + document.activeElement + ); + } }); + it('handles single selection', async () => { const el = await fixture(html` @@ -462,15 +486,6 @@ describe('Menu', () => { Third `); - - await waitUntil( - () => el.childItems.length == 3, - 'expected menu to manage 3 items' - ); - await waitUntil( - () => el.selectedItems.length == 1, - 'expected menu to have 1 selected item' - ); await elementUpdated(el); const firstItem = el.querySelector( diff --git a/packages/menu/test/submenu.test.ts b/packages/menu/test/submenu.test.ts index 288a7a1035b..f4b9d8a0abf 100644 --- a/packages/menu/test/submenu.test.ts +++ b/packages/menu/test/submenu.test.ts @@ -32,6 +32,7 @@ import '@spectrum-web-components/overlay/sp-overlay.js'; import '@spectrum-web-components/icons-workflow/icons/sp-icon-show-menu.js'; import { TemplateResult } from 'lit-html'; import { slottableRequest } from '@spectrum-web-components/overlay/src/slottable-request-directive.js'; +import { isWebKit } from '@spectrum-web-components/shared'; type SelectsWithKeyboardTest = { dir: 'ltr' | 'rtl' | 'auto'; @@ -110,19 +111,36 @@ describe('Submenu', () => { this.el.parentElement.dir = testData.dir; await elementUpdated(this.el); - expect(this.rootItem.open).to.be.false; + expect( + this.rootItem.open, + `rootItem open before ${testData.openKey}` + ).to.be.false; const input = document.createElement('input'); this.el.insertAdjacentElement('beforebegin', input); - input.focus(); - await sendKeys({ - press: 'Tab', - }); + this.el.focus(); + + // by default, Safari doesn't tab to some elements + if (!isWebKit) { + await sendKeys({ + press: 'Shift+Tab', + }); + + expect(document.activeElement).to.equal(input); + await sendKeys({ + press: 'Tab', + }); + + expect(document.activeElement).to.equal(this.el); + } await sendKeys({ press: 'ArrowDown', }); await elementUpdated(this.rootItem); - expect(this.rootItem.focused).to.be.true; + expect( + this.rootItem.focused, + `rootItem focused before ${testData.openKey}` + ).to.be.true; let opened = oneEvent(this.rootItem, 'sp-opened'); await sendKeys({ @@ -130,28 +148,33 @@ 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; expect( - submenu === document.activeElement, - `${document.activeElement?.id}` + this.rootItem.open, + `rootItem open after ${testData.openKey}` ).to.be.true; + //opening a menu via keyboard should set focus on first item + expect(document.activeElement).to.equal(submenuItem); + let closed = oneEvent(this.rootItem, 'sp-closed'); await sendKeys({ press: testData.closeKey, }); await closed; - expect(this.rootItem.open).to.be.false; expect( - this.el === document.activeElement, - `${document.activeElement?.id}` - ).to.be.true; + this.rootItem.open, + `rootItem open after ${testData.closeKey}` + ).to.be.false; + + //closing a submenu via keyboard should set focus on its parent menuitem + expect(document.activeElement).to.equal(rootItem); opened = oneEvent(this.rootItem, 'sp-opened'); await sendKeys({ @@ -160,9 +183,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).to.equal(submenuItem); await sendKeys({ press: 'ArrowDown', @@ -170,9 +194,10 @@ 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, `submenu focused`).to.be.true; + expect(document.activeElement === submenuItem, `submenu active`).to + .be.true; closed = oneEvent(this.rootItem, 'sp-closed'); await sendKeys({ @@ -193,13 +218,20 @@ describe('Submenu', () => { it('returns visible focus when submenu closed', async function () { const input = document.createElement('input'); this.el.insertAdjacentElement('beforebegin', input); - input.focus(); - await sendKeys({ - press: 'Tab', - }); - await elementUpdated(this.el); - await nextFrame(); - await nextFrame(); + // by default, Safari doesn't tab to some elements + if (!isWebKit) { + await sendKeys({ + press: 'Shift+Tab', + }); + + expect(document.activeElement).to.equal(input); + await sendKeys({ + press: 'Tab', + }); + + expect(document.activeElement).to.equal(this.el); + } + this.el.focus(); await sendKeys({ press: 'ArrowDown', }); @@ -212,6 +244,7 @@ describe('Submenu', () => { `focused: ${document.activeElement?.localName}` ).to.be.true; expect(this.rootItem.open, 'not open').to.be.false; + expect(document.activeElement).to.equal(this.rootItem); const opened = oneEvent(this.rootItem, 'sp-opened'); await sendKeys({ @@ -330,7 +363,6 @@ describe('Submenu', () => { ], }); await opened; - expect(this.rootItem.open).to.be.true; const closed = oneEvent(this.rootItem, 'sp-closed'); @@ -552,7 +584,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); @@ -716,7 +748,7 @@ describe('Submenu', () => { expect(subSubmenuChanged.calledWith('C'), 'sub submenu changed').to.be .true; }); - it('closes all decendent submenus when closing a ancestor menu', async () => { + it('closes all descendant submenus when closing a ancestor menu', async () => { const el = await fixture(html` @@ -879,7 +911,7 @@ describe('Submenu', () => { }); await closed; }); - it('closes decendent menus when Menu Item in ancestor without a submenu is pointerentered', async function () { + it('closes descendant menus when Menu Item in ancestor without a submenu is pointerentered', async function () { const rootMenu = this.el.querySelector( '#submenu-item-1' ) as MenuItem; @@ -903,7 +935,7 @@ describe('Submenu', () => { ); await closed; }); - it('closes decendent menus when Menu Item in ancestor is clicked', async function () { + it('closes descendant menus when Menu Item in ancestor is clicked', async function () { const rootMenu1 = this.el.querySelector( '#submenu-item-1' ) as MenuItem; @@ -1135,9 +1167,15 @@ describe('Submenu', () => { Parent Item
- ${Array(20).fill(0).map((_, i) => html` - Submenu Item ${i + 1} - `)} + ${Array(20) + .fill(0) + .map( + (_, i) => html` + + Submenu Item ${i + 1} + + ` + )}
@@ -1146,7 +1184,9 @@ describe('Submenu', () => { await elementUpdated(el); const menuItem = el.querySelector('sp-menu-item') as MenuItem; - const submenu = menuItem.querySelector('[slot="submenu"]') as HTMLElement; + const submenu = menuItem.querySelector( + '[slot="submenu"]' + ) as HTMLElement; // Open the submenu const opened = oneEvent(menuItem, 'sp-opened'); diff --git a/packages/overlay/stories/overlay.stories.ts b/packages/overlay/stories/overlay.stories.ts index 2b10a4361e7..c0f29d5494f 100644 --- a/packages/overlay/stories/overlay.stories.ts +++ b/packages/overlay/stories/overlay.stories.ts @@ -473,6 +473,7 @@ class ComplexModalReady extends HTMLElement { handlePickerOpen = async (): Promise => { const picker = document.querySelector('#test-picker') as Picker; const actions = [nextFrame, picker.updateComplete]; + picker.focus(); await Promise.all(actions); diff --git a/packages/picker/src/InteractionController.ts b/packages/picker/src/InteractionController.ts index 5c93d6a3bb8..1256b4c40db 100644 --- a/packages/picker/src/InteractionController.ts +++ b/packages/picker/src/InteractionController.ts @@ -137,7 +137,10 @@ export class InteractionController implements ReactiveController { this.host.isMobile.matches && !this.host.forcePopover ? undefined : this.host.placement; - this.overlay.receivesFocus = 'true'; + // We should not be applying open is set programmatically via the picker's open.property. + // Focus should only be applied if a user action causes the menu to open. Otherwise, + // we could be pulling focus from a user when an picker with an open menu loads. + this.overlay.receivesFocus = 'false'; this.overlay.willPreventClose = this.preventNextToggle !== 'no' && this.open; this.overlay.addEventListener( @@ -158,6 +161,7 @@ export class InteractionController implements ReactiveController { ) { this.preventNextToggle = 'yes'; } + if (this.preventNextToggle === 'no') this.host.close(); } public handleActivate(_event: Event): void {} @@ -172,6 +176,11 @@ export class InteractionController implements ReactiveController { hostConnected(): void { this.init(); + this.host.addEventListener('sp-closed', ()=> { + if(!this.open && this.host.optionsMenu.matches(':focus-within') && !this.host.button.matches(':focus')) { + this.host.button.focus(); + } + }); } hostDisconnected(): void { diff --git a/packages/picker/src/MobileController.ts b/packages/picker/src/MobileController.ts index 0599d6d7924..cde57289c40 100644 --- a/packages/picker/src/MobileController.ts +++ b/packages/picker/src/MobileController.ts @@ -20,8 +20,12 @@ export class MobileController extends InteractionController { override type = InteractionTypes.mobile; handleClick(): void { + if (this.host.disabled) { + return; + } if (this.preventNextToggle == 'no') { - this.open = !this.open; + + this.host.toggle(); } this.preventNextToggle = 'no'; } diff --git a/packages/picker/src/Picker.ts b/packages/picker/src/Picker.ts index e311910ef53..350417becd2 100644 --- a/packages/picker/src/Picker.ts +++ b/packages/picker/src/Picker.ts @@ -18,6 +18,7 @@ import { PropertyValues, render, SizedMixin, + SpectrumElement, TemplateResult, } from '@spectrum-web-components/base'; import { @@ -35,7 +36,6 @@ import { import pickerStyles from './picker.css.js'; import chevronStyles from '@spectrum-web-components/icon/src/spectrum-icon-chevron.css.js'; -import { Focusable } from '@spectrum-web-components/shared/src/focusable.js'; import type { Tooltip } from '@spectrum-web-components/tooltip'; import '@spectrum-web-components/icons-ui/icons/sp-icon-chevron100.js'; import '@spectrum-web-components/icons-workflow/icons/sp-icon-alert.js'; @@ -45,6 +45,8 @@ import type { MenuItem, MenuItemChildren, } from '@spectrum-web-components/menu'; + +import type { MenuItemKeydownEvent } from '@spectrum-web-components/menu'; import { Placement } from '@spectrum-web-components/overlay'; import { IS_MOBILE, @@ -68,7 +70,14 @@ const chevronClass = { }; export const DESCRIPTION_ID = 'option-picker'; -export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { +export class PickerBase extends SizedMixin(SpectrumElement, { + noDefaultSize: true, +}) { + static override shadowRootOptions = { + ...SpectrumElement.shadowRootOptions, + delegatesFocus: true, + }; + public isMobile = new MatchMediaController(this, IS_MOBILE); public strategy!: DesktopController | MobileController; @@ -84,7 +93,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { private deprecatedMenu: Menu | null = null; @property({ type: Boolean, reflect: true }) - public override disabled = false; + public disabled = false; @property({ type: Boolean, reflect: true }) public focused = false; @@ -132,10 +141,11 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { @query('sp-menu') public optionsMenu!: Menu; - private _selfManageFocusElement = false; - - public override get selfManageFocusElement(): boolean { - return this._selfManageFocusElement; + /** + * @deprecated + * */ + public get selfManageFocusElement(): boolean { + return true; } @query('sp-overlay') @@ -189,7 +199,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { protected listRole: 'listbox' | 'menu' = 'listbox'; protected itemRole = 'option'; - public override get focusElement(): HTMLElement { + public get focusElement(): HTMLElement { if (this.open) { return this.optionsMenu; } @@ -204,11 +214,16 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { this.focused = true; } + // handled by interaction controller, desktop or mobile; this is only called with a programmatic this.click() public override click(): void { + this.toggle(); + } + + // pointer events handled by interaction controller, desktop or mobile; this is only called with a programmatic this.button.click() + public handleButtonClick(): void { if (this.disabled) { return; } - this.toggle(); } @@ -217,19 +232,23 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { } public override focus(options?: FocusOptions): void { - super.focus(options); - - if (!this.disabled && this.focusElement) { - this.focused = this.hasVisibleFocusInTree(); - } + this.focusElement?.focus(options); } - + /** + * @deprecated - Use `focus` instead. + */ public handleHelperFocus(): void { // set focused to true here instead of handleButtonFocus so clicks don't flash a focus outline this.focused = true; this.button.focus(); } + public handleFocus(): void { + if (!this.disabled && this.focusElement) { + this.focused = this.hasVisibleFocusInTree(); + } + } + public handleChange(event: Event): void { if (this.strategy) { this.strategy.preventNextToggle = 'no'; @@ -253,16 +272,38 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { this.strategy?.handleButtonFocus(event); } + protected handleEscape = ( + event: MenuItemKeydownEvent | KeyboardEvent + ): void => { + if (event.key === 'Escape') { + event.stopPropagation(); + event.preventDefault(); + this.toggle(false); + } + }; + protected handleKeydown = (event: KeyboardEvent): void => { this.focused = true; - if (event.code !== 'ArrowDown' && event.code !== 'ArrowUp') { + if ( + !['ArrowUp', 'ArrowDown', 'Enter', ' ', 'Escape'].includes( + event.key + ) + ) { + return; + } + if (event.key === 'Escape') { + this.handleEscape(event); return; } event.stopPropagation(); event.preventDefault(); - this.toggle(true); + this.keyboardOpen(); }; + protected async keyboardOpen(): Promise { + this.toggle(true); + } + protected async setValueFromItem( item: MenuItem, menuChangeEvent?: Event @@ -321,18 +362,23 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { } public toggle(target?: boolean): void { - if (this.readonly || this.pending) { + if (this.readonly || this.pending || this.disabled) { return; } - this.open = typeof target !== 'undefined' ? target : !this.open; + const open = typeof target !== 'undefined' ? target : !this.open; + if (open && !this.open) + this.addEventListener( + 'sp-opened', + () => this.optionsMenu?.focusOnFirstSelectedItem(), + { + once: true, + } + ); + + this.open = open; if (this.strategy) { this.strategy.open = this.open; } - if (this.open) { - this._selfManageFocusElement = true; - } else { - this._selfManageFocusElement = false; - } } public close(): void { @@ -451,6 +497,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { aria-hidden="true" name="tooltip" id="tooltip" + @keydown=${this.handleKeydown} @slotchange=${this.handleTooltipSlotchange} > `, @@ -465,6 +512,41 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { this.labelAlignment = labelElement.sideAligned ? 'inline' : undefined; }; + protected hasAccessibleLabel(): boolean { + const slotContent = + this.querySelector('[slot="label"]')?.textContent && + this.querySelector('[slot="label"]')?.textContent?.trim() !== ''; + const slotAlt = + this.querySelector('[slot="label"]')?.getAttribute('alt')?.trim() && + this.querySelector('[slot="label"]') + ?.getAttribute('alt') + ?.trim() !== ''; + return ( + !!this.label || + !!this.getAttribute('aria-label') || + !!this.getAttribute('aria-labelledby') || + !!this.appliedLabel || + !!slotContent || + !!slotAlt + ); + } + + 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 renderOverlay(menu: TemplateResult): TemplateResult { if (this.strategy?.overlay === undefined) { return menu; @@ -490,15 +572,9 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { this.tooltipEl.disabled = this.open; } return html` - @@ -522,6 +598,14 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { `; } + protected override willUpdate(changes: PropertyValues): void { + super.willUpdate(changes); + if (changes.has('tabIndex') && !!this.tabIndex) { + this.button.tabIndex = this.tabIndex; + this.removeAttribute('tabindex'); + } + } + protected override update(changes: PropertyValues): void { if (this.selects) { /** @@ -532,23 +616,17 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { this.selects = 'single'; } if (changes.has('disabled') && this.disabled) { - if (this.strategy) { - this.open = false; - this.strategy.open = false; - } + this.close(); } if (changes.has('pending') && this.pending) { - if (this.strategy) { - this.open = false; - this.strategy.open = false; - } + this.close(); } if (changes.has('value')) { // MenuItems update a frame late for management, // await the same here. this.shouldScheduleManageSelection(); } - // Maybe it's finally time to remove this support? + // Maybe it's finally time to remove this support?s if (!this.hasUpdated) { this.deprecatedMenu = this.querySelector(':scope > sp-menu'); this.deprecatedMenu?.toggleAttribute('ignore', true); @@ -569,25 +647,8 @@ 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(); } }); } @@ -681,6 +742,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { .selects=${this.selects} .selected=${this.value ? [this.value] : []} size=${this.size} + @sp-menu-item-keydown=${this.handleEscape} @sp-menu-item-added-or-updated=${this.shouldManageSelection} > @@ -700,8 +762,15 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { return menu; } - private willManageSelection = false; + /** + * whether a selection change is already scheduled + */ + public willManageSelection = false; + /** + * when the value changes or the menu slot changes, manage the selection on the next frame, if not already scheduled + * @param event + */ protected shouldScheduleManageSelection(event?: Event): void { if ( !this.willManageSelection && @@ -709,6 +778,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { ((event.target as HTMLElement).getRootNode() as ShadowRoot) .host === this) ) { + //s set a flag to manage selection on the next frame this.willManageSelection = true; requestAnimationFrame(() => { requestAnimationFrame(() => { @@ -718,6 +788,9 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { } } + /** + * when an item is added or updated, manage the selection, if it's not already scheduled + */ protected shouldManageSelection(): void { if (this.willManageSelection) { return; @@ -726,6 +799,9 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { this.manageSelection(); } + /** + * updates menu selection based on value + */ protected async manageSelection(): Promise { if (this.selects == null) return; @@ -779,7 +855,12 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { private enterKeydownOn: EventTarget | null = null; protected handleEnterKeydown = (event: KeyboardEvent): void => { - if (event.code !== 'Enter') { + if (event.key !== 'Enter') { + return; + } + const target = event?.target as MenuItem; + if (!target.open && target.hasSubmenu) { + event.preventDefault(); return; } @@ -791,7 +872,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { this.addEventListener( 'keyup', async (keyupEvent: KeyboardEvent) => { - if (keyupEvent.code !== 'Enter') { + if (keyupEvent.key !== 'Enter') { return; } this.enterKeydownOn = null; @@ -812,6 +893,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { public override connectedCallback(): void { super.connectedCallback(); this.recentlyConnected = this.hasUpdated; + this.addEventListener('focus', this.handleFocus); } public override disconnectedCallback(): void { @@ -846,34 +928,38 @@ export class Picker extends PickerBase { } protected override handleKeydown = (event: KeyboardEvent): void => { - const { code } = event; + const { key } = event; + const handledKeys = [ + 'ArrowUp', + 'ArrowDown', + 'ArrowLeft', + 'ArrowRight', + 'Enter', + ' ', + 'Escape', + ].includes(key); + const openKeys = ['ArrowUp', 'ArrowDown', 'Enter', ' '].includes(key); this.focused = true; - if (!code.startsWith('Arrow') || this.readonly || this.pending) { + if ('Escape' === key) { + this.handleEscape(event); return; } - if (code === 'ArrowUp' || code === 'ArrowDown') { - this.toggle(true); - event.preventDefault(); + if (!handledKeys || this.readonly || this.pending) { return; } - event.preventDefault(); - const selectedIndex = this.selectedItem - ? this.menuItems.indexOf(this.selectedItem) - : -1; - // use a positive offset to find the first non-disabled item when no selection is available. - const nextOffset = selectedIndex < 0 || code === 'ArrowRight' ? 1 : -1; - let nextIndex = selectedIndex + nextOffset; - while ( - this.menuItems[nextIndex] && - this.menuItems[nextIndex].disabled - ) { - nextIndex += nextOffset; - } - if (!this.menuItems[nextIndex] || this.menuItems[nextIndex].disabled) { + if (openKeys) { + this.keyboardOpen(); + event.preventDefault(); return; } - if (!this.value || nextIndex !== selectedIndex) { - this.setValueFromItem(this.menuItems[nextIndex]); + event.preventDefault(); + const nextItem = this.optionsMenu?.getNeighboringFocusableElement( + this.selectedItem, + key === 'ArrowLeft' + ); + if (!this.value || nextItem !== this.selectedItem) { + // updates picker text but does not fire change event until action is completed + if (!!nextItem) this.setValueFromItem(nextItem as MenuItem); } }; } diff --git a/packages/picker/test/index.ts b/packages/picker/test/index.ts index 8d969aa0317..94eb499a4f7 100644 --- a/packages/picker/test/index.ts +++ b/packages/picker/test/index.ts @@ -12,7 +12,6 @@ governing permissions and limitations under the License. import type { Picker } from '@spectrum-web-components/picker'; -import type { MenuItem } from '@spectrum-web-components/menu'; import { aTimeout, elementUpdated, @@ -23,21 +22,38 @@ import { oneEvent, waitUntil, } from '@open-wc/testing'; +import '@spectrum-web-components/field-label/sp-field-label.js'; +import { FieldLabel } from '@spectrum-web-components/field-label/src/FieldLabel.js'; +import type { Menu, MenuItem } from '@spectrum-web-components/menu'; +import '@spectrum-web-components/menu/sp-menu-group.js'; +import '@spectrum-web-components/menu/sp-menu-item.js'; +import '@spectrum-web-components/menu/sp-menu.js'; +import '@spectrum-web-components/picker/sp-picker.js'; +import { SAFARI_FOCUS_RING_CLASS } from '@spectrum-web-components/picker/src/InteractionController.js'; +import { isWebKit } from '@spectrum-web-components/shared'; import '@spectrum-web-components/shared/src/focus-visible.js'; +import '@spectrum-web-components/theme/src/themes.js'; +import { Tooltip } from '@spectrum-web-components/tooltip'; +import type { Icon } from '@spectrum-web-components/icon'; + +import { + a11ySnapshot, + findAccessibilityNode, + sendKeys, + setViewport, +} from '@web/test-runner-commands'; import { spy, stub } from 'sinon'; +import { sendMouse } from '../../../test/plugins/browser.js'; import { arrowDownEvent, arrowRightEvent, arrowUpEvent, + ignoreResizeObserverLoopError, + fixture as styledFixture, testForLitDevWarnings, tEvent, } from '../../../test/testing-helpers.js'; -import { - a11ySnapshot, - findAccessibilityNode, - sendKeys, - setViewport, -} from '@web/test-runner-commands'; +import { M as pending } from '../stories/picker-pending.stories.js'; import { Default, disabled, @@ -47,32 +63,18 @@ import { slottedLabel, tooltip, } from '../stories/picker.stories.js'; -import { M as pending } from '../stories/picker-pending.stories.js'; -import { sendMouse } from '../../../test/plugins/browser.js'; -import { - ignoreResizeObserverLoopError, - fixture as styledFixture, -} from '../../../test/testing-helpers.js'; -import '@spectrum-web-components/picker/sp-picker.js'; -import '@spectrum-web-components/field-label/sp-field-label.js'; -import '@spectrum-web-components/menu/sp-menu.js'; -import '@spectrum-web-components/menu/sp-menu-group.js'; -import '@spectrum-web-components/menu/sp-menu-item.js'; -import '@spectrum-web-components/theme/src/themes.js'; -import type { Icon } from '@spectrum-web-components/icon'; -import type { Menu } from '@spectrum-web-components/menu'; -import { Tooltip } from '@spectrum-web-components/tooltip'; -import { FieldLabel } from '@spectrum-web-components/field-label/src/FieldLabel.js'; -import { isWebKit } from '@spectrum-web-components/shared'; -import { SAFARI_FOCUS_RING_CLASS } from '@spectrum-web-components/picker/src/InteractionController.js'; export type TestablePicker = { optionsMenu: Menu }; ignoreResizeObserverLoopError(before, after); const isMenuActiveElement = function (el: Picker): boolean { - return el.shadowRoot.activeElement?.localName === 'sp-menu'; + return ( + document.activeElement?.tagName === 'SP-MENU-ITEM' && + el.contains(document.activeElement) + ); }; +const isSafari = /^((?!chrome|android).)*safari/i.test(navigator.userAgent); export function runPickerTests(): void { let el: Picker; @@ -255,7 +257,7 @@ export function runPickerTests(): void { el.open = true; await opened; - expect(el.open).to.be.true; + expect(el.open, 'open?').to.be.true; const accessibleCloseButton = el.shadowRoot.querySelector( '.visually-hidden button' ) as HTMLButtonElement; @@ -270,7 +272,7 @@ export function runPickerTests(): void { await elementUpdated(el); - expect(el.open).to.be.false; + expect(el.open, 'open?').to.be.false; expect(el.shadowRoot.activeElement).to.equal(el.button); expect(document.activeElement).to.eq(el); }); @@ -443,25 +445,6 @@ export function runPickerTests(): void { '`name` is the selected item text plus the label text' ).to.not.be.null; }); - it('manages `aria-activedescendant`', async () => { - const firstItem = el.querySelector('sp-menu-item:nth-child(1)'); - const secondItem = el.querySelector('sp-menu-item:nth-child(2)'); - const opened = oneEvent(el, 'sp-opened'); - el.open = true; - await opened; - expect( - (el as unknown as TestablePicker).optionsMenu.getAttribute( - 'aria-activedescendant' - ) - ).to.equal(firstItem?.id); - await sendKeys({ press: 'ArrowDown' }); - await elementUpdated(el); - expect( - (el as unknown as TestablePicker).optionsMenu.getAttribute( - 'aria-activedescendant' - ) - ).to.equal(secondItem?.id); - }); it('renders invalid accessibly', async () => { el.invalid = true; await elementUpdated(el); @@ -477,69 +460,73 @@ export function runPickerTests(): void { }); it('opens with visible focus on a menu item on `DownArrow`', async () => { const firstItem = el.querySelector('sp-menu-item') as MenuItem; + const opened = oneEvent(el, 'sp-opened'); + const closed = oneEvent(el, 'sp-closed'); - await elementUpdated(el); - - expect(firstItem.focused, 'should not visually focused').to.be - .false; + expect( + firstItem.focused, + 'first item should not be visually focused before opening' + ).to.be.false; el.focus(); await elementUpdated(el); - const opened = oneEvent(el, 'sp-opened'); - await sendKeys({ press: 'ArrowRight' }); - await sendKeys({ press: 'ArrowLeft' }); + await sendKeys({ press: 'ArrowDown' }); await opened; - expect(el.open).to.be.true; - expect(firstItem.focused, 'should be visually focused').to.be.true; + expect(el.open, 'picker should be open').to.be.true; + expect( + firstItem.focused, + 'first item should be visually focused after opening' + ).to.be.true; - const closed = oneEvent(el, 'sp-closed'); await sendKeys({ press: 'Escape', }); await closed; - expect(el.open).to.be.false; - expect( - document.activeElement === el, - `focused ${document.activeElement?.localName} instead of back on Picker` - ).to.be.true; - expect( - el.shadowRoot.activeElement === el.button, - `focused ${el.shadowRoot.activeElement?.localName} instead of back on button` - ).to.be.true; + expect(el.open, 'picker should be closed').to.be.false; + + expect(document.activeElement).to.equal(el); + expect(el.shadowRoot.activeElement).to.equal(el.button); await waitUntil( () => !firstItem.focused, 'finally, not visually focused' ); + expect( + firstItem.focused, + 'first item should not be visually focused after closing' + ).to.be.false; }); it('opens with visible focus on a menu item on `Space`', async function () { const firstItem = el.querySelector('sp-menu-item') as MenuItem; + const opened = oneEvent(el, 'sp-opened'); + const closed = oneEvent(el, 'sp-closed'); - await elementUpdated(el); - - expect(firstItem.focused, 'should not visually focused').to.be - .false; + expect( + firstItem.focused, + 'should not be visually focused before opening' + ).to.be.false; el.focus(); await elementUpdated(el); - const opened = oneEvent(el, 'sp-opened'); - await sendKeys({ press: 'ArrowRight' }); - await sendKeys({ press: 'ArrowLeft' }); + await sendKeys({ press: 'Space' }); await opened; - expect(el.open).to.be.true; - expect(firstItem.focused, 'should be visually focused').to.be.true; + expect(el.open, 'open?').to.be.true; + expect( + firstItem.focused, + 'should be visually focused after opening' + ).to.be.true; - const closed = oneEvent(el, 'sp-closed'); await sendKeys({ press: 'Escape', }); await closed; - expect(el.open).to.be.false; + expect(el.open, 'picker should be closed').to.be.false; + expect( document.activeElement === el, `focused ${document.activeElement?.localName} instead of back on Picker` @@ -552,8 +539,12 @@ export function runPickerTests(): void { () => !firstItem.focused, 'finally, not visually focused' ); + expect( + firstItem.focused, + 'first item should not be visually focused after closing' + ).to.be.false; }); - it('opens, on click, without visible focus on a menu item', async () => { + it('opens, on click, with visible focus on a menu item', async () => { await nextFrame(); await nextFrame(); const firstItem = el.querySelector('sp-menu-item') as MenuItem; @@ -574,8 +565,8 @@ export function runPickerTests(): void { }); await opened; - expect(el.open).to.be.true; - expect(firstItem.focused, 'still not visually focused').to.be.false; + expect(el.open, 'open?').to.be.true; + expect(firstItem.focused, 'firstItem focused?').to.be.true; }); it('opens and selects in a single pointer button interaction', async () => { await nextFrame(); @@ -621,13 +612,11 @@ export function runPickerTests(): void { }); await closed; - expect(el.open).to.be.false; + expect(el.open, 'open?').to.be.false; expect(el.value).to.equal(thirdItem.value); }); it('opens/closes multiple times', async () => { - await nextFrame(); - await nextFrame(); - expect(el.open).to.be.false; + expect(el.open, 'open?').to.be.false; const boundingRect = el.button.getBoundingClientRect(); let opened = oneEvent(el, 'sp-opened'); sendMouse({ @@ -642,7 +631,7 @@ export function runPickerTests(): void { ], }); await opened; - expect(el.open).to.be.true; + expect(el.open, 'open?').to.be.true; let closed = oneEvent(el, 'sp-closed'); sendMouse({ @@ -657,7 +646,7 @@ export function runPickerTests(): void { ], }); await closed; - expect(el.open).to.be.false; + expect(el.open, 'open?').to.be.false; opened = oneEvent(el, 'sp-opened'); sendMouse({ @@ -672,7 +661,7 @@ export function runPickerTests(): void { ], }); await opened; - expect(el.open).to.be.true; + expect(el.open, 'open?').to.be.true; closed = oneEvent(el, 'sp-closed'); sendMouse({ @@ -687,18 +676,18 @@ export function runPickerTests(): void { ], }); await closed; - expect(el.open).to.be.false; + expect(el.open, 'open?').to.be.false; }); it('closes when becoming disabled', async () => { - expect(el.open).to.be.false; + expect(el.open, 'open before click?').to.be.false; el.click(); await elementUpdated(el); - expect(el.open).to.be.true; + expect(el.open, 'open after click?').to.be.true; el.disabled = true; - await elementUpdated(el); + await closed; - expect(el.open).to.be.false; + expect(el.open, 'open after disabled?').to.be.false; }); it('closes when clicking away', async () => { el.id = 'closing'; @@ -707,13 +696,13 @@ export function runPickerTests(): void { await elementUpdated(el); - expect(el.open).to.be.false; + expect(el.open, 'open?').to.be.false; const opened = oneEvent(el, 'sp-opened'); el.click(); await opened; await elementUpdated(el); - expect(el.open).to.be.true; + expect(el.open, 'open?').to.be.true; const closed = oneEvent(el, 'sp-closed'); other.click(); closed; @@ -730,7 +719,7 @@ export function runPickerTests(): void { el.click(); await opened; - expect(el.open).to.be.true; + expect(el.open, 'open?').to.be.true; expect(el.selectedItem?.itemText).to.be.undefined; expect(el.value).to.equal(''); @@ -738,7 +727,7 @@ export function runPickerTests(): void { secondItem.click(); await closed; - expect(el.open).to.be.false; + expect(el.open, 'open?').to.be.false; expect(el.selectedItem?.itemText).to.equal('Select Inverse'); expect(el.value).to.equal('option-2'); }); @@ -754,7 +743,7 @@ export function runPickerTests(): void { el.click(); await opened; - expect(el.open).to.be.true; + expect(el.open, 'open?').to.be.true; expect(el.selectedItem?.itemText).to.be.undefined; expect(el.value).to.equal(''); @@ -762,7 +751,7 @@ export function runPickerTests(): void { secondItem.click(); await closed; - expect(el.open).to.be.false; + expect(el.open, 'open?').to.be.false; expect(el.selectedItem?.itemText).to.equal('Select Inverse'); expect(el.value).to.equal('option-2'); @@ -770,7 +759,7 @@ export function runPickerTests(): void { el.click(); await opened; - expect(el.open).to.be.true; + expect(el.open, 'open?').to.be.true; expect(el.selectedItem?.itemText).to.equal('Select Inverse'); expect(el.value).to.equal('option-2'); @@ -778,7 +767,7 @@ export function runPickerTests(): void { firstItem.click(); await closed; - expect(el.open).to.be.false; + expect(el.open, 'open?').to.be.false; expect(el.selectedItem?.itemText).to.equal('Deselect'); expect(el.value).to.equal('Deselect'); }); @@ -815,7 +804,7 @@ export function runPickerTests(): void { el.click(); await opened; - expect(el.open).to.be.true; + expect(el.open, 'open?').to.be.true; expect(el.selectedItem?.itemText).to.be.undefined; expect(el.value).to.equal(''); expect(secondItem.selected).to.be.false; @@ -834,9 +823,40 @@ export function runPickerTests(): void { preventChangeSpy.callCount.toString() ).to.be.true; expect(secondItem.selected, 'selection prevented').to.be.false; - expect(el.open).to.be.true; + expect(el.open, 'open?').to.be.true; + }); + it('should return focus after click', async () => { + const input = document.createElement('input'); + document.body.append(input); + + await elementUpdated(el); + + const secondItem = el.querySelector( + 'sp-menu-item:nth-of-type(2)' + ) as MenuItem; + + const opened = oneEvent(el, 'sp-opened'); + el.click(); + await opened; + await elementUpdated(el); + + expect(el.open, 'open?').to.be.true; + expect(el.selectedItem?.itemText).to.be.undefined; + expect(el.value).to.equal(''); + expect(secondItem.selected).to.be.false; + + secondItem.click(); + await waitUntil( + () => document.activeElement === el, + 'focused', { timeout: 300 } + ); + + expect(el.open, 'open?').to.be.false; + expect(el.value, 'value changed').to.equal('option-2'); + expect(secondItem.selected, 'selected changed').to.be.true; + input.remove(); }); - it('can throw focus after `change`', async () => { + it('should throw focus after `change`', async () => { const input = document.createElement('input'); document.body.append(input); @@ -851,7 +871,7 @@ export function runPickerTests(): void { await opened; await elementUpdated(el); - expect(el.open).to.be.true; + expect(el.open, 'open?').to.be.true; expect(el.selectedItem?.itemText).to.be.undefined; expect(el.value).to.equal(''); expect(secondItem.selected).to.be.false; @@ -860,18 +880,15 @@ export function runPickerTests(): void { input.focus(); }); - const closed = oneEvent(el, 'sp-closed'); secondItem.click(); - await closed; - await elementUpdated(el); - - expect(el.open).to.be.false; - expect(el.value, 'value changed').to.equal('option-2'); - expect(secondItem.selected, 'selected changed').to.be.true; await waitUntil( () => document.activeElement === input, - 'focus throw' + 'focus throw', { timeout: 300 } ); + + expect(el.open, 'open?').to.be.false; + expect(el.value, 'value changed').to.equal('option-2'); + expect(secondItem.selected, 'selected changed').to.be.true; input.remove(); }); it('opens on ArrowUp', async () => { @@ -899,7 +916,8 @@ export function runPickerTests(): void { press: 'Escape', }); await closed; - expect(el.open).to.be.false; + expect(el.open, 'should be closed after escape key is pressed').to + .be.false; }); it('opens on ArrowDown', async () => { const firstItem = el.querySelector( @@ -924,7 +942,7 @@ export function runPickerTests(): void { firstItem.click(); await closed; - expect(el.open).to.be.false; + expect(el.open, 'open?').to.be.false; expect(el.selectedItem?.itemText).to.equal('Deselect'); expect(el.value).to.equal('Deselect'); }); @@ -948,21 +966,27 @@ export function runPickerTests(): void { }); await elementUpdated(el); - expect(selectionSpy.callCount).to.equal(1); + expect( + selectionSpy.callCount, + `selectionSpy.callCount: ${selectionSpy.callCount}` + ).to.equal(1); expect(selectionSpy.calledWith('Deselected')); await sendKeys({ press: 'ArrowLeft', }); await elementUpdated(el); - expect(selectionSpy.callCount).to.equal(1); + expect( + selectionSpy.callCount, + `selectionSpy.callCount: ${selectionSpy.callCount}` + ).to.equal(1); await sendKeys({ press: 'ArrowRight', }); await nextFrame(); await nextFrame(); - expect(selectionSpy.calledWith('option-2')); + expect(selectionSpy.calledWith('option-2'), 'option-2'); await sendKeys({ press: 'ArrowRight', @@ -984,9 +1008,18 @@ export function runPickerTests(): void { }); await nextFrame(); await nextFrame(); - expect(selectionSpy.calledWith('Save Selection')); - expect(selectionSpy.calledWith('Make Work Path')).to.be.false; - expect(selectionSpy.callCount).to.equal(5); + expect( + selectionSpy.calledWith('Save Selection'), + 'selectionSpy.calledWith("Save Selection")' + ); + expect( + selectionSpy.calledWith('Make Work Path'), + 'selectionSpy.calledWith("Make Work Path")' + ).to.be.false; + expect( + selectionSpy.callCount, + `selectionSpy.callCount: ${selectionSpy.callCount}` + ).to.equal(5); }); it('quick selects first item on ArrowRight when no value', async () => { await nextFrame(); @@ -1018,10 +1051,12 @@ export function runPickerTests(): void { el.insertAdjacentElement('afterend', input); el.focus(); - await sendKeys({ press: 'Tab' }); - expect(document.activeElement === input).to.be.true; - await sendKeys({ press: 'Shift+Tab' }); - expect(document.activeElement === el).to.be.true; + if (!isSafari) { + await sendKeys({ press: 'Tab' }); + expect(document.activeElement).to.equal(input); + await sendKeys({ press: 'Shift+Tab' }); + } + expect(document.activeElement).to.equal(el); const opened = oneEvent(el, 'sp-opened'); sendKeys({ press: 'Enter' }); await opened; @@ -1034,13 +1069,13 @@ export function runPickerTests(): void { await sendKeys({ press: 'ArrowDown' }); await sendKeys({ press: 'ArrowDown' }); - expect(thirdItem.focused).to.be.true; + expect(thirdItem.focused, 'thirdItem focused?').to.be.true; const closed = oneEvent(el, 'sp-closed'); button.focus(); await closed; expect(isMenuActiveElement(el)).to.be.false; - expect(el.open).to.be.false; + expect(el.open, 'open?').to.be.false; }); it('does not listen to streaming `Enter` keydown', async () => { const openSpy = spy(); @@ -1055,10 +1090,12 @@ export function runPickerTests(): void { el.insertAdjacentElement('afterend', input); el.focus(); - await sendKeys({ press: 'Tab' }); - expect(document.activeElement === input).to.be.true; - await sendKeys({ press: 'Shift+Tab' }); - expect(document.activeElement === el).to.be.true; + if (!isSafari) { + await sendKeys({ press: 'Tab' }); + expect(document.activeElement).to.equal(input); + await sendKeys({ press: 'Shift+Tab' }); + } + expect(document.activeElement).to.equal(el); const opened = oneEvent(el, 'sp-opened'); sendKeys({ down: 'Enter' }); await opened; @@ -1073,7 +1110,7 @@ export function runPickerTests(): void { await sendKeys({ press: 'ArrowDown' }); await sendKeys({ press: 'ArrowDown' }); - expect(thirdItem.focused).to.be.true; + expect(thirdItem.focused, 'thirdItem focused?').to.be.true; const closed = oneEvent(el, 'sp-closed'); sendKeys({ down: 'Enter' }); @@ -1085,7 +1122,7 @@ export function runPickerTests(): void { expect(closedSpy.callCount).to.equal(1); await sendKeys({ up: 'Enter' }); }); - it('allows tabing to close', async () => { + it('allows tabbing to close', async () => { const input = document.createElement('input'); el.insertAdjacentElement('afterend', input); const opened = oneEvent(el, 'sp-opened'); @@ -1093,7 +1130,7 @@ export function runPickerTests(): void { await opened; await nextFrame(); - expect(el.open).to.be.true; + expect(el.open, 'open?').to.be.true; el.focus(); const closed = oneEvent(el, 'sp-closed'); @@ -1123,14 +1160,20 @@ export function runPickerTests(): void { input2.remove(); }); it('tabs forward through the element', async () => { - // start at input1 - input1.focus(); - await nextFrame(); - expect(document.activeElement === input1, 'focuses input 1').to - .true; - // tab to the picker - let focused = oneEvent(el, 'focus'); - await sendKeys({ press: 'Tab' }); + let focused: Promise>; + if (!isSafari) { + // start at input1 + input1.focus(); + await nextFrame(); + expect(document.activeElement === input1, 'focuses input 1') + .to.true; + // tab to the picker + focused = oneEvent(el, 'focus'); + await sendKeys({ press: 'Tab' }); + } else { + focused = oneEvent(el, 'focus'); + el.focus(); + } await focused; expect(el.focused, 'focused').to.be.true; @@ -1150,25 +1193,25 @@ export function runPickerTests(): void { expect(document.activeElement, 'focuses input 2').to.equal( input2 ); - // tab to the picker let focused = oneEvent(el, 'focus'); - await sendKeys({ press: 'Shift+Tab' }); - await focused; - - expect(el.focused, 'focused').to.be.true; - expect(el.open, 'closed').to.be.false; - expect(document.activeElement, 'focuses el').to.equal(el); + if (!isSafari) { + await sendKeys({ press: 'Shift+Tab' }); + await focused; + + expect(el.focused, 'focused').to.be.true; + expect(el.open, 'closed').to.be.false; + expect(document.activeElement, 'focuses el').to.equal(el); + } else { + el.focus(); + } // tab through the picker to input2 focused = oneEvent(input1, 'focus'); await sendKeys({ press: 'Shift+Tab' }); await focused; - expect(document.activeElement, 'focuses input 1').to.equal( - input1 - ); + expect(document.activeElement).to.equal(input1); }); it('can close and immediately tab to the next tab stop', async () => { el.focus(); - await nextFrame(); expect(document.activeElement, 'focuses el').to.equal(el); // press down to open the picker const opened = oneEvent(el, 'sp-opened'); @@ -1176,24 +1219,16 @@ export function runPickerTests(): void { await opened; expect(el.open, 'opened').to.be.true; - await waitUntil( - () => isMenuActiveElement(el), - 'first item focused' - ); - const closed = oneEvent(el, 'sp-closed'); - el.open = false; + el.close(); await closed; - expect(el.open).to.be.false; - expect(document.activeElement === el).to.be.true; - - const focused = oneEvent(input2, 'focus'); + expect(el.open, 'open?').to.be.false; + expect(document.activeElement).to.equal(el); await sendKeys({ press: 'Tab' }); - await focused; - expect(el.open).to.be.false; - expect(document.activeElement === input2).to.be.true; + expect(el.open, 'open?').to.be.false; + expect(document.activeElement).to.equal(input2); }); it('can close and immediate shift+tab to the previous tab stop', async () => { el.focus(); @@ -1205,24 +1240,21 @@ export function runPickerTests(): void { await opened; expect(el.open, 'opened').to.be.true; - await waitUntil( - () => isMenuActiveElement(el), - 'first item focused' - ); const closed = oneEvent(el, 'sp-closed'); - el.open = false; + el.close(); await closed; - expect(el.open).to.be.false; - expect(document.activeElement === el).to.be.true; + expect(el.open, 'open?').to.be.false; + expect(document.activeElement).to.equal(el); const focused = oneEvent(input1, 'focus'); sendKeys({ press: 'Shift+Tab' }); await focused; - expect(el.open).to.be.false; - expect(document.activeElement === input1).to.be.true; + expect(el.open, 'open?').to.be.false; + expect(document.activeElement === input1, 'input has focus').to + .be.true; }); }); it('does not open when [readonly]', async () => { @@ -1233,7 +1265,7 @@ export function runPickerTests(): void { el.click(); await elementUpdated(el); - expect(el.open).to.be.false; + expect(el.open, 'open?').to.be.false; }); it('scrolls selected into view on open', async () => { // the Popover is transient, you need to be able to apply custom styles to it... @@ -1253,12 +1285,14 @@ export function runPickerTests(): void { await elementUpdated(el); const opened = oneEvent(el, 'sp-opened'); - el.open = true; + el.focus(); + await sendKeys({ + press: 'ArrowDown', + }); await opened; - await waitUntil( - () => isMenuActiveElement(el), - 'first item focused' - ); + await waitUntil(() => isMenuActiveElement(el), 'menu item focused'); + await nextFrame(); + await nextFrame(); const getParentOffset = (el: HTMLElement): number => { const parentScroll = ( (el as HTMLElement & { assignedSlot: HTMLSlotElement }) @@ -1270,9 +1304,6 @@ export function runPickerTests(): void { expect(getParentOffset(lastItem)).to.be.lessThan(40); expect(getParentOffset(firstItem)).to.be.lessThan(-1); - lastItem.dispatchEvent( - new FocusEvent('focusin', { bubbles: true }) - ); await sendKeys({ press: 'ArrowDown', }); @@ -1313,31 +1344,29 @@ export function runPickerTests(): void { await opened; const tray = el.shadowRoot.querySelector('sp-tray'); - expect(tray).to.not.be.null; + expect(tray, 'has tray').to.not.be.null; // Make a selection let closed = oneEvent(el, 'sp-closed'); const firstItem = el.querySelector('sp-menu-item') as MenuItem; firstItem.click(); - - await elementUpdated(el); await closed; // expect the tray to be closed - expect(el.open).to.be.false; + expect(el.open, 'open?').to.be.false; const button = el.shadowRoot.querySelector( '#button' ) as HTMLButtonElement; - expect(button).to.not.be.null; + expect(button, 'has button').to.not.be.null; // we should have SAFARI_FOCUS_RING_CLASS in the classList - expect(button.classList.contains(SAFARI_FOCUS_RING_CLASS)).to.be + expect(button.classList.contains(SAFARI_FOCUS_RING_CLASS), 'has focus ring?').to.be .true; // picker should still have focus - expect(document.activeElement === el).to.be.true; + expect(document.activeElement).to.equal(el); // click outside (0,0) await sendMouse({ @@ -1350,30 +1379,26 @@ export function runPickerTests(): void { }); // picker should not have focus - expect(document.activeElement === el).to.be.false; + expect(document.activeElement).not.to.equal(el); // Let's use keyboard to open the tray now opened = oneEvent(el, 'sp-opened'); - await sendKeys({ - press: 'Tab', - }); + el.focus(); await sendKeys({ press: 'Enter', }); - await elementUpdated(el); await opened; // Make a selection again closed = oneEvent(el, 'sp-closed'); firstItem.click(); - await elementUpdated(el); await closed; // expect the tray to be closed - expect(el.open).to.be.false; + expect(el.open, 'open?').to.be.false; // we should not have SAFARI_FOCUS_RING_CLASS in the classList - expect(button.classList.contains(SAFARI_FOCUS_RING_CLASS)).to.be + expect(button.classList.contains(SAFARI_FOCUS_RING_CLASS), 'has focus ring?').to.be .false; }); }); @@ -1445,7 +1470,7 @@ export function runPickerTests(): void { await nextFrame(); }); afterEach(async () => { - if (el.open) { + if (el && el.open) { const closed = oneEvent(el, 'sp-closed'); el.open = false; await closed; @@ -1468,7 +1493,7 @@ export function runPickerTests(): void { after(async () => { window.__swc.verbose = false; consoleWarnStub.restore(); - if (el.open) { + if (el?.open) { const closed = oneEvent(el, 'sp-closed'); el.open = false; await closed; @@ -1534,7 +1559,6 @@ export function runPickerTests(): void { await elementUpdated(el); await nextFrame(); await nextFrame(); - expect(consoleWarnStub.called).to.be.true; const spyCall = consoleWarnStub.getCall(0); expect( @@ -1603,7 +1627,7 @@ export function runPickerTests(): void { el.click(); await opened; - expect(el.open).to.be.true; + expect(el.open, 'open?').to.be.true; expect(el.selectedItem?.itemText).to.be.undefined; expect(el.value).to.equal(''); @@ -1611,7 +1635,7 @@ export function runPickerTests(): void { secondItem.click(); await closed; - expect(el.open).to.be.false; + expect(el.open, 'open?').to.be.false; expect(el.selectedItem?.itemText).to.equal('Select Inverse'); expect(el.value).to.equal('option-2'); }); @@ -1657,44 +1681,49 @@ export function runPickerTests(): void { ).to.not.be.null; }); it('toggles between pickers', async () => { - const el2 = await pickerFixture(); const el1 = await pickerFixture(); + const el2 = await pickerFixture(); - (el1.parentElement as HTMLElement).style.float = 'left'; - (el2.parentElement as HTMLElement).style.float = 'left'; el1.id = 'away'; el2.id = 'other'; - await Promise.all([elementUpdated(el1), elementUpdated(el2)]); + expect(el1.open, 'el1 to be closed').to.be.false; + expect(el2.open, 'el2 to be closed').to.be.false; + + const el1open = oneEvent(el1, 'sp-opened'); + let el1closed = oneEvent(el1, 'sp-closed'); + const el2open = oneEvent(el2, 'sp-opened'); + const el2closed = oneEvent(el2, 'sp-closed'); - expect(el1.open, 'closed 1').to.be.false; - expect(el2.open, 'closed 1').to.be.false; - let open = oneEvent(el1, 'sp-opened'); el1.click(); - await open; - expect(el1.open).to.be.true; - expect(el2.open).to.be.false; - open = oneEvent(el2, 'sp-opened'); - let closed = oneEvent(el1, 'sp-closed'); + await el1open; + + expect(el1.open, 'click el1: el1 to be open').to.be.true; + expect(el2.open, 'click el1: el2 to be closed').to.be.false; + el2.click(); - await Promise.all([open, closed]); - expect(el1.open).to.be.false; - expect(el2.open).to.be.true; - open = oneEvent(el1, 'sp-opened'); - closed = oneEvent(el2, 'sp-closed'); + await el1closed; + await el2open; + + expect(el1.open, 'click el2: el1 to be closed').to.be.false; + expect(el2.open, 'click el2: el2 to be open').to.be.true; + el1.click(); - await Promise.all([open, closed]); - expect(el2.open).to.be.false; - expect(el1.open).to.be.true; - closed = oneEvent(el1, 'sp-closed'); + await el2closed; + await el1open; + + expect(el2.open, 'click el1 again: el2 to be closed').to.be.false; + expect(el1.open, 'click el1 again: el1 to be open').to.be.true; + + el1closed = oneEvent(el1, 'sp-closed'); sendKeys({ press: 'Escape', }); - await closed; - expect(el1.open).to.be.false; + await el1closed; + expect(el1.open, 'escape key: el1 to be closed').to.be.false; }); it('displays selected item text by default', async () => { const el = await fixture(html` @@ -1740,22 +1769,12 @@ export function runPickerTests(): void { await opened; expect( - el === document.activeElement, + el.selectedItem === document.activeElement, `activeElement is ${document.activeElement?.localName}` ).to.be.true; - expect( - (el as unknown as TestablePicker).optionsMenu === - el.shadowRoot.activeElement, - `activeElement is ${el.shadowRoot.activeElement?.localName}` - ).to.be.true; expect(firstItem.focused, 'firstItem NOT "focused"').to.be.false; expect(secondItem.focused, 'secondItem "focused"').to.be.true; - expect( - (el as unknown as TestablePicker).optionsMenu.getAttribute( - 'aria-activedescendant' - ) - ).to.equal(secondItem.id); }); it('resets value when item not available', async () => { const el = await fixture(html` @@ -1800,18 +1819,18 @@ export function runPickerTests(): void { await opened; await elementUpdated(el); - expect(el.open).to.be.true; + expect(el.open, 'open?').to.be.true; hoverEl.dispatchEvent(new MouseEvent('mouseenter')); await elementUpdated(el); - expect(el.open).to.be.true; + expect(el.open, 'open?').to.be.true; const closed = oneEvent(el, 'sp-closed'); el.open = false; await closed; await elementUpdated(el); - expect(el.open).to.be.false; + expect(el.open, 'open?').to.be.false; expect(mouseenterSpy.calledOnce).to.be.true; }); it('dispatches events on open/close', async () => { @@ -1854,23 +1873,30 @@ export function runPickerTests(): void { await elementUpdated(el); const input1 = document.createElement('input'); const input2 = document.createElement('input'); + input1.id = 'input1'; + input2.id = 'input2'; const tooltipEl = el.querySelector('sp-tooltip') as Tooltip; el.insertAdjacentElement('beforebegin', input1); el.insertAdjacentElement('afterend', input2); input1.focus(); - expect(document.activeElement === input1).to.be.true; + expect(document.activeElement).to.equal(input1); const tooltipOpened = oneEvent(el, 'sp-opened'); - await sendKeys({ - press: 'Tab', - }); + if (!isSafari) { + await sendKeys({ + press: 'Tab', + }); + } else { + // by default Safari does not focus the button on tab unless user sets preferences + el.focus(); + } await tooltipOpened; expect( document.activeElement === el, `Actually, ${document.activeElement?.localName}` ).to.be.true; - expect(tooltipEl.open).to.be.true; - expect(el.open).to.be.false; - expect(el.focused).to.be.true; + expect(tooltipEl.open, 'tooltipEl open?').to.be.true; + expect(el.open, 'open?').to.be.false; + expect(el.focused, 'el focused?').to.be.true; const menuOpen = oneEvent(el, 'sp-opened'); const tooltipClosed = oneEvent(el, 'sp-closed'); @@ -1879,18 +1905,22 @@ export function runPickerTests(): void { }); await menuOpen; await tooltipClosed; - expect(document.activeElement === el).to.be.true; - expect(tooltipEl.open).to.be.false; - expect(el.open).to.be.true; + const firstOption = el.querySelector('sp-menu-item') as MenuItem; + expect( + document.activeElement === firstOption, + 'firstOption is activeElement' + ).to.be.true; + expect(tooltipEl.open, 'tooltip open').to.be.false; + expect(el.open, 'menu open').to.be.true; const menuClosed = oneEvent(el, 'sp-closed'); await sendKeys({ press: 'Tab', }); await menuClosed; - expect(document.activeElement === el).to.be.false; - expect(tooltipEl.open).to.be.false; - expect(el.open).to.be.false; + expect(document.activeElement).not.to.equal(el); + expect(tooltipEl.open, 'tooltipEl open?').to.be.false; + expect(el.open, 'open?').to.be.false; }); describe('disabled', function () { beforeEach(async function () { @@ -1902,26 +1932,26 @@ export function runPickerTests(): void { await elementUpdated(this.elel); }); it('does not recieve focus from an ``', async function () { - expect(this.el.disabled).to.be.true; - expect(this.el.focused).to.be.false; + expect(this.el.disabled, 'this.el disabled?').to.be.true; + expect(this.el.focused, 'this.el focused?').to.be.false; this.label.click(); await elementUpdated(this.el); - expect(this.el.focused).to.be.false; + expect(this.el.focused, 'this.el focused?').to.be.false; }); it('does not open from `click()`', async function () { - expect(this.el.disabled).to.be.true; - expect(this.el.open).to.be.false; + expect(this.el.disabled, 'this.el disabled?').to.be.true; + expect(this.el.focused, 'this.el open?').to.be.false; this.el.click(); await elementUpdated(this.el); - expect(this.el.open).to.be.false; + expect(this.el.focused, 'this.el open?').to.be.false; }); it('does not open from `sendMouse()`', async function () { - expect(this.el.disabled).to.be.true; - expect(this.el.open).to.be.false; + expect(this.el.disabled, 'this.el disabled?').to.be.true; + expect(this.el.focused, 'this.el open?').to.be.false; const boundingRect = this.el.button.getBoundingClientRect(); @@ -1942,7 +1972,7 @@ export function runPickerTests(): void { await nextFrame(); await nextFrame(); - expect(this.el.open).to.be.false; + expect(this.el.focused, 'this.el open?').to.be.false; }); }); describe('pending', function () { @@ -1955,20 +1985,20 @@ export function runPickerTests(): void { await elementUpdated(this.elel); }); it('receives focus from an ``', async function () { - expect(this.el.focused).to.be.false; + expect(this.el.focused, 'this.el focused?').to.be.false; this.label.click(); await elementUpdated(this.el); - expect(this.el.focused).to.be.true; + expect(this.el.focused, 'this.el focused?').to.be.true; }); it('does not open from `click()`', async function () { - expect(this.el.open).to.be.false; + expect(this.el.focused, 'this.el open?').to.be.false; this.el.click(); await elementUpdated(this.el); - expect(this.el.open).to.be.false; + expect(this.el.focused, 'this.el open?').to.be.false; }); it('manages its "name" value in the accessibility tree when [pending]', async () => { type NamedNode = { name: string; role: string }; @@ -2002,6 +2032,7 @@ export function runPickerTests(): void { await nextFrame(); await nextFrame(); await nextFrame(); + await nextFrame(); // Check that the displayed icon matches the selected item's icon. const picker: Picker = this.el; diff --git a/packages/picker/test/picker-reparenting.test.ts b/packages/picker/test/picker-reparenting.test.ts index 4a337fc046c..d7e517b6d77 100644 --- a/packages/picker/test/picker-reparenting.test.ts +++ b/packages/picker/test/picker-reparenting.test.ts @@ -15,7 +15,14 @@ import '@spectrum-web-components/menu/sp-menu-item.js'; import '@spectrum-web-components/menu/sp-menu-divider.js'; import { Picker } from '@spectrum-web-components/picker'; import { MenuItem } from '@spectrum-web-components/menu'; -import { expect, fixture, html, nextFrame, oneEvent } from '@open-wc/testing'; +import { + elementUpdated, + expect, + fixture, + html, + nextFrame, + oneEvent, +} from '@open-wc/testing'; import '@spectrum-web-components/theme/sp-theme.js'; import '@spectrum-web-components/theme/src/themes.js'; @@ -79,6 +86,7 @@ describe('Reparented Picker', () => { const { picker, before, after } = await fixtureElements(); expect(picker.value).to.equal(''); + picker.id = 'nikki'; const item2 = picker.querySelector('[value="2"]') as MenuItem; const item3 = picker.querySelector('[value="3"]') as MenuItem; @@ -110,11 +118,12 @@ describe('Reparented Picker', () => { opened = oneEvent(picker, 'sp-opened'); picker.click(); await opened; + await nextFrame(); expect(picker.open).to.be.true; expect(picker.value).to.equal('3'); - closed = oneEvent(picker, 'sp-closed'); + opened = oneEvent(picker, 'sp-opened'); before.append(picker); - await closed; + await elementUpdated(picker); expect( (picker as unknown as TestablePicker).optionsMenu.value diff --git a/packages/tabs/test/tabs.test.ts b/packages/tabs/test/tabs.test.ts index 99635a1cc58..98609912043 100644 --- a/packages/tabs/test/tabs.test.ts +++ b/packages/tabs/test/tabs.test.ts @@ -353,48 +353,6 @@ describe('Tabs', () => { await elementUpdated(el); expect(el.selected).to.be.equal('first'); }); - it('prevents [tabindex=0] while `focusin`', async () => { - const el = await fixture(html` - - - - - - - - - `); - - const selected = el.querySelector('[value="first"]') as Tab; - const toBeSelected = el.querySelector('[value="second"]') as Tab; - - await elementUpdated(el); - await waitUntil(() => el.selected === 'first', 'wait for selection'); - - expect(el.selected).to.equal('first'); - expect(selected.tabIndex).to.equal(0); - - toBeSelected.dispatchEvent(new Event('focusin', { bubbles: true })); - - await elementUpdated(el); - - expect(el.selected).to.equal('first'); - expect(selected.tabIndex).to.equal(-1); - - toBeSelected.dispatchEvent(new Event('focusout', { bubbles: true })); - - await elementUpdated(el); - - expect(el.selected).to.equal('first'); - expect(selected.tabIndex).to.equal(0); - - toBeSelected.click(); - - await elementUpdated(el); - - expect(el.selected).to.equal('second'); - expect(toBeSelected.tabIndex).to.equal(0); - }); it('accepts keyboard based selection', async () => { const el = await fixture(html` diff --git a/tools/reactive-controllers/src/FocusGroup.ts b/tools/reactive-controllers/src/FocusGroup.ts index ec2d726cbeb..7cdce8a153c 100644 --- a/tools/reactive-controllers/src/FocusGroup.ts +++ b/tools/reactive-controllers/src/FocusGroup.ts @@ -13,6 +13,7 @@ import type { ReactiveController, ReactiveElement } from 'lit'; type DirectionTypes = 'horizontal' | 'vertical' | 'both' | 'grid'; export type FocusGroupConfig = { + hostDelegatesFocus?: boolean; focusInIndex?: (_elements: T[]) => number; direction?: DirectionTypes | (() => DirectionTypes); elementEnterAction?: (el: T) => void; @@ -63,6 +64,8 @@ export class FocusGroupController public directionLength = 5; + public hostDelegatesFocus = false; + elementEnterAction = (_el: T): void => { return; }; @@ -120,6 +123,7 @@ export class FocusGroupController constructor( host: ReactiveElement, { + hostDelegatesFocus, direction, elementEnterAction, elements, @@ -131,6 +135,7 @@ export class FocusGroupController this.mutationObserver = new MutationObserver(() => { this.handleItemMutation(); }); + this.hostDelegatesFocus = hostDelegatesFocus || false; this.host = host; this.host.addController(this); this._elements = elements; @@ -180,18 +185,60 @@ export class FocusGroupController this.manage(); } - focus(options?: FocusOptions): void { + /** + * resets the focusedItem to initial item + */ + reset(): void { const elements = this.elements; if (!elements.length) return; + this.setCurrentIndexCircularly(this.focusInIndex - this.currentIndex); let focusElement = elements[this.currentIndex]; + if (this.currentIndex < 0) { + return; + } if (!focusElement || !this.isFocusableElement(focusElement)) { this.setCurrentIndexCircularly(1); focusElement = elements[this.currentIndex]; } if (focusElement && this.isFocusableElement(focusElement)) { elements[this.prevIndex]?.setAttribute('tabindex', '-1'); + focusElement.setAttribute('tabindex', '0'); + } + } + + focusOnItem(item?: T, options?: FocusOptions): void { + if ( + item && + this.isFocusableElement(item) && + this.elements.indexOf(item) + ) { + const diff = this.elements.indexOf(item) - this.currentIndex; + this.setCurrentIndexCircularly(diff); + this.elements[this.prevIndex]?.setAttribute('tabindex', '-1'); + } + this.focus(options); + } + + focus(options?: FocusOptions): void { + const elements = this.elements; + if (!elements.length) return; + let focusElement = elements[this.currentIndex]; + if (!focusElement || !this.isFocusableElement(focusElement)) { + this.setCurrentIndexCircularly(1); + focusElement = elements[this.currentIndex]; + } + if (focusElement && this.isFocusableElement(focusElement)) { + if ( + !this.hostDelegatesFocus || + elements[this.prevIndex] !== focusElement + ) { + elements[this.prevIndex]?.setAttribute('tabindex', '-1'); + } focusElement.tabIndex = 0; focusElement.focus(options); + if (this.hostDelegatesFocus && !this.focused) { + this.hostContainsFocus(); + } } } @@ -298,28 +345,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; diff --git a/tools/reactive-controllers/src/RovingTabindex.ts b/tools/reactive-controllers/src/RovingTabindex.ts index 30081c719b4..41b49deeb82 100644 --- a/tools/reactive-controllers/src/RovingTabindex.ts +++ b/tools/reactive-controllers/src/RovingTabindex.ts @@ -45,7 +45,7 @@ export class RovingTabindexController< } manageTabindexes(): void { - if (this.focused) { + if (this.focused && !this.hostDelegatesFocus) { this.updateTabindexes(() => ({ tabIndex: -1 })); } else { this.updateTabindexes((el: HTMLElement): UpdateTabIndexes => { @@ -72,7 +72,6 @@ export class RovingTabindexController< } return; } - el.removeAttribute('tabindex'); const updatable = el as unknown as { requestUpdate?: () => void; }; diff --git a/tools/reactive-controllers/test/roving-tabindex-integration.test.ts b/tools/reactive-controllers/test/roving-tabindex-integration.test.ts index 85a903a14a0..7fa455d765a 100644 --- a/tools/reactive-controllers/test/roving-tabindex-integration.test.ts +++ b/tools/reactive-controllers/test/roving-tabindex-integration.test.ts @@ -272,35 +272,27 @@ describe('tabIndex is cached properly', () => { `); - expect(menuEl.tabIndex).to.equal(0); - expect(menuEl.focusElement?.tabIndex).to.equal(0); - - menuEl.tabIndex = 1; - - await elementUpdated(menuEl); - - expect(menuEl.tabIndex).to.equal(1); - expect(menuEl.focusElement?.tabIndex).to.equal(1); + expect( + menuEl.focusElement?.tabIndex, + 'button tabindex before disabling' + ).to.equal(0); menuEl.disabled = true; await elementUpdated(menuEl); - expect(menuEl.tabIndex).to.equal(-1); - expect(menuEl.focusElement?.tabIndex).to.equal(-1); - - menuEl.tabIndex = 2; - - await elementUpdated(menuEl); - - expect(menuEl.tabIndex).to.equal(-1); - expect(menuEl.focusElement?.tabIndex).to.equal(-1); + expect( + menuEl.focusElement?.tabIndex, + 'button tabindex after disabling' + ).to.equal(-1); menuEl.disabled = false; await elementUpdated(menuEl); - expect(menuEl.tabIndex).to.equal(2); - expect(menuEl.focusElement?.tabIndex).to.equal(2); + expect( + menuEl.focusElement?.tabIndex, + 'button tabindex after setting to 0' + ).to.equal(0); }); });