fix(a11y): move the card grids by a row, not to the end
Build & publish Arch package / arch-package (push) Successful in 2m6s
CI / check (push) Successful in 2m28s
Search index maintenance / maintain-index (push) Successful in 6s
CI / e2e (push) Successful in 5m6s

`RovingGridController.measureColumns` read `offsetTop`, and every card
in these grids is drawn by a `lit-virtualizer`, which positions its
children with a transform — which `offsetTop` does not see. So all of
them reported 0, every rendered card counted as one row, and ArrowDown
was `min(i + everything, last)` while ArrowUp was `max(i - everything,
0)`: the vertical arrows have been End and Home in the albums, artists
and genres grids since the day this was written. At 700x700 with three
real rows of 3/3/2, ArrowDown from card 0 landed on card 7.

Two things behind it, both only visible once the grid splits:

`cover-grid`'s scrollToIndex was `querySelector('lit-virtualizer')` —
always `#grid-before` — while the roving index spans the whole album
list, so with a dropdown open End scrolled the wrong half to an index
it does not contain. It now picks the half that holds the index and
rebases it.

And the focus is retried on a deadline rather than taken once at the
host's `updateComplete`: a scroll of 5 000 rows produces the card a few
hundred ms later, so the tab stop moved and nothing took focus, which
looks exactly like the key not being handled.

