fix: undo/redo

packages/excalidraw/change.ts

@@ -17,13 +17,16 @@ import {
   hasBoundTextElement,
   isBindableElement,
   isBoundToContainer,
+  isImageElement,
   isTextElement,
 } from "./element/typeChecks";
 import type {
   ExcalidrawElement,
+  ExcalidrawImageElement,
   ExcalidrawLinearElement,
   ExcalidrawTextElement,
   NonDeleted,
+  Ordered,
   OrderedExcalidrawElement,
   SceneElementsMap,
 } from "./element/types";
@@ -626,6 +629,18 @@ export class AppStateChange implements Change<AppState> {
             );
 
             break;
+          case "croppingElementId": {
+            const croppingElementId = nextAppState[key];
+            const element =
+              croppingElementId && nextElements.get(croppingElementId);
+
+            if (element && !element.isDeleted) {
+              visibleDifferenceFlag.value = true;
+            } else {
+              nextAppState[key] = null;
+            }
+            break;
+          }
           case "editingGroupId":
             const editingGroupId = nextAppState[key];
 
@@ -756,6 +771,7 @@ export class AppStateChange implements Change<AppState> {
       selectedElementIds,
       editingLinearElementId,
       selectedLinearElementId,
+      croppingElementId,
       ...standaloneProps
     } = delta as ObservedAppState;
 
@@ -779,7 +795,10 @@ export class AppStateChange implements Change<AppState> {
   }
 }
 
-type ElementPartial = Omit<ElementUpdate<OrderedExcalidrawElement>, "seed">;
+type ElementPartial<T extends ExcalidrawElement = ExcalidrawElement> = Omit<
+  ElementUpdate<Ordered<T>>,
+  "seed"
+>;
 
 /**
  * Elements change is a low level primitive to capture a change between two sets of elements.
@@ -1216,6 +1235,18 @@ export class ElementsChange implements Change<SceneElementsMap> {
       });
     }
 
+    if (isImageElement(element)) {
+      const _delta = delta as Delta<ElementPartial<ExcalidrawImageElement>>;
+      // we want to override `crop` only if modified so that we don't reset
+      // when undoing/redoing unrelated change
+      if (_delta.deleted.crop || _delta.inserted.crop) {
+        Object.assign(directlyApplicablePartial, {
+          // apply change verbatim
+          crop: _delta.inserted.crop ?? null,
+        });
+      }
+    }
+
     if (!flags.containsVisibleDifference) {
       // strip away fractional as even if it would be different, it doesn't have to result in visible change
       const { index, ...rest } = directlyApplicablePartial;

packages/excalidraw/components/App.tsx

@@ -5173,15 +5173,19 @@ class App extends React.Component<AppProps, AppState> {
   };
 
   private startImageCropping = (image: ExcalidrawImageElement) => {
+    this.store.shouldCaptureIncrement();
     this.setState({
       croppingElementId: image.id,
     });
   };
 
   private finishImageCropping = () => {
+    if (this.state.croppingElementId) {
+      this.store.shouldCaptureIncrement();
       this.setState({
         croppingElementId: null,
       });
+    }
   };
 
   private handleCanvasDoubleClick = (

packages/excalidraw/element/cropElement.ts

@@ -11,6 +11,7 @@ import {
   vectorScale,
   pointFromVector,
   clamp,
+  isCloseTo,
 } from "../../math";
 import type { TransformHandleType } from "./transformHandles";
 import type {
@@ -64,12 +65,14 @@ export const cropElement = (
 
   let nextWidth = element.width;
   let nextHeight = element.height;
-  const crop = element.crop ?? {
+
+  let crop: ImageCrop | null = element.crop ?? {
     x: 0,
     y: 0,
     width: naturalWidth,
     height: naturalHeight,
-    naturalDimension: [naturalWidth, naturalHeight],
+    naturalWidth,
+    naturalHeight,
   };
 
   const previousCropHeight = crop.height;
@@ -138,6 +141,14 @@ export const cropElement = (
     nextHeight,
   );
 
+  // reset crop to null if we're back to orig size
+  if (
+    isCloseTo(crop.width, crop.naturalWidth) &&
+    isCloseTo(crop.height, crop.naturalHeight)
+  ) {
+    crop = null;
+  }
+
   return {
     x: newOrigin[0],
     y: newOrigin[1],
@@ -242,12 +253,12 @@ export const getUncroppedImageElement = (
         topLeftVector,
         vectorScale(
           topEdgeNormalized,
-          (-cropX * width) / element.crop.naturalDimension[0],
+          (-cropX * width) / element.crop.naturalWidth,
         ),
       ),
       vectorScale(
         leftEdgeNormalized,
-        (-cropY * height) / element.crop.naturalDimension[1],
+        (-cropY * height) / element.crop.naturalHeight,
       ),
     );
 
@@ -282,9 +293,9 @@ export const getUncroppedImageElement = (
 export const getUncroppedWidthAndHeight = (element: ExcalidrawImageElement) => {
   if (element.crop) {
     const width =
-      element.width / (element.crop.width / element.crop.naturalDimension[0]);
+      element.width / (element.crop.width / element.crop.naturalWidth);
     const height =
-      element.height / (element.crop.height / element.crop.naturalDimension[1]);
+      element.height / (element.crop.height / element.crop.naturalHeight);
 
     return {
       width,
@@ -309,11 +320,11 @@ const adjustCropPosition = (
   const flipY = scale[1] === -1;
 
   if (flipX) {
-    cropX = crop.naturalDimension[0] - Math.abs(cropX) - crop.width;
+    cropX = crop.naturalWidth - Math.abs(cropX) - crop.width;
   }
 
   if (flipY) {
-    cropY = crop.naturalDimension[1] - Math.abs(cropY) - crop.height;
+    cropY = crop.naturalHeight - Math.abs(cropY) - crop.height;
   }
 
   return {

packages/excalidraw/element/types.ts

@@ -137,7 +137,8 @@ export type ImageCrop = {
   y: number;
   width: number;
   height: number;
-  naturalDimension: [number, number];
+  naturalWidth: number;
+  naturalHeight: number;
 };
 
 export type ExcalidrawImageElement = _ExcalidrawElementBase &

packages/excalidraw/renderer/staticSvgScene.ts

@@ -428,11 +428,9 @@ const renderElementToSvg = (
             symbol.setAttribute(
               "viewBox",
               `${
-                element.crop.x /
+                element.crop.x / (element.crop.naturalWidth / uncroppedWidth)
-                (element.crop.naturalDimension[0] / uncroppedWidth)
               } ${
-                element.crop.y /
+                element.crop.y / (element.crop.naturalHeight / uncroppedHeight)
-                (element.crop.naturalDimension[1] / uncroppedHeight)
               } ${width} ${height}`,
             );
             image.setAttribute("width", `${uncroppedWidth}`);

packages/excalidraw/store.ts

@@ -21,6 +21,7 @@ export const getObservedAppState = (appState: AppState): ObservedAppState => {
     selectedGroupIds: appState.selectedGroupIds,
     editingLinearElementId: appState.editingLinearElement?.elementId || null,
     selectedLinearElementId: appState.selectedLinearElement?.elementId || null,
+    croppingElementId: appState.croppingElementId,
   };
 
   Reflect.defineProperty(observedAppState, hiddenObservedAppStateProp, {

packages/excalidraw/tests/cropElement.test.tsx

@@ -3,7 +3,7 @@ import ReactDOM from "react-dom";
 import { vi } from "vitest";
 import { Keyboard, Pointer, UI } from "./helpers/ui";
 import type { ExcalidrawImageElement, ImageCrop } from "../element/types";
-import { GlobalTestState, render } from "./test-utils";
+import { act, GlobalTestState, render } from "./test-utils";
 import { Excalidraw, exportToCanvas, exportToSvg } from "..";
 import { API } from "./helpers/api";
 import type { NormalizedZoomValue } from "../types";
@@ -60,15 +60,7 @@ const compareCrops = (cropA: ImageCrop, cropB: ImageCrop) => {
     const propA = cropA[key];
     const propB = cropB[key];
 
-    if (key === "naturalDimension") {
-      const [naturalWidthA, naturalHeightA] = propA as [number, number];
-      const [naturalWidthB, naturalHeightB] = propB as [number, number];
-
-      expect(naturalWidthA).toBeCloseTo(naturalWidthB);
-      expect(naturalHeightA).toBeCloseTo(naturalHeightB);
-    } else {
     expect(propA as number).toBeCloseTo(propB as number);
-    }
   });
 };
 
@@ -158,7 +150,9 @@ describe("Cropping and other features", async () => {
     ]);
     Keyboard.keyDown(KEYS.ESCAPE);
     const duplicatedImage = duplicateElement(null, new Map(), image, {});
+    act(() => {
       h.app.scene.insertElement(duplicatedImage);
+    });
 
     expect(duplicatedImage.width).toBe(image.width);
     expect(duplicatedImage.height).toBe(image.height);

packages/excalidraw/tests/helpers/ui.ts

@@ -590,7 +590,7 @@ export class UI {
       clientY + mouseMove[1],
     );
 
-    mutateElement(element, mutations);
+    API.updateElement(element, mutations);
   }
 
   static rotate(

packages/excalidraw/types.ts

@@ -224,6 +224,7 @@ export type ObservedElementsAppState = {
   editingLinearElementId: LinearElementEditor["elementId"] | null;
   // Right now it's coupled to `editingLinearElement`, ideally it should not be really needed as we already have selectedElementIds & editingLinearElementId
   selectedLinearElementId: LinearElementEditor["elementId"] | null;
+  croppingElementId: AppState["croppingElementId"];
 };
 
 export interface AppState {

packages/math/utils.ts

@@ -28,3 +28,6 @@ export const average = (a: number, b: number) => (a + b) / 2;
 export const isFiniteNumber = (value: any): value is number => {
   return typeof value === "number" && Number.isFinite(value);
 };
+
+export const isCloseTo = (a: number, b: number, precision = PRECISION) =>
+  Math.abs(a - b) < precision;