From 64801d5735b53804c887c03d1dc97a4e8e6f8c1a Mon Sep 17 00:00:00 2001
From: OmikronApex <45161725+OmikronApex@users.noreply.github.com>
Date: Wed, 24 Jun 2026 21:12:55 +0200
Subject: [PATCH] fix(player): Space bar play/pause when focus is on sidebar or
 rail buttons (#593)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* fix(player): Space bar play/pause when focus is on sidebar or rail buttons

When any <button> in the player rail (viz, audio, mixer, etc.), a sidebar
nav link, or a popover control has keyboard focus, pressing Space was
blocked by _shortcutDispatchBlocked → _isInsideInteractiveControl, which
returns true for BUTTON elements. The Space shortcut never reached the
shortcut dispatcher and togglePlay() was never called.

The fix extends the same carve-out pattern already used for the section
practice bar: when the player screen is active, Space is always dispatched
through the shortcut system. The shortcut handler's preventDefault() stops
the focused element from also activating, so this is not a double-trigger.

* test(player): cover Space play/pause carve-out + add CHANGELOG entry

Adds two Playwright regression tests for #593 in
tests/browser/keyboard-shortcuts.spec.ts:
- Space toggles play/pause when a player rail <button> has focus, and
  the focused button does NOT also activate (dispatcher preventDefault).
  Fails on base (Space blocked, played=0), passes with the carve-out.
- Space in a player-screen text input still types a space and never
  reaches play/pause (locks the _isTextInput exemption ordering).

Also records the fix under CHANGELOG [Unreleased] -> Fixed, per the
project workflow that every PR updates the changelog.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(player): don't override Space inside modal dialogs over the player

The player-screen Space carve-out keyed off the active *screen*, so it
also hijacked Space inside a true modal dialog layered over the player
(e.g. the keyboard-shortcuts help modal, edit modal): Space toggled
playback behind the modal and preventDefault blocked the modal's focused
control (Close) from activating — contradicting aria-modal semantics.

Narrow the carve-out to skip focus inside a modal
(role="dialog" aria-modal="true" or .feedBack-modal). Non-modal player
popovers/toasts (loop A/B, arrangement pin, role=dialog aria-modal=false)
are not dialogs and stay covered, so the original fix is unchanged for
the cases it targeted. Adds a Playwright regression test (Space inside a
modal reaches the modal's button, not play/pause) and updates the
CHANGELOG entry.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: byrongamatos <xasiklas@gmail.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---
 CHANGELOG.md                             |   1 +
 static/app.js                            |  15 +++
 tests/browser/keyboard-shortcuts.spec.ts | 131 +++++++++++++++++++++++
 3 files changed, 147 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index e1b8720..6fd04dc 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -34,6 +34,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - **v3 library: exact artist/album filters + scroll/page-depth restore** (feedBack#857). The v3 Songs toolbar gains Artist and Album dropdowns (Album populates from the selected artist and stays disabled until one is chosen), backed by new exact, case-insensitive (`COLLATE NOCASE`) `artist` / `album` query params threaded through `MetadataDB._build_where` → `query_page` / `query_artists` / `query_stats` and the `/api/library`, `/api/library/artists`, `/api/library/stats` endpoints (the free-text `q` search stays fuzzy and composes with the exact filters). The artist/album catalog is fetched independently of the active artist/album selection so the dropdowns always list the full set for the current provider/search. The toolbar is now sticky so filter controls stay reachable when browsing deep libraries, and returning from the player restores the previous scroll position **and** the loaded infinite-scroll page depth via a `sessionStorage` snapshot keyed by a filter/sort/view state hash (invalidated whenever those change, so a filter change still resets to the top). Tests: `tests/test_library_filters.py` (backend artist/album filters), `tests/js/v3_songs_scroll.test.js` (state-hash + snapshot helpers).
 
 ### Fixed
+- **Space bar now plays/pauses on the player screen even when a sidebar nav link or rail button has focus.** When any `<button>` in the player rail (viz, audio, mixer, lyrics, plugins, advanced), a sidebar nav link, or a popover control held keyboard focus, pressing Space was swallowed by `_shortcutDispatchBlocked` → `_isInsideInteractiveControl` (which treats `BUTTON`/`A` as interactive), so the Space shortcut never reached the dispatcher and `togglePlay()` never ran. `_shortcutDispatchBlocked` (`static/app.js`) now extends the same carve-out already used for the Section Practice bar: while the player screen is active, Space is always routed through the shortcut system — the dispatcher calls `e.preventDefault()` before invoking the handler, so the focused element does not also activate. Text inputs (`_isTextInput`) remain exempted first, so typing space in a search/input field still works normally, and focus inside a true modal dialog (`role="dialog" aria-modal="true"` / `.feedBack-modal`) layered over the player is also exempted so Space reaches the modal's focused control (e.g. its Close button) instead of toggling playback behind it — non-modal player popovers/toasts (loop A/B, arrangement pin) stay covered. Regression tests in `tests/browser/keyboard-shortcuts.spec.ts` cover the focused-rail-button play/pause, the text-input exemption, and the modal-dialog exemption.
 - **A song's accuracy badge now updates on its library card right after you play it — no restart needed.** The v3 library (`static/v3/songs.js`) loaded the best-accuracy map (`/api/stats/best`) once into `state.accuracy` at render time and only ever refreshed it on a full re-render; the play→return flow takes the screen-entry fast-path that restores the cached grid DOM without re-fetching, so a just-earned score stayed invisible until the next restart re-ran `render()`. The `stats-recorder` now emits a `stats:recorded` event (carrying `filename`/`arrangement`) once the scored `POST /api/stats` resolves on the server — the correct moment, since `song:stop` fires before the POST completes. `songs.js` listens: if the library is the active screen it re-fetches `/api/stats/best` and patches the affected card/row badge in place; otherwise it marks the filename dirty and `onV3SongsScreenEnter` applies it on return (a failed fetch keeps the entry dirty to retry). Badge markup was factored into a shared `accuracyBadge(filename, variant)` (grid pill + tree-row percentage, both tagged `.fb-acc-badge`) so the in-place `repaintAccuracy` can find and replace them without a full list re-render (scroll/pagination preserved). The old empty `song:stop` "refresh lazily next render" placeholder is replaced.
 - **Changing Settings → 3D Highway → Fret spacing no longer ejects you to the home screen.** The `highway_3d` plugin's `h3dSetFretSpacing` was the lone 3D-highway setting that called `location.reload()` to apply — and since the SPA boots with `#home` as the active screen (`index.html` `.screen.active`), the reload dropped the user out of Settings onto the homescreen. It now applies live like every other 3D-highway setting: it rebinds the module-scope `_h3dFretUniform` flag (so panels mounted later this session pick up the new mode), recomputes the two `fretX`-derived scalars that were baked at init (`_fretLabelScaleRefW` for fret-label sprite scaling, `FRET_WIDTH_MID` for camera hysteresis), and broadcasts a `fretSpacing` change over the existing `_bgEmitChange` pub-sub so every mounted panel rebuilds its board via `buildBoard()`. Per-frame note geometry already reads `fretX` live and needs no rebuild. No page reload, so the Settings screen stays put. Source-level regression tests in `tests/js/highway_3d_fret_spacing.test.js` now pin the no-reload / live-rebuild behavior.
 - **v3 library scroll-restore no longer breaks the classic v2 UI or drops off-screen searches** (feedBack#857). Two regressions in the scroll-restore work above: (1) `playSong` remapped `home`-launched songs to return to the `#v3-songs` screen unconditionally, but `static/app.js` is shared with the v2 UI (served at `/v2` / `FEEDBACK_UI=v2`) where that screen does not exist — Esc-from-player then called `showScreen('v3-songs')`, which threw on the missing element and stranded the user on a blank screen with playback still running; the remap now applies only when `#v3-songs` is present. (2) The Songs screen-entry fast-path skips reloading to preserve scroll, but the global topbar search routed through it, so once Songs had been visited, searching from another screen navigated there without applying the new query; the screen now tracks the state hash each fetch reflects and refetches when it has drifted, keeping the scroll-preserving no-op only when nothing changed.
diff --git a/static/app.js b/static/app.js
index 1360887..0cf847d 100644
--- a/static/app.js
+++ b/static/app.js
@@ -445,6 +445,21 @@ function _shortcutDispatchBlocked(e) {
     // the popover's own keydown listener) — suppress the player-scope
     // "back to library" Esc so the user doesn't get bounced out of the player.
     if (e.key === 'Escape' && _sectionPracticePopoverOpen()) return true;
+    // Space on the player screen should always play/pause, even if focus is on a
+    // sidebar nav link, player rail button, popover control, or any other
+    // interactive element — the shortcut dispatcher calls preventDefault so the
+    // focused element won't also activate. Two exceptions keep native Space:
+    // text inputs (already exempted above), and focus inside a true modal
+    // dialog (role="dialog" aria-modal="true", or a .feedBack-modal overlay)
+    // layered over the player — a modal traps interaction, so Space must reach
+    // its focused control (e.g. the Close button) rather than toggle playback
+    // behind it. Non-modal player popovers/toasts (loop A/B, arrangement pin,
+    // role="dialog" aria-modal="false") are not modals and stay covered.
+    if (_isSpaceKey(e) && _getCurrentContext().isPlayer &&
+        !(e.target && e.target.closest &&
+          e.target.closest('[role="dialog"][aria-modal="true"], .feedBack-modal'))) {
+        return false;
+    }
     return _isInsideInteractiveControl(e.target);
 }
 
diff --git a/tests/browser/keyboard-shortcuts.spec.ts b/tests/browser/keyboard-shortcuts.spec.ts
index 3804513..d307ab0 100644
--- a/tests/browser/keyboard-shortcuts.spec.ts
+++ b/tests/browser/keyboard-shortcuts.spec.ts
@@ -651,6 +651,137 @@ test('should support condition callbacks', async ({ page }) => {
     expect(result.calledByKey).toBe(true);
   });
 
+  test('Space toggles play/pause when a player rail button is focused (#593)', async ({ page }) => {
+    await openPlayerWithMockSong(page);
+
+    // Inject a focusable <button> into the player (the bug: BUTTON elements
+    // are "interactive controls" so Space was blocked before reaching the
+    // shortcut dispatcher) and spy on the player-scope Space shortcut so the
+    // assertion does not depend on the real audio path. The dispatcher calls
+    // preventDefault() before the handler, so the focused button must NOT
+    // also activate.
+    await page.evaluate(() => {
+      // @ts-ignore
+      window.__spacePlayCount = 0;
+      // @ts-ignore
+      window.__railBtnClicked = 0;
+      // @ts-ignore
+      window.registerShortcut({
+        key: 'Space',
+        description: 'Play/Pause (test spy)',
+        scope: 'player',
+        // @ts-ignore
+        handler: () => { window.__spacePlayCount++; },
+      });
+      const btn = document.createElement('button');
+      btn.id = '__test-rail-btn';
+      btn.textContent = 'Mixer';
+      // @ts-ignore
+      btn.addEventListener('click', () => { window.__railBtnClicked++; });
+      document.getElementById('player')!.appendChild(btn);
+    });
+
+    await page.locator('#__test-rail-btn').focus();
+    await expect(page.locator('#__test-rail-btn')).toBeFocused();
+
+    await page.keyboard.press('Space');
+
+    const result = await page.evaluate(() => ({
+      // @ts-ignore
+      played: window.__spacePlayCount,
+      // @ts-ignore
+      clicked: window.__railBtnClicked,
+    }));
+    // Play/pause fired despite the button holding focus…
+    expect(result.played).toBe(1);
+    // …and the focused button did not also activate (dispatcher preventDefault()).
+    expect(result.clicked).toBe(0);
+  });
+
+  test('Space in a player-screen text input still types a space, not play/pause (#593)', async ({ page }) => {
+    await openPlayerWithMockSong(page);
+
+    // The text-input exemption (_isTextInput) is checked before the player
+    // Space carve-out, so typing space in an input must never toggle playback.
+    await page.evaluate(() => {
+      // @ts-ignore
+      window.__spacePlayCount = 0;
+      // @ts-ignore
+      window.registerShortcut({
+        key: 'Space',
+        description: 'Play/Pause (test spy)',
+        scope: 'player',
+        // @ts-ignore
+        handler: () => { window.__spacePlayCount++; },
+      });
+      const input = document.createElement('input');
+      input.type = 'text';
+      input.id = '__test-player-input';
+      document.getElementById('player')!.appendChild(input);
+    });
+
+    await page.locator('#__test-player-input').focus();
+    await page.keyboard.press('Space');
+
+    const result = await page.evaluate(() => ({
+      // @ts-ignore
+      played: window.__spacePlayCount,
+      value: (document.getElementById('__test-player-input') as HTMLInputElement).value,
+    }));
+    expect(result.played).toBe(0);
+    expect(result.value).toBe(' ');
+  });
+
+  test('Space inside a modal dialog over the player reaches the modal, not play/pause (#593)', async ({ page }) => {
+    await openPlayerWithMockSong(page);
+
+    // A true modal dialog (role="dialog" aria-modal="true" / .feedBack-modal)
+    // layered over the player must trap interaction: Space activates the
+    // modal's focused control (native), it does NOT toggle playback behind it.
+    await page.evaluate(() => {
+      // @ts-ignore
+      window.__spacePlayCount = 0;
+      // @ts-ignore
+      window.__modalBtnClicked = 0;
+      // @ts-ignore
+      window.registerShortcut({
+        key: 'Space',
+        description: 'Play/Pause (test spy)',
+        scope: 'player',
+        // @ts-ignore
+        handler: () => { window.__spacePlayCount++; },
+      });
+      const modal = document.createElement('div');
+      modal.id = '__test-modal';
+      modal.className = 'feedBack-modal';
+      modal.setAttribute('role', 'dialog');
+      modal.setAttribute('aria-modal', 'true');
+      const btn = document.createElement('button');
+      btn.id = '__test-modal-btn';
+      btn.textContent = 'Close';
+      // @ts-ignore
+      btn.addEventListener('click', () => { window.__modalBtnClicked++; });
+      modal.appendChild(btn);
+      document.body.appendChild(modal);
+    });
+
+    await page.locator('#__test-modal-btn').focus();
+    await expect(page.locator('#__test-modal-btn')).toBeFocused();
+
+    await page.keyboard.press('Space');
+
+    const result = await page.evaluate(() => ({
+      // @ts-ignore
+      played: window.__spacePlayCount,
+      // @ts-ignore
+      clicked: window.__modalBtnClicked,
+    }));
+    // Playback is NOT toggled behind the modal…
+    expect(result.played).toBe(0);
+    // …and Space activated the modal's focused button natively.
+    expect(result.clicked).toBe(1);
+  });
+
   test('should warn on invalid scope', async ({ page }) => {
     const messages: string[] = [];
     page.on('console', msg => {