Also waits for the virtualizer in album-dropdown.spec's expandCard,
which flaked on roughly one run in two on main.
This commit is contained in:
2026-08-12 17:44:22 -04:00
parent dddf54ba0c
commit 65c1b4fd53
4 changed files with 319 additions and 15 deletions
+81
View File
@@ -137,10 +137,82 @@ test.describe('the album dropdown', () => {
await app.setViewportSize({ width: 1440, height: 900 });
}
});
test('the arrow keys still move by a row across the split', async ({
app,
}) => {
// The dropdown draws two virtualizers where there was one, and the
// roving tab stop indexes the whole album list rather than either
// half. Home and End have to cross the dropdown, and ArrowDown has
// to move by a row rather than to the end — which it did not, in
// any of these grids, because `offsetTop` inside a virtualizer is
// always 0 and every rendered card counted as one row.
await app.setViewportSize({ width: 700, height: 700 });
try {
await expandCard(app, 1);
await expect.poll(() => dropdownState(app)).toMatchObject({
present: true,
split: true,
});
const moves = await app.evaluate(async () => {
const grid = document.querySelector('cover-grid');
const root = grid?.shadowRoot;
const container = root?.querySelector('.grid-scroll-container');
const cards = () =>
[...(root?.querySelectorAll<HTMLElement>('.album-card') ?? [])];
const at = () => {
const active = root?.activeElement as HTMLElement | null;
return active ? Number(active.dataset['index']) : null;
};
const press = async (key: string) => {
container?.dispatchEvent(
new KeyboardEvent('keydown', {
key,
bubbles: true,
composed: true,
}),
);
await new Promise((r) => setTimeout(r, 400));
return at();
};
cards()[0]?.focus();
const columns = cards().filter(
(c) =>
Math.round(c.getBoundingClientRect().top) ===
Math.round(cards()[0]!.getBoundingClientRect().top),
).length;
const down = await press('ArrowDown');
const end = await press('End');
const home = await press('Home');
return { columns, down, end, home, last: cards().length - 1 };
});
expect(moves.columns).toBeGreaterThan(1);
expect(moves.down).toBe(moves.columns);
expect(moves.end).toBe(moves.last);
expect(moves.home).toBe(0);
} finally {
await app.setViewportSize({ width: 1440, height: 900 });
}
});
});
/** Focus a card and press Enter, which is the only thing that expands one. */
async function expandCard(app: Page, index: number): Promise<void> {
// The cards come from a virtualizer, so they are not there when the
// view is: a keydown dispatched into an empty grid hits nothing, and
// what fails is the *poll* several lines later, which reads as the
// dropdown being broken rather than as a race. This spec flaked on
// roughly one run in two on main without this wait.
await expect.poll(() => cardCount(app)).toBeGreaterThan(index);
await app.evaluate((i) => {
const card = document
.querySelector('cover-grid')
@@ -157,6 +229,15 @@ async function expandCard(app: Page, index: number): Promise<void> {
}, index);
}
async function cardCount(app: Page): Promise<number> {
return app.evaluate(
() =>
document
.querySelector('cover-grid')
?.shadowRoot?.querySelectorAll('.album-card').length ?? 0,
);
}
async function closeDropdown(app: Page): Promise<void> {
await app.evaluate(() => {
const grid = document.querySelector('cover-grid') as
@@ -289,11 +289,7 @@ export class CoverGrid
private roving = new RovingGridController(this, {
cardSelector: '.album-card',
count: () => this.buildGridEntries().length,
scrollToIndex: (index) => {
this.shadowRoot
?.querySelector<LitVirtualizer>('lit-virtualizer')
?.scrollToIndex(index, 'nearest');
},
scrollToIndex: (index) => this.scrollGridToIndex(index),
});
private contextMenuTarget: ContextMenuTarget = {
@@ -849,6 +845,33 @@ export class CoverGrid
return entries;
}
/**
* Bring a card into view, in whichever virtualizer currently holds
* it.
*
* The roving tab stop's index is an index into the whole filtered
* album list, but a split grid draws that list across *two*
* virtualizers, each of which indexes only its own slice. This used
* to be `querySelector('lit-virtualizer')` — always `#grid-before`
* — so with a dropdown open, End scrolled the before-grid to an
* index it does not contain and the card that should have taken
* focus was never rendered to take it.
*/
private scrollGridToIndex(index: number): void {
const split = this.splitMode && this.expandedTracks.length > 0;
const after = split && index >= this.splitIndex;
const id = !split
? '#grid-single'
: after
? '#grid-after'
: '#grid-before';
this.shadowRoot
?.querySelector<LitVirtualizer>(id)
?.scrollToIndex(after ? index - this.splitIndex : index, 'nearest');
}
/** Rebuild before/after caches if the entries or splitIndex changed. */
private ensureSplitCache(): void {
const entries = this.buildGridEntries();
+67 -10
View File
@@ -12,9 +12,30 @@
* computed from the layout config, because all three grids are
* virtualized with a centring `justify` and the arithmetic would be a
* second description of a layout the DOM already knows.
*
* It has to be measured with `getBoundingClientRect()`, though, and
* not with `offsetTop`: `lit-virtualizer` positions its children with
* a `transform`, which `offsetTop` does not see, so **every** card in
* every one of these grids reported `offsetTop === 0`. That made the
* measured column count the number of rendered cards, which made
* ArrowDown `min(i + everything, last)` and ArrowUp `max(i - everything,
* 0)` — the vertical arrows were End and Home, in all three grids,
* from the day this was written. Reproduced at 700×700 with three real
* rows of 3/3/2: ArrowDown from card 0 landed on card 7.
*/
import type { ReactiveController, ReactiveControllerHost } from 'lit';
/**
* How long to keep retrying the focus while a virtualizer catches up.
*
* A deadline rather than a frame count because the wait is a scroll and
* a re-render, not a fixed number of paints: at 5 000 albums, End from
* the top produced the card in under 500 ms and a ten-frame budget
* (~160 ms) expired first — the index moved and nothing took focus,
* which is indistinguishable from the key not being handled at all.
*/
const focusRetryBudgetMs = 1000;
export interface RovingGridHost extends ReactiveControllerHost {
shadowRoot: ShadowRoot | null;
}
@@ -96,19 +117,45 @@ export class RovingGridController implements ReactiveController {
this.focus(next);
};
/** Move the tab stop, scrolling and focusing the card. */
/**
* Move the tab stop, scrolling and focusing the card.
*
* The focus is retried across a few frames because the host
* finishing its update is not the virtualizer finishing its own: a
* scroll of several thousand rows produces the card a frame or two
* later, and a single query at `updateComplete` finds nothing and
* silently leaves focus where it was. Measured at 5 000 albums with
* a dropdown open, where End moved the index and focused nothing.
*/
focus(index: number): void {
this.focusedIndex = index;
this.opts.scrollToIndex?.(index);
this.host.requestUpdate();
void this.host.updateComplete.then(() => {
const cards = this.cards();
const deadline = performance.now() + focusRetryBudgetMs;
cards
.find((card) => Number(card.dataset['index']) === index)
?.focus();
});
void this.host.updateComplete.then(() => this.focusCard(index, deadline));
}
private focusCard(index: number, deadline: number): void {
const card = this.cards().find(
(c) => Number(c.dataset['index']) === index,
);
if (card) {
card.focus();
return;
}
// Give up rather than spin: the index can also simply be gone,
// if a rescan shortened the list mid-keypress.
if (performance.now() >= deadline) return;
// Stop chasing an index the user has already moved away from.
if (this.focusedIndex !== index) return;
requestAnimationFrame(() => this.focusCard(index, deadline));
}
private cards(): HTMLElement[] {
@@ -119,14 +166,24 @@ export class RovingGridController implements ReactiveController {
];
}
/** Cards sharing a top offset are one row. */
/**
* Cards sharing a top edge are one row.
*
* Rounded because a virtualizer's transforms are fractional and two
* cards in the same row routinely differ in the third decimal, and
* measured against the *first* card's row because that row is the
* only one guaranteed to be full — a short last row would
* under-count the columns.
*/
private measureColumns(): number {
const cards = this.cards();
if (cards.length === 0) return 1;
const top = cards[0]!.offsetTop;
const inRow = cards.filter((card) => card.offsetTop === top).length;
const top = Math.round(cards[0]!.getBoundingClientRect().top);
const inRow = cards.filter(
(card) => Math.round(card.getBoundingClientRect().top) === top,
).length;
return Math.max(1, inRow);
}
+143
View File
@@ -0,0 +1,143 @@
/**
* The roving tab stop for the card grids, and specifically the one
* thing in it that was wrong from the day it was written: how it
* measures a row.
*
* `measureColumns` used `offsetTop`, and every card in these grids is
* produced by a `lit-virtualizer`, which positions its children with a
* `transform`. `offsetTop` does not see a transform — so all of them
* reported 0, every rendered card counted as one row, and ArrowDown
* became `min(i + everything, last)` while ArrowUp became `max(i -
* everything, 0)`. The vertical arrows were End and Home, in the
* albums, artists and genres grids alike.
*
* These cards are positioned the same way the virtualizer positions
* its own, which is the point: laid out with `top` the old code passes.
*/
import { describe, expect, it } from 'vitest';
import { RovingGridController } from '@utils/roving-grid';
const CARD = 100;
const ROW = 120;
/**
* A grid of `n` cards in rows of `columns`, positioned with a
* transform, inside a host that satisfies ReactiveControllerHost.
*/
function grid(n: number, columns: number) {
const el = document.createElement('div');
el.attachShadow({ mode: 'open' });
document.body.append(el);
for (let i = 0; i < n; i++) {
const card = document.createElement('div');
card.className = 'album-card';
card.dataset['index'] = String(i);
card.tabIndex = -1;
card.style.cssText = `position:absolute;width:${CARD}px;height:${CARD}px;transform:translate(${
(i % columns) * CARD
}px, ${Math.floor(i / columns) * ROW}px)`;
el.shadowRoot!.append(card);
}
const host = {
shadowRoot: el.shadowRoot,
addController: () => {},
removeController: () => {},
requestUpdate: () => {},
updateComplete: Promise.resolve(true),
};
const controller = new RovingGridController(host, {
cardSelector: '.album-card',
count: () => n,
});
const press = (key: string) => {
controller.handleKeydown(
new KeyboardEvent('keydown', { key, bubbles: true }),
);
};
/** Which card holds the tab stop. */
const stop = () => {
for (let i = 0; i < n; i++) {
if (controller.tabIndexFor(i) === 0) return i;
}
return -1;
};
return { press, stop, cleanup: () => el.remove() };
}
describe('roving grid: moving by a row', () => {
it('moves down by the number of columns, not by the whole grid', () => {
const g = grid(12, 4);
g.press('ArrowDown');
// 4, not 11. With offsetTop this was 11 — the last card.
expect(g.stop()).toBe(4);
g.cleanup();
});
it('moves back up by the same amount', () => {
const g = grid(12, 4);
g.press('ArrowDown');
g.press('ArrowDown');
g.press('ArrowUp');
expect(g.stop()).toBe(4);
g.cleanup();
});
it('clamps at the last card rather than overshooting', () => {
const g = grid(10, 4);
for (let i = 0; i < 5; i++) g.press('ArrowDown');
expect(g.stop()).toBe(9);
g.cleanup();
});
it('counts the columns of a single-row grid', () => {
const g = grid(3, 4);
g.press('ArrowDown');
expect(g.stop()).toBe(2);
g.cleanup();
});
it('still moves one card at a time horizontally', () => {
const g = grid(12, 4);
g.press('ArrowRight');
g.press('ArrowRight');
expect(g.stop()).toBe(2);
g.cleanup();
});
it('takes Home and End to the ends', () => {
const g = grid(12, 4);
g.press('End');
expect(g.stop()).toBe(11);
g.press('Home');
expect(g.stop()).toBe(0);
g.cleanup();
});
});