fix(menu): manage tabindex and focus entry correctly

adobe · Apr 13, 2021 · 3b1a250 · 3b1a250
1 parent 535113d
commit 3b1a250
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 81 deletions.
diff --git a/packages/menu/src/Menu.ts b/packages/menu/src/Menu.ts
@@ -64,6 +64,7 @@ export class Menu extends SpectrumElement {
         this.onClick = this.onClick.bind(this);
         this.addEventListener('click', this.onClick);
         this.addEventListener('focusin', this.startListeningToKeyboard);
+        this.addEventListener('focus', this.focus);
     }
 
     public focus(): void {
@@ -166,7 +167,6 @@ export class Menu extends SpectrumElement {
         itemToFocus.focused = true;
         itemToFocus.scrollIntoView({ block: 'nearest' });
         this.setAttribute('aria-activedescendant', itemToFocus.id);
-        focusedItem.tabIndex = -1;
     }
 
     private prepareToCleanUp(): void {
@@ -182,17 +182,7 @@ export class Menu extends SpectrumElement {
                         this.focusedItemIndex
                     ] as MenuItem;
                     focusedItem.focused = false;
-                    if (this.querySelector('[selected]')) {
-                        const itemToBlur = this.menuItems[
-                            this.focusInItemIndex
-                        ] as MenuItem;
-                        itemToBlur.tabIndex = -1;
-                    }
                     this.updateSelectedItemIndex();
-                    const itemToFocus = this.menuItems[
-                        this.focusInItemIndex
-                    ] as MenuItem;
-                    itemToFocus.tabIndex = 0;
                 });
             },
             { once: true }
@@ -225,7 +215,6 @@ export class Menu extends SpectrumElement {
         }
         this.updateSelectedItemIndex();
         const focusInItem = this.menuItems[this.focusInItemIndex] as MenuItem;
-        focusInItem.tabIndex = 0;
         if ((this.getRootNode() as Document).activeElement === this) {
             focusInItem.focused = true;
         }

diff --git a/packages/menu/src/MenuItem.ts b/packages/menu/src/MenuItem.ts
@@ -83,6 +83,7 @@ export class MenuItem extends ActionButton {
     }
 
     protected firstUpdated(changes: PropertyValues): void {
+        this.setAttribute('tabindex', '-1');
         super.firstUpdated(changes);
         if (!this.hasAttribute('id')) {
             this.id = `sp-menu-item-${MenuItem.instanceCount++}`;

diff --git a/packages/menu/test/menu.test.ts b/packages/menu/test/menu.test.ts
@@ -112,47 +112,33 @@ describe('Menu', () => {
         const el = await fixture<Menu>(
             html`
                 <sp-menu>
-                    <sp-menu-item>
-                        Deselect
-                    </sp-menu-item>
-                    <sp-menu-item>
-                        Select Inverse
-                    </sp-menu-item>
-                    <sp-menu-item>
-                        Feather...
-                    </sp-menu-item>
-                    <sp-menu-item>
-                        Select and Mask...
-                    </sp-menu-item>
+                    <sp-menu-item>Deselect</sp-menu-item>
+                    <sp-menu-item>Select Inverse</sp-menu-item>
+                    <sp-menu-item>Feather...</sp-menu-item>
+                    <sp-menu-item>Select and Mask...</sp-menu-item>
                     <sp-menu-divider></sp-menu-divider>
-                    <sp-menu-item>
-                        Save Selection
-                    </sp-menu-item>
-                    <sp-menu-item disabled>
-                        Make Work Path
-                    </sp-menu-item>
+                    <sp-menu-item>Save Selection</sp-menu-item>
+                    <sp-menu-item disabled>Make Work Path</sp-menu-item>
                 </sp-menu>
             `
         );
 
         await elementUpdated(el);
 
+        const inTabindexElement = el.querySelector(
+            '[tabindex]:not([tabindex="-1"])'
+        );
+        expect(inTabindexElement).to.be.null;
         await expect(el).to.be.accessible();
     });
 
     it('renders w/ selected', async () => {
         const el = await fixture<Menu>(
             html`
                 <sp-menu>
-                    <sp-menu-item>
-                        Not Selected
-                    </sp-menu-item>
-                    <sp-menu-item selected>
-                        Selected
-                    </sp-menu-item>
-                    <sp-menu-item>
-                        Other
-                    </sp-menu-item>
+                    <sp-menu-item>Not Selected</sp-menu-item>
+                    <sp-menu-item selected>Selected</sp-menu-item>
+                    <sp-menu-item>Other</sp-menu-item>
                 </sp-menu>
             `
         );
@@ -166,25 +152,13 @@ describe('Menu', () => {
         const el = await fixture<Menu>(
             html`
                 <sp-menu>
-                    <sp-menu-item>
-                        Deselect
-                    </sp-menu-item>
-                    <sp-menu-item>
-                        Select Inverse
-                    </sp-menu-item>
-                    <sp-menu-item>
-                        Feather...
-                    </sp-menu-item>
-                    <sp-menu-item>
-                        Select and Mask...
-                    </sp-menu-item>
+                    <sp-menu-item>Deselect</sp-menu-item>
+                    <sp-menu-item>Select Inverse</sp-menu-item>
+                    <sp-menu-item>Feather...</sp-menu-item>
+                    <sp-menu-item>Select and Mask...</sp-menu-item>
                     <sp-menu-divider></sp-menu-divider>
-                    <sp-menu-item>
-                        Save Selection
-                    </sp-menu-item>
-                    <sp-menu-item disabled>
-                        Make Work Path
-                    </sp-menu-item>
+                    <sp-menu-item>Save Selection</sp-menu-item>
+                    <sp-menu-item disabled>Make Work Path</sp-menu-item>
                 </sp-menu>
             `
         );
@@ -225,9 +199,7 @@ describe('Menu', () => {
                 <sp-menu>
                     <sp-menu-group>
                         <span slot="header">Options</span>
-                        <sp-menu-item>
-                            Deselect
-                        </sp-menu-item>
+                        <sp-menu-item>Deselect</sp-menu-item>
                     </sp-menu-group>
                 </sp-menu>
             `
@@ -274,15 +246,9 @@ describe('Menu', () => {
         const el = await fixture<Menu>(
             html`
                 <sp-menu tabindex="0">
-                    <sp-menu-item>
-                        Deselect
-                    </sp-menu-item>
-                    <sp-menu-item>
-                        Select Inverse
-                    </sp-menu-item>
-                    <sp-menu-item>
-                        Third Item
-                    </sp-menu-item>
+                    <sp-menu-item>Deselect</sp-menu-item>
+                    <sp-menu-item>Select Inverse</sp-menu-item>
+                    <sp-menu-item>Third Item</sp-menu-item>
                 </sp-menu>
             `
         );
@@ -324,18 +290,10 @@ describe('Menu', () => {
         const el = await fixture<Menu>(
             html`
                 <sp-menu id="test">
-                    <sp-menu-item class="first">
-                        Deselect
-                    </sp-menu-item>
-                    <sp-menu-item>
-                        Invert Selection
-                    </sp-menu-item>
-                    <sp-menu-item>
-                        Feather...
-                    </sp-menu-item>
-                    <sp-menu-item>
-                        Select and Mask...
-                    </sp-menu-item>
+                    <sp-menu-item class="first">Deselect</sp-menu-item>
+                    <sp-menu-item>Invert Selection</sp-menu-item>
+                    <sp-menu-item>Feather...</sp-menu-item>
+                    <sp-menu-item>Select and Mask...</sp-menu-item>
                     <sp-menu-item selected class="selected">
                         Save Selection
                     </sp-menu-item>