diff --git a/.changeset/lemon-points-ring.md b/.changeset/lemon-points-ring.md new file mode 100644 index 0000000000..1a3222cdc5 --- /dev/null +++ b/.changeset/lemon-points-ring.md @@ -0,0 +1,16 @@ +--- +'@spectrum-web-components/reactive-controllers': minor +'@spectrum-web-components/action-menu': minor +'@spectrum-web-components/picker': minor +'@spectrum-web-components/menu': minor +--- + +Used WAI ARIA Authoring Practices Guide (APG) to make accessibility improvements for ``, ``, and ``, including: + - Numpad keys now work with `` and `` + -``'s `` elements can now be read by a screen reader ([#4556](https://github.com/adobe/spectrum-web-components/issues/4556)) + - `` href can now be clicked by a screen reader ([#4997](https://github.com/adobe/spectrum-web-components/issues/4997)) + - Opening a ``, ``, and `` with a keyboard now sets focus on an item within the menu. ([#4557](https://github.com/adobe/spectrum-web-components/issues/4557)) + + See the following APG examples for more information: + - [Navigation Menu Example](https://www.w3.org/WAI/ARIA/apg/patterns/menubar/examples/menubar-navigation/) + - [Editor Menubar Example](https://www.w3.org/WAI/ARIA/apg/patterns/menubar/examples/menubar-editor/) diff --git a/packages/action-menu/src/ActionMenu.ts b/packages/action-menu/src/ActionMenu.ts index 24615f07dc..e167887b12 100644 --- a/packages/action-menu/src/ActionMenu.ts +++ b/packages/action-menu/src/ActionMenu.ts @@ -135,4 +135,32 @@ export class ActionMenu extends ObserveSlotPresence( } super.update(changedProperties); } + + protected override hasAccessibleLabel(): boolean { + return ( + !!this.label || + !!this.getAttribute('aria-label') || + !!this.getAttribute('aria-labelledby') || + !!this.appliedLabel || + this.hasLabel || + this.labelOnly + ); + } + + protected override warnNoLabel(): void { + window.__swc.warn( + this, + `<${this.localName}> needs one of the following to be accessible:`, + 'https://opensource.adobe.com/spectrum-web-components/components/action-menu/#accessibility', + { + type: 'accessibility', + issues: [ + `an element with a \`for\` attribute referencing the \`id\` of the \`<${this.localName}>\`, or`, + 'value supplied to the "label" attribute, which will be displayed visually as placeholder text', + 'text content supplied in a with slot="label", or, text content supplied in a with slot="label-only"', + 'which will also be displayed visually as placeholder text.', + ], + } + ); + } } diff --git a/packages/action-menu/test/action-menu-groups.test.ts b/packages/action-menu/test/action-menu-groups.test.ts index d6f88f9934..0d3059bf0c 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 092b213098..fd2543be5d 100644 --- a/packages/action-menu/test/index.ts +++ b/packages/action-menu/test/index.ts @@ -436,56 +436,6 @@ export const testActionMenu = (mode: 'sync' | 'async'): void => { expect(firstRect).to.deep.equal(secondRect); }); - it('opens and selects in a single pointer button interaction', async () => { - const el = await actionMenuFixture(); - const thirdItem = el.querySelector( - 'sp-menu-item:nth-of-type(3)' - ) as MenuItem; - const boundingRect = el.button.getBoundingClientRect(); - - expect(el.value).to.not.equal(thirdItem.value); - const opened = oneEvent(el, 'sp-opened'); - await sendMouse({ - steps: [ - { - type: 'move', - position: [ - boundingRect.x + boundingRect.width / 2, - boundingRect.y + boundingRect.height / 2, - ], - }, - { - type: 'down', - }, - ], - }); - await opened; - - const thirdItemRect = thirdItem.getBoundingClientRect(); - const closed = oneEvent(el, 'sp-closed'); - let selected = ''; - el.addEventListener('change', (event: Event) => { - selected = (event.target as ActionMenu).value; - }); - await sendMouse({ - steps: [ - { - type: 'move', - position: [ - thirdItemRect.x + thirdItemRect.width / 2, - thirdItemRect.y + thirdItemRect.height / 2, - ], - }, - { - type: 'up', - }, - ], - }); - await closed; - - expect(el.open).to.be.false; - expect(selected).to.equal(thirdItem.value); - }); it('has attribute aria-describedby', async () => { const name = 'sp-picker'; const description = 'Rendering a Picker'; @@ -581,94 +531,84 @@ 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 - + + One + + Two, with B selected + + A + B - C `); - const unselectedItem = root.querySelector( - 'sp-menu-item' + const item1 = root.querySelector( + '#item-1' ) as MenuItem; - const selectedItem = root.querySelector( - '#root-selected-item' + const item2 = root.querySelector( + '#item-2' + ) as MenuItem; + const itemA = root.querySelector( + '#item-2a' + ) as MenuItem; + const itemB = root.querySelector( + '#item-2b' ) 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; 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; + + expect(root.open, 'after clicking open: open?').to.be.true; - // close by clicking selected - // (with event listener: should set selected = false) 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; - - // close by clicking selected - // (with event listener: should set selected = false) + expect(root.open, 'after reopening: open?').to.be.true; + 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 07d91e9736..90675e1cde 100644 --- a/packages/combobox/src/Combobox.ts +++ b/packages/combobox/src/Combobox.ts @@ -219,6 +219,7 @@ export class Combobox extends Textfield { (this.availableOptions.length + activeIndex + 1) % this.availableOptions.length; this.activeDescendant = this.availableOptions[nextActiveIndex]; + this.optionEls.forEach(el=> el.setAttribute('aria-selected', el.value === this.activeDescendant?.value ? 'true' : 'false')); } public activatePreviousDescendant(): void { @@ -229,6 +230,7 @@ export class Combobox extends Textfield { (this.availableOptions.length + activeIndex - 1) % this.availableOptions.length; this.activeDescendant = this.availableOptions[previousActiveIndex]; + this.optionEls.forEach(el=> el.setAttribute('aria-selected', el.value === this.activeDescendant?.value ? 'true' : 'false')); } public selectDescendant(): void { @@ -267,9 +269,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 71501f3327..984375aa6d 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 ad82a9a89b..6543990a98 100644 --- a/packages/combobox/test/combobox.test.ts +++ b/packages/combobox/test/combobox.test.ts @@ -487,27 +487,24 @@ 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; - + expect(el.open, 'open?').to.be.true; await elementUpdated(el); - el.focusElement.dispatchEvent(arrowDownEvent()); - - 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(); @@ -537,9 +534,9 @@ 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(); @@ -548,7 +545,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(); @@ -566,9 +563,9 @@ 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 8b79c1b205..1b896ffdf8 100644 --- a/packages/combobox/test/helpers.ts +++ b/packages/combobox/test/helpers.ts @@ -69,9 +69,9 @@ 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/dialog/src/spectrum-dialog.css b/packages/dialog/src/spectrum-dialog.css index a12f1431b0..c72787b882 100644 --- a/packages/dialog/src/spectrum-dialog.css +++ b/packages/dialog/src/spectrum-dialog.css @@ -65,13 +65,12 @@ governing permissions and limitations under the License. } .grid { - grid-template-columns: var( - --mod-dialog-confirm-padding-grid, - var(--spectrum-dialog-confirm-padding-grid) - ) auto 1fr auto minmax(0, auto) var( + grid-template-columns: + var( --mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid) - ); + ) + auto 1fr auto minmax(0, auto) var(--mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid)); grid-template-areas: 'hero hero hero hero hero hero' '. . . . . .' @@ -80,10 +79,12 @@ governing permissions and limitations under the License. '. content content content content .' '. footer footer buttonGroup buttonGroup .' '. . . . . .'; - grid-template-rows: auto var( + grid-template-rows: + auto var( --mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid) - ) auto auto 1fr auto var( + ) + auto auto 1fr auto var( --mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid) ); @@ -248,16 +249,19 @@ governing permissions and limitations under the License. } :host([dismissable]) .grid { - grid-template-columns: var( + grid-template-columns: + var( --mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid) - ) auto 1fr auto minmax(0, auto) minmax( + ) + auto 1fr auto minmax(0, auto) minmax( 0, var( --mod-dialog-confirm-close-button-size, var(--spectrum-dialog-confirm-close-button-size) ) - ) var( + ) + var( --mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid) ); @@ -269,10 +273,12 @@ governing permissions and limitations under the License. '. content content content content content .' '. footer footer buttonGroup buttonGroup buttonGroup .' '. . . . . . .'; - grid-template-rows: auto var( + grid-template-rows: + auto var( --mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid) - ) auto auto 1fr auto var( + ) + auto auto 1fr auto var( --mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid) ); @@ -322,17 +328,21 @@ governing permissions and limitations under the License. :host([mode='fullscreen']) .grid, :host([mode='fullscreenTakeover']) .grid { - grid-template-columns: var( + grid-template-columns: + var( --mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid) - ) 1fr auto auto var( + ) + 1fr auto auto var( --mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid) ); - grid-template-rows: var( + grid-template-rows: + var( --mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid) - ) auto auto 1fr var( + ) + auto auto 1fr var( --mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid) ); @@ -378,13 +388,12 @@ governing permissions and limitations under the License. @media screen and (width <= 700px) { .grid { - grid-template-columns: var( - --mod-dialog-confirm-padding-grid, - var(--spectrum-dialog-confirm-padding-grid) - ) auto 1fr auto minmax(0, auto) var( + grid-template-columns: + var( --mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid) - ); + ) + auto 1fr auto minmax(0, auto) var(--mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid)); grid-template-areas: 'hero hero hero hero hero hero' '. . . . . .' @@ -398,26 +407,31 @@ governing permissions and limitations under the License. .grid, :host([dismissable]) .grid { - grid-template-rows: auto var( + grid-template-rows: + auto var( --mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid) - ) auto auto auto 1fr auto var( + ) + auto auto auto 1fr auto var( --mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid) ); } :host([dismissable]) .grid { - grid-template-columns: var( + grid-template-columns: + var( --mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid) - ) auto 1fr auto minmax(0, auto) minmax( + ) + auto 1fr auto minmax(0, auto) minmax( 0, var( --mod-dialog-confirm-close-button-size, var(--spectrum-dialog-confirm-close-button-size) ) - ) var( + ) + var( --mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid) ); @@ -438,17 +452,21 @@ governing permissions and limitations under the License. :host([mode='fullscreen']) .grid, :host([mode='fullscreenTakeover']) .grid { - grid-template-columns: var( + grid-template-columns: + var( --mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid) - ) 1fr var( + ) + 1fr var( --mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid) ); - grid-template-rows: var( + grid-template-rows: + var( --mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid) - ) auto auto auto 1fr auto var( + ) + auto auto auto 1fr auto var( --mod-dialog-confirm-padding-grid, var(--spectrum-dialog-confirm-padding-grid) ); diff --git a/packages/menu/menu-group.md b/packages/menu/menu-group.md index 4cf9a26c4e..e434869e53 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 752564b7b5..7cf93ac519 100644 --- a/packages/menu/menu-item.md +++ b/packages/menu/menu-item.md @@ -95,12 +95,35 @@ Content assigned to the `value` slot will be placed at the end of the `` can also accept content addressed to its `submenu` slot. Using the `` element with this slot name the options will be surfaced in flyout menu that can be activated by hovering over the root menu item with your pointer or focusing the menu item and pressing the appropriate `ArrowRight` or `ArrowLeft` key based on text direction to move into the submenu. ```html - +

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

+ + + New York + + Central Park + Flushing Meadows Corona Park + Prospect Park + + - Item with submenu - - Additional options - Available on request + San Francisco + + Golden Gate Park + John McLaren Park + Lake Merced Park diff --git a/packages/menu/src/Menu.ts b/packages/menu/src/Menu.ts index 5a64c94aa0..69cd8f40d4 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,15 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { public focusedItemIndex = 0; public focusInItemIndex = 0; + 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(); @@ -166,6 +198,8 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { ); } + this.rovingTabindexController?.clearElementCache(); + return this.cachedChildItems; } @@ -194,7 +228,14 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { return 'menu'; } + /** + * menuitem role based on selection type + */ private resolvedSelects?: SelectsType; + + /** + * menu role based on selection type + */ private resolvedRole?: RoleType; /** @@ -232,10 +273,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 +296,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'; @@ -312,14 +353,34 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { ); this.addEventListener('click', this.handleClick); + this.addEventListener('sp-menu-item-keydown', this.handleKeydown); this.addEventListener('pointerup', this.handlePointerup); - this.addEventListener('focusin', this.handleFocusin); - this.addEventListener('blur', this.handleBlur); this.addEventListener('sp-opened', this.handleSubmenuOpened); this.addEventListener('sp-closed', this.handleSubmenuClosed); } + + /** + * 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; + } + + if (selectedItem && !preventScroll) { + selectedItem.scrollIntoView({ block: 'nearest' }); + } + this.rovingTabindexController?.focusOnItem(selectedItem); + } + public override focus({ preventScroll }: FocusOptions = {}): void { + if (!this.rovingTabindexController) return; if ( !this.childItems.length || this.childItems.every((childItem) => childItem.disabled) @@ -334,12 +395,11 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { super.focus({ preventScroll }); return; } - this.focusMenuItemByOffset(0); - super.focus({ preventScroll }); const selectedItem = this.selectedItems[0]; if (selectedItem && !preventScroll) { selectedItem.scrollIntoView({ block: 'nearest' }); } + this.rovingTabindexController.focus({ preventScroll }); } // if the click and pointerup events are on the same target, we should not @@ -405,49 +465,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 +495,24 @@ 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 quickSelectItem(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 +522,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 +536,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,37 +598,26 @@ 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; + protected navigateBetweenRelatedMenus(event: MenuItemKeydownEvent): void { + const { key, root } = event; event.stopPropagation(); const shouldOpenSubmenu = (this.isLTR && key === 'ArrowRight') || (!this.isLTR && key === 'ArrowLeft'); const shouldCloseSelfAsSubmenu = (this.isLTR && key === 'ArrowLeft') || - (!this.isLTR && key === 'ArrowRight'); + (!this.isLTR && key === 'ArrowRight') || + key === 'Escape'; + const lastFocusedItem = root as MenuItem; if (shouldOpenSubmenu) { - const lastFocusedItem = this.childItems[this.focusedItemIndex]; if (lastFocusedItem?.hasSubmenu) { - // Remove focus while opening overlay from keyboard or the visible focus - // will slip back to the first item in the menu. - lastFocusedItem.openOverlay(); + //open submenu and set focus + lastFocusedItem.openOverlay(true); } } else if (shouldCloseSelfAsSubmenu && this.isSubmenu) { this.dispatchEvent(new Event('close', { bubbles: true })); @@ -610,20 +625,12 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { } } - 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; + if (shiftKey && target !== this && this.hasAttribute('tabindex')) { this.removeAttribute('tabindex'); const replaceTabindex = ( event: FocusEvent | KeyboardEvent @@ -632,7 +639,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 +647,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 (key === ' ' && root?.hasSubmenu) { + // 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?.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.rovingTabindexController?.focusInElement; if (focusedItem) { focusedItem.focused = false; - this.updateSelectedItemIndex(); } }); }, @@ -721,7 +686,9 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { ); } - private _hasUpdatedSelectedItemIndex = false; + public isDebugging(): boolean { + return this.hasAttribute('debugging'); + } public updateSelectedItemIndex(): void { let firstOrFirstSelectedIndex = 0; @@ -746,17 +713,10 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { // Remove "focused" from non-"selected" items ONLY // Preserve "focused" on index===0 when no selection if (itemIndex !== firstOrFirstSelectedIndex) { - childItem.focused = false; + //childItem.focused = false; } } } - 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; @@ -788,20 +748,16 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { this.updateSelectedItemIndex(); this.updateItemFocus(); } + this._willUpdateItems = false; } private updateItemFocus(): void { + const focusInElement = this.rovingTabindexController?.focusInElement; + focusInElement?.setAttribute('tabindex', '0'); if (this.childItems.length == 0) { return; } - const focusInItem = this.childItems[this.focusInItemIndex]; - if ( - (this.getRootNode() as Document).activeElement === - focusInItem.menuData.focusRoot - ) { - this.forwardFocusVisibleToItem(focusInItem); - } } public closeDescendentOverlays(): void { @@ -811,26 +767,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 { @@ -868,14 +804,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))), ]; @@ -920,9 +848,44 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { if (!this.hasAttribute('role') && !this.ignore) { this.setAttribute('role', this.ownRole); } + + /** + * only create an RTI if menu controls keyboard navigation and one does not already exist + */ + if (!this.rovingTabindexController && this.controlsRovingTabindex) { + this.rovingTabindexController = + new RovingTabindexController(this, { + 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.cachedChildItems || [], + isFocusableElement: this.isFocusableElement.bind(this), + }); + } this.updateComplete.then(() => this.updateItemFocus()); } + private isFocusableElement(el: MenuItem): boolean { + return !el.disabled; + } + public override disconnectedCallback(): void { this.cachedChildItems = undefined; this.selectedItems = []; diff --git a/packages/menu/src/MenuGroup.ts b/packages/menu/src/MenuGroup.ts index 69fc3a50ba..7311706d24 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 75c955c946..ab0d1f95d7 100644 --- a/packages/menu/src/MenuItem.ts +++ b/packages/menu/src/MenuItem.ts @@ -54,6 +54,9 @@ type MenuCascadeItem = { ancestorWithSelects?: HTMLElement; }; +/** + * Fires when a menu item is added or updated so that a parent menu can track it. + */ export class MenuItemAddedOrUpdatedEvent extends Event { constructor(item: MenuItem) { super('sp-menu-item-added-or-updated', { @@ -81,6 +84,55 @@ export class MenuItemAddedOrUpdatedEvent extends Event { currentAncestorWithSelects?: Menu; } +/** + * Fires to forward keyboard event information to parent menu. + */ +export class MenuItemKeydownEvent extends KeyboardEvent { + root?: MenuItem; + private _event?: KeyboardEvent; + constructor({ root, event }: { root?: MenuItem; event?: KeyboardEvent }) { + super('sp-menu-item-keydown', { bubbles: true, composed: true }); + this.root = root; + this._event = event; + } + + public override get altKey(): boolean { + return this._event?.altKey || false; + } + + public override get code(): string { + return this._event?.code || ''; + } + + public override get ctrlKey(): boolean { + return this._event?.ctrlKey || false; + } + + public override get isComposing(): boolean { + return this._event?.isComposing || false; + } + + public override get key(): string { + return this._event?.key || ''; + } + + public override get location(): number { + return this._event?.location || 0; + } + + public override get metaKey(): boolean { + return this._event?.metaKey || false; + } + + public override get repeat(): boolean { + return this._event?.repeat || false; + } + + public override get shiftKey(): boolean { + return this._event?.shiftKey || false; + } +} + export type MenuItemChildren = { icon: Element[]; content: Node[] }; /** @@ -108,17 +160,29 @@ export class MenuItem extends LikeAnchor( abortControllerSubmenu!: AbortController; + /** + * whether the menu item is active or has an active descendant + */ @property({ type: Boolean, reflect: true }) public active = false; private dependencyManager = new DependencyManagerController(this); + /** + * whether the menu item has keyboard focus + */ @property({ type: Boolean, reflect: true }) public focused = false; + /** + * whether the menu item is selected + */ @property({ type: Boolean, reflect: true }) public selected = false; + /** + * value of the menu item which is used for selection + */ @property({ type: String }) public get value(): string { return this._value || this.itemText; @@ -140,6 +204,7 @@ export class MenuItem extends LikeAnchor( /** * @private + * text content of the menu item minus whitespace */ public get itemText(): string { return this.itemChildren.content.reduce( @@ -148,6 +213,9 @@ export class MenuItem extends LikeAnchor( ); } + /** + * whether the menu item has a submenu + */ @property({ type: Boolean, reflect: true, attribute: 'has-submenu' }) public hasSubmenu = false; @@ -157,6 +225,9 @@ export class MenuItem extends LikeAnchor( @query('slot[name="icon"]') iconSlot!: HTMLSlotElement; + /** + * whether menu item text content should not wrap + */ @property({ type: Boolean, reflect: true, @@ -175,6 +246,9 @@ export class MenuItem extends LikeAnchor( private submenuElement?: HTMLElement; + /** + * the focusable element of the menu item + */ public override get focusElement(): HTMLElement { return this; } @@ -214,6 +288,8 @@ export class MenuItem extends LikeAnchor( this.addEventListener('click', this.handleClickCapture, { capture: true, }); + this.addEventListener('focus', this.handleFocus); + this.addEventListener('blur', this.handleBlur); new MutationController(this, { config: { @@ -234,21 +310,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(); @@ -256,6 +323,10 @@ export class MenuItem extends LikeAnchor( event.stopPropagation(); return false; } + + if (this.shouldProxyClick()) { + return; + } } private handleSlottableRequest = (event: SlottableRequestEvent): void => { @@ -367,6 +438,9 @@ export class MenuItem extends LikeAnchor( `; } + /** + * determines if item has a submenu and updates the `aria-haspopup` attribute + */ protected manageSubmenu(event: Event & { target: HTMLSlotElement }): void { this.submenuElement = event.target.assignedElements({ flatten: true, @@ -392,6 +466,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')) { @@ -399,11 +474,36 @@ export class MenuItem extends LikeAnchor( } } + /** + * forward key info from keydown event to parent menu + */ + handleKeydown = (event: KeyboardEvent): void => { + const { target } = event; + if (target === this) + 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; @@ -489,10 +589,18 @@ export class MenuItem extends LikeAnchor( this.active = false; } - public async openOverlay(): Promise { + public async openOverlay(focus: boolean = false): Promise { if (!this.hasSubmenu || this.open || this.disabled) { return; } + if (focus) { + const handleSubmenuOpen = (): void => { + this.submenuElement?.focus(); + }; + this.addEventListener('sp-menu-submenu-opened', handleSubmenuOpen, { + once: true, + }); + } this.open = true; this.active = true; this.setAttribute('aria-expanded', 'true'); @@ -518,6 +626,19 @@ export class MenuItem extends LikeAnchor( this.updateAriaSelected(); } + protected override willUpdate(changes: PropertyValues): void { + super.updated(changes); + + // make sure focus returns to the anchor element when submenu is closed + if ( + changes.has('open') && + !this.open && this.hasSubmenu && + this.hasVisibleFocusInTree() + ) { + this.focus(); + } + } + protected override updated(changes: PropertyValues): void { super.updated(changes); if ( @@ -618,8 +739,10 @@ export class MenuItem extends LikeAnchor( selectionRoot?: Menu; cleanupSteps: ((item: MenuItem) => void)[]; } = { + // menu that controls ArrowUp/ArrowDown navigation focusRoot: undefined, parentMenu: undefined, + // menu or menu group that controls selection selectionRoot: undefined, cleanupSteps: [], }; diff --git a/packages/menu/stories/index.ts b/packages/menu/stories/index.ts index 28d99a1275..efc0b96dd3 100644 --- a/packages/menu/stories/index.ts +++ b/packages/menu/stories/index.ts @@ -53,19 +53,25 @@ export class ComplexSlottedGroup extends LitElement { get menu(): Menu { return this.renderRoot.querySelector('sp-menu') as Menu; } + + static override shadowRootOptions = { + ...LitElement.shadowRootOptions, + delegatesFocus: true, + }; + protected override render(): TemplateResult { return html` - - + + Before First - + Sibling 1 Sibling 2 - + After 1 After 2 @@ -84,9 +90,15 @@ export class ComplexSlottedMenu extends LitElement { ) as ComplexSlottedGroup ).menu; } + + static override shadowRootOptions = { + ...LitElement.shadowRootOptions, + delegatesFocus: true, + }; + protected override render(): TemplateResult { return html` - + Middle 1 Middle 2 Middle 3 diff --git a/packages/menu/test/menu-group.test.ts b/packages/menu/test/menu-group.test.ts index bf9e1a9a17..98f2fefa09 100644 --- a/packages/menu/test/menu-group.test.ts +++ b/packages/menu/test/menu-group.test.ts @@ -43,28 +43,7 @@ const focusableItems = (menu: Menu | MenuGroup): MenuItem[] => { describe('Menu group', () => { testForLitDevWarnings( async () => - await fixture( - html` - - - Section Heading - Action 1 - Action 2 - Action 3 - - - - Section Heading - Save - Download - - - ` - ) - ); - it('renders', async () => { - const el = await fixture( - html` + await fixture(html` Section Heading @@ -79,22 +58,40 @@ describe('Menu group', () => { Download - ` - ); + `) + ); + it('renders', async () => { + const el = await fixture(html` + + + Section Heading + Action 1 + Action 2 + Action 3 + + + + Section Heading + Save + Download + + + `); - await waitUntil(() => { - return managedItems(el).length === 5; - }, `expected menu group to manage 5 children, received ${managedItems(el).length} of ${el.childItems.length}`); + await waitUntil( + () => { + return managedItems(el).length === 5; + }, + `expected menu group to manage 5 children, received ${managedItems(el).length} of ${el.childItems.length}` + ); await elementUpdated(el); await expect(el).to.be.accessible(); }); it('manages [slot="header"] content', async () => { - const el = await fixture( - html` - - ` - ); + const el = await fixture(html` + + `); await elementUpdated(el); const slot = el.shadowRoot.querySelector( '[name="header"' @@ -116,46 +113,44 @@ describe('Menu group', () => { expect(header.id).to.equal(''); }); it('handles selects for nested menu groups', async () => { - const el = await fixture( - html` - - First - - Second - - - Multi1 + const el = await fixture(html` + + First + + Second + + + Multi1 + + Multi2 + + + SubInherit1 - Multi2 + SubInherit2 - - SubInherit1 - - SubInherit2 - - - - Single1 - - Single2 - - - - Inherit1 - - Inherit2 - - - - Inherit1 - - Inherit2 - - - - ` - ); + + + Single1 + + Single2 + + + + Inherit1 + + Inherit2 + + + + Inherit1 + + Inherit2 + + + + `); // 1 & 3 should be menuitemradio // 2 shouwl menuitemcheckbox @@ -317,22 +312,20 @@ describe('Menu group', () => { }); it('handles changing managed items for selects changes', async () => { - const el = await fixture( - html` - - First - Second - - Inherit1 - Inherit2 - - SubInherit1 - SubInherit2 - + const el = await fixture(html` + + First + Second + + Inherit1 + Inherit2 + + SubInherit1 + SubInherit2 - - ` - ); + + + `); await waitUntil( () => managedItems(el).length == 6, @@ -381,9 +374,12 @@ describe('Menu group', () => { await elementUpdated(inheritGroup); await elementUpdated(el); - await waitUntil(() => { - return managedItems(inheritGroup).length === 4; - }, `expected new single sub-group to manage 4 items, received ${managedItems(inheritGroup).length} because "selects === ${inheritGroup.selects}`); + await waitUntil( + () => { + return managedItems(inheritGroup).length === 4; + }, + `expected new single sub-group to manage 4 items, received ${managedItems(inheritGroup).length} because "selects === ${inheritGroup.selects}` + ); await waitUntil( () => managedItems(el).length === 2, @@ -419,13 +415,13 @@ describe('Menu group', () => { items.i6 = el.renderRoot.querySelector('#i-6') as MenuItem; items.i7 = el.renderRoot.querySelector('#i-7') as MenuItem; const group = el.renderRoot.querySelector( - '#group' + '#complex-slotted-group' ) as ComplexSlottedGroup; - items.i1 = group.renderRoot.querySelector('#i-1') as MenuItem; - items.i4 = group.renderRoot.querySelector('#i-4') as MenuItem; - items.i10 = group.renderRoot.querySelector('#i-10') as MenuItem; - items.i11 = group.renderRoot.querySelector('#i-11') as MenuItem; - items.i12 = group.renderRoot.querySelector('#i-12') as MenuItem; + items.i1 = group?.renderRoot.querySelector('#i-1') as MenuItem; + items.i4 = group?.renderRoot.querySelector('#i-4') as MenuItem; + items.i10 = group?.renderRoot.querySelector('#i-10') as MenuItem; + items.i11 = group?.renderRoot.querySelector('#i-11') as MenuItem; + items.i12 = group?.renderRoot.querySelector('#i-12') as MenuItem; const rect = items.i9.getBoundingClientRect(); await sendMouse({ diff --git a/packages/menu/test/menu-item.test.ts b/packages/menu/test/menu-item.test.ts index ec6ae45767..bed959601d 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 90bbb2c68b..a6abd9794d 100644 --- a/packages/menu/test/menu-selects.test.ts +++ b/packages/menu/test/menu-selects.test.ts @@ -29,15 +29,13 @@ describe('Menu [selects]', () => { let el!: Menu; let options!: MenuItem[]; beforeEach(async () => { - el = await fixture( - html` - - Option 1 - Option 2 - Option 3 - - ` - ); + el = await fixture(html` + + Option 1 + Option 2 + Option 3 + + `); options = [...el.querySelectorAll('sp-menu-item')] as MenuItem[]; await Promise.all(options.map((option) => option.updateComplete)); await nextFrame(); @@ -151,17 +149,15 @@ describe('Menu [selects] w/ group', () => { let el!: Menu; let options!: MenuItem[]; beforeEach(async () => { - el = await fixture( - html` - - - Option 1 - Option 2 - Option 3 - - - ` - ); + el = await fixture(html` + + + Option 1 + Option 2 + Option 3 + + + `); options = [...el.querySelectorAll('sp-menu-item')] as MenuItem[]; await Promise.all(options.map((option) => option.updateComplete)); await nextFrame(); @@ -275,17 +271,15 @@ describe('Menu w/ group [selects]', () => { let group!: MenuGroup; let options!: MenuItem[]; beforeEach(async () => { - el = await fixture( - html` - - - Option 1 - Option 2 - Option 3 - - - ` - ); + el = await fixture(html` + + + Option 1 + Option 2 + Option 3 + + + `); group = el.querySelector('sp-menu-group') as MenuGroup; options = [...el.querySelectorAll('sp-menu-item')] as MenuItem[]; await Promise.all(options.map((option) => option.updateComplete)); @@ -403,22 +397,20 @@ describe('Menu w/ groups [selects]', () => { let groupB!: MenuGroup; let options!: MenuItem[]; beforeEach(async () => { - el = await fixture( - html` - - - Option 1a - Option 2a - Option 3a - - - Option 1b - Option 2b - Option 3b - - - ` - ); + el = await fixture(html` + + + Option 1a + Option 2a + Option 3a + + + Option 1b + Option 2b + Option 3b + + + `); groupA = el.querySelector('sp-menu-group:first-child') as MenuGroup; groupB = el.querySelector('sp-menu-group:last-child') as MenuGroup; options = [...el.querySelectorAll('sp-menu-item')] as MenuItem[]; @@ -638,16 +630,17 @@ describe('Menu w/ groups [selects]', () => { input.focus(); expect(document.activeElement === input).to.be.true; await sendKeys({ press: 'Shift+Tab' }); - expect(document.activeElement === groupA).to.be.true; + expect(document.activeElement === options[0]).to.be.true; await sendKeys({ press: 'ArrowDown' }); + expect(document.activeElement === options[1]).to.be.true; await sendKeys({ press: 'ArrowUp' }); await elementUpdated(el); let optionCount = 0; for (const option of options) { const parentElement = option.parentElement as Menu; - expect(document.activeElement === parentElement, 'parent focused') - .to.be.true; + expect(document.activeElement === option, 'option focused').to.be + .true; expect(option.focused, `option ${optionCount} visually focused`).to .be.true; await sendKeys({ press: 'Space' }); diff --git a/packages/menu/test/menu.test.ts b/packages/menu/test/menu.test.ts index 74d2d41396..bec103cc41 100644 --- a/packages/menu/test/menu.test.ts +++ b/packages/menu/test/menu.test.ts @@ -413,8 +413,6 @@ describe('Menu', () => { }) ); await nextFrame(); - // re-bind keyevents - el.startListeningToKeyboard(); // focus management should start again from the first item. el.dispatchEvent(arrowDownEvent()); expect(secondItem.focused, 'second').to.be.true; diff --git a/packages/menu/test/submenu.test.ts b/packages/menu/test/submenu.test.ts index 4365b8d60b..1f13318983 100644 --- a/packages/menu/test/submenu.test.ts +++ b/packages/menu/test/submenu.test.ts @@ -130,14 +130,17 @@ describe('Submenu', () => { }); await opened; + const rootItem = this.el.querySelector('.root') as MenuItem; let submenu = this.el.querySelector('[slot="submenu"]') as Menu; let submenuItem = this.el.querySelector( - '.submenu-item-2' + '.submenu-item-1' ) as MenuItem; expect(this.rootItem.open).to.be.true; + + //opening a menu via keyboard should set focus on first item expect( - submenu === document.activeElement, + submenuItem === document.activeElement, `${document.activeElement?.id}` ).to.be.true; @@ -148,8 +151,10 @@ describe('Submenu', () => { await closed; expect(this.rootItem.open).to.be.false; + + //closing a submenu via keyboard should set focus on its parent menuitem expect( - this.el === document.activeElement, + rootItem === document.activeElement, `${document.activeElement?.id}` ).to.be.true; @@ -160,9 +165,10 @@ describe('Submenu', () => { await opened; submenu = this.el.querySelector('[slot="submenu"]') as Menu; - submenuItem = this.el.querySelector('.submenu-item-2') as MenuItem; expect(this.rootItem.open).to.be.true; + expect(submenuItem.focused).to.be.true; + expect(document.activeElement === submenuItem).to.be.true; await sendKeys({ press: 'ArrowDown', @@ -170,9 +176,9 @@ describe('Submenu', () => { await elementUpdated(submenu); await elementUpdated(submenuItem); - expect(submenu.getAttribute('aria-activedescendant')).to.equal( - submenuItem.id - ); + submenuItem = this.el.querySelector('.submenu-item-2') as MenuItem; + expect(submenuItem.focused).to.be.true; + expect(document.activeElement === submenuItem).to.be.true; closed = oneEvent(this.rootItem, 'sp-closed'); await sendKeys({ @@ -552,7 +558,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); diff --git a/packages/picker/src/InteractionController.ts b/packages/picker/src/InteractionController.ts index 5c93d6a3bb..3810076db5 100644 --- a/packages/picker/src/InteractionController.ts +++ b/packages/picker/src/InteractionController.ts @@ -158,6 +158,7 @@ export class InteractionController implements ReactiveController { ) { this.preventNextToggle = 'yes'; } + if(this.preventNextToggle === 'no') this.host.close(); } public handleActivate(_event: Event): void {} diff --git a/packages/picker/src/MobileController.ts b/packages/picker/src/MobileController.ts index 0599d6d792..cde57289c4 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 30671866be..5f78a6d0ce 100644 --- a/packages/picker/src/Picker.ts +++ b/packages/picker/src/Picker.ts @@ -70,6 +70,11 @@ const chevronClass = { export const DESCRIPTION_ID = 'option-picker'; export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { + static override shadowRootOptions = { + ...Focusable.shadowRootOptions, + delegatesFocus: true, + }; + public isMobile = new MatchMediaController(this, IS_MOBILE); public strategy!: DesktopController | MobileController; @@ -205,11 +210,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.focusElement.click(); + } + + // 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(); } @@ -219,18 +229,22 @@ 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(); - } } - + /** + * @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'; @@ -256,14 +270,21 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { protected handleKeydown = (event: KeyboardEvent): void => { this.focused = true; - if (event.code !== 'ArrowDown' && event.code !== 'ArrowUp') { + if (!['ArrowUp', 'ArrowDown', 'Enter', ' '].includes(event.code)) { return; } event.stopPropagation(); event.preventDefault(); - this.toggle(true); + this.keyboardOpen(); }; + protected async keyboardOpen(): Promise { + this.addEventListener('sp-opened', () => this.optionsMenu?.focusOnFirstSelectedItem(), { + once: true, + }); + this.toggle(true); + } + protected async setValueFromItem( item: MenuItem, menuChangeEvent?: Event @@ -329,11 +350,6 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { if (this.strategy) { this.strategy.open = this.open; } - if (this.open) { - this._selfManageFocusElement = true; - } else { - this._selfManageFocusElement = false; - } } public close(): void { @@ -452,6 +468,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { aria-hidden="true" name="tooltip" id="tooltip" + @keydown=${this.handleKeydown} @slotchange=${this.handleTooltipSlotchange} > `, @@ -466,6 +483,33 @@ 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; @@ -491,15 +535,9 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { this.tooltipEl.disabled = this.open; } return html` - @@ -523,6 +561,20 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { `; } + protected override willUpdate(changes: PropertyValues): void { + super.willUpdate(changes); + if ( + changes.has('open') && + !this.open && + this.focusElement === this.optionsMenu + && this.focusElement?.matches(':focus-within') + ) { + this.updateComplete.then(async () => { + this.button.focus(); + }); + } + } + protected override update(changes: PropertyValues): void { if (this.selects) { /** @@ -549,7 +601,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { // 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); @@ -570,25 +622,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(); } }); } @@ -667,11 +702,16 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { ); } + public isDebugging(): boolean { + return this.hasAttribute('debugging'); + } + protected get renderMenu(): TemplateResult { const menu = html` { requestAnimationFrame(() => { @@ -719,6 +767,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; @@ -727,6 +778,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; @@ -813,6 +867,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 { @@ -848,33 +903,22 @@ export class Picker extends PickerBase { protected override handleKeydown = (event: KeyboardEvent): void => { const { code } = event; + const handledCodes = ['ArrowUp', 'ArrowDown', 'ArrowLeft', 'ArrowRight', 'Enter', ' '].includes(code); + const openCodes = ['ArrowUp', 'ArrowDown', 'Enter', ' '].includes(code); this.focused = true; - if (!code.startsWith('Arrow') || this.readonly || this.pending) { + if (!handledCodes || this.readonly || this.pending) { return; } - if (code === 'ArrowUp' || code === 'ArrowDown') { - this.toggle(true); + if (openCodes) { + this.keyboardOpen(); event.preventDefault(); 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) { - return; - } - if (!this.value || nextIndex !== selectedIndex) { - this.setValueFromItem(this.menuItems[nextIndex]); - } + const nextItem = this.optionsMenu?.quickSelectItem(this.selectedItem, code === '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 94a35f2bab..e16f6b37c5 100644 --- a/packages/picker/test/index.ts +++ b/packages/picker/test/index.ts @@ -69,8 +69,11 @@ 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; @@ -441,25 +444,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); @@ -551,7 +535,7 @@ export function runPickerTests(): void { 'finally, not visually focused' ); }); - 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; @@ -573,7 +557,7 @@ export function runPickerTests(): void { await opened; expect(el.open).to.be.true; - expect(firstItem.focused, 'still not visually focused').to.be.false; + expect(firstItem.focused).to.be.true; }); it('opens and selects in a single pointer button interaction', async () => { await nextFrame(); @@ -622,6 +606,7 @@ export function runPickerTests(): void { expect(el.open).to.be.false; expect(el.value).to.equal(thirdItem.value); }); + // TODO why is this timing out on Chromium? it('opens/closes multiple times', async () => { await nextFrame(); await nextFrame(); @@ -926,10 +911,13 @@ export function runPickerTests(): void { expect(el.selectedItem?.itemText).to.equal('Deselect'); expect(el.value).to.equal('Deselect'); }); + //TODO: not sure why this is failing it('quick selects on ArrowLeft/Right', async () => { + //el.classList.add('debugging'); const selectionSpy = spy(); el.addEventListener('change', (event: Event) => { const { value } = event.target as Picker; + console.log('change', value, el.selectedItem?.itemText, el.appliedLabel); selectionSpy(value); }); @@ -946,21 +934,21 @@ 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', @@ -982,9 +970,9 @@ 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(); @@ -1016,9 +1004,11 @@ 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' }); + if(!isSafari) { + await sendKeys({ press: 'Tab' }); + expect(document.activeElement === input).to.be.true; + await sendKeys({ press: 'Shift+Tab' }); + } expect(document.activeElement === el).to.be.true; const opened = oneEvent(el, 'sp-opened'); sendKeys({ press: 'Enter' }); @@ -1053,9 +1043,11 @@ 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' }); + if(!isSafari) { + await sendKeys({ press: 'Tab' }); + expect(document.activeElement === input).to.be.true; + await sendKeys({ press: 'Shift+Tab' }); + } expect(document.activeElement === el).to.be.true; const opened = oneEvent(el, 'sp-opened'); sendKeys({ down: 'Enter' }); @@ -1083,7 +1075,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'); @@ -1121,14 +1113,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; @@ -1148,14 +1146,17 @@ 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' }); @@ -1172,10 +1173,12 @@ export function runPickerTests(): void { const opened = oneEvent(el, 'sp-opened'); await sendKeys({ press: 'ArrowUp' }); await opened; + const firstItem = el.querySelector('sp-menu-item') as MenuItem; expect(el.open, 'opened').to.be.true; await waitUntil( - () => isMenuActiveElement(el), + // menu delegates focus + () => document.activeElement === firstItem, 'first item focused' ); @@ -1251,12 +1254,17 @@ 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' + 'menu item focused' ); + await nextFrame(); + await nextFrame(); const getParentOffset = (el: HTMLElement): number => { const parentScroll = ( (el as HTMLElement & { assignedSlot: HTMLSlotElement }) @@ -1268,9 +1276,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', }); @@ -1279,6 +1284,7 @@ export function runPickerTests(): void { expect(getParentOffset(lastItem)).to.be.greaterThan(40); expect(getParentOffset(firstItem)).to.be.greaterThan(-1); }); + //TODO: This times out before the first open event is dispatched it('manages focus-ring styles', async () => { if (!isWebKit()) { return; @@ -1352,9 +1358,7 @@ export function runPickerTests(): void { // Let's use keyboard to open the tray now opened = oneEvent(el, 'sp-opened'); - await sendKeys({ - press: 'Tab', - }); + el.focus(); await sendKeys({ press: 'Enter', }); @@ -1532,7 +1536,6 @@ export function runPickerTests(): void { await elementUpdated(el); await nextFrame(); await nextFrame(); - expect(consoleWarnStub.called).to.be.true; const spyCall = consoleWarnStub.getCall(0); expect( @@ -1738,22 +1741,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` @@ -1852,15 +1845,22 @@ 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; 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, @@ -1877,9 +1877,10 @@ 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({ diff --git a/packages/tabs/test/tabs.test.ts b/packages/tabs/test/tabs.test.ts index 99635a1cc5..9860991204 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/packages/tray/stories/tray.stories.ts b/packages/tray/stories/tray.stories.ts index 11578eb2fa..8570dd02fa 100644 --- a/packages/tray/stories/tray.stories.ts +++ b/packages/tray/stories/tray.stories.ts @@ -70,3 +70,14 @@ export const menu = (args: StoryArgs): TemplateResult => { `; }; + +export const buttons = (args: StoryArgs): TemplateResult => { + return html` + + Toggle menu + + Can you click me? + + + `; +} diff --git a/tools/reactive-controllers/src/FocusGroup.ts b/tools/reactive-controllers/src/FocusGroup.ts index ec2d726cbe..c8d31ae32a 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 = { + delegatesFocus?: boolean; focusInIndex?: (_elements: T[]) => number; direction?: DirectionTypes | (() => DirectionTypes); elementEnterAction?: (el: T) => void; @@ -63,6 +64,8 @@ export class FocusGroupController public directionLength = 5; + public delegatesFocus = false; + elementEnterAction = (_el: T): void => { return; }; @@ -120,6 +123,7 @@ export class FocusGroupController constructor( host: ReactiveElement, { + delegatesFocus, direction, elementEnterAction, elements, @@ -131,6 +135,7 @@ export class FocusGroupController this.mutationObserver = new MutationObserver(() => { this.handleItemMutation(); }); + this.delegatesFocus = delegatesFocus || false; this.host = host; this.host.addController(this); this._elements = elements; @@ -180,6 +185,16 @@ export class FocusGroupController this.manage(); } + 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); + item = this.elements[this.currentIndex]; + this.elements[this.prevIndex]?.setAttribute('tabindex', '-1'); + } + this.focus(options); + } + focus(options?: FocusOptions): void { const elements = this.elements; if (!elements.length) return; @@ -189,7 +204,9 @@ export class FocusGroupController focusElement = elements[this.currentIndex]; } if (focusElement && this.isFocusableElement(focusElement)) { - elements[this.prevIndex]?.setAttribute('tabindex', '-1'); + if (!this.delegatesFocus || elements[this.prevIndex] !== focusElement) { + elements[this.prevIndex]?.setAttribute('tabindex', '-1'); + } focusElement.tabIndex = 0; focusElement.focus(options); } @@ -298,28 +315,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 30081c719b..6bda036346 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.delegatesFocus) { this.updateTabindexes(() => ({ tabIndex: -1 })); } else { this.updateTabindexes((el: HTMLElement): UpdateTabIndexes => {