diff --git a/.github/skills/bloom-automation/ai-image-editor-driving.md b/.github/skills/bloom-automation/ai-image-editor-driving.md index 7a107eb44b32..fe4578f054ce 100644 --- a/.github/skills/bloom-automation/ai-image-editor-driving.md +++ b/.github/skills/bloom-automation/ai-image-editor-driving.md @@ -62,15 +62,20 @@ Right-click the canvas element → **"Edit with AI…"**. Loading the iframe dir `aiImageEditorOverlay.ts`'s postMessage handler, so the commit + current-page save path wouldn't run. **The overlay does not appear on the same tick as the click.** The menu command posts -`aiImageEditor/saveThenLaunch`, which makes C# save the current page — reloading the `page` -frame — and then, *once the reloaded page reports back*, call +`aiImageEditor/saveThenLaunch`, which makes C# save the current page and then call `workspaceBundle.openAiImageEditor(...)` in the shell (BL-16682; the saved book DOM has to be -current or the editor opens with an empty "Image to Edit" slot). So the wait spans a whole page -load: **wait for the overlay**, e.g. `await page.waitForSelector("#ai-image-editor-overlay iframe")`, -and don't re-use any content-frame handle taken before the click — that frame is gone. - -Note also that the overlay itself belongs to the **shell**, not the `page` frame, so it survives -page reloads: a commit saves the page immediately and the ✕ keeps working across that reload. +current or the editor opens with an empty "Image to Edit" slot). **Wait for the overlay**, e.g. +`await page.waitForSelector("#ai-image-editor-overlay iframe")`. + +That save used to end in a navigation, so the wait spanned a whole page load and any content-frame +handle taken before the click was stale. Since BL-13502 a save does not navigate: the page frame +stays put and its handles stay good. Waiting for the overlay is still the right thing — the save +and the round trip take a moment — but the page is no longer replaced underneath you. (Continuing +to wait for a page load that no longer happens is precisely what once left the editor never +opening; see the comment on `HandleSaveThenLaunch`.) + +Note also that the overlay itself belongs to the **shell**, not the `page` frame, so it survives a +reload when one does happen. (It does *not* survive a reload of the whole workspace root, which `EditingView.StartNavigationToEditPage` does once `MemoryUtils.SystemIsShortOfMemory()` — Bloom's own private bytes past ~2GB, i.e. a long session on a big book. That closes the overlay like any diff --git a/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts b/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts index e52de8566019..2eb3ff282ceb 100644 --- a/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts +++ b/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts @@ -44,6 +44,7 @@ import { RenderCanvasElementRoot } from "./CanvasElementFormatPage"; import { CanvasElementManager } from "../js/canvasElementManager/CanvasElementManager"; import { kCanvasElementSelector } from "../toolbox/canvas/canvasElementConstants"; import { getPageIFrame } from "../../utils/shared"; +import { getEditablePageBundleExports } from "../js/workspaceFrames"; // Controls the CSS text-align value // Note: CSS text-align W3 standard does not specify "start" or "end", but Firefox/Chrome/Edge do support it. @@ -587,6 +588,22 @@ export default class StyleEditor { if (styleSheet == null) { return null; } + if (create) { + // A caller asking us to create the rule means it is about to change it, and every + // change we make to these styles goes through the CSSOM -- setProperty, deleteRule, + // insertRule -- which mutates no DOM node. So the page watcher, which is a + // MutationObserver, cannot see it, and a formatting change that leaves the text alone + // would never be volunteered to C#: leaving the Edit tab or quitting would write the + // styles as they were. This is the one place every such change passes through. + // + // It is deliberately said BEFORE the change rather than after: the watcher waits a + // moment before reading the page, and the caller's edits are synchronous, so they are + // in by the time it looks. Saying so needlessly costs nothing -- an unchanged page + // produces no post. + // Through the page frame's exports rather than a direct import, because this class + // is used from the toolbox frame as well, and it is the PAGE frame that watches. + getEditablePageBundleExports()?.notePageContentMayHaveChanged(); + } let ruleList: CSSRuleList = styleSheet.cssRules; if (ruleList == null) { diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/AGENTS.md b/src/BloomBrowserUI/bookEdit/aiImageEditor/AGENTS.md index cd368ee3423a..caaea42a1355 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/AGENTS.md +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/AGENTS.md @@ -32,12 +32,12 @@ So: is there specifically so the two halves cannot disagree about which commit results belong to the page being edited. -The overlay is in the top window because a page save reloads the page iframe, which would tear -down an overlay hosted there — the same reason the image-gallery and copyright/license dialogs -live up there (see the comments on those commands in `canvasControlRegistry.ts`). Note that this -is *not* enough on its own: a save can also reload the whole workspace root, so C# waits for the -page to come back before opening the overlay. `AiImageEditorApi.HandleSaveThenLaunch` explains -that in full. +The overlay is in the top window, like the image-gallery and copyright/license dialogs (see the +comments on those commands in `canvasControlRegistry.ts`): plenty of operations replace the page +iframe underneath it. Opening it first saves the page, because everything the editor is told about +the book is read from the saved file; a save no longer reloads the page (BL-13502), so the editor +opens as soon as the save returns, and does not open at all if the save did not happen. +`AiImageEditorApi.HandleSaveThenLaunch` explains that in full. ## Tests diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.test.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.test.ts index 78ef29c20c61..b268b2c2e830 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.test.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.test.ts @@ -9,10 +9,11 @@ import { beforeEach, describe, expect, test, vi } from "vitest"; // - The edit target. C# hands over the page id and file name of the image the user // right-clicked (it survived a page save, which reloaded the page frame), and the overlay // matches that against the book image list to fill the "Image to Edit" slot (BL-16682). -// - Saving after a commit. The current-page swaps only touched the LIVE DOM, so unless we -// save, a second commit in the same session would read its oldSrc from a saved page still -// showing the pre-edit image and match nothing. Because this overlay lives in the top -// window, we can save immediately: the page reload underneath leaves its controls alone. +// - NOT saving after a commit. A current-page swap lives in the live page DOM only, like an +// image pasted or chosen from the gallery, and is persisted the same way: by the normal page +// save when the user moves on (BL-16330). A retry from this still-open overlay would read a +// stale oldSrc, which the page frame handles by remembering what it already swapped rather +// than by saving here. const post = vi.fn(); const postJson = vi.fn(); @@ -363,7 +364,7 @@ describe("aiImageEditorOverlay: the live page is NOT saved after a commit", () = }; expect(ack.ok).toBe(true); expect(ack.error).toBeUndefined(); - // Nothing landed on this page, so nothing to save. + // We never save from here at all -- see the note at the top of this file. expect(postThatMightNavigate).not.toHaveBeenCalled(); expect(applyAiImageEditorReplacements).not.toHaveBeenCalled(); postMessageToEditor.mockRestore(); @@ -389,7 +390,7 @@ describe("aiImageEditorOverlay: the live page is NOT saved after a commit", () = expect(ack.ok).toBe(false); expect(ack.error).toContain("not available"); expect(ack.error).toContain("other pages were made"); - // Nothing landed, so nothing to save. + // We never save from here at all -- see the note at the top of this file. expect(postThatMightNavigate).not.toHaveBeenCalled(); postMessageToEditor.mockRestore(); }); diff --git a/src/BloomBrowserUI/bookEdit/editablePage.ts b/src/BloomBrowserUI/bookEdit/editablePage.ts index c88eafcf3748..802733b74913 100644 --- a/src/BloomBrowserUI/bookEdit/editablePage.ts +++ b/src/BloomBrowserUI/bookEdit/editablePage.ts @@ -17,6 +17,11 @@ import { } from "./js/canvasElementManager/CanvasElementManager"; import { kCanvasElementSelector } from "./toolbox/canvas/canvasElementConstants"; import { renderDragActivityTabControl } from "./js/AbovePageControls"; +import { + getPageLoadId, + notePageContentMayHaveChanged, + startWatchingPageForSnapshots, +} from "./js/pageSnapshot"; function getPageId(): string { const page = document.querySelector(".bloom-page"); @@ -34,7 +39,12 @@ function getPageId(): string { // It is important that this does not get pulled into any other compiled bundle, // since it will generate errors when loaded into any page that does not have a .bloom-page. document.addEventListener("DOMContentLoaded", () => { - postString("editView/pageDomLoaded", getPageId()); + // The load id goes with it: from here until the next page reports ready, C# accepts snapshots + // only from this load. See getPageLoadId(). + postJson("editView/pageDomLoaded", { + pageId: getPageId(), + loadId: getPageLoadId(), + }); }); // This allows strong typing to be done for exported functions. @@ -48,8 +58,13 @@ document.addEventListener("DOMContentLoaded", () => { // but I think it is unwise. It is so easy for an extra file to get imported into another bundle, // and then it will bring this along, with disastrous results. export interface IPageFrameExports { - requestPageContent(): void; + // The combined "body userCss" string that a save needs, gathered without + // disturbing the live page. + getPageContentForSaveWhenReady(): Promise; pageUnloading(): void; + // Say that the saved form of the page may have changed in a way the page watcher cannot see -- + // the user's style definitions, which are changed through the CSSOM and mutate no DOM node. + notePageContentMayHaveChanged(): void; copySelection(): void; cutSelection(): void; pasteClipboard(): void; @@ -110,12 +125,10 @@ export interface IPageFrameExports { } // This exports the functions that should be accessible from other IFrames or from C#. -// For example, workspaceBundle.getEditablePageBundleExports().requestPageContent() can be called. +// For example, workspaceBundle.getEditablePageBundleExports().pageUnloading() can be called. import { - getBodyContentForSavePage, - requestPageContent, + getPageContentForSaveWhenReady, captureContentForExternalProcessing, - userStylesheetContent, pageUnloading, topBarButtonClick, copySelection, @@ -129,9 +142,11 @@ import { changeImageByElement, imageOperationCanUndo, imageOperationUndo, +} from "./js/bloomEditing"; +import { addRequestPageContentDelay, removeRequestPageContentDelay, -} from "./js/bloomEditing"; +} from "./js/pageContentDelays"; import { showGamePromptDialog } from "./toolbox/games/GameTool"; // Called from the AI Image Editor overlay in the top window, which owns the session but // cannot touch this page itself; see aiImageEditorPageCommands.ts and aiImageEditorOverlay.ts. @@ -141,11 +156,10 @@ import type { IAiImageEditorCommitResult, } from "./aiImageEditor/aiImageEditorShared"; export { - getBodyContentForSavePage, - requestPageContent, + getPageContentForSaveWhenReady, captureContentForExternalProcessing, - userStylesheetContent, pageUnloading, + notePageContentMayHaveChanged, topBarButtonClick, copySelection, cutSelection, @@ -166,7 +180,7 @@ export { applyAiImageEditorReplacements, }; import { origamiCanUndo, origamiUndo } from "./js/origami"; -import { postString } from "../utils/bloomApi"; +import { postJson } from "../utils/bloomApi"; export { origamiCanUndo, origamiUndo }; const styleSheets = [ @@ -383,6 +397,11 @@ $(document).ready(() => { // in the live editor, which never reads this flag. window.__bloomEditablePageReady = true; + // Start volunteering the page's content to C# whenever it changes and settles, so a save never + // has to ask for it and wait. See pageSnapshot.ts. Deliberately after bootstrap(), so the + // load-time fix-ups it applies are not themselves reported as the user's changes. + startWatchingPageForSnapshots(getPageContentForSaveWhenReady); + // If the user clicks outside of the page thumbnail context menu, we want to close it. // Since it is currently a winforms menu, we do that by sending a message // back to c#-land. We have a similar listener in the pageThumbnailList itself. @@ -400,11 +419,10 @@ export function SayHello() { // Legacy global exposure: mimic old webpack window["editablePageBundle"] contract used by other iframes / C# // NOTE: Keep this as a minimal curated surface: only expose functions intentionally callable cross-frame. interface EditablePageBundleApi { - requestPageContent: typeof requestPageContent; captureContentForExternalProcessing: typeof captureContentForExternalProcessing; - getBodyContentForSavePage: typeof getBodyContentForSavePage; - userStylesheetContent: typeof userStylesheetContent; + getPageContentForSaveWhenReady: typeof getPageContentForSaveWhenReady; pageUnloading: typeof pageUnloading; + notePageContentMayHaveChanged: typeof notePageContentMayHaveChanged; copySelection: typeof copySelection; cutSelection: typeof cutSelection; pasteClipboard: typeof pasteClipboard; @@ -460,8 +478,8 @@ declare global { // the finished page onto window.__bloomExternalPageContent. // 3. C# polls window.__bloomExternalPageContent until it is non-empty and reads it back. // - // Why globals + polling, rather than posting to the editView/pageContent API the way the live - // editor's requestPageContent() does: + // Why globals + polling, rather than posting to the editView/pageSnapshot API the way the + // live editor does: // - That API feeds the live EditingModel; reusing it off-screen would corrupt the real // editor's state. We want the same page-cleanup output, delivered out-of-band. // - C#'s JS runner on this path (RunJavascriptWithStringResult_Sync_Dangerous) is @@ -479,11 +497,10 @@ declare global { } window.editablePageBundle = { - requestPageContent, captureContentForExternalProcessing, - getBodyContentForSavePage, - userStylesheetContent, + getPageContentForSaveWhenReady, pageUnloading, + notePageContentMayHaveChanged, copySelection, cutSelection, pasteClipboard, diff --git a/src/BloomBrowserUI/bookEdit/js/AbovePageControls.tsx b/src/BloomBrowserUI/bookEdit/js/AbovePageControls.tsx index d0d4a918199d..2635d636068d 100644 --- a/src/BloomBrowserUI/bookEdit/js/AbovePageControls.tsx +++ b/src/BloomBrowserUI/bookEdit/js/AbovePageControls.tsx @@ -50,7 +50,7 @@ export function updateAbovePageControls( export function resetAbovePageControls(): void { currentState = defaultState; // Off-screen (e.g. process-book) there is no workspace frame, so this is a no-op there. - // (Reached via removeEditingDebris() in the shared extractAndStripPageContentForSave() save path.) + // (Reached via pageUnloading() in bloomEditing.ts.) tryGetWorkspaceBundleExports()?.setToolboxEnabled(true); const container = document.getElementsByClassName( diff --git a/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts b/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts index 30ab1554988c..8012ac28e0e6 100644 --- a/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts +++ b/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts @@ -25,8 +25,15 @@ import StyleEditor from "../StyleEditor/StyleEditor"; import OverflowChecker from "../OverflowChecker/OverflowChecker"; import BloomField from "../bloomField/BloomField"; import BloomNotices from "./bloomNotices"; +import { reportError } from "../../lib/errorHandler"; import BloomSourceBubbles from "../sourceBubbles/BloomSourceBubbles"; import BloomHintBubbles from "./BloomHintBubbles"; +import { + addRequestPageContentDelay, + removeRequestPageContentDelay, + whenNoActiveDelays, + wrapWithRequestPageContentDelay, +} from "./pageContentDelays"; import { CanvasElementManager, initializeCanvasElementManager, @@ -59,7 +66,13 @@ import { showInvisibles, hideInvisibles } from "./showInvisibles"; //import promise = require('es6-promise'); //promise.Promise.polyfill(); import axios from "axios"; -import { post, postBoolean, postJson, postString } from "../../utils/bloomApi"; +import { + post, + postBoolean, + postJson, + postString, + postThatMightNavigate, +} from "../../utils/bloomApi"; import { showRequestStringDialog } from "../../react_components/RequestStringDialog"; import { hookupLinkHandler } from "../../utils/linkHandler"; @@ -71,6 +84,9 @@ import { ckeditableSelector } from "../../utils/shared"; import { EditableDivUtils } from "./editableDivUtils"; import { setupDragActivityTabControl } from "../toolbox/games/GameTool"; import { addScrollbarsToPage, cleanupNiceScroll } from "bloom-player"; +import { removeNiceScrollArtifacts } from "./niceScrollCleanup"; +import { removeEditorChromeFromClone } from "./editorChromeCleanup"; +import { stopWatchingPageForSnapshots } from "./pageSnapshot"; import { setupBookLinkGrids } from "./linkGrid"; import { fitImageOverTextSplits } from "./autoFitImageOverTextSplits"; import PlaceholderProvider from "./PlaceholderProvider"; @@ -166,6 +182,9 @@ function Cleanup() { cleanupImages(); cleanupOrigami(); + // The live page, so we want bloom-player's version: it tears down the niceScroll instances + // themselves, not just the traces they leave in the DOM (which is all removeNiceScrollArtifacts + // can do, since that has to work on a detached clone). cleanupNiceScroll(); } @@ -1320,117 +1339,51 @@ export function localizeCkeditorTooltips(bar: JQuery) { }); } -// This is invoked when we are about to change pages. -function removeEditingDebris() { - resetAbovePageControls(); - // We are mirroring the Change Layout mode toggle behavior here, in case the user changes - // pages while the Change Layout mode toggle is on. +// Take out of the copy we are about to save the editing-only markup that the C# save pipeline +// does NOT already strip for us. (It removes anything with class bloom-ui or ui-resizable-handle +// and any cke_* classes: see HtmlDom.ProcessPageAfterEditing. It also keeps only the .bloom-page +// div, so nothing outside that div matters either.) +// +// This works entirely on 'cloneOfBody', a detached copy of the live body, so the live page is +// untouched and remains editable. +// +// Note that there is deliberately nothing here corresponding to the old call to +// resetAbovePageControls(): the above-page controls are a bloom-ui element that lives outside the +// .bloom-page div, so they are never saved. Unmounting them belongs to leaving the page, and is +// now done in pageUnloading(). +function removeEditingDebrisFromClone(cloneOfBody: HTMLElement) { + // We are mirroring the Change Layout mode toggle behavior here, in case the user saves + // while the Change Layout mode toggle is on. // The DOM here is for just one page, so there's only ever one marginBox. - const marginBox = document.getElementsByClassName("marginBox")[0]; + const marginBox = cloneOfBody.getElementsByClassName("marginBox")[0]; marginBox.classList.remove("origami-layout-mode"); - const textLabels = marginBox.getElementsByClassName("textBox-identifier"); - for (let i = 0; i < textLabels.length; i++) { - textLabels[i].remove(); - } - removeTransientVideoTimestampParams(document.body); - cleanupNiceScroll(); // don't leave the nicescroll debris around -} - -// Delay notification management for requestPageContent -const activeDelays: string[] = []; -// Upper bound (not a fixed wait) on how long we wait for in-flight async DOM work -// (image sizing, canvas-element layout, etc.) to finish before capturing anyway. The -// wait ends as soon as activeDelays empties, so simple pages are unaffected by this value; -// it only gives slower computers with complex pages more headroom before we give up. -const kMaxWaitTimeMs = 4000; -let requestPageContentTimeout: number | null = null; - -// Add a delay notification that will prevent requestPageContent from running immediately. -// The caller must provide a string ID and pass it to removeRequestPageContentDelay when done. -// IDs do not need to be unique; the same ID can be added multiple times. -export function addRequestPageContentDelay(id: string): void { - activeDelays.push(id); -} - -// Remove a delay notification, allowing requestPageContent to proceed if no other delays are active. -// If this was the last delay, proceed with requesting page content. -export function removeRequestPageContentDelay(id: string): void { - const index = activeDelays.indexOf(id); - if (index === -1) { - console.error( - `removeRequestPageContentDelay: ID "${id}" not found in active delays. Active delays: [${activeDelays.join( - ", ", - )}]`, - ); - return; - } - activeDelays.splice(index, 1); - - // If there are no more delays, go on and request page content. - if (activeDelays.length === 0 && requestPageContentTimeout) { - requestPageContentInternal(); + for (const textLabel of Array.from( + marginBox.getElementsByClassName("textBox-identifier"), + )) { + textLabel.remove(); } + // The scratch element measureText.ts appends to the body to measure text with. It is hidden, + // it is transient (a timer removes it), and it is not part of the page -- but the gather + // clones the whole body, so a save that happens while it is there writes it into the book. + // The window is real: it is created while text is being fitted, which is exactly when the + // user is typing, and a save right after typing is the commonest save there is. + cloneOfBody.querySelector("#measureTextDiv")?.remove(); + removeTransientVideoTimestampParams(cloneOfBody); + removeEditorChromeFromClone(cloneOfBody); } -// Wrap a function that returns a promise with delay management. -// The delay is added before the function is called, and removed when the promise settles (resolves or rejects). -// This ensures that requestPageContent waits for the async operation to complete before saving the page. -export async function wrapWithRequestPageContentDelay( - fn: () => Promise, - delayId: string, -): Promise { - addRequestPageContentDelay(delayId); - try { - const result = await fn(); - removeRequestPageContentDelay(delayId); - return result; - } catch (error) { - removeRequestPageContentDelay(delayId); - throw error; - } -} - -// This is invoked from C# to get the current page content when we want to save it. It removes markup we don't want to save. -// Then it calls an API with the information we need to save. This works around the lack of a -// non-async runJavascript API in WebView2. +// Return the page body + user stylesheet combined with the delimiter that C# splits +// on. Shared by the live editor's gathers (getPageContentForSaveWhenReady) and the off-screen +// capture path (captureContentForExternalProcessing), so the cleanup steps and the delimiter can't +// drift between them. // -// When other javascript code is doing something that will change the page DOM asynchronously and will also cause the -// document to be saved, race conditions are possible. In such cases the delay functions above -// (preferably wrapWithRequestPageContentDelay) should be used to wrap the asynchronous DOM changes to ensure that this -// function does not return the page content for saving until after the changes have been completed. -// The current delay mechanism is not designed to handle multiple concurrent requests. -export function requestPageContent() { - // Check if there are active delay requests. - if (activeDelays.length > 0) { - requestPageContentTimeout = window.setTimeout(() => { - console.warn( - `requestPageContent: Maximum wait time (${kMaxWaitTimeMs}ms) exceeded with active delay(s): [${activeDelays.join( - ", ", - )}]. Proceeding anyway.`, - ); - requestPageContentInternal(); - }, kMaxWaitTimeMs); - } else { - requestPageContentInternal(); - } -} - -// Run the load-time cleanup and return the page body + user stylesheet combined with the -// delimiter that C# splits on. Shared by the live save path (requestPageContentInternal) -// and the off-screen capture path (captureContentForExternalProcessing) so the cleanup steps and the -// delimiter can't drift between them. +// Deliberately NOT exported: every caller should come through getPageContentForSaveWhenReady() (or +// the off-screen path, which does its own waiting), so that nobody can gather the page while +// asynchronous work that belongs in it is still running. It is also deliberately synchronous, so +// that no other event handler can run part way through capturing the page. // -// DESTRUCTIVE READ: this mutates the live DOM as a side effect (removeToolboxMarkup(), -// removeEditingDebris(), and getBodyContentForSavePage() all strip classes, blur elements, turn off -// canvas-element editing, and do CKEditor cleanup) and does NOT restore it afterward. Both current -// callers tolerate this: the live editor re-navigates the page after saving, and the off-screen path -// uses a fresh disposable browser per page. Don't call this from a context where the page must stay -// live and editable afterward. -function extractAndStripPageContentForSave(): string { - // The toolbox is in a separate iframe, hence the call to getToolboxBundleExports(). (Off-screen, - // e.g. process-book, there is no toolbox iframe, so this is a no-op there.) - getToolboxBundleExports()?.removeToolboxMarkup(); - removeEditingDebris(); +// This leaves the live page fully editable: see getBodyContentForSavePage. +function getPageContentForSave(): string { const content = getBodyContentForSavePage(); const userStylesheet = userStylesheetContent(); // (We tossed up whether to use a JSON object instead of a delimiter, but combining two strings is @@ -1438,73 +1391,136 @@ function extractAndStripPageContentForSave(): string { return content + "" + userStylesheet; } -function requestPageContentInternal() { - if (requestPageContentTimeout !== null) { - clearTimeout(requestPageContentTimeout); - } - requestPageContentTimeout = null; +// The way anything outside this file gets the current page's content: wait for any in-flight async +// DOM work that belongs in the saved page, then gather. The page snapshot (pageSnapshot.ts) reads +// the page through this, and so do the page list's commands (see collectCurrentPageContent in +// pageThumbnailList/currentPageContent.ts) to send the content along with a request that will make +// C# save it. +// +// Note the gather happens in the continuation of the await, with nothing awaited in between, so no +// timer can start new work between our finding the register empty and our reading the page. +export async function getPageContentForSaveWhenReady(): Promise { + await whenNoActiveDelays(); + return getPageContentForSave(); +} + +// Save the page and have C# rebuild it from the updated book DOM. Unlike an ordinary save, the +// page IS reloaded, and for these callers that is the point rather than a cost: they have +// restructured the page in ways that have never been through SetupElements (a new origami layout, +// an imported video, a translation group replaced by a derived field), and the reload is what runs +// the page's setup over the result. +// +// Sending the content with the request means C# does not have to ask us for it and wait for the +// answer on a separate API before it can do anything. See EditingModel.SavePageAndReloadIt. +// +// The post itself might navigate this very frame out from under us, hence postThatMightNavigate. +// +// Every caller does `void saveChangesAndRethinkPage()`, so nothing here may reject. The post +// cannot (wrapAxios swallows the rejection), but the gather can -- and a rejection nobody catches +// is silent, because the global unhandledrejection handler is commented out in lib/errorHandler.ts. +// The user would be left looking at a restructured page -- a new origami layout, a video they just +// imported -- that was never saved and never rebuilt, with no hint that anything went wrong. So we +// say so, the same way pageSnapshot.ts does for the failure it cannot afford to be quiet about. +export async function saveChangesAndRethinkPage(): Promise { + let content: string; try { - postString("editView/pageContent", extractAndStripPageContentForSave()); - } catch (e) { - postString( - "editView/pageContent", - "ERROR: " + - e.message + - "\n" + - e.stack + - "\n\n" + - `document ${document ? "exists" : "does not exist"}` + - "\n" + - "body.innerHTML: " + - document?.body?.innerHTML, + content = await getPageContentForSaveWhenReady(); + } catch (error) { + reportError( + "Bloom could not save your changes to this page: " + + (error instanceof Error ? error.message : String(error)), + error instanceof Error ? error.stack : undefined, ); + return; } + await postThatMightNavigate( + "common/saveChangesAndRethinkPageEvent", + content, + ); } -// Caution: We don't want this to become an async method because we don't want -// any other event handlers running between cleaning up the page and -// getting the content to save. (Or think hard before changing that.) -export function getBodyContentForSavePage() { +// Produce the HTML of the current page as it should be saved: a copy of the body with all the +// editing-only markup taken out. +// +// NON-DESTRUCTIVE (BL-13502). We clone the body and do every bit of the cleanup on the CLONE, so +// when we return, the live page has not been touched at all and is still editable. That is what +// allows a Save that does not have to be followed by reloading the page. +// +// Caution: We don't want this to become an async method because we don't want any other event +// handlers running between cleaning up the page and getting the content to save. (Or think hard +// before changing that.) +function getBodyContentForSavePage() { if (hadOrigamiWhenWeLoadedThePage && !hasOrigami(document.body)) { throw new Error( "getBodyContentForSavePage(): The page had origami when it loaded, but it doesn't now (check before cleanup). BL-13120", ); } - const canvasElementEditingOn = - theOneCanvasElementManager.isCanvasElementEditingOn; - if (canvasElementEditingOn) { - theOneCanvasElementManager.turnOffCanvasElementEditing(); - } - // Active element should be forced to blur - if (document.activeElement instanceof HTMLElement) { - document.activeElement.blur(); - } - - const editableDivs = ( - Array.from(document.querySelectorAll("div.bloom-editable")) - ); + // Note: unlike the older, destructive version of this code we deliberately do NOT blur the + // active element. Blurring was harmless when the page was about to be reloaded anyway, but now + // that we save without reloading, it would throw the user's cursor out of the box they are + // typing in on every save. We get the up-to-date text from CKEditor's getData() instead, which + // does not need the box to be blurred. - // We don't think we need to create ckEditor bookmarks and restore the selection - // in this case because we are just saving the page. - // In fact, it was causing problems when we were using them at one point. - // (unfortunately, I don't remember what those problems were...). - const createCkEditorBookMarks = false; - EditableDivUtils.doCkEditorCleanup(editableDivs, createCkEditorBookMarks); + const cloneOfBody = document.body.cloneNode(true) as HTMLElement; + cleanCloneOfBodyForSave(cloneOfBody); - if (hadOrigamiWhenWeLoadedThePage && !hasOrigami(document.body)) { + if (hadOrigamiWhenWeLoadedThePage && !hasOrigami(cloneOfBody)) { throw new Error( "getBodyContentForSavePage(): The page had origami when it loaded, but it doesn't now (check after cleanup). BL-13120", ); } - const result = document.body.innerHTML; + return cloneOfBody.innerHTML; +} - if (canvasElementEditingOn) { - theOneCanvasElementManager.turnOnCanvasElementEditing(); +// Do all the "strip the editing markup" work on 'cloneOfBody', a detached deep copy of the live +// document.body. Nothing here may touch the live page. +function cleanCloneOfBodyForSave(cloneOfBody: HTMLElement) { + // CKEditor's cleaned-up text has to be read from the live editors, since the clone has no + // editors attached to it (BL-12391, BL-16490). + // + // This necessarily happens BEFORE the tool cleanup below, which is the opposite of the order + // the old destructive code used (it detached the tool from the live page and then asked + // CKEditor for the result). We can't do it that way any more: getData() can only report what + // the live editors hold, and the live page must keep its tool markup. So the tools clean the + // text CKEditor gave us, instead of CKEditor cleaning the text the tools left behind. + // + // That order matters to any tool whose cleanup reaches INSIDE an editable, because whatever it + // did there would be overwritten if the CKEditor copy came afterwards. Today that is only the + // Talking Book tool (the phrase-delimiter spans and the audio highlighting). The reader tools + // used to be in that category, but no longer are: their word and sentence highlighting is now + // painted with the CSS Custom Highlight API and puts nothing in the text, so all they clean is + // a class on the page div. + EditableDivUtils.copyCkEditorDataToClone(document.body, cloneOfBody); + + // The bubble tails Comical draws, and the canvas element state that goes with them. Like + // CKEditor, Comical can only produce this from the live editing state, so this reads from the + // live page and writes into the clone. + // + // Only when canvas-element editing is actually on, which is the guard the old destructive code + // had: it reached this work through `if (canvasElementEditingOn) turnOffCanvasElementEditing()`. + // Doing it unconditionally would write balloon position and tail data on pages where editing is + // suspended (the Image Description and Motion tools, a game page in Play mode) -- pages whose + // balloon data a save used to leave exactly as it found it. + if (theOneCanvasElementManager.isCanvasElementEditingOn) { + theOneCanvasElementManager.prepareCloneOfBodyForSave(cloneOfBody); } - return result; + // The toolbox is in a separate iframe, hence the call to getToolboxBundleExports(). (Off-screen, + // e.g. process-book, there is no toolbox iframe, so this is a no-op there.) + const clonedPage = cloneOfBody.getElementsByClassName( + "bloom-page", + )[0] as HTMLElement; + if (clonedPage) { + getToolboxBundleExports()?.removeToolMarkupFromPageClone(clonedPage); + } + + // The scroll bars an overflowing text box gets. Note that this takes the whole body: niceScroll + // puts its rails on the nearest positioned ancestor, which may or may not be inside the page. + removeNiceScrollArtifacts(cloneOfBody); + + removeEditingDebrisFromClone(cloneOfBody); } // Resize each text canvas element (bloom-canvas-element) to fit its content -- growing or shrinking @@ -1557,15 +1573,14 @@ function resizeCanvasElementsToFitContent(): void { } // Used by the off-screen "process whole book" path (C# BookProcessor, driven by the -// external/process-book API). It gathers the same page content that requestPageContent() would save -// (via the shared extractAndStripPageContentForSave()), but instead of posting it to the editView/pageContent -// API (which feeds the LIVE EditingModel and would corrupt the live editor's state), it stashes the -// combined result on window.__bloomExternalPageContent for the C# caller to poll. Like -// requestPageContent(), it first waits for any in-flight async DOM work (activeDelays) to finish, up to -// kMaxWaitTimeMs, so browser-based measurements (image sizing, canvas-element layout, etc.) are complete -// before we capture the page. It also resizes text canvas elements to fit their content (see -// resizeCanvasElementsToFitContent), since that auto-height adjustment is otherwise deferred on a -// timer the wait loop does not track. +// external/process-book API). It gathers the same page content a save would (via the shared +// getPageContentForSave()), but instead of posting it to an API that feeds the LIVE EditingModel +// (which would corrupt the live editor's state), it stashes the combined result on +// window.__bloomExternalPageContent for the C# caller to poll. Like every other gathering path it +// goes through whenNoActiveDelays() first, so browser-based measurements (image sizing, +// canvas-element layout, etc.) are complete before we capture the page. It also resizes text canvas +// elements to fit their content (see resizeCanvasElementsToFitContent), since that auto-height +// adjustment is otherwise deferred on a timer the wait loop does not track. export function captureContentForExternalProcessing( fitImageTextSplits?: boolean, ): void { @@ -1597,30 +1612,22 @@ export function captureContentForExternalProcessing( } } - const start = Date.now(); - const finish = () => { + void whenNoActiveDelays().then(() => { try { resizeCanvasElementsToFitContent(); - window.__bloomExternalPageContent = - extractAndStripPageContentForSave(); + window.__bloomExternalPageContent = getPageContentForSave(); } catch (e) { window.__bloomExternalPageContent = "ERROR: " + (e && e.message) + "\n" + (e && e.stack); } - }; - const waitForDelaysThenFinish = () => { - if (activeDelays.length === 0 || Date.now() - start > kMaxWaitTimeMs) { - finish(); - } else { - setTimeout(waitForDelaysThenFinish, 50); - } - }; - waitForDelaysThenFinish(); + }); } -// Called from C# by a RunJavaScript() in EditingView.CleanHtmlAndCopyToPageDom via -// workspaceBundle.getEditablePageBundleExports(). -export const userStylesheetContent = () => { +// The user-defined styles, which travel to C# as the second half of what +// getPageContentForSave() returns. (This used to say it was called from C# by a RunJavaScript in +// EditingView.CleanHtmlAndCopyToPageDom; that method is long gone, and nothing outside this file +// calls this now.) +const userStylesheetContent = () => { const ss = Array.from(document.styleSheets).find( (s) => s.title === "userModifiedStyles", ) as CSSStyleSheet | undefined; @@ -1630,12 +1637,43 @@ export const userStylesheetContent = () => { .join("\n"); }; +// Whether this page has already been torn down. A page document only ever goes away once, but +// pageUnloading() can be ASKED for twice on the same one: leaving the Edit tab runs it (from +// EditingView.OnHideEditTab, since nothing navigates the page frame then), and coming back +// re-navigates that frame, which runs it again from switchContentPage() before the new page +// replaces this document. +// +// The second run is not harmless. detachCurrentTool() does not forget the current tool after +// detaching it, so it detaches again -- and the second detach usually does not reach +// removeToolMarkup(), which makes detachToolFromPage() report the tool for "forgetting" to call +// super.detachFromPage(). That accusation is false, and it points at a tool that is behaving +// perfectly well. +let thisPageHasBeenUnloaded = false; + export const pageUnloading = () => { + if (thisPageHasBeenUnloaded) return; + thisPageHasBeenUnloaded = true; + // Stop volunteering snapshots of a page that is going away. C# clears its copy when it starts + // navigating, so anything we sent after that would be for a page nobody is on. See + // pageSnapshot.ts. + stopWatchingPageForSnapshots(); // It's just possible that 'theOneCanvasElementManager' hasn't been initialized. // If not, just ignore this, since it's a no-op at this point anyway. if (theOneCanvasElementManager) { theOneCanvasElementManager.cleanUp(); } + // Shut the open toolbox tool down. This releases whatever it was holding on the page we are + // leaving -- observers, listeners, and any UI it had opened such as a colour picker -- and it + // is the counterpart of the newPageReady() the tool gets for the page we are going to. + // + // Like resetAbovePageControls() below, this used to happen as a side effect of saving, because + // gathering the page content began by detaching the tool from the live page. A save no longer + // touches the live page, so without this nothing detaches the tool at all, and every page + // change leaks another page's worth of the tool's hooks. + getToolboxBundleExports()?.removeToolboxMarkup(); + // Unmount the React root for the controls above the page and re-enable the toolbox (the + // Change Layout toggle disables it). Same story as above: it used to ride along with the save. + resetAbovePageControls(); }; export function topBarButtonClick(button: { command: string }) { @@ -1967,6 +2005,32 @@ export function attachToCkEditor(element) { $("body").addClass("hideAllCKEditors"); const ckedit = CKEDITOR.inline(element); + // Until this editor is ready, we cannot read the true saved text of the box it owns: + // copyCkEditorDataToClone gets the text from the live editors, and before instanceReady there + // is no editor to ask, so the gather reports whatever is in the DOM instead. That is not the + // same thing. SetupElements puts an empty

