fix: ensure relative z-index of elements added to frame is retained (#7134)

dev-docs/docs/codebase/frames.mdx

@@ -0,0 +1,22 @@
+# Frames
+
+## Ordering
+
+Frames should be ordered where frame children come first, followed by the frame element itself:
+
+```
+[
+  other_element,
+  frame1_child1,
+  frame1_child2,
+  frame1,
+  other_element,
+  frame2_child1,
+  frame2_child2,
+  frame2,
+  other_element,
+  ...
+]
+```
+
+If not oredered correctly, the editor will still function, but the elements may not be rendered and clipped correctly. Further, the renderer relies on this ordering for performance optimizations.

dev-docs/sidebars.js

@@ -23,7 +23,11 @@ const sidebars = {
       },
       items: ["introduction/development", "introduction/contributing"],
     },
-    { type: "category", label: "Codebase", items: ["codebase/json-schema"] },
+    {
+      type: "category",
+      label: "Codebase",
+      items: ["codebase/json-schema", "codebase/frames"],
+    },
     {
       type: "category",
       label: "@excalidraw/excalidraw",

src/frame.test.tsx

@@ -177,7 +177,7 @@ describe("adding elements to frames", () => {
         expectEqualIds([rect2, frame]);
       });
 
-      it.skip("should add elements", async () => {
+      it("should add elements", async () => {
         h.elements = [rect2, rect3, frame];
 
         func(frame, rect2);
@@ -188,7 +188,7 @@ describe("adding elements to frames", () => {
         expectEqualIds([rect3, rect2, frame]);
       });
 
-      it.skip("should add elements when there are other other elements in between", async () => {
+      it("should add elements when there are other other elements in between", async () => {
         h.elements = [rect1, rect2, rect4, rect3, frame];
 
         func(frame, rect2);
@@ -199,7 +199,7 @@ describe("adding elements to frames", () => {
         expectEqualIds([rect1, rect4, rect3, rect2, frame]);
       });
 
-      it.skip("should add elements when there are other elements in between and the order is reversed", async () => {
+      it("should add elements when there are other elements in between and the order is reversed", async () => {
         h.elements = [rect3, rect4, rect2, rect1, frame];
 
         func(frame, rect2);
@@ -234,7 +234,7 @@ describe("adding elements to frames", () => {
         expectEqualIds([rect1, rect2, rect3, frame, rect4]);
       });
 
-      it.skip("should add elements when there are other elements in between and the order is reversed", async () => {
+      it("should add elements when there are other elements in between and the order is reversed", async () => {
         h.elements = [rect3, rect4, frame, rect2, rect1];
 
         func(frame, rect2);
@@ -436,5 +436,121 @@ describe("adding elements to frames", () => {
       expect(rect2.frameId).toBe(null);
       expectEqualIds([rect2_copy, frame, rect2]);
     });
+
+    it("random order 01", () => {
+      const frame1 = API.createElement({
+        type: "frame",
+        x: 0,
+        y: 0,
+        width: 100,
+        height: 100,
+      });
+      const frame2 = API.createElement({
+        type: "frame",
+        x: 200,
+        y: 0,
+        width: 100,
+        height: 100,
+      });
+      const frame3 = API.createElement({
+        type: "frame",
+        x: 300,
+        y: 0,
+        width: 100,
+        height: 100,
+      });
+
+      const rectangle1 = API.createElement({
+        type: "rectangle",
+        x: 25,
+        y: 25,
+        width: 50,
+        height: 50,
+        frameId: frame1.id,
+      });
+      const rectangle2 = API.createElement({
+        type: "rectangle",
+        x: 225,
+        y: 25,
+        width: 50,
+        height: 50,
+        frameId: frame2.id,
+      });
+      const rectangle3 = API.createElement({
+        type: "rectangle",
+        x: 325,
+        y: 25,
+        width: 50,
+        height: 50,
+        frameId: frame3.id,
+      });
+      const rectangle4 = API.createElement({
+        type: "rectangle",
+        x: 350,
+        y: 25,
+        width: 50,
+        height: 50,
+        frameId: frame3.id,
+      });
+
+      h.elements = [
+        frame1,
+        rectangle4,
+        rectangle1,
+        rectangle3,
+        frame3,
+        rectangle2,
+        frame2,
+      ];
+
+      API.setSelectedElements([rectangle2]);
+
+      const origSize = h.elements.length;
+
+      expect(h.elements.length).toBe(origSize);
+      dragElementIntoFrame(frame3, rectangle2);
+      expect(h.elements.length).toBe(origSize);
+    });
+
+    it("random order 02", () => {
+      const frame1 = API.createElement({
+        type: "frame",
+        x: 0,
+        y: 0,
+        width: 100,
+        height: 100,
+      });
+      const frame2 = API.createElement({
+        type: "frame",
+        x: 200,
+        y: 0,
+        width: 100,
+        height: 100,
+      });
+      const rectangle1 = API.createElement({
+        type: "rectangle",
+        x: 25,
+        y: 25,
+        width: 50,
+        height: 50,
+        frameId: frame1.id,
+      });
+      const rectangle2 = API.createElement({
+        type: "rectangle",
+        x: 225,
+        y: 25,
+        width: 50,
+        height: 50,
+        frameId: frame2.id,
+      });
+
+      h.elements = [rectangle1, rectangle2, frame1, frame2];
+
+      API.setSelectedElements([rectangle2]);
+
+      expect(h.elements.length).toBe(4);
+      dragElementIntoFrame(frame2, rectangle1);
+      expect(h.elements.length).toBe(4);
+    });
   });
 });

src/frame.ts

@@ -452,22 +452,31 @@ export const getContainingFrame = (
 };
 
 // --------------------------- Frame Operations -------------------------------
+
+/**
+ * Retains (or repairs for target frame) the ordering invriant where children
+ * elements come right before the parent frame:
+ * [el, el, child, child, frame, el]
+ */
 export const addElementsToFrame = (
   allElements: ExcalidrawElementsIncludingDeleted,
   elementsToAdd: NonDeletedExcalidrawElement[],
   frame: ExcalidrawFrameElement,
 ) => {
-  const currTargetFrameChildrenMap = new Map(
+  const { allElementsIndexMap, currTargetFrameChildrenMap } =
     allElements.reduce(
-      (acc: [ExcalidrawElement["id"], ExcalidrawElement][], element) => {
+      (acc, element, index) => {
+        acc.allElementsIndexMap.set(element.id, index);
         if (element.frameId === frame.id) {
-          acc.push([element.id, element]);
+          acc.currTargetFrameChildrenMap.set(element.id, true);
         }
         return acc;
       },
-      [],
-    ),
-  );
+      {
+        allElementsIndexMap: new Map<ExcalidrawElement["id"], number>(),
+        currTargetFrameChildrenMap: new Map<ExcalidrawElement["id"], true>(),
+      },
+    );
 
   const suppliedElementsToAddSet = new Set(elementsToAdd.map((el) => el.id));
 
@@ -520,12 +529,36 @@ export const addElementsToFrame = (
       currFrameChildren.forEach((child) => {
         processedElements.add(child.id);
       });
-      // console.log(currFrameChildren, finalElementsToAdd, element);
-      nextElements.push(...currFrameChildren, ...finalElementsToAdd, element);
+
+      // if not found, add all children on top by assigning the lowest index
+      const targetFrameIndex = allElementsIndexMap.get(frame.id) ?? -1;
+
+      const { newChildren_left, newChildren_right } = finalElementsToAdd.reduce(
+        (acc, element) => {
+          // if index not found, add on top of current frame children
+          const elementIndex = allElementsIndexMap.get(element.id) ?? Infinity;
+          if (elementIndex < targetFrameIndex) {
+            acc.newChildren_left.push(element);
+          } else {
+            acc.newChildren_right.push(element);
+          }
+          return acc;
+        },
+        {
+          newChildren_left: [] as ExcalidrawElement[],
+          newChildren_right: [] as ExcalidrawElement[],
+        },
+      );
+
+      nextElements.push(
+        ...newChildren_left,
+        ...currFrameChildren,
+        ...newChildren_right,
+        element,
+      );
       continue;
     }
 
-    // console.log("(2)", element.frameId);
     nextElements.push(element);
   }
 

src/tests/helpers/api.ts

@@ -94,6 +94,7 @@ export class API {
     angle?: number;
     id?: string;
     isDeleted?: boolean;
+    frameId?: ExcalidrawElement["id"];
     groupIds?: string[];
     // generic element props
     strokeColor?: ExcalidrawGenericElement["strokeColor"];
@@ -151,12 +152,12 @@ export class API {
       | "versionNonce"
       | "isDeleted"
       | "groupIds"
-      | "frameId"
       | "link"
       | "updated"
     > = {
       x,
       y,
+      frameId: rest.frameId ?? null,
       angle: rest.angle ?? 0,
       strokeColor: rest.strokeColor ?? appState.currentItemStrokeColor,
       backgroundColor: