From d2557474e2a9cfccd0a94ab950a2e763ab12823a Mon Sep 17 00:00:00 2001 From: David Luzar <5153846+dwelle@users.noreply.github.com> Date: Tue, 5 May 2026 21:35:32 +0200 Subject: [PATCH] fix(editor): fix target element index when creating/adding elements to frames (#11257) --- packages/element/src/Scene.ts | 48 +- packages/element/src/collision.ts | 9 +- packages/element/src/duplicate.ts | 21 +- packages/element/src/frame.ts | 130 +++-- packages/element/src/typeChecks.ts | 20 + packages/element/tests/frame.test.tsx | 552 +++++++++++++++++- packages/excalidraw/actions/actionDeselect.ts | 2 + .../excalidraw/actions/actionFinalize.tsx | 1 + packages/excalidraw/actions/actionFrame.ts | 1 - packages/excalidraw/actions/actionGroup.tsx | 1 - packages/excalidraw/components/App.tsx | 307 ++++++++-- .../components/LibraryMenuItems.tsx | 1 + .../excalidraw/components/Stats/Dimension.tsx | 2 - .../components/Stats/MultiDimension.tsx | 2 - .../components/canvases/InteractiveCanvas.tsx | 6 +- .../components/canvases/NewElementCanvas.tsx | 3 +- .../components/canvases/StaticCanvas.tsx | 6 +- packages/excalidraw/polyfill.ts | 88 +++ packages/excalidraw/scene/Renderer.ts | 313 ++++++---- packages/excalidraw/tests/clipboard.test.tsx | 97 ++- .../excalidraw/wysiwyg/textWysiwyg.test.tsx | 57 ++ 21 files changed, 1405 insertions(+), 262 deletions(-) diff --git a/packages/element/src/Scene.ts b/packages/element/src/Scene.ts index 4ba663ceba..35b0cf4b95 100644 --- a/packages/element/src/Scene.ts +++ b/packages/element/src/Scene.ts @@ -338,29 +338,20 @@ export class Scene { this.callbacks.clear(); } - insertElementAtIndex(element: ExcalidrawElement, index: number) { - if (!Number.isFinite(index) || index < 0) { - throw new Error( - "insertElementAtIndex can only be called with index >= 0", - ); - } - - const nextElements = [ - ...this.elements.slice(0, index), - element, - ...this.elements.slice(index), - ]; - - syncMovedIndices(nextElements, arrayToMap([element])); - - this.replaceAllElements(nextElements); - } - - insertElementsAtIndex(elements: ExcalidrawElement[], index: number) { + /** low-level - generally use app.insertNewElements() */ + insertElementsAtIndex( + elements: ExcalidrawElement[], + /** null indicates end of the array */ + index: number | null, + ) { if (!elements.length) { return; } + if (index === null) { + index = this.elements.length; + } + if (!Number.isFinite(index) || index < 0) { throw new Error( "insertElementAtIndex can only be called with index >= 0", @@ -378,24 +369,9 @@ export class Scene { this.replaceAllElements(nextElements); } + /** low-level - generally use app.insertNewElement() */ insertElement = (element: ExcalidrawElement) => { - const index = element.frameId - ? this.getElementIndex(element.frameId) - : this.elements.length; - - this.insertElementAtIndex(element, index); - }; - - insertElements = (elements: ExcalidrawElement[]) => { - if (!elements.length) { - return; - } - - const index = elements[0]?.frameId - ? this.getElementIndex(elements[0].frameId) - : this.elements.length; - - this.insertElementsAtIndex(elements, index); + this.insertElementsAtIndex([element], null); }; getElementIndex(elementId: string) { diff --git a/packages/element/src/collision.ts b/packages/element/src/collision.ts index c260ae5267..6999512b44 100644 --- a/packages/element/src/collision.ts +++ b/packages/element/src/collision.ts @@ -61,6 +61,8 @@ import { distanceToElement } from "./distance"; import { getBindingGap } from "./binding"; +import { hasBackground } from "./comparisons"; + import type { ElementsMap, ExcalidrawArrowElement, @@ -83,7 +85,7 @@ export const shouldTestInside = (element: ExcalidrawElement) => { } const isDraggableFromInside = - !isTransparent(element.backgroundColor) || + (hasBackground(element.type) && !isTransparent(element.backgroundColor)) || hasBoundTextElement(element) || isIframeLikeElement(element) || isTextElement(element); @@ -324,7 +326,10 @@ export const getAllHoveredElementAtPoint = ( ) { candidateElements.push(element); - if (!isTransparent(element.backgroundColor)) { + if ( + hasBackground(element.type) && + !isTransparent(element.backgroundColor) + ) { break; } } diff --git a/packages/element/src/duplicate.ts b/packages/element/src/duplicate.ts index 235fd1e47d..24135c0879 100644 --- a/packages/element/src/duplicate.ts +++ b/packages/element/src/duplicate.ts @@ -111,6 +111,9 @@ export const duplicateElements = ( * user interaction. */ type: "everything"; + // TODO remove/review this once we add frame children order migration + // and invariant checks + preserveFrameChildrenOrder?: boolean; } | { /** @@ -170,6 +173,8 @@ export const duplicateElements = ( opts.type === "in-place" ? opts.idsOfElementsToDuplicate : new Map(elements.map((el) => [el.id, el])); + const preserveFrameChildrenOrder = + opts.type === "everything" && opts.preserveFrameChildrenOrder; // For sanity if (opts.type === "in-place") { @@ -277,7 +282,7 @@ export const duplicateElements = ( if (groupId) { const groupElements = getElementsInGroup(elements, groupId).flatMap( (element) => - isFrameLikeElement(element) + isFrameLikeElement(element) && !preserveFrameChildrenOrder ? [...getFrameChildren(elements, element.id), element] : [element], ); @@ -293,13 +298,25 @@ export const duplicateElements = ( // frame duplication // ------------------------------------------------------------------------- - if (element.frameId && frameIdsToDuplicate.has(element.frameId)) { + if ( + !preserveFrameChildrenOrder && + element.frameId && + frameIdsToDuplicate.has(element.frameId) + ) { continue; } if (isFrameLikeElement(element)) { const frameId = element.id; + if (preserveFrameChildrenOrder) { + insertBeforeOrAfterIndex( + findLastIndex(elementsWithDuplicates, (el) => el.id === frameId), + copyElements(element), + ); + continue; + } + const frameChildren = getFrameChildren(elements, frameId); const targetIndex = findLastIndex(elementsWithDuplicates, (el) => { diff --git a/packages/element/src/frame.ts b/packages/element/src/frame.ts index 6138633ff4..3d1449a072 100644 --- a/packages/element/src/frame.ts +++ b/packages/element/src/frame.ts @@ -23,6 +23,7 @@ import { } from "./bounds"; import { mutateElement } from "./mutateElement"; import { getBoundTextElement, getContainerElement } from "./textElement"; +import { syncMovedIndices } from "./fractionalIndex"; import { isFrameElement, isFrameLikeElement, @@ -491,10 +492,44 @@ export const filterElementsEligibleAsFrameChildren = ( return eligibleElements; }; +export const getCommonFrameId = (elements: readonly ExcalidrawElement[]) => { + let commonFrameId: ExcalidrawElement["frameId"] | undefined; + + for (const element of elements) { + if (isFrameLikeElement(element) || !element.frameId) { + return null; + } + + if (commonFrameId === undefined) { + commonFrameId = element.frameId; + } else if (commonFrameId !== element.frameId) { + return null; + } + } + + return commonFrameId ?? null; +}; + +export const getFrameChildrenInsertionIndex = ( + elements: readonly ExcalidrawElement[], + frameId: ExcalidrawFrameLikeElement["id"], +): number | null => { + for (let index = elements.length - 1; index >= 0; index--) { + const element = elements[index]; + + if (element.id === frameId) { + return index; + } else if (element.frameId === frameId) { + return index + 1; + } + } + + return null; +}; + /** - * Retains (or repairs for target frame) the ordering invriant where children - * elements come right before the parent frame: - * [el, el, child, child, frame, el] + * Adds elements and their bound elements to frame. Reorders added elements to + * be just below frame, or just above its highest child (whichever is higher). * * @returns mutated allElements (same data structure) */ @@ -502,19 +537,11 @@ export const addElementsToFrame = ( allElements: T, elementsToAdd: NonDeletedExcalidrawElement[], frame: ExcalidrawFrameLikeElement, - appState: AppState, ): T => { const elementsMap = arrayToMap(allElements); - const currTargetFrameChildrenMap = new Map(); - for (const element of allElements.values()) { - if (element.frameId === frame.id) { - currTargetFrameChildrenMap.set(element.id, true); - } - } + const commonFrameId = getCommonFrameId(elementsToAdd); - const suppliedElementsToAddSet = new Set(elementsToAdd.map((el) => el.id)); - - const finalElementsToAdd: ExcalidrawElement[] = []; + const finalElementsToAdd = new Set(); const otherFrames = new Set(); @@ -525,7 +552,8 @@ export const addElementsToFrame = ( } // - add bound text elements if not already in the array - // - filter out elements that are already in the frame + // - keep elements already in the frame so mixed selections can be reordered + // together for (const element of omitGroupsContainingFrameLikes( allElements, elementsToAdd, @@ -538,38 +566,68 @@ export const addElementsToFrame = ( continue; } - // if the element is already in another frame (which is also in elementsToAdd), - // it means that frame and children are selected at the same time - // => keep original frame membership, do not add to the target frame - if ( - element.frameId && - appState.selectedElementIds[element.id] && - appState.selectedElementIds[element.frameId] - ) { + if (element.frameId && element.frameId !== frame.id) { continue; } - if (!currTargetFrameChildrenMap.has(element.id)) { - finalElementsToAdd.push(element); - } + finalElementsToAdd.add(element); const boundTextElement = getBoundTextElement(element, elementsMap); - if ( - boundTextElement && - !suppliedElementsToAddSet.has(boundTextElement.id) && - !currTargetFrameChildrenMap.has(boundTextElement.id) - ) { - finalElementsToAdd.push(boundTextElement); + if (boundTextElement && !finalElementsToAdd.has(boundTextElement)) { + finalElementsToAdd.add(boundTextElement); } } for (const element of finalElementsToAdd) { - mutateElement(element, elementsMap, { - frameId: frame.id, - }); + // we don't always need to update the element if it's already in the frame, + // but we still need to accumulate in finalElementsToAdd so we potentially + // reorder them if added together + if (element.frameId !== frame.id) { + mutateElement(element, elementsMap, { + frameId: frame.id, + }); + } } - return allElements; + // (re)order elements to be just below the frame, + // or just above the highest child if that is higher + // (latter case is denormalized order until we migrate) + // --------------------------------------------------------------------------- + + if ( + !finalElementsToAdd.size || + // if all elements to add already belong to the frame, then we don't want to + // reorder (case: we're dragging element children within the frame) + commonFrameId === frame.id + ) { + return allElements; + } + + const otherElements = Array.from(allElements.values()).filter( + (element) => !finalElementsToAdd.has(element), + ); + const insertionIndex = getFrameChildrenInsertionIndex( + otherElements, + frame.id, + ); + + if (insertionIndex === null) { + return allElements; + } + + const reorderedElements = [ + ...otherElements.slice(0, insertionIndex), + ...finalElementsToAdd, + ...otherElements.slice(insertionIndex), + ]; + + syncMovedIndices(reorderedElements, arrayToMap([...finalElementsToAdd])); + + return ( + Array.isArray(allElements) + ? reorderedElements + : new Map(reorderedElements.map((element) => [element.id, element])) + ) as T; }; export const removeElementsFromFrame = ( @@ -623,13 +681,11 @@ export const replaceAllElementsInFrame = ( allElements: readonly T[], nextElementsInFrame: ExcalidrawElement[], frame: ExcalidrawFrameLikeElement, - app: AppClassProperties, ): T[] => { return addElementsToFrame( removeAllElementsFromFrame(allElements, frame), nextElementsInFrame, frame, - app.state, ).slice(); }; diff --git a/packages/element/src/typeChecks.ts b/packages/element/src/typeChecks.ts index b609cc3f8a..3a8f5e36ef 100644 --- a/packages/element/src/typeChecks.ts +++ b/packages/element/src/typeChecks.ts @@ -392,3 +392,23 @@ export const canBecomePolygon = ( (points.length === 3 && !pointsEqual(points[0], points[points.length - 1])) ); }; + +export const isEligibleFrameChildType = (type: ElementOrToolType) => { + switch (type) { + case "rectangle": + case "diamond": + case "ellipse": + case "arrow": + case "line": + case "freedraw": + case "text": + case "image": + case "frame": + case "embeddable": { + return true; + } + default: { + return false; + } + } +}; diff --git a/packages/element/tests/frame.test.tsx b/packages/element/tests/frame.test.tsx index e92267130a..b419d00f02 100644 --- a/packages/element/tests/frame.test.tsx +++ b/packages/element/tests/frame.test.tsx @@ -5,12 +5,15 @@ import { import { arrayToMap } from "@excalidraw/common"; import { API } from "@excalidraw/excalidraw/tests/helpers/api"; -import { Keyboard, Pointer } from "@excalidraw/excalidraw/tests/helpers/ui"; +import { Keyboard, Pointer, UI } from "@excalidraw/excalidraw/tests/helpers/ui"; +import { getTextEditor } from "@excalidraw/excalidraw/tests/queries/dom"; import { getCloneByOrigId, render, } from "@excalidraw/excalidraw/tests/test-utils"; +import { getSelectedElements } from "@excalidraw/excalidraw/scene"; + import { elementOverlapsWithFrame } from "../src/frame"; import type { @@ -151,6 +154,230 @@ describe("adding elements to frames", () => { ).toBe(true); }); + it("should not add a newly created element to a frame behind a non-frame element", () => { + const cover = API.createElement({ + id: "cover", + type: "rectangle", + x: 10, + y: 10, + width: 80, + height: 80, + backgroundColor: "#ffc9c9", + }); + + API.setElements([frame, cover]); + + UI.clickTool("rectangle"); + mouse.downAt(20, 20); + mouse.moveTo(40, 40); + mouse.upAt(40, 40); + + const createdElement = h.elements.find( + (element) => element.id !== frame.id && element.id !== cover.id, + ); + + expect(createdElement?.frameId).toBe(null); + expect(h.elements.map((element) => element.id)).toEqual([ + frame.id, + cover.id, + createdElement?.id, + ]); + }); + + it("should add a newly created element to a frame over a non-frame element", () => { + const cover = API.createElement({ + id: "cover", + type: "rectangle", + x: 10, + y: 10, + width: 80, + height: 80, + backgroundColor: "#ffc9c9", + }); + + API.setElements([cover, frame]); + + UI.clickTool("rectangle"); + mouse.downAt(20, 20); + mouse.moveTo(40, 40); + mouse.upAt(40, 40); + + const createdElement = h.elements.find( + (element) => element.id !== frame.id && element.id !== cover.id, + ); + + expect(createdElement?.frameId).toBe(frame.id); + }); + + it("should highlight the target frame while creating a new element", () => { + API.setElements([frame]); + + UI.clickTool("rectangle"); + mouse.downAt(20, 20); + mouse.moveTo(40, 40); + + expect(h.state.frameToHighlight?.id).toBe(frame.id); + + mouse.upAt(40, 40); + + expect(h.state.frameToHighlight).toBe(null); + }); + + it("should highlight the target frame while hovering with a creation tool", () => { + API.setElements([frame]); + + UI.clickTool("rectangle"); + mouse.moveTo(20, 20); + + expect(h.state.frameToHighlight?.id).toBe(frame.id); + + mouse.moveTo(200, 200); + + expect(h.state.frameToHighlight).toBe(null); + }); + + it("should not add grid-snapped text outside the frame to the clicked frame", async () => { + const offsetFrame = API.createElement({ + id: "offsetFrame", + type: "frame", + x: 10, + y: 0, + width: 150, + height: 150, + }); + + API.setElements([offsetFrame]); + API.setAppState({ + gridModeEnabled: true, + }); + + UI.clickTool("text"); + mouse.clickAt(12, 0); + + await getTextEditor(); + + const createdText = h.elements.find( + (element) => element.id !== offsetFrame.id, + ); + + expect(createdText?.x).toBe(0); + expect(createdText?.y).toBe(0); + expect(createdText?.frameId).toBe(null); + }); + + it("should add a newly created element to a frame behind another frame", () => { + const lockedFrame = API.createElement({ + id: "lockedFrame", + type: "frame", + x: 10, + y: 10, + width: 80, + height: 80, + locked: true, + }); + + API.setElements([frame, lockedFrame]); + + UI.clickTool("rectangle"); + mouse.downAt(20, 20); + mouse.moveTo(40, 40); + mouse.upAt(40, 40); + + const createdElement = h.elements.find( + (element) => element.id !== frame.id && element.id !== lockedFrame.id, + ); + + expect(createdElement?.frameId).toBe(frame.id); + }); + + it("should insert a newly created frame child just below its frame", () => { + const frameChildUnderCursor = API.createElement({ + id: "frameChildUnderCursor", + type: "rectangle", + x: 10, + y: 10, + width: 80, + height: 80, + backgroundColor: "#ffc9c9", + frameId: frame.id, + }); + const otherFrameChild = API.createElement({ + id: "otherFrameChild", + type: "rectangle", + x: 100, + y: 20, + width: 20, + height: 20, + frameId: frame.id, + }); + + API.setElements([frameChildUnderCursor, otherFrameChild, frame]); + + UI.clickTool("rectangle"); + mouse.downAt(20, 20); + mouse.moveTo(40, 40); + mouse.upAt(40, 40); + + const createdElement = h.elements.find( + (element) => + element.id !== frame.id && + element.id !== frameChildUnderCursor.id && + element.id !== otherFrameChild.id, + ); + + expect(createdElement?.frameId).toBe(frame.id); + expect(h.elements.map((element) => element.id)).toEqual([ + frameChildUnderCursor.id, + otherFrameChild.id, + createdElement?.id, + frame.id, + ]); + }); + + it("should insert a newly created frame child above the highest frame child", () => { + const frameChildUnderCursor = API.createElement({ + id: "frameChildUnderCursor", + type: "rectangle", + x: 10, + y: 10, + width: 80, + height: 80, + backgroundColor: "#ffc9c9", + frameId: frame.id, + }); + const otherFrameChild = API.createElement({ + id: "otherFrameChild", + type: "rectangle", + x: 100, + y: 20, + width: 20, + height: 20, + frameId: frame.id, + }); + + API.setElements([frame, frameChildUnderCursor, otherFrameChild]); + + UI.clickTool("rectangle"); + mouse.downAt(20, 20); + mouse.moveTo(40, 40); + mouse.upAt(40, 40); + + const createdElement = h.elements.find( + (element) => + element.id !== frame.id && + element.id !== frameChildUnderCursor.id && + element.id !== otherFrameChild.id, + ); + + expect(createdElement?.frameId).toBe(frame.id); + expect(h.elements.map((element) => element.id)).toEqual([ + frame.id, + frameChildUnderCursor.id, + otherFrameChild.id, + createdElement?.id, + ]); + }); + const commonTestCases = async ( func: typeof resizeFrameOverElement | typeof dragElementIntoFrame, ) => { @@ -457,6 +684,329 @@ describe("adding elements to frames", () => { expect(API.getElement(containingRect).frameId).toBe(frame.id); }); + it("should drag an element into a frame", () => { + API.setElements([rect2, frame]); + + dragElementIntoFrame(frame, rect2); + + expect(rect2.frameId).toBe(frame.id); + }); + + it("should layer a dragged element above the highest frame child", () => { + const frameChild = API.createElement({ + id: "frameChild", + type: "rectangle", + x: 10, + y: 10, + width: 20, + height: 20, + frameId: frame.id, + }); + + API.setElements([frame, frameChild, rect2]); + + dragElementIntoFrame(frame, rect2); + + expect(rect2.frameId).toBe(frame.id); + expect(h.elements.map((element) => element.id)).toEqual([ + frame.id, + frameChild.id, + rect2.id, + ]); + expect(rect2.index! > frameChild.index!).toBe(true); + expect(rect2.index! > frame.index!).toBe(true); + }); + + it("should preview a dragged element above the highest frame child before pointerup", () => { + const frameChild = API.createElement({ + id: "frameChild", + type: "rectangle", + x: 10, + y: 10, + width: 20, + height: 20, + frameId: frame.id, + }); + + API.setElements([rect2, frame, frameChild]); + API.setSelectedElements([rect2]); + API.updateElement(rect2, { + x: 10, + y: 10, + }); + + const getRenderableElementIds = ( + selectedElementsAreBeingDragged: boolean, + ) => { + return h.app.renderer + .getRenderableElements({ + zoom: h.state.zoom, + offsetLeft: 0, + offsetTop: 0, + scrollX: 0, + scrollY: 0, + height: 1000, + width: 1000, + editingTextElement: h.state.editingTextElement, + newElement: h.state.newElement, + selectedElements: getSelectedElements(h.elements, h.state), + selectedElementsAreBeingDragged, + frameToHighlight: frame as ExcalidrawFrameLikeElement, + }) + .visibleElements.map((element) => element.id); + }; + + expect(h.elements.map((element) => element.id)).toEqual([ + rect2.id, + frame.id, + frameChild.id, + ]); + expect(getRenderableElementIds(false)).toEqual([ + rect2.id, + frame.id, + frameChild.id, + ]); + expect(getRenderableElementIds(true)).toEqual([ + frame.id, + frameChild.id, + rect2.id, + ]); + expect(h.elements.map((element) => element.id)).toEqual([ + rect2.id, + frame.id, + frameChild.id, + ]); + expect(rect2.frameId).toBe(null); + }); + + it("should not preview reorder dragged elements already in the highlighted frame", () => { + const frameChild = API.createElement({ + id: "frameChild", + type: "rectangle", + x: 10, + y: 10, + width: 20, + height: 20, + frameId: frame.id, + }); + const otherFrameChild = API.createElement({ + id: "otherFrameChild", + type: "rectangle", + x: 40, + y: 10, + width: 20, + height: 20, + frameId: frame.id, + }); + + API.setElements([frameChild, frame, otherFrameChild]); + API.setSelectedElements([frameChild]); + + const renderableElementIds = h.app.renderer + .getRenderableElements({ + zoom: h.state.zoom, + offsetLeft: 0, + offsetTop: 0, + scrollX: 0, + scrollY: 0, + height: 1000, + width: 1000, + editingTextElement: h.state.editingTextElement, + newElement: h.state.newElement, + selectedElements: getSelectedElements(h.elements, h.state), + selectedElementsAreBeingDragged: true, + frameToHighlight: frame as ExcalidrawFrameLikeElement, + }) + .visibleElements.map((element) => element.id); + + expect(renderableElementIds).toEqual([ + frameChild.id, + frame.id, + otherFrameChild.id, + ]); + }); + + it("should put a dragged mixed selection above the highest frame child", () => { + const frameChild = API.createElement({ + id: "frameChild", + type: "rectangle", + x: 50, + y: 10, + width: 20, + height: 20, + frameId: frame.id, + boundElements: [{ id: "boundText", type: "text" }], + }); + const boundText = API.createElement({ + id: "boundText", + type: "text", + x: 50, + y: 10, + width: 20, + height: 20, + containerId: frameChild.id, + frameId: frame.id, + }); + const otherFrameChild = API.createElement({ + id: "otherFrameChild", + type: "rectangle", + x: 80, + y: 10, + width: 20, + height: 20, + frameId: frame.id, + }); + const nonFrameElement = API.createElement({ + id: "nonFrameElement", + type: "rectangle", + x: 155, + y: 10, + width: 20, + height: 20, + }); + + API.setElements([ + frame, + frameChild, + boundText, + otherFrameChild, + nonFrameElement, + ]); + API.setSelectedElements([frameChild, nonFrameElement]); + + mouse.downAt( + nonFrameElement.x + nonFrameElement.width / 2, + nonFrameElement.y + nonFrameElement.height / 2, + ); + mouse.moveTo(frame.x + frame.width - 5, nonFrameElement.y + 10); + mouse.up(); + + expect(frameChild.frameId).toBe(frame.id); + expect(boundText.frameId).toBe(frame.id); + expect(nonFrameElement.frameId).toBe(frame.id); + expect(h.elements.map((element) => element.id)).toEqual([ + frame.id, + otherFrameChild.id, + frameChild.id, + boundText.id, + nonFrameElement.id, + ]); + }); + + it("should not reorder dragged elements already in the highlighted frame", () => { + const frameChild = API.createElement({ + id: "frameChild", + type: "rectangle", + x: 50, + y: 10, + width: 20, + height: 20, + frameId: frame.id, + }); + const otherFrameChild = API.createElement({ + id: "otherFrameChild", + type: "rectangle", + x: 80, + y: 10, + width: 20, + height: 20, + frameId: frame.id, + }); + + API.setElements([frame, frameChild, otherFrameChild]); + API.setSelectedElements([frameChild]); + + mouse.downAt( + frameChild.x + frameChild.width / 2, + frameChild.y + frameChild.height / 2, + ); + mouse.moveTo(frameChild.x + frameChild.width / 2 + 5, frameChild.y + 10); + mouse.up(); + + expect(frameChild.frameId).toBe(frame.id); + expect(h.elements.map((element) => element.id)).toEqual([ + frame.id, + frameChild.id, + otherFrameChild.id, + ]); + }); + + it("should not drag an element into a frame behind a non-frame element", () => { + const cover = API.createElement({ + id: "cover", + type: "rectangle", + x: 10, + y: 10, + width: 80, + height: 80, + backgroundColor: "#ffc9c9", + }); + API.setElements([frame, cover, rect2]); + + mouse.clickAt(rect2.x, rect2.y); + mouse.downAt(rect2.x + rect2.width / 2, rect2.y + rect2.height / 2); + mouse.moveTo(20, 20); + mouse.upAt(20, 20); + + expect(rect2.frameId).toBe(null); + }); + + it("should drag an element into a frame over a non-frame element", () => { + const cover = API.createElement({ + id: "cover", + type: "rectangle", + x: 10, + y: 10, + width: 80, + height: 80, + backgroundColor: "#ffc9c9", + }); + API.setElements([cover, rect2, frame]); + + mouse.clickAt(rect2.x, rect2.y); + mouse.downAt(rect2.x + rect2.width / 2, rect2.y + rect2.height / 2); + mouse.moveTo(20, 20); + mouse.upAt(20, 20); + + expect(rect2.frameId).toBe(frame.id); + }); + + it("should keep dragging a frame child over a non-frame element above its frame", () => { + const cover = API.createElement({ + id: "cover", + type: "rectangle", + x: 10, + y: 10, + width: 80, + height: 80, + backgroundColor: "#ffc9c9", + }); + const frameChild = API.createElement({ + id: "frameChild", + type: "rectangle", + x: 100, + y: 20, + width: 20, + height: 20, + frameId: frame.id, + }); + + API.setElements([frameChild, frame, cover]); + API.setSelectedElements([frameChild]); + + mouse.downAt( + frameChild.x + frameChild.width / 2, + frameChild.y + frameChild.height / 2, + ); + mouse.moveTo(20, 20); + + expect(h.state.frameToHighlight?.id).toBe(frame.id); + + mouse.upAt(20, 20); + + expect(frameChild.frameId).toBe(frame.id); + }); + it.skip("should drag element inside, duplicate it and keep it in frame", () => { API.setElements([frame, rect2]); diff --git a/packages/excalidraw/actions/actionDeselect.ts b/packages/excalidraw/actions/actionDeselect.ts index 2b48eb825e..c9a92c7138 100644 --- a/packages/excalidraw/actions/actionDeselect.ts +++ b/packages/excalidraw/actions/actionDeselect.ts @@ -101,6 +101,7 @@ export const actionDeselect = register({ selectionElement: null, showHyperlinkPopup: false, suggestedBinding: null, + frameToHighlight: null, }, captureUpdate: CaptureUpdateAction.IMMEDIATELY, }; @@ -118,6 +119,7 @@ export const actionDeselect = register({ selectionElement: null, showHyperlinkPopup: false, suggestedBinding: null, + frameToHighlight: null, }, captureUpdate: CaptureUpdateAction.IMMEDIATELY, }; diff --git a/packages/excalidraw/actions/actionFinalize.tsx b/packages/excalidraw/actions/actionFinalize.tsx index c52bbe00a4..f2e5db5613 100644 --- a/packages/excalidraw/actions/actionFinalize.tsx +++ b/packages/excalidraw/actions/actionFinalize.tsx @@ -330,6 +330,7 @@ export const actionFinalize = register({ multiElement: null, editingTextElement: null, suggestedBinding: null, + frameToHighlight: null, selectedElementIds: element && !appState.activeTool.locked && diff --git a/packages/excalidraw/actions/actionFrame.ts b/packages/excalidraw/actions/actionFrame.ts index 3a1b3635d3..81a43e243e 100644 --- a/packages/excalidraw/actions/actionFrame.ts +++ b/packages/excalidraw/actions/actionFrame.ts @@ -205,7 +205,6 @@ export const actionWrapSelectionInFrame = register({ [...app.scene.getElementsIncludingDeleted(), frame], selectedElements, frame, - appState, ); return { diff --git a/packages/excalidraw/actions/actionGroup.tsx b/packages/excalidraw/actions/actionGroup.tsx index c72216b761..c9bb8cf21d 100644 --- a/packages/excalidraw/actions/actionGroup.tsx +++ b/packages/excalidraw/actions/actionGroup.tsx @@ -277,7 +277,6 @@ export const actionUngroup = register({ elementsMap, ), frame, - app, ); } }); diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index 3fd81e2ac7..4601808950 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -176,7 +176,9 @@ import { isValidTextContainer, redrawTextBoundingBox, hasBoundingBox, + getCommonFrameId, getFrameChildren, + getFrameChildrenInsertionIndex, isCursorInFrame, addElementsToFrame, replaceAllElementsInFrame, @@ -259,6 +261,7 @@ import { maybeHandleArrowPointlikeDrag, getUncroppedWidthAndHeight, getActiveTextElement, + isEligibleFrameChildType, } from "@excalidraw/element"; import type { GlobalPoint, LocalPoint, Radians } from "@excalidraw/math"; @@ -2092,20 +2095,30 @@ class App extends React.Component { const selectedElements = this.scene.getSelectedElements(this.state); const { renderTopRightUI, renderTopLeftUI, renderCustomStats } = this.props; - const sceneNonce = this.scene.getSceneNonce(); - const { elementsMap, visibleElements } = - this.renderer.getRenderableElements({ - sceneNonce, - zoom: this.state.zoom, - offsetLeft: this.state.offsetLeft, - offsetTop: this.state.offsetTop, - scrollX: this.state.scrollX, - scrollY: this.state.scrollY, - height: this.state.height, - width: this.state.width, - editingTextElement: this.state.editingTextElement, - newElementId: this.state.newElement?.id, - }); + const { + elementsMap, + visibleElements, + canvasNonce, + /** + * element to draw on the for optimization purposes. + * Can be null even if this.state.newElement defined + * (e.g. when its zIndex isn't on top) */ + newElementCanvasElement, + } = this.renderer.getRenderableElements({ + zoom: this.state.zoom, + offsetLeft: this.state.offsetLeft, + offsetTop: this.state.offsetTop, + scrollX: this.state.scrollX, + scrollY: this.state.scrollY, + height: this.state.height, + width: this.state.width, + editingTextElement: this.state.editingTextElement, + newElement: this.state.newElement, + selectedElements, + selectedElementsAreBeingDragged: + this.state.selectedElementsAreBeingDragged, + frameToHighlight: this.state.frameToHighlight, + }); this.visibleElements = visibleElements; const allElementsMap = this.scene.getNonDeletedElementsMap(); @@ -2324,7 +2337,7 @@ class App extends React.Component { elementsMap={elementsMap} allElementsMap={allElementsMap} visibleElements={visibleElements} - sceneNonce={sceneNonce} + canvasNonce={canvasNonce} selectionNonce={ this.state.selectionElement?.versionNonce } @@ -2345,9 +2358,10 @@ class App extends React.Component { theme: this.state.theme, }} /> - {this.state.newElement && ( + {newElementCanvasElement && ( { visibleElements={visibleElements} allElementsMap={allElementsMap} selectedElements={selectedElements} - sceneNonce={sceneNonce} + canvasNonce={canvasNonce} selectionNonce={ this.state.selectionElement?.versionNonce } @@ -2687,7 +2701,7 @@ class App extends React.Component { locked: false, }); - this.scene.insertElement(frame); + this.insertNewElement(frame); for (const child of selectedElements) { this.scene.mutateElement(child, { frameId: frame.id }); @@ -3765,6 +3779,7 @@ class App extends React.Component { position: this.editorInterface.formFactor === "desktop" ? "cursor" : "center", retainSeed: isPlainPaste, + preserveFrameChildrenOrder: true, }); return; } @@ -3908,6 +3923,7 @@ class App extends React.Component { position: { clientX: number; clientY: number } | "cursor" | "center"; retainSeed?: boolean; fitToContent?: boolean; + preserveFrameChildrenOrder?: boolean; }) => { const elements = restoreElements(opts.elements, null, { deleteInvisibleElements: true, @@ -3949,6 +3965,7 @@ class App extends React.Component { }); }), randomizeSeed: !opts.retainSeed, + preserveFrameChildrenOrder: opts.preserveFrameChildrenOrder, }); const prevElements = this.scene.getElementsIncludingDeleted(); @@ -3970,11 +3987,10 @@ class App extends React.Component { duplicatedElements, topLayerFrame, ); - addElementsToFrame( + nextElements = addElementsToFrame( nextElements, eligibleElements, topLayerFrame, - this.state, ); } @@ -4200,7 +4216,7 @@ class App extends React.Component { return; } - this.scene.insertElements(textElements); + this.insertNewElements(textElements); this.store.scheduleCapture(); this.setState({ selectedElementIds: makeNextSelectedElementIds( @@ -5456,7 +5472,7 @@ class App extends React.Component { if (!event[KEYS.CTRL_OR_CMD]) { if (this.flowChartCreator.isCreatingChart) { if (this.flowChartCreator.pendingNodes?.length) { - this.scene.insertElements(this.flowChartCreator.pendingNodes); + this.insertNewElements(this.flowChartCreator.pendingNodes); } const firstNode = this.flowChartCreator.pendingNodes?.[0]; @@ -5554,6 +5570,7 @@ class App extends React.Component { selectedLinearElement: isSelectionLikeTool(nextActiveTool.type) ? prevState.selectedLinearElement : null, + frameToHighlight: null, } as const; if (nextActiveTool.type === "freedraw") { @@ -6151,6 +6168,12 @@ class App extends React.Component { hitElement = elements[index]; break; } else if (x1 < x && x < x2 && y1 < y && y < y2) { + // to allow binding to containers within frames, + // ignore frames in hit testing + if (isFrameLikeElement(elements[index])) { + continue; + } + hitElement = elements[index]; break; } @@ -6240,11 +6263,6 @@ class App extends React.Component { } } - const topLayerFrame = this.getTopLayerFrameAtSceneCoords({ - x: sceneX, - y: sceneY, - }); - const textCreationGridPoint = this.getTextCreationGridPoint(sceneX, sceneY); const newTextElementPosition = parentCenterPosition @@ -6266,6 +6284,20 @@ class App extends React.Component { y: sceneY, }; + const topLayerFrame = this.getTopLayerFrameAtSceneCoords({ + x: newTextElementPosition.x, + y: newTextElementPosition.y, + }); + + // container has higher priority. Only add to frame if container is in the same frame. + const frameId = + topLayerFrame && + (!shouldBindToContainer || + !container || + container.frameId === topLayerFrame.id) + ? topLayerFrame.id + : null; + const element = existingTextElement || newTextElement({ @@ -6295,7 +6327,7 @@ class App extends React.Component { ? (0 as Radians) : container.angle : (0 as Radians), - frameId: topLayerFrame ? topLayerFrame.id : null, + frameId, }); if (!existingTextElement && shouldBindToContainer && container) { @@ -6311,9 +6343,12 @@ class App extends React.Component { if (!existingTextElement) { if (container && shouldBindToContainer) { const containerIndex = this.scene.getElementIndex(container.id); - this.scene.insertElementAtIndex(element, containerIndex + 1); + // TODO should use insertNewElement, after we update it to handle + // elements with containerId + frameId at the same time (containerId + // should take precedence when it comes to z-index) + this.scene.insertElementsAtIndex([element], containerIndex + 1); } else { - this.scene.insertElement(element); + this.insertNewElement(element); } } @@ -6695,19 +6730,161 @@ class App extends React.Component { } }; - private getTopLayerFrameAtSceneCoords = (sceneCoords: { - x: number; - y: number; - }) => { + /** + * finds candidate frame under cursor (when dragging frame children/elements + * inside frames) + */ + private getTopLayerFrameAtSceneCoords = ( + /** + * should be already grid aligned (basically should be what the call site + * sets the element's coords to, if applicable) + */ + sceneCoords: { + x: number; + y: number; + }, + opts?: { + /** to exclude selected elements when dragging, etc. */ + excludeElementIds?: AppState["selectedElementIds"]; + currentFrameId?: ExcalidrawElement["frameId"]; + }, + ) => { const elementsMap = this.scene.getNonDeletedElementsMap(); - const frames = this.scene + const framesUnderCursor = this.scene .getNonDeletedFramesLikes() .filter( (frame): frame is ExcalidrawFrameLikeElement => !frame.locked && isCursorInFrame(sceneCoords, frame, elementsMap), ); - return frames.length ? frames[frames.length - 1] : null; + if (!framesUnderCursor.length) { + return null; + } + + const topLayerFrame = framesUnderCursor.at(-1)!; + + const hitElement = this.getElementsAtPosition( + sceneCoords.x, + sceneCoords.y, + { + includeLockedElements: true, + }, + ).findLast((element) => !opts?.excludeElementIds?.[element.id]); + + if (hitElement) { + if ( + isFrameLikeElement(hitElement) && + // case: we're hitting a locked frame itself (frame's outline + // or later its bg once implemented) + !hitElement.locked + ) { + return topLayerFrame; + } + + const hitElementIndex = this.scene.getElementIndex(hitElement.id); + const topLayerFrameIndex = this.scene.getElementIndex(topLayerFrame.id); + + if ( + hitElementIndex !== -1 && + topLayerFrameIndex !== -1 && + hitElementIndex <= topLayerFrameIndex + ) { + return topLayerFrame; + } + + // to support a case of dragging a pre-existing frame child underneath + // a non-frame element covering the cursor + const currentFrame = opts?.currentFrameId + ? framesUnderCursor.find((frame) => frame.id === opts.currentFrameId) ?? + null + : null; + + if (currentFrame) { + return currentFrame; + } + + return hitElement.frameId + ? framesUnderCursor.find((frame) => frame.id === hitElement.frameId) ?? + null + : null; + } + + return topLayerFrame; + }; + + private updateFrameToHighlight = ( + frameToHighlight: AppState["frameToHighlight"], + ) => { + if (this.state.frameToHighlight !== frameToHighlight) { + this.setState({ frameToHighlight }); + } + }; + + private maybeUpdateFrameToHighlightOnPointerMove = ( + sceneCoords: { x: number; y: number }, + isOverScrollBar: boolean, + ) => { + // currently this function is being called even during pointerdown so we + // need to make sure we don't re-set the state when dragging and similar + // + // But, we still want to reset on pointermove in case the state is stale + // so we updte even for non-eligible tool types + if ( + this.state.newElement || + this.state.multiElement || + this.state.selectionElement || + this.state.selectedElementsAreBeingDragged + ) { + return; + } + + this.updateFrameToHighlight( + !isOverScrollBar && isEligibleFrameChildType(this.state.activeTool.type) + ? this.getTopLayerFrameAtSceneCoords(sceneCoords) + : null, + ); + }; + + private insertNewElements = (elements: readonly ExcalidrawElement[]) => { + if (!elements.length) { + return; + } + + const chunkedElements: ExcalidrawElement[][] = []; + + for (const element of elements) { + const currentChunk = chunkedElements[chunkedElements.length - 1]; + + if (currentChunk?.[0].frameId === element.frameId) { + currentChunk.push(element); + } else { + chunkedElements.push([element]); + } + } + + for (const chunk of chunkedElements) { + const frameId = chunk[0].frameId; + + const insertionIndex = frameId + ? getFrameChildrenInsertionIndex( + this.scene.getElementsIncludingDeleted(), + frameId, + ) + : null; + this.scene.insertElementsAtIndex(chunk, insertionIndex); + } + }; + + private insertNewElement = (element: ExcalidrawElement) => { + this.insertNewElements([element]); + + const frame = element.frameId + ? this.scene.getNonDeletedElement(element.frameId) + : null; + + this.updateFrameToHighlight( + frame && isFrameLikeElement(frame) ? frame : null, + ); }; private handleCanvasPointerMove = ( @@ -6809,6 +6986,14 @@ class App extends React.Component { } } + this.maybeUpdateFrameToHighlightOnPointerMove( + { + x: scenePointerX, + y: scenePointerY, + }, + isOverScrollBar, + ); + if ( !this.state.newElement && isActiveToolNonLinearSnappable(this.state.activeTool.type) @@ -8863,7 +9048,7 @@ class App extends React.Component { pressures: simulatePressure ? [] : [event.pressure], }); - this.scene.insertElement(element); + this.insertNewElement(element); this.setState((prevState) => { const nextSelectedElementIds = { @@ -8920,7 +9105,7 @@ class App extends React.Component { height, }); - this.scene.insertElement(element); + this.insertNewElement(element); return element; }; @@ -8974,7 +9159,7 @@ class App extends React.Component { link, }); - this.scene.insertElement(element); + this.insertNewElement(element); return element; }; @@ -9218,7 +9403,7 @@ class App extends React.Component { points: [pointFrom(0, 0), pointFrom(0, 0)], }); - this.scene.insertElement(element); + this.insertNewElement(element); if (isBindingElement(element)) { // Do the initial binding so the binding strategy has the initial state @@ -9376,7 +9561,7 @@ class App extends React.Component { selectionElement: element, }); } else { - this.scene.insertElement(element); + this.insertNewElement(element); this.setState({ multiElement: null, newElement: element, @@ -9409,7 +9594,7 @@ class App extends React.Component { ? newMagicFrameElement(constructorOpts) : newFrameElement(constructorOpts); - this.scene.insertElement(frame); + this.insertNewElement(frame); this.setState({ multiElement: null, @@ -9777,18 +9962,17 @@ class App extends React.Component { return; } - const selectedElementsHasAFrame = selectedElements.find((e) => + const selectedElementsHasAFrame = selectedElements.some((e) => isFrameLikeElement(e), ); - const topLayerFrame = this.getTopLayerFrameAtSceneCoords(pointerCoords); - const frameToHighlight = - topLayerFrame && !selectedElementsHasAFrame ? topLayerFrame : null; + const frameToHighlight = selectedElementsHasAFrame + ? null + : this.getTopLayerFrameAtSceneCoords(pointerCoords, { + currentFrameId: getCommonFrameId(selectedElements), + excludeElementIds: this.state.selectedElementIds, + }); // Only update the state if there is a difference - if (this.state.frameToHighlight !== frameToHighlight) { - flushSync(() => { - this.setState({ frameToHighlight }); - }); - } + this.updateFrameToHighlight(frameToHighlight); // Marking that click was used for dragging to check // if elements should be deselected on pointerup @@ -10817,7 +11001,6 @@ class App extends React.Component { this.scene.getElementsMapIncludingDeleted(), elementsInsideFrame, newElement, - this.state, ), ); } @@ -10877,9 +11060,14 @@ class App extends React.Component { } } else { // update the relationships between selected elements and frames - const topLayerFrame = this.getTopLayerFrameAtSceneCoords(sceneCoords); - const selectedElements = this.scene.getSelectedElements(this.state); + const topLayerFrame = this.getTopLayerFrameAtSceneCoords( + sceneCoords, + { + currentFrameId: getCommonFrameId(selectedElements), + excludeElementIds: this.state.selectedElementIds, + }, + ); let nextElements = this.scene.getElementsMapIncludingDeleted(); const updateGroupIdsAfterEditingGroup = ( @@ -10928,10 +11116,8 @@ class App extends React.Component { topLayerFrame && !this.state.selectedElementIds[topLayerFrame.id] ) { - const elementsToAdd = selectedElements.filter( - (element) => - element.frameId !== topLayerFrame.id && - isElementInFrame(element, nextElements, this.state), + const elementsToAdd = selectedElements.filter((element) => + isElementInFrame(element, nextElements, this.state), ); if (this.state.editingGroupId) { @@ -10942,7 +11128,6 @@ class App extends React.Component { nextElements, elementsToAdd, topLayerFrame, - this.state, ); } else if (!topLayerFrame) { if (this.state.editingGroupId) { @@ -11004,7 +11189,6 @@ class App extends React.Component { elementsMap, ), frame, - this, ); } @@ -11820,7 +12004,7 @@ class App extends React.Component { sceneY, gridPadding, ); - placeholders.forEach((el) => this.scene.insertElement(el)); + this.insertNewElements(placeholders); // Create, position, insert and select initialized (replacing placeholders) const initialized = await Promise.all( @@ -11948,6 +12132,7 @@ class App extends React.Component { type: "everything", elements: item.elements, randomizeSeed: true, + preserveFrameChildrenOrder: true, }).duplicatedElements, })); diff --git a/packages/excalidraw/components/LibraryMenuItems.tsx b/packages/excalidraw/components/LibraryMenuItems.tsx index 547c3bd66f..e619342ca6 100644 --- a/packages/excalidraw/components/LibraryMenuItems.tsx +++ b/packages/excalidraw/components/LibraryMenuItems.tsx @@ -199,6 +199,7 @@ export default function LibraryMenuItems({ type: "everything", elements: item.elements, randomizeSeed: true, + preserveFrameChildrenOrder: true, }).duplicatedElements, }; }); diff --git a/packages/excalidraw/components/Stats/Dimension.tsx b/packages/excalidraw/components/Stats/Dimension.tsx index cfc0f2dda1..a83cf835fc 100644 --- a/packages/excalidraw/components/Stats/Dimension.tsx +++ b/packages/excalidraw/components/Stats/Dimension.tsx @@ -206,7 +206,6 @@ const handleDimensionChange: DragInputCallbackType< scene.getElementsIncludingDeleted(), nextElementsInFrame, latestElement, - app, ); scene.replaceAllElements(updatedElements); @@ -302,7 +301,6 @@ const handleDragFinished: DragFinishedCallbackType = ({ app.scene.getElementsIncludingDeleted(), nextElementsInFrame, latestElement, - app, ); app.scene.replaceAllElements(updatedElements); diff --git a/packages/excalidraw/components/Stats/MultiDimension.tsx b/packages/excalidraw/components/Stats/MultiDimension.tsx index 4680858dcd..5224928ce7 100644 --- a/packages/excalidraw/components/Stats/MultiDimension.tsx +++ b/packages/excalidraw/components/Stats/MultiDimension.tsx @@ -261,7 +261,6 @@ const handleDimensionChange: DragInputCallbackType< scene.getElementsIncludingDeleted(), nextElementsInFrame, latestElement, - app, ); scene.replaceAllElements(updatedElements); @@ -416,7 +415,6 @@ const handleDragFinished: DragFinishedCallbackType = ({ app.scene.getElementsIncludingDeleted(), nextElementsInFrame, latestElement, - app, ); app.scene.replaceAllElements(updatedElements); diff --git a/packages/excalidraw/components/canvases/InteractiveCanvas.tsx b/packages/excalidraw/components/canvases/InteractiveCanvas.tsx index 2aabddfbab..dc7dcd9f9b 100644 --- a/packages/excalidraw/components/canvases/InteractiveCanvas.tsx +++ b/packages/excalidraw/components/canvases/InteractiveCanvas.tsx @@ -39,7 +39,7 @@ type InteractiveCanvasProps = { visibleElements: readonly NonDeletedExcalidrawElement[]; selectedElements: readonly NonDeletedExcalidrawElement[]; allElementsMap: NonDeletedSceneElementsMap; - sceneNonce: number | undefined; + canvasNonce: string; selectionNonce: number | undefined; scale: number; appState: InteractiveCanvasAppState; @@ -279,10 +279,10 @@ const areEqual = ( // This could be further optimised if needed, as we don't have to render interactive canvas on each scene mutation if ( prevProps.selectionNonce !== nextProps.selectionNonce || - prevProps.sceneNonce !== nextProps.sceneNonce || + prevProps.canvasNonce !== nextProps.canvasNonce || prevProps.scale !== nextProps.scale || // we need to memoize on elementsMap because they may have renewed - // even if sceneNonce didn't change (e.g. we filter elements out based + // even if canvasNonce didn't change (e.g. we filter elements out based // on appState) prevProps.elementsMap !== nextProps.elementsMap || prevProps.visibleElements !== nextProps.visibleElements || diff --git a/packages/excalidraw/components/canvases/NewElementCanvas.tsx b/packages/excalidraw/components/canvases/NewElementCanvas.tsx index 4310f1bd1a..dd8948b1f3 100644 --- a/packages/excalidraw/components/canvases/NewElementCanvas.tsx +++ b/packages/excalidraw/components/canvases/NewElementCanvas.tsx @@ -14,6 +14,7 @@ import type { RoughCanvas } from "roughjs/bin/canvas"; interface NewElementCanvasProps { appState: AppState; + newElement: NonNullable; elementsMap: RenderableElementsMap; allElementsMap: NonDeletedSceneElementsMap; scale: number; @@ -31,7 +32,7 @@ const NewElementCanvas = (props: NewElementCanvasProps) => { { canvas: canvasRef.current, scale: props.scale, - newElement: props.appState.newElement, + newElement: props.newElement, elementsMap: props.elementsMap, allElementsMap: props.allElementsMap, rc: props.rc, diff --git a/packages/excalidraw/components/canvases/StaticCanvas.tsx b/packages/excalidraw/components/canvases/StaticCanvas.tsx index 9e6a3324a4..e46ac041bc 100644 --- a/packages/excalidraw/components/canvases/StaticCanvas.tsx +++ b/packages/excalidraw/components/canvases/StaticCanvas.tsx @@ -23,7 +23,7 @@ type StaticCanvasProps = { elementsMap: RenderableElementsMap; allElementsMap: NonDeletedSceneElementsMap; visibleElements: readonly NonDeletedExcalidrawElement[]; - sceneNonce: number | undefined; + canvasNonce: string; selectionNonce: number | undefined; scale: number; appState: StaticCanvasAppState; @@ -110,10 +110,10 @@ const areEqual = ( nextProps: StaticCanvasProps, ) => { if ( - prevProps.sceneNonce !== nextProps.sceneNonce || + prevProps.canvasNonce !== nextProps.canvasNonce || prevProps.scale !== nextProps.scale || // we need to memoize on elementsMap because they may have renewed - // even if sceneNonce didn't change (e.g. we filter elements out based + // even if canvasNonce didn't change (e.g. we filter elements out based // on appState) prevProps.elementsMap !== nextProps.elementsMap || prevProps.visibleElements !== nextProps.visibleElements diff --git a/packages/excalidraw/polyfill.ts b/packages/excalidraw/polyfill.ts index b8b080f754..6de2c40eab 100644 --- a/packages/excalidraw/polyfill.ts +++ b/packages/excalidraw/polyfill.ts @@ -23,6 +23,94 @@ const polyfill = () => { }); } + if (!Array.prototype.findLast) { + Object.defineProperty(Array.prototype, "findLast", { + value: function ( + this: T[], + predicate: (value: T, index: number, array: T[]) => unknown, + thisArg?: unknown, + ) { + return this + .slice() + .reverse() + .find((value, index) => + predicate.call(thisArg, value, this.length - index - 1, this), + ); + }, + writable: true, + enumerable: false, + configurable: true, + }); + } + + if (!Array.prototype.findIndex) { + Object.defineProperty(Array.prototype, "findIndex", { + value: function ( + this: T[], + predicate: (value: T, index: number, array: T[]) => unknown, + thisArg?: unknown, + ) { + for (let index = 0; index < this.length; index++) { + if (predicate.call(thisArg, this[index], index, this)) { + return index; + } + } + + return -1; + }, + writable: true, + enumerable: false, + configurable: true, + }); + } + + if (!Array.prototype.findLastIndex) { + Object.defineProperty(Array.prototype, "findLastIndex", { + value: function ( + this: T[], + predicate: (value: T, index: number, array: T[]) => unknown, + thisArg?: unknown, + ) { + const index = this + .slice() + .reverse() + .findIndex((value, index) => + predicate.call(thisArg, value, this.length - index - 1, this), + ); + + return index === -1 ? -1 : this.length - index - 1; + }, + writable: true, + enumerable: false, + configurable: true, + }); + } + + if (!Array.prototype.toReversed) { + Object.defineProperty(Array.prototype, "toReversed", { + value: function (this: T[]) { + return this.slice().reverse(); + }, + writable: true, + enumerable: false, + configurable: true, + }); + } + + if (!Array.prototype.toSorted) { + Object.defineProperty(Array.prototype, "toSorted", { + value: function ( + this: T[], + compareFn?: (a: T, b: T) => number, + ) { + return this.slice().sort(compareFn); + }, + writable: true, + enumerable: false, + configurable: true, + }); + } + if (!Element.prototype.replaceChildren) { Element.prototype.replaceChildren = function (...nodes) { this.innerHTML = ""; diff --git a/packages/excalidraw/scene/Renderer.ts b/packages/excalidraw/scene/Renderer.ts index 56ae3e5c58..fdcee2a68d 100644 --- a/packages/excalidraw/scene/Renderer.ts +++ b/packages/excalidraw/scene/Renderer.ts @@ -1,9 +1,15 @@ -import { isElementInViewport } from "@excalidraw/element"; +import { + getCommonFrameId, + getFrameChildrenInsertionIndex, + isElementInViewport, +} from "@excalidraw/element"; -import { memoize, toBrandedType } from "@excalidraw/common"; +import { arrayToMap, memoize, toBrandedType } from "@excalidraw/common"; import type { ExcalidrawElement, + ExcalidrawFrameLikeElement, + NonDeleted, NonDeletedElementsMap, NonDeletedExcalidrawElement, } from "@excalidraw/element/types"; @@ -16,6 +22,21 @@ import type { RenderableElementsMap } from "./types"; import type { AppState } from "../types"; +type GetRenderableElementsOpts = { + zoom: AppState["zoom"]; + offsetLeft: AppState["offsetLeft"]; + offsetTop: AppState["offsetTop"]; + scrollX: AppState["scrollX"]; + scrollY: AppState["scrollY"]; + height: AppState["height"]; + width: AppState["width"]; + editingTextElement: AppState["editingTextElement"]; + newElement: AppState["newElement"]; + selectedElements: readonly NonDeletedExcalidrawElement[]; + selectedElementsAreBeingDragged: AppState["selectedElementsAreBeingDragged"]; + frameToHighlight: AppState["frameToHighlight"]; +}; + export class Renderer { private scene: Scene; @@ -23,9 +44,121 @@ export class Renderer { this.scene = scene; } - public getRenderableElements = (() => { - const getVisibleCanvasElements = ({ - elementsMap, + private getVisibleCanvasElements({ + elementsMap, + zoom, + offsetLeft, + offsetTop, + scrollX, + scrollY, + height, + width, + }: { + elementsMap: NonDeletedElementsMap; + zoom: AppState["zoom"]; + offsetLeft: AppState["offsetLeft"]; + offsetTop: AppState["offsetTop"]; + scrollX: AppState["scrollX"]; + scrollY: AppState["scrollY"]; + height: AppState["height"]; + width: AppState["width"]; + }): readonly NonDeletedExcalidrawElement[] { + const visibleElements: NonDeletedExcalidrawElement[] = []; + for (const element of elementsMap.values()) { + if ( + isElementInViewport( + element, + width, + height, + { + zoom, + offsetLeft, + offsetTop, + scrollX, + scrollY, + }, + elementsMap, + ) + ) { + visibleElements.push(element); + } + } + return visibleElements; + } + + private getRenderableElementsMap({ + elements, + editingTextElement, + newElement, + }: { + elements: readonly NonDeletedExcalidrawElement[]; + editingTextElement: AppState["editingTextElement"]; + newElement: AppState["newElement"]; + }) { + const elementsMap = toBrandedType(new Map()); + const newElementCanvasElement = newElement?.frameId ? null : newElement; + + for (const element of elements) { + if (newElementCanvasElement?.id === element.id) { + continue; + } + + // we don't want to render text element that's being currently edited + // (it's rendered on remote only) + if ( + !editingTextElement || + editingTextElement.type !== "text" || + element.id !== editingTextElement.id + ) { + elementsMap.set(element.id, element); + } + } + return { elementsMap, newElementCanvasElement }; + } + + private sortSelectedElementsIntoHighlightedFrame< + T extends ExcalidrawElement, + >({ + visibleElements, + selectedElements, + frameToHighlight, + }: { + selectedElements: readonly NonDeletedExcalidrawElement[]; + visibleElements: readonly T[]; + frameToHighlight: NonDeleted; + }): readonly T[] { + if (!selectedElements.length) { + return visibleElements; + } + + // we assume all selected elements are eligible frame children if + // frameToHighlight is defined + const selectedElementsMap = arrayToMap(selectedElements); + + // thus, all deselected elements are the ones we won't reorder + const deselectedElements = visibleElements.filter( + (element) => !selectedElementsMap.has(element.id), + ); + + const insertionIndex = getFrameChildrenInsertionIndex( + deselectedElements, + frameToHighlight.id, + ); + + if (insertionIndex === null) { + return visibleElements; + } + + return [ + ...deselectedElements.slice(0, insertionIndex), + ...selectedElements, + ...deselectedElements.slice(insertionIndex), + ] as readonly T[]; + } + + private _getRenderableElements = memoize( + ({ + canvasNonce, zoom, offsetLeft, offsetTop, @@ -33,70 +166,27 @@ export class Renderer { scrollY, height, width, - }: { - elementsMap: NonDeletedElementsMap; - zoom: AppState["zoom"]; - offsetLeft: AppState["offsetLeft"]; - offsetTop: AppState["offsetTop"]; - scrollX: AppState["scrollX"]; - scrollY: AppState["scrollY"]; - height: AppState["height"]; - width: AppState["width"]; - }): readonly NonDeletedExcalidrawElement[] => { - const visibleElements: NonDeletedExcalidrawElement[] = []; - for (const element of elementsMap.values()) { - if ( - isElementInViewport( - element, - width, - height, - { - zoom, - offsetLeft, - offsetTop, - scrollX, - scrollY, - }, - elementsMap, - ) - ) { - visibleElements.push(element); - } - } - return visibleElements; - }; - - const getRenderableElements = ({ - elements, editingTextElement, - newElementId, - }: { - elements: readonly NonDeletedExcalidrawElement[]; - editingTextElement: AppState["editingTextElement"]; - newElementId: ExcalidrawElement["id"] | undefined; + newElement, + }: Omit< + GetRenderableElementsOpts, + | "selectedElements" + | "selectedElementsAreBeingDragged" + | "frameToHighlight" + > & { + canvasNonce: string; }) => { - const elementsMap = toBrandedType(new Map()); + const elements = this.scene.getNonDeletedElements(); - for (const element of elements) { - if (newElementId === element.id) { - continue; - } + const { elementsMap, newElementCanvasElement } = + this.getRenderableElementsMap({ + elements, + editingTextElement, + newElement, + }); - // we don't want to render text element that's being currently edited - // (it's rendered on remote only) - if ( - !editingTextElement || - editingTextElement.type !== "text" || - element.id !== editingTextElement.id - ) { - elementsMap.set(element.id, element); - } - } - return elementsMap; - }; - - return memoize( - ({ + const visibleElements = this.getVisibleCanvasElements({ + elementsMap, zoom, offsetLeft, offsetTop, @@ -104,52 +194,69 @@ export class Renderer { scrollY, height, width, - editingTextElement, - newElementId, - // cache-invalidation nonce - sceneNonce: _sceneNonce, - }: { - zoom: AppState["zoom"]; - offsetLeft: AppState["offsetLeft"]; - offsetTop: AppState["offsetTop"]; - scrollX: AppState["scrollX"]; - scrollY: AppState["scrollY"]; - height: AppState["height"]; - width: AppState["width"]; - editingTextElement: AppState["editingTextElement"]; - /** note: first render of newElement will always bust the cache - * (we'd have to prefilter elements outside of this function) */ - newElementId: ExcalidrawElement["id"] | undefined; - sceneNonce: ReturnType["getSceneNonce"]>; - }) => { - const elements = this.scene.getNonDeletedElements(); + }); - const elementsMap = getRenderableElements({ - elements, - editingTextElement, - newElementId, + return { + elementsMap, + visibleElements, + newElementCanvasElement, + canvasNonce, + }; + }, + ); + + public getRenderableElements = (opts: GetRenderableElementsOpts) => { + const { newElement } = opts; + const canvasNonce = `${this.scene.getSceneNonce()}${ + newElement?.frameId ? `:${newElement.versionNonce}` : "" + }`; + + const ret = this._getRenderableElements({ + canvasNonce, + + // don't spread `opts` because we don't want to memoize on some props + + zoom: opts.zoom, + offsetLeft: opts.offsetLeft, + offsetTop: opts.offsetTop, + scrollX: opts.scrollX, + scrollY: opts.scrollY, + height: opts.height, + width: opts.width, + editingTextElement: opts.editingTextElement, + newElement: opts.newElement, + }); + + // if we're dragging elements over a frame, reorder the selected elements + // inside the frame during render (we don't set the `element.frameId` until + // pointerup else we'd have to painstainly restore the orig index if user + // didn't end up adding elements to the frame) + if ( + opts.frameToHighlight && + opts.selectedElementsAreBeingDragged && + // if all dragged elements are already in the frame, don't reorder + getCommonFrameId(opts.selectedElements) !== opts.frameToHighlight.id + ) { + const reorderedVisibleElements = + this.sortSelectedElementsIntoHighlightedFrame({ + visibleElements: ret.visibleElements, + selectedElements: opts.selectedElements, + frameToHighlight: opts.frameToHighlight, }); - const visibleElements = getVisibleCanvasElements({ - elementsMap, - zoom, - offsetLeft, - offsetTop, - scrollX, - scrollY, - height, - width, - }); + return { + ...ret, + visibleElements: reorderedVisibleElements, + }; + } - return { elementsMap, visibleElements }; - }, - ); - })(); + return ret; + }; // NOTE Doesn't destroy everything (scene, rc, etc.) because it may not be // safe to break TS contract here (for upstream cases) public destroy() { renderStaticSceneThrottled.cancel(); - this.getRenderableElements.clear(); + this._getRenderableElements.clear(); } } diff --git a/packages/excalidraw/tests/clipboard.test.tsx b/packages/excalidraw/tests/clipboard.test.tsx index f3fda88d11..9520810583 100644 --- a/packages/excalidraw/tests/clipboard.test.tsx +++ b/packages/excalidraw/tests/clipboard.test.tsx @@ -305,8 +305,88 @@ describe("pasting & frames", () => { await waitFor(() => { expect(h.elements.length).toBe(2); + expect(h.elements[0].type).toBe(rect.type); + expect(h.elements[0].frameId).toBe(frame.id); + expect(h.elements[1].id).toBe(frame.id); + expect(h.elements[0].index! < frame.index!).toBe(true); + }); + }); + + it("should layer pasted elements above the highest frame child", async () => { + const frame = API.createElement({ + type: "frame", + width: 100, + height: 100, + x: 0, + y: 0, + }); + const frameChild = API.createElement({ + id: "frameChild", + type: "rectangle", + x: 10, + y: 10, + width: 20, + height: 20, + frameId: frame.id, + }); + const rect = API.createElement({ type: "rectangle" }); + + API.setElements([frameChild, frame]); + + const clipboardJSON = await serializeAsClipboardJSON({ + elements: [rect], + files: null, + }); + + mouse.moveTo(50, 50); + + pasteWithCtrlCmdV(clipboardJSON); + + await waitFor(() => { + expect(h.elements.length).toBe(3); expect(h.elements[1].type).toBe(rect.type); expect(h.elements[1].frameId).toBe(frame.id); + expect(h.elements.map((element) => element.id)).toEqual([ + frameChild.id, + h.elements[1].id, + frame.id, + ]); + expect(h.elements[1].index! > frameChild.index!).toBe(true); + expect(h.elements[1].index! < frame.index!).toBe(true); + }); + }); + + it("should preserve denormalized pasted frame child order", async () => { + const frame = API.createElement({ + type: "frame", + width: 100, + height: 100, + x: 0, + y: 0, + }); + const frameChild = API.createElement({ + type: "rectangle", + x: 10, + y: 10, + width: 20, + height: 20, + frameId: frame.id, + }); + + const clipboardJSON = await serializeAsClipboardJSON({ + elements: [frame, frameChild], + files: null, + }); + + mouse.moveTo(200, 200); + + pasteWithCtrlCmdV(clipboardJSON); + + await waitFor(() => { + expect(h.elements.length).toBe(2); + expect(h.elements[0].type).toBe(frame.type); + expect(h.elements[1].type).toBe(frameChild.type); + expect(h.elements[1].frameId).toBe(h.elements[0].id); }); }); @@ -379,8 +459,9 @@ describe("pasting & frames", () => { await waitFor(() => { expect(h.elements.length).toBe(3); - expect(h.elements[1].type).toBe(rect.type); - expect(h.elements[1].frameId).toBe(frame.id); + expect(h.elements[0].type).toBe(rect.type); + expect(h.elements[0].frameId).toBe(frame.id); + expect(h.elements[1].id).toBe(frame.id); expect(h.elements[2].type).toBe(rect2.type); expect(h.elements[2].frameId).toBe(null); }); @@ -422,10 +503,11 @@ describe("pasting & frames", () => { await waitFor(() => { expect(h.elements.length).toBe(3); - expect(h.elements[1].type).toBe(rect.type); + expect(h.elements[0].type).toBe(rect.type); + expect(h.elements[0].frameId).toBe(frame.id); + expect(h.elements[1].type).toBe(rect2.type); expect(h.elements[1].frameId).toBe(frame.id); - expect(h.elements[2].type).toBe(rect2.type); - expect(h.elements[2].frameId).toBe(frame.id); + expect(h.elements[2].id).toBe(frame.id); }); }); @@ -473,8 +555,9 @@ describe("pasting & frames", () => { await waitFor(() => { expect(h.elements.length).toBe(4); - expect(h.elements[1].type).toBe(rect.type); - expect(h.elements[1].frameId).toBe(frame.id); + expect(h.elements[0].type).toBe(rect.type); + expect(h.elements[0].frameId).toBe(frame.id); + expect(h.elements[1].id).toBe(frame.id); expect(h.elements[2].type).toBe(rect2.type); expect(h.elements[2].frameId).toBe(h.elements[3].id); expect(h.elements[3].type).toBe(frame2.type); diff --git a/packages/excalidraw/wysiwyg/textWysiwyg.test.tsx b/packages/excalidraw/wysiwyg/textWysiwyg.test.tsx index a092576de0..f044553fd5 100644 --- a/packages/excalidraw/wysiwyg/textWysiwyg.test.tsx +++ b/packages/excalidraw/wysiwyg/textWysiwyg.test.tsx @@ -769,6 +769,63 @@ describe("textWysiwyg", () => { ]); }); + it("should not add bound text to a frame when its container is not a frame child", async () => { + const frame = API.createElement({ + type: "frame", + x: 0, + y: 0, + width: 200, + height: 200, + }); + const rectangle = API.createElement({ + type: "rectangle", + x: 10, + y: 20, + width: 90, + height: 75, + backgroundColor: "red", + }); + API.setElements([frame, rectangle]); + + mouse.doubleClickAt(rectangle.x + 10, rectangle.y + 10); + + const text = h.elements[2] as ExcalidrawTextElementWithContainer; + expect(text.type).toBe("text"); + expect(text.containerId).toBe(rectangle.id); + expect(text.frameId).toBe(null); + }); + + it("should bind text to a frame child container when single clicking its center", async () => { + const frame = API.createElement({ + type: "frame", + x: 0, + y: 0, + width: 200, + height: 200, + }); + const rectangle = API.createElement({ + type: "rectangle", + x: 10, + y: 20, + width: 90, + height: 75, + backgroundColor: "red", + frameId: frame.id, + }); + API.setElements([rectangle, frame]); + + UI.clickTool("text"); + mouse.clickAt( + rectangle.x + rectangle.width / 2, + rectangle.y + rectangle.height / 2, + ); + + const text = h.elements[1] as ExcalidrawTextElementWithContainer; + expect(text.type).toBe("text"); + expect(text.containerId).toBe(rectangle.id); + expect(text.frameId).toBe(frame.id); + }); + it("should set the text element angle to same as container angle when binding to rotated container", async () => { const rectangle = API.createElement({ type: "rectangle",