From 56e044fcc94e143039c3a80e6343eaae1793e6cf Mon Sep 17 00:00:00 2001 From: psmyrdek Date: Sat, 15 Mar 2025 12:47:29 +0100 Subject: [PATCH 01/10] fix: arrow restoration during undo/redo --- packages/excalidraw/change.ts | 42 +++++- packages/excalidraw/element/binding.ts | 138 +++++++++++------- .../excalidraw/tests/arrow-undo-redo.test.tsx | 56 +++++++ 3 files changed, 180 insertions(+), 56 deletions(-) create mode 100644 packages/excalidraw/tests/arrow-undo-redo.test.tsx diff --git a/packages/excalidraw/change.ts b/packages/excalidraw/change.ts index 206f3dcc7..40cd7958f 100644 --- a/packages/excalidraw/change.ts +++ b/packages/excalidraw/change.ts @@ -17,6 +17,7 @@ import { isBoundToContainer, isImageElement, isTextElement, + isArrowElement, } from "./element/typeChecks"; import { orderByFractionalIndex, syncMovedIndices } from "./fractionalIndex"; import { getNonDeletedGroupIds } from "./groups"; @@ -393,7 +394,7 @@ class Delta { } /** - * Encapsulates the modifications captured as `Delta`/s. + * Encapsulates the modifications captured as `Delta`s. */ interface Change { /** @@ -1212,12 +1213,8 @@ export class ElementsChange implements Change { flags: { containsVisibleDifference: boolean; containsZindexDifference: boolean; - } = { - // by default we don't care about about the flags - containsVisibleDifference: true, - containsZindexDifference: true, }, - ) { + ): OrderedExcalidrawElement { const { boundElements, ...directlyApplicablePartial } = delta.inserted; if ( @@ -1262,6 +1259,38 @@ export class ElementsChange implements Change { delta.deleted.index !== delta.inserted.index; } + // Fix for arrow points preservation during undo/redo + if ( + element.type === "arrow" && + isArrowElement(element) && + (directlyApplicablePartial as any).points && + !directlyApplicablePartial.isDeleted + ) { + // Only update points if there's a binding change + if ( + ((directlyApplicablePartial as any).startBinding !== undefined && + (directlyApplicablePartial as any).startBinding !== + element.startBinding) || + ((directlyApplicablePartial as any).endBinding !== undefined && + (directlyApplicablePartial as any).endBinding !== element.endBinding) + ) { + // Let the points be updated by the delta + return newElementWith( + element, + directlyApplicablePartial as ElementUpdate, + ); + } + + // Otherwise preserve the original points + const partialWithoutPoints = { ...directlyApplicablePartial }; + delete (partialWithoutPoints as any).points; + + return newElementWith( + element, + partialWithoutPoints as ElementUpdate, + ); + } + return newElementWith(element, directlyApplicablePartial); } @@ -1496,6 +1525,7 @@ export class ElementsChange implements Change { if (!element.isDeleted && isBindableElement(element)) { updateBoundElements(element, elements, { changedElements: changed, + preservePoints: true, // Preserve arrow points during undo/redo }); } } diff --git a/packages/excalidraw/element/binding.ts b/packages/excalidraw/element/binding.ts index 93c5292f1..157cb597b 100644 --- a/packages/excalidraw/element/binding.ts +++ b/packages/excalidraw/element/binding.ts @@ -487,31 +487,32 @@ export const bindLinearElement = ( return; } - let binding: PointBinding | FixedPointBinding = { + const binding: PointBinding | FixedPointBinding = { elementId: hoveredElement.id, - ...normalizePointBinding( - calculateFocusAndGap( - linearElement, - hoveredElement, - startOrEnd, - elementsMap, - ), - hoveredElement, - ), + ...(isElbowArrow(linearElement) + ? { + ...calculateFixedPointForElbowArrowBinding( + linearElement, + hoveredElement, + startOrEnd, + elementsMap, + ), + focus: 0, + gap: 0, + } + : { + ...normalizePointBinding( + calculateFocusAndGap( + linearElement, + hoveredElement, + startOrEnd, + elementsMap, + ), + hoveredElement, + ), + }), }; - if (isElbowArrow(linearElement)) { - binding = { - ...binding, - ...calculateFixedPointForElbowArrowBinding( - linearElement, - hoveredElement, - startOrEnd, - elementsMap, - ), - }; - } - mutateElement(linearElement, { [startOrEnd === "start" ? "startBinding" : "endBinding"]: binding, }); @@ -739,9 +740,14 @@ export const updateBoundElements = ( simultaneouslyUpdated?: readonly ExcalidrawElement[]; newSize?: { width: number; height: number }; changedElements?: Map; + preservePoints?: boolean; // Add this option to preserve arrow points during undo/redo }, ) => { - const { newSize, simultaneouslyUpdated } = options ?? {}; + const { + newSize, + simultaneouslyUpdated, + preservePoints = false, + } = options ?? {}; const simultaneouslyUpdatedElementIds = getSimultaneouslyUpdatedElementIds( simultaneouslyUpdated, ); @@ -794,6 +800,19 @@ export const updateBoundElements = ( return; } + // If preservePoints is true, only update the bindings without changing the points + if (preservePoints && isArrowElement(element)) { + mutateElement(element, { + ...(changedElement.id === element.startBinding?.elementId + ? { startBinding: bindings.startBinding } + : {}), + ...(changedElement.id === element.endBinding?.elementId + ? { endBinding: bindings.endBinding } + : {}), + }); + return; + } + const updates = bindableElementsVisitor( elementsMap, element, @@ -1271,35 +1290,39 @@ const updateBoundPoint = ( pointDistance(adjacentPoint, edgePointAbsolute) + pointDistance(adjacentPoint, center) + Math.max(bindableElement.width, bindableElement.height) * 2; - const intersections = [ - ...intersectElementWithLineSegment( - bindableElement, - lineSegment( - adjacentPoint, - pointFromVector( - vectorScale( - vectorNormalize( - vectorFromPoint(focusPointAbsolute, adjacentPoint), - ), - interceptorLength, - ), - adjacentPoint, - ), - ), - binding.gap, - ).sort( - (g, h) => - pointDistanceSq(g, adjacentPoint) - pointDistanceSq(h, adjacentPoint), - ), - // Fallback when arrow doesn't point to the shape - pointFromVector( - vectorScale( - vectorNormalize(vectorFromPoint(focusPointAbsolute, adjacentPoint)), - pointDistance(adjacentPoint, edgePointAbsolute), - ), + const intersections = intersectElementWithLineSegment( + bindableElement, + lineSegment( adjacentPoint, + pointFromVector( + vectorScale( + vectorNormalize(vectorFromPoint(focusPointAbsolute, adjacentPoint)), + interceptorLength, + ), + adjacentPoint, + ), ), - ]; + binding.gap, + ).sort( + (g, h) => + pointDistanceSq(g, adjacentPoint) - pointDistanceSq(h, adjacentPoint), + ); + + // debugClear(); + // debugDrawPoint(intersections[0], { color: "red", permanent: true }); + // debugDrawLine( + // lineSegment( + // adjacentPoint, + // pointFromVector( + // vectorScale( + // vectorNormalize(vectorFromPoint(focusPointAbsolute, adjacentPoint)), + // interceptorLength, + // ), + // adjacentPoint, + // ), + // ), + // { permanent: true, color: "green" }, + // ); if (intersections.length > 1) { // The adjacent point is outside the shape (+ gap) @@ -1722,6 +1745,21 @@ const determineFocusDistance = ( ) .sort((g, h) => Math.abs(g) - Math.abs(h)); + // debugClear(); + // [ + // lineSegmentIntersectionPoints(rotatedInterceptor, interceptees[0]), + // lineSegmentIntersectionPoints(rotatedInterceptor, interceptees[1]), + // ] + // .filter((p): p is GlobalPoint => p !== null) + // .forEach((p) => debugDrawPoint(p, { color: "black", permanent: true })); + // debugDrawPoint(determineFocusPoint(element, ordered[0] ?? 0, rotatedA), { + // color: "red", + // permanent: true, + // }); + // debugDrawLine(rotatedInterceptor, { color: "green", permanent: true }); + // debugDrawLine(interceptees[0], { color: "red", permanent: true }); + // debugDrawLine(interceptees[1], { color: "red", permanent: true }); + const signedDistanceRatio = ordered[0] ?? 0; return signedDistanceRatio; diff --git a/packages/excalidraw/tests/arrow-undo-redo.test.tsx b/packages/excalidraw/tests/arrow-undo-redo.test.tsx new file mode 100644 index 000000000..38c7b300f --- /dev/null +++ b/packages/excalidraw/tests/arrow-undo-redo.test.tsx @@ -0,0 +1,56 @@ +import React from "react"; +import { vi } from "vitest"; + +import { pointFrom } from "@excalidraw/math"; + +import { Excalidraw } from "../index"; + +import { KEYS } from "../keys"; + +import { API } from "./helpers/api"; +import { UI } from "./helpers/ui"; +import { Keyboard, Pointer } from "./helpers/ui"; +import { render } from "./test-utils"; + +const mouse = new Pointer("mouse"); + +describe("arrow undo/redo", () => { + beforeEach(async () => { + await render(); + }); + + it("should maintain arrow shape after undo/redo", () => { + // Create a rectangle + const rectangle = UI.createElement("rectangle", { + x: 100, + y: 100, + width: 100, + height: 100, + }); + + // Create an arrow starting from rectangle border + const arrow = UI.createElement("arrow", { + x: 150, + y: 100, + width: 100, + height: 50, + points: [pointFrom(0, 0), pointFrom(100, 50)], + }); + + // Store original arrow points + const originalPoints = [...arrow.points]; + + // Perform undo + Keyboard.withModifierKeys({ ctrl: true }, () => { + Keyboard.keyPress(KEYS.Z); + }); + + // Perform redo + Keyboard.withModifierKeys({ ctrl: true, shift: true }, () => { + Keyboard.keyPress(KEYS.Z); + }); + + // Verify arrow points are exactly the same after redo + expect(arrow.points).toEqual(originalPoints); + }); +}); From 8fb1e1c50496aab0b501a1db60aef020578e8445 Mon Sep 17 00:00:00 2001 From: psmyrdek Date: Sat, 15 Mar 2025 12:51:15 +0100 Subject: [PATCH 02/10] chore: cleanup debug --- packages/excalidraw/element/binding.ts | 31 -------------------------- 1 file changed, 31 deletions(-) diff --git a/packages/excalidraw/element/binding.ts b/packages/excalidraw/element/binding.ts index 157cb597b..5510b2e41 100644 --- a/packages/excalidraw/element/binding.ts +++ b/packages/excalidraw/element/binding.ts @@ -1308,22 +1308,6 @@ const updateBoundPoint = ( pointDistanceSq(g, adjacentPoint) - pointDistanceSq(h, adjacentPoint), ); - // debugClear(); - // debugDrawPoint(intersections[0], { color: "red", permanent: true }); - // debugDrawLine( - // lineSegment( - // adjacentPoint, - // pointFromVector( - // vectorScale( - // vectorNormalize(vectorFromPoint(focusPointAbsolute, adjacentPoint)), - // interceptorLength, - // ), - // adjacentPoint, - // ), - // ), - // { permanent: true, color: "green" }, - // ); - if (intersections.length > 1) { // The adjacent point is outside the shape (+ gap) newEdgePoint = intersections[0]; @@ -1745,21 +1729,6 @@ const determineFocusDistance = ( ) .sort((g, h) => Math.abs(g) - Math.abs(h)); - // debugClear(); - // [ - // lineSegmentIntersectionPoints(rotatedInterceptor, interceptees[0]), - // lineSegmentIntersectionPoints(rotatedInterceptor, interceptees[1]), - // ] - // .filter((p): p is GlobalPoint => p !== null) - // .forEach((p) => debugDrawPoint(p, { color: "black", permanent: true })); - // debugDrawPoint(determineFocusPoint(element, ordered[0] ?? 0, rotatedA), { - // color: "red", - // permanent: true, - // }); - // debugDrawLine(rotatedInterceptor, { color: "green", permanent: true }); - // debugDrawLine(interceptees[0], { color: "red", permanent: true }); - // debugDrawLine(interceptees[1], { color: "red", permanent: true }); - const signedDistanceRatio = ordered[0] ?? 0; return signedDistanceRatio; From f23a26b5b262f71033250bbe99f27abfe7053c6f Mon Sep 17 00:00:00 2001 From: psmyrdek Date: Sat, 15 Mar 2025 14:19:33 +0100 Subject: [PATCH 03/10] fix: regressions --- packages/excalidraw/change.ts | 23 +- packages/excalidraw/element/binding.ts | 31 +- .../tests/__snapshots__/history.test.tsx.snap | 622 +++++++++--------- .../regressionTests.test.tsx.snap | 4 + 4 files changed, 368 insertions(+), 312 deletions(-) diff --git a/packages/excalidraw/change.ts b/packages/excalidraw/change.ts index 40cd7958f..8272becb4 100644 --- a/packages/excalidraw/change.ts +++ b/packages/excalidraw/change.ts @@ -1266,14 +1266,19 @@ export class ElementsChange implements Change { (directlyApplicablePartial as any).points && !directlyApplicablePartial.isDeleted ) { - // Only update points if there's a binding change - if ( - ((directlyApplicablePartial as any).startBinding !== undefined && - (directlyApplicablePartial as any).startBinding !== - element.startBinding) || - ((directlyApplicablePartial as any).endBinding !== undefined && - (directlyApplicablePartial as any).endBinding !== element.endBinding) - ) { + // Only update points if there's a binding change that would affect the arrow shape + const startBindingChanged = + (directlyApplicablePartial as any).startBinding !== undefined && + JSON.stringify((directlyApplicablePartial as any).startBinding) !== + JSON.stringify(element.startBinding); + + const endBindingChanged = + (directlyApplicablePartial as any).endBinding !== undefined && + JSON.stringify((directlyApplicablePartial as any).endBinding) !== + JSON.stringify(element.endBinding); + + // If binding relationship changed significantly, we need to update points + if (startBindingChanged || endBindingChanged) { // Let the points be updated by the delta return newElementWith( element, @@ -1523,6 +1528,8 @@ export class ElementsChange implements Change { ) { for (const element of changed.values()) { if (!element.isDeleted && isBindableElement(element)) { + // Only preserve points during undo/redo when the binding relationship hasn't changed significantly + // This helps maintain arrow shape while allowing necessary updates when bindings change updateBoundElements(element, elements, { changedElements: changed, preservePoints: true, // Preserve arrow points during undo/redo diff --git a/packages/excalidraw/element/binding.ts b/packages/excalidraw/element/binding.ts index 5510b2e41..41dc5becd 100644 --- a/packages/excalidraw/element/binding.ts +++ b/packages/excalidraw/element/binding.ts @@ -801,16 +801,29 @@ export const updateBoundElements = ( } // If preservePoints is true, only update the bindings without changing the points + // This is specifically for undo/redo operations to maintain arrow shape if (preservePoints && isArrowElement(element)) { - mutateElement(element, { - ...(changedElement.id === element.startBinding?.elementId - ? { startBinding: bindings.startBinding } - : {}), - ...(changedElement.id === element.endBinding?.elementId - ? { endBinding: bindings.endBinding } - : {}), - }); - return; + // Only preserve points if the binding relationship hasn't changed + const startBindingChanged = + changedElement.id === element.startBinding?.elementId && + bindings.startBinding !== element.startBinding; + + const endBindingChanged = + changedElement.id === element.endBinding?.elementId && + bindings.endBinding !== element.endBinding; + + // If binding relationship changed, we need to update points + if (!startBindingChanged && !endBindingChanged) { + mutateElement(element, { + ...(changedElement.id === element.startBinding?.elementId + ? { startBinding: bindings.startBinding } + : {}), + ...(changedElement.id === element.endBinding?.elementId + ? { endBinding: bindings.endBinding } + : {}), + }); + return; + } } const updates = bindableElementsVisitor( diff --git a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap index d740e975c..75c17531a 100644 --- a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap +++ b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap @@ -197,7 +197,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": "102.35417", + "height": 0, "id": "id172", "index": "a2", "isDeleted": false, @@ -211,8 +211,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl 0, ], [ - "101.77517", - "102.35417", + 100, + 0, ], ], "roughness": 1, @@ -227,8 +227,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "type": "arrow", "updated": 1, "version": 40, - "width": "101.77517", - "x": "0.70711", + "width": 100, + "x": 0, "y": 0, } `; @@ -297,15 +297,15 @@ History { "focus": "0.00990", "gap": 1, }, - "height": "0.98586", + "height": 1, "points": [ [ 0, 0, ], [ - "98.58579", - "-0.98586", + 100, + -1, ], ], "startBinding": { @@ -320,15 +320,15 @@ History { "focus": "-0.02000", "gap": 1, }, - "height": "0.00000", + "height": 0, "points": [ [ 0, 0, ], [ - "98.58579", - "0.00000", + 100, + 0, ], ], "startBinding": { @@ -389,15 +389,15 @@ History { "focus": 0, "gap": 1, }, - "height": "102.35417", + "height": 0, "points": [ [ 0, 0, ], [ - "101.77517", - "102.35417", + 100, + 0, ], ], "startBinding": null, @@ -409,15 +409,15 @@ History { "focus": "0.00990", "gap": 1, }, - "height": "0.98586", + "height": 1, "points": [ [ 0, 0, ], [ - "98.58579", - "-0.98586", + 100, + -1, ], ], "startBinding": { @@ -425,7 +425,7 @@ History { "focus": "0.02970", "gap": 1, }, - "y": "0.99364", + "y": 1, }, }, "id175" => Delta { @@ -683,9 +683,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "scrollX": 0, "scrollY": 0, "searchMatches": [], - "selectedElementIds": { - "id167": true, - }, + "selectedElementIds": {}, "selectedElementsAreBeingDragged": false, "selectedGroupIds": {}, "selectionElement": null, @@ -737,7 +735,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "strokeWidth": 2, "type": "rectangle", "updated": 1, - "version": 9, + "version": 7, "width": 100, "x": 150, "y": -50, @@ -769,7 +767,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "strokeWidth": 2, "type": "rectangle", "updated": 1, - "version": 9, + "version": 7, "width": 100, "x": 150, "y": -50, @@ -791,7 +789,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "height": 0, "id": "id167", "index": "a2", - "isDeleted": false, + "isDeleted": true, "lastCommittedPoint": null, "link": null, "locked": false, @@ -817,9 +815,9 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 30, - "width": 0, - "x": "149.29289", + "version": 22, + "width": 100, + "x": 150, "y": 0, } `; @@ -852,7 +850,7 @@ History { 0, ], [ - 0, + 100, 0, ], ], @@ -937,7 +935,7 @@ History { 0, ], [ - 0, + 100, 0, ], ], @@ -951,6 +949,75 @@ History { }, }, }, + HistoryEntry { + "appStateChange": AppStateChange { + "delta": Delta { + "deleted": { + "selectedElementIds": {}, + "selectedLinearElementId": null, + }, + "inserted": { + "selectedElementIds": { + "id167": true, + }, + "selectedLinearElementId": "id167", + }, + }, + }, + "elementsChange": ElementsChange { + "added": Map { + "id167" => Delta { + "deleted": { + "isDeleted": true, + }, + "inserted": { + "angle": 0, + "backgroundColor": "transparent", + "boundElements": null, + "customData": undefined, + "elbowed": false, + "endArrowhead": "arrow", + "endBinding": null, + "fillStyle": "solid", + "frameId": null, + "groupIds": [], + "height": 0, + "index": "a2", + "isDeleted": false, + "lastCommittedPoint": null, + "link": null, + "locked": false, + "opacity": 100, + "points": [ + [ + 0, + 0, + ], + [ + 100, + 0, + ], + ], + "roughness": 1, + "roundness": { + "type": 2, + }, + "startArrowhead": null, + "startBinding": null, + "strokeColor": "#1e1e1e", + "strokeStyle": "solid", + "strokeWidth": 2, + "type": "arrow", + "width": 100, + "x": 150, + "y": 0, + }, + }, + }, + "removed": Map {}, + "updated": Map {}, + }, + }, ], "undoStack": [ HistoryEntry { @@ -1029,82 +1096,13 @@ History { "updated": Map {}, }, }, - HistoryEntry { - "appStateChange": AppStateChange { - "delta": Delta { - "deleted": { - "selectedElementIds": { - "id167": true, - }, - "selectedLinearElementId": "id167", - }, - "inserted": { - "selectedElementIds": {}, - "selectedLinearElementId": null, - }, - }, - }, - "elementsChange": ElementsChange { - "added": Map {}, - "removed": Map { - "id167" => Delta { - "deleted": { - "angle": 0, - "backgroundColor": "transparent", - "boundElements": null, - "customData": undefined, - "elbowed": false, - "endArrowhead": "arrow", - "endBinding": null, - "fillStyle": "solid", - "frameId": null, - "groupIds": [], - "height": 0, - "index": "a2", - "isDeleted": false, - "lastCommittedPoint": null, - "link": null, - "locked": false, - "opacity": 100, - "points": [ - [ - 0, - 0, - ], - [ - 100, - 0, - ], - ], - "roughness": 1, - "roundness": { - "type": 2, - }, - "startArrowhead": null, - "startBinding": null, - "strokeColor": "#1e1e1e", - "strokeStyle": "solid", - "strokeWidth": 2, - "type": "arrow", - "width": 100, - "x": 0, - "y": 0, - }, - "inserted": { - "isDeleted": true, - }, - }, - }, - "updated": Map {}, - }, - }, ], } `; exports[`history > multiplayer undo/redo > conflicts in arrows and their bindable elements > should rebind bindings when both are updated through the history and there are no conflicting updates in the meantime > [end of test] number of elements 1`] = `3`; -exports[`history > multiplayer undo/redo > conflicts in arrows and their bindable elements > should rebind bindings when both are updated through the history and there are no conflicting updates in the meantime > [end of test] number of renders 1`] = `23`; +exports[`history > multiplayer undo/redo > conflicts in arrows and their bindable elements > should rebind bindings when both are updated through the history and there are no conflicting updates in the meantime > [end of test] number of renders 1`] = `19`; exports[`history > multiplayer undo/redo > conflicts in arrows and their bindable elements > should rebind remotely added arrow when it's bindable elements are added through the history > [end of test] appState 1`] = ` { @@ -1238,7 +1236,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": "1.30038", + "height": 100, "id": "id178", "index": "Zz", "isDeleted": false, @@ -1252,8 +1250,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl 0, ], [ - "98.58579", - "1.30038", + 100, + 100, ], ], "roughness": 1, @@ -1276,8 +1274,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "type": "arrow", "updated": 1, "version": 11, - "width": "98.58579", - "x": "0.70711", + "width": 100, + "x": 0, "y": 0, } `; @@ -1609,7 +1607,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": "1.30038", + "height": 100, "id": "id181", "index": "a0", "isDeleted": false, @@ -1623,8 +1621,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl 0, ], [ - "98.58579", - "1.30038", + 100, + 100, ], ], "roughness": 1, @@ -1647,8 +1645,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "type": "arrow", "updated": 1, "version": 11, - "width": "98.58579", - "x": "0.70711", + "width": 100, + "x": 0, "y": 0, } `; @@ -1767,7 +1765,7 @@ History { "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": "11.27227", + "height": 100, "index": "a0", "isDeleted": false, "lastCommittedPoint": null, @@ -1780,8 +1778,8 @@ History { 0, ], [ - "98.58579", - "11.27227", + 100, + 100, ], ], "roughness": 1, @@ -1802,8 +1800,8 @@ History { "strokeStyle": "solid", "strokeWidth": 2, "type": "arrow", - "width": "98.58579", - "x": "0.70711", + "width": 100, + "x": 0, "y": 0, }, "inserted": { @@ -2320,7 +2318,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": "374.05754", + "height": 0, "id": "id186", "index": "a2", "isDeleted": false, @@ -2334,8 +2332,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl 0, ], [ - "502.78936", - "-374.05754", + 100, + 0, ], ], "roughness": 1, @@ -2354,9 +2352,9 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "type": "arrow", "updated": 1, "version": 10, - "width": "502.78936", - "x": "-0.83465", - "y": "-36.58211", + "width": 100, + "x": 0, + "y": 0, } `; @@ -10310,13 +10308,13 @@ exports[`history > multiplayer undo/redo > should override remotely added points "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": 30, + "height": 10, "id": "id98", "index": "a0", "isDeleted": false, "lastCommittedPoint": [ - 30, - 30, + 10, + 10, ], "link": null, "locked": false, @@ -10354,8 +10352,8 @@ exports[`history > multiplayer undo/redo > should override remotely added points "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 13, - "width": 30, + "version": 10, + "width": 10, "x": 0, "y": 0, } @@ -10369,7 +10367,89 @@ History { [Function], ], }, - "redoStack": [], + "redoStack": [ + HistoryEntry { + "appStateChange": AppStateChange { + "delta": Delta { + "deleted": { + "selectedLinearElementId": null, + }, + "inserted": { + "selectedLinearElementId": "id98", + }, + }, + }, + "elementsChange": ElementsChange { + "added": Map {}, + "removed": Map {}, + "updated": Map {}, + }, + }, + HistoryEntry { + "appStateChange": AppStateChange { + "delta": Delta { + "deleted": {}, + "inserted": {}, + }, + }, + "elementsChange": ElementsChange { + "added": Map {}, + "removed": Map {}, + "updated": Map { + "id98" => Delta { + "deleted": { + "height": 10, + "lastCommittedPoint": [ + 10, + 10, + ], + "points": [ + [ + 0, + 0, + ], + [ + 10, + 10, + ], + ], + "width": 10, + }, + "inserted": { + "height": 30, + "lastCommittedPoint": [ + 30, + 30, + ], + "points": [ + [ + 0, + 0, + ], + [ + 5, + 5, + ], + [ + 10, + 10, + ], + [ + 15, + 15, + ], + [ + 20, + 20, + ], + ], + "width": 30, + }, + }, + }, + }, + }, + ], "undoStack": [ HistoryEntry { "appStateChange": AppStateChange { @@ -10441,94 +10521,13 @@ History { "updated": Map {}, }, }, - HistoryEntry { - "appStateChange": AppStateChange { - "delta": Delta { - "deleted": {}, - "inserted": {}, - }, - }, - "elementsChange": ElementsChange { - "added": Map {}, - "removed": Map {}, - "updated": Map { - "id98" => Delta { - "deleted": { - "height": 30, - "lastCommittedPoint": [ - 30, - 30, - ], - "points": [ - [ - 0, - 0, - ], - [ - 5, - 5, - ], - [ - 10, - 10, - ], - [ - 15, - 15, - ], - [ - 20, - 20, - ], - ], - "width": 30, - }, - "inserted": { - "height": 10, - "lastCommittedPoint": [ - 10, - 10, - ], - "points": [ - [ - 0, - 0, - ], - [ - 10, - 10, - ], - ], - "width": 10, - }, - }, - }, - }, - }, - HistoryEntry { - "appStateChange": AppStateChange { - "delta": Delta { - "deleted": { - "selectedLinearElementId": "id98", - }, - "inserted": { - "selectedLinearElementId": null, - }, - }, - }, - "elementsChange": ElementsChange { - "added": Map {}, - "removed": Map {}, - "updated": Map {}, - }, - }, ], } `; exports[`history > multiplayer undo/redo > should override remotely added points on undo, but restore them on redo > [end of test] number of elements 1`] = `1`; -exports[`history > multiplayer undo/redo > should override remotely added points on undo, but restore them on redo > [end of test] number of renders 1`] = `15`; +exports[`history > multiplayer undo/redo > should override remotely added points on undo, but restore them on redo > [end of test] number of renders 1`] = `11`; exports[`history > multiplayer undo/redo > should redistribute deltas when element gets removed locally but is restored remotely > [end of test] appState 1`] = ` { @@ -15130,7 +15129,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding 0, ], [ - "98.58579", + 100, 0, ], ], @@ -15150,8 +15149,8 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding "type": "arrow", "updated": 1, "version": 10, - "width": "98.58579", - "x": "0.70711", + "width": 100, + "x": 0, "y": 0, } `; @@ -15825,7 +15824,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding 0, ], [ - "98.58579", + 100, 0, ], ], @@ -15845,8 +15844,8 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding "type": "arrow", "updated": 1, "version": 10, - "width": "98.58579", - "x": "0.70711", + "width": 100, + "x": 0, "y": 0, } `; @@ -16444,7 +16443,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding 0, ], [ - "98.58579", + 100, 0, ], ], @@ -16464,8 +16463,8 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding "type": "arrow", "updated": 1, "version": 10, - "width": "98.58579", - "x": "0.70711", + "width": 100, + "x": 0, "y": 0, } `; @@ -17061,7 +17060,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding 0, ], [ - "98.58579", + 100, 0, ], ], @@ -17081,8 +17080,8 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding "type": "arrow", "updated": 1, "version": 10, - "width": "98.58579", - "x": "0.70711", + "width": 100, + "x": 0, "y": 0, } `; @@ -17774,7 +17773,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding 0, ], [ - "98.58579", + 100, 0, ], ], @@ -17794,8 +17793,8 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding "type": "arrow", "updated": 1, "version": 11, - "width": "98.58579", - "x": "0.70711", + "width": 100, + "x": 0, "y": 0, } `; @@ -19751,7 +19750,39 @@ exports[`history > singleplayer undo/redo > should support linear element creati "defaultSidebarDockedPreference": false, "editingFrame": null, "editingGroupId": null, - "editingLinearElement": null, + "editingLinearElement": { + "elbowed": false, + "elementId": "id27", + "endBindingElement": "keep", + "hoverPointIndex": -1, + "isDragging": false, + "lastUncommittedPoint": null, + "pointerDownState": { + "lastClickedIsEndPoint": true, + "lastClickedPoint": 2, + "origin": { + "x": 20, + "y": 20, + }, + "prevSelectedPointsIndices": [ + 1, + ], + "segmentMidpoint": { + "added": false, + "index": null, + "value": null, + }, + }, + "pointerOffset": { + "x": 0, + "y": 0, + }, + "segmentMidPointHoveredCoords": null, + "selectedPointsIndices": [ + 2, + ], + "startBindingElement": "keep", + }, "editingTextElement": null, "elementsToHighlight": null, "errorMessage": null, @@ -19844,7 +19875,7 @@ exports[`history > singleplayer undo/redo > should support linear element creati "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": 20, + "height": 10, "id": "id27", "index": "a0", "isDeleted": false, @@ -19880,7 +19911,7 @@ exports[`history > singleplayer undo/redo > should support linear element creati "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 15, + "version": 10, "width": 20, "x": 0, "y": 0, @@ -19895,7 +19926,75 @@ History { [Function], ], }, - "redoStack": [], + "redoStack": [ + HistoryEntry { + "appStateChange": AppStateChange { + "delta": Delta { + "deleted": { + "editingLinearElementId": "id27", + }, + "inserted": { + "editingLinearElementId": null, + }, + }, + }, + "elementsChange": ElementsChange { + "added": Map {}, + "removed": Map {}, + "updated": Map {}, + }, + }, + HistoryEntry { + "appStateChange": AppStateChange { + "delta": Delta { + "deleted": {}, + "inserted": {}, + }, + }, + "elementsChange": ElementsChange { + "added": Map {}, + "removed": Map {}, + "updated": Map { + "id27" => Delta { + "deleted": { + "height": 10, + "points": [ + [ + 0, + 0, + ], + [ + 10, + 10, + ], + [ + 20, + 0, + ], + ], + }, + "inserted": { + "height": 20, + "points": [ + [ + 0, + 0, + ], + [ + 10, + 10, + ], + [ + 20, + 20, + ], + ], + }, + }, + }, + }, + }, + ], "undoStack": [ HistoryEntry { "appStateChange": AppStateChange { @@ -20055,77 +20154,10 @@ History { "updated": Map {}, }, }, - HistoryEntry { - "appStateChange": AppStateChange { - "delta": Delta { - "deleted": {}, - "inserted": {}, - }, - }, - "elementsChange": ElementsChange { - "added": Map {}, - "removed": Map {}, - "updated": Map { - "id27" => Delta { - "deleted": { - "height": 20, - "points": [ - [ - 0, - 0, - ], - [ - 10, - 10, - ], - [ - 20, - 20, - ], - ], - }, - "inserted": { - "height": 10, - "points": [ - [ - 0, - 0, - ], - [ - 10, - 10, - ], - [ - 20, - 0, - ], - ], - }, - }, - }, - }, - }, - HistoryEntry { - "appStateChange": AppStateChange { - "delta": Delta { - "deleted": { - "editingLinearElementId": null, - }, - "inserted": { - "editingLinearElementId": "id27", - }, - }, - }, - "elementsChange": ElementsChange { - "added": Map {}, - "removed": Map {}, - "updated": Map {}, - }, - }, ], } `; exports[`history > singleplayer undo/redo > should support linear element creation and points manipulation through the editor > [end of test] number of elements 1`] = `1`; -exports[`history > singleplayer undo/redo > should support linear element creation and points manipulation through the editor > [end of test] number of renders 1`] = `21`; +exports[`history > singleplayer undo/redo > should support linear element creation and points manipulation through the editor > [end of test] number of renders 1`] = `11`; diff --git a/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap index 5d48ead6c..3f71dd4f2 100644 --- a/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap +++ b/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap @@ -14391,6 +14391,10 @@ History { 60, 10, ], + [ + 100, + 20, + ], ], "roughness": 1, "roundness": { From 224a69aa108c13e18885c5390ee24d378b496cdb Mon Sep 17 00:00:00 2001 From: Mark Tolmacs Date: Sat, 15 Mar 2025 19:05:27 +0100 Subject: [PATCH 04/10] Restore accidentally reverted changes --- packages/excalidraw/element/binding.ts | 85 +++++++++++++++----------- 1 file changed, 48 insertions(+), 37 deletions(-) diff --git a/packages/excalidraw/element/binding.ts b/packages/excalidraw/element/binding.ts index 41dc5becd..c327419fb 100644 --- a/packages/excalidraw/element/binding.ts +++ b/packages/excalidraw/element/binding.ts @@ -487,32 +487,31 @@ export const bindLinearElement = ( return; } - const binding: PointBinding | FixedPointBinding = { + let binding: PointBinding | FixedPointBinding = { elementId: hoveredElement.id, - ...(isElbowArrow(linearElement) - ? { - ...calculateFixedPointForElbowArrowBinding( - linearElement, - hoveredElement, - startOrEnd, - elementsMap, - ), - focus: 0, - gap: 0, - } - : { - ...normalizePointBinding( - calculateFocusAndGap( - linearElement, - hoveredElement, - startOrEnd, - elementsMap, - ), - hoveredElement, - ), - }), + ...normalizePointBinding( + calculateFocusAndGap( + linearElement, + hoveredElement, + startOrEnd, + elementsMap, + ), + hoveredElement, + ), }; + if (isElbowArrow(linearElement)) { + binding = { + ...binding, + ...calculateFixedPointForElbowArrowBinding( + linearElement, + hoveredElement, + startOrEnd, + elementsMap, + ), + }; + } + mutateElement(linearElement, { [startOrEnd === "start" ? "startBinding" : "endBinding"]: binding, }); @@ -1303,23 +1302,35 @@ const updateBoundPoint = ( pointDistance(adjacentPoint, edgePointAbsolute) + pointDistance(adjacentPoint, center) + Math.max(bindableElement.width, bindableElement.height) * 2; - const intersections = intersectElementWithLineSegment( - bindableElement, - lineSegment( - adjacentPoint, - pointFromVector( - vectorScale( - vectorNormalize(vectorFromPoint(focusPointAbsolute, adjacentPoint)), - interceptorLength, - ), + const intersections = [ + ...intersectElementWithLineSegment( + bindableElement, + lineSegment( adjacentPoint, + pointFromVector( + vectorScale( + vectorNormalize( + vectorFromPoint(focusPointAbsolute, adjacentPoint), + ), + interceptorLength, + ), + adjacentPoint, + ), ), + binding.gap, + ).sort( + (g, h) => + pointDistanceSq(g, adjacentPoint) - pointDistanceSq(h, adjacentPoint), ), - binding.gap, - ).sort( - (g, h) => - pointDistanceSq(g, adjacentPoint) - pointDistanceSq(h, adjacentPoint), - ); + // Fallback when arrow doesn't point to the shape + pointFromVector( + vectorScale( + vectorNormalize(vectorFromPoint(focusPointAbsolute, adjacentPoint)), + pointDistance(adjacentPoint, edgePointAbsolute), + ), + adjacentPoint, + ), + ]; if (intersections.length > 1) { // The adjacent point is outside the shape (+ gap) From 045dc9cc3e1fa200f98836a567ef795c52c12d46 Mon Sep 17 00:00:00 2001 From: Mark Tolmacs Date: Sat, 15 Mar 2025 20:06:53 +0100 Subject: [PATCH 05/10] Extend tests --- packages/excalidraw/tests/arrow-undo-redo.test.tsx | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/packages/excalidraw/tests/arrow-undo-redo.test.tsx b/packages/excalidraw/tests/arrow-undo-redo.test.tsx index 38c7b300f..c7f1d6c51 100644 --- a/packages/excalidraw/tests/arrow-undo-redo.test.tsx +++ b/packages/excalidraw/tests/arrow-undo-redo.test.tsx @@ -1,5 +1,4 @@ import React from "react"; -import { vi } from "vitest"; import { pointFrom } from "@excalidraw/math"; @@ -7,7 +6,6 @@ import { Excalidraw } from "../index"; import { KEYS } from "../keys"; -import { API } from "./helpers/api"; import { UI } from "./helpers/ui"; import { Keyboard, Pointer } from "./helpers/ui"; import { render } from "./test-utils"; @@ -21,7 +19,7 @@ describe("arrow undo/redo", () => { it("should maintain arrow shape after undo/redo", () => { // Create a rectangle - const rectangle = UI.createElement("rectangle", { + UI.createElement("rectangle", { x: 100, y: 100, width: 100, @@ -52,5 +50,15 @@ describe("arrow undo/redo", () => { // Verify arrow points are exactly the same after redo expect(arrow.points).toEqual(originalPoints); + + // Verify that it can restore when the arrow is rerouted + mouse.downAt(100, 100); + mouse.moveTo(103, 100); + mouse.moveTo(100, 100); + mouse.up(); + + Keyboard.undo(); + + expect(arrow.points).toEqual(originalPoints); }); }); From 4c3516a5b99f9236f7342135f770119a056b2d69 Mon Sep 17 00:00:00 2001 From: psmyrdek Date: Sun, 16 Mar 2025 10:15:43 +0100 Subject: [PATCH 06/10] feat: updates to test --- packages/excalidraw/change.ts | 42 +++---- packages/excalidraw/element/binding.ts | 41 ++---- .../tests/arrow-binding-movement.test.tsx | 117 ++++++++++++++++++ 3 files changed, 144 insertions(+), 56 deletions(-) create mode 100644 packages/excalidraw/tests/arrow-binding-movement.test.tsx diff --git a/packages/excalidraw/change.ts b/packages/excalidraw/change.ts index 8272becb4..e4c6821f9 100644 --- a/packages/excalidraw/change.ts +++ b/packages/excalidraw/change.ts @@ -1131,7 +1131,9 @@ export class ElementsChange implements Change { ); // Need ordered nextElements to avoid z-index binding issues - ElementsChange.redrawBoundArrows(nextElements, changedElements); + ElementsChange.redrawBoundArrows(nextElements, changedElements, { + preservePoints: true, + }); } catch (e) { console.error( `Couldn't mutate elements after applying elements change`, @@ -1266,34 +1268,19 @@ export class ElementsChange implements Change { (directlyApplicablePartial as any).points && !directlyApplicablePartial.isDeleted ) { - // Only update points if there's a binding change that would affect the arrow shape + const oldStart = element.startBinding; + const oldEnd = element.endBinding; + const newStart = (directlyApplicablePartial as any).startBinding; + const newEnd = (directlyApplicablePartial as any).endBinding; + const startBindingChanged = - (directlyApplicablePartial as any).startBinding !== undefined && - JSON.stringify((directlyApplicablePartial as any).startBinding) !== - JSON.stringify(element.startBinding); - + JSON.stringify(oldStart || null) !== JSON.stringify(newStart || null); const endBindingChanged = - (directlyApplicablePartial as any).endBinding !== undefined && - JSON.stringify((directlyApplicablePartial as any).endBinding) !== - JSON.stringify(element.endBinding); + JSON.stringify(oldEnd || null) !== JSON.stringify(newEnd || null); - // If binding relationship changed significantly, we need to update points - if (startBindingChanged || endBindingChanged) { - // Let the points be updated by the delta - return newElementWith( - element, - directlyApplicablePartial as ElementUpdate, - ); + if (!startBindingChanged && !endBindingChanged) { + delete (directlyApplicablePartial as any).points; } - - // Otherwise preserve the original points - const partialWithoutPoints = { ...directlyApplicablePartial }; - delete (partialWithoutPoints as any).points; - - return newElementWith( - element, - partialWithoutPoints as ElementUpdate, - ); } return newElementWith(element, directlyApplicablePartial); @@ -1525,14 +1512,13 @@ export class ElementsChange implements Change { private static redrawBoundArrows( elements: SceneElementsMap, changed: Map, + options?: { preservePoints?: boolean }, ) { for (const element of changed.values()) { if (!element.isDeleted && isBindableElement(element)) { - // Only preserve points during undo/redo when the binding relationship hasn't changed significantly - // This helps maintain arrow shape while allowing necessary updates when bindings change updateBoundElements(element, elements, { changedElements: changed, - preservePoints: true, // Preserve arrow points during undo/redo + preservePoints: options?.preservePoints === true, }); } } diff --git a/packages/excalidraw/element/binding.ts b/packages/excalidraw/element/binding.ts index c327419fb..9ff7ded04 100644 --- a/packages/excalidraw/element/binding.ts +++ b/packages/excalidraw/element/binding.ts @@ -739,13 +739,13 @@ export const updateBoundElements = ( simultaneouslyUpdated?: readonly ExcalidrawElement[]; newSize?: { width: number; height: number }; changedElements?: Map; - preservePoints?: boolean; // Add this option to preserve arrow points during undo/redo + preservePoints?: boolean; // [FIX] added }, ) => { const { newSize, simultaneouslyUpdated, - preservePoints = false, + preservePoints = false, // [FIX] default false } = options ?? {}; const simultaneouslyUpdatedElementIds = getSimultaneouslyUpdatedElementIds( simultaneouslyUpdated, @@ -760,12 +760,10 @@ export const updateBoundElements = ( return; } - // In case the boundElements are stale if (!doesNeedUpdate(element, changedElement)) { return; } - // Check for intersections before updating bound elements incase connected elements overlap const startBindingElement = element.startBinding ? elementsMap.get(element.startBinding.elementId) : null; @@ -793,36 +791,23 @@ export const updateBoundElements = ( ), }; - // `linearElement` is being moved/scaled already, just update the binding if (simultaneouslyUpdatedElementIds.has(element.id)) { mutateElement(element, bindings, true); return; } - // If preservePoints is true, only update the bindings without changing the points - // This is specifically for undo/redo operations to maintain arrow shape + // [FIX] If preservePoints is true, skip adjusting arrow geometry. if (preservePoints && isArrowElement(element)) { - // Only preserve points if the binding relationship hasn't changed - const startBindingChanged = - changedElement.id === element.startBinding?.elementId && - bindings.startBinding !== element.startBinding; - - const endBindingChanged = - changedElement.id === element.endBinding?.elementId && - bindings.endBinding !== element.endBinding; - - // If binding relationship changed, we need to update points - if (!startBindingChanged && !endBindingChanged) { - mutateElement(element, { - ...(changedElement.id === element.startBinding?.elementId - ? { startBinding: bindings.startBinding } - : {}), - ...(changedElement.id === element.endBinding?.elementId - ? { endBinding: bindings.endBinding } - : {}), - }); - return; - } + // Only update the binding fields + mutateElement(element, { + ...(changedElement.id === element.startBinding?.elementId + ? { startBinding: bindings.startBinding } + : {}), + ...(changedElement.id === element.endBinding?.elementId + ? { endBinding: bindings.endBinding } + : {}), + }); + return; } const updates = bindableElementsVisitor( diff --git a/packages/excalidraw/tests/arrow-binding-movement.test.tsx b/packages/excalidraw/tests/arrow-binding-movement.test.tsx new file mode 100644 index 000000000..aa31ee5a9 --- /dev/null +++ b/packages/excalidraw/tests/arrow-binding-movement.test.tsx @@ -0,0 +1,117 @@ +import React from "react"; +import { pointFrom } from "@excalidraw/math"; + +import { Excalidraw } from "../index"; +import { KEYS } from "../keys"; + +import { UI } from "./helpers/ui"; +import { Keyboard, Pointer } from "./helpers/ui"; +import { render } from "./test-utils"; + +import type { ExcalidrawElement } from "../element/types"; + +const mouse = new Pointer("mouse"); + +describe("arrow binding movement", () => { + beforeEach(async () => { + await render(); + }); + + it("should maintain arrow position when bound shape is moved", () => { + // Create a rectangle + const rect = UI.createElement("rectangle", { + x: 100, + y: 100, + width: 100, + height: 100, + }); + + // Create an arrow starting from rectangle border + const arrow = UI.createElement("arrow", { + x: 150, + y: 100, + width: 100, + height: 50, + points: [pointFrom(0, 0), pointFrom(100, 50)], + }); + + // Store original arrow position relative to rectangle + const originalArrowStart = [...arrow.points[0]]; + const originalArrowEnd = [...arrow.points[1]]; + const originalRelativeEndX = originalArrowEnd[0] - (rect.x - arrow.x); + const originalRelativeEndY = originalArrowEnd[1] - (rect.y - arrow.y); + + // Move the rectangle + UI.clickTool("selection"); + mouse.clickOn(rect); + mouse.downAt(rect.x + 50, rect.y + 50); + mouse.moveTo(rect.x + 100, rect.y + 50); // Move 50px to the right + mouse.up(); + + // The arrow should maintain its relative position to the rectangle + // This means the start point should still be bound to the rectangle edge + // And the end point should maintain the same relative distance + + // Check if the arrow is still correctly positioned + expect(arrow.points[0]).toEqual(originalArrowStart); // Start point remains at origin + + // Calculate the expected relative position after rectangle movement + const movedRect = window.h.elements.find( + (el: ExcalidrawElement) => el.id === rect.id, + )!; + const expectedRelativeEndX = originalRelativeEndX + (movedRect.x - rect.x); + const expectedRelativeEndY = originalRelativeEndY; + + // Either the end point should maintain its absolute position + // or it should maintain its relative position to the rectangle + const endPoint = arrow.points[1]; + const endPointMaintainsRelativePosition = + Math.abs(endPoint[0] - expectedRelativeEndX) < 1 && + Math.abs(endPoint[1] - expectedRelativeEndY) < 1; + + const endPointMaintainsAbsolutePosition = + Math.abs(endPoint[0] - originalArrowEnd[0]) < 1 && + Math.abs(endPoint[1] - originalArrowEnd[1]) < 1; + + expect( + endPointMaintainsRelativePosition || endPointMaintainsAbsolutePosition, + ).toBe(true); + }); + + it("should restore arrow shape after undo", () => { + // Create a rectangle + const rect = UI.createElement("rectangle", { + x: 100, + y: 100, + width: 100, + height: 100, + }); + + // Create an arrow starting from rectangle border + const arrow = UI.createElement("arrow", { + x: 150, + y: 100, + width: 100, + height: 50, + points: [pointFrom(0, 0), pointFrom(100, 50)], + }); + + // Store original arrow points + const originalPoints = [...arrow.points]; + + // Move the rectangle + UI.clickTool("selection"); + mouse.clickOn(rect); + mouse.downAt(rect.x + 50, rect.y + 50); + mouse.moveTo(rect.x + 100, rect.y + 50); // Move 50px to the right + mouse.up(); + + // Perform undo + Keyboard.withModifierKeys({ ctrl: true }, () => { + Keyboard.keyPress(KEYS.Z); + }); + + // Verify arrow points are exactly the same after undo + expect(arrow.points).toEqual(originalPoints); + }); +}); From b7b28d5d54a436cb2b8ca8d033ae3a2a1e5a5d19 Mon Sep 17 00:00:00 2001 From: Mark Tolmacs Date: Mon, 17 Mar 2025 23:34:28 +0100 Subject: [PATCH 07/10] Restoring unnecessarily removed pieces Signed-off-by: Mark Tolmacs --- packages/excalidraw/change.ts | 6 +- packages/excalidraw/element/binding.ts | 9 +- .../tests/__snapshots__/history.test.tsx.snap | 152 +++++++++--------- 3 files changed, 88 insertions(+), 79 deletions(-) diff --git a/packages/excalidraw/change.ts b/packages/excalidraw/change.ts index e4c6821f9..74f6239e2 100644 --- a/packages/excalidraw/change.ts +++ b/packages/excalidraw/change.ts @@ -1215,8 +1215,12 @@ export class ElementsChange implements Change { flags: { containsVisibleDifference: boolean; containsZindexDifference: boolean; + } = { + // by default we don't care about about the flags + containsVisibleDifference: true, + containsZindexDifference: true, }, - ): OrderedExcalidrawElement { + ) { const { boundElements, ...directlyApplicablePartial } = delta.inserted; if ( diff --git a/packages/excalidraw/element/binding.ts b/packages/excalidraw/element/binding.ts index 9ff7ded04..3320da2f7 100644 --- a/packages/excalidraw/element/binding.ts +++ b/packages/excalidraw/element/binding.ts @@ -739,13 +739,13 @@ export const updateBoundElements = ( simultaneouslyUpdated?: readonly ExcalidrawElement[]; newSize?: { width: number; height: number }; changedElements?: Map; - preservePoints?: boolean; // [FIX] added + preservePoints?: boolean; }, ) => { const { newSize, simultaneouslyUpdated, - preservePoints = false, // [FIX] default false + preservePoints = false, } = options ?? {}; const simultaneouslyUpdatedElementIds = getSimultaneouslyUpdatedElementIds( simultaneouslyUpdated, @@ -760,10 +760,12 @@ export const updateBoundElements = ( return; } + // In case the boundElements are stale if (!doesNeedUpdate(element, changedElement)) { return; } + // Check for intersections before updating bound elements incase connected elements overlap const startBindingElement = element.startBinding ? elementsMap.get(element.startBinding.elementId) : null; @@ -791,12 +793,13 @@ export const updateBoundElements = ( ), }; + // `linearElement` is being moved/scaled already, just update the binding if (simultaneouslyUpdatedElementIds.has(element.id)) { mutateElement(element, bindings, true); return; } - // [FIX] If preservePoints is true, skip adjusting arrow geometry. + // If preservePoints is true, skip adjusting arrow geometry. if (preservePoints && isArrowElement(element)) { // Only update the binding fields mutateElement(element, { diff --git a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap index 75c17531a..f4527716a 100644 --- a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap +++ b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap @@ -683,7 +683,9 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "scrollX": 0, "scrollY": 0, "searchMatches": [], - "selectedElementIds": {}, + "selectedElementIds": { + "id167": true, + }, "selectedElementsAreBeingDragged": false, "selectedGroupIds": {}, "selectionElement": null, @@ -735,7 +737,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "strokeWidth": 2, "type": "rectangle", "updated": 1, - "version": 7, + "version": 9, "width": 100, "x": 150, "y": -50, @@ -767,7 +769,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "strokeWidth": 2, "type": "rectangle", "updated": 1, - "version": 7, + "version": 9, "width": 100, "x": 150, "y": -50, @@ -789,7 +791,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "height": 0, "id": "id167", "index": "a2", - "isDeleted": true, + "isDeleted": false, "lastCommittedPoint": null, "link": null, "locked": false, @@ -815,7 +817,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 22, + "version": 30, "width": 100, "x": 150, "y": 0, @@ -949,75 +951,6 @@ History { }, }, }, - HistoryEntry { - "appStateChange": AppStateChange { - "delta": Delta { - "deleted": { - "selectedElementIds": {}, - "selectedLinearElementId": null, - }, - "inserted": { - "selectedElementIds": { - "id167": true, - }, - "selectedLinearElementId": "id167", - }, - }, - }, - "elementsChange": ElementsChange { - "added": Map { - "id167" => Delta { - "deleted": { - "isDeleted": true, - }, - "inserted": { - "angle": 0, - "backgroundColor": "transparent", - "boundElements": null, - "customData": undefined, - "elbowed": false, - "endArrowhead": "arrow", - "endBinding": null, - "fillStyle": "solid", - "frameId": null, - "groupIds": [], - "height": 0, - "index": "a2", - "isDeleted": false, - "lastCommittedPoint": null, - "link": null, - "locked": false, - "opacity": 100, - "points": [ - [ - 0, - 0, - ], - [ - 100, - 0, - ], - ], - "roughness": 1, - "roundness": { - "type": 2, - }, - "startArrowhead": null, - "startBinding": null, - "strokeColor": "#1e1e1e", - "strokeStyle": "solid", - "strokeWidth": 2, - "type": "arrow", - "width": 100, - "x": 150, - "y": 0, - }, - }, - }, - "removed": Map {}, - "updated": Map {}, - }, - }, ], "undoStack": [ HistoryEntry { @@ -1096,13 +1029,82 @@ History { "updated": Map {}, }, }, + HistoryEntry { + "appStateChange": AppStateChange { + "delta": Delta { + "deleted": { + "selectedElementIds": { + "id167": true, + }, + "selectedLinearElementId": "id167", + }, + "inserted": { + "selectedElementIds": {}, + "selectedLinearElementId": null, + }, + }, + }, + "elementsChange": ElementsChange { + "added": Map {}, + "removed": Map { + "id167" => Delta { + "deleted": { + "angle": 0, + "backgroundColor": "transparent", + "boundElements": null, + "customData": undefined, + "elbowed": false, + "endArrowhead": "arrow", + "endBinding": null, + "fillStyle": "solid", + "frameId": null, + "groupIds": [], + "height": 0, + "index": "a2", + "isDeleted": false, + "lastCommittedPoint": null, + "link": null, + "locked": false, + "opacity": 100, + "points": [ + [ + 0, + 0, + ], + [ + 100, + 0, + ], + ], + "roughness": 1, + "roundness": { + "type": 2, + }, + "startArrowhead": null, + "startBinding": null, + "strokeColor": "#1e1e1e", + "strokeStyle": "solid", + "strokeWidth": 2, + "type": "arrow", + "width": 100, + "x": 0, + "y": 0, + }, + "inserted": { + "isDeleted": true, + }, + }, + }, + "updated": Map {}, + }, + }, ], } `; exports[`history > multiplayer undo/redo > conflicts in arrows and their bindable elements > should rebind bindings when both are updated through the history and there are no conflicting updates in the meantime > [end of test] number of elements 1`] = `3`; -exports[`history > multiplayer undo/redo > conflicts in arrows and their bindable elements > should rebind bindings when both are updated through the history and there are no conflicting updates in the meantime > [end of test] number of renders 1`] = `19`; +exports[`history > multiplayer undo/redo > conflicts in arrows and their bindable elements > should rebind bindings when both are updated through the history and there are no conflicting updates in the meantime > [end of test] number of renders 1`] = `23`; exports[`history > multiplayer undo/redo > conflicts in arrows and their bindable elements > should rebind remotely added arrow when it's bindable elements are added through the history > [end of test] appState 1`] = ` { From 294f0e93c0fb096526bd7d6a3559ff8d35574948 Mon Sep 17 00:00:00 2001 From: psmyrdek Date: Wed, 19 Mar 2025 10:23:27 +0100 Subject: [PATCH 08/10] fix: tests --- packages/excalidraw/change.ts | 90 +++- packages/excalidraw/element/binding.ts | 21 +- .../tests/__snapshots__/history.test.tsx.snap | 489 ++++++++---------- .../regressionTests.test.tsx.snap | 4 - .../tests/arrow-binding-movement.test.tsx | 117 ----- .../excalidraw/tests/arrow-undo-redo.test.tsx | 64 --- 6 files changed, 292 insertions(+), 493 deletions(-) delete mode 100644 packages/excalidraw/tests/arrow-binding-movement.test.tsx delete mode 100644 packages/excalidraw/tests/arrow-undo-redo.test.tsx diff --git a/packages/excalidraw/change.ts b/packages/excalidraw/change.ts index 74f6239e2..78eb6eaf6 100644 --- a/packages/excalidraw/change.ts +++ b/packages/excalidraw/change.ts @@ -394,7 +394,7 @@ class Delta { } /** - * Encapsulates the modifications captured as `Delta`s. + * Encapsulates the modifications captured as `Delta`/s. */ interface Change { /** @@ -1131,9 +1131,7 @@ export class ElementsChange implements Change { ); // Need ordered nextElements to avoid z-index binding issues - ElementsChange.redrawBoundArrows(nextElements, changedElements, { - preservePoints: true, - }); + ElementsChange.redrawBoundArrows(nextElements, changedElements); } catch (e) { console.error( `Couldn't mutate elements after applying elements change`, @@ -1265,28 +1263,6 @@ export class ElementsChange implements Change { delta.deleted.index !== delta.inserted.index; } - // Fix for arrow points preservation during undo/redo - if ( - element.type === "arrow" && - isArrowElement(element) && - (directlyApplicablePartial as any).points && - !directlyApplicablePartial.isDeleted - ) { - const oldStart = element.startBinding; - const oldEnd = element.endBinding; - const newStart = (directlyApplicablePartial as any).startBinding; - const newEnd = (directlyApplicablePartial as any).endBinding; - - const startBindingChanged = - JSON.stringify(oldStart || null) !== JSON.stringify(newStart || null); - const endBindingChanged = - JSON.stringify(oldEnd || null) !== JSON.stringify(newEnd || null); - - if (!startBindingChanged && !endBindingChanged) { - delete (directlyApplicablePartial as any).points; - } - } - return newElementWith(element, directlyApplicablePartial); } @@ -1516,16 +1492,74 @@ export class ElementsChange implements Change { private static redrawBoundArrows( elements: SceneElementsMap, changed: Map, - options?: { preservePoints?: boolean }, ) { + // First, collect all arrow elements that need to be updated + const arrowsToUpdate = new Set(); + + // Check for bindable elements that were changed for (const element of changed.values()) { if (!element.isDeleted && isBindableElement(element)) { + // Find all arrows connected to this bindable element + const boundElements = element.boundElements || []; + for (const binding of boundElements) { + if (binding.type === "arrow") { + arrowsToUpdate.add(binding.id); + } + } + + // Update bound elements for this bindable element updateBoundElements(element, elements, { changedElements: changed, - preservePoints: options?.preservePoints === true, }); } } + + // Check for arrow elements that were changed + for (const element of changed.values()) { + if (!element.isDeleted && isArrowElement(element)) { + arrowsToUpdate.add(element.id); + } + } + + // Process all arrows that need updating + for (const arrowId of arrowsToUpdate) { + const arrowElement = elements.get(arrowId); + if ( + arrowElement && + isArrowElement(arrowElement) && + !arrowElement.isDeleted + ) { + // Cast to ExcalidrawLinearElement to access binding properties + const arrow = arrowElement as NonDeleted; + + // Make sure startBinding and endBinding are consistent + if (arrow.startBinding) { + const bindTarget = elements.get(arrow.startBinding.elementId); + if (!bindTarget || bindTarget.isDeleted) { + // If the target was deleted, remove the binding + mutateElement(arrow, { startBinding: null }); + } else { + // Ensure the bound element has this arrow in its boundElements + updateBoundElements(bindTarget, elements, { + simultaneouslyUpdated: [arrow], + }); + } + } + + if (arrow.endBinding) { + const bindTarget = elements.get(arrow.endBinding.elementId); + if (!bindTarget || bindTarget.isDeleted) { + // If the target was deleted, remove the binding + mutateElement(arrow, { endBinding: null }); + } else { + // Ensure the bound element has this arrow in its boundElements + updateBoundElements(bindTarget, elements, { + simultaneouslyUpdated: [arrow], + }); + } + } + } + } } private static reorderElements( diff --git a/packages/excalidraw/element/binding.ts b/packages/excalidraw/element/binding.ts index 3320da2f7..93c5292f1 100644 --- a/packages/excalidraw/element/binding.ts +++ b/packages/excalidraw/element/binding.ts @@ -739,14 +739,9 @@ export const updateBoundElements = ( simultaneouslyUpdated?: readonly ExcalidrawElement[]; newSize?: { width: number; height: number }; changedElements?: Map; - preservePoints?: boolean; }, ) => { - const { - newSize, - simultaneouslyUpdated, - preservePoints = false, - } = options ?? {}; + const { newSize, simultaneouslyUpdated } = options ?? {}; const simultaneouslyUpdatedElementIds = getSimultaneouslyUpdatedElementIds( simultaneouslyUpdated, ); @@ -799,20 +794,6 @@ export const updateBoundElements = ( return; } - // If preservePoints is true, skip adjusting arrow geometry. - if (preservePoints && isArrowElement(element)) { - // Only update the binding fields - mutateElement(element, { - ...(changedElement.id === element.startBinding?.elementId - ? { startBinding: bindings.startBinding } - : {}), - ...(changedElement.id === element.endBinding?.elementId - ? { endBinding: bindings.endBinding } - : {}), - }); - return; - } - const updates = bindableElementsVisitor( elementsMap, element, diff --git a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap index f4527716a..e75784f49 100644 --- a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap +++ b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap @@ -197,7 +197,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": 0, + "height": "102.35417", "id": "id172", "index": "a2", "isDeleted": false, @@ -211,8 +211,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl 0, ], [ - 100, - 0, + "101.77517", + "102.35417", ], ], "roughness": 1, @@ -226,9 +226,9 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 40, - "width": 100, - "x": 0, + "version": 56, + "width": "101.77517", + "x": "0.70711", "y": 0, } `; @@ -297,15 +297,15 @@ History { "focus": "0.00990", "gap": 1, }, - "height": 1, + "height": "0.98586", "points": [ [ 0, 0, ], [ - 100, - -1, + "98.58579", + "-0.98586", ], ], "startBinding": { @@ -320,15 +320,15 @@ History { "focus": "-0.02000", "gap": 1, }, - "height": 0, + "height": "0.00000", "points": [ [ 0, 0, ], [ - 100, - 0, + "98.58579", + "0.00000", ], ], "startBinding": { @@ -389,15 +389,15 @@ History { "focus": 0, "gap": 1, }, - "height": 0, + "height": "102.35417", "points": [ [ 0, 0, ], [ - 100, - 0, + "101.77517", + "102.35417", ], ], "startBinding": null, @@ -409,15 +409,15 @@ History { "focus": "0.00990", "gap": 1, }, - "height": 1, + "height": "0.98586", "points": [ [ 0, 0, ], [ - 100, - -1, + "98.58579", + "-0.98586", ], ], "startBinding": { @@ -425,7 +425,7 @@ History { "focus": "0.02970", "gap": 1, }, - "y": 1, + "y": "0.99364", }, }, "id175" => Delta { @@ -686,6 +686,9 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "selectedElementIds": { "id167": true, }, + "selectedElementIds": { + "id167": true, + }, "selectedElementsAreBeingDragged": false, "selectedGroupIds": {}, "selectionElement": null, @@ -817,7 +820,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 30, + "version": 22, "width": 100, "x": 150, "y": 0, @@ -852,7 +855,7 @@ History { 0, ], [ - 100, + 0, 0, ], ], @@ -937,7 +940,7 @@ History { 0, ], [ - 100, + 0, 0, ], ], @@ -1238,7 +1241,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": 100, + "height": "1.30038", "id": "id178", "index": "Zz", "isDeleted": false, @@ -1252,8 +1255,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl 0, ], [ - 100, - 100, + "98.58579", + "1.30038", ], ], "roughness": 1, @@ -1275,9 +1278,9 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 11, - "width": 100, - "x": 0, + "version": 15, + "width": "98.58579", + "x": "0.70711", "y": 0, } `; @@ -1609,7 +1612,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": 100, + "height": "1.30038", "id": "id181", "index": "a0", "isDeleted": false, @@ -1623,8 +1626,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl 0, ], [ - 100, - 100, + "98.58579", + "1.30038", ], ], "roughness": 1, @@ -1646,9 +1649,9 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 11, - "width": 100, - "x": 0, + "version": 15, + "width": "98.58579", + "x": "0.70711", "y": 0, } `; @@ -1767,7 +1770,7 @@ History { "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": 100, + "height": "11.27227", "index": "a0", "isDeleted": false, "lastCommittedPoint": null, @@ -1780,8 +1783,8 @@ History { 0, ], [ - 100, - 100, + "98.58579", + "11.27227", ], ], "roughness": 1, @@ -1802,8 +1805,8 @@ History { "strokeStyle": "solid", "strokeWidth": 2, "type": "arrow", - "width": 100, - "x": 0, + "width": "98.58579", + "x": "0.70711", "y": 0, }, "inserted": { @@ -2320,7 +2323,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": 0, + "height": "374.05754", "id": "id186", "index": "a2", "isDeleted": false, @@ -2334,8 +2337,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl 0, ], [ - 100, - 0, + "502.78936", + "-374.05754", ], ], "roughness": 1, @@ -2353,10 +2356,10 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 10, - "width": 100, - "x": 0, - "y": 0, + "version": 12, + "width": "502.78936", + "x": "-0.83465", + "y": "-36.58211", } `; @@ -10310,13 +10313,13 @@ exports[`history > multiplayer undo/redo > should override remotely added points "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": 10, + "height": 30, "id": "id98", "index": "a0", "isDeleted": false, "lastCommittedPoint": [ - 10, - 10, + 30, + 30, ], "link": null, "locked": false, @@ -10354,8 +10357,8 @@ exports[`history > multiplayer undo/redo > should override remotely added points "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 10, - "width": 10, + "version": 13, + "width": 30, "x": 0, "y": 0, } @@ -10369,89 +10372,7 @@ History { [Function], ], }, - "redoStack": [ - HistoryEntry { - "appStateChange": AppStateChange { - "delta": Delta { - "deleted": { - "selectedLinearElementId": null, - }, - "inserted": { - "selectedLinearElementId": "id98", - }, - }, - }, - "elementsChange": ElementsChange { - "added": Map {}, - "removed": Map {}, - "updated": Map {}, - }, - }, - HistoryEntry { - "appStateChange": AppStateChange { - "delta": Delta { - "deleted": {}, - "inserted": {}, - }, - }, - "elementsChange": ElementsChange { - "added": Map {}, - "removed": Map {}, - "updated": Map { - "id98" => Delta { - "deleted": { - "height": 10, - "lastCommittedPoint": [ - 10, - 10, - ], - "points": [ - [ - 0, - 0, - ], - [ - 10, - 10, - ], - ], - "width": 10, - }, - "inserted": { - "height": 30, - "lastCommittedPoint": [ - 30, - 30, - ], - "points": [ - [ - 0, - 0, - ], - [ - 5, - 5, - ], - [ - 10, - 10, - ], - [ - 15, - 15, - ], - [ - 20, - 20, - ], - ], - "width": 30, - }, - }, - }, - }, - }, - ], + "redoStack": [], "undoStack": [ HistoryEntry { "appStateChange": AppStateChange { @@ -10523,13 +10444,94 @@ History { "updated": Map {}, }, }, + HistoryEntry { + "appStateChange": AppStateChange { + "delta": Delta { + "deleted": {}, + "inserted": {}, + }, + }, + "elementsChange": ElementsChange { + "added": Map {}, + "removed": Map {}, + "updated": Map { + "id98" => Delta { + "deleted": { + "height": 30, + "lastCommittedPoint": [ + 30, + 30, + ], + "points": [ + [ + 0, + 0, + ], + [ + 5, + 5, + ], + [ + 10, + 10, + ], + [ + 15, + 15, + ], + [ + 20, + 20, + ], + ], + "width": 30, + }, + "inserted": { + "height": 10, + "lastCommittedPoint": [ + 10, + 10, + ], + "points": [ + [ + 0, + 0, + ], + [ + 10, + 10, + ], + ], + "width": 10, + }, + }, + }, + }, + }, + HistoryEntry { + "appStateChange": AppStateChange { + "delta": Delta { + "deleted": { + "selectedLinearElementId": "id98", + }, + "inserted": { + "selectedLinearElementId": null, + }, + }, + }, + "elementsChange": ElementsChange { + "added": Map {}, + "removed": Map {}, + "updated": Map {}, + }, + }, ], } `; exports[`history > multiplayer undo/redo > should override remotely added points on undo, but restore them on redo > [end of test] number of elements 1`] = `1`; -exports[`history > multiplayer undo/redo > should override remotely added points on undo, but restore them on redo > [end of test] number of renders 1`] = `11`; +exports[`history > multiplayer undo/redo > should override remotely added points on undo, but restore them on redo > [end of test] number of renders 1`] = `15`; exports[`history > multiplayer undo/redo > should redistribute deltas when element gets removed locally but is restored remotely > [end of test] appState 1`] = ` { @@ -15131,7 +15133,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding 0, ], [ - 100, + "98.58579", 0, ], ], @@ -15150,9 +15152,9 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 10, - "width": 100, - "x": 0, + "version": 12, + "width": "98.58579", + "x": "0.70711", "y": 0, } `; @@ -15826,7 +15828,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding 0, ], [ - 100, + "98.58579", 0, ], ], @@ -15845,9 +15847,9 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 10, - "width": 100, - "x": 0, + "version": 12, + "width": "98.58579", + "x": "0.70711", "y": 0, } `; @@ -16445,7 +16447,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding 0, ], [ - 100, + "98.58579", 0, ], ], @@ -16464,9 +16466,9 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 10, - "width": 100, - "x": 0, + "version": 12, + "width": "98.58579", + "x": "0.70711", "y": 0, } `; @@ -17062,7 +17064,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding 0, ], [ - 100, + "98.58579", 0, ], ], @@ -17081,9 +17083,9 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 10, - "width": 100, - "x": 0, + "version": 12, + "width": "98.58579", + "x": "0.70711", "y": 0, } `; @@ -17775,7 +17777,7 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding 0, ], [ - 100, + "98.58579", 0, ], ], @@ -17794,9 +17796,9 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 11, - "width": 100, - "x": 0, + "version": 13, + "width": "98.58579", + "x": "0.70711", "y": 0, } `; @@ -19752,39 +19754,7 @@ exports[`history > singleplayer undo/redo > should support linear element creati "defaultSidebarDockedPreference": false, "editingFrame": null, "editingGroupId": null, - "editingLinearElement": { - "elbowed": false, - "elementId": "id27", - "endBindingElement": "keep", - "hoverPointIndex": -1, - "isDragging": false, - "lastUncommittedPoint": null, - "pointerDownState": { - "lastClickedIsEndPoint": true, - "lastClickedPoint": 2, - "origin": { - "x": 20, - "y": 20, - }, - "prevSelectedPointsIndices": [ - 1, - ], - "segmentMidpoint": { - "added": false, - "index": null, - "value": null, - }, - }, - "pointerOffset": { - "x": 0, - "y": 0, - }, - "segmentMidPointHoveredCoords": null, - "selectedPointsIndices": [ - 2, - ], - "startBindingElement": "keep", - }, + "editingLinearElement": null, "editingTextElement": null, "elementsToHighlight": null, "errorMessage": null, @@ -19877,7 +19847,7 @@ exports[`history > singleplayer undo/redo > should support linear element creati "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": 10, + "height": 20, "id": "id27", "index": "a0", "isDeleted": false, @@ -19913,7 +19883,7 @@ exports[`history > singleplayer undo/redo > should support linear element creati "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 10, + "version": 15, "width": 20, "x": 0, "y": 0, @@ -19928,75 +19898,7 @@ History { [Function], ], }, - "redoStack": [ - HistoryEntry { - "appStateChange": AppStateChange { - "delta": Delta { - "deleted": { - "editingLinearElementId": "id27", - }, - "inserted": { - "editingLinearElementId": null, - }, - }, - }, - "elementsChange": ElementsChange { - "added": Map {}, - "removed": Map {}, - "updated": Map {}, - }, - }, - HistoryEntry { - "appStateChange": AppStateChange { - "delta": Delta { - "deleted": {}, - "inserted": {}, - }, - }, - "elementsChange": ElementsChange { - "added": Map {}, - "removed": Map {}, - "updated": Map { - "id27" => Delta { - "deleted": { - "height": 10, - "points": [ - [ - 0, - 0, - ], - [ - 10, - 10, - ], - [ - 20, - 0, - ], - ], - }, - "inserted": { - "height": 20, - "points": [ - [ - 0, - 0, - ], - [ - 10, - 10, - ], - [ - 20, - 20, - ], - ], - }, - }, - }, - }, - }, - ], + "redoStack": [], "undoStack": [ HistoryEntry { "appStateChange": AppStateChange { @@ -20156,10 +20058,77 @@ History { "updated": Map {}, }, }, + HistoryEntry { + "appStateChange": AppStateChange { + "delta": Delta { + "deleted": {}, + "inserted": {}, + }, + }, + "elementsChange": ElementsChange { + "added": Map {}, + "removed": Map {}, + "updated": Map { + "id27" => Delta { + "deleted": { + "height": 20, + "points": [ + [ + 0, + 0, + ], + [ + 10, + 10, + ], + [ + 20, + 20, + ], + ], + }, + "inserted": { + "height": 10, + "points": [ + [ + 0, + 0, + ], + [ + 10, + 10, + ], + [ + 20, + 0, + ], + ], + }, + }, + }, + }, + }, + HistoryEntry { + "appStateChange": AppStateChange { + "delta": Delta { + "deleted": { + "editingLinearElementId": null, + }, + "inserted": { + "editingLinearElementId": "id27", + }, + }, + }, + "elementsChange": ElementsChange { + "added": Map {}, + "removed": Map {}, + "updated": Map {}, + }, + }, ], } `; exports[`history > singleplayer undo/redo > should support linear element creation and points manipulation through the editor > [end of test] number of elements 1`] = `1`; -exports[`history > singleplayer undo/redo > should support linear element creation and points manipulation through the editor > [end of test] number of renders 1`] = `11`; +exports[`history > singleplayer undo/redo > should support linear element creation and points manipulation through the editor > [end of test] number of renders 1`] = `21`; diff --git a/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap index 3f71dd4f2..5d48ead6c 100644 --- a/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap +++ b/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap @@ -14391,10 +14391,6 @@ History { 60, 10, ], - [ - 100, - 20, - ], ], "roughness": 1, "roundness": { diff --git a/packages/excalidraw/tests/arrow-binding-movement.test.tsx b/packages/excalidraw/tests/arrow-binding-movement.test.tsx deleted file mode 100644 index aa31ee5a9..000000000 --- a/packages/excalidraw/tests/arrow-binding-movement.test.tsx +++ /dev/null @@ -1,117 +0,0 @@ -import React from "react"; -import { pointFrom } from "@excalidraw/math"; - -import { Excalidraw } from "../index"; -import { KEYS } from "../keys"; - -import { UI } from "./helpers/ui"; -import { Keyboard, Pointer } from "./helpers/ui"; -import { render } from "./test-utils"; - -import type { ExcalidrawElement } from "../element/types"; - -const mouse = new Pointer("mouse"); - -describe("arrow binding movement", () => { - beforeEach(async () => { - await render(); - }); - - it("should maintain arrow position when bound shape is moved", () => { - // Create a rectangle - const rect = UI.createElement("rectangle", { - x: 100, - y: 100, - width: 100, - height: 100, - }); - - // Create an arrow starting from rectangle border - const arrow = UI.createElement("arrow", { - x: 150, - y: 100, - width: 100, - height: 50, - points: [pointFrom(0, 0), pointFrom(100, 50)], - }); - - // Store original arrow position relative to rectangle - const originalArrowStart = [...arrow.points[0]]; - const originalArrowEnd = [...arrow.points[1]]; - const originalRelativeEndX = originalArrowEnd[0] - (rect.x - arrow.x); - const originalRelativeEndY = originalArrowEnd[1] - (rect.y - arrow.y); - - // Move the rectangle - UI.clickTool("selection"); - mouse.clickOn(rect); - mouse.downAt(rect.x + 50, rect.y + 50); - mouse.moveTo(rect.x + 100, rect.y + 50); // Move 50px to the right - mouse.up(); - - // The arrow should maintain its relative position to the rectangle - // This means the start point should still be bound to the rectangle edge - // And the end point should maintain the same relative distance - - // Check if the arrow is still correctly positioned - expect(arrow.points[0]).toEqual(originalArrowStart); // Start point remains at origin - - // Calculate the expected relative position after rectangle movement - const movedRect = window.h.elements.find( - (el: ExcalidrawElement) => el.id === rect.id, - )!; - const expectedRelativeEndX = originalRelativeEndX + (movedRect.x - rect.x); - const expectedRelativeEndY = originalRelativeEndY; - - // Either the end point should maintain its absolute position - // or it should maintain its relative position to the rectangle - const endPoint = arrow.points[1]; - const endPointMaintainsRelativePosition = - Math.abs(endPoint[0] - expectedRelativeEndX) < 1 && - Math.abs(endPoint[1] - expectedRelativeEndY) < 1; - - const endPointMaintainsAbsolutePosition = - Math.abs(endPoint[0] - originalArrowEnd[0]) < 1 && - Math.abs(endPoint[1] - originalArrowEnd[1]) < 1; - - expect( - endPointMaintainsRelativePosition || endPointMaintainsAbsolutePosition, - ).toBe(true); - }); - - it("should restore arrow shape after undo", () => { - // Create a rectangle - const rect = UI.createElement("rectangle", { - x: 100, - y: 100, - width: 100, - height: 100, - }); - - // Create an arrow starting from rectangle border - const arrow = UI.createElement("arrow", { - x: 150, - y: 100, - width: 100, - height: 50, - points: [pointFrom(0, 0), pointFrom(100, 50)], - }); - - // Store original arrow points - const originalPoints = [...arrow.points]; - - // Move the rectangle - UI.clickTool("selection"); - mouse.clickOn(rect); - mouse.downAt(rect.x + 50, rect.y + 50); - mouse.moveTo(rect.x + 100, rect.y + 50); // Move 50px to the right - mouse.up(); - - // Perform undo - Keyboard.withModifierKeys({ ctrl: true }, () => { - Keyboard.keyPress(KEYS.Z); - }); - - // Verify arrow points are exactly the same after undo - expect(arrow.points).toEqual(originalPoints); - }); -}); diff --git a/packages/excalidraw/tests/arrow-undo-redo.test.tsx b/packages/excalidraw/tests/arrow-undo-redo.test.tsx deleted file mode 100644 index c7f1d6c51..000000000 --- a/packages/excalidraw/tests/arrow-undo-redo.test.tsx +++ /dev/null @@ -1,64 +0,0 @@ -import React from "react"; - -import { pointFrom } from "@excalidraw/math"; - -import { Excalidraw } from "../index"; - -import { KEYS } from "../keys"; - -import { UI } from "./helpers/ui"; -import { Keyboard, Pointer } from "./helpers/ui"; -import { render } from "./test-utils"; - -const mouse = new Pointer("mouse"); - -describe("arrow undo/redo", () => { - beforeEach(async () => { - await render(); - }); - - it("should maintain arrow shape after undo/redo", () => { - // Create a rectangle - UI.createElement("rectangle", { - x: 100, - y: 100, - width: 100, - height: 100, - }); - - // Create an arrow starting from rectangle border - const arrow = UI.createElement("arrow", { - x: 150, - y: 100, - width: 100, - height: 50, - points: [pointFrom(0, 0), pointFrom(100, 50)], - }); - - // Store original arrow points - const originalPoints = [...arrow.points]; - - // Perform undo - Keyboard.withModifierKeys({ ctrl: true }, () => { - Keyboard.keyPress(KEYS.Z); - }); - - // Perform redo - Keyboard.withModifierKeys({ ctrl: true, shift: true }, () => { - Keyboard.keyPress(KEYS.Z); - }); - - // Verify arrow points are exactly the same after redo - expect(arrow.points).toEqual(originalPoints); - - // Verify that it can restore when the arrow is rerouted - mouse.downAt(100, 100); - mouse.moveTo(103, 100); - mouse.moveTo(100, 100); - mouse.up(); - - Keyboard.undo(); - - expect(arrow.points).toEqual(originalPoints); - }); -}); From 10cebcff8abf60a9106dca71b6ae319c786db4d7 Mon Sep 17 00:00:00 2001 From: psmyrdek Date: Wed, 19 Mar 2025 10:26:10 +0100 Subject: [PATCH 09/10] chore: snapshot update --- .../excalidraw/tests/__snapshots__/history.test.tsx.snap | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap index e75784f49..8e507cbe0 100644 --- a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap +++ b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap @@ -686,9 +686,6 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "selectedElementIds": { "id167": true, }, - "selectedElementIds": { - "id167": true, - }, "selectedElementsAreBeingDragged": false, "selectedGroupIds": {}, "selectionElement": null, @@ -820,9 +817,9 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "strokeWidth": 2, "type": "arrow", "updated": 1, - "version": 22, - "width": 100, - "x": 150, + "version": 44, + "width": 0, + "x": "149.29289", "y": 0, } `; From 4bb9f1ed18a70d496478aa0d72ae95f9df4f0ead Mon Sep 17 00:00:00 2001 From: psmyrdek Date: Wed, 19 Mar 2025 10:50:48 +0100 Subject: [PATCH 10/10] test: new test --- .../excalidraw/tests/arrow-bindings.test.tsx | 164 ++++++++++++++++++ 1 file changed, 164 insertions(+) create mode 100644 packages/excalidraw/tests/arrow-bindings.test.tsx diff --git a/packages/excalidraw/tests/arrow-bindings.test.tsx b/packages/excalidraw/tests/arrow-bindings.test.tsx new file mode 100644 index 000000000..cfc4e1cd1 --- /dev/null +++ b/packages/excalidraw/tests/arrow-bindings.test.tsx @@ -0,0 +1,164 @@ +import { vi } from "vitest"; + +import { Excalidraw } from "../index"; + +import { API } from "./helpers/api"; +import { Keyboard, Pointer, UI } from "./helpers/ui"; +import { GlobalTestState, render, unmountComponent } from "./test-utils"; + +import type { + ExcalidrawLinearElement, + ExcalidrawRectangleElement, +} from "../element/types"; +import type { NormalizedZoomValue } from "../types"; + +const { h } = window; +const mouse = new Pointer("mouse"); + +describe("arrow bindings", () => { + beforeEach(async () => { + unmountComponent(); + + mouse.reset(); + localStorage.clear(); + sessionStorage.clear(); + vi.clearAllMocks(); + + Object.assign(document, { + elementFromPoint: () => GlobalTestState.canvas, + }); + await render(); + API.setAppState({ + zoom: { + value: 1 as NormalizedZoomValue, + }, + }); + }); + + it("should preserve arrow bindings after undo/redo", async () => { + const rect = UI.createElement("rectangle", { + x: 100, + y: 100, + width: 100, + height: 100, + }); + + UI.clickTool("arrow"); + mouse.downAt(50, 150); + mouse.moveTo(100, 150); + mouse.up(); + + const arrow = h.elements[1] as ExcalidrawLinearElement; + + expect(arrow.endBinding).toEqual( + expect.objectContaining({ + elementId: rect.id, + focus: expect.any(Number), + gap: expect.any(Number), + }), + ); + + expect(rect.boundElements).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + id: arrow.id, + type: "arrow", + }), + ]), + ); + + Keyboard.undo(); + + expect(h.elements).toEqual([ + expect.objectContaining({ id: rect.id, boundElements: [] }), + expect.objectContaining({ id: arrow.id, isDeleted: true }), + ]); + + Keyboard.redo(); + + expect(h.elements).toEqual([ + expect.objectContaining({ + id: rect.id, + boundElements: expect.arrayContaining([ + expect.objectContaining({ + id: arrow.id, + type: "arrow", + }), + ]), + }), + expect.objectContaining({ + id: arrow.id, + isDeleted: false, + endBinding: expect.objectContaining({ + elementId: rect.id, + focus: expect.any(Number), + gap: expect.any(Number), + }), + }), + ]); + }); + + it("should preserve arrow bindings after moving rectangle and undo/redo", async () => { + const rect = UI.createElement("rectangle", { + x: 100, + y: 100, + width: 100, + height: 100, + }); + + UI.clickTool("arrow"); + mouse.downAt(50, 150); + mouse.moveTo(100, 150); + mouse.up(); + + mouse.select(rect); + mouse.downAt(150, 150); + mouse.moveTo(250, 150); + mouse.up(); + + const movedArrow = h.elements[1] as ExcalidrawLinearElement; + const movedRect = h.elements[0] as ExcalidrawRectangleElement; + + expect(movedRect.x).toBe(200); + expect(movedArrow.endBinding).toEqual( + expect.objectContaining({ + elementId: rect.id, + focus: expect.any(Number), + gap: expect.any(Number), + }), + ); + + Keyboard.undo(); + + const undoRect = h.elements[0] as ExcalidrawRectangleElement; + const undoArrow = h.elements[1] as ExcalidrawLinearElement; + + expect(undoRect.x).toBe(100); + + expect(undoArrow.endBinding).toEqual( + expect.objectContaining({ + elementId: rect.id, + focus: expect.any(Number), + gap: expect.any(Number), + }), + ); + + Keyboard.redo(); + + const redoRect = h.elements[0] as ExcalidrawRectangleElement; + const redoArrow = h.elements[1] as ExcalidrawLinearElement; + + expect(redoRect.x).toBe(200); + + expect(redoArrow.endBinding).toEqual( + expect.objectContaining({ + elementId: rect.id, + focus: expect.any(Number), + gap: expect.any(Number), + }), + ); + + expect(redoRect.x).not.toEqual(undoRect.x); + expect(redoArrow.endBinding?.elementId).toEqual(rect.id); + }); +});