From dda3affcb081c40e2333cc2019dddadd3a37609f Mon Sep 17 00:00:00 2001 From: David Luzar <5153846+dwelle@users.noreply.github.com> Date: Tue, 12 Aug 2025 11:56:11 +0200 Subject: [PATCH] fix: do not strip invisible elements from array (#9844) --- excalidraw-app/data/firebase.ts | 2 +- excalidraw-app/data/index.ts | 4 +- .../excalidraw/actions/actionFinalize.tsx | 29 +++++++-- packages/excalidraw/components/App.tsx | 4 +- packages/excalidraw/data/blob.ts | 2 +- packages/excalidraw/data/restore.ts | 61 +++++++++++-------- .../excalidraw/tests/data/restore.test.ts | 30 +++++++-- packages/excalidraw/tests/dragCreate.test.tsx | 14 ++++- packages/utils/src/export.ts | 4 +- 9 files changed, 104 insertions(+), 46 deletions(-) diff --git a/excalidraw-app/data/firebase.ts b/excalidraw-app/data/firebase.ts index d05966df51..4e4c60b291 100644 --- a/excalidraw-app/data/firebase.ts +++ b/excalidraw-app/data/firebase.ts @@ -260,7 +260,7 @@ export const loadFromFirebase = async ( const storedScene = docSnap.data() as FirebaseStoredScene; const elements = getSyncableElements( restoreElements(await decryptElements(storedScene, roomKey), null, { - deleteEmptyTextElements: true, + deleteInvisibleElements: true, }), ); diff --git a/excalidraw-app/data/index.ts b/excalidraw-app/data/index.ts index 73512f3786..cc7d5e8cc9 100644 --- a/excalidraw-app/data/index.ts +++ b/excalidraw-app/data/index.ts @@ -261,13 +261,13 @@ export const loadScene = async ( { repairBindings: true, refreshDimensions: false, - deleteEmptyTextElements: true, + deleteInvisibleElements: true, }, ); } else { data = restore(localDataState || null, null, null, { repairBindings: true, - deleteEmptyTextElements: true, + deleteInvisibleElements: true, }); } diff --git a/packages/excalidraw/actions/actionFinalize.tsx b/packages/excalidraw/actions/actionFinalize.tsx index 9baeb0b6f0..f9ff6e79f7 100644 --- a/packages/excalidraw/actions/actionFinalize.tsx +++ b/packages/excalidraw/actions/actionFinalize.tsx @@ -5,7 +5,11 @@ import { bindOrUnbindLinearElement, isBindingEnabled, } from "@excalidraw/element/binding"; -import { isValidPolygon, LinearElementEditor } from "@excalidraw/element"; +import { + isValidPolygon, + LinearElementEditor, + newElementWith, +} from "@excalidraw/element"; import { isBindingElement, @@ -78,7 +82,14 @@ export const actionFinalize = register({ let newElements = elements; if (element && isInvisiblySmallElement(element)) { // TODO: #7348 in theory this gets recorded by the store, so the invisible elements could be restored by the undo/redo, which might be not what we would want - newElements = newElements.filter((el) => el.id !== element!.id); + newElements = newElements.map((el) => { + if (el.id === element.id) { + return newElementWith(el, { + isDeleted: true, + }); + } + return el; + }); } return { elements: newElements, @@ -117,7 +128,12 @@ export const actionFinalize = register({ return { elements: element.points.length < 2 || isInvisiblySmallElement(element) - ? elements.filter((el) => el.id !== element.id) + ? elements.map((el) => { + if (el.id === element.id) { + return newElementWith(el, { isDeleted: true }); + } + return el; + }) : undefined, appState: { ...appState, @@ -172,7 +188,12 @@ export const actionFinalize = register({ if (element && isInvisiblySmallElement(element)) { // TODO: #7348 in theory this gets recorded by the store, so the invisible elements could be restored by the undo/redo, which might be not what we would want - newElements = newElements.filter((el) => el.id !== element!.id); + newElements = newElements.map((el) => { + if (el.id === element?.id) { + return newElementWith(el, { isDeleted: true }); + } + return el; + }); } if (isLinearElement(element) || isFreeDrawElement(element)) { diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index 40a942e96b..5d0ce49e24 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -2344,7 +2344,7 @@ class App extends React.Component { } const scene = restore(initialData, null, null, { repairBindings: true, - deleteEmptyTextElements: true, + deleteInvisibleElements: true, }); scene.appState = { ...scene.appState, @@ -3204,7 +3204,7 @@ class App extends React.Component { fitToContent?: boolean; }) => { const elements = restoreElements(opts.elements, null, { - deleteEmptyTextElements: true, + deleteInvisibleElements: true, }); const [minX, minY, maxX, maxY] = getCommonBounds(elements); diff --git a/packages/excalidraw/data/blob.ts b/packages/excalidraw/data/blob.ts index 5db4c6cfe0..d990fd0500 100644 --- a/packages/excalidraw/data/blob.ts +++ b/packages/excalidraw/data/blob.ts @@ -173,7 +173,7 @@ export const loadSceneOrLibraryFromBlob = async ( { repairBindings: true, refreshDimensions: false, - deleteEmptyTextElements: true, + deleteInvisibleElements: true, }, ), }; diff --git a/packages/excalidraw/data/restore.ts b/packages/excalidraw/data/restore.ts index 10addb47e5..40c57b9cf6 100644 --- a/packages/excalidraw/data/restore.ts +++ b/packages/excalidraw/data/restore.ts @@ -243,7 +243,7 @@ const restoreElementWithProperties = < export const restoreElement = ( element: Exclude, - opts?: { deleteEmptyTextElements?: boolean }, + opts?: { deleteInvisibleElements?: boolean }, ): typeof element | null => { element = { ...element }; @@ -291,7 +291,7 @@ export const restoreElement = ( // if empty text, mark as deleted. We keep in array // for data integrity purposes (collab etc.) - if (opts?.deleteEmptyTextElements && !text && !element.isDeleted) { + if (opts?.deleteInvisibleElements && !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); @@ -529,7 +529,7 @@ export const restoreElements = ( | { refreshDimensions?: boolean; repairBindings?: boolean; - deleteEmptyTextElements?: boolean; + deleteInvisibleElements?: boolean; } | undefined, ): OrderedExcalidrawElement[] => { @@ -540,29 +540,38 @@ export const restoreElements = ( (elements || []).reduce((elements, element) => { // 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, - { - deleteEmptyTextElements: opts?.deleteEmptyTextElements, - }, - ); - if (migratedElement) { - const localElement = localElementsMap?.get(element.id); - if (localElement && localElement.version > migratedElement.version) { - migratedElement = bumpVersion( - migratedElement, - localElement.version, - ); - } - if (existingIds.has(migratedElement.id)) { - migratedElement = { ...migratedElement, id: randomId() }; - } - existingIds.add(migratedElement.id); - - elements.push(migratedElement); - } + if (element.type === "selection") { + return elements; } + + let migratedElement: ExcalidrawElement | null = restoreElement(element, { + deleteInvisibleElements: opts?.deleteInvisibleElements, + }); + if (migratedElement) { + const localElement = localElementsMap?.get(element.id); + + const shouldMarkAsDeleted = + opts?.deleteInvisibleElements && isInvisiblySmallElement(element); + + if ( + shouldMarkAsDeleted || + (localElement && localElement.version > migratedElement.version) + ) { + migratedElement = bumpVersion(migratedElement, localElement?.version); + } + + if (shouldMarkAsDeleted) { + migratedElement = { ...migratedElement, isDeleted: true }; + } + + if (existingIds.has(migratedElement.id)) { + migratedElement = { ...migratedElement, id: randomId() }; + } + existingIds.add(migratedElement.id); + + elements.push(migratedElement); + } + return elements; }, [] as ExcalidrawElement[]), ); @@ -806,7 +815,7 @@ export const restore = ( elementsConfig?: { refreshDimensions?: boolean; repairBindings?: boolean; - deleteEmptyTextElements?: boolean; + deleteInvisibleElements?: boolean; }, ): RestoredDataState => { return { diff --git a/packages/excalidraw/tests/data/restore.test.ts b/packages/excalidraw/tests/data/restore.test.ts index 1cae91416b..3eee14d4a4 100644 --- a/packages/excalidraw/tests/data/restore.test.ts +++ b/packages/excalidraw/tests/data/restore.test.ts @@ -60,7 +60,11 @@ describe("restoreElements", () => { const rectElement = API.createElement({ type: "rectangle" }); mockSizeHelper.mockImplementation(() => true); - expect(restore.restoreElements([rectElement], null).length).toBe(0); + expect( + restore.restoreElements([rectElement], null, { + deleteInvisibleElements: true, + }), + ).toEqual([expect.objectContaining({ isDeleted: true })]); }); it("should restore text element correctly passing value for each attribute", () => { @@ -85,7 +89,7 @@ describe("restoreElements", () => { }); }); - it("should not delete empty text element when deleteEmptyTextElements is not defined", () => { + it("should not delete empty text element when opts.deleteInvisibleElements is not defined", () => { const textElement = API.createElement({ type: "text", text: "", @@ -115,7 +119,7 @@ describe("restoreElements", () => { expect(textElement.isDeleted).toBe(false); const restoredText = restore.restoreElements([textElement], null, { - deleteEmptyTextElements: true, + deleteInvisibleElements: true, })[0] as ExcalidrawTextElement; expect(restoredText.isDeleted).toBe(true); expect(restoredText).toMatchSnapshot({ @@ -193,13 +197,16 @@ describe("restoreElements", () => { y: 0, }); - const restoredElements = restore.restoreElements([arrowElement], null); + const restoredElements = restore.restoreElements([arrowElement], null, { + deleteInvisibleElements: true, + }); const restoredArrow = restoredElements[0] as | ExcalidrawArrowElement | undefined; - expect(restoredArrow).toBeUndefined(); + expect(restoredArrow).not.toBeUndefined(); + expect(restoredArrow?.isDeleted).toBe(true); }); it("should keep 'imperceptibly' small freedraw/line elements", () => { @@ -864,6 +871,7 @@ describe("repairing bindings", () => { let restoredElements = restore.restoreElements( [container, invisibleBoundElement, boundElement], null, + { deleteInvisibleElements: true }, ); expect(restoredElements).toEqual([ @@ -871,6 +879,11 @@ describe("repairing bindings", () => { id: container.id, boundElements: [obsoleteBinding, invisibleBinding, nonExistentBinding], }), + expect.objectContaining({ + id: invisibleBoundElement.id, + containerId: container.id, + isDeleted: true, + }), expect.objectContaining({ id: boundElement.id, containerId: container.id, @@ -880,7 +893,7 @@ describe("repairing bindings", () => { restoredElements = restore.restoreElements( [container, invisibleBoundElement, boundElement], null, - { repairBindings: true }, + { repairBindings: true, deleteInvisibleElements: true }, ); expect(restoredElements).toEqual([ @@ -888,6 +901,11 @@ describe("repairing bindings", () => { id: container.id, boundElements: [], }), + expect.objectContaining({ + id: invisibleBoundElement.id, + containerId: container.id, + isDeleted: true, + }), expect.objectContaining({ id: boundElement.id, containerId: container.id, diff --git a/packages/excalidraw/tests/dragCreate.test.tsx b/packages/excalidraw/tests/dragCreate.test.tsx index 810efa973e..566c839050 100644 --- a/packages/excalidraw/tests/dragCreate.test.tsx +++ b/packages/excalidraw/tests/dragCreate.test.tsx @@ -315,7 +315,12 @@ describe("Test dragCreate", () => { ); expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`5`); expect(h.state.selectionElement).toBeNull(); - expect(h.elements.length).toEqual(0); + expect(h.elements).toEqual([ + expect.objectContaining({ + type: "arrow", + isDeleted: true, + }), + ]); }); it("line", async () => { @@ -344,7 +349,12 @@ describe("Test dragCreate", () => { ); expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`5`); expect(h.state.selectionElement).toBeNull(); - expect(h.elements.length).toEqual(0); + expect(h.elements).toEqual([ + expect.objectContaining({ + type: "line", + isDeleted: true, + }), + ]); }); }); }); diff --git a/packages/utils/src/export.ts b/packages/utils/src/export.ts index 7821b51608..fefd0db2e0 100644 --- a/packages/utils/src/export.ts +++ b/packages/utils/src/export.ts @@ -49,7 +49,7 @@ export const exportToCanvas = ({ { elements, appState }, null, null, - { deleteEmptyTextElements: true }, + { deleteInvisibleElements: true }, ); const { exportBackground, viewBackgroundColor } = restoredAppState; return _exportToCanvas( @@ -180,7 +180,7 @@ export const exportToSvg = async ({ { elements, appState }, null, null, - { deleteEmptyTextElements: true }, + { deleteInvisibleElements: true }, ); const exportAppState = {