From a13aed92f2c6336a370fe5814e474a700e6ad522 Mon Sep 17 00:00:00 2001 From: Marcel Mraz Date: Thu, 7 Sep 2023 00:41:44 +0200 Subject: [PATCH] fix: z-index inconsistencies during addition / deletion in frames (#6914) Co-authored-by: Marcel Mraz Co-authored-by: dwelle --- src/components/App.tsx | 2 +- src/frame.test.tsx | 424 +++++++++++++++++++++++++++++++++++------ src/frame.ts | 77 +++++--- 3 files changed, 424 insertions(+), 79 deletions(-) diff --git a/src/components/App.tsx b/src/components/App.tsx index a975b8928..48eda7677 100644 --- a/src/components/App.tsx +++ b/src/components/App.tsx @@ -6501,7 +6501,7 @@ class App extends React.Component { } nextElements = updateFrameMembershipOfSelectedElements( - this.scene.getElementsIncludingDeleted(), + nextElements, this.state, this, ); diff --git a/src/frame.test.tsx b/src/frame.test.tsx index e9562a6f3..85417ddf3 100644 --- a/src/frame.test.tsx +++ b/src/frame.test.tsx @@ -1,9 +1,10 @@ +import { ExcalidrawElement } from "./element/types"; import { convertToExcalidrawElements, Excalidraw, } from "./packages/excalidraw/index"; import { API } from "./tests/helpers/api"; -import { Pointer } from "./tests/helpers/ui"; +import { Keyboard, Pointer } from "./tests/helpers/ui"; import { render } from "./tests/test-utils"; const { h } = window; @@ -28,83 +29,301 @@ describe("adding elements to frames", () => { }, []); }; - describe("resizing frame over elements", () => { - const testElements = async ( - containerType: "arrow" | "rectangle", - initialOrder: ElementType[], - expectedOrder: ElementType[], - ) => { - await render(); + function resizeFrameOverElement( + frame: ExcalidrawElement, + element: ExcalidrawElement, + ) { + mouse.clickAt(0, 0); + mouse.downAt(frame.x + frame.width, frame.y + frame.height); + mouse.moveTo( + element.x + element.width + 50, + element.y + element.height + 50, + ); + mouse.up(); + } - const frame = API.createElement({ type: "frame", x: 0, y: 0 }); + function dragElementIntoFrame( + frame: ExcalidrawElement, + element: ExcalidrawElement, + ) { + mouse.clickAt(element.x, element.y); + mouse.downAt(element.x + element.width / 2, element.y + element.height / 2); + mouse.moveTo(frame.x + frame.width / 2, frame.y + frame.height / 2); + mouse.up(); + } - h.elements = reorderElements( - [ - frame, - ...convertToExcalidrawElements([ - { - type: containerType, - x: 100, - y: 100, - height: 10, - label: { text: "xx" }, - }, - ]), - ], - initialOrder, - ); + function selectElementAndDuplicate( + element: ExcalidrawElement, + moveTo: [number, number] = [element.x + 25, element.y + 25], + ) { + const [x, y] = [ + element.x + element.width / 2, + element.y + element.height / 2, + ]; - assertOrder(h.elements, initialOrder); - - expect(h.elements[1].frameId).toBe(null); - expect(h.elements[2].frameId).toBe(null); - - const container = h.elements[1]; - - mouse.clickAt(0, 0); - mouse.downAt(frame.x + frame.width, frame.y + frame.height); - mouse.moveTo( - container.x + container.width + 100, - container.y + container.height + 100, - ); + Keyboard.withModifierKeys({ alt: true }, () => { + mouse.downAt(x, y); + mouse.moveTo(moveTo[0], moveTo[1]); mouse.up(); - assertOrder(h.elements, expectedOrder); + }); + } - expect(h.elements[0].frameId).toBe(frame.id); - expect(h.elements[1].frameId).toBe(frame.id); - }; + function expectEqualIds(expected: ExcalidrawElement[]) { + expect(h.elements.map((x) => x.id)).toEqual(expected.map((x) => x.id)); + } - it("resizing over text containers / labelled arrows", async () => { - await testElements( + let frame: ExcalidrawElement; + let rect1: ExcalidrawElement; + let rect2: ExcalidrawElement; + let rect3: ExcalidrawElement; + let rect4: ExcalidrawElement; + let text: ExcalidrawElement; + let arrow: ExcalidrawElement; + + beforeEach(async () => { + await render(); + + frame = API.createElement({ id: "id0", type: "frame", x: 0, width: 150 }); + rect1 = API.createElement({ + id: "id1", + type: "rectangle", + x: -1000, + }); + rect2 = API.createElement({ + id: "id2", + type: "rectangle", + x: 200, + width: 50, + }); + rect3 = API.createElement({ + id: "id3", + type: "rectangle", + x: 400, + width: 50, + }); + rect4 = API.createElement({ + id: "id4", + type: "rectangle", + x: 1000, + width: 50, + }); + text = API.createElement({ + id: "id5", + type: "text", + x: 100, + }); + arrow = API.createElement({ + id: "id6", + type: "arrow", + x: 100, + boundElements: [{ id: text.id, type: "text" }], + }); + }); + + const commonTestCases = async ( + func: typeof resizeFrameOverElement | typeof dragElementIntoFrame, + ) => { + describe("when frame is in a layer below", async () => { + it("should add an element", async () => { + h.elements = [frame, rect2]; + + func(frame, rect2); + + expect(h.elements[0].frameId).toBe(frame.id); + expectEqualIds([rect2, frame]); + }); + + it("should add elements", async () => { + h.elements = [frame, rect2, rect3]; + + func(frame, rect2); + func(frame, rect3); + + expect(rect2.frameId).toBe(frame.id); + expect(rect3.frameId).toBe(frame.id); + expectEqualIds([rect2, rect3, frame]); + }); + + it("should add elements when there are other other elements in between", async () => { + h.elements = [frame, rect1, rect2, rect4, rect3]; + + func(frame, rect2); + func(frame, rect3); + + expect(rect2.frameId).toBe(frame.id); + expect(rect3.frameId).toBe(frame.id); + expectEqualIds([rect2, rect3, frame, rect1, rect4]); + }); + + it("should add elements when there are other elements in between and the order is reversed", async () => { + h.elements = [frame, rect3, rect4, rect2, rect1]; + + func(frame, rect2); + func(frame, rect3); + + expect(rect2.frameId).toBe(frame.id); + expect(rect3.frameId).toBe(frame.id); + expectEqualIds([rect2, rect3, frame, rect4, rect1]); + }); + }); + + describe("when frame is in a layer above", async () => { + it("should add an element", async () => { + h.elements = [rect2, frame]; + + func(frame, rect2); + + expect(h.elements[0].frameId).toBe(frame.id); + expectEqualIds([rect2, frame]); + }); + + it("should add elements", async () => { + h.elements = [rect2, rect3, frame]; + + func(frame, rect2); + func(frame, rect3); + + expect(rect2.frameId).toBe(frame.id); + expect(rect3.frameId).toBe(frame.id); + expectEqualIds([rect3, rect2, frame]); + }); + + it("should add elements when there are other other elements in between", async () => { + h.elements = [rect1, rect2, rect4, rect3, frame]; + + func(frame, rect2); + func(frame, rect3); + + expect(rect2.frameId).toBe(frame.id); + expect(rect3.frameId).toBe(frame.id); + expectEqualIds([rect1, rect4, rect3, rect2, frame]); + }); + + 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); + func(frame, rect3); + + expect(rect2.frameId).toBe(frame.id); + expect(rect3.frameId).toBe(frame.id); + expectEqualIds([rect4, rect1, rect3, rect2, frame]); + }); + }); + + describe("when frame is in an inner layer", async () => { + it("should add elements", async () => { + h.elements = [rect2, frame, rect3]; + + func(frame, rect2); + func(frame, rect3); + + expect(rect2.frameId).toBe(frame.id); + expect(rect3.frameId).toBe(frame.id); + expectEqualIds([rect2, rect3, frame]); + }); + + it("should add elements when there are other other elements in between", async () => { + h.elements = [rect2, rect1, frame, rect4, rect3]; + + func(frame, rect2); + func(frame, rect3); + + expect(rect2.frameId).toBe(frame.id); + expect(rect3.frameId).toBe(frame.id); + expectEqualIds([rect1, rect2, rect3, frame, rect4]); + }); + + 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); + func(frame, rect3); + + expect(rect2.frameId).toBe(frame.id); + expect(rect3.frameId).toBe(frame.id); + expectEqualIds([rect4, rect3, rect2, frame, rect1]); + }); + }); + }; + + const resizingTest = async ( + containerType: "arrow" | "rectangle", + initialOrder: ElementType[], + expectedOrder: ElementType[], + ) => { + await render(); + + const frame = API.createElement({ type: "frame", x: 0, y: 0 }); + + h.elements = reorderElements( + [ + frame, + ...convertToExcalidrawElements([ + { + type: containerType, + x: 100, + y: 100, + height: 10, + label: { text: "xx" }, + }, + ]), + ], + initialOrder, + ); + + assertOrder(h.elements, initialOrder); + + expect(h.elements[1].frameId).toBe(null); + expect(h.elements[2].frameId).toBe(null); + + const container = h.elements[1]; + + resizeFrameOverElement(frame, container); + assertOrder(h.elements, expectedOrder); + + expect(h.elements[0].frameId).toBe(frame.id); + expect(h.elements[1].frameId).toBe(frame.id); + }; + + describe("resizing frame over elements", async () => { + await commonTestCases(resizeFrameOverElement); + + it("resizing over text containers and labelled arrows", async () => { + await resizingTest( "rectangle", ["frame", "rectangle", "text"], ["rectangle", "text", "frame"], ); - await testElements( + await resizingTest( "rectangle", ["frame", "text", "rectangle"], ["rectangle", "text", "frame"], ); - await testElements( + await resizingTest( "rectangle", ["rectangle", "text", "frame"], ["rectangle", "text", "frame"], ); - await testElements( + await resizingTest( "rectangle", ["text", "rectangle", "frame"], - ["text", "rectangle", "frame"], + ["rectangle", "text", "frame"], ); - - await testElements( + await resizingTest( "arrow", ["frame", "arrow", "text"], ["arrow", "text", "frame"], ); - await testElements( + await resizingTest( "arrow", ["text", "arrow", "frame"], - ["text", "arrow", "frame"], + ["arrow", "text", "frame"], + ); + await resizingTest( + "arrow", + ["frame", "arrow", "text"], + ["arrow", "text", "frame"], ); // FIXME failing in tests (it fails to add elements to frame for some @@ -118,11 +337,104 @@ describe("adding elements to frames", () => { // ["arrow", "text", "frame"], // ["arrow", "text", "frame"], // ); - // await testElements( - // "arrow", - // ["frame", "text", "arrow"], - // ["text", "arrow", "frame"], - // ); + }); + + it("should add arrow bound with text when frame is in a layer below", async () => { + h.elements = [frame, arrow, text]; + + resizeFrameOverElement(frame, arrow); + + expect(arrow.frameId).toBe(frame.id); + expect(text.frameId).toBe(frame.id); + expectEqualIds([arrow, text, frame]); + }); + + it("should add arrow bound with text when frame is in a layer above", async () => { + h.elements = [arrow, text, frame]; + + resizeFrameOverElement(frame, arrow); + + expect(arrow.frameId).toBe(frame.id); + expect(text.frameId).toBe(frame.id); + expectEqualIds([arrow, text, frame]); + }); + + it("should add arrow bound with text when frame is in an inner layer", async () => { + h.elements = [arrow, frame, text]; + + resizeFrameOverElement(frame, arrow); + + expect(arrow.frameId).toBe(frame.id); + expect(text.frameId).toBe(frame.id); + expectEqualIds([arrow, text, frame]); + }); + }); + + describe("resizing frame over elements but downwards", async () => { + it("should add elements when frame is in a layer below", async () => { + h.elements = [frame, rect1, rect2, rect3, rect4]; + + resizeFrameOverElement(frame, rect4); + resizeFrameOverElement(frame, rect3); + + expect(rect2.frameId).toBe(frame.id); + expect(rect3.frameId).toBe(frame.id); + expectEqualIds([rect2, rect3, frame, rect4, rect1]); + }); + + it("should add elements when frame is in a layer above", async () => { + h.elements = [rect1, rect2, rect3, rect4, frame]; + + resizeFrameOverElement(frame, rect4); + resizeFrameOverElement(frame, rect3); + + expect(rect2.frameId).toBe(frame.id); + expect(rect3.frameId).toBe(frame.id); + expectEqualIds([rect1, rect2, rect3, frame, rect4]); + }); + + it("should add elements when frame is in an inner layer", async () => { + h.elements = [rect1, rect2, frame, rect3, rect4]; + + resizeFrameOverElement(frame, rect4); + resizeFrameOverElement(frame, rect3); + + expect(rect2.frameId).toBe(frame.id); + expect(rect3.frameId).toBe(frame.id); + expectEqualIds([rect1, rect2, rect3, frame, rect4]); + }); + }); + + describe("dragging elements into the frame", async () => { + await commonTestCases(dragElementIntoFrame); + + it("should drag element inside, duplicate it and keep it in frame", () => { + h.elements = [frame, rect2]; + + dragElementIntoFrame(frame, rect2); + + const rect2_copy = { ...rect2, id: `${rect2.id}_copy` }; + + selectElementAndDuplicate(rect2); + + expect(rect2_copy.frameId).toBe(frame.id); + expect(rect2.frameId).toBe(frame.id); + expectEqualIds([rect2_copy, rect2, frame]); + }); + + it("should drag element inside, duplicate it and remove it from frame", () => { + h.elements = [frame, rect2]; + + dragElementIntoFrame(frame, rect2); + + const rect2_copy = { ...rect2, id: `${rect2.id}_copy` }; + + // move the rect2 outside the frame + selectElementAndDuplicate(rect2, [-1000, -1000]); + + expect(rect2_copy.frameId).toBe(frame.id); + expect(rect2.frameId).toBe(null); + expectEqualIds([rect2_copy, frame, rect2]); }); }); }); diff --git a/src/frame.ts b/src/frame.ts index d5599157c..e366ff430 100644 --- a/src/frame.ts +++ b/src/frame.ts @@ -468,14 +468,39 @@ export const addElementsToFrame = ( } } - let nextElements = allElements.slice(); + const allElementsIndex = allElements.reduce( + (acc: Record, element, index) => { + acc[element.id] = index; + return acc; + }, + {}, + ); + + const frameIndex = allElementsIndex[frame.id]; + // need to be calculated before the mutation below occurs + const leftFrameBoundaryIndex = findIndex( + allElements, + (e) => e.frameId === frame.id, + ); + + const existingFrameChildren = allElements.filter( + (element) => element.frameId === frame.id, + ); + + const addedFrameChildren_left: ExcalidrawElement[] = []; + const addedFrameChildren_right: ExcalidrawElement[] = []; - const frameBoundary = findIndex(nextElements, (e) => e.frameId === frame.id); for (const element of omitGroupsContainingFrames( allElements, _elementsToAdd, )) { if (element.frameId !== frame.id && !isFrameElement(element)) { + if (allElementsIndex[element.id] > frameIndex) { + addedFrameChildren_right.push(element); + } else { + addedFrameChildren_left.push(element); + } + mutateElement( element, { @@ -483,28 +508,35 @@ export const addElementsToFrame = ( }, false, ); - - const frameIndex = findIndex(nextElements, (e) => e.id === frame.id); - const elementIndex = findIndex(nextElements, (e) => e.id === element.id); - - if (elementIndex < frameBoundary) { - nextElements = [ - ...nextElements.slice(0, elementIndex), - ...nextElements.slice(elementIndex + 1, frameBoundary), - element, - ...nextElements.slice(frameBoundary), - ]; - } else if (elementIndex > frameIndex) { - nextElements = [ - ...nextElements.slice(0, frameIndex), - element, - ...nextElements.slice(frameIndex, elementIndex), - ...nextElements.slice(elementIndex + 1), - ]; - } } } + const frameElement = allElements[frameIndex]; + const nextFrameChildren = addedFrameChildren_left + .concat(existingFrameChildren) + .concat(addedFrameChildren_right); + + const nextFrameChildrenMap = nextFrameChildren.reduce( + (acc: Record, element) => { + acc[element.id] = true; + return acc; + }, + {}, + ); + + const nextOtherElements_left = allElements + .slice(0, leftFrameBoundaryIndex >= 0 ? leftFrameBoundaryIndex : frameIndex) + .filter((element) => !nextFrameChildrenMap[element.id]); + + const nextOtherElement_right = allElements + .slice(frameIndex + 1) + .filter((element) => !nextFrameChildrenMap[element.id]); + + const nextElements = nextOtherElements_left + .concat(nextFrameChildren) + .concat([frameElement]) + .concat(nextOtherElement_right); + return nextElements; }; @@ -518,6 +550,7 @@ export const removeElementsFromFrame = ( for (const element of elementsToRemove) { if (element.frameId) { _elementsToRemove.push(element); + const boundTextElement = getBoundTextElement(element); if (boundTextElement) { _elementsToRemove.push(boundTextElement); @@ -566,7 +599,7 @@ export const replaceAllElementsInFrame = ( ); }; -/** does not mutate elements, but return new ones */ +/** does not mutate elements, but returns new ones */ export const updateFrameMembershipOfSelectedElements = ( allElements: ExcalidrawElementsIncludingDeleted, appState: AppState,