Fix tabs when they programmatically become disabled (#2051)

* additional fixes for tab component roving tabindex

* prettier

* fix tests

* fix tests

* prettier

* fix tabs

* Update src/components/tab/tab.component.ts

Co-authored-by: Manuel Correia <20400660+manuelc2209@users.noreply.github.com>

---------

Co-authored-by: Manuel Correia <20400660+manuelc2209@users.noreply.github.com>

src/components/radio-button/radio-button.component.ts

@@ -125,7 +125,6 @@ export default class SlRadioButton extends ShoelaceElement {
           aria-disabled=${this.disabled}
           type="button"
           value=${ifDefined(this.value)}
-          tabindex="${this.checked ? '0' : '-1'}"
           @blur=${this.handleBlur}
           @focus=${this.handleFocus}
           @click=${this.handleClick}

src/components/radio-group/radio-group.component.ts

@@ -169,7 +169,7 @@ export default class SlRadioGroup extends ShoelaceElement implements ShoelaceFor
       radio.checked = false;
 
       if (!this.hasButtonGroup) {
-        radio.tabIndex = -1;
+        radio.setAttribute('tabindex', '-1');
       }
     });
 
@@ -177,7 +177,7 @@ export default class SlRadioGroup extends ShoelaceElement implements ShoelaceFor
     radios[index].checked = true;
 
     if (!this.hasButtonGroup) {
-      radios[index].tabIndex = 0;
+      radios[index].setAttribute('tabindex', '0');
       radios[index].focus();
     } else {
       radios[index].shadowRoot!.querySelector('button')!.focus();
@@ -226,10 +226,10 @@ export default class SlRadioGroup extends ShoelaceElement implements ShoelaceFor
         const buttonRadio = radios[0].shadowRoot?.querySelector('button');
 
         if (buttonRadio) {
-          buttonRadio.tabIndex = 0;
+          buttonRadio.setAttribute('tabindex', '0');
         }
       } else {
-        radios[0].tabIndex = 0;
+        radios[0].setAttribute('tabindex', '0');
       }
     }
 

src/components/tab-group/tab-group.component.ts

@@ -50,6 +50,7 @@ export default class SlTabGroup extends ShoelaceElement {
   private mutationObserver: MutationObserver;
   private resizeObserver: ResizeObserver;
   private tabs: SlTab[] = [];
+  private focusableTabs: SlTab[] = [];
   private panels: SlTabPanel[] = [];
 
   @query('.tab-group') tabGroup: HTMLElement;
@@ -123,14 +124,10 @@ export default class SlTabGroup extends ShoelaceElement {
     this.resizeObserver.unobserve(this.nav);
   }
 
-  private getAllTabs(options: { includeDisabled: boolean } = { includeDisabled: true }) {
+  private getAllTabs() {
     const slot = this.shadowRoot!.querySelector<HTMLSlotElement>('slot[name="nav"]')!;
 
-    return [...(slot.assignedElements() as SlTab[])].filter(el => {
-      return options.includeDisabled
-        ? el.tagName.toLowerCase() === 'sl-tab'
-        : el.tagName.toLowerCase() === 'sl-tab' && !el.disabled;
-    });
+    return slot.assignedElements() as SlTab[];
   }
 
   private getAllPanels() {
@@ -178,48 +175,44 @@ export default class SlTabGroup extends ShoelaceElement {
     if (['ArrowLeft', 'ArrowRight', 'ArrowUp', 'ArrowDown', 'Home', 'End'].includes(event.key)) {
       const activeEl = this.tabs.find(t => t.matches(':focus'));
       const isRtl = this.matches(':dir(rtl)');
+      let nextTab: null | SlTab = null;
 
       if (activeEl?.tagName.toLowerCase() === 'sl-tab') {
-        let index = this.tabs.indexOf(activeEl);
-
         if (event.key === 'Home') {
-          index = 0;
+          nextTab = this.focusableTabs[0];
         } else if (event.key === 'End') {
-          index = this.tabs.length - 1;
+          nextTab = this.focusableTabs[this.focusableTabs.length - 1];
         } else if (
           (['top', 'bottom'].includes(this.placement) && event.key === (isRtl ? 'ArrowRight' : 'ArrowLeft')) ||
           (['start', 'end'].includes(this.placement) && event.key === 'ArrowUp')
         ) {
-          index--;
+          const currentIndex = this.tabs.findIndex(el => el === activeEl);
+          nextTab = this.findNextFocusableTab(currentIndex, 'backward');
         } else if (
           (['top', 'bottom'].includes(this.placement) && event.key === (isRtl ? 'ArrowLeft' : 'ArrowRight')) ||
           (['start', 'end'].includes(this.placement) && event.key === 'ArrowDown')
         ) {
-          index++;
+          const currentIndex = this.tabs.findIndex(el => el === activeEl);
+          nextTab = this.findNextFocusableTab(currentIndex, 'forward');
         }
 
-        if (index < 0) {
-          index = this.tabs.length - 1;
+        if (!nextTab) {
+          return;
         }
 
-        if (index > this.tabs.length - 1) {
-          index = 0;
-        }
-
-        const currentTab = this.tabs[index];
-        currentTab.tabIndex = 0;
-        currentTab.focus({ preventScroll: true });
+        nextTab.tabIndex = 0;
+        nextTab.focus({ preventScroll: true });
 
         if (this.activation === 'auto') {
-          this.setActiveTab(currentTab, { scrollBehavior: 'smooth' });
+          this.setActiveTab(nextTab, { scrollBehavior: 'smooth' });
         } else {
           this.tabs.forEach(tabEl => {
-            tabEl.tabIndex = tabEl === currentTab ? 0 : -1;
+            tabEl.tabIndex = tabEl === nextTab ? 0 : -1;
           });
         }
 
         if (['top', 'bottom'].includes(this.placement)) {
-          scrollIntoView(this.tabs[index], this.nav, 'horizontal');
+          scrollIntoView(nextTab, this.nav, 'horizontal');
         }
 
         event.preventDefault();
@@ -334,7 +327,8 @@ export default class SlTabGroup extends ShoelaceElement {
 
   // This stores tabs and panels so we can refer to a cache instead of calling querySelectorAll() multiple times.
   private syncTabsAndPanels() {
-    this.tabs = this.getAllTabs({ includeDisabled: false });
+    this.tabs = this.getAllTabs();
+    this.focusableTabs = this.tabs.filter(el => !el.disabled);
 
     this.panels = this.getAllPanels();
     this.syncIndicator();
@@ -343,6 +337,34 @@ export default class SlTabGroup extends ShoelaceElement {
     this.updateComplete.then(() => this.updateScrollControls());
   }
 
+  private findNextFocusableTab(currentIndex: number, direction: 'forward' | 'backward') {
+    let nextTab = null;
+    const iterator = direction === 'forward' ? 1 : -1;
+    let nextIndex = currentIndex + iterator;
+
+    while (currentIndex < this.tabs.length) {
+      nextTab = this.tabs[nextIndex] || null;
+
+      if (nextTab === null) {
+        // This is where wrapping happens. If we're moving forward and get to the end, then we jump to the beginning. If we're moving backward and get to the start, then we jump to the end.
+        if (direction === 'forward') {
+          nextTab = this.focusableTabs[0];
+        } else {
+          nextTab = this.focusableTabs[this.focusableTabs.length - 1];
+        }
+        break;
+      }
+
+      if (!nextTab.disabled) {
+        break;
+      }
+
+      nextIndex += iterator;
+    }
+
+    return nextTab;
+  }
+
   @watch('noScrollControls', { waitUntilFirstUpdate: true })
   updateScrollControls() {
     if (this.noScrollControls) {

src/components/tab/tab.component.ts

@@ -50,7 +50,11 @@ export default class SlTab extends ShoelaceElement {
   /** Disables the tab and prevents selection. */
   @property({ type: Boolean, reflect: true }) disabled = false;
 
-  tabIndex = -1;
+  /**
+   * @internal
+   * Need to wrap in a `@property()` otherwise CustomElement throws a "The result must not have attributes" runtime error.
+   */
+  @property({ type: Number, reflect: true }) tabIndex = 0;
 
   connectedCallback() {
     super.connectedCallback();
@@ -70,7 +74,12 @@ export default class SlTab extends ShoelaceElement {
   @watch('disabled')
   handleDisabledChange() {
     this.setAttribute('aria-disabled', this.disabled ? 'true' : 'false');
-    this.tabIndex = -1;
+
+    if (this.disabled && !this.active) {
+      this.tabIndex = -1;
+    } else {
+      this.tabIndex = 0;
+    }
   }
 
   render() {

src/components/tab/tab.test.ts

@@ -24,7 +24,7 @@ describe('<sl-tab>', () => {
     expect(el.getAttribute('role')).to.equal('tab');
     expect(el.getAttribute('aria-disabled')).to.equal('false');
     expect(el.getAttribute('aria-selected')).to.equal('false');
-    expect(el.getAttribute('tabindex')).to.equal('-1');
+    expect(el.getAttribute('tabindex')).to.equal('0');
     expect(base.getAttribute('class')).to.equal(' tab ');
     expect(el.active).to.equal(false);
     expect(el.closable).to.equal(false);
@@ -50,7 +50,7 @@ describe('<sl-tab>', () => {
     expect(el.active).to.equal(true);
     expect(el.getAttribute('aria-selected')).to.equal('true');
     expect(base.getAttribute('class')).to.equal(' tab tab--active ');
-    expect(el.getAttribute('tabindex')).to.equal('-1');
+    expect(el.getAttribute('tabindex')).to.equal('0');
   });
 
   it('should set closable by attribute', async () => {