From dff69e91912507bbfcc68b35277cc6031ce5b437 Mon Sep 17 00:00:00 2001 From: jhanma17dev Date: Wed, 9 Apr 2025 10:04:51 -0500 Subject: [PATCH 01/15] chore: Element center point util (#9298) --- packages/common/src/utils.ts | 17 ++++++++- packages/element/src/binding.ts | 46 ++++++------------------- packages/element/src/collision.ts | 21 +++-------- packages/element/src/cropElement.ts | 4 ++- packages/element/src/distance.ts | 18 +++------- packages/element/src/shapes.ts | 3 +- packages/element/src/utils.ts | 12 +++---- packages/excalidraw/tests/helpers/ui.ts | 6 ++-- 8 files changed, 49 insertions(+), 78 deletions(-) diff --git a/packages/common/src/utils.ts b/packages/common/src/utils.ts index 7fa98eb2d..54eaa67cc 100644 --- a/packages/common/src/utils.ts +++ b/packages/common/src/utils.ts @@ -1,9 +1,10 @@ -import { average } from "@excalidraw/math"; +import { average, pointFrom, type GlobalPoint } from "@excalidraw/math"; import type { ExcalidrawBindableElement, FontFamilyValues, FontString, + ExcalidrawElement, } from "@excalidraw/element/types"; import type { @@ -1201,3 +1202,17 @@ export const escapeDoubleQuotes = (str: string) => { export const castArray = (value: T | T[]): T[] => Array.isArray(value) ? value : [value]; + +export const elementCenterPoint = ( + element: ExcalidrawElement, + xOffset: number = 0, + yOffset: number = 0, +) => { + const { x, y, width, height } = element; + + const centerXPoint = x + width / 2 + xOffset; + + const centerYPoint = y + height / 2 + yOffset; + + return pointFrom(centerXPoint, centerYPoint); +}; diff --git a/packages/element/src/binding.ts b/packages/element/src/binding.ts index 7a67cf0a1..5c32e8c81 100644 --- a/packages/element/src/binding.ts +++ b/packages/element/src/binding.ts @@ -6,6 +6,7 @@ import { invariant, isDevEnv, isTestEnv, + elementCenterPoint, } from "@excalidraw/common"; import { @@ -904,13 +905,7 @@ export const getHeadingForElbowArrowSnap = ( if (!distance) { return vectorToHeading( - vectorFromPoint( - p, - pointFrom( - bindableElement.x + bindableElement.width / 2, - bindableElement.y + bindableElement.height / 2, - ), - ), + vectorFromPoint(p, elementCenterPoint(bindableElement)), ); } @@ -1040,10 +1035,7 @@ export const avoidRectangularCorner = ( element: ExcalidrawBindableElement, p: GlobalPoint, ): GlobalPoint => { - const center = pointFrom( - element.x + element.width / 2, - element.y + element.height / 2, - ); + const center = elementCenterPoint(element); const nonRotatedPoint = pointRotateRads(p, center, -element.angle as Radians); if (nonRotatedPoint[0] < element.x && nonRotatedPoint[1] < element.y) { @@ -1140,10 +1132,9 @@ export const snapToMid = ( tolerance: number = 0.05, ): GlobalPoint => { const { x, y, width, height, angle } = element; - const center = pointFrom( - x + width / 2 - 0.1, - y + height / 2 - 0.1, - ); + + const center = elementCenterPoint(element, -0.1, -0.1); + const nonRotated = pointRotateRads(p, center, -angle as Radians); // snap-to-center point is adaptive to element size, but we don't want to go @@ -1228,10 +1219,7 @@ const updateBoundPoint = ( startOrEnd === "startBinding" ? "start" : "end", elementsMap, ).fixedPoint; - const globalMidPoint = pointFrom( - bindableElement.x + bindableElement.width / 2, - bindableElement.y + bindableElement.height / 2, - ); + const globalMidPoint = elementCenterPoint(bindableElement); const global = pointFrom( bindableElement.x + fixedPoint[0] * bindableElement.width, bindableElement.y + fixedPoint[1] * bindableElement.height, @@ -1275,10 +1263,7 @@ const updateBoundPoint = ( elementsMap, ); - const center = pointFrom( - bindableElement.x + bindableElement.width / 2, - bindableElement.y + bindableElement.height / 2, - ); + const center = elementCenterPoint(bindableElement); const interceptorLength = pointDistance(adjacentPoint, edgePointAbsolute) + pointDistance(adjacentPoint, center) + @@ -1771,10 +1756,7 @@ const determineFocusDistance = ( // Another point on the line, in absolute coordinates (closer to element) b: GlobalPoint, ): number => { - const center = pointFrom( - element.x + element.width / 2, - element.y + element.height / 2, - ); + const center = elementCenterPoint(element); if (pointsEqual(a, b)) { return 0; @@ -1904,10 +1886,7 @@ const determineFocusPoint = ( focus: number, adjacentPoint: GlobalPoint, ): GlobalPoint => { - const center = pointFrom( - element.x + element.width / 2, - element.y + element.height / 2, - ); + const center = elementCenterPoint(element); if (focus === 0) { return center; @@ -2338,10 +2317,7 @@ export const getGlobalFixedPointForBindableElement = ( element.x + element.width * fixedX, element.y + element.height * fixedY, ), - pointFrom( - element.x + element.width / 2, - element.y + element.height / 2, - ), + elementCenterPoint(element), element.angle, ); }; diff --git a/packages/element/src/collision.ts b/packages/element/src/collision.ts index 0fabe9839..07b17bfde 100644 --- a/packages/element/src/collision.ts +++ b/packages/element/src/collision.ts @@ -1,4 +1,4 @@ -import { isTransparent } from "@excalidraw/common"; +import { isTransparent, elementCenterPoint } from "@excalidraw/common"; import { curveIntersectLineSegment, isPointWithinBounds, @@ -16,7 +16,7 @@ import { } from "@excalidraw/math/ellipse"; import { isPointInShape, isPointOnShape } from "@excalidraw/utils/collision"; -import { getPolygonShape } from "@excalidraw/utils/shape"; +import { type GeometricShape, getPolygonShape } from "@excalidraw/utils/shape"; import type { GlobalPoint, @@ -26,8 +26,6 @@ import type { Radians, } from "@excalidraw/math"; -import type { GeometricShape } from "@excalidraw/utils/shape"; - import type { FrameNameBounds } from "@excalidraw/excalidraw/types"; import { getBoundTextShape, isPathALoop } from "./shapes"; @@ -191,10 +189,7 @@ const intersectRectanguloidWithLineSegment = ( l: LineSegment, offset: number = 0, ): GlobalPoint[] => { - const center = pointFrom( - element.x + element.width / 2, - element.y + element.height / 2, - ); + const center = elementCenterPoint(element); // To emulate a rotated rectangle we rotate the point in the inverse angle // instead. It's all the same distance-wise. const rotatedA = pointRotateRads( @@ -253,10 +248,7 @@ const intersectDiamondWithLineSegment = ( l: LineSegment, offset: number = 0, ): GlobalPoint[] => { - const center = pointFrom( - element.x + element.width / 2, - element.y + element.height / 2, - ); + const center = elementCenterPoint(element); // Rotate the point to the inverse direction to simulate the rotated diamond // points. It's all the same distance-wise. @@ -304,10 +296,7 @@ const intersectEllipseWithLineSegment = ( l: LineSegment, offset: number = 0, ): GlobalPoint[] => { - const center = pointFrom( - element.x + element.width / 2, - element.y + element.height / 2, - ); + const center = elementCenterPoint(element); const rotatedA = pointRotateRads(l[0], center, -element.angle as Radians); const rotatedB = pointRotateRads(l[1], center, -element.angle as Radians); diff --git a/packages/element/src/cropElement.ts b/packages/element/src/cropElement.ts index dd75f9360..2bc930d66 100644 --- a/packages/element/src/cropElement.ts +++ b/packages/element/src/cropElement.ts @@ -14,6 +14,8 @@ import { } from "@excalidraw/math"; import { type Point } from "points-on-curve"; +import { elementCenterPoint } from "@excalidraw/common"; + import { getElementAbsoluteCoords, getResizedElementAbsoluteCoords, @@ -61,7 +63,7 @@ export const cropElement = ( const rotatedPointer = pointRotateRads( pointFrom(pointerX, pointerY), - pointFrom(element.x + element.width / 2, element.y + element.height / 2), + elementCenterPoint(element), -element.angle as Radians, ); diff --git a/packages/element/src/distance.ts b/packages/element/src/distance.ts index d9db939e4..d261faf7d 100644 --- a/packages/element/src/distance.ts +++ b/packages/element/src/distance.ts @@ -1,12 +1,13 @@ import { curvePointDistance, distanceToLineSegment, - pointFrom, pointRotateRads, } from "@excalidraw/math"; import { ellipse, ellipseDistanceFromPoint } from "@excalidraw/math/ellipse"; +import { elementCenterPoint } from "@excalidraw/common"; + import type { GlobalPoint, Radians } from "@excalidraw/math"; import { @@ -53,10 +54,7 @@ const distanceToRectanguloidElement = ( element: ExcalidrawRectanguloidElement, p: GlobalPoint, ) => { - const center = pointFrom( - element.x + element.width / 2, - element.y + element.height / 2, - ); + const center = elementCenterPoint(element); // To emulate a rotated rectangle we rotate the point in the inverse angle // instead. It's all the same distance-wise. const rotatedPoint = pointRotateRads(p, center, -element.angle as Radians); @@ -84,10 +82,7 @@ const distanceToDiamondElement = ( element: ExcalidrawDiamondElement, p: GlobalPoint, ): number => { - const center = pointFrom( - element.x + element.width / 2, - element.y + element.height / 2, - ); + const center = elementCenterPoint(element); // Rotate the point to the inverse direction to simulate the rotated diamond // points. It's all the same distance-wise. @@ -115,10 +110,7 @@ const distanceToEllipseElement = ( element: ExcalidrawEllipseElement, p: GlobalPoint, ): number => { - const center = pointFrom( - element.x + element.width / 2, - element.y + element.height / 2, - ); + const center = elementCenterPoint(element); return ellipseDistanceFromPoint( // Instead of rotating the ellipse, rotate the point to the inverse angle pointRotateRads(p, center, -element.angle as Radians), diff --git a/packages/element/src/shapes.ts b/packages/element/src/shapes.ts index 1d6e13340..96542c538 100644 --- a/packages/element/src/shapes.ts +++ b/packages/element/src/shapes.ts @@ -4,6 +4,7 @@ import { LINE_CONFIRM_THRESHOLD, ROUNDNESS, invariant, + elementCenterPoint, } from "@excalidraw/common"; import { isPoint, @@ -297,7 +298,7 @@ export const aabbForElement = ( midY: element.y + element.height / 2, }; - const center = pointFrom(bbox.midX, bbox.midY); + const center = elementCenterPoint(element); const [topLeftX, topLeftY] = pointRotateRads( pointFrom(bbox.minX, bbox.minY), center, diff --git a/packages/element/src/utils.ts b/packages/element/src/utils.ts index 7042b5d8f..57b1e4346 100644 --- a/packages/element/src/utils.ts +++ b/packages/element/src/utils.ts @@ -10,6 +10,8 @@ import { type GlobalPoint, } from "@excalidraw/math"; +import { elementCenterPoint } from "@excalidraw/common"; + import type { Curve, LineSegment } from "@excalidraw/math"; import { getCornerRadius } from "./shapes"; @@ -68,10 +70,7 @@ export function deconstructRectanguloidElement( return [sides, []]; } - const center = pointFrom( - element.x + element.width / 2, - element.y + element.height / 2, - ); + const center = elementCenterPoint(element); const r = rectangle( pointFrom(element.x, element.y), @@ -254,10 +253,7 @@ export function deconstructDiamondElement( return [[topRight, bottomRight, bottomLeft, topLeft], []]; } - const center = pointFrom( - element.x + element.width / 2, - element.y + element.height / 2, - ); + const center = elementCenterPoint(element); const [top, right, bottom, left]: GlobalPoint[] = [ pointFrom(element.x + topX, element.y + topY), diff --git a/packages/excalidraw/tests/helpers/ui.ts b/packages/excalidraw/tests/helpers/ui.ts index 0e5e43367..32de489f1 100644 --- a/packages/excalidraw/tests/helpers/ui.ts +++ b/packages/excalidraw/tests/helpers/ui.ts @@ -20,7 +20,7 @@ import { isTextElement, isFrameLikeElement, } from "@excalidraw/element/typeChecks"; -import { KEYS, arrayToMap } from "@excalidraw/common"; +import { KEYS, arrayToMap, elementCenterPoint } from "@excalidraw/common"; import type { GlobalPoint, LocalPoint, Radians } from "@excalidraw/math"; @@ -151,7 +151,7 @@ export class Keyboard { const getElementPointForSelection = ( element: ExcalidrawElement, ): GlobalPoint => { - const { x, y, width, height, angle } = element; + const { x, y, width, angle } = element; const target = pointFrom( x + (isLinearElement(element) || isFreeDrawElement(element) ? 0 : width / 2), @@ -166,7 +166,7 @@ const getElementPointForSelection = ( (bounds[1] + bounds[3]) / 2, ); } else { - center = pointFrom(x + width / 2, y + height / 2); + center = elementCenterPoint(element); } if (isTextElement(element)) { From 01304aac498f9b653bb7019cafc7f8a6eb5936c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1rk=20Tolm=C3=A1cs?= Date: Sun, 13 Apr 2025 21:21:49 +0200 Subject: [PATCH 02/15] feat: Keep text label horizontal (#9364) Co-authored-by: dwelle <5153846+dwelle@users.noreply.github.com> --- packages/common/src/constants.ts | 1 + packages/common/src/utils.ts | 2 + packages/element/src/textElement.ts | 23 ++++- .../excalidraw/actions/actionBoundText.tsx | 4 + packages/excalidraw/components/App.tsx | 62 ++++++------ packages/excalidraw/data/restore.ts | 6 +- .../excalidraw/wysiwyg/textWysiwyg.test.tsx | 97 +++++++++++++++++++ 7 files changed, 161 insertions(+), 34 deletions(-) diff --git a/packages/common/src/constants.ts b/packages/common/src/constants.ts index 7e8c49ea1..7eb36d5d9 100644 --- a/packages/common/src/constants.ts +++ b/packages/common/src/constants.ts @@ -112,6 +112,7 @@ export const YOUTUBE_STATES = { export const ENV = { TEST: "test", DEVELOPMENT: "development", + PRODUCTION: "production", }; export const CLASSES = { diff --git a/packages/common/src/utils.ts b/packages/common/src/utils.ts index 54eaa67cc..b6e9fdd78 100644 --- a/packages/common/src/utils.ts +++ b/packages/common/src/utils.ts @@ -739,6 +739,8 @@ export const isTestEnv = () => import.meta.env.MODE === ENV.TEST; export const isDevEnv = () => import.meta.env.MODE === ENV.DEVELOPMENT; +export const isProdEnv = () => import.meta.env.MODE === ENV.PRODUCTION; + export const isServerEnv = () => typeof process !== "undefined" && !!process?.env?.NODE_ENV; diff --git a/packages/element/src/textElement.ts b/packages/element/src/textElement.ts index ea27c318f..55c3f692c 100644 --- a/packages/element/src/textElement.ts +++ b/packages/element/src/textElement.ts @@ -6,6 +6,8 @@ import { TEXT_ALIGN, VERTICAL_ALIGN, getFontString, + isProdEnv, + invariant, } from "@excalidraw/common"; import type { AppState } from "@excalidraw/excalidraw/types"; @@ -26,6 +28,8 @@ import { isTextElement, } from "./typeChecks"; +import type { Radians } from "../../math/src"; + import type { MaybeTransformHandleType } from "./transformHandles"; import type { ElementsMap, @@ -44,13 +48,25 @@ export const redrawTextBoundingBox = ( informMutation = true, ) => { let maxWidth = undefined; + + if (!isProdEnv()) { + invariant( + !container || !isArrowElement(container) || textElement.angle === 0, + "text element angle must be 0 if bound to arrow container", + ); + } + const boundTextUpdates = { x: textElement.x, y: textElement.y, text: textElement.text, width: textElement.width, height: textElement.height, - angle: container?.angle ?? textElement.angle, + angle: (container + ? isArrowElement(container) + ? 0 + : container.angle + : textElement.angle) as Radians, }; boundTextUpdates.text = textElement.text; @@ -335,7 +351,10 @@ export const getTextElementAngle = ( textElement: ExcalidrawTextElement, container: ExcalidrawTextContainer | null, ) => { - if (!container || isArrowElement(container)) { + if (isArrowElement(container)) { + return 0; + } + if (!container) { return textElement.angle; } return container.angle; diff --git a/packages/excalidraw/actions/actionBoundText.tsx b/packages/excalidraw/actions/actionBoundText.tsx index ae18c0d98..d08ad341e 100644 --- a/packages/excalidraw/actions/actionBoundText.tsx +++ b/packages/excalidraw/actions/actionBoundText.tsx @@ -21,6 +21,7 @@ import { import { hasBoundTextElement, + isArrowElement, isTextBindableContainer, isTextElement, isUsingAdaptiveRadius, @@ -46,6 +47,8 @@ import { CaptureUpdateAction } from "../store"; import { register } from "./register"; +import type { Radians } from "../../math/src"; + import type { AppState } from "../types"; export const actionUnbindText = register({ @@ -155,6 +158,7 @@ export const actionBindText = register({ verticalAlign: VERTICAL_ALIGN.MIDDLE, textAlign: TEXT_ALIGN.CENTER, autoResize: true, + angle: (isArrowElement(container) ? 0 : container?.angle ?? 0) as Radians, }); mutateElement(container, { boundElements: (container.boundElements || []).concat({ diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index 276cde027..976abfd76 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -5352,37 +5352,37 @@ class App extends React.Component { y: sceneY, }); - const element = existingTextElement - ? existingTextElement - : newTextElement({ - x: parentCenterPosition - ? parentCenterPosition.elementCenterX - : sceneX, - y: parentCenterPosition - ? parentCenterPosition.elementCenterY - : sceneY, - strokeColor: this.state.currentItemStrokeColor, - backgroundColor: this.state.currentItemBackgroundColor, - fillStyle: this.state.currentItemFillStyle, - strokeWidth: this.state.currentItemStrokeWidth, - strokeStyle: this.state.currentItemStrokeStyle, - roughness: this.state.currentItemRoughness, - opacity: this.state.currentItemOpacity, - text: "", - fontSize, - fontFamily, - textAlign: parentCenterPosition - ? "center" - : this.state.currentItemTextAlign, - verticalAlign: parentCenterPosition - ? VERTICAL_ALIGN.MIDDLE - : DEFAULT_VERTICAL_ALIGN, - containerId: shouldBindToContainer ? container?.id : undefined, - groupIds: container?.groupIds ?? [], - lineHeight, - angle: container?.angle ?? (0 as Radians), - frameId: topLayerFrame ? topLayerFrame.id : null, - }); + const element = + existingTextElement || + newTextElement({ + x: parentCenterPosition ? parentCenterPosition.elementCenterX : sceneX, + y: parentCenterPosition ? parentCenterPosition.elementCenterY : sceneY, + strokeColor: this.state.currentItemStrokeColor, + backgroundColor: this.state.currentItemBackgroundColor, + fillStyle: this.state.currentItemFillStyle, + strokeWidth: this.state.currentItemStrokeWidth, + strokeStyle: this.state.currentItemStrokeStyle, + roughness: this.state.currentItemRoughness, + opacity: this.state.currentItemOpacity, + text: "", + fontSize, + fontFamily, + textAlign: parentCenterPosition + ? "center" + : this.state.currentItemTextAlign, + verticalAlign: parentCenterPosition + ? VERTICAL_ALIGN.MIDDLE + : DEFAULT_VERTICAL_ALIGN, + containerId: shouldBindToContainer ? container?.id : undefined, + groupIds: container?.groupIds ?? [], + lineHeight, + angle: container + ? isArrowElement(container) + ? (0 as Radians) + : container.angle + : (0 as Radians), + frameId: topLayerFrame ? topLayerFrame.id : null, + }); if (!existingTextElement && shouldBindToContainer && container) { mutateElement(container, { diff --git a/packages/excalidraw/data/restore.ts b/packages/excalidraw/data/restore.ts index 4f050c922..1811cbb57 100644 --- a/packages/excalidraw/data/restore.ts +++ b/packages/excalidraw/data/restore.ts @@ -439,7 +439,7 @@ const repairContainerElement = ( // if defined, lest boundElements is stale !boundElement.containerId ) { - (boundElement as Mutable).containerId = + (boundElement as Mutable).containerId = container.id; } } @@ -464,6 +464,10 @@ const repairBoundElement = ( ? elementsMap.get(boundElement.containerId) : null; + (boundElement as Mutable).angle = ( + isArrowElement(container) ? 0 : container?.angle ?? 0 + ) as Radians; + if (!container) { boundElement.containerId = null; return; diff --git a/packages/excalidraw/wysiwyg/textWysiwyg.test.tsx b/packages/excalidraw/wysiwyg/textWysiwyg.test.tsx index 959c5a012..0ba1960d6 100644 --- a/packages/excalidraw/wysiwyg/textWysiwyg.test.tsx +++ b/packages/excalidraw/wysiwyg/textWysiwyg.test.tsx @@ -31,6 +31,7 @@ import { mockBoundingClientRect, restoreOriginalGetBoundingClientRect, } from "../tests/test-utils"; +import { actionBindText } from "../actions"; unmountComponent(); @@ -1568,5 +1569,101 @@ describe("textWysiwyg", () => { expect(text.containerId).toBe(null); expect(text.text).toBe("Excalidraw"); }); + + it("should reset the text element angle to the container's when binding to rotated non-arrow container", async () => { + const text = API.createElement({ + type: "text", + text: "Hello World!", + angle: 45, + }); + const rectangle = API.createElement({ + type: "rectangle", + width: 90, + height: 75, + angle: 30, + }); + + API.setElements([rectangle, text]); + + API.setSelectedElements([rectangle, text]); + + h.app.actionManager.executeAction(actionBindText); + + expect(text.angle).toBe(30); + expect(rectangle.angle).toBe(30); + }); + + it("should reset the text element angle to 0 when binding to rotated arrow container", async () => { + const text = API.createElement({ + type: "text", + text: "Hello World!", + angle: 45, + }); + const arrow = API.createElement({ + type: "arrow", + width: 90, + height: 75, + angle: 30, + }); + + API.setElements([arrow, text]); + + API.setSelectedElements([arrow, text]); + + h.app.actionManager.executeAction(actionBindText); + + expect(text.angle).toBe(0); + expect(arrow.angle).toBe(30); + }); + + it("should keep the text label at 0 degrees when used as an arrow label", async () => { + const arrow = API.createElement({ + type: "arrow", + width: 90, + height: 75, + angle: 30, + }); + + API.setElements([arrow]); + API.setSelectedElements([arrow]); + + mouse.doubleClickAt( + arrow.x + arrow.width / 2, + arrow.y + arrow.height / 2, + ); + + const editor = await getTextEditor(textEditorSelector, true); + + updateTextEditor(editor, "Hello World!"); + + Keyboard.exitTextEditor(editor); + + expect(h.elements[1].angle).toBe(0); + }); + + it("should keep the text label at the same degrees when used as a non-arrow label", async () => { + const rectangle = API.createElement({ + type: "rectangle", + width: 90, + height: 75, + angle: 30, + }); + + API.setElements([rectangle]); + API.setSelectedElements([rectangle]); + + mouse.doubleClickAt( + rectangle.x + rectangle.width / 2, + rectangle.y + rectangle.height / 2, + ); + + const editor = await getTextEditor(textEditorSelector, true); + + updateTextEditor(editor, "Hello World!"); + + Keyboard.exitTextEditor(editor); + + expect(h.elements[1].angle).toBe(30); + }); }); }); From 6fe7de802014d0c967e3b839ccda7711b1028029 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Mon, 14 Apr 2025 20:25:18 +0100 Subject: [PATCH 03/15] fix: Add DOCTYPE and XML preamble in exported SVG documents (#9386) * Add DOCTYPE and XML preamble in exported SVG documents * Update packages/excalidraw/data/index.ts --------- Co-authored-by: David Luzar <5153846+dwelle@users.noreply.github.com> --- packages/common/src/constants.ts | 3 +++ packages/excalidraw/data/index.ts | 7 ++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/common/src/constants.ts b/packages/common/src/constants.ts index 7eb36d5d9..cd3bd7a15 100644 --- a/packages/common/src/constants.ts +++ b/packages/common/src/constants.ts @@ -319,6 +319,9 @@ export const DEFAULT_MAX_IMAGE_WIDTH_OR_HEIGHT = 1440; export const MAX_ALLOWED_FILE_BYTES = 4 * 1024 * 1024; export const SVG_NS = "http://www.w3.org/2000/svg"; +export const SVG_DOCUMENT_PREAMBLE = ` + +`; export const ENCRYPTION_KEY_BITS = 128; diff --git a/packages/excalidraw/data/index.ts b/packages/excalidraw/data/index.ts index ac8147e85..93d5f5677 100644 --- a/packages/excalidraw/data/index.ts +++ b/packages/excalidraw/data/index.ts @@ -5,6 +5,7 @@ import { isFirefox, MIME_TYPES, cloneJSON, + SVG_DOCUMENT_PREAMBLE, } from "@excalidraw/common"; import { getNonDeletedElements } from "@excalidraw/element"; @@ -134,7 +135,11 @@ export const exportCanvas = async ( if (type === "svg") { return fileSave( svgPromise.then((svg) => { - return new Blob([svg.outerHTML], { type: MIME_TYPES.svg }); + // adding SVG preamble so that older software parse the SVG file + // properly + return new Blob([SVG_DOCUMENT_PREAMBLE + svg.outerHTML], { + type: MIME_TYPES.svg, + }); }), { description: "Export to SVG", From 58f7d33d80198a2a80c6d3eb38b3432e062dfbea Mon Sep 17 00:00:00 2001 From: Ryan Di Date: Wed, 16 Apr 2025 00:58:45 +1000 Subject: [PATCH 04/15] perf: make eraser great again (#9352) * perf: make eraser great again * lint * refactor and improve perf * lint --- packages/excalidraw/components/App.tsx | 127 ++---------- packages/excalidraw/eraser/index.ts | 239 +++++++++++++++++++++++ packages/excalidraw/lasso/utils.ts | 8 +- packages/excalidraw/tests/lasso.test.tsx | 3 +- packages/math/src/types.ts | 2 + 5 files changed, 259 insertions(+), 120 deletions(-) create mode 100644 packages/excalidraw/eraser/index.ts diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index 976abfd76..242fd8e46 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -454,7 +454,6 @@ import { import { Emitter } from "../emitter"; import { ElementCanvasButtons } from "../components/ElementCanvasButtons"; import { Store, CaptureUpdateAction } from "../store"; -import { AnimatedTrail } from "../animated-trail"; import { LaserTrails } from "../laser-trails"; import { withBatchedUpdates, withBatchedUpdatesThrottled } from "../reactUtils"; import { textWysiwyg } from "../wysiwyg/textWysiwyg"; @@ -464,6 +463,8 @@ import { isMaybeMermaidDefinition } from "../mermaid"; import { LassoTrail } from "../lasso"; +import { EraserTrail } from "../eraser"; + import { activeConfirmDialogAtom } from "./ActiveConfirmDialog"; import BraveMeasureTextError from "./BraveMeasureTextError"; import { ContextMenu, CONTEXT_MENU_SEPARATOR } from "./ContextMenu"; @@ -675,26 +676,7 @@ class App extends React.Component { animationFrameHandler = new AnimationFrameHandler(); laserTrails = new LaserTrails(this.animationFrameHandler, this); - eraserTrail = new AnimatedTrail(this.animationFrameHandler, this, { - streamline: 0.2, - size: 5, - keepHead: true, - sizeMapping: (c) => { - const DECAY_TIME = 200; - const DECAY_LENGTH = 10; - const t = Math.max(0, 1 - (performance.now() - c.pressure) / DECAY_TIME); - const l = - (DECAY_LENGTH - - Math.min(DECAY_LENGTH, c.totalLength - c.currentIndex)) / - DECAY_LENGTH; - - return Math.min(easeOut(l), easeOut(t)); - }, - fill: () => - this.state.theme === THEME.LIGHT - ? "rgba(0, 0, 0, 0.2)" - : "rgba(255, 255, 255, 0.2)", - }); + eraserTrail = new EraserTrail(this.animationFrameHandler, this); lassoTrail = new LassoTrail(this.animationFrameHandler, this); onChangeEmitter = new Emitter< @@ -1676,8 +1658,8 @@ class App extends React.Component { {selectedElements.length === 1 && @@ -5163,7 +5145,7 @@ class App extends React.Component { return elements; } - private getElementHitThreshold() { + getElementHitThreshold() { return DEFAULT_COLLISION_THRESHOLD / this.state.zoom.value; } @@ -6219,101 +6201,16 @@ class App extends React.Component { private handleEraser = ( event: PointerEvent, - pointerDownState: PointerDownState, scenePointer: { x: number; y: number }, ) => { - this.eraserTrail.addPointToPath(scenePointer.x, scenePointer.y); - - let didChange = false; - - const processedGroups = new Set(); - const nonDeletedElements = this.scene.getNonDeletedElements(); - - const processElements = (elements: ExcalidrawElement[]) => { - for (const element of elements) { - if (element.locked) { - return; - } - - if (event.altKey) { - if (this.elementsPendingErasure.delete(element.id)) { - didChange = true; - } - } else if (!this.elementsPendingErasure.has(element.id)) { - didChange = true; - this.elementsPendingErasure.add(element.id); - } - - // (un)erase groups atomically - if (didChange && element.groupIds?.length) { - const shallowestGroupId = element.groupIds.at(-1)!; - if (!processedGroups.has(shallowestGroupId)) { - processedGroups.add(shallowestGroupId); - const elems = getElementsInGroup( - nonDeletedElements, - shallowestGroupId, - ); - for (const elem of elems) { - if (event.altKey) { - this.elementsPendingErasure.delete(elem.id); - } else { - this.elementsPendingErasure.add(elem.id); - } - } - } - } - } - }; - - const distance = pointDistance( - pointFrom(pointerDownState.lastCoords.x, pointerDownState.lastCoords.y), - pointFrom(scenePointer.x, scenePointer.y), + const elementsToErase = this.eraserTrail.addPointToPath( + scenePointer.x, + scenePointer.y, + event.altKey, ); - const threshold = this.getElementHitThreshold(); - const p = { ...pointerDownState.lastCoords }; - let samplingInterval = 0; - while (samplingInterval <= distance) { - const hitElements = this.getElementsAtPosition(p.x, p.y); - processElements(hitElements); - // Exit since we reached current point - if (samplingInterval === distance) { - break; - } - - // Calculate next point in the line at a distance of sampling interval - samplingInterval = Math.min(samplingInterval + threshold, distance); - - const distanceRatio = samplingInterval / distance; - const nextX = (1 - distanceRatio) * p.x + distanceRatio * scenePointer.x; - const nextY = (1 - distanceRatio) * p.y + distanceRatio * scenePointer.y; - p.x = nextX; - p.y = nextY; - } - - pointerDownState.lastCoords.x = scenePointer.x; - pointerDownState.lastCoords.y = scenePointer.y; - - if (didChange) { - for (const element of this.scene.getNonDeletedElements()) { - if ( - isBoundToContainer(element) && - (this.elementsPendingErasure.has(element.id) || - this.elementsPendingErasure.has(element.containerId)) - ) { - if (event.altKey) { - this.elementsPendingErasure.delete(element.id); - this.elementsPendingErasure.delete(element.containerId); - } else { - this.elementsPendingErasure.add(element.id); - this.elementsPendingErasure.add(element.containerId); - } - } - } - - this.elementsPendingErasure = new Set(this.elementsPendingErasure); - this.triggerRender(); - } + this.elementsPendingErasure = new Set(elementsToErase); + this.triggerRender(); }; // set touch moving for mobile context menu @@ -8159,7 +8056,7 @@ class App extends React.Component { } if (isEraserActive(this.state)) { - this.handleEraser(event, pointerDownState, pointerCoords); + this.handleEraser(event, pointerCoords); return; } diff --git a/packages/excalidraw/eraser/index.ts b/packages/excalidraw/eraser/index.ts new file mode 100644 index 000000000..a9f9103f5 --- /dev/null +++ b/packages/excalidraw/eraser/index.ts @@ -0,0 +1,239 @@ +import { arrayToMap, easeOut, THEME } from "@excalidraw/common"; +import { getElementLineSegments } from "@excalidraw/element/bounds"; +import { + lineSegment, + lineSegmentIntersectionPoints, + pointFrom, +} from "@excalidraw/math"; + +import { getElementsInGroup } from "@excalidraw/element/groups"; + +import { getElementShape } from "@excalidraw/element/shapes"; +import { shouldTestInside } from "@excalidraw/element/collision"; +import { isPointInShape } from "@excalidraw/utils/collision"; +import { + hasBoundTextElement, + isBoundToContainer, +} from "@excalidraw/element/typeChecks"; +import { getBoundTextElementId } from "@excalidraw/element/textElement"; + +import type { GeometricShape } from "@excalidraw/utils/shape"; +import type { + ElementsSegmentsMap, + GlobalPoint, + LineSegment, +} from "@excalidraw/math/types"; +import type { ExcalidrawElement } from "@excalidraw/element/types"; + +import { AnimatedTrail } from "../animated-trail"; + +import type { AnimationFrameHandler } from "../animation-frame-handler"; + +import type App from "../components/App"; + +// just enough to form a segment; this is sufficient for eraser +const POINTS_ON_TRAIL = 2; + +export class EraserTrail extends AnimatedTrail { + private elementsToErase: Set = new Set(); + private groupsToErase: Set = new Set(); + private segmentsCache: Map[]> = new Map(); + private geometricShapesCache: Map> = + new Map(); + + constructor(animationFrameHandler: AnimationFrameHandler, app: App) { + super(animationFrameHandler, app, { + streamline: 0.2, + size: 5, + keepHead: true, + sizeMapping: (c) => { + const DECAY_TIME = 200; + const DECAY_LENGTH = 10; + const t = Math.max( + 0, + 1 - (performance.now() - c.pressure) / DECAY_TIME, + ); + const l = + (DECAY_LENGTH - + Math.min(DECAY_LENGTH, c.totalLength - c.currentIndex)) / + DECAY_LENGTH; + + return Math.min(easeOut(l), easeOut(t)); + }, + fill: () => + app.state.theme === THEME.LIGHT + ? "rgba(0, 0, 0, 0.2)" + : "rgba(255, 255, 255, 0.2)", + }); + } + + startPath(x: number, y: number): void { + this.endPath(); + super.startPath(x, y); + this.elementsToErase.clear(); + } + + addPointToPath(x: number, y: number, restore = false) { + super.addPointToPath(x, y); + + const elementsToEraser = this.updateElementsToBeErased(restore); + + return elementsToEraser; + } + + private updateElementsToBeErased(restoreToErase?: boolean) { + let eraserPath: GlobalPoint[] = + super + .getCurrentTrail() + ?.originalPoints?.map((p) => pointFrom(p[0], p[1])) || []; + + // for efficiency and avoid unnecessary calculations, + // take only POINTS_ON_TRAIL points to form some number of segments + eraserPath = eraserPath?.slice(eraserPath.length - POINTS_ON_TRAIL); + + const visibleElementsMap = arrayToMap(this.app.visibleElements); + + const pathSegments = eraserPath.reduce((acc, point, index) => { + if (index === 0) { + return acc; + } + acc.push(lineSegment(eraserPath[index - 1], point)); + return acc; + }, [] as LineSegment[]); + + if (pathSegments.length === 0) { + return []; + } + + for (const element of this.app.visibleElements) { + // restore only if already added to the to-be-erased set + if (restoreToErase && this.elementsToErase.has(element.id)) { + const intersects = eraserTest( + pathSegments, + element, + this.segmentsCache, + this.geometricShapesCache, + visibleElementsMap, + this.app, + ); + + if (intersects) { + const shallowestGroupId = element.groupIds.at(-1)!; + + if (this.groupsToErase.has(shallowestGroupId)) { + const elementsInGroup = getElementsInGroup( + this.app.scene.getNonDeletedElementsMap(), + shallowestGroupId, + ); + for (const elementInGroup of elementsInGroup) { + this.elementsToErase.delete(elementInGroup.id); + } + this.groupsToErase.delete(shallowestGroupId); + } + + if (isBoundToContainer(element)) { + this.elementsToErase.delete(element.containerId); + } + + if (hasBoundTextElement(element)) { + const boundText = getBoundTextElementId(element); + + if (boundText) { + this.elementsToErase.delete(boundText); + } + } + + this.elementsToErase.delete(element.id); + } + } else if (!restoreToErase && !this.elementsToErase.has(element.id)) { + const intersects = eraserTest( + pathSegments, + element, + this.segmentsCache, + this.geometricShapesCache, + visibleElementsMap, + this.app, + ); + + if (intersects) { + const shallowestGroupId = element.groupIds.at(-1)!; + + if (!this.groupsToErase.has(shallowestGroupId)) { + const elementsInGroup = getElementsInGroup( + this.app.scene.getNonDeletedElementsMap(), + shallowestGroupId, + ); + + for (const elementInGroup of elementsInGroup) { + this.elementsToErase.add(elementInGroup.id); + } + this.groupsToErase.add(shallowestGroupId); + } + + if (hasBoundTextElement(element)) { + const boundText = getBoundTextElementId(element); + + if (boundText) { + this.elementsToErase.add(boundText); + } + } + + if (isBoundToContainer(element)) { + this.elementsToErase.add(element.containerId); + } + + this.elementsToErase.add(element.id); + } + } + } + + return Array.from(this.elementsToErase); + } + + endPath(): void { + super.endPath(); + super.clearTrails(); + this.elementsToErase.clear(); + this.groupsToErase.clear(); + this.segmentsCache.clear(); + } +} + +const eraserTest = ( + pathSegments: LineSegment[], + element: ExcalidrawElement, + elementsSegments: ElementsSegmentsMap, + shapesCache = new Map>(), + visibleElementsMap = new Map(), + app: App, +): boolean => { + let shape = shapesCache.get(element.id); + + if (!shape) { + shape = getElementShape(element, visibleElementsMap); + shapesCache.set(element.id, shape); + } + + const lastPoint = pathSegments[pathSegments.length - 1][1]; + if (shouldTestInside(element) && isPointInShape(lastPoint, shape)) { + return true; + } + + let elementSegments = elementsSegments.get(element.id); + + if (!elementSegments) { + elementSegments = getElementLineSegments(element, visibleElementsMap); + elementsSegments.set(element.id, elementSegments); + } + + return pathSegments.some((pathSegment) => + elementSegments?.some( + (elementSegment) => + lineSegmentIntersectionPoints( + pathSegment, + elementSegment, + app.getElementHitThreshold(), + ) !== null, + ), + ); +}; diff --git a/packages/excalidraw/lasso/utils.ts b/packages/excalidraw/lasso/utils.ts index f5a7eefdc..d05f39998 100644 --- a/packages/excalidraw/lasso/utils.ts +++ b/packages/excalidraw/lasso/utils.ts @@ -7,11 +7,13 @@ import { polygonIncludesPointNonZero, } from "@excalidraw/math"; -import type { GlobalPoint, LineSegment } from "@excalidraw/math/types"; +import type { + ElementsSegmentsMap, + GlobalPoint, + LineSegment, +} from "@excalidraw/math/types"; import type { ExcalidrawElement } from "@excalidraw/element/types"; -export type ElementsSegmentsMap = Map[]>; - export const getLassoSelectedElementIds = (input: { lassoPath: GlobalPoint[]; elements: readonly ExcalidrawElement[]; diff --git a/packages/excalidraw/tests/lasso.test.tsx b/packages/excalidraw/tests/lasso.test.tsx index 00e0ec2b4..aa32b13d6 100644 --- a/packages/excalidraw/tests/lasso.test.tsx +++ b/packages/excalidraw/tests/lasso.test.tsx @@ -19,6 +19,7 @@ import { type LocalPoint, pointFrom, type Radians, + type ElementsSegmentsMap, } from "@excalidraw/math"; import { getElementLineSegments } from "@excalidraw/element/bounds"; @@ -33,8 +34,6 @@ import { getLassoSelectedElementIds } from "../lasso/utils"; import { act, render } from "./test-utils"; -import type { ElementsSegmentsMap } from "../lasso/utils"; - const { h } = window; beforeEach(async () => { diff --git a/packages/math/src/types.ts b/packages/math/src/types.ts index a2a575bd7..da7d5d6ab 100644 --- a/packages/math/src/types.ts +++ b/packages/math/src/types.ts @@ -138,3 +138,5 @@ export type Ellipse = { } & { _brand: "excalimath_ellipse"; }; + +export type ElementsSegmentsMap = Map[]>; From 0cf36d6b30fc84085aa903460a04d0d568518f30 Mon Sep 17 00:00:00 2001 From: David Luzar <5153846+dwelle@users.noreply.github.com> Date: Wed, 16 Apr 2025 10:28:56 +0200 Subject: [PATCH 05/15] fix: erasing locked elements (#9400) * fix: erasing locked elements * signature tweaks --- packages/excalidraw/eraser/index.ts | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/packages/excalidraw/eraser/index.ts b/packages/excalidraw/eraser/index.ts index a9f9103f5..2ea668aef 100644 --- a/packages/excalidraw/eraser/index.ts +++ b/packages/excalidraw/eraser/index.ts @@ -23,7 +23,7 @@ import type { GlobalPoint, LineSegment, } from "@excalidraw/math/types"; -import type { ExcalidrawElement } from "@excalidraw/element/types"; +import type { ElementsMap, ExcalidrawElement } from "@excalidraw/element/types"; import { AnimatedTrail } from "../animated-trail"; @@ -91,7 +91,11 @@ export class EraserTrail extends AnimatedTrail { // take only POINTS_ON_TRAIL points to form some number of segments eraserPath = eraserPath?.slice(eraserPath.length - POINTS_ON_TRAIL); - const visibleElementsMap = arrayToMap(this.app.visibleElements); + const candidateElements = this.app.visibleElements.filter( + (el) => !el.locked, + ); + + const candidateElementsMap = arrayToMap(candidateElements); const pathSegments = eraserPath.reduce((acc, point, index) => { if (index === 0) { @@ -105,7 +109,7 @@ export class EraserTrail extends AnimatedTrail { return []; } - for (const element of this.app.visibleElements) { + for (const element of candidateElements) { // restore only if already added to the to-be-erased set if (restoreToErase && this.elementsToErase.has(element.id)) { const intersects = eraserTest( @@ -113,7 +117,7 @@ export class EraserTrail extends AnimatedTrail { element, this.segmentsCache, this.geometricShapesCache, - visibleElementsMap, + candidateElementsMap, this.app, ); @@ -151,7 +155,7 @@ export class EraserTrail extends AnimatedTrail { element, this.segmentsCache, this.geometricShapesCache, - visibleElementsMap, + candidateElementsMap, this.app, ); @@ -203,14 +207,14 @@ const eraserTest = ( pathSegments: LineSegment[], element: ExcalidrawElement, elementsSegments: ElementsSegmentsMap, - shapesCache = new Map>(), - visibleElementsMap = new Map(), + shapesCache: Map>, + elementsMap: ElementsMap, app: App, ): boolean => { let shape = shapesCache.get(element.id); if (!shape) { - shape = getElementShape(element, visibleElementsMap); + shape = getElementShape(element, elementsMap); shapesCache.set(element.id, shape); } @@ -222,7 +226,7 @@ const eraserTest = ( let elementSegments = elementsSegments.get(element.id); if (!elementSegments) { - elementSegments = getElementLineSegments(element, visibleElementsMap); + elementSegments = getElementLineSegments(element, elementsMap); elementsSegments.set(element.id, elementSegments); } From a5d69398266a993e590c020d5495e2e1b2f271e3 Mon Sep 17 00:00:00 2001 From: David Luzar <5153846+dwelle@users.noreply.github.com> Date: Thu, 17 Apr 2025 16:08:07 +0200 Subject: [PATCH 06/15] fix: keep orig elem in place on alt-duplication (#9403) * fix: keep orig elem in place on alt-duplication * clarify comment * fix: incorrect selection on duplicating labeled containers * fix: duplicating within group outside frame should remove from group --- packages/element/src/binding.ts | 244 ++---------------- packages/element/src/duplicate.ts | 137 +++++----- packages/element/src/frame.ts | 30 +-- packages/element/src/selection.ts | 54 +++- packages/element/tests/duplicate.test.tsx | 225 ++++++++++++---- .../actions/actionDuplicateSelection.tsx | 101 +++----- packages/excalidraw/components/App.tsx | 160 ++++++++---- .../components/LibraryMenuItems.tsx | 2 +- .../tests/__snapshots__/move.test.tsx.snap | 72 +++--- .../regressionTests.test.tsx.snap | 114 ++++---- packages/excalidraw/tests/clipboard.test.tsx | 35 +++ .../excalidraw/tests/cropElement.test.tsx | 2 +- packages/excalidraw/types.ts | 6 +- 13 files changed, 603 insertions(+), 579 deletions(-) diff --git a/packages/element/src/binding.ts b/packages/element/src/binding.ts index 5c32e8c81..1d39ce2f0 100644 --- a/packages/element/src/binding.ts +++ b/packages/element/src/binding.ts @@ -56,7 +56,6 @@ import { getBoundTextElement, handleBindTextResize } from "./textElement"; import { isArrowElement, isBindableElement, - isBindingElement, isBoundToContainer, isElbowArrow, isFixedPointBinding, @@ -1409,19 +1408,19 @@ const getLinearElementEdgeCoors = ( }; export const fixDuplicatedBindingsAfterDuplication = ( - newElements: ExcalidrawElement[], - oldIdToDuplicatedId: Map, - duplicatedElementsMap: NonDeletedSceneElementsMap, + duplicatedElements: ExcalidrawElement[], + origIdToDuplicateId: Map, + duplicateElementsMap: NonDeletedSceneElementsMap, ) => { - for (const element of newElements) { - if ("boundElements" in element && element.boundElements) { - Object.assign(element, { - boundElements: element.boundElements.reduce( + for (const duplicateElement of duplicatedElements) { + if ("boundElements" in duplicateElement && duplicateElement.boundElements) { + Object.assign(duplicateElement, { + boundElements: duplicateElement.boundElements.reduce( ( acc: Mutable>, binding, ) => { - const newBindingId = oldIdToDuplicatedId.get(binding.id); + const newBindingId = origIdToDuplicateId.get(binding.id); if (newBindingId) { acc.push({ ...binding, id: newBindingId }); } @@ -1432,46 +1431,47 @@ export const fixDuplicatedBindingsAfterDuplication = ( }); } - if ("containerId" in element && element.containerId) { - Object.assign(element, { - containerId: oldIdToDuplicatedId.get(element.containerId) ?? null, + if ("containerId" in duplicateElement && duplicateElement.containerId) { + Object.assign(duplicateElement, { + containerId: + origIdToDuplicateId.get(duplicateElement.containerId) ?? null, }); } - if ("endBinding" in element && element.endBinding) { - const newEndBindingId = oldIdToDuplicatedId.get( - element.endBinding.elementId, + if ("endBinding" in duplicateElement && duplicateElement.endBinding) { + const newEndBindingId = origIdToDuplicateId.get( + duplicateElement.endBinding.elementId, ); - Object.assign(element, { + Object.assign(duplicateElement, { endBinding: newEndBindingId ? { - ...element.endBinding, + ...duplicateElement.endBinding, elementId: newEndBindingId, } : null, }); } - if ("startBinding" in element && element.startBinding) { - const newEndBindingId = oldIdToDuplicatedId.get( - element.startBinding.elementId, + if ("startBinding" in duplicateElement && duplicateElement.startBinding) { + const newEndBindingId = origIdToDuplicateId.get( + duplicateElement.startBinding.elementId, ); - Object.assign(element, { + Object.assign(duplicateElement, { startBinding: newEndBindingId ? { - ...element.startBinding, + ...duplicateElement.startBinding, elementId: newEndBindingId, } : null, }); } - if (isElbowArrow(element)) { + if (isElbowArrow(duplicateElement)) { Object.assign( - element, - updateElbowArrowPoints(element, duplicatedElementsMap, { + duplicateElement, + updateElbowArrowPoints(duplicateElement, duplicateElementsMap, { points: [ - element.points[0], - element.points[element.points.length - 1], + duplicateElement.points[0], + duplicateElement.points[duplicateElement.points.length - 1], ], }), ); @@ -1479,196 +1479,6 @@ export const fixDuplicatedBindingsAfterDuplication = ( } }; -const fixReversedBindingsForBindables = ( - original: ExcalidrawBindableElement, - duplicate: ExcalidrawBindableElement, - originalElements: Map, - elementsWithClones: ExcalidrawElement[], - oldIdToDuplicatedId: Map, -) => { - original.boundElements?.forEach((binding, idx) => { - if (binding.type !== "arrow") { - return; - } - - const oldArrow = elementsWithClones.find((el) => el.id === binding.id); - - if (!isBindingElement(oldArrow)) { - return; - } - - if (originalElements.has(binding.id)) { - // Linked arrow is in the selection, so find the duplicate pair - const newArrowId = oldIdToDuplicatedId.get(binding.id) ?? binding.id; - const newArrow = elementsWithClones.find( - (el) => el.id === newArrowId, - )! as ExcalidrawArrowElement; - - mutateElement(newArrow, { - startBinding: - oldArrow.startBinding?.elementId === binding.id - ? { - ...oldArrow.startBinding, - elementId: duplicate.id, - } - : newArrow.startBinding, - endBinding: - oldArrow.endBinding?.elementId === binding.id - ? { - ...oldArrow.endBinding, - elementId: duplicate.id, - } - : newArrow.endBinding, - }); - mutateElement(duplicate, { - boundElements: [ - ...(duplicate.boundElements ?? []).filter( - (el) => el.id !== binding.id && el.id !== newArrowId, - ), - { - type: "arrow", - id: newArrowId, - }, - ], - }); - } else { - // Linked arrow is outside the selection, - // so we move the binding to the duplicate - mutateElement(oldArrow, { - startBinding: - oldArrow.startBinding?.elementId === original.id - ? { - ...oldArrow.startBinding, - elementId: duplicate.id, - } - : oldArrow.startBinding, - endBinding: - oldArrow.endBinding?.elementId === original.id - ? { - ...oldArrow.endBinding, - elementId: duplicate.id, - } - : oldArrow.endBinding, - }); - mutateElement(duplicate, { - boundElements: [ - ...(duplicate.boundElements ?? []), - { - type: "arrow", - id: oldArrow.id, - }, - ], - }); - mutateElement(original, { - boundElements: - original.boundElements?.filter((_, i) => i !== idx) ?? null, - }); - } - }); -}; - -const fixReversedBindingsForArrows = ( - original: ExcalidrawArrowElement, - duplicate: ExcalidrawArrowElement, - originalElements: Map, - bindingProp: "startBinding" | "endBinding", - oldIdToDuplicatedId: Map, - elementsWithClones: ExcalidrawElement[], -) => { - const oldBindableId = original[bindingProp]?.elementId; - - if (oldBindableId) { - if (originalElements.has(oldBindableId)) { - // Linked element is in the selection - const newBindableId = - oldIdToDuplicatedId.get(oldBindableId) ?? oldBindableId; - const newBindable = elementsWithClones.find( - (el) => el.id === newBindableId, - ) as ExcalidrawBindableElement; - mutateElement(duplicate, { - [bindingProp]: { - ...original[bindingProp], - elementId: newBindableId, - }, - }); - mutateElement(newBindable, { - boundElements: [ - ...(newBindable.boundElements ?? []).filter( - (el) => el.id !== original.id && el.id !== duplicate.id, - ), - { - id: duplicate.id, - type: "arrow", - }, - ], - }); - } else { - // Linked element is outside the selection - const originalBindable = elementsWithClones.find( - (el) => el.id === oldBindableId, - ); - if (originalBindable) { - mutateElement(duplicate, { - [bindingProp]: original[bindingProp], - }); - mutateElement(original, { - [bindingProp]: null, - }); - mutateElement(originalBindable, { - boundElements: [ - ...(originalBindable.boundElements?.filter( - (el) => el.id !== original.id, - ) ?? []), - { - id: duplicate.id, - type: "arrow", - }, - ], - }); - } - } - } -}; - -export const fixReversedBindings = ( - originalElements: Map, - elementsWithClones: ExcalidrawElement[], - oldIdToDuplicatedId: Map, -) => { - for (const original of originalElements.values()) { - const duplicate = elementsWithClones.find( - (el) => el.id === oldIdToDuplicatedId.get(original.id), - )!; - - if (isBindableElement(original) && isBindableElement(duplicate)) { - fixReversedBindingsForBindables( - original, - duplicate, - originalElements, - elementsWithClones, - oldIdToDuplicatedId, - ); - } else if (isArrowElement(original) && isArrowElement(duplicate)) { - fixReversedBindingsForArrows( - original, - duplicate, - originalElements, - "startBinding", - oldIdToDuplicatedId, - elementsWithClones, - ); - fixReversedBindingsForArrows( - original, - duplicate, - originalElements, - "endBinding", - oldIdToDuplicatedId, - elementsWithClones, - ); - } - } -}; - export const fixBindingsAfterDeletion = ( sceneElements: readonly ExcalidrawElement[], deletedElements: readonly ExcalidrawElement[], diff --git a/packages/element/src/duplicate.ts b/packages/element/src/duplicate.ts index ded1fd26c..c2cee4c08 100644 --- a/packages/element/src/duplicate.ts +++ b/packages/element/src/duplicate.ts @@ -36,10 +36,7 @@ import { import { getBoundTextElement, getContainerElement } from "./textElement"; -import { - fixDuplicatedBindingsAfterDuplication, - fixReversedBindings, -} from "./binding"; +import { fixDuplicatedBindingsAfterDuplication } from "./binding"; import type { ElementsMap, @@ -60,16 +57,14 @@ import type { * multiple elements at once, share this map * amongst all of them * @param element Element to duplicate - * @param overrides Any element properties to override */ export const duplicateElement = ( editingGroupId: AppState["editingGroupId"], groupIdMapForOperation: Map, element: TElement, - overrides?: Partial, randomizeSeed?: boolean, ): Readonly => { - let copy = deepCopyElement(element); + const copy = deepCopyElement(element); if (isTestEnv()) { __test__defineOrigId(copy, element.id); @@ -92,9 +87,6 @@ export const duplicateElement = ( return groupIdMapForOperation.get(groupId)!; }, ); - if (overrides) { - copy = Object.assign(copy, overrides); - } return copy; }; @@ -102,9 +94,14 @@ export const duplicateElements = ( opts: { elements: readonly ExcalidrawElement[]; randomizeSeed?: boolean; - overrides?: ( - originalElement: ExcalidrawElement, - ) => Partial; + overrides?: (data: { + duplicateElement: ExcalidrawElement; + origElement: ExcalidrawElement; + origIdToDuplicateId: Map< + ExcalidrawElement["id"], + ExcalidrawElement["id"] + >; + }) => Partial; } & ( | { /** @@ -132,14 +129,6 @@ export const duplicateElements = ( editingGroupId: AppState["editingGroupId"]; selectedGroupIds: AppState["selectedGroupIds"]; }; - /** - * If true, duplicated elements are inserted _before_ specified - * elements. Case: alt-dragging elements to duplicate them. - * - * TODO: remove this once (if) we stop replacing the original element - * with the duplicated one in the scene array. - */ - reverseOrder: boolean; } ), ) => { @@ -153,8 +142,6 @@ export const duplicateElements = ( selectedGroupIds: {}, } as const); - const reverseOrder = opts.type === "in-place" ? opts.reverseOrder : false; - // Ids of elements that have already been processed so we don't push them // into the array twice if we end up backtracking when retrieving // discontiguous group of elements (can happen due to a bug, or in edge @@ -167,10 +154,17 @@ export const duplicateElements = ( // loop over them. const processedIds = new Map(); const groupIdMap = new Map(); - const newElements: ExcalidrawElement[] = []; - const oldElements: ExcalidrawElement[] = []; - const oldIdToDuplicatedId = new Map(); - const duplicatedElementsMap = new Map(); + const duplicatedElements: ExcalidrawElement[] = []; + const origElements: ExcalidrawElement[] = []; + const origIdToDuplicateId = new Map< + ExcalidrawElement["id"], + ExcalidrawElement["id"] + >(); + const duplicateIdToOrigElement = new Map< + ExcalidrawElement["id"], + ExcalidrawElement + >(); + const duplicateElementsMap = new Map(); const elementsMap = arrayToMap(elements) as ElementsMap; const _idsOfElementsToDuplicate = opts.type === "in-place" @@ -188,7 +182,7 @@ export const duplicateElements = ( elements = normalizeElementOrder(elements); - const elementsWithClones: ExcalidrawElement[] = elements.slice(); + const elementsWithDuplicates: ExcalidrawElement[] = elements.slice(); // helper functions // ------------------------------------------------------------------------- @@ -214,17 +208,17 @@ export const duplicateElements = ( appState.editingGroupId, groupIdMap, element, - opts.overrides?.(element), opts.randomizeSeed, ); processedIds.set(newElement.id, true); - duplicatedElementsMap.set(newElement.id, newElement); - oldIdToDuplicatedId.set(element.id, newElement.id); + duplicateElementsMap.set(newElement.id, newElement); + origIdToDuplicateId.set(element.id, newElement.id); + duplicateIdToOrigElement.set(newElement.id, element); - oldElements.push(element); - newElements.push(newElement); + origElements.push(element); + duplicatedElements.push(newElement); acc.push(newElement); return acc; @@ -248,21 +242,12 @@ export const duplicateElements = ( return; } - if (reverseOrder && index < 1) { - elementsWithClones.unshift(...castArray(elements)); + if (index > elementsWithDuplicates.length - 1) { + elementsWithDuplicates.push(...castArray(elements)); return; } - if (!reverseOrder && index > elementsWithClones.length - 1) { - elementsWithClones.push(...castArray(elements)); - return; - } - - elementsWithClones.splice( - index + (reverseOrder ? 0 : 1), - 0, - ...castArray(elements), - ); + elementsWithDuplicates.splice(index + 1, 0, ...castArray(elements)); }; const frameIdsToDuplicate = new Set( @@ -294,13 +279,9 @@ export const duplicateElements = ( : [element], ); - const targetIndex = reverseOrder - ? elementsWithClones.findIndex((el) => { - return el.groupIds?.includes(groupId); - }) - : findLastIndex(elementsWithClones, (el) => { - return el.groupIds?.includes(groupId); - }); + const targetIndex = findLastIndex(elementsWithDuplicates, (el) => { + return el.groupIds?.includes(groupId); + }); insertBeforeOrAfterIndex(targetIndex, copyElements(groupElements)); continue; @@ -318,7 +299,7 @@ export const duplicateElements = ( const frameChildren = getFrameChildren(elements, frameId); - const targetIndex = findLastIndex(elementsWithClones, (el) => { + const targetIndex = findLastIndex(elementsWithDuplicates, (el) => { return el.frameId === frameId || el.id === frameId; }); @@ -335,7 +316,7 @@ export const duplicateElements = ( if (hasBoundTextElement(element)) { const boundTextElement = getBoundTextElement(element, elementsMap); - const targetIndex = findLastIndex(elementsWithClones, (el) => { + const targetIndex = findLastIndex(elementsWithDuplicates, (el) => { return ( el.id === element.id || ("containerId" in el && el.containerId === element.id) @@ -344,7 +325,7 @@ export const duplicateElements = ( if (boundTextElement) { insertBeforeOrAfterIndex( - targetIndex + (reverseOrder ? -1 : 0), + targetIndex, copyElements([element, boundTextElement]), ); } else { @@ -357,7 +338,7 @@ export const duplicateElements = ( if (isBoundToContainer(element)) { const container = getContainerElement(element, elementsMap); - const targetIndex = findLastIndex(elementsWithClones, (el) => { + const targetIndex = findLastIndex(elementsWithDuplicates, (el) => { return el.id === element.id || el.id === container?.id; }); @@ -377,7 +358,7 @@ export const duplicateElements = ( // ------------------------------------------------------------------------- insertBeforeOrAfterIndex( - findLastIndex(elementsWithClones, (el) => el.id === element.id), + findLastIndex(elementsWithDuplicates, (el) => el.id === element.id), copyElements(element), ); } @@ -385,28 +366,38 @@ export const duplicateElements = ( // --------------------------------------------------------------------------- fixDuplicatedBindingsAfterDuplication( - newElements, - oldIdToDuplicatedId, - duplicatedElementsMap as NonDeletedSceneElementsMap, + duplicatedElements, + origIdToDuplicateId, + duplicateElementsMap as NonDeletedSceneElementsMap, ); - if (reverseOrder) { - fixReversedBindings( - _idsOfElementsToDuplicate, - elementsWithClones, - oldIdToDuplicatedId, - ); - } - bindElementsToFramesAfterDuplication( - elementsWithClones, - oldElements, - oldIdToDuplicatedId, + elementsWithDuplicates, + origElements, + origIdToDuplicateId, ); + if (opts.overrides) { + for (const duplicateElement of duplicatedElements) { + const origElement = duplicateIdToOrigElement.get(duplicateElement.id); + if (origElement) { + Object.assign( + duplicateElement, + opts.overrides({ + duplicateElement, + origElement, + origIdToDuplicateId, + }), + ); + } + } + } + return { - newElements, - elementsWithClones, + duplicatedElements, + duplicateElementsMap, + elementsWithDuplicates, + origIdToDuplicateId, }; }; diff --git a/packages/element/src/frame.ts b/packages/element/src/frame.ts index 7f40148b7..664c8d98f 100644 --- a/packages/element/src/frame.ts +++ b/packages/element/src/frame.ts @@ -41,30 +41,28 @@ import type { // --------------------------- Frame State ------------------------------------ export const bindElementsToFramesAfterDuplication = ( nextElements: readonly ExcalidrawElement[], - oldElements: readonly ExcalidrawElement[], - oldIdToDuplicatedId: Map, + origElements: readonly ExcalidrawElement[], + origIdToDuplicateId: Map, ) => { const nextElementMap = arrayToMap(nextElements) as Map< ExcalidrawElement["id"], ExcalidrawElement >; - for (const element of oldElements) { + for (const element of origElements) { if (element.frameId) { // use its frameId to get the new frameId - const nextElementId = oldIdToDuplicatedId.get(element.id); - const nextFrameId = oldIdToDuplicatedId.get(element.frameId); - if (nextElementId) { - const nextElement = nextElementMap.get(nextElementId); - if (nextElement) { - mutateElement( - nextElement, - { - frameId: nextFrameId ?? element.frameId, - }, - false, - ); - } + const nextElementId = origIdToDuplicateId.get(element.id); + const nextFrameId = origIdToDuplicateId.get(element.frameId); + const nextElement = nextElementId && nextElementMap.get(nextElementId); + if (nextElement) { + mutateElement( + nextElement, + { + frameId: nextFrameId ?? null, + }, + false, + ); } } } diff --git a/packages/element/src/selection.ts b/packages/element/src/selection.ts index a76703920..e07c96d38 100644 --- a/packages/element/src/selection.ts +++ b/packages/element/src/selection.ts @@ -7,13 +7,20 @@ import type { import { getElementAbsoluteCoords, getElementBounds } from "./bounds"; import { isElementInViewport } from "./sizeHelpers"; -import { isBoundToContainer, isFrameLikeElement } from "./typeChecks"; +import { + isBoundToContainer, + isFrameLikeElement, + isLinearElement, +} from "./typeChecks"; import { elementOverlapsWithFrame, getContainingFrame, getFrameChildren, } from "./frame"; +import { LinearElementEditor } from "./linearElementEditor"; +import { selectGroupsForSelectedElements } from "./groups"; + import type { ElementsMap, ElementsMapOrArray, @@ -254,3 +261,48 @@ export const makeNextSelectedElementIds = ( return nextSelectedElementIds; }; + +const _getLinearElementEditor = ( + targetElements: readonly ExcalidrawElement[], +) => { + const linears = targetElements.filter(isLinearElement); + if (linears.length === 1) { + const linear = linears[0]; + const boundElements = linear.boundElements?.map((def) => def.id) ?? []; + const onlySingleLinearSelected = targetElements.every( + (el) => el.id === linear.id || boundElements.includes(el.id), + ); + + if (onlySingleLinearSelected) { + return new LinearElementEditor(linear); + } + } + + return null; +}; + +export const getSelectionStateForElements = ( + targetElements: readonly ExcalidrawElement[], + allElements: readonly NonDeletedExcalidrawElement[], + appState: AppState, +) => { + return { + selectedLinearElement: _getLinearElementEditor(targetElements), + ...selectGroupsForSelectedElements( + { + editingGroupId: appState.editingGroupId, + selectedElementIds: excludeElementsInFramesFromSelection( + targetElements, + ).reduce((acc: Record, element) => { + if (!isBoundToContainer(element)) { + acc[element.id] = true; + } + return acc; + }, {}), + }, + allElements, + appState, + null, + ), + }; +}; diff --git a/packages/element/tests/duplicate.test.tsx b/packages/element/tests/duplicate.test.tsx index 7492bcc58..409200b6e 100644 --- a/packages/element/tests/duplicate.test.tsx +++ b/packages/element/tests/duplicate.test.tsx @@ -67,7 +67,7 @@ describe("duplicating single elements", () => { points: [pointFrom(1, 2), pointFrom(3, 4)], }); - const copy = duplicateElement(null, new Map(), element, undefined, true); + const copy = duplicateElement(null, new Map(), element, true); assertCloneObjects(element, copy); @@ -173,7 +173,7 @@ describe("duplicating multiple elements", () => { // ------------------------------------------------------------------------- const origElements = [rectangle1, text1, arrow1, arrow2, text2] as const; - const { newElements: clonedElements } = duplicateElements({ + const { duplicatedElements } = duplicateElements({ type: "everything", elements: origElements, }); @@ -181,10 +181,10 @@ describe("duplicating multiple elements", () => { // generic id in-equality checks // -------------------------------------------------------------------------- expect(origElements.map((e) => e.type)).toEqual( - clonedElements.map((e) => e.type), + duplicatedElements.map((e) => e.type), ); origElements.forEach((origElement, idx) => { - const clonedElement = clonedElements[idx]; + const clonedElement = duplicatedElements[idx]; expect(origElement).toEqual( expect.objectContaining({ id: expect.not.stringMatching(clonedElement.id), @@ -217,12 +217,12 @@ describe("duplicating multiple elements", () => { }); // -------------------------------------------------------------------------- - const clonedArrows = clonedElements.filter( + const clonedArrows = duplicatedElements.filter( (e) => e.type === "arrow", ) as ExcalidrawLinearElement[]; const [clonedRectangle, clonedText1, , clonedArrow2, clonedArrowLabel] = - clonedElements as any as typeof origElements; + duplicatedElements as any as typeof origElements; expect(clonedText1.containerId).toBe(clonedRectangle.id); expect( @@ -327,10 +327,10 @@ describe("duplicating multiple elements", () => { // ------------------------------------------------------------------------- const origElements = [rectangle1, text1, arrow1, arrow2, arrow3] as const; - const { newElements: clonedElements } = duplicateElements({ + const duplicatedElements = duplicateElements({ type: "everything", elements: origElements, - }) as any as { newElements: typeof origElements }; + }).duplicatedElements as any as typeof origElements; const [ clonedRectangle, @@ -338,7 +338,7 @@ describe("duplicating multiple elements", () => { clonedArrow1, clonedArrow2, clonedArrow3, - ] = clonedElements; + ] = duplicatedElements; expect(clonedRectangle.boundElements).toEqual([ { id: clonedArrow1.id, type: "arrow" }, @@ -374,12 +374,12 @@ describe("duplicating multiple elements", () => { }); const origElements = [rectangle1, rectangle2, rectangle3] as const; - const { newElements: clonedElements } = duplicateElements({ + const { duplicatedElements } = duplicateElements({ type: "everything", elements: origElements, - }) as any as { newElements: typeof origElements }; + }); const [clonedRectangle1, clonedRectangle2, clonedRectangle3] = - clonedElements; + duplicatedElements; expect(rectangle1.groupIds[0]).not.toBe(clonedRectangle1.groupIds[0]); expect(rectangle2.groupIds[0]).not.toBe(clonedRectangle2.groupIds[0]); @@ -399,7 +399,7 @@ describe("duplicating multiple elements", () => { }); const { - newElements: [clonedRectangle1], + duplicatedElements: [clonedRectangle1], } = duplicateElements({ type: "everything", elements: [rectangle1] }); expect(typeof clonedRectangle1.groupIds[0]).toBe("string"); @@ -408,6 +408,117 @@ describe("duplicating multiple elements", () => { }); }); +describe("group-related duplication", () => { + beforeEach(async () => { + await render(); + }); + + it("action-duplicating within group", async () => { + const rectangle1 = API.createElement({ + type: "rectangle", + x: 0, + y: 0, + groupIds: ["group1"], + }); + const rectangle2 = API.createElement({ + type: "rectangle", + x: 10, + y: 10, + groupIds: ["group1"], + }); + + API.setElements([rectangle1, rectangle2]); + API.setSelectedElements([rectangle2], "group1"); + + act(() => { + h.app.actionManager.executeAction(actionDuplicateSelection); + }); + + assertElements(h.elements, [ + { id: rectangle1.id }, + { id: rectangle2.id }, + { [ORIG_ID]: rectangle2.id, selected: true, groupIds: ["group1"] }, + ]); + expect(h.state.editingGroupId).toBe("group1"); + }); + + it("alt-duplicating within group", async () => { + const rectangle1 = API.createElement({ + type: "rectangle", + x: 0, + y: 0, + groupIds: ["group1"], + }); + const rectangle2 = API.createElement({ + type: "rectangle", + x: 10, + y: 10, + groupIds: ["group1"], + }); + + API.setElements([rectangle1, rectangle2]); + API.setSelectedElements([rectangle2], "group1"); + + Keyboard.withModifierKeys({ alt: true }, () => { + mouse.down(rectangle2.x + 5, rectangle2.y + 5); + mouse.up(rectangle2.x + 50, rectangle2.y + 50); + }); + + assertElements(h.elements, [ + { id: rectangle1.id }, + { id: rectangle2.id }, + { [ORIG_ID]: rectangle2.id, selected: true, groupIds: ["group1"] }, + ]); + expect(h.state.editingGroupId).toBe("group1"); + }); + + it.skip("alt-duplicating within group away outside frame", () => { + const frame = API.createElement({ + type: "frame", + x: 0, + y: 0, + width: 100, + height: 100, + }); + const rectangle1 = API.createElement({ + type: "rectangle", + x: 0, + y: 0, + width: 50, + height: 50, + groupIds: ["group1"], + frameId: frame.id, + }); + const rectangle2 = API.createElement({ + type: "rectangle", + x: 10, + y: 10, + width: 50, + height: 50, + groupIds: ["group1"], + frameId: frame.id, + }); + + API.setElements([frame, rectangle1, rectangle2]); + API.setSelectedElements([rectangle2], "group1"); + + Keyboard.withModifierKeys({ alt: true }, () => { + mouse.down(rectangle2.x + 5, rectangle2.y + 5); + mouse.up(frame.x + frame.width + 50, frame.y + frame.height + 50); + }); + + // console.log(h.elements); + + assertElements(h.elements, [ + { id: frame.id }, + { id: rectangle1.id, frameId: frame.id }, + { id: rectangle2.id, frameId: frame.id }, + { [ORIG_ID]: rectangle2.id, selected: true, groupIds: [], frameId: null }, + ]); + expect(h.state.editingGroupId).toBe(null); + }); +}); + describe("duplication z-order", () => { beforeEach(async () => { await render(); @@ -503,8 +614,8 @@ describe("duplication z-order", () => { }); assertElements(h.elements, [ - { [ORIG_ID]: rectangle1.id }, - { id: rectangle1.id, selected: true }, + { id: rectangle1.id }, + { [ORIG_ID]: rectangle1.id, selected: true }, { id: rectangle2.id }, { id: rectangle3.id }, ]); @@ -538,8 +649,8 @@ describe("duplication z-order", () => { assertElements(h.elements, [ { id: rectangle1.id }, { id: rectangle2.id }, - { [ORIG_ID]: rectangle3.id }, - { id: rectangle3.id, selected: true }, + { id: rectangle3.id }, + { [ORIG_ID]: rectangle3.id, selected: true }, ]); }); @@ -569,8 +680,8 @@ describe("duplication z-order", () => { }); assertElements(h.elements, [ - { [ORIG_ID]: rectangle1.id }, - { id: rectangle1.id, selected: true }, + { id: rectangle1.id }, + { [ORIG_ID]: rectangle1.id, selected: true }, { id: rectangle2.id }, { id: rectangle3.id }, ]); @@ -605,19 +716,19 @@ describe("duplication z-order", () => { }); assertElements(h.elements, [ - { [ORIG_ID]: rectangle1.id }, - { [ORIG_ID]: rectangle2.id }, - { [ORIG_ID]: rectangle3.id }, - { id: rectangle1.id, selected: true }, - { id: rectangle2.id, selected: true }, - { id: rectangle3.id, selected: true }, + { id: rectangle1.id }, + { id: rectangle2.id }, + { id: rectangle3.id }, + { [ORIG_ID]: rectangle1.id, selected: true }, + { [ORIG_ID]: rectangle2.id, selected: true }, + { [ORIG_ID]: rectangle3.id, selected: true }, ]); }); - it("reverse-duplicating text container (in-order)", async () => { + it("alt-duplicating text container (in-order)", async () => { const [rectangle, text] = API.createTextContainer(); API.setElements([rectangle, text]); - API.setSelectedElements([rectangle, text]); + API.setSelectedElements([rectangle]); Keyboard.withModifierKeys({ alt: true }, () => { mouse.down(rectangle.x + 5, rectangle.y + 5); @@ -625,20 +736,20 @@ describe("duplication z-order", () => { }); assertElements(h.elements, [ - { [ORIG_ID]: rectangle.id }, + { id: rectangle.id }, + { id: text.id, containerId: rectangle.id }, + { [ORIG_ID]: rectangle.id, selected: true }, { [ORIG_ID]: text.id, containerId: getCloneByOrigId(rectangle.id)?.id, }, - { id: rectangle.id, selected: true }, - { id: text.id, containerId: rectangle.id, selected: true }, ]); }); - it("reverse-duplicating text container (out-of-order)", async () => { + it("alt-duplicating text container (out-of-order)", async () => { const [rectangle, text] = API.createTextContainer(); API.setElements([text, rectangle]); - API.setSelectedElements([rectangle, text]); + API.setSelectedElements([rectangle]); Keyboard.withModifierKeys({ alt: true }, () => { mouse.down(rectangle.x + 5, rectangle.y + 5); @@ -646,21 +757,21 @@ describe("duplication z-order", () => { }); assertElements(h.elements, [ - { [ORIG_ID]: rectangle.id }, + { id: rectangle.id }, + { id: text.id, containerId: rectangle.id }, + { [ORIG_ID]: rectangle.id, selected: true }, { [ORIG_ID]: text.id, containerId: getCloneByOrigId(rectangle.id)?.id, }, - { id: rectangle.id, selected: true }, - { id: text.id, containerId: rectangle.id, selected: true }, ]); }); - it("reverse-duplicating labeled arrows (in-order)", async () => { + it("alt-duplicating labeled arrows (in-order)", async () => { const [arrow, text] = API.createLabeledArrow(); API.setElements([arrow, text]); - API.setSelectedElements([arrow, text]); + API.setSelectedElements([arrow]); Keyboard.withModifierKeys({ alt: true }, () => { mouse.down(arrow.x + 5, arrow.y + 5); @@ -668,21 +779,24 @@ describe("duplication z-order", () => { }); assertElements(h.elements, [ - { [ORIG_ID]: arrow.id }, + { id: arrow.id }, + { id: text.id, containerId: arrow.id }, + { [ORIG_ID]: arrow.id, selected: true }, { [ORIG_ID]: text.id, containerId: getCloneByOrigId(arrow.id)?.id, }, - { id: arrow.id, selected: true }, - { id: text.id, containerId: arrow.id, selected: true }, ]); + expect(h.state.selectedLinearElement).toEqual( + expect.objectContaining({ elementId: getCloneByOrigId(arrow.id)?.id }), + ); }); - it("reverse-duplicating labeled arrows (out-of-order)", async () => { + it("alt-duplicating labeled arrows (out-of-order)", async () => { const [arrow, text] = API.createLabeledArrow(); API.setElements([text, arrow]); - API.setSelectedElements([arrow, text]); + API.setSelectedElements([arrow]); Keyboard.withModifierKeys({ alt: true }, () => { mouse.down(arrow.x + 5, arrow.y + 5); @@ -690,17 +804,17 @@ describe("duplication z-order", () => { }); assertElements(h.elements, [ - { [ORIG_ID]: arrow.id }, + { id: arrow.id }, + { id: text.id, containerId: arrow.id }, + { [ORIG_ID]: arrow.id, selected: true }, { [ORIG_ID]: text.id, containerId: getCloneByOrigId(arrow.id)?.id, }, - { id: arrow.id, selected: true }, - { id: text.id, containerId: arrow.id, selected: true }, ]); }); - it("reverse-duplicating bindable element with bound arrow should keep the arrow on the duplicate", () => { + it("alt-duplicating bindable element with bound arrow should keep the arrow on the duplicate", async () => { const rect = UI.createElement("rectangle", { x: 0, y: 0, @@ -722,11 +836,18 @@ describe("duplication z-order", () => { mouse.up(15, 15); }); - expect(window.h.elements).toHaveLength(3); - - const newRect = window.h.elements[0]; - - expect(arrow.endBinding?.elementId).toBe(newRect.id); - expect(newRect.boundElements?.[0]?.id).toBe(arrow.id); + assertElements(h.elements, [ + { + id: rect.id, + boundElements: expect.arrayContaining([ + expect.objectContaining({ id: arrow.id }), + ]), + }, + { [ORIG_ID]: rect.id, boundElements: [], selected: true }, + { + id: arrow.id, + endBinding: expect.objectContaining({ elementId: rect.id }), + }, + ]); }); }); diff --git a/packages/excalidraw/actions/actionDuplicateSelection.tsx b/packages/excalidraw/actions/actionDuplicateSelection.tsx index 231fe1913..17b452c36 100644 --- a/packages/excalidraw/actions/actionDuplicateSelection.tsx +++ b/packages/excalidraw/actions/actionDuplicateSelection.tsx @@ -7,26 +7,17 @@ import { import { getNonDeletedElements } from "@excalidraw/element"; -import { - isBoundToContainer, - isLinearElement, -} from "@excalidraw/element/typeChecks"; - import { LinearElementEditor } from "@excalidraw/element/linearElementEditor"; -import { selectGroupsForSelectedElements } from "@excalidraw/element/groups"; - import { - excludeElementsInFramesFromSelection, getSelectedElements, + getSelectionStateForElements, } from "@excalidraw/element/selection"; import { syncMovedIndices } from "@excalidraw/element/fractionalIndex"; import { duplicateElements } from "@excalidraw/element/duplicate"; -import type { ExcalidrawElement } from "@excalidraw/element/types"; - import { ToolButton } from "../components/ToolButton"; import { DuplicateIcon } from "../components/icons"; @@ -65,52 +56,49 @@ export const actionDuplicateSelection = register({ } } - let { newElements: duplicatedElements, elementsWithClones: nextElements } = - duplicateElements({ - type: "in-place", - elements, - idsOfElementsToDuplicate: arrayToMap( - getSelectedElements(elements, appState, { - includeBoundTextElement: true, - includeElementsInFrames: true, - }), - ), - appState, - randomizeSeed: true, - overrides: (element) => ({ - x: element.x + DEFAULT_GRID_SIZE / 2, - y: element.y + DEFAULT_GRID_SIZE / 2, + let { duplicatedElements, elementsWithDuplicates } = duplicateElements({ + type: "in-place", + elements, + idsOfElementsToDuplicate: arrayToMap( + getSelectedElements(elements, appState, { + includeBoundTextElement: true, + includeElementsInFrames: true, }), - reverseOrder: false, - }); + ), + appState, + randomizeSeed: true, + overrides: ({ origElement, origIdToDuplicateId }) => { + const duplicateFrameId = + origElement.frameId && origIdToDuplicateId.get(origElement.frameId); + return { + x: origElement.x + DEFAULT_GRID_SIZE / 2, + y: origElement.y + DEFAULT_GRID_SIZE / 2, + frameId: duplicateFrameId ?? origElement.frameId, + }; + }, + }); - if (app.props.onDuplicate && nextElements) { - const mappedElements = app.props.onDuplicate(nextElements, elements); + if (app.props.onDuplicate && elementsWithDuplicates) { + const mappedElements = app.props.onDuplicate( + elementsWithDuplicates, + elements, + ); if (mappedElements) { - nextElements = mappedElements; + elementsWithDuplicates = mappedElements; } } return { - elements: syncMovedIndices(nextElements, arrayToMap(duplicatedElements)), + elements: syncMovedIndices( + elementsWithDuplicates, + arrayToMap(duplicatedElements), + ), appState: { ...appState, - ...updateLinearElementEditors(duplicatedElements), - ...selectGroupsForSelectedElements( - { - editingGroupId: appState.editingGroupId, - selectedElementIds: excludeElementsInFramesFromSelection( - duplicatedElements, - ).reduce((acc: Record, element) => { - if (!isBoundToContainer(element)) { - acc[element.id] = true; - } - return acc; - }, {}), - }, - getNonDeletedElements(nextElements), + ...getSelectionStateForElements( + duplicatedElements, + getNonDeletedElements(elementsWithDuplicates), appState, - null, ), }, captureUpdate: CaptureUpdateAction.IMMEDIATELY, @@ -130,24 +118,3 @@ export const actionDuplicateSelection = register({ /> ), }); - -const updateLinearElementEditors = (clonedElements: ExcalidrawElement[]) => { - const linears = clonedElements.filter(isLinearElement); - if (linears.length === 1) { - const linear = linears[0]; - const boundElements = linear.boundElements?.map((def) => def.id) ?? []; - const onlySingleLinearSelected = clonedElements.every( - (el) => el.id === linear.id || boundElements.includes(el.id), - ); - - if (onlySingleLinearSelected) { - return { - selectedLinearElement: new LinearElementEditor(linear), - }; - } - } - - return { - selectedLinearElement: null, - }; -}; diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index 242fd8e46..6a7306422 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -279,6 +279,7 @@ import { import { excludeElementsInFramesFromSelection, + getSelectionStateForElements, makeNextSelectedElementIds, } from "@excalidraw/element/selection"; @@ -3267,7 +3268,7 @@ class App extends React.Component { const [gridX, gridY] = getGridPoint(dx, dy, this.getEffectiveGridSize()); - const { newElements } = duplicateElements({ + const { duplicatedElements } = duplicateElements({ type: "everything", elements: elements.map((element) => { return newElementWith(element, { @@ -3279,7 +3280,7 @@ class App extends React.Component { }); const prevElements = this.scene.getElementsIncludingDeleted(); - let nextElements = [...prevElements, ...newElements]; + let nextElements = [...prevElements, ...duplicatedElements]; const mappedNewSceneElements = this.props.onDuplicate?.( nextElements, @@ -3288,13 +3289,13 @@ class App extends React.Component { nextElements = mappedNewSceneElements || nextElements; - syncMovedIndices(nextElements, arrayToMap(newElements)); + syncMovedIndices(nextElements, arrayToMap(duplicatedElements)); const topLayerFrame = this.getTopLayerFrameAtSceneCoords({ x, y }); if (topLayerFrame) { const eligibleElements = filterElementsEligibleAsFrameChildren( - newElements, + duplicatedElements, topLayerFrame, ); addElementsToFrame( @@ -3307,7 +3308,7 @@ class App extends React.Component { this.scene.replaceAllElements(nextElements); - newElements.forEach((newElement) => { + duplicatedElements.forEach((newElement) => { if (isTextElement(newElement) && isBoundToContainer(newElement)) { const container = getContainerElement( newElement, @@ -3323,7 +3324,7 @@ class App extends React.Component { // paste event may not fire FontFace loadingdone event in Safari, hence loading font faces manually if (isSafari) { - Fonts.loadElementsFonts(newElements).then((fontFaces) => { + Fonts.loadElementsFonts(duplicatedElements).then((fontFaces) => { this.fonts.onLoaded(fontFaces); }); } @@ -3335,7 +3336,7 @@ class App extends React.Component { this.store.shouldCaptureIncrement(); const nextElementsToSelect = - excludeElementsInFramesFromSelection(newElements); + excludeElementsInFramesFromSelection(duplicatedElements); this.setState( { @@ -3378,7 +3379,7 @@ class App extends React.Component { this.setActiveTool({ type: "selection" }); if (opts.fitToContent) { - this.scrollToContent(newElements, { + this.scrollToContent(duplicatedElements, { fitToContent: true, canvasOffsets: this.getEditorUIOffsets(), }); @@ -6942,6 +6943,7 @@ class App extends React.Component { drag: { hasOccurred: false, offset: null, + origin: { ...origin }, }, eventListeners: { onMove: null, @@ -8236,8 +8238,8 @@ class App extends React.Component { this.state.activeEmbeddable?.state !== "active" ) { const dragOffset = { - x: pointerCoords.x - pointerDownState.origin.x, - y: pointerCoords.y - pointerDownState.origin.y, + x: pointerCoords.x - pointerDownState.drag.origin.x, + y: pointerCoords.y - pointerDownState.drag.origin.y, }; const originalElements = [ @@ -8432,52 +8434,112 @@ class App extends React.Component { selectedElements.map((el) => [el.id, el]), ); - const { newElements: clonedElements, elementsWithClones } = - duplicateElements({ - type: "in-place", - elements, - appState: this.state, - randomizeSeed: true, - idsOfElementsToDuplicate, - overrides: (el) => { - const origEl = pointerDownState.originalElements.get(el.id); - - if (origEl) { - return { - x: origEl.x, - y: origEl.y, - seed: origEl.seed, - }; - } - - return {}; - }, - reverseOrder: true, - }); - clonedElements.forEach((element) => { - pointerDownState.originalElements.set(element.id, element); + const { + duplicatedElements, + duplicateElementsMap, + elementsWithDuplicates, + origIdToDuplicateId, + } = duplicateElements({ + type: "in-place", + elements, + appState: this.state, + randomizeSeed: true, + idsOfElementsToDuplicate, + overrides: ({ duplicateElement, origElement }) => { + return { + // reset to the original element's frameId (unless we've + // duplicated alongside a frame in which case we need to + // keep the duplicate frame's id) so that the element + // frame membership is refreshed on pointerup + // NOTE this is a hacky solution and should be done + // differently + frameId: duplicateElement.frameId ?? origElement.frameId, + seed: randomInteger(), + }; + }, + }); + duplicatedElements.forEach((element) => { + pointerDownState.originalElements.set( + element.id, + deepCopyElement(element), + ); }); - const mappedNewSceneElements = this.props.onDuplicate?.( - elementsWithClones, - elements, - ); - - const nextSceneElements = syncMovedIndices( - mappedNewSceneElements || elementsWithClones, - arrayToMap(clonedElements), - ).map((el) => { + const mappedClonedElements = elementsWithDuplicates.map((el) => { if (idsOfElementsToDuplicate.has(el.id)) { - return newElementWith(el, { - seed: randomInteger(), - }); + const origEl = pointerDownState.originalElements.get(el.id); + + if (origEl) { + return newElementWith(el, { + x: origEl.x, + y: origEl.y, + }); + } } return el; }); - this.scene.replaceAllElements(nextSceneElements); - this.maybeCacheVisibleGaps(event, selectedElements, true); - this.maybeCacheReferenceSnapPoints(event, selectedElements, true); + const mappedNewSceneElements = this.props.onDuplicate?.( + mappedClonedElements, + elements, + ); + + const elementsWithIndices = syncMovedIndices( + mappedNewSceneElements || mappedClonedElements, + arrayToMap(duplicatedElements), + ); + + // we need to update synchronously so as to keep pointerDownState, + // appState, and scene elements in sync + flushSync(() => { + // swap hit element with the duplicated one + if (pointerDownState.hit.element) { + const cloneId = origIdToDuplicateId.get( + pointerDownState.hit.element.id, + ); + const clonedElement = + cloneId && duplicateElementsMap.get(cloneId); + pointerDownState.hit.element = clonedElement || null; + } + // swap hit elements with the duplicated ones + pointerDownState.hit.allHitElements = + pointerDownState.hit.allHitElements.reduce( + ( + acc: typeof pointerDownState.hit.allHitElements, + origHitElement, + ) => { + const cloneId = origIdToDuplicateId.get(origHitElement.id); + const clonedElement = + cloneId && duplicateElementsMap.get(cloneId); + if (clonedElement) { + acc.push(clonedElement); + } + + return acc; + }, + [], + ); + + // update drag origin to the position at which we started + // the duplication so that the drag offset is correct + pointerDownState.drag.origin = viewportCoordsToSceneCoords( + event, + this.state, + ); + + // switch selected elements to the duplicated ones + this.setState((prevState) => ({ + ...getSelectionStateForElements( + duplicatedElements, + this.scene.getNonDeletedElements(), + prevState, + ), + })); + + this.scene.replaceAllElements(elementsWithIndices); + this.maybeCacheVisibleGaps(event, selectedElements, true); + this.maybeCacheReferenceSnapPoints(event, selectedElements, true); + }); } return; diff --git a/packages/excalidraw/components/LibraryMenuItems.tsx b/packages/excalidraw/components/LibraryMenuItems.tsx index f70315953..160cc2640 100644 --- a/packages/excalidraw/components/LibraryMenuItems.tsx +++ b/packages/excalidraw/components/LibraryMenuItems.tsx @@ -166,7 +166,7 @@ export default function LibraryMenuItems({ type: "everything", elements: item.elements, randomizeSeed: true, - }).newElements, + }).duplicatedElements, }; }); }, diff --git a/packages/excalidraw/tests/__snapshots__/move.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/move.test.tsx.snap index 4b863d4e7..5078a31a0 100644 --- a/packages/excalidraw/tests/__snapshots__/move.test.tsx.snap +++ b/packages/excalidraw/tests/__snapshots__/move.test.tsx.snap @@ -1,40 +1,6 @@ // Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html exports[`duplicate element on move when ALT is clicked > rectangle 5`] = ` -{ - "angle": 0, - "backgroundColor": "transparent", - "boundElements": null, - "customData": undefined, - "fillStyle": "solid", - "frameId": null, - "groupIds": [], - "height": 50, - "id": "id2", - "index": "Zz", - "isDeleted": false, - "link": null, - "locked": false, - "opacity": 100, - "roughness": 1, - "roundness": { - "type": 3, - }, - "seed": 1278240551, - "strokeColor": "#1e1e1e", - "strokeStyle": "solid", - "strokeWidth": 2, - "type": "rectangle", - "updated": 1, - "version": 6, - "versionNonce": 1604849351, - "width": 30, - "x": 30, - "y": 20, -} -`; - -exports[`duplicate element on move when ALT is clicked > rectangle 6`] = ` { "angle": 0, "backgroundColor": "transparent", @@ -54,13 +20,47 @@ exports[`duplicate element on move when ALT is clicked > rectangle 6`] = ` "roundness": { "type": 3, }, - "seed": 1505387817, + "seed": 1278240551, "strokeColor": "#1e1e1e", "strokeStyle": "solid", "strokeWidth": 2, "type": "rectangle", "updated": 1, - "version": 6, + "version": 5, + "versionNonce": 1505387817, + "width": 30, + "x": 30, + "y": 20, +} +`; + +exports[`duplicate element on move when ALT is clicked > rectangle 6`] = ` +{ + "angle": 0, + "backgroundColor": "transparent", + "boundElements": null, + "customData": undefined, + "fillStyle": "solid", + "frameId": null, + "groupIds": [], + "height": 50, + "id": "id2", + "index": "a1", + "isDeleted": false, + "link": null, + "locked": false, + "opacity": 100, + "roughness": 1, + "roundness": { + "type": 3, + }, + "seed": 1604849351, + "strokeColor": "#1e1e1e", + "strokeStyle": "solid", + "strokeWidth": 2, + "type": "rectangle", + "updated": 1, + "version": 7, "versionNonce": 915032327, "width": 30, "x": -10, diff --git a/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap index 319287792..68d4d5d79 100644 --- a/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap +++ b/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap @@ -2038,7 +2038,7 @@ exports[`regression tests > alt-drag duplicates an element > [end of test] appSt "scrolledOutside": false, "searchMatches": [], "selectedElementIds": { - "id0": true, + "id2": true, }, "selectedElementsAreBeingDragged": false, "selectedGroupIds": {}, @@ -2128,8 +2128,16 @@ History { HistoryEntry { "appStateChange": AppStateChange { "delta": Delta { - "deleted": {}, - "inserted": {}, + "deleted": { + "selectedElementIds": { + "id2": true, + }, + }, + "inserted": { + "selectedElementIds": { + "id0": true, + }, + }, }, }, "elementsChange": ElementsChange { @@ -2145,7 +2153,7 @@ History { "frameId": null, "groupIds": [], "height": 10, - "index": "Zz", + "index": "a1", "isDeleted": false, "link": null, "locked": false, @@ -2159,26 +2167,15 @@ History { "strokeWidth": 2, "type": "rectangle", "width": 10, - "x": 10, - "y": 10, + "x": 20, + "y": 20, }, "inserted": { "isDeleted": true, }, }, }, - "updated": Map { - "id0" => Delta { - "deleted": { - "x": 20, - "y": 20, - }, - "inserted": { - "x": 10, - "y": 10, - }, - }, - }, + "updated": Map {}, }, }, ], @@ -10378,13 +10375,13 @@ exports[`regression tests > make a group and duplicate it > [end of test] appSta "scrolledOutside": false, "searchMatches": [], "selectedElementIds": { - "id0": true, - "id1": true, - "id2": true, + "id6": true, + "id8": true, + "id9": true, }, "selectedElementsAreBeingDragged": false, "selectedGroupIds": { - "id4": true, + "id7": true, }, "selectedLinearElement": null, "selectionElement": null, @@ -10648,8 +10645,26 @@ History { HistoryEntry { "appStateChange": AppStateChange { "delta": Delta { - "deleted": {}, - "inserted": {}, + "deleted": { + "selectedElementIds": { + "id6": true, + "id8": true, + "id9": true, + }, + "selectedGroupIds": { + "id7": true, + }, + }, + "inserted": { + "selectedElementIds": { + "id0": true, + "id1": true, + "id2": true, + }, + "selectedGroupIds": { + "id4": true, + }, + }, }, }, "elementsChange": ElementsChange { @@ -10667,7 +10682,7 @@ History { "id7", ], "height": 10, - "index": "Zx", + "index": "a3", "isDeleted": false, "link": null, "locked": false, @@ -10681,8 +10696,8 @@ History { "strokeWidth": 2, "type": "rectangle", "width": 10, - "x": 10, - "y": 10, + "x": 20, + "y": 20, }, "inserted": { "isDeleted": true, @@ -10700,7 +10715,7 @@ History { "id7", ], "height": 10, - "index": "Zy", + "index": "a4", "isDeleted": false, "link": null, "locked": false, @@ -10714,8 +10729,8 @@ History { "strokeWidth": 2, "type": "rectangle", "width": 10, - "x": 30, - "y": 10, + "x": 40, + "y": 20, }, "inserted": { "isDeleted": true, @@ -10733,7 +10748,7 @@ History { "id7", ], "height": 10, - "index": "Zz", + "index": "a5", "isDeleted": false, "link": null, "locked": false, @@ -10747,46 +10762,15 @@ History { "strokeWidth": 2, "type": "rectangle", "width": 10, - "x": 50, - "y": 10, + "x": 60, + "y": 20, }, "inserted": { "isDeleted": true, }, }, }, - "updated": Map { - "id0" => Delta { - "deleted": { - "x": 20, - "y": 20, - }, - "inserted": { - "x": 10, - "y": 10, - }, - }, - "id1" => Delta { - "deleted": { - "x": 40, - "y": 20, - }, - "inserted": { - "x": 30, - "y": 10, - }, - }, - "id2" => Delta { - "deleted": { - "x": 60, - "y": 20, - }, - "inserted": { - "x": 50, - "y": 10, - }, - }, - }, + "updated": Map {}, }, }, ], diff --git a/packages/excalidraw/tests/clipboard.test.tsx b/packages/excalidraw/tests/clipboard.test.tsx index 0759afd94..7d0e3906c 100644 --- a/packages/excalidraw/tests/clipboard.test.tsx +++ b/packages/excalidraw/tests/clipboard.test.tsx @@ -307,6 +307,41 @@ describe("pasting & frames", () => { }); }); + it("should remove element from frame when pasted outside", async () => { + const frame = API.createElement({ + type: "frame", + width: 100, + height: 100, + x: 0, + y: 0, + }); + const rect = API.createElement({ + type: "rectangle", + frameId: frame.id, + x: 10, + y: 10, + width: 50, + height: 50, + }); + + API.setElements([frame]); + + const clipboardJSON = await serializeAsClipboardJSON({ + elements: [rect], + files: null, + }); + + mouse.moveTo(150, 150); + + pasteWithCtrlCmdV(clipboardJSON); + + await waitFor(() => { + expect(h.elements.length).toBe(2); + expect(h.elements[1].type).toBe(rect.type); + expect(h.elements[1].frameId).toBe(null); + }); + }); + it("should filter out elements not overlapping frame", async () => { const frame = API.createElement({ type: "frame", diff --git a/packages/excalidraw/tests/cropElement.test.tsx b/packages/excalidraw/tests/cropElement.test.tsx index 8011483fa..8764962fe 100644 --- a/packages/excalidraw/tests/cropElement.test.tsx +++ b/packages/excalidraw/tests/cropElement.test.tsx @@ -218,7 +218,7 @@ describe("Cropping and other features", async () => { initialHeight / 2, ]); Keyboard.keyDown(KEYS.ESCAPE); - const duplicatedImage = duplicateElement(null, new Map(), image, {}); + const duplicatedImage = duplicateElement(null, new Map(), image); act(() => { h.app.scene.insertElement(duplicatedImage); }); diff --git a/packages/excalidraw/types.ts b/packages/excalidraw/types.ts index 717993b43..4d3fe4fdc 100644 --- a/packages/excalidraw/types.ts +++ b/packages/excalidraw/types.ts @@ -724,7 +724,8 @@ export type PointerDownState = Readonly<{ scrollbars: ReturnType; // The previous pointer position lastCoords: { x: number; y: number }; - // map of original elements data + // original element frozen snapshots so we can access the original + // element attribute values at time of pointerdown originalElements: Map>; resize: { // Handle when resizing, might change during the pointer interaction @@ -758,6 +759,9 @@ export type PointerDownState = Readonly<{ hasOccurred: boolean; // Might change during the pointer interaction offset: { x: number; y: number } | null; + // by default same as PointerDownState.origin. On alt-duplication, reset + // to current pointer position at time of duplication. + origin: { x: number; y: number }; }; // We need to have these in the state so that we can unsubscribe them eventListeners: { From b5d60973b7188f58f543da23502d8de65c96b475 Mon Sep 17 00:00:00 2001 From: David Luzar <5153846+dwelle@users.noreply.github.com> Date: Fri, 18 Apr 2025 11:11:12 +0200 Subject: [PATCH 07/15] fix: duplication tests pointer state leaking between tests (#9414) * fix: duplication tests pointer state leaking between tests * fix snapshots --- packages/element/tests/duplicate.test.tsx | 3 +- .../tests/__snapshots__/history.test.tsx.snap | 28 +++++++++---------- packages/excalidraw/tests/helpers/api.ts | 1 - packages/excalidraw/tests/helpers/ui.ts | 9 +++++- packages/excalidraw/tests/test-utils.ts | 6 +++- setupTests.ts | 5 ---- 6 files changed, 28 insertions(+), 24 deletions(-) diff --git a/packages/element/tests/duplicate.test.tsx b/packages/element/tests/duplicate.test.tsx index 409200b6e..fd748757b 100644 --- a/packages/element/tests/duplicate.test.tsx +++ b/packages/element/tests/duplicate.test.tsx @@ -1,4 +1,3 @@ -import React from "react"; import { pointFrom } from "@excalidraw/math"; import { @@ -472,7 +471,7 @@ describe("group-related duplication", () => { expect(h.state.editingGroupId).toBe("group1"); }); - it.skip("alt-duplicating within group away outside frame", () => { + it("alt-duplicating within group away outside frame", () => { const frame = API.createElement({ type: "frame", x: 0, diff --git a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap index 9ffb97128..7b249da27 100644 --- a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap +++ b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap @@ -7348,8 +7348,8 @@ exports[`history > multiplayer undo/redo > should iterate through the history wh "updated": 1, "version": 7, "width": 10, - "x": -10, - "y": -10, + "x": 0, + "y": 0, } `; @@ -7422,8 +7422,8 @@ History { "strokeWidth": 2, "type": "arrow", "width": 10, - "x": -10, - "y": -10, + "x": 0, + "y": 0, }, "inserted": { "isDeleted": true, @@ -12138,8 +12138,8 @@ exports[`history > singleplayer undo/redo > should create entry when selecting f "updated": 1, "version": 3, "width": 10, - "x": 10, - "y": 10, + "x": -10, + "y": -10, } `; @@ -12192,8 +12192,8 @@ exports[`history > singleplayer undo/redo > should create entry when selecting f "updated": 1, "version": 5, "width": 50, - "x": 60, - "y": 0, + "x": 40, + "y": -20, } `; @@ -12246,8 +12246,8 @@ exports[`history > singleplayer undo/redo > should create entry when selecting f "updated": 1, "version": 4, "width": 50, - "x": 150, - "y": -10, + "x": 130, + "y": -30, } `; @@ -12301,8 +12301,8 @@ History { "strokeWidth": 2, "type": "rectangle", "width": 10, - "x": 10, - "y": 10, + "x": -10, + "y": -10, }, "inserted": { "isDeleted": true, @@ -12387,8 +12387,8 @@ History { "strokeWidth": 2, "type": "freedraw", "width": 50, - "x": 150, - "y": -10, + "x": 130, + "y": -30, }, "inserted": { "isDeleted": true, diff --git a/packages/excalidraw/tests/helpers/api.ts b/packages/excalidraw/tests/helpers/api.ts index 09aa308a5..3a83f2763 100644 --- a/packages/excalidraw/tests/helpers/api.ts +++ b/packages/excalidraw/tests/helpers/api.ts @@ -444,7 +444,6 @@ export class API { const text = API.createElement({ type: "text", - id: "text2", width: 50, height: 20, containerId: arrow.id, diff --git a/packages/excalidraw/tests/helpers/ui.ts b/packages/excalidraw/tests/helpers/ui.ts index 32de489f1..38070d38b 100644 --- a/packages/excalidraw/tests/helpers/ui.ts +++ b/packages/excalidraw/tests/helpers/ui.ts @@ -180,10 +180,17 @@ export class Pointer { public clientX = 0; public clientY = 0; + static activePointers: Pointer[] = []; + static resetAll() { + Pointer.activePointers.forEach((pointer) => pointer.reset()); + } + constructor( private readonly pointerType: "mouse" | "touch" | "pen", private readonly pointerId = 1, - ) {} + ) { + Pointer.activePointers.push(this); + } reset() { this.clientX = 0; diff --git a/packages/excalidraw/tests/test-utils.ts b/packages/excalidraw/tests/test-utils.ts index b2b8aff9c..894d748dd 100644 --- a/packages/excalidraw/tests/test-utils.ts +++ b/packages/excalidraw/tests/test-utils.ts @@ -19,7 +19,7 @@ import type { AllPossibleKeys } from "@excalidraw/common/utility-types"; import { STORAGE_KEYS } from "../../../excalidraw-app/app_constants"; -import { UI } from "./helpers/ui"; +import { Pointer, UI } from "./helpers/ui"; import * as toolQueries from "./queries/toolQueries"; import type { RenderResult, RenderOptions } from "@testing-library/react"; @@ -42,6 +42,10 @@ type TestRenderFn = ( ) => Promise>; const renderApp: TestRenderFn = async (ui, options) => { + // when tests reuse Pointer instances let's reset the last + // pointer poisitions so there's no leak between tests + Pointer.resetAll(); + if (options?.localStorageData) { initLocalStorage(options.localStorageData); delete options.localStorageData; diff --git a/setupTests.ts b/setupTests.ts index 2aec616b4..245c57326 100644 --- a/setupTests.ts +++ b/setupTests.ts @@ -94,11 +94,6 @@ vi.mock( }, ); -vi.mock("nanoid", () => { - return { - nanoid: vi.fn(() => "test-id"), - }; -}); // ReactDOM is located inside index.tsx file // as a result, we need a place for it to render into const element = document.createElement("div"); From 5fc13e43096d666d2b7d1f6d688860de92a0c9c8 Mon Sep 17 00:00:00 2001 From: Jack Walsh Date: Sun, 20 Apr 2025 07:50:44 +1000 Subject: [PATCH 08/15] feat: add `props.renderScrollbars` (#9399) * Expose renderScrollbars to AppState * fix: scrollbar rendering should use al renderable elements * remove `appState.renderScrollbars` * clean unused --------- Co-authored-by: dwelle <5153846+dwelle@users.noreply.github.com> --- .../@excalidraw/excalidraw/api/props/props.mdx | 1 + .../components/ExampleApp.tsx | 10 ++++++++++ packages/common/src/utils.ts | 15 +++++++++++++++ packages/element/src/bounds.ts | 12 +++++++++--- packages/excalidraw/components/App.tsx | 3 +++ .../components/canvases/InteractiveCanvas.tsx | 6 ++++-- packages/excalidraw/index.tsx | 2 ++ packages/excalidraw/renderer/interactiveScene.ts | 2 +- packages/excalidraw/scene/Scene.ts | 9 ++++----- packages/excalidraw/scene/scrollbars.ts | 8 +++----- packages/excalidraw/types.ts | 1 + 11 files changed, 53 insertions(+), 16 deletions(-) diff --git a/dev-docs/docs/@excalidraw/excalidraw/api/props/props.mdx b/dev-docs/docs/@excalidraw/excalidraw/api/props/props.mdx index 5c2a5501b..607e97182 100644 --- a/dev-docs/docs/@excalidraw/excalidraw/api/props/props.mdx +++ b/dev-docs/docs/@excalidraw/excalidraw/api/props/props.mdx @@ -31,6 +31,7 @@ All `props` are _optional_. | [`generateIdForFile`](#generateidforfile) | `function` | \_ | Allows you to override `id` generation for files added on canvas | | [`validateEmbeddable`](#validateembeddable) | `string[]` \| `boolean` \| `RegExp` \| `RegExp[]` \| ((link: string) => boolean | undefined) | \_ | use for custom src url validation | | [`renderEmbeddable`](/docs/@excalidraw/excalidraw/api/props/render-props#renderEmbeddable) | `function` | \_ | Render function that can override the built-in `