diff --git a/e2e/specs/album-dropdown.spec.ts b/e2e/specs/album-dropdown.spec.ts index 8a97ae4..324a194 100644 --- a/e2e/specs/album-dropdown.spec.ts +++ b/e2e/specs/album-dropdown.spec.ts @@ -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('.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 { + // 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 { }, index); } +async function cardCount(app: Page): Promise { + return app.evaluate( + () => + document + .querySelector('cover-grid') + ?.shadowRoot?.querySelectorAll('.album-card').length ?? 0, + ); +} + async function closeDropdown(app: Page): Promise { await app.evaluate(() => { const grid = document.querySelector('cover-grid') as diff --git a/frontend/src/components/cover-grid/cover-grid.ts b/frontend/src/components/cover-grid/cover-grid.ts index 2c8bcc8..5f8a843 100644 --- a/frontend/src/components/cover-grid/cover-grid.ts +++ b/frontend/src/components/cover-grid/cover-grid.ts @@ -289,11 +289,7 @@ export class CoverGrid private roving = new RovingGridController(this, { cardSelector: '.album-card', count: () => this.buildGridEntries().length, - scrollToIndex: (index) => { - this.shadowRoot - ?.querySelector('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(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(); diff --git a/frontend/src/utils/roving-grid.ts b/frontend/src/utils/roving-grid.ts index 47cdf95..5ffbf61 100644 --- a/frontend/src/utils/roving-grid.ts +++ b/frontend/src/utils/roving-grid.ts @@ -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); } diff --git a/frontend/test/utils/roving-grid.test.ts b/frontend/test/utils/roving-grid.test.ts new file mode 100644 index 0000000..218cb90 --- /dev/null +++ b/frontend/test/utils/roving-grid.test.ts @@ -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(); + }); +});