From 0bd3a077bf3e7be1e072a4e554f9bcdcfa43e66a Mon Sep 17 00:00:00 2001 From: dwelle <5153846+dwelle@users.noreply.github.com> Date: Sun, 14 Jun 2026 14:10:24 +0200 Subject: [PATCH] feat(editor): remove non-overlapping frame children on restore --- packages/element/tests/frame.test.tsx | 105 +++++++++ packages/excalidraw/data/restore.ts | 98 ++++++++- .../excalidraw/tests/data/restore.test.ts | 205 +++++++++++++++++- packages/excalidraw/tests/helpers/svg.ts | 52 +++++ 4 files changed, 453 insertions(+), 7 deletions(-) create mode 100644 packages/excalidraw/tests/helpers/svg.ts diff --git a/packages/element/tests/frame.test.tsx b/packages/element/tests/frame.test.tsx index 635a5bc2e9..851f0fecb6 100644 --- a/packages/element/tests/frame.test.tsx +++ b/packages/element/tests/frame.test.tsx @@ -3,6 +3,7 @@ import { Excalidraw, } from "@excalidraw/excalidraw"; import { arrayToMap } from "@excalidraw/common"; +import { degreesToRadians, pointFrom } from "@excalidraw/math"; import { API } from "@excalidraw/excalidraw/tests/helpers/api"; import { Keyboard, Pointer, UI } from "@excalidraw/excalidraw/tests/helpers/ui"; @@ -14,6 +15,8 @@ import { import { getSelectedElements } from "@excalidraw/excalidraw/scene"; +import type { Degrees } from "@excalidraw/math"; + import { elementOverlapsWithFrame } from "../src/frame"; import type { @@ -154,6 +157,108 @@ describe("adding elements to frames", () => { ).toBe(true); }); + it("uses precise geometry (not the bounding box) to detect frame overlap", () => { + // a 300x200 frame at the origin, with shapes positioned just outside each + // corner so that their bounding box clips the frame but their actual + // outline does not. The precise `isElementIntersectingFrame` must report + // no overlap. + const overlapFrame = API.createElement({ + type: "frame", + x: 0, + y: 0, + width: 300, + height: 200, + }); + // clips the bottom-right corner; the rhombus tip stays out + const diamond = API.createElement({ + type: "diamond", + x: 290, + y: 175, + width: 100, + height: 100, + }); + // clips the bottom-left corner; rotated (NOT 45°, which would degenerate + // into an axis-aligned square whose shape fills its AABB) + const diamondRotated = API.createElement({ + type: "diamond", + x: -50, + y: 195, + width: 100, + height: 100, + angle: degreesToRadians(45 as Degrees), + }); + // clips the top-left corner; the curve doesn't reach the corner + const ellipse = API.createElement({ + type: "ellipse", + x: -90, + y: -90, + width: 100, + height: 100, + }); + // clips the top edge; the polyline weaves outside the frame + const line = API.createElement({ + type: "line", + x: 130, + y: -20, + width: 293, + height: 183, + points: [ + pointFrom(0, 0), + pointFrom(-165, -96), + pointFrom(-293, -16), + pointFrom(-162, 87), + ], + }); + // clips the bottom-left corner; the stroke stays outside + const freedraw = API.createElement({ + type: "freedraw", + x: -15, + y: 180, + width: 207, + height: 209, + points: [ + pointFrom(0, 0), + pointFrom(-69, 99), + pointFrom(-33, 183), + pointFrom(48, 209), + pointFrom(124, 195), + pointFrom(138, 145), + pointFrom(123, 85), + ], + }); + // clips the top-right corner, but its rounded corner is cut away from + // exactly that corner — so its actual outline stays outside + const roundedRect = API.createElement({ + type: "rectangle", + x: 295, + y: -100, + width: 160, + height: 110, + roundness: { type: 3 }, + }); + + API.setElements([ + overlapFrame, + diamond, + diamondRotated, + ellipse, + line, + freedraw, + roundedRect, + ]); + const elementsMap = arrayToMap(h.elements); + const f = overlapFrame as ExcalidrawFrameLikeElement; + + expect(elementOverlapsWithFrame(diamond, f, elementsMap)).toBe(false); + expect(elementOverlapsWithFrame(diamondRotated, f, elementsMap)).toBe( + false, + ); + expect(elementOverlapsWithFrame(ellipse, f, elementsMap)).toBe(false); + expect(elementOverlapsWithFrame(line, f, elementsMap)).toBe(false); + expect(elementOverlapsWithFrame(freedraw, f, elementsMap)).toBe(false); + expect(elementOverlapsWithFrame(roundedRect, f, elementsMap)).toBe(false); + }); + it("should not add a newly created element to a frame behind a non-frame element", () => { const cover = API.createElement({ id: "cover", diff --git a/packages/excalidraw/data/restore.ts b/packages/excalidraw/data/restore.ts index 58c2ec26e2..43917caf34 100644 --- a/packages/excalidraw/data/restore.ts +++ b/packages/excalidraw/data/restore.ts @@ -54,6 +54,11 @@ import { getNormalizedDimensions } from "@excalidraw/element"; import { isInvisiblySmallElement } from "@excalidraw/element"; +import { + elementOverlapsWithFrame, + isFrameLikeElement, +} from "@excalidraw/element"; + import type { LocalPoint, Radians } from "@excalidraw/math"; import type { @@ -744,20 +749,78 @@ const repairBoundElement = ( }; /** - * Remove an element's frameId if its containing frame is non-existent + * Remove an element's frameId if: + * - its containing frame is non-existent (or isn't actually a frame), or + * - the element no longer (precisely) overlaps its containing frame. + * + * Frame membership is treated atomically per group: a grouped element keeps its + * frameId as long as ANY element of its (outermost) group still overlaps the + * frame, so we never split a group across frame membership. Bound text inherits + * its container's geometry/membership. * * NOTE mutates elements. */ const repairFrameMembership = ( element: Mutable, elementsMap: Map>, + /** elements bucketed by outermost groupId (see `restoreElements`) */ + elementsByGroup: Map[]>, + /** memoizes the overlap decision per `${frameId}:${groupId}` */ + checkedFrameGroups: Map, ) => { - if (element.frameId) { - const containingFrame = elementsMap.get(element.frameId); + if (!element.frameId) { + return; + } - if (!containingFrame) { - element.frameId = null; + const containingFrame = elementsMap.get(element.frameId); + + // frame no longer exists (or the frameId doesn't point to a frame) + if (!containingFrame || !isFrameLikeElement(containingFrame)) { + element.frameId = null; + return; + } + + // don't recompute geometry for deleted elements + if (element.isDeleted) { + return; + } + + // a bound text follows its container's geometry / membership + const geometryElement = + isTextElement(element) && element.containerId + ? elementsMap.get(element.containerId) ?? element + : element; + + const outerGroupId = + geometryElement.groupIds[geometryElement.groupIds.length - 1]; + + let overlapsFrame: boolean; + if (outerGroupId) { + const cacheKey = `${containingFrame.id}:${outerGroupId}`; + const cached = checkedFrameGroups.get(cacheKey); + if (cached !== undefined) { + overlapsFrame = cached; + } else { + // group membership is atomic — keep the element as long as ANY element + // of its group still overlaps the frame, so we never split a group. + const groupElements = elementsByGroup.get(outerGroupId) ?? [ + geometryElement, + ]; + overlapsFrame = groupElements.some((groupElement) => + elementOverlapsWithFrame(groupElement, containingFrame, elementsMap), + ); + checkedFrameGroups.set(cacheKey, overlapsFrame); } + } else { + overlapsFrame = elementOverlapsWithFrame( + geometryElement, + containingFrame, + elementsMap, + ); + } + + if (!overlapsFrame) { + element.frameId = null; } }; @@ -836,9 +899,32 @@ export const restoreElements = ( // repair binding. Mutates elements. const restoredElementsMap = arrayToMap(restoredElements); + + // index elements by their outermost groupId once, so frame-membership repair + // can resolve a group's members in O(1) instead of scanning all elements per + // group (getElementsInGroup) — a big win on large, heavily-grouped scenes. + const elementsByGroup = new Map[]>(); + for (const element of restoredElements) { + const outerGroupId = element.groupIds[element.groupIds.length - 1]; + if (outerGroupId) { + const members = elementsByGroup.get(outerGroupId); + if (members) { + members.push(element); + } else { + elementsByGroup.set(outerGroupId, [element]); + } + } + } + + const checkedFrameGroups = new Map(); for (const element of restoredElements) { if (element.frameId) { - repairFrameMembership(element, restoredElementsMap); + repairFrameMembership( + element, + restoredElementsMap, + elementsByGroup, + checkedFrameGroups, + ); } if (isTextElement(element) && element.containerId) { diff --git a/packages/excalidraw/tests/data/restore.test.ts b/packages/excalidraw/tests/data/restore.test.ts index aa634d32dc..3bc23d7597 100644 --- a/packages/excalidraw/tests/data/restore.test.ts +++ b/packages/excalidraw/tests/data/restore.test.ts @@ -1,7 +1,12 @@ import { pointFrom } from "@excalidraw/math"; import { vi } from "vitest"; -import { DEFAULT_SIDEBAR, FONT_FAMILY, ROUNDNESS } from "@excalidraw/common"; +import { + arrayToMap, + DEFAULT_SIDEBAR, + FONT_FAMILY, + ROUNDNESS, +} from "@excalidraw/common"; import { newElementWith } from "@excalidraw/element"; import * as sizeHelpers from "@excalidraw/element"; @@ -1140,3 +1145,201 @@ describe("repairing bindings", () => { ]); }); }); + +describe("repairing frame membership", () => { + const makeFrame = () => + API.createElement({ + type: "frame", + id: "frame", + x: 0, + y: 0, + width: 100, + height: 100, + }); + + it("keeps frameId for an element fully inside the frame", () => { + const frame = makeFrame(); + const inside = API.createElement({ + type: "rectangle", + id: "inside", + x: 10, + y: 10, + width: 20, + height: 20, + frameId: frame.id, + }); + + const restoredElementsMap = arrayToMap( + restore.restoreElements([frame, inside], null, { repairBindings: true }), + ); + expect(restoredElementsMap.get("inside")?.frameId).toBe(frame.id); + }); + + it("keeps frameId for an element intersecting the frame border", () => { + const frame = makeFrame(); + // spans x:[90,130] — crosses the frame's right edge (x=100) + const straddling = API.createElement({ + type: "rectangle", + id: "straddling", + x: 90, + y: 10, + width: 40, + height: 20, + frameId: frame.id, + }); + + const restoredElementsMap = arrayToMap( + restore.restoreElements([frame, straddling], null, { + repairBindings: true, + }), + ); + expect(restoredElementsMap.get("straddling")?.frameId).toBe(frame.id); + }); + + it("removes frameId for an element fully outside the frame", () => { + const frame = makeFrame(); + const outside = API.createElement({ + type: "rectangle", + id: "outside", + x: 200, + y: 200, + width: 20, + height: 20, + frameId: frame.id, + }); + + const restoredElementsMap = arrayToMap( + restore.restoreElements([frame, outside], null, { repairBindings: true }), + ); + expect(restoredElementsMap.get("outside")?.frameId).toBe(null); + }); + + it("removes frameId when the containing frame no longer exists", () => { + const orphan = API.createElement({ + type: "rectangle", + id: "orphan", + x: 10, + y: 10, + frameId: "non-existent-frame", + }); + + const restoredElementsMap = arrayToMap( + restore.restoreElements([orphan], null, { repairBindings: true }), + ); + expect(restoredElementsMap.get("orphan")?.frameId).toBe(null); + }); + + it("does NOT touch frameId when repairBindings is off", () => { + const frame = makeFrame(); + const outside = API.createElement({ + type: "rectangle", + id: "outside", + x: 200, + y: 200, + width: 20, + height: 20, + frameId: frame.id, + }); + + const restoredElementsMap = arrayToMap( + restore.restoreElements([frame, outside], null, { + repairBindings: false, + }), + ); + expect(restoredElementsMap.get("outside")?.frameId).toBe(frame.id); + }); + + it("keeps a group atomic: members stay if ANY group member overlaps", () => { + const frame = makeFrame(); + const insideMember = API.createElement({ + type: "rectangle", + id: "insideMember", + x: 10, + y: 10, + width: 20, + height: 20, + groupIds: ["g1"], + frameId: frame.id, + }); + const outsideMember = API.createElement({ + type: "rectangle", + id: "outsideMember", + x: 300, + y: 300, + width: 20, + height: 20, + groupIds: ["g1"], + frameId: frame.id, + }); + + const restoredElementsMap = arrayToMap( + restore.restoreElements([frame, insideMember, outsideMember], null, { + repairBindings: true, + }), + ); + // the whole group overlaps (via insideMember), so neither is evicted + expect(restoredElementsMap.get("insideMember")?.frameId).toBe(frame.id); + expect(restoredElementsMap.get("outsideMember")?.frameId).toBe(frame.id); + }); + + it("removes frameId for a whole group when no member overlaps", () => { + const frame = makeFrame(); + const a = API.createElement({ + type: "rectangle", + id: "a", + x: 200, + y: 200, + width: 20, + height: 20, + groupIds: ["g1"], + frameId: frame.id, + }); + const b = API.createElement({ + type: "rectangle", + id: "b", + x: 300, + y: 300, + width: 20, + height: 20, + groupIds: ["g1"], + frameId: frame.id, + }); + + const restoredElementsMap = arrayToMap( + restore.restoreElements([frame, a, b], null, { repairBindings: true }), + ); + expect(restoredElementsMap.get("a")?.frameId).toBe(null); + expect(restoredElementsMap.get("b")?.frameId).toBe(null); + }); + + it("bound text follows its container's frame membership", () => { + const frameOutside = makeFrame(); + const container = API.createElement({ + type: "rectangle", + id: "container", + x: 300, + y: 300, + width: 40, + height: 40, + frameId: frameOutside.id, + boundElements: [{ type: "text", id: "label" }], + }); + const label = API.createElement({ + type: "text", + id: "label", + x: 305, + y: 305, + containerId: container.id, + frameId: frameOutside.id, + }); + + const restoredElementsMap = arrayToMap( + restore.restoreElements([frameOutside, container, label], null, { + repairBindings: true, + }), + ); + // container is fully outside -> both it and its bound text leave the frame + expect(restoredElementsMap.get("container")?.frameId).toBe(null); + expect(restoredElementsMap.get("label")?.frameId).toBe(null); + }); +}); diff --git a/packages/excalidraw/tests/helpers/svg.ts b/packages/excalidraw/tests/helpers/svg.ts new file mode 100644 index 0000000000..9703a120fc --- /dev/null +++ b/packages/excalidraw/tests/helpers/svg.ts @@ -0,0 +1,52 @@ +import type { + ExcalidrawElement, + NonDeletedExcalidrawElement, +} from "@excalidraw/element/types"; + +import { exportToSvg } from "../../scene/export"; + +/** + * Test-only helper: render the supplied elements to an SVG string for visual + * debugging (e.g. to eyeball frame-membership / overlap geometry). + * + * Frames are drawn with their outline but are NOT clipped, and every element's + * `frameId` is ignored, so elements that fall outside a frame stay visible and + * within the exported viewBox instead of being clipped or cropped away. + * + * Pass `outFile` to also write the SVG to disk — open it in a browser to view. + * + * @example + * await exportElementsToSVG([frame, diamond], { + * outFile: "/tmp/frame-debug.svg", + * }); + */ +export const exportElementsToSVG = async ( + elements: readonly ExcalidrawElement[], + opts?: { outFile?: string; padding?: number; background?: string }, +): Promise => { + // ignore frame membership so nothing gets clipped/cropped out of view + const rootElements = elements.map( + (element) => ({ ...element, frameId: null } as NonDeletedExcalidrawElement), + ); + + const svg = await exportToSvg( + rootElements, + { + exportBackground: true, + viewBackgroundColor: opts?.background ?? "#ffffff", + exportPadding: opts?.padding ?? 24, + frameRendering: { enabled: true, name: true, outline: true, clip: false }, + }, + null, + { skipInliningFonts: true }, + ); + + const svgString = svg.outerHTML; + + if (opts?.outFile) { + const { writeFileSync } = await import("fs"); + writeFileSync(opts.outFile, svgString); + } + + return svgString; +};