diff --git a/e2e/specs/queue-reorder.spec.ts b/e2e/specs/queue-reorder.spec.ts new file mode 100644 index 0000000..533c70d --- /dev/null +++ b/e2e/specs/queue-reorder.spec.ts @@ -0,0 +1,120 @@ +import { test, expect, callBinding } from '../support/fixtures.js'; +import type { Page } from '@playwright/test'; + +/** + * `a11y.11` — the queue's order can be changed without a mouse. + * + * The component tier pins the arithmetic against a faked binding. This + * one is here because the arithmetic is only half of it: `toIndex` is + * interpreted by `Queue.MoveQueueTracks`, whose contiguous-block guard + * turns the plausible-looking `i + 1` into a silent no-op. Nothing but + * the real backend can say whether the order actually moved. + * + * Reproduced first: with a row focused, Alt/Ctrl/Shift/Meta + arrows all + * left the order untouched. + */ + +/** The queue's order, asked of the backend rather than of the DOM. */ +async function order(app: Page): Promise { + const state = await callBinding<{ tracks: { title: string }[] }>( + app, + 'queue.Queue.GetState', + ); + + return state.tracks.map((t) => t.title); +} + +async function queueFourAndOpen(app: Page): Promise { + const paths: string[] = await app.evaluate(async () => { + const tracks = await window.__yjEvents.call( + 'library.Library.GetAllTracks', + [], + 10_000, + ); + + return (tracks as { FilePath: string }[]).slice(0, 4).map((t) => t.FilePath); + }); + + await callBinding(app, 'queue.Queue.SetQueue', [paths, 0, false]); + + // A closed panel renders no list at all, so there is no row to focus. + await app.locator('#queue-button').click(); + await expect(app.locator('queue-panel .track-item').first()).toBeVisible(); + + return order(app); +} + +test.describe('reordering the queue from the keyboard', () => { + // The 36 specs share one backend process in file order, and these + // leave two things behind that outlive the page: a reordered queue + // and an open panel. Both are put back, because a spec that spends + // state fails the *next* one, in a list that reads like a regression + // in whatever you are holding. + test.afterEach(async ({ app }) => { + await callBinding(app, 'queue.Queue.Clear').catch(() => { + /* nothing queued is the state we wanted anyway */ + }); + + const open = await app.locator('queue-panel[open]').count(); + + if (open > 0) await app.locator('#queue-button').click(); + }); + + test('Alt+Arrow moves the focused row, and puts it back', async ({ app }) => { + const start = await queueFourAndOpen(app); + + expect(start.length).toBe(4); + + await app.locator('queue-panel .track-item').nth(1).focus(); + await app.keyboard.press('Alt+ArrowUp'); + await expect.poll(() => order(app)).toEqual([start[1], start[0], ...start.slice(2)]); + + // Down is the direction the obvious index arithmetic gets wrong: it + // has to ask for i + 2, because i + 1 is a no-op once the row's own + // removal is accounted for. A spec that only moved up would pass + // against a build where down does nothing. + await app.keyboard.press('Alt+ArrowDown'); + await expect.poll(() => order(app)).toEqual(start); + }); + + test('says where the row went', async ({ app }) => { + await queueFourAndOpen(app); + + await app.locator('queue-panel .track-item').nth(1).focus(); + await app.keyboard.press('Alt+ArrowUp'); + + await expect( + app.locator('queue-panel [role="status"]'), + ).toHaveText(/Moved to position 1 of 4/); + }); + + test('refuses at the ends without reordering anything', async ({ app }) => { + const start = await queueFourAndOpen(app); + + await app.locator('queue-panel .track-item').first().focus(); + await app.keyboard.press('Alt+ArrowUp'); + + await expect( + app.locator('queue-panel [role="status"]'), + ).toHaveText(/Already first/); + expect(await order(app)).toEqual(start); + }); + + // The plain arrows belong to the roving tab stop, and must not reach + // the global volume binding from a focused row. + test('leaves the unmodified arrows roving', async ({ app }) => { + const start = await queueFourAndOpen(app); + + await app.locator('queue-panel .track-item').first().focus(); + await app.keyboard.press('ArrowDown'); + + const focused = await app.evaluate( + () => + document + .querySelector('queue-panel') + ?.shadowRoot?.activeElement?.getAttribute('data-index') ?? null, + ); + + expect([focused, await order(app)]).toEqual(['1', start]); + }); +}); diff --git a/frontend/src/components/queue-panel/queue-panel.ts b/frontend/src/components/queue-panel/queue-panel.ts index 31b94c8..f26e607 100644 --- a/frontend/src/components/queue-panel/queue-panel.ts +++ b/frontend/src/components/queue-panel/queue-panel.ts @@ -1,5 +1,6 @@ import { LitElement, html, svg, css, nothing, unsafeCSS } from 'lit'; import { designTokens } from '../../styles/tokens.css'; +import { srOnly } from '../../styles/sr-only.css'; import { customElement, property, @@ -162,6 +163,16 @@ export class QueuePanel /** The row holding the roving tab stop. */ @state() private focusedIndex = 0; + /** + * What the live region says about the last keyboard reorder. + * + * Empty until there has been one — the region itself renders + * unconditionally, because a screen reader announces a *change* to a + * region it is already watching and ignores one that appears with + * its text already in it. + */ + @state() private moveAnnouncement = ''; + private panelWidth = DEFAULT_WIDTH; private scrollbarDragging = false; @@ -221,7 +232,7 @@ export class QueuePanel return this.playlistSubmenuPopup; } - static override styles = [designTokens, contextMenuStyles, exploreLinkStyles, css` + static override styles = [designTokens, srOnly, contextMenuStyles, exploreLinkStyles, css` :host { flex-shrink: 0; width: 0; @@ -893,6 +904,19 @@ export class QueuePanel '.track-item', ); + // `focusedIndex` is the roving tab stop, and until now only the + // arrow keys moved it — so a row focused by a click or by Tab + // left it saying 0, and every key below acted on the wrong row. + // Enter played the first track in the queue from any focused + // row, which is a pre-existing bug that Alt+Arrow made visible + // by moving something. The key event knows which row it came + // from; use that. + const rowIndex = Number(row?.dataset.index ?? NaN); + + if (Number.isInteger(rowIndex) && rowIndex !== this.focusedIndex) { + this.focusedIndex = rowIndex; + } + if (isContextMenuKey(e) && row) { e.preventDefault(); e.stopPropagation(); @@ -911,6 +935,21 @@ export class QueuePanel return; } + // a11y.11: reordering the queue was drag-only, so its order + // could not be changed without a mouse at all. + // + // This has to come before `nextRovingIndex`, which switches on + // `e.key` and does not look at the modifiers — so Alt+ArrowUp + // already moved the roving focus, and would have gone on doing + // that *as well* as moving the row. + if (e.altKey && (e.key === 'ArrowUp' || e.key === 'ArrowDown')) { + e.preventDefault(); + e.stopPropagation(); + this.moveFocusedRow(e.key === 'ArrowUp' ? -1 : 1, count); + + return; + } + const next = nextRovingIndex(e.key, this.focusedIndex, count); if (next === null) return; @@ -927,6 +966,48 @@ export class QueuePanel ); }; + /** + * Move the focused row one position, and say where it went. + * + * It moves the *focused* row rather than the selection, which the + * drag path uses: the keyboard model already keeps those in step + * (every roving move re-selects the row it lands on), and "Alt+Down + * moved four rows you cannot see" is not a thing to do without an + * undo. + * + * The asymmetry in the target index is `MoveQueueTracks`'s, not + * ours. `toIndex` is an index into the array *before* the move, so + * moving down by one has to ask for `i + 2`: `i + 1` is where the + * row already is once you account for its own removal, and the + * backend's contiguous-block guard correctly treats it as a no-op. + */ + private moveFocusedRow(delta: -1 | 1, count: number): void { + const from = this.focusedIndex; + const to = from + delta; + + if (to < 0 || to >= count) { + this.moveAnnouncement = + delta < 0 + ? 'Already first in the queue' + : 'Already last in the queue'; + + return; + } + + this.queue.moveTracksInQueue([from], delta < 0 ? to : from + 2); + + this.focusedIndex = to; + this.selection.handleContextMenu(String(to)); + this.moveAnnouncement = `Moved to position ${to + 1} of ${count}`; + + void focusRovingRow( + this, + this.virtualizer, + to, + (i) => `.track-item[data-index="${i}"]`, + ); + } + private onContextMenuAction(action: string) { const indices = this.selection.getSelectedIndices(); @@ -1569,6 +1650,9 @@ export class QueuePanel : ''}" @mousedown=${this.handleMouseDown} > +
+ ${this.moveAnnouncement} +

Queue

diff --git a/frontend/test/components/queue-reorder.test.ts b/frontend/test/components/queue-reorder.test.ts new file mode 100644 index 0000000..e60d5f2 --- /dev/null +++ b/frontend/test/components/queue-reorder.test.ts @@ -0,0 +1,172 @@ +/** + * `a11y.11` — the queue's order can be changed without a mouse. + * + * Reproduced in the running app first: with a queue row focused, five + * plausible combinations (Alt/Ctrl/Shift/Meta + arrows) all left the + * order untouched, because reordering existed only as a drag whose drop + * index is computed from the cursor's Y position. + * + * The arithmetic is what these pin. `MoveQueueTracks`'s `toIndex` is an + * index into the array *before* the move, so up-by-one and down-by-one + * are not symmetric: up asks for `i - 1` and down has to ask for + * `i + 2`, because `i + 1` is where the row already is once its own + * removal is accounted for — and the backend's contiguous-block guard + * correctly treats that as a no-op. A fix written to look symmetric + * silently does nothing in one direction. + */ +import { describe, expect, it, beforeEach } from 'vitest'; + +import '@components/queue-panel/queue-panel'; +import type { QueuePanel } from '@components/queue-panel/queue-panel'; +import { Events } from '../../src/events'; +import { emit, calls, flush, lastArgs } from '@test/support/harness'; +import { fixture, shadow, shadowAll } from '@test/support/render'; +import type { QueueTrack } from '@store/queue-store'; + +function queueTrack(n: number, title: string): QueueTrack { + return { + id: n, + audioFileId: n, + filePath: `/music/${n}.mp3`, + position: n, + title, + artist: 'Artist', + album: 'Album', + coverArtPath: '', + artistMbid: '', + releaseGroupMbid: '', + recordingMbid: '', + }; +} + +const TRACKS = ['First', 'Second', 'Third', 'Fourth'].map((t, i) => + queueTrack(i + 1, t), +); + +type Panel = QueuePanel; + +async function panelWithQueue(): Promise { + const el = await fixture('queue-panel', { open: true }); + + emit(Events.QueueChanged, { + tracks: TRACKS, + currentIndex: 0, + shuffleMode: false, + repeatMode: 'off', + sourcePlaylistId: 0, + }); + await flush(); + await el.updateComplete; + await new Promise((r) => { + requestAnimationFrame(() => r(null)); + }); + + return el; +} + +/** + * Press a key *from a row*, the way the delegated handler receives it. + * + * The index comes off the event's own row rather than from the + * component's `focusedIndex`, which is the fix for a pre-existing bug: + * only the arrow keys used to move that field, so a row reached by a + * click or by Tab left it saying 0 and every key acted on the wrong row. + */ +function pressFrom(el: Panel, index: number, key: string, alt: boolean) { + const row = shadowAll(el, `.track-item[data-index="${index}"]`)[0]; + + row?.dispatchEvent( + new KeyboardEvent('keydown', { key, altKey: alt, bubbles: true }), + ); +} + +const live = (el: Panel) => + shadow(el, '[role="status"]')?.textContent?.trim() ?? ''; + +describe(' keyboard reorder', () => { + beforeEach(() => { + emit(Events.QueueChanged, { + tracks: [], + currentIndex: -1, + shuffleMode: false, + repeatMode: 'off', + sourcePlaylistId: 0, + }); + }); + + it('moves a row up by one', async () => { + const el = await panelWithQueue(); + + pressFrom(el, 2, 'ArrowUp', true); + await el.updateComplete; + + expect(lastArgs('queue.Queue.MoveQueueTracks')).toEqual([[2], 1]); + }); + + // The asymmetry, pinned. `[[1], 2]` would be the symmetric-looking + // version and is precisely the no-op the backend guards against. + it('moves a row down by one, past its own removal', async () => { + const el = await panelWithQueue(); + + pressFrom(el, 1, 'ArrowDown', true); + await el.updateComplete; + + expect(lastArgs('queue.Queue.MoveQueueTracks')).toEqual([[1], 3]); + }); + + it('acts on the row the key came from, not the last one arrowed to', async () => { + const el = await panelWithQueue(); + + pressFrom(el, 3, 'ArrowUp', true); + await el.updateComplete; + + expect(lastArgs('queue.Queue.MoveQueueTracks')).toEqual([[3], 2]); + }); + + it('says where the row went', async () => { + const el = await panelWithQueue(); + + pressFrom(el, 2, 'ArrowUp', true); + await el.updateComplete; + + expect(live(el)).toBe('Moved to position 2 of 4'); + }); + + it('refuses at the ends, and says so rather than silently doing nothing', async () => { + const el = await panelWithQueue(); + + pressFrom(el, 0, 'ArrowUp', true); + await el.updateComplete; + const top = live(el); + + pressFrom(el, 3, 'ArrowDown', true); + await el.updateComplete; + + expect([top, live(el), calls().some((c) => c.path.includes('Move'))]).toEqual( + ['Already first in the queue', 'Already last in the queue', false], + ); + }); + + // The live region has to be in the DOM before it has anything to say: + // most screen readers announce a change to a region they are already + // watching and ignore one that appears with its content already in it. + it('has the live region mounted and empty before any move', async () => { + const el = await panelWithQueue(); + + expect([shadow(el, '[role="status"]') !== null, live(el)]).toEqual([ + true, + '', + ]); + }); + + // Without the modifier the same keys must still rove, and must not + // reach the global volume binding. + it('leaves the plain arrows as a roving move', async () => { + const el = await panelWithQueue(); + + pressFrom(el, 0, 'ArrowDown', false); + await el.updateComplete; + + expect(calls().some((c) => c.path.includes('Move'))).toBe(false); + }); +});