Skip to content

Delay page saves for async canvas image resize (BL-16302) #7947

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JohnThomson merged 1 commit into from
Jun 10, 2026
Merged

Delay page saves for async canvas image resize (BL-16302) #7947

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 72 additions & 27 deletions src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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<void>((resolve) => {
Expand Down
8 changes: 4 additions & 4 deletions src/BloomExe/Edit/EditingModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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)
{
Expand Down