From de1ebad75534d915234cca83c3205acf5446cd2d Mon Sep 17 00:00:00 2001
From: David Luzar <luzar.david@gmail.com>
Date: Fri, 18 Aug 2023 16:34:01 +0200
Subject: [PATCH] fix: regression in indexing when adding elements to frame
 (#6904)

---
 src/frame.test.tsx       | 128 +++++++++++++++++++++++++++++++++++++++
 src/frame.ts             |  12 +---
 src/tests/helpers/api.ts |  11 ++++
 3 files changed, 141 insertions(+), 10 deletions(-)
 create mode 100644 src/frame.test.tsx

diff --git a/src/frame.test.tsx b/src/frame.test.tsx
new file mode 100644
index 00000000..e9562a6f
--- /dev/null
+++ b/src/frame.test.tsx
@@ -0,0 +1,128 @@
+import {
+  convertToExcalidrawElements,
+  Excalidraw,
+} from "./packages/excalidraw/index";
+import { API } from "./tests/helpers/api";
+import { Pointer } from "./tests/helpers/ui";
+import { render } from "./tests/test-utils";
+
+const { h } = window;
+const mouse = new Pointer("mouse");
+
+describe("adding elements to frames", () => {
+  type ElementType = string;
+  const assertOrder = (
+    els: readonly { type: ElementType }[],
+    order: ElementType[],
+  ) => {
+    expect(els.map((el) => el.type)).toEqual(order);
+  };
+
+  const reorderElements = <T extends { type: ElementType }>(
+    els: readonly T[],
+    order: ElementType[],
+  ) => {
+    return order.reduce((acc: T[], el) => {
+      acc.push(els.find((e) => e.type === el)!);
+      return acc;
+    }, []);
+  };
+
+  describe("resizing frame over elements", () => {
+    const testElements = async (
+      containerType: "arrow" | "rectangle",
+      initialOrder: ElementType[],
+      expectedOrder: ElementType[],
+    ) => {
+      await render(<Excalidraw />);
+
+      const frame = API.createElement({ type: "frame", x: 0, y: 0 });
+
+      h.elements = reorderElements(
+        [
+          frame,
+          ...convertToExcalidrawElements([
+            {
+              type: containerType,
+              x: 100,
+              y: 100,
+              height: 10,
+              label: { text: "xx" },
+            },
+          ]),
+        ],
+        initialOrder,
+      );
+
+      assertOrder(h.elements, initialOrder);
+
+      expect(h.elements[1].frameId).toBe(null);
+      expect(h.elements[2].frameId).toBe(null);
+
+      const container = h.elements[1];
+
+      mouse.clickAt(0, 0);
+      mouse.downAt(frame.x + frame.width, frame.y + frame.height);
+      mouse.moveTo(
+        container.x + container.width + 100,
+        container.y + container.height + 100,
+      );
+      mouse.up();
+      assertOrder(h.elements, expectedOrder);
+
+      expect(h.elements[0].frameId).toBe(frame.id);
+      expect(h.elements[1].frameId).toBe(frame.id);
+    };
+
+    it("resizing over text containers / labelled arrows", async () => {
+      await testElements(
+        "rectangle",
+        ["frame", "rectangle", "text"],
+        ["rectangle", "text", "frame"],
+      );
+      await testElements(
+        "rectangle",
+        ["frame", "text", "rectangle"],
+        ["rectangle", "text", "frame"],
+      );
+      await testElements(
+        "rectangle",
+        ["rectangle", "text", "frame"],
+        ["rectangle", "text", "frame"],
+      );
+      await testElements(
+        "rectangle",
+        ["text", "rectangle", "frame"],
+        ["text", "rectangle", "frame"],
+      );
+
+      await testElements(
+        "arrow",
+        ["frame", "arrow", "text"],
+        ["arrow", "text", "frame"],
+      );
+      await testElements(
+        "arrow",
+        ["text", "arrow", "frame"],
+        ["text", "arrow", "frame"],
+      );
+
+      // FIXME failing in tests (it fails to add elements to frame for some
+      // reason) but works in browser. (╯°□°)╯︵ ┻━┻
+      //
+      // Looks like the `getElementsCompletelyInFrame()` doesn't work
+      // in these cases.
+      //
+      // await testElements(
+      //   "arrow",
+      //   ["arrow", "text", "frame"],
+      //   ["arrow", "text", "frame"],
+      // );
+      // await testElements(
+      //   "arrow",
+      //   ["frame", "text", "arrow"],
+      //   ["text", "arrow", "frame"],
+      // );
+    });
+  });
+});
diff --git a/src/frame.ts b/src/frame.ts
index 7f0f42d7..d5599157 100644
--- a/src/frame.ts
+++ b/src/frame.ts
@@ -469,14 +469,6 @@ export const addElementsToFrame = (
   }
 
   let nextElements = allElements.slice();
-  // Optimisation since findIndex on "newElements" is slow
-  const nextElementsIndex = nextElements.reduce(
-    (acc: Record<string, number | undefined>, element, index) => {
-      acc[element.id] = index;
-      return acc;
-    },
-    {},
-  );
 
   const frameBoundary = findIndex(nextElements, (e) => e.frameId === frame.id);
   for (const element of omitGroupsContainingFrames(
@@ -492,8 +484,8 @@ export const addElementsToFrame = (
         false,
       );
 
-      const frameIndex = nextElementsIndex[frame.id] ?? -1;
-      const elementIndex = nextElementsIndex[element.id] ?? -1;
+      const frameIndex = findIndex(nextElements, (e) => e.id === frame.id);
+      const elementIndex = findIndex(nextElements, (e) => e.id === element.id);
 
       if (elementIndex < frameBoundary) {
         nextElements = [
diff --git a/src/tests/helpers/api.ts b/src/tests/helpers/api.ts
index 46361cf3..1b1943de 100644
--- a/src/tests/helpers/api.ts
+++ b/src/tests/helpers/api.ts
@@ -17,6 +17,7 @@ import path from "path";
 import { getMimeType } from "../../data/blob";
 import {
   newEmbeddableElement,
+  newFrameElement,
   newFreeDrawElement,
   newImageElement,
 } from "../../element/newElement";
@@ -24,6 +25,7 @@ import { Point } from "../../types";
 import { getSelectedElements } from "../../scene/selection";
 import { isLinearElementType } from "../../element/typeChecks";
 import { Mutable } from "../../utility-types";
+import { assertNever } from "../../utils";
 
 const readFile = util.promisify(fs.readFile);
 
@@ -244,6 +246,15 @@ export class API {
           scale: rest.scale || [1, 1],
         });
         break;
+      case "frame":
+        element = newFrameElement({ ...base, width, height });
+        break;
+      default:
+        assertNever(
+          type,
+          `API.createElement: unimplemented element type ${type}}`,
+        );
+        break;
     }
     if (element.type === "arrow") {
       element.startBinding = rest.startBinding ?? null;