Skip to content

[7/10] fix: let keyframed clips take effects and transitions in the toolbar #255

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.

Sign up for GitHub

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

Merged
dazzatronus merged 1 commit into from
Oct 26, 2026
Merged

[7/10] fix: let keyframed clips take effects and transitions in the toolbar #255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 0 additions & 18 deletions src/core/shared/clip-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,3 @@ export function stripInternalProperties(clip: Clip): Clip {
export function isKeyframedValue(value: unknown): boolean {
return Boolean(value) && typeof value !== "number";
}

/**
* True when any visual property is keyframed or bound.
*
* The toolbars keep effects and transitions off such clips, so they must agree on
* the property list; keep this the only copy.
*/
export function hasKeyframedVisualProperty(clip: Clip): boolean {
return [
clip.opacity,
clip.scale,
clip.offset?.x,
clip.offset?.y,
clip.transform?.rotate?.angle,
clip.transform?.skew?.x,
clip.transform?.skew?.y
].some(isKeyframedValue);
}
49 changes: 3 additions & 46 deletions src/core/ui/media-toolbar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import {
} from "@core/animations/opacity-keyframes";
import type { Edit } from "@core/edit-session";
import { EditEvent } from "@core/events/edit-events";
import { hasKeyframedVisualProperty } from "@core/shared/clip-utils";
import { validateAssetUrl } from "@core/shared/utils";
import { ShotstackEdit } from "@core/shotstack-edit";
import type { ResolvedClip } from "@schemas";
Expand Down Expand Up @@ -949,20 +948,16 @@ export class MediaToolbar extends BaseToolbar {
const fps = this.edit.getOutputFps();
const currentPoint = localTime === null ? undefined : findOpacityPoint(visiblePoints, localTime, fps);

const hasEffect = Boolean(clip.effect);
const hasTransition = Boolean(clip.transition?.in || clip.transition?.out);
const hasPreset = hasEffect || hasTransition;
const animated = Array.isArray(clip.opacity) || (points?.length ?? 0) > 0;
let disabledReason = "";
if (!editable) disabledReason = "This opacity animation can be previewed but not edited";
else if (isBound) disabledReason = "Remove the merge field before keyframing opacity";
else if (hasPreset) disabledReason = "Remove the clip effect or transition before keyframing opacity";
else if (localTime === null) disabledReason = "Move the playhead over the clip to edit opacity keyframes";
else if (currentPoint && points?.length === 2 && points.some(point => point !== currentPoint && point.time > clip.length + TIME_EPSILON)) {
disabledReason = "Extend the clip before removing this keyframe";
}

this.opacitySlider?.setEnabled(!isBound && editable && (!animated || (!hasPreset && localTime !== null)));
this.opacitySlider?.setEnabled(!isBound && editable && (!animated || localTime !== null));

if (this.opacityKeyframeBtn) {
let state: keyof typeof KEYFRAME_BUTTON_STATES = "static";
Expand All @@ -979,23 +974,6 @@ export class MediaToolbar extends BaseToolbar {
const navigationTime = this.edit.playbackTime - clip.start;
if (this.opacityPreviousKeyframeBtn) this.opacityPreviousKeyframeBtn.disabled = !findOpacityPoint(visiblePoints, navigationTime, fps, -1);
if (this.opacityNextKeyframeBtn) this.opacityNextKeyframeBtn.disabled = !findOpacityPoint(visiblePoints, navigationTime, fps, 1);

// Only committed keyframes gate presets. An armed-but-unwritten point has no
// document presence, so gating on it would disable controls with nothing to undo.
const hasVisualKeyframes = hasKeyframedVisualProperty(clip);
this.setPresetButtonEnabled(this.effectBtn, !(hasVisualKeyframes && !hasEffect), "Effects can't be combined with keyframes in the editor");
this.setPresetButtonEnabled(
this.transitionBtn,
!(hasVisualKeyframes && !hasTransition),
"Transitions can't be combined with keyframes in the editor"
);
}

private setPresetButtonEnabled(button: HTMLButtonElement | null, enabled: boolean, disabledReason: string): void {
if (!button) return;
Object.assign(button, { disabled: !enabled, title: enabled ? "" : disabledReason });
if (enabled) button.removeAttribute("aria-label");
else button.setAttribute("aria-label", disabledReason);
}

// ─── Two-Phase Drag Helpers ──────────────────────────────────────────────────
Expand Down Expand Up @@ -1076,7 +1054,7 @@ export class MediaToolbar extends BaseToolbar {
let opacity: ResolvedClip["opacity"] = nextValue;
if (points.length > 0) {
const localTime = this.opacityDragTime ?? this.getOpacityTime(clip);
if (localTime === null || clip.effect || clip.transition?.in || clip.transition?.out) {
if (localTime === null) {
this.syncOpacityState(clip);
return;
}
Expand Down Expand Up @@ -1107,14 +1085,7 @@ export class MediaToolbar extends BaseToolbar {

const points = this.getOpacityPoints(clip, clipId);
const localTime = this.getOpacityTime(clip);
if (
points === null ||
localTime === null ||
this.getShotstackEdit()?.getMergeFieldForProperty(clipId, "opacity") ||
clip.effect ||
clip.transition?.in ||
clip.transition?.out
) {
if (points === null || localTime === null || this.getShotstackEdit()?.getMergeFieldForProperty(clipId, "opacity")) {
this.syncOpacityState(clip);
return;
}
Expand Down Expand Up @@ -1329,27 +1300,13 @@ export class MediaToolbar extends BaseToolbar {

private applyTransitionUpdate(): void {
const transition = this.transitionPanel?.getClipValue();
const clip = this.edit.getResolvedClip(this.selectedTrackIdx, this.selectedClipIdx);
if (clip && hasKeyframedVisualProperty(clip)) {
const changesIn = Boolean(transition?.in && transition.in !== clip.transition?.in);
const changesOut = Boolean(transition?.out && transition.out !== clip.transition?.out);
if (changesIn || changesOut) {
this.transitionPanel?.setFromClip(clip.transition);
return;
}
}
this.applyClipUpdate({ transition });
}

// ─── Effect (using composite) ────────────────────────────────────────────────

private applyEffect(): void {
const effectValue = this.effectPanel?.getClipValue();
const clip = this.edit.getResolvedClip(this.selectedTrackIdx, this.selectedClipIdx);
if (effectValue && clip && hasKeyframedVisualProperty(clip)) {
this.effectPanel?.setFromClip(clip.effect);
return;
}
this.applyClipUpdate({ effect: effectValue });
}

Expand Down
32 changes: 1 addition & 31 deletions tests/clip-utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
import { hasKeyframedVisualProperty, isKeyframedValue } from "@core/shared/clip-utils";
import type { Clip } from "@schemas";
import { isKeyframedValue } from "@core/shared/clip-utils";

const tween = [{ from: 0, to: 1, start: 0, length: 1, interpolation: "linear" as const }];

function clip(overrides: Partial<Clip> = {}): Clip {
return { asset: { type: "image", src: "https://example.com/a.jpg" }, start: 0, length: 5, ...overrides } as Clip;
}

describe("isKeyframedValue", () => {
it("accepts plain numbers as editable, including zero", () => {
expect(isKeyframedValue(0)).toBe(false);
Expand All @@ -19,28 +14,3 @@ describe("isKeyframedValue", () => {
expect(isKeyframedValue("{{ MEDIA_OPACITY }}")).toBe(true);
});
});

describe("hasKeyframedVisualProperty", () => {
it("is false for a clip whose visual properties are all fixed or absent", () => {
expect(hasKeyframedVisualProperty(clip())).toBe(false);
expect(hasKeyframedVisualProperty(clip({ opacity: 0, scale: 1 }))).toBe(false);
});

it.each([
["opacity", { opacity: tween }],
["scale", { scale: tween }],
["offset.x", { offset: { x: tween } }],
["offset.y", { offset: { y: tween } }],
["rotation", { transform: { rotate: { angle: tween } } }],
["skew.x", { transform: { skew: { x: tween } } }],
["skew.y", { transform: { skew: { y: tween } } }]
])("is true when %s is keyframed", (_name, overrides) => {
expect(hasKeyframedVisualProperty(clip(overrides as Partial<Clip>))).toBe(true);
});

it("ignores volume, which is asset-level and takes no part in preset composition", () => {
expect(hasKeyframedVisualProperty(clip({ asset: { type: "video", src: "https://example.com/a.mp4", volume: tween } } as Partial<Clip>))).toBe(
false
);
});
});
51 changes: 44 additions & 7 deletions tests/media-toolbar.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,27 +508,64 @@ describe("MediaToolbar", () => {
toolbar.dispose();
});

it("blocks effects and transitions while opacity is animated", () => {
it("offers effects and transitions while opacity is animated", () => {
const mockEdit = createMockEditSession();
mockEdit.getResolvedClip.mockReturnValue(createImageClip({ opacity: opacityTweens }));

const { toolbar, parent } = mountToolbar(mockEdit);
toolbar.show(0, 0);

expect((parent.querySelector('[data-action="effect"]') as HTMLButtonElement).disabled).toBe(true);
expect((parent.querySelector('[data-action="transition"]') as HTMLButtonElement).disabled).toBe(true);
expect((parent.querySelector('[data-action="effect"]') as HTMLButtonElement).disabled).toBe(false);
expect((parent.querySelector('[data-action="transition"]') as HTMLButtonElement).disabled).toBe(false);
toolbar.dispose();
});

it("adds an effect and a transition to a clip with animated opacity", () => {
const mockEdit = createMockEditSession();
mockEdit.getResolvedClip.mockReturnValue(createImageClip({ opacity: opacityTweens }));

const { toolbar, parent } = mountToolbar(mockEdit);
toolbar.show(0, 0);
(parent.querySelector('[data-effect-type="zoom"]') as HTMLButtonElement).click();
(parent.querySelector('[data-effect="fade"]') as HTMLButtonElement).click();

expect(mockEdit.updateClip).toHaveBeenCalledWith(0, 0, { effect: expect.stringMatching(/^zoom/) });
expect(mockEdit.updateClip).toHaveBeenCalledWith(0, 0, { transition: expect.objectContaining({ in: "fade" }) });
toolbar.dispose();
});

it("blocks keyframe activation while a preset is present", () => {
it("keyframes opacity on a clip with an effect and a transition", () => {
const mockEdit = createMockEditSession();
mockEdit.getResolvedClip.mockReturnValue(createImageClip({ effect: "zoomIn" }));
mockEdit.playbackTime = 1;
mockEdit.getResolvedClip.mockReturnValue(createImageClip({ effect: "zoomIn", transition: { in: "fade" } }));

const { toolbar, parent } = mountToolbar(mockEdit);
toolbar.show(0, 0);
const keyframe = parent.querySelector("[data-opacity-keyframe]") as HTMLButtonElement;
expect(keyframe.disabled).toBe(true);
expect(keyframe.title).toContain("effect or transition");
expect(keyframe.disabled).toBe(false);
keyframe.click();
mockEdit.playbackTime = 3;
keyframe.click();

expect(mockEdit.updateClip).toHaveBeenCalledWith(0, 0, { opacity: expect.arrayContaining([expect.objectContaining({ start: 1, length: 2 })]) });
toolbar.dispose();
});

it("auto-keys a slider drag on an animated clip with a transition", () => {
const mockEdit = createMockEditSession();
mockEdit.playbackTime = 2;
mockEdit.getResolvedClip.mockReturnValue(createImageClip({ opacity: opacityTweens, transition: { in: "fade" } }));

const { toolbar, parent } = mountToolbar(mockEdit);
toolbar.show(0, 0);
const range = parent.querySelector("[data-opacity-slider-mount] input[type='range']") as HTMLInputElement;
range.dispatchEvent(new Event("pointerdown", { bubbles: true }));
range.value = "40";
range.dispatchEvent(new Event("input", { bubbles: true }));
range.dispatchEvent(new Event("change", { bubbles: true }));

const update = mockEdit.updateClipInDocument.mock.calls[0]?.[1];
expect(update?.opacity).toEqual(expect.arrayContaining([{ from: 0.2, to: 0.4, start: 1, length: 1, interpolation: "linear" }]));
toolbar.dispose();
});

Expand Down
Loading