From 4e8abcc25e9f3a68502dce2f4a1b6af5c5867881 Mon Sep 17 00:00:00 2001 From: sudacode Date: Wed, 5 Aug 2026 02:48:10 -0700 Subject: [PATCH] fix(overlay): fix changelog modal keyboard nav and version parsing - Extract shared modal-focus-guard module reused by changelog and session-help modals - Only fold the selected changelog entry on Enter/Space; leave close/refresh buttons and nested folds to their own activation - Stop re-stealing focus after a mouse click already selected an entry - Parse prerelease and build metadata separately in version headings (e.g. 0.15.0-rc.1+build.2) - Resolve tray menu test assertions by label instead of index --- src/core/utils/changelog-parse.test.ts | 27 ++ src/core/utils/changelog-parse.ts | 11 +- src/main/runtime/tray-runtime.test.ts | 70 +++-- src/renderer/modals/changelog.test.ts | 166 +++++++++- src/renderer/modals/changelog.ts | 152 +++------ src/renderer/modals/modal-focus-guard.test.ts | 290 ++++++++++++++++++ src/renderer/modals/modal-focus-guard.ts | 141 +++++++++ src/renderer/modals/session-help.ts | 125 +------- 8 files changed, 721 insertions(+), 261 deletions(-) create mode 100644 src/renderer/modals/modal-focus-guard.test.ts create mode 100644 src/renderer/modals/modal-focus-guard.ts diff --git a/src/core/utils/changelog-parse.test.ts b/src/core/utils/changelog-parse.test.ts index ff0bd3ee..fe371535 100644 --- a/src/core/utils/changelog-parse.test.ts +++ b/src/core/utils/changelog-parse.test.ts @@ -141,6 +141,33 @@ test('changelog parser nests three bullet levels and rejoins wrapped lines', () ]); }); +test('changelog parser reads prerelease and build metadata version headings', () => { + const entries = parseChangelog( + [ + '## v0.16.0 (2026-06-01)', + '', + '### Added', + '- New in 0.16.', + '', + '## v0.15.0-rc.1+build.2 (2026-05-29)', + '', + '### Added', + '- Release candidate note.', + '', + ].join('\n'), + ); + + // An unrecognized heading does not just vanish: its notes fold into the + // previous release, so the version list has to stay exact. + assert.deepEqual( + entries.map((entry) => entry.version), + ['0.16.0', '0.15.0-rc.1+build.2'], + ); + assert.equal(entries[1]?.date, '2026-05-29'); + assert.equal(entries[1]?.groupKey, '0.15'); + assert.equal(entries[0]?.sections.length, 1); +}); + test('changelog parser handles the repo CHANGELOG.md', () => { const markdown = fs.readFileSync(path.join(process.cwd(), 'CHANGELOG.md'), 'utf8'); const entries = parseChangelog(markdown); diff --git a/src/core/utils/changelog-parse.ts b/src/core/utils/changelog-parse.ts index b8f16cf2..ab21ebd5 100644 --- a/src/core/utils/changelog-parse.ts +++ b/src/core/utils/changelog-parse.ts @@ -1,6 +1,10 @@ import type { ChangelogEntry, ChangelogItem, ChangelogSection } from '../../types/changelog'; -const VERSION_HEADING = /^##\s+v(\d+\.\d+\.\d+(?:[-+][0-9A-Za-z.-]+)?)\s*(?:\(([^)]*)\))?\s*$/; +// Prerelease and build metadata are matched separately: a single `[-+]`-led +// group cannot span `-rc.1+build.2`, and an unmatched heading silently folds +// that release's notes into the previous entry. +const VERSION_HEADING = + /^##\s+v(\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?)\s*(?:\(([^)]*)\))?\s*$/; const SECTION_HEADING = /^###\s+(.+?)\s*$/; const BULLET = /^(\s*)[-*]\s+(.*)$/; @@ -106,8 +110,9 @@ export function parseChangelog(markdown: string): ChangelogEntry[] { continue; } - // A blank line ends the current bullet run; an indented non-bullet line is a - // wrapped continuation of the bullet above it. + // An indented non-bullet line continues the bullet above it, including + // across a blank line: that is CommonMark's continuation paragraph, and + // dropping the open bullets here would silently discard the text. if (!trimmed) { continue; } diff --git a/src/main/runtime/tray-runtime.test.ts b/src/main/runtime/tray-runtime.test.ts index 4edd847f..7c2ab149 100644 --- a/src/main/runtime/tray-runtime.test.ts +++ b/src/main/runtime/tray-runtime.test.ts @@ -50,35 +50,49 @@ test('tray menu template contains expected entries and handlers', () => { quitApp: () => calls.push('quit'), }); - assert.equal(template.length, 15); - assert.equal( - template.some((entry) => entry.label === 'Open Runtime Options'), - false, + // Resolve by label, not index: adding a menu entry should not force every + // later assertion in this test to be renumbered. + const entryFor = (label: string) => { + const entry = template.find((candidate) => candidate.label === label); + assert.ok(entry, `expected a "${label}" tray entry`); + return entry; + }; + + assert.deepEqual( + template.map((entry) => entry.label ?? `<${entry.type}>`), + [ + 'Open Help', + 'View Changelog', + 'Open Texthooker', + 'Complete Setup', + 'Open SubMiner Setup', + 'Open Yomitan Settings', + 'Open SubMiner Settings', + 'Sync Stats && History', + 'Export Logs', + 'Configure Jellyfin', + 'Jellyfin Discovery', + 'Configure AniList', + 'Check for Updates', + '', + 'Quit', + ], ); - assert.equal( - template.some((entry) => entry.label === 'Open Overlay'), - false, - ); - assert.equal(template[0]!.label, 'Open Help'); - assert.equal(template[4]!.label, 'Open SubMiner Setup'); - const discovery = template.find((entry) => entry.label === 'Jellyfin Discovery'); - assert.equal(discovery?.type, 'checkbox'); - assert.equal(discovery?.checked, false); - discovery?.click?.({ checked: true }); - template[0]!.click?.(); - assert.equal(template[1]!.label, 'View Changelog'); - template[1]!.click?.(); - assert.equal(template[2]!.label, 'Open Texthooker'); - template[2]!.click?.(); - assert.equal(template[6]!.label, 'Open SubMiner Settings'); - assert.equal(template[7]!.label, 'Sync Stats && History'); - template[7]!.click?.(); - assert.equal(template[8]!.label, 'Export Logs'); - template[8]!.click?.(); - assert.equal(template[12]!.label, 'Check for Updates'); - template[12]!.click?.(); - template[13]!.type === 'separator' ? calls.push('separator') : calls.push('bad'); - template[14]!.click?.(); + + const discovery = entryFor('Jellyfin Discovery'); + assert.equal(discovery.type, 'checkbox'); + assert.equal(discovery.checked, false); + discovery.click?.({ checked: true }); + + entryFor('Open Help').click?.(); + entryFor('View Changelog').click?.(); + entryFor('Open Texthooker').click?.(); + entryFor('Sync Stats && History').click?.(); + entryFor('Export Logs').click?.(); + entryFor('Check for Updates').click?.(); + calls.push(template.some((entry) => entry.type === 'separator') ? 'separator' : 'bad'); + entryFor('Quit').click?.(); + assert.deepEqual(calls, [ 'jellyfin-discovery:true', 'help', diff --git a/src/renderer/modals/changelog.test.ts b/src/renderer/modals/changelog.test.ts index f51105c0..b7210904 100644 --- a/src/renderer/modals/changelog.test.ts +++ b/src/renderer/modals/changelog.test.ts @@ -24,7 +24,37 @@ function createClassList(initialTokens: string[] = []) { }; } +type SummaryStub = { + classList: ReturnType; + tabIndex: number; + dataset: Record; + getClientRects: () => Array<{ width: number; height: number }>; + focusCount: number; + scrollCount: number; + focus: () => void; + scrollIntoView: () => void; +}; + +function createSummaryStub(index: number): SummaryStub { + const summary: SummaryStub = { + classList: createClassList(), + tabIndex: -1, + dataset: { changelogIndex: String(index) }, + getClientRects: () => [{ width: 10, height: 10 }], + focusCount: 0, + scrollCount: 0, + focus: () => { + summary.focusCount += 1; + }, + scrollIntoView: () => { + summary.scrollCount += 1; + }, + }; + return summary; +} + function createElementStub() { + const listeners = new Map void>>(); return { value: '', textContent: '', @@ -32,11 +62,19 @@ function createElementStub() { classList: createClassList(['hidden']), contains: () => false, setAttribute: () => {}, - addEventListener: () => {}, + addEventListener: (type: string, listener: (event?: unknown) => void) => { + listeners.set(type, [...(listeners.get(type) ?? []), listener]); + }, removeEventListener: () => {}, appendChild: () => {}, - querySelectorAll: () => [], + summaries: [] as SummaryStub[], + querySelectorAll(this: { summaries: SummaryStub[] }) { + return this.summaries; + }, focus: () => {}, + dispatchEventType: (type: string, event?: unknown) => { + for (const listener of listeners.get(type) ?? []) listener(event); + }, }; } @@ -66,6 +104,7 @@ function createHarness( const previousDocument = Object.getOwnPropertyDescriptor(globalThis, 'document'); const previousHTMLElement = Object.getOwnPropertyDescriptor(globalThis, 'HTMLElement'); const previousElement = Object.getOwnPropertyDescriptor(globalThis, 'Element'); + const previousDetails = Object.getOwnPropertyDescriptor(globalThis, 'HTMLDetailsElement'); const snapshotRequests: Array<{ refresh?: boolean } | undefined> = []; const modalClosedNotifications: string[] = []; @@ -78,6 +117,15 @@ function createHarness( value: TestElement, }); } + // getSelectedEntry() narrows with `instanceof HTMLDetailsElement`. + class TestDetailsElement { + open = false; + } + Object.defineProperty(globalThis, 'HTMLDetailsElement', { + configurable: true, + writable: true, + value: TestDetailsElement, + }); Object.defineProperty(globalThis, 'window', { configurable: true, writable: true, @@ -153,6 +201,7 @@ function createHarness( ['document', previousDocument], ['HTMLElement', previousHTMLElement], ['Element', previousElement], + ['HTMLDetailsElement', previousDetails], ] as const) { if (descriptor) { Object.defineProperty(globalThis, name, descriptor); @@ -282,6 +331,119 @@ test('changelog modal clears stale metadata when a refresh fails', async () => { } }); +test('changelog modal moves selection styling and focus together on J/K', async () => { + const harness = createHarness(); + try { + harness.modal.openChangelogModal(); + await new Promise((resolve) => setTimeout(resolve, 0)); + + const summaries = [createSummaryStub(0), createSummaryStub(1), createSummaryStub(2)]; + harness.dom.changelogList!.summaries = summaries; + + harness.modal.handleChangelogKeydown({ key: 'j', preventDefault: () => {} } as KeyboardEvent); + + assert.deepEqual( + summaries.map((summary) => summary.classList.contains('active')), + [false, true, false], + ); + assert.deepEqual( + summaries.map((summary) => summary.tabIndex), + [-1, 0, -1], + ); + assert.equal(summaries[1]?.focusCount, 1, 'the keyboard path focuses the new selection'); + assert.equal(summaries[1]?.scrollCount, 1); + + // Wraps backwards past the start. + harness.modal.handleChangelogKeydown({ key: 'k', preventDefault: () => {} } as KeyboardEvent); + harness.modal.handleChangelogKeydown({ key: 'k', preventDefault: () => {} } as KeyboardEvent); + assert.deepEqual( + summaries.map((summary) => summary.classList.contains('active')), + [false, false, true], + ); + } finally { + harness.restore(); + } +}); + +test('changelog modal click selection restyles without stealing focus back', async () => { + const harness = createHarness(); + try { + harness.modal.wireDomEvents(); + harness.modal.openChangelogModal(); + await new Promise((resolve) => setTimeout(resolve, 0)); + + const summaries = [createSummaryStub(0), createSummaryStub(1)]; + harness.dom.changelogList!.summaries = summaries; + + // The handler guards on `instanceof Element`, so the target has to inherit + // from the Element stand-in the harness installs. + const elementCtor = (globalThis as unknown as { Element: { prototype: object } }).Element; + const clickTarget = Object.assign(Object.create(elementCtor.prototype), { + closest: (selector: string) => + selector === '.changelog-entry-summary' ? summaries[1] : null, + }); + harness.dom.changelogList!.dispatchEventType('click', { target: clickTarget }); + + assert.deepEqual( + summaries.map((summary) => summary.classList.contains('active')), + [false, true], + ); + assert.deepEqual( + summaries.map((summary) => summary.tabIndex), + [-1, 0], + ); + // The browser already focused the clicked summary; re-focusing would fight it. + assert.equal(summaries[1]?.focusCount, 0); + } finally { + harness.restore(); + } +}); + +test('changelog modal folds on Enter only from the selected summary', async () => { + const harness = createHarness(); + try { + harness.modal.openChangelogModal(); + await new Promise((resolve) => setTimeout(resolve, 0)); + + const detailsCtor = ( + globalThis as unknown as { HTMLDetailsElement: new () => { open: boolean } } + ).HTMLDetailsElement; + const entry = new detailsCtor(); + entry.open = true; + + const summaries = [createSummaryStub(0), createSummaryStub(1)]; + Object.assign(summaries[0]!, { parentElement: entry }); + harness.dom.changelogList!.summaries = summaries; + + let prevented = 0; + const press = (target: unknown) => + harness.modal.handleChangelogKeydown({ + key: 'Enter', + target, + preventDefault: () => { + prevented += 1; + }, + } as unknown as KeyboardEvent); + + // Close button focused: the button must keep its own Enter activation. + assert.equal(press(harness.dom.changelogClose), true); + assert.equal(prevented, 0, 'Enter on a button is not swallowed'); + assert.equal(entry.open, true); + + // A non-selected summary (the nested "Internal changes" fold) is left alone. + assert.equal(press(summaries[1]), true); + assert.equal(prevented, 0); + assert.equal(entry.open, true); + + // The selected summary does fold, exactly once. + assert.equal(press(summaries[0]), true); + assert.equal(prevented, 1); + assert.equal(entry.open, false); + } finally { + harness.restore(); + } +}); + test('changelog modal closes on Escape and notifies the main process', async () => { const harness = createHarness(); try { diff --git a/src/renderer/modals/changelog.ts b/src/renderer/modals/changelog.ts index c1df36e8..9eac6e65 100644 --- a/src/renderer/modals/changelog.ts +++ b/src/renderer/modals/changelog.ts @@ -6,6 +6,7 @@ import { resolveEntryBadge, shouldEntryStartExpanded, } from './changelog-render'; +import { createModalFocusGuard } from './modal-focus-guard'; export function createChangelogModal( ctx: RendererContext, @@ -15,11 +16,6 @@ export function createChangelogModal( }, ) { let priorFocus: Element | null = null; - let focusGuard: ((event: FocusEvent) => void) | null = null; - let windowFocusGuard: (() => void) | null = null; - let modalPointerFocusGuard: ((event: Event) => void) | null = null; - let isRecoveringModalFocus = false; - let lastFocusRecoveryAt = 0; let loadToken = 0; function getSummaries(): HTMLElement[] { @@ -28,19 +24,25 @@ export function createChangelogModal( ) as HTMLElement[]; } - function setSelected(index: number): void { + function applySelectionStyles(index: number): HTMLElement[] { const summaries = getSummaries(); - if (summaries.length === 0) return; - - const wrapped = index % summaries.length; - const next = wrapped < 0 ? wrapped + summaries.length : wrapped; - ctx.state.changelogSelectedIndex = next; - + ctx.state.changelogSelectedIndex = index; summaries.forEach((summary, idx) => { - summary.classList.toggle('active', idx === next); - summary.tabIndex = idx === next ? 0 : -1; + summary.classList.toggle('active', idx === index); + summary.tabIndex = idx === index ? 0 : -1; }); - const active = summaries[next]; + return summaries; + } + + function setSelected(index: number): void { + const count = getSummaries().length; + if (count === 0) return; + + const wrapped = index % count; + const next = wrapped < 0 ? wrapped + count : wrapped; + + // Only the keyboard path moves focus; clicking already focused the summary. + const active = applySelectionStyles(next)[next]; if (!active) return; active.focus({ preventScroll: true }); active.scrollIntoView({ block: 'nearest', inline: 'nearest' }); @@ -52,82 +54,13 @@ export function createChangelogModal( return entry instanceof HTMLDetailsElement ? entry : null; } - function isChangelogModalFocusTarget(target: EventTarget | null): boolean { - return target instanceof Element && ctx.dom.changelogModal.contains(target); - } - - function focusFallbackTarget(): boolean { - if (!ctx.platform.isModalLayer) { - void window.electronAPI.focusMainWindow(); - } - const firstSummary = getSummaries().find((summary) => summary.offsetParent !== null); - if (firstSummary) { - firstSummary.focus({ preventScroll: true }); - return document.activeElement === firstSummary; - } - if (ctx.dom.changelogClose instanceof HTMLElement) { - ctx.dom.changelogClose.focus({ preventScroll: true }); - return document.activeElement === ctx.dom.changelogClose; - } - window.focus(); - return false; - } - - function enforceModalFocus(): void { - if (!ctx.state.changelogModalOpen) return; - if (isChangelogModalFocusTarget(document.activeElement)) return; - if (isRecoveringModalFocus) return; - - const now = Date.now(); - if (now - lastFocusRecoveryAt < 120) return; - - isRecoveringModalFocus = true; - lastFocusRecoveryAt = now; - focusFallbackTarget(); - window.setTimeout(() => { - isRecoveringModalFocus = false; - }, 120); - } - - function requestOverlayFocus(): void { - if (!ctx.platform.isModalLayer) { - void window.electronAPI.focusMainWindow(); - } - } - - function addPointerFocusListener(): void { - if (modalPointerFocusGuard) return; - modalPointerFocusGuard = () => { - requestOverlayFocus(); - enforceModalFocus(); - }; - ctx.dom.changelogModal.addEventListener('pointerdown', modalPointerFocusGuard); - ctx.dom.changelogModal.addEventListener('click', modalPointerFocusGuard); - } - - function removePointerFocusListener(): void { - if (!modalPointerFocusGuard) return; - ctx.dom.changelogModal.removeEventListener('pointerdown', modalPointerFocusGuard); - ctx.dom.changelogModal.removeEventListener('click', modalPointerFocusGuard); - modalPointerFocusGuard = null; - } - - function startFocusRecoveryGuards(): void { - if (windowFocusGuard) return; - windowFocusGuard = () => { - requestOverlayFocus(); - enforceModalFocus(); - }; - window.addEventListener('blur', windowFocusGuard); - window.addEventListener('focus', windowFocusGuard); - } - - function stopFocusRecoveryGuards(): void { - if (!windowFocusGuard) return; - window.removeEventListener('blur', windowFocusGuard); - window.removeEventListener('focus', windowFocusGuard); - windowFocusGuard = null; - } + const focus = createModalFocusGuard({ + isOpen: () => ctx.state.changelogModalOpen, + getModalRoot: () => ctx.dom.changelogModal, + getPreferredFocusTargets: () => getSummaries(), + getFallbackFocusTarget: () => ctx.dom.changelogClose, + isModalLayer: ctx.platform.isModalLayer, + }); function renderSnapshot(snapshot: ChangelogSnapshot): void { ctx.dom.changelogList.innerHTML = ''; @@ -206,22 +139,10 @@ export function createChangelogModal( window.electronAPI.setIgnoreMouseEvents(false); } - if (focusGuard === null) { - focusGuard = (event: FocusEvent) => { - if (!ctx.state.changelogModalOpen) return; - if (!isChangelogModalFocusTarget(event.target)) { - event.preventDefault(); - enforceModalFocus(); - } - }; - document.addEventListener('focusin', focusGuard); - } - - addPointerFocusListener(); - startFocusRecoveryGuards(); - requestOverlayFocus(); + focus.attach(); + focus.requestOverlayFocus(); window.focus(); - enforceModalFocus(); + focus.enforceModalFocus(); void load(); } @@ -239,12 +160,7 @@ export function createChangelogModal( ctx.dom.overlay.classList.remove('interactive'); } - if (focusGuard) { - document.removeEventListener('focusin', focusGuard); - focusGuard = null; - } - removePointerFocusListener(); - stopFocusRecoveryGuards(); + focus.detach(); if (priorFocus instanceof HTMLElement && priorFocus.isConnected) { priorFocus.focus({ preventScroll: true }); @@ -295,6 +211,12 @@ export function createChangelogModal( } if (key === 'enter' || key === ' ') { + // Only the selected release summary folds from here. The Close/Refresh + // buttons and the nested "Internal changes" fold activate themselves, and + // swallowing Enter/Space would make them unreachable by keyboard. + if (e.target !== summaries[ctx.state.changelogSelectedIndex]) { + return true; + } e.preventDefault(); const entry = getSelectedEntry(); if (entry) entry.open = !entry.open; @@ -334,11 +256,7 @@ export function createChangelogModal( if (!summary) return; const index = Number.parseInt(summary.dataset.changelogIndex ?? '', 10); if (!Number.isFinite(index)) return; - ctx.state.changelogSelectedIndex = index; - getSummaries().forEach((item, idx) => { - item.classList.toggle('active', idx === index); - item.tabIndex = idx === index ? 0 : -1; - }); + applySelectionStyles(index); }); } diff --git a/src/renderer/modals/modal-focus-guard.test.ts b/src/renderer/modals/modal-focus-guard.test.ts new file mode 100644 index 00000000..121cb1f6 --- /dev/null +++ b/src/renderer/modals/modal-focus-guard.test.ts @@ -0,0 +1,290 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; + +import { createModalFocusGuard } from './modal-focus-guard'; + +type Listener = (event?: unknown) => void; + +function createRoot(contains: boolean) { + const listeners: Array<{ type: string; listener: Listener }> = []; + return { + contains: () => contains, + addEventListener: (type: string, listener: Listener) => { + listeners.push({ type, listener }); + }, + removeEventListener: (type: string, listener: Listener) => { + const index = listeners.findIndex( + (entry) => entry.type === type && entry.listener === listener, + ); + if (index >= 0) listeners.splice(index, 1); + }, + listeners, + }; +} + +type Harness = { + guard: ReturnType; + root: ReturnType; + focusMainWindowCalls: () => number; + focused: () => string[]; + documentListeners: () => string[]; + windowListeners: () => string[]; + setActiveElement: (value: unknown) => void; + advanceClock: (ms: number) => void; + runTimers: () => void; + restore: () => void; +}; + +function createHarness( + options: { + isOpen?: () => boolean; + isModalLayer?: boolean; + contains?: boolean; + preferredVisible?: boolean; + fallback?: 'element' | null; + } = {}, +): Harness { + const previous = (['window', 'document', 'HTMLElement', 'Element'] as const).map( + (name) => [name, Object.getOwnPropertyDescriptor(globalThis, name)] as const, + ); + + class TestElement {} + for (const name of ['HTMLElement', 'Element'] as const) { + Object.defineProperty(globalThis, name, { + configurable: true, + writable: true, + value: TestElement, + }); + } + + let focusMainWindowCalls = 0; + let now = 1_000; + const realDateNow = Date.now; + Date.now = () => now; + const focused: string[] = []; + const documentListeners: string[] = []; + const windowListeners: string[] = []; + const timers: Array<() => void> = []; + let activeElement: unknown = null; + + const root = createRoot(options.contains ?? false); + + const preferred = Object.assign(new TestElement(), { + // A position:fixed element has a null offsetParent but still has rects. + offsetParent: null, + getClientRects: () => (options.preferredVisible === false ? [] : [{ width: 10, height: 10 }]), + focus: () => { + focused.push('preferred'); + activeElement = preferred; + }, + }); + const fallback = + options.fallback === null + ? null + : Object.assign(new TestElement(), { + focus: () => { + focused.push('fallback'); + activeElement = fallback; + }, + }); + + Object.defineProperty(globalThis, 'window', { + configurable: true, + writable: true, + value: { + electronAPI: { + focusMainWindow: async () => { + focusMainWindowCalls += 1; + }, + }, + focus: () => { + focused.push('window'); + }, + addEventListener: (type: string) => { + windowListeners.push(type); + }, + removeEventListener: (type: string) => { + const index = windowListeners.indexOf(type); + if (index >= 0) windowListeners.splice(index, 1); + }, + setTimeout: (callback: () => void) => { + timers.push(callback); + return timers.length; + }, + }, + }); + Object.defineProperty(globalThis, 'document', { + configurable: true, + writable: true, + value: { + get activeElement() { + return activeElement; + }, + addEventListener: (type: string) => { + documentListeners.push(type); + }, + removeEventListener: (type: string) => { + const index = documentListeners.indexOf(type); + if (index >= 0) documentListeners.splice(index, 1); + }, + }, + }); + + const guard = createModalFocusGuard({ + isOpen: options.isOpen ?? (() => true), + getModalRoot: () => root as unknown as Element, + getPreferredFocusTargets: () => [preferred as unknown as HTMLElement], + getFallbackFocusTarget: () => fallback as unknown as Element | null, + isModalLayer: options.isModalLayer ?? true, + }); + + return { + guard, + root, + focusMainWindowCalls: () => focusMainWindowCalls, + focused: () => focused, + documentListeners: () => documentListeners, + windowListeners: () => windowListeners, + setActiveElement: (value: unknown) => { + activeElement = value; + }, + advanceClock: (ms: number) => { + now += ms; + }, + runTimers: () => { + while (timers.length > 0) timers.shift()?.(); + }, + restore: () => { + Date.now = realDateNow; + for (const [name, descriptor] of previous) { + if (descriptor) { + Object.defineProperty(globalThis, name, descriptor); + } else { + delete (globalThis as Record)[name]; + } + } + }, + }; +} + +test('modal focus guard attaches once and detaches every listener', () => { + const harness = createHarness(); + try { + harness.guard.attach(); + harness.guard.attach(); + + assert.deepEqual(harness.documentListeners(), ['focusin']); + assert.deepEqual(harness.windowListeners(), ['blur', 'focus']); + assert.deepEqual( + harness.root.listeners.map((entry) => entry.type), + ['pointerdown', 'click'], + ); + + harness.guard.detach(); + + assert.deepEqual(harness.documentListeners(), []); + assert.deepEqual(harness.windowListeners(), []); + assert.deepEqual(harness.root.listeners, []); + } finally { + harness.restore(); + } +}); + +test('modal focus guard restores focus to the first rendered target', () => { + // The target is position:fixed (null offsetParent) yet visible, so it must + // still win over the fallback. + const harness = createHarness(); + try { + harness.guard.enforceModalFocus(); + assert.deepEqual(harness.focused(), ['preferred']); + } finally { + harness.restore(); + } +}); + +test('modal focus guard falls back when no preferred target is rendered', () => { + const harness = createHarness({ preferredVisible: false }); + try { + harness.guard.enforceModalFocus(); + assert.deepEqual(harness.focused(), ['fallback']); + } finally { + harness.restore(); + } +}); + +test('modal focus guard focuses the window when nothing else can take focus', () => { + const harness = createHarness({ preferredVisible: false, fallback: null }); + try { + assert.equal(harness.guard.focusFallbackTarget(), false); + assert.deepEqual(harness.focused(), ['window']); + } finally { + harness.restore(); + } +}); + +test('modal focus guard leaves focus alone while it is already inside the modal', () => { + const harness = createHarness({ contains: true }); + try { + harness.setActiveElement( + Object.create((globalThis as { Element: { prototype: object } }).Element.prototype), + ); + harness.guard.enforceModalFocus(); + + assert.deepEqual(harness.focused(), []); + } finally { + harness.restore(); + } +}); + +test('modal focus guard does nothing while the modal is closed', () => { + const harness = createHarness({ isOpen: () => false }); + try { + harness.guard.enforceModalFocus(); + assert.deepEqual(harness.focused(), []); + } finally { + harness.restore(); + } +}); + +test('modal focus guard debounces recovery so a focus fight cannot spin', () => { + const harness = createHarness(); + try { + harness.guard.enforceModalFocus(); + harness.setActiveElement(null); + + // Re-entry guard: the recovery timer has not fired yet. + harness.guard.enforceModalFocus(); + assert.deepEqual(harness.focused(), ['preferred']); + + // Timer cleared the re-entry flag, but the debounce window still holds. + harness.runTimers(); + harness.guard.enforceModalFocus(); + assert.deepEqual(harness.focused(), ['preferred'], 'debounce still blocks the retry'); + + // Past the window, recovery resumes. + harness.advanceClock(200); + harness.setActiveElement(null); + harness.guard.enforceModalFocus(); + assert.deepEqual(harness.focused(), ['preferred', 'preferred']); + } finally { + harness.restore(); + } +}); + +test('modal focus guard asks the main window for focus off the modal layer only', () => { + const onModalLayer = createHarness({ isModalLayer: true }); + try { + onModalLayer.guard.requestOverlayFocus(); + assert.equal(onModalLayer.focusMainWindowCalls(), 0); + } finally { + onModalLayer.restore(); + } + + const onOverlayLayer = createHarness({ isModalLayer: false }); + try { + onOverlayLayer.guard.requestOverlayFocus(); + assert.equal(onOverlayLayer.focusMainWindowCalls(), 1); + } finally { + onOverlayLayer.restore(); + } +}); diff --git a/src/renderer/modals/modal-focus-guard.ts b/src/renderer/modals/modal-focus-guard.ts new file mode 100644 index 00000000..68ffa1b5 --- /dev/null +++ b/src/renderer/modals/modal-focus-guard.ts @@ -0,0 +1,141 @@ +/** + * Keeps focus inside an overlay-hosted modal. + * + * The overlay can lose focus to mpv or to the compositor while a modal is up, + * which leaves the modal visible but inert. Recovery is debounced (and guarded + * against re-entry) so a focus fight with the window manager cannot spin. + */ +export type ModalFocusGuardDeps = { + isOpen: () => boolean; + /** Modal root; focus inside it counts as "still in the modal". */ + getModalRoot: () => Element; + /** Preferred focus targets in order; the first rendered one wins. */ + getPreferredFocusTargets: () => HTMLElement[]; + /** Used when no preferred target is rendered, e.g. the close button. */ + getFallbackFocusTarget: () => Element | null; + /** Modal-layer windows own their focus; other layers ask the main window. */ + isModalLayer: boolean; +}; + +const FOCUS_RECOVERY_DEBOUNCE_MS = 120; + +export function createModalFocusGuard(deps: ModalFocusGuardDeps) { + let focusinGuard: ((event: FocusEvent) => void) | null = null; + let windowFocusGuard: (() => void) | null = null; + let pointerFocusGuard: ((event: Event) => void) | null = null; + let isRecovering = false; + let lastRecoveryAt = 0; + + function isModalFocusTarget(target: EventTarget | null): boolean { + return target instanceof Element && deps.getModalRoot().contains(target); + } + + function requestOverlayFocus(): void { + if (!deps.isModalLayer) { + // Best-effort: a rejected focus request must not surface as an unhandled + // rejection, since this runs from blur/focus handlers. + void Promise.resolve(window.electronAPI.focusMainWindow()).catch(() => {}); + } + } + + function focusFallbackTarget(): boolean { + requestOverlayFocus(); + + // getClientRects() rather than offsetParent: the latter is null for + // position:fixed elements, which would skip a perfectly visible target. + const preferred = deps + .getPreferredFocusTargets() + .find((target) => target.getClientRects().length > 0); + if (preferred) { + preferred.focus({ preventScroll: true }); + return document.activeElement === preferred; + } + + const fallback = deps.getFallbackFocusTarget(); + if (fallback instanceof HTMLElement) { + fallback.focus({ preventScroll: true }); + return document.activeElement === fallback; + } + + window.focus(); + return false; + } + + function enforceModalFocus(): void { + if (!deps.isOpen()) return; + if (isModalFocusTarget(document.activeElement)) return; + if (isRecovering) return; + + const now = Date.now(); + if (now - lastRecoveryAt < FOCUS_RECOVERY_DEBOUNCE_MS) return; + + isRecovering = true; + lastRecoveryAt = now; + focusFallbackTarget(); + window.setTimeout(() => { + isRecovering = false; + }, FOCUS_RECOVERY_DEBOUNCE_MS); + } + + /** Idempotent; safe to call on every open. */ + function attach(): void { + if (focusinGuard === null) { + focusinGuard = (event: FocusEvent) => { + if (!deps.isOpen()) return; + if (!isModalFocusTarget(event.target)) { + event.preventDefault(); + enforceModalFocus(); + } + }; + document.addEventListener('focusin', focusinGuard); + } + + if (pointerFocusGuard === null) { + pointerFocusGuard = () => { + requestOverlayFocus(); + enforceModalFocus(); + }; + const root = deps.getModalRoot(); + root.addEventListener('pointerdown', pointerFocusGuard); + root.addEventListener('click', pointerFocusGuard); + } + + if (windowFocusGuard === null) { + windowFocusGuard = () => { + requestOverlayFocus(); + enforceModalFocus(); + }; + window.addEventListener('blur', windowFocusGuard); + window.addEventListener('focus', windowFocusGuard); + } + } + + function detach(): void { + if (focusinGuard) { + document.removeEventListener('focusin', focusinGuard); + focusinGuard = null; + } + + if (pointerFocusGuard) { + const root = deps.getModalRoot(); + root.removeEventListener('pointerdown', pointerFocusGuard); + root.removeEventListener('click', pointerFocusGuard); + pointerFocusGuard = null; + } + + if (windowFocusGuard) { + window.removeEventListener('blur', windowFocusGuard); + window.removeEventListener('focus', windowFocusGuard); + windowFocusGuard = null; + } + } + + return { + attach, + detach, + enforceModalFocus, + focusFallbackTarget, + isModalFocusTarget, + requestOverlayFocus, + }; +} diff --git a/src/renderer/modals/session-help.ts b/src/renderer/modals/session-help.ts index 1a8321a7..b66fba6d 100644 --- a/src/renderer/modals/session-help.ts +++ b/src/renderer/modals/session-help.ts @@ -7,6 +7,7 @@ import { } from './session-help-sections'; import { createSessionHelpSectionNode } from './session-help-render'; import { buildVisibleSessionHelpSections, createSessionHelpTabBar } from './session-help-tabs'; +import { createModalFocusGuard } from './modal-focus-guard'; export { buildSessionHelpSections, @@ -69,11 +70,6 @@ export function createSessionHelpModal( let helpFilterValue = ''; let helpSections: SessionHelpSection[] = []; let activeTabId: SessionHelpTabId = 'essentials'; - let focusGuard: ((event: FocusEvent) => void) | null = null; - let windowFocusGuard: (() => void) | null = null; - let modalPointerFocusGuard: ((event: Event) => void) | null = null; - let isRecoveringModalFocus = false; - let lastFocusRecoveryAt = 0; function getItems(): HTMLButtonElement[] { return Array.from( @@ -102,47 +98,13 @@ export function createSessionHelpModal( }); } - function isSessionHelpModalFocusTarget(target: EventTarget | null): boolean { - return target instanceof Element && ctx.dom.sessionHelpModal.contains(target); - } - - function focusFallbackTarget(): boolean { - if (!ctx.platform.isModalLayer) { - void window.electronAPI.focusMainWindow(); - } - const items = getItems(); - const firstItem = items.find((item) => item.offsetParent !== null); - if (firstItem) { - firstItem.focus({ preventScroll: true }); - return document.activeElement === firstItem; - } - - if (ctx.dom.sessionHelpClose instanceof HTMLElement) { - ctx.dom.sessionHelpClose.focus({ preventScroll: true }); - return document.activeElement === ctx.dom.sessionHelpClose; - } - - window.focus(); - return false; - } - - function enforceModalFocus(): void { - if (!ctx.state.sessionHelpModalOpen) return; - if (!isSessionHelpModalFocusTarget(document.activeElement)) { - if (isRecoveringModalFocus) return; - - const now = Date.now(); - if (now - lastFocusRecoveryAt < 120) return; - - isRecoveringModalFocus = true; - lastFocusRecoveryAt = now; - focusFallbackTarget(); - - window.setTimeout(() => { - isRecoveringModalFocus = false; - }, 120); - } - } + const focus = createModalFocusGuard({ + isOpen: () => ctx.state.sessionHelpModalOpen, + getModalRoot: () => ctx.dom.sessionHelpModal, + getPreferredFocusTargets: () => getItems(), + getFallbackFocusTarget: () => ctx.dom.sessionHelpClose, + isModalLayer: ctx.platform.isModalLayer, + }); function isFilterInputFocused(): boolean { return document.activeElement === ctx.dom.sessionHelpFilter; @@ -192,48 +154,6 @@ export function createSessionHelpModal( setSelected(0); } - function requestOverlayFocus(): void { - if (!ctx.platform.isModalLayer) { - void window.electronAPI.focusMainWindow(); - } - } - - function addPointerFocusListener(): void { - if (modalPointerFocusGuard) return; - - modalPointerFocusGuard = () => { - requestOverlayFocus(); - enforceModalFocus(); - }; - ctx.dom.sessionHelpModal.addEventListener('pointerdown', modalPointerFocusGuard); - ctx.dom.sessionHelpModal.addEventListener('click', modalPointerFocusGuard); - } - - function removePointerFocusListener(): void { - if (!modalPointerFocusGuard) return; - ctx.dom.sessionHelpModal.removeEventListener('pointerdown', modalPointerFocusGuard); - ctx.dom.sessionHelpModal.removeEventListener('click', modalPointerFocusGuard); - modalPointerFocusGuard = null; - } - - function startFocusRecoveryGuards(): void { - if (windowFocusGuard) return; - - windowFocusGuard = () => { - requestOverlayFocus(); - enforceModalFocus(); - }; - window.addEventListener('blur', windowFocusGuard); - window.addEventListener('focus', windowFocusGuard); - } - - function stopFocusRecoveryGuards(): void { - if (!windowFocusGuard) return; - window.removeEventListener('blur', windowFocusGuard); - window.removeEventListener('focus', windowFocusGuard); - windowFocusGuard = null; - } - function showRenderError(message: string): void { helpSections = []; helpFilterValue = ''; @@ -310,22 +230,10 @@ export function createSessionHelpModal( } ctx.dom.sessionHelpStatus.textContent = 'Loading session help data...'; - if (focusGuard === null) { - focusGuard = (event: FocusEvent) => { - if (!ctx.state.sessionHelpModalOpen) return; - if (!isSessionHelpModalFocusTarget(event.target)) { - event.preventDefault(); - enforceModalFocus(); - } - }; - document.addEventListener('focusin', focusGuard); - } - - addPointerFocusListener(); - startFocusRecoveryGuards(); - requestOverlayFocus(); + focus.attach(); + focus.requestOverlayFocus(); window.focus(); - enforceModalFocus(); + focus.enforceModalFocus(); void render().then((dataLoaded) => { if (!ctx.state.sessionHelpModalOpen) return; @@ -353,12 +261,7 @@ export function createSessionHelpModal( ctx.dom.overlay.classList.remove('interactive'); } - if (focusGuard) { - document.removeEventListener('focusin', focusGuard); - focusGuard = null; - } - removePointerFocusListener(); - stopFocusRecoveryGuards(); + focus.detach(); if (priorFocus instanceof HTMLElement && priorFocus.isConnected) { priorFocus.focus({ preventScroll: true }); @@ -395,7 +298,7 @@ export function createSessionHelpModal( helpFilterValue = ''; ctx.dom.sessionHelpFilter.value = ''; applyFilterAndRender(); - focusFallbackTarget(); + focus.focusFallbackTarget(); return true; } return false; @@ -442,7 +345,7 @@ export function createSessionHelpModal( ctx.dom.sessionHelpFilter.addEventListener('keydown', (event: KeyboardEvent) => { if (event.key === 'Enter') { event.preventDefault(); - focusFallbackTarget(); + focus.focusFallbackTarget(); } });