From 5af4500bbcf9364e7b00b87cbc6622577e30257a Mon Sep 17 00:00:00 2001 From: Mark Tolmacs Date: Fri, 28 Mar 2025 20:41:26 +0100 Subject: [PATCH] Half done --- packages/common/src/utils.ts | 4 - packages/element/src/binding.ts | 184 +++++++++++++++--------- packages/element/tests/binding.test.tsx | 78 ++++++++++ packages/excalidraw/components/App.tsx | 26 ++-- packages/excalidraw/tests/helpers/ui.ts | 13 ++ 5 files changed, 222 insertions(+), 83 deletions(-) diff --git a/packages/common/src/utils.ts b/packages/common/src/utils.ts index 7e078f68d..02e94bfdf 100644 --- a/packages/common/src/utils.ts +++ b/packages/common/src/utils.ts @@ -7,7 +7,6 @@ import { } from "@excalidraw/math"; import type { - ExcalidrawBindableElement, ExcalidrawElement, FontFamilyValues, FontString, @@ -559,9 +558,6 @@ export const isTransparent = (color: string) => { ); }; -export const isBindingFallthroughEnabled = (el: ExcalidrawBindableElement) => - el.fillStyle !== "solid" || isTransparent(el.backgroundColor); - export type ResolvablePromise = Promise & { resolve: [T] extends [undefined] ? (value?: MaybePromise>) => void diff --git a/packages/element/src/binding.ts b/packages/element/src/binding.ts index 3084924ca..3866b46f0 100644 --- a/packages/element/src/binding.ts +++ b/packages/element/src/binding.ts @@ -1,7 +1,6 @@ import { KEYS, arrayToMap, - isBindingFallthroughEnabled, tupleToCoors, invariant, isDevEnv, @@ -427,7 +426,7 @@ export const getSuggestedBindingsForArrows = ( export const maybeBindLinearElement = ( linearElement: NonDeleted, appState: AppState, - pointerCoords: { x: number; y: number }, + startOrEnd: "start" | "end", elementsMap: NonDeletedSceneElementsMap, elements: readonly NonDeletedExcalidrawElement[], ): void => { @@ -441,7 +440,20 @@ export const maybeBindLinearElement = ( } const hoveredElement = getHoveredElementForBinding( - pointerCoords, + tupleToCoors( + LinearElementEditor.getPointAtIndexGlobalCoordinates( + linearElement, + startOrEnd ? 0 : -1, + elementsMap, + ), + ), + tupleToCoors( + LinearElementEditor.getPointAtIndexGlobalCoordinates( + linearElement, + startOrEnd ? -1 : 0, + elementsMap, + ), + ), elements, elementsMap, appState.zoom, @@ -568,6 +580,10 @@ export const getHoveredElementForBinding = ( x: number; y: number; }, + otherPointerCoords: { + x: number; + y: number; + }, elements: readonly NonDeletedExcalidrawElement[], elementsMap: NonDeletedSceneElementsMap, zoom?: AppState["zoom"], @@ -575,59 +591,89 @@ export const getHoveredElementForBinding = ( considerAllElements?: boolean, ): NonDeleted | null => { if (considerAllElements) { - let cullRest = false; - const candidateElements = getAllElementsAtPositionForBinding( - elements, - (element) => - isBindableElement(element, false) && - bindingBorderTest( - element, - pointerCoords, - elementsMap, - zoom, - (fullShape || - !isBindingFallthroughEnabled( - element as ExcalidrawBindableElement, - )) && - // disable fullshape snapping for frame elements so we - // can bind to frame children - !isFrameLikeElement(element), - ), - ).filter((element) => { - if (cullRest) { - return false; - } + const otherCandidateElement = + getAllElementsAtPositionForBinding( + elements, + (element) => + isBindableElement(element, false) && + bindingBorderTest( + element, + otherPointerCoords, + elementsMap, + zoom, + fullShape || + // disable fullshape snapping for frame elements so we + // can bind to frame children + !isFrameLikeElement(element), + ), + ) + .filter( + (element): element is NonDeleted => + element != null, + ) + // Prefer the shape with the border being tested (if any) + .filter( + (element, _, arr) => + arr.length <= 1 || + bindingBorderTest( + element as NonDeleted, + otherPointerCoords, + elementsMap, + zoom, + false, + ), + ) + // Prefer smaller bindables to be consisent with the check for the other + // point + .sort( + (a, b) => + b.width ** 2 + b.height ** 2 - (a.width ** 2 + a.height ** 2), + ) + .pop() ?? null; - if (!isBindingFallthroughEnabled(element as ExcalidrawBindableElement)) { - cullRest = true; - } + const candidateElement = + getAllElementsAtPositionForBinding( + elements, + (element) => + isBindableElement(element, false) && + bindingBorderTest( + element, + pointerCoords, + elementsMap, + zoom, + fullShape || + // disable fullshape snapping for frame elements so we + // can bind to frame children + !isFrameLikeElement(element), + ), + ) + .filter( + (element): element is NonDeleted => + element != null, + ) // Prefer the shape with the border being tested (if any) + .filter( + (element, _, arr) => + arr.length <= 1 || + bindingBorderTest( + element as NonDeleted, + pointerCoords, + elementsMap, + zoom, + false, + ), + ) + // Prefer smaller bindables + .sort( + (a, b) => + b.width ** 2 + b.height ** 2 - (a.width ** 2 + a.height ** 2), + ) + .pop() ?? null; - return true; - }) as NonDeleted[] | null; - - // Return early if there are no candidates or just one candidate - if (!candidateElements || candidateElements.length === 0) { + if (otherCandidateElement === candidateElement) { return null; } - if (candidateElements.length === 1) { - return candidateElements[0] as NonDeleted; - } - - // Prefer the shape with the border being tested (if any) - const borderTestElements = candidateElements.filter((element) => - bindingBorderTest(element, pointerCoords, elementsMap, zoom, false), - ); - if (borderTestElements.length === 1) { - return borderTestElements[0]; - } - - // Prefer smaller shapes - return candidateElements - .sort( - (a, b) => b.width ** 2 + b.height ** 2 - (a.width ** 2 + a.height ** 2), - ) - .pop() as NonDeleted; + return candidateElement; } const hoveredElement = getElementAtPositionForBinding( @@ -641,8 +687,7 @@ export const getHoveredElementForBinding = ( zoom, // disable fullshape snapping for frame elements so we // can bind to frame children - (fullShape || !isBindingFallthroughEnabled(element)) && - !isFrameLikeElement(element), + fullShape || !isFrameLikeElement(element), ), ); @@ -1179,34 +1224,39 @@ export const snapToMid = ( export const getOutlineAvoidingPoint = ( element: NonDeleted, - coords: GlobalPoint, - pointIndex: number, + startOrEnd: "start" | "end", scene: Scene, zoom: AppState["zoom"], fallback?: GlobalPoint, ): GlobalPoint => { const elementsMap = scene.getNonDeletedElementsMap(); + const elements = scene.getNonDeletedElements(); + const coords = LinearElementEditor.getPointAtIndexGlobalCoordinates( + element, + startOrEnd ? 0 : -1, + elementsMap, + ); const hoveredElement = getHoveredElementForBinding( - { x: coords[0], y: coords[1] }, - scene.getNonDeletedElements(), + tupleToCoors(coords), + tupleToCoors( + LinearElementEditor.getPointAtIndexGlobalCoordinates( + element, + startOrEnd ? -1 : 0, + elementsMap, + ), + ), + elements, elementsMap, zoom, true, isElbowArrow(element), ); - if (hoveredElement) { - const newPoints = Array.from(element.points); - newPoints[pointIndex] = pointFrom( - coords[0] - element.x, - coords[1] - element.y, - ); + const pointIndex = startOrEnd === "start" ? 0 : element.points.length - 1; + if (hoveredElement) { return bindPointToSnapToElementOutline( - { - ...element, - points: newPoints, - }, + element, hoveredElement, pointIndex === 0 ? "start" : "end", elementsMap, diff --git a/packages/element/tests/binding.test.tsx b/packages/element/tests/binding.test.tsx index 2b6585965..a667597b6 100644 --- a/packages/element/tests/binding.test.tsx +++ b/packages/element/tests/binding.test.tsx @@ -503,4 +503,82 @@ describe("element binding", () => { }); }); }); + + // UX RATIONALE: The arrow might be outside of the shape at high zoom and you + // won't see what's going on. + it( + "allow non-binding simple (complex) arrow creation while start and end" + + " points are in the same shape", + () => { + UI.createElement("rectangle", { + x: 0, + y: 0, + width: 100, + height: 100, + }); + + const arrow = UI.createElement("arrow", { + x: 5, + y: 5, + height: 95, + width: 95, + }); + + expect(arrow.startBinding).toBe(null); + expect(arrow.endBinding).toBe(null); + expect(arrow.points).toCloselyEqualPoints([ + [0, 0], + [95, 95], + ]); + + const rect2 = API.createElement({ + type: "rectangle", + x: 300, + y: 300, + width: 100, + height: 100, + backgroundColor: "red", + fillStyle: "solid", + }); + + API.setElements([rect2]); + + const arrow2 = UI.createElement("arrow", { + x: 305, + y: 305, + height: 95, + width: 95, + }); + + expect(arrow2.startBinding).toBe(null); + expect(arrow2.endBinding).toBe(null); + expect(arrow2.points).toCloselyEqualPoints([ + [0, 0], + [95, 95], + ]); + + UI.createElement("rectangle", { + x: 0, + y: 0, + width: 100, + height: 100, + }); + + const arrow3 = UI.createElement("arrow", { + x: 5, + y: 5, + height: 95, + width: 95, + elbowed: true, + }); + + expect(arrow3.startBinding).toBe(null); + expect(arrow3.endBinding).toBe(null); + expect(arrow3.points).toCloselyEqualPoints([ + [0, 0], + [45, 45], + [95, 95], + ]); + }, + ); }); diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index d00ec458c..ee991b58d 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -2919,14 +2919,8 @@ class App extends React.Component { maybeBindLinearElement( multiElement, this.state, - tupleToCoors( - LinearElementEditor.getPointAtIndexGlobalCoordinates( - multiElement, - -1, - nonDeletedElementsMap, - ), - ), - this.scene.getNonDeletedElementsMap(), + "end", + nonDeletedElementsMap, this.scene.getNonDeletedElements(), ); } @@ -5976,8 +5970,7 @@ class App extends React.Component { toLocalPoint( getOutlineAvoidingPoint( multiElement, - pointFrom(scenePointerX, scenePointerY), - multiElement.points.length - 1, + "end", this.scene, this.state.zoom, pointFrom( @@ -8667,7 +8660,16 @@ class App extends React.Component { ...points.slice(0, -1), toLocalPoint( getOutlineAvoidingPoint( - newElement, + { + ...newElement, + points: [ + ...points.slice(0, -1), + pointFrom( + pointerCoords.x - newElement.x, + pointerCoords.y - newElement.y, + ), + ], + }, pointFrom(pointerCoords.x, pointerCoords.y), newElement.points.length - 1, this.scene, @@ -9091,7 +9093,7 @@ class App extends React.Component { maybeBindLinearElement( newElement, this.state, - pointerCoords, + "end", this.scene.getNonDeletedElementsMap(), this.scene.getNonDeletedElements(), ); diff --git a/packages/excalidraw/tests/helpers/ui.ts b/packages/excalidraw/tests/helpers/ui.ts index c328ae105..510d61d6e 100644 --- a/packages/excalidraw/tests/helpers/ui.ts +++ b/packages/excalidraw/tests/helpers/ui.ts @@ -464,6 +464,7 @@ export class UI { height: initialHeight = initialWidth, angle = 0, points: initialPoints, + elbowed = false, }: { position?: number; x?: number; @@ -473,6 +474,7 @@ export class UI { height?: number; angle?: number; points?: T extends "line" | "arrow" | "freedraw" ? LocalPoint[] : never; + elbowed?: boolean; } = {}, ): Element & { /** Returns the actual, current element from the elements array, instead @@ -491,6 +493,17 @@ export class UI { if (type === "text") { mouse.reset(); mouse.click(x, y); + } else if (type === "arrow" && points.length === 2 && elbowed) { + UI.clickOnTestId("elbow-arrow"); + mouse.reset(); + mouse.moveTo(x + points[0][0], y + points[0][1]); + mouse.click(); + mouse.moveTo( + x + points[points.length - 1][0], + y + points[points.length - 1][1], + ); + mouse.click(); + Keyboard.keyPress(KEYS.ESCAPE); } else if ((type === "line" || type === "arrow") && points.length > 2) { points.forEach((point) => { mouse.reset();