From 262879bb9e69c93615c2cf476e408e1f1063a17f Mon Sep 17 00:00:00 2001 From: dazzatronus Date: Mon, 26 Oct 2026 17:57:03 +1100 Subject: [PATCH] fix: let keyframed clips take effects and transitions in the toolbar --- src/core/shared/clip-utils.ts | 18 ------------- src/core/ui/media-toolbar.ts | 49 +++------------------------------ tests/clip-utils.test.ts | 32 +--------------------- tests/media-toolbar.test.ts | 51 ++++++++++++++++++++++++++++++----- 4 files changed, 48 insertions(+), 102 deletions(-) diff --git a/src/core/shared/clip-utils.ts b/src/core/shared/clip-utils.ts index f567efb9..b6a858a7 100644 --- a/src/core/shared/clip-utils.ts +++ b/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); -} diff --git a/src/core/ui/media-toolbar.ts b/src/core/ui/media-toolbar.ts index 32245cd2..16dd7d04 100644 --- a/src/core/ui/media-toolbar.ts +++ b/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,15 +1300,6 @@ 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 }); } @@ -1345,11 +1307,6 @@ export class MediaToolbar extends BaseToolbar { 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 }); } diff --git a/tests/clip-utils.test.ts b/tests/clip-utils.test.ts index 15de182f..865ecf79 100644 --- a/tests/clip-utils.test.ts +++ b/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 { - 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))).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))).toBe( - false - ); - }); -}); diff --git a/tests/media-toolbar.test.ts b/tests/media-toolbar.test.ts index 272bf0ba..bd776793 100644 --- a/tests/media-toolbar.test.ts +++ b/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(); });