From 3f75a4d04f9cb7bb83a7e3420f0e61ff8a9ca70a Mon Sep 17 00:00:00 2001 From: dwelle <5153846+dwelle@users.noreply.github.com> Date: Thu, 17 Apr 2025 12:03:11 +0200 Subject: [PATCH] fix: incorrect selection on duplicating labeled containers --- packages/element/src/selection.ts | 54 ++++++++++++++++++- packages/element/tests/duplicate.test.tsx | 25 +++++---- .../actions/actionDuplicateSelection.tsx | 48 ++--------------- packages/excalidraw/components/App.tsx | 21 ++++---- 4 files changed, 80 insertions(+), 68 deletions(-) diff --git a/packages/element/src/selection.ts b/packages/element/src/selection.ts index a76703920..e07c96d38 100644 --- a/packages/element/src/selection.ts +++ b/packages/element/src/selection.ts @@ -7,13 +7,20 @@ import type { import { getElementAbsoluteCoords, getElementBounds } from "./bounds"; import { isElementInViewport } from "./sizeHelpers"; -import { isBoundToContainer, isFrameLikeElement } from "./typeChecks"; +import { + isBoundToContainer, + isFrameLikeElement, + isLinearElement, +} from "./typeChecks"; import { elementOverlapsWithFrame, getContainingFrame, getFrameChildren, } from "./frame"; +import { LinearElementEditor } from "./linearElementEditor"; +import { selectGroupsForSelectedElements } from "./groups"; + import type { ElementsMap, ElementsMapOrArray, @@ -254,3 +261,48 @@ export const makeNextSelectedElementIds = ( return nextSelectedElementIds; }; + +const _getLinearElementEditor = ( + targetElements: readonly ExcalidrawElement[], +) => { + const linears = targetElements.filter(isLinearElement); + if (linears.length === 1) { + const linear = linears[0]; + const boundElements = linear.boundElements?.map((def) => def.id) ?? []; + const onlySingleLinearSelected = targetElements.every( + (el) => el.id === linear.id || boundElements.includes(el.id), + ); + + if (onlySingleLinearSelected) { + return new LinearElementEditor(linear); + } + } + + return null; +}; + +export const getSelectionStateForElements = ( + targetElements: readonly ExcalidrawElement[], + allElements: readonly NonDeletedExcalidrawElement[], + appState: AppState, +) => { + return { + selectedLinearElement: _getLinearElementEditor(targetElements), + ...selectGroupsForSelectedElements( + { + editingGroupId: appState.editingGroupId, + selectedElementIds: excludeElementsInFramesFromSelection( + targetElements, + ).reduce((acc: Record, element) => { + if (!isBoundToContainer(element)) { + acc[element.id] = true; + } + return acc; + }, {}), + }, + allElements, + appState, + null, + ), + }; +}; diff --git a/packages/element/tests/duplicate.test.tsx b/packages/element/tests/duplicate.test.tsx index 057428ca8..24042b347 100644 --- a/packages/element/tests/duplicate.test.tsx +++ b/packages/element/tests/duplicate.test.tsx @@ -614,10 +614,10 @@ describe("duplication z-order", () => { ]); }); - it("reverse-duplicating text container (in-order)", async () => { + it("alt-duplicating text container (in-order)", async () => { const [rectangle, text] = API.createTextContainer(); API.setElements([rectangle, text]); - API.setSelectedElements([rectangle, text]); + API.setSelectedElements([rectangle]); Keyboard.withModifierKeys({ alt: true }, () => { mouse.down(rectangle.x + 5, rectangle.y + 5); @@ -631,15 +631,14 @@ describe("duplication z-order", () => { { [ORIG_ID]: text.id, containerId: getCloneByOrigId(rectangle.id)?.id, - selected: true, }, ]); }); - it("reverse-duplicating text container (out-of-order)", async () => { + it("alt-duplicating text container (out-of-order)", async () => { const [rectangle, text] = API.createTextContainer(); API.setElements([text, rectangle]); - API.setSelectedElements([rectangle, text]); + API.setSelectedElements([rectangle]); Keyboard.withModifierKeys({ alt: true }, () => { mouse.down(rectangle.x + 5, rectangle.y + 5); @@ -653,16 +652,15 @@ describe("duplication z-order", () => { { [ORIG_ID]: text.id, containerId: getCloneByOrigId(rectangle.id)?.id, - selected: true, }, ]); }); - it("reverse-duplicating labeled arrows (in-order)", async () => { + it("alt-duplicating labeled arrows (in-order)", async () => { const [arrow, text] = API.createLabeledArrow(); API.setElements([arrow, text]); - API.setSelectedElements([arrow, text]); + API.setSelectedElements([arrow]); Keyboard.withModifierKeys({ alt: true }, () => { mouse.down(arrow.x + 5, arrow.y + 5); @@ -676,16 +674,18 @@ describe("duplication z-order", () => { { [ORIG_ID]: text.id, containerId: getCloneByOrigId(arrow.id)?.id, - selected: true, }, ]); + expect(h.state.selectedLinearElement).toEqual( + expect.objectContaining({ elementId: getCloneByOrigId(arrow.id)?.id }), + ); }); - it("reverse-duplicating labeled arrows (out-of-order)", async () => { + it("alt-duplicating labeled arrows (out-of-order)", async () => { const [arrow, text] = API.createLabeledArrow(); API.setElements([text, arrow]); - API.setSelectedElements([arrow, text]); + API.setSelectedElements([arrow]); Keyboard.withModifierKeys({ alt: true }, () => { mouse.down(arrow.x + 5, arrow.y + 5); @@ -699,12 +699,11 @@ describe("duplication z-order", () => { { [ORIG_ID]: text.id, containerId: getCloneByOrigId(arrow.id)?.id, - selected: true, }, ]); }); - it("reverse-duplicating bindable element with bound arrow should keep the arrow on the duplicate", () => { + it("alt-duplicating bindable element with bound arrow should keep the arrow on the duplicate", () => { const rect = UI.createElement("rectangle", { x: 0, y: 0, diff --git a/packages/excalidraw/actions/actionDuplicateSelection.tsx b/packages/excalidraw/actions/actionDuplicateSelection.tsx index 7c14dceea..17b452c36 100644 --- a/packages/excalidraw/actions/actionDuplicateSelection.tsx +++ b/packages/excalidraw/actions/actionDuplicateSelection.tsx @@ -7,26 +7,17 @@ import { import { getNonDeletedElements } from "@excalidraw/element"; -import { - isBoundToContainer, - isLinearElement, -} from "@excalidraw/element/typeChecks"; - import { LinearElementEditor } from "@excalidraw/element/linearElementEditor"; -import { selectGroupsForSelectedElements } from "@excalidraw/element/groups"; - import { - excludeElementsInFramesFromSelection, getSelectedElements, + getSelectionStateForElements, } from "@excalidraw/element/selection"; import { syncMovedIndices } from "@excalidraw/element/fractionalIndex"; import { duplicateElements } from "@excalidraw/element/duplicate"; -import type { ExcalidrawElement } from "@excalidraw/element/types"; - import { ToolButton } from "../components/ToolButton"; import { DuplicateIcon } from "../components/icons"; @@ -104,22 +95,10 @@ export const actionDuplicateSelection = register({ ), appState: { ...appState, - ...updateLinearElementEditors(duplicatedElements), - ...selectGroupsForSelectedElements( - { - editingGroupId: appState.editingGroupId, - selectedElementIds: excludeElementsInFramesFromSelection( - duplicatedElements, - ).reduce((acc: Record, element) => { - if (!isBoundToContainer(element)) { - acc[element.id] = true; - } - return acc; - }, {}), - }, + ...getSelectionStateForElements( + duplicatedElements, getNonDeletedElements(elementsWithDuplicates), appState, - null, ), }, captureUpdate: CaptureUpdateAction.IMMEDIATELY, @@ -139,24 +118,3 @@ export const actionDuplicateSelection = register({ /> ), }); - -const updateLinearElementEditors = (clonedElements: ExcalidrawElement[]) => { - const linears = clonedElements.filter(isLinearElement); - if (linears.length === 1) { - const linear = linears[0]; - const boundElements = linear.boundElements?.map((def) => def.id) ?? []; - const onlySingleLinearSelected = clonedElements.every( - (el) => el.id === linear.id || boundElements.includes(el.id), - ); - - if (onlySingleLinearSelected) { - return { - selectedLinearElement: new LinearElementEditor(linear), - }; - } - } - - return { - selectedLinearElement: null, - }; -}; diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index 388638535..2b75249ba 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -279,6 +279,7 @@ import { import { excludeElementsInFramesFromSelection, + getSelectionStateForElements, makeNextSelectedElementIds, } from "@excalidraw/element/selection"; @@ -8457,9 +8458,6 @@ class App extends React.Component { ); }); - const nextSelectedElementIds: Record = - Object.fromEntries(duplicatedElements.map((el) => [el.id, true])); - const mappedClonedElements = elementsWithDuplicates.map((el) => { if (idsOfElementsToDuplicate.has(el.id)) { const origEl = pointerDownState.originalElements.get(el.id); @@ -8487,6 +8485,15 @@ class App extends React.Component { // we need to update synchronously so as to keep pointerDownState, // appState, and scene elements in sync flushSync(() => { + // swap hit element with the duplicated one + if (pointerDownState.hit.element) { + const cloneId = origIdToDuplicateId.get( + pointerDownState.hit.element.id, + ); + const clonedElement = + cloneId && duplicateElementsMap.get(cloneId); + pointerDownState.hit.element = clonedElement || null; + } // swap hit elements with the duplicated ones pointerDownState.hit.allHitElements = pointerDownState.hit.allHitElements.reduce( @@ -8515,14 +8522,10 @@ class App extends React.Component { // switch selected elements to the duplicated ones this.setState((prevState) => ({ - ...selectGroupsForSelectedElements( - { - editingGroupId: prevState.editingGroupId, - selectedElementIds: nextSelectedElementIds, - }, + ...getSelectionStateForElements( + duplicatedElements, this.scene.getNonDeletedElements(), prevState, - this, ), }));