From 4d66d323913e7a4383ce5fc1cb3f02fe4dcd6036 Mon Sep 17 00:00:00 2001
From: Jeri Peier <jeremias.peier@sbb.ch>
Date: Thu, 21 Nov 2024 17:42:32 +0100
Subject: [PATCH] fix(sbb-select): improve connected label handling (#3229)

---
 .../paginator.snapshot.spec.snap.js           |  13 +-
 .../paginator/paginator/paginator.spec.ts     |  24 +++-
 src/elements/paginator/paginator/paginator.ts |  28 +---
 src/elements/paginator/paginator/readme.md    |  20 +--
 src/elements/select/readme.md                 |  26 +++-
 src/elements/select/select.spec.ts            | 134 +++++++++++++++++-
 src/elements/select/select.ts                 |  53 ++++---
 7 files changed, 237 insertions(+), 61 deletions(-)

diff --git a/src/elements/paginator/paginator/__snapshots__/paginator.snapshot.spec.snap.js b/src/elements/paginator/paginator/__snapshots__/paginator.snapshot.spec.snap.js
index c533c9e2b0..2b06cb26da 100644
--- a/src/elements/paginator/paginator/__snapshots__/paginator.snapshot.spec.snap.js
+++ b/src/elements/paginator/paginator/__snapshots__/paginator.snapshot.spec.snap.js
@@ -542,7 +542,7 @@ snapshots["sbb-paginator renders with options Chrome-Firefox Shadow DOM"] =
     </ul>
   </span>
   <div class="sbb-paginator__page-size-options">
-    <label id="sbb-paginator-options-label-10">
+    <label for="select">
       Items per page
     </label>
     <sbb-form-field
@@ -557,7 +557,7 @@ snapshots["sbb-paginator renders with options Chrome-Firefox Shadow DOM"] =
         aria-controls="sbb-select-2"
         aria-expanded="false"
         aria-haspopup="listbox"
-        aria-labelledby="sbb-paginator-options-label-10"
+        aria-label="Items per page"
         aria-owns="sbb-select-2"
         aria-required="false"
         class="sbb-screen-reader-only"
@@ -568,9 +568,9 @@ snapshots["sbb-paginator renders with options Chrome-Firefox Shadow DOM"] =
         10
       </div>
       <sbb-select
-        aria-labelledby="sbb-paginator-options-label-10"
         data-option-panel-origin-borderless=""
         data-state="closed"
+        id="select"
         value="10"
       >
         <sbb-option
@@ -725,7 +725,7 @@ snapshots["sbb-paginator renders with options Safari Shadow DOM"] =
     </ul>
   </span>
   <div class="sbb-paginator__page-size-options">
-    <label id="sbb-paginator-options-label-10">
+    <label for="select">
       Items per page
     </label>
     <sbb-form-field
@@ -740,7 +740,7 @@ snapshots["sbb-paginator renders with options Safari Shadow DOM"] =
         aria-controls="sbb-select-2"
         aria-expanded="false"
         aria-haspopup="listbox"
-        aria-labelledby="sbb-paginator-options-label-10"
+        aria-label="Items per page"
         aria-owns="sbb-select-2"
         aria-required="false"
         class="sbb-screen-reader-only"
@@ -751,10 +751,9 @@ snapshots["sbb-paginator renders with options Safari Shadow DOM"] =
         10
       </div>
       <sbb-select
-        aria-labelledby="sbb-paginator-options-label-10"
         data-option-panel-origin-borderless=""
         data-state="closed"
-        id="sbb-select-2"
+        id="select"
         role="listbox"
         value="10"
       >
diff --git a/src/elements/paginator/paginator/paginator.spec.ts b/src/elements/paginator/paginator/paginator.spec.ts
index d258b6fcac..221dac53fe 100644
--- a/src/elements/paginator/paginator/paginator.spec.ts
+++ b/src/elements/paginator/paginator/paginator.spec.ts
@@ -1,4 +1,4 @@
-import { assert, expect } from '@open-wc/testing';
+import { assert, aTimeout, expect } from '@open-wc/testing';
 import { sendKeys } from '@web/test-runner-commands';
 import { html } from 'lit/static-html.js';
 import { spy } from 'sinon';
@@ -219,4 +219,26 @@ describe('sbb-paginator', () => {
     await waitForLitRender(element);
     expect(pageEventSpy.count).to.be.equal(1);
   });
+
+  it('should update items per page label on language change', async () => {
+    element = await fixture(
+      html`<sbb-paginator length="50" page-size="5" .pageSizeOptions=${[5, 10]}></sbb-paginator>`,
+    );
+
+    const comboBoxElement = element.shadowRoot!.querySelector('[role="combobox"]')!;
+
+    expect(comboBoxElement).to.have.attribute('aria-label', 'Items per page');
+
+    const lang = document.documentElement.getAttribute('lang')!;
+    document.documentElement.setAttribute('lang', 'de');
+
+    await waitForLitRender(element);
+    // We have to wait a tick until the label sync can happen
+    await aTimeout(0);
+
+    expect(comboBoxElement).to.have.attribute('aria-label', 'Einträge pro Seite');
+
+    // Restore language
+    document.documentElement.setAttribute('lang', lang);
+  });
 });
diff --git a/src/elements/paginator/paginator/paginator.ts b/src/elements/paginator/paginator/paginator.ts
index b92d6d920d..faaa74148d 100644
--- a/src/elements/paginator/paginator/paginator.ts
+++ b/src/elements/paginator/paginator/paginator.ts
@@ -23,8 +23,6 @@ import '../../screen-reader-only.js';
 
 const MAX_PAGE_NUMBERS_DISPLAYED = 3;
 
-let nextId = 0;
-
 /**
  * It displays a paginator component.
  *
@@ -40,14 +38,7 @@ class SbbPaginatorElement extends SbbPaginatorCommonElementMixin(LitElement) {
 
   /** The available `pageSize` choices. */
   @property({ attribute: 'page-size-options', type: Array })
-  public set pageSizeOptions(value: number[]) {
-    this._pageSizeOptions = value;
-    this._updateSelectAriaLabelledBy = true;
-  }
-  public get pageSizeOptions(): number[] | undefined {
-    return this._pageSizeOptions;
-  }
-  private _pageSizeOptions?: number[];
+  public accessor pageSizeOptions: number[] = [];
 
   /**
    * Position of the prev/next buttons: if `pageSizeOptions` is set,
@@ -57,9 +48,7 @@ class SbbPaginatorElement extends SbbPaginatorCommonElementMixin(LitElement) {
     | 'start'
     | 'end' = 'start';
 
-  private _paginatorOptionsLabel = `sbb-paginator-options-label-${++nextId}`;
   private _markForFocus: number | null = null;
-  private _updateSelectAriaLabelledBy: boolean = false;
 
   protected override updated(changedProperties: PropertyValues<this>): void {
     super.updated(changedProperties);
@@ -75,16 +64,6 @@ class SbbPaginatorElement extends SbbPaginatorCommonElementMixin(LitElement) {
       // Reset mark for focus
       this._markForFocus = null;
     }
-
-    /**
-     * TODO: Accessibility fix required to correctly read the label;
-     * can be possibly removed after the merge of https://github.com/sbb-design-systems/lyne-components/issues/3062
-     */
-    const select = this.shadowRoot!.querySelector('sbb-select');
-    if (select && this._updateSelectAriaLabelledBy) {
-      select.setAttribute('aria-labelledby', this._paginatorOptionsLabel);
-      this._updateSelectAriaLabelledBy = false;
-    }
   }
 
   /**
@@ -159,9 +138,7 @@ class SbbPaginatorElement extends SbbPaginatorCommonElementMixin(LitElement) {
     return this.pageSizeOptions && this.pageSizeOptions.length > 0
       ? html`
           <div class="sbb-paginator__page-size-options">
-            <label id=${this._paginatorOptionsLabel}>
-              ${i18nItemsPerPage[this.language.current]}
-            </label>
+            <label for="select">${i18nItemsPerPage[this.language.current]}</label>
             <sbb-form-field
               borderless
               width="collapse"
@@ -169,6 +146,7 @@ class SbbPaginatorElement extends SbbPaginatorCommonElementMixin(LitElement) {
               size=${this.size}
             >
               <sbb-select
+                id="select"
                 ?disabled=${this.disabled}
                 value=${this.pageSizeOptions?.find((e) => e === this.pageSize) ??
                 this.pageSizeOptions![0]}
diff --git a/src/elements/paginator/paginator/readme.md b/src/elements/paginator/paginator/readme.md
index 7e9d77d7b6..5fc8b0311f 100644
--- a/src/elements/paginator/paginator/readme.md
+++ b/src/elements/paginator/paginator/readme.md
@@ -65,16 +65,16 @@ that describes the content controlled by the paginator.
 
 ## Properties
 
-| Name              | Attribute           | Privacy | Type                    | Default   | Description                                                                                                                                                                       |
-| ----------------- | ------------------- | ------- | ----------------------- | --------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| `disabled`        | `disabled`          | public  | `boolean`               | `false`   | Whether the component is disabled.                                                                                                                                                |
-| `length`          | `length`            | public  | `number`                | `0`       | Total number of items.                                                                                                                                                            |
-| `negative`        | `negative`          | public  | `boolean`               | `false`   | Negative coloring variant flag.                                                                                                                                                   |
-| `pageIndex`       | `page-index`        | public  | `number`                | `0`       | Current page index.                                                                                                                                                               |
-| `pagerPosition`   | `pager-position`    | public  | `'start' \| 'end'`      | `'start'` | Position of the prev/next buttons: if `pageSizeOptions` is set, the sbb-select for the pageSize change will be positioned oppositely, with the page numbers always in the center. |
-| `pageSize`        | `page-size`         | public  | `number`                | `10`      | Number of items per page.                                                                                                                                                         |
-| `pageSizeOptions` | `page-size-options` | public  | `number[] \| undefined` |           | The available `pageSize` choices.                                                                                                                                                 |
-| `size`            | `size`              | public  | `'m' \| 's'`            | `'m'`     | Size variant, either m or s.                                                                                                                                                      |
+| Name              | Attribute           | Privacy | Type               | Default   | Description                                                                                                                                                                       |
+| ----------------- | ------------------- | ------- | ------------------ | --------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| `disabled`        | `disabled`          | public  | `boolean`          | `false`   | Whether the component is disabled.                                                                                                                                                |
+| `length`          | `length`            | public  | `number`           | `0`       | Total number of items.                                                                                                                                                            |
+| `negative`        | `negative`          | public  | `boolean`          | `false`   | Negative coloring variant flag.                                                                                                                                                   |
+| `pageIndex`       | `page-index`        | public  | `number`           | `0`       | Current page index.                                                                                                                                                               |
+| `pagerPosition`   | `pager-position`    | public  | `'start' \| 'end'` | `'start'` | Position of the prev/next buttons: if `pageSizeOptions` is set, the sbb-select for the pageSize change will be positioned oppositely, with the page numbers always in the center. |
+| `pageSize`        | `page-size`         | public  | `number`           | `10`      | Number of items per page.                                                                                                                                                         |
+| `pageSizeOptions` | `page-size-options` | public  | `number[]`         | `[]`      | The available `pageSize` choices.                                                                                                                                                 |
+| `size`            | `size`              | public  | `'m' \| 's'`       | `'m'`     | Size variant, either m or s.                                                                                                                                                      |
 
 ## Events
 
diff --git a/src/elements/select/readme.md b/src/elements/select/readme.md
index 57679bdc03..8d50c861d8 100644
--- a/src/elements/select/readme.md
+++ b/src/elements/select/readme.md
@@ -77,7 +77,7 @@ The component has no `size` property but, when slotted in a `sbb-form-field`, it
 ```html
 <sbb-form-field size="s">
   <label>Train types</label>
-  <sbb-select> ... </sbb-select>
+  <sbb-select>...</sbb-select>
 </sbb-form-field>
 ```
 
@@ -87,6 +87,30 @@ Consumers can listen to the native `change`/`input` event on the `sbb-select` co
 the current value can be read from `event.target.value`.
 Additionally `sbb-option` will emit `optionSelected` when selected via user interaction.
 
+## Accessibility
+
+The select follows the combobox pattern. As a technical difficulty, we have to copy the combobox element into the light DOM.
+As a consequence, linking labels is not fully supported. While `aria-label`, `aria-labelledby` and `aria-describedby` on the `sbb-select` work,
+using `<label>` together with `sbb-select` is only partially supported.
+As workaround, we copy the text into the aria-label of the combobox element, but this remains not synchronized.
+Whenever a `<label>` gets a change, we won't be able to detect it, and we won't be able to update the `aria-label`.
+The only two exceptions are when `connectedCallback()` gets called and when the document language changes.
+
+Fully supported:
+
+```html
+<sbb-select aria-label="Select train type">...</sbb-select>
+```
+
+Changes to the `<label>`-text might not be reflected after initialization:
+
+```html
+<sbb-form-field size="s">
+  <label>Train types</label>
+  <sbb-select>...</sbb-select>
+</sbb-form-field>
+```
+
 ## Keyboard interaction
 
 Closed panel, `sbb-select` has focus:
diff --git a/src/elements/select/select.spec.ts b/src/elements/select/select.spec.ts
index fcffce68b5..24475a6474 100644
--- a/src/elements/select/select.spec.ts
+++ b/src/elements/select/select.spec.ts
@@ -43,7 +43,7 @@ describe(`sbb-select`, () => {
       assert.instanceOf(firstOption, SbbOptionElement);
     });
 
-    it('opens and closes the dialog', async () => {
+    it('opens and closes the select', async () => {
       const willOpen = new EventSpy(SbbSelectElement.events.willOpen);
       const didOpen = new EventSpy(SbbSelectElement.events.didOpen);
       const willClose = new EventSpy(SbbSelectElement.events.willClose);
@@ -555,4 +555,136 @@ describe(`sbb-select`, () => {
       compareToNative();
     });
   });
+
+  describe('label handling', () => {
+    it('should sync aria-label initially', async () => {
+      const element = await fixture(html`
+        <sbb-select aria-label="Test">
+          <sbb-option id="option-1" value="1">First</sbb-option>
+        </sbb-select>
+      `);
+
+      const comboBoxElement = element.parentElement!.querySelector('[role="combobox"]')!;
+
+      expect(comboBoxElement).to.have.attribute('aria-label', 'Test');
+    });
+
+    it('should sync aria-label on change', async () => {
+      const element = await fixture(html`
+        <sbb-select>
+          <sbb-option id="option-1" value="1">First</sbb-option>
+        </sbb-select>
+      `);
+      const comboBoxElement = element.parentElement!.querySelector('[role="combobox"]')!;
+      expect(comboBoxElement).not.to.have.attribute('aria-label');
+
+      element.setAttribute('aria-label', 'Test');
+      await waitForLitRender(element);
+
+      expect(comboBoxElement).to.have.attribute('aria-label', 'Test');
+    });
+
+    it('should prefer aria-label over label element', async () => {
+      const element = await fixture(html`
+        <label for="select">Ignored</label>
+        <sbb-select aria-label="Test" id="select">
+          <sbb-option id="option-1" value="1">First</sbb-option>
+        </sbb-select>
+      `);
+
+      const comboBoxElement = element.parentElement!.querySelector('[role="combobox"]')!;
+
+      expect(comboBoxElement).to.have.attribute('aria-label', 'Test');
+    });
+
+    it('should sync aria-labelledby initially', async () => {
+      const element = await fixture(html`
+        <sbb-select aria-labelledby="Test">
+          <sbb-option id="option-1" value="1">First</sbb-option>
+        </sbb-select>
+      `);
+
+      const comboBoxElement = element.parentElement!.querySelector('[role="combobox"]')!;
+
+      expect(comboBoxElement).to.have.attribute('aria-labelledby', 'Test');
+    });
+
+    it('should sync aria-labelledby on change', async () => {
+      const element = await fixture(html`
+        <sbb-select>
+          <sbb-option id="option-1" value="1">First</sbb-option>
+        </sbb-select>
+      `);
+      const comboBoxElement = element.parentElement!.querySelector('[role="combobox"]')!;
+      expect(comboBoxElement).not.to.have.attribute('aria-labelledby');
+
+      element.setAttribute('aria-labelledby', 'Test');
+      await waitForLitRender(element);
+
+      expect(comboBoxElement).to.have.attribute('aria-labelledby', 'Test');
+    });
+
+    it('should prefer aria-labelledby over label element', async () => {
+      const element = await fixture(html`
+        <label for="select">Ignored</label>
+        <sbb-select aria-labelledby="Test" id="select">
+          <sbb-option id="option-1" value="1">First</sbb-option>
+        </sbb-select>
+      `);
+
+      const comboBoxElement = element.parentElement!.querySelector('[role="combobox"]')!;
+
+      expect(comboBoxElement).to.have.attribute('aria-labelledby', 'Test');
+    });
+
+    it('should combine aria-describedby with label element', async () => {
+      const element = await fixture(html`
+        <label for="select">Label</label>
+        <sbb-select aria-describedby="Test" id="select">
+          <sbb-option id="option-1" value="1">First</sbb-option>
+        </sbb-select>
+      `);
+
+      const comboBoxElement = element.parentElement!.querySelector('[role="combobox"]')!;
+
+      expect(comboBoxElement).to.have.attribute('aria-describedby', 'Test');
+      expect(comboBoxElement).to.have.attribute('aria-label', 'Label');
+    });
+
+    it('should take label elements as aria-label', async () => {
+      const element = await fixture(html`
+        <label for="select">Label</label>
+        <label for="select">Label 2</label>
+        <sbb-select id="select">
+          <sbb-option id="option-1" value="1">First</sbb-option>
+        </sbb-select>
+      `);
+
+      const comboBoxElement = element.parentElement!.querySelector('[role="combobox"]')!;
+
+      expect(comboBoxElement).to.have.attribute('aria-label', 'Label, Label 2');
+    });
+
+    it('should remove label when disappearing', async () => {
+      const root = await fixture(
+        html`<div>
+          <label for="select">Label</label>
+          <sbb-select id="select">
+            <sbb-option id="option-1" value="1">First</sbb-option>
+          </sbb-select>
+        </div> `,
+      );
+
+      const element = root.querySelector('sbb-select')!;
+      const comboBoxElement = root.querySelector('[role="combobox"]')!;
+      expect(comboBoxElement).to.have.attribute('aria-label', 'Label');
+
+      root.querySelector('label')!.remove();
+
+      // Trigger sync by triggering connectedCallback()
+      element.connectedCallback();
+
+      expect(comboBoxElement).not.to.have.attribute('aria-label');
+    });
+  });
 });
diff --git a/src/elements/select/select.ts b/src/elements/select/select.ts
index 8814654d37..231b093d9b 100644
--- a/src/elements/select/select.ts
+++ b/src/elements/select/select.ts
@@ -7,9 +7,9 @@ import { until } from 'lit/directives/until.js';
 
 import { getNextElementIndex } from '../core/a11y.js';
 import { SbbOpenCloseBaseElement } from '../core/base-elements.js';
-import { SbbConnectedAbortController } from '../core/controllers.js';
+import { SbbConnectedAbortController, SbbLanguageController } from '../core/controllers.js';
 import { forceType, hostAttributes } from '../core/decorators.js';
-import { isNextjs, isSafari } from '../core/dom.js';
+import { isNextjs, isSafari, setOrRemoveAttribute } from '../core/dom.js';
 import { EventEmitter } from '../core/eventing.js';
 import {
   type FormRestoreReason,
@@ -161,23 +161,43 @@ class SbbSelectElement extends SbbUpdateSchedulerMixin(
     super();
 
     new MutationController(this, {
-      config: { attributeFilter: ['aria-labelledby'] },
-      callback: (mutationsList: MutationRecord[]) => this._onSelectAttributesChange(mutationsList),
+      config: { attributeFilter: ['aria-labelledby', 'aria-label', 'aria-describedby'] },
+      callback: () => this._syncAriaLabels(),
     });
+
+    new SbbLanguageController(this).withHandler(() => setTimeout(() => this._syncAriaLabels()));
   }
 
-  /**
-   * TODO: Accessibility fix required to correctly read the label;
-   * can be possibly removed after the merge of https://github.com/sbb-design-systems/lyne-components/issues/3062
-   */
-  private _onSelectAttributesChange(mutationsList: MutationRecord[]): void {
-    for (const mutation of mutationsList) {
-      if (mutation.attributeName === 'aria-labelledby') {
-        this._triggerElement?.setAttribute(
-          'aria-labelledby',
-          this.getAttribute('aria-labelledby')!,
-        );
-      }
+  private _syncAriaLabels(): void {
+    if (!this._triggerElement) {
+      return;
+    }
+
+    setOrRemoveAttribute(
+      this._triggerElement,
+      'aria-labelledby',
+      this.getAttribute('aria-labelledby'),
+    );
+    setOrRemoveAttribute(this._triggerElement, 'aria-label', this.getAttribute('aria-label'));
+    setOrRemoveAttribute(
+      this._triggerElement,
+      'aria-describedby',
+      this.getAttribute('aria-describedby'),
+    );
+
+    // Using the associated labels is only a fallback.
+    // The drawback is, that it doesn't get updated automatically when the list of label changes.
+    if (
+      !this.getAttribute('aria-label') &&
+      !this.getAttribute('aria-labelledby') &&
+      this.internals.labels.length
+    ) {
+      this._triggerElement?.setAttribute(
+        'aria-label',
+        Array.from(this.internals.labels)
+          .map((label) => label.textContent)
+          .join(', '),
+      );
     }
   }
 
@@ -311,6 +331,7 @@ class SbbSelectElement extends SbbUpdateSchedulerMixin(
       this.negative = formField.hasAttribute('negative');
     }
     this._syncProperties();
+    this._syncAriaLabels();
 
     if (this._didLoad) {
       this._setupOrigin();