Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(sbb-select): improve connected label handling #3229

Merged
merged 3 commits into from
Nov 21, 2024
Merged

fix(sbb-select): improve connected label handling #3229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d09c91e
fix(sbb-select): improve connected label handling
jeripeierSBB Nov 21, 2024
3dfab69
fix: fix integrity
jeripeierSBB Nov 21, 2024
b1a332e
fix: fix review
jeripeierSBB Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions src/elements/paginator/paginator/__snapshots__/paginator.snapshot.spec.snap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,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 @@ -543,7 +543,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 @@ -554,9 +554,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 @@ -708,7 +708,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 @@ -723,7 +723,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 @@ -734,10 +734,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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading