Skip to content

Commit

Permalink
fix(sbb-select): improve connected label handling (#3229)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jeripeierSBB
jeripeierSBB authored and github-actions committed Nov 21, 2024
1 parent edb639b commit 4d66d32
Show file tree
Hide file tree
Showing 7 changed files with 237 additions and 61 deletions.
13 changes: 6 additions & 7 deletions src/elements/paginator/paginator/__snapshots__/paginator.snapshot.spec.snap.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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"
Expand All @@ -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"
>
Expand Down
24 changes: 23 additions & 1 deletion src/elements/paginator/paginator/paginator.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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);
});
});
28 changes: 3 additions & 25 deletions src/elements/paginator/paginator/paginator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ import '../../screen-reader-only.js';

const MAX_PAGE_NUMBERS_DISPLAYED = 3;

let nextId = 0;

/**
* It displays a paginator component.
*
Expand All @@ -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,
Expand All @@ -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);
Expand All @@ -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;
}
}

/**
Expand Down Expand Up @@ -159,16 +138,15 @@ 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"
?negative=${this.negative}
size=${this.size}
>
<sbb-select
id="select"
?disabled=${this.disabled}
value=${this.pageSizeOptions?.find((e) => e === this.pageSize) ??
this.pageSizeOptions![0]}
Expand Down
20 changes: 10 additions & 10 deletions src/elements/paginator/paginator/readme.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
26 changes: 25 additions & 1 deletion src/elements/select/readme.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
```

Expand All @@ -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:
Expand Down
134 changes: 133 additions & 1 deletion src/elements/select/select.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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');
});
});
});
Loading

0 comments on commit 4d66d32

Please sign in to comment.