From 52eaf645919361c05d5ff75bd32d0e0e3a54e7e1 Mon Sep 17 00:00:00 2001
From: Ryan Di <ryan.weihao.di@gmail.com>
Date: Wed, 29 Jan 2025 08:10:16 +1100
Subject: [PATCH] feat: box select frame & children to allow resizing at the
 same time (#9031)

* box select frame & children

* avoid selecting children twice to avoid double their moving

* do not show ele stats if frame and children selected together

* do not update frame membership if selected together

* do not group frame and its children

* comment and refactor code

* hide align altogether

* include frame children when selecting all

* simplify

---------

Co-authored-by: dwelle <5153846+dwelle@users.noreply.github.com>
---
 packages/excalidraw/actions/actionAlign.tsx   | 34 +++++++------
 packages/excalidraw/actions/actionFrame.ts    |  1 +
 packages/excalidraw/actions/actionGroup.tsx   | 17 +++++--
 .../excalidraw/actions/actionSelectAll.ts     | 15 +++---
 packages/excalidraw/components/Actions.tsx    |  8 ++-
 packages/excalidraw/components/App.tsx        | 24 +++++++--
 packages/excalidraw/components/LayerUI.tsx    |  1 +
 packages/excalidraw/components/MobileMenu.tsx |  1 +
 .../excalidraw/components/Stats/index.tsx     |  7 ++-
 packages/excalidraw/frame.ts                  | 49 ++++++++++++++++---
 packages/excalidraw/scene/selection.ts        |  7 ++-
 .../tests/__snapshots__/move.test.tsx.snap    |  2 +-
 12 files changed, 122 insertions(+), 44 deletions(-)

diff --git a/packages/excalidraw/actions/actionAlign.tsx b/packages/excalidraw/actions/actionAlign.tsx
index ed078e9d54..2acc12bfe5 100644
--- a/packages/excalidraw/actions/actionAlign.tsx
+++ b/packages/excalidraw/actions/actionAlign.tsx
@@ -21,10 +21,8 @@ import type { AppClassProperties, AppState, UIAppState } from "../types";
 import { arrayToMap, getShortcutKey } from "../utils";
 import { register } from "./register";
 
-const alignActionsPredicate = (
-  elements: readonly ExcalidrawElement[],
+export const alignActionsPredicate = (
   appState: UIAppState,
-  _: unknown,
   app: AppClassProperties,
 ) => {
   const selectedElements = app.scene.getSelectedElements(appState);
@@ -65,7 +63,8 @@ export const actionAlignTop = register({
   label: "labels.alignTop",
   icon: AlignTopIcon,
   trackEvent: { category: "element" },
-  predicate: alignActionsPredicate,
+  predicate: (elements, appState, appProps, app) =>
+    alignActionsPredicate(appState, app),
   perform: (elements, appState, _, app) => {
     return {
       appState,
@@ -80,7 +79,7 @@ export const actionAlignTop = register({
     event[KEYS.CTRL_OR_CMD] && event.shiftKey && event.key === KEYS.ARROW_UP,
   PanelComponent: ({ elements, appState, updateData, app }) => (
     <ToolButton
-      hidden={!alignActionsPredicate(elements, appState, null, app)}
+      hidden={!alignActionsPredicate(appState, app)}
       type="button"
       icon={AlignTopIcon}
       onClick={() => updateData(null)}
@@ -98,7 +97,8 @@ export const actionAlignBottom = register({
   label: "labels.alignBottom",
   icon: AlignBottomIcon,
   trackEvent: { category: "element" },
-  predicate: alignActionsPredicate,
+  predicate: (elements, appState, appProps, app) =>
+    alignActionsPredicate(appState, app),
   perform: (elements, appState, _, app) => {
     return {
       appState,
@@ -113,7 +113,7 @@ export const actionAlignBottom = register({
     event[KEYS.CTRL_OR_CMD] && event.shiftKey && event.key === KEYS.ARROW_DOWN,
   PanelComponent: ({ elements, appState, updateData, app }) => (
     <ToolButton
-      hidden={!alignActionsPredicate(elements, appState, null, app)}
+      hidden={!alignActionsPredicate(appState, app)}
       type="button"
       icon={AlignBottomIcon}
       onClick={() => updateData(null)}
@@ -131,7 +131,8 @@ export const actionAlignLeft = register({
   label: "labels.alignLeft",
   icon: AlignLeftIcon,
   trackEvent: { category: "element" },
-  predicate: alignActionsPredicate,
+  predicate: (elements, appState, appProps, app) =>
+    alignActionsPredicate(appState, app),
   perform: (elements, appState, _, app) => {
     return {
       appState,
@@ -146,7 +147,7 @@ export const actionAlignLeft = register({
     event[KEYS.CTRL_OR_CMD] && event.shiftKey && event.key === KEYS.ARROW_LEFT,
   PanelComponent: ({ elements, appState, updateData, app }) => (
     <ToolButton
-      hidden={!alignActionsPredicate(elements, appState, null, app)}
+      hidden={!alignActionsPredicate(appState, app)}
       type="button"
       icon={AlignLeftIcon}
       onClick={() => updateData(null)}
@@ -164,7 +165,8 @@ export const actionAlignRight = register({
   label: "labels.alignRight",
   icon: AlignRightIcon,
   trackEvent: { category: "element" },
-  predicate: alignActionsPredicate,
+  predicate: (elements, appState, appProps, app) =>
+    alignActionsPredicate(appState, app),
   perform: (elements, appState, _, app) => {
     return {
       appState,
@@ -179,7 +181,7 @@ export const actionAlignRight = register({
     event[KEYS.CTRL_OR_CMD] && event.shiftKey && event.key === KEYS.ARROW_RIGHT,
   PanelComponent: ({ elements, appState, updateData, app }) => (
     <ToolButton
-      hidden={!alignActionsPredicate(elements, appState, null, app)}
+      hidden={!alignActionsPredicate(appState, app)}
       type="button"
       icon={AlignRightIcon}
       onClick={() => updateData(null)}
@@ -197,7 +199,8 @@ export const actionAlignVerticallyCentered = register({
   label: "labels.centerVertically",
   icon: CenterVerticallyIcon,
   trackEvent: { category: "element" },
-  predicate: alignActionsPredicate,
+  predicate: (elements, appState, appProps, app) =>
+    alignActionsPredicate(appState, app),
   perform: (elements, appState, _, app) => {
     return {
       appState,
@@ -210,7 +213,7 @@ export const actionAlignVerticallyCentered = register({
   },
   PanelComponent: ({ elements, appState, updateData, app }) => (
     <ToolButton
-      hidden={!alignActionsPredicate(elements, appState, null, app)}
+      hidden={!alignActionsPredicate(appState, app)}
       type="button"
       icon={CenterVerticallyIcon}
       onClick={() => updateData(null)}
@@ -226,7 +229,8 @@ export const actionAlignHorizontallyCentered = register({
   label: "labels.centerHorizontally",
   icon: CenterHorizontallyIcon,
   trackEvent: { category: "element" },
-  predicate: alignActionsPredicate,
+  predicate: (elements, appState, appProps, app) =>
+    alignActionsPredicate(appState, app),
   perform: (elements, appState, _, app) => {
     return {
       appState,
@@ -239,7 +243,7 @@ export const actionAlignHorizontallyCentered = register({
   },
   PanelComponent: ({ elements, appState, updateData, app }) => (
     <ToolButton
-      hidden={!alignActionsPredicate(elements, appState, null, app)}
+      hidden={!alignActionsPredicate(appState, app)}
       type="button"
       icon={CenterHorizontallyIcon}
       onClick={() => updateData(null)}
diff --git a/packages/excalidraw/actions/actionFrame.ts b/packages/excalidraw/actions/actionFrame.ts
index ffa33b6d7c..4ff8243fcf 100644
--- a/packages/excalidraw/actions/actionFrame.ts
+++ b/packages/excalidraw/actions/actionFrame.ts
@@ -200,6 +200,7 @@ export const actionWrapSelectionInFrame = register({
       [...app.scene.getElementsIncludingDeleted(), frame],
       selectedElements,
       frame,
+      appState,
     );
 
     return {
diff --git a/packages/excalidraw/actions/actionGroup.tsx b/packages/excalidraw/actions/actionGroup.tsx
index 6a6e735b7d..f61986f823 100644
--- a/packages/excalidraw/actions/actionGroup.tsx
+++ b/packages/excalidraw/actions/actionGroup.tsx
@@ -25,8 +25,10 @@ import type {
 import type { AppClassProperties, AppState } from "../types";
 import { isBoundToContainer } from "../element/typeChecks";
 import {
+  frameAndChildrenSelectedTogether,
   getElementsInResizingFrame,
   getFrameLikeElements,
+  getRootElements,
   groupByFrameLikes,
   removeElementsFromFrame,
   replaceAllElementsInFrame,
@@ -60,8 +62,11 @@ const enableActionGroup = (
     selectedElementIds: appState.selectedElementIds,
     includeBoundTextElement: true,
   });
+
   return (
-    selectedElements.length >= 2 && !allElementsInSameGroup(selectedElements)
+    selectedElements.length >= 2 &&
+    !allElementsInSameGroup(selectedElements) &&
+    !frameAndChildrenSelectedTogether(selectedElements)
   );
 };
 
@@ -71,10 +76,12 @@ export const actionGroup = register({
   icon: (appState) => <GroupIcon theme={appState.theme} />,
   trackEvent: { category: "element" },
   perform: (elements, appState, _, app) => {
-    const selectedElements = app.scene.getSelectedElements({
-      selectedElementIds: appState.selectedElementIds,
-      includeBoundTextElement: true,
-    });
+    const selectedElements = getRootElements(
+      app.scene.getSelectedElements({
+        selectedElementIds: appState.selectedElementIds,
+        includeBoundTextElement: true,
+      }),
+    );
     if (selectedElements.length < 2) {
       // nothing to group
       return { appState, elements, storeAction: StoreAction.NONE };
diff --git a/packages/excalidraw/actions/actionSelectAll.ts b/packages/excalidraw/actions/actionSelectAll.ts
index e95c5251a4..f8f9b775fe 100644
--- a/packages/excalidraw/actions/actionSelectAll.ts
+++ b/packages/excalidraw/actions/actionSelectAll.ts
@@ -5,7 +5,6 @@ import { getNonDeletedElements, isTextElement } from "../element";
 import type { ExcalidrawElement } from "../element/types";
 import { isLinearElement } from "../element/typeChecks";
 import { LinearElementEditor } from "../element/linearElementEditor";
-import { excludeElementsInFramesFromSelection } from "../scene/selection";
 import { selectAllIcon } from "../components/icons";
 import { StoreAction } from "../store";
 
@@ -20,17 +19,17 @@ export const actionSelectAll = register({
       return false;
     }
 
-    const selectedElementIds = excludeElementsInFramesFromSelection(
-      elements.filter(
+    const selectedElementIds = elements
+      .filter(
         (element) =>
           !element.isDeleted &&
           !(isTextElement(element) && element.containerId) &&
           !element.locked,
-      ),
-    ).reduce((map: Record<ExcalidrawElement["id"], true>, element) => {
-      map[element.id] = true;
-      return map;
-    }, {});
+      )
+      .reduce((map: Record<ExcalidrawElement["id"], true>, element) => {
+        map[element.id] = true;
+        return map;
+      }, {});
 
     return {
       appState: {
diff --git a/packages/excalidraw/components/Actions.tsx b/packages/excalidraw/components/Actions.tsx
index 329be75015..82deee1a88 100644
--- a/packages/excalidraw/components/Actions.tsx
+++ b/packages/excalidraw/components/Actions.tsx
@@ -51,6 +51,7 @@ import {
 import { KEYS } from "../keys";
 import { useTunnels } from "../context/tunnels";
 import { CLASSES } from "../constants";
+import { alignActionsPredicate } from "../actions/actionAlign";
 
 export const canChangeStrokeColor = (
   appState: UIAppState,
@@ -90,10 +91,12 @@ export const SelectedShapeActions = ({
   appState,
   elementsMap,
   renderAction,
+  app,
 }: {
   appState: UIAppState;
   elementsMap: NonDeletedElementsMap | NonDeletedSceneElementsMap;
   renderAction: ActionManager["renderAction"];
+  app: AppClassProperties;
 }) => {
   const targetElements = getTargetElements(elementsMap, appState);
 
@@ -133,6 +136,9 @@ export const SelectedShapeActions = ({
     targetElements.length === 1 &&
     isImageElement(targetElements[0]);
 
+  const showAlignActions =
+    !isSingleElementBoundContainer && alignActionsPredicate(appState, app);
+
   return (
     <div className="panelColumn">
       <div>
@@ -200,7 +206,7 @@ export const SelectedShapeActions = ({
         </div>
       </fieldset>
 
-      {targetElements.length > 1 && !isSingleElementBoundContainer && (
+      {showAlignActions && !isSingleElementBoundContainer && (
         <fieldset>
           <legend>{t("labels.align")}</legend>
           <div className="buttonList">
diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx
index 1f44a81a5c..f16f886574 100644
--- a/packages/excalidraw/components/App.tsx
+++ b/packages/excalidraw/components/App.tsx
@@ -3235,7 +3235,12 @@ class App extends React.Component<AppProps, AppState> {
         newElements,
         topLayerFrame,
       );
-      addElementsToFrame(nextElements, eligibleElements, topLayerFrame);
+      addElementsToFrame(
+        nextElements,
+        eligibleElements,
+        topLayerFrame,
+        this.state,
+      );
     }
 
     this.scene.replaceAllElements(nextElements);
@@ -4326,10 +4331,14 @@ class App extends React.Component<AppProps, AppState> {
         }
 
         selectedElements.forEach((element) => {
-          mutateElement(element, {
-            x: element.x + offsetX,
-            y: element.y + offsetY,
-          });
+          mutateElement(
+            element,
+            {
+              x: element.x + offsetX,
+              y: element.y + offsetY,
+            },
+            false,
+          );
 
           updateBoundElements(element, this.scene.getNonDeletedElementsMap(), {
             simultaneouslyUpdated: selectedElements,
@@ -4346,6 +4355,8 @@ class App extends React.Component<AppProps, AppState> {
           ),
         });
 
+        this.scene.triggerUpdate();
+
         event.preventDefault();
       } else if (event.key === KEYS.ENTER) {
         const selectedElements = this.scene.getSelectedElements(this.state);
@@ -8586,6 +8597,7 @@ class App extends React.Component<AppProps, AppState> {
                 elements,
                 this.state.selectionElement,
                 this.scene.getNonDeletedElementsMap(),
+                false,
               )
             : [];
 
@@ -9014,6 +9026,7 @@ class App extends React.Component<AppProps, AppState> {
             this.scene.getElementsMapIncludingDeleted(),
             elementsInsideFrame,
             newElement,
+            this.state,
           ),
         );
       }
@@ -9131,6 +9144,7 @@ class App extends React.Component<AppProps, AppState> {
               nextElements,
               elementsToAdd,
               topLayerFrame,
+              this.state,
             );
           } else if (!topLayerFrame) {
             if (this.state.editingGroupId) {
diff --git a/packages/excalidraw/components/LayerUI.tsx b/packages/excalidraw/components/LayerUI.tsx
index cca66a6a45..69bd398cb4 100644
--- a/packages/excalidraw/components/LayerUI.tsx
+++ b/packages/excalidraw/components/LayerUI.tsx
@@ -219,6 +219,7 @@ const LayerUI = ({
           appState={appState}
           elementsMap={app.scene.getNonDeletedElementsMap()}
           renderAction={actionManager.renderAction}
+          app={app}
         />
       </Island>
     </Section>
diff --git a/packages/excalidraw/components/MobileMenu.tsx b/packages/excalidraw/components/MobileMenu.tsx
index 91e67d7476..e0b8fb2c07 100644
--- a/packages/excalidraw/components/MobileMenu.tsx
+++ b/packages/excalidraw/components/MobileMenu.tsx
@@ -179,6 +179,7 @@ export const MobileMenu = ({
                 appState={appState}
                 elementsMap={app.scene.getNonDeletedElementsMap()}
                 renderAction={actionManager.renderAction}
+                app={app}
               />
             </Section>
           ) : null}
diff --git a/packages/excalidraw/components/Stats/index.tsx b/packages/excalidraw/components/Stats/index.tsx
index 3b07ce9544..88a05f7df2 100644
--- a/packages/excalidraw/components/Stats/index.tsx
+++ b/packages/excalidraw/components/Stats/index.tsx
@@ -31,6 +31,7 @@ import "./Stats.scss";
 import { isGridModeEnabled } from "../../snapping";
 import { getUncroppedWidthAndHeight } from "../../element/cropElement";
 import { round } from "../../../math";
+import { frameAndChildrenSelectedTogether } from "../../frame";
 
 interface StatsProps {
   app: AppClassProperties;
@@ -170,6 +171,10 @@ export const StatsInner = memo(
       return getAtomicUnits(selectedElements, appState);
     }, [selectedElements, appState]);
 
+    const _frameAndChildrenSelectedTogether = useMemo(() => {
+      return frameAndChildrenSelectedTogether(selectedElements);
+    }, [selectedElements]);
+
     return (
       <div className="exc-stats">
         <Island padding={3}>
@@ -226,7 +231,7 @@ export const StatsInner = memo(
             {renderCustomStats?.(elements, appState)}
           </Collapsible>
 
-          {selectedElements.length > 0 && (
+          {!_frameAndChildrenSelectedTogether && selectedElements.length > 0 && (
             <div
               id="elementStats"
               style={{
diff --git a/packages/excalidraw/frame.ts b/packages/excalidraw/frame.ts
index 7734e88f77..952521d3ef 100644
--- a/packages/excalidraw/frame.ts
+++ b/packages/excalidraw/frame.ts
@@ -498,6 +498,7 @@ export const addElementsToFrame = <T extends ElementsMapOrArray>(
   allElements: T,
   elementsToAdd: NonDeletedExcalidrawElement[],
   frame: ExcalidrawFrameLikeElement,
+  appState: AppState,
 ): T => {
   const elementsMap = arrayToMap(allElements);
   const currTargetFrameChildrenMap = new Map<ExcalidrawElement["id"], true>();
@@ -533,6 +534,17 @@ export const addElementsToFrame = <T extends ElementsMapOrArray>(
       continue;
     }
 
+    // if the element is already in another frame (which is also in elementsToAdd),
+    // it means that frame and children are selected at the same time
+    // => keep original frame membership, do not add to the target frame
+    if (
+      element.frameId &&
+      appState.selectedElementIds[element.id] &&
+      appState.selectedElementIds[element.frameId]
+    ) {
+      continue;
+    }
+
     if (!currTargetFrameChildrenMap.has(element.id)) {
       finalElementsToAdd.push(element);
     }
@@ -621,6 +633,7 @@ export const replaceAllElementsInFrame = <T extends ExcalidrawElement>(
     removeAllElementsFromFrame(allElements, frame),
     nextElementsInFrame,
     frame,
+    app.state,
   ).slice();
 };
 
@@ -727,6 +740,16 @@ export const getTargetFrame = (
     ? getContainerElement(element, elementsMap) || element
     : element;
 
+  // if the element and its containing frame are both selected, then
+  // the containing frame is the target frame
+  if (
+    _element.frameId &&
+    appState.selectedElementIds[_element.id] &&
+    appState.selectedElementIds[_element.frameId]
+  ) {
+    return getContainingFrame(_element, elementsMap);
+  }
+
   return appState.selectedElementIds[_element.id] &&
     appState.selectedElementsAreBeingDragged
     ? appState.frameToHighlight
@@ -763,13 +786,14 @@ export const isElementInFrame = (
     }
   };
 
-  // Perf improvement:
-  // For an element that's already in a frame, if it's not being dragged
-  // then there is no need to refer to geometry (which, yes, is slow) to check if it's in a frame.
-  // It has to be in its containing frame.
   if (
-    !appState.selectedElementIds[element.id] ||
-    !appState.selectedElementsAreBeingDragged
+    // if the element is not selected, or it is selected but not being dragged,
+    // frame membership won't update, so return true
+    !appState.selectedElementIds[_element.id] ||
+    !appState.selectedElementsAreBeingDragged ||
+    // if both frame and element are selected, won't update membership, so return true
+    (appState.selectedElementIds[_element.id] &&
+      appState.selectedElementIds[frame.id])
   ) {
     return true;
   }
@@ -912,3 +936,16 @@ export const getElementsOverlappingFrame = (
       .filter((el) => !el.frameId || el.frameId === frame.id)
   );
 };
+
+export const frameAndChildrenSelectedTogether = (
+  selectedElements: readonly ExcalidrawElement[],
+) => {
+  const selectedElementsMap = arrayToMap(selectedElements);
+
+  return (
+    selectedElements.length > 1 &&
+    selectedElements.some(
+      (element) => element.frameId && selectedElementsMap.has(element.frameId),
+    )
+  );
+};
diff --git a/packages/excalidraw/scene/selection.ts b/packages/excalidraw/scene/selection.ts
index 06fb2c604a..3ca91cd50a 100644
--- a/packages/excalidraw/scene/selection.ts
+++ b/packages/excalidraw/scene/selection.ts
@@ -183,10 +183,12 @@ export const getSelectedElements = (
     includeElementsInFrames?: boolean;
   },
 ) => {
+  const addedElements = new Set<ExcalidrawElement["id"]>();
   const selectedElements: ExcalidrawElement[] = [];
   for (const element of elements.values()) {
     if (appState.selectedElementIds[element.id]) {
       selectedElements.push(element);
+      addedElements.add(element.id);
       continue;
     }
     if (
@@ -195,6 +197,7 @@ export const getSelectedElements = (
       appState.selectedElementIds[element?.containerId]
     ) {
       selectedElements.push(element);
+      addedElements.add(element.id);
       continue;
     }
   }
@@ -203,8 +206,8 @@ export const getSelectedElements = (
     const elementsToInclude: ExcalidrawElement[] = [];
     selectedElements.forEach((element) => {
       if (isFrameLikeElement(element)) {
-        getFrameChildren(elements, element.id).forEach((e) =>
-          elementsToInclude.push(e),
+        getFrameChildren(elements, element.id).forEach(
+          (e) => !addedElements.has(e.id) && elementsToInclude.push(e),
         );
       }
       elementsToInclude.push(element);
diff --git a/packages/excalidraw/tests/__snapshots__/move.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/move.test.tsx.snap
index 654eccfea4..1fc6156fe6 100644
--- a/packages/excalidraw/tests/__snapshots__/move.test.tsx.snap
+++ b/packages/excalidraw/tests/__snapshots__/move.test.tsx.snap
@@ -233,7 +233,7 @@ exports[`move element > rectangles with binding arrow 7`] = `
   "type": "arrow",
   "updated": 1,
   "version": 11,
-  "versionNonce": 1996028265,
+  "versionNonce": 1051383431,
   "width": 81,
   "x": 110,
   "y": 50,