WEBDEV-9046: Address the ia-playback-controls review follow-ups

src/elements/ia-playback-controls/assets/next-section.ts

@@ -1,13 +1,16 @@
 /**
  * @file Jump to next section icon
  *
- * Inline so the button can recolor it with `currentColor`, which is not
- * possible on the contents of an `<img>`.
+ * Decorative: the button around it carries the accessible name, so the icon
+ * is hidden from assistive tech. Inline so the button can recolor it with
+ * `currentColor`, which is not possible on the contents of an `<img>`.
  */
 import { svg } from 'lit';
 
 export default svg`
-<svg xmlns="http://www.w3.org/2000/svg" width="18" height="20" viewBox="0 0 18 20">
+<svg xmlns="http://www.w3.org/2000/svg" width="18" height="20" viewBox="0 0 18 20"
+  aria-hidden="true"
+>
   <g fill="none" fill-rule="evenodd" transform="translate(1)">
     <polygon fill="currentColor" points="7 3 16 17 -2 17" transform="rotate(90 7 10)" />
     <line x1="15" x2="15" y1="20" stroke="currentColor" stroke-width="2" />

src/elements/ia-playback-controls/assets/pause.ts

@@ -1,13 +1,16 @@
 /**
  * @file Pause icon
  *
- * Inline so the button can recolor it with `currentColor`, which is not
- * possible on the contents of an `<img>`.
+ * Decorative: the button around it carries the accessible name, so the icon
+ * is hidden from assistive tech. Inline so the button can recolor it with
+ * `currentColor`, which is not possible on the contents of an `<img>`.
  */
 import { svg } from 'lit';
 
 export default svg`
-<svg height="60" viewBox="0 0 60 60" width="60" xmlns="http://www.w3.org/2000/svg">
+<svg height="60" viewBox="0 0 60 60" width="60" xmlns="http://www.w3.org/2000/svg"
+  aria-hidden="true"
+>
   <path
     d="m32 18h6v25h-6zm-10 0h6v25h-6z"
     fill="currentColor"

src/elements/ia-playback-controls/assets/play.ts

@@ -1,13 +1,16 @@
 /**
  * @file Play icon
  *
- * Inline so the button can recolor it with `currentColor`, which is not
- * possible on the contents of an `<img>`.
+ * Decorative: the button around it carries the accessible name, so the icon
+ * is hidden from assistive tech. Inline so the button can recolor it with
+ * `currentColor`, which is not possible on the contents of an `<img>`.
  */
 import { svg } from 'lit';
 
 export default svg`
-<svg height="60" viewBox="0 0 60 60" width="60" xmlns="http://www.w3.org/2000/svg">
+<svg height="60" viewBox="0 0 60 60" width="60" xmlns="http://www.w3.org/2000/svg"
+  aria-hidden="true"
+>
   <path
     d="m34.5 18 12.5 25h-25z"
     fill="currentColor"

src/elements/ia-playback-controls/assets/playback-speed.ts

@@ -1,13 +1,16 @@
 /**
  * @file Playback speed icon, a speedometer
  *
- * Inline so the button can recolor it with `currentColor`, which is not
- * possible on the contents of an `<img>`.
+ * Decorative: the button around it carries the accessible name, so the icon
+ * is hidden from assistive tech. Inline so the button can recolor it with
+ * `currentColor`, which is not possible on the contents of an `<img>`.
  */
 import { svg } from 'lit';
 
 export default svg`
-<svg xmlns="http://www.w3.org/2000/svg" width="23" height="17" viewBox="0 0 23 17">
+<svg xmlns="http://www.w3.org/2000/svg" width="23" height="17" viewBox="0 0 23 17"
+  aria-hidden="true"
+>
   <g fill="none" fill-rule="evenodd" stroke="currentColor" transform="translate(1 1)">
     <g stroke-linecap="round" transform="translate(2 2.522)">
       <path d="M8.5,9.97826087 L15.5,2.97826087" transform="rotate(180 12 6.478)" />

src/elements/ia-playback-controls/assets/previous-section.ts

@@ -1,13 +1,16 @@
 /**
  * @file Jump to previous section icon
  *
- * Inline so the button can recolor it with `currentColor`, which is not
- * possible on the contents of an `<img>`.
+ * Decorative: the button around it carries the accessible name, so the icon
+ * is hidden from assistive tech. Inline so the button can recolor it with
+ * `currentColor`, which is not possible on the contents of an `<img>`.
  */
 import { svg } from 'lit';
 
 export default svg`
-<svg xmlns="http://www.w3.org/2000/svg" width="18" height="20" viewBox="0 0 18 20">
+<svg xmlns="http://www.w3.org/2000/svg" width="18" height="20" viewBox="0 0 18 20"
+  aria-hidden="true"
+>
   <g fill="none" fill-rule="evenodd" transform="translate(1 1)">
     <polygon fill="currentColor" points="9 2 18 16 0 16" transform="rotate(-90 9 9)" />
     <line x1=".5" x2=".5" y1="18" stroke="currentColor" stroke-width="2" />

src/elements/ia-playback-controls/assets/replay.ts

@@ -1,13 +1,16 @@
 /**
  * @file Skip back ten seconds icon
  *
- * Inline so the button can recolor it with `currentColor`, which is not
- * possible on the contents of an `<img>`.
+ * Decorative: the button around it carries the accessible name, so the icon
+ * is hidden from assistive tech. Inline so the button can recolor it with
+ * `currentColor`, which is not possible on the contents of an `<img>`.
  */
 import { svg } from 'lit';
 
 export default svg`
-<svg width="20" height="20" viewBox="0 0 20 20" xmlns="http://www.w3.org/2000/svg">
+<svg width="20" height="20" viewBox="0 0 20 20" xmlns="http://www.w3.org/2000/svg"
+  aria-hidden="true"
+>
   <g stroke="none" stroke-width="1" fill="none" fill-rule="evenodd">
     <polyline
       stroke="currentColor"

src/elements/ia-playback-controls/assets/skip-ahead.ts

@@ -1,13 +1,16 @@
 /**
  * @file Skip ahead ten seconds icon
  *
- * Inline so the button can recolor it with `currentColor`, which is not
- * possible on the contents of an `<img>`.
+ * Decorative: the button around it carries the accessible name, so the icon
+ * is hidden from assistive tech. Inline so the button can recolor it with
+ * `currentColor`, which is not possible on the contents of an `<img>`.
  */
 import { svg } from 'lit';
 
 export default svg`
-<svg height="20" viewBox="0 0 20 20" width="20" xmlns="http://www.w3.org/2000/svg">
+<svg height="20" viewBox="0 0 20 20" width="20" xmlns="http://www.w3.org/2000/svg"
+  aria-hidden="true"
+>
   <g fill="none" fill-rule="evenodd" transform="matrix(-1 0 0 1 20 0)">
     <path
       d="m14.4444444 16.6666667h5.5555556v-13.33333337h-14.44444444"

src/elements/ia-playback-controls/assets/volume-full.ts

@@ -1,13 +1,16 @@
 /**
  * @file Full volume icon, a speaker with two waves
  *
- * Inline so the button can recolor it with `currentColor`, which is not
- * possible on the contents of an `<img>`.
+ * Decorative: the button around it carries the accessible name, so the icon
+ * is hidden from assistive tech. Inline so the button can recolor it with
+ * `currentColor`, which is not possible on the contents of an `<img>`.
  */
 import { svg } from 'lit';
 
 export default svg`
-<svg height="22" viewBox="0 0 22 22" width="22" xmlns="http://www.w3.org/2000/svg">
+<svg height="22" viewBox="0 0 22 22" width="22" xmlns="http://www.w3.org/2000/svg"
+  aria-hidden="true"
+>
   <g fill="none" fill-rule="evenodd" stroke="currentColor" stroke-width="2">
     <path
       d="m14 3c2 2.05128205 3 4.71794872 3 8 0 3.2820513-1 5.9487179-3 8m4-18c2 2.56410256 3 5.8974359 3 10s-1 7.4358974-3 10"

src/elements/ia-playback-controls/assets/volume-medium.ts

@@ -1,13 +1,16 @@
 /**
  * @file Partial volume icon, a speaker with the outer wave faded out
  *
- * Inline so the button can recolor it with `currentColor`, which is not
- * possible on the contents of an `<img>`.
+ * Decorative: the button around it carries the accessible name, so the icon
+ * is hidden from assistive tech. Inline so the button can recolor it with
+ * `currentColor`, which is not possible on the contents of an `<img>`.
  */
 import { svg } from 'lit';
 
 export default svg`
-<svg height="22" viewBox="0 0 22 22" width="22" xmlns="http://www.w3.org/2000/svg">
+<svg height="22" viewBox="0 0 22 22" width="22" xmlns="http://www.w3.org/2000/svg"
+  aria-hidden="true"
+>
   <g fill="none" fill-rule="evenodd" stroke="currentColor" stroke-width="2">
     <path
       d="m14 3c2 2.05128205 3 4.71794872 3 8 0 3.2820513-1 5.9487179-3 8"

src/elements/ia-playback-controls/assets/volume-mute.ts

@@ -1,13 +1,16 @@
 /**
  * @file Muted icon, a speaker with a cross beside it
  *
- * Inline so the button can recolor it with `currentColor`, which is not
- * possible on the contents of an `<img>`.
+ * Decorative: the button around it carries the accessible name, so the icon
+ * is hidden from assistive tech. Inline so the button can recolor it with
+ * `currentColor`, which is not possible on the contents of an `<img>`.
  */
 import { svg } from 'lit';
 
 export default svg`
-<svg xmlns="http://www.w3.org/2000/svg" width="22" height="22" viewBox="0 0 22 22">
+<svg xmlns="http://www.w3.org/2000/svg" width="22" height="22" viewBox="0 0 22 22"
+  aria-hidden="true"
+>
   <g fill="none" fill-rule="evenodd">
     <path
       stroke="currentColor"

src/elements/ia-playback-controls/ia-playback-controls.test.ts

@@ -286,4 +286,98 @@ describe('IA Playback Controls', () => {
       ),
     ).to.exist;
   });
+
+  test('the themed icon colour reaches the buttons', async () => {
+    const el = await controlsFixture();
+    el.style.setProperty(
+      '--ia-theme-playback-controls-icon-color',
+      'rgb(255, 0, 0)',
+    );
+    await elementUpdated(el);
+
+    // Buttons don't inherit colour from their parent by default, so without
+    // `color: inherit` on .unstyled-button the container's themed colour never
+    // reaches the icons and their currentColor falls back to the UA default.
+    // The play/pause button is excluded on purpose: it sets its own colour.
+    ['prev-section-btn', 'back-btn', 'forward-btn', 'next-section-btn'].forEach(
+      (id) => {
+        expect(getComputedStyle(buttonIn(el, id)).color, id).to.equal(
+          'rgb(255, 0, 0)',
+        );
+      },
+    );
+  });
+
+  test('hides the decorative icons from assistive tech', async () => {
+    const el = await controlsFixture();
+
+    // Three of the ten icons only appear in a state the default fixture is
+    // not in: pause while playing, and the two quieter volume icons.
+    const states: Array<() => void> = [
+      () => {},
+      () => {
+        el.playbackMode = PlaybackMode.playing;
+      },
+      () => {
+        el.volume = 0.5;
+      },
+      () => {
+        el.volume = 0;
+      },
+    ];
+
+    const seen = new Set<string>();
+    for (const enterState of states) {
+      enterState();
+
+      await elementUpdated(el);
+
+      el.shadowRoot?.querySelectorAll('button svg').forEach((icon) => {
+        seen.add(icon.parentElement?.id ?? '');
+        expect(
+          icon.getAttribute('aria-hidden'),
+          `${icon.parentElement?.id} icon should be aria-hidden`,
+        ).to.equal('true');
+      });
+    }
+
+    // Every button's icon has now been through the assertion above.
+    expect([...seen].sort()).to.deep.equal([
+      'back-btn',
+      'forward-btn',
+      'next-section-btn',
+      'play-pause-btn',
+      'playback-rate-btn',
+      'prev-section-btn',
+      'volume-control-btn',
+    ]);
+  });
+
+  test('renders the playback rate formatted for the locale', async () => {
+    const el = await controlsFixture();
+    // A rate the two treatments disagree about in every locale: number
+    // formatting stops at three fraction digits, while dropping the raw value
+    // into the template spells out all sixteen.
+    el.playbackRate = 1 / 3;
+    await elementUpdated(el);
+
+    const formatted = new Intl.NumberFormat().format(1 / 3);
+    const value = el.shadowRoot?.querySelector('.vertical-button-value');
+
+    expect(value?.textContent?.trim()).to.equal(`${formatted}x`);
+    expect(
+      buttonIn(el, 'playback-rate-btn').getAttribute('aria-label'),
+    ).to.contain(formatted);
+  });
+
+  test('never announces an unusable playback rate', async () => {
+    const el = await fixture<IAPlaybackControls>(
+      html`<ia-playback-controls playbackRate="fast"></ia-playback-controls>`,
+    );
+
+    expect(
+      buttonIn(el, 'playback-rate-btn').getAttribute('aria-label'),
+    ).to.equal('Playback speed, currently 1');
+    expect(el.shadowRoot?.textContent).to.include('1x');
+  });
 });

src/elements/ia-playback-controls/ia-playback-controls.ts

@@ -8,6 +8,7 @@ import {
 import { customElement, property } from 'lit/decorators.js';
 import { msg, str } from '@lit/localize';
 
+import { formatPlaybackRate } from './playback-rate-formatter';
 import { PlaybackMode } from './models';
 
 import nextSectionIcon from './assets/next-section';
@@ -34,11 +35,22 @@ const Events = {
   NextSectionButtonPressed: 'next-section-button-pressed',
 };
 
+/** The rate the controls start at, and the fallback for an unusable one */
+const DEFAULT_PLAYBACK_RATE = 1;
+
 /** How much one press of the speed button moves the playback rate */
 const PLAYBACK_RATE_STEP = 0.25;
 
-/** The rate the speed button wraps back around to once it passes the top */
+/**
+ * The slowest rate the speed button offers, and the one it wraps back around
+ * to once it passes the top.
+ */
 const MIN_PLAYBACK_RATE = 0.5;
+
+/**
+ * The fastest rate the speed button offers. Pressing it here is what sends the
+ * rate back round to the slowest.
+ */
 const MAX_PLAYBACK_RATE = 2;
 
 /** How much one press of the volume button moves the volume */
@@ -57,7 +69,7 @@ export class IAPlaybackControls extends LitElement {
   @property({ type: String }) playbackMode: PlaybackMode = PlaybackMode.paused;
 
   /** Playback speed multiplier, where 1 is normal speed */
-  @property({ type: Number }) playbackRate = 1;
+  @property({ type: Number }) playbackRate = DEFAULT_PLAYBACK_RATE;
 
   /** Playback volume, from 0 (muted) to 1 (full) */
   @property({ type: Number }) volume = 1;
@@ -77,7 +89,7 @@ export class IAPlaybackControls extends LitElement {
             </button>
           </div>
           <div class="vertical-button-value" aria-hidden="true">
-            ${this.playbackRate}x
+            ${this.formattedPlaybackRate}x
           </div>
         </div>
 
@@ -174,7 +186,25 @@ export class IAPlaybackControls extends LitElement {
    * label beside the button carries the "x".
    */
   private get playbackRateLabel(): string {
-    return msg(str`Playback speed, currently ${this.playbackRate}`);
+    return msg(str`Playback speed, currently ${this.formattedPlaybackRate}`);
+  }
+
+  /**
+   * The playback rate written for the reader's locale, so the quarter steps
+   * use whatever decimal separator they expect rather than always a point.
+   *
+   * `playbackRate` is a public property, so it can arrive as something that
+   * isn't a usable number. Everything that displays or announces the rate
+   * reads it through here, so `NaN` never reaches the screen reader. Rates
+   * outside the range the button steps through are shown as they are: they
+   * are still rates a media element will play at.
+   */
+  private get formattedPlaybackRate(): string {
+    const rate = Number.isFinite(this.playbackRate)
+      ? this.playbackRate
+      : DEFAULT_PLAYBACK_RATE;
+
+    return formatPlaybackRate(rate);
   }
 
   /**

src/elements/ia-playback-controls/playback-rate-formatter.test.ts

@@ -0,0 +1,16 @@
+import { describe, expect, test } from 'vitest';
+
+import { formatPlaybackRate } from './playback-rate-formatter';
+
+describe('formatPlaybackRate', () => {
+  test('uses the locale’s decimal separator', () => {
+    expect(formatPlaybackRate(1.5, 'en-US')).to.equal('1.5');
+    expect(formatPlaybackRate(1.5, 'de-DE')).to.equal('1,5');
+    expect(formatPlaybackRate(0.75, 'de-DE')).to.equal('0,75');
+  });
+
+  test('leaves whole rates without a decimal part', () => {
+    expect(formatPlaybackRate(1, 'en-US')).to.equal('1');
+    expect(formatPlaybackRate(2, 'de-DE')).to.equal('2');
+  });
+});