fix: bound text not atomic with container when changing z-index (#4414)

Co-authored-by: Aakansha Doshi <aakansha1216@gmail.com>
Co-authored-by: dwelle <luzar.david@gmail.com>

src/tests/helpers/api.ts

@@ -94,6 +94,10 @@ export class API {
     verticalAlign?: T extends "text"
       ? ExcalidrawTextElement["verticalAlign"]
       : never;
+    boundElements?: ExcalidrawGenericElement["boundElements"];
+    containerId?: T extends "text"
+      ? ExcalidrawTextElement["containerId"]
+      : never;
   }): T extends "arrow" | "line"
     ? ExcalidrawLinearElement
     : T extends "freedraw"
@@ -118,6 +122,7 @@ export class API {
         rest.strokeSharpness ?? appState.currentItemStrokeSharpness,
       roughness: rest.roughness ?? appState.currentItemRoughness,
       opacity: rest.opacity ?? appState.currentItemOpacity,
+      boundElements: rest.boundElements ?? null,
     };
     switch (type) {
       case "rectangle":
@@ -138,6 +143,7 @@ export class API {
           fontFamily: rest.fontFamily ?? appState.currentItemFontFamily,
           textAlign: rest.textAlign ?? appState.currentItemTextAlign,
           verticalAlign: rest.verticalAlign ?? DEFAULT_VERTICAL_ALIGN,
+          containerId: rest.containerId ?? undefined,
         });
         element.width = width;
         element.height = height;

src/tests/zindex.test.tsx

@@ -32,11 +32,12 @@ const populateElements = (
     x?: number;
     width?: number;
     height?: number;
+    containerId?: string;
   }[],
 ) => {
   const selectedElementIds: any = {};
 
-  h.elements = elements.map(
+  const newElements = elements.map(
     ({
       id,
       isDeleted = false,
@@ -46,9 +47,10 @@ const populateElements = (
       x = 100,
       width = 100,
       height = 100,
+      containerId = null,
     }) => {
       const element = API.createElement({
-        type: "rectangle",
+        type: containerId ? "text" : "rectangle",
         id,
         isDeleted,
         x,
@@ -56,6 +58,7 @@ const populateElements = (
         width,
         height,
         groupIds,
+        containerId,
       });
       if (isSelected) {
         selectedElementIds[element.id] = true;
@@ -64,6 +67,22 @@ const populateElements = (
     },
   );
 
+  // initialize `boundElements` on containers, if applicable
+  h.elements = newElements.map((element, index, elements) => {
+    const nextElement = elements[index + 1];
+    if (
+      nextElement &&
+      "containerId" in nextElement &&
+      element.id === nextElement.containerId
+    ) {
+      return {
+        ...element,
+        boundElements: [{ type: "text", id: nextElement.id }],
+      };
+    }
+    return element;
+  });
+
   h.setState({
     selectedElementIds,
   });
@@ -87,6 +106,7 @@ const assertZindex = ({
     isDeleted?: true;
     isSelected?: true;
     groupIds?: string[];
+    containerId?: string;
   }[];
   appState?: Partial<AppState>;
   operations: [Actions, string[]][];
@@ -1051,4 +1071,73 @@ describe("z-index manipulation", () => {
       "C_copy",
     ]);
   });
+
+  it("text-container binding should be atomic", () => {
+    assertZindex({
+      elements: [
+        { id: "A", isSelected: true },
+        { id: "B" },
+        { id: "C", containerId: "B" },
+      ],
+      operations: [
+        [actionBringForward, ["B", "C", "A"]],
+        [actionSendBackward, ["A", "B", "C"]],
+      ],
+    });
+
+    assertZindex({
+      elements: [
+        { id: "A" },
+        { id: "B", isSelected: true },
+        { id: "C", containerId: "B" },
+      ],
+      operations: [
+        [actionSendBackward, ["B", "C", "A"]],
+        [actionBringForward, ["A", "B", "C"]],
+      ],
+    });
+
+    assertZindex({
+      elements: [
+        { id: "A", isSelected: true, groupIds: ["g1"] },
+        { id: "B", groupIds: ["g1"] },
+        { id: "C", containerId: "B", groupIds: ["g1"] },
+      ],
+      appState: {
+        editingGroupId: "g1",
+      },
+      operations: [
+        [actionBringForward, ["B", "C", "A"]],
+        [actionSendBackward, ["A", "B", "C"]],
+      ],
+    });
+
+    assertZindex({
+      elements: [
+        { id: "A", groupIds: ["g1"] },
+        { id: "B", groupIds: ["g1"], isSelected: true },
+        { id: "C", containerId: "B", groupIds: ["g1"] },
+      ],
+      appState: {
+        editingGroupId: "g1",
+      },
+      operations: [
+        [actionSendBackward, ["B", "C", "A"]],
+        [actionBringForward, ["A", "B", "C"]],
+      ],
+    });
+
+    assertZindex({
+      elements: [
+        { id: "A", groupIds: ["g1"] },
+        { id: "B", isSelected: true, groupIds: ["g1"] },
+        { id: "C" },
+        { id: "D", containerId: "C" },
+      ],
+      appState: {
+        editingGroupId: "g1",
+      },
+      operations: [[actionBringForward, ["A", "B", "C", "D"]]],
+    });
+  });
 });

src/zindex.ts

@@ -1,8 +1,10 @@
 import { bumpVersion } from "./element/mutateElement";
 import { ExcalidrawElement } from "./element/types";
 import { getElementsInGroup } from "./groups";
+import { getSelectedElements } from "./scene";
+import Scene from "./scene/Scene";
 import { AppState } from "./types";
-import { findIndex, findLastIndex } from "./utils";
+import { arrayToMap, findIndex, findLastIndex } from "./utils";
 
 /**
  * Returns indices of elements to move based on selected elements.
@@ -17,8 +19,11 @@ const getIndicesToMove = (
   let deletedIndices: number[] = [];
   let includeDeletedIndex = null;
   let index = -1;
+  const selectedElementIds = arrayToMap(
+    getSelectedElements(elements, appState, true),
+  );
   while (++index < elements.length) {
-    if (appState.selectedElementIds[elements[index].id]) {
+    if (selectedElementIds.get(elements[index].id)) {
       if (deletedIndices.length) {
         selectedIndices = selectedIndices.concat(deletedIndices);
         deletedIndices = [];
@@ -46,6 +51,45 @@ const toContiguousGroups = (array: number[]) => {
   }, [] as number[][]);
 };
 
+/**
+ * @returns index of target element, consindering tightly-bound elements
+ * (currently non-linear elements bound to a container) as a one unit.
+ * If no binding present, returns `undefined`.
+ */
+const getTargetIndexAccountingForBinding = (
+  nextElement: ExcalidrawElement,
+  elements: readonly ExcalidrawElement[],
+  direction: "left" | "right",
+) => {
+  if ("containerId" in nextElement && nextElement.containerId) {
+    if (direction === "left") {
+      const containerElement = Scene.getScene(nextElement)!.getElement(
+        nextElement.containerId,
+      );
+      if (containerElement) {
+        return elements.indexOf(containerElement);
+      }
+    } else {
+      return elements.indexOf(nextElement);
+    }
+  } else {
+    const boundElementId = nextElement.boundElements?.find(
+      (binding) => binding.type !== "arrow",
+    )?.id;
+    if (boundElementId) {
+      if (direction === "left") {
+        return elements.indexOf(nextElement);
+      }
+      const boundTextElement =
+        Scene.getScene(nextElement)!.getElement(boundElementId);
+
+      if (boundTextElement) {
+        return elements.indexOf(boundTextElement);
+      }
+    }
+  }
+};
+
 /**
  * Returns next candidate index that's available to be moved to. Currently that
  *  is a non-deleted element, and not inside a group (unless we're editing it).
@@ -86,7 +130,10 @@ const getTargetIndex = (
       // candidate element is a sibling in current editing group → return
       sourceElement?.groupIds.join("") === nextElement?.groupIds.join("")
     ) {
-      return candidateIndex;
+      return (
+        getTargetIndexAccountingForBinding(nextElement, elements, direction) ??
+        candidateIndex
+      );
     } else if (!nextElement?.groupIds.includes(appState.editingGroupId)) {
       // candidate element is outside current editing group → prevent
       return -1;
@@ -94,7 +141,10 @@ const getTargetIndex = (
   }
 
   if (!nextElement.groupIds.length) {
-    return candidateIndex;
+    return (
+      getTargetIndexAccountingForBinding(nextElement, elements, direction) ??
+      candidateIndex
+    );
   }
 
   const siblingGroupId = appState.editingGroupId