Repository navigation
feat(editor): support keyboard shortcuts for timeline start/end navigation (fixes #2120) #2292
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c658632
7774d07
239c68d
a6b54dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The new Context Used: CLAUDE.md (source) Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/desktop/src/routes/editor/useEditorShortcuts.test.ts
Line: 1
Comment:
**Test Filename Violates Convention**
The new `useEditorShortcuts.test.ts` filename violates the repository directive that TypeScript files use kebab-case names. Rename it to `use-editor-shortcuts.test.ts`; this repository requirement must be satisfied before merging.
**Context Used:** CLAUDE.md ([source](https://github.com/capsoftware/cap/blob/main/CLAUDE.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
| import { normalizeCombo } from "./useEditorShortcuts"; | ||
|
|
||
| function createKeyboardEvent( | ||
| code: string, | ||
| 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; | ||
| } | ||
|
|
||
| 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"); | ||
| }); | ||
|
|
||
| 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"); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<void>; | ||
| 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; | ||
|
|
||
| function normalizeCombo(e: KeyboardEvent): string { | ||
| 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"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/desktop/src/routes/editor/useEditorShortcuts.ts
Line: 16
Comment:
**Plus no longer zooms**
Adding `Shift` to every normalized combination breaks zoom-in when users press Ctrl/Cmd and `+` (`Shift+Equal`). That now becomes `Mod+Shift+=`, but `Player` only binds `Mod+=`, so the shortcut does nothing. Keep a shifted alias for zoom-in while retaining the modifier guards for navigation keys.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. |
||
|
|
||
| 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; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,134 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { | ||
| isIgnoredTimelineKeyboardTarget, | ||
| resolveTimelineKeyAction, | ||
| } from "@/app/s/[videoId]/_components/timeline/TimelineView"; | ||
|
|
||
| describe("resolveTimelineKeyAction", () => { | ||
| 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, false)).toEqual({ | ||
| type: "seekTo", | ||
| time: 0, | ||
| }); | ||
| }); | ||
|
|
||
| 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, 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new comments narrate the assertions, such as “ArrowUp increments slider.” The repository requires comments to explain non-obvious context, not restate the code. Remove these comments; the same pattern appears before the other arrow and Home/End assertions. This repository requirement must be satisfied before merging. Context Used: AGENTS.md (source) Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/web/__tests__/unit/timeline-keyboard.test.ts
Line: 31
Comment:
**Comments repeat the tests**
The new comments narrate the assertions, such as “ArrowUp increments slider.” The repository requires comments to explain non-obvious context, not restate the code. Remove these comments; the same pattern appears before the other arrow and Home/End assertions. This repository requirement must be satisfied before merging.
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
| 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, false)).toEqual({ | ||
| type: "seekTo", | ||
| time: 0, | ||
| }); | ||
| expect(resolveTimelineKeyAction("End", false, -10, false, false)).toEqual({ | ||
| type: "seekTo", | ||
| time: 0, | ||
| }); | ||
| }); | ||
|
|
||
| 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, | ||
| }); | ||
| }); | ||
|
|
||
| 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(); | ||
| }); | ||
|
|
||
| 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(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("isIgnoredTimelineKeyboardTarget", () => { | ||
| it("returns false for null target or generic divs", () => { | ||
| expect(isIgnoredTimelineKeyboardTarget(null)).toBe(false); | ||
| const div = document.createElement("div"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new tests call Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/web/__tests__/unit/timeline-keyboard.test.ts
Line: 108
Comment:
**New tests lack a DOM**
The new tests call `document.createElement`, but Vitest defaults to `environment: "node"` and the shared setup provides no DOM. Both target-guard tests will fail with `document is not defined`, preventing the web test suite from passing. Give this test file a DOM-capable environment.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. |
||
| 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); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new document-level ArrowUp and ArrowDown shortcuts also run when an editor overlay is selected. Existing canvas and text overlay handlers use the same events to nudge the selected element without stopping propagation, so one keypress now both moves the element and seeks the playhead to the start or end. Suppress the timeline shortcut while an overlay owns the arrow keys, or otherwise make these actions mutually exclusive.
Prompt To Fix With AI