fix: text restore & deletion issues (#9853)

excalidraw-app/data/firebase.ts

@@ -259,7 +259,9 @@ export const loadFromFirebase = async (
   }
   const storedScene = docSnap.data() as FirebaseStoredScene;
   const elements = getSyncableElements(
-    restoreElements(await decryptElements(storedScene, roomKey), null),
+    restoreElements(await decryptElements(storedScene, roomKey), null, {
+      deleteEmptyTextElements: true,
+    }),
   );
 
   if (socket) {

excalidraw-app/data/index.ts

@@ -258,11 +258,16 @@ export const loadScene = async (
       await importFromBackend(id, privateKey),
       localDataState?.appState,
       localDataState?.elements,
-      { repairBindings: true, refreshDimensions: false },
+      {
+        repairBindings: true,
+        refreshDimensions: false,
+        deleteEmptyTextElements: true,
+      },
     );
   } else {
     data = restore(localDataState || null, null, null, {
       repairBindings: true,
+      deleteEmptyTextElements: true,
     });
   }
 

packages/element/src/delta.ts

@@ -1088,7 +1088,7 @@ export class ElementsDelta implements DeltaContainer<SceneElementsMap> {
       const nextElement = nextElements.get(prevElement.id);
 
       if (!nextElement) {
-        const deleted = { ...prevElement, isDeleted: false } as ElementPartial;
+        const deleted = { ...prevElement } as ElementPartial;
 
         const inserted = {
           isDeleted: true,
@@ -1102,7 +1102,10 @@ export class ElementsDelta implements DeltaContainer<SceneElementsMap> {
           ElementsDelta.stripIrrelevantProps,
         );
 
-        removed[prevElement.id] = delta;
+        // ignore updates which would "delete" already deleted element
+        if (!prevElement.isDeleted) {
+          removed[prevElement.id] = delta;
+        }
       }
     }
 
@@ -1118,7 +1121,6 @@ export class ElementsDelta implements DeltaContainer<SceneElementsMap> {
 
         const inserted = {
           ...nextElement,
-          isDeleted: false,
         } as ElementPartial;
 
         const delta = Delta.create(
@@ -1127,7 +1129,10 @@ export class ElementsDelta implements DeltaContainer<SceneElementsMap> {
           ElementsDelta.stripIrrelevantProps,
         );
 
-        added[nextElement.id] = delta;
+        // ignore updates which would "delete" already deleted element
+        if (!nextElement.isDeleted) {
+          added[nextElement.id] = delta;
+        }
 
         continue;
       }
@@ -1156,8 +1161,13 @@ export class ElementsDelta implements DeltaContainer<SceneElementsMap> {
           continue;
         }
 
-        // making sure there are at least some changes
-        if (!Delta.isEmpty(delta)) {
+        const strippedDeleted = ElementsDelta.stripVersionProps(delta.deleted);
+        const strippedInserted = ElementsDelta.stripVersionProps(
+          delta.inserted,
+        );
+
+        // making sure there are at least some changes and only changed version & versionNonce does not count!
+        if (Delta.isInnerDifferent(strippedDeleted, strippedInserted, true)) {
           updated[nextElement.id] = delta;
         }
       }
@@ -1273,8 +1283,15 @@ export class ElementsDelta implements DeltaContainer<SceneElementsMap> {
           latestDelta = delta;
         }
 
+        const strippedDeleted = ElementsDelta.stripVersionProps(
+          latestDelta.deleted,
+        );
+        const strippedInserted = ElementsDelta.stripVersionProps(
+          latestDelta.inserted,
+        );
+
         // it might happen that after applying latest changes the delta itself does not contain any changes
-        if (Delta.isInnerDifferent(latestDelta.deleted, latestDelta.inserted)) {
+        if (Delta.isInnerDifferent(strippedDeleted, strippedInserted)) {
           modifiedDeltas[id] = latestDelta;
         }
       }
@@ -1854,4 +1871,12 @@ export class ElementsDelta implements DeltaContainer<SceneElementsMap> {
 
     return strippedPartial;
   }
+
+  private static stripVersionProps(
+    partial: Partial<OrderedExcalidrawElement>,
+  ): ElementPartial {
+    const { version, versionNonce, ...strippedPartial } = partial;
+
+    return strippedPartial;
+  }
 }

packages/element/tests/delta.test.tsx

@@ -1,7 +1,74 @@
+import { API } from "@excalidraw/excalidraw/tests/helpers/api";
+
 import type { ObservedAppState } from "@excalidraw/excalidraw/types";
 import type { LinearElementEditor } from "@excalidraw/element";
+import type { SceneElementsMap } from "@excalidraw/element/types";
+
+import { AppStateDelta, ElementsDelta } from "../src/delta";
+
+describe("ElementsDelta", () => {
+  describe("elements delta calculation", () => {
+    it("should not create removed delta when element gets removed but was already deleted", () => {
+      const element = API.createElement({
+        type: "rectangle",
+        x: 100,
+        y: 100,
+        isDeleted: true,
+      });
+
+      const prevElements = new Map([[element.id, element]]);
+      const nextElements = new Map();
+
+      const delta = ElementsDelta.calculate(prevElements, nextElements);
+
+      expect(delta.isEmpty()).toBeTruthy();
+    });
+
+    it("should not create added delta when adding element as already deleted", () => {
+      const element = API.createElement({
+        type: "rectangle",
+        x: 100,
+        y: 100,
+        isDeleted: true,
+      });
+
+      const prevElements = new Map();
+      const nextElements = new Map([[element.id, element]]);
+
+      const delta = ElementsDelta.calculate(prevElements, nextElements);
 
-import { AppStateDelta } from "../src/delta";
+      expect(delta.isEmpty()).toBeTruthy();
+    });
+
+    it("should not create updated delta when there is only version and versionNonce change", () => {
+      const baseElement = API.createElement({
+        type: "rectangle",
+        x: 100,
+        y: 100,
+        strokeColor: "#000000",
+        backgroundColor: "#ffffff",
+      });
+
+      const modifiedElement = {
+        ...baseElement,
+        version: baseElement.version + 1,
+        versionNonce: baseElement.versionNonce + 1,
+      };
+
+      // Create maps for the delta calculation
+      const prevElements = new Map([[baseElement.id, baseElement]]);
+      const nextElements = new Map([[modifiedElement.id, modifiedElement]]);
+
+      // Calculate the delta
+      const delta = ElementsDelta.calculate(
+        prevElements as SceneElementsMap,
+        nextElements as SceneElementsMap,
+      );
+
+      expect(delta.isEmpty()).toBeTruthy();
+    });
+  });
+});
 
 describe("AppStateDelta", () => {
   describe("ensure stable delta properties order", () => {

packages/excalidraw/components/App.tsx

@@ -2342,7 +2342,10 @@ class App extends React.Component<AppProps, AppState> {
         },
       };
     }
-    const scene = restore(initialData, null, null, { repairBindings: true });
+    const scene = restore(initialData, null, null, {
+      repairBindings: true,
+      deleteEmptyTextElements: true,
+    });
     scene.appState = {
       ...scene.appState,
       theme: this.props.theme || scene.appState.theme,
@@ -3200,7 +3203,9 @@ class App extends React.Component<AppProps, AppState> {
     retainSeed?: boolean;
     fitToContent?: boolean;
   }) => {
-    const elements = restoreElements(opts.elements, null, undefined);
+    const elements = restoreElements(opts.elements, null, {
+      deleteEmptyTextElements: true,
+    });
     const [minX, minY, maxX, maxY] = getCommonBounds(elements);
 
     const elementsCenterX = distance(minX, maxX) / 2;
@@ -4927,17 +4932,8 @@ class App extends React.Component<AppProps, AppState> {
       }),
       onSubmit: withBatchedUpdates(({ viaKeyboard, nextOriginalText }) => {
         const isDeleted = !nextOriginalText.trim();
+        updateElement(nextOriginalText, isDeleted);
 
-        if (isDeleted && !isExistingElement) {
-          // let's just remove the element from the scene, as it's an empty just created text element
-          this.scene.replaceAllElements(
-            this.scene
-              .getElementsIncludingDeleted()
-              .filter((x) => x.id !== element.id),
-          );
-        } else {
-          updateElement(nextOriginalText, isDeleted);
-        }
         // select the created text element only if submitting via keyboard
         // (when submitting via click it should act as signal to deselect)
         if (!isDeleted && viaKeyboard) {
@@ -4961,15 +4957,16 @@ class App extends React.Component<AppProps, AppState> {
             }));
           });
         }
+
         if (isDeleted) {
           fixBindingsAfterDeletion(this.scene.getNonDeletedElements(), [
             element,
           ]);
         }
 
-        // we need to record either way, whether the text element was added or removed
-        // since we need to sync this delta to other clients, otherwise it would end up with inconsistencies
-        this.store.scheduleCapture();
+        if (!isDeleted || isExistingElement) {
+          this.store.scheduleCapture();
+        }
 
         flushSync(() => {
           this.setState({

packages/excalidraw/data/blob.ts

@@ -170,7 +170,11 @@ export const loadSceneOrLibraryFromBlob = async (
           },
           localAppState,
           localElements,
-          { repairBindings: true, refreshDimensions: false },
+          {
+            repairBindings: true,
+            refreshDimensions: false,
+            deleteEmptyTextElements: true,
+          },
         ),
       };
     } else if (isValidLibrary(data)) {

packages/excalidraw/data/restore.ts

@@ -241,8 +241,9 @@ const restoreElementWithProperties = <
   return ret;
 };
 
-const restoreElement = (
+export const restoreElement = (
   element: Exclude<ExcalidrawElement, ExcalidrawSelectionElement>,
+  opts?: { deleteEmptyTextElements?: boolean },
 ): typeof element | null => {
   element = { ...element };
 
@@ -290,7 +291,7 @@ const restoreElement = (
 
       // if empty text, mark as deleted. We keep in array
       // for data integrity purposes (collab etc.)
-      if (!text && !element.isDeleted) {
+      if (opts?.deleteEmptyTextElements && !text && !element.isDeleted) {
         // TODO: we should not do this since it breaks sync / versioning when we exchange / apply just deltas and restore the elements (deletion isn't recorded)
         element = { ...element, originalText: text, isDeleted: true };
         element = bumpVersion(element);
@@ -524,7 +525,13 @@ export const restoreElements = (
   elements: ImportedDataState["elements"],
   /** NOTE doesn't serve for reconciliation */
   localElements: readonly ExcalidrawElement[] | null | undefined,
-  opts?: { refreshDimensions?: boolean; repairBindings?: boolean } | undefined,
+  opts?:
+    | {
+        refreshDimensions?: boolean;
+        repairBindings?: boolean;
+        deleteEmptyTextElements?: boolean;
+      }
+    | undefined,
 ): OrderedExcalidrawElement[] => {
   // used to detect duplicate top-level element ids
   const existingIds = new Set<string>();
@@ -534,7 +541,12 @@ export const restoreElements = (
       // filtering out selection, which is legacy, no longer kept in elements,
       // and causing issues if retained
       if (element.type !== "selection" && !isInvisiblySmallElement(element)) {
-        let migratedElement: ExcalidrawElement | null = restoreElement(element);
+        let migratedElement: ExcalidrawElement | null = restoreElement(
+          element,
+          {
+            deleteEmptyTextElements: opts?.deleteEmptyTextElements,
+          },
+        );
         if (migratedElement) {
           const localElement = localElementsMap?.get(element.id);
           if (localElement && localElement.version > migratedElement.version) {
@@ -791,7 +803,11 @@ export const restore = (
    */
   localAppState: Partial<AppState> | null | undefined,
   localElements: readonly ExcalidrawElement[] | null | undefined,
-  elementsConfig?: { refreshDimensions?: boolean; repairBindings?: boolean },
+  elementsConfig?: {
+    refreshDimensions?: boolean;
+    repairBindings?: boolean;
+    deleteEmptyTextElements?: boolean;
+  },
 ): RestoredDataState => {
   return {
     elements: restoreElements(data?.elements, localElements, elementsConfig),

packages/excalidraw/index.tsx

@@ -229,6 +229,7 @@ export { defaultLang, useI18n, languages } from "./i18n";
 export {
   restore,
   restoreAppState,
+  restoreElement,
   restoreElements,
   restoreLibraryItems,
 } from "./data/restore";

packages/excalidraw/tests/__snapshots__/history.test.tsx.snap

@@ -282,14 +282,6 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl
       "added": {},
       "removed": {},
       "updated": {
-        "id0": {
-          "deleted": {
-            "version": 17,
-          },
-          "inserted": {
-            "version": 15,
-          },
-        },
         "id1": {
           "deleted": {
             "boundElements": [],
@@ -404,14 +396,6 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl
             "version": 17,
           },
         },
-        "id15": {
-          "deleted": {
-            "version": 14,
-          },
-          "inserted": {
-            "version": 12,
-          },
-        },
         "id4": {
           "deleted": {
             "height": "99.19972",
@@ -853,14 +837,6 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl
       "added": {},
       "removed": {},
       "updated": {
-        "id0": {
-          "deleted": {
-            "version": 18,
-          },
-          "inserted": {
-            "version": 16,
-          },
-        },
         "id1": {
           "deleted": {
             "boundElements": [],
@@ -2656,7 +2632,7 @@ exports[`history > multiplayer undo/redo > conflicts in bound text elements and
   "height": 100,
   "id": "id0",
   "index": "a0",
-  "isDeleted": false,
+  "isDeleted": true,
   "link": null,
   "locked": false,
   "opacity": 100,
@@ -2667,7 +2643,7 @@ exports[`history > multiplayer undo/redo > conflicts in bound text elements and
   "strokeWidth": 2,
   "type": "rectangle",
   "updated": 1,
-  "version": 6,
+  "version": 9,
   "width": 100,
   "x": 10,
   "y": 10,
@@ -2719,7 +2695,7 @@ exports[`history > multiplayer undo/redo > conflicts in bound text elements and
   "autoResize": true,
   "backgroundColor": "transparent",
   "boundElements": null,
-  "containerId": "id0",
+  "containerId": null,
   "customData": undefined,
   "fillStyle": "solid",
   "fontFamily": 5,
@@ -2744,7 +2720,7 @@ exports[`history > multiplayer undo/redo > conflicts in bound text elements and
   "textAlign": "left",
   "type": "text",
   "updated": 1,
-  "version": 7,
+  "version": 10,
   "verticalAlign": "top",
   "width": 30,
   "x": 15,
@@ -2766,15 +2742,49 @@ exports[`history > multiplayer undo/redo > conflicts in bound text elements and
       },
     },
     "elements": {
-      "added": {},
+      "added": {
+        "id0": {
+          "deleted": {
+            "isDeleted": true,
+            "version": 9,
+          },
+          "inserted": {
+            "angle": 0,
+            "backgroundColor": "transparent",
+            "boundElements": null,
+            "customData": undefined,
+            "fillStyle": "solid",
+            "frameId": null,
+            "groupIds": [],
+            "height": 100,
+            "index": "a0",
+            "isDeleted": false,
+            "link": null,
+            "locked": false,
+            "opacity": 100,
+            "roughness": 1,
+            "roundness": null,
+            "strokeColor": "#1e1e1e",
+            "strokeStyle": "solid",
+            "strokeWidth": 2,
+            "type": "rectangle",
+            "version": 8,
+            "width": 100,
+            "x": 10,
+            "y": 10,
+          },
+        },
+      },
       "removed": {},
       "updated": {
         "id5": {
           "deleted": {
-            "version": 7,
+            "containerId": null,
+            "version": 10,
           },
           "inserted": {
-            "version": 5,
+            "containerId": "id0",
+            "version": 9,
           },
         },
       },
@@ -3086,14 +3096,6 @@ exports[`history > multiplayer undo/redo > conflicts in bound text elements and
             "version": 10,
           },
         },
-        "id5": {
-          "deleted": {
-            "version": 11,
-          },
-          "inserted": {
-            "version": 9,
-          },
-        },
       },
     },
     "id": "id9",
@@ -4643,15 +4645,15 @@ exports[`history > multiplayer undo/redo > conflicts in bound text elements and
         "id1": {
           "deleted": {
             "angle": 0,
-            "version": 5,
+            "version": 4,
             "x": 15,
             "y": 15,
           },
           "inserted": {
-            "angle": 0,
-            "version": 7,
-            "x": 15,
-            "y": 15,
+            "angle": 90,
+            "version": 3,
+            "x": 205,
+            "y": 205,
           },
         },
       },
@@ -5630,12 +5632,12 @@ exports[`history > multiplayer undo/redo > conflicts in frames and their childre
       "updated": {
         "id1": {
           "deleted": {
-            "frameId": null,
-            "version": 10,
+            "frameId": "id0",
+            "version": 5,
           },
           "inserted": {
             "frameId": null,
-            "version": 8,
+            "version": 6,
           },
         },
       },
@@ -15773,14 +15775,6 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding
             "version": 5,
           },
         },
-        "id1": {
-          "deleted": {
-            "version": 6,
-          },
-          "inserted": {
-            "version": 4,
-          },
-        },
         "id2": {
           "deleted": {
             "boundElements": [
@@ -16742,14 +16736,6 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding
             "version": 5,
           },
         },
-        "id1": {
-          "deleted": {
-            "version": 8,
-          },
-          "inserted": {
-            "version": 6,
-          },
-        },
         "id2": {
           "deleted": {
             "boundElements": [
@@ -17375,14 +17361,6 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding
             "version": 9,
           },
         },
-        "id1": {
-          "deleted": {
-            "version": 12,
-          },
-          "inserted": {
-            "version": 10,
-          },
-        },
         "id2": {
           "deleted": {
             "boundElements": [
@@ -17744,14 +17722,6 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding
             "version": 7,
           },
         },
-        "id2": {
-          "deleted": {
-            "version": 5,
-          },
-          "inserted": {
-            "version": 3,
-          },
-        },
       },
     },
     "id": "id21",

packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap

@@ -2216,16 +2216,7 @@ exports[`regression tests > alt-drag duplicates an element > [end of test] undo
           },
         },
       },
-      "updated": {
-        "id0": {
-          "deleted": {
-            "version": 5,
-          },
-          "inserted": {
-            "version": 3,
-          },
-        },
-      },
+      "updated": {},
     },
     "id": "id6",
   },
@@ -10901,32 +10892,7 @@ exports[`regression tests > make a group and duplicate it > [end of test] undo s
           },
         },
       },
-      "updated": {
-        "id0": {
-          "deleted": {
-            "version": 6,
-          },
-          "inserted": {
-            "version": 4,
-          },
-        },
-        "id3": {
-          "deleted": {
-            "version": 6,
-          },
-          "inserted": {
-            "version": 4,
-          },
-        },
-        "id6": {
-          "deleted": {
-            "version": 6,
-          },
-          "inserted": {
-            "version": 4,
-          },
-        },
-      },
+      "updated": {},
     },
     "id": "id21",
   },

packages/excalidraw/tests/data/restore.test.ts

@@ -85,6 +85,23 @@ describe("restoreElements", () => {
     });
   });
 
+  it("should not delete empty text element when deleteEmptyTextElements is not defined", () => {
+    const textElement = API.createElement({
+      type: "text",
+      text: "",
+      isDeleted: false,
+    });
+
+    const restoredElements = restore.restoreElements([textElement], null);
+
+    expect(restoredElements).toEqual([
+      expect.objectContaining({
+        id: textElement.id,
+        isDeleted: false,
+      }),
+    ]);
+  });
+
   it("should restore text element correctly with unknown font family, null text and undefined alignment", () => {
     const textElement: any = API.createElement({
       type: "text",
@@ -97,10 +114,9 @@ describe("restoreElements", () => {
     textElement.font = "10 unknown";
 
     expect(textElement.isDeleted).toBe(false);
-    const restoredText = restore.restoreElements(
-      [textElement],
-      null,
-    )[0] as ExcalidrawTextElement;
+    const restoredText = restore.restoreElements([textElement], null, {
+      deleteEmptyTextElements: true,
+    })[0] as ExcalidrawTextElement;
     expect(restoredText.isDeleted).toBe(true);
     expect(restoredText).toMatchSnapshot({
       seed: expect.any(Number),

packages/excalidraw/tests/history.test.tsx

@@ -4055,7 +4055,7 @@ describe("history", () => {
             expect.objectContaining({
               id: container.id,
               boundElements: [{ id: remoteText.id, type: "text" }],
-              isDeleted: false, // isDeleted got remotely updated to false
+              isDeleted: true,
             }),
             expect.objectContaining({
               id: text.id,
@@ -4065,7 +4065,7 @@ describe("history", () => {
             expect.objectContaining({
               id: remoteText.id,
               // unbound
-              containerId: container.id,
+              containerId: null,
               isDeleted: false,
             }),
           ]);

packages/excalidraw/wysiwyg/textWysiwyg.test.tsx

@@ -704,7 +704,7 @@ describe("textWysiwyg", () => {
         rectangle.x + rectangle.width / 2,
         rectangle.y + rectangle.height / 2,
       );
-      expect(h.elements.length).toBe(2);
+      expect(h.elements.length).toBe(3);
 
       text = h.elements[1] as ExcalidrawTextElementWithContainer;
       expect(text.type).toBe("text");
@@ -1198,7 +1198,11 @@ describe("textWysiwyg", () => {
       updateTextEditor(editor, "   ");
       Keyboard.exitTextEditor(editor);
       expect(rectangle.boundElements).toStrictEqual([]);
-      expect(h.elements[1]).toBeUndefined();
+      expect(h.elements[1]).toEqual(
+        expect.objectContaining({
+          isDeleted: true,
+        }),
+      );
     });
 
     it("should restore original container height and clear cache once text is unbind", async () => {

packages/utils/src/export.ts

@@ -49,6 +49,7 @@ export const exportToCanvas = ({
     { elements, appState },
     null,
     null,
+    { deleteEmptyTextElements: true },
   );
   const { exportBackground, viewBackgroundColor } = restoredAppState;
   return _exportToCanvas(
@@ -179,6 +180,7 @@ export const exportToSvg = async ({
     { elements, appState },
     null,
     null,
+    { deleteEmptyTextElements: true },
   );
 
   const exportAppState = {