From 4c3516a5b99f9236f7342135f770119a056b2d69 Mon Sep 17 00:00:00 2001 From: psmyrdek Date: Sun, 16 Mar 2025 10:15:43 +0100 Subject: [PATCH] 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); + }); +});