From 281c99e2d1240bdc2c71f9f32a02dd0d9bf3c64b Mon Sep 17 00:00:00 2001 From: Mark Tolmacs Date: Thu, 5 Feb 2026 22:10:03 +0100 Subject: [PATCH] fix: Binding hit test Signed-off-by: Mark Tolmacs --- packages/element/src/arrows/focus.ts | 6 +- packages/element/src/binding.ts | 15 ++- packages/element/src/collision.ts | 170 +++++++++++++------------ packages/element/src/elbowArrow.ts | 16 +-- packages/element/src/zindex.ts | 2 +- packages/excalidraw/components/App.tsx | 26 +++- 6 files changed, 128 insertions(+), 107 deletions(-) diff --git a/packages/element/src/arrows/focus.ts b/packages/element/src/arrows/focus.ts index fa8018adbe..643ec48ac5 100644 --- a/packages/element/src/arrows/focus.ts +++ b/packages/element/src/arrows/focus.ts @@ -20,7 +20,7 @@ import { isElbowArrow, } from "../typeChecks"; import { LinearElementEditor } from "../linearElementEditor"; -import { getHoveredElementForFocusPoint, hitElementItself } from "../collision"; +import { getHoveredElementForBinding, hitElementItself } from "../collision"; import { moveArrowAboveBindable } from "../zindex"; import type { @@ -234,9 +234,9 @@ export const handleFocusPointDrag = ( pointerCoords.y - offsetY, ); const bindingField = isStartBinding ? "startBinding" : "endBinding"; - const hit = getHoveredElementForFocusPoint( - point, + const hit = getHoveredElementForBinding( arrow, + point, scene.getNonDeletedElements(), elementsMap, maxBindingDistance_simple(appState.zoom), diff --git a/packages/element/src/binding.ts b/packages/element/src/binding.ts index e21542ebb8..bf3044b95d 100644 --- a/packages/element/src/binding.ts +++ b/packages/element/src/binding.ts @@ -270,6 +270,7 @@ const bindingStrategyForElbowArrowEndpointDragging = ( elementsMap, ); const hit = getHoveredElementForBinding( + arrow, globalPoint, elements, elementsMap, @@ -321,7 +322,7 @@ const bindingStrategyForNewSimpleArrowEndpointDragging = ( draggingPoints.get(startDragged ? startIdx : endIdx)!.point, elementsMap, ); - const hit = getHoveredElementForBinding(point, elements, elementsMap); + const hit = getHoveredElementForBinding(arrow, point, elements, elementsMap); // With new arrows this handles the binding at arrow creation if (startDragged) { @@ -366,7 +367,12 @@ const bindingStrategyForNewSimpleArrowEndpointDragging = ( // Check and handle nested shapes if (hit && arrow.startBinding) { const startBinding = arrow.startBinding; - const allHits = getAllHoveredElementAtPoint(point, elements, elementsMap); + const allHits = getAllHoveredElementAtPoint( + arrow, + point, + elements, + elementsMap, + ); if (allHits.find((el) => el.id === startBinding.elementId)) { const otherElement = elementsMap.get( @@ -468,9 +474,9 @@ const bindingStrategyForSimpleArrowEndpointDragging_complex = ( let other: BindingStrategy = { mode: undefined }; const isMultiPoint = arrow.points.length > 2; - const hit = getHoveredElementForBinding(point, elements, elementsMap); + const hit = getHoveredElementForBinding(arrow, point, elements, elementsMap); const isOverlapping = oppositeBinding - ? getAllHoveredElementAtPoint(point, elements, elementsMap).some( + ? getAllHoveredElementAtPoint(arrow, point, elements, elementsMap).some( (el) => el.id === oppositeBinding.elementId, ) : false; @@ -695,6 +701,7 @@ const getBindingStrategyForDraggingBindingElementEndpoints_simple = ( elementsMap, ); const hit = getHoveredElementForBinding( + arrow, globalPoint, elements, elementsMap, diff --git a/packages/element/src/collision.ts b/packages/element/src/collision.ts index 6999512b44..8289c5adbc 100644 --- a/packages/element/src/collision.ts +++ b/packages/element/src/collision.ts @@ -65,7 +65,6 @@ import { hasBackground } from "./comparisons"; import type { ElementsMap, - ExcalidrawArrowElement, ExcalidrawBindableElement, ExcalidrawDiamondElement, ExcalidrawElement, @@ -254,25 +253,20 @@ export const hitElementBoundText = ( return isPointInElement(point, boundTextElement, elementsMap); }; -const bindingBorderTest = ( +const borderDistance = ( element: NonDeleted, - [x, y]: Readonly, - elementsMap: NonDeletedSceneElementsMap, + point: GlobalPoint, + elementsMap: ElementsMap, tolerance: number = 0, -): boolean => { - const p = pointFrom(x, y); - const shouldTestInside = - // disable fullshape snapping for frame elements so we - // can bind to frame children - !isFrameLikeElement(element); - +) => { // PERF: Run a cheap test to see if the binding element // is even close to the element + const [x, y] = point; const t = Math.max(1, tolerance); const bounds = [x - t, y - t, x + t, y + t] as Bounds; const elementBounds = getElementBounds(element, elementsMap); if (!doBoundsIntersect(bounds, elementBounds)) { - return false; + return -Infinity; } // If the element is inside a frame, we should clip the element @@ -283,26 +277,70 @@ const bindingBorderTest = ( enclosingFrame, elementsMap, ); - if (!pointInsideBounds(p, enclosingFrameBounds)) { - return false; + if (!pointInsideBounds(point, enclosingFrameBounds)) { + return -Infinity; } } } - // Do the intersection test against the element since it's close enough - const intersections = intersectElementWithLineSegment( - element, - elementsMap, - lineSegment(elementCenterPoint(element, elementsMap), p), - ); - const distance = distanceToElement(element, elementsMap, p); + const distance = distanceToElement(element, elementsMap, point); + if (isPointInElement(point, element, elementsMap)) { + return distance; + } - return shouldTestInside - ? intersections.length === 0 || distance <= tolerance - : intersections.length > 0 && distance <= t; + return distance > tolerance ? -Infinity : -distance; }; +// const bindingBorderTest = ( +// element: NonDeleted, +// [x, y]: Readonly, +// elementsMap: NonDeletedSceneElementsMap, +// tolerance: number = 0, +// ): boolean => { +// const p = pointFrom(x, y); +// const shouldTestInside = +// // disable fullshape snapping for frame elements so we +// // can bind to frame children +// !isFrameLikeElement(element); + +// // PERF: Run a cheap test to see if the binding element +// // is even close to the element +// const t = Math.max(1, tolerance); +// const bounds = [x - t, y - t, x + t, y + t] as Bounds; +// const elementBounds = getElementBounds(element, elementsMap); +// if (!doBoundsIntersect(bounds, elementBounds)) { +// return false; +// } + +// // If the element is inside a frame, we should clip the element +// if (element.frameId) { +// const enclosingFrame = elementsMap.get(element.frameId); +// if (enclosingFrame && isFrameLikeElement(enclosingFrame)) { +// const enclosingFrameBounds = getElementBounds( +// enclosingFrame, +// elementsMap, +// ); +// if (!pointInsideBounds(p, enclosingFrameBounds)) { +// return false; +// } +// } +// } + +// // Do the intersection test against the element since it's close enough +// const intersections = intersectElementWithLineSegment( +// element, +// elementsMap, +// lineSegment(elementCenterPoint(element, elementsMap), p), +// ); +// const distance = distanceToElement(element, elementsMap, p); + +// return shouldTestInside +// ? intersections.length === 0 || distance <= tolerance +// : intersections.length > 0 && distance <= t; +// }; + export const getAllHoveredElementAtPoint = ( + arrow: { elbowed: boolean }, point: Readonly, elements: readonly Ordered[], elementsMap: NonDeletedSceneElementsMap, @@ -322,7 +360,13 @@ export const getAllHoveredElementAtPoint = ( if ( isBindableElement(element, false) && - bindingBorderTest(element, point, elementsMap, tolerance) + hitElementItself({ + element, + point, + elementsMap, + threshold: tolerance ?? getBindingGap(element, arrow), + overrideShouldTestInside: true, + }) ) { candidateElements.push(element); @@ -339,82 +383,42 @@ export const getAllHoveredElementAtPoint = ( }; export const getHoveredElementForBinding = ( + arrow: { elbowed: boolean }, point: Readonly, elements: readonly Ordered[], elementsMap: NonDeletedSceneElementsMap, tolerance?: number, ): NonDeleted | null => { - const candidateElements = getAllHoveredElementAtPoint( - point, - elements, - elementsMap, - tolerance, - ); - - if (!candidateElements || candidateElements.length === 0) { - return null; - } - - if (candidateElements.length === 1) { - return candidateElements[0]; - } - - // Prefer smaller shapes - return candidateElements - .sort( - (a, b) => b.width ** 2 + b.height ** 2 - (a.width ** 2 + a.height ** 2), - ) - .pop() as NonDeleted; -}; - -export const getHoveredElementForFocusPoint = ( - point: GlobalPoint, - arrow: ExcalidrawArrowElement, - elements: readonly Ordered[], - elementsMap: NonDeletedSceneElementsMap, - tolerance?: number, -): ExcalidrawBindableElement | null => { - const candidateElements: NonDeleted[] = []; - // We need to to hit testing from front (end of the array) to back (beginning of the array) - // because array is ordered from lower z-index to highest and we want element z-index - // with higher z-index + const candidateElements: { + element: NonDeleted; + distance: number; + }[] = []; for (let index = elements.length - 1; index >= 0; --index) { const element = elements[index]; - invariant( - !element.isDeleted, - "Elements in the function parameter for getAllElementsAtPositionForBinding() should not contain deleted elements", - ); + if (!isBindableElement(element, false)) { + continue; + } - if ( - isBindableElement(element, false) && - bindingBorderTest(element, point, elementsMap, tolerance) - ) { - candidateElements.push(element); + const distance = borderDistance(element, point, elementsMap, tolerance); + const bindingGap = getBindingGap(element, arrow); + + if (distance > -(tolerance ?? bindingGap)) { + candidateElements.push({ element, distance }); } } - if (!candidateElements || candidateElements.length === 0) { + if (candidateElements.length === 0) { return null; } if (candidateElements.length === 1) { - return candidateElements[0]; + return candidateElements[0].element; } - const distanceFilteredCandidateElements = candidateElements - // Resolve by distance - .filter( - (el) => - distanceToElement(el, elementsMap, point) <= getBindingGap(el, arrow) || - isPointInElement(point, el, elementsMap), - ); - - if (distanceFilteredCandidateElements.length === 0) { - return null; - } - - return distanceFilteredCandidateElements[0] as NonDeleted; + return candidateElements + .sort((a, b) => a.distance - b.distance) + .map((c) => c.element)[0]; }; /** diff --git a/packages/element/src/elbowArrow.ts b/packages/element/src/elbowArrow.ts index 024b846b12..d4034920ac 100644 --- a/packages/element/src/elbowArrow.ts +++ b/packages/element/src/elbowArrow.ts @@ -318,6 +318,7 @@ const handleSegmentRelease = ( ...rest } = getElbowArrowData( { + ...arrow, x, y, startBinding, @@ -1041,6 +1042,7 @@ export const updateElbowArrowPoints = ( ...rest } = getElbowArrowData( { + ...arrow, x: arrow.x, y: arrow.y, startBinding, @@ -1190,15 +1192,7 @@ export const updateElbowArrowPoints = ( * - hoveredEndElement: The element being hovered over at the end point. */ const getElbowArrowData = ( - arrow: { - x: number; - y: number; - startBinding: FixedPointBinding | null; - endBinding: FixedPointBinding | null; - startArrowhead: Arrowhead | null; - endArrowhead: Arrowhead | null; - points: readonly LocalPoint[]; - }, + arrow: ExcalidrawElbowArrowElement, elementsMap: NonDeletedSceneElementsMap, nextPoints: readonly LocalPoint[], options?: { @@ -1223,6 +1217,7 @@ const getElbowArrowData = ( const elements = Array.from(elementsMap.values()); hoveredStartElement = getHoveredElement( + arrow, origStartGlobalPoint, elementsMap, elements, @@ -1230,6 +1225,7 @@ const getElbowArrowData = ( ) || null; hoveredEndElement = getHoveredElement( + arrow, origEndGlobalPoint, elementsMap, elements, @@ -2279,12 +2275,14 @@ const getBindPointHeading = ( ); const getHoveredElement = ( + arrow: ExcalidrawElbowArrowElement, origPoint: GlobalPoint, elementsMap: NonDeletedSceneElementsMap, elements: readonly Ordered[], zoom?: AppState["zoom"], ) => { return getHoveredElementForBinding( + arrow, origPoint, elements, elementsMap, diff --git a/packages/element/src/zindex.ts b/packages/element/src/zindex.ts index c4171c6bbd..3eda1eec90 100644 --- a/packages/element/src/zindex.ts +++ b/packages/element/src/zindex.ts @@ -160,7 +160,7 @@ export const moveArrowAboveBindable = ( ): readonly OrderedExcalidrawElement[] => { const hoveredElement = hit ? hit - : getHoveredElementForBinding(point, elements, elementsMap); + : getHoveredElementForBinding(arrow, point, elements, elementsMap); if (!hoveredElement) { return elements; diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index 118147fafa..d1412583bb 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -940,6 +940,9 @@ class App extends React.Component { ); const elementsMap = this.scene.getNonDeletedElementsMap(); const hoveredElement = getHoveredElementForBinding( + elementsMap.get( + this.state.selectedLinearElement.elementId, + ) as ExcalidrawArrowElement, pointFrom( this.lastPointerMoveCoords.x, this.lastPointerMoveCoords.y, @@ -1076,6 +1079,7 @@ class App extends React.Component { const { x, y } = this.lastPointerMoveCoords; const hoveredElement = getHoveredElementForBinding( + arrow, pointFrom(x, y), this.scene.getNonDeletedElements(), this.scene.getNonDeletedElementsMap(), @@ -5381,12 +5385,6 @@ class App extends React.Component { this.state, ); - const hoveredElement = getHoveredElementForBinding( - pointFrom(scenePointer.x, scenePointer.y), - this.scene.getNonDeletedElements(), - this.scene.getNonDeletedElementsMap(), - ); - if (this.state.selectedLinearElement) { const element = LinearElementEditor.getElement( this.state.selectedLinearElement.elementId, @@ -5394,6 +5392,13 @@ class App extends React.Component { ); if (isBindingElement(element)) { + const hoveredElement = getHoveredElementForBinding( + element, + pointFrom(scenePointer.x, scenePointer.y), + this.scene.getNonDeletedElements(), + this.scene.getNonDeletedElementsMap(), + ); + this.handleDelayedBindModeChange(element, hoveredElement); } } @@ -7080,7 +7085,8 @@ class App extends React.Component { ); const elementsMap = this.scene.getNonDeletedElementsMap(); const hoveredElement = getHoveredElementForBinding( - globalPoint, + { elbowed: this.state.currentItemArrowType === ARROW_TYPE.elbow }, + pointFrom(scenePointerX, scenePointerY), this.scene.getNonDeletedElements(), elementsMap, maxBindingDistance_simple(this.state.zoom), @@ -7119,6 +7125,7 @@ class App extends React.Component { isArrowElement(this.state.newElement) && isBindingEnabled(this.state) && getHoveredElementForBinding( + this.state.newElement, pointFrom(scenePointerX, scenePointerY), this.scene.getNonDeletedElements(), this.scene.getNonDeletedElementsMap(), @@ -7237,6 +7244,7 @@ class App extends React.Component { if (isSimpleArrow(multiElement)) { const hoveredElement = getHoveredElementForBinding( + multiElement, pointFrom(scenePointerX, scenePointerY), this.scene.getNonDeletedElements(), elementsMap, @@ -7269,6 +7277,7 @@ class App extends React.Component { if (this.state.activeTool.type === "arrow") { const hit = getHoveredElementForBinding( + { elbowed: this.state.currentItemArrowType === ARROW_TYPE.elbow }, pointFrom(scenePointerX, scenePointerY), this.scene.getNonDeletedElements(), this.scene.getNonDeletedElementsMap(), @@ -9271,6 +9280,7 @@ class App extends React.Component { const hoveredElementForBinding = isBindingEnabled(this.state) && getHoveredElementForBinding( + { elbowed: isElbowArrow(multiElement) }, pointFrom( this.lastPointerMoveCoords?.x ?? rx + multiElement.points[multiElement.points.length - 1][0], @@ -9393,6 +9403,7 @@ class App extends React.Component { const elementsMap = this.scene.getNonDeletedElementsMap(); const boundElement = isBindingEnabled(this.state) ? getHoveredElementForBinding( + { elbowed: this.state.currentItemArrowType === ARROW_TYPE.elbow }, point, this.scene.getNonDeletedElements(), elementsMap, @@ -9876,6 +9887,7 @@ class App extends React.Component { if (isBindingElement(element)) { const hoveredElement = getHoveredElementForBinding( + element, pointFrom(pointerCoords.x, pointerCoords.y), this.scene.getNonDeletedElements(), elementsMap,