diff --git a/packages/studio/src/player/components/timelineClipDragCommit.test.ts b/packages/studio/src/player/components/timelineClipDragCommit.test.ts index 41501ae97e..43a82e6b23 100644 --- a/packages/studio/src/player/components/timelineClipDragCommit.test.ts +++ b/packages/studio/src/player/components/timelineClipDragCommit.test.ts @@ -588,6 +588,28 @@ describe("commitDraggedClipMove", () => { expectZLiftedToSix(onStackingPatches); }); + it("a multi-selection lane move restacks only the dragged clip", async () => { + // a moves up from row 2 to row 1 over c; the selected scene keeps its row, so it keeps its z. + const elements = [ + el("scene", 0, 0, 10), + el("b", 1, 5, 5), + el("a", 2, 0, 4), + el("c", 3, 0, 4), + ]; + const z: Record = { scene: 0, b: 2, a: 1, c: 3 }; + const onStackingPatches = vi.fn(); + runClipMove(drag(elements[2], { previewStart: 0, previewTrack: 1 }), { + elements, + trackOrder: [0, 1, 2, 3], + selectedKeys: new Set(["a", "scene"]), + readZIndex: (e) => z[e.key ?? e.id] ?? 0, + onStackingPatches, + }); + await flushMicrotasks(); + expect(onStackingPatches).toHaveBeenCalledTimes(1); + expect(onStackingPatches.mock.calls[0][0]).toEqual([{ key: "a", zIndex: 4 }]); + }); + it("partial z-sync deps (no readZIndex) → move persists but no stacking call", async () => { const elements = [el("a", 1, 0, 10), el("b", 0, 0, 10)]; // onStackingPatches present but readZIndex absent → syncStackingForEdit needs diff --git a/packages/studio/src/player/components/timelineClipDragCommit.ts b/packages/studio/src/player/components/timelineClipDragCommit.ts index 1078cdc9d2..e91b5c7dff 100644 --- a/packages/studio/src/player/components/timelineClipDragCommit.ts +++ b/packages/studio/src/player/components/timelineClipDragCommit.ts @@ -337,7 +337,6 @@ export function commitDraggedClipMove(drag: DraggedClipState, deps: DragCommitDe if (multi?.keys.has(keyOf(e))) return { ...e, start: multi.movedStart(e) }; return e; }); - const multiKeys = multi ? multi.keys : null; if (!isVertical || !deps.readZIndex || !deps.onStackingPatches) { void refreshAfterDurableLaneMove( persistMoveEdits(edits, deps, coalesceKey, "lane-reorder"), @@ -354,7 +353,6 @@ export function commitDraggedClipMove(drag: DraggedClipState, deps: DragCommitDe dragKey, drag.element.track, drag.previewTrack, - multiKeys, deps, coalesceKey, ), @@ -472,7 +470,6 @@ function commitTrackInsert( dragKey, drag.element.track, drag.insertRow!, - multi ? multi.keys : null, deps, coalesceKey, ), @@ -532,8 +529,8 @@ export function commitZMirrorLaneMove( * vertical lane change. Projects the drop-intent element set (`candidate`: the * dragged clip at its new / fractional-insert lane, others at their current tracks) * onto StackingElement using the caller-supplied live z-index reader, then - * delegates the minimal-z resolution to computeStackingPatches — a clip on the - * upper lane paints above every clip it time-overlaps. No-op unless both z-sync + * delegates to computeStackingPatches — the moved clip alone rises (moved up) or + * sinks (moved down) past the clips it time-overlaps. No-op unless both z-sync * deps are present, and never when the gesture aimed at the clip's OWN current * lane (`aimedLane === currentLane` — not a relocation). */ @@ -542,7 +539,6 @@ function syncStackingForEdit( dragKey: string, currentLane: number, aimedLane: number, - multiKeys: ReadonlySet | null, deps: DragCommitDeps, coalesceKey?: string, ): Promise { @@ -567,10 +563,11 @@ function syncStackingForEdit( stackingContextId: el.stackingContextId ?? null, })); - const editedKeys = [dragKey]; - if (multiKeys) for (const k of multiKeys) if (k !== dragKey) editedKeys.push(k); - - const patches = computeStackingPatches(stackingEls, editedKeys); + const patches = computeStackingPatches( + stackingEls, + [dragKey], + aimedLane < currentLane ? "up" : "down", + ); if (patches.length === 0) return Promise.resolve(); return Promise.resolve(onStackingPatches(patches, coalesceKey)).then(() => undefined); } diff --git a/packages/studio/src/player/components/timelineStackingSync.test.ts b/packages/studio/src/player/components/timelineStackingSync.test.ts index a7d1e81e74..de94a0debb 100644 --- a/packages/studio/src/player/components/timelineStackingSync.test.ts +++ b/packages/studio/src/player/components/timelineStackingSync.test.ts @@ -3,6 +3,7 @@ import { computeStackingPatches, laneIsAbove, samePaintScope, + type StackingDirection, type StackingElement, } from "./timelineStackingSync"; @@ -18,9 +19,13 @@ function el( return { key, track, start, duration, zIndex, isAudio, domIndex }; } -function patchMap(elements: StackingElement[], edited: string[]): Record { +function patchMap( + elements: StackingElement[], + edited: string[], + direction: StackingDirection, +): Record { const out: Record = {}; - for (const p of computeStackingPatches(elements, edited)) out[p.key] = p.zIndex; + for (const p of computeStackingPatches(elements, edited, direction)) out[p.key] = p.zIndex; return out; } @@ -58,7 +63,7 @@ describe("stacking-context partitioning", () => { stackingContextId: null, }; - expect(patchMap([root, scene], ["root"])).toEqual({}); + expect(patchMap([root, scene], ["root"], "up")).toEqual({}); }); it("never compares or patches across stacking contexts", () => { @@ -86,7 +91,7 @@ describe("stacking-context partitioning", () => { }; // X edited: only same-context neighbours participate — none here, so X keeps // its z (nothing to fix WITHIN its context) and Y is never touched. - expect(patchMap([x, y], ["x"])).toEqual({}); + expect(patchMap([x, y], ["x"], "up")).toEqual({}); }); it("still resolves within the edited clip's own context", () => { @@ -115,7 +120,7 @@ describe("stacking-context partitioning", () => { // by flipping z so a MUST be lifted). const aWrong = { ...a, track: 0, zIndex: 1 }; const bLow = { ...b, track: 1, zIndex: 5 }; - const patches = patchMap([aWrong, bLow], ["a"]); + const patches = patchMap([aWrong, bLow], ["a"], "up"); expect(patches.a).toBeGreaterThan(5); }); }); @@ -132,214 +137,168 @@ describe("computeStackingPatches", () => { it("no overlapping clips → no patch", () => { // a (0..5 on track 0) and b (10..15 on track 1) never overlap in time. const elements = [el("a", 0, 0, 5, 10), el("b", 1, 10, 5, 5)]; - expect(patchMap(elements, ["a"])).toEqual({}); + expect(patchMap(elements, ["a"], "up")).toEqual({}); }); - it("edited clip moved to a HIGHER lane (top) but z too low → raised above the below-neighbour", () => { - // a on top lane (0) overlaps b on lane 1; a.z=1 is below b.z=5 → wrong. + it("moved up with z too low → raised above the clip it now sits above", () => { const elements = [el("a", 0, 0, 10, 1), el("b", 1, 0, 10, 5)]; - // Only a is edited → only a gets a patch, lifting it above b (5) → 6. - expect(patchMap(elements, ["a"])).toEqual({ a: 6 }); + expect(patchMap(elements, ["a"], "up")).toEqual({ a: 6 }); }); - it("edited clip moved to a LOWER lane (bottom) but z too high → lowered below the above-neighbour", () => { - // a on lane 2 (bottom) overlaps b on lane 0 (top); a.z=9 above b.z=5 → wrong. + it("moved down with z too high → lowered below the clip now above it", () => { const elements = [el("a", 2, 0, 10, 9), el("b", 0, 0, 10, 5)]; - expect(patchMap(elements, ["a"])).toEqual({ a: 4 }); + expect(patchMap(elements, ["a"], "down")).toEqual({ a: 4 }); }); - it("edited clip already correctly ordered → no patch (authored z preserved)", () => { - // a on top lane already has higher z than the lower-lane b it overlaps. + it("already in order → no patch (authored z preserved)", () => { const elements = [el("a", 0, 0, 10, 8), el("b", 1, 0, 10, 3)]; - expect(patchMap(elements, ["a"])).toEqual({}); + expect(patchMap(elements, ["a"], "up")).toEqual({}); }); it("untouched clips never get a patch even when they overlap the edit", () => { - // b is out of order relative to a, but a is the only edited clip. const elements = [el("a", 0, 0, 10, 1), el("b", 1, 0, 10, 5), el("c", 2, 0, 10, 9)]; - const patches = computeStackingPatches(elements, ["a"]); + const patches = computeStackingPatches(elements, ["a"], "up"); expect(patches.map((p) => p.key)).toEqual(["a"]); }); - it("sits strictly between neighbours when there is integer room", () => { - // edited a on middle lane 1 between below-lane-2 (z=2) and above-lane-0 (z=10). + it("moved up rises one above the clips now below it and ignores the clips above it", () => { const elements = [el("a", 1, 0, 10, 0), el("below", 2, 0, 10, 2), el("above", 0, 0, 10, 10)]; - // Between 2 and 10 → floor((2+10)/2)=6. - expect(patchMap(elements, ["a"])).toEqual({ a: 6 }); + expect(patchMap(elements, ["a"], "up")).toEqual({ a: 3 }); }); - it("adjacent neighbours (no integer gap) → edited lands above lower, upper is bumped", () => { - // below z=4, above z=5 (adjacent). There is no integer strictly between 4 and - // 5, so the old single-patch a=5 left `a` TIED with `above` — and with no DOM - // order to break the tie, `above` no longer paints strictly above `a` (the - // under-patch bug). Tie-aware cascade: a→5 (above below's 4) AND above→6 so it - // stays strictly on top. Minimal: only the two overlapping neighbours move. - const elements = [el("a", 1, 0, 10, 0), el("below", 2, 0, 10, 4), el("above", 0, 0, 10, 5)]; - expect(patchMap(elements, ["a"])).toEqual({ a: 5, above: 6 }); + it("moved down sinks one below the clips now above it and ignores the clips below it", () => { + const elements = [el("a", 1, 0, 10, 20), el("below", 2, 0, 10, 2), el("above", 0, 0, 10, 10)]; + expect(patchMap(elements, ["a"], "down")).toEqual({ a: 9 }); }); it("audio clips are excluded — an audio edit yields no patch", () => { const elements = [el("music", 3, 0, 10, 0, true), el("v", 0, 0, 10, 5)]; - expect(patchMap(elements, ["music"])).toEqual({}); + expect(patchMap(elements, ["music"], "down")).toEqual({}); }); it("audio clips are excluded as neighbours — a visual edit ignores overlapping audio", () => { - // The only overlapping clip is audio → treated as no visual overlap → no patch. const elements = [el("v", 0, 0, 10, 3), el("music", 3, 0, 10, 99, true)]; - expect(patchMap(elements, ["v"])).toEqual({}); + expect(patchMap(elements, ["v"], "up")).toEqual({}); }); - it("only-below neighbours → maxBelow + 1", () => { + it("moved up over several clips → one above the highest of them", () => { const elements = [el("a", 0, 0, 10, 0), el("b", 1, 0, 10, 3), el("c", 2, 0, 10, 7)]; - // a on top overlaps b(3) and c(7), both below → 7+1=8. - expect(patchMap(elements, ["a"])).toEqual({ a: 8 }); + expect(patchMap(elements, ["a"], "up")).toEqual({ a: 8 }); }); - it("only-above neighbours → minAbove - 1 (clamped ≥ 0)", () => { + it("moved down under several clips → one below the lowest of them, never under 0", () => { const elements = [el("a", 2, 0, 10, 9), el("b", 0, 0, 10, 1), el("c", 1, 0, 10, 4)]; - // a on bottom overlaps b(1) and c(4), both above → min(1)-1=0. - expect(patchMap(elements, ["a"])).toEqual({ a: 0 }); + expect(patchMap(elements, ["a"], "down")).toEqual({ a: 0 }); }); it("partial time overlap still counts", () => { - // a: 0..6, b: 5..15 overlap in [5,6). const elements = [el("a", 0, 0, 6, 1), el("b", 1, 5, 10, 5)]; - expect(patchMap(elements, ["a"])).toEqual({ a: 6 }); + expect(patchMap(elements, ["a"], "up")).toEqual({ a: 6 }); }); it("touching-but-not-overlapping intervals do NOT count", () => { - // a ends exactly where b starts (t=5) → half-open, no overlap. const elements = [el("a", 0, 0, 5, 1), el("b", 1, 5, 5, 5)]; - expect(patchMap(elements, ["a"])).toEqual({}); + expect(patchMap(elements, ["a"], "up")).toEqual({}); }); - it("multi-clip edit: two dragged clips resolve consistently against the region", () => { - // Drag a (lane 0) and b (lane 1) onto a region already holding c (lane 2, z=5). - // Both overlap c. Lower-lane b resolves first (above c → 6), then a (above b → 7). + it("multi-clip move up: the bottom member resolves first, the top one rises above it", () => { const elements = [el("a", 0, 0, 10, 0), el("b", 1, 0, 10, 0), el("c", 2, 0, 10, 5)]; - expect(patchMap(elements, ["a", "b"])).toEqual({ a: 7, b: 6 }); + expect(patchMap(elements, ["a", "b"], "up")).toEqual({ a: 7, b: 6 }); }); - it("multi-clip edit skips a member that is already correctly ordered", () => { + it("multi-clip move down: the top member resolves first, the bottom one sinks below it", () => { + const elements = [el("a", 1, 0, 10, 9), el("b", 2, 0, 10, 9), el("c", 0, 0, 10, 5)]; + expect(patchMap(elements, ["a", "b"], "down")).toEqual({ a: 4, b: 3 }); + }); + + it("multi-clip move skips a member that is already in order", () => { const elements = [el("a", 0, 0, 10, 20), el("b", 1, 0, 10, 0), el("c", 2, 0, 10, 5)]; - // a(20) already above everything → no patch. b (lane 1) sits between - // below-neighbour c(5) and above-neighbour a(20) → floor((5+20)/2)=12. - expect(patchMap(elements, ["a", "b"])).toEqual({ b: 12 }); + expect(patchMap(elements, ["a", "b"], "up")).toEqual({ b: 6 }); }); it("empty edited set → no patches", () => { const elements = [el("a", 0, 0, 10, 1), el("b", 1, 0, 10, 5)]; - expect(computeStackingPatches(elements, [])).toEqual([]); + expect(computeStackingPatches(elements, [], "up")).toEqual([]); }); - it("item 13: an unresolved neighbour (non-finite z) is EXCLUDED, not treated as z=0", () => { - // `ghost` is an overlapping upper-lane clip whose live node could not be - // resolved, so its z came back NaN. If it were fabricated to 0 it would be a - // real above-neighbour at the z-floor and drag the edited clip's cascade down. - // Excluded, the only real overlap is below-neighbour b(3) → a rises to 4. + it("an unresolved neighbour (non-finite z) is excluded, not treated as z=0", () => { const elements = [ - el("a", 1, 0, 10, 0), - el("b", 2, 0, 10, 3), - el("ghost", 0, 0, 10, Number.NaN), + el("a", 2, 0, 10, 9), + el("b", 0, 0, 10, 3), + el("ghost", 1, 0, 10, Number.NaN), ]; - expect(patchMap(elements, ["a"])).toEqual({ a: 4 }); + expect(patchMap(elements, ["a"], "down")).toEqual({ a: 2 }); }); - it("item 13: an edited clip whose own z is unresolved (non-finite) yields no patch", () => { - // The edited clip itself couldn't be resolved → it is dropped from the working - // set and produces nothing (no fabricated-0 self-patch). + it("an edited clip whose own z is unresolved (non-finite) yields no patch", () => { const elements = [el("a", 0, 0, 10, Number.NaN), el("b", 1, 0, 10, 5)]; - expect(patchMap(elements, ["a"])).toEqual({}); + expect(patchMap(elements, ["a"], "up")).toEqual({}); }); }); -describe("computeStackingPatches — tie-aware cascade (lane move always realisable)", () => { - it("drag below an overlapping z=0 neighbour → cascade bumps the neighbour, edit→0", () => { - // edited `v` (z=2) dragged to the BOTTOM lane, overlapping `r` (z=0) which is - // now on the upper lane. No z ≥ 0 fits strictly below 0, so the old resolver - // clamped v to 0 (tied with r) and nothing changed on canvas — the reported - // bug. Tie-aware: v→0 AND r bumped to 1 so r paints strictly above v. - const elements = [el("v", 1, 0, 10, 2), el("r", 0, 0, 10, 0)]; - expect(patchMap(elements, ["v"])).toEqual({ v: 0, r: 1 }); - }); - - it("equal-z + domIndex: dragging the LATER-in-DOM clip below is realised via a bump", () => { - // Two equal-z clips; `v` is later in DOM (domIndex 1) so it currently paints - // ON TOP of `r` (domIndex 0). User drags v to the lower lane (track 1). With - // domIndex the sync SEES that v is currently above r and must be lowered: - // v→0 (already 0, stays) then r bumped to 1 so r wins. Without domIndex the - // equal z would look already-correct and under-patch. +describe("computeStackingPatches — only the moved clip changes", () => { + it("a full-frame scene on the top row stays behind a caption moved up a row under it", () => { + // Agent-made films often put the scene on track 0. The caption (z1, later in the + // file) moves from track 3 to 2, still under the scene's row: nothing is below + // it, so nothing changes. Lifting the scene over it would hide the caption. + const elements = [el("scene", 0, 0, 10, 0, false, 0), el("caption", 2, 0, 10, 1, false, 1)]; + expect(patchMap(elements, ["caption"], "up")).toEqual({}); + }); + + it("a caption moved to the top row comes in front of the full-frame scene", () => { + const elements = [el("scene", 0, 0, 10, 5, false, 0), el("caption", -0.5, 0, 10, 1, false, 1)]; + expect(patchMap(elements, ["caption"], "up")).toEqual({ caption: 6 }); + }); + + it("moved down under a z-0 clip earlier in the file: drops to 0 and the neighbour stays", () => { + // z never goes negative (it could paint behind the composition's background), + // and neighbours never move, so this order cannot be fully expressed. + const elements = [el("r", 0, 0, 10, 0, false, 0), el("v", 1, 0, 10, 2, false, 1)]; + expect(patchMap(elements, ["v"], "down")).toEqual({ v: 0 }); + }); + + it("moved down when it cannot go lower → no patch", () => { const elements = [el("r", 0, 0, 10, 0, false, 0), el("v", 1, 0, 10, 0, false, 1)]; - expect(patchMap(elements, ["v"])).toEqual({ r: 1 }); - }); - - it("equal-z without domIndex is ambiguous → conservatively bumps to guarantee order", () => { - // Same shape but NO domIndex: equal z is ambiguous, so the resolver cannot - // prove v is already below r and patches to make the order explicit (r above). - const elements = [el("r", 0, 0, 10, 0), el("v", 1, 0, 10, 0)]; - const out = patchMap(elements, ["v"]); - // r must end up strictly above v (higher z) regardless of the exact numbers. - const vz = out.v ?? 0; - const rz = out.r ?? 0; - expect(rz).toBeGreaterThan(vz); - }); - - it("#2198 (Abhai repro): a lift cascades transitively so an UNTOUCHED pair never inverts", () => { - // m (z1, lane0, dom0), n (z0, lane1, dom1), e (z2, lane2, dom2, edited). - // e overlaps n [5,10); n overlaps m [12,15); e does NOT overlap m. - // Dragging e to the bottom lane forces n up to paint above e. A naive lift sets - // n→1, which TIES m (z1) and — n being later in the DOM — paints n above m, - // inverting the untouched (m,n) pair (which the next normalize would reshuffle). - // The transitive cascade lifts m too (→2) so m stays strictly above n. - const elements = [ - el("m", 0, 12, 8, 1, false, 0), - el("n", 1, 5, 10, 0, false, 1), - el("e", 2, 0, 10, 2, false, 2), - ]; - expect(patchMap(elements, ["e"])).toEqual({ e: 0, n: 1, m: 2 }); + expect(patchMap(elements, ["v"], "down")).toEqual({}); }); - it("cascade patches as FEW clips as possible (only the blockers move)", () => { - // v dragged to bottom under r(z0) and s(z0) both on higher lanes; a distant - // non-overlapping clip x is never touched. + it("moved down never goes under a clip on a lower row it paints over now", () => { + // v cannot get under r (z0, earlier in the file); dropping it to 0 would hide it under w. const elements = [ - el("v", 2, 0, 10, 5), - el("r", 0, 0, 10, 0), - el("s", 1, 0, 10, 0), - el("x", 3, 50, 10, 0), // no time overlap → untouched + el("r", 0, 0, 10, 0, false, 0), + el("v", 1, 0, 10, 2, false, 1), + el("w", 2, 0, 10, 1, false, 2), ]; - const out = patchMap(elements, ["v"]); - expect("x" in out).toBe(false); - // v below both r and s. - expect(out.v).toBeLessThan(out.r ?? 0); - expect(out.v).toBeLessThan(out.s ?? 0); + expect(patchMap(elements, ["v"], "down")).toEqual({}); }); -}); -describe("computeStackingPatches — DOM tie-break gates the cascade (item 12)", () => { - it("tie ACCEPTABLE: edited may sit AT minAbove when the above-neighbour is later in DOM → single patch, no neighbour bump", () => { - // below b (z3, lane2, dom0), edited e (lane1, dom1), above a (z4, lane0, dom2). - // e must paint above b and below a. There is no integer strictly between 3 and - // 4, but a is LATER in the DOM, so e=4 ties a and a still paints on top by DOM - // order — a valid SINGLE patch. The old gap<2 rule cascaded and needlessly - // bumped a's authored z (the over-patch). Only e changes here. - const elements = [ - el("b", 2, 0, 10, 3, false, 0), - el("e", 1, 0, 10, 0, false, 1), - el("a", 0, 0, 10, 4, false, 2), - ]; - expect(patchMap(elements, ["e"])).toEqual({ e: 4 }); + it("moved down sinks only as far as the clips on lower rows allow", () => { + const elements = [el("s", 0, 0, 10, 5), el("v", 1, 0, 10, 9), el("w", 2, 0, 10, 7)]; + expect(patchMap(elements, ["v"], "down")).toEqual({ v: 8 }); }); - it("tie INVERTING: edited tying minAbove would paint ABOVE it (earlier in DOM) → cascade bumps the neighbour", () => { - // Same z's, but a is EARLIER in the DOM than e (dom0 vs dom2). Now e=4 would - // tie a AND paint on top (e later in DOM), violating the lane order, so the - // tie-break can't save it: e→4 and a is bumped to 5. + it("moved down with equal z: the clip above, later in the file, already paints over it", () => { + const elements = [el("v", 1, 0, 10, 3, false, 0), el("r", 0, 0, 10, 3, false, 1)]; + expect(patchMap(elements, ["v"], "down")).toEqual({}); + }); + + it("#2198: an untouched pair keeps its order because neighbours never move", () => { + // e overlaps n [5,10); n overlaps m [12,15); e does not overlap m. const elements = [ - el("a", 0, 0, 10, 4, false, 0), - el("b", 2, 0, 10, 3, false, 1), - el("e", 1, 0, 10, 0, false, 2), + el("m", 0, 12, 8, 1, false, 0), + el("n", 1, 5, 10, 0, false, 1), + el("e", 2, 0, 10, 2, false, 2), ]; - expect(patchMap(elements, ["e"])).toEqual({ e: 4, a: 5 }); + expect(patchMap(elements, ["e"], "down")).toEqual({ e: 0 }); + }); + + it("equal z broken by file order: the clip later in the file already paints above", () => { + const elements = [el("b", 1, 0, 10, 3, false, 0), el("e", 0, 0, 10, 3, false, 1)]; + expect(patchMap(elements, ["e"], "up")).toEqual({}); + }); + + it("equal z with no file order is ambiguous → the move still writes a z", () => { + const elements = [el("b", 1, 0, 10, 3), el("e", 0, 0, 10, 3)]; + expect(patchMap(elements, ["e"], "up")).toEqual({ e: 4 }); }); }); diff --git a/packages/studio/src/player/components/timelineStackingSync.ts b/packages/studio/src/player/components/timelineStackingSync.ts index 8f0002ba06..978c58c9af 100644 --- a/packages/studio/src/player/components/timelineStackingSync.ts +++ b/packages/studio/src/player/components/timelineStackingSync.ts @@ -1,10 +1,10 @@ /** * timelineStackingSync — lane ↔ stacking unification (pure). * - * The approved design: **lane order implies stacking**. A clip on a higher lane - * (rendered ABOVE another in the timeline) should render ON TOP of any clip it - * OVERLAPS IN TIME. But authored z-indexes are sacred: z only changes on a user - * edit, and ONLY for the clip(s) the user actually edited. + * A row move restacks the moved clip only: moved up, it rises above every clip + * it overlaps in time on the rows now below it; moved down, it sinks below every + * such clip on the rows now above it. Neighbours never change, so a clip nobody + * moved never goes behind anything (a full-frame scene on the top row stays put). * * Lane → screen mapping (see Timeline.tsx trackOrder / TimelineCanvas rows): * tracks are sorted ASCENDING and rendered top → bottom, so a LOWER `track` @@ -38,7 +38,7 @@ export interface StackingElement { * (e.g. an unmounted / nested sub-comp element, or one outside the active file). * A non-finite-z clip is EXCLUDED from the computation — it is neither a stacking * neighbour nor resolvable as an edit — so an unresolved node never fabricates a - * z=0 neighbour that poisons the boundary math (item 13). The reader signals a + * z=0 neighbour. The reader signals a * miss with NaN rather than null so the value stays assignable to the existing * `(el) => number` reader contract the drag hook / commit deps declare. */ @@ -57,10 +57,9 @@ export interface StackingElement { /** * Discovery / DOM document position (optional). Two clips with EQUAL z paint by * DOM order — the one LATER in the DOM paints ON TOP. When supplied, "is A above - * B" uses (zIndex, domIndex); without it equal-z is ambiguous and the sync can - * under-patch (the reported bug: a clip dragged to the bottom lane over an - * equal-z neighbour changed nothing on canvas). Callers pass the index of the - * element in the discovery order array. + * B" uses (zIndex, domIndex); without it equal z counts as "not above", so the + * move writes a z. Callers pass the index of the element in the discovery order + * array. */ domIndex?: number; } @@ -74,7 +73,7 @@ export interface StackingPatch { /** * Canonical paint-scope key: leaf z-indexes are comparable only within the same * source document and CSS stacking context. The ONLY place this normalization - * lives — partitioning, membership checks, and pairwise equality all use it. + * lives; samePaintScope compares with it. */ const paintScopeKey = (el: { sourceFile?: string; stackingContextId?: string | null }): string => JSON.stringify([el.sourceFile ?? null, el.stackingContextId ?? null]); @@ -114,21 +113,13 @@ export function laneIsAbove( return a.track < b.track; } -/** - * Working record for the cascade resolver: a live-mutable, RESOLVED (non-null) z - * the resolver can bump, plus the immutable identity/lane/time/dom fields. Clips - * whose z could not be resolved (null) are dropped before this stage. - */ -interface MutZ extends StackingElement { - zIndex: number; -} +/** Which way the edited clips moved between rows. */ +export type StackingDirection = "up" | "down"; /** * Does `a` currently paint ON TOP of `b`? Higher z wins; equal z breaks by DOM - * order (later in DOM paints on top). When either domIndex is absent, equal z is - * treated as "not strictly above" (ambiguous) — callers should supply domIndex to - * disambiguate (see StackingElement.domIndex). Exported (like laneIsAbove) as the - * ONE paint-order predicate so every consumer agrees on what "paints above" means. + * order (later in DOM paints on top). Without both domIndex values equal z is + * ambiguous and counts as "not above", so the move still writes a z. */ function paintsAbove( a: Pick, @@ -139,233 +130,58 @@ function paintsAbove( return false; } -/** Reduce a neighbour set's z-indices to a single bound, or null when empty. */ -function boundaryZ(neighbours: MutZ[], reduce: (zs: number[]) => number): number | null { - return neighbours.length > 0 ? reduce(neighbours.map((o) => o.zIndex)) : null; +/** Moved up: one above the highest clip it now sits above, or null when it already paints above them all. */ +function raisedZ(clip: StackingElement, overlapping: StackingElement[]): number | null { + const below = overlapping.filter((o) => laneIsAbove(clip, o)); + if (below.every((o) => paintsAbove(clip, o))) return null; + return Math.max(...below.map((o) => o.zIndex)) + 1; } -/** - * Resolve `edited` so that, among the clips it OVERLAPS IN TIME, its paint order - * matches its lane order (lower lane ⇒ paints on top). Records every z change - * (edited clip AND any neighbours that must be bumped) into `patchZ`. - * - * Fast path (unchanged behaviour): when a single non-negative z for the edited - * clip alone realises the order — strictly between the neighbours if there is - * integer room, else just above the lower neighbour, else just below the upper — - * emit only that. This keeps every existing single-patch test passing. - * - * Cascade path: when ties/clamping make the single-clip patch impossible or - * ineffective (must sit below an overlapping z=0 neighbour, or between adjacent / - * equal-z neighbours where DOM order alone can't express it), bump the minimum set - * of overlapping neighbours that must stay ABOVE by +1 (cascading only as far as - * needed) so the edited clip's intended lane order is realised with all z ≥ 0. - * "Authored z sacred" stays the default — neighbours are touched only when the - * user's explicit lane move is otherwise inexpressible (same precedent as the - * canvas context-menu tie-aware fix). - * - * Returns true when any z changed (recorded in `patchZ`), false for a no-op. - */ -function resolveEditedZ( - edited: MutZ, - overlapping: MutZ[], - overlappersOf: (clip: MutZ) => MutZ[], - patchZ: (clip: MutZ, z: number) => void, -): boolean { - const visualOverlap = overlapping.filter((o) => !o.isAudio); - if (visualOverlap.length === 0) return false; - - // Neighbours that must end up BELOW edited (lower lane) vs ABOVE (higher lane). - const below = visualOverlap.filter((o) => laneIsAbove(edited, o)); - const above = visualOverlap.filter((o) => laneIsAbove(o, edited)); - - // Already correct against every overlapping neighbour → no-op (authored z kept). - const correct = - below.every((o) => paintsAbove(edited, o)) && above.every((o) => paintsAbove(o, edited)); - if (correct) return false; - - const maxBelow = boundaryZ(below, (zs) => Math.max(...zs)); - - // ── Fast path: try to realise the order by moving only `edited`. ────────────── - const single = trySingleZ(edited, below, above); - if (single != null) { - if (single !== edited.zIndex) patchZ(edited, single); - // Even at an unchanged z the DOM-order ties may already be satisfied; if not, - // `trySingleZ` returned null and we fall through to the cascade. - return single !== edited.zIndex; - } - - // ── Cascade path: can't fit `edited` between the neighbours with one z ≥ 0. ─── - // Sit edited at maxBelow+1 (or 0 when it only has above-neighbours) and lift the - // above-neighbours that are now not strictly above, minimally, one step past it. - const target = maxBelow != null ? maxBelow + 1 : 0; - const clamped = Math.max(0, target); - if (clamped !== edited.zIndex) patchZ(edited, clamped); - liftAbove(edited, overlappersOf, patchZ); - return true; +/** Moved down: one below the lowest clip it now sits under, but never under z 0 (a negative z can paint behind the + * composition's own background) and never under a clip on a lower row it paints over now, so the move cannot hide it. + * Null when it already paints under them all or cannot go lower. */ +function loweredZ(clip: StackingElement, overlapping: StackingElement[]): number | null { + const above = overlapping.filter((o) => laneIsAbove(o, clip)); + if (above.every((o) => paintsAbove(o, clip))) return null; + const keepOver = overlapping.filter((o) => laneIsAbove(clip, o) && paintsAbove(clip, o)); + const floor = Math.max( + 0, + ...keepOver.map((o) => + paintsAbove({ ...clip, zIndex: o.zIndex }, o) ? o.zIndex : o.zIndex + 1, + ), + ); + const z = Math.max(floor, Math.min(...above.map((o) => o.zIndex)) - 1); + return z < clip.zIndex ? z : null; } /** - * Pick a single non-negative z for `edited` that lands it correctly against its - * neighbours (paints above every below-neighbour, below every above-neighbour), or - * null when no such z exists and the caller must cascade. - * - * The candidate is verified with the SAME `paintsAbove` predicate the resolver uses - * (z + DOM tie-break), so an authored z that already paints correctly by DOM order - * is honoured instead of over-patched: with below=z3 and an above-neighbour at z4 - * that is LATER in the DOM, edited=4 ties the neighbour but the neighbour still - * paints on top by DOM order — a valid single patch, no neighbour bump (item 12). - * When the tie would INVERT (the above-neighbour is earlier in DOM) the candidate - * fails verification and the caller cascades. - */ -// edited has neighbours on BOTH sides: prefer the integer midpoint of a real gap; -// with no strict gap a DOM tie-break may still let it sit AT minAbove (item 12), -// else the caller cascades. -function zBetweenNeighbours( - maxBelow: number, - minAbove: number, - correctAt: (z: number) => boolean, -): number | null { - if (minAbove - maxBelow >= 2) { - const mid = Math.floor((maxBelow + minAbove) / 2); - return mid > maxBelow && mid < minAbove ? mid : null; - } - return correctAt(minAbove) ? minAbove : null; -} - -// edited has only above-neighbours: sit one step below minAbove, or at the z=0 -// floor tie minAbove when a DOM tie-break keeps that neighbour on top. -function zBelowOnly(minAbove: number, correctAt: (z: number) => boolean): number | null { - const candidate = minAbove - 1; - if (candidate >= 0) return candidate; - return correctAt(minAbove) ? minAbove : null; -} - -function trySingleZ(edited: MutZ, below: MutZ[], above: MutZ[]): number | null { - const maxBelow = boundaryZ(below, (zs) => Math.max(...zs)); - const minAbove = boundaryZ(above, (zs) => Math.min(...zs)); - - const correctAt = (z: number): boolean => { - const probe: MutZ = { ...edited, zIndex: z }; - return below.every((b) => paintsAbove(probe, b)) && above.every((a) => paintsAbove(a, probe)); - }; - - if (maxBelow != null && minAbove != null) - return zBetweenNeighbours(maxBelow, minAbove, correctAt); - if (maxBelow != null) return maxBelow + 1; // only below-neighbours → grow upward - if (minAbove != null) return zBelowOnly(minAbove, correctAt); - return null; -} - -/** - * Enforce the module invariant — for every OVERLAPPING pair, the clip on the upper - * lane paints on top — starting from `edited` and cascading TRANSITIVELY. - * - * Seeded with `edited`: each of its upper-lane overlappers must paint strictly - * above it (the deliberate lane move). Raising a clip can then tie or cross ANOTHER - * clip it overlaps that sits on an even higher lane — that clip must be lifted too, - * and so on. Without the cascade a lifted neighbour could tie an untouched clip on - * a higher lane and, being later in the DOM, paint above it — an untouched pair - * visibly inverting (#2198). The condition is LANE order (not "was originally - * above"), so a clip that was already violating lane order — e.g. a bottom-lane - * clip painting on top — is fixed, never preserved. Only clips whose z actually - * changes are patched; z climbs by +1 each step so the walk terminates. - */ -function liftAbove( - edited: MutZ, - overlappersOf: (clip: MutZ) => MutZ[], - patchZ: (clip: MutZ, z: number) => void, -): void { - const queue: MutZ[] = [edited]; - const raiseAbove = (clip: MutZ, floor: MutZ): void => { - if (paintsAbove(clip, floor)) return; // already strictly on top - patchZ(clip, floor.zIndex + 1); // patchZ mutates clip.zIndex in place - queue.push(clip); - }; - while (queue.length > 0) { - const floor = queue.shift()!; - for (const other of overlappersOf(floor)) { - if (laneIsAbove(other, floor) && !paintsAbove(other, floor)) raiseAbove(other, floor); - } - } -} - -/** - * Compute z-index patches so each edited clip's stacking matches its lane order. - * - * @param elements The FULL post-edit element set (edited clips already carry - * their new lane/time). Untouched clips keep their current z. - * @param editedKeys Keys of the clip(s) the user just edited. - * @returns Minimal z patches. When a single-clip patch realises the order it is - * the only patch (authored z of neighbours untouched); when ties or a - * z=0 floor make that impossible, the minimum set of overlapping - * neighbours is bumped too so the lane move is always realisable with - * all z ≥ 0. Non-overlapping / already-correct edits yield nothing. - * - * Multi-clip edits: each edited clip is resolved against the CURRENT (already- - * patched) z of all OTHER clips, lower lane first, so a group dragged onto a busy - * region stacks consistently. + * The z-index patches for a row move: each edited clip, and only it, is raised (`up`) or lowered (`down`) against the + * clips it overlaps in time in its own paint scope. Clips whose live z is unresolved (non-finite) and audio clips take + * no part. Several edited clips resolve against each other's new z: the bottom one first when moving up, the top one + * first when moving down. */ export function computeStackingPatches( elements: StackingElement[], editedKeys: Iterable, + direction: StackingDirection, ): StackingPatch[] { const editedSet = new Set(editedKeys); - if (editedSet.size === 0) return []; + const live = elements + .filter((e) => Number.isFinite(e.zIndex) && !e.isAudio) + .map((e) => ({ ...e })); + const edited = live + .filter((e) => editedSet.has(e.key)) + .sort((a, b) => (direction === "up" ? b.track - a.track : a.track - b.track)); - // Drop clips whose live z couldn't be resolved (non-finite / NaN): a fabricated - // z=0 would enter the boundary math as a phantom neighbour at the z-floor. An - // unresolved clip is neither a neighbour nor resolvable as an edit, so it is - // excluded outright (item 13). - const allResolved = elements.filter((e) => Number.isFinite(e.zIndex)); - - // Leaf z is only meaningful within ONE source document and stacking context: - // across either boundary the ancestor composition/context decides paint order. - // Restrict the computation to the edited clips' own paint scope(s). - const editedScopes = new Set(allResolved.filter((e) => editedSet.has(e.key)).map(paintScopeKey)); - const resolved = allResolved.filter((e) => editedScopes.has(paintScopeKey(e))); - - // Mutable z snapshot so edits + cascaded bumps see each other's applied z. - const byKey = new Map(resolved.map((e) => [e.key, { ...e }])); - const edited = resolved - .filter((e) => editedSet.has(e.key) && !e.isAudio) - .map((e) => byKey.get(e.key)!) - // Resolve lower-lane (renders below) clips first so their new z is visible - // to higher-lane siblings resolved after them. - .sort((a, b) => b.track - a.track); - - const changed = new Map(); - const patchZ = (clip: MutZ, z: number): void => { - clip.zIndex = z; - changed.set(clip.key, z); - }; - - // The full live set, so the transitive cascade can reach clips that overlap a - // LIFTED neighbour without overlapping the edited clip itself (#2198). - const all = [...byKey.values()]; - const overlappersOf = (clip: MutZ): MutZ[] => - all.filter( - (o) => o.key !== clip.key && !o.isAudio && samePaintScope(clip, o) && overlapsInTime(clip, o), - ); - - for (const clip of edited) { - resolveEditedZ(clip, overlappersOf(clip), overlappersOf, patchZ); - } - - // Emit in a stable order (edited clips first in their resolve order, then any - // cascaded neighbours) — deterministic for tests and undo grouping. - const emitted = new Set(); const patches: StackingPatch[] = []; for (const clip of edited) { - if (changed.has(clip.key) && !emitted.has(clip.key)) { - patches.push({ key: clip.key, zIndex: changed.get(clip.key)! }); - emitted.add(clip.key); - } - } - for (const [key, zIndex] of changed) { - if (!emitted.has(key)) { - patches.push({ key, zIndex }); - emitted.add(key); - } + const overlapping = live.filter( + (o) => o.key !== clip.key && overlapsInTime(clip, o) && samePaintScope(clip, o), + ); + const zIndex = direction === "up" ? raisedZ(clip, overlapping) : loweredZ(clip, overlapping); + if (zIndex === null) continue; + clip.zIndex = zIndex; + patches.push({ key: clip.key, zIndex }); } return patches; } diff --git a/packages/studio/src/player/components/useTimelineStackingSync.ts b/packages/studio/src/player/components/useTimelineStackingSync.ts index 32b82bd8da..c0ccf16b8c 100644 --- a/packages/studio/src/player/components/useTimelineStackingSync.ts +++ b/packages/studio/src/player/components/useTimelineStackingSync.ts @@ -55,7 +55,7 @@ export function useTimelineStackingSync({ // NaN (NOT 0) when the element can't be resolved in the preview iframe — a // nested / unmounted sub-comp node, or one outside the active file. Fabricating // z=0 would enter computeStackingPatches as a real overlapping neighbour at the - // z-floor and skew the boundary math; a non-finite value tells it to EXCLUDE this + // z-floor; a non-finite value tells it to EXCLUDE this // clip instead. NaN (rather than null) keeps the return assignable to the // `(el) => number` reader contract the drag hook / commit deps declare. const readClipZIndex = useCallback( diff --git a/packages/studio/src/timelineStackingSyncExport.test.tsx b/packages/studio/src/timelineStackingSyncExport.test.tsx index adaed4b523..2022b95ba0 100644 --- a/packages/studio/src/timelineStackingSyncExport.test.tsx +++ b/packages/studio/src/timelineStackingSyncExport.test.tsx @@ -24,8 +24,9 @@ it("restacks the host's preview when a lane move puts a clip below another", asy const iframe = document.createElement("iframe"); document.body.appendChild(iframe); const doc = iframe.contentDocument!; - // No authored z: DOM order paints tag over title, which matches the rows until the move. - doc.body.innerHTML = '
'; + // Tag (top row) paints over title, matching the rows until the move. + doc.body.innerHTML = + '
'; const clip = (id: string, start: number, duration: number, track: number) => ({ id, key: id, domId: id, tag: "div", start, duration, track }) as const; usePlayerStore.setState({