From c658632e68830b5760f6f7a04c2a0bf31fc838cc Mon Sep 17 00:00:00 2001 From: shuoYun114 Date: Mon, 14 Sep 2026 22:02:52 +0800 Subject: [PATCH 1/3] feat(editor): support keyboard shortcuts for timeline start/end navigation (fixes #2120) --- apps/desktop/src/routes/editor/Player.tsx | 58 ++++++++ .../routes/editor/useEditorShortcuts.test.ts | 34 +++++ .../src/routes/editor/useEditorShortcuts.ts | 2 +- .../__tests__/unit/timeline-keyboard.test.ts | 132 ++++++++++++++++++ .../_components/timeline/TimelineView.tsx | 15 +- 5 files changed, 239 insertions(+), 2 deletions(-) create mode 100644 apps/desktop/src/routes/editor/useEditorShortcuts.test.ts create mode 100644 apps/web/__tests__/unit/timeline-keyboard.test.ts diff --git a/apps/desktop/src/routes/editor/Player.tsx b/apps/desktop/src/routes/editor/Player.tsx index 3ef4105cf0f..6a1db92719c 100644 --- a/apps/desktop/src/routes/editor/Player.tsx +++ b/apps/desktop/src/routes/editor/Player.tsx @@ -361,6 +361,64 @@ export function PlayerContent(props: { compactness?: number }) { await handlePlayPauseClick(); }, }, + { + combo: "ArrowUp", + handler: async () => { + if (editorState.playing) { + await commands.stopPlayback(); + setEditorState("playing", false); + } + setEditorState("playbackTime", 0); + setEditorState("previewTime", null); + if (!handoffPlaybackPending()) { + await commands.seekTo(0); + } + }, + }, + { + combo: "Home", + handler: async () => { + if (editorState.playing) { + await commands.stopPlayback(); + setEditorState("playing", false); + } + setEditorState("playbackTime", 0); + setEditorState("previewTime", null); + if (!handoffPlaybackPending()) { + await commands.seekTo(0); + } + }, + }, + { + combo: "ArrowDown", + handler: async () => { + if (editorState.playing) { + await commands.stopPlayback(); + setEditorState("playing", false); + } + const endTime = totalDuration(); + setEditorState("playbackTime", endTime); + setEditorState("previewTime", null); + if (!handoffPlaybackPending()) { + await commands.seekTo(Math.floor(endTime * FPS)); + } + }, + }, + { + combo: "End", + handler: async () => { + if (editorState.playing) { + await commands.stopPlayback(); + setEditorState("playing", false); + } + const endTime = totalDuration(); + setEditorState("playbackTime", endTime); + setEditorState("previewTime", null); + if (!handoffPlaybackPending()) { + await commands.seekTo(Math.floor(endTime * FPS)); + } + }, + }, ]); return ( diff --git a/apps/desktop/src/routes/editor/useEditorShortcuts.test.ts b/apps/desktop/src/routes/editor/useEditorShortcuts.test.ts new file mode 100644 index 00000000000..61455d0c744 --- /dev/null +++ b/apps/desktop/src/routes/editor/useEditorShortcuts.test.ts @@ -0,0 +1,34 @@ +import { describe, expect, it } from "vitest"; +import { normalizeCombo } from "./useEditorShortcuts"; + +function createKeyboardEvent( + code: string, + options: { metaKey?: boolean; ctrlKey?: boolean } = {}, +): KeyboardEvent { + return { + code, + metaKey: options.metaKey ?? false, + ctrlKey: options.ctrlKey ?? false, + } as KeyboardEvent; +} + +describe("useEditorShortcuts: normalizeCombo", () => { + it("normalizes navigation and boundary keys without modifiers", () => { + expect(normalizeCombo(createKeyboardEvent("ArrowUp"))).toBe("ArrowUp"); + expect(normalizeCombo(createKeyboardEvent("ArrowDown"))).toBe("ArrowDown"); + expect(normalizeCombo(createKeyboardEvent("Home"))).toBe("Home"); + expect(normalizeCombo(createKeyboardEvent("End"))).toBe("End"); + expect(normalizeCombo(createKeyboardEvent("Space"))).toBe("Space"); + }); + + it("strips Key prefix for standard letters", () => { + expect(normalizeCombo(createKeyboardEvent("KeyS"))).toBe("S"); + expect(normalizeCombo(createKeyboardEvent("KeyC"))).toBe("C"); + }); + + it("normalizes Mod modifier and special symbols", () => { + expect(normalizeCombo(createKeyboardEvent("Equal", { metaKey: true }))).toBe("Mod+="); + expect(normalizeCombo(createKeyboardEvent("Minus", { ctrlKey: true }))).toBe("Mod+-"); + expect(normalizeCombo(createKeyboardEvent("KeyS", { metaKey: true }))).toBe("Mod+S"); + }); +}); diff --git a/apps/desktop/src/routes/editor/useEditorShortcuts.ts b/apps/desktop/src/routes/editor/useEditorShortcuts.ts index 073ff0fbb17..62b475e9944 100644 --- a/apps/desktop/src/routes/editor/useEditorShortcuts.ts +++ b/apps/desktop/src/routes/editor/useEditorShortcuts.ts @@ -9,7 +9,7 @@ export type ShortcutBinding = { const isMod = (e: KeyboardEvent) => e.metaKey || e.ctrlKey; // treat Cmd/Ctrl as Mod -function normalizeCombo(e: KeyboardEvent): string { +export function normalizeCombo(e: KeyboardEvent): string { const parts: string[] = []; if (isMod(e)) parts.push("Mod"); diff --git a/apps/web/__tests__/unit/timeline-keyboard.test.ts b/apps/web/__tests__/unit/timeline-keyboard.test.ts new file mode 100644 index 00000000000..dfcecda4443 --- /dev/null +++ b/apps/web/__tests__/unit/timeline-keyboard.test.ts @@ -0,0 +1,132 @@ +import { describe, expect, it, vi } from "vitest"; + +type KeyHandler = (event: { + key: string; + target: unknown; + preventDefault: () => void; +}) => void; + +function createTimelineKeyHandler(playback: { + seek: (time: number) => void; + getCurrentTime: () => number; + getDuration: () => number; + getPlaying: () => boolean; + play: () => void; + pause: () => void; +}): KeyHandler { + const KEYBOARD_SEEK_STEP = 5; + + return (event) => { + const target = event.target as { + tagName?: string; + isContentEditable?: boolean; + closest?: (sel: string) => unknown; + } | null; + + if ( + target?.tagName === "INPUT" || + target?.tagName === "TEXTAREA" || + target?.isContentEditable + ) { + return; + } + + if (event.key === "ArrowLeft" || event.key === "ArrowRight") { + event.preventDefault(); + const delta = + event.key === "ArrowLeft" ? -KEYBOARD_SEEK_STEP : KEYBOARD_SEEK_STEP; + playback.seek(playback.getCurrentTime() + delta); + return; + } + + if (event.key === "ArrowUp" || event.key === "Home") { + event.preventDefault(); + playback.seek(0); + return; + } + + if (event.key === "ArrowDown" || event.key === "End") { + event.preventDefault(); + playback.seek(playback.getDuration()); + return; + } + + if (event.key === " " || event.key === "Spacebar") { + if (target?.closest?.("[data-timeline-node]")) return; + event.preventDefault(); + if (playback.getPlaying()) playback.pause(); + else playback.play(); + } + }; +} + +describe("timeline keyboard navigation", () => { + it("seeks to start (0) on ArrowUp and Home with preventDefault", () => { + const seek = vi.fn(); + const preventDefault = vi.fn(); + const playback = { + seek, + getCurrentTime: () => 45, + getDuration: () => 120, + getPlaying: () => false, + play: vi.fn(), + pause: vi.fn(), + }; + + const handler = createTimelineKeyHandler(playback); + + handler({ key: "ArrowUp", target: null, preventDefault }); + expect(preventDefault).toHaveBeenCalledTimes(1); + expect(seek).toHaveBeenCalledWith(0); + + handler({ key: "Home", target: null, preventDefault }); + expect(preventDefault).toHaveBeenCalledTimes(2); + expect(seek).toHaveBeenLastCalledWith(0); + }); + + it("seeks to end (duration) on ArrowDown and End with preventDefault", () => { + const seek = vi.fn(); + const preventDefault = vi.fn(); + const playback = { + seek, + getCurrentTime: () => 10, + getDuration: () => 120, + getPlaying: () => false, + play: vi.fn(), + pause: vi.fn(), + }; + + const handler = createTimelineKeyHandler(playback); + + handler({ key: "ArrowDown", target: null, preventDefault }); + expect(preventDefault).toHaveBeenCalledTimes(1); + expect(seek).toHaveBeenCalledWith(120); + + handler({ key: "End", target: null, preventDefault }); + expect(preventDefault).toHaveBeenCalledTimes(2); + expect(seek).toHaveBeenLastCalledWith(120); + }); + + it("does not navigate or prevent default when typing in inputs, textareas, or contenteditable", () => { + const seek = vi.fn(); + const preventDefault = vi.fn(); + const playback = { + seek, + getCurrentTime: () => 10, + getDuration: () => 120, + getPlaying: () => false, + play: vi.fn(), + pause: vi.fn(), + }; + + const handler = createTimelineKeyHandler(playback); + + handler({ key: "ArrowUp", target: { tagName: "INPUT" }, preventDefault }); + handler({ key: "ArrowDown", target: { tagName: "TEXTAREA" }, preventDefault }); + handler({ key: "Home", target: { isContentEditable: true }, preventDefault }); + handler({ key: "End", target: { isContentEditable: true }, preventDefault }); + + expect(seek).not.toHaveBeenCalled(); + expect(preventDefault).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/web/app/s/[videoId]/_components/timeline/TimelineView.tsx b/apps/web/app/s/[videoId]/_components/timeline/TimelineView.tsx index bbd40bcd27f..76ef105f842 100644 --- a/apps/web/app/s/[videoId]/_components/timeline/TimelineView.tsx +++ b/apps/web/app/s/[videoId]/_components/timeline/TimelineView.tsx @@ -411,7 +411,8 @@ function TimelineBand({ const target = event.target as HTMLElement | null; if ( target instanceof HTMLInputElement || - target instanceof HTMLTextAreaElement + target instanceof HTMLTextAreaElement || + target?.isContentEditable ) { return; } @@ -424,6 +425,18 @@ function TimelineBand({ return; } + if (event.key === "ArrowUp" || event.key === "Home") { + event.preventDefault(); + playback.seek(0); + return; + } + + if (event.key === "ArrowDown" || event.key === "End") { + event.preventDefault(); + playback.seek(playback.getDuration()); + return; + } + // Space over a branch node belongs to that button, not to playback. if (event.key === " " || event.key === "Spacebar") { if (target?.closest("[data-timeline-node]")) return; From 7774d07746e56b7925b944c1ea9ca49d0699e335 Mon Sep 17 00:00:00 2001 From: shuoYun114 Date: Mon, 14 Sep 2026 22:15:17 +0800 Subject: [PATCH 2/3] fix(editor): address adversarial audit feedback on modifiers, bounds, and focus guards --- apps/desktop/src/routes/editor/Player.tsx | 80 +++----- .../routes/editor/useEditorShortcuts.test.ts | 16 +- .../src/routes/editor/useEditorShortcuts.ts | 11 +- .../__tests__/unit/timeline-keyboard.test.ts | 182 ++++++------------ .../_components/timeline/TimelineView.tsx | 95 ++++++--- 5 files changed, 178 insertions(+), 206 deletions(-) diff --git a/apps/desktop/src/routes/editor/Player.tsx b/apps/desktop/src/routes/editor/Player.tsx index 6a1db92719c..3fe3abe2aa5 100644 --- a/apps/desktop/src/routes/editor/Player.tsx +++ b/apps/desktop/src/routes/editor/Player.tsx @@ -309,16 +309,38 @@ export function PlayerContent(props: { compactness?: number }) { ); } - // Register keyboard shortcuts in one place + const seekToBoundary = async (targetSeconds: number) => { + if (!Number.isFinite(targetSeconds) || targetSeconds < 0) return; + const targetFrame = Math.max(0, Math.floor(targetSeconds * FPS)); + try { + const pending = requestHandoffPlayback(false); + if (pending) await pending; + if (editorState.playing) { + await commands.stopPlayback(); + setEditorState("playing", false); + } + setEditorState("playbackTime", targetSeconds); + setEditorState("previewTime", null); + await commands.seekTo(targetFrame); + } catch (error) { + console.error("Failed to seek to boundary:", error); + setEditorState("playing", false); + } + }; + useEditorShortcuts(() => { - const el = document.activeElement; + const el = document.activeElement as HTMLElement | null; if (!el) return true; const tagName = el.tagName.toLowerCase(); - const isContentEditable = el.getAttribute("contenteditable") === "true"; + const role = el.getAttribute("role"); return !( tagName === "input" || tagName === "textarea" || - isContentEditable + tagName === "select" || + el.isContentEditable || + role === "slider" || + role === "listbox" || + role === "menu" ); }, [ { @@ -363,61 +385,19 @@ export function PlayerContent(props: { compactness?: number }) { }, { combo: "ArrowUp", - handler: async () => { - if (editorState.playing) { - await commands.stopPlayback(); - setEditorState("playing", false); - } - setEditorState("playbackTime", 0); - setEditorState("previewTime", null); - if (!handoffPlaybackPending()) { - await commands.seekTo(0); - } - }, + handler: () => seekToBoundary(0), }, { combo: "Home", - handler: async () => { - if (editorState.playing) { - await commands.stopPlayback(); - setEditorState("playing", false); - } - setEditorState("playbackTime", 0); - setEditorState("previewTime", null); - if (!handoffPlaybackPending()) { - await commands.seekTo(0); - } - }, + handler: () => seekToBoundary(0), }, { combo: "ArrowDown", - handler: async () => { - if (editorState.playing) { - await commands.stopPlayback(); - setEditorState("playing", false); - } - const endTime = totalDuration(); - setEditorState("playbackTime", endTime); - setEditorState("previewTime", null); - if (!handoffPlaybackPending()) { - await commands.seekTo(Math.floor(endTime * FPS)); - } - }, + handler: () => seekToBoundary(totalDuration()), }, { combo: "End", - handler: async () => { - if (editorState.playing) { - await commands.stopPlayback(); - setEditorState("playing", false); - } - const endTime = totalDuration(); - setEditorState("playbackTime", endTime); - setEditorState("previewTime", null); - if (!handoffPlaybackPending()) { - await commands.seekTo(Math.floor(endTime * FPS)); - } - }, + handler: () => seekToBoundary(totalDuration()), }, ]); diff --git a/apps/desktop/src/routes/editor/useEditorShortcuts.test.ts b/apps/desktop/src/routes/editor/useEditorShortcuts.test.ts index 61455d0c744..c3d02238101 100644 --- a/apps/desktop/src/routes/editor/useEditorShortcuts.test.ts +++ b/apps/desktop/src/routes/editor/useEditorShortcuts.test.ts @@ -3,12 +3,19 @@ import { normalizeCombo } from "./useEditorShortcuts"; function createKeyboardEvent( code: string, - options: { metaKey?: boolean; ctrlKey?: boolean } = {}, + options: { + metaKey?: boolean; + ctrlKey?: boolean; + altKey?: boolean; + shiftKey?: boolean; + } = {}, ): KeyboardEvent { return { code, metaKey: options.metaKey ?? false, ctrlKey: options.ctrlKey ?? false, + altKey: options.altKey ?? false, + shiftKey: options.shiftKey ?? false, } as KeyboardEvent; } @@ -31,4 +38,11 @@ describe("useEditorShortcuts: normalizeCombo", () => { expect(normalizeCombo(createKeyboardEvent("Minus", { ctrlKey: true }))).toBe("Mod+-"); expect(normalizeCombo(createKeyboardEvent("KeyS", { metaKey: true }))).toBe("Mod+S"); }); + + it("preserves Shift and Alt modifiers to avoid collision with bare navigation keys", () => { + expect(normalizeCombo(createKeyboardEvent("ArrowUp", { shiftKey: true }))).toBe("Shift+ArrowUp"); + expect(normalizeCombo(createKeyboardEvent("ArrowDown", { altKey: true }))).toBe("Alt+ArrowDown"); + expect(normalizeCombo(createKeyboardEvent("Home", { ctrlKey: true, shiftKey: true }))).toBe("Mod+Shift+Home"); + expect(normalizeCombo(createKeyboardEvent("KeyS", { altKey: true }))).toBe("Alt+S"); + }); }); diff --git a/apps/desktop/src/routes/editor/useEditorShortcuts.ts b/apps/desktop/src/routes/editor/useEditorShortcuts.ts index 62b475e9944..2c3e310b191 100644 --- a/apps/desktop/src/routes/editor/useEditorShortcuts.ts +++ b/apps/desktop/src/routes/editor/useEditorShortcuts.ts @@ -1,17 +1,19 @@ import { createEventListener } from "@solid-primitives/event-listener"; export type ShortcutBinding = { - combo: string; // e.g. "Mod+=", "Mod+-", "Space", "S", "C" + combo: string; handler: (e: KeyboardEvent) => void | Promise; - preventDefault?: boolean; // default: true - when?: () => boolean; // optional enablement gate + preventDefault?: boolean; + when?: () => boolean; }; -const isMod = (e: KeyboardEvent) => e.metaKey || e.ctrlKey; // treat Cmd/Ctrl as Mod +const isMod = (e: KeyboardEvent) => e.metaKey || e.ctrlKey; export function normalizeCombo(e: KeyboardEvent): string { const parts: string[] = []; if (isMod(e)) parts.push("Mod"); + if (e.altKey) parts.push("Alt"); + if (e.shiftKey) parts.push("Shift"); let key: string; switch (e.code) { @@ -38,7 +40,6 @@ export function useEditorShortcuts( ); createEventListener(document, "keydown", async (e: KeyboardEvent) => { - // Basic guards if (!getScopeActive()) return; if (e.repeat) return; diff --git a/apps/web/__tests__/unit/timeline-keyboard.test.ts b/apps/web/__tests__/unit/timeline-keyboard.test.ts index dfcecda4443..8fdbc684d2e 100644 --- a/apps/web/__tests__/unit/timeline-keyboard.test.ts +++ b/apps/web/__tests__/unit/timeline-keyboard.test.ts @@ -1,132 +1,68 @@ -import { describe, expect, it, vi } from "vitest"; - -type KeyHandler = (event: { - key: string; - target: unknown; - preventDefault: () => void; -}) => void; - -function createTimelineKeyHandler(playback: { - seek: (time: number) => void; - getCurrentTime: () => number; - getDuration: () => number; - getPlaying: () => boolean; - play: () => void; - pause: () => void; -}): KeyHandler { - const KEYBOARD_SEEK_STEP = 5; - - return (event) => { - const target = event.target as { - tagName?: string; - isContentEditable?: boolean; - closest?: (sel: string) => unknown; - } | null; - - if ( - target?.tagName === "INPUT" || - target?.tagName === "TEXTAREA" || - target?.isContentEditable - ) { - return; - } - - if (event.key === "ArrowLeft" || event.key === "ArrowRight") { - event.preventDefault(); - const delta = - event.key === "ArrowLeft" ? -KEYBOARD_SEEK_STEP : KEYBOARD_SEEK_STEP; - playback.seek(playback.getCurrentTime() + delta); - return; - } - - if (event.key === "ArrowUp" || event.key === "Home") { - event.preventDefault(); - playback.seek(0); - return; - } - - if (event.key === "ArrowDown" || event.key === "End") { - event.preventDefault(); - playback.seek(playback.getDuration()); - return; - } - - if (event.key === " " || event.key === "Spacebar") { - if (target?.closest?.("[data-timeline-node]")) return; - event.preventDefault(); - if (playback.getPlaying()) playback.pause(); - else playback.play(); - } - }; -} - -describe("timeline keyboard navigation", () => { - it("seeks to start (0) on ArrowUp and Home with preventDefault", () => { - const seek = vi.fn(); - const preventDefault = vi.fn(); - const playback = { - seek, - getCurrentTime: () => 45, - getDuration: () => 120, - getPlaying: () => false, - play: vi.fn(), - pause: vi.fn(), - }; - - const handler = createTimelineKeyHandler(playback); - - handler({ key: "ArrowUp", target: null, preventDefault }); - expect(preventDefault).toHaveBeenCalledTimes(1); - expect(seek).toHaveBeenCalledWith(0); - - handler({ key: "Home", target: null, preventDefault }); - expect(preventDefault).toHaveBeenCalledTimes(2); - expect(seek).toHaveBeenLastCalledWith(0); +import { describe, expect, it } from "vitest"; +import { resolveTimelineKeyAction } from "@/app/s/[videoId]/_components/timeline/TimelineView"; + +describe("resolveTimelineKeyAction", () => { + it("seeks to start (0) on bare ArrowUp and Home", () => { + expect(resolveTimelineKeyAction("ArrowUp", false, 120, false)).toEqual({ + type: "seekTo", + time: 0, + }); + expect(resolveTimelineKeyAction("Home", false, 120, false)).toEqual({ + type: "seekTo", + time: 0, + }); }); - it("seeks to end (duration) on ArrowDown and End with preventDefault", () => { - const seek = vi.fn(); - const preventDefault = vi.fn(); - const playback = { - seek, - getCurrentTime: () => 10, - getDuration: () => 120, - getPlaying: () => false, - play: vi.fn(), - pause: vi.fn(), - }; - - const handler = createTimelineKeyHandler(playback); - - handler({ key: "ArrowDown", target: null, preventDefault }); - expect(preventDefault).toHaveBeenCalledTimes(1); - expect(seek).toHaveBeenCalledWith(120); - - handler({ key: "End", target: null, preventDefault }); - expect(preventDefault).toHaveBeenCalledTimes(2); - expect(seek).toHaveBeenLastCalledWith(120); + it("seeks to end on bare ArrowDown and End", () => { + expect(resolveTimelineKeyAction("ArrowDown", false, 120, false)).toEqual({ + type: "seekTo", + time: 120, + }); + expect(resolveTimelineKeyAction("End", false, 120, false)).toEqual({ + type: "seekTo", + time: 120, + }); }); - it("does not navigate or prevent default when typing in inputs, textareas, or contenteditable", () => { - const seek = vi.fn(); - const preventDefault = vi.fn(); - const playback = { - seek, - getCurrentTime: () => 10, - getDuration: () => 120, - getPlaying: () => false, - play: vi.fn(), - pause: vi.fn(), - }; + it("handles non-finite or negative durations safely", () => { + expect(resolveTimelineKeyAction("ArrowDown", false, Number.NaN, false)).toEqual({ + type: "seekTo", + time: 0, + }); + expect(resolveTimelineKeyAction("End", false, -10, false)).toEqual({ + type: "seekTo", + time: 0, + }); + }); - const handler = createTimelineKeyHandler(playback); + it("seeks delta on ArrowLeft and ArrowRight", () => { + expect(resolveTimelineKeyAction("ArrowLeft", false, 120, false)).toEqual({ + type: "seekDelta", + delta: -5, + }); + expect(resolveTimelineKeyAction("ArrowRight", false, 120, false)).toEqual({ + type: "seekDelta", + delta: 5, + }); + }); - handler({ key: "ArrowUp", target: { tagName: "INPUT" }, preventDefault }); - handler({ key: "ArrowDown", target: { tagName: "TEXTAREA" }, preventDefault }); - handler({ key: "Home", target: { isContentEditable: true }, preventDefault }); - handler({ key: "End", target: { isContentEditable: true }, preventDefault }); + it("toggles play on Space unless focused over a branch node button", () => { + expect(resolveTimelineKeyAction(" ", false, 120, false)).toEqual({ + type: "togglePlay", + }); + expect(resolveTimelineKeyAction("Spacebar", false, 120, false)).toEqual({ + type: "togglePlay", + }); + expect(resolveTimelineKeyAction(" ", false, 120, true)).toBeNull(); + }); - expect(seek).not.toHaveBeenCalled(); - expect(preventDefault).not.toHaveBeenCalled(); + it("strictly ignores actions when any modifier is pressed", () => { + expect(resolveTimelineKeyAction("ArrowUp", true, 120, false)).toBeNull(); + expect(resolveTimelineKeyAction("ArrowDown", true, 120, false)).toBeNull(); + expect(resolveTimelineKeyAction("Home", true, 120, false)).toBeNull(); + expect(resolveTimelineKeyAction("End", true, 120, false)).toBeNull(); + expect(resolveTimelineKeyAction("ArrowLeft", true, 120, false)).toBeNull(); + expect(resolveTimelineKeyAction("ArrowRight", true, 120, false)).toBeNull(); + expect(resolveTimelineKeyAction(" ", true, 120, false)).toBeNull(); }); }); diff --git a/apps/web/app/s/[videoId]/_components/timeline/TimelineView.tsx b/apps/web/app/s/[videoId]/_components/timeline/TimelineView.tsx index 76ef105f842..a2e80d70229 100644 --- a/apps/web/app/s/[videoId]/_components/timeline/TimelineView.tsx +++ b/apps/web/app/s/[videoId]/_components/timeline/TimelineView.tsx @@ -59,6 +59,43 @@ const EMPTY_CHAPTERS: TimelineChapter[] = []; const WHEEL_ZOOM_RATE = 0.012; const WHEEL_ZOOM_MAX = 1.6; +export type TimelineKeyAction = + | { type: "seekDelta"; delta: number } + | { type: "seekTo"; time: number } + | { type: "togglePlay" } + | null; + +export function resolveTimelineKeyAction( + key: string, + hasModifier: boolean, + duration: number, + isNodeButton: boolean, +): TimelineKeyAction { + if (hasModifier) return null; + + if (key === "ArrowLeft" || key === "ArrowRight") { + return { + type: "seekDelta", + delta: key === "ArrowLeft" ? -KEYBOARD_SEEK_STEP : KEYBOARD_SEEK_STEP, + }; + } + + if (key === "ArrowUp" || key === "Home") { + return { type: "seekTo", time: 0 }; + } + + if (key === "ArrowDown" || key === "End") { + const safeDuration = Number.isFinite(duration) && duration >= 0 ? duration : 0; + return { type: "seekTo", time: safeDuration }; + } + + if ((key === " " || key === "Spacebar") && !isNodeButton) { + return { type: "togglePlay" }; + } + + return null; +} + export interface TimelineViewProps { comments: CommentType[]; videoId: Video.VideoId; @@ -409,40 +446,44 @@ function TimelineBand({ const handleKeyDown = useCallback( (event: React.KeyboardEvent) => { const target = event.target as HTMLElement | null; + const tagName = target?.tagName?.toLowerCase(); + const role = target?.getAttribute?.("role"); if ( - target instanceof HTMLInputElement || - target instanceof HTMLTextAreaElement || - target?.isContentEditable + tagName === "input" || + tagName === "textarea" || + tagName === "select" || + target?.isContentEditable || + role === "slider" || + role === "listbox" || + role === "menu" ) { return; } - if (event.key === "ArrowLeft" || event.key === "ArrowRight") { - event.preventDefault(); - const delta = - event.key === "ArrowLeft" ? -KEYBOARD_SEEK_STEP : KEYBOARD_SEEK_STEP; - playback.seek(playback.getCurrentTime() + delta); - return; - } + const hasModifier = + event.shiftKey || event.altKey || event.ctrlKey || event.metaKey; + const isNodeButton = Boolean(target?.closest("[data-timeline-node]")); + const action = resolveTimelineKeyAction( + event.key, + hasModifier, + playback.getDuration(), + isNodeButton, + ); - if (event.key === "ArrowUp" || event.key === "Home") { - event.preventDefault(); - playback.seek(0); - return; - } + if (!action) return; - if (event.key === "ArrowDown" || event.key === "End") { - event.preventDefault(); - playback.seek(playback.getDuration()); - return; - } - - // Space over a branch node belongs to that button, not to playback. - if (event.key === " " || event.key === "Spacebar") { - if (target?.closest("[data-timeline-node]")) return; - event.preventDefault(); - if (playback.getPlaying()) playback.pause(); - else playback.play(); + event.preventDefault(); + switch (action.type) { + case "seekDelta": + playback.seek(playback.getCurrentTime() + action.delta); + break; + case "seekTo": + playback.seek(action.time); + break; + case "togglePlay": + if (playback.getPlaying()) playback.pause(); + else playback.play(); + break; } }, [playback], From 239c68deea821ec88ca5229d852208f5754def62 Mon Sep 17 00:00:00 2001 From: shuoYun114 Date: Tue, 15 Sep 2026 06:37:47 +0800 Subject: [PATCH 3/3] fix(shortcuts): address review feedback on handoff seeking, overlay nudge, and slider a11y --- .../routes/editor/CanvasElementsOverlay.tsx | 1 + apps/desktop/src/routes/editor/Player.tsx | 91 ++++++++++--------- .../desktop/src/routes/editor/TextOverlay.tsx | 1 + ...s.test.ts => use-editor-shortcuts.test.ts} | 0 .../__tests__/unit/timeline-keyboard.test.ts | 84 +++++++++++++++-- .../_components/timeline/TimelineView.tsx | 57 +++++++++--- 6 files changed, 171 insertions(+), 63 deletions(-) rename apps/desktop/src/routes/editor/{useEditorShortcuts.test.ts => use-editor-shortcuts.test.ts} (100%) diff --git a/apps/desktop/src/routes/editor/CanvasElementsOverlay.tsx b/apps/desktop/src/routes/editor/CanvasElementsOverlay.tsx index 3b017e92ce4..d10b29b7db3 100644 --- a/apps/desktop/src/routes/editor/CanvasElementsOverlay.tsx +++ b/apps/desktop/src/routes/editor/CanvasElementsOverlay.tsx @@ -652,6 +652,7 @@ export function CanvasElementsOverlay(props: { size: Size }) { if (!rect) return; e.preventDefault(); + e.stopPropagation(); const px = e.shiftKey ? 10 : 1; const x = clamp( rect.x + (dir[0] * px) / props.size.width, diff --git a/apps/desktop/src/routes/editor/Player.tsx b/apps/desktop/src/routes/editor/Player.tsx index 3fe3abe2aa5..753601c2e38 100644 --- a/apps/desktop/src/routes/editor/Player.tsx +++ b/apps/desktop/src/routes/editor/Player.tsx @@ -309,25 +309,46 @@ export function PlayerContent(props: { compactness?: number }) { ); } - const seekToBoundary = async (targetSeconds: number) => { - if (!Number.isFinite(targetSeconds) || targetSeconds < 0) return; - const targetFrame = Math.max(0, Math.floor(targetSeconds * FPS)); - try { - const pending = requestHandoffPlayback(false); - if (pending) await pending; - if (editorState.playing) { - await commands.stopPlayback(); - setEditorState("playing", false); - } - setEditorState("playbackTime", targetSeconds); - setEditorState("previewTime", null); - await commands.seekTo(targetFrame); - } catch (error) { - console.error("Failed to seek to boundary:", error); - setEditorState("playing", false); + const seekToStart = async () => { + const pending = requestHandoffPlayback(false, 0); + if (pending) { + editorState.timeline.transform.setPosition(0); + await pending; + return; + } + await commands.stopPlayback(); + setEditorState("playing", false); + setEditorState("playbackTime", 0); + setEditorState("previewTime", null); + editorState.timeline.transform.setPosition(0); + if (!handoffPlaybackPending()) { + await commands.seekTo(0); } }; + const seekToEnd = async () => { + const total = totalDuration(); + if (!Number.isFinite(total) || total < 0) return; + const pending = requestHandoffPlayback(false, total); + if (pending) { + await pending; + return; + } + await commands.stopPlayback(); + setEditorState("playing", false); + setEditorState("playbackTime", total); + setEditorState("previewTime", null); + if (!handoffPlaybackPending()) { + await commands.seekTo(Math.floor(total * FPS)); + } + }; + + const hasActiveOverlayNudge = () => { + if (editorState.canvasSelection) return true; + const selType = editorState.timeline.selection?.type; + return selType === "text" || selType === "image"; + }; + useEditorShortcuts(() => { const el = document.activeElement as HTMLElement | null; if (!el) return true; @@ -385,19 +406,25 @@ export function PlayerContent(props: { compactness?: number }) { }, { combo: "ArrowUp", - handler: () => seekToBoundary(0), + handler: () => { + if (hasActiveOverlayNudge()) return; + void seekToStart(); + }, }, { combo: "Home", - handler: () => seekToBoundary(0), + handler: () => void seekToStart(), }, { combo: "ArrowDown", - handler: () => seekToBoundary(totalDuration()), + handler: () => { + if (hasActiveOverlayNudge()) return; + void seekToEnd(); + }, }, { combo: "End", - handler: () => seekToBoundary(totalDuration()), + handler: () => void seekToEnd(), }, ]); @@ -486,18 +513,7 @@ export function PlayerContent(props: { compactness?: number }) { @@ -517,16 +533,7 @@ export function PlayerContent(props: { compactness?: number }) { diff --git a/apps/desktop/src/routes/editor/TextOverlay.tsx b/apps/desktop/src/routes/editor/TextOverlay.tsx index 359b285eb6f..54948f5bf27 100644 --- a/apps/desktop/src/routes/editor/TextOverlay.tsx +++ b/apps/desktop/src/routes/editor/TextOverlay.tsx @@ -213,6 +213,7 @@ export function TextOverlay(props: TextOverlayProps) { if (!dir) return; e.preventDefault(); + e.stopPropagation(); const px = e.shiftKey ? 10 : 1; updateSegmentByIndex(index, (s) => { s.center.x = clamp( diff --git a/apps/desktop/src/routes/editor/useEditorShortcuts.test.ts b/apps/desktop/src/routes/editor/use-editor-shortcuts.test.ts similarity index 100% rename from apps/desktop/src/routes/editor/useEditorShortcuts.test.ts rename to apps/desktop/src/routes/editor/use-editor-shortcuts.test.ts diff --git a/apps/web/__tests__/unit/timeline-keyboard.test.ts b/apps/web/__tests__/unit/timeline-keyboard.test.ts index 8fdbc684d2e..8ea3d9939f0 100644 --- a/apps/web/__tests__/unit/timeline-keyboard.test.ts +++ b/apps/web/__tests__/unit/timeline-keyboard.test.ts @@ -1,35 +1,70 @@ import { describe, expect, it } from "vitest"; -import { resolveTimelineKeyAction } from "@/app/s/[videoId]/_components/timeline/TimelineView"; +import { + isIgnoredTimelineKeyboardTarget, + resolveTimelineKeyAction, +} from "@/app/s/[videoId]/_components/timeline/TimelineView"; describe("resolveTimelineKeyAction", () => { - it("seeks to start (0) on bare ArrowUp and Home", () => { - expect(resolveTimelineKeyAction("ArrowUp", false, 120, false)).toEqual({ + it("seeks to start (0) on bare ArrowUp and Home when not focused on slider", () => { + expect(resolveTimelineKeyAction("ArrowUp", false, 120, false, false)).toEqual({ type: "seekTo", time: 0, }); - expect(resolveTimelineKeyAction("Home", false, 120, false)).toEqual({ + expect(resolveTimelineKeyAction("Home", false, 120, false, false)).toEqual({ type: "seekTo", time: 0, }); }); - it("seeks to end on bare ArrowDown and End", () => { - expect(resolveTimelineKeyAction("ArrowDown", false, 120, false)).toEqual({ + it("seeks to end on bare ArrowDown and End when not focused on slider", () => { + expect(resolveTimelineKeyAction("ArrowDown", false, 120, false, false)).toEqual({ type: "seekTo", time: 120, }); - expect(resolveTimelineKeyAction("End", false, 120, false)).toEqual({ + expect(resolveTimelineKeyAction("End", false, 120, false, false)).toEqual({ + type: "seekTo", + time: 120, + }); + }); + + it("steps incrementally on ArrowUp/Down/Left/Right when focused on ARIA slider per accessibility guidelines", () => { + // ArrowUp increments slider + expect(resolveTimelineKeyAction("ArrowUp", false, 120, false, true)).toEqual({ + type: "seekDelta", + delta: 5, + }); + // ArrowDown decrements slider + expect(resolveTimelineKeyAction("ArrowDown", false, 120, false, true)).toEqual({ + type: "seekDelta", + delta: -5, + }); + // ArrowRight increments slider + expect(resolveTimelineKeyAction("ArrowRight", false, 120, false, true)).toEqual({ + type: "seekDelta", + delta: 5, + }); + // ArrowLeft decrements slider + expect(resolveTimelineKeyAction("ArrowLeft", false, 120, false, true)).toEqual({ + type: "seekDelta", + delta: -5, + }); + // Home/End on slider still jump to extremes + expect(resolveTimelineKeyAction("Home", false, 120, false, true)).toEqual({ + type: "seekTo", + time: 0, + }); + expect(resolveTimelineKeyAction("End", false, 120, false, true)).toEqual({ type: "seekTo", time: 120, }); }); it("handles non-finite or negative durations safely", () => { - expect(resolveTimelineKeyAction("ArrowDown", false, Number.NaN, false)).toEqual({ + expect(resolveTimelineKeyAction("ArrowDown", false, Number.NaN, false, false)).toEqual({ type: "seekTo", time: 0, }); - expect(resolveTimelineKeyAction("End", false, -10, false)).toEqual({ + expect(resolveTimelineKeyAction("End", false, -10, false, false)).toEqual({ type: "seekTo", time: 0, }); @@ -66,3 +101,34 @@ describe("resolveTimelineKeyAction", () => { expect(resolveTimelineKeyAction(" ", true, 120, false)).toBeNull(); }); }); + +describe("isIgnoredTimelineKeyboardTarget", () => { + it("returns false for null target or generic divs", () => { + expect(isIgnoredTimelineKeyboardTarget(null)).toBe(false); + const div = document.createElement("div"); + expect(isIgnoredTimelineKeyboardTarget(div)).toBe(false); + }); + + it("returns true for input, textarea, select, contenteditable, and listbox/menu roles", () => { + const input = document.createElement("input"); + expect(isIgnoredTimelineKeyboardTarget(input)).toBe(true); + + const textarea = document.createElement("textarea"); + expect(isIgnoredTimelineKeyboardTarget(textarea)).toBe(true); + + const select = document.createElement("select"); + expect(isIgnoredTimelineKeyboardTarget(select)).toBe(true); + + const editable = document.createElement("div"); + editable.contentEditable = "true"; + expect(isIgnoredTimelineKeyboardTarget(editable)).toBe(true); + + const listbox = document.createElement("div"); + listbox.setAttribute("role", "listbox"); + expect(isIgnoredTimelineKeyboardTarget(listbox)).toBe(true); + + const menu = document.createElement("div"); + menu.setAttribute("role", "menu"); + expect(isIgnoredTimelineKeyboardTarget(menu)).toBe(true); + }); +}); diff --git a/apps/web/app/s/[videoId]/_components/timeline/TimelineView.tsx b/apps/web/app/s/[videoId]/_components/timeline/TimelineView.tsx index a2e80d70229..27ba95dab40 100644 --- a/apps/web/app/s/[videoId]/_components/timeline/TimelineView.tsx +++ b/apps/web/app/s/[videoId]/_components/timeline/TimelineView.tsx @@ -65,14 +65,52 @@ export type TimelineKeyAction = | { type: "togglePlay" } | null; +export function isIgnoredTimelineKeyboardTarget( + target: HTMLElement | null, +): boolean { + if (!target) return false; + const tagName = target.tagName?.toLowerCase(); + const role = target.getAttribute?.("role"); + return ( + tagName === "input" || + tagName === "textarea" || + tagName === "select" || + target.isContentEditable || + role === "listbox" || + role === "menu" + ); +} + export function resolveTimelineKeyAction( key: string, hasModifier: boolean, duration: number, isNodeButton: boolean, + isSlider = false, ): TimelineKeyAction { if (hasModifier) return null; + if (isSlider) { + if (key === "ArrowRight" || key === "ArrowUp") { + return { type: "seekDelta", delta: KEYBOARD_SEEK_STEP }; + } + if (key === "ArrowLeft" || key === "ArrowDown") { + return { type: "seekDelta", delta: -KEYBOARD_SEEK_STEP }; + } + if (key === "Home") { + return { type: "seekTo", time: 0 }; + } + if (key === "End") { + const safeDuration = + Number.isFinite(duration) && duration >= 0 ? duration : 0; + return { type: "seekTo", time: safeDuration }; + } + if (key === " " || key === "Spacebar") { + return { type: "togglePlay" }; + } + return null; + } + if (key === "ArrowLeft" || key === "ArrowRight") { return { type: "seekDelta", @@ -85,7 +123,8 @@ export function resolveTimelineKeyAction( } if (key === "ArrowDown" || key === "End") { - const safeDuration = Number.isFinite(duration) && duration >= 0 ? duration : 0; + const safeDuration = + Number.isFinite(duration) && duration >= 0 ? duration : 0; return { type: "seekTo", time: safeDuration }; } @@ -446,20 +485,13 @@ function TimelineBand({ const handleKeyDown = useCallback( (event: React.KeyboardEvent) => { const target = event.target as HTMLElement | null; - const tagName = target?.tagName?.toLowerCase(); - const role = target?.getAttribute?.("role"); - if ( - tagName === "input" || - tagName === "textarea" || - tagName === "select" || - target?.isContentEditable || - role === "slider" || - role === "listbox" || - role === "menu" - ) { + if (isIgnoredTimelineKeyboardTarget(target)) { return; } + const role = target?.getAttribute?.("role"); + const isSlider = + role === "slider" || Boolean(target?.closest?.("[data-timeline-rail]")); const hasModifier = event.shiftKey || event.altKey || event.ctrlKey || event.metaKey; const isNodeButton = Boolean(target?.closest("[data-timeline-node]")); @@ -468,6 +500,7 @@ function TimelineBand({ hasModifier, playback.getDuration(), isNodeButton, + isSlider, ); if (!action) return;