feat: capture images after they initialize (#9643)

Co-authored-by: dwelle <[email protected]>

packages/element/src/store.ts

@@ -23,6 +23,8 @@ import {
   syncInvalidIndicesImmutable,
   hashElementsVersion,
   hashString,
+  isInitializedImageElement,
+  isImageElement,
 } from "./index";
 
 import type {
@@ -137,6 +139,8 @@ export class Store {
     } else {
       // immediately create an immutable change of the scheduled updates,
       // compared to the current state, so that they won't mutate later on during batching
+      // also, we have to compare against the current state,
+      // as comparing against the snapshot might include yet uncomitted changes (i.e. async freedraw / text / image, etc.)
       const currentSnapshot = StoreSnapshot.create(
         this.app.scene.getElementsMapIncludingDeleted(),
         this.app.state,
@@ -869,7 +873,7 @@ export class StoreSnapshot {
   }
 
   /**
-   * Detect if there any changed elements.
+   * Detect if there are any changed elements.
    */
   private detectChangedElements(
     nextElements: SceneElementsMap,
@@ -904,6 +908,14 @@ export class StoreSnapshot {
         !prevElement || // element was added
         prevElement.version < nextElement.version // element was updated
       ) {
+        if (
+          isImageElement(nextElement) &&
+          !isInitializedImageElement(nextElement)
+        ) {
+          // ignore any updates on uninitialized image elements
+          continue;
+        }
+
         changedElements.set(nextElement.id, nextElement);
       }
     }

packages/excalidraw/components/App.tsx

@@ -3063,18 +3063,7 @@ class App extends React.Component<AppProps, AppState> {
           return;
         }
 
-        const imageElement = this.createImageElement({ sceneX, sceneY });
-        this.insertImageElement(imageElement, file);
-        this.initializeImageDimensions(imageElement);
-        this.store.scheduleCapture();
-        this.setState({
-          selectedElementIds: makeNextSelectedElementIds(
-            {
-              [imageElement.id]: true,
-            },
-            this.state,
-          ),
-        });
+        this.createImageElement({ sceneX, sceneY, imageFile: file });
 
         return;
       }
@@ -3380,15 +3369,12 @@ class App extends React.Component<AppProps, AppState> {
       const nextSelectedIds: Record<ExcalidrawElement["id"], true> = {};
       for (const response of responses) {
         if (response.file) {
-          const imageElement = this.createImageElement({
+          const initializedImageElement = await this.createImageElement({
             sceneX,
             sceneY: y,
+            imageFile: response.file,
           });
 
-          const initializedImageElement = await this.insertImageElement(
-            imageElement,
-            response.file,
-          );
           if (initializedImageElement) {
             // vertically center first image in the batch
             if (!firstImageYOffsetDone) {
@@ -3403,9 +3389,9 @@ class App extends React.Component<AppProps, AppState> {
               { informMutation: false, isDragging: false },
             );
 
-            y = imageElement.y + imageElement.height + 25;
+            y = initializedImageElement.y + initializedImageElement.height + 25;
 
-            nextSelectedIds[imageElement.id] = true;
+            nextSelectedIds[initializedImageElement.id] = true;
           }
         }
       }
@@ -7628,14 +7614,16 @@ class App extends React.Component<AppProps, AppState> {
     return element;
   };
 
-  private createImageElement = ({
+  private createImageElement = async ({
     sceneX,
     sceneY,
     addToFrameUnderCursor = true,
+    imageFile,
   }: {
     sceneX: number;
     sceneY: number;
     addToFrameUnderCursor?: boolean;
+    imageFile: File;
   }) => {
     const [gridX, gridY] = getGridPoint(
       sceneX,
@@ -7652,10 +7640,10 @@ class App extends React.Component<AppProps, AppState> {
         })
       : null;
 
-    const element = newImageElement({
+    const placeholderSize = 100 / this.state.zoom.value;
+
+    const placeholderImageElement = newImageElement({
       type: "image",
-      x: gridX,
-      y: gridY,
       strokeColor: this.state.currentItemStrokeColor,
       backgroundColor: this.state.currentItemBackgroundColor,
       fillStyle: this.state.currentItemFillStyle,
@@ -7666,9 +7654,18 @@ class App extends React.Component<AppProps, AppState> {
       opacity: this.state.currentItemOpacity,
       locked: false,
       frameId: topLayerFrame ? topLayerFrame.id : null,
+      x: gridX - placeholderSize / 2,
+      y: gridY - placeholderSize / 2,
+      width: placeholderSize,
+      height: placeholderSize,
     });
 
-    return element;
+    const initializedImageElement = await this.insertImageElement(
+      placeholderImageElement,
+      imageFile,
+    );
+
+    return initializedImageElement;
   };
 
   private handleLinearElementOnPointerDown = (
@@ -9092,32 +9089,6 @@ class App extends React.Component<AppProps, AppState> {
 
         return;
       }
-      if (isImageElement(newElement)) {
-        const imageElement = newElement;
-        try {
-          this.initializeImageDimensions(imageElement);
-          this.setState(
-            {
-              selectedElementIds: makeNextSelectedElementIds(
-                { [imageElement.id]: true },
-                this.state,
-              ),
-            },
-            () => {
-              this.actionManager.executeAction(actionFinalize);
-            },
-          );
-        } catch (error: any) {
-          console.error(error);
-          this.scene.replaceAllElements(
-            this.scene
-              .getElementsIncludingDeleted()
-              .filter((el) => el.id !== imageElement.id),
-          );
-          this.actionManager.executeAction(actionFinalize);
-        }
-        return;
-      }
 
       if (isLinearElement(newElement)) {
         if (newElement!.points.length > 1) {
@@ -9829,13 +9800,10 @@ class App extends React.Component<AppProps, AppState> {
     }
   };
 
-  private initializeImage = async ({
-    imageFile,
-    imageElement: _imageElement,
-  }: {
-    imageFile: File;
-    imageElement: ExcalidrawImageElement;
-  }) => {
+  private initializeImage = async (
+    placeholderImageElement: ExcalidrawImageElement,
+    imageFile: File,
+  ) => {
     // at this point this should be guaranteed image file, but we do this check
     // to satisfy TS down the line
     if (!isSupportedImageFile(imageFile)) {
@@ -9895,13 +9863,14 @@ class App extends React.Component<AppProps, AppState> {
     const dataURL =
       this.files[fileId]?.dataURL || (await getDataURL(imageFile));
 
-    let imageElement = newElementWith(_imageElement, {
-      fileId,
-    }) as NonDeleted<InitializedExcalidrawImageElement>;
-
     return new Promise<NonDeleted<InitializedExcalidrawImageElement>>(
       async (resolve, reject) => {
         try {
+          let initializedImageElement = this.getLatestInitializedImageElement(
+            placeholderImageElement,
+            fileId,
+          );
+
           this.addMissingFiles([
             {
               mimeType,
@@ -9912,34 +9881,39 @@ class App extends React.Component<AppProps, AppState> {
             },
           ]);
 
-          let cachedImageData = this.imageCache.get(fileId);
-
-          if (!cachedImageData) {
+          if (!this.imageCache.get(fileId)) {
             this.addNewImagesToImageCache();
 
-            const { updatedFiles } = await this.updateImageCache([
-              imageElement,
+            const { erroredFiles } = await this.updateImageCache([
+              initializedImageElement,
             ]);
 
-            if (updatedFiles.size) {
-              ShapeCache.delete(_imageElement);
+            if (erroredFiles.size) {
+              throw new Error("Image cache update resulted with an error.");
             }
-
-            cachedImageData = this.imageCache.get(fileId);
           }
 
-          const imageHTML = await cachedImageData?.image;
+          const imageHTML = await this.imageCache.get(fileId)?.image;
+
+          if (
+            imageHTML &&
+            this.state.newElement?.id !== initializedImageElement.id
+          ) {
+            initializedImageElement = this.getLatestInitializedImageElement(
+              placeholderImageElement,
+              fileId,
+            );
 
-          if (imageHTML && this.state.newElement?.id !== imageElement.id) {
             const naturalDimensions = this.getImageNaturalDimensions(
-              imageElement,
+              initializedImageElement,
               imageHTML,
             );
 
-            imageElement = newElementWith(imageElement, naturalDimensions);
+            // no need to create a new instance anymore, just assign the natural dimensions
+            Object.assign(initializedImageElement, naturalDimensions);
           }
 
-          resolve(imageElement);
+          resolve(initializedImageElement);
         } catch (error: any) {
           console.error(error);
           reject(new Error(t("errors.imageInsertError")));
@@ -9948,11 +9922,31 @@ class App extends React.Component<AppProps, AppState> {
     );
   };
 
+  /**
+   * use during async image initialization,
+   * when the placeholder image could have been modified in the meantime,
+   * and when you don't want to loose those modifications
+   */
+  private getLatestInitializedImageElement = (
+    imagePlaceholder: ExcalidrawImageElement,
+    fileId: FileId,
+  ) => {
+    const latestImageElement =
+      this.scene.getElement(imagePlaceholder.id) ?? imagePlaceholder;
+
+    return newElementWith(
+      latestImageElement as InitializedExcalidrawImageElement,
+      {
+        fileId,
+      },
+    );
+  };
+
   /**
    * inserts image into elements array and rerenders
    */
-  insertImageElement = async (
-    imageElement: ExcalidrawImageElement,
+  private insertImageElement = async (
+    placeholderImageElement: ExcalidrawImageElement,
     imageFile: File,
   ) => {
     // we should be handling all cases upstream, but in case we forget to handle
@@ -9962,34 +9956,39 @@ class App extends React.Component<AppProps, AppState> {
       return;
     }
 
-    this.scene.insertElement(imageElement);
+    this.scene.insertElement(placeholderImageElement);
 
     try {
-      const image = await this.initializeImage({
+      const initializedImageElement = await this.initializeImage(
+        placeholderImageElement,
         imageFile,
-        imageElement,
-      });
+      );
 
       const nextElements = this.scene
         .getElementsIncludingDeleted()
         .map((element) => {
-          if (element.id === image.id) {
-            return image;
+          if (element.id === initializedImageElement.id) {
+            return initializedImageElement;
           }
 
           return element;
         });
 
-      // schedules an immediate micro action, which will update snapshot,
-      // but won't be undoable, which is what we want!
       this.updateScene({
-        captureUpdate: CaptureUpdateAction.NEVER,
+        captureUpdate: CaptureUpdateAction.IMMEDIATELY,
         elements: nextElements,
+        appState: {
+          selectedElementIds: makeNextSelectedElementIds(
+            { [initializedImageElement.id]: true },
+            this.state,
+          ),
+        },
       });
 
-      return image;
+      return initializedImageElement;
     } catch (error: any) {
-      this.scene.mutateElement(imageElement, {
+      this.store.scheduleAction(CaptureUpdateAction.NEVER);
+      this.scene.mutateElement(placeholderImageElement, {
         isDeleted: true,
       });
       this.actionManager.executeAction(actionFinalize);
@@ -10017,26 +10016,17 @@ class App extends React.Component<AppProps, AppState> {
         ) as (keyof typeof IMAGE_MIME_TYPES)[],
       });
 
-      const imageElement = this.createImageElement({
+      await this.createImageElement({
         sceneX: x,
         sceneY: y,
         addToFrameUnderCursor: false,
+        imageFile,
       });
 
-      this.insertImageElement(imageElement, imageFile);
-      this.initializeImageDimensions(imageElement);
-      this.store.scheduleCapture();
-      this.setState(
-        {
-          selectedElementIds: makeNextSelectedElementIds(
-            { [imageElement.id]: true },
-            this.state,
-          ),
-        },
-        () => {
-          this.actionManager.executeAction(actionFinalize);
-        },
-      );
+      // avoid being batched (just in case)
+      this.setState({}, () => {
+        this.actionManager.executeAction(actionFinalize);
+      });
     } catch (error: any) {
       if (error.name !== "AbortError") {
         console.error(error);
@@ -10055,45 +10045,6 @@ class App extends React.Component<AppProps, AppState> {
     }
   };
 
-  initializeImageDimensions = (imageElement: ExcalidrawImageElement) => {
-    const imageHTML =
-      isInitializedImageElement(imageElement) &&
-      this.imageCache.get(imageElement.fileId)?.image;
-
-    if (!imageHTML || imageHTML instanceof Promise) {
-      if (
-        imageElement.width < DRAGGING_THRESHOLD / this.state.zoom.value &&
-        imageElement.height < DRAGGING_THRESHOLD / this.state.zoom.value
-      ) {
-        const placeholderSize = 100 / this.state.zoom.value;
-
-        this.scene.mutateElement(imageElement, {
-          x: imageElement.x - placeholderSize / 2,
-          y: imageElement.y - placeholderSize / 2,
-          width: placeholderSize,
-          height: placeholderSize,
-        });
-      }
-
-      return;
-    }
-
-    // if user-created bounding box is below threshold, assume the
-    // intention was to click instead of drag, and use the image's
-    // intrinsic size
-    if (
-      imageElement.width < DRAGGING_THRESHOLD / this.state.zoom.value &&
-      imageElement.height < DRAGGING_THRESHOLD / this.state.zoom.value
-    ) {
-      const naturalDimensions = this.getImageNaturalDimensions(
-        imageElement,
-        imageHTML,
-      );
-
-      this.scene.mutateElement(imageElement, naturalDimensions);
-    }
-  };
-
   private getImageNaturalDimensions = (
     imageElement: ExcalidrawImageElement,
     imageHTML: HTMLImageElement,
@@ -10135,8 +10086,9 @@ class App extends React.Component<AppProps, AppState> {
     });
 
     if (erroredFiles.size) {
+      this.store.scheduleAction(CaptureUpdateAction.NEVER);
       this.scene.replaceAllElements(
-        this.scene.getElementsIncludingDeleted().map((element) => {
+        elements.map((element) => {
           if (
             isInitializedImageElement(element) &&
             erroredFiles.has(element.fileId)
@@ -10357,17 +10309,7 @@ class App extends React.Component<AppProps, AppState> {
         // if no scene is embedded or we fail for whatever reason, fall back
         // to importing as regular image
         // ---------------------------------------------------------------------
-
-        const imageElement = this.createImageElement({ sceneX, sceneY });
-        this.insertImageElement(imageElement, file);
-        this.initializeImageDimensions(imageElement);
-        this.store.scheduleCapture();
-        this.setState({
-          selectedElementIds: makeNextSelectedElementIds(
-            { [imageElement.id]: true },
-            this.state,
-          ),
-        });
+        this.createImageElement({ sceneX, sceneY, imageFile: file });
 
         return;
       }

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

@@ -12514,7 +12514,7 @@ exports[`history > singleplayer undo/redo > should create new history entry on i
   "strokeWidth": 2,
   "type": "image",
   "updated": 1,
-  "version": 7,
+  "version": 5,
   "width": 318,
   "x": -159,
   "y": "-167.50000",
@@ -12573,14 +12573,14 @@ exports[`history > singleplayer undo/redo > should create new history entry on i
             "strokeStyle": "solid",
             "strokeWidth": 2,
             "type": "image",
-            "version": 7,
+            "version": 5,
             "width": 318,
             "x": -159,
             "y": "-167.50000",
           },
           "inserted": {
             "isDeleted": true,
-            "version": 6,
+            "version": 4,
           },
         },
       },
@@ -12737,7 +12737,7 @@ exports[`history > singleplayer undo/redo > should create new history entry on i
   "strokeWidth": 2,
   "type": "image",
   "updated": 1,
-  "version": 7,
+  "version": 5,
   "width": 56,
   "x": -28,
   "y": "-38.50000",
@@ -12796,14 +12796,14 @@ exports[`history > singleplayer undo/redo > should create new history entry on i
             "strokeStyle": "solid",
             "strokeWidth": 2,
             "type": "image",
-            "version": 7,
+            "version": 5,
             "width": 56,
             "x": -28,
             "y": "-38.50000",
           },
           "inserted": {
             "isDeleted": true,
-            "version": 6,
+            "version": 4,
           },
         },
       },

packages/excalidraw/tests/flip.test.tsx

@@ -38,6 +38,8 @@ import {
 
 import { getTextEditor } from "./queries/dom";
 
+import { mockHTMLImageElement } from "./helpers/mocks";
+
 import type { NormalizedZoomValue } from "../types";
 
 const { h } = window;
@@ -742,6 +744,28 @@ describe("freedraw", () => {
 //image
 //TODO: currently there is no test for pixel colors at flipped positions.
 describe("image", () => {
+  const smileyImageDimensions = {
+    width: 56,
+    height: 77,
+  };
+
+  beforeEach(() => {
+    // it's necessary to specify the height in order to calculate natural dimensions of the image
+    h.state.height = 1000;
+  });
+
+  beforeAll(() => {
+    mockHTMLImageElement(
+      smileyImageDimensions.width,
+      smileyImageDimensions.height,
+    );
+  });
+
+  afterAll(() => {
+    vi.unstubAllGlobals();
+    h.state.height = 0;
+  });
+
   const createImage = async () => {
     const sendPasteEvent = (file?: File) => {
       const clipboardEvent = createPasteEvent({ files: file ? [file] : [] });

packages/excalidraw/tests/history.test.tsx

@@ -642,6 +642,19 @@ describe("history", () => {
             ...deerImageDimensions,
           }),
         ]);
+
+        // need to check that delta actually contains initialized image element (with fileId & natural dimensions)
+        expect(
+          Object.values(h.history.undoStack[0].elements.removed)[0].deleted,
+        ).toEqual(
+          expect.objectContaining({
+            type: "image",
+            fileId: expect.any(String),
+            x: expect.toBeNonNaNNumber(),
+            y: expect.toBeNonNaNNumber(),
+            ...deerImageDimensions,
+          }),
+        );
       });
 
       Keyboard.undo();
@@ -753,6 +766,18 @@ describe("history", () => {
             ...smileyImageDimensions,
           }),
         ]);
+        // need to check that delta actually contains initialized image element (with fileId & natural dimensions)
+        expect(
+          Object.values(h.history.undoStack[0].elements.removed)[0].deleted,
+        ).toEqual(
+          expect.objectContaining({
+            type: "image",
+            fileId: expect.any(String),
+            x: expect.toBeNonNaNNumber(),
+            y: expect.toBeNonNaNNumber(),
+            ...smileyImageDimensions,
+          }),
+        );
       });
 
       Keyboard.undo();