into an empty editable; CKEditor’s getData() + // reports the box as empty, which is what the book on disk says. So a gather taken in this + // window differs from one taken just after it, for every empty box on the page. + // + // That difference is what made a page nobody had touched decide it had unsaved changes: the + // page snapshot’s baseline is taken as soon as the delay register is clear, which used to be + // before any editor was ready. Registering here (and releasing at instanceReady) puts CKEditor + // attachment under the same gate as image sizing and the other load-time work that finishes + // asynchronously -- which is exactly what the register is for, and it means an early SAVE gets + // the real text too, instead of writing

into boxes the user left empty. + const ckEditorDelayId = "attachToCkEditor " + ckedit.id; + addRequestPageContentDelay(ckEditorDelayId); + let ckEditorDelayReleased = false; + const releaseCkEditorDelay = () => { + if (ckEditorDelayReleased) return; + ckEditorDelayReleased = true; + removeRequestPageContentDelay(ckEditorDelayId); + }; + // (instanceReady is not on CKEditor’s TypeScript type; toolbox.ts declares it the same way.) + if ((ckedit as { instanceReady?: boolean }).instanceReady) + releaseCkEditorDelay(); + else ckedit.on("instanceReady", releaseCkEditorDelay); + // Record the div of the edit box for use later in positioning the format bar. mapCkeditDiv[ckedit.id] = element; diff --git a/src/BloomBrowserUI/bookEdit/js/bloomImages.ts b/src/BloomBrowserUI/bookEdit/js/bloomImages.ts index c48ca0abc0b7..adf9369c18c9 100644 --- a/src/BloomBrowserUI/bookEdit/js/bloomImages.ts +++ b/src/BloomBrowserUI/bookEdit/js/bloomImages.ts @@ -22,11 +22,8 @@ import { farthest } from "../../utils/elementUtils"; import { EditableDivUtils } from "./editableDivUtils"; import { playingBloomGame } from "../toolbox/games/DragActivityTabControl"; import { getWorkspaceBundleExports } from "./workspaceFrames"; -import { - changeImage, - IImageInfo, - wrapWithRequestPageContentDelay, -} from "./bloomEditing"; +import { changeImage, IImageInfo } from "./bloomEditing"; +import { wrapWithRequestPageContentDelay } from "./pageContentDelays"; import { getCanvasElementManager } from "../toolbox/canvas/canvasElementPageBridge"; import BloomMessageBoxSupport from "../../utils/bloomMessageBoxSupport"; import $ from "jquery"; diff --git a/src/BloomBrowserUI/bookEdit/js/bloomVideo.ts b/src/BloomBrowserUI/bookEdit/js/bloomVideo.ts index e93ed298dea5..6ddc1a7cc5ab 100644 --- a/src/BloomBrowserUI/bookEdit/js/bloomVideo.ts +++ b/src/BloomBrowserUI/bookEdit/js/bloomVideo.ts @@ -1,4 +1,4 @@ -import { postThatMightNavigate } from "../../utils/bloomApi"; +import { saveChangesAndRethinkPage } from "./bloomEditing"; // The code in this file supports operations on video panels in custom pages (and potentially elsewhere). // It sets things up for the button (plural eventually) to appear when hovering over the video. @@ -196,7 +196,7 @@ export function doVideoCommand( // Makes sure the page gets saved with a reference to the new video, // and incidentally that everything gets updated to be consistent with the // new state of things. - postThatMightNavigate("common/saveChangesAndRethinkPageEvent"); + void saveChangesAndRethinkPage(); } }); } else if (command === "record") { diff --git a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementAlternates.test.ts b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementAlternates.test.ts new file mode 100644 index 000000000000..c30417c8ec7a --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementAlternates.test.ts @@ -0,0 +1,76 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +// Every test passes the language explicitly, so the collection settings this module would +// otherwise consult are never reached. +import { saveStateOfCanvasElementAsCurrentLangAlternate } from "./CanvasElementAlternates"; + +const kLang = "xyz"; + +function makeCanvasElement(dataBubble: string | null): HTMLElement { + const el = document.createElement("div"); + el.className = "bloom-canvas-element"; + el.setAttribute("style", "left: 10px; top: 20px;"); + if (dataBubble !== null) el.setAttribute("data-bubble", dataBubble); + const editable = document.createElement("div"); + editable.className = "bloom-editable"; + editable.setAttribute("lang", kLang); + el.appendChild(editable); + return el; +} + +function alternateOn(canvasElement: HTMLElement): string | null { + return canvasElement + .getElementsByClassName("bloom-editable")[0] + .getAttribute("data-bubble-alternate"); +} + +describe("saveStateOfCanvasElementAsCurrentLangAlternate", () => { + beforeEach(() => { + vi.spyOn(console, "warn").mockImplementation(() => {}); + }); + + it("records the alternate when the bubble data is readable", () => { + // Bloom stores this JSON with backticks standing in for the quotes. + const el = makeCanvasElement("{`version`:`1.0`,`tails`:[{`tipX`:1}]}"); + + saveStateOfCanvasElementAsCurrentLangAlternate(el, kLang); + + const written = alternateOn(el); + expect(written).not.toBeNull(); + expect(written).toContain("`lang`:`" + kLang + "`"); + expect(written).toContain("tipX"); + }); + + it("does not lose the whole page when a canvas element has no bubble data", () => { + // The real hazard: this runs inside the clone gather, so throwing here does not merely + // skip one alternate, it aborts gathering the page -- and then the page cannot be saved at + // all. A missing attribute used to throw, because JSON.parse("") is an error. + const el = makeCanvasElement(null); + + expect(() => + saveStateOfCanvasElementAsCurrentLangAlternate(el, kLang), + ).not.toThrow(); + expect(alternateOn(el)).toBeNull(); + }); + + it("does not lose the whole page when the bubble data is malformed", () => { + const el = makeCanvasElement("{this is not json"); + + expect(() => + saveStateOfCanvasElementAsCurrentLangAlternate(el, kLang), + ).not.toThrow(); + expect(alternateOn(el)).toBeNull(); + }); + + it("leaves an existing alternate alone rather than replacing it with an empty one", () => { + // Recording an alternate with no tails would claim this language's copy has none, and the + // user would lose them on switching to it. Skipping is the conservative choice. + const el = makeCanvasElement("{broken"); + const editable = el.getElementsByClassName("bloom-editable")[0]; + editable.setAttribute("data-bubble-alternate", "{`lang`:`xyz`}"); + + saveStateOfCanvasElementAsCurrentLangAlternate(el, kLang); + + expect(alternateOn(el)).toBe("{`lang`:`xyz`}"); + }); +}); diff --git a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementAlternates.ts b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementAlternates.ts index bfa40064e2a8..8fc1893b45fe 100644 --- a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementAlternates.ts +++ b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementAlternates.ts @@ -39,7 +39,29 @@ export const saveStateOfCanvasElementAsCurrentLangAlternate = ( ).find((e) => e.getAttribute("lang") === canvasElementLang); if (editable) { const bubbleData = canvasElement.getAttribute("data-bubble") ?? ""; - const bubbleDataObj = JSON.parse(bubbleData.replace(/`/g, '"')); + // A canvas element with no data-bubble at all, or one we cannot read, must not cost the + // user the whole page. This runs inside the clone gather, so a throw here does not just + // skip one alternate: it aborts gathering the page, which means the page cannot be saved + // and -- since the browser now gathers after every change, not only when saving -- says so + // over and over. Note that the ?? "" above makes a MISSING attribute throw too, because + // JSON.parse("") is an error; that is the likelier of the two ways in. + // + // Skipping leaves any alternate already on the editable alone, which is the conservative + // choice: recording one with no tails would claim this language's copy has none, and lose + // the tails when the user switches to it. + let bubbleDataObj: { tails?: object[] }; + try { + bubbleDataObj = JSON.parse(bubbleData.replace(/`/g, '"')); + } catch (e) { + console.warn( + "Not recording a canvas-element alternate for lang " + + canvasElementLang + + ": its data-bubble could not be read (" + + bubbleData.slice(0, 60) + + ")", + ); + return; + } const alternate = { lang: canvasElementLang, style: canvasElement.getAttribute("style") ?? "", diff --git a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementBackgroundImageManager.ts b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementBackgroundImageManager.ts index 70a5fc379a6a..a3de38f0c749 100644 --- a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementBackgroundImageManager.ts +++ b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementBackgroundImageManager.ts @@ -8,7 +8,7 @@ import { isPlaceHolderImage, SetupMetadataButton, } from "../bloomImages"; -import { wrapWithRequestPageContentDelay } from "../bloomEditing"; +import { wrapWithRequestPageContentDelay } from "../pageContentDelays"; import { getExactClientSize } from "../../../utils/elementUtils"; import type { IImageCropInfo } from "../ImageUndoManager"; import { @@ -277,7 +277,9 @@ function putBubbleBefore( const bubble = new Bubble(b as HTMLElement); const spec = bubble.getBubbleSpec(); // the one previously at minLevel will now be at requiredLevel+1, others higher in same sequence. - spec.level += requiredLevel - minLevel + 1; + // Treat a missing level as 0, exactly as the minLevel computation above does. (Before + // comicaljs 0.4.x we could not see that level is optional, and a missing one made this NaN.) + spec.level = (spec.level ?? 0) + requiredLevel - minLevel + 1; bubble.persistBubbleSpec(); }); minLevel = 2; diff --git a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementBubbleLevelUtils.ts b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementBubbleLevelUtils.ts index 7a849534ca9e..f886a4c5631c 100644 --- a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementBubbleLevelUtils.ts +++ b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementBubbleLevelUtils.ts @@ -28,7 +28,9 @@ export const putBubbleBefore = ( const bubble = new Bubble(b as HTMLElement); const spec = bubble.getBubbleSpec(); // the one previously at minLevel will now be at requiredLevel+1, others higher in same sequence. - spec.level += requiredLevel - minLevel + 1; + // Treat a missing level as 0, exactly as the minLevel computation above does. (Before + // comicaljs 0.4.x we could not see that level is optional, and a missing one made this NaN.) + spec.level = (spec.level ?? 0) + requiredLevel - minLevel + 1; bubble.persistBubbleSpec(); }); minLevel = 2; diff --git a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementClipboard.test.ts b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementClipboard.test.ts index 49b4e40b37fa..082fdd9c912d 100644 --- a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementClipboard.test.ts +++ b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementClipboard.test.ts @@ -35,6 +35,9 @@ vi.mock("../bloomEditing", () => ({ }, ), notifyToolOfChangedImage: vi.fn(), +})); + +vi.mock("../pageContentDelays", () => ({ wrapWithRequestPageContentDelay: vi.fn(), })); @@ -64,10 +67,8 @@ vi.mock("../../toolbox/canvas/CanvasElementItem", () => ({ })); import { SetupMetadataButton } from "../bloomImages"; -import { - changeImageInfo, - wrapWithRequestPageContentDelay, -} from "../bloomEditing"; +import { changeImageInfo } from "../bloomEditing"; +import { wrapWithRequestPageContentDelay } from "../pageContentDelays"; import { CanvasElementClipboard, ICanvasElementClipboardHost, diff --git a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementClipboard.ts b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementClipboard.ts index 410007b90d54..001422567ccd 100644 --- a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementClipboard.ts +++ b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementClipboard.ts @@ -8,8 +8,8 @@ import { kMakeNewCanvasElement, changeImageInfo, notifyToolOfChangedImage, - wrapWithRequestPageContentDelay, } from "../bloomEditing"; +import { wrapWithRequestPageContentDelay } from "../pageContentDelays"; import { getBackgroundCanvasElementFromBloomCanvas, isPlaceHolderImage, diff --git a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementFactories.ts b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementFactories.ts index b9ea73d5d9cb..b93069d58204 100644 --- a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementFactories.ts +++ b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementFactories.ts @@ -874,7 +874,8 @@ export class CanvasElementFactories { bloomCanvas.getElementsByClassName(kCanvasElementClass), ) as HTMLElement[] ).filter((x) => x !== backgroundImage), - Bubble.getBubbleSpec(backgroundImage).level + 1, + // A missing level counts as 0, as everywhere else we do this arithmetic. + (Bubble.getBubbleSpec(backgroundImage).level ?? 0) + 1, ); } } diff --git a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementManager.ts b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementManager.ts index a8165a66fe6b..dfb97b45b436 100644 --- a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementManager.ts +++ b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementManager.ts @@ -21,11 +21,13 @@ import { getRgbaColorStringFromColorAndOpacity } from "../../../utils/colorUtils import { IImageInfo, SetupElements, - addRequestPageContentDelay, attachToCkEditor, notifyToolOfChangedImage, - removeRequestPageContentDelay, } from "../bloomEditing"; +import { + addRequestPageContentDelay, + removeRequestPageContentDelay, +} from "../pageContentDelays"; import { EnableAllImageEditing, getImageFromCanvasElement, @@ -2355,6 +2357,54 @@ export class CanvasElementManager { ); } + // The save-a-page-without-reloading counterpart of turnOffCanvasElementEditing(): put into + // 'cloneOfBody' -- a detached copy of the live document.body -- everything that turning canvas + // element editing off would have put into the page, and leave the live page still being edited. + // + // Only three of the things turnOffCanvasElementEditing() does affect what gets saved: + // * Comical converts its editing into the that draws the bubble tails without + // Javascript. exportSvgToCopiesOfParents does that into the copy while leaving the live + // paper projects alone (comicaljs 0.4.1; before that there was only the destructive + // stopEditing()). + // * The current canvas element positions are recorded as the alternate for the current + // language. That is pure attribute manipulation -- it reads style and data-bubble and + // writes data-bubble-alternate -- so it works on a detached clone, which has no layout. + // * The bloom-focusedCanvasElement class comes off. Nothing else strips it: it is not a + // bloom-ui element, so the C# save pipeline would keep it. + // The rest is live-only: the control frame is a bloom-ui element (so C# discards it anyway), + // EnableAllImageEditing only adds bloom-ui buttons back to the live page, and the listener + // removal has no bearing on the HTML. + public prepareCloneOfBodyForSave(cloneOfBody: HTMLElement): void { + const liveBloomCanvases = this.getAllBloomCanvasesOnPage(); + const clonedBloomCanvases = Array.from( + cloneOfBody.getElementsByClassName(kBloomCanvasClass), + ) as HTMLElement[]; + if (liveBloomCanvases.length !== clonedBloomCanvases.length) { + throw new Error( + `prepareCloneOfBodyForSave(): the clone has ${clonedBloomCanvases.length} bloom-canvases but the live page has ${liveBloomCanvases.length}. The clone must be an untouched copy of the live page.`, + ); + } + + Comical.exportSvgToCopiesOfParents( + liveBloomCanvases.map((liveBloomCanvas, index) => [ + liveBloomCanvas, + clonedBloomCanvases[index], + ]), + ); + + clonedBloomCanvases.forEach((clonedBloomCanvas) => + this.saveCurrentCanvasElementStateAsCurrentLangAlternate( + clonedBloomCanvas, + ), + ); + + Array.from( + cloneOfBody.getElementsByClassName("bloom-focusedCanvasElement"), + ).forEach((element) => + element.classList.remove("bloom-focusedCanvasElement"), + ); + } + public cleanUp(): void { // We used to close a WebSocket here; saving the hook in case we need it someday. } diff --git a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementResizeAdjustments.ts b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementResizeAdjustments.ts index 5a58121ce008..ec994e816328 100644 --- a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementResizeAdjustments.ts +++ b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementResizeAdjustments.ts @@ -155,10 +155,13 @@ export function adjustCanvasElementChildrenIfSizeChanged( let newChildHeight = child.clientHeight; let reposition = true; const bubbleSpec = Bubble.getBubbleSpec(child); - needComicalUpdate = - needComicalUpdate || - (!!bubbleSpec.tails && bubbleSpec.tails.length > 0) || - bubbleSpec.spec !== "none"; + // This used to end with `|| bubbleSpec.spec !== "none"`. BubbleSpec has no `spec` member — + // it has `style` — so that term was always true, and this has in fact always been set for + // every child. Until comicaljs 0.4.x a broken import in its .d.ts files typed BubbleSpec as + // `any`, which is why the compiler never objected. Keeping the behavior we have actually + // been shipping rather than quietly changing it to `style` while bumping a dependency; + // whether it SHOULD test style is a separate question. See Edit/SavingWithoutReloading.md. + needComicalUpdate = true; if ( Array.from(child.children).some( (c: HTMLElement) => diff --git a/src/BloomBrowserUI/bookEdit/js/editableDivUtils.ts b/src/BloomBrowserUI/bookEdit/js/editableDivUtils.ts index b7feafb5da25..620ea1940637 100644 --- a/src/BloomBrowserUI/bookEdit/js/editableDivUtils.ts +++ b/src/BloomBrowserUI/bookEdit/js/editableDivUtils.ts @@ -388,6 +388,49 @@ export class EditableDivUtils { return bookmarksForEachEditable; } + // The non-destructive counterpart of doCkEditorCleanup(). Instead of writing CKEditor's + // cleaned-up data back into the LIVE editable divs (which disturbs the running editors and is + // one of the reasons the old save path had to reload the page afterwards), this reads the data + // from the live editors and writes it into the corresponding divs of a detached CLONE of the + // page. The live page is left completely alone. + // liveRoot and cloneRoot must be a live element and a deep clone of it, so that the Nth + // div.bloom-editable in each corresponds; we throw if they have drifted apart. + // See doCkEditorCleanup for why we want getData() rather than the raw innerHTML (BL-12391), + // and removeCkEditorFillingChars for the stray filling char case (BL-16490). + public static copyCkEditorDataToClone( + liveRoot: HTMLElement, + cloneRoot: HTMLElement, + ): void { + const liveDivs = Array.from( + liveRoot.querySelectorAll("div.bloom-editable"), + ); + const cloneDivs = Array.from( + cloneRoot.querySelectorAll("div.bloom-editable"), + ); + if (liveDivs.length !== cloneDivs.length) { + throw new Error( + `copyCkEditorDataToClone(): the clone has ${cloneDivs.length} bloom-editables but the live page has ${liveDivs.length}. The clone must be an untouched copy of the live page.`, + ); + } + liveDivs.forEach((liveDiv, index) => { + const ckeditorOfThisBox = (liveDiv).bloomCkEditor; + if (!ckeditorOfThisBox) { + return; // no editor attached (e.g. an invisible language), so nothing to clean. + } + const ckEditorData = EditableDivUtils.removeCkEditorFillingChars( + ckeditorOfThisBox.getData(), + ); + // Same test as doCkEditorCleanup: only bother when getData() actually differs from + // what is in the DOM. + if (ckEditorData !== liveDiv.innerHTML) { + this.safelyReplaceContentWithCkEditorData( + cloneDivs[index], + ckEditorData, + ); + } + }); + } + // public for unit testing public static safelyReplaceContentWithCkEditorData( div: HTMLDivElement, diff --git a/src/BloomBrowserUI/bookEdit/js/editorChromeCleanup.spec.ts b/src/BloomBrowserUI/bookEdit/js/editorChromeCleanup.spec.ts new file mode 100644 index 000000000000..8bfc013d198e --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/editorChromeCleanup.spec.ts @@ -0,0 +1,155 @@ +import { describe, it, expect } from "vitest"; +import { removeEditorChromeFromClone } from "./editorChromeCleanup"; + +// A clone of the body as it looks once the editor has finished waking up on an xmatter page: the +// page itself, plus everything CKEditor and qTip added around and inside it. Compare the same page +// as it sits on disk, which is just the .bloom-page div. +function makeClonedBodyWithChrome(): HTMLElement { + const body = document.createElement("div"); // stands in for the cloned document.body + body.innerHTML = ` +
Change Layout
+
+
+
+ +
+ +
+
+
+

The title

+
+
+
+
+
+
+
+ Bold +
+
+
Book title in Temein
+
`; + return body; +} + +describe("removeEditorChromeFromClone", () => { + it("removes the chrome and leaves the page itself alone", () => { + const body = makeClonedBodyWithChrome(); + + // Sanity check the fixture really is in the "editor is running" state, so that a test + // which passes because the chrome was never there cannot masquerade as a passing test. + expect(body.querySelectorAll(".bloom-ui").length).toBe(3); + expect(body.querySelector("#cke_editor1")).not.toBeNull(); + expect(body.querySelector("div.qtip")).not.toBeNull(); + expect(body.querySelector(".ui-resizable-handle")).not.toBeNull(); + + removeEditorChromeFromClone(body); + + expect(body.querySelectorAll(".bloom-ui").length).toBe(0); + expect(body.querySelector("#cke_editor1")).toBeNull(); + expect(body.querySelector("div.qtip")).toBeNull(); + expect(body.querySelector(".ui-resizable-handle")).toBeNull(); + expect(body.querySelector(".cke_widget_wrapper")).toBeNull(); + + // The page's own content survives untouched. + const page = body.querySelector(".bloom-page")!; + expect(page).not.toBeNull(); + expect(page.querySelector("img")!.getAttribute("src")).toBe( + "cover.jpg", + ); + expect(page.querySelector(".bloom-editable p")!.textContent).toBe( + "The title", + ); + }); + + it("strips cke_ classes but keeps the classes that mean something to Bloom", () => { + const body = makeClonedBodyWithChrome(); + const editableBefore = body.querySelector(".bloom-editable")!; + expect(editableBefore.className).toContain("cke_editable"); + + removeEditorChromeFromClone(body); + + const editable = body.querySelector(".bloom-editable")!; + expect(editable.className).toBe("bloom-editable normal-style"); + }); + + it("removes the class attribute entirely when only cke_ classes were on it", () => { + const body = document.createElement("div"); + body.innerHTML = `
x
`; + + removeEditorChromeFromClone(body); + + const span = body.querySelector("span")!; + expect(span).not.toBeNull(); // it is kept; only its class goes + expect(span.hasAttribute("class")).toBe(false); + }); + + it("removes qTip's bookkeeping attributes, which otherwise churn between runs", () => { + const body = makeClonedBodyWithChrome(); + expect(body.querySelectorAll("[data-hasqtip]").length).toBe(2); + + removeEditorChromeFromClone(body); + + expect(body.querySelectorAll("[data-hasqtip]").length).toBe(0); + expect(body.querySelectorAll("[aria-describedby]").length).toBe(0); + }); + + it("keeps an aria-describedby that is not qTip's", () => { + const body = document.createElement("div"); + body.innerHTML = `
`; + + removeEditorChromeFromClone(body); + + expect( + body.querySelector("img")!.getAttribute("aria-describedby"), + ).toBe("figdesc7"); + }); + + it("drops the regenerated ids from Comical's SVG but keeps the SVG itself", () => { + // The SVG is saved on purpose -- it is what draws the bubbles for a reader that has no + // Comical -- but paper.js stamps a fresh GUID into its ids on every redraw, which made + // every page with a bubble look edited on every visit. + const body = document.createElement("div"); + body.innerHTML = `
+ + + +
`; + expect(body.querySelectorAll("svg.comical-generated [id]").length).toBe( + 2, + ); + + removeEditorChromeFromClone(body); + + const svg = body.querySelector("svg.comical-generated")!; + expect(svg).not.toBeNull(); // the drawing itself must survive + expect(svg.querySelectorAll("[id]").length).toBe(0); + // and the geometry, which is the part that actually means something, is untouched + expect(svg.querySelectorAll("path").length).toBe(2); + expect(svg.querySelector("path")!.getAttribute("d")).toBe( + "M-3,328v-331h475v331z", + ); + }); + + it("leaves ids alone on an svg that is not Comical's", () => { + const body = document.createElement("div"); + body.innerHTML = `
`; + + removeEditorChromeFromClone(body); + + expect(body.querySelector("#keep-me")).not.toBeNull(); + }); + + it("does not remove CKEditor bookmark spans, whose ids also start with cke_", () => { + const body = document.createElement("div"); + body.innerHTML = `

ab

`; + + removeEditorChromeFromClone(body); + + expect(body.querySelector("#cke_bm_71S")).not.toBeNull(); + expect(body.querySelector("p")!.textContent).toBe("ab"); + }); +}); diff --git a/src/BloomBrowserUI/bookEdit/js/editorChromeCleanup.ts b/src/BloomBrowserUI/bookEdit/js/editorChromeCleanup.ts new file mode 100644 index 000000000000..4d0e9c1da05a --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/editorChromeCleanup.ts @@ -0,0 +1,85 @@ +// Strip, from a CLONE of the editing page, the chrome that only exists because the page is being +// edited -- so that what we hand C# is the page, not the editor. +// +// Everything here is something HtmlDom.ProcessPageAfterEditing already removes on the C# side, and +// still does; this is not a replacement for it. What it changes is what we SEND, and that matters +// because of how the page snapshot decides to send anything at all: it posts whenever the gathered +// string differs from the last one it sent (see pageSnapshot.ts). Chrome in that string therefore +// made pages look edited when nobody had touched them -- C# would hold a snapshot, conclude there +// were unsaved changes, and save on the way out. +// +// The offenders: +// * CKEditor’s toolbars and qTip’s bubbles, which those libraries append to the document body. +// Big (they were 20 KB of a 26 KB page) and restless: a bubble fades in and slides into place, +// so its inline style changes several times a second while it appears. +// * bloom-ui elements inside the page -- the image buttons, the format cog. +// * the cke_ classes CKEditor puts on each editable as it attaches. +// * qTip’s bookkeeping attributes. These are the ones that churn between RUNS rather than +// within one: the number in "qtip-0" is handed out in the order the bubbles happen to be +// created, so it rarely matches the number the box was saved with. BloomHintBubbles has long +// noted the wart -- "we unfortunately save in the file the qtip attributes that get added like +// aria-describedby=qtip-0 and has-qtip=true" -- and BookData._attributesNotToCopy already +// refuses to copy them into the data div, calling them "junk that gets left behind by UI". +// +// Nothing here may touch the live page; the caller passes a detached deep copy of document.body. +export function removeEditorChromeFromClone(cloneOfBody: HTMLElement) { + for (const element of Array.from( + cloneOfBody.querySelectorAll(".bloom-ui, .ui-resizable-handle"), + )) { + element.remove(); + } + + // CKEditor’s floating toolbars and qTip’s bubbles. Matching CKEditor by the "cke" class + // rather than the id, because ids beginning "cke_" are also used for bookmark spans INSIDE the + // text, which must not be removed here. bloomQtipUtils.cleanupBubbles() removes the same + // div.qtip elements from the live page. + for (const element of Array.from( + cloneOfBody.querySelectorAll(".cke, div.qtip"), + )) { + element.remove(); + } + + // Only qtip-* values, so that an aria-describedby someone put there on purpose survives. + for (const element of Array.from( + cloneOfBody.querySelectorAll( + "[aria-describedby], [data-hasqtip], [ariasecondary-describedby]", + ), + )) { + if (element.getAttribute("aria-describedby")?.startsWith("qtip-")) + element.removeAttribute("aria-describedby"); + if ( + element + .getAttribute("ariasecondary-describedby") + ?.startsWith("qtip-") + ) + element.removeAttribute("ariasecondary-describedby"); + element.removeAttribute("data-hasqtip"); + } + + // The ids paper.js leaves on the SVG Comical draws for the speech bubbles. Unlike everything + // else here this markup IS saved -- the SVG is what draws the bubbles in the reader, which has + // no Comical to redraw them -- but the ids are regenerated with a fresh GUID every time the + // SVG is, so an otherwise identical redraw produced a different page and any page with a + // bubble looked edited on every visit, forever. On one test book that was five or six + // snapshots per page visit, all of them this. + // + // Safe to drop rather than stabilise: nothing inside the SVG references them (no url(#...), + // no href="#..."), the GUID appears nowhere else in the page, and they are not even unique -- + // "...outlineShape 1 1" occurs twice in one SVG. They are debris, not identifiers. + for (const element of Array.from( + cloneOfBody.querySelectorAll("svg.comical-generated [id]"), + )) { + element.removeAttribute("id"); + } + + // The classes CKEditor adds to each editable it attaches to (cke_editable, cke_focus, ...). + for (const element of Array.from( + cloneOfBody.querySelectorAll("[class*='cke_']"), + )) { + const kept = Array.from(element.classList).filter( + (c) => !c.startsWith("cke_"), + ); + if (kept.length === 0) element.removeAttribute("class"); + else element.setAttribute("class", kept.join(" ")); + } +} diff --git a/src/BloomBrowserUI/bookEdit/js/niceScrollCleanup.spec.ts b/src/BloomBrowserUI/bookEdit/js/niceScrollCleanup.spec.ts new file mode 100644 index 000000000000..0b308e504841 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/niceScrollCleanup.spec.ts @@ -0,0 +1,136 @@ +import { describe, it, expect, beforeEach } from "vitest"; +import { removeNiceScrollArtifacts } from "./niceScrollCleanup"; + +// A translationGroup whose editable has been given a niceScroll, in the state bloom-player's +// addScrollbarsToPage() and niceScroll between them leave it: the alignment class moved aside to +// its "-removed" marker, inline styles on the editable, and a rail (with its cursor inside) +// inserted into the nearest positioned ancestor. +function makeScrolledPage(): HTMLElement { + const body = document.createElement("div"); // stands in for the cloned document.body + body.innerHTML = ` +
+
+
+
+
+

Some text that overflows.

+
+
+
+
+
+
+
+
`; + return body; +} + +describe("removeNiceScrollArtifacts", () => { + let body: HTMLElement; + beforeEach(() => { + body = makeScrolledPage(); + }); + + it("sanity check: the test page starts out with all the artifacts", () => { + expect(body.querySelectorAll(".nicescroll-rails").length).toBe(1); + expect(body.querySelectorAll(".nicescroll-cursors").length).toBe(1); + expect( + body.querySelectorAll(".bloom-vertical-align-center-removed") + .length, + ).toBe(1); + expect( + body.querySelector(".bloom-editable")!.style.overflowY, + ).toBe("hidden"); + }); + + it("removes the rails and the cursors niceScroll inserted", () => { + removeNiceScrollArtifacts(body); + + expect(body.querySelectorAll(".nicescroll-rails").length).toBe(0); + expect(body.querySelectorAll(".nicescroll-cursors").length).toBe(0); + }); + + it("puts back the vertical alignment class, so we don't save the page having lost it", () => { + removeNiceScrollArtifacts(body); + + const group = body.querySelector(".bloom-translationGroup")!; + expect(group.classList.contains("bloom-vertical-align-center")).toBe( + true, + ); + expect( + group.classList.contains("bloom-vertical-align-center-removed"), + ).toBe(false); + }); + + it("puts back bloom-vertical-align-bottom too", () => { + const group = body.querySelector(".bloom-translationGroup")!; + group.classList.remove("bloom-vertical-align-center-removed"); + group.classList.add("bloom-vertical-align-bottom-removed"); + + removeNiceScrollArtifacts(body); + + expect(group.classList.contains("bloom-vertical-align-bottom")).toBe( + true, + ); + expect( + group.classList.contains("bloom-vertical-align-bottom-removed"), + ).toBe(false); + }); + + it("removes the scrolling-bubble class added to a canvas element's editable", () => { + const editable = body.querySelector(".bloom-editable")!; + editable.classList.add("scrolling-bubble"); + + removeNiceScrollArtifacts(body); + + expect(editable.classList.contains("scrolling-bubble")).toBe(false); + }); + + it("clears the inline styles niceScroll leaves, and the empty style attribute with them", () => { + removeNiceScrollArtifacts(body); + + const editable = body.querySelector(".bloom-editable")!; + expect(editable.style.overflowY).toBe(""); + expect(editable.style.overflowX).toBe(""); + expect(editable.style.outline).toBe(""); + expect(editable.style.width).toBe(""); + expect(editable.hasAttribute("style")).toBe(false); + }); + + it("keeps other inline styles on a box niceScroll did touch", () => { + const editable = body.querySelector(".bloom-editable")!; + editable.style.color = "red"; + + removeNiceScrollArtifacts(body); + + expect(editable.style.color).toBe("red"); + expect(editable.style.overflowY).toBe(""); + }); + + it("leaves alone an inline width on a box niceScroll never touched", () => { + // No inline overflow-y, so this box was never given a niceScroll and its width is the + // author's, not niceScroll's Chrome workaround. + const editable = body.querySelector(".bloom-editable")!; + editable.setAttribute("style", "width: 200px"); + + removeNiceScrollArtifacts(body); + + expect(editable.style.width).toBe("200px"); + }); + + it("does nothing to a page that never had scroll bars", () => { + const untouched = document.createElement("div"); + untouched.innerHTML = ` +
+
+

Short.

+
+
`; + const before = untouched.innerHTML; + + removeNiceScrollArtifacts(untouched); + + expect(untouched.innerHTML).toBe(before); + }); +}); diff --git a/src/BloomBrowserUI/bookEdit/js/niceScrollCleanup.ts b/src/BloomBrowserUI/bookEdit/js/niceScrollCleanup.ts new file mode 100644 index 000000000000..c0df30f9d3e6 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/niceScrollCleanup.ts @@ -0,0 +1,104 @@ +import { kSelectorForPotentialNiceScrollElements } from "bloom-player"; + +// The classes niceScroll gives the elements it inserts. Each rail contains a cursor (its word for +// the thumb); we list both so a stray one can't survive. +const kNiceScrollInsertedElementSelector = + ".nicescroll-rails, .nicescroll-cursors"; + +// The alignment classes bloom-player's addScrollbarsToPage() takes off a translationGroup before +// applying niceScroll, leaving a "-removed" marker in their place so they can be restored. +const kVerticalAlignClassesRemovedForNiceScroll = [ + "bloom-vertical-align-center", + "bloom-vertical-align-bottom", +]; + +/** + * Undo, within 'root', everything that giving an overflowing text box a scroll bar did to the page, + * so that none of it gets saved into the book. + * + * The point of this existing at all — bloom-player already has cleanupNiceScroll() — is that this + * works on any root, including a DETACHED CLONE of the page. bloom-player's version can only work + * on the live page, because it does the job by asking each live niceScroll instance to remove + * itself. Doing that on every save meant tearing the scroll bars off the page the user was looking + * at and building them again (see getBodyContentForSavePage in bloomEditing.ts). + * + * There are three kinds of leftovers: + * + * 1. The elements niceScroll inserts: a .nicescroll-rails div (vertical, plus a horizontal one if + * needed), each containing a .nicescroll-cursors div. It appends them to the nearest positioned + * or scrollable ancestor and falls back to the body. Bloom pages do contain absolutely + * positioned ancestors (origami split-pane components, image-description groups), so they can + * land inside the .bloom-page div; when there is no such ancestor they go on the body instead. + * We are given the whole body, so we catch them either way. + * + * 2. Classes that addScrollbarsToPage() changed, because niceScroll does not work with the + * display:flex our vertical alignment implies: it moves bloom-vertical-align-center / + * bloom-vertical-align-bottom aside to a "-removed" marker on the translationGroup, and adds + * scrolling-bubble to a canvas element's editable. This is the part that matters most — + * saving a page in that state would silently lose the user's vertical alignment choice. + * + * 3. Inline styles niceScroll sets on the box it scrolls: overflow-x and overflow-y (hidden), + * outline (none, on webkit), and a pixel width (part of a Chrome scrollbar workaround, which it + * tries but does not always manage to undo — BL-14052). Those three are exactly what + * bloom-player's cleanup clears after asking niceScroll to remove itself, i.e. the ones + * niceScroll sets without recording so that it can restore them. + * (It can also set position:relative on the scrolled element, but only when it was created with + * a wrapper — the two-argument niceScroll() form — which bloom-player does not use, so that + * case cannot arise here.) + */ +export function removeNiceScrollArtifacts(root: HTMLElement): void { + for (const inserted of Array.from( + root.querySelectorAll(kNiceScrollInsertedElementSelector), + )) { + inserted.remove(); + } + + for (const alignClass of kVerticalAlignClassesRemovedForNiceScroll) { + const removedMarker = alignClass + "-removed"; + // getElementsByClassName is live, and we are about to remove the very class it selects on, + // so take a copy first. + for (const translationGroup of Array.from( + root.getElementsByClassName(removedMarker), + )) { + translationGroup.classList.remove(removedMarker); + translationGroup.classList.add(alignClass); + } + } + + for (const scrollingBubble of Array.from( + root.getElementsByClassName("scrolling-bubble"), + )) { + scrollingBubble.classList.remove("scrolling-bubble"); + } + + for (const scrollBox of Array.from( + root.querySelectorAll( + kSelectorForPotentialNiceScrollElements, + ), + )) { + // An inline overflow-y is niceScroll's fingerprint: it is the first thing it sets on a box + // it is going to scroll, and nothing in Bloom sets one. Checking for it means we can't + // blank an inline width that really was the author's on a box niceScroll never touched. + // (bloom-player's cleanup clears all three unconditionally; it can afford to, because it + // only reaches boxes that had a live niceScroll instance.) + if (!scrollBox.style.overflowY) { + continue; + } + // Naming the longhands explicitly rather than clearing the "overflow" shorthand: whether + // clearing a shorthand takes its longhands with it varies between CSSOM implementations + // (jsdom, where our tests run, does not do it). + for (const property of [ + "overflow", + "overflow-x", + "overflow-y", + "outline", + "width", + ]) { + scrollBox.style.removeProperty(property); + } + if (!scrollBox.getAttribute("style")) { + // Don't leave an empty style attribute behind in the saved HTML. + scrollBox.removeAttribute("style"); + } + } +} diff --git a/src/BloomBrowserUI/bookEdit/js/origami.ts b/src/BloomBrowserUI/bookEdit/js/origami.ts index 84708ef39f77..6dff927184e4 100644 --- a/src/BloomBrowserUI/bookEdit/js/origami.ts +++ b/src/BloomBrowserUI/bookEdit/js/origami.ts @@ -2,7 +2,8 @@ import { SetupImage } from "./bloomImages"; import { kBloomCanvasClass } from "../toolbox/canvas/canvasElementPageBridge"; import "../../lib/split-pane/split-pane.js"; import TextBoxProperties from "../TextBoxProperties/TextBoxProperties"; -import { post, postThatMightNavigate } from "../../utils/bloomApi"; +import { post } from "../../utils/bloomApi"; +import { saveChangesAndRethinkPage } from "./bloomEditing"; import { theOneCanvasElementManager } from "./canvasElementManager/CanvasElementManager"; import { getFeatureStatusAsync } from "../../react_components/featureStatus"; import $ from "jquery"; @@ -190,7 +191,7 @@ function changeLayoutModeToggleClickHandler() { const toggleTransitionLength = 450; setTimeout(() => { $("html").off("keydown.origami"); - postThatMightNavigate("common/saveChangesAndRethinkPageEvent"); + void saveChangesAndRethinkPage(); }, toggleTransitionLength); } } diff --git a/src/BloomBrowserUI/bookEdit/js/pageContentDelays.spec.ts b/src/BloomBrowserUI/bookEdit/js/pageContentDelays.spec.ts new file mode 100644 index 000000000000..e0502cd6ba7b --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/pageContentDelays.spec.ts @@ -0,0 +1,196 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { + addRequestPageContentDelay, + getActiveDelayIdsForTesting, + kMaxWaitTimeMs, + onDelayRegisterChanged, + removeRequestPageContentDelay, + whenNoActiveDelays, + wrapWithRequestPageContentDelay, +} from "./pageContentDelays"; + +// The gate that keeps a save from reading a page that is still being changed. Everything that +// gathers page content waits on whenNoActiveDelays(), so if this is wrong, half-finished work +// (an image still being sized, a paste still in progress) gets written into the user's book. + +// Has the promise settled? Attaches a callback and then lets the microtask queue drain, which is +// enough for a promise that is already resolved (or resolves synchronously from a call we just +// made) and not enough for one still waiting on a timer. +const isResolved = async (p: Promise): Promise => { + let resolved = false; + void p.then(() => { + resolved = true; + }); + for (let i = 0; i < 5; i++) await Promise.resolve(); + return resolved; +}; + +describe("pageContentDelays", () => { + beforeEach(() => { + vi.useFakeTimers(); + // Sanity check: nothing left over from another test, or the assertions below are meaningless. + expect(getActiveDelayIdsForTesting()).toEqual([]); + }); + + afterEach(() => { + vi.useRealTimers(); + if (getActiveDelayIdsForTesting().length) + throw new Error( + "test leaked delays: " + + getActiveDelayIdsForTesting().join(", "), + ); + }); + + it("resolves immediately when nothing is registered", async () => { + expect(await isResolved(whenNoActiveDelays())).toBe(true); + }); + + it("waits while work is registered, and resolves when the last of it finishes", async () => { + addRequestPageContentDelay("sizingAnImage"); + addRequestPageContentDelay("fittingACanvasElement"); + const gate = whenNoActiveDelays(); + + expect(await isResolved(gate)).toBe(false); + + removeRequestPageContentDelay("sizingAnImage"); + expect(await isResolved(gate)).toBe(false); // one still outstanding + + removeRequestPageContentDelay("fittingACanvasElement"); + expect(await isResolved(gate)).toBe(true); + }); + + it("counts repeats of the same id separately", async () => { + // The same operation can legitimately be in flight twice (two images sizing at once). + addRequestPageContentDelay("sizingAnImage"); + addRequestPageContentDelay("sizingAnImage"); + const gate = whenNoActiveDelays(); + + removeRequestPageContentDelay("sizingAnImage"); + expect(await isResolved(gate)).toBe(false); + + removeRequestPageContentDelay("sizingAnImage"); + expect(await isResolved(gate)).toBe(true); + }); + + it("releases every waiter, not just the first", async () => { + addRequestPageContentDelay("work"); + const first = whenNoActiveDelays(); + const second = whenNoActiveDelays(); + + removeRequestPageContentDelay("work"); + + expect(await isResolved(first)).toBe(true); + expect(await isResolved(second)).toBe(true); + }); + + it("gives up after the maximum wait rather than blocking the save forever", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + addRequestPageContentDelay("workThatNeverFinishes"); + const gate = whenNoActiveDelays(); + + await vi.advanceTimersByTimeAsync(kMaxWaitTimeMs - 1); + expect(await isResolved(gate)).toBe(false); + + await vi.advanceTimersByTimeAsync(2); + expect(await isResolved(gate)).toBe(true); + expect(warn).toHaveBeenCalled(); + expect(warn.mock.calls[0][0]).toContain("workThatNeverFinishes"); + + warn.mockRestore(); + removeRequestPageContentDelay("workThatNeverFinishes"); // tidy up for afterEach + }); + + it("does not fire the timeout warning for a wait that finished normally", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + addRequestPageContentDelay("work"); + const gate = whenNoActiveDelays(); + removeRequestPageContentDelay("work"); + await gate; + + // Well past the deadline: the timeout must have been cleared, not merely ignored. + await vi.advanceTimersByTimeAsync(kMaxWaitTimeMs * 2); + expect(warn).not.toHaveBeenCalled(); + warn.mockRestore(); + }); + + it("wrapWithRequestPageContentDelay holds the gate for the whole operation", async () => { + let releaseTheWork: (() => void) | undefined; + const work = new Promise((r) => (releaseTheWork = r)); + + const wrapped = wrapWithRequestPageContentDelay(() => work, "theWork"); + const gate = whenNoActiveDelays(); + expect(getActiveDelayIdsForTesting()).toEqual(["theWork"]); + expect(await isResolved(gate)).toBe(false); + + releaseTheWork!(); + await wrapped; + + expect(await isResolved(gate)).toBe(true); + expect(getActiveDelayIdsForTesting()).toEqual([]); + }); + + it("wrapWithRequestPageContentDelay releases the gate even when the work throws", async () => { + await expect( + wrapWithRequestPageContentDelay( + () => Promise.reject(new Error("the work failed")), + "theWork", + ), + ).rejects.toThrow("the work failed"); + + // The point: a failed operation must not block every save from now on. + expect(getActiveDelayIdsForTesting()).toEqual([]); + expect(await isResolved(whenNoActiveDelays())).toBe(true); + }); + + it("tells a listener when the register becomes busy, and when it empties, but not in between", () => { + // The page snapshot relays these to C#, which waits for the idle one before a save that + // uses the snapshot. Every add and remove would be noise; the transitions are the signal. + const heard: (string | undefined)[] = []; + const unsubscribe = onDelayRegisterChanged((busyWith) => + heard.push(busyWith), + ); + + addRequestPageContentDelay("sizing an image"); + addRequestPageContentDelay("settling a paste"); + expect(heard).toEqual(["sizing an image"]); + + removeRequestPageContentDelay("sizing an image"); + expect(heard, "still busy with the paste").toEqual(["sizing an image"]); + removeRequestPageContentDelay("settling a paste"); + expect(heard).toEqual(["sizing an image", undefined]); + + unsubscribe(); + addRequestPageContentDelay("later work"); + removeRequestPageContentDelay("later work"); + expect(heard, "nothing more after unsubscribing").toEqual([ + "sizing an image", + undefined, + ]); + }); + it("tells a listener that subscribes while the register is already busy", () => { + // The page snapshot subscribes after bootstrap(), when load-time work has usually already + // registered; a listener that heard only transitions would miss all of it. + addRequestPageContentDelay("sizing an image"); + const heard: (string | undefined)[] = []; + const unsubscribe = onDelayRegisterChanged((busyWith) => + heard.push(busyWith), + ); + + expect(heard).toEqual(["sizing an image"]); + + removeRequestPageContentDelay("sizing an image"); + expect(heard).toEqual(["sizing an image", undefined]); + unsubscribe(); + }); + it("complains about, and ignores, a removal of something never registered", () => { + const error = vi.spyOn(console, "error").mockImplementation(() => {}); + addRequestPageContentDelay("realWork"); + + removeRequestPageContentDelay("neverRegistered"); + + expect(error).toHaveBeenCalled(); + expect(getActiveDelayIdsForTesting()).toEqual(["realWork"]); + error.mockRestore(); + removeRequestPageContentDelay("realWork"); + }); +}); diff --git a/src/BloomBrowserUI/bookEdit/js/pageContentDelays.ts b/src/BloomBrowserUI/bookEdit/js/pageContentDelays.ts new file mode 100644 index 000000000000..c407a6cc48e1 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/pageContentDelays.ts @@ -0,0 +1,128 @@ +// The register of asynchronous work that must finish before the page can be saved, and the gate +// every page-content-gathering path waits on. +// +// The problem it solves: saving means reading the page's DOM, and quite a lot of the editor changes +// that DOM asynchronously -- sizing an image, fitting a canvas element's background, pasting from +// the clipboard, building a custom xmatter page. Read the page while one of those is half done and +// that is what gets written into the user's book. +// +// So any code doing such work registers here for its duration (preferably via +// wrapWithRequestPageContentDelay, which cannot forget to deregister), and every route that gathers +// page content goes through whenNoActiveDelays() first: +// - getPageContentForSaveWhenReady in bloomEditing.ts, which is how the page snapshot +// (pageSnapshot.ts) reads the page after every change, and how the page list's commands +// collect it to send along (collectCurrentPageContent). Javascript could in principle await +// its own work instead, but it cannot know about work someone else started, so it waits here. +// That also means the *command* does not begin -- C# is not asked to duplicate or delete a +// page until the page has settled. +// - the off-screen book processor (captureContentForExternalProcessing). + +// Upper bound (not a fixed wait) on how long we wait for in-flight async DOM work to finish before +// gathering anyway. The wait ends as soon as the register empties, so simple pages are unaffected +// by this value; it only gives slower computers with complex pages more headroom before we give up. +export const kMaxWaitTimeMs = 4000; + +const activeDelays: string[] = []; + +// Callbacks waiting for activeDelays to empty; see whenNoActiveDelays(). +const delayWaiters: (() => void)[] = []; + +// Told when the register goes from empty to busy (with what it is busy with) and back to empty +// (with undefined). See onDelayRegisterChanged. +const registerListeners: ((busyWith: string | undefined) => void)[] = []; + +// Be told when the register becomes busy -- with the id of the work, or the ids if several +// started together -- and when it empties again. Only the transitions, not every add and remove. +// The page snapshot uses this to tell C# that a snapshot-based save should wait, and what for. +// Returns a function that unsubscribes. +// +// If the register is already busy when the listener subscribes, it is told so at once: the page +// snapshot subscribes after bootstrap(), by which time the load-time work (image sizing, CKEditor +// attaching) has usually registered, and a listener that only heard about transitions would miss +// all of it. +export function onDelayRegisterChanged( + listener: (busyWith: string | undefined) => void, +): () => void { + registerListeners.push(listener); + if (activeDelays.length > 0) listener(activeDelays.join(", ")); + return () => { + const index = registerListeners.indexOf(listener); + if (index >= 0) registerListeners.splice(index, 1); + }; +} + +// Register asynchronous work whose results belong in the saved page. The caller must pass the same +// id to removeRequestPageContentDelay when the work finishes -- see wrapWithRequestPageContentDelay, +// which does that for you. IDs do not need to be unique; the same ID can be added multiple times. +export function addRequestPageContentDelay(id: string): void { + activeDelays.push(id); + if (activeDelays.length === 1) { + registerListeners.forEach((listener) => listener(id)); + } +} + +// Deregister work, releasing anyone waiting if this was the last of it. +export function removeRequestPageContentDelay(id: string): void { + const index = activeDelays.indexOf(id); + if (index === -1) { + console.error( + `removeRequestPageContentDelay: ID "${id}" not found in active delays. Active delays: [${activeDelays.join( + ", ", + )}]`, + ); + return; + } + activeDelays.splice(index, 1); + + if (activeDelays.length === 0) { + // Take the list before calling anyone, so that a waiter which starts new work (and so + // registers a new delay) does not get released a second time by that work finishing. + delayWaiters.splice(0).forEach((release) => release()); + registerListeners.forEach((listener) => listener(undefined)); + } +} + +// Run some asynchronous work with its delay registered for the duration, whether it succeeds or +// throws. Prefer this to the add/remove pair: a delay that is never removed blocks every save for +// kMaxWaitTimeMs and then gets overridden anyway. +export async function wrapWithRequestPageContentDelay( + fn: () => Promise, + delayId: string, +): Promise { + addRequestPageContentDelay(delayId); + try { + return await fn(); + } finally { + removeRequestPageContentDelay(delayId); + } +} + +// Resolves once no registered work is outstanding: immediately if there is none, otherwise as soon +// as the last of it finishes, and after kMaxWaitTimeMs regardless -- saving a slightly stale page +// beats not saving at all, so we warn and go on rather than block the user forever. +export function whenNoActiveDelays(): Promise { + if (activeDelays.length === 0) return Promise.resolve(); + return new Promise((resolve) => { + let timeout: number | undefined; + const release = () => { + if (timeout !== undefined) window.clearTimeout(timeout); + resolve(); + }; + delayWaiters.push(release); + timeout = window.setTimeout(() => { + console.warn( + `Waited the maximum ${kMaxWaitTimeMs}ms for in-flight page changes [${activeDelays.join( + ", ", + )}]. Gathering the page content anyway.`, + ); + const index = delayWaiters.indexOf(release); + if (index >= 0) delayWaiters.splice(index, 1); + resolve(); + }, kMaxWaitTimeMs); + }); +} + +// For tests and diagnostics only: what is currently registered. +export function getActiveDelayIdsForTesting(): string[] { + return [...activeDelays]; +} diff --git a/src/BloomBrowserUI/bookEdit/js/pageSnapshot.spec.ts b/src/BloomBrowserUI/bookEdit/js/pageSnapshot.spec.ts new file mode 100644 index 000000000000..f149f632e40d --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/pageSnapshot.spec.ts @@ -0,0 +1,716 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { + notePageContentMayHaveChanged, + startWatchingPageForSnapshots, + stopWatchingPageForSnapshots, + quietMsForTests, + retryMsForTests, + getPageLoadId, +} from "./pageSnapshot"; +import { + addRequestPageContentDelay, + removeRequestPageContentDelay, +} from "./pageContentDelays"; + +const posted: Array<{ url: string; body: string }> = []; + +// Lets a test hold a POST open, to check that a second one never starts alongside it. +let postHook: (() => Promise) | undefined; + +// What C# answers. A real post resolves to the axios response, and this endpoint answers with a +// boolean, so `{ data: true }` is an ordinary success. `{ data: false }` is a refusal: the snapshot +// was for a page load it is not showing. And `undefined` -- no response at all -- is what a FAILED +// post looks like, because postStringQuietly goes through wrapAxios, which turns a rejected request into a +// resolved promise carrying nothing. +let postReply: unknown = { data: true }; + +const reported: string[] = []; +vi.mock("../../lib/errorHandler", () => ({ + reportError: (message: string) => reported.push(message), +})); + +vi.mock("../../utils/bloomApi", () => ({ + postStringQuietly: (url: string, body: string) => { + posted.push({ url, body }); + return postHook ? postHook() : Promise.resolve(postReply); + }, +})); + +// The page as the gather would report it. Tests change this to simulate the user editing. +let contentToReport = ""; +const gather = () => Promise.resolve(contentToReport); + +function setUpPage(pageId = "page-1") { + document.body.innerHTML = `

hello

`; +} + +function changeThePage(text: string) { + document.querySelector(".bloom-page p")!.textContent = text; +} + +// startWatching... reads the page once to learn what "unchanged" looks like after loading has +// finished. Nothing is posted until that has resolved. +async function letTheBaselineSettle() { + await vi.runAllTicks(); + await Promise.resolve(); +} + +// Walks the slower timer used to offer content again when C# did not take it. +async function letTheRetryHappen() { + vi.advanceTimersByTime(retryMsForTests); + await vi.runAllTicks(); + await Promise.resolve(); // the gather's await + await Promise.resolve(); // the post's await +} + +// A MutationObserver delivers its callback in a microtask, and the module then waits kQuietMs. +// This walks both forward. +async function letTheSnapshotHappen() { + await Promise.resolve(); // let the observer fire + vi.advanceTimersByTime(quietMsForTests); + await vi.runAllTicks(); + await Promise.resolve(); // the gather's await + await Promise.resolve(); // the post's await +} + +describe("pageSnapshot", () => { + beforeEach(() => { + vi.useFakeTimers(); + posted.length = 0; + reported.length = 0; + contentToReport = ""; + postHook = undefined; + postReply = { data: true }; + setUpPage(); + }); + + afterEach(() => { + stopWatchingPageForSnapshots(); + vi.useRealTimers(); + document.body.innerHTML = ""; + }); + + it("posts nothing for a page the user never changes", async () => { + contentToReport = "the untouched page"; + startWatchingPageForSnapshots(gather); + await letTheBaselineSettle(); + + vi.advanceTimersByTime(quietMsForTests * 5); + await vi.runAllTicks(); + + expect( + posted.length, + "a page nobody edited must produce no snapshot, so that C# can tell 'nothing to save' from 'not asked yet'", + ).toBe(0); + }); + + it("does not treat the page finishing loading as an edit", async () => { + // Loading is not over when we start watching: image sizing and canvas-element layout + // complete afterwards and mutate the page. The observer cannot tell those from the user, + // so the baseline has to. Without it the real app posted a snapshot for every page opened, + // which would have made "no snapshot" meaningless on the C# side. + contentToReport = "the settled page"; + startWatchingPageForSnapshots(gather); + + changeThePage("a late load-time fix-up"); + await letTheBaselineSettle(); + changeThePage("and another"); + await letTheSnapshotHappen(); + + expect( + posted.length, + "mutations that do not change the page's saved form are not edits", + ).toBe(0); + }); + + it("posts the content, with the page id, once the page has been changed and settles", async () => { + startWatchingPageForSnapshots(gather); + await letTheBaselineSettle(); + contentToReport = "edited content"; + changeThePage("goodbye"); + await letTheSnapshotHappen(); + + expect(posted.length).toBe(1); + expect(posted[0].body).toBe("edited content"); + expect(posted[0].url).toContain("editView/pageSnapshot"); + expect(posted[0].url).toContain("pageId=page-1"); + }); + + it("does not post again when the content has not actually changed", async () => { + startWatchingPageForSnapshots(gather); + await letTheBaselineSettle(); + contentToReport = "same every time"; + changeThePage("a"); + await letTheSnapshotHappen(); + expect(posted.length, "sanity: the first change posts").toBe(1); + + // Tools constantly add and remove editing decorations, which the gather strips. Those + // mutations must not produce a stream of identical posts. + changeThePage("b"); + await letTheSnapshotHappen(); + + expect(posted.length).toBe(1); + }); + + it("stops posting once the page is unloaded", async () => { + startWatchingPageForSnapshots(gather); + await letTheBaselineSettle(); + contentToReport = "first"; + changeThePage("a"); + await letTheSnapshotHappen(); + expect(posted.length, "sanity: it was posting before we stopped").toBe( + 1, + ); + + stopWatchingPageForSnapshots(); + contentToReport = "second"; + changeThePage("b"); + await letTheSnapshotHappen(); + + expect(posted.length).toBe(1); + }); + + it("waits for the page to be quiet rather than posting per change", async () => { + startWatchingPageForSnapshots(gather); + await letTheBaselineSettle(); + contentToReport = "typed a word"; + + // Three changes in quick succession, as typing produces. + changeThePage("a"); + await Promise.resolve(); + vi.advanceTimersByTime(quietMsForTests / 4); + changeThePage("ab"); + await Promise.resolve(); + vi.advanceTimersByTime(quietMsForTests / 4); + changeThePage("abc"); + await letTheSnapshotHappen(); + + expect( + posted.length, + "the debounce should collapse a burst of changes into one snapshot", + ).toBe(1); + expect(posted[0].body).toBe("typed a word"); + }); + + it("never has two posts in flight at once", async () => { + // HTTP does not promise that two outstanding POSTs arrive in the order they were sent, so + // an older snapshot could land after a newer one and C# would keep the older content. That + // needs a machine slow enough for a post to still be in flight when the next keystroke's + // snapshot comes round -- so it must be enforced, not left to timing. + let inFlight = 0; + let maxInFlight = 0; + let releasePost: () => void = () => {}; + postHook = () => + new Promise((resolve) => { + inFlight++; + maxInFlight = Math.max(maxInFlight, inFlight); + releasePost = () => { + inFlight--; + resolve({ data: true }); // an ordinary successful post + }; + }); + + startWatchingPageForSnapshots(gather); + await letTheBaselineSettle(); + + contentToReport = "first"; + changeThePage("a"); + await Promise.resolve(); + vi.advanceTimersByTime(quietMsForTests); + await vi.runAllTicks(); + await Promise.resolve(); + expect(inFlight, "sanity: a post is outstanding").toBe(1); + + // More edits arrive while that post is still outstanding. + contentToReport = "second"; + changeThePage("b"); + await Promise.resolve(); + vi.advanceTimersByTime(quietMsForTests * 3); + await vi.runAllTicks(); + await Promise.resolve(); + + expect( + maxInFlight, + "a second post must not start while one is outstanding", + ).toBe(1); + + // Once it completes, the newer content still gets sent. + releasePost(); + await vi.runAllTicks(); + await Promise.resolve(); + vi.advanceTimersByTime(quietMsForTests); + await vi.runAllTicks(); + await Promise.resolve(); + releasePost(); + await vi.runAllTicks(); + await Promise.resolve(); + + expect( + posted.map((p) => p.body), + "the later edit must still reach C#, just after the first post finished", + ).toEqual(["first", "second"]); + }); + + it("takes another snapshot when the page changes while one is being gathered", async () => { + let release: (value: string) => void = () => {}; + let gatherCount = 0; + const slowGather = () => { + gatherCount++; + return new Promise((resolve) => { + release = resolve; + }); + }; + startWatchingPageForSnapshots(slowGather); + + // The first gather is the baseline; let it finish. + expect(gatherCount, "sanity: the baseline gather started").toBe(1); + release("baseline"); + await letTheBaselineSettle(); + + changeThePage("a"); + await Promise.resolve(); + vi.advanceTimersByTime(quietMsForTests); + await vi.runAllTicks(); + expect(gatherCount, "sanity: a snapshot gather started").toBe(2); + + // While that gather is outstanding, the user types again. That change is not in what the + // gather is about to hand us, so it must not be silently dropped. + changeThePage("b"); + await Promise.resolve(); + + release("first content"); + await vi.runAllTicks(); + await Promise.resolve(); + await Promise.resolve(); + expect(posted.length, "sanity: the first gather posted").toBe(1); + + vi.advanceTimersByTime(quietMsForTests); + await vi.runAllTicks(); + expect( + gatherCount, + "the change that landed mid-gather must trigger another snapshot, not be dropped", + ).toBe(3); + }); + it("does not count an edit made while the baseline was being read as already sent", async () => { + // The baseline gather waits for the page's load-time work to finish, and the user can start + // typing before it does. The baseline then already contains that typing. Treating it as + // "already sent" would swallow the edit: nothing would ever post it, and quitting would + // write what C# holds. + let release: (value: string) => void = () => {}; + const slowGather = () => + new Promise((resolve) => { + release = resolve; + }); + startWatchingPageForSnapshots(slowGather); + + changeThePage("typed while the page was still loading"); + await Promise.resolve(); // let the observer see it + release("hello, typed while the page was still loading"); + await letTheBaselineSettle(); + + // The follow-up gather reports the same content the baseline did. + vi.advanceTimersByTime(quietMsForTests); + await vi.runAllTicks(); + release("hello, typed while the page was still loading"); + await vi.runAllTicks(); + await Promise.resolve(); + await Promise.resolve(); + + expect( + posted.length, + "the edit that landed during the baseline must be posted", + ).toBe(1); + expect(posted[0].body).toBe( + "hello, typed while the page was still loading", + ); + }); + it("tells C# what the page is busy with, and says idle only after posting the finished page", async () => { + // A save C# makes from the snapshot (leaving the tab, quitting) cannot wait for the delay + // register the way a gather here does, so it waits for our idle notice instead. That notice + // must therefore come AFTER the snapshot of the finished page, or C# would save the old one. + startWatchingPageForSnapshots(gather); + await letTheBaselineSettle(); + posted.length = 0; + + addRequestPageContentDelay("sizing an image"); + await Promise.resolve(); + expect(posted.map((p) => p.url.split("?")[0])).toEqual([ + "editView/pageBusy", + ]); + expect(posted[0].body).toBe("sizing an image"); + expect(posted[0].url).toContain("loadId=" + getPageLoadId()); + + // The work changes the page, then finishes. + contentToReport = "with the image sized"; + changeThePage("with the image sized"); + await Promise.resolve(); + removeRequestPageContentDelay("sizing an image"); + await vi.runAllTicks(); + await Promise.resolve(); // the gather + await Promise.resolve(); // the snapshot post + await Promise.resolve(); // the idle post + + expect(posted.map((p) => p.url.split("?")[0])).toEqual([ + "editView/pageBusy", + "editView/pageSnapshot", + "editView/pageIdle", + ]); + expect(posted[1].body).toBe("with the image sized"); + }); + + it("says idle only after the page as it is AFTER the work has been posted, even if a post was already in flight", async () => { + // The gather may have read the page before the work began and be sitting in its post when + // the work finishes; what it sends predates the work. Idle must still wait for a snapshot + // taken afterwards, or C# saves the old page. + let release: (value: string) => void = () => {}; + const slowGather = () => + new Promise((resolve) => { + release = resolve; + }); + startWatchingPageForSnapshots(slowGather); + release("baseline"); + await letTheBaselineSettle(); + posted.length = 0; + + // The user types; a snapshot run starts and is now reading the page. + changeThePage("before the work"); + await Promise.resolve(); + vi.advanceTimersByTime(quietMsForTests); + await vi.runAllTicks(); + + // Meanwhile some work registers, changes the page, and finishes -- all while that run is + // still out. + addRequestPageContentDelay("sizing an image"); + changeThePage("after the work"); + await Promise.resolve(); + removeRequestPageContentDelay("sizing an image"); + await Promise.resolve(); + + // The run comes back with what it read before the work. + release("before the work"); + await vi.runAllTicks(); + for (let i = 0; i < 4; i++) await Promise.resolve(); + // The gather taken because of the idle notice reports the finished page. + release("after the work"); + await vi.runAllTicks(); + for (let i = 0; i < 6; i++) await Promise.resolve(); + + const urls = posted.map((p) => p.url.split("?")[0]); + expect(urls).toEqual([ + "editView/pageBusy", + "editView/pageSnapshot", + "editView/pageSnapshot", + "editView/pageIdle", + ]); + expect(posted[1].body).toBe("before the work"); + expect( + posted[2].body, + "the page as it is after the work must have been sent before idle", + ).toBe("after the work"); + }); + + it("reports work that was already registered when watching began", async () => { + addRequestPageContentDelay("sizing an image"); + startWatchingPageForSnapshots(gather); + await Promise.resolve(); + + expect(posted.map((p) => p.url.split("?")[0])).toEqual([ + "editView/pageBusy", + ]); + expect(posted[0].body).toBe("sizing an image"); + + postReply = { data: true }; + removeRequestPageContentDelay("sizing an image"); + await letTheBaselineSettle(); + for (let i = 0; i < 6; i++) await Promise.resolve(); + }); + + it("offers the busy notice again when the post fails outright", async () => { + // A failed post looks like a successful one apart from the missing reply (wrapAxios + // swallows the rejection). C# has not heard, so a save it makes meanwhile would not wait. + startWatchingPageForSnapshots(gather); + await letTheBaselineSettle(); + posted.length = 0; + postReply = undefined; + + addRequestPageContentDelay("settling a paste"); + await Promise.resolve(); + await Promise.resolve(); + expect(posted.length, "sanity: the first notice went out").toBe(1); + + vi.advanceTimersByTime(retryMsForTests); + await Promise.resolve(); + expect(posted.length, "not taken, so offered again").toBe(2); + + postReply = { data: true }; + removeRequestPageContentDelay("settling a paste"); + await vi.runAllTicks(); + for (let i = 0; i < 6; i++) await Promise.resolve(); + }); + + it("offers the idle notice again when it is not taken, so C# is not left believing the page busy", async () => { + startWatchingPageForSnapshots(gather); + await letTheBaselineSettle(); + addRequestPageContentDelay("sizing an image"); + await Promise.resolve(); + posted.length = 0; + + postReply = undefined; // the server drops the idle post + removeRequestPageContentDelay("sizing an image"); + await vi.runAllTicks(); + for (let i = 0; i < 6; i++) await Promise.resolve(); + expect( + posted.map((p) => p.url.split("?")[0]), + "sanity: the idle notice went out once", + ).toEqual(["editView/pageIdle"]); + + postReply = { data: true }; + vi.advanceTimersByTime(retryMsForTests); + for (let i = 0; i < 4; i++) await Promise.resolve(); + expect(posted.map((p) => p.url.split("?")[0])).toEqual([ + "editView/pageIdle", + "editView/pageIdle", + ]); + }); + + it("numbers the busy and idle notices in the order they are sent", async () => { + // They travel as separate requests, and C# uses the numbers to ignore one that arrives + // after a later one -- an idle notice landing after the busy notice for newer work. + startWatchingPageForSnapshots(gather); + await letTheBaselineSettle(); + posted.length = 0; + + addRequestPageContentDelay("sizing an image"); + await Promise.resolve(); + removeRequestPageContentDelay("sizing an image"); + await vi.runAllTicks(); + for (let i = 0; i < 6; i++) await Promise.resolve(); + + const notices = posted.filter( + (p) => !p.url.startsWith("editView/pageSnapshot"), + ); + expect(notices.map((p) => p.url.split("?")[0])).toEqual([ + "editView/pageBusy", + "editView/pageIdle", + ]); + const seqOf = (url: string) => Number(/[?&]seq=(\d+)/.exec(url)![1]); + expect(seqOf(notices[1].url)).toBeGreaterThan(seqOf(notices[0].url)); + }); + + it("offers the busy notice again when C# refuses it, while the work is still going", async () => { + // C# refuses notices about a load it is not yet showing, exactly as it refuses snapshots, + // and this page may simply not have reported itself ready yet. + startWatchingPageForSnapshots(gather); + await letTheBaselineSettle(); + posted.length = 0; + postReply = { data: false }; + + addRequestPageContentDelay("settling a paste"); + await Promise.resolve(); + await Promise.resolve(); + expect(posted.length, "sanity: the first notice went out").toBe(1); + + vi.advanceTimersByTime(retryMsForTests); + await Promise.resolve(); + expect(posted.length, "refused, so offered again").toBe(2); + expect(posted[1].url.split("?")[0]).toBe("editView/pageBusy"); + + postReply = { data: true }; + removeRequestPageContentDelay("settling a paste"); + await vi.runAllTicks(); + for (let i = 0; i < 4; i++) await Promise.resolve(); + }); + + it("does not treat a failed post as sent, so the content is offered again", async () => { + // Recording it as sent before the post resolved would mean C# never got this content and + // we never tried again -- the next save would then write what C# still held, losing + // everything typed since. + contentToReport = "first"; + startWatchingPageForSnapshots(gather); + await letTheBaselineSettle(); + + postHook = () => Promise.reject(new Error("network gone")); + contentToReport = "second"; + changeThePage("second"); + await letTheSnapshotHappen(); + expect(posted.map((p) => p.body)).toEqual(["second"]); + + // The post failed, so the same content must still be offered on the next attempt. + postHook = undefined; + changeThePage("second again"); + await letTheSnapshotHappen(); + expect(posted.map((p) => p.body)).toEqual(["second", "second"]); + }); + + it("reports a gather that throws, once per page, instead of losing the edits silently", async () => { + // If the gather throws and nobody says so, C# concludes there is nothing to save and the + // user's work disappears without a word. + contentToReport = "fine"; + startWatchingPageForSnapshots(gather); + await letTheBaselineSettle(); + + const exploding = () => Promise.reject(new Error("no marginBox")); + stopWatchingPageForSnapshots(); + startWatchingPageForSnapshots(exploding); + await letTheBaselineSettle(); + + changeThePage("one"); + await letTheSnapshotHappen(); + changeThePage("two"); + await letTheSnapshotHappen(); + + expect(reported.length).toBe(1); + expect(reported[0]).toContain("could not keep track of your changes"); + expect(posted.length).toBe(0); + }); + it("stamps every post with the id of this page load", async () => { + // C# refuses a snapshot that does not carry the load it is currently showing, so that a + // post overtaking a reload of the same page cannot be merged over what the reload built. + contentToReport = "before"; + startWatchingPageForSnapshots(gather); + await letTheBaselineSettle(); + + contentToReport = "after"; + changeThePage("after"); + await letTheSnapshotHappen(); + + expect(posted.length).toBe(1); + expect(posted[0].url).toContain( + "loadId=" + encodeURIComponent(getPageLoadId()), + ); + expect(getPageLoadId()).not.toBe(""); + }); + + it("posts when told the content changed in a way it cannot observe", async () => { + // The user's style definitions are gathered too, but they are changed through the CSSOM -- + // setProperty, deleteRule, insertRule -- which mutates no DOM node, so a MutationObserver + // cannot see it. Changing a style's size or colour without touching the text would + // otherwise produce no snapshot at all, and leaving the tab would write the old styles. + contentToReport = "first"; + startWatchingPageForSnapshots(gather); + await letTheBaselineSettle(); + expect(posted.length, "sanity: nothing posted yet").toBe(0); + + // The style editor changed a rule. Nothing in the page changed. + contentToReport = "first, but with bigger type"; + notePageContentMayHaveChanged(); + vi.advanceTimersByTime(quietMsForTests); + await vi.runAllTicks(); + await Promise.resolve(); + await Promise.resolve(); + + expect(posted.map((p) => p.body)).toEqual([ + "first, but with bigger type", + ]); + }); + + it("posts nothing when told of a change that turns out not to be one", async () => { + // Callers are told to err towards saying so, which is only safe because an unchanged page + // costs nothing. + contentToReport = "first"; + startWatchingPageForSnapshots(gather); + await letTheBaselineSettle(); + + notePageContentMayHaveChanged(); + vi.advanceTimersByTime(quietMsForTests); + await vi.runAllTicks(); + await Promise.resolve(); + await Promise.resolve(); + + expect(posted.length).toBe(0); + }); + + it("offers the content again when C# refuses the snapshot", async () => { + // C# refuses anything from a page load it is not showing. Because the snapshot endpoint is + // not ordered against the "page is ready" one, a snapshot can genuinely arrive first and be + // refused; counting it as delivered would leave C# with nothing to save. + contentToReport = "first"; + startWatchingPageForSnapshots(gather); + await letTheBaselineSettle(); + + postReply = { data: false }; // refused + contentToReport = "typed"; + changeThePage("typed"); + await letTheSnapshotHappen(); + expect(posted.map((p) => p.body)).toEqual(["typed"]); + + // Refused, so the very same content must be offered again rather than treated as sent. + // Nothing the user did caused the refusal, so the retry is on the slower timer. + postReply = { data: true }; + await letTheRetryHappen(); + expect(posted.map((p) => p.body)).toEqual(["typed", "typed"]); + }); + + it("does not treat a post that failed outright as sent", async () => { + // The realistic shape of a failed post, and the one that nearly slipped through: the post + // goes through wrapAxios, which swallows the rejection and resolves with NOTHING. So a + // failed post is indistinguishable from a successful one except that no response comes + // back -- and reading only `.data` took that for an acceptance. The content was then + // recorded as sent and never offered again, and the next save wrote what C# still held. + contentToReport = "first"; + startWatchingPageForSnapshots(gather); + await letTheBaselineSettle(); + + postReply = undefined; // the post failed; wrapAxios gives us nothing + contentToReport = "typed"; + changeThePage("typed"); + await letTheSnapshotHappen(); + expect(posted.map((p) => p.body)).toEqual(["typed"]); + + postReply = { data: true }; + await letTheRetryHappen(); + expect( + posted.map((p) => p.body), + "content C# never received must be offered again, not counted as sent", + ).toEqual(["typed", "typed"]); + }); + + it("keeps offering a failing post, backing off, and tells the user only once", async () => { + // Two things have to be true at the same time here, and they pull against each other. + // + // We must not stop retrying: while C# has not got this content, quitting writes what it + // still holds, so a server that comes back must be given the content even if the user + // never types again. But we must also not report the failure on every attempt, or an + // outage puts an error in front of the user again and again. Hence a quiet post and one + // report per page. + contentToReport = "first"; + startWatchingPageForSnapshots(gather); + await letTheBaselineSettle(); + + postReply = undefined; // every post fails + contentToReport = "typed"; + changeThePage("typed"); + await letTheSnapshotHappen(); + expect(posted.length, "sanity: the first attempt happened").toBe(1); + + // A minute of outage, walked in 10s steps. + for (let i = 0; i < 6; i++) { + vi.advanceTimersByTime(10000); + await vi.runAllTicks(); + await Promise.resolve(); + await Promise.resolve(); + } + expect( + posted.length, + "it must keep offering rather than give up", + ).toBeGreaterThan(1); + expect( + posted.length, + "backing off: a minute of outage must not mean a minute of attempts", + ).toBeLessThan(12); + expect( + reported.length, + "the user must be told once, not once per attempt", + ).toBe(1); + + // When the server comes back, the content gets there with no further typing. + postReply = { data: true }; + const attemptsWhileDown = posted.length; + vi.advanceTimersByTime(60000); + await vi.runAllTicks(); + await Promise.resolve(); + await Promise.resolve(); + expect(posted.length).toBe(attemptsWhileDown + 1); + expect(posted[posted.length - 1].body).toBe("typed"); + }); +}); diff --git a/src/BloomBrowserUI/bookEdit/js/pageSnapshot.ts b/src/BloomBrowserUI/bookEdit/js/pageSnapshot.ts new file mode 100644 index 000000000000..fa9247c3c273 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/pageSnapshot.ts @@ -0,0 +1,482 @@ +import { postStringQuietly } from "../../utils/bloomApi"; +import { reportError } from "../../lib/errorHandler"; +import { onDelayRegisterChanged } from "./pageContentDelays"; + +// Keep C# supplied with the current content of the page being edited, so that a save never has to +// ask for it and wait. +// +// The old arrangement was a round trip: C# wanted the page, told the browser to send it, and then +// had to have somewhere to wait until the answer arrived on a separate API call. That wait is what +// the editing state machine's SavePending state exists for, and it is why everything that has to +// save first -- leaving the Edit tab, closing the collection, a page-list command -- had to be +// split into a "before" and an "after" around an asynchronous gap. +// +// Since BL-13502 gathering the page is cheap (~0.7 ms) and does not touch the live page at all, so +// the browser can simply volunteer it: after any change that settles, post the current content. +// C# stores the string (see PageSnapshot.cs) and a save then takes it synchronously. +// +// What makes this safe to rely on is that we post only when the page's SAVED FORM has actually +// changed, so "no snapshot" on the C# side means "no unsaved changes" rather than "we have not +// been told yet". Two things are needed for that, and neither is optional: +// +// * A baseline taken once the page has finished loading. Loading is not over when bootstrap() +// returns -- image sizing and canvas layout finish afterwards and mutate the page -- so without +// one, every page posts a snapshot seconds after opening even if nobody touches it. +// * Comparing each gather against the last thing we sent. Tools constantly add and remove editing +// decorations, which the gather strips anyway, so without this they produce a stream of +// identical posts. + +const kApi = "editView/pageSnapshot"; +// Where we tell C# that asynchronous work belonging in the saved page has begun (the body names +// it) and that it has finished. See tellCSharpBusy / tellCSharpIdle. +const kBusyApi = "editView/pageBusy"; +const kIdleApi = "editView/pageIdle"; + +// Identifies THIS load of THIS page, so C# can tell our snapshots from those of a load it has +// already moved on from. A module-level constant is exactly the right scope: the page frame gets a +// fresh document, and so a fresh module, on every page load. +// +// It exists because the snapshot endpoint is deliberately unsynchronised (a keystroke has no +// business queueing behind a save), so a post sent moments before a navigation can be processed +// after C# has cleared the snapshot for it. Moving to a DIFFERENT page was harmless -- the stale +// entry is filed under a page id nobody asks about again -- but reloading the SAME page is not: +// Change Layout, importing a video and changing the topic all rebuild the page under its own id, +// and a snapshot of the pre-reload page would then be merged over what the reload built. +const pageLoadId = + Date.now().toString(36) + "-" + Math.random().toString(36).slice(2, 10); + +/** + * Identifies this load of this page. Sent with the "page is ready" notification and with every + * snapshot, so C# can ignore anything from a load it has superseded. + */ +export function getPageLoadId(): string { + return pageLoadId; +} + +// How long the page must be quiet before we take a snapshot. +// +// This is small on purpose, and the size of it decides how much typing an exit could lose. What +// it has to buy is coalescing: ONE keystroke produces about nine MutationObserver batches, because +// CKEditor does a lot of DOM work per key. 25 ms collapses those into a single gather, and no lower +// value would buy anything more -- below about 25 ms the lag is dominated by the POST, not by us. +// +// The cost of being this eager is one POST per keystroke instead of one per pause, and one extra +// snapshot per page visit (a short debounce catches the page mid-settle as well as settled). Both +// are cheap: the gather takes well under a millisecond, the POST goes to localhost and C# only +// stores the string, replacing the last one. Measurements are in Edit/SavingWithoutReloading.md. +const kQuietMs = 25; + +// How long to wait before offering the content again when C# did not take it. Longer than the +// debounce on purpose: nothing the user did causes these, so there is nothing to be responsive to, +// and a 25ms retry against a server that is not answering would be a busy loop. A real change +// reschedules at kQuietMs and so overtakes this. +const kRetryAfterRefusalMs = 1000; + +// A run of failures backs off from kRetryAfterRefusalMs up to this. We never give up: the browser +// holding content C# has not got is exactly the state that loses the user's work at exit, so it +// has to keep offering until something takes it. What made giving up look attractive was the +// noise, and that is dealt with separately -- the post is made quietly and we report once per +// page, rather than once per attempt. +const kMaxRetryMs = 30000; + +let observer: MutationObserver | undefined; +let timer: number | undefined; +let lastPosted: string | undefined; +let pageIdBeingWatched: string | undefined; +// How we read the page. Passed in by the caller rather than imported, so this module does not +// depend on bloomEditing (which depends on it, for the teardown) -- and so a test can drive it +// without a real page. +let gatherPageContent: (() => Promise) | undefined; +// Until the post-load baseline is in, we do not know which of the mutations we are seeing are the +// page finishing loading and which are the user, so we hold off posting. See startWatching... +let baselineTaken = false; +// True while a gather-and-post is under way. See takeSnapshot: overlapping posts could arrive out +// of order, which would let an older snapshot overwrite a newer one on the C# side. +let busy = false; +// Settles when the gather-and-post under way finishes; see tellCSharpIdle, which has to let the +// post go out before it says the page is idle. +let runDone: Promise = Promise.resolve(); +// What the delay register says the page is busy with, or undefined when it is empty. Kept so that +// a refused busy notice is offered again only while it is still true. +let busyWith: string | undefined; +let unsubscribeFromDelayRegister: (() => void) | undefined; +// Numbers the busy and idle notices, so that C# can ignore one that arrives after a later one. +// The two are separate HTTP requests and HTTP does not promise to deliver them in order, so an +// idle notice (or its retry) can land after the busy notice for work that began afterwards; taken +// at face value, that would let a save go ahead in the middle of the work. +let noticeSequence = 0; +// Bumped every time a change arrives. The async gather checks it afterwards, so a change that +// lands while we were gathering schedules another pass instead of being lost. +let changeCount = 0; +// The page we have already complained about, so that a page which fails every time reports once +// rather than on every keystroke. +let pageWeReportedAFailureFor: string | undefined; +// How many posts in a row have failed outright. Governs the backoff. +let consecutiveFailedPosts = 0; +// Tell the user, at most once for this page. Reporting is the whole reason a snapshot post is +// made quietly (see postStringQuietly): so that WE decide when to speak, rather than the request +// layer speaking on every attempt. +function reportFailureOncePerPage( + pageId: string, + message: string, + stack: string | undefined, +): void { + if (pageWeReportedAFailureFor === pageId) return; + pageWeReportedAFailureFor = pageId; + reportError(message, stack); +} + +// Offer the content again after a failure, backing off 1s, 2s, 4s... to kMaxRetryMs and then +// staying there. Never gives up: while C# has not got this content, quitting writes what it still +// holds. A change the user makes reschedules at kQuietMs and overtakes this. +function retryAfterFailure(): void { + consecutiveFailedPosts++; + scheduleSnapshot( + Math.min( + kRetryAfterRefusalMs * Math.pow(2, consecutiveFailedPosts - 1), + kMaxRetryMs, + ), + ); +} + +function currentPageId(): string | undefined { + return document.querySelector(".bloom-page")?.id || undefined; +} + +async function takeSnapshot(): Promise { + const pageId = pageIdBeingWatched; + if (!pageId || !gatherPageContent) return; + if (!baselineTaken) { + // The page is still finishing loading. Come back once we know what "unchanged" looks like. + scheduleSnapshot(); + return; + } + // Only ever one gather-and-post at a time. + // + // Two would be a correctness bug, not just waste: HTTP does not promise that two outstanding + // POSTs arrive in the order they were sent, so an OLDER snapshot could land after a newer one + // and C# would keep the older content -- silently dropping the newest edits. It takes a slow + // enough machine, or a big enough page, for a post to still be in flight when the next + // keystroke's snapshot comes round, which is exactly the case this has to survive. + // + // Returning here loses nothing: the run that is already going re-schedules if anything changed + // while it worked, and it reads changeCount after it finishes, so it sees those changes. The + // effect on a slow machine is that snapshots coalesce by themselves rather than piling up. + if (busy) return; + busy = true; + let markRunDone: () => void = () => {}; + runDone = new Promise((resolve) => { + markRunDone = resolve; + }); + const countWhenStarted = changeCount; + try { + // Waits for any in-flight work that belongs in the page (see pageContentDelays), then + // reads the page the same way a real save does, so a snapshot can never differ from what + // a save would have produced at the same moment. + const content = await gatherPageContent(); + + // The page may have been unloaded, or navigated, while we were waiting. + if (pageIdBeingWatched !== pageId) return; + + if (content !== lastPosted) { + const reply = await postStringQuietly( + snapshotUrl(kApi, pageId), + content, + ); + // Two different things can mean C# does not have this content, and both must count as + // NOT sent: + // + // * C# refused it, answering false. It refuses a snapshot from a load it is not + // showing, including in the moment before this page has reported itself ready, since + // the two APIs are not ordered with respect to each other. A refusal is not a + // failure; it just means try again. + // * The POST failed and we got no answer at all. The post goes through wrapAxios, + // which turns a rejected request into a resolved promise carrying nothing -- so a + // failed post looks exactly like a successful one apart from the missing response. + // Reading only `.data` would therefore take a failure for an acceptance, record the + // content as sent, and never offer it again; the next save would write what C# still + // held, losing everything typed since the snapshot before. + // + // They are retried differently. A refusal costs nothing and ends by itself the moment + // the page reports ready, so we simply keep offering. A failure means the server is + // not answering, so that one backs off (and is reported once, see + // reportFailureOncePerPage, rather than on every attempt). + const response = reply as { data?: boolean | string } | void; + const refused = !!response && response.data === false; + const failed = !response; + if (refused) { + consecutiveFailedPosts = 0; + scheduleSnapshot(kRetryAfterRefusalMs); + return; + } + if (failed) { + reportFailureOncePerPage( + pageId, + "Bloom could not keep track of your changes to this page: the request to save them did not get through.", + undefined, + ); + retryAfterFailure(); + return; + } + consecutiveFailedPosts = 0; + // Only once the post has actually resolved AND been taken. Recording it earlier would + // mean content C# never received still counted as sent: we would never retry it, and + // the next save would write what C# still held, losing everything typed since. + lastPosted = content; + } + } catch (error) { + // Gathering the page can legitimately throw -- the BL-13120 origami guard, a missing + // marginBox, the canvas-element count checks -- and so can the post. Either way this is + // the one failure the whole design cannot afford to be quiet about: C# concludes "no + // snapshot, so nothing to save", and the user's edits are dropped without a word. (The + // global unhandledrejection handler is commented out in lib/errorHandler.ts, so nothing + // else would report it.) Before BL-13502 the equivalent failure came back through the + // state machine as "Bloom had trouble saving a page"; this keeps that promise. + // + // Once per page: a page that fails will fail again on the very next keystroke, and we + // also retry on a timer, so without this the same error would be put in front of the user + // over and over. + reportFailureOncePerPage( + pageId, + "Bloom could not keep track of your changes to this page: " + + (error instanceof Error ? error.message : String(error)), + error instanceof Error ? error.stack : undefined, + ); + // Keep offering, on the same backoff as a failed post. A gather is deterministic, so this + // will usually fail the same way -- but it costs no further reports now, and if the + // failure did depend on something transient in the page, this is what recovers from it. + retryAfterFailure(); + } finally { + // Only release the lock if we are still the run that took it. If the page was unloaded + // and another started while we were awaiting, this run belongs to the old page, and + // clearing the flag here would unlock the NEW page's in-flight post -- allowing two at + // once, which is the one thing the flag exists to prevent. Not reachable today, because + // each page load is a fresh document with its own module state, but the module claims to + // be safe to restart and this is what makes that true. + if (pageIdBeingWatched === pageId) busy = false; + markRunDone(); + } + // Something changed while we were gathering or posting: that change is not in what we just + // sent, so go round again. + if (changeCount !== countWhenStarted) scheduleSnapshot(); +} + +// The delay register (pageContentDelays.ts) has gone busy or idle. C# needs to know, because a +// save it makes from the snapshot -- leaving the Edit tab, quitting, a command from a separate +// dialog -- cannot wait for the register the way a gather here does: the snapshot it holds simply +// predates the work. So we tell it what the page is busy with, and it waits a bounded time for us +// to say the page is idle again (PageSnapshot.WaitUntilIdle), logging the culprit if we do not. +function handleDelayRegisterChange(nowBusyWith: string | undefined): void { + const pageId = pageIdBeingWatched; + if (!pageId) return; + busyWith = nowBusyWith; + if (nowBusyWith !== undefined) void tellCSharpBusy(pageId, nowBusyWith); + else void tellCSharpIdle(pageId); +} + +function snapshotUrl(api: string, pageId: string): string { + return `${api}?pageId=${encodeURIComponent(pageId)}&loadId=${encodeURIComponent( + pageLoadId, + )}`; +} + +// The url for a busy or idle notice: a snapshot url plus this notice's place in the sequence. +function noticeUrl(api: string, pageId: string): string { + return `${snapshotUrl(api, pageId)}&seq=${++noticeSequence}`; +} + +// C# refuses a notice about a load it is not showing, exactly as it refuses such a snapshot, and +// for the same reason the refusal must not be the end of it: this page may simply not have +// reported itself ready yet. A post that fails outright (no reply at all; see takeSnapshot for +// why that looks the way it does) has not reached C# either. In both cases, offer it again while +// the work is still going -- a save made meanwhile would otherwise not know to wait. +async function tellCSharpBusy(pageId: string, what: string): Promise { + const reply = await postStringQuietly(noticeUrl(kBusyApi, pageId), what); + if (wasTaken(reply)) return; + if (busyWith === what && pageIdBeingWatched === pageId) { + window.setTimeout(() => { + if (busyWith === what && pageIdBeingWatched === pageId) + void tellCSharpBusy(pageId, what); + }, kRetryAfterRefusalMs); + } +} + +// Whether C# took a notice: it answered, and did not answer false. +function wasTaken(reply: unknown): boolean { + const response = reply as { data?: boolean | string } | void; + return !!response && response.data !== false; +} + +// Idle means "and you already have the page as it is now", so the snapshot goes first, and only +// then do we say idle -- and only if the page has not gone busy again meanwhile; the next idle +// will speak for that. +// +// A run already under way is not enough on its own. It may have been parked behind the register +// (see takeSnapshot), in which case it will post the finished page; but it may equally have read +// the page BEFORE the work began and be sitting in its post, in which case what it sends predates +// the work. So once it is done we gather again regardless: that posts only if the page differs +// from what was last sent, so in the first case it costs one gather and no post. +async function tellCSharpIdle(pageId: string): Promise { + if (busy) await runDone; + await takeSnapshot(); + await postIdleNotice(pageId); +} + +// The idle notice itself. If C# does not take it -- the post failed, or C# refused it because it +// had not yet accepted this load -- it is offered again while the page is still idle. A lost idle +// would leave C# believing the page busy until the next navigation, and every snapshot-based save +// in between would sit out the whole wait. +async function postIdleNotice(pageId: string): Promise { + if (pageIdBeingWatched !== pageId || busyWith !== undefined) return; + const reply = await postStringQuietly(noticeUrl(kIdleApi, pageId), ""); + if (wasTaken(reply)) return; + window.setTimeout(() => { + void postIdleNotice(pageId); + }, kRetryAfterRefusalMs); +} + +function scheduleSnapshot(delayMs: number = kQuietMs): void { + if (timer !== undefined) window.clearTimeout(timer); + timer = window.setTimeout(() => { + timer = undefined; + void takeSnapshot(); + }, delayMs); +} + +function noteChange(): void { + changeCount++; + scheduleSnapshot(); +} + +/** + * Tell the watcher that the saved form of the page may have changed in a way it cannot see. + * + * The MutationObserver covers everything in the body, which is nearly all of what we gather. It + * does NOT cover the user's own style definitions: those are gathered too (see + * getPageContentForSave), they live in a