mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-08-11 03:09:57 +00:00
fix(player): Space bar play/pause when focus is on sidebar or rail buttons (#593)
* 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>
This commit is contained in:
OmikronApex
co-authored by
Claude Opus 4.8
byrongamatos
Claude Opus 4.8
byrongamatos
parent
d2569cc2a8
commit
64801d5735
@@ -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 => {
|
||||
|
||||
Reference in New Issue
Block a user