fix: Always bind to container selected by user (#5880)

* fix: Always bind to container selected by user

* Don't bind to container when using text tool

* adjust z-index for bound text

* fix

* Add spec

* Add test

* Allow double click on transparent container and add spec

* fix spec

* adjust z-index only when binding

* update index

* fix

* add index check

* Update src/scene/Scene.ts

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

src/components/App.tsx

@@ -133,7 +133,6 @@ import {
   isInitializedImageElement,
   isLinearElement,
   isLinearElementType,
-  isTextBindableContainer,
 } from "../element/typeChecks";
 import {
   ExcalidrawBindableElement,
@@ -175,7 +174,6 @@ import { renderScene } from "../renderer/renderScene";
 import { invalidateShapeForElement } from "../renderer/renderElement";
 import {
   calculateScrollCenter,
-  getTextBindableContainerAtPosition,
   getElementsAtPosition,
   getElementsWithinSelection,
   getNormalizedZoom,
@@ -255,6 +253,7 @@ import {
   getApproxMinLineWidth,
   getBoundTextElement,
   getContainerDims,
+  getTextBindableContainerAtPosition,
   isValidTextContainer,
 } from "../element/textElement";
 import { isHittingElementNotConsideringBoundingBox } from "../element/collision";
@@ -1990,10 +1989,14 @@ class App extends React.Component<AppProps, AppState> {
             isTextElement(selectedElement) ||
             isValidTextContainer(selectedElement)
           ) {
+            let container;
+            if (!isTextElement(selectedElement)) {
+              container = selectedElement as ExcalidrawTextContainer;
+            }
             this.startTextEditing({
               sceneX: selectedElement.x + selectedElement.width / 2,
               sceneY: selectedElement.y + selectedElement.height / 2,
-              shouldBind: true,
+              container,
             });
             event.preventDefault();
             return;
@@ -2317,7 +2320,6 @@ class App extends React.Component<AppProps, AppState> {
     const element = this.getElementAtPosition(x, y, {
       includeBoundTextElement: true,
     });
-
     if (element && isTextElement(element) && !element.isDeleted) {
       return element;
     }
@@ -2394,29 +2396,31 @@ class App extends React.Component<AppProps, AppState> {
   private startTextEditing = ({
     sceneX,
     sceneY,
-    shouldBind,
     insertAtParentCenter = true,
+    container,
   }: {
     /** X position to insert text at */
     sceneX: number;
     /** Y position to insert text at */
     sceneY: number;
-    shouldBind: boolean;
     /** whether to attempt to insert at element center if applicable */
     insertAtParentCenter?: boolean;
+    container?: ExcalidrawTextContainer | null;
   }) => {
+    let shouldBindToContainer = false;
+
     let parentCenterPosition =
       insertAtParentCenter &&
       this.getTextWysiwygSnappedToCenterPosition(
         sceneX,
         sceneY,
         this.state,
-        this.canvas,
-        window.devicePixelRatio,
+        container,
       );
-
+    if (container && parentCenterPosition) {
+      shouldBindToContainer = true;
+    }
     let existingTextElement: NonDeleted<ExcalidrawTextElement> | null = null;
-    let container: ExcalidrawTextContainer | null = null;
 
     const selectedElements = getSelectedElements(
       this.scene.getNonDeletedElements(),
@@ -2426,7 +2430,7 @@ class App extends React.Component<AppProps, AppState> {
     if (selectedElements.length === 1) {
       if (isTextElement(selectedElements[0])) {
         existingTextElement = selectedElements[0];
-      } else if (isTextBindableContainer(selectedElements[0], false)) {
+      } else if (container) {
         existingTextElement = getBoundTextElement(selectedElements[0]);
       } else {
         existingTextElement = this.getTextElementAtPosition(sceneX, sceneY);
@@ -2435,26 +2439,7 @@ class App extends React.Component<AppProps, AppState> {
       existingTextElement = this.getTextElementAtPosition(sceneX, sceneY);
     }
 
-    // bind to container when shouldBind is true or
-    // clicked on center of container
-    if (
-      !container &&
-      !existingTextElement &&
-      (shouldBind || parentCenterPosition)
-    ) {
-      container = getTextBindableContainerAtPosition(
-        this.scene
-          .getNonDeletedElements()
-          .filter(
-            (ele) =>
-              isTextBindableContainer(ele, false) && !getBoundTextElement(ele),
-          ),
-        sceneX,
-        sceneY,
-      );
-    }
-
-    if (!existingTextElement && container) {
+    if (!existingTextElement && shouldBindToContainer && container) {
       const fontString = {
         fontSize: this.state.currentItemFontSize,
         fontFamily: this.state.currentItemFontFamily,
@@ -2472,12 +2457,10 @@ class App extends React.Component<AppProps, AppState> {
           sceneX,
           sceneY,
           this.state,
-          this.canvas,
-          window.devicePixelRatio,
+          container,
         );
       }
     }
-
     const element = existingTextElement
       ? existingTextElement
       : newTextElement({
@@ -2504,7 +2487,7 @@ class App extends React.Component<AppProps, AppState> {
           verticalAlign: parentCenterPosition
             ? VERTICAL_ALIGN.MIDDLE
             : DEFAULT_VERTICAL_ALIGN,
-          containerId: container?.id ?? undefined,
+          containerId: shouldBindToContainer ? container?.id : undefined,
           groupIds: container?.groupIds ?? [],
           locked: false,
         });
@@ -2512,10 +2495,15 @@ class App extends React.Component<AppProps, AppState> {
     this.setState({ editingElement: element });
 
     if (!existingTextElement) {
-      this.scene.replaceAllElements([
-        ...this.scene.getElementsIncludingDeleted(),
-        element,
-      ]);
+      if (container && shouldBindToContainer) {
+        const containerIndex = this.scene.getElementIndex(container.id);
+        this.scene.insertElementAtIndex(element, containerIndex + 1);
+      } else {
+        this.scene.replaceAllElements([
+          ...this.scene.getElementsIncludingDeleted(),
+          element,
+        ]);
+      }
 
       // case: creating new text not centered to parent element → offset Y
       // so that the text is centered to cursor position
@@ -2603,22 +2591,23 @@ class App extends React.Component<AppProps, AppState> {
 
     resetCursor(this.canvas);
     if (!event[KEYS.CTRL_OR_CMD] && !this.state.viewModeEnabled) {
-      const selectedElements = getSelectedElements(
+      const container = getTextBindableContainerAtPosition(
         this.scene.getNonDeletedElements(),
         this.state,
+        sceneX,
+        sceneY,
       );
-      if (selectedElements.length === 1) {
-        const selectedElement = selectedElements[0];
-        if (hasBoundTextElement(selectedElement)) {
-          sceneX = selectedElement.x + selectedElement.width / 2;
-          sceneY = selectedElement.y + selectedElement.height / 2;
+      if (container) {
+        if (hasBoundTextElement(container)) {
+          sceneX = container.x + container.width / 2;
+          sceneY = container.y + container.height / 2;
         }
       }
       this.startTextEditing({
         sceneX,
         sceneY,
-        shouldBind: false,
         insertAtParentCenter: !event.altKey,
+        container,
       });
     }
   };
@@ -3911,15 +3900,23 @@ class App extends React.Component<AppProps, AppState> {
       includeBoundTextElement: true,
     });
 
+    let container = getTextBindableContainerAtPosition(
+      this.scene.getNonDeletedElements(),
+      this.state,
+      sceneX,
+      sceneY,
+    );
+
     if (hasBoundTextElement(element)) {
+      container = element as ExcalidrawTextContainer;
       sceneX = element.x + element.width / 2;
       sceneY = element.y + element.height / 2;
     }
     this.startTextEditing({
       sceneX,
       sceneY,
-      shouldBind: false,
       insertAtParentCenter: !event.altKey,
+      container,
     });
 
     resetCursor(this.canvas);
@@ -6171,21 +6168,11 @@ class App extends React.Component<AppProps, AppState> {
     x: number,
     y: number,
     appState: AppState,
-    canvas: HTMLCanvasElement | null,
-    scale: number,
+    container?: ExcalidrawTextContainer | null,
   ) {
-    const elementClickedInside = getTextBindableContainerAtPosition(
-      this.scene
-        .getElementsIncludingDeleted()
-        .filter((element) => !isTextElement(element)),
-      x,
-      y,
-    );
-    if (elementClickedInside) {
-      const elementCenterX =
-        elementClickedInside.x + elementClickedInside.width / 2;
-      const elementCenterY =
-        elementClickedInside.y + elementClickedInside.height / 2;
+    if (container) {
+      const elementCenterX = container.x + container.width / 2;
+      const elementCenterY = container.y + container.height / 2;
       const distanceToCenter = Math.hypot(
         x - elementCenterX,
         y - elementCenterY,

src/element/textElement.ts

@@ -1,6 +1,7 @@
 import { getFontString, arrayToMap, isTestEnv } from "../utils";
 import {
   ExcalidrawElement,
+  ExcalidrawTextContainer,
   ExcalidrawTextElement,
   ExcalidrawTextElementWithContainer,
   FontString,
@@ -12,6 +13,10 @@ import { MaybeTransformHandleType } from "./transformHandles";
 import Scene from "../scene/Scene";
 import { isTextElement } from ".";
 import { getMaxContainerHeight, getMaxContainerWidth } from "./newElement";
+import { isTextBindableContainer } from "./typeChecks";
+import { getElementAbsoluteCoords } from "../element";
+import { AppState } from "../types";
+import { getSelectedElements } from "../scene";
 import { isImageElement } from "./typeChecks";
 
 export const redrawTextBoundingBox = (
@@ -492,6 +497,32 @@ export const getContainerDims = (element: ExcalidrawElement) => {
   return { width: element.width, height: element.height };
 };
 
+export const getTextBindableContainerAtPosition = (
+  elements: readonly ExcalidrawElement[],
+  appState: AppState,
+  x: number,
+  y: number,
+): ExcalidrawTextContainer | null => {
+  const selectedElements = getSelectedElements(elements, appState);
+  if (selectedElements.length === 1) {
+    return selectedElements[0] as ExcalidrawTextContainer;
+  }
+  let hitElement = null;
+  // We need to to hit testing from front (end of the array) to back (beginning of the array)
+  for (let index = elements.length - 1; index >= 0; --index) {
+    if (elements[index].isDeleted) {
+      continue;
+    }
+    const [x1, y1, x2, y2] = getElementAbsoluteCoords(elements[index]);
+    if (x1 < x && x < x2 && y1 < y && y < y2) {
+      hitElement = elements[index];
+      break;
+    }
+  }
+
+  return isTextBindableContainer(hitElement, false) ? hitElement : null;
+};
+
 export const isValidTextContainer = (element: ExcalidrawElement) => {
   return (
     element.type === "rectangle" ||

src/element/textWysiwyg.test.tsx

@@ -500,7 +500,7 @@ describe("textWysiwyg", () => {
       });
     });
 
-    it("should bind text to container when double clicked on center", async () => {
+    it("should bind text to container when double clicked on center of filled container", async () => {
       expect(h.elements.length).toBe(1);
       expect(h.elements[0].id).toBe(rectangle.id);
 
@@ -527,6 +527,40 @@ describe("textWysiwyg", () => {
       ]);
     });
 
+    it("should bind text to container when double clicked on center of transparent container", async () => {
+      const rectangle = API.createElement({
+        type: "rectangle",
+        x: 10,
+        y: 20,
+        width: 90,
+        height: 75,
+        backgroundColor: "transparent",
+      });
+      h.elements = [rectangle];
+
+      mouse.doubleClickAt(
+        rectangle.x + rectangle.width / 2,
+        rectangle.y + rectangle.height / 2,
+      );
+      expect(h.elements.length).toBe(2);
+
+      const text = h.elements[1] as ExcalidrawTextElementWithContainer;
+      expect(text.type).toBe("text");
+      expect(text.containerId).toBe(rectangle.id);
+      mouse.down();
+      const editor = document.querySelector(
+        ".excalidraw-textEditorContainer > textarea",
+      ) as HTMLTextAreaElement;
+
+      fireEvent.change(editor, { target: { value: "Hello World!" } });
+
+      await new Promise((r) => setTimeout(r, 0));
+      editor.blur();
+      expect(rectangle.boundElements).toStrictEqual([
+        { id: text.id, type: "text" },
+      ]);
+    });
+
     it("should bind text to container when clicked on container and enter pressed", async () => {
       expect(h.elements.length).toBe(1);
       expect(h.elements[0].id).toBe(rectangle.id);
@@ -825,9 +859,9 @@ describe("textWysiwyg", () => {
       expect(h.elements.length).toBe(2);
 
       // Bind first text
-      let text = h.elements[1] as ExcalidrawTextElementWithContainer;
+      const text = h.elements[1] as ExcalidrawTextElementWithContainer;
       expect(text.containerId).toBe(rectangle.id);
-      let editor = document.querySelector(
+      const editor = document.querySelector(
         ".excalidraw-textEditorContainer > textarea",
       ) as HTMLTextAreaElement;
       await new Promise((r) => setTimeout(r, 0));
@@ -837,25 +871,14 @@ describe("textWysiwyg", () => {
         { id: text.id, type: "text" },
       ]);
 
-      // Attempt to bind another text
-      UI.clickTool("text");
-      mouse.clickAt(
-        rectangle.x + rectangle.width / 2,
-        rectangle.y + rectangle.height / 2,
-      );
-      mouse.down();
-      expect(h.elements.length).toBe(3);
-      text = h.elements[2] as ExcalidrawTextElementWithContainer;
-      editor = document.querySelector(
-        ".excalidraw-textEditorContainer > textarea",
-      ) as HTMLTextAreaElement;
-      await new Promise((r) => setTimeout(r, 0));
-      fireEvent.change(editor, { target: { value: "Whats up?" } });
-      editor.blur();
+      mouse.select(rectangle);
+      Keyboard.keyPress(KEYS.ENTER);
+      expect(h.elements.length).toBe(2);
+
       expect(rectangle.boundElements).toStrictEqual([
         { id: h.elements[1].id, type: "text" },
       ]);
-      expect(text.containerId).toBe(null);
+      expect(text.containerId).toBe(rectangle.id);
     });
 
     it("should respect text alignment when resizing", async () => {
@@ -1045,5 +1068,36 @@ describe("textWysiwyg", () => {
         `"Wikipedia is hosted by the Wikimedia Foundation, a non-profit organization that also hosts a range of other projects.Hello this text should get merged with the existing one"`,
       );
     });
+
+    it("should always bind to selected container and insert it in correct position", async () => {
+      const rectangle2 = UI.createElement("rectangle", {
+        x: 5,
+        y: 10,
+        width: 120,
+        height: 100,
+      });
+
+      API.setSelectedElements([rectangle]);
+      Keyboard.keyPress(KEYS.ENTER);
+
+      expect(h.elements.length).toBe(3);
+      expect(h.elements[1].type).toBe("text");
+      const text = h.elements[1] as ExcalidrawTextElementWithContainer;
+      expect(text.type).toBe("text");
+      expect(text.containerId).toBe(rectangle.id);
+      mouse.down();
+      const editor = document.querySelector(
+        ".excalidraw-textEditorContainer > textarea",
+      ) as HTMLTextAreaElement;
+
+      fireEvent.change(editor, { target: { value: "Hello World!" } });
+
+      await new Promise((r) => setTimeout(r, 0));
+      editor.blur();
+      expect(rectangle2.boundElements).toBeNull();
+      expect(rectangle.boundElements).toStrictEqual([
+        { id: text.id, type: "text" },
+      ]);
+    });
   });
 });

src/scene/Scene.ts

@@ -150,6 +150,24 @@ class Scene {
     // (I guess?)
     this.callbacks.clear();
   }
+
+  insertElementAtIndex(element: ExcalidrawElement, index: number) {
+    if (!Number.isFinite(index) || index < 0) {
+      throw new Error(
+        "insertElementAtIndex can only be called with index >= 0",
+      );
+    }
+    const nextElements = [
+      ...this.elements.slice(0, index),
+      element,
+      ...this.elements.slice(index),
+    ];
+    this.replaceAllElements(nextElements);
+  }
+
+  getElementIndex(elementId: string) {
+    return this.elements.findIndex((element) => element.id === elementId);
+  }
 }
 
 export default Scene;

src/scene/comparisons.ts

@@ -1,11 +1,4 @@
-import {
-  ExcalidrawElement,
-  ExcalidrawTextContainer,
-  NonDeletedExcalidrawElement,
-} from "../element/types";
-
-import { getElementAbsoluteCoords } from "../element";
-import { isTextBindableContainer } from "../element/typeChecks";
+import { NonDeletedExcalidrawElement } from "../element/types";
 
 export const hasBackground = (type: string) =>
   type === "rectangle" ||
@@ -73,23 +66,3 @@ export const getElementsAtPosition = (
     (element) => !element.isDeleted && isAtPositionFn(element),
   );
 };
-
-export const getTextBindableContainerAtPosition = (
-  elements: readonly ExcalidrawElement[],
-  x: number,
-  y: number,
-): ExcalidrawTextContainer | null => {
-  let hitElement = null;
-  // We need to to hit testing from front (end of the array) to back (beginning of the array)
-  for (let index = elements.length - 1; index >= 0; --index) {
-    if (elements[index].isDeleted) {
-      continue;
-    }
-    const [x1, y1, x2, y2] = getElementAbsoluteCoords(elements[index]);
-    if (x1 < x && x < x2 && y1 < y && y < y2) {
-      hitElement = elements[index];
-      break;
-    }
-  }
-  return isTextBindableContainer(hitElement, false) ? hitElement : null;
-};

src/scene/index.ts

@@ -14,7 +14,6 @@ export {
   canHaveArrowheads,
   canChangeSharpness,
   getElementAtPosition,
-  getTextBindableContainerAtPosition,
   hasText,
   getElementsAtPosition,
 } from "./comparisons";

src/tests/elementLocking.test.tsx

@@ -232,7 +232,7 @@ describe("element locking", () => {
     API.setSelectedElements([container]);
     Keyboard.keyPress(KEYS.ENTER);
     expect(h.state.editingElement?.id).not.toBe(text.id);
-    expect(h.state.editingElement?.id).toBe(h.elements[2].id);
+    expect(h.state.editingElement?.id).toBe(h.elements[1].id);
   });
 
   it("should ignore locked text under cursor when clicked with text tool", () => {