diff --git a/dev-docs/docs/codebase/frames.mdx b/dev-docs/docs/codebase/frames.mdx new file mode 100644 index 00000000..45a551f2 --- /dev/null +++ b/dev-docs/docs/codebase/frames.mdx @@ -0,0 +1,22 @@ +# Frames + +## Ordering + +Frames should be ordered where frame children come first, followed by the frame element itself: + +``` +[ + other_element, + frame1_child1, + frame1_child2, + frame1, + other_element, + frame2_child1, + frame2_child2, + frame2, + other_element, + ... +] +``` + +If not oredered correctly, the editor will still function, but the elements may not be rendered and clipped correctly. Further, the renderer relies on this ordering for performance optimizations. diff --git a/dev-docs/sidebars.js b/dev-docs/sidebars.js index 257d16b5..2b4ab8d8 100644 --- a/dev-docs/sidebars.js +++ b/dev-docs/sidebars.js @@ -23,7 +23,11 @@ const sidebars = { }, items: ["introduction/development", "introduction/contributing"], }, - { type: "category", label: "Codebase", items: ["codebase/json-schema"] }, + { + type: "category", + label: "Codebase", + items: ["codebase/json-schema", "codebase/frames"], + }, { type: "category", label: "@excalidraw/excalidraw", diff --git a/src/frame.test.tsx b/src/frame.test.tsx index 75c515d9..5dbfdb05 100644 --- a/src/frame.test.tsx +++ b/src/frame.test.tsx @@ -177,7 +177,7 @@ describe("adding elements to frames", () => { expectEqualIds([rect2, frame]); }); - it.skip("should add elements", async () => { + it("should add elements", async () => { h.elements = [rect2, rect3, frame]; func(frame, rect2); @@ -188,7 +188,7 @@ describe("adding elements to frames", () => { expectEqualIds([rect3, rect2, frame]); }); - it.skip("should add elements when there are other other elements in between", async () => { + it("should add elements when there are other other elements in between", async () => { h.elements = [rect1, rect2, rect4, rect3, frame]; func(frame, rect2); @@ -199,7 +199,7 @@ describe("adding elements to frames", () => { expectEqualIds([rect1, rect4, rect3, rect2, frame]); }); - it.skip("should add elements when there are other elements in between and the order is reversed", async () => { + it("should add elements when there are other elements in between and the order is reversed", async () => { h.elements = [rect3, rect4, rect2, rect1, frame]; func(frame, rect2); @@ -234,7 +234,7 @@ describe("adding elements to frames", () => { expectEqualIds([rect1, rect2, rect3, frame, rect4]); }); - it.skip("should add elements when there are other elements in between and the order is reversed", async () => { + it("should add elements when there are other elements in between and the order is reversed", async () => { h.elements = [rect3, rect4, frame, rect2, rect1]; func(frame, rect2); @@ -436,5 +436,121 @@ describe("adding elements to frames", () => { expect(rect2.frameId).toBe(null); expectEqualIds([rect2_copy, frame, rect2]); }); + + it("random order 01", () => { + const frame1 = API.createElement({ + type: "frame", + x: 0, + y: 0, + width: 100, + height: 100, + }); + const frame2 = API.createElement({ + type: "frame", + x: 200, + y: 0, + width: 100, + height: 100, + }); + const frame3 = API.createElement({ + type: "frame", + x: 300, + y: 0, + width: 100, + height: 100, + }); + + const rectangle1 = API.createElement({ + type: "rectangle", + x: 25, + y: 25, + width: 50, + height: 50, + frameId: frame1.id, + }); + const rectangle2 = API.createElement({ + type: "rectangle", + x: 225, + y: 25, + width: 50, + height: 50, + frameId: frame2.id, + }); + const rectangle3 = API.createElement({ + type: "rectangle", + x: 325, + y: 25, + width: 50, + height: 50, + frameId: frame3.id, + }); + const rectangle4 = API.createElement({ + type: "rectangle", + x: 350, + y: 25, + width: 50, + height: 50, + frameId: frame3.id, + }); + + h.elements = [ + frame1, + rectangle4, + rectangle1, + rectangle3, + frame3, + rectangle2, + frame2, + ]; + + API.setSelectedElements([rectangle2]); + + const origSize = h.elements.length; + + expect(h.elements.length).toBe(origSize); + dragElementIntoFrame(frame3, rectangle2); + expect(h.elements.length).toBe(origSize); + }); + + it("random order 02", () => { + const frame1 = API.createElement({ + type: "frame", + x: 0, + y: 0, + width: 100, + height: 100, + }); + const frame2 = API.createElement({ + type: "frame", + x: 200, + y: 0, + width: 100, + height: 100, + }); + const rectangle1 = API.createElement({ + type: "rectangle", + x: 25, + y: 25, + width: 50, + height: 50, + frameId: frame1.id, + }); + const rectangle2 = API.createElement({ + type: "rectangle", + x: 225, + y: 25, + width: 50, + height: 50, + frameId: frame2.id, + }); + + h.elements = [rectangle1, rectangle2, frame1, frame2]; + + API.setSelectedElements([rectangle2]); + + expect(h.elements.length).toBe(4); + dragElementIntoFrame(frame2, rectangle1); + expect(h.elements.length).toBe(4); + }); }); }); diff --git a/src/frame.ts b/src/frame.ts index 1da9cfa1..6dccedd2 100644 --- a/src/frame.ts +++ b/src/frame.ts @@ -452,22 +452,31 @@ export const getContainingFrame = ( }; // --------------------------- Frame Operations ------------------------------- + +/** + * Retains (or repairs for target frame) the ordering invriant where children + * elements come right before the parent frame: + * [el, el, child, child, frame, el] + */ export const addElementsToFrame = ( allElements: ExcalidrawElementsIncludingDeleted, elementsToAdd: NonDeletedExcalidrawElement[], frame: ExcalidrawFrameElement, ) => { - const currTargetFrameChildrenMap = new Map( + const { allElementsIndexMap, currTargetFrameChildrenMap } = allElements.reduce( - (acc: [ExcalidrawElement["id"], ExcalidrawElement][], element) => { + (acc, element, index) => { + acc.allElementsIndexMap.set(element.id, index); if (element.frameId === frame.id) { - acc.push([element.id, element]); + acc.currTargetFrameChildrenMap.set(element.id, true); } return acc; }, - [], - ), - ); + { + allElementsIndexMap: new Map(), + currTargetFrameChildrenMap: new Map(), + }, + ); const suppliedElementsToAddSet = new Set(elementsToAdd.map((el) => el.id)); @@ -520,12 +529,36 @@ export const addElementsToFrame = ( currFrameChildren.forEach((child) => { processedElements.add(child.id); }); - // console.log(currFrameChildren, finalElementsToAdd, element); - nextElements.push(...currFrameChildren, ...finalElementsToAdd, element); + + // if not found, add all children on top by assigning the lowest index + const targetFrameIndex = allElementsIndexMap.get(frame.id) ?? -1; + + const { newChildren_left, newChildren_right } = finalElementsToAdd.reduce( + (acc, element) => { + // if index not found, add on top of current frame children + const elementIndex = allElementsIndexMap.get(element.id) ?? Infinity; + if (elementIndex < targetFrameIndex) { + acc.newChildren_left.push(element); + } else { + acc.newChildren_right.push(element); + } + return acc; + }, + { + newChildren_left: [] as ExcalidrawElement[], + newChildren_right: [] as ExcalidrawElement[], + }, + ); + + nextElements.push( + ...newChildren_left, + ...currFrameChildren, + ...newChildren_right, + element, + ); continue; } - // console.log("(2)", element.frameId); nextElements.push(element); } diff --git a/src/tests/helpers/api.ts b/src/tests/helpers/api.ts index 1b1943de..1fb95804 100644 --- a/src/tests/helpers/api.ts +++ b/src/tests/helpers/api.ts @@ -94,6 +94,7 @@ export class API { angle?: number; id?: string; isDeleted?: boolean; + frameId?: ExcalidrawElement["id"]; groupIds?: string[]; // generic element props strokeColor?: ExcalidrawGenericElement["strokeColor"]; @@ -151,12 +152,12 @@ export class API { | "versionNonce" | "isDeleted" | "groupIds" - | "frameId" | "link" | "updated" > = { x, y, + frameId: rest.frameId ?? null, angle: rest.angle ?? 0, strokeColor: rest.strokeColor ?? appState.currentItemStrokeColor, backgroundColor: