fix: let keyframed clips take effects and transitions in the toolbar (#255)

Author: dazzatronus

src/core/shared/clip-utils.ts

@@ -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);
-}

src/core/ui/media-toolbar.ts

@@ -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";
@@ -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";
@@ -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 ──────────────────────────────────────────────────
@@ -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;
 			}
@@ -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;
 		}
@@ -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 });
 	}
 

tests/clip-utils.test.ts

@@ -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);
@@ -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
-		);
-	});
-});

tests/media-toolbar.test.ts

@@ -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();
 		});