From b324a85ab1c53d1a96b351cdb4e9292cd41cab6d Mon Sep 17 00:00:00 2001 From: David Luzar <5153846+dwelle@users.noreply.github.com> Date: Sat, 13 Jun 2026 20:32:45 +0200 Subject: [PATCH] fix(editor): elements duplicated when moving frame children (#11485) * fix(editor): elements duplicated when moving frame children * fix(editor): accumulate reorders across frames in bring-to-front/back * add invalid-order tests * fix: make sure moved indices are within range * add length/duplicate elements guards --- packages/element/src/zindex.ts | 72 +++++++++- packages/element/tests/zindex.test.tsx | 186 +++++++++++++++++++++++++ 2 files changed, 255 insertions(+), 3 deletions(-) diff --git a/packages/element/src/zindex.ts b/packages/element/src/zindex.ts index c4171c6bbd..d2b400e45f 100644 --- a/packages/element/src/zindex.ts +++ b/packages/element/src/zindex.ts @@ -1,5 +1,7 @@ import { arrayToMap, findIndex, findLastIndex } from "@excalidraw/common"; +import { isFiniteNumber } from "@excalidraw/math"; + import type { AppState } from "@excalidraw/excalidraw/types"; import type { GlobalPoint } from "@excalidraw/math"; @@ -313,12 +315,46 @@ const getTargetElementsMap = ( }, new Map()); }; +const hasSameElementIds = ( + prevElements: readonly ExcalidrawElement[], + nextElements: readonly ExcalidrawElement[], +) => { + if (prevElements.length !== nextElements.length) { + console.error( + "z-index reordering failed: resulting array have different lengths", + ); + return false; + } + + const prevElementIdCounts = new Map(); + for (const element of prevElements) { + prevElementIdCounts.set( + element.id, + (prevElementIdCounts.get(element.id) || 0) + 1, + ); + } + + for (const element of nextElements) { + const count = prevElementIdCounts.get(element.id); + if (!count) { + console.error( + "z-index reordering failed: element id mismatch / duplicate ids", + ); + return false; + } + prevElementIdCounts.set(element.id, count - 1); + } + + return true; +}; + const shiftElementsByOne = ( elements: readonly ExcalidrawElement[], appState: AppState, direction: "left" | "right", scene: Scene, ) => { + const originalElements = elements; const indicesToMove = getIndicesToMove(elements, appState); const targetElementsMap = getTargetElementsMap(elements, indicesToMove); @@ -389,6 +425,10 @@ const shiftElementsByOne = ( ]; }); + if (!hasSameElementIds(originalElements, elements)) { + return originalElements; + } + syncMovedIndices(elements, targetElementsMap); return elements; @@ -402,11 +442,20 @@ const shiftElementsToEnd = ( elementsToBeMoved?: readonly ExcalidrawElement[], ) => { const indicesToMove = getIndicesToMove(elements, appState, elementsToBeMoved); + + // Nothing to move (e.g. `elementsToBeMoved` is empty because all selected + // elements were frame children handled in a prior pass). Bail out early — + // otherwise `leadingIndex`/`trailingIndex` below resolve to `undefined` and + // the resulting `slice()` calls overlap, duplicating elements. + if (indicesToMove.length === 0) { + return elements; + } + const targetElementsMap = getTargetElementsMap(elements, indicesToMove); const displacedElements: ExcalidrawElement[] = []; - let leadingIndex: number; - let trailingIndex: number; + let leadingIndex: number | undefined; + let trailingIndex: number | undefined; if (direction === "left") { if (containingFrame) { leadingIndex = findIndex(elements, (el) => @@ -451,6 +500,19 @@ const shiftElementsToEnd = ( leadingIndex = 0; } + const isValidIndex = (index: number | undefined): index is number => { + return isFiniteNumber(index) && index >= 0; + }; + + if ( + !isValidIndex(leadingIndex) || + !isValidIndex(trailingIndex) || + leadingIndex > trailingIndex || + indicesToMove.some((index) => index < leadingIndex || index > trailingIndex) + ) { + return elements; + } + for (let index = leadingIndex; index < trailingIndex + 1; index++) { if (!indicesToMove.includes(index)) { displacedElements.push(elements[index]); @@ -475,6 +537,10 @@ const shiftElementsToEnd = ( ...trailingElements, ]; + if (!hasSameElementIds(elements, nextElements)) { + return elements; + } + syncMovedIndices(nextElements, targetElementsMap); return nextElements; @@ -543,7 +609,7 @@ function shiftElementsAccountingForFrames( for (const [frameId, children] of frameChildrenSets) { nextElements = shiftFunction( - allElements, + nextElements, appState, direction, frameId, diff --git a/packages/element/tests/zindex.test.tsx b/packages/element/tests/zindex.test.tsx index 997cb56f81..1861bf5d8d 100644 --- a/packages/element/tests/zindex.test.tsx +++ b/packages/element/tests/zindex.test.tsx @@ -1509,4 +1509,190 @@ describe("z-indexing with frames", () => { ], }); }); + + it("bringing to front / sending to back children of MULTIPLE frames at once moves all of them", () => { + assertZindex({ + elements: [ + { id: "F1_1", frameId: "F1", isSelected: true }, + { id: "F1_2", frameId: "F1" }, + { id: "F1", type: "frame" }, + { id: "F2_1", frameId: "F2", isSelected: true }, + { id: "F2_2", frameId: "F2" }, + { id: "F2", type: "frame" }, + ], + operations: [ + // +∞: each selected child moves to the front of its own frame + [actionBringToFront, ["F1_2", "F1", "F1_1", "F2_2", "F2", "F2_1"]], + // -∞: each selected child moves to the back of its own frame + [actionSendToBack, ["F1_1", "F1_2", "F1", "F2_1", "F2_2", "F2"]], + ], + }); + }); + + it("send to back / bring to front of a grouped frame child (in group-editing mode) must not duplicate elements", () => { + assertZindex({ + elements: [ + { id: "F1_1", frameId: "F1", groupIds: ["g1"] }, + { id: "F1_2", frameId: "F1", groupIds: ["g1"], isSelected: true }, + { id: "F1", type: "frame" }, + { id: "F2_1", frameId: "F2", groupIds: ["g2"] }, + { id: "F2_2", frameId: "F2", groupIds: ["g2"] }, + { id: "F2", type: "frame" }, + ], + appState: { editingGroupId: "g1" }, + operations: [ + // -∞ (send to back, within the frame) + [actionSendToBack, ["F1_2", "F1_1", "F1", "F2_1", "F2_2", "F2"]], + // +∞ (bring to front, within the frame) + [actionBringToFront, ["F1_1", "F1", "F1_2", "F2_1", "F2_2", "F2"]], + ], + }); + }); +}); + +/** + * The inputs in this block intentionally VIOLATE the (soft) invariant that a + * frame's children — and a group's members — are contiguous in the elements + * array. Such states shouldn't occur in normal use, but they CAN arise from + * bugs or broken input, because nothing re-defragments element order during + * a reorder (`normalizeElementOrder` only runs on duplication). We keep these + * tests so the reordering ops stay exercised against malformed order. + * + * HARD CONTRACT (a failure here is a real bug): a reorder must never throw, + * duplicate, or drop elements. `assertReorderPreservesElements` checks this. + * + * SOFT SNAPSHOT (read before "fixing"): the exact resulting ORDER is NOT a + * contract for invalid input — it's whatever the slice math happens to + * produce. If a future change alters an `expected` order below, that is NOT + * necessarily a functional regression. First confirm from the diff that the + * hard contract still holds (nothing duplicated/lost), then update the + * expected order to match, provided it's deemed an improvement over the + * previous order, or it's an acceptable change given the underlying logic + * change. + */ +describe("z-index reordering with broken contiguity (invariant-violating input)", () => { + beforeEach(async () => { + await render(); + }); + + const assertReorderPreservesElements = ( + elements: Parameters[0], + appState: Parameters[1], + // each op is applied to a freshly-populated (broken) state + cases: [Actions, string[]][], + ) => { + for (const [action, expected] of cases) { + populateElements(elements, appState); + const before = h.elements.map((el) => el.id); + + expect(() => API.executeAction(action)).not.toThrow(); + + const after = h.elements.map((el) => el.id); + // hard contract: + expect(after.length).toBe(before.length); // no loss + expect(new Set(after).size).toBe(after.length); // no duplication + // soft snapshot (see block comment before changing): + expect(after).toEqual(expected); + } + }; + + it("discontiguous frame children (foreign frame's child interleaved in span)", () => { + // F2_1 (a child of frame F2) sits INSIDE frame F1's z-span. Reordering F1's + // child sweeps F2_1 along (span-based frame handling) — wrong ordering, but + // never a duplication/loss, and the op does not throw. + const elements: Parameters[0] = [ + { id: "F1_1", frameId: "F1", isSelected: true }, + { id: "F2_1", frameId: "F2" }, + { id: "F1_2", frameId: "F1" }, + { id: "F1", type: "frame" }, + { id: "F2", type: "frame" }, + ]; + assertReorderPreservesElements(elements, undefined, [ + [actionBringForward, ["F2_1", "F1_2", "F1_1", "F1", "F2"]], + [actionSendBackward, ["F1_1", "F2_1", "F1_2", "F1", "F2"]], + [actionBringToFront, ["F2_1", "F1_2", "F1", "F1_1", "F2"]], + [actionSendToBack, ["F1_1", "F2_1", "F1_2", "F1", "F2"]], + ]); + }); + + it("discontiguous group, whole group selected", () => { + // g1 = {A, C}, scattered by the loose elements B and D. + const elements: Parameters[0] = [ + { id: "A", groupIds: ["g1"], isSelected: true }, + { id: "B" }, + { id: "C", groupIds: ["g1"], isSelected: true }, + { id: "D" }, + ]; + assertReorderPreservesElements(elements, undefined, [ + // move-by-one leaves the group scattered (each run moves independently) + [actionBringForward, ["B", "A", "D", "C"]], + [actionSendBackward, ["A", "C", "B", "D"]], + // to-front / to-back gather the scattered members back into one block + [actionBringToFront, ["B", "D", "A", "C"]], + [actionSendToBack, ["A", "C", "B", "D"]], + ]); + }); + + it("discontiguous group, single member selected in group-editing mode", () => { + const elements: Parameters[0] = [ + { id: "A", groupIds: ["g1"] }, + { id: "B" }, + { id: "C", groupIds: ["g1"], isSelected: true }, + { id: "D" }, + ]; + assertReorderPreservesElements(elements, { editingGroupId: "g1" }, [ + [actionBringForward, ["A", "B", "C", "D"]], + [actionSendBackward, ["C", "A", "B", "D"]], + [actionBringToFront, ["A", "B", "C", "D"]], + [actionSendToBack, ["C", "A", "B", "D"]], + ]); + }); + + it("two interleaved groups, both fully selected", () => { + const elements: Parameters[0] = [ + { id: "A", groupIds: ["g1"], isSelected: true }, + { id: "X", groupIds: ["g2"], isSelected: true }, + { id: "C", groupIds: ["g1"], isSelected: true }, + { id: "Y", groupIds: ["g2"], isSelected: true }, + { id: "Z" }, + ]; + assertReorderPreservesElements(elements, undefined, [ + [actionBringForward, ["Z", "A", "X", "C", "Y"]], + [actionSendBackward, ["A", "X", "C", "Y", "Z"]], + [actionBringToFront, ["Z", "A", "X", "C", "Y"]], + [actionSendToBack, ["A", "X", "C", "Y", "Z"]], + ]); + }); +}); + +describe("z-index reordering with inconsistent group-editing state", () => { + beforeEach(async () => { + await render(); + }); + + it("does not duplicate or drop elements when selected elements fall outside the edited group scope", () => { + assertZindex({ + elements: [ + { id: "A", groupIds: ["g1"], isSelected: true }, + { id: "C", groupIds: ["g1"] }, + { id: "X", groupIds: ["g2"] }, + { id: "Y", groupIds: ["g2"] }, + { id: "R" }, + ], + appState: { editingGroupId: "g2" }, + operations: [[actionSendToBack, ["A", "C", "X", "Y", "R"]]], + }); + + assertZindex({ + elements: [ + { id: "A", groupIds: ["g1"] }, + { id: "C", groupIds: ["g1"] }, + { id: "X", groupIds: ["g2"], isSelected: true }, + { id: "Y", groupIds: ["g2"] }, + { id: "R" }, + ], + appState: { editingGroupId: "g1" }, + operations: [[actionBringToFront, ["A", "C", "X", "Y", "R"]]], + }); + }); });