refactor: move tabindex logic into JS class (#34741)

Author: marchbox

change/@fluentui-web-components-90308b69-87ab-46bd-bcb7-f8668dbc567b.json

@@ -0,0 +1,7 @@
+{
+  "type": "prerelease",
+  "comment": "refactor: move tabindex logic into JS class",
+  "packageName": "@fluentui/web-components",
+  "email": "machi@microsoft.com",
+  "dependentChangeType": "patch"
+}

packages/web-components/docs/web-components.api.md

@@ -480,7 +480,9 @@ export class BaseButton extends FASTElement {
     // (undocumented)
     connectedCallback(): void;
     defaultSlottedContent: HTMLElement[];
-    disabled?: boolean;
+    disabled: boolean;
+    // (undocumented)
+    protected disabledChanged(): void;
     disabledFocusable: boolean;
     // @internal
     disabledFocusableChanged(previous: boolean, next: boolean): void;
@@ -501,7 +503,6 @@ export class BaseButton extends FASTElement {
     name?: string;
     protected press(): void;
     resetForm(): void;
-    tabIndex: number;
     type: ButtonType;
     // @internal
     typeChanged(previous: ButtonType, next: ButtonType): void;
@@ -3284,8 +3285,6 @@ export type ProgressBarValidationState = ValuesOf<typeof ProgressBarValidationSt
 // @public
 export class Radio extends BaseCheckbox {
     constructor();
-    // (undocumented)
-    connectedCallback(): void;
     // @internal @override
     protected disabledChanged(prev: boolean | undefined, next: boolean | undefined): void;
     // @internal @override

packages/web-components/src/button/button.base.ts

@@ -1,6 +1,6 @@
-import { attr, FASTElement, nullableNumberConverter, observable } from '@microsoft/fast-element';
+import { attr, FASTElement, observable } from '@microsoft/fast-element';
 import { keyEnter, keySpace } from '@microsoft/fast-web-utilities';
-import { ButtonFormTarget, ButtonType } from './button.options.js';
+import { type ButtonFormTarget, ButtonType } from './button.options.js';
 
 /**
  * A Button Custom HTML Element.
@@ -42,7 +42,19 @@ export class BaseButton extends FASTElement {
    * HTML Attribute: `disabled`
    */
   @attr({ mode: 'boolean' })
-  disabled?: boolean;
+  disabled = false;
+
+  protected disabledChanged() {
+    if (this.disabled) {
+      this.removeAttribute('tabindex');
+    } else {
+      // If author sets tabindex to a non-positive value, the component should
+      // respect it, otherwise set it to 0 to avoid the anti-pattern of setting
+      // tabindex to a positive number. See details:
+      // https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Global_attributes/tabindex
+      this.tabIndex = Number(this.getAttribute('tabindex') ?? 0) < 0 ? -1 : 0;
+    }
+  }
 
   /**
    * Indicates that the button is focusable while disabled.
@@ -54,16 +66,6 @@ export class BaseButton extends FASTElement {
   @attr({ attribute: 'disabled-focusable', mode: 'boolean' })
   public disabledFocusable: boolean = false;
 
-  /**
-   * Sets that the button tabindex attribute
-   *
-   * @public
-   * @remarks
-   * HTML Attribute: `tabindex`
-   */
-  @attr({ attribute: 'tabindex', mode: 'fromView', converter: nullableNumberConverter })
-  public override tabIndex: number = 0;
-
   /**
    * Sets the element's internal disabled state when the element is focusable while disabled.
    *

packages/web-components/src/button/button.template.ts

@@ -11,7 +11,6 @@ import type { ButtonOptions } from './button.options.js';
 export function buttonTemplate<T extends Button>(options: ButtonOptions = {}): ElementViewTemplate<T> {
   return html<T>`
     <template
-      tabindex="${x => (x.disabled ? null : x.tabIndex ?? 0)}"
       @click="${(x, c) => x.clickHandler(c.event as MouseEvent)}"
       @keypress="${(x, c) => x.keypressHandler(c.event as KeyboardEvent)}"
     >

packages/web-components/src/checkbox/checkbox.base.ts

@@ -51,6 +51,15 @@ export class BaseCheckbox extends FASTElement {
    * @internal
    */
   protected disabledChanged(prev: boolean | undefined, next: boolean | undefined): void {
+    if (this.disabled) {
+      this.removeAttribute('tabindex');
+    } else {
+      // If author sets tabindex to a non-positive value, the component should
+      // respect it, otherwise set it to 0 to avoid the anti-pattern of setting
+      // tabindex to a positive number. See details:
+      // https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Global_attributes/tabindex
+      this.tabIndex = Number(this.getAttribute('tabindex') ?? 0) < 0 ? -1 : 0;
+    }
     this.elementInternals.ariaDisabled = this.disabled ? 'true' : 'false';
     toggleState(this.elementInternals, 'disabled', this.disabled);
   }
@@ -343,6 +352,7 @@ export class BaseCheckbox extends FASTElement {
   connectedCallback() {
     super.connectedCallback();
 
+    this.disabled = !!this.disabledAttribute;
     this.setAriaChecked();
     this.setValidity();
   }

packages/web-components/src/checkbox/checkbox.template.ts

@@ -28,7 +28,6 @@ const indeterminateIndicator = html.partial(/* html */ `
 export function checkboxTemplate<T extends Checkbox>(options: CheckboxOptions = {}): ElementViewTemplate<T> {
   return html<T>`
     <template
-      tabindex="${x => (!x.disabled ? 0 : void 0)}"
       @click="${(x, c) => x.clickHandler(c.event as MouseEvent)}"
       @input="${(x, c) => x.inputHandler(c.event as InputEvent)}"
       @keydown="${(x, c) => x.keydownHandler(c.event as KeyboardEvent)}"

packages/web-components/src/radio-group/radio-group.ts

@@ -65,7 +65,7 @@ export class RadioGroup extends FASTElement {
     if (this.$fastController.isConnected) {
       this.checkedIndex = -1;
       this.radios?.forEach(radio => {
-        radio.disabled = radio.disabledAttribute || this.disabled;
+        radio.disabled = !!radio.disabledAttribute || !!this.disabled;
       });
       this.restrictFocus();
     }
@@ -172,7 +172,7 @@ export class RadioGroup extends FASTElement {
       }
 
       radio.name = this.name ?? radio.name;
-      radio.disabled = this.disabled || radio.disabledAttribute;
+      radio.disabled = !!this.disabled || !!radio.disabledAttribute;
     });
 
     if (!this.dirtyState && this.initialValue) {

packages/web-components/src/radio/radio.spec.ts

@@ -26,7 +26,7 @@ test.describe('Radio', () => {
     });
 
     await test.step('ariaDisabled', async () => {
-      await expect(element).toHaveJSProperty('elementInternals.ariaDisabled', null);
+      await expect(element).toHaveJSProperty('elementInternals.ariaDisabled', 'false');
 
       await element.evaluate((node: Radio) => (node.disabled = true));
 
@@ -44,12 +44,12 @@ test.describe('Radio', () => {
     await expect(element).toHaveAttribute('tabindex', '0');
   });
 
-  test('should set the tabindex to -1 when disabled is `true`', async ({ fastPage }) => {
+  test('should not be focusable when disabled is `true`', async ({ fastPage }) => {
     const { element } = fastPage;
 
     await fastPage.setTemplate({ attributes: { disabled: true } });
 
-    await expect(element).toHaveAttribute('tabindex', '-1');
+    await expect(element).toHaveJSProperty('tabIndex', -1);
   });
 
   test('should initialize to the initial value if no value property is set', async ({ fastPage }) => {

packages/web-components/src/radio/radio.ts

@@ -13,12 +13,6 @@ import { BaseCheckbox } from '../checkbox/checkbox.base.js';
  * @public
  */
 export class Radio extends BaseCheckbox {
-  connectedCallback() {
-    super.connectedCallback();
-
-    this.tabIndex = this.disabled ? -1 : 0;
-  }
-
   constructor() {
     super();
     this.elementInternals.role = 'radio';
@@ -34,10 +28,6 @@ export class Radio extends BaseCheckbox {
    */
   protected disabledChanged(prev: boolean | undefined, next: boolean | undefined): void {
     super.disabledChanged(prev, next);
-    if (next) {
-      this.tabIndex = -1;
-    }
-
     this.$emit('disabled', next, { bubbles: true });
   }