fix(a11y): make the library badge a badge, not an inert button
`library-status-indicator` was a <button> whose click handler was a stopPropagation() and a comment saying to wire up the download client later. On an Explore results page that is 20 of 66 tab stops (measured in the running app, before and after: 66/20 → 46/0) that announce themselves as buttons and do nothing. It is role="img" with its existing label now, and the label for an unowned entity says "… is not in your library" rather than "Add … to library" — the old copy was the button's promise written out. The day there is a download client to call, the right change is a <button> *with* a handler, not a handler bolted onto something already shaped like one. box-sizing: border-box is explicit because a <button> gets it from the UA stylesheet and a <span> does not, so the badge grew 36px → 38px. Caught by the stored screenshot.
This commit is contained in:
@@ -64,6 +64,35 @@ test.describe('playing an album from its page', () => {
|
|||||||
app.locator('explore-album-details').locator('.tracklist-legend'),
|
app.locator('explore-album-details').locator('.tracklist-legend'),
|
||||||
).toContainText('in your library');
|
).toContainText('in your library');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('the ticks are badges, not keyboard stops', async ({ app }) => {
|
||||||
|
// Every one of them was a <button> whose click handler was a
|
||||||
|
// stopPropagation() and a comment saying to wire up the download
|
||||||
|
// client later: on an Explore results page, 20 of 66 tab stops
|
||||||
|
// promised an action and performed none. Counted here rather than
|
||||||
|
// reasoned about, because the count is the finding.
|
||||||
|
const stops = await app.evaluate(() => {
|
||||||
|
const badges = [
|
||||||
|
...(document
|
||||||
|
.querySelector('explore-album-details')
|
||||||
|
?.shadowRoot?.querySelectorAll('library-status-indicator') ?? []),
|
||||||
|
];
|
||||||
|
|
||||||
|
return {
|
||||||
|
badges: badges.length,
|
||||||
|
focusable: badges.filter((b) =>
|
||||||
|
b.shadowRoot?.querySelector('button, [tabindex]:not([tabindex="-1"])'),
|
||||||
|
).length,
|
||||||
|
labelled: badges.filter((b) =>
|
||||||
|
b.shadowRoot?.querySelector('[role="img"][aria-label]'),
|
||||||
|
).length,
|
||||||
|
};
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(stops.badges).toBeGreaterThan(0);
|
||||||
|
expect(stops.focusable).toBe(0);
|
||||||
|
expect(stops.labelled).toBe(stops.badges);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
/** Albums → click the second card, which navigates to the album page. */
|
/** Albums → click the second card, which navigates to the album page. */
|
||||||
|
|||||||
@@ -9,17 +9,26 @@ import '@awesome.me/webawesome/dist/components/icon/icon.js';
|
|||||||
* - `queued`: the entity has been handed off to a download client but
|
* - `queued`: the entity has been handed off to a download client but
|
||||||
* hasn't arrived yet. Reserved for future download-client plumbing.
|
* hasn't arrived yet. Reserved for future download-client plumbing.
|
||||||
* - `not-in-library` (default): the entity is not owned and has not
|
* - `not-in-library` (default): the entity is not owned and has not
|
||||||
* been requested. A click should eventually kick off a download,
|
* been requested.
|
||||||
* but for now the button is inert.
|
|
||||||
*/
|
*/
|
||||||
export type LibraryStatus = 'in-library' | 'queued' | 'not-in-library';
|
export type LibraryStatus = 'in-library' | 'queued' | 'not-in-library';
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Tri-state library status indicator rendered as a small circular
|
* Tri-state library status indicator: a small circular badge embedded
|
||||||
* button. Intended to be embedded in track rows, album cards, and
|
* in track rows, album cards, and artist cards.
|
||||||
* artist cards. The click handler is a no-op for now — the button
|
*
|
||||||
* exists so the layout is stable when "add to library" integration
|
* **It is a badge, not a control.** It was a `<button>` whose click
|
||||||
* lands later.
|
* handler was a `stopPropagation()` and a comment saying to wire up
|
||||||
|
* the download client later — so an Explore results page offered 20
|
||||||
|
* keyboard stops (of 66) that promised an action and performed none,
|
||||||
|
* and every one of them announced itself as a button. A control that
|
||||||
|
* cannot act is worse than no control: it costs the keyboard user the
|
||||||
|
* tab stop *and* the expectation.
|
||||||
|
*
|
||||||
|
* So it is `role="img"` with a label, until there is something to
|
||||||
|
* click. When the download-client integration lands, the right change
|
||||||
|
* is to make it a `<button>` again *with a handler* — not to add the
|
||||||
|
* handler to something already shaped like a button.
|
||||||
*
|
*
|
||||||
* Colours and glyphs:
|
* Colours and glyphs:
|
||||||
* - in-library → green circle, check mark
|
* - in-library → green circle, check mark
|
||||||
@@ -83,7 +92,12 @@ export class LibraryStatusIndicator extends LitElement {
|
|||||||
--indicator-border: rgba(255, 255, 255, 0.2);
|
--indicator-border: rgba(255, 255, 255, 0.2);
|
||||||
}
|
}
|
||||||
|
|
||||||
button {
|
.badge {
|
||||||
|
/* A <button> gets box-sizing: border-box from the UA
|
||||||
|
* stylesheet and a <span> does not, so dropping the button
|
||||||
|
* grew the badge by its 1px border on each side — 36px to
|
||||||
|
* 38px, caught by the stored screenshot. */
|
||||||
|
box-sizing: border-box;
|
||||||
width: var(--indicator-size);
|
width: var(--indicator-size);
|
||||||
height: var(--indicator-size);
|
height: var(--indicator-size);
|
||||||
min-width: var(--indicator-size);
|
min-width: var(--indicator-size);
|
||||||
@@ -93,33 +107,12 @@ export class LibraryStatusIndicator extends LitElement {
|
|||||||
color: var(--indicator-fg);
|
color: var(--indicator-fg);
|
||||||
border: 1px solid var(--indicator-border);
|
border: 1px solid var(--indicator-border);
|
||||||
padding: 0;
|
padding: 0;
|
||||||
cursor: pointer;
|
|
||||||
display: inline-flex;
|
display: inline-flex;
|
||||||
align-items: center;
|
align-items: center;
|
||||||
justify-content: center;
|
justify-content: center;
|
||||||
transition:
|
|
||||||
background-color 150ms ease,
|
|
||||||
color 150ms ease,
|
|
||||||
transform 120ms ease,
|
|
||||||
border-color 150ms ease;
|
|
||||||
-webkit-tap-highlight-color: transparent;
|
-webkit-tap-highlight-color: transparent;
|
||||||
}
|
}
|
||||||
|
|
||||||
button:hover {
|
|
||||||
transform: scale(1.08);
|
|
||||||
}
|
|
||||||
|
|
||||||
:host([status='not-in-library']) button:hover {
|
|
||||||
background: rgba(255, 255, 255, 0.14);
|
|
||||||
color: #fff;
|
|
||||||
border-color: rgba(255, 255, 255, 0.3);
|
|
||||||
}
|
|
||||||
|
|
||||||
button:focus-visible {
|
|
||||||
outline: 2px solid var(--yj-accent, #1db954);
|
|
||||||
outline-offset: 2px;
|
|
||||||
}
|
|
||||||
|
|
||||||
wa-icon {
|
wa-icon {
|
||||||
font-size: calc(var(--indicator-size) * 0.55);
|
font-size: calc(var(--indicator-size) * 0.55);
|
||||||
line-height: 1;
|
line-height: 1;
|
||||||
@@ -158,23 +151,9 @@ export class LibraryStatusIndicator extends LitElement {
|
|||||||
case 'queued':
|
case 'queued':
|
||||||
return `${capitalize(kind)}${name} is queued for download`;
|
return `${capitalize(kind)}${name} is queued for download`;
|
||||||
default:
|
default:
|
||||||
return `Add ${kind}${name} to library`;
|
// Not "Add … to library": nothing here adds anything.
|
||||||
}
|
// The old copy was the button's promise written out.
|
||||||
}
|
return `${capitalize(kind)}${name} is not in your library`;
|
||||||
|
|
||||||
private handleClick(e: Event) {
|
|
||||||
// Stop propagation so clicking the button doesn't bubble up
|
|
||||||
// to the parent card and trigger navigation. The click
|
|
||||||
// itself is a no-op for now — wire up download-client
|
|
||||||
// integration later.
|
|
||||||
e.stopPropagation();
|
|
||||||
}
|
|
||||||
|
|
||||||
private handleKeydown(e: KeyboardEvent) {
|
|
||||||
// Same reasoning: don't let Enter/Space bubble to a wrapping
|
|
||||||
// card and trigger navigation.
|
|
||||||
if (e.key === 'Enter' || e.key === ' ') {
|
|
||||||
e.stopPropagation();
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -187,17 +166,11 @@ export class LibraryStatusIndicator extends LitElement {
|
|||||||
const title = this.tooltip();
|
const title = this.tooltip();
|
||||||
|
|
||||||
return html`
|
return html`
|
||||||
<button
|
<span class="badge" role="img" title=${title} aria-label=${title}>
|
||||||
type="button"
|
|
||||||
title=${title}
|
|
||||||
aria-label=${title}
|
|
||||||
@click=${this.handleClick}
|
|
||||||
@keydown=${this.handleKeydown}
|
|
||||||
>
|
|
||||||
${this.iconName()
|
${this.iconName()
|
||||||
? html`<wa-icon name=${this.iconName()}></wa-icon>`
|
? html`<wa-icon name=${this.iconName()} aria-hidden="true"></wa-icon>`
|
||||||
: nothing}
|
: nothing}
|
||||||
</button>
|
</span>
|
||||||
`;
|
`;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -176,26 +176,43 @@ describe('<library-status-indicator>', () => {
|
|||||||
label: 'Abbey Road',
|
label: 'Abbey Road',
|
||||||
});
|
});
|
||||||
|
|
||||||
expect(shadow(el, 'button')?.getAttribute('aria-label')).toBe(
|
expect(shadow(el, '.badge')?.getAttribute('aria-label')).toBe(
|
||||||
'Album "Abbey Road" is in your library',
|
'Album "Abbey Road" is in your library',
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('phrases an unowned entity as an invitation', async () => {
|
it('states an unowned entity rather than offering to add it', async () => {
|
||||||
|
// The old copy was "Add artist “Eno” to library", which is the
|
||||||
|
// promise the inert button was making. Nothing here adds anything.
|
||||||
const el = await fixture('library-status-indicator', {
|
const el = await fixture('library-status-indicator', {
|
||||||
entityType: 'artist',
|
entityType: 'artist',
|
||||||
label: 'Eno',
|
label: 'Eno',
|
||||||
});
|
});
|
||||||
|
|
||||||
expect(shadow(el, 'button')?.getAttribute('aria-label')).toBe(
|
expect(shadow(el, '.badge')?.getAttribute('aria-label')).toBe(
|
||||||
'Add artist "Eno" to library',
|
'Artist "Eno" is not in your library',
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('is a badge, not a keyboard stop', async () => {
|
||||||
|
// 20 of the 66 tab stops on an Explore results page were these,
|
||||||
|
// each announcing itself as a button and doing nothing.
|
||||||
|
const el = await fixture('library-status-indicator', {
|
||||||
|
status: 'in-library',
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(shadow(el, 'button')).toBeNull();
|
||||||
|
|
||||||
|
const badge = shadow(el, '.badge');
|
||||||
|
|
||||||
|
expect(badge?.getAttribute('role')).toBe('img');
|
||||||
|
expect(badge?.hasAttribute('tabindex')).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
it('drops the quoted name when it has none', async () => {
|
it('drops the quoted name when it has none', async () => {
|
||||||
const el = await fixture('library-status-indicator', { status: 'queued' });
|
const el = await fixture('library-status-indicator', { status: 'queued' });
|
||||||
|
|
||||||
expect(shadow(el, 'button')?.getAttribute('aria-label')).toBe(
|
expect(shadow(el, '.badge')?.getAttribute('aria-label')).toBe(
|
||||||
'Track is queued for download',
|
'Track is queued for download',
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
@@ -205,14 +222,15 @@ describe('<library-status-indicator>', () => {
|
|||||||
status: 'in-library',
|
status: 'in-library',
|
||||||
});
|
});
|
||||||
|
|
||||||
const button = shadow(el, 'button');
|
const badge = shadow(el, '.badge');
|
||||||
|
|
||||||
expect(button?.getAttribute('title')).toBe(
|
expect(badge?.getAttribute('title')).toBe(badge?.getAttribute('aria-label'));
|
||||||
button?.getAttribute('aria-label'),
|
|
||||||
);
|
|
||||||
});
|
});
|
||||||
|
|
||||||
it('swallows the click, so it does not navigate the card it sits on', async () => {
|
it('lets a click reach the card it sits on, and calls nothing itself', async () => {
|
||||||
|
// It used to stopPropagation() so its own no-op click would not
|
||||||
|
// navigate the card. With no click of its own, the badge is part
|
||||||
|
// of the card and a click on it means what the card means.
|
||||||
const el = await fixture('library-status-indicator');
|
const el = await fixture('library-status-indicator');
|
||||||
let bubbled = 0;
|
let bubbled = 0;
|
||||||
|
|
||||||
@@ -220,27 +238,9 @@ describe('<library-status-indicator>', () => {
|
|||||||
bubbled += 1;
|
bubbled += 1;
|
||||||
});
|
});
|
||||||
|
|
||||||
shadow<HTMLElement>(el, 'button')?.click();
|
shadow<HTMLElement>(el, '.badge')?.click();
|
||||||
|
|
||||||
expect([bubbled, calls()]).toEqual([0, []]);
|
expect([bubbled, calls()]).toEqual([1, []]);
|
||||||
});
|
|
||||||
|
|
||||||
it('swallows Enter and Space for the same reason', async () => {
|
|
||||||
const el = await fixture('library-status-indicator');
|
|
||||||
let bubbled = 0;
|
|
||||||
|
|
||||||
el.addEventListener('keydown', () => {
|
|
||||||
bubbled += 1;
|
|
||||||
});
|
|
||||||
|
|
||||||
for (const key of ['Enter', ' ', 'Tab']) {
|
|
||||||
shadow(el, 'button')?.dispatchEvent(
|
|
||||||
new KeyboardEvent('keydown', { key, bubbles: true, composed: true }),
|
|
||||||
);
|
|
||||||
}
|
|
||||||
|
|
||||||
// Tab still gets through: it is navigation, not activation.
|
|
||||||
expect(bubbled).toBe(1);
|
|
||||||
});
|
});
|
||||||
|
|
||||||
it('honours a non-default size', async () => {
|
it('honours a non-default size', async () => {
|
||||||
@@ -256,7 +256,7 @@ describe('<library-status-indicator>', () => {
|
|||||||
|
|
||||||
await update(el, { size: 40 });
|
await update(el, { size: 40 });
|
||||||
await visual(el, 'library-status-indicator-in-library');
|
await visual(el, 'library-status-indicator-in-library');
|
||||||
expect(shadow(el, 'button')).not.toBeNull();
|
expect(shadow(el, '.badge')).not.toBeNull();
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user