fix: duplicating/removing frame while children selected (#9079)

packages/excalidraw/tests/__snapshots__/contextmenu.test.tsx.snap

@@ -2517,7 +2517,7 @@ exports[`contextMenu element > selecting 'Duplicate' in context menu duplicates
   "scrolledOutside": false,
   "searchMatches": [],
   "selectedElementIds": {
-    "id0_copy": true,
+    "id1": true,
   },
   "selectedElementsAreBeingDragged": false,
   "selectedGroupIds": {},
@@ -2590,7 +2590,7 @@ exports[`contextMenu element > selecting 'Duplicate' in context menu duplicates
   "frameId": null,
   "groupIds": [],
   "height": 20,
-  "id": "id0_copy",
+  "id": "id1",
   "index": "a1",
   "isDeleted": false,
   "link": null,
@@ -2680,7 +2680,7 @@ History {
         "delta": Delta {
           "deleted": {
             "selectedElementIds": {
-              "id0_copy": true,
+              "id1": true,
             },
           },
           "inserted": {
@@ -2693,7 +2693,7 @@ History {
       "elementsChange": ElementsChange {
         "added": Map {},
         "removed": Map {
-          "id0_copy" => Delta {
+          "id1" => Delta {
             "deleted": {
               "angle": 0,
               "backgroundColor": "transparent",

packages/excalidraw/tests/__snapshots__/move.test.tsx.snap

@@ -10,7 +10,7 @@ exports[`duplicate element on move when ALT is clicked > rectangle 5`] = `
   "frameId": null,
   "groupIds": [],
   "height": 50,
-  "id": "id0_copy",
+  "id": "id2",
   "index": "a0",
   "isDeleted": false,
   "link": null,

packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap

@@ -2129,7 +2129,7 @@ History {
       "elementsChange": ElementsChange {
         "added": Map {},
         "removed": Map {
-          "id0_copy" => Delta {
+          "id2" => Delta {
             "deleted": {
               "angle": 0,
               "backgroundColor": "transparent",
@@ -10619,7 +10619,7 @@ History {
       "elementsChange": ElementsChange {
         "added": Map {},
         "removed": Map {
-          "id0_copy" => Delta {
+          "id6" => Delta {
             "deleted": {
               "angle": 0,
               "backgroundColor": "transparent",
@@ -10628,7 +10628,7 @@ History {
               "fillStyle": "solid",
               "frameId": null,
               "groupIds": [
-                "id4_copy",
+                "id7",
               ],
               "height": 10,
               "index": "a0",
@@ -10652,7 +10652,7 @@ History {
               "isDeleted": true,
             },
           },
-          "id1_copy" => Delta {
+          "id8" => Delta {
             "deleted": {
               "angle": 0,
               "backgroundColor": "transparent",
@@ -10661,7 +10661,7 @@ History {
               "fillStyle": "solid",
               "frameId": null,
               "groupIds": [
-                "id4_copy",
+                "id7",
               ],
               "height": 10,
               "index": "a1",
@@ -10685,7 +10685,7 @@ History {
               "isDeleted": true,
             },
           },
-          "id2_copy" => Delta {
+          "id9" => Delta {
             "deleted": {
               "angle": 0,
               "backgroundColor": "transparent",
@@ -10694,7 +10694,7 @@ History {
               "fillStyle": "solid",
               "frameId": null,
               "groupIds": [
-                "id4_copy",
+                "id7",
               ],
               "height": 10,
               "index": "a2",

packages/excalidraw/tests/helpers/api.ts

@@ -40,6 +40,7 @@ import { createTestHook } from "../../components/App";
 import type { Action } from "../../actions/types";
 import { mutateElement } from "../../element/mutateElement";
 import { pointFrom, type LocalPoint, type Radians } from "../../../math";
+import { selectGroupsForSelectedElements } from "../../groups";
 
 const readFile = util.promisify(fs.readFile);
 // so that window.h is available when App.tsx is not imported as well.
@@ -68,13 +69,21 @@ export class API {
     });
   };
 
-  static setSelectedElements = (elements: ExcalidrawElement[]) => {
+  static setSelectedElements = (elements: ExcalidrawElement[], editingGroupId?: string | null) => {
     act(() => {
       h.setState({
-        selectedElementIds: elements.reduce((acc, element) => {
-          acc[element.id] = true;
-          return acc;
-        }, {} as Record<ExcalidrawElement["id"], true>),
+        ...selectGroupsForSelectedElements(
+        {
+          editingGroupId: editingGroupId ?? null,
+          selectedElementIds: elements.reduce((acc, element) => {
+            acc[element.id] = true;
+            return acc;
+          }, {} as Record<ExcalidrawElement["id"], true>),
+        },
+        elements,
+        h.state,
+        h.app,
+        )
       });
     });
   };
@@ -158,7 +167,7 @@ export class API {
     isDeleted?: boolean;
     frameId?: ExcalidrawElement["id"] | null;
     index?: ExcalidrawElement["index"];
-    groupIds?: string[];
+    groupIds?: ExcalidrawElement["groupIds"];
     // generic element props
     strokeColor?: ExcalidrawGenericElement["strokeColor"];
     backgroundColor?: ExcalidrawGenericElement["backgroundColor"];
@@ -369,6 +378,84 @@ export class API {
     return element as any;
   };
 
+  static createTextContainer = (opts?: {
+    frameId?: ExcalidrawElement["id"];
+    groupIds?: ExcalidrawElement["groupIds"];
+    label?: {
+      text?: string;
+      frameId?: ExcalidrawElement["id"] | null;
+      groupIds?: ExcalidrawElement["groupIds"];
+    };
+  }) => {
+    const rectangle = API.createElement({
+      type: "rectangle",
+      frameId: opts?.frameId || null,
+      groupIds: opts?.groupIds,
+    });
+
+    const text = API.createElement({
+      type: "text",
+      text: opts?.label?.text || "sample-text",
+      width: 50,
+      height: 20,
+      fontSize: 16,
+      containerId: rectangle.id,
+      frameId:
+        opts?.label?.frameId === undefined
+          ? opts?.frameId ?? null
+          : opts?.label?.frameId ?? null,
+      groupIds: opts?.label?.groupIds === undefined
+      ? opts?.groupIds
+      : opts?.label?.groupIds ,
+
+    });
+
+    mutateElement(
+      rectangle,
+      {
+        boundElements: [{ type: "text", id: text.id }],
+      },
+      false,
+    );
+
+    return [rectangle, text];
+  };
+
+  static createLabeledArrow = (opts?: {
+    frameId?: ExcalidrawElement["id"];
+    label?: {
+      text?: string;
+      frameId?: ExcalidrawElement["id"] | null;
+    };
+  }) => {
+    const arrow = API.createElement({
+      type: "arrow",
+      frameId: opts?.frameId || null,
+    });
+
+    const text = API.createElement({
+      type: "text",
+      id: "text2",
+      width: 50,
+      height: 20,
+      containerId: arrow.id,
+      frameId:
+        opts?.label?.frameId === undefined
+          ? opts?.frameId ?? null
+          : opts?.label?.frameId ?? null,
+    });
+
+    mutateElement(
+      arrow,
+      {
+        boundElements: [{ type: "text", id: text.id }],
+      },
+      false,
+    );
+
+    return [arrow, text];
+  };
+
   static readFile = async <T extends "utf8" | null>(
     filepath: string,
     encoding?: T,

packages/excalidraw/tests/history.test.tsx

@@ -7,6 +7,7 @@ import {
   assertSelectedElements,
   render,
   togglePopover,
+  getCloneByOrigId,
 } from "./test-utils";
 import { Excalidraw } from "../index";
 import { Keyboard, Pointer, UI } from "./helpers/ui";
@@ -15,7 +16,7 @@ import { getDefaultAppState } from "../appState";
 import { fireEvent, queryByTestId, waitFor } from "@testing-library/react";
 import { createUndoAction, createRedoAction } from "../actions/actionHistory";
 import { actionToggleViewMode } from "../actions/actionToggleViewMode";
-import { EXPORT_DATA_TYPES, MIME_TYPES } from "../constants";
+import { EXPORT_DATA_TYPES, MIME_TYPES, ORIG_ID } from "../constants";
 import type { AppState } from "../types";
 import { arrayToMap } from "../utils";
 import {
@@ -1138,8 +1139,8 @@ describe("history", () => {
       expect(h.elements).toEqual([
         expect.objectContaining({ id: rect1.id, isDeleted: false }),
         expect.objectContaining({ id: rect2.id, isDeleted: false }),
-        expect.objectContaining({ id: `${rect1.id}_copy`, isDeleted: true }),
-        expect.objectContaining({ id: `${rect2.id}_copy`, isDeleted: true }),
+        expect.objectContaining({ [ORIG_ID]: rect1.id, isDeleted: true }),
+        expect.objectContaining({ [ORIG_ID]: rect2.id, isDeleted: true }),
       ]);
       expect(h.state.editingGroupId).toBeNull();
       expect(h.state.selectedGroupIds).toEqual({ A: true });
@@ -1151,8 +1152,8 @@ describe("history", () => {
       expect(h.elements).toEqual([
         expect.objectContaining({ id: rect1.id, isDeleted: false }),
         expect.objectContaining({ id: rect2.id, isDeleted: false }),
-        expect.objectContaining({ id: `${rect1.id}_copy`, isDeleted: false }),
-        expect.objectContaining({ id: `${rect2.id}_copy`, isDeleted: false }),
+        expect.objectContaining({ [ORIG_ID]: rect1.id, isDeleted: false }),
+        expect.objectContaining({ [ORIG_ID]: rect2.id, isDeleted: false }),
       ]);
       expect(h.state.editingGroupId).toBeNull();
       expect(h.state.selectedGroupIds).not.toEqual(
@@ -1171,14 +1172,14 @@ describe("history", () => {
         expect.arrayContaining([
           expect.objectContaining({ id: rect1.id, isDeleted: false }),
           expect.objectContaining({ id: rect2.id, isDeleted: false }),
-          expect.objectContaining({ id: `${rect1.id}_copy`, isDeleted: true }),
-          expect.objectContaining({ id: `${rect2.id}_copy`, isDeleted: true }),
+          expect.objectContaining({ [ORIG_ID]: rect1.id, isDeleted: true }),
+          expect.objectContaining({ [ORIG_ID]: rect2.id, isDeleted: true }),
           expect.objectContaining({
-            id: `${rect1.id}_copy_copy`,
+            [ORIG_ID]: getCloneByOrigId(rect1.id)?.id,
             isDeleted: false,
           }),
           expect.objectContaining({
-            id: `${rect2.id}_copy_copy`,
+            [ORIG_ID]: getCloneByOrigId(rect2.id)?.id,
             isDeleted: false,
           }),
         ]),

packages/excalidraw/tests/library.test.tsx

@@ -1,11 +1,11 @@
 import React from "react";
 import { vi } from "vitest";
-import { fireEvent, render, waitFor } from "./test-utils";
+import { fireEvent, getCloneByOrigId, render, waitFor } from "./test-utils";
 import { act, queryByTestId } from "@testing-library/react";
 
 import { Excalidraw } from "../index";
 import { API } from "./helpers/api";
-import { MIME_TYPES } from "../constants";
+import { MIME_TYPES, ORIG_ID } from "../constants";
 import type { LibraryItem, LibraryItems } from "../types";
 import { UI } from "./helpers/ui";
 import { serializeLibraryAsJSON } from "../data/json";
@@ -76,7 +76,7 @@ describe("library", () => {
       }),
     );
     await waitFor(() => {
-      expect(h.elements).toEqual([expect.objectContaining({ id: "A_copy" })]);
+      expect(h.elements).toEqual([expect.objectContaining({ [ORIG_ID]: "A" })]);
     });
   });
 
@@ -125,23 +125,27 @@ describe("library", () => {
     );
 
     await waitFor(() => {
-      expect(h.elements).toEqual([
-        expect.objectContaining({
-          id: "rectangle1_copy",
-          boundElements: expect.arrayContaining([
-            { type: "text", id: "text1_copy" },
-            { type: "arrow", id: "arrow1_copy" },
-          ]),
-        }),
-        expect.objectContaining({
-          id: "text1_copy",
-          containerId: "rectangle1_copy",
-        }),
-        expect.objectContaining({
-          id: "arrow1_copy",
-          endBinding: expect.objectContaining({ elementId: "rectangle1_copy" }),
-        }),
-      ]);
+      expect(h.elements).toEqual(
+        expect.arrayContaining([
+          expect.objectContaining({
+            [ORIG_ID]: "rectangle1",
+            boundElements: expect.arrayContaining([
+              { type: "text", id: getCloneByOrigId("text1").id },
+              { type: "arrow", id: getCloneByOrigId("arrow1").id },
+            ]),
+          }),
+          expect.objectContaining({
+            [ORIG_ID]: "text1",
+            containerId: getCloneByOrigId("rectangle1").id,
+          }),
+          expect.objectContaining({
+            [ORIG_ID]: "arrow1",
+            endBinding: expect.objectContaining({
+              elementId: getCloneByOrigId("rectangle1").id,
+            }),
+          }),
+        ]),
+      );
     });
   });
 
@@ -170,10 +174,11 @@ describe("library", () => {
     await waitFor(() => {
       expect(h.elements).toEqual([
         expect.objectContaining({
-          id: "elem1_copy",
+          [ORIG_ID]: "elem1",
         }),
         expect.objectContaining({
-          id: expect.not.stringMatching(/^(elem1_copy|elem1)$/),
+          id: expect.not.stringMatching(/^elem1$/),
+          [ORIG_ID]: expect.not.stringMatching(/^\w+$/),
         }),
       ]);
     });
@@ -189,7 +194,7 @@ describe("library", () => {
       }),
     );
     await waitFor(() => {
-      expect(h.elements).toEqual([expect.objectContaining({ id: "A_copy" })]);
+      expect(h.elements).toEqual([expect.objectContaining({ [ORIG_ID]: "A" })]);
     });
     expect(h.state.activeTool.type).toBe("selection");
   });

packages/excalidraw/tests/test-utils.ts

@@ -11,6 +11,10 @@ import { getSelectedElements } from "../scene/selection";
 import type { ExcalidrawElement } from "../element/types";
 import { UI } from "./helpers/ui";
 import { diffStringsUnified } from "jest-diff";
+import ansi from "ansicolor";
+import { ORIG_ID } from "../constants";
+import { arrayToMap } from "../utils";
+import type { AllPossibleKeys } from "../utility-types";
 
 const customQueries = {
   ...queries,
@@ -295,3 +299,150 @@ expect.addSnapshotSerializer({
     );
   },
 });
+
+export const getCloneByOrigId = <T extends boolean = false>(
+  origId: ExcalidrawElement["id"],
+  returnNullIfNotExists: T = false as T,
+): T extends true ? ExcalidrawElement | null : ExcalidrawElement => {
+  const clonedElement = window.h.elements?.find(
+    (el) => (el as any)[ORIG_ID] === origId,
+  );
+  if (clonedElement) {
+    return clonedElement;
+  }
+  if (returnNullIfNotExists !== true) {
+    throw new Error(`cloned element not found for origId: ${origId}`);
+  }
+  return null as T extends true ? ExcalidrawElement | null : ExcalidrawElement;
+};
+
+/**
+ * Assertion helper that strips the actual elements of extra attributes
+ * so that diffs are easier to read in case of failure.
+ *
+ * Asserts element order as well, and selected element ids
+ * (when `selected: true` set for given element).
+ *
+ * If testing cloned elements, you can use { `[ORIG_ID]: origElement.id }
+ * If you need to refer to cloned element properties, you can use
+ * `getCloneByOrigId()`, e.g.: `{ frameId: getCloneByOrigId(origFrame.id)?.id }`
+ */
+export const assertElements = <T extends AllPossibleKeys<ExcalidrawElement>>(
+  actualElements: readonly ExcalidrawElement[],
+  /** array order matters */
+  expectedElements: (Partial<Record<T, any>> & {
+    /** meta, will be stripped for element attribute checks */
+    selected?: true;
+  } & (
+      | {
+          id: ExcalidrawElement["id"];
+        }
+      | { [ORIG_ID]?: string }
+    ))[],
+) => {
+  const h = window.h;
+
+  const expectedElementsWithIds: (typeof expectedElements[number] & {
+    id: ExcalidrawElement["id"];
+  })[] = expectedElements.map((el) => {
+    if ("id" in el) {
+      return el;
+    }
+    const actualElement = actualElements.find(
+      (act) => (act as any)[ORIG_ID] === el[ORIG_ID],
+    );
+    if (actualElement) {
+      return { ...el, id: actualElement.id };
+    }
+    return {
+      ...el,
+      id: "UNKNOWN_ID",
+    };
+  });
+
+  const map_expectedElements = arrayToMap(expectedElementsWithIds);
+
+  const selectedElementIds = expectedElementsWithIds.reduce(
+    (acc: Record<ExcalidrawElement["id"], true>, el) => {
+      if (el.selected) {
+        acc[el.id] = true;
+      }
+      return acc;
+    },
+    {},
+  );
+
+  const mappedActualElements = actualElements.map((el) => {
+    const expectedElement = map_expectedElements.get(el.id);
+    if (expectedElement) {
+      const pickedAttrs: Record<string, any> = {};
+
+      for (const key of Object.keys(expectedElement)) {
+        if (key === "selected") {
+          delete expectedElement.selected;
+          continue;
+        }
+        pickedAttrs[key] = (el as any)[key];
+      }
+
+      if (ORIG_ID in expectedElement) {
+        // @ts-ignore
+        pickedAttrs[ORIG_ID] = (el as any)[ORIG_ID];
+      }
+
+      return pickedAttrs;
+    }
+    return el;
+  });
+
+  try {
+    // testing order separately for even easier diffs
+    expect(actualElements.map((x) => x.id)).toEqual(
+      expectedElementsWithIds.map((x) => x.id),
+    );
+  } catch (err: any) {
+    let errStr = "\n\nmismatched element order\n\n";
+
+    errStr += `actual:   ${ansi.lightGray(
+      `[${err.actual
+        .map((id: string, index: number) => {
+          const act = actualElements[index];
+
+          return `${
+            id === err.expected[index] ? ansi.green(id) : ansi.red(id)
+          } (${act.type.slice(0, 4)}${
+            ORIG_ID in act ? ` ↳ ${(act as any)[ORIG_ID]}` : ""
+          })`;
+        })
+        .join(", ")}]`,
+    )}\n${ansi.lightGray(
+      `expected: [${err.expected
+        .map((exp: string, index: number) => {
+          const expEl = actualElements.find((el) => el.id === exp);
+          const origEl =
+            expEl &&
+            actualElements.find((el) => el.id === (expEl as any)[ORIG_ID]);
+          return expEl
+            ? `${
+                exp === err.actual[index]
+                  ? ansi.green(expEl.id)
+                  : ansi.red(expEl.id)
+              } (${expEl.type.slice(0, 4)}${origEl ? ` ↳ ${origEl.id}` : ""})`
+            : exp;
+        })
+        .join(", ")}]\n`,
+    )}`;
+
+    const error = new Error(errStr);
+    const stack = err.stack.split("\n");
+    stack.splice(1, 1);
+    error.stack = stack.join("\n");
+    throw error;
+  }
+
+  expect(mappedActualElements).toEqual(
+    expect.arrayContaining(expectedElementsWithIds),
+  );
+
+  expect(h.state.selectedElementIds).toEqual(selectedElementIds);
+};

packages/excalidraw/tests/zindex.test.tsx

@@ -1,6 +1,6 @@
 import React from "react";
 import ReactDOM from "react-dom";
-import { act, render } from "./test-utils";
+import { act, getCloneByOrigId, render } from "./test-utils";
 import { Excalidraw } from "../index";
 import { reseed } from "../random";
 import {
@@ -916,9 +916,9 @@ describe("z-index manipulation", () => {
     API.executeAction(actionDuplicateSelection);
     expect(h.elements).toMatchObject([
       { id: "A" },
-      { id: "A_copy" },
+      { id: getCloneByOrigId("A").id },
       { id: "B" },
-      { id: "B_copy" },
+      { id: getCloneByOrigId("B").id },
     ]);
 
     populateElements([
@@ -930,12 +930,12 @@ describe("z-index manipulation", () => {
       { id: "A" },
       { id: "B" },
       {
-        id: "A_copy",
+        id: getCloneByOrigId("A").id,
 
         groupIds: [expect.stringMatching(/.{3,}/)],
       },
       {
-        id: "B_copy",
+        id: getCloneByOrigId("B").id,
 
         groupIds: [expect.stringMatching(/.{3,}/)],
       },
@@ -951,12 +951,12 @@ describe("z-index manipulation", () => {
       { id: "A" },
       { id: "B" },
       {
-        id: "A_copy",
+        id: getCloneByOrigId("A").id,
 
         groupIds: [expect.stringMatching(/.{3,}/)],
       },
       {
-        id: "B_copy",
+        id: getCloneByOrigId("B").id,
 
         groupIds: [expect.stringMatching(/.{3,}/)],
       },
@@ -972,10 +972,10 @@ describe("z-index manipulation", () => {
     expect(h.elements.map((element) => element.id)).toEqual([
       "A",
       "B",
-      "A_copy",
-      "B_copy",
+      getCloneByOrigId("A").id,
+      getCloneByOrigId("B").id,
       "C",
-      "C_copy",
+      getCloneByOrigId("C").id,
     ]);
 
     populateElements([
@@ -988,12 +988,12 @@ describe("z-index manipulation", () => {
     expect(h.elements.map((element) => element.id)).toEqual([
       "A",
       "B",
-      "A_copy",
-      "B_copy",
+      getCloneByOrigId("A").id,
+      getCloneByOrigId("B").id,
       "C",
       "D",
-      "C_copy",
-      "D_copy",
+      getCloneByOrigId("C").id,
+      getCloneByOrigId("D").id,
     ]);
 
     populateElements(
@@ -1010,10 +1010,10 @@ describe("z-index manipulation", () => {
     expect(h.elements.map((element) => element.id)).toEqual([
       "A",
       "B",
-      "A_copy",
-      "B_copy",
+      getCloneByOrigId("A").id,
+      getCloneByOrigId("B").id,
       "C",
-      "C_copy",
+      getCloneByOrigId("C").id,
     ]);
 
     populateElements(
@@ -1031,9 +1031,9 @@ describe("z-index manipulation", () => {
       "A",
       "B",
       "C",
-      "A_copy",
-      "B_copy",
-      "C_copy",
+      getCloneByOrigId("A").id,
+      getCloneByOrigId("B").id,
+      getCloneByOrigId("C").id,
     ]);
 
     populateElements(
@@ -1054,15 +1054,15 @@ describe("z-index manipulation", () => {
       "A",
       "B",
       "C",
-      "A_copy",
-      "B_copy",
-      "C_copy",
+      getCloneByOrigId("A").id,
+      getCloneByOrigId("B").id,
+      getCloneByOrigId("C").id,
       "D",
       "E",
       "F",
-      "D_copy",
-      "E_copy",
-      "F_copy",
+      getCloneByOrigId("D").id,
+      getCloneByOrigId("E").id,
+      getCloneByOrigId("F").id,
     ]);
 
     populateElements(
@@ -1076,7 +1076,7 @@ describe("z-index manipulation", () => {
     API.executeAction(actionDuplicateSelection);
     expect(h.elements.map((element) => element.id)).toEqual([
       "A",
-      "A_copy",
+      getCloneByOrigId("A").id,
       "B",
       "C",
     ]);
@@ -1093,7 +1093,7 @@ describe("z-index manipulation", () => {
     expect(h.elements.map((element) => element.id)).toEqual([
       "A",
       "B",
-      "B_copy",
+      getCloneByOrigId("B").id,
       "C",
     ]);
 
@@ -1108,9 +1108,9 @@ describe("z-index manipulation", () => {
     API.executeAction(actionDuplicateSelection);
     expect(h.elements.map((element) => element.id)).toEqual([
       "A",
-      "A_copy",
+      getCloneByOrigId("A").id,
       "B",
-      "B_copy",
+      getCloneByOrigId("B").id,
       "C",
     ]);
   });
@@ -1125,8 +1125,8 @@ describe("z-index manipulation", () => {
     expect(h.elements.map((element) => element.id)).toEqual([
       "A",
       "C",
-      "A_copy",
-      "C_copy",
+      getCloneByOrigId("A").id,
+      getCloneByOrigId("C").id,
       "B",
     ]);
   });
@@ -1144,9 +1144,9 @@ describe("z-index manipulation", () => {
       "A",
       "B",
       "C",
-      "A_copy",
-      "B_copy",
-      "C_copy",
+      getCloneByOrigId("A").id,
+      getCloneByOrigId("B").id,
+      getCloneByOrigId("C").id,
       "D",
     ]);
   });