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

@@ -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>
     `;

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;
     }

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() {

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>`;
 }

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,20 +32,21 @@ 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.
       this.dispatchEvent(new Event('selected', {bubbles: true}));
     }
   }
 
+  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>
     `;
   }
 

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,
-    });
+    }));
   }
 }

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';

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>`;
   }