fix: don't mutate the bounded text if not updated when submitted (#4543)

* fix: don't mutate the bounded text if not updated when submitted

* dont update text for bounded text unless submitted

* add specs

* use node 16

* fix

* Update text when editing and cache prev text

* update prev text when props updated

* remove only

* type properly and remove unnecessary type checks

* cache original text and compare with editor value to fix alignement issue after editing and add specs

* naming tweak

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

.github/workflows/test.yml

@@ -10,7 +10,7 @@ jobs:
       - name: Setup Node.js 14.x
         uses: actions/setup-node@v2
         with:
-          node-version: 14.x
+          node-version: 16.x
       - name: Install and test
         run: |
           yarn --frozen-lockfile

src/components/App.tsx

@@ -1911,8 +1911,8 @@ class App extends React.Component<AppProps, AppState> {
     const updateElement = (
       text: string,
       originalText: string,
-      isDeleted = false,
-      updateDimensions = false,
+      isDeleted: boolean,
+      isSubmit: boolean,
     ) => {
       this.scene.replaceAllElements([
         ...this.scene.getElementsIncludingDeleted().map((_element) => {
@@ -1924,7 +1924,7 @@ class App extends React.Component<AppProps, AppState> {
                 isDeleted,
                 originalText,
               },
-              updateDimensions,
+              isSubmit,
             );
           }
           return _element;
@@ -1950,7 +1950,7 @@ class App extends React.Component<AppProps, AppState> {
         ];
       },
       onChange: withBatchedUpdates((text) => {
-        updateElement(text, text, false, !element.containerId);
+        updateElement(text, text, false, false);
         if (isNonDeletedElement(element)) {
           updateBoundElements(element);
         }
@@ -1996,7 +1996,7 @@ class App extends React.Component<AppProps, AppState> {
 
     // do an initial update to re-initialize element position since we were
     // modifying element's x/y for sake of editor (case: syncing to remote)
-    updateElement(element.text, element.originalText);
+    updateElement(element.text, element.originalText, false, false);
   }
 
   private deselectElements() {

src/element/newElement.ts

@@ -158,7 +158,11 @@ const getAdjustedDimensions = (
   height: number;
   baseline: number;
 } => {
-  const maxWidth = element.containerId ? element.width : null;
+  let maxWidth = null;
+  if (element.containerId) {
+    const container = Scene.getScene(element)!.getElement(element.containerId)!;
+    maxWidth = container.width - BOUND_TEXT_PADDING * 2;
+  }
   const {
     width: nextWidth,
     height: nextHeight,
@@ -246,11 +250,15 @@ export const updateTextElement = (
     originalText,
   }: { text: string; isDeleted?: boolean; originalText: string },
 
-  updateDimensions: boolean,
+  isSubmit: boolean,
 ): ExcalidrawTextElement => {
-  const dimensions = updateDimensions
-    ? getAdjustedDimensions(element, text)
-    : undefined;
+  const boundToContainer = isBoundToContainer(element);
+
+  // Don't update dimensions and text value for bounded text unless submitted
+  const dimensions =
+    boundToContainer && !isSubmit
+      ? undefined
+      : getAdjustedDimensions(element, text);
   return newElementWith(element, {
     text,
     originalText,

src/element/textWysiwyg.test.tsx

@@ -1,22 +1,27 @@
 import ReactDOM from "react-dom";
 import ExcalidrawApp from "../excalidraw-app";
-import { render } from "../tests/test-utils";
-import { Pointer, UI } from "../tests/helpers/ui";
+import { render, screen } from "../tests/test-utils";
+import { Keyboard, Pointer, UI } from "../tests/helpers/ui";
 import { KEYS } from "../keys";
-
+import { fireEvent } from "../tests/test-utils";
+import { BOUND_TEXT_PADDING, FONT_FAMILY } from "../constants";
+import { ExcalidrawTextElementWithContainer } from "./types";
+import * as textElementUtils from "./textElement";
 // Unmount ReactDOM from root
 ReactDOM.unmountComponentAtNode(document.getElementById("root")!);
 
 const tab = "    ";
+const mouse = new Pointer("mouse");
 
 describe("textWysiwyg", () => {
+  describe("Test unbounded text", () => {
     let textarea: HTMLTextAreaElement;
     beforeEach(async () => {
       await render(<ExcalidrawApp />);
 
       const element = UI.createElement("text");
 
-    new Pointer("mouse").clickOn(element);
+      mouse.clickOn(element);
       textarea = document.querySelector(
         ".excalidraw-textEditorContainer > textarea",
       )!;
@@ -166,4 +171,243 @@ describe("textWysiwyg", () => {
 
       expect(textarea.value).toEqual(`Line#1\nLine#2`);
     });
+  });
+  describe("Test bounded text", () => {
+    let rectangle: any;
+    const {
+      h,
+    }: {
+      h: {
+        elements: any;
+      };
+    } = window;
+
+    const DUMMY_HEIGHT = 240;
+    const DUMMY_WIDTH = 160;
+    const APPROX_LINE_HEIGHT = 25;
+    const INITIAL_WIDTH = 10;
+
+    beforeAll(() => {
+      jest
+        .spyOn(textElementUtils, "getApproxLineHeight")
+        .mockReturnValue(APPROX_LINE_HEIGHT);
+    });
+
+    beforeEach(async () => {
+      await render(<ExcalidrawApp />);
+
+      rectangle = UI.createElement("rectangle", {
+        x: 10,
+        y: 20,
+        width: 90,
+        height: 75,
+      });
+    });
+
+    it("should bind text to container when double clicked on center", async () => {
+      expect(h.elements.length).toBe(1);
+      expect(h.elements[0].id).toBe(rectangle.id);
+
+      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;
+
+      await new Promise((r) => setTimeout(r, 0));
+
+      fireEvent.change(editor, { target: { value: "Hello World!" } });
+      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);
+
+      Keyboard.withModifierKeys({}, () => {
+        Keyboard.keyPress(KEYS.ENTER);
+      });
+
+      expect(h.elements.length).toBe(2);
+
+      const text = h.elements[1] as ExcalidrawTextElementWithContainer;
+      expect(text.type).toBe("text");
+      expect(text.containerId).toBe(rectangle.id);
+      const editor = document.querySelector(
+        ".excalidraw-textEditorContainer > textarea",
+      ) as HTMLTextAreaElement;
+
+      await new Promise((r) => setTimeout(r, 0));
+
+      fireEvent.change(editor, { target: { value: "Hello World!" } });
+      editor.blur();
+      expect(rectangle.boundElements).toStrictEqual([
+        { id: text.id, type: "text" },
+      ]);
+    });
+
+    it("should update font family correctly on undo/redo by selecting bounded text when font family was updated", async () => {
+      mouse.doubleClickAt(
+        rectangle.x + rectangle.width / 2,
+        rectangle.y + rectangle.height / 2,
+      );
+      mouse.down();
+
+      const text = h.elements[1] as ExcalidrawTextElementWithContainer;
+      let editor = document.querySelector(
+        ".excalidraw-textEditorContainer > textarea",
+      ) as HTMLTextAreaElement;
+
+      await new Promise((r) => setTimeout(r, 0));
+      fireEvent.change(editor, { target: { value: "Hello World!" } });
+      editor.blur();
+      expect(text.fontFamily).toEqual(FONT_FAMILY.Virgil);
+      UI.clickTool("text");
+
+      mouse.clickAt(
+        rectangle.x + rectangle.width / 2,
+        rectangle.y + rectangle.height / 2,
+      );
+      mouse.down();
+      editor = document.querySelector(
+        ".excalidraw-textEditorContainer > textarea",
+      ) as HTMLTextAreaElement;
+
+      editor.select();
+      fireEvent.click(screen.getByTitle(/code/i));
+
+      await new Promise((r) => setTimeout(r, 0));
+      editor.blur();
+      expect(h.elements[1].fontFamily).toEqual(FONT_FAMILY.Cascadia);
+
+      //undo
+      Keyboard.withModifierKeys({ ctrl: true }, () => {
+        Keyboard.keyPress(KEYS.Z);
+      });
+      expect(h.elements[1].fontFamily).toEqual(FONT_FAMILY.Virgil);
+
+      //redo
+      Keyboard.withModifierKeys({ ctrl: true, shift: true }, () => {
+        Keyboard.keyPress(KEYS.Z);
+      });
+      expect(h.elements[1].fontFamily).toEqual(FONT_FAMILY.Cascadia);
+    });
+
+    it("should wrap text and vertcially center align once text submitted", async () => {
+      jest
+        .spyOn(textElementUtils, "measureText")
+        .mockImplementation((text, font, maxWidth) => {
+          let width = INITIAL_WIDTH;
+          let height = APPROX_LINE_HEIGHT;
+          let baseline = 10;
+          if (!text) {
+            return {
+              width,
+              height,
+              baseline,
+            };
+          }
+          baseline = 30;
+          width = DUMMY_WIDTH;
+          if (text === "Hello \nWorld!") {
+            height = APPROX_LINE_HEIGHT * 2;
+          }
+          if (maxWidth) {
+            width = maxWidth;
+            // To capture cases where maxWidth passed is initial width
+            // due to which the text is not wrapped correctly
+            if (maxWidth === INITIAL_WIDTH) {
+              height = DUMMY_HEIGHT;
+            }
+          }
+          return {
+            width,
+            height,
+            baseline,
+          };
+        });
+
+      Keyboard.withModifierKeys({}, () => {
+        Keyboard.keyPress(KEYS.ENTER);
+      });
+
+      let text = h.elements[1] as ExcalidrawTextElementWithContainer;
+      let editor = document.querySelector(
+        ".excalidraw-textEditorContainer > textarea",
+      ) as HTMLTextAreaElement;
+
+      // mock scroll height
+      jest
+        .spyOn(editor, "scrollHeight", "get")
+        .mockImplementation(() => APPROX_LINE_HEIGHT * 2);
+
+      fireEvent.change(editor, {
+        target: {
+          value: "Hello World!",
+        },
+      });
+
+      editor.dispatchEvent(new Event("input"));
+
+      await new Promise((cb) => setTimeout(cb, 0));
+      editor.blur();
+      text = h.elements[1] as ExcalidrawTextElementWithContainer;
+      expect(text.text).toBe("Hello \nWorld!");
+      expect(text.originalText).toBe("Hello World!");
+      expect(text.y).toBe(
+        rectangle.y + rectangle.height / 2 - (APPROX_LINE_HEIGHT * 2) / 2,
+      );
+      expect(text.x).toBe(rectangle.x + BOUND_TEXT_PADDING);
+      expect(text.height).toBe(APPROX_LINE_HEIGHT * 2);
+      expect(text.width).toBe(rectangle.width - BOUND_TEXT_PADDING * 2);
+
+      // Edit and text by removing second line and it should
+      // still vertically align correctly
+      mouse.select(rectangle);
+      Keyboard.withModifierKeys({}, () => {
+        Keyboard.keyPress(KEYS.ENTER);
+      });
+      editor = document.querySelector(
+        ".excalidraw-textEditorContainer > textarea",
+      ) as HTMLTextAreaElement;
+
+      fireEvent.change(editor, {
+        target: {
+          value: "Hello",
+        },
+      });
+
+      // mock scroll height
+      jest
+        .spyOn(editor, "scrollHeight", "get")
+        .mockImplementation(() => APPROX_LINE_HEIGHT);
+      editor.style.height = "25px";
+      editor.dispatchEvent(new Event("input"));
+
+      await new Promise((r) => setTimeout(r, 0));
+
+      editor.blur();
+      text = h.elements[1] as ExcalidrawTextElementWithContainer;
+
+      expect(text.text).toBe("Hello");
+      expect(text.originalText).toBe("Hello");
+      expect(text.y).toBe(
+        rectangle.y + rectangle.height / 2 - APPROX_LINE_HEIGHT / 2,
+      );
+      expect(text.x).toBe(rectangle.x + BOUND_TEXT_PADDING);
+      expect(text.height).toBe(APPROX_LINE_HEIGHT);
+      expect(text.width).toBe(rectangle.width - BOUND_TEXT_PADDING * 2);
+    });
+  });
 });

src/element/textWysiwyg.tsx

@@ -3,6 +3,7 @@ import {
   isWritableElement,
   getFontString,
   getFontFamilyString,
+  isTestEnv,
 } from "../utils";
 import Scene from "../scene/Scene";
 import { isBoundToContainer, isTextElement } from "./typeChecks";
@@ -72,7 +73,7 @@ export const textWysiwyg = ({
     originalText: string;
   }) => void;
   getViewportCoords: (x: number, y: number) => [number, number];
-  element: ExcalidrawElement;
+  element: ExcalidrawTextElement;
   canvas: HTMLCanvasElement | null;
   excalidrawContainer: HTMLDivElement | null;
 }) => {
@@ -93,10 +94,9 @@ export const textWysiwyg = ({
     return false;
   };
   let originalContainerHeight: number;
-  let approxLineHeight = isTextElement(element)
-    ? getApproxLineHeight(getFontString(element))
-    : 0;
+  let approxLineHeight = getApproxLineHeight(getFontString(element));
 
+  const initialText = element.originalText;
   const updateWysiwygStyle = () => {
     const updatedElement = Scene.getScene(element)?.getElement(id);
     if (updatedElement && isTextElement(updatedElement)) {
@@ -123,9 +123,7 @@ export const textWysiwyg = ({
           height = editorHeight;
         }
         if (propertiesUpdated) {
-          approxLineHeight = isTextElement(updatedElement)
-            ? getApproxLineHeight(getFontString(updatedElement))
-            : 0;
+          approxLineHeight = getApproxLineHeight(getFontString(updatedElement));
 
           originalContainerHeight = container.height;
 
@@ -164,7 +162,7 @@ export const textWysiwyg = ({
       }
       const [viewportX, viewportY] = getViewportCoords(coordX, coordY);
       const { textAlign } = updatedElement;
-      editable.value = updatedElement.originalText || updatedElement.text;
+      editable.value = updatedElement.originalText;
       const lines = updatedElement.originalText.split("\n");
       const lineHeight = updatedElement.containerId
         ? approxLineHeight
@@ -217,6 +215,11 @@ export const textWysiwyg = ({
         maxWidth: `${maxWidth}px`,
         maxHeight: `${editorMaxHeight}px`,
       });
+      // For some reason updating font attribute doesn't set font family
+      // hence updating font family explicitly for test environment
+      if (isTestEnv()) {
+        editable.style.fontFamily = getFontFamilyString(updatedElement);
+      }
     }
   };
 
@@ -450,9 +453,12 @@ export const textWysiwyg = ({
           getFontString(updateElement),
           container.width,
         );
-        if (isTextElement(updateElement) && updateElement.containerId) {
+
+        if (updateElement.containerId) {
           const editorHeight = Number(editable.style.height.slice(0, -2));
           if (editable.value) {
+            // Don't mutate if text is not updated
+            if (initialText !== editable.value) {
               mutateElement(updateElement, {
                 // vertically center align
                 y: container.y + container.height / 2 - editorHeight / 2,
@@ -462,6 +468,8 @@ export const textWysiwyg = ({
                 x: container.x + BOUND_TEXT_PADDING,
                 angle: container.angle,
               });
+            }
+
             const boundTextElementId = getBoundTextElementId(container);
             if (!boundTextElementId || boundTextElementId !== element.id) {
               mutateElement(container, {