fix: binding text to non-bindable containers and not always preferring selection (#4655)

src/components/App.tsx

@@ -127,6 +127,7 @@ import {
   isInitializedImageElement,
   isLinearElement,
   isLinearElementType,
+  isTextBindableContainer,
 } from "../element/typeChecks";
 import {
   ExcalidrawBindableElement,
@@ -140,6 +141,7 @@ import {
   ExcalidrawImageElement,
   FileId,
   NonDeletedExcalidrawElement,
+  ExcalidrawTextContainer,
 } from "../element/types";
 import { getCenter, getDistance } from "../gesture";
 import {
@@ -167,7 +169,7 @@ import { renderScene } from "../renderer";
 import { invalidateShapeForElement } from "../renderer/renderElement";
 import {
   calculateScrollCenter,
-  getElementContainingPosition,
+  getTextBindableContainerAtPosition,
   getElementsAtPosition,
   getElementsWithinSelection,
   getNormalizedZoom,
@@ -238,7 +240,7 @@ import {
   bindTextToShapeAfterDuplication,
   getApproxMinLineHeight,
   getApproxMinLineWidth,
-  getBoundTextElementId,
+  getBoundTextElement,
 } from "../element/textElement";
 import { isHittingElementNotConsideringBoundingBox } from "../element/collision";
 import {
@@ -2157,28 +2159,40 @@ class App extends React.Component<AppProps, AppState> {
         window.devicePixelRatio,
       );
 
-    // bind to container when shouldBind is true or
-    // clicked on center of container
-    const container =
-      shouldBind || parentCenterPosition
-        ? getElementContainingPosition(
-            this.scene.getElements().filter((ele) => !isTextElement(ele)),
-            sceneX,
-            sceneY,
-          )
-        : null;
+    let existingTextElement: NonDeleted<ExcalidrawTextElement> | null = null;
+    let container: ExcalidrawTextContainer | null = null;
 
-    let existingTextElement = this.getTextElementAtPosition(sceneX, sceneY);
+    const selectedElements = getSelectedElements(
+      this.scene.getElements(),
+      this.state,
+    );
 
-    // consider bounded text element if container present
-    if (container) {
-      const boundTextElementId = getBoundTextElementId(container);
-      if (boundTextElementId) {
-        existingTextElement = this.scene.getElement(
-          boundTextElementId,
-        ) as ExcalidrawTextElement;
+    if (selectedElements.length === 1) {
+      if (isTextElement(selectedElements[0])) {
+        existingTextElement = selectedElements[0];
+      } else if (isTextBindableContainer(selectedElements[0])) {
+        container = selectedElements[0];
+        existingTextElement = getBoundTextElement(container);
       }
     }
+
+    existingTextElement =
+      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.getElements().filter((ele) => !isTextElement(ele)),
+        sceneX,
+        sceneY,
+      );
+    }
+
     if (!existingTextElement && container) {
       const fontString = {
         fontSize: this.state.currentItemFontSize,
@@ -5432,7 +5446,7 @@ class App extends React.Component<AppProps, AppState> {
     canvas: HTMLCanvasElement | null,
     scale: number,
   ) {
-    const elementClickedInside = getElementContainingPosition(
+    const elementClickedInside = getTextBindableContainerAtPosition(
       this.scene
         .getElementsIncludingDeleted()
         .filter((element) => !isTextElement(element)),

src/element/dragElements.ts

@@ -3,10 +3,9 @@ import { updateBoundElements } from "./binding";
 import { getCommonBounds } from "./bounds";
 import { mutateElement } from "./mutateElement";
 import { getPerfectElementSize } from "./sizeHelpers";
-import Scene from "../scene/Scene";
 import { NonDeletedExcalidrawElement } from "./types";
 import { AppState, PointerDownState } from "../types";
-import { getBoundTextElementId } from "./textElement";
+import { getBoundTextElement } from "./textElement";
 import { isSelectedViaGroup } from "../groups";
 
 export const dragSelectedElements = (
@@ -39,16 +38,14 @@ export const dragSelectedElements = (
       // container is part of a group, but we're dragging the container directly
       (appState.editingGroupId && !isSelectedViaGroup(appState, element))
     ) {
-      const boundTextElementId = getBoundTextElementId(element);
-      if (boundTextElementId) {
-        const textElement =
-          Scene.getScene(element)!.getElement(boundTextElementId);
+      const textElement = getBoundTextElement(element);
+      if (textElement) {
         updateElementCoords(
           lockDirection,
           distanceX,
           distanceY,
           pointerDownState,
-          textElement!,
+          textElement,
           offset,
         );
       }

src/element/newElement.ts

@@ -22,7 +22,6 @@ import { getElementAbsoluteCoords } from ".";
 import { adjustXYWithRotation } from "../math";
 import { getResizedElementAbsoluteCoords } from "./bounds";
 import { getContainerElement, measureText, wrapText } from "./textElement";
-import { isBoundToContainer } from "./typeChecks";
 import { BOUND_TEXT_PADDING, VERTICAL_ALIGN } from "../constants";
 
 type ElementConstructorOpts = MarkOptional<
@@ -221,8 +220,7 @@ const getAdjustedDimensions = (
 
   // make sure container dimensions are set properly when
   // text editor overflows beyond viewport dimensions
-  if (isBoundToContainer(element)) {
-    const container = getContainerElement(element)!;
+  if (container) {
     let height = container.height;
     let width = container.width;
     if (nextHeight > height - BOUND_TEXT_PADDING * 2) {

src/element/textElement.ts

@@ -1,6 +1,5 @@
 import { getFontString, arrayToMap, isTestEnv } from "../utils";
 import {
-  ExcalidrawBindableElement,
   ExcalidrawElement,
   ExcalidrawTextElement,
   ExcalidrawTextElementWithContainer,
@@ -12,6 +11,7 @@ import { BOUND_TEXT_PADDING, VERTICAL_ALIGN } from "../constants";
 import { MaybeTransformHandleType } from "./transformHandles";
 import Scene from "../scene/Scene";
 import { AppState } from "../types";
+import { isTextElement } from ".";
 
 export const redrawTextBoundingBox = (
   element: ExcalidrawTextElement,
@@ -79,22 +79,24 @@ export const bindTextToShapeAfterDuplication = (
     const boundTextElementId = getBoundTextElementId(element);
 
     if (boundTextElementId) {
-      const newTextElementId = oldIdToDuplicatedId.get(boundTextElementId)!;
-      mutateElement(
-        sceneElementMap.get(newElementId) as ExcalidrawBindableElement,
-        {
-          boundElements: element.boundElements?.concat({
-            type: "text",
-            id: newTextElementId,
-          }),
-        },
-      );
-      mutateElement(
-        sceneElementMap.get(newTextElementId) as ExcalidrawTextElement,
-        {
-          containerId: newElementId,
-        },
-      );
+      const newTextElementId = oldIdToDuplicatedId.get(boundTextElementId);
+      if (newTextElementId) {
+        const newContainer = sceneElementMap.get(newElementId);
+        if (newContainer) {
+          mutateElement(newContainer, {
+            boundElements: element.boundElements?.concat({
+              type: "text",
+              id: newTextElementId,
+            }),
+          });
+        }
+        const newTextElement = sceneElementMap.get(newTextElementId);
+        if (newTextElement && isTextElement(newTextElement)) {
+          mutateElement(newTextElement, {
+            containerId: newContainer ? newElementId : null,
+          });
+        }
+      }
     }
   });
 };
@@ -440,7 +442,10 @@ export const getApproxCharsToFitInWidth = (font: FontString, width: number) => {
 };
 
 export const getBoundTextElementId = (container: ExcalidrawElement | null) => {
-  return container?.boundElements?.filter((ele) => ele.type === "text")[0]?.id;
+  return container?.boundElements?.length
+    ? container?.boundElements?.filter((ele) => ele.type === "text")[0]?.id ||
+        null
+    : null;
 };
 
 export const getBoundTextElement = (element: ExcalidrawElement | null) => {

src/element/textWysiwyg.test.tsx

@@ -12,6 +12,8 @@ import {
   ExcalidrawTextElementWithContainer,
 } from "./types";
 import * as textElementUtils from "./textElement";
+import { API } from "../tests/helpers/api";
+import { mutateElement } from "./mutateElement";
 // Unmount ReactDOM from root
 ReactDOM.unmountComponentAtNode(document.getElementById("root")!);
 
@@ -19,7 +21,201 @@ const tab = "    ";
 const mouse = new Pointer("mouse");
 
 describe("textWysiwyg", () => {
-  describe("Test unbounded text", () => {
+  describe("start text editing", () => {
+    const { h } = window;
+    beforeEach(async () => {
+      await render(<ExcalidrawApp />);
+      h.elements = [];
+    });
+
+    it("should prefer editing selected text element (non-bindable container present)", async () => {
+      const line = API.createElement({
+        type: "line",
+        width: 100,
+        height: 0,
+        points: [
+          [0, 0],
+          [100, 0],
+        ],
+      });
+      const textSize = 20;
+      const text = API.createElement({
+        type: "text",
+        text: "ola",
+        x: line.width / 2 - textSize / 2,
+        y: -textSize / 2,
+        width: textSize,
+        height: textSize,
+      });
+      h.elements = [text, line];
+
+      API.setSelectedElements([text]);
+
+      Keyboard.keyPress(KEYS.ENTER);
+
+      expect(h.state.editingElement?.id).toBe(text.id);
+      expect(
+        (h.state.editingElement as ExcalidrawTextElement).containerId,
+      ).toBe(null);
+    });
+
+    it("should prefer editing selected text element (bindable container present)", async () => {
+      const container = API.createElement({
+        type: "rectangle",
+        width: 100,
+        boundElements: [],
+      });
+      const textSize = 20;
+
+      const boundText = API.createElement({
+        type: "text",
+        text: "ola",
+        x: container.width / 2 - textSize / 2,
+        y: container.height / 2 - textSize / 2,
+        width: textSize,
+        height: textSize,
+        containerId: container.id,
+      });
+
+      const boundText2 = API.createElement({
+        type: "text",
+        text: "ola",
+        x: container.width / 2 - textSize / 2,
+        y: container.height / 2 - textSize / 2,
+        width: textSize,
+        height: textSize,
+        containerId: container.id,
+      });
+
+      h.elements = [container, boundText, boundText2];
+
+      mutateElement(container, {
+        boundElements: [{ type: "text", id: boundText.id }],
+      });
+
+      API.setSelectedElements([boundText2]);
+
+      Keyboard.keyPress(KEYS.ENTER);
+
+      expect(h.state.editingElement?.id).toBe(boundText2.id);
+    });
+
+    it("should not create bound text on ENTER if text exists at container center", () => {
+      const container = API.createElement({
+        type: "rectangle",
+        width: 100,
+      });
+      const textSize = 20;
+      const text = API.createElement({
+        type: "text",
+        text: "ola",
+        x: container.width / 2 - textSize / 2,
+        y: container.height / 2 - textSize / 2,
+        width: textSize,
+        height: textSize,
+        containerId: container.id,
+      });
+
+      h.elements = [container, text];
+
+      API.setSelectedElements([container]);
+
+      Keyboard.keyPress(KEYS.ENTER);
+
+      expect(h.state.editingElement?.id).toBe(text.id);
+    });
+
+    it("should edit existing bound text on ENTER even if higher z-index unbound text exists at container center", () => {
+      const container = API.createElement({
+        type: "rectangle",
+        width: 100,
+        boundElements: [],
+      });
+      const textSize = 20;
+
+      const boundText = API.createElement({
+        type: "text",
+        text: "ola",
+        x: container.width / 2 - textSize / 2,
+        y: container.height / 2 - textSize / 2,
+        width: textSize,
+        height: textSize,
+        containerId: container.id,
+      });
+
+      const boundText2 = API.createElement({
+        type: "text",
+        text: "ola",
+        x: container.width / 2 - textSize / 2,
+        y: container.height / 2 - textSize / 2,
+        width: textSize,
+        height: textSize,
+        containerId: container.id,
+      });
+
+      h.elements = [container, boundText, boundText2];
+
+      mutateElement(container, {
+        boundElements: [{ type: "text", id: boundText.id }],
+      });
+
+      API.setSelectedElements([container]);
+
+      Keyboard.keyPress(KEYS.ENTER);
+
+      expect(h.state.editingElement?.id).toBe(boundText.id);
+    });
+
+    it("should edit text under cursor when clicked with text tool", () => {
+      const text = API.createElement({
+        type: "text",
+        text: "ola",
+        x: 60,
+        y: 0,
+        width: 100,
+        height: 100,
+      });
+
+      h.elements = [text];
+      UI.clickTool("text");
+
+      mouse.clickAt(text.x + 50, text.y + 50);
+
+      const editor = document.querySelector(
+        ".excalidraw-textEditorContainer > textarea",
+      ) as HTMLTextAreaElement;
+
+      expect(editor).not.toBe(null);
+      expect(h.state.editingElement?.id).toBe(text.id);
+      expect(h.elements.length).toBe(1);
+    });
+
+    it("should edit text under cursor when double-clicked with selection tool", () => {
+      const text = API.createElement({
+        type: "text",
+        text: "ola",
+        x: 60,
+        y: 0,
+        width: 100,
+        height: 100,
+      });
+
+      h.elements = [text];
+      UI.clickTool("selection");
+
+      mouse.doubleClickAt(text.x + 50, text.y + 50);
+
+      const editor = document.querySelector(
+        ".excalidraw-textEditorContainer > textarea",
+      ) as HTMLTextAreaElement;
+
+      expect(editor).not.toBe(null);
+      expect(h.state.editingElement?.id).toBe(text.id);
+      expect(h.elements.length).toBe(1);
+    });
+  });
+
+  describe("Test container-unbound text", () => {
     const { h } = window;
 
     let textarea: HTMLTextAreaElement;
@@ -235,7 +431,7 @@ describe("textWysiwyg", () => {
     });
   });
 
-  describe("Test bounded text", () => {
+  describe("Test container-bound text", () => {
     let rectangle: any;
     const { h } = window;
 
@@ -315,6 +511,39 @@ describe("textWysiwyg", () => {
       ]);
     });
 
+    it("shouldn't bind to non-text-bindable containers", async () => {
+      const line = API.createElement({
+        type: "line",
+        width: 100,
+        height: 0,
+        points: [
+          [0, 0],
+          [100, 0],
+        ],
+      });
+      h.elements = [line];
+
+      UI.clickTool("text");
+
+      mouse.clickAt(line.x + line.width / 2, line.y + line.height / 2);
+
+      const editor = document.querySelector(
+        ".excalidraw-textEditorContainer > textarea",
+      ) as HTMLTextAreaElement;
+
+      fireEvent.change(editor, {
+        target: {
+          value: "Hello World!",
+        },
+      });
+      fireEvent.keyDown(editor, { key: KEYS.ESCAPE });
+      editor.dispatchEvent(new Event("input"));
+
+      expect(line.boundElements).toBe(null);
+      expect(h.elements[1].type).toBe("text");
+      expect((h.elements[1] as ExcalidrawTextElement).containerId).toBe(null);
+    });
+
     it("should update font family correctly on undo/redo by selecting bounded text when font family was updated", async () => {
       expect(h.elements.length).toBe(1);
 

src/element/typeChecks.ts

@@ -8,6 +8,7 @@ import {
   InitializedExcalidrawImageElement,
   ExcalidrawImageElement,
   ExcalidrawTextElementWithContainer,
+  ExcalidrawTextContainer,
 } from "./types";
 
 export const isGenericElement = (
@@ -91,7 +92,9 @@ export const isBindableElement = (
   );
 };
 
-export const isTextBindableContainer = (element: ExcalidrawElement | null) => {
+export const isTextBindableContainer = (
+  element: ExcalidrawElement | null,
+): element is ExcalidrawTextContainer => {
   return (
     element != null &&
     (element.type === "rectangle" ||

src/element/types.ts

@@ -135,8 +135,14 @@ export type ExcalidrawBindableElement =
   | ExcalidrawTextElement
   | ExcalidrawImageElement;
 
+export type ExcalidrawTextContainer =
+  | ExcalidrawRectangleElement
+  | ExcalidrawDiamondElement
+  | ExcalidrawEllipseElement
+  | ExcalidrawImageElement;
+
 export type ExcalidrawTextElementWithContainer = {
-  containerId: ExcalidrawGenericElement["id"];
+  containerId: ExcalidrawTextContainer["id"];
 } & ExcalidrawTextElement;
 
 export type PointBinding = {

src/groups.ts

@@ -1,13 +1,7 @@
-import {
-  GroupId,
-  ExcalidrawElement,
-  NonDeleted,
-  ExcalidrawTextElementWithContainer,
-} from "./element/types";
+import { GroupId, ExcalidrawElement, NonDeleted } from "./element/types";
 import { AppState } from "./types";
 import { getSelectedElements } from "./scene";
-import { getBoundTextElementId } from "./element/textElement";
-import Scene from "./scene/Scene";
+import { getBoundTextElement } from "./element/textElement";
 
 export const selectGroup = (
   groupId: GroupId,
@@ -182,13 +176,10 @@ export const getMaximumGroups = (
 
     const currentGroupMembers = groups.get(groupId) || [];
 
-    // Include bounded text if present when grouping
-    const boundTextElementId = getBoundTextElementId(element);
-    if (boundTextElementId) {
-      const textElement = Scene.getScene(element)!.getElement(
-        boundTextElementId,
-      ) as ExcalidrawTextElementWithContainer;
-      currentGroupMembers.push(textElement);
+    // Include bound text if present when grouping
+    const boundTextElement = getBoundTextElement(element);
+    if (boundTextElement) {
+      currentGroupMembers.push(boundTextElement);
     }
     groups.set(groupId, [...currentGroupMembers, element]);
   });

src/scene/comparisons.ts

@@ -1,9 +1,11 @@
 import {
   ExcalidrawElement,
+  ExcalidrawTextContainer,
   NonDeletedExcalidrawElement,
 } from "../element/types";
 
 import { getElementAbsoluteCoords } from "../element";
+import { isTextBindableContainer } from "../element/typeChecks";
 
 export const hasBackground = (type: string) =>
   type === "rectangle" ||
@@ -72,11 +74,11 @@ export const getElementsAtPosition = (
   );
 };
 
-export const getElementContainingPosition = (
+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) {
@@ -89,5 +91,5 @@ export const getElementContainingPosition = (
       break;
     }
   }
-  return hitElement;
+  return isTextBindableContainer(hitElement) ? hitElement : null;
 };

src/scene/index.ts

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

src/tests/binding.test.tsx

@@ -152,6 +152,7 @@ describe("element binding", () => {
     UI.clickTool("text");
 
     mouse.clickAt(text.x + 50, text.y + 50);
+
     const editor = document.querySelector(
       ".excalidraw-textEditorContainer > textarea",
     ) as HTMLTextAreaElement;

src/tests/data/__snapshots__/restore.test.ts.snap

@@ -9,7 +9,7 @@ Object {
   "endBinding": null,
   "fillStyle": "hachure",
   "groupIds": Array [],
-  "height": 0,
+  "height": 100,
   "id": "id-arrow01",
   "isDeleted": false,
   "lastCommittedPoint": null,
@@ -21,8 +21,8 @@ Object {
       0,
     ],
     Array [
-      0,
-      0,
+      100,
+      100,
     ],
   ],
   "roughness": 1,
@@ -37,7 +37,7 @@ Object {
   "updated": 1,
   "version": 1,
   "versionNonce": 0,
-  "width": 0,
+  "width": 100,
   "x": 0,
   "y": 0,
 }
@@ -180,7 +180,7 @@ Object {
   "endBinding": null,
   "fillStyle": "hachure",
   "groupIds": Array [],
-  "height": 0,
+  "height": 100,
   "id": "id-line01",
   "isDeleted": false,
   "lastCommittedPoint": null,
@@ -192,8 +192,8 @@ Object {
       0,
     ],
     Array [
-      0,
-      0,
+      100,
+      100,
     ],
   ],
   "roughness": 1,
@@ -208,7 +208,7 @@ Object {
   "updated": 1,
   "version": 1,
   "versionNonce": 0,
-  "width": 0,
+  "width": 100,
   "x": 0,
   "y": 0,
 }
@@ -223,7 +223,7 @@ Object {
   "endBinding": null,
   "fillStyle": "hachure",
   "groupIds": Array [],
-  "height": 0,
+  "height": 100,
   "id": "id-draw01",
   "isDeleted": false,
   "lastCommittedPoint": null,
@@ -235,8 +235,8 @@ Object {
       0,
     ],
     Array [
-      0,
-      0,
+      100,
+      100,
     ],
   ],
   "roughness": 1,
@@ -251,7 +251,7 @@ Object {
   "updated": 1,
   "version": 1,
   "versionNonce": 0,
-  "width": 0,
+  "width": 100,
   "x": 0,
   "y": 0,
 }

src/tests/helpers/api.ts

@@ -14,6 +14,7 @@ import util from "util";
 import path from "path";
 import { getMimeType } from "../../data/blob";
 import { newFreeDrawElement } from "../../element/newElement";
+import { Point } from "../../types";
 
 const readFile = util.promisify(fs.readFile);
 
@@ -98,6 +99,7 @@ export class API {
     containerId?: T extends "text"
       ? ExcalidrawTextElement["containerId"]
       : never;
+    points?: T extends "arrow" | "line" ? readonly Point[] : never;
   }): T extends "arrow" | "line"
     ? ExcalidrawLinearElement
     : T extends "freedraw"
@@ -158,10 +160,13 @@ export class API {
       case "arrow":
       case "line":
         element = newLinearElement({
+          ...base,
+          width,
+          height,
           type: type as "arrow" | "line",
           startArrowhead: null,
           endArrowhead: null,
-          ...base,
+          points: rest.points ?? [],
         });
         break;
     }