Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions apps/desktop/src/routes/editor/CanvasElementsOverlay.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
105 changes: 79 additions & 26 deletions apps/desktop/src/routes/editor/Player.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -309,16 +309,59 @@ export function PlayerContent(props: { compactness?: number }) {
);
}

// Register keyboard shortcuts in one place
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;
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"
);
}, [
{
Expand All @@ -338,6 +381,14 @@ export function PlayerContent(props: { compactness?: number }) {
editorState.playbackTime,
),
},
{
combo: "Mod+Shift+=",
handler: () =>
editorState.timeline.transform.updateZoom(
editorState.timeline.transform.zoom / 1.1,
editorState.playbackTime,
),
},
{
combo: "Mod+-",
handler: () =>
Expand All @@ -361,6 +412,28 @@ export function PlayerContent(props: { compactness?: number }) {
await handlePlayPauseClick();
},
},
{
combo: "ArrowUp",
handler: () => {
if (hasActiveOverlayNudge()) return;
void seekToStart();
},
},
{
combo: "Home",
handler: () => void seekToStart(),
},
{
combo: "ArrowDown",
handler: () => {
if (hasActiveOverlayNudge()) return;
void seekToEnd();
},
},
{
combo: "End",
handler: () => void seekToEnd(),
},
]);

return (
Expand Down Expand Up @@ -448,18 +521,7 @@ export function PlayerContent(props: { compactness?: number }) {
<button
type="button"
class="text-ed-text-2 transition-opacity hover:opacity-70 will-change-[opacity]"
onClick={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);
editorState.timeline.transform.setPosition(0);
}}
onClick={seekToStart}
>
<IconCapPrev class="size-3.5" />
</button>
Expand All @@ -479,16 +541,7 @@ export function PlayerContent(props: { compactness?: number }) {
<button
type="button"
class="text-ed-text-2 transition-opacity hover:opacity-70 will-change-[opacity]"
onClick={async () => {
const pending = requestHandoffPlayback(false, totalDuration());
if (pending) {
await pending;
return;
}
await commands.stopPlayback();
setEditorState("playing", false);
setEditorState("playbackTime", totalDuration());
}}
onClick={seekToEnd}
>
<IconCapNext class="size-3.5" />
</button>
Expand Down
1 change: 1 addition & 0 deletions apps/desktop/src/routes/editor/TextOverlay.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
49 changes: 49 additions & 0 deletions apps/desktop/src/routes/editor/use-editor-shortcuts.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { describe, expect, it } from "vitest";
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");
Comment on lines +23 to +28

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Shortcut behavior goes untested

These tests only check normalizeCombo; they never dispatch a key through useEditorShortcuts or exercise the new handlers in PlayerContent. They would still pass if the boundary bindings were removed or the handoff and overlay guards broke. Add event-level tests for boundary seeking, pending handoff, and selected-overlay nudging.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/routes/editor/use-editor-shortcuts.test.ts
Line: 23-28

Comment:
**Shortcut behavior goes untested**

These tests only check `normalizeCombo`; they never dispatch a key through `useEditorShortcuts` or exercise the new handlers in `PlayerContent`. They would still pass if the boundary bindings were removed or the handoff and overlay guards broke. Add event-level tests for boundary seeking, pending handoff, and selected-overlay nudging.

---

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!

});

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("Equal", { metaKey: true, shiftKey: true }))).toBe("Mod+Shift+=");
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");
});
});
13 changes: 7 additions & 6 deletions apps/desktop/src/routes/editor/useEditorShortcuts.ts
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");

let key: string;
switch (e.code) {
Expand All @@ -38,7 +40,6 @@ export function useEditorShortcuts(
);

createEventListener(document, "keydown", async (e: KeyboardEvent) => {
// Basic guards
if (!getScopeActive()) return;
if (e.repeat) return;

Expand Down
130 changes: 130 additions & 0 deletions apps/web/__tests__/unit/timeline-keyboard.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
// @vitest-environment jsdom
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", () => {
expect(resolveTimelineKeyAction("ArrowUp", false, 120, false, true)).toEqual({
type: "seekDelta",
delta: 5,
});
expect(resolveTimelineKeyAction("ArrowDown", false, 120, false, true)).toEqual({
type: "seekDelta",
delta: -5,
});
expect(resolveTimelineKeyAction("ArrowRight", false, 120, false, true)).toEqual({
type: "seekDelta",
delta: 5,
});
expect(resolveTimelineKeyAction("ArrowLeft", false, 120, false, true)).toEqual({
type: "seekDelta",
delta: -5,
});
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");
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);
});
});
Loading