fix: prevent infinite loops when all children are [disabled]

packages/accordion/src/Accordion.ts

@@ -99,9 +99,16 @@ export class Accordion extends Focusable {
         const items = this.items;
         const focused = items.indexOf(getActiveElement(this) as AccordionItem);
         let next = focused;
-        while (items[next].disabled || next === focused) {
+        let availableItems = items.length;
+        // cycle through the available items in the directions of the offset to find the next non-disabled item
+        while ((items[next].disabled || next === focused) && availableItems) {
+            availableItems -= 1;
             next = (items.length + next + direction) % items.length;
         }
+        // if there are no non-disabled items, skip the work to focus a child
+        if (items[next].disabled || next === focused) {
+            return;
+        }
         items[next].focus();
     }
 

packages/accordion/test/accordion.test.ts

@@ -45,6 +45,29 @@ describe('Accordion', () => {
 
         expect(document.activeElement === el).to.be.false;
     });
+    it('does not accept focus when all children [disabled]', async () => {
+        const el = await fixture<Accordion>(
+            html`
+                <sp-accordion>
+                    <sp-accordion-item disabled label="Heading 1">
+                        <div>Item 1</div>
+                    </sp-accordion-item>
+                    <sp-accordion-item disabled label="Heading 2">
+                        <div>Item 2</div>
+                    </sp-accordion-item>
+                </sp-accordion>
+            `
+        );
+
+        await elementUpdated(el);
+
+        expect(document.activeElement === el).to.be.false;
+
+        el.focus();
+        await elementUpdated(el);
+
+        expect(document.activeElement === el).to.be.false;
+    });
     it('only allows one open item by default', async () => {
         const el = await fixture<Accordion>(Default());
         await elementUpdated(el);

packages/menu/src/Menu.ts

@@ -59,7 +59,6 @@ export class Menu extends SpectrumElement {
         this.onClick = this.onClick.bind(this);
         this.addEventListener('click', this.onClick);
         this.addEventListener('focusin', this.startListeningToKeyboard);
-        this.addEventListener('focusout', this.stopListeningToKeyboard);
     }
 
     public focus(): void {
@@ -90,10 +89,16 @@ export class Menu extends SpectrumElement {
     }
 
     public startListeningToKeyboard(): void {
-        if (this.menuItems.length === 0) {
-            return;
-        }
         this.addEventListener('keydown', this.handleKeydown);
+        this.addEventListener('focusout', this.handleFocusout);
+    }
+
+    public handleFocusout(): void {
+        this.stopListeningToKeyboard();
+        const focusedItem = this.menuItems[this.focusedItemIndex] as MenuItem;
+        if (focusedItem) {
+            focusedItem.focused = false;
+        }
     }
 
     public stopListeningToKeyboard(): void {
@@ -119,18 +124,26 @@ export class Menu extends SpectrumElement {
     }
 
     public focusMenuItemByOffset(offset: number): void {
+        const step = offset || 1;
         const focusedItem = this.menuItems[this.focusedItemIndex] as MenuItem;
         focusedItem.focused = false;
         this.focusedItemIndex =
             (this.menuItems.length + this.focusedItemIndex + offset) %
             this.menuItems.length;
         let itemToFocus = this.menuItems[this.focusedItemIndex] as MenuItem;
-        while (itemToFocus.disabled) {
+        let availableItems = this.menuItems.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.menuItems.length + this.focusedItemIndex + offset) %
+                (this.menuItems.length + this.focusedItemIndex + step) %
                 this.menuItems.length;
             itemToFocus = this.menuItems[this.focusedItemIndex] as MenuItem;
         }
+        // if there are no non-disabled items, skip the work to focus a child
+        if (itemToFocus.disabled) {
+            return;
+        }
         itemToFocus.focused = true;
         this.setAttribute('aria-activedescendant', itemToFocus.id);
         focusedItem.tabIndex = -1;
@@ -145,6 +158,10 @@ export class Menu extends SpectrumElement {
                     if (this.menuItems.length === 0) {
                         return;
                     }
+                    const focusedItem = this.menuItems[
+                        this.focusedItemIndex
+                    ] as MenuItem;
+                    focusedItem.focused = false;
                     if (this.querySelector('[selected]')) {
                         const itemToBlur = this.menuItems[
                             this.focusInItemIndex
@@ -189,6 +206,9 @@ 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;
+        }
     };
 
     public render(): TemplateResult {

packages/menu/test/menu.test.ts

@@ -27,6 +27,7 @@ import {
     tabEvent,
     tEvent,
 } from '../../../test/testing-helpers.js';
+import { spy } from 'sinon';
 
 describe('Menu', () => {
     it('renders empty', async () => {
@@ -60,9 +61,10 @@ describe('Menu', () => {
         expect(document.activeElement === anchor, 'anchor').to.be.true;
     });
     it('renders w/ [disabled] menu items', async () => {
+        const focusinSpy = spy();
         const el = await fixture<Menu>(
             html`
-                <sp-menu tabindex="0">
+                <sp-menu tabindex="0" @focusin=${() => focusinSpy()}>
                     <sp-menu-item disabled>Disabled item</sp-menu-item>
                 </sp-menu>
             `
@@ -76,6 +78,35 @@ describe('Menu', () => {
         await elementUpdated(el);
         expect(document.activeElement === el, 'self not focused, 2').to.be
             .false;
+        expect(focusinSpy.callCount).to.equal(0);
+    });
+    it('renders w/ all [disabled] menu items', async () => {
+        const focusinSpy = spy();
+        const el = await fixture<Menu>(
+            html`
+                <sp-menu tabindex="0" @focusin=${() => focusinSpy()}>
+                    <sp-menu-item disabled>Disabled item 1</sp-menu-item>
+                    <sp-menu-item disabled>Disabled item 2</sp-menu-item>
+                </sp-menu>
+            `
+        );
+        const firstItem = el.querySelector('sp-menu-item') as MenuItem;
+
+        await elementUpdated(el);
+        expect(document.activeElement === el, 'self not focused, 1').to.be
+            .false;
+
+        el.focus();
+        await elementUpdated(el);
+        expect(document.activeElement === el, 'self not focused, 2').to.be
+            .false;
+        expect(focusinSpy.callCount).to.equal(0);
+        firstItem.focus();
+        await elementUpdated(el);
+        expect(document.activeElement === el, 'self not focused, 2').to.be
+            .false;
+        expect(focusinSpy.callCount).to.equal(0);
+        expect(el.matches(':focus-within')).to.be.false;
     });
     it('renders w/ menu items', async () => {
         const el = await fixture<Menu>(
@@ -239,7 +270,6 @@ describe('Menu', () => {
         expect(document.activeElement === el).to.be.true;
         expect(appendedItem.focused).to.be.true;
     });
-
     it('cleans up when tabbing away', async () => {
         const el = await fixture<Menu>(
             html`
@@ -290,4 +320,42 @@ describe('Menu', () => {
         el.dispatchEvent(arrowDownEvent);
         expect(secondItem.focused, 'second').to.be.true;
     });
+    it('handles focus across focused MenuItem removals', async () => {
+        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 selected class="selected">
+                        Save Selection
+                    </sp-menu-item>
+                </sp-menu>
+            `
+        );
+        const firstItem = el.querySelector('.first') as MenuItem;
+        const selectedItem = el.querySelector('.selected') as MenuItem;
+
+        await elementUpdated(el);
+
+        el.focus();
+
+        expect(document.activeElement === el);
+        expect(selectedItem.focused);
+
+        selectedItem.remove();
+        await elementUpdated(el);
+
+        expect(document.activeElement === el);
+        expect(firstItem.focused);
+    });
 });

packages/sidenav/src/Sidenav.ts

@@ -133,9 +133,16 @@ export class SideNav extends Focusable {
         const focused = items.indexOf(getActiveElement(this) as SideNavItem);
         let next = focused;
         next = (items.length + next + direction) % items.length;
-        while (this.isDisabledChild(items[next])) {
+        let availableItems = items.length;
+        // cycle through the available items in the directions of the offset to find the next non-disabled item
+        while (this.isDisabledChild(items[next]) && availableItems) {
+            availableItems -= 1;
             next = (items.length + next + direction) % items.length;
         }
+        // if there are no non-disabled items, skip the work to focus a child
+        if (this.isDisabledChild(items[next])) {
+            return;
+        }
         items[next].focus();
     }
 

packages/sidenav/test/sidenav.test.ts

@@ -68,6 +68,34 @@ describe('Sidenav', () => {
 
         expect(document.activeElement === el).to.be.false;
     });
+    it('does not accept focus when all children [disabled]', async () => {
+        const el = await fixture<SideNav>(
+            html`
+                <sp-sidenav>
+                    <sp-sidenav-item
+                        disabled
+                        value="Section 1"
+                        label="Section 1"
+                    ></sp-sidenav-item>
+                    <sp-sidenav-item
+                        disabled
+                        value="Section 2"
+                        label="Section 2"
+                    ></sp-sidenav-item>
+                </sp-sidenav>
+            `
+        );
+
+        await elementUpdated(el);
+
+        expect(document.activeElement === el).to.be.false;
+
+        el.focus();
+        await elementUpdated(el);
+
+        expect(document.activeElement === el).to.be.false;
+        expect(el.matches(':focus-within')).to.be.false;
+    });
     it('sets manageTabIndex on new children', async () => {
         const el = await fixture<SideNav>(
             html`