fix(action menu): keyboard accessibility omnibus

.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 `<sp-action-menu>`, `<sp-menu>`, and `<sp-picker>`, including:
+ - Numpad keys now work with `<sp-picker>` and `<sp-action-menu>`
+ -`<sp-action-menu>`'s `<sp-menu-item>` elements can now be read by a screen reader ([#4556](https://github.com/adobe/spectrum-web-components/issues/4556))
+ - `<sp-menu-item>` href can now be clicked by a screen reader ([#4997](https://github.com/adobe/spectrum-web-components/issues/4997))
+ - Opening a `<sp-action-menu>`, `<sp-menu>`, and `<sp-picker>` 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/)

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 <sp-field-label> 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 <span> with slot="label", or, text content supplied in a <span> with slot="label-only"',
+                    'which will also be displayed visually as placeholder text.',
+                ],
+            }
+        );
+    }
 }

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,15 +56,15 @@ 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();
         await sendKeys({
             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;
     });
 });

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<ActionMenu>(html`
-                <sp-action-menu label="More Actions" @change=${handleChange}>
-                    <sp-menu-item>One</sp-menu-item>
-                    <sp-menu-item selected value="test" id="root-selected-item">
-                        Two
-                    </sp-menu-item>
-                    <sp-menu-item id="item-with-submenu">
-                        B should be selected
-                        <sp-menu slot="submenu">
-                            <sp-menu-item>A</sp-menu-item>
-                            <sp-menu-item selected id="sub-selected-item">
+                <sp-action-menu id="actionmenu" label="More Actions" debugging>
+                    <sp-menu-item id="item-1">One</sp-menu-item>
+                    <sp-menu-item id="item-2">
+                        Two, with B selected
+                        <sp-menu slot="submenu" id="menu-2" selects="single" debugging>
+                            <sp-menu-item id="item-2a" selected>A</sp-menu-item>
+                            <sp-menu-item id="item-2b">
                                 B
                             </sp-menu-item>
-                            <sp-menu-item>C</sp-menu-item>
                         </sp-menu>
                     </sp-menu-item>
                 </sp-action-menu>
             `);
 
-            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();
+            item2.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();