From fbff9a34637fb5861d0be5471d74066ddb9e5a3e Mon Sep 17 00:00:00 2001 From: Mark Tolmacs Date: Wed, 25 Jun 2025 18:44:29 +0200 Subject: [PATCH] Fixed point binding for simple arrows when the arrow doesn't point to the element --- packages/element/src/binding.ts | 111 ++++++++++++++++-------- packages/element/tests/binding.test.tsx | 106 ++++++++++++++++++++-- packages/excalidraw/components/App.tsx | 1 - 3 files changed, 172 insertions(+), 46 deletions(-) diff --git a/packages/element/src/binding.ts b/packages/element/src/binding.ts index afc1fdf68..bca0e1628 100644 --- a/packages/element/src/binding.ts +++ b/packages/element/src/binding.ts @@ -404,7 +404,6 @@ export const maybeSuggestBindingsForLinearElementAtCoords = ( Array.from( pointerCoords.reduce( (acc: Set>, coords) => { - const p = pointFrom(coords.x, coords.y); const hoveredBindableElement = getHoveredElementForBinding( coords, scene.getNonDeletedElements(), @@ -413,19 +412,10 @@ export const maybeSuggestBindingsForLinearElementAtCoords = ( isElbowArrow(linearElement), isElbowArrow(linearElement), ); - const pointIsInside = - hoveredBindableElement != null && - hitElementItself({ - point: p, - element: hoveredBindableElement, - elementsMap, - threshold: 0, // TODO: Not ideal, should be calculated from the same source - }); if ( hoveredBindableElement != null && - ((pointIsInside && - oppositeBindingBoundElement?.id === hoveredBindableElement.id) || + (oppositeBindingBoundElement?.id === hoveredBindableElement.id || !isLinearElementSimpleAndAlreadyBound( linearElement, oppositeBindingBoundElement?.id, @@ -504,6 +494,11 @@ export const bindLinearElement = ( } const elementsMap = scene.getNonDeletedElementsMap(); + const edgePoint = LinearElementEditor.getPointAtIndexGlobalCoordinates( + linearElement, + startOrEnd === "start" ? 0 : -1, + elementsMap, + ); let binding: PointBinding | FixedPointBinding; if (isElbowArrow(linearElement)) { @@ -525,36 +520,67 @@ export const bindLinearElement = ( elementsMap, ), }; + } else if ( + hitElementItself({ + point: edgePoint, + element: hoveredElement, + elementsMap, + threshold: 0, // TODO: Not ideal, should be calculated from the same source + }) + ) { + // Use FixedPoint binding when the arrow endpoint is inside the shape + binding = { + elementId: hoveredElement.id, + focus: 0, + gap: 0, + ...calculateFixedPointForNonElbowArrowBinding( + linearElement, + hoveredElement, + startOrEnd, + elementsMap, + ), + }; } else { - // For non-elbow arrows, check if the endpoint is inside the shape - const edgePoint = LinearElementEditor.getPointAtIndexGlobalCoordinates( + // For non-elbow arrows, extend the last segment and check intersection + const adjacentPoint = LinearElementEditor.getPointAtIndexGlobalCoordinates( linearElement, - startOrEnd === "start" ? 0 : -1, + startOrEnd === "start" ? 1 : -2, elementsMap, ); - - if ( - hitElementItself({ - point: edgePoint, - element: hoveredElement, - elementsMap, - threshold: 0, // TODO: Not ideal, should be calculated from the same source - }) - ) { - // Use FixedPoint binding when the arrow endpoint is inside the shape - binding = { - elementId: hoveredElement.id, - focus: 0, - gap: 0, - ...calculateFixedPointForNonElbowArrowBinding( - linearElement, - hoveredElement, - startOrEnd, - elementsMap, + const extendedDirection = vectorScale( + vectorNormalize( + vectorFromPoint( + pointFrom( + edgePoint[0] - adjacentPoint[0], + edgePoint[1] - adjacentPoint[1], + ), ), - }; - } else { - // Use traditional focus/gap binding when the endpoint is outside the shape + ), + Math.max(hoveredElement.width, hoveredElement.height) * 2, + ); + const intersector = lineSegment( + edgePoint, + pointFromVector( + vectorFromPoint( + pointFrom( + edgePoint[0] + extendedDirection[0], + edgePoint[1] + extendedDirection[1], + ), + ), + ), + ); + + // Check if this extended segment intersects the bindable element + const intersections = intersectElementWithLineSegment( + hoveredElement, + elementsMap, + intersector, + ); + + const intersectsElement = intersections.length > 0; + + if (intersectsElement) { + // Use traditional focus/gap binding when the extended segment intersects binding = { elementId: hoveredElement.id, ...normalizePointBinding( @@ -567,6 +593,19 @@ export const bindLinearElement = ( hoveredElement, ), }; + } else { + // Use FixedPoint binding when the extended segment doesn't intersect + binding = { + elementId: hoveredElement.id, + focus: 0, + gap: 0, + ...calculateFixedPointForNonElbowArrowBinding( + linearElement, + hoveredElement, + startOrEnd, + elementsMap, + ), + }; } } diff --git a/packages/element/tests/binding.test.tsx b/packages/element/tests/binding.test.tsx index f374b2be2..b30894c05 100644 --- a/packages/element/tests/binding.test.tsx +++ b/packages/element/tests/binding.test.tsx @@ -8,7 +8,13 @@ import { Excalidraw, isLinearElement } from "@excalidraw/excalidraw"; import { API } from "@excalidraw/excalidraw/tests/helpers/api"; import { UI, Pointer, Keyboard } from "@excalidraw/excalidraw/tests/helpers/ui"; -import { fireEvent, render } from "@excalidraw/excalidraw/tests/test-utils"; +import { + act, + fireEvent, + render, +} from "@excalidraw/excalidraw/tests/test-utils"; + +import { defaultLang, setLanguage } from "@excalidraw/excalidraw/i18n"; import { getTransformHandles } from "../src/transformHandles"; import { @@ -73,8 +79,9 @@ describe("element binding", () => { expect(arrow.startBinding).toEqual({ elementId: rect.id, - focus: expect.toBeNonNaNNumber(), - gap: expect.toBeNonNaNNumber(), + focus: 0, + gap: 0, + fixedPoint: expect.arrayContaining([1.1, 0]), }); // Move the end point to the overlapping binding position @@ -85,13 +92,15 @@ describe("element binding", () => { // Both the start and the end points should be bound expect(arrow.startBinding).toEqual({ elementId: rect.id, - focus: expect.toBeNonNaNNumber(), - gap: expect.toBeNonNaNNumber(), + focus: 0, + gap: 0, + fixedPoint: expect.arrayContaining([1.1, 0]), }); expect(arrow.endBinding).toEqual({ elementId: rect.id, - focus: expect.toBeNonNaNNumber(), - gap: expect.toBeNonNaNNumber(), + focus: 0, + gap: 0, + fixedPoint: expect.arrayContaining([1.1, 0]), }); }); @@ -190,9 +199,9 @@ describe("element binding", () => { // Test sticky connection expect(API.getSelectedElement().type).toBe("arrow"); Keyboard.keyPress(KEYS.ARROW_RIGHT); - expect(arrow.endBinding?.elementId).toBe(rectangle.id); + expect(arrow.endBinding?.elementId).not.toBe(rectangle.id); Keyboard.keyPress(KEYS.ARROW_LEFT); - expect(arrow.endBinding?.elementId).toBe(rectangle.id); + expect(arrow.endBinding?.elementId).not.toBe(rectangle.id); // Sever connection expect(API.getSelectedElement().type).toBe("arrow"); @@ -750,3 +759,82 @@ describe("Fixed-point arrow binding", () => { expect(arrow.points[2][1]).toBe(originalInnerPoint2[1]); }); }); + +describe("line segment extension binding", () => { + beforeEach(async () => { + mouse.reset(); + + await act(() => { + return setLanguage(defaultLang); + }); + await render(); + }); + + it("should use point binding when extended segment intersects element", () => { + // Create a rectangle that will be intersected by the extended arrow segment + const rect = API.createElement({ + type: "rectangle", + x: 100, + y: 100, + width: 100, + height: 100, + }); + + API.setElements([rect]); + + // Draw an arrow that points at the rectangle (extended segment will intersect) + UI.clickTool("arrow"); + mouse.downAt(0, 0); // Start point + mouse.moveTo(120, 95); // End point - arrow direction points toward rectangle + mouse.up(); + + const arrow = API.getSelectedElement() as ExcalidrawLinearElement; + + // Should create a normal point binding since the extended line segment + // from the last arrow segment intersects the rectangle + expect(arrow.endBinding?.elementId).toBe(rect.id); + expect(arrow.endBinding).toHaveProperty("focus"); + expect(arrow.endBinding).toHaveProperty("gap"); + expect(arrow.endBinding).not.toHaveProperty("fixedPoint"); + }); + + it("should use fixed point binding when extended segment misses element", () => { + // Create a rectangle positioned so the extended arrow segment will miss it + const rect = API.createElement({ + type: "rectangle", + x: 100, + y: 100, + width: 100, + height: 100, + }); + + API.setElements([rect]); + + // Draw an arrow that doesn't point at the rectangle (extended segment will miss) + UI.clickTool("arrow"); + mouse.reset(); + mouse.downAt(125, 93); // Start point + mouse.moveTo(175, 93); // End point - arrow direction is horizontal, misses rectangle + mouse.up(); + + const arrow = API.getSelectedElement() as ExcalidrawLinearElement; + + // Should create a fixed point binding since the extended line segment + // from the last arrow segment misses the rectangle + expect(arrow.startBinding?.elementId).toBe(rect.id); + expect(arrow.startBinding).toHaveProperty("fixedPoint"); + expect( + (arrow.startBinding as FixedPointBinding).fixedPoint[0], + ).toBeGreaterThanOrEqual(0); + expect( + (arrow.startBinding as FixedPointBinding).fixedPoint[0], + ).toBeLessThanOrEqual(1); + expect( + (arrow.startBinding as FixedPointBinding).fixedPoint[1], + ).toBeLessThanOrEqual(0); + expect( + (arrow.startBinding as FixedPointBinding).fixedPoint[1], + ).toBeLessThanOrEqual(1); + expect(arrow.endBinding).toBe(null); + }); +}); diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index b88d813fc..785f66dd9 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -5902,7 +5902,6 @@ class App extends React.Component { this.scene, this.state.zoom, this.scene.getNonDeletedElementsMap(), - //this.state.startBoundElement, ), }); } else {