From 36a01b8ee4c5549544c8259e30d65b8da0215d73 Mon Sep 17 00:00:00 2001
From: Mert Akinc <7282195+m-akinc@users.noreply.github.com>
Date: Mon, 20 May 2024 18:39:36 -0500
Subject: [PATCH] Feedback

---
 .../src/button/tests/button.spec.ts           | 12 ++++-----
 .../nimble-components/src/checkbox/index.ts   |  8 ++++++
 .../src/checkbox/template.ts                  |  3 +--
 .../src/checkbox/tests/checkbox.spec.ts       | 12 ++++-----
 .../src/toggle-button/index.ts                |  8 ++++++
 .../src/toggle-button/template.ts             | 27 +++++++++++--------
 6 files changed, 45 insertions(+), 25 deletions(-)

diff --git a/packages/nimble-components/src/button/tests/button.spec.ts b/packages/nimble-components/src/button/tests/button.spec.ts
index 43aea1bd1e..59b6e1a2e2 100644
--- a/packages/nimble-components/src/button/tests/button.spec.ts
+++ b/packages/nimble-components/src/button/tests/button.spec.ts
@@ -4,10 +4,8 @@ import { fixture, type Fixture } from '../../utilities/tests/fixture';
 import { waitForUpdatesAsync } from '../../testing/async-helpers';
 
 describe('Button', () => {
-    async function setup(tabIndex?: string): Promise<Fixture<Button>> {
-        return fixture<Button>(
-            html`<${buttonTag} ${tabIndex ? `tabindex=${tabIndex}` : ''}></${buttonTag}>`
-        );
+    async function setup(): Promise<Fixture<Button>> {
+        return fixture<Button>(html`<${buttonTag}></${buttonTag}>`);
     }
 
     it('should export its tag', () => {
@@ -30,7 +28,8 @@ describe('Button', () => {
     });
 
     it('should set the `tabindex` attribute on the internal button when provided', async () => {
-        const { element, connect, disconnect } = await setup('-1');
+        const { element, connect, disconnect } = await setup();
+        element.setAttribute('tabindex', '-1');
         await connect();
 
         const innerButton = element.shadowRoot!.querySelector('button')!;
@@ -41,7 +40,8 @@ describe('Button', () => {
     });
 
     it('should clear the `tabindex` attribute on the internal button when cleared from the host', async () => {
-        const { element, connect, disconnect } = await setup('-1');
+        const { element, connect, disconnect } = await setup();
+        element.setAttribute('tabindex', '-1');
         await connect();
 
         element.removeAttribute('tabindex');
diff --git a/packages/nimble-components/src/checkbox/index.ts b/packages/nimble-components/src/checkbox/index.ts
index 9462f023eb..e09dc3b850 100644
--- a/packages/nimble-components/src/checkbox/index.ts
+++ b/packages/nimble-components/src/checkbox/index.ts
@@ -25,6 +25,14 @@ export class Checkbox extends FoundationCheckbox {
      */
     @attr({ attribute: 'tabindex', converter: nullableNumberConverter })
     public override tabIndex!: number;
+
+    /**
+     * @internal
+     */
+    public getEffectiveTabIndex(): number | null {
+        // prettier-ignore
+        return this.disabled ? null : (this.tabIndex ?? 0);
+    }
 }
 
 const nimbleCheckbox = Checkbox.compose<CheckboxOptions>({
diff --git a/packages/nimble-components/src/checkbox/template.ts b/packages/nimble-components/src/checkbox/template.ts
index 075a9a65a8..6e71c8ede0 100644
--- a/packages/nimble-components/src/checkbox/template.ts
+++ b/packages/nimble-components/src/checkbox/template.ts
@@ -1,4 +1,3 @@
-/* eslint-disable no-restricted-syntax */
 import { html, slotted, type ViewTemplate } from '@microsoft/fast-element';
 import type {
     CheckboxOptions,
@@ -16,7 +15,7 @@ CheckboxOptions
         aria-required="${x => x.required}"
         aria-disabled="${x => x.disabled}"
         aria-readonly="${x => x.readOnly}"
-        tabindex="${x => (x.disabled ? null : x.tabIndex ?? 0)}"
+        tabindex="${x => x.getEffectiveTabIndex()}"
         @keypress="${(x, c) => x.keypressHandler(c.event as KeyboardEvent)}"
         @click="${(x, c) => x.clickHandler(c.event as MouseEvent)}"
         class="${x => (x.readOnly ? 'readonly' : '')} ${x => (x.checked ? 'checked' : '')} ${x => (x.indeterminate ? 'indeterminate' : '')}"
diff --git a/packages/nimble-components/src/checkbox/tests/checkbox.spec.ts b/packages/nimble-components/src/checkbox/tests/checkbox.spec.ts
index 2b95a1f1d5..45a8cbd4f6 100644
--- a/packages/nimble-components/src/checkbox/tests/checkbox.spec.ts
+++ b/packages/nimble-components/src/checkbox/tests/checkbox.spec.ts
@@ -4,10 +4,8 @@ import { fixture, type Fixture } from '../../utilities/tests/fixture';
 import { waitForUpdatesAsync } from '../../testing/async-helpers';
 
 describe('Checkbox', () => {
-    async function setup(tabIndex?: string): Promise<Fixture<Checkbox>> {
-        return fixture<Checkbox>(
-            html`<${checkboxTag} ${tabIndex ? `tabindex=${tabIndex}` : ''}></${checkboxTag}>`
-        );
+    async function setup(): Promise<Fixture<Checkbox>> {
+        return fixture<Checkbox>(html`<${checkboxTag}></${checkboxTag}>`);
     }
 
     it('should export its tag', () => {
@@ -21,7 +19,8 @@ describe('Checkbox', () => {
     });
 
     it('should honor provided `tabindex` value', async () => {
-        const { element, connect, disconnect } = await setup('-1');
+        const { element, connect, disconnect } = await setup();
+        element.setAttribute('tabindex', '-1');
         await connect();
 
         expect(element.getAttribute('tabindex')).toEqual('-1');
@@ -31,7 +30,8 @@ describe('Checkbox', () => {
     });
 
     it('should default `tabindex` back to 0 when removed', async () => {
-        const { element, connect, disconnect } = await setup('-1');
+        const { element, connect, disconnect } = await setup();
+        element.setAttribute('tabindex', '-1');
         await connect();
 
         element.removeAttribute('tabindex');
diff --git a/packages/nimble-components/src/toggle-button/index.ts b/packages/nimble-components/src/toggle-button/index.ts
index ccba784d1b..f3d3b37cd6 100644
--- a/packages/nimble-components/src/toggle-button/index.ts
+++ b/packages/nimble-components/src/toggle-button/index.ts
@@ -56,6 +56,14 @@ export class ToggleButton extends FoundationSwitch implements ButtonPattern {
 
     /** @internal */
     public readonly control!: HTMLElement;
+
+    /**
+     * @internal
+     */
+    public getEffectiveTabIndex(): number | null {
+        // prettier-ignore
+        return this.disabled ? null : (this.tabIndex ?? 0);
+    }
 }
 applyMixins(ToggleButton, StartEnd, DelegatesARIAButton);
 export interface ToggleButton extends StartEnd, DelegatesARIAButton {}
diff --git a/packages/nimble-components/src/toggle-button/template.ts b/packages/nimble-components/src/toggle-button/template.ts
index 606a74fb1f..28c34b8465 100644
--- a/packages/nimble-components/src/toggle-button/template.ts
+++ b/packages/nimble-components/src/toggle-button/template.ts
@@ -8,6 +8,8 @@ import {
 } from '@microsoft/fast-foundation';
 import type { ToggleButton } from '.';
 
+/* eslint-disable @typescript-eslint/indent */
+// prettier-ignore
 export const template: FoundationElementTemplate<
 ViewTemplate<ToggleButton>,
 ButtonOptions
@@ -15,23 +17,26 @@ ButtonOptions
     <div
         role="button"
         part="control"
-        tabindex="${x => (x.disabled ? null : x.tabIndex ?? 0)}"
+        tabindex="${x => x.getEffectiveTabIndex()}"
         @keypress="${(x, c) => x.keypressHandler(c.event as KeyboardEvent)}"
         @click="${(x, c) => x.clickHandler(c.event as MouseEvent)}"
         class="control ${x => (x.checked ? 'checked' : '')}"
-        ?disabled="${x => x.disabled}"
-        ${
-    '' /* Configure aria-disabled, aria-readonly, and aria-pressed based on the
-        toggle button's state to keep the ARIA attributes consistent with the component's
-        state without a client having to configure ARIA attributes directly */
-}
+        ${''
+            /**
+             * Configure aria-disabled, aria-readonly, and aria-pressed based on the
+             * toggle button's state to keep the ARIA attributes consistent with the component's
+             * state without a client having to configure ARIA attributes directly
+             */
+        }
         aria-disabled="${x => x.disabled}"
         aria-readonly="${x => x.readOnly}"
         aria-pressed="${x => x.checked}"
-        ${
-    '' /* Configure all other ARIA attributes based on the aria attributes
-        configured on the toggle button */
-}
+        ${''
+            /**
+             * Configure all other ARIA attributes based on the aria attributes
+             * configured on the toggle button
+             */
+        }
         aria-atomic="${x => x.ariaAtomic}"
         aria-busy="${x => x.ariaBusy}"
         aria-controls="${x => x.ariaControls}"