feat: tweak editing behavior (#2668)

* feat: tweak editing behavior

* fix tests

Co-authored-by: dwelle <luzar.david@gmail.com>

src/components/App.tsx

@@ -2462,8 +2462,7 @@ class App extends React.Component<ExcalidrawProps, AppState> {
           // otherwise, it will trigger selection based on current
           // state of the box
           if (!this.state.selectedElementIds[hitElement.id]) {
-            // if we are currently editing a group, treat all selections outside of the group
-            // as exiting editing mode.
+            // if we are currently editing a group, exiting editing mode and deselect the group.
             if (
               this.state.editingGroupId &&
               !isElementInGroup(hitElement, this.state.editingGroupId)
@@ -2473,7 +2472,6 @@ class App extends React.Component<ExcalidrawProps, AppState> {
                 selectedGroupIds: {},
                 editingGroupId: null,
               });
-              return true;
             }
 
             // Add hit element to selection. At this point if we're not holding

src/tests/__snapshots__/regressionTests.test.tsx.snap

@@ -18537,7 +18537,9 @@ Object {
   "offsetLeft": 0,
   "offsetTop": 0,
   "openMenu": null,
-  "previousSelectedElementIds": Object {},
+  "previousSelectedElementIds": Object {
+    "id0": true,
+  },
   "resizingElement": null,
   "scrollX": 0,
   "scrollY": 0,
@@ -18575,7 +18577,7 @@ Object {
   "groupIds": Array [
     "id3",
   ],
-  "height": 10,
+  "height": 50,
   "id": "id1",
   "isDeleted": false,
   "opacity": 100,
@@ -18588,9 +18590,9 @@ Object {
   "type": "rectangle",
   "version": 3,
   "versionNonce": 1116226695,
-  "width": 10,
-  "x": 30,
-  "y": 10,
+  "width": 50,
+  "x": 100,
+  "y": 100,
 }
 `;
 
@@ -18604,7 +18606,7 @@ Object {
     "id5",
     "id3",
   ],
-  "height": 10,
+  "height": 50,
   "id": "id0",
   "isDeleted": false,
   "opacity": 100,
@@ -18617,9 +18619,9 @@ Object {
   "type": "rectangle",
   "version": 4,
   "versionNonce": 400692809,
-  "width": 10,
-  "x": 10,
-  "y": 10,
+  "width": 50,
+  "x": 0,
+  "y": 0,
 }
 `;
 
@@ -18633,7 +18635,7 @@ Object {
     "id5",
     "id3",
   ],
-  "height": 10,
+  "height": 50,
   "id": "id2",
   "isDeleted": false,
   "opacity": 100,
@@ -18646,9 +18648,9 @@ Object {
   "type": "rectangle",
   "version": 4,
   "versionNonce": 1604849351,
-  "width": 10,
-  "x": 50,
-  "y": 10,
+  "width": 50,
+  "x": 200,
+  "y": 200,
 }
 `;
 
@@ -18684,7 +18686,7 @@ Object {
           "boundElementIds": null,
           "fillStyle": "hachure",
           "groupIds": Array [],
-          "height": 10,
+          "height": 50,
           "id": "id0",
           "isDeleted": false,
           "opacity": 100,
@@ -18697,9 +18699,9 @@ Object {
           "type": "rectangle",
           "version": 2,
           "versionNonce": 1278240551,
-          "width": 10,
-          "x": 10,
-          "y": 10,
+          "width": 50,
+          "x": 0,
+          "y": 0,
         },
       ],
     },
@@ -18720,7 +18722,7 @@ Object {
           "boundElementIds": null,
           "fillStyle": "hachure",
           "groupIds": Array [],
-          "height": 10,
+          "height": 50,
           "id": "id0",
           "isDeleted": false,
           "opacity": 100,
@@ -18733,9 +18735,9 @@ Object {
           "type": "rectangle",
           "version": 2,
           "versionNonce": 1278240551,
-          "width": 10,
-          "x": 10,
-          "y": 10,
+          "width": 50,
+          "x": 0,
+          "y": 0,
         },
         Object {
           "angle": 0,
@@ -18743,7 +18745,7 @@ Object {
           "boundElementIds": null,
           "fillStyle": "hachure",
           "groupIds": Array [],
-          "height": 10,
+          "height": 50,
           "id": "id1",
           "isDeleted": false,
           "opacity": 100,
@@ -18756,9 +18758,9 @@ Object {
           "type": "rectangle",
           "version": 2,
           "versionNonce": 453191,
-          "width": 10,
-          "x": 30,
-          "y": 10,
+          "width": 50,
+          "x": 100,
+          "y": 100,
         },
       ],
     },
@@ -18779,7 +18781,7 @@ Object {
           "boundElementIds": null,
           "fillStyle": "hachure",
           "groupIds": Array [],
-          "height": 10,
+          "height": 50,
           "id": "id0",
           "isDeleted": false,
           "opacity": 100,
@@ -18792,9 +18794,9 @@ Object {
           "type": "rectangle",
           "version": 2,
           "versionNonce": 1278240551,
-          "width": 10,
-          "x": 10,
-          "y": 10,
+          "width": 50,
+          "x": 0,
+          "y": 0,
         },
         Object {
           "angle": 0,
@@ -18802,7 +18804,7 @@ Object {
           "boundElementIds": null,
           "fillStyle": "hachure",
           "groupIds": Array [],
-          "height": 10,
+          "height": 50,
           "id": "id1",
           "isDeleted": false,
           "opacity": 100,
@@ -18815,9 +18817,9 @@ Object {
           "type": "rectangle",
           "version": 2,
           "versionNonce": 453191,
-          "width": 10,
-          "x": 30,
-          "y": 10,
+          "width": 50,
+          "x": 100,
+          "y": 100,
         },
         Object {
           "angle": 0,
@@ -18825,7 +18827,7 @@ Object {
           "boundElementIds": null,
           "fillStyle": "hachure",
           "groupIds": Array [],
-          "height": 10,
+          "height": 50,
           "id": "id2",
           "isDeleted": false,
           "opacity": 100,
@@ -18838,9 +18840,9 @@ Object {
           "type": "rectangle",
           "version": 2,
           "versionNonce": 2019559783,
-          "width": 10,
-          "x": 50,
-          "y": 10,
+          "width": 50,
+          "x": 200,
+          "y": 200,
         },
       ],
     },
@@ -18865,7 +18867,7 @@ Object {
           "groupIds": Array [
             "id3",
           ],
-          "height": 10,
+          "height": 50,
           "id": "id0",
           "isDeleted": false,
           "opacity": 100,
@@ -18878,9 +18880,9 @@ Object {
           "type": "rectangle",
           "version": 3,
           "versionNonce": 1150084233,
-          "width": 10,
-          "x": 10,
-          "y": 10,
+          "width": 50,
+          "x": 0,
+          "y": 0,
         },
         Object {
           "angle": 0,
@@ -18890,7 +18892,7 @@ Object {
           "groupIds": Array [
             "id3",
           ],
-          "height": 10,
+          "height": 50,
           "id": "id1",
           "isDeleted": false,
           "opacity": 100,
@@ -18903,9 +18905,9 @@ Object {
           "type": "rectangle",
           "version": 3,
           "versionNonce": 1116226695,
-          "width": 10,
-          "x": 30,
-          "y": 10,
+          "width": 50,
+          "x": 100,
+          "y": 100,
         },
         Object {
           "angle": 0,
@@ -18915,7 +18917,7 @@ Object {
           "groupIds": Array [
             "id3",
           ],
-          "height": 10,
+          "height": 50,
           "id": "id2",
           "isDeleted": false,
           "opacity": 100,
@@ -18928,9 +18930,9 @@ Object {
           "type": "rectangle",
           "version": 3,
           "versionNonce": 1014066025,
-          "width": 10,
-          "x": 50,
-          "y": 10,
+          "width": 50,
+          "x": 200,
+          "y": 200,
         },
       ],
     },
@@ -18955,7 +18957,7 @@ Object {
           "groupIds": Array [
             "id3",
           ],
-          "height": 10,
+          "height": 50,
           "id": "id0",
           "isDeleted": false,
           "opacity": 100,
@@ -18968,9 +18970,9 @@ Object {
           "type": "rectangle",
           "version": 3,
           "versionNonce": 1150084233,
-          "width": 10,
-          "x": 10,
-          "y": 10,
+          "width": 50,
+          "x": 0,
+          "y": 0,
         },
         Object {
           "angle": 0,
@@ -18980,7 +18982,7 @@ Object {
           "groupIds": Array [
             "id3",
           ],
-          "height": 10,
+          "height": 50,
           "id": "id1",
           "isDeleted": false,
           "opacity": 100,
@@ -18993,9 +18995,9 @@ Object {
           "type": "rectangle",
           "version": 3,
           "versionNonce": 1116226695,
-          "width": 10,
-          "x": 30,
-          "y": 10,
+          "width": 50,
+          "x": 100,
+          "y": 100,
         },
         Object {
           "angle": 0,
@@ -19005,7 +19007,7 @@ Object {
           "groupIds": Array [
             "id3",
           ],
-          "height": 10,
+          "height": 50,
           "id": "id2",
           "isDeleted": false,
           "opacity": 100,
@@ -19018,9 +19020,9 @@ Object {
           "type": "rectangle",
           "version": 3,
           "versionNonce": 1014066025,
-          "width": 10,
-          "x": 50,
-          "y": 10,
+          "width": 50,
+          "x": 200,
+          "y": 200,
         },
       ],
     },
@@ -19045,7 +19047,7 @@ Object {
           "groupIds": Array [
             "id3",
           ],
-          "height": 10,
+          "height": 50,
           "id": "id1",
           "isDeleted": false,
           "opacity": 100,
@@ -19058,9 +19060,9 @@ Object {
           "type": "rectangle",
           "version": 3,
           "versionNonce": 1116226695,
-          "width": 10,
-          "x": 30,
-          "y": 10,
+          "width": 50,
+          "x": 100,
+          "y": 100,
         },
         Object {
           "angle": 0,
@@ -19071,7 +19073,7 @@ Object {
             "id5",
             "id3",
           ],
-          "height": 10,
+          "height": 50,
           "id": "id0",
           "isDeleted": false,
           "opacity": 100,
@@ -19084,9 +19086,9 @@ Object {
           "type": "rectangle",
           "version": 4,
           "versionNonce": 400692809,
-          "width": 10,
-          "x": 10,
-          "y": 10,
+          "width": 50,
+          "x": 0,
+          "y": 0,
         },
         Object {
           "angle": 0,
@@ -19097,7 +19099,7 @@ Object {
             "id5",
             "id3",
           ],
-          "height": 10,
+          "height": 50,
           "id": "id2",
           "isDeleted": false,
           "opacity": 100,
@@ -19110,9 +19112,9 @@ Object {
           "type": "rectangle",
           "version": 4,
           "versionNonce": 1604849351,
-          "width": 10,
-          "x": 50,
-          "y": 10,
+          "width": 50,
+          "x": 200,
+          "y": 200,
         },
       ],
     },
@@ -19138,7 +19140,7 @@ Object {
           "groupIds": Array [
             "id3",
           ],
-          "height": 10,
+          "height": 50,
           "id": "id1",
           "isDeleted": false,
           "opacity": 100,
@@ -19151,9 +19153,9 @@ Object {
           "type": "rectangle",
           "version": 3,
           "versionNonce": 1116226695,
-          "width": 10,
-          "x": 30,
-          "y": 10,
+          "width": 50,
+          "x": 100,
+          "y": 100,
         },
         Object {
           "angle": 0,
@@ -19164,7 +19166,7 @@ Object {
             "id5",
             "id3",
           ],
-          "height": 10,
+          "height": 50,
           "id": "id0",
           "isDeleted": false,
           "opacity": 100,
@@ -19177,9 +19179,9 @@ Object {
           "type": "rectangle",
           "version": 4,
           "versionNonce": 400692809,
-          "width": 10,
-          "x": 10,
-          "y": 10,
+          "width": 50,
+          "x": 0,
+          "y": 0,
         },
         Object {
           "angle": 0,
@@ -19190,7 +19192,7 @@ Object {
             "id5",
             "id3",
           ],
-          "height": 10,
+          "height": 50,
           "id": "id2",
           "isDeleted": false,
           "opacity": 100,
@@ -19203,9 +19205,9 @@ Object {
           "type": "rectangle",
           "version": 4,
           "versionNonce": 1604849351,
-          "width": 10,
-          "x": 50,
-          "y": 10,
+          "width": 50,
+          "x": 200,
+          "y": 200,
         },
       ],
     },
@@ -19215,7 +19217,7 @@ Object {
 
 exports[`regression tests supports nested groups: [end of test] number of elements 1`] = `3`;
 
-exports[`regression tests supports nested groups: [end of test] number of renders 1`] = `29`;
+exports[`regression tests supports nested groups: [end of test] number of renders 1`] = `28`;
 
 exports[`regression tests switches from group of selected elements to another element on pointer down: [end of test] appState 1`] = `
 Object {

src/tests/helpers/ui.ts

@@ -169,6 +169,12 @@ export class Pointer {
     this.click(element.x, element.y);
     this.reset();
   }
+
+  doubleClickOn(element: ExcalidrawElement) {
+    this.reset();
+    this.doubleClick(element.x, element.y);
+    this.reset();
+  }
 }
 
 const mouse = new Pointer("mouse");
@@ -178,32 +184,72 @@ export class UI {
     fireEvent.click(GlobalTestState.renderResult.getByToolName(toolName));
   };
 
+  /**
+   * Creates an Excalidraw element, and returns a proxy that wraps it so that
+   * accessing props will return the latest ones from the object existing in
+   * the app's elements array. This is because across the app lifecycle we tend
+   * to recreate element objects and the returned reference will become stale.
+   *
+   * If you need to get the actual element, not the proxy, call `get()` method
+   * on the proxy object.
+   */
   static createElement<T extends ToolName>(
     type: T,
     {
-      x = 0,
-      y = 0,
+      position = 0,
+      x = position,
+      y = position,
       size = 10,
       width = size,
       height = width,
     }: {
+      position?: number;
       x?: number;
       y?: number;
       size?: number;
       width?: number;
       height?: number;
     } = {},
-  ): T extends "arrow" | "line" | "draw"
+  ): (T extends "arrow" | "line" | "draw"
     ? ExcalidrawLinearElement
     : T extends "text"
     ? ExcalidrawTextElement
-    : ExcalidrawElement {
+    : ExcalidrawElement) & {
+    /** Returns the actual, current element from the elements array, instead
+        of the proxy */
+    get(): T extends "arrow" | "line" | "draw"
+      ? ExcalidrawLinearElement
+      : T extends "text"
+      ? ExcalidrawTextElement
+      : ExcalidrawElement;
+  } {
     UI.clickTool(type);
     mouse.reset();
     mouse.down(x, y);
     mouse.reset();
     mouse.up(x + (width ?? height ?? size), y + (height ?? size));
-    return h.elements[h.elements.length - 1] as any;
+
+    const origElement = h.elements[h.elements.length - 1] as any;
+
+    return new Proxy(
+      {},
+      {
+        get(target, prop) {
+          const currentElement = h.elements.find(
+            (element) => element.id === origElement.id,
+          ) as any;
+          if (prop === "get") {
+            if (currentElement.hasOwnProperty("get")) {
+              throw new Error(
+                "trying to get `get` test property, but ExcalidrawElement seems to define its own",
+              );
+            }
+            return () => currentElement;
+          }
+          return currentElement[prop];
+        },
+      },
+    ) as any;
   }
 
   static group(elements: ExcalidrawElement[]) {

src/tests/move.test.tsx

@@ -69,9 +69,9 @@ describe("move element", () => {
 
     // bind line to two rectangles
     bindOrUnbindLinearElement(
-      line as NonDeleted<ExcalidrawLinearElement>,
-      rectA as ExcalidrawRectangleElement,
-      rectB as ExcalidrawRectangleElement,
+      line.get() as NonDeleted<ExcalidrawLinearElement>,
+      rectA.get() as ExcalidrawRectangleElement,
+      rectB.get() as ExcalidrawRectangleElement,
     );
 
     // select the second rectangles

src/tests/regressionTests.test.tsx

@@ -558,64 +558,46 @@ describe("regression tests", () => {
   });
 
   it("supports nested groups", () => {
-    const positions: number[][] = [];
-
-    UI.clickTool("rectangle");
-    mouse.down(10, 10);
-    mouse.up(10, 10);
-    positions.push(mouse.getPosition());
-
-    UI.clickTool("rectangle");
-    mouse.down(10, -10);
-    mouse.up(10, 10);
-    positions.push(mouse.getPosition());
-
-    UI.clickTool("rectangle");
-    mouse.down(10, -10);
-    mouse.up(10, 10);
-    positions.push(mouse.getPosition());
+    const rectA = UI.createElement("rectangle", { position: 0, size: 50 });
+    const rectB = UI.createElement("rectangle", { position: 100, size: 50 });
+    const rectC = UI.createElement("rectangle", { position: 200, size: 50 });
 
     Keyboard.withModifierKeys({ ctrl: true }, () => {
       Keyboard.keyPress(KEYS.A);
       Keyboard.codePress(CODES.G);
     });
 
-    mouse.doubleClick();
+    mouse.doubleClickOn(rectC);
     Keyboard.withModifierKeys({ shift: true }, () => {
-      mouse.restorePosition(...positions[0]);
-      mouse.click();
+      mouse.clickOn(rectA);
     });
     Keyboard.withModifierKeys({ ctrl: true }, () => {
       Keyboard.codePress(CODES.G);
     });
 
-    const groupIds = h.elements[2].groupIds;
-    expect(groupIds.length).toBe(2);
-    expect(h.elements[1].groupIds).toEqual(groupIds);
-    expect(h.elements[0].groupIds).toEqual(groupIds.slice(1));
+    expect(rectC.groupIds.length).toBe(2);
+    expect(rectA.groupIds).toEqual(rectC.groupIds);
+    expect(rectB.groupIds).toEqual(rectA.groupIds.slice(1));
 
-    mouse.click(50, 50);
+    mouse.click(0, 100);
     expect(API.getSelectedElements().length).toBe(0);
-    mouse.restorePosition(...positions[0]);
-    mouse.click();
+
+    mouse.clickOn(rectA);
     expect(API.getSelectedElements().length).toBe(3);
     expect(h.state.editingGroupId).toBe(null);
 
-    mouse.doubleClick();
+    mouse.doubleClickOn(rectA);
     expect(API.getSelectedElements().length).toBe(2);
-    expect(h.state.editingGroupId).toBe(groupIds[1]);
+    expect(h.state.editingGroupId).toBe(rectA.groupIds[1]);
 
-    mouse.doubleClick();
+    mouse.doubleClickOn(rectA);
     expect(API.getSelectedElements().length).toBe(1);
-    expect(h.state.editingGroupId).toBe(groupIds[0]);
+    expect(h.state.editingGroupId).toBe(rectA.groupIds[0]);
 
-    // click out of the group
-    mouse.restorePosition(...positions[1]);
-    mouse.click();
-    expect(API.getSelectedElements().length).toBe(0);
-    mouse.click();
+    // click outside current (sub)group
+    mouse.clickOn(rectB);
     expect(API.getSelectedElements().length).toBe(3);
-    mouse.doubleClick();
+    mouse.doubleClickOn(rectB);
     expect(API.getSelectedElements().length).toBe(1);
   });