From 54c148f3902de857c813a0e7fdd598c21fc67214 Mon Sep 17 00:00:00 2001 From: Marcel Mraz Date: Tue, 12 Aug 2025 09:27:04 +0200 Subject: [PATCH] fix: text restore & deletion issues (#9853) --- excalidraw-app/data/firebase.ts | 4 +- excalidraw-app/data/index.ts | 7 +- packages/element/src/delta.ts | 39 +++++- packages/element/tests/delta.test.tsx | 69 +++++++++- packages/excalidraw/components/App.tsx | 27 ++-- packages/excalidraw/data/blob.ts | 6 +- packages/excalidraw/data/restore.ts | 26 +++- packages/excalidraw/index.tsx | 1 + .../tests/__snapshots__/history.test.tsx.snap | 128 +++++++----------- .../regressionTests.test.tsx.snap | 38 +----- .../excalidraw/tests/data/restore.test.ts | 24 +++- packages/excalidraw/tests/history.test.tsx | 4 +- .../excalidraw/wysiwyg/textWysiwyg.test.tsx | 8 +- packages/utils/src/export.ts | 2 + 14 files changed, 229 insertions(+), 154 deletions(-) diff --git a/excalidraw-app/data/firebase.ts b/excalidraw-app/data/firebase.ts index 568054f7e..d05966df5 100644 --- a/excalidraw-app/data/firebase.ts +++ b/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) { diff --git a/excalidraw-app/data/index.ts b/excalidraw-app/data/index.ts index 75aa27877..73512f378 100644 --- a/excalidraw-app/data/index.ts +++ b/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, }); } diff --git a/packages/element/src/delta.ts b/packages/element/src/delta.ts index 87a23d0c2..38839fa84 100644 --- a/packages/element/src/delta.ts +++ b/packages/element/src/delta.ts @@ -1088,7 +1088,7 @@ export class ElementsDelta implements DeltaContainer { 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 { 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 { const inserted = { ...nextElement, - isDeleted: false, } as ElementPartial; const delta = Delta.create( @@ -1127,7 +1129,10 @@ export class ElementsDelta implements DeltaContainer { 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 { 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 { 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 { return strippedPartial; } + + private static stripVersionProps( + partial: Partial, + ): ElementPartial { + const { version, versionNonce, ...strippedPartial } = partial; + + return strippedPartial; + } } diff --git a/packages/element/tests/delta.test.tsx b/packages/element/tests/delta.test.tsx index 89501c866..81c9d4591 100644 --- a/packages/element/tests/delta.test.tsx +++ b/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 } from "../src/delta"; +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); + + 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", () => { diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index 548df6f9d..40a942e96 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -2342,7 +2342,10 @@ class App extends React.Component { }, }; } - 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 { 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 { }), 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 { })); }); } + 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({ diff --git a/packages/excalidraw/data/blob.ts b/packages/excalidraw/data/blob.ts index 3e5db7c29..5db4c6cfe 100644 --- a/packages/excalidraw/data/blob.ts +++ b/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)) { diff --git a/packages/excalidraw/data/restore.ts b/packages/excalidraw/data/restore.ts index c7ec8d728..10addb47e 100644 --- a/packages/excalidraw/data/restore.ts +++ b/packages/excalidraw/data/restore.ts @@ -241,8 +241,9 @@ const restoreElementWithProperties = < return ret; }; -const restoreElement = ( +export const restoreElement = ( element: Exclude, + 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(); @@ -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 | 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), diff --git a/packages/excalidraw/index.tsx b/packages/excalidraw/index.tsx index a592e2ea9..1b1f83043 100644 --- a/packages/excalidraw/index.tsx +++ b/packages/excalidraw/index.tsx @@ -229,6 +229,7 @@ export { defaultLang, useI18n, languages } from "./i18n"; export { restore, restoreAppState, + restoreElement, restoreElements, restoreLibraryItems, } from "./data/restore"; diff --git a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap index 50f08c4c2..c9a299283 100644 --- a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap +++ b/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", diff --git a/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap index a895eb636..c16cd9884 100644 --- a/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap +++ b/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", }, diff --git a/packages/excalidraw/tests/data/restore.test.ts b/packages/excalidraw/tests/data/restore.test.ts index cc3807382..1cae91416 100644 --- a/packages/excalidraw/tests/data/restore.test.ts +++ b/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), diff --git a/packages/excalidraw/tests/history.test.tsx b/packages/excalidraw/tests/history.test.tsx index 707fe4e48..09510e5eb 100644 --- a/packages/excalidraw/tests/history.test.tsx +++ b/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, }), ]); diff --git a/packages/excalidraw/wysiwyg/textWysiwyg.test.tsx b/packages/excalidraw/wysiwyg/textWysiwyg.test.tsx index 08301a304..b03fab739 100644 --- a/packages/excalidraw/wysiwyg/textWysiwyg.test.tsx +++ b/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 () => { diff --git a/packages/utils/src/export.ts b/packages/utils/src/export.ts index 4559fe1af..7821b5160 100644 --- a/packages/utils/src/export.ts +++ b/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 = {