diff --git a/packages/element/src/binding.ts b/packages/element/src/binding.ts index e21542ebb8..4f126b52b7 100644 --- a/packages/element/src/binding.ts +++ b/packages/element/src/binding.ts @@ -643,10 +643,13 @@ const getBindingStrategyForDraggingBindingElementEndpoints_simple = ( let start: BindingStrategy = { mode: undefined }; let end: BindingStrategy = { mode: undefined }; - invariant( - arrow.points.length > 1, - "Do not attempt to bind linear elements with a single point", - ); + if (arrow.points.length < 2) { + console.error( + "Attempting to bind a linear element with less than 2 points", + ); + // a single-point can't be bound -> cancel + return { start: { mode: undefined }, end: { mode: undefined } }; + } // If none of the ends are dragged, we don't change anything if (!startDragged && !endDragged) { @@ -890,10 +893,13 @@ const getBindingStrategyForDraggingBindingElementEndpoints_complex = ( let start: BindingStrategy = { mode: undefined }; let end: BindingStrategy = { mode: undefined }; - invariant( - arrow.points.length > 1, - "Do not attempt to bind linear elements with a single point", - ); + if (arrow.points.length < 2) { + console.error( + "Attempting to bind a linear element with less than 2 points", + ); + // a single-point can't be bound -> cancel + return { start: { mode: undefined }, end: { mode: undefined } }; + } // If none of the ends are dragged, we don't change anything if (!startDragged && !endDragged) { diff --git a/packages/element/tests/binding.test.tsx b/packages/element/tests/binding.test.tsx index 85a613bf87..6f4515113e 100644 --- a/packages/element/tests/binding.test.tsx +++ b/packages/element/tests/binding.test.tsx @@ -178,6 +178,64 @@ describe("binding for simple arrows", () => { }); }); + describe("self-binding (both ends to the same element) single-click finalize", () => { + // rect spans x:200..400, y:200..400; orbit ring is ~15px outside the outline + const INSIDE: [number, number] = [250, 250]; + const ORBIT_LEFT: [number, number] = [187, 300]; + const ORBIT_RIGHT: [number, number] = [413, 300]; + const MIDDLE: [number, number] = [550, 100]; + + beforeEach(async () => { + mouse.reset(); + await act(() => setLanguage(defaultLang)); + await render(); + UI.createElement("rectangle", { + x: 200, + y: 200, + width: 200, + height: 200, + }); + }); + + const drawSelfArrow = (start: [number, number], end: [number, number]) => { + UI.clickTool("arrow"); + mouse.reset(); + mouse.clickAt(...start); + mouse.moveTo(...MIDDLE); + mouse.clickAt(...MIDDLE); // commit a middle point so it's a multi-point arrow + mouse.moveTo(...end); + mouse.clickAt(...end); // single click at the end + }; + + it("orbit -> orbit finalizes on a single click", () => { + drawSelfArrow(ORBIT_LEFT, ORBIT_RIGHT); + + const arrow = h.elements[h.elements.length - 1] as ExcalidrawArrowElement; + expect(h.state.multiElement).toBe(null); + expect(h.state.activeTool.type).toBe("selection"); + expect(arrow.startBinding?.elementId).toBe(arrow.endBinding?.elementId); + expect(arrow.endBinding?.elementId).not.toBe(undefined); + }); + + it("inside -> orbit finalizes on a single click", () => { + drawSelfArrow(INSIDE, ORBIT_RIGHT); + + const arrow = h.elements[h.elements.length - 1] as ExcalidrawArrowElement; + expect(h.state.multiElement).toBe(null); + expect(h.state.activeTool.type).toBe("selection"); + expect(arrow.startBinding?.elementId).toBe(arrow.endBinding?.elementId); + expect(arrow.endBinding?.elementId).not.toBe(undefined); + }); + + it("inside -> inside keep in multi-point mode (no single-click finalize)", () => { + drawSelfArrow(INSIDE, [INSIDE[0] + 50, INSIDE[1] + 50]); // end dropped inside the rect + + // ambiguous → must be confirmed with a second click, so still in progress + expect(h.state.multiElement).not.toBe(null); + expect(h.state.activeTool.type).toBe("arrow"); + }); + }); + describe("when arrow is outside of shape", () => { beforeEach(async () => { mouse.reset(); @@ -403,6 +461,7 @@ describe("binding for simple arrows", () => { mouse.moveTo(340, 251); mouse.moveTo(410, 251); mouse.clickAt(410, 251); + mouse.clickAt(410, 251); const arrow = h.elements[h.elements.length - 1] as any; expect(arrow.startBinding?.elementId).toBe(rectLeft.id); @@ -447,6 +506,7 @@ describe("binding for simple arrows", () => { mouse.moveTo(350, 251); mouse.moveTo(410, 251); mouse.clickAt(410, 251); + mouse.clickAt(410, 251); const arrow = API.getSelectedElement() as ExcalidrawArrowElement; diff --git a/packages/excalidraw/actions/actionFinalize.tsx b/packages/excalidraw/actions/actionFinalize.tsx index f2e5db5613..c5dc9f3d72 100644 --- a/packages/excalidraw/actions/actionFinalize.tsx +++ b/packages/excalidraw/actions/actionFinalize.tsx @@ -54,6 +54,7 @@ export const actionFinalize = register({ label: "", trackEvent: false, perform: (elements, appState, data, app) => { + let shouldCommit = true; let newElements = elements; const { interactiveCanvas, focusContainer, scene } = app; const elementsMap = scene.getNonDeletedElementsMap(); @@ -222,9 +223,44 @@ export const actionFinalize = register({ !lastCommittedPoint || points[points.length - 1] !== lastCommittedPoint ) { + shouldCommit = false; scene.mutateElement(element, { points: element.points.slice(0, -1), }); + if ( + isBindingElement(element) && + element.endBinding && + // after slicing the trailing point a <2-point arrow may be left + element.points.length > 1 + ) { + const newArrow = !!appState.newElement; + const draggedPoints: PointsPositionUpdates = new Map([ + [ + element.points.length - 1, + { + point: element.points[element.points.length - 1], + isDragging: false, + }, + ], + ]); + const globalPoint = + LinearElementEditor.getPointAtIndexGlobalCoordinates( + element, + -1, + elementsMap, + ); + bindOrUnbindBindingElement( + element, + draggedPoints, + globalPoint[0], + globalPoint[1], + scene, + appState, + { + newArrow, + }, + ); + } } } @@ -344,7 +380,9 @@ export const actionFinalize = register({ selectedLinearElement, }, // TODO: #7348 we should not capture everything, but if we don't, it leads to incosistencies -> revisit - captureUpdate: CaptureUpdateAction.IMMEDIATELY, + captureUpdate: shouldCommit + ? CaptureUpdateAction.IMMEDIATELY + : CaptureUpdateAction.NEVER, }; }, keyTest: (event, appState) => diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index 899b98af19..2b04162491 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -260,6 +260,7 @@ import { getUncroppedWidthAndHeight, getActiveTextElement, isEligibleFrameChildType, + getBindingStrategyForDraggingBindingElementEndpoints, } from "@excalidraw/element"; import type { GlobalPoint, LocalPoint, Radians } from "@excalidraw/math"; @@ -7112,45 +7113,7 @@ class App extends React.Component { setCursorForShape(this.interactiveCanvas, this.state); if (lastPoint === lastCommittedPoint) { - const hoveredElement = - isArrowElement(this.state.newElement) && - isBindingEnabled(this.state) && - getHoveredElementForBinding( - pointFrom(scenePointerX, scenePointerY), - this.scene.getNonDeletedElements(), - this.scene.getNonDeletedElementsMap(), - maxBindingDistance_simple(this.state.zoom), - ); - if (hoveredElement) { - this.actionManager.executeAction(actionFinalize, "ui", { - event: event.nativeEvent, - sceneCoords: { - x: scenePointerX, - y: scenePointerY, - }, - }); - this.setState({ suggestedBinding: null }); - if (!this.state.activeTool.locked) { - resetCursor(this.interactiveCanvas); - this.setState((prevState) => ({ - newElement: null, - activeTool: updateActiveTool(this.state, { - type: this.state.preferredSelectionTool.type, - }), - selectedElementIds: makeNextSelectedElementIds( - { - ...prevState.selectedElementIds, - [multiElement.id]: true, - }, - prevState, - ), - selectedLinearElement: new LinearElementEditor( - multiElement, - this.scene.getNonDeletedElementsMap(), - ), - })); - } - } else if ( + if ( // if we haven't yet created a temp point and we're beyond commit-zone // threshold, add a point pointDistance( @@ -7158,6 +7121,24 @@ class App extends React.Component { lastPoint, ) >= LINE_CONFIRM_THRESHOLD ) { + this.store.scheduleCapture(); + flushSync(() => { + invariant( + this.state.selectedLinearElement?.initialState, + "initialState must be set", + ); + this.setState({ + selectedLinearElement: { + ...this.state.selectedLinearElement, + lastCommittedPoint: points[points.length - 1], + selectedPointsIndices: [multiElement.points.length], + initialState: { + ...this.state.selectedLinearElement.initialState, + lastClickedPoint: multiElement.points.length, + }, + }, + }); + }); this.scene.mutateElement( multiElement, { @@ -7168,21 +7149,6 @@ class App extends React.Component { }, { informMutation: false, isDragging: false }, ); - invariant( - this.state.selectedLinearElement?.initialState, - "initialState must be set", - ); - this.setState({ - selectedLinearElement: { - ...this.state.selectedLinearElement, - lastCommittedPoint: points[points.length - 1], - selectedPointsIndices: [multiElement.points.length - 1], - initialState: { - ...this.state.selectedLinearElement.initialState, - lastClickedPoint: multiElement.points.length - 1, - }, - }, - }); } else { setCursor(this.interactiveCanvas, CURSOR_TYPE.POINTER); // in this branch, we're inside the commit zone, and no uncommitted @@ -9264,32 +9230,58 @@ class App extends React.Component { const { x: rx, y: ry } = multiElement; const { lastCommittedPoint } = selectedLinearElement; + const sceneCoords = viewportCoordsToSceneCoords(event, this.state); + const { start, end } = + isBindingElement(multiElement) && isBindingEnabled(this.state) + ? getBindingStrategyForDraggingBindingElementEndpoints( + multiElement, + new Map([ + [ + multiElement.points.length - 1, + { + point: multiElement.points[multiElement.points.length - 1], + isDragging: false, + }, + ], + ]), + sceneCoords.x, + sceneCoords.y, + this.scene.getNonDeletedElementsMap(), + this.scene.getNonDeletedElements(), + this.state, + { + newArrow: Boolean(this.state.newElement), + zoom: this.state.zoom, + }, + ) + : { end: { mode: undefined } }; - const hoveredElementForBinding = - isBindingEnabled(this.state) && - getHoveredElementForBinding( - pointFrom( - this.lastPointerMoveCoords?.x ?? - rx + multiElement.points[multiElement.points.length - 1][0], - this.lastPointerMoveCoords?.y ?? - ry + multiElement.points[multiElement.points.length - 1][1], + const elementsMap = this.scene.getNonDeletedElementsMap(); + // Auto-confirm when both ends bind to the SAME element and the end point + // lands on the outline rather than inside it + const endOutsideSameElement = + start?.mode != null && + end.mode != null && + start.element.id === end.element.id && + !isPointInElement(end.focusPoint, end.element, elementsMap); + const boundOutsideFromElsewhere = + end.mode === "orbit" && + multiElement.startBinding?.elementId !== end.element?.id; + const lastCommittedPointIsInsideCommitZone = + lastCommittedPoint && + pointDistance( + pointFrom( + pointerDownState.origin.x - rx, + pointerDownState.origin.y - ry, ), - this.scene.getNonDeletedElements(), - this.scene.getNonDeletedElementsMap(), - ); + lastCommittedPoint, + ) < LINE_CONFIRM_THRESHOLD; // clicking inside commit zone → finalize arrow if ( - (isBindingElement(multiElement) && hoveredElementForBinding) || - (multiElement.points.length > 1 && - lastCommittedPoint && - pointDistance( - pointFrom( - pointerDownState.origin.x - rx, - pointerDownState.origin.y - ry, - ), - lastCommittedPoint, - ) < LINE_CONFIRM_THRESHOLD) + boundOutsideFromElsewhere || // Outside -> orbit: Bind immediately + endOutsideSameElement || // End outside the start's element: Bind immediately + (multiElement.points.length > 1 && lastCommittedPointIsInsideCommitZone) ) { this.actionManager.executeAction(actionFinalize, "ui", { event: event.nativeEvent, @@ -10864,13 +10856,6 @@ class App extends React.Component { } if (isLinearElement(newElement)) { - if ( - newElement!.points.length > 1 && - newElement.points[1][0] !== 0 && - newElement.points[1][1] !== 0 - ) { - this.store.scheduleCapture(); - } const pointerCoords = viewportCoordsToSceneCoords( childEvent, this.state, @@ -10908,23 +10893,15 @@ class App extends React.Component { this.actionManager.executeAction(actionFinalize); } else { - const dx = pointerCoords.x - newElement.x; - const dy = pointerCoords.y - newElement.y; - - this.scene.mutateElement( - newElement, - { - points: [newElement.points[0], pointFrom(dx, dy)], - }, - { informMutation: false, isDragging: false }, - ); - + // Movement out of commit area will create the point this.setState({ multiElement: newElement, newElement, }); } } else if (pointerDownState.drag.hasOccurred && !multiElement) { + this.store.scheduleCapture(); + if (isLinearElement(newElement)) { this.actionManager.executeAction(actionFinalize, "ui", { event: childEvent,