fix: incorrect selection on duplicating labeled containers

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<ExcalidrawElement["id"], true>, element) => {
+          if (!isBoundToContainer(element)) {
+            acc[element.id] = true;
+          }
+          return acc;
+        }, {}),
+      },
+      allElements,
+      appState,
+      null,
+    ),
+  };
+};

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,

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<ExcalidrawElement["id"], true>, 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,
-  };
-};

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<AppProps, AppState> {
               );
             });
 
-            const nextSelectedElementIds: Record<string, true> =
-              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<AppProps, AppState> {
             // 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<AppProps, AppState> {
 
               // switch selected elements to the duplicated ones
               this.setState((prevState) => ({
-                ...selectGroupsForSelectedElements(
-                  {
-                    editingGroupId: prevState.editingGroupId,
-                    selectedElementIds: nextSelectedElementIds,
-                  },
+                ...getSelectionStateForElements(
+                  duplicatedElements,
                   this.scene.getNonDeletedElements(),
                   prevState,
-                  this,
                 ),
               }));