fix: stabilize `selectedElementIds` when box selecting (#6912)

src/groups.ts

@@ -71,6 +71,7 @@ export const selectGroupsForSelectedElements = (function () {
     selectedElements: readonly NonDeleted<ExcalidrawElement>[],
     elements: readonly NonDeleted<ExcalidrawElement>[],
     appState: Pick<AppState, "selectedElementIds" | "editingGroupId">,
+    prevAppState: InteractiveCanvasAppState,
   ): SelectGroupsReturnType => {
     if (
       lastReturnValue !== undefined &&
@@ -134,10 +135,13 @@ export const selectGroupsForSelectedElements = (function () {
     lastReturnValue = {
       editingGroupId: appState.editingGroupId,
       selectedGroupIds,
-      selectedElementIds: {
-        ...appState.selectedElementIds,
-        ...selectedElementIdsInGroups,
-      },
+      selectedElementIds: makeNextSelectedElementIds(
+        {
+          ...appState.selectedElementIds,
+          ...selectedElementIdsInGroups,
+        },
+        prevAppState,
+      ),
     };
 
     return lastReturnValue;
@@ -181,7 +185,7 @@ export const selectGroupsForSelectedElements = (function () {
       };
     }
 
-    return _selectGroups(selectedElements, elements, appState);
+    return _selectGroups(selectedElements, elements, appState, prevAppState);
   };
 
   selectGroupsForSelectedElements.clearCache = () => {

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

@@ -1734,7 +1734,7 @@ exports[`regression tests > Cmd/Ctrl-click exclusively select element under poin
 
 exports[`regression tests > Cmd/Ctrl-click exclusively select element under pointer > [end of test] number of elements 1`] = `0`;
 
-exports[`regression tests > Cmd/Ctrl-click exclusively select element under pointer > [end of test] number of renders 1`] = `32`;
+exports[`regression tests > Cmd/Ctrl-click exclusively select element under pointer > [end of test] number of renders 1`] = `30`;
 
 exports[`regression tests > Drags selected element when hitting only bounding box and keeps element selected > [end of test] appState 1`] = `
 {
@@ -4609,7 +4609,7 @@ exports[`regression tests > click-drag to select a group > [end of test] history
 
 exports[`regression tests > click-drag to select a group > [end of test] number of elements 1`] = `0`;
 
-exports[`regression tests > click-drag to select a group > [end of test] number of renders 1`] = `19`;
+exports[`regression tests > click-drag to select a group > [end of test] number of renders 1`] = `18`;
 
 exports[`regression tests > deleting last but one element in editing group should unselect the group > [end of test] appState 1`] = `
 {
@@ -15305,7 +15305,7 @@ exports[`regression tests > single-clicking on a subgroup of a selected group sh
 
 exports[`regression tests > single-clicking on a subgroup of a selected group should not alter selection > [end of test] number of elements 1`] = `0`;
 
-exports[`regression tests > single-clicking on a subgroup of a selected group should not alter selection > [end of test] number of renders 1`] = `31`;
+exports[`regression tests > single-clicking on a subgroup of a selected group should not alter selection > [end of test] number of renders 1`] = `30`;
 
 exports[`regression tests > spacebar + drag scrolls the canvas > [end of test] appState 1`] = `
 {

src/tests/linearElementEditor.test.tsx

@@ -275,7 +275,7 @@ describe("Test Linear Elements", () => {
       // drag line from midpoint
       drag(midpoint, [midpoint[0] + delta, midpoint[1] + delta]);
       expect(renderInteractiveScene).toHaveBeenCalledTimes(14);
-      expect(renderStaticScene).toHaveBeenCalledTimes(8);
+      expect(renderStaticScene).toHaveBeenCalledTimes(6);
 
       expect(line.points.length).toEqual(3);
       expect(line.points).toMatchInlineSnapshot(`
@@ -418,7 +418,7 @@ describe("Test Linear Elements", () => {
         ]);
 
         expect(renderInteractiveScene).toHaveBeenCalledTimes(21);
-        expect(renderStaticScene).toHaveBeenCalledTimes(11);
+        expect(renderStaticScene).toHaveBeenCalledTimes(9);
 
         expect(line.points.length).toEqual(5);
 
@@ -521,7 +521,7 @@ describe("Test Linear Elements", () => {
         deletePoint(points[2]);
         expect(line.points.length).toEqual(3);
         expect(renderInteractiveScene).toHaveBeenCalledTimes(21);
-        expect(renderStaticScene).toHaveBeenCalledTimes(12);
+        expect(renderStaticScene).toHaveBeenCalledTimes(9);
 
         const newMidPoints = LinearElementEditor.getEditorMidPoints(
           line,
@@ -568,7 +568,7 @@ describe("Test Linear Elements", () => {
           lastSegmentMidpoint[1] + delta,
         ]);
         expect(renderInteractiveScene).toHaveBeenCalledTimes(21);
-        expect(renderStaticScene).toHaveBeenCalledTimes(11);
+        expect(renderStaticScene).toHaveBeenCalledTimes(9);
         expect(line.points.length).toEqual(5);
 
         expect((h.elements[0] as ExcalidrawLinearElement).points)

src/tests/selection.test.tsx

@@ -308,7 +308,7 @@ describe("select single element on the scene", () => {
     fireEvent.pointerUp(canvas);
 
     expect(renderInteractiveScene).toHaveBeenCalledTimes(9);
-    expect(renderStaticScene).toHaveBeenCalledTimes(9);
+    expect(renderStaticScene).toHaveBeenCalledTimes(8);
     expect(h.state.selectionElement).toBeNull();
     expect(h.elements.length).toEqual(1);
     expect(h.state.selectedElementIds[h.elements[0].id]).toBeTruthy();
@@ -338,7 +338,7 @@ describe("select single element on the scene", () => {
     fireEvent.pointerUp(canvas);
 
     expect(renderInteractiveScene).toHaveBeenCalledTimes(9);
-    expect(renderStaticScene).toHaveBeenCalledTimes(9);
+    expect(renderStaticScene).toHaveBeenCalledTimes(8);
     expect(h.state.selectionElement).toBeNull();
     expect(h.elements.length).toEqual(1);
     expect(h.state.selectedElementIds[h.elements[0].id]).toBeTruthy();
@@ -368,7 +368,7 @@ describe("select single element on the scene", () => {
     fireEvent.pointerUp(canvas);
 
     expect(renderInteractiveScene).toHaveBeenCalledTimes(9);
-    expect(renderStaticScene).toHaveBeenCalledTimes(9);
+    expect(renderStaticScene).toHaveBeenCalledTimes(8);
     expect(h.state.selectionElement).toBeNull();
     expect(h.elements.length).toEqual(1);
     expect(h.state.selectedElementIds[h.elements[0].id]).toBeTruthy();
@@ -411,7 +411,7 @@ describe("select single element on the scene", () => {
     fireEvent.pointerUp(canvas);
 
     expect(renderInteractiveScene).toHaveBeenCalledTimes(9);
-    expect(renderStaticScene).toHaveBeenCalledTimes(9);
+    expect(renderStaticScene).toHaveBeenCalledTimes(8);
     expect(h.state.selectionElement).toBeNull();
     expect(h.elements.length).toEqual(1);
     expect(h.state.selectedElementIds[h.elements[0].id]).toBeTruthy();
@@ -453,7 +453,7 @@ describe("select single element on the scene", () => {
     fireEvent.pointerUp(canvas);
 
     expect(renderInteractiveScene).toHaveBeenCalledTimes(9);
-    expect(renderStaticScene).toHaveBeenCalledTimes(9);
+    expect(renderStaticScene).toHaveBeenCalledTimes(8);
     expect(h.state.selectionElement).toBeNull();
     expect(h.elements.length).toEqual(1);
     expect(h.state.selectedElementIds[h.elements[0].id]).toBeTruthy();
@@ -477,3 +477,46 @@ describe("tool locking & selection", () => {
     }
   });
 });
+
+describe("selectedElementIds stability", () => {
+  beforeEach(async () => {
+    await render(<ExcalidrawApp />);
+  });
+
+  it("box-selection should be stable when not changing selection", () => {
+    const rectangle = API.createElement({
+      type: "rectangle",
+      x: 0,
+      y: 0,
+      width: 10,
+      height: 10,
+    });
+
+    h.elements = [rectangle];
+
+    const selectedElementIds_1 = h.state.selectedElementIds;
+
+    mouse.downAt(-100, -100);
+    mouse.moveTo(-50, -50);
+    mouse.up();
+
+    expect(h.state.selectedElementIds).toBe(selectedElementIds_1);
+
+    mouse.downAt(-50, -50);
+    mouse.moveTo(50, 50);
+
+    const selectedElementIds_2 = h.state.selectedElementIds;
+
+    expect(selectedElementIds_2).toEqual({ [rectangle.id]: true });
+
+    mouse.moveTo(60, 60);
+
+    // box-selecting further without changing selection should keep
+    // selectedElementIds stable (the same object)
+    expect(h.state.selectedElementIds).toBe(selectedElementIds_2);
+
+    mouse.up();
+
+    expect(h.state.selectedElementIds).toBe(selectedElementIds_2);
+  });
+});