From 1aa1598ecbd1574ad510d078f8674523289146b8 Mon Sep 17 00:00:00 2001 From: Logan Date: Wed, 12 Aug 2026 12:33:50 -0400 Subject: [PATCH] feat(shortcuts): tell the key story once, and give the arrows back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Decision 1 keeps the unmodified single-key bindings, and Settings was the only place they were written down — three of the four categories of them, because config-page listed the categories by hand, so the autotag keys were written down nowhere at all. `?` now opens an overlay from anywhere the app owns the keyboard, and both surfaces read one table (services/shortcut-meta.ts, moved out of config-page's private static). The other half is the same explanation from the other side. Phase 1 gave the arrow keys to the grid, correctly — but all six of them, and no list in this app moves horizontally: track-list's own handler and utils/roving-rows both take Up/Down/Home/End and ignore Left/Right. So seeking stopped working from a focused row and nothing gained the keys. Reproduced in the running app: two ArrowRights on a focused track row, zero Player.Seek calls, against one per press from the body. A shifted character no longer reports Shift, so the binding is `?` and not `Shift+?` — the character already carries the shift, and a layout where it does not is a layout where "Shift+?" is wrong anyway. --- backend/shortcuts/config.go | 3 +- e2e/specs/key-story.spec.ts | 131 +++++++++ e2e/specs/player-truth.spec.ts | 9 +- frontend/index.html | 1 + frontend/index.ts | 4 + .../src/components/config-page/config-page.ts | 164 +----------- .../shortcuts-overlay/shortcuts-overlay.ts | 249 ++++++++++++++++++ .../src/services/keyboard-shortcut-service.ts | 41 ++- frontend/src/services/shortcut-meta.ts | 166 ++++++++++++ .../test/components/shortcuts-overlay.test.ts | 81 ++++++ .../test/stores/keyboard-shortcuts.test.ts | 45 ++++ 11 files changed, 730 insertions(+), 164 deletions(-) create mode 100644 e2e/specs/key-story.spec.ts create mode 100644 frontend/src/components/shortcuts-overlay/shortcuts-overlay.ts create mode 100644 frontend/src/services/shortcut-meta.ts create mode 100644 frontend/test/components/shortcuts-overlay.test.ts diff --git a/backend/shortcuts/config.go b/backend/shortcuts/config.go index 6981beb..cf8521b 100644 --- a/backend/shortcuts/config.go +++ b/backend/shortcuts/config.go @@ -29,8 +29,9 @@ func DefaultBindings() map[string]string { "nav.searchAlt": "Ctrl+F", "nav.queue": "Q", - // App actions (Global scope, Ctrl modifier) + // App actions "app.selectAll": "Ctrl+A", + "app.shortcuts": "?", // Panel-specific (track list). There is no `tracklist.delete`: // it was bound to Delete and advertised in Settings as diff --git a/e2e/specs/key-story.spec.ts b/e2e/specs/key-story.spec.ts new file mode 100644 index 0000000..a467952 --- /dev/null +++ b/e2e/specs/key-story.spec.ts @@ -0,0 +1,131 @@ +import { + test, + expect, + callBinding, + resetEvents, + waitForEvent, + LONG_TRACK, +} from '../support/fixtures.js'; +import type { Page } from '@playwright/test'; + +/** + * Plan 007 phase 5: the key story, told once. + * + * Decision 1 keeps the unmodified single-key bindings, and until now + * Settings was the only place they were written down — three of the + * four categories of them, so the autotag keys were written down + * nowhere. `?` opens the overlay from anywhere the app owns the + * keyboard. + * + * The other half is the same explanation from the other side: Phase 1 + * gave the arrow keys to the grid, correctly, but all six of them — + * and no list in this app moves horizontally, so seeking stopped + * working from a focused row and nothing gained the keys. Reproduced + * in the running app before the fix: two ArrowRights on a focused + * track row produced zero `Player.Seek` calls, against one per press + * with focus on the body. + */ +test.describe('the shortcuts overlay', () => { + // Two things about locating a `wa-dialog`, both found here: + // + // - the host is `display: contents`, so the element carrying the + // testid always reports hidden; what is visible is the native + // `` inside its shadow root, and + // - that dialog has **no accessible name**. Web Awesome renders the + // `label` into an `

` in the same shadow root and + // never points `aria-labelledby` at it, so every dialog in this + // app is an unnamed dialog to a screen reader. Not fixed here: + // it is eight call sites and a helper, and it is worth doing on + // purpose rather than as a side effect of a spec. + const overlay = (app: Page) => + app.locator('shortcuts-overlay').getByRole('dialog'); + + test('? opens it, and it names the keys', async ({ app }) => { + await app.keyboard.press('?'); + + await expect(overlay(app)).toBeVisible(); + // The rows are slotted, so they are in the overlay's own shadow + // root rather than inside the native ``. + const content = app.locator('shortcuts-overlay'); + + await expect(content).toContainText('Play / Pause'); + // Autotag's bindings are in the table Settings renders three + // quarters of. + await expect(content).toContainText('Apply Match'); + }); + + test('Escape closes it', async ({ app }) => { + await app.keyboard.press('?'); + await expect(overlay(app)).toBeVisible(); + + // Not a toggle: a dialog owns every unmodified key while it is up, + // so a second `?` never reaches the shortcut service. Escape is + // what closes a dialog here, and `wa-dialog` brings it. + await app.keyboard.press('Escape'); + await expect(overlay(app)).toBeHidden(); + }); +}); + +test.describe('seeking from a focused track row', () => { + test('Left and Right seek; Up and Down still move the row', async ({ + app, + }) => { + await app.getByTestId('nav-tracks').click(); + await callBinding(app, 'queue.Queue.Clear'); + await resetEvents(app); + + await app + .getByTestId('track-row') + .filter({ hasText: LONG_TRACK }) + .first() + .dblclick(); + await waitForEvent(app, 'TrackChanged'); + + // Focus the roving row, which is what took the arrows. + await app.evaluate(() => { + const list = document.querySelector('track-list'); + const row = list?.shadowRoot?.querySelector( + '[role="row"][tabindex="0"]', + ); + + row?.focus(); + }); + + const position = async (): Promise => + (await callBinding(app, 'player.Player.CurrentPositionSeconds')) as number; + + const before = await position(); + + for (let i = 0; i < 3; i++) await app.keyboard.press('ArrowRight'); + + // Three 5 s steps, against a clock that advances 1 s per second: + // the assertion is a jump the passage of time cannot account for. + await expect.poll(position).toBeGreaterThan(before + 8); + + // …and the list still moves vertically, which is what the arrows + // were given to the grid for. + const focusedBefore = await app.evaluate( + () => + ( + document.querySelector('track-list') as unknown as { + focusedIndex: number; + } + ).focusedIndex, + ); + + await app.keyboard.press('ArrowDown'); + + await expect + .poll(() => + app.evaluate( + () => + ( + document.querySelector('track-list') as unknown as { + focusedIndex: number; + } + ).focusedIndex, + ), + ) + .toBe(focusedBefore + 1); + }); +}); diff --git a/e2e/specs/player-truth.spec.ts b/e2e/specs/player-truth.spec.ts index 8abffd4..69b728d 100644 --- a/e2e/specs/player-truth.spec.ts +++ b/e2e/specs/player-truth.spec.ts @@ -77,9 +77,12 @@ async function readClocks( * Move focus off the track row. * * Phase 1 made rows a real grid with roving tabindex, and global - * single-key bindings yield to a focused control that owns the key — - * so with a row focused the arrows navigate the list rather than - * seeking, which is correct and is not what this spec is about. + * single-key bindings yield to a focused control that owns the key. + * A row owns the *vertical* arrows only now (Phase 5), so Left/Right + * seek from a focused row too and this is no longer load-bearing — it + * stays because this spec is about the clock, not about scope + * resolution, and it should keep measuring the same thing if that + * rule changes again. */ async function blurDeepActive(app: Page): Promise { await app.evaluate(() => { diff --git a/frontend/index.html b/frontend/index.html index fc6b2a4..c452149 100644 --- a/frontend/index.html +++ b/frontend/index.html @@ -37,6 +37,7 @@ + diff --git a/frontend/index.ts b/frontend/index.ts index 1f5c4ca..e046550 100644 --- a/frontend/index.ts +++ b/frontend/index.ts @@ -28,6 +28,10 @@ import '@components/first-run-wizard/first-run-wizard.ts'; import '@components/notifications/notification-host.ts'; import '@components/notifications/inline-notice.ts'; import '@components/confirm-dialog/confirm-dialog.ts'; +// The `?` overlay: help, so it is eager for the same reason the failure +// surface is — the moment it is asked for is the moment the user does +// not know what is going on. It costs a dialog and a table. +import '@components/shortcuts-overlay/shortcuts-overlay.ts'; import '@components/jobs/job-indicator.ts'; import '@awesome.me/webawesome/dist/styles/themes/default.css'; import '@awesome.me/webawesome/dist/components/icon/icon.js'; diff --git a/frontend/src/components/config-page/config-page.ts b/frontend/src/components/config-page/config-page.ts index 41ea8b3..00f1e4e 100644 --- a/frontend/src/components/config-page/config-page.ts +++ b/frontend/src/components/config-page/config-page.ts @@ -25,6 +25,10 @@ import { FavoritesController } from '@store/controllers/favorites-controller'; import { GetAllPlaylists } from '@go/playlist/Service'; import type { playlist } from '@go/models'; import { Events } from '../../events'; +import { + SHORTCUT_CATEGORIES, + SHORTCUT_META, +} from '../../services/shortcut-meta'; import { ViewLifecycleMixin } from '../../utils/view-lifecycle'; import type { ConfigFieldChangeEvent } from './config-field'; import type { BackgroundShade } from '@store/theme-store'; @@ -63,150 +67,6 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) { // --- Shortcuts controller --- private shortcutsCtrl = new ShortcutsController(this); - // --- Shortcut metadata for UI grouping --- - private static readonly SHORTCUT_META: Record< - string, - { - label: string; - category: string; - scope: string; - defaultKey: string; - } - > = { - 'player.playPause': { - label: 'Play / Pause', - category: 'Player', - scope: 'global', - defaultKey: 'Space', - }, - 'player.next': { - label: 'Next Track', - category: 'Player', - scope: 'global', - defaultKey: 'N', - }, - 'player.previous': { - label: 'Previous Track', - category: 'Player', - scope: 'global', - defaultKey: 'P', - }, - 'player.volumeUp': { - label: 'Volume Up', - category: 'Player', - scope: 'global', - defaultKey: 'Up', - }, - 'player.volumeDown': { - label: 'Volume Down', - category: 'Player', - scope: 'global', - defaultKey: 'Down', - }, - 'player.seekForward': { - label: 'Seek Forward', - category: 'Player', - scope: 'global', - defaultKey: 'Right', - }, - 'player.seekBack': { - label: 'Seek Back', - category: 'Player', - scope: 'global', - defaultKey: 'Left', - }, - 'player.shuffle': { - label: 'Toggle Shuffle', - category: 'Player', - scope: 'global', - defaultKey: 'S', - }, - 'player.repeat': { - label: 'Cycle Repeat', - category: 'Player', - scope: 'global', - defaultKey: 'R', - }, - 'player.mute': { - label: 'Toggle Mute', - category: 'Player', - scope: 'global', - defaultKey: 'M', - }, - 'nav.search': { - label: 'Focus Search', - category: 'Navigation', - scope: 'global', - defaultKey: '/', - }, - 'nav.searchAlt': { - label: 'Focus Search (Alt)', - category: 'Navigation', - scope: 'global', - defaultKey: 'Ctrl+F', - }, - 'nav.queue': { - label: 'Toggle Queue', - category: 'Navigation', - scope: 'global', - defaultKey: 'Q', - }, - 'app.selectAll': { - label: 'Select All', - category: 'App', - scope: 'global', - defaultKey: 'Ctrl+A', - }, - 'tracklist.play': { - label: 'Play Selected', - category: 'Navigation', - scope: 'panel:track-list', - defaultKey: 'Enter', - }, - 'autotag.apply': { - label: 'Apply Match', - category: 'Autotag', - scope: 'panel:autotag', - defaultKey: 'A', - }, - 'autotag.skip': { - label: 'Skip Folder', - category: 'Autotag', - scope: 'panel:autotag', - defaultKey: 'S', - }, - 'autotag.leave': { - label: 'Leave As Is', - category: 'Autotag', - scope: 'panel:autotag', - defaultKey: 'L', - }, - 'autotag.paste': { - label: 'Paste Release URL', - category: 'Autotag', - scope: 'panel:autotag', - defaultKey: 'U', - }, - 'autotag.search': { - label: 'Search Candidates', - category: 'Autotag', - scope: 'panel:autotag', - defaultKey: 'F', - }, - 'autotag.next': { - label: 'Next Folder', - category: 'Autotag', - scope: 'panel:autotag', - defaultKey: 'Down', - }, - 'autotag.previous': { - label: 'Previous Folder', - category: 'Autotag', - scope: 'panel:autotag', - defaultKey: 'Up', - }, - }; - // --- Now Playing state --- @state() private scrollMode = 'hover'; @@ -1338,7 +1198,7 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) { const { action, key } = e.detail; // Check for conflict — find any other action with the same key in the same or overlapping scope - const meta = ConfigPage.SHORTCUT_META[action]; + const meta = SHORTCUT_META[action]; const conflict = shortcutsStore.findConflict( key, meta?.scope ?? 'global', @@ -1901,11 +1761,10 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) { private renderShortcutsSection() { const bindings = this.shortcutsCtrl.state.bindings; - const categories = [ - 'Player', - 'Navigation', - 'App', - ]; + // All four, from the shared table: the Autotag bindings are + // persisted and rebindable like any other, and listing three of + // four categories is how they came to be written down nowhere. + const categories = SHORTCUT_CATEGORIES; return html` ${categories.map((cat) => { const actions = Object.entries( - ConfigPage.SHORTCUT_META, + SHORTCUT_META, ).filter( ([, meta]) => meta.category === cat, @@ -1990,8 +1849,7 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) { > is already bound to ${ConfigPage - .SHORTCUT_META[ + >${SHORTCUT_META[ this .shortcutConflict .existingAction diff --git a/frontend/src/components/shortcuts-overlay/shortcuts-overlay.ts b/frontend/src/components/shortcuts-overlay/shortcuts-overlay.ts new file mode 100644 index 0000000..573f82e --- /dev/null +++ b/frontend/src/components/shortcuts-overlay/shortcuts-overlay.ts @@ -0,0 +1,249 @@ +/** + * The key story, told once. + * + * The single-key global bindings are staying (plan 007, Decision 1), + * and until now Settings was the only place they were written down — + * three of the four categories of them, at that, so the autotag keys + * were written down nowhere. `?` opens this from anywhere the app owns + * the keyboard. + * + * It reads `services/shortcut-meta` for the labels and the *store* for + * the keys, so a rebound key shows its new binding rather than the + * default it shipped with. + * + * It is a `wa-dialog` for the reason every dialog in this app is: the + * focus trap, Escape and focus restore come with it. Being help rather + * than a question, it is a dialog in a host template rather than a + * `confirmAction()` call. + */ +import { LitElement, css, html, nothing } from 'lit'; +import { customElement, query, state } from 'lit/decorators.js'; +import '@awesome.me/webawesome/dist/components/dialog/dialog.js'; + +import { designTokens } from '../../styles/tokens.css'; +import { + SHORTCUT_CATEGORIES, + SHORTCUT_META, +} from '../../services/shortcut-meta'; +import { shortcutsStore } from '@store/shortcuts-store'; + +/** How a key string reads to a person: `Ctrl+F` is two keys. */ +function keyParts(key: string): string[] { + return key.split('+'); +} + +/** Where a binding applies, in the user's terms. A panel binding that + * does not say so reads as broken everywhere else. */ +function scopeNote(scope: string): string { + if (scope === 'panel:track-list') return 'in the track list'; + if (scope === 'panel:autotag') return 'on the Autotag page'; + + return ''; +} + +@customElement('shortcuts-overlay') +export class ShortcutsOverlay extends LitElement { + @query('wa-dialog') private dialog?: HTMLElement & { open: boolean }; + + @state() private isOpen = false; + + private unsubscribe: (() => void) | null = null; + + static override styles = [ + designTokens, + css` + :host { + display: contents; + } + + wa-dialog::part(dialog) { + background: var(--yj-bg-surface, #212529); + color: var(--yj-text-primary, #fff); + } + + .category { + margin-bottom: 1.25em; + } + + .category:last-of-type { + margin-bottom: 0; + } + + h3 { + margin: 0 0 0.5em; + font-size: var(--yj-text-sm, 0.8125rem); + font-weight: 700; + text-transform: uppercase; + letter-spacing: 0.06em; + color: var(--yj-text-secondary, #b3b3b3); + } + + .row { + display: flex; + align-items: baseline; + justify-content: space-between; + gap: 1em; + padding: 0.25em 0; + } + + .label { + font-size: var(--yj-text-sm, 0.8125rem); + } + + .scope { + color: var(--yj-text-tertiary, #888); + } + + .keys { + display: flex; + gap: 0.25em; + flex-shrink: 0; + } + + kbd { + background: var(--yj-bg-elevated, #343a40); + border: 1px solid var(--yj-border, #495057); + border-radius: 4px; + padding: 0.1em 0.45em; + font-family: inherit; + font-size: var(--yj-text-xs, 0.6875rem); + } + + .footnote { + margin: 1em 0 0; + font-size: var(--yj-text-xs, 0.6875rem); + color: var(--yj-text-tertiary, #888); + } + `, + ]; + + override connectedCallback(): void { + super.connectedCallback(); + document.addEventListener('shortcut:app-shortcuts', this.open); + // The keys come from the backend, so the first open can arrive + // before they do. + this.unsubscribe = shortcutsStore.subscribe(() => + this.requestUpdate(), + ); + } + + override disconnectedCallback(): void { + super.disconnectedCallback(); + document.removeEventListener('shortcut:app-shortcuts', this.open); + this.unsubscribe?.(); + this.unsubscribe = null; + } + + /** + * `?` opens; Escape closes, as it does for every dialog here. + * + * It is deliberately not a toggle. A dialog owns the whole keyboard + * while it is up — `focusedControlOwnsKey` yields every unmodified + * key to anything inside one — so a second `?` never reaches the + * service, and a toggle would be a promise the shortcut layer + * cannot keep. Written after watching an e2e spec assert it and + * fail. + */ + private open = (): void => { + if (this.isOpen) return; + + this.isOpen = true; + void this.updateComplete.then(() => { + if (this.dialog) this.dialog.open = true; + }); + }; + + private close(): void { + if (this.dialog) this.dialog.open = false; + this.isOpen = false; + } + + override render() { + if (!this.isOpen) return nothing; + + const bindings = shortcutsStore.getBindings(); + + return html` + this.close()} + > + ${SHORTCUT_CATEGORIES.map((category) => { + const rows = Object.entries(SHORTCUT_META).filter( + ([, meta]) => meta.category === category, + ); + + if (rows.length === 0) return nothing; + + // A note every row in the category repeats is a + // note about the category. "— on the Autotag page" + // seven times is noise; once is the heading. + const first = scopeNote(rows[0]![1].scope); + const uniform = rows.every( + ([, meta]) => scopeNote(meta.scope) === first, + ); + // …and a note the *heading* already says is not a + // note either: "Autotag — on the Autotag page". + const shared = + uniform && + !first + .toLowerCase() + .includes(category.toLowerCase()) + ? first + : ''; + + return html` +
+

+ ${category} + ${shared + ? html`— ${shared}` + : nothing} +

+ ${rows.map(([action, meta]) => { + const key = + bindings.get(action) ?? meta.defaultKey; + const note = + shared || uniform + ? '' + : scopeNote(meta.scope); + + return html` +
+ + ${meta.label} + ${note + ? html`— ${note}` + : nothing} + + + ${keyParts(key).map( + (part) => + html`${part}`, + )} + +
+ `; + })} +
+ `; + })} +

+ Every one of these can be rebound in Settings → + Keyboard Shortcuts. +

+ + `; + } +} + +declare global { + interface HTMLElementTagNameMap { + 'shortcuts-overlay': ShortcutsOverlay; + } +} diff --git a/frontend/src/services/keyboard-shortcut-service.ts b/frontend/src/services/keyboard-shortcut-service.ts index 63f85dd..035c20a 100644 --- a/frontend/src/services/keyboard-shortcut-service.ts +++ b/frontend/src/services/keyboard-shortcut-service.ts @@ -30,6 +30,14 @@ const KEY_ALIASES: Record = { ' ': 'Space', }; +/** Whether a resolved key is a printable character that a Shift press + * produced, rather than a key Shift was held alongside. Letters are + * excluded: `A` and `Shift+A` are the same character, so Shift stays + * meaningful there. */ +function isShiftedCharacter(key: string): boolean { + return key.length === 1 && !/[A-Z0-9]/.test(key); +} + /** Keys that are modifier-only presses and should be ignored. */ const MODIFIER_KEYS = new Set([ 'Control', @@ -52,11 +60,6 @@ export function buildKeyString(e: KeyboardEvent): string { const parts: string[] = []; - // Modifiers in fixed order. Treat Meta (Cmd on Mac) as Ctrl. - if (e.ctrlKey || e.metaKey) parts.push('Ctrl'); - if (e.altKey) parts.push('Alt'); - if (e.shiftKey) parts.push('Shift'); - // Normalize the key name. let key = KEY_ALIASES[e.key] ?? e.key; @@ -65,6 +68,16 @@ export function buildKeyString(e: KeyboardEvent): string { key = key.toUpperCase(); } + // Modifiers in fixed order. Treat Meta (Cmd on Mac) as Ctrl. + if (e.ctrlKey || e.metaKey) parts.push('Ctrl'); + if (e.altKey) parts.push('Alt'); + + // Shift is only a modifier when it did not *produce* the key. + // `?` is Shift+/ on a US layout and something else elsewhere, and + // "Shift+?" is a binding nobody would write down; the character + // already carries the shift. + if (e.shiftKey && !isShiftedCharacter(key)) parts.push('Shift'); + parts.push(key); return parts.join('+'); @@ -143,6 +156,7 @@ type ShortcutScope = */ const ACTIVATION_KEYS = new Set(['Space', 'Enter']); const ARROW_KEYS = new Set(['Up', 'Down', 'Left', 'Right', 'Home', 'End']); +const VERTICAL_KEYS = new Set(['Up', 'Down', 'Home', 'End']); const LIST_KEYS = new Set([...ARROW_KEYS, ...ACTIVATION_KEYS]); const SLIDER_KEYS = new Set([...ARROW_KEYS, 'PageUp', 'PageDown']); @@ -160,7 +174,14 @@ function keysOwnedBy(el: Element): ReadonlySet | null { } // A grid row or a listbox option moves with the arrow keys, which is - // what makes a track list navigable without a mouse. + // what makes a track list navigable without a mouse — but only the + // *vertical* ones. Every list in this app (`track-list`'s own + // handler, `utils/roving-rows.ts`, the card grids that use it) moves + // on Up/Down/Home/End and does nothing with Left/Right, so granting + // those took keyboard seeking away from a focused row and gave it to + // nobody: two ArrowRights on a focused track row produced zero + // `Player.Seek` calls, against one per press with focus on the body. + // Grant them back here if a list ever moves horizontally. if ( role === 'row' || role === 'gridcell' || @@ -168,7 +189,7 @@ function keysOwnedBy(el: Element): ReadonlySet | null { role === 'option' || role === 'treeitem' ) { - return ARROW_KEYS; + return VERTICAL_KEYS; } if (tag === 'INPUT') { @@ -398,6 +419,12 @@ async function dispatch(action: string): Promise { } // App actions + case 'app.shortcuts': + document.dispatchEvent( + new CustomEvent('shortcut:app-shortcuts'), + ); + break; + case 'app.selectAll': document.dispatchEvent( new CustomEvent('shortcut:select-all'), diff --git a/frontend/src/services/shortcut-meta.ts b/frontend/src/services/shortcut-meta.ts new file mode 100644 index 0000000..1ea3b59 --- /dev/null +++ b/frontend/src/services/shortcut-meta.ts @@ -0,0 +1,166 @@ +/** + * What each shortcut action is called, where it applies, and what it is + * bound to out of the box. + * + * This lived as a private static in `config-page`, which meant Settings + * was the only place in the app the key bindings were written down — + * and Settings renders three of the four categories, so the autotag + * keys were written down nowhere at all. The `?` overlay and the + * Settings editor now read one table. + */ + +export interface ShortcutMeta { + label: string; + category: string; + scope: string; + defaultKey: string; +} + +/** The categories, in the order both surfaces show them. */ +export const SHORTCUT_CATEGORIES = [ + 'Player', + 'Navigation', + 'App', + 'Autotag', +] as const; + +export const SHORTCUT_META: Record = { + 'player.playPause': { + label: 'Play / Pause', + category: 'Player', + scope: 'global', + defaultKey: 'Space', + }, + 'player.next': { + label: 'Next Track', + category: 'Player', + scope: 'global', + defaultKey: 'N', + }, + 'player.previous': { + label: 'Previous Track', + category: 'Player', + scope: 'global', + defaultKey: 'P', + }, + 'player.volumeUp': { + label: 'Volume Up', + category: 'Player', + scope: 'global', + defaultKey: 'Up', + }, + 'player.volumeDown': { + label: 'Volume Down', + category: 'Player', + scope: 'global', + defaultKey: 'Down', + }, + 'player.seekForward': { + label: 'Seek Forward', + category: 'Player', + scope: 'global', + defaultKey: 'Right', + }, + 'player.seekBack': { + label: 'Seek Back', + category: 'Player', + scope: 'global', + defaultKey: 'Left', + }, + 'player.shuffle': { + label: 'Toggle Shuffle', + category: 'Player', + scope: 'global', + defaultKey: 'S', + }, + 'player.repeat': { + label: 'Cycle Repeat', + category: 'Player', + scope: 'global', + defaultKey: 'R', + }, + 'player.mute': { + label: 'Toggle Mute', + category: 'Player', + scope: 'global', + defaultKey: 'M', + }, + 'nav.search': { + label: 'Focus Search', + category: 'Navigation', + scope: 'global', + defaultKey: '/', + }, + 'nav.searchAlt': { + label: 'Focus Search (Alt)', + category: 'Navigation', + scope: 'global', + defaultKey: 'Ctrl+F', + }, + 'nav.queue': { + label: 'Toggle Queue', + category: 'Navigation', + scope: 'global', + defaultKey: 'Q', + }, + 'app.shortcuts': { + label: 'Keyboard Shortcuts', + category: 'App', + scope: 'global', + defaultKey: '?', + }, + 'app.selectAll': { + label: 'Select All', + category: 'App', + scope: 'global', + defaultKey: 'Ctrl+A', + }, + 'tracklist.play': { + label: 'Play Selected', + category: 'Navigation', + scope: 'panel:track-list', + defaultKey: 'Enter', + }, + 'autotag.apply': { + label: 'Apply Match', + category: 'Autotag', + scope: 'panel:autotag', + defaultKey: 'A', + }, + 'autotag.skip': { + label: 'Skip Folder', + category: 'Autotag', + scope: 'panel:autotag', + defaultKey: 'S', + }, + 'autotag.leave': { + label: 'Leave As Is', + category: 'Autotag', + scope: 'panel:autotag', + defaultKey: 'L', + }, + 'autotag.paste': { + label: 'Paste Release URL', + category: 'Autotag', + scope: 'panel:autotag', + defaultKey: 'U', + }, + 'autotag.search': { + label: 'Search Candidates', + category: 'Autotag', + scope: 'panel:autotag', + defaultKey: 'F', + }, + 'autotag.next': { + label: 'Next Folder', + category: 'Autotag', + scope: 'panel:autotag', + defaultKey: 'Down', + }, + 'autotag.previous': { + label: 'Previous Folder', + category: 'Autotag', + scope: 'panel:autotag', + defaultKey: 'Up', + }, + }; diff --git a/frontend/test/components/shortcuts-overlay.test.ts b/frontend/test/components/shortcuts-overlay.test.ts new file mode 100644 index 0000000..cac6d2b --- /dev/null +++ b/frontend/test/components/shortcuts-overlay.test.ts @@ -0,0 +1,81 @@ +/** + * The key story, told once. + * + * Decision 1 keeps the unmodified single-key bindings, and Settings was + * the only place they were written down — three of the four categories + * of them, so the autotag keys were written down nowhere at all. + */ +import { describe, expect, it } from 'vitest'; + +import '@components/shortcuts-overlay/shortcuts-overlay'; +import { emit } from '@test/support/harness'; +import { Events } from '../../src/events'; +import { fixture, shadow, shadowAll } from '@test/support/render'; + +/** Open it the way the shortcut service does. */ +async function openOverlay(el: HTMLElement): Promise { + document.dispatchEvent(new CustomEvent('shortcut:app-shortcuts')); + await (el as HTMLElement & { updateComplete: Promise }) + .updateComplete; +} + +describe('', () => { + it('renders nothing until it is asked for', async () => { + const el = await fixture('shortcuts-overlay'); + + expect(shadow(el, 'wa-dialog')).toBeNull(); + }); + + it('opens on the shortcut event, and stays open on a second one', async () => { + const el = await fixture('shortcuts-overlay'); + + await openOverlay(el); + expect(shadow(el, '[data-testid="shortcuts-overlay"]')).toBeTruthy(); + + // Deliberately not a toggle: a dialog owns every unmodified key + // while it is up, so a second `?` never reaches the service and a + // toggle would be a promise the shortcut layer cannot keep. + await openOverlay(el); + expect(shadow(el, '[data-testid="shortcuts-overlay"]')).toBeTruthy(); + }); + + it('lists the autotag keys, which Settings never showed', async () => { + const el = await fixture('shortcuts-overlay'); + + await openOverlay(el); + + const headings = shadowAll(el, 'h3').map((h) => + h.textContent!.trim().split(' ')[0], + ); + + expect(headings).toContain('Autotag'); + }); + + it('shows the bound key, not the default it shipped with', async () => { + const el = await fixture('shortcuts-overlay'); + + emit(Events.ShortcutsConfigChanged, { 'player.playPause': 'K' }); + await openOverlay(el); + + const playPause = shadowAll(el, '.row').find((row) => + row.textContent!.includes('Play / Pause'), + ); + + expect(playPause?.querySelector('kbd')?.textContent).toBe('K'); + }); + + it('splits a combination into one key each', async () => { + const el = await fixture('shortcuts-overlay'); + + emit(Events.ShortcutsConfigChanged, { 'app.selectAll': 'Ctrl+A' }); + await openOverlay(el); + + const selectAll = shadowAll(el, '.row').find((row) => + row.textContent!.includes('Select All'), + ); + + expect( + [...selectAll!.querySelectorAll('kbd')].map((k) => k.textContent), + ).toEqual(['Ctrl', 'A']); + }); +}); diff --git a/frontend/test/stores/keyboard-shortcuts.test.ts b/frontend/test/stores/keyboard-shortcuts.test.ts index 3fa5c1e..2085c0d 100644 --- a/frontend/test/stores/keyboard-shortcuts.test.ts +++ b/frontend/test/stores/keyboard-shortcuts.test.ts @@ -86,6 +86,20 @@ describe('buildKeyString', () => { expect(names).toEqual(['Up', 'Down', 'Left', 'Right', 'Space']); }); + it('does not report Shift for a character Shift produced', () => { + // `?` is Shift+/ on a US layout and something else elsewhere, so + // "Shift+?" is a binding nobody would write down. Letters keep it: + // `Shift+A` and `A` are the same character. + const question = new KeyboardEvent('keydown', { + key: '?', + shiftKey: true, + }); + const letter = new KeyboardEvent('keydown', { key: 'A', shiftKey: true }); + + expect(buildKeyString(question)).toBe('?'); + expect(buildKeyString(letter)).toBe('Shift+A'); + }); + it('leaves multi-character named keys alone', () => { expect( buildKeyString(new KeyboardEvent('keydown', { key: 'Escape' })), @@ -233,6 +247,37 @@ describe('shortcut dispatch: scope', () => { expect(calls('player.Player.ChangeVolume')).toHaveLength(0); }); + it('leaves Up and Down to a focused row, which moves on them', () => { + bindings({ 'player.volumeUp': 'Up' }); + + const row = mount(document.createElement('div')); + + row.setAttribute('role', 'row'); + row.tabIndex = 0; + row.focus(); + press('ArrowUp'); + + expect(calls('player.Player.ChangeVolume')).toHaveLength(0); + }); + + it('still seeks on Left/Right from a focused row', () => { + // Phase 1 gave the arrows to the grid, correctly — but all six of + // them, and no list in this app moves horizontally, so seeking + // stopped working from a focused row and nothing gained the keys. + // Reproduced in the app: two ArrowRights on a focused track row, + // zero Player.Seek calls, against one per press from the body. + bindings({ 'player.seekForward': 'Right' }); + + const row = mount(document.createElement('div')); + + row.setAttribute('role', 'row'); + row.tabIndex = 0; + row.focus(); + press('ArrowRight'); + + expect(calls('player.Player.CurrentPositionSeconds')).toHaveLength(1); + }); + it('leaves every unmodified key to an open dialog', () => { const dialog = mount(document.createElement('div')); const button = document.createElement('button');