fix: deselected hit element being duplicated + incorrect re-seeding (#9333)

* fix: deselected hit element being duplicated + incorrect re-seeding

* snapshots

* Fix alt-drag binding

Signed-off-by: Mark Tolmacs <mark@lazycat.hu>

* Add alt-drag bound arrow test

Signed-off-by: Mark Tolmacs <mark@lazycat.hu>

---------

Signed-off-by: Mark Tolmacs <mark@lazycat.hu>
Co-authored-by: Mark Tolmacs <mark@lazycat.hu>

packages/element/src/binding.ts

@@ -55,6 +55,7 @@ import { getBoundTextElement, handleBindTextResize } from "./textElement";
 import {
   isArrowElement,
   isBindableElement,
+  isBindingElement,
   isBoundToContainer,
   isElbowArrow,
   isFixedPointBinding,
@@ -1422,7 +1423,7 @@ const getLinearElementEdgeCoors = (
   );
 };
 
-export const fixBindingsAfterDuplication = (
+export const fixDuplicatedBindingsAfterDuplication = (
   newElements: ExcalidrawElement[],
   oldIdToDuplicatedId: Map<ExcalidrawElement["id"], ExcalidrawElement["id"]>,
   duplicatedElementsMap: NonDeletedSceneElementsMap,
@@ -1493,6 +1494,196 @@ export const fixBindingsAfterDuplication = (
   }
 };
 
+const fixReversedBindingsForBindables = (
+  original: ExcalidrawBindableElement,
+  duplicate: ExcalidrawBindableElement,
+  originalElements: Map<string, ExcalidrawElement>,
+  elementsWithClones: ExcalidrawElement[],
+  oldIdToDuplicatedId: Map<ExcalidrawElement["id"], ExcalidrawElement["id"]>,
+) => {
+  original.boundElements?.forEach((binding, idx) => {
+    if (binding.type !== "arrow") {
+      return;
+    }
+
+    const oldArrow = elementsWithClones.find((el) => el.id === binding.id);
+
+    if (!isBindingElement(oldArrow)) {
+      return;
+    }
+
+    if (originalElements.has(binding.id)) {
+      // Linked arrow is in the selection, so find the duplicate pair
+      const newArrowId = oldIdToDuplicatedId.get(binding.id) ?? binding.id;
+      const newArrow = elementsWithClones.find(
+        (el) => el.id === newArrowId,
+      )! as ExcalidrawArrowElement;
+
+      mutateElement(newArrow, {
+        startBinding:
+          oldArrow.startBinding?.elementId === binding.id
+            ? {
+                ...oldArrow.startBinding,
+                elementId: duplicate.id,
+              }
+            : newArrow.startBinding,
+        endBinding:
+          oldArrow.endBinding?.elementId === binding.id
+            ? {
+                ...oldArrow.endBinding,
+                elementId: duplicate.id,
+              }
+            : newArrow.endBinding,
+      });
+      mutateElement(duplicate, {
+        boundElements: [
+          ...(duplicate.boundElements ?? []).filter(
+            (el) => el.id !== binding.id && el.id !== newArrowId,
+          ),
+          {
+            type: "arrow",
+            id: newArrowId,
+          },
+        ],
+      });
+    } else {
+      // Linked arrow is outside the selection,
+      // so we move the binding to the duplicate
+      mutateElement(oldArrow, {
+        startBinding:
+          oldArrow.startBinding?.elementId === original.id
+            ? {
+                ...oldArrow.startBinding,
+                elementId: duplicate.id,
+              }
+            : oldArrow.startBinding,
+        endBinding:
+          oldArrow.endBinding?.elementId === original.id
+            ? {
+                ...oldArrow.endBinding,
+                elementId: duplicate.id,
+              }
+            : oldArrow.endBinding,
+      });
+      mutateElement(duplicate, {
+        boundElements: [
+          ...(duplicate.boundElements ?? []),
+          {
+            type: "arrow",
+            id: oldArrow.id,
+          },
+        ],
+      });
+      mutateElement(original, {
+        boundElements:
+          original.boundElements?.filter((_, i) => i !== idx) ?? null,
+      });
+    }
+  });
+};
+
+const fixReversedBindingsForArrows = (
+  original: ExcalidrawArrowElement,
+  duplicate: ExcalidrawArrowElement,
+  originalElements: Map<string, ExcalidrawElement>,
+  bindingProp: "startBinding" | "endBinding",
+  oldIdToDuplicatedId: Map<ExcalidrawElement["id"], ExcalidrawElement["id"]>,
+  elementsWithClones: ExcalidrawElement[],
+) => {
+  const oldBindableId = original[bindingProp]?.elementId;
+
+  if (oldBindableId) {
+    if (originalElements.has(oldBindableId)) {
+      // Linked element is in the selection
+      const newBindableId =
+        oldIdToDuplicatedId.get(oldBindableId) ?? oldBindableId;
+      const newBindable = elementsWithClones.find(
+        (el) => el.id === newBindableId,
+      ) as ExcalidrawBindableElement;
+      mutateElement(duplicate, {
+        [bindingProp]: {
+          ...original[bindingProp],
+          elementId: newBindableId,
+        },
+      });
+      mutateElement(newBindable, {
+        boundElements: [
+          ...(newBindable.boundElements ?? []).filter(
+            (el) => el.id !== original.id && el.id !== duplicate.id,
+          ),
+          {
+            id: duplicate.id,
+            type: "arrow",
+          },
+        ],
+      });
+    } else {
+      // Linked element is outside the selection
+      const originalBindable = elementsWithClones.find(
+        (el) => el.id === oldBindableId,
+      );
+      if (originalBindable) {
+        mutateElement(duplicate, {
+          [bindingProp]: original[bindingProp],
+        });
+        mutateElement(original, {
+          [bindingProp]: null,
+        });
+        mutateElement(originalBindable, {
+          boundElements: [
+            ...(originalBindable.boundElements?.filter(
+              (el) => el.id !== original.id,
+            ) ?? []),
+            {
+              id: duplicate.id,
+              type: "arrow",
+            },
+          ],
+        });
+      }
+    }
+  }
+};
+
+export const fixReversedBindings = (
+  originalElements: Map<string, ExcalidrawElement>,
+  elementsWithClones: ExcalidrawElement[],
+  oldIdToDuplicatedId: Map<ExcalidrawElement["id"], ExcalidrawElement["id"]>,
+) => {
+  for (const original of originalElements.values()) {
+    const duplicate = elementsWithClones.find(
+      (el) => el.id === oldIdToDuplicatedId.get(original.id),
+    )!;
+
+    if (isBindableElement(original) && isBindableElement(duplicate)) {
+      fixReversedBindingsForBindables(
+        original,
+        duplicate,
+        originalElements,
+        elementsWithClones,
+        oldIdToDuplicatedId,
+      );
+    } else if (isArrowElement(original) && isArrowElement(duplicate)) {
+      fixReversedBindingsForArrows(
+        original,
+        duplicate,
+        originalElements,
+        "startBinding",
+        oldIdToDuplicatedId,
+        elementsWithClones,
+      );
+      fixReversedBindingsForArrows(
+        original,
+        duplicate,
+        originalElements,
+        "endBinding",
+        oldIdToDuplicatedId,
+        elementsWithClones,
+      );
+    }
+  }
+};
+
 export const fixBindingsAfterDeletion = (
   sceneElements: readonly ExcalidrawElement[],
   deletedElements: readonly ExcalidrawElement[],

packages/element/src/duplicate.ts

@@ -36,7 +36,10 @@ import {
 
 import { getBoundTextElement, getContainerElement } from "./textElement";
 
-import { fixBindingsAfterDuplication } from "./binding";
+import {
+  fixDuplicatedBindingsAfterDuplication,
+  fixReversedBindings,
+} from "./binding";
 
 import type {
   ElementsMap,
@@ -381,12 +384,20 @@ export const duplicateElements = (
 
   // ---------------------------------------------------------------------------
 
-  fixBindingsAfterDuplication(
+  fixDuplicatedBindingsAfterDuplication(
     newElements,
     oldIdToDuplicatedId,
     duplicatedElementsMap as NonDeletedSceneElementsMap,
   );
 
+  if (reverseOrder) {
+    fixReversedBindings(
+      _idsOfElementsToDuplicate,
+      elementsWithClones,
+      oldIdToDuplicatedId,
+    );
+  }
+
   bindElementsToFramesAfterDuplication(
     elementsWithClones,
     oldElements,

packages/element/tests/duplicate.test.tsx

@@ -14,7 +14,7 @@ import { actionDuplicateSelection } from "@excalidraw/excalidraw/actions";
 
 import { API } from "@excalidraw/excalidraw/tests/helpers/api";
 
-import { Keyboard, Pointer } from "@excalidraw/excalidraw/tests/helpers/ui";
+import { UI, Keyboard, Pointer } from "@excalidraw/excalidraw/tests/helpers/ui";
 
 import {
   act,
@@ -699,4 +699,34 @@ describe("duplication z-order", () => {
       { id: text.id, containerId: arrow.id, selected: true },
     ]);
   });
+
+  it("reverse-duplicating bindable element with bound arrow should keep the arrow on the duplicate", () => {
+    const rect = UI.createElement("rectangle", {
+      x: 0,
+      y: 0,
+      width: 100,
+      height: 100,
+    });
+
+    const arrow = UI.createElement("arrow", {
+      x: -100,
+      y: 50,
+      width: 95,
+      height: 0,
+    });
+
+    expect(arrow.endBinding?.elementId).toBe(rect.id);
+
+    Keyboard.withModifierKeys({ alt: true }, () => {
+      mouse.down(5, 5);
+      mouse.up(15, 15);
+    });
+
+    expect(window.h.elements).toHaveLength(3);
+
+    const newRect = window.h.elements[0];
+
+    expect(arrow.endBinding?.elementId).toBe(newRect.id);
+    expect(newRect.boundElements?.[0]?.id).toBe(arrow.id);
+  });
 });

packages/excalidraw/components/App.tsx

@@ -99,6 +99,7 @@ import {
   isShallowEqual,
   arrayToMap,
   type EXPORT_IMAGE_TYPES,
+  randomInteger,
 } from "@excalidraw/common";
 
 import {
@@ -8521,20 +8522,26 @@ class App extends React.Component<AppProps, AppState> {
             });
             if (
               hitElement &&
+              // hit element may not end up being selected
+              // if we're alt-dragging a common bounding box
+              // over the hit element
+              pointerDownState.hit.wasAddedToSelection &&
               !selectedElements.find((el) => el.id === hitElement.id)
             ) {
               selectedElements.push(hitElement);
             }
 
+            const idsOfElementsToDuplicate = new Map(
+              selectedElements.map((el) => [el.id, el]),
+            );
+
             const { newElements: clonedElements, elementsWithClones } =
               duplicateElements({
                 type: "in-place",
                 elements,
                 appState: this.state,
                 randomizeSeed: true,
-                idsOfElementsToDuplicate: new Map(
+                idsOfElementsToDuplicate,
-                  selectedElements.map((el) => [el.id, el]),
-                ),
                 overrides: (el) => {
                   const origEl = pointerDownState.originalElements.get(el.id);
 
@@ -8542,6 +8549,7 @@ class App extends React.Component<AppProps, AppState> {
                     return {
                       x: origEl.x,
                       y: origEl.y,
+                      seed: origEl.seed,
                     };
                   }
 
@@ -8561,7 +8569,14 @@ class App extends React.Component<AppProps, AppState> {
             const nextSceneElements = syncMovedIndices(
               mappedNewSceneElements || elementsWithClones,
               arrayToMap(clonedElements),
-            );
+            ).map((el) => {
+              if (idsOfElementsToDuplicate.has(el.id)) {
+                return newElementWith(el, {
+                  seed: randomInteger(),
+                });
+              }
+              return el;
+            });
 
             this.scene.replaceAllElements(nextSceneElements);
             this.maybeCacheVisibleGaps(event, selectedElements, true);

packages/excalidraw/tests/__snapshots__/move.test.tsx.snap

@@ -20,7 +20,7 @@ exports[`duplicate element on move when ALT is clicked > rectangle 5`] = `
   "roundness": {
     "type": 3,
   },
-  "seed": 238820263,
+  "seed": 1278240551,
   "strokeColor": "#1e1e1e",
   "strokeStyle": "solid",
   "strokeWidth": 2,
@@ -54,14 +54,14 @@ exports[`duplicate element on move when ALT is clicked > rectangle 6`] = `
   "roundness": {
     "type": 3,
   },
-  "seed": 1278240551,
+  "seed": 1505387817,
   "strokeColor": "#1e1e1e",
   "strokeStyle": "solid",
   "strokeWidth": 2,
   "type": "rectangle",
   "updated": 1,
-  "version": 5,
+  "version": 6,
-  "versionNonce": 23633383,
+  "versionNonce": 915032327,
   "width": 30,
   "x": -10,
   "y": 60,