From 7a298ca5926c89fe517b14344cd253681512c96a Mon Sep 17 00:00:00 2001 From: Andrew Polk Date: Fri, 5 Jun 2026 11:45:52 -0700 Subject: [PATCH] Delay page saves for async canvas image resize (BL-16302) When adjustContainerAspectRatio waits for a newly changed image to load, it can update canvas geometry after changeImage() returns. That left a window where requestPageContent could save stale width, height, and position data. Register a requestPageContent delay around that image-load retry, keep the existing paste-image wrapper for the separate async feature-status check, and update the C# comment that describes the async save behavior. --- .../bookEdit/js/CanvasElementManager.ts | 99 ++++++++++++++----- src/BloomExe/Edit/EditingModel.cs | 8 +- 2 files changed, 76 insertions(+), 31 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts b/src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts index 809302870cf6..ecce9538351a 100644 --- a/src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts +++ b/src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts @@ -23,10 +23,12 @@ import { getRgbaColorStringFromColorAndOpacity } from "../../utils/colorUtils"; import { IImageInfo, SetupElements, + addRequestPageContentDelay, attachToCkEditor, changeImageInfo, kMakeNewCanvasElement, notifyToolOfChangedImage, + removeRequestPageContentDelay, wrapWithRequestPageContentDelay, } from "./bloomEditing"; import { addSkeletonIfEmpty } from "./linkGrid"; @@ -104,6 +106,8 @@ export interface ITextOutlineColorInfo { } const kComicalGeneratedClass: string = "comical-generated"; +const kAdjustContainerAspectRatioDelayId = + "adjustContainerAspectRatioImageLoad"; const kTransformPropName = "bloom-zoomTransformForInitialFocus"; export const kBackgroundImageClass = "bloom-backgroundImage"; // split-pane.js and editMode.less know about this too @@ -2764,6 +2768,19 @@ export class CanvasElementManager { // width as necessary, or possibly increasing one if the usual adjustment would make it too small. // After making the adjustment if necessary (which might be delayed if the image dimensions // are not available), align the control frame with the active element. + // + // SAVE-RACE INVARIANT: when the adjustment must wait for the image to load, the geometry it + // eventually writes (width/height/left/top) would be stale if a save (requestPageContent) ran + // first. We guard the *deferred* path below with addRequestPageContentDelay, but that guard is + // only registered once we reach this method. requestPageContent always arrives as a separate + // JS task (posted by C# via RunJavascript), so it cannot interleave with a synchronous call + // stack. The guard is therefore sufficient ONLY because every caller reaches this method + // synchronously from the DOM mutation that necessitates the adjustment -- no await, setTimeout, + // or event-handler boundary in between -- so no save task can be processed in that gap. If you + // add a caller that does async work *before* the mutation (as the paste-new-element path does), + // it must hold its own requestPageContent delay across that gap; see the wrapWithRequestPageContentDelay + // usage in the paste branch, whose delay overlaps the one registered here so there is never an + // uncovered window. public adjustContainerAspectRatio( canvasElement: HTMLElement, useSizeOfNewImage = false, @@ -2818,30 +2835,13 @@ export class CanvasElementManager { return; } if (imgHeight === 0 || useSizeOfNewImage) { - // image not ready yet, try again later. - const handle = setTimeout( - () => - this.adjustContainerAspectRatio( - canvasElement, - false, // if we've got dimensions just use them - 0, - ), // if we get this call we don't have a timeout to cancel - // I think this is long enough that we won't be seeing obsolete data (from a previous src). - // OTOH it's not hopelessly long for the user to wait when we don't get an onload. - // If by any chance this happens when the image really isn't loaded enough to - // have naturalHeight/Width, the zero checks above will force another iteration. - 100, - // somehow Typescript is confused and thinks this is a NodeJS version of setTimeout. - ) as unknown as number; - imgOrVideo.addEventListener( - "load", - () => - this.adjustContainerAspectRatio( - canvasElement, - false, // it's loaded, we don't want to wait again - handle, - ), // if we get this call we can cancel the timeout above. - { once: true }, + // A newly changed image often needs an async retry before the browser reports + // natural dimensions. That retry will later write width/height/left/top, so it + // must participate in requestPageContent delay bookkeeping or saves can capture + // stale geometry. + this.waitForImageAndAdjustContainerAspectRatio( + canvasElement, + imgOrVideo, ); return; // control frame will be aligned when the image is loaded } @@ -2929,6 +2929,50 @@ export class CanvasElementManager { copyContentToTargetAndCleanup(canvasElement); } + private waitForImageAndAdjustContainerAspectRatio( + canvasElement: HTMLElement, + image: HTMLImageElement, + ): void { + // This delay is separate from the wrapper around the "add a new canvas element" paste path. + // That wrapper only covers the async feature-status request before an element is added. + // This one covers the later async wait for the image itself to finish loading so our final + // geometry updates become part of any saved page content. + // NB: this is registered only once we reach here; it closes the save-race window for the + // image-load wait, but relies on callers reaching adjustContainerAspectRatio synchronously + // from the triggering DOM mutation. See the SAVE-RACE INVARIANT note on that method. + addRequestPageContentDelay(kAdjustContainerAspectRatioDelayId); + let adjustmentResumed = false; + const resumeAdjustment = (timeoutHandler: number) => { + if (adjustmentResumed) { + return; + } + adjustmentResumed = true; + try { + this.adjustContainerAspectRatio( + canvasElement, + false, // if we've got dimensions just use them + timeoutHandler, + ); + } finally { + removeRequestPageContentDelay( + kAdjustContainerAspectRatioDelayId, + ); + } + }; + const handle = setTimeout( + () => resumeAdjustment(0), + // I think this is long enough that we won't be seeing obsolete data (from a previous src). + // OTOH it's not hopelessly long for the user to wait when we don't get an onload. + // If by any chance this happens when the image really isn't loaded enough to + // have naturalHeight/Width, the zero checks above will force another iteration. + 100, + // somehow Typescript is confused and thinks this is a NodeJS version of setTimeout. + ) as unknown as number; + image.addEventListener("load", () => resumeAdjustment(handle), { + once: true, + }); + } + // When the image is changed in a canvas element (e.g., choose or paste image), // we remove cropping, adjust the aspect ratio, and move the control frame. updateCanvasElementForChangedImage(imgOrImageContainer: HTMLElement) { @@ -5308,9 +5352,10 @@ export class CanvasElementManager { } } // otherwise we will add a new canvas element...but only if subscription allows it. - // This branch has an extra async feature-status request before the DOM changes happen. - // Keep requestPageContent delayed until that callback has either added the element or - // decided not to, otherwise the page can be saved before the pasted element exists. + // Keep this wrapper even though adjustContainerAspectRatio now manages its own image-load delay. + // This branch still has a separate async feature-status request before we even know whether a + // new element will be created. Without this wrapper, requestPageContent can run before the + // callback adds the pasted element or decides to show the subscription dialog instead. wrapWithRequestPageContentDelay( () => new Promise((resolve) => { diff --git a/src/BloomExe/Edit/EditingModel.cs b/src/BloomExe/Edit/EditingModel.cs index 07cf4ee1508e..a8a8b2dabd24 100644 --- a/src/BloomExe/Edit/EditingModel.cs +++ b/src/BloomExe/Edit/EditingModel.cs @@ -1631,8 +1631,8 @@ public void ChangePicture( public void UpdateImageInBrowser(PageEditingModel.ImageInfoForJavascript args) { // We generally don't need to wait. Even if we decide to save, its call to RunJavascriptAsync() will come in after ours. - // changeImage() itself is synchronous. The one async path (adjustBackgroundImageSize) is wrapped with - // wrapWithRequestPageContentDelay, which ensures any C#-triggered request for page content waits until + // changeImage() itself is synchronous. When it triggers async image-resize work in the editable page, + // that code registers requestPageContent delays so any C#-triggered request for page content waits until // those async DOM adjustments settle before retrieving the page HTML. GetEditingBrowser() .RunJavascriptFireAndForget( @@ -1646,8 +1646,8 @@ public void UpdateImageInBrowser(PageEditingModel.ImageInfoForJavascript args) * talk directly to the BloomServer and tell it that image needs transparency.] * Another possible reason to Save is that it is needed if we're going to update the thumbnail, but we decided * we can live without this...probably we can get that behavior back once the page list is in the same browser. - * Note: although changeImage() is synchronous, the async background-image-size adjustment it triggers is wrapped - * with wrapWithRequestPageContentDelay, so any save below will correctly wait for DOM adjustments to settle. + * Note: although changeImage() is synchronous, any async image-resize adjustments it triggers in the editable + * page register requestPageContent delays, so any save below will correctly wait for DOM adjustments to settle. */ if (CurrentPage.IsCoverPage) {