From dfc87f32e8621b0bbcf8156a276bd67a5ecd1ddc Mon Sep 17 00:00:00 2001
From: Elizabeth Mitchell <lizmitchell@google.com>
Date: Thu, 24 Aug 2023 16:43:44 -0700
Subject: [PATCH] fix(chips): linear navigation not working in VoiceOver

A lot of the changes are in order to fix filter chips with VoiceOver. With the extra divs and no aria-hidden on ripple/focus ring, VoiceOver got stuck on the first filter chip with linear navigation.

Elevated link chips also got their border changed to white instead of yellow in HCM to match Wiz.

PiperOrigin-RevId: 559908582
---
 chips/demo/stories.ts                    |  6 +-
 chips/internal/_elevated.scss            |  4 --
 chips/internal/assist-chip.ts            | 20 +++---
 chips/internal/chip.ts                   | 78 ++++++++++++++----------
 chips/internal/filter-chip.ts            | 28 ++++-----
 chips/internal/input-chip.ts             | 29 +++++----
 chips/internal/multi-action-chip.ts      | 10 +--
 chips/internal/multi-action-chip_test.ts |  6 +-
 8 files changed, 99 insertions(+), 82 deletions(-)

diff --git a/chips/demo/stories.ts b/chips/demo/stories.ts
index b9252f88d0..3b5e5020e2 100644
--- a/chips/demo/stories.ts
+++ b/chips/demo/stories.ts
@@ -62,7 +62,7 @@ const standard: MaterialStoryInit<StoryKnobs> = {
           ?disabled=${disabled}
           ?elevated=${elevated}
         >
-          <md-icon slot="icon" aria-hidden="true">local_laundry_service</md-icon>
+          <md-icon slot="icon">local_laundry_service</md-icon>
         </md-assist-chip>
       </md-chip-set>
     `;
@@ -108,7 +108,7 @@ const filters: MaterialStoryInit<StoryKnobs> = {
           ?disabled=${disabled}
           ?elevated=${elevated}
         >
-          <md-icon slot="icon" aria-hidden="true">local_laundry_service</md-icon>
+          <md-icon slot="icon">local_laundry_service</md-icon>
         </md-filter-chip>
         <md-filter-chip
           label=${label || 'Removable filter chip'}
@@ -193,7 +193,7 @@ const suggestions: MaterialStoryInit<StoryKnobs> = {
           ?disabled=${disabled}
           ?elevated=${elevated}
         >
-          <md-icon slot="icon" aria-hidden="true">local_laundry_service</md-icon>
+          <md-icon slot="icon">local_laundry_service</md-icon>
         </md-suggestion-chip>
       </md-chip-set>
     `;
diff --git a/chips/internal/_elevated.scss b/chips/internal/_elevated.scss
index 0d413543f2..751107a564 100644
--- a/chips/internal/_elevated.scss
+++ b/chips/internal/_elevated.scss
@@ -63,10 +63,6 @@
       border: 1px solid CanvasText;
     }
 
-    .elevated.link md-elevation {
-      border-color: ActiveText;
-    }
-
     .elevated.disabled md-elevation {
       border-color: GrayText;
     }
diff --git a/chips/internal/assist-chip.ts b/chips/internal/assist-chip.ts
index de6f5ac211..014baabc09 100644
--- a/chips/internal/assist-chip.ts
+++ b/chips/internal/assist-chip.ts
@@ -11,7 +11,7 @@ import {property} from 'lit/decorators.js';
 
 import {ARIAMixinStrict} from '../../internal/aria/aria.js';
 
-import {Chip} from './chip.js';
+import {Chip, renderGridAction, renderGridContainer} from './chip.js';
 
 /**
  * An assist chip component.
@@ -30,6 +30,10 @@ export class AssistChip extends Chip {
     return !this.href && this.disabled;
   }
 
+  protected override renderContainer(content: unknown) {
+    return renderGridContainer(content, this.getContainerClasses());
+  }
+
   protected override getContainerClasses() {
     return {
       ...super.getContainerClasses(),
@@ -40,27 +44,27 @@ export class AssistChip extends Chip {
     };
   }
 
-  protected override renderAction() {
+  protected override renderPrimaryAction(content: unknown) {
     const {ariaLabel} = this as ARIAMixinStrict;
     if (this.href) {
-      return html`
+      return renderGridAction(html`
         <a class="primary action"
           id="link"
           aria-label=${ariaLabel || nothing}
           href=${this.href}
           target=${this.target || nothing}
-        >${this.renderContent()}</a>
-      `;
+        >${content}</a>
+      `);
     }
 
-    return html`
+    return renderGridAction(html`
       <button class="primary action"
         id="button"
         aria-label=${ariaLabel || nothing}
         ?disabled=${this.disabled}
         type="button"
-      >${this.renderContent()}</button>
-    `;
+      >${content}</button>
+    `);
   }
 
   protected override renderOutline() {
diff --git a/chips/internal/chip.ts b/chips/internal/chip.ts
index aff6095ec6..088549869a 100644
--- a/chips/internal/chip.ts
+++ b/chips/internal/chip.ts
@@ -8,8 +8,8 @@ import '../../focus/md-focus-ring.js';
 import '../../ripple/ripple.js';
 
 import {html, LitElement, nothing, TemplateResult} from 'lit';
-import {property, state} from 'lit/decorators.js';
-import {classMap} from 'lit/directives/class-map.js';
+import {property} from 'lit/decorators.js';
+import {ClassInfo, classMap} from 'lit/directives/class-map.js';
 
 import {requestUpdateOnAriaChange} from '../../internal/aria/delegate.js';
 
@@ -43,47 +43,43 @@ export abstract class Chip extends LitElement {
     return this.disabled;
   }
 
-  /**
-   * The aria role of the container. Defaults to `row` for grid chip sets.
-   * Listbox chip sets should remove this since they do not contain cells.
-   */
-  @state() protected containerRole?: 'row' = 'row';
-
   protected override render() {
-    return html`
-      <div class="container ${classMap(this.getContainerClasses())}"
-          role=${this.containerRole || nothing}>
-        ${this.renderOutline()}
-        <md-focus-ring part="focus-ring" for=${this.primaryId}></md-focus-ring>
-        <md-ripple for=${this.primaryId}
-          ?disabled=${this.rippleDisabled}></md-ripple>
-        ${this.renderActions()}
-      </div>
-    `;
+    return this.renderContainer(this.renderContainerContent());
   }
 
+  protected abstract renderContainer(content: unknown): unknown;
+
   protected getContainerClasses() {
     return {
       disabled: this.disabled,
     };
   }
 
-  protected renderActions() {
-    return this.renderActionCell(this.renderAction());
+  protected renderContainerContent() {
+    // Note: add aria-hidden="true" to focus ring and ripple. For some reason
+    // they cause VoiceOver to get stuck inside filter chip sets without it.
+    // TODO(b/297428579): investigate and file VoiceOver bug
+    return html`
+      ${this.renderOutline()}
+      <md-focus-ring part="focus-ring" for=${this.primaryId}
+        aria-hidden="true"></md-focus-ring>
+      <md-ripple for=${this.primaryId} ?disabled=${this.rippleDisabled}
+        aria-hidden="true"></md-ripple>
+      ${this.renderPrimaryAction(this.renderPrimaryContent())}
+    `;
   }
 
-  protected renderActionCell(content: TemplateResult|
-                             typeof nothing): TemplateResult|typeof nothing {
-    if (content === nothing) {
-      return content;
-    }
+  protected renderOutline() {
+    return html`<span class="outline"></span>`;
+  }
 
-    return html`<div class="cell" role="cell">${content}</div>`;
+  protected renderLeadingIcon(): TemplateResult {
+    return html`<slot name="icon"></slot>`;
   }
 
-  protected abstract renderAction(): TemplateResult;
+  protected abstract renderPrimaryAction(content: unknown): unknown;
 
-  protected renderContent() {
+  private renderPrimaryContent() {
     return html`
       <span class="leading icon" aria-hidden="true">
         ${this.renderLeadingIcon()}
@@ -92,12 +88,28 @@ export abstract class Chip extends LitElement {
       <span class="touch"></span>
     `;
   }
+}
 
-  protected renderOutline() {
-    return html`<span class="outline"></span>`;
-  }
+/**
+ * Renders a chip container that follows the grid/row/cell a11y pattern.
+ *
+ * This renders the container with `role="row"`.
+ */
+export function renderGridContainer(content: unknown, classes: ClassInfo) {
+  return html`
+    <div class="container ${classMap(classes)}" role="row">${content}</div>
+  `;
+}
 
-  protected renderLeadingIcon(): TemplateResult {
-    return html`<slot name="icon"></slot>`;
+/**
+ * Renders a chip action that follows the grid/row/cell a11y pattern.
+ *
+ * This wraps actions in a `role="cell"` div.
+ */
+export function renderGridAction(content: unknown) {
+  if (content === nothing) {
+    return content;
   }
+
+  return html`<div class="cell" role="cell">${content}</div>`;
 }
diff --git a/chips/internal/filter-chip.ts b/chips/internal/filter-chip.ts
index 42fdf1e362..a4f2f24247 100644
--- a/chips/internal/filter-chip.ts
+++ b/chips/internal/filter-chip.ts
@@ -6,8 +6,9 @@
 
 import '../../elevation/elevation.js';
 
-import {html, nothing, PropertyValues, svg, TemplateResult} from 'lit';
+import {html, nothing, PropertyValues, svg} from 'lit';
 import {property, query} from 'lit/decorators.js';
+import {classMap} from 'lit/directives/class-map.js';
 
 import {ARIAMixinStrict} from '../../internal/aria/aria.js';
 import {redispatchEvent} from '../../internal/controller/events.js';
@@ -31,13 +32,6 @@ export class FilterChip extends MultiActionChip {
   @query('.trailing.action')
   protected readonly trailingAction!: HTMLElement|null;
 
-  constructor() {
-    super();
-    // Remove the `row` role from the container, since filter chips do not use a
-    // `grid` navigation model.
-    this.containerRole = undefined;
-  }
-
   protected override updated(changed: PropertyValues<this>) {
     if (changed.has('selected') && changed.get('selected') !== undefined) {
       // Dispatch when `selected` changes, except for the first update.
@@ -45,6 +39,14 @@ export class FilterChip extends MultiActionChip {
     }
   }
 
+  protected override renderContainer(content: unknown) {
+    const classes = this.getContainerClasses();
+    // Note: an explicit `role="presentation"` is needed for VoiceOver. Without
+    // it, linear navigation gets stuck on the first filter chip.
+    return html`<div class="container ${
+        classMap(classes)}" role="presentation">${content}</div>`;
+  }
+
   protected override getContainerClasses() {
     return {
       ...super.getContainerClasses(),
@@ -54,13 +56,7 @@ export class FilterChip extends MultiActionChip {
     };
   }
 
-  protected override renderActionCell(content: TemplateResult|typeof nothing) {
-    // Filter chips use a `listbox`/`option` model, and do not need `gridcell`
-    // wrappers around their actions.
-    return content;
-  }
-
-  protected override renderAction() {
+  protected override renderPrimaryAction(content: unknown) {
     const {ariaLabel} = this as ARIAMixinStrict;
     return html`
       <button class="primary action"
@@ -70,7 +66,7 @@ export class FilterChip extends MultiActionChip {
         ?disabled=${this.disabled || nothing}
         role="option"
         @click=${this.handleClick}
-      >${this.renderContent()}</button>
+      >${content}</button>
     `;
   }
 
diff --git a/chips/internal/input-chip.ts b/chips/internal/input-chip.ts
index 83f53990fe..cd45428594 100644
--- a/chips/internal/input-chip.ts
+++ b/chips/internal/input-chip.ts
@@ -9,6 +9,7 @@ import {property, query} from 'lit/decorators.js';
 
 import {ARIAMixinStrict} from '../../internal/aria/aria.js';
 
+import {renderGridAction, renderGridContainer} from './chip.js';
 import {MultiActionChip} from './multi-action-chip.js';
 import {renderRemoveButton} from './trailing-icons.js';
 
@@ -52,6 +53,10 @@ export class InputChip extends MultiActionChip {
   @query('.trailing.action')
   protected readonly trailingAction!: HTMLElement|null;
 
+  protected override renderContainer(content: unknown) {
+    return renderGridContainer(content, this.getContainerClasses());
+  }
+
   protected override getContainerClasses() {
     return {
       ...super.getContainerClasses(),
@@ -64,42 +69,42 @@ export class InputChip extends MultiActionChip {
     };
   }
 
-  protected override renderAction() {
+  protected override renderPrimaryAction(content: unknown) {
     const {ariaLabel} = this as ARIAMixinStrict;
     if (this.href) {
-      return html`
+      return renderGridAction(html`
         <a class="primary action"
           id="link"
           aria-label=${ariaLabel || nothing}
           href=${this.href}
           target=${this.target || nothing}
-        >${this.renderContent()}</a>
-      `;
+        >${content}</a>
+      `);
     }
 
     if (this.removeOnly) {
-      return html`
+      return renderGridAction(html`
         <span class="primary action" aria-label=${ariaLabel || nothing}>
-          ${this.renderContent()}
+          ${content}
         </span>
-      `;
+      `);
     }
 
-    return html`
+    return renderGridAction(html`
       <button class="primary action"
         id="button"
         aria-label=${ariaLabel || nothing}
         ?disabled=${this.disabled}
         type="button"
-      >${this.renderContent()}</button>
-    `;
+      >${content}</button>
+    `);
   }
 
   protected override renderTrailingAction() {
-    return renderRemoveButton({
+    return renderGridAction(renderRemoveButton({
       ariaLabel: this.ariaLabelRemove,
       disabled: !this.href && this.disabled,
       tabbable: this.removeOnly,
-    });
+    }));
   }
 }
diff --git a/chips/internal/multi-action-chip.ts b/chips/internal/multi-action-chip.ts
index ff074c173e..4465a17b42 100644
--- a/chips/internal/multi-action-chip.ts
+++ b/chips/internal/multi-action-chip.ts
@@ -4,7 +4,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import {html, isServer, nothing, TemplateResult} from 'lit';
+import {html, isServer} from 'lit';
 
 import {ARIAMixinStrict} from '../../internal/aria/aria.js';
 
@@ -58,14 +58,14 @@ export abstract class MultiActionChip extends Chip {
     super.focus(options);
   }
 
-  protected override renderActions() {
+  protected override renderContainerContent() {
     return html`
-      ${super.renderActions()}
-      ${this.renderActionCell(this.renderTrailingAction())}
+      ${super.renderContainerContent()}
+      ${this.renderTrailingAction()}
     `;
   }
 
-  protected abstract renderTrailingAction(): TemplateResult|typeof nothing;
+  protected abstract renderTrailingAction(): unknown;
 
   private handleKeyDown(event: KeyboardEvent) {
     const isLeft = event.key === 'ArrowLeft';
diff --git a/chips/internal/multi-action-chip_test.ts b/chips/internal/multi-action-chip_test.ts
index 77b801a364..aa73c8e797 100644
--- a/chips/internal/multi-action-chip_test.ts
+++ b/chips/internal/multi-action-chip_test.ts
@@ -25,7 +25,11 @@ class TestMultiActionChip extends MultiActionChip {
 
   protected primaryId = 'primary';
 
-  protected override renderAction() {
+  protected override renderContainer(content: unknown) {
+    return html`<div>${content}</div>`;
+  }
+
+  protected override renderPrimaryAction() {
     return html`<button id="primary"></button>`;
   }