fix(a11y): name every form control in Settings
Measured with Accessibility.getFullAXTree against the running app with all seven sections expanded: 24 of 93 controls computed an empty name. Every config-field select and toggle, and all eighteen track-list column checkboxes, had a <label> sitting right beside them with nothing associating the two. Now 0 of 93. Not in the audit, and a11y.6 says why in its own line: it scanned every <button>, and none of these is one. Same shape as the count that sent Phase 1 looking for an unnamed sort control — the claim was answering a narrower question than it reads as. The fields use `for`/`id` rather than aria-label, for what it buys beyond the name: the label text becomes a click target for the control. A fixed id is safe only because each config-field is its own shadow root. Two more are named but identify nothing, which is a11y.32's complaint one page over: three shortcut buttons announced themselves as "S", and thirty-six column arrows as "Move up".
This commit is contained in:
@@ -1,6 +1,26 @@
|
|||||||
import { LitElement, html, css, nothing } from 'lit';
|
import { LitElement, html, css, nothing } from 'lit';
|
||||||
import { customElement, property } from 'lit/decorators.js';
|
import { customElement, property } from 'lit/decorators.js';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The id every field's control carries, so its `<label>` can name it.
|
||||||
|
*
|
||||||
|
* The label was a *sibling* of the control with no `for`, which names
|
||||||
|
* nothing — so every select, toggle and text field in Settings computed
|
||||||
|
* an empty accessible name. Measured on the expanded Settings page:
|
||||||
|
* **24 of 93 controls unnamed**, six of them here and the rest the
|
||||||
|
* column toggles in `config-page`. `a11y.6` is not wrong about this;
|
||||||
|
* it says in the same line that it scanned every `<button>`, and none
|
||||||
|
* of these is one.
|
||||||
|
*
|
||||||
|
* A fixed id is safe, and only because each `config-field` is its own
|
||||||
|
* shadow root — the whole page renders a dozen elements with
|
||||||
|
* `id="control"` and each `for` resolves within its own root. It is
|
||||||
|
* preferred over `aria-label` for what it buys beyond the name: a real
|
||||||
|
* label association also makes the label text a click target for the
|
||||||
|
* control, which is behaviour, not annotation.
|
||||||
|
*/
|
||||||
|
const CONTROL_ID = 'control';
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Schema describing a single config field.
|
* Schema describing a single config field.
|
||||||
*
|
*
|
||||||
@@ -225,7 +245,9 @@ export class ConfigField extends LitElement {
|
|||||||
${this.schema.type === 'toggle'
|
${this.schema.type === 'toggle'
|
||||||
? this.renderToggle()
|
? this.renderToggle()
|
||||||
: html`
|
: html`
|
||||||
<label>${this.schema.label}</label>
|
<label for=${CONTROL_ID}>
|
||||||
|
${this.schema.label}
|
||||||
|
</label>
|
||||||
${this.renderInput()}
|
${this.renderInput()}
|
||||||
`}
|
`}
|
||||||
${this.schema.description
|
${this.schema.description
|
||||||
@@ -257,6 +279,7 @@ export class ConfigField extends LitElement {
|
|||||||
private renderText() {
|
private renderText() {
|
||||||
return html`
|
return html`
|
||||||
<input
|
<input
|
||||||
|
id=${CONTROL_ID}
|
||||||
type="text"
|
type="text"
|
||||||
.value=${String(this.value ?? '')}
|
.value=${String(this.value ?? '')}
|
||||||
?disabled=${this.schema.disabled}
|
?disabled=${this.schema.disabled}
|
||||||
@@ -268,6 +291,7 @@ export class ConfigField extends LitElement {
|
|||||||
private renderNumber() {
|
private renderNumber() {
|
||||||
return html`
|
return html`
|
||||||
<input
|
<input
|
||||||
|
id=${CONTROL_ID}
|
||||||
type="number"
|
type="number"
|
||||||
.value=${String(this.value ?? '')}
|
.value=${String(this.value ?? '')}
|
||||||
?disabled=${this.schema.disabled}
|
?disabled=${this.schema.disabled}
|
||||||
@@ -281,6 +305,7 @@ export class ConfigField extends LitElement {
|
|||||||
|
|
||||||
return html`
|
return html`
|
||||||
<select
|
<select
|
||||||
|
id=${CONTROL_ID}
|
||||||
?disabled=${this.schema.disabled}
|
?disabled=${this.schema.disabled}
|
||||||
@change=${this.onSelectChange}
|
@change=${this.onSelectChange}
|
||||||
>
|
>
|
||||||
@@ -304,6 +329,7 @@ export class ConfigField extends LitElement {
|
|||||||
return html`
|
return html`
|
||||||
<div class="color-wrapper">
|
<div class="color-wrapper">
|
||||||
<input
|
<input
|
||||||
|
id=${CONTROL_ID}
|
||||||
type="color"
|
type="color"
|
||||||
.value=${hex}
|
.value=${hex}
|
||||||
?disabled=${this.schema.disabled}
|
?disabled=${this.schema.disabled}
|
||||||
@@ -318,11 +344,13 @@ export class ConfigField extends LitElement {
|
|||||||
return html`
|
return html`
|
||||||
<div class="input-row">
|
<div class="input-row">
|
||||||
<input
|
<input
|
||||||
|
id=${CONTROL_ID}
|
||||||
type="text"
|
type="text"
|
||||||
.value=${String(this.value ?? '')}
|
.value=${String(this.value ?? '')}
|
||||||
readonly
|
readonly
|
||||||
/>
|
/>
|
||||||
<button
|
<button
|
||||||
|
aria-label="Browse for ${this.schema.label}"
|
||||||
?disabled=${this.schema.disabled}
|
?disabled=${this.schema.disabled}
|
||||||
@click=${this.onBrowseClick}
|
@click=${this.onBrowseClick}
|
||||||
>
|
>
|
||||||
@@ -337,9 +365,10 @@ export class ConfigField extends LitElement {
|
|||||||
|
|
||||||
return html`
|
return html`
|
||||||
<div class="toggle-row">
|
<div class="toggle-row">
|
||||||
<label>${this.schema.label}</label>
|
<label for=${CONTROL_ID}>${this.schema.label}</label>
|
||||||
<label class="toggle-switch">
|
<label class="toggle-switch">
|
||||||
<input
|
<input
|
||||||
|
id=${CONTROL_ID}
|
||||||
type="checkbox"
|
type="checkbox"
|
||||||
?checked=${checked}
|
?checked=${checked}
|
||||||
?disabled=${this.schema.disabled}
|
?disabled=${this.schema.disabled}
|
||||||
|
|||||||
@@ -1693,6 +1693,9 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) {
|
|||||||
const isLast =
|
const isLast =
|
||||||
idx === order.length - 1;
|
idx === order.length - 1;
|
||||||
|
|
||||||
|
const columnLabel =
|
||||||
|
COLUMN_DEFS[id]?.label ?? id;
|
||||||
|
|
||||||
return html`
|
return html`
|
||||||
<li
|
<li
|
||||||
class="column-item ${checked ? 'enabled' : 'disabled'}"
|
class="column-item ${checked ? 'enabled' : 'disabled'}"
|
||||||
@@ -1700,6 +1703,7 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) {
|
|||||||
<input
|
<input
|
||||||
type="checkbox"
|
type="checkbox"
|
||||||
class="column-toggle"
|
class="column-toggle"
|
||||||
|
aria-label="Show the ${columnLabel} column"
|
||||||
.checked=${checked}
|
.checked=${checked}
|
||||||
?disabled=${onlyOne}
|
?disabled=${onlyOne}
|
||||||
@change=${() =>
|
@change=${() =>
|
||||||
@@ -1710,9 +1714,7 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) {
|
|||||||
<span
|
<span
|
||||||
class="column-label"
|
class="column-label"
|
||||||
>
|
>
|
||||||
${COLUMN_DEFS[id]
|
${columnLabel}
|
||||||
?.label ??
|
|
||||||
id}
|
|
||||||
</span>
|
</span>
|
||||||
<span
|
<span
|
||||||
class="column-arrows"
|
class="column-arrows"
|
||||||
@@ -1723,6 +1725,7 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) {
|
|||||||
<button
|
<button
|
||||||
class="column-arrow-btn"
|
class="column-arrow-btn"
|
||||||
title="Move up"
|
title="Move up"
|
||||||
|
aria-label="Move ${columnLabel} up"
|
||||||
@click=${() =>
|
@click=${() =>
|
||||||
this.handleColumnMove(
|
this.handleColumnMove(
|
||||||
id,
|
id,
|
||||||
@@ -1738,6 +1741,7 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) {
|
|||||||
<button
|
<button
|
||||||
class="column-arrow-btn"
|
class="column-arrow-btn"
|
||||||
title="Move down"
|
title="Move down"
|
||||||
|
aria-label="Move ${columnLabel} down"
|
||||||
@click=${() =>
|
@click=${() =>
|
||||||
this.handleColumnMove(
|
this.handleColumnMove(
|
||||||
id,
|
id,
|
||||||
@@ -1812,6 +1816,7 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) {
|
|||||||
</span>
|
</span>
|
||||||
<shortcut-capture
|
<shortcut-capture
|
||||||
.action=${action}
|
.action=${action}
|
||||||
|
.label=${meta.label}
|
||||||
.currentKey=${bindings.get(
|
.currentKey=${bindings.get(
|
||||||
action,
|
action,
|
||||||
) ?? ''}
|
) ?? ''}
|
||||||
|
|||||||
@@ -8,6 +8,17 @@ export class ShortcutCapture extends LitElement {
|
|||||||
@property() currentKey = '';
|
@property() currentKey = '';
|
||||||
@property() defaultKey = '';
|
@property() defaultKey = '';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* What the binding does, for the name.
|
||||||
|
*
|
||||||
|
* The button's text is the *key* — so the shortcuts list rendered
|
||||||
|
* three buttons called "S", two called "Down" and one called "?",
|
||||||
|
* beside a visible label that named none of them (it is a sibling,
|
||||||
|
* in another shadow root, and nothing associated the two). The
|
||||||
|
* label is what a control is for; the key is its value.
|
||||||
|
*/
|
||||||
|
@property() label = '';
|
||||||
|
|
||||||
@state() private recording = false;
|
@state() private recording = false;
|
||||||
|
|
||||||
static override styles = css`
|
static override styles = css`
|
||||||
@@ -134,6 +145,9 @@ export class ShortcutCapture extends LitElement {
|
|||||||
: this.currentKey
|
: this.currentKey
|
||||||
? ''
|
? ''
|
||||||
: 'not-set'}
|
: 'not-set'}
|
||||||
|
aria-label=${this.label
|
||||||
|
? `${this.label} shortcut: ${this.currentKey || 'not set'}`
|
||||||
|
: ''}
|
||||||
@click=${this.handleClick}
|
@click=${this.handleClick}
|
||||||
@keydown=${this.handleKeydown}
|
@keydown=${this.handleKeydown}
|
||||||
@blur=${this.handleBlur}
|
@blur=${this.handleBlur}
|
||||||
@@ -148,6 +162,8 @@ export class ShortcutCapture extends LitElement {
|
|||||||
class="reset-btn"
|
class="reset-btn"
|
||||||
@click=${this.handleReset}
|
@click=${this.handleReset}
|
||||||
title="Reset to default (${this.defaultKey})"
|
title="Reset to default (${this.defaultKey})"
|
||||||
|
aria-label="Reset ${this.label ||
|
||||||
|
this.action} to ${this.defaultKey}"
|
||||||
>
|
>
|
||||||
\u21BA
|
\u21BA
|
||||||
</button>
|
</button>
|
||||||
|
|||||||
@@ -0,0 +1,116 @@
|
|||||||
|
/**
|
||||||
|
* Every form control in Settings had a visible label and no name.
|
||||||
|
*
|
||||||
|
* Not in the audit, and `a11y.6` says why in its own text: it scanned
|
||||||
|
* every `<button>`, and a `<select>` is not one. Measured with
|
||||||
|
* `Accessibility.getFullAXTree` against the running app with all seven
|
||||||
|
* sections expanded — **24 of 93 controls unnamed**: every
|
||||||
|
* `config-field` select and toggle, and all eighteen track-list column
|
||||||
|
* checkboxes. In each case a `<label>` sat right beside the control
|
||||||
|
* with nothing associating the two.
|
||||||
|
*
|
||||||
|
* Two of the fixes here are about a name that exists and does not
|
||||||
|
* identify anything, which is `a11y.32`'s complaint one page over:
|
||||||
|
* three shortcut buttons announced themselves as "S", and thirty-six
|
||||||
|
* column arrows as "Move up" or "Move down".
|
||||||
|
*/
|
||||||
|
import { describe, expect, it } from 'vitest';
|
||||||
|
|
||||||
|
import '@components/config-page/config-field';
|
||||||
|
import '@components/config-page/shortcut-capture';
|
||||||
|
import { fixture, shadow } from '@test/support/render';
|
||||||
|
|
||||||
|
/** What the `<label>` in this shadow root actually points at. */
|
||||||
|
function labelledControl(host: Element): Element | null {
|
||||||
|
const forId = shadow(host, 'label[for]')?.getAttribute('for');
|
||||||
|
|
||||||
|
return forId ? shadow(host, `#${CSS.escape(forId)}`) : null;
|
||||||
|
}
|
||||||
|
|
||||||
|
describe('a config field names its control', () => {
|
||||||
|
it('associates the label with a select', async () => {
|
||||||
|
const el = await fixture('config-field', {
|
||||||
|
schema: {
|
||||||
|
key: 'theme.shade',
|
||||||
|
label: 'Shade',
|
||||||
|
type: 'select',
|
||||||
|
options: [{ value: 'dark', label: 'Dark' }],
|
||||||
|
},
|
||||||
|
value: 'dark',
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(labelledControl(el)?.tagName).toBe('SELECT');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('associates the label with a toggle, which is a second label deep', async () => {
|
||||||
|
const el = await fixture('config-field', {
|
||||||
|
schema: { key: 'x', label: 'Scan on startup', type: 'toggle' },
|
||||||
|
value: true,
|
||||||
|
});
|
||||||
|
|
||||||
|
// The checkbox is wrapped in a *styling* label carrying only a
|
||||||
|
// span, which contributes no text and so named nothing.
|
||||||
|
const control = labelledControl(el) as HTMLInputElement | null;
|
||||||
|
|
||||||
|
expect(control?.type).toBe('checkbox');
|
||||||
|
});
|
||||||
|
|
||||||
|
it.each(['text', 'number', 'color', 'directory'] as const)(
|
||||||
|
'associates the label with a %s field',
|
||||||
|
async (type) => {
|
||||||
|
const el = await fixture('config-field', {
|
||||||
|
schema: { key: 'k', label: 'Music folder', type },
|
||||||
|
value: '',
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(labelledControl(el)).toBeTruthy();
|
||||||
|
},
|
||||||
|
);
|
||||||
|
|
||||||
|
it('says which field a Browse button browses for', async () => {
|
||||||
|
const el = await fixture('config-field', {
|
||||||
|
schema: { key: 'k', label: 'Music folder', type: 'directory' },
|
||||||
|
value: '',
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(shadow(el, 'button')?.getAttribute('aria-label'))
|
||||||
|
.toBe('Browse for Music folder');
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe('a shortcut button says what it binds', () => {
|
||||||
|
it('names the action, and keeps the key as the value', async () => {
|
||||||
|
const el = await fixture('shortcut-capture', {
|
||||||
|
action: 'player.playPause',
|
||||||
|
label: 'Play / Pause',
|
||||||
|
currentKey: 'Space',
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(shadow(el, 'button')?.getAttribute('aria-label'))
|
||||||
|
.toBe('Play / Pause shortcut: Space');
|
||||||
|
expect(shadow(el, 'button')?.textContent?.trim()).toBe('Space');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('says so when there is no key rather than announcing nothing', async () => {
|
||||||
|
const el = await fixture('shortcut-capture', {
|
||||||
|
action: 'tracklist.delete',
|
||||||
|
label: 'Delete Track',
|
||||||
|
currentKey: '',
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(shadow(el, 'button')?.getAttribute('aria-label'))
|
||||||
|
.toBe('Delete Track shortcut: not set');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('names the action in the reset button too', async () => {
|
||||||
|
const el = await fixture('shortcut-capture', {
|
||||||
|
action: 'player.next',
|
||||||
|
label: 'Next Track',
|
||||||
|
currentKey: 'X',
|
||||||
|
defaultKey: 'N',
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(shadow(el, '.reset-btn')?.getAttribute('aria-label'))
|
||||||
|
.toBe('Reset Next Track to N');
|
||||||
|
});
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user