From 8f2e3277871ac5e459b1eeee85baea16a3955a24 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Wed, 26 Aug 2026 11:48:59 -0500 Subject: [PATCH] Save a page without wrecking it (BL-13502) Saving a page used to destroy it. To get the page's content, Bloom stripped the live page -- pulled the toolbox tool off it, took CKEditor down, unwound the canvas-element machinery -- and what was left was no longer editable. That is why a save was always followed by a reload: the state machine had a whole state (SavedAndStripped) whose only purpose was to remember that the page had to be navigated away from before the user could touch it again. Now the content is gathered from a CLONE. getBodyContentForSavePage() clones the body, cleans the clone, and never touches the live page. A save can therefore finish and leave the user exactly where they were, which is what savePageWithoutReloading() does -- the AI image editor's post-commit save is the first caller, and it no longer yanks the page out from under its own overlay. The same clone makes page-list commands one step instead of two. A click, Copy, Paste, Duplicate, Delete or reorder now sends the outgoing page's content along WITH the command, so C# merges it, runs the command and navigates in a single pass (SaveThen's pageContentFromBrowser, reaching EditingStateMachine.ToSavedInPlaceThenNavigating). Previously each of these had to ask the browser for content and wait for a second round trip. Because a save no longer strips the page, the teardown that used to ride along with it needed a home of its own: pageUnloading() now detaches the tool, resets the controls above the page and cleans up the canvas machinery, and EditingView.OnHideEditTab calls it when the user leaves the Edit tab (nothing unloads the page frame on that path). Notable pieces: - InPlaceSaveOutcome distinguishes Saved / Declined / Failed / Refused. Only Declined may fall back to the old ask-the-browser route: Failed means the action may already have run, and Refused means an external process replaced the book and this page must not be written at all. - pageContentDelays.ts is the single gate that stops content being captured while an async command that should be saved is still running. - Tool markup is now taken off the clone via ITool.removeToolMarkup, the same method detachFromPage runs on the live page, so the two cannot drift. removeReaderMarkup.ts and niceScrollCleanup.ts split that work out. - The Talking Book tool stamps the highlight spans it creates, so undoing them cannot strip highlight markup the book itself contains. - src/BloomExe/Edit/SavingWithoutReloading.md explains the design, and the two benchmark scripts it cites measure the round trip this removes. Co-Authored-By: Claude Opus 5 (1M context) --- .claude/skills/run-bloom/benchPageChange.mjs | 176 ++++++ .claude/skills/run-bloom/benchSaveGather.mjs | 75 +++ .../aiImageEditor/aiEditorOverlay.test.ts | 81 ++- .../bookEdit/aiImageEditor/aiEditorOverlay.ts | 66 +- src/BloomBrowserUI/bookEdit/editablePage.ts | 27 +- .../bookEdit/js/bloomEditing.ts | 341 +++++----- src/BloomBrowserUI/bookEdit/js/bloomImages.ts | 7 +- src/BloomBrowserUI/bookEdit/js/bloomVideo.ts | 4 +- .../CanvasElementBackgroundImageManager.ts | 6 +- .../CanvasElementBubbleLevelUtils.ts | 4 +- .../CanvasElementClipboard.test.ts | 9 +- .../CanvasElementClipboard.ts | 2 +- .../CanvasElementFactories.ts | 3 +- .../CanvasElementManager.ts | 54 +- .../CanvasElementResizeAdjustments.ts | 11 +- .../bookEdit/js/editableDivUtils.ts | 43 ++ .../bookEdit/js/niceScrollCleanup.spec.ts | 136 ++++ .../bookEdit/js/niceScrollCleanup.ts | 104 ++++ src/BloomBrowserUI/bookEdit/js/origami.ts | 7 +- .../bookEdit/js/pageContentDelays.spec.ts | 155 +++++ .../bookEdit/js/pageContentDelays.ts | 101 +++ .../pageThumbnailList/currentPageContent.ts | 64 ++ .../pageControls/pageControls.tsx | 18 +- .../pageThumbnailList/pageThumbnailList.tsx | 71 ++- .../canvas/canvasControlTextMenuItems.ts | 9 +- .../bookEdit/toolbox/canvas/canvasTool.tsx | 1 + .../toolbox/canvas/customXmatterPage.tsx | 6 +- .../bookEdit/toolbox/games/GameTool.tsx | 11 +- .../imageDescription/imageDescription.tsx | 20 +- .../imageDescription/imageDescriptionUtils.ts | 14 +- .../impairmentVisualizer.tsx | 25 +- .../bookEdit/toolbox/motion/motionTool.tsx | 35 +- .../decodableReader/decodableReaderTool.tsx | 10 + .../leveledReader/leveledReaderTool.tsx | 10 + .../readers/removeReaderMarkup.spec.ts | 73 +++ .../toolbox/readers/removeReaderMarkup.ts | 31 + .../toolbox/signLanguage/signLanguageTool.tsx | 1 + .../toolbox/talkingBook/IAudioRecorder.ts | 1 + .../toolbox/talkingBook/audioRecording.ts | 103 ++- .../toolbox/talkingBook/audioRecordingSpec.ts | 182 +++++- .../toolbox/talkingBook/talkingBookTool.tsx | 29 +- .../bookEdit/toolbox/toolbox.ts | 62 +- .../bookEdit/toolbox/toolboxBootstrap.ts | 9 +- .../bookEdit/toolbox/toolboxGlobals.d.ts | 1 + .../toolbox/toolboxToolReactAdaptor.tsx | 28 +- src/BloomBrowserUI/package.json | 2 +- src/BloomBrowserUI/pnpm-lock.yaml | 10 +- src/BloomBrowserUI/utils/bloomApi.ts | 11 +- src/BloomExe/Edit/EditingModel.cs | 289 +++++++-- src/BloomExe/Edit/EditingStateMachine.cs | 238 +++++++ src/BloomExe/Edit/EditingView.cs | 18 + src/BloomExe/Edit/PageControlsApi.cs | 6 +- src/BloomExe/Edit/PageListController.cs | 24 +- src/BloomExe/Edit/PageThumbnailList.cs | 62 +- src/BloomExe/Edit/SavingWithoutReloading.md | 366 +++++++++++ src/BloomExe/Edit/ToolboxView.cs | 6 + src/BloomExe/web/PageListApi.cs | 61 +- src/BloomExe/web/controllers/ApiRequest.cs | 14 + .../web/controllers/EditingViewApi.cs | 21 + .../Edit/EditingStateMachineTests.cs | 589 ++++++++++++++++++ 60 files changed, 3535 insertions(+), 408 deletions(-) create mode 100644 .claude/skills/run-bloom/benchPageChange.mjs create mode 100644 .claude/skills/run-bloom/benchSaveGather.mjs create mode 100644 src/BloomBrowserUI/bookEdit/js/niceScrollCleanup.spec.ts create mode 100644 src/BloomBrowserUI/bookEdit/js/niceScrollCleanup.ts create mode 100644 src/BloomBrowserUI/bookEdit/js/pageContentDelays.spec.ts create mode 100644 src/BloomBrowserUI/bookEdit/js/pageContentDelays.ts create mode 100644 src/BloomBrowserUI/bookEdit/pageThumbnailList/currentPageContent.ts create mode 100644 src/BloomBrowserUI/bookEdit/toolbox/readers/removeReaderMarkup.spec.ts create mode 100644 src/BloomBrowserUI/bookEdit/toolbox/readers/removeReaderMarkup.ts create mode 100644 src/BloomExe/Edit/SavingWithoutReloading.md create mode 100644 src/BloomTests/Edit/EditingStateMachineTests.cs diff --git a/.claude/skills/run-bloom/benchPageChange.mjs b/.claude/skills/run-bloom/benchPageChange.mjs new file mode 100644 index 000000000000..9145587ea535 --- /dev/null +++ b/.claude/skills/run-bloom/benchPageChange.mjs @@ -0,0 +1,176 @@ +// Benchmark a real page change end to end, from outside Bloom, so the same harness can be +// run before and after the pageClicked change and the numbers compared. +// +// Phases (all observed over CDP; no instrumentation added to Bloom): +// click -> we dispatch a real click on the page-list thumbnail +// pageClicked -> POST pageList/pageClicked completes +// pageContent -> POST editView/pageContent completes (the browser has handed C# the +// outgoing page's content: this is the round trip the change removes) +// domLoaded -> POST editView/pageDomLoaded fires (the NEW page's DOM is up) +// editable -> the new page reports its id with CKEditor attached (usable) +import { createRequire } from "node:module"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +// Find playwright through the repo's own copy, locating the repo from where THIS script lives +// (.claude/skills/run-bloom/) rather than a hard-coded path -- otherwise it only runs on the +// machine it was written on. +const repoRoot = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "../../..", +); +const r = createRequire( + path.join( + repoRoot, + "src/BloomBrowserUI/react_components/component-tester/package.json", + ), +); +const { chromium } = r("playwright"); +const sleep = (ms) => new Promise((x) => setTimeout(x, ms)); + +const PAGES = [ + { id: "e9f55da7-b76d-4178-aa66-b062d744c6c0", label: "Basic Text & Image" }, + { id: "6799f146-e29d-4521-89d3-c1192ab606b4", label: "Title Page" }, +]; +const ITERATIONS = Number(process.argv[2] ?? 8); + +// The launcher picks the CDP port; pass it in when it is not the usual one. +const cdpPort = process.env.BLOOM_CDP_PORT ?? 8091; +const b = await chromium.connectOverCDP(`http://127.0.0.1:${cdpPort}`); +const shell = b + .contexts() + .flatMap((c) => c.pages()) + .find( + (p) => + p.url().includes("/bloom/") && !p.url().startsWith("devtools://"), + ); +const listFrame = () => shell.frames().find((f) => f.name() === "pageList"); +const pageFrame = () => shell.frames().find((f) => f.name() === "page"); + +let marks = {}; +const stamp = (name) => { + if (marks[name] === undefined) marks[name] = Date.now(); +}; +shell.on("response", (res) => { + const u = res.url(); + if (!u.includes("/bloom/api/")) return; + if (u.includes("pageList/pageClicked")) stamp("pageClicked"); + else if (u.includes("editView/pageContent")) stamp("pageContent"); + else if (u.includes("editView/savePageInPlace")) stamp("savePageInPlace"); + else if (u.includes("editView/pageDomLoaded")) stamp("domLoaded"); +}); + +const waitForPage = async (wantId, deadlineMs = 25000) => { + const start = Date.now(); + while (Date.now() - start < deadlineMs) { + const f = pageFrame(); + if (f) { + try { + const ok = await f.evaluate((wantId) => { + const p = document.querySelector(".bloom-page"); + if (!p || p.getAttribute("id") !== wantId) return false; + const eds = Array.from( + document.querySelectorAll("div.bloom-editable"), + ); + // "usable" = at least one editor attached (every page here has editable text) + return eds.some((d) => !!d.bloomCkEditor); + }, wantId); + if (ok) return Date.now(); + } catch { + /* frame swapping */ + } + } + await sleep(20); + } + return null; +}; + +const clickPage = async (id) => { + return listFrame().evaluate((id) => { + const item = document.querySelector(`.gridItem[id="${id}"]`); + if (!item) return "no gridItem " + id; + const cover = item.querySelector(".invisibleThumbnailCover") || item; + cover.dispatchEvent( + new MouseEvent("click", { + bubbles: true, + cancelable: true, + view: window, + }), + ); + return "ok"; + }, id); +}; + +const currentPageId = async () => { + const f = pageFrame(); + if (!f) return null; + try { + return await f.evaluate( + () => + document.querySelector(".bloom-page")?.getAttribute("id") ?? + null, + ); + } catch { + return null; + } +}; + +// Settle before timing anything. Never assume which page Bloom starts on: each iteration below +// targets whichever of the two we are NOT currently on, so every timed click is a real change. +// (Clicking the page we are already on is not a no-op we can wait for -- and a click that lands +// while a navigation is still in flight is silently dropped, because SaveThen's "not in a state +// to save" fallback for pageClicked does nothing at all.) +await sleep(1500); + +const rows = []; +for (let i = 0; i < ITERATIONS; i++) { + const from = await currentPageId(); + const target = PAGES.find((p) => p.id !== from) ?? PAGES[0]; + marks = {}; + const t0 = Date.now(); + const clicked = await clickPage(target.id); + if (clicked !== "ok") { + console.log("CLICK FAILED:", clicked); + break; + } + const tEditable = await waitForPage(target.id); + if (!tEditable) { + console.log("TIMED OUT waiting for", target.label); + break; + } + rows.push({ + to: target.label, + pageClicked: marks.pageClicked ? marks.pageClicked - t0 : null, + pageContent: marks.pageContent ? marks.pageContent - t0 : null, + savePageInPlace: marks.savePageInPlace + ? marks.savePageInPlace - t0 + : null, + domLoaded: marks.domLoaded ? marks.domLoaded - t0 : null, + editable: tEditable - t0, + }); + await sleep(1200); // let things quiesce between runs +} + +const median = (xs) => { + const v = xs + .filter((x) => x !== null && x !== undefined) + .sort((a, b) => a - b); + if (!v.length) return null; + return v.length % 2 + ? v[(v.length - 1) / 2] + : Math.round((v[v.length / 2 - 1] + v[v.length / 2]) / 2); +}; + +console.log("\nper-change timings (ms from click):"); +for (const row of rows) console.log(" " + JSON.stringify(row)); +console.log("\nMEDIANS over " + rows.length + " changes:"); +for (const k of [ + "pageClicked", + "pageContent", + "savePageInPlace", + "domLoaded", + "editable", +]) { + const m = median(rows.map((x) => x[k])); + console.log(` ${k.padEnd(16)} ${m === null ? "(never seen)" : m + " ms"}`); +} +await b.close(); diff --git a/.claude/skills/run-bloom/benchSaveGather.mjs b/.claude/skills/run-bloom/benchSaveGather.mjs new file mode 100644 index 000000000000..9c481eb40609 --- /dev/null +++ b/.claude/skills/run-bloom/benchSaveGather.mjs @@ -0,0 +1,75 @@ +// Decompose the save round trip: how much of it is real work (gathering the page content in +// the browser, and C# writing it) versus the overhead of C# having to ASK the browser and +// wait for an HTTP callback -- which is the only part removing the round trip can save. +import { createRequire } from "node:module"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +// Find playwright through the repo's own copy, locating the repo from where THIS script lives +// (.claude/skills/run-bloom/) rather than a hard-coded path -- otherwise it only runs on the +// machine it was written on. +const repoRoot = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "../../..", +); +const r = createRequire( + path.join( + repoRoot, + "src/BloomBrowserUI/react_components/component-tester/package.json", + ), +); +const { chromium } = r("playwright"); +const sleep = (ms) => new Promise((x) => setTimeout(x, ms)); + +// The launcher picks the CDP port; pass it in when it is not the usual one. +const cdpPort = process.env.BLOOM_CDP_PORT ?? 8091; +const b = await chromium.connectOverCDP(`http://127.0.0.1:${cdpPort}`); +const shell = b + .contexts() + .flatMap((c) => c.pages()) + .find( + (p) => + p.url().includes("/bloom/") && !p.url().startsWith("devtools://"), + ); +const frame = () => shell.frames().find((f) => f.name() === "page"); + +const res = await frame().evaluate(async () => { + const ex = window.editablePageBundle; + const gather = []; + const save = []; + let size = 0; + // warm up + await ex.getPageContentForSaveWhenReady(); + for (let i = 0; i < 15; i++) { + const t = performance.now(); + // Includes the (normally zero) wait for in-flight page changes to settle, because that + // is what a real save pays: see whenNoActiveDelays in bookEdit/js/pageContentDelays.ts. + const s = await ex.getPageContentForSaveWhenReady(); + gather.push(performance.now() - t); + size = s.length; + } + for (let i = 0; i < 8; i++) { + const t = performance.now(); + await ex.savePageWithoutReloading(); + save.push(performance.now() - t); + } + const med = (a) => { + const v = [...a].sort((x, y) => x - y); + return ( + Math.round( + (v.length % 2 + ? v[(v.length - 1) / 2] + : (v[v.length / 2 - 1] + v[v.length / 2]) / 2) * 10, + ) / 10 + ); + }; + return { + pageId: document.querySelector(".bloom-page")?.getAttribute("id"), + contentBytes: size, + gatherMedianMs: med(gather), + gatherAllMs: gather.map((x) => Math.round(x * 10) / 10), + saveRoundTripMedianMs: med(save), + saveAllMs: save.map((x) => Math.round(x)), + }; +}); +console.log(JSON.stringify(res, null, 1)); +await b.close(); diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.test.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.test.ts index 649bd65c4462..181d8281fffd 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.test.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.test.ts @@ -11,11 +11,12 @@ import { beforeEach, describe, expect, test, vi } from "vitest"; // 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. +// showing the pre-edit image and match nothing. We save immediately, via the page frame's +// savePageWithoutReloading, which leaves the page under the overlay alone (BL-13502). const post = vi.fn(); const postJson = vi.fn(); +const savePageWithoutReloading = vi.fn(); const postThatMightNavigate = vi.fn(); const trackEvent = vi.fn(); const trackChangePicture = vi.fn(); @@ -37,7 +38,6 @@ vi.mock("../js/workspaceFrames", () => ({ import { openAiImageEditor } from "./aiEditorOverlay"; -const kSaveEvent = "common/saveChangesAndRethinkPageEvent"; const kEditorUrl = "http://localhost:8089/bloom/aiImageEditor/index.html"; const kPageId = "page1"; const kImageFile = "old.png"; @@ -149,6 +149,9 @@ const commitAndReplyFromHost = ( beforeEach(() => { post.mockClear(); postJson.mockClear(); + savePageWithoutReloading.mockClear(); + // It answers whether C# actually saved; the overlay chains onto that to complain if not. + savePageWithoutReloading.mockResolvedValue(true); postThatMightNavigate.mockClear(); trackEvent.mockClear(); trackChangePicture.mockClear(); @@ -159,6 +162,7 @@ beforeEach(() => { }); getEditablePageBundleExports.mockReturnValue({ applyAiImageEditorReplacements, + savePageWithoutReloading, }); delete (window as Window & { __bloomAiImageEditorCleanup?: () => void }) .__bloomAiImageEditorCleanup; @@ -233,8 +237,52 @@ describe("aiEditorOverlay: saving the live page after a commit", () => { // assertions below aren't just watching a no-op. expect(applyAiImageEditorReplacements).toHaveBeenCalledTimes(1); expect(document.getElementById("ai-editor-overlay")).toBeNull(); - expect(postThatMightNavigate).toHaveBeenCalledTimes(1); - expect(postThatMightNavigate).toHaveBeenCalledWith(kSaveEvent); + expect(savePageWithoutReloading).toHaveBeenCalledTimes(1); + }); + + test("a save Bloom refuses is complained about, not swallowed", async () => { + // Bloom can decline (the user may have started changing pages). The book on disk then + // still has the old image, which is exactly what this save exists to prevent, so the + // least we can do is say so rather than let it look like it worked. + const error = vi.spyOn(console, "error").mockImplementation(() => {}); + savePageWithoutReloading.mockResolvedValue(false); + const { postFromEditor } = openAgainstABookWithOneImage(); + + commitAndReplyFromHost(postFromEditor, true); + await vi.waitFor(() => expect(error).toHaveBeenCalled()); + + expect(error.mock.calls[0][0]).toContain("was not saved"); + error.mockRestore(); + }); + + test("a save that errors is complained about too", async () => { + // Not just the refusal: the request itself can fail, and the consequence for the user is + // the same -- the book still has the old image. + const error = vi.spyOn(console, "error").mockImplementation(() => {}); + savePageWithoutReloading.mockRejectedValue(new Error("network gone")); + const { postFromEditor } = openAgainstABookWithOneImage(); + + commitAndReplyFromHost(postFromEditor, true); + await vi.waitFor(() => expect(error).toHaveBeenCalled()); + + expect(error.mock.calls[0][0]).toContain("saving the page failed"); + error.mockRestore(); + }); + + test("a page frame that has gone away is complained about too", () => { + const error = vi.spyOn(console, "error").mockImplementation(() => {}); + const { postFromEditor } = openAgainstABookWithOneImage(); + // The commit applies through the exports we already handed out, then the frame goes. + applyAiImageEditorReplacements.mockImplementation(() => { + getEditablePageBundleExports.mockReturnValue(undefined); + return { applied: 1, failures: [] }; + }); + + commitAndReplyFromHost(postFromEditor, true); + + expect(error).toHaveBeenCalled(); + expect(error.mock.calls[0][0]).toContain("not available to save"); + error.mockRestore(); }); test("a partial failure keeps the overlay up AND still saves what landed", () => { @@ -247,14 +295,13 @@ describe("aiEditorOverlay: saving the live page after a commit", () => { // it, so the swap that did land is persisted immediately rather than held hostage // until the user closes the overlay. expect(document.getElementById("ai-editor-overlay")).not.toBeNull(); - expect(postThatMightNavigate).toHaveBeenCalledTimes(1); - expect(postThatMightNavigate).toHaveBeenCalledWith(kSaveEvent); + expect(savePageWithoutReloading).toHaveBeenCalledTimes(1); // The ✕ still works after that save, because these controls belong to the top // window, not to the page frame the save reloaded. closeButton.click(); expect(document.getElementById("ai-editor-overlay")).toBeNull(); - expect(postThatMightNavigate).toHaveBeenCalledTimes(1); + expect(savePageWithoutReloading).toHaveBeenCalledTimes(1); }); test("a commit that changed nothing on this page never saves", () => { @@ -268,9 +315,9 @@ describe("aiEditorOverlay: saving the live page after a commit", () => { // live-DOM change here to persist. commitAndReplyFromHost(postFromEditor, true); - expect(postThatMightNavigate).not.toHaveBeenCalled(); + expect(savePageWithoutReloading).not.toHaveBeenCalled(); closeButton.click(); - expect(postThatMightNavigate).not.toHaveBeenCalled(); + expect(savePageWithoutReloading).not.toHaveBeenCalled(); }); test("a failed swap's reason reaches the AI Image Editor, not just the count", () => { @@ -297,7 +344,7 @@ describe("aiEditorOverlay: saving the live page after a commit", () => { expect(ack.error).toContain("Only 1 of 2"); expect(ack.error).toContain("kaboom"); // What did land still gets saved. - expect(postThatMightNavigate).toHaveBeenCalledWith(kSaveEvent); + expect(savePageWithoutReloading).toHaveBeenCalledTimes(1); postMessageToEditor.mockRestore(); }); @@ -348,7 +395,7 @@ describe("aiEditorOverlay: saving the live page after a commit", () => { expect(ack.ok).toBe(true); expect(ack.error).toBeUndefined(); // Nothing landed on this page, so nothing to save. - expect(postThatMightNavigate).not.toHaveBeenCalled(); + expect(savePageWithoutReloading).not.toHaveBeenCalled(); expect(applyAiImageEditorReplacements).not.toHaveBeenCalled(); postMessageToEditor.mockRestore(); }); @@ -374,7 +421,7 @@ describe("aiEditorOverlay: saving the live page after a commit", () => { expect(ack.error).toContain("not available"); expect(ack.error).toContain("other pages were made"); // Nothing landed, so nothing to save. - expect(postThatMightNavigate).not.toHaveBeenCalled(); + expect(savePageWithoutReloading).not.toHaveBeenCalled(); postMessageToEditor.mockRestore(); }); }); @@ -512,9 +559,11 @@ describe("aiEditorOverlay: analytics", () => { // And the swap that landed on the page is still saved. Answering an AI Image Editor that has // gone away used to throw from inside postMessage, which skipped everything after it // in the finally block -- including this save, losing the user's picture. - expect(postThatMightNavigate).toHaveBeenCalledWith( - "common/saveChangesAndRethinkPageEvent", - ); + // + // The save itself is now savePageWithoutReloading rather than the old + // "common/saveChangesAndRethinkPageEvent" post (BL-13502): same guard, new mechanism. + // The point of the change is that this no longer reloads the page under the overlay. + expect(savePageWithoutReloading).toHaveBeenCalled(); }); test("closing while a commit is in flight DOES report a cancel if the commit then fails", () => { diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts index 0cc607e129c1..2ebcdbddffd9 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts @@ -32,7 +32,6 @@ import { post, postJson, - postThatMightNavigate, trackChangePicture, trackEvent, } from "../../utils/bloomApi"; @@ -543,23 +542,62 @@ export function openAiImageEditor(target: IAiImageEditorTarget): void { ackEditor(finalOk, message); // changeImageByElement only mutated the LIVE page DOM; // unlike the off-page slots (which C# saved), a - // current-page swap is not otherwise persisted. Save + - // rethink the page so the saved DOM matches the live one: - // otherwise a second commit in this same session would - // read its oldSrc from a saved page still showing the - // pre-edit image and match nothing ("0 of N could be - // updated"). Mirrors doVideoCommand's save after - // updateVideoInContainer. + // current-page swap is not otherwise persisted. Save the + // page so the saved DOM matches the live one: otherwise a + // second commit in this same session would read its oldSrc + // from a saved page still showing the pre-edit image and + // match nothing ("0 of N could be updated"). // - // We can save right now, even with the overlay still up, - // precisely because this overlay lives in the top window: - // the page reload underneath it leaves its controls alone. + // savePageWithoutReloading leaves the page frame alone + // (BL-13502), so the user keeps whatever they had selected + // underneath the overlay, and nothing here has to care about + // a reload happening beneath it. // (currentPageApplied is what the page frame says landed, // so a failure part way through still saves the rest.) + // + // C# can decline the save (the user may have started changing + // pages), and then the file still shows the pre-edit image -- + // exactly the state this save exists to avoid. We can't undo + // that here, but we can make sure it isn't silent. if (currentPageApplied > 0) { - postThatMightNavigate( - "common/saveChangesAndRethinkPageEvent", - ); + const complain = (why: string) => + console.error( + `AI image editor: ${why} after applying ${currentPageApplied} ` + + "replacement(s) to the open page. The book on disk still has the " + + "old image(s), so editing these again in this session may report " + + "that nothing matched.", + ); + // Every way this can go wrong has to say so, not just the + // refusal: the page frame may be gone (the optional call then + // yields nothing at all), and the request itself can fail. + // + // Note we deliberately do NOT claim which of those happened + // when we simply get back "not saved". A failed request does + // not reject: bloomApi's wrapAxios catches it (and reports the + // network error itself), so it arrives here as a false exactly + // like a refusal does. The rejection handler below is kept for + // the day that changes, but it is not what fires today. + const save = + getEditablePageBundleExports()?.savePageWithoutReloading(); + if (!save) { + complain( + "the page frame was not available to save", + ); + } else { + void save.then( + (saved) => { + if (!saved) + complain( + "the page was not saved (Bloom declined, " + + "or the request failed)", + ); + }, + (error) => + complain( + `saving the page failed (${error})`, + ), + ); + } } noteCommitSettled(); // Now, and only now, is the applied count a fact. Counted from diff --git a/src/BloomBrowserUI/bookEdit/editablePage.ts b/src/BloomBrowserUI/bookEdit/editablePage.ts index 38c5fe5d01f5..7b1e63d03bfb 100644 --- a/src/BloomBrowserUI/bookEdit/editablePage.ts +++ b/src/BloomBrowserUI/bookEdit/editablePage.ts @@ -49,6 +49,13 @@ document.addEventListener("DOMContentLoaded", () => { // and then it will bring this along, with disastrous results. export interface IPageFrameExports { requestPageContent(): void; + // Gather the current page's content and have C# save it, without the page being reloaded + // afterwards. Unlike requestPageContent(), this is initiated from the Javascript side. + // Resolves false if C# declined to save; see savePageWithoutReloading in bloomEditing.ts. + savePageWithoutReloading(): Promise; + // The combined "body userCss" string that a save needs, gathered without + // disturbing the live page. + getPageContentForSaveWhenReady(): Promise; pageUnloading(): void; copySelection(): void; cutSelection(): void; @@ -112,10 +119,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. import { - getBodyContentForSavePage, + getPageContentForSaveWhenReady, requestPageContent, + savePageWithoutReloading, captureContentForExternalProcessing, - userStylesheetContent, pageUnloading, topBarButtonClick, copySelection, @@ -129,9 +136,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 aiEditorPageCommands.ts and aiEditorOverlay.ts. @@ -141,10 +150,10 @@ import type { IAiImageEditorCommitResult, } from "./aiImageEditor/aiEditorShared"; export { - getBodyContentForSavePage, + getPageContentForSaveWhenReady, requestPageContent, + savePageWithoutReloading, captureContentForExternalProcessing, - userStylesheetContent, pageUnloading, topBarButtonClick, copySelection, @@ -401,9 +410,9 @@ export function SayHello() { // NOTE: Keep this as a minimal curated surface: only expose functions intentionally callable cross-frame. interface EditablePageBundleApi { requestPageContent: typeof requestPageContent; + savePageWithoutReloading: typeof savePageWithoutReloading; captureContentForExternalProcessing: typeof captureContentForExternalProcessing; - getBodyContentForSavePage: typeof getBodyContentForSavePage; - userStylesheetContent: typeof userStylesheetContent; + getPageContentForSaveWhenReady: typeof getPageContentForSaveWhenReady; pageUnloading: typeof pageUnloading; copySelection: typeof copySelection; cutSelection: typeof cutSelection; @@ -480,9 +489,9 @@ declare global { window.editablePageBundle = { requestPageContent, + savePageWithoutReloading, captureContentForExternalProcessing, - getBodyContentForSavePage, - userStylesheetContent, + getPageContentForSaveWhenReady, pageUnloading, copySelection, cutSelection, diff --git a/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts b/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts index 82429ecac618..e52d107b62d1 100644 --- a/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts +++ b/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts @@ -27,6 +27,10 @@ import BloomField from "../bloomField/BloomField"; import BloomNotices from "./bloomNotices"; import BloomSourceBubbles from "../sourceBubbles/BloomSourceBubbles"; import BloomHintBubbles from "./BloomHintBubbles"; +import { + whenNoActiveDelays, + wrapWithRequestPageContentDelay, +} from "./pageContentDelays"; import { CanvasElementManager, initializeCanvasElementManager, @@ -58,7 +62,7 @@ import { showInvisibles, hideInvisibles } from "./showInvisibles"; //promise may be needed to run tests with phantomjs //import promise = require('es6-promise'); //promise.Promise.polyfill(); -import axios from "axios"; +import axios, { AxiosResponse } from "axios"; import { postBoolean, postJson, @@ -76,6 +80,7 @@ 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 { setupBookLinkGrids } from "./linkGrid"; import { fitImageOverTextSplits } from "./autoFitImageOverTextSplits"; import PlaceholderProvider from "./PlaceholderProvider"; @@ -171,6 +176,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(); } @@ -1312,117 +1320,57 @@ 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. Compare the old removeEditingDebris(), which did this to the +// live DOM and so forced a page reload after every save. +// +// 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(); - } -} - -// 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; + for (const textLabel of Array.from( + marginBox.getElementsByClassName("textBox-identifier"), + )) { + textLabel.remove(); } + removeTransientVideoTimestampParams(cloneOfBody); } // 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. // -// 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. +// C# picks the moment, so any asynchronous DOM work in flight has to have registered itself with +// the delay functions above (preferably wrapWithRequestPageContentDelay) for us to wait for it. +// That is the whole reason those functions exist; Javascript-initiated saves can simply await their +// own work before calling getPageContentForSaveWhenReady(). 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(); - } + void whenNoActiveDelays().then(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. +// Return the page body + user stylesheet combined with the delimiter that C# splits +// on. Shared by the live save path (requestPageContentInternal), the save-without-reloading path +// (savePageWithoutReloading), and the off-screen capture path (captureContentForExternalProcessing), +// so the cleanup steps and the delimiter can't drift between them. // -// 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(); +// Deliberately NOT exported: every caller should come through getPageContentForSaveWhenReady() (or +// one of the two paths above, which do their 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. +// +// 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 @@ -1430,13 +1378,60 @@ function extractAndStripPageContentForSave(): string { return content + "" + userStylesheet; } +// 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. This is what the page list's commands use +// (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(); +} + +// Gather the current page's content and ask C# to save it into the book, WITHOUT the page being +// reloaded afterwards. This is the counterpart of requestPageContent(): that one exists because C# +// initiated the save and drives its own state machine through the reply; this one lets Javascript +// initiate a save at a point of its own choosing (e.g. before some operation that needs the book on +// disk to be up to date) and simply carry on editing the same page. +// +// Resolves TRUE once the book DOM has been updated and written to disk, and FALSE if C# declined +// to save -- the user may have started changing pages, or an external process may have replaced +// the book on disk. Callers that save so that the file will match the page they are about to work +// from must check: carrying on after a refused save means reading a file that does not say what +// they think it says. +export async function savePageWithoutReloading(): Promise { + const content = await getPageContentForSaveWhenReady(); + const response = await postString("editView/savePageInPlace", content); + // C# sends this as JSON, so axios normally hands us a real boolean. Accept the string too: + // "did we save?" is not worth making dependent on the reply's content type, and getting it + // wrong the other way would have the AI image editor cry failure after every good save. + const data = (response as AxiosResponse | void)?.data; + return data === true || data === "true"; +} + +// Save the page and have C# rebuild it from the updated book DOM. Unlike +// savePageWithoutReloading(), 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. +// +// What has gone is the round trip. Sending the content with the request means C# no longer has 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. +export async function saveChangesAndRethinkPage(): Promise { + await postThatMightNavigate( + "common/saveChangesAndRethinkPageEvent", + await getPageContentForSaveWhenReady(), + ); +} + function requestPageContentInternal() { - if (requestPageContentTimeout !== null) { - clearTimeout(requestPageContentTimeout); - } - requestPageContentTimeout = null; try { - postString("editView/pageContent", extractAndStripPageContentForSave()); + postString("editView/pageContent", getPageContentForSave()); } catch (e) { postString( "editView/pageContent", @@ -1453,50 +1448,88 @@ function requestPageContentInternal() { } } -// 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 @@ -1552,10 +1585,10 @@ function resizeCanvasElementsToFitContent(): void { // 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 +// 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( @@ -1589,30 +1622,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; @@ -1628,6 +1653,18 @@ export const pageUnloading = () => { 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 }) { diff --git a/src/BloomBrowserUI/bookEdit/js/bloomImages.ts b/src/BloomBrowserUI/bookEdit/js/bloomImages.ts index d59b2e41297f..49ee252aec71 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/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/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..7cb628e1f008 100644 --- a/src/BloomBrowserUI/bookEdit/js/origami.ts +++ b/src/BloomBrowserUI/bookEdit/js/origami.ts @@ -1,8 +1,9 @@ -import { SetupImage } from "./bloomImages"; +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..95cc3d81a659 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/pageContentDelays.spec.ts @@ -0,0 +1,155 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { + addRequestPageContentDelay, + getActiveDelayIdsForTesting, + kMaxWaitTimeMs, + 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("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..a6a1f78b2aef --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/pageContentDelays.ts @@ -0,0 +1,101 @@ +// 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: +// - the C#-initiated save (requestPageContent in bloomEditing.ts). This is the route the register +// really exists for: C# picks the moment, so in-flight work has no other way to hold it off. +// - the browser-initiated ones (getPageContentForSaveWhenReady, used by savePageWithoutReloading +// and by the page list's commands, via collectCurrentPageContent). Javascript could in +// principle await its own work instead, but it cannot know about work someone else started, so +// it waits here too. 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)[] = []; + +// 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); +} + +// 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()); + } +} + +// 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/pageThumbnailList/currentPageContent.ts b/src/BloomBrowserUI/bookEdit/pageThumbnailList/currentPageContent.ts new file mode 100644 index 000000000000..4bcd493dde77 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/pageThumbnailList/currentPageContent.ts @@ -0,0 +1,64 @@ +import { getEditablePageBundleExports } from "../js/workspaceFrames"; + +// Collect the content of the page the user is currently editing, to send along with a request +// that will make C# save it. +// +// C# has to save the current page before it can change pages, duplicate one, delete one, and so +// on. Sending the content with the request lets it do all of that in one step. Otherwise it has +// to ask the browser for the content and wait for the answer to arrive on a separate API, and +// while it waits it is in a state where a further request of the same kind is silently thrown +// away. (See EditingModel.SavePageInPlaceThen.) +// +// This is async because it must NOT read the page while asynchronous work whose results belong in +// the saved page is still running -- image sizing, canvas-element fitting, a clipboard paste. That +// is what the delay register in bloomEditing.ts tracks, and awaiting getPageContentForSaveWhenReady +// is how we stay behind it. The gathering itself is still cheap (well under a millisecond: it works +// on a clone and does no layout), and the wait is normally zero, and capped either way. +// +// Because the whole command waits on this, the command cannot start mid-change either: C# is not +// asked to duplicate, delete or reorder anything until the page has settled. +// +// If we cannot collect it we return undefined and leave it out of the request; C# then falls back +// to asking. That is the honest thing to do for the cases where there is nothing to collect (no +// page loaded yet) or where the page is in a state we should not be reading (mid-navigation), +// rather than sending something half-formed: this content is about to be written into the user's +// book. +// +// The promise we await belongs to the PAGE frame. If that frame navigates while we are waiting, +// its timers and microtask queue go with it and the promise simply never settles -- and since the +// whole command is waiting on us, the command would be dropped without a trace, which is worse +// than doing it without the content. So we give up after a while and let C# ask for the content +// the old way. The timer is ours, in this frame, precisely so that it survives the page frame +// going away. +const kGiveUpWaitingMs = 6000; // comfortably past the page frame's own 4s cap + +export async function collectCurrentPageContent( + whatFor: string, +): Promise { + try { + const content = + getEditablePageBundleExports()?.getPageContentForSaveWhenReady(); + if (!content) return undefined; + let giveUp: number | undefined; + const abandoned = new Promise((resolve) => { + giveUp = window.setTimeout(() => { + console.warn( + `gave up waiting for the current page's content for ${whatFor} (the page frame ` + + `may have navigated away mid-wait); C# will ask the page frame for it instead.`, + ); + resolve(undefined); + }, kGiveUpWaitingMs); + }); + try { + return await Promise.race([content, abandoned]); + } finally { + if (giveUp !== undefined) window.clearTimeout(giveUp); + } + } catch (error) { + console.warn( + `could not collect the current page's content for ${whatFor}; C# will ask the page frame for it instead.`, + error, + ); + return undefined; + } +} diff --git a/src/BloomBrowserUI/bookEdit/pageThumbnailList/pageControls/pageControls.tsx b/src/BloomBrowserUI/bookEdit/pageThumbnailList/pageControls/pageControls.tsx index 6d543b5f7b0d..fe457aa341c6 100644 --- a/src/BloomBrowserUI/bookEdit/pageThumbnailList/pageControls/pageControls.tsx +++ b/src/BloomBrowserUI/bookEdit/pageThumbnailList/pageControls/pageControls.tsx @@ -4,6 +4,7 @@ import { renderRoot } from "../../../utils/reactRender"; import BloomButton from "../../../react_components/bloomButton"; import WebSocketManager from "../../../utils/WebSocketManager"; import { confirmRemovePage } from "../confirmRemovePage"; +import { collectCurrentPageContent } from "../currentPageContent"; import "./pageControls.less"; import "errorHandler"; @@ -20,6 +21,14 @@ import "errorHandler"; const kPageControlsContext = "pageThumbnailList-pageControls"; +// Duplicating or deleting a page makes C# save the current page first, so send its content along +// and save it the round trip of asking us for it. Note this waits for any in-flight change to the +// page to settle before it posts, so the command does not start mid-change either. See +// collectCurrentPageContent(). +async function postPageControlCommand(endpoint: string) { + postThatMightNavigate(endpoint, await collectCurrentPageContent(endpoint)); +} + interface IPageControlsState { canAddState: boolean; canDuplicateState: boolean; @@ -112,8 +121,11 @@ class PageControls extends React.Component { enabled={this.state.canDuplicateState} l10nKey="EditTab.DuplicatePageButton" l10nComment="Button that tells Bloom to duplicate the currently selected page." - clickApiEndpoint="edit/pageControls/duplicatePage" - mightNavigate={true} + onClick={() => + postPageControlCommand( + "edit/pageControls/duplicatePage", + ) + } enabledImageFile="/bloom/bookEdit/pageThumbnailList/pageControls/duplicatePage.svg" disabledImageFile="/bloom/bookEdit/pageThumbnailList/pageControls/duplicatePageDisabled.svg" hasText={false} @@ -127,7 +139,7 @@ class PageControls extends React.Component { enabled={this.state.canDeleteState} onClick={() => confirmRemovePage(() => - postThatMightNavigate( + postPageControlCommand( "edit/pageControls/deletePage", ), ) diff --git a/src/BloomBrowserUI/bookEdit/pageThumbnailList/pageThumbnailList.tsx b/src/BloomBrowserUI/bookEdit/pageThumbnailList/pageThumbnailList.tsx index bc6320c35b9f..324e3ae188ff 100644 --- a/src/BloomBrowserUI/bookEdit/pageThumbnailList/pageThumbnailList.tsx +++ b/src/BloomBrowserUI/bookEdit/pageThumbnailList/pageThumbnailList.tsx @@ -28,6 +28,7 @@ import { postString, useApiData, } from "../../utils/bloomApi"; +import { collectCurrentPageContent } from "./currentPageContent"; import { PageThumbnail } from "./PageThumbnail"; import LazyLoad, { forceCheck } from "react-lazyload"; import { useL10n } from "../../react_components/l10nHooks"; @@ -702,10 +703,7 @@ const PageList: React.FunctionComponent<{ initialPageLayout: string }> = ( const pageElt = e.currentTarget.closest("[id]")!; const pageId = pageElt.getAttribute("id"); const caption = pageElt.getAttribute("data-caption"); - postJson("pageList/pageClicked", { - pageId, - detail: caption, - }); + postPageClicked(pageId!, caption ?? ""); } } }; @@ -817,10 +815,15 @@ const PageList: React.FunctionComponent<{ initialPageLayout: string }> = ( closeContextMenuOnBlurCleanupRef.current = undefined; const pageId = contextMenuPoint.pageId; - const postCommand = () => + // Most of these commands (duplicate, copy, paste, remove) have to save the current page + // first, so send its content along. See collectCurrentPageContent(). + const postCommand = async () => postJson("pageList/contextMenuItemClicked", { pageId, commandId, + pageContent: await collectCurrentPageContent( + `the ${commandId} command`, + ), }); if (commandId === "removePage") { confirmRemovePage(postCommand); @@ -1057,16 +1060,35 @@ function onDragStop( // the page clicked. (Note however that this seems to get fired on any click, // even just closing a popup menu, so it's possible that we might get more // click events than we really want.) - postJson("pageList/pageClicked", { - pageId: movedPageId, - detail: "unknown", - }); + postPageClicked(movedPageId, "unknown"); return; } // Needs more smarts if we ever do other than two columns. const newIndex = newItem.y * 2 + newItem.x; - postJson("pageList/pageMoved", { movedPageId, newIndex }); + // Moving a page saves the current one first; see collectCurrentPageContent(). + void collectCurrentPageContent("the page move").then((pageContent) => + postJson("pageList/pageMoved", { + movedPageId, + newIndex, + pageContent, + }), + ); +} + +// Tell C# the user picked a page, sending the CURRENT page's content along with the click so it +// can save the page we are leaving in the same step. See collectCurrentPageContent(). +async function postPageClicked( + pageId: string, + detail: string, + onSuccess?: () => void, +): Promise { + const pageContent = await collectCurrentPageContent("the page change"); + postJson( + "pageList/pageClicked", + { pageId, detail, pageContent }, + onSuccess, + ); } function ContinueAutomatedPageClicking( @@ -1082,22 +1104,15 @@ function ContinueAutomatedPageClicking( "** pageThumbnailList: user initiated Automated Page Clicking test function", ); } - postJson( - "pageList/pageClicked", - { - pageId: pagesRemaining[0].key, - detail: pagesRemaining[0].caption, - }, - () => { - const remaining = pagesRemaining.slice(1); - if (remaining.length > 0) - window.setTimeout( - () => { - ContinueAutomatedPageClicking(remaining, count + 1); - }, - 8 * 1000, // leave time for the browser to redraw - ); - else window.alert("Done with automated page clicking"); - }, - ); + postPageClicked(pagesRemaining[0].key, pagesRemaining[0].caption, () => { + const remaining = pagesRemaining.slice(1); + if (remaining.length > 0) + window.setTimeout( + () => { + ContinueAutomatedPageClicking(remaining, count + 1); + }, + 8 * 1000, // leave time for the browser to redraw + ); + else window.alert("Done with automated page clicking"); + }); } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlTextMenuItems.ts b/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlTextMenuItems.ts index de31057c1a97..66a82dde27be 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlTextMenuItems.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlTextMenuItems.ts @@ -5,8 +5,9 @@ import * as React from "react"; import { default as CheckIcon } from "@mui/icons-material/Check"; -import { get, postThatMightNavigate } from "../../../utils/bloomApi"; -import { wrapWithRequestPageContentDelay } from "../../js/bloomEditing"; +import { get } from "../../../utils/bloomApi"; +import { saveChangesAndRethinkPage } from "../../js/bloomEditing"; +import { wrapWithRequestPageContentDelay } from "../../js/pageContentDelays"; import { getCanvasElementManager } from "./canvasElementPageBridge"; import { IControlContext, IControlMenuCommandRow } from "./canvasControlTypes"; @@ -388,9 +389,7 @@ export function makeFieldTypeMenuItem( translationGroup, ); translationGroup.remove(); - postThatMightNavigate( - "common/saveChangesAndRethinkPageEvent", - ); + void saveChangesAndRethinkPage(); return; } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasTool.tsx index d3d6f2588cef..37436338330d 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasTool.tsx @@ -64,6 +64,7 @@ export class CanvasTool extends ToolboxToolReactAdaptor { } public detachFromPage() { + super.detachFromPage(); // this tool has no removeToolMarkup work, but see ITool.detachFromPage const canvasElementManager = getCanvasElementManager(); if (canvasElementManager) { // For now we are leaving canvas element editing on, because even with the toolbox hidden, diff --git a/src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.tsx b/src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.tsx index 23a110a078eb..36a2d400d0f0 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.tsx @@ -14,10 +14,8 @@ import { import { ensureFieldFitsOnCustomPage } from "./derivedFieldFitting"; import { getAsync, postData, postString } from "../../../utils/bloomApi"; import { Bubble, BubbleSpec } from "comicaljs"; -import { - recomputeSourceBubblesForPage, - wrapWithRequestPageContentDelay, -} from "../../js/bloomEditing"; +import { recomputeSourceBubblesForPage } from "../../js/bloomEditing"; +import { wrapWithRequestPageContentDelay } from "../../js/pageContentDelays"; import { updateAbovePageControls } from "../../js/AbovePageControls"; import BloomSourceBubbles from "../../sourceBubbles/BloomSourceBubbles"; import { ILanguageNameValues } from "../../bookAndPageSettings/FieldVisibilityGroup"; diff --git a/src/BloomBrowserUI/bookEdit/toolbox/games/GameTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/games/GameTool.tsx index 4d47fc9688a3..6b6bf011b838 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/games/GameTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/games/GameTool.tsx @@ -1837,11 +1837,12 @@ export class GameTool extends ToolboxToolReactAdaptor { } } - public detachFromPage() { - const page = GameTool.getBloomPage(); - if (page) { - undoPrepareActivity(page); - } + // While the user is on the Play tab, prepareActivity() has put the page into play mode; that + // markup must not be saved. undoPrepareActivity() is pure DOM surgery on the page element it is + // given, so the save path can run it on a clone and leave the live page in play mode, while + // detachFromPage (inherited) runs the very same thing on the live page. + public removeToolMarkup(pageOrClone: HTMLElement): void { + undoPrepareActivity(pageOrClone); } } export function playSound( diff --git a/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescription.tsx b/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescription.tsx index 1939e861c08e..11b03ca546bb 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescription.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescription.tsx @@ -13,8 +13,8 @@ import { Link } from "../../../react_components/link"; import { ToolBottomHelpLink } from "../../../react_components/ToolBottomHelpLink"; import { BloomCheckbox } from "../../../react_components/BloomCheckBox"; import { - hideImageDescriptions, showImageDescriptions, + unwrapDescribedImages, } from "./imageDescriptionUtils"; import { getCanvasElementManager } from "../canvas/canvasElementPageBridge"; import { kBloomCanvasClass } from "../canvas/canvasElementConstants"; @@ -357,11 +357,23 @@ export class ImageDescriptionAdapter extends ToolboxToolReactAdaptor { ); } + // The only thing this tool adds inside the page that would otherwise be saved is the + // bloom-describedImage wrapper. (The bloom-showImageDescriptions class is on the body, which is + // outside the page div we save, so it belongs in detachFromPage below.) + public removeToolMarkup(pageOrClone: HTMLElement): void { + unwrapDescribedImages(pageOrClone); + } + public detachFromPage() { - const page = ToolBox.getPage(); - if (page) { - hideImageDescriptions(page); + const bodyOfPageIframe = ToolBox.getPage(); + if (!bodyOfPageIframe) { + return; } + // Removing the class and the wrappers must both happen before we resume comic editing; + // resume may not work right while the extra wrapper is present. + bodyOfPageIframe.classList.remove("bloom-showImageDescriptions"); + super.detachFromPage(); // removeToolMarkup: unwraps the bloom-describedImage wrappers + getCanvasElementManager()?.resumeComicEditing(); } public isExperimental(): boolean { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescriptionUtils.ts b/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescriptionUtils.ts index 6449a57af919..fff9309275be 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescriptionUtils.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/imageDescription/imageDescriptionUtils.ts @@ -32,14 +32,22 @@ export function hideImageDescriptions(bodyOfPageIframe: HTMLElement) { // removing the class and wrapper should be done first; resume may not work // right while the extra wrapper is present. bodyOfPageIframe.classList.remove("bloom-showImageDescriptions"); - // unwrap the contents of each bloom-describedImage + unwrapDescribedImages(bodyOfPageIframe); + canvasElementManager?.resumeComicEditing(); +} + +// Undo the bloom-describedImage wrapper that showImageDescriptions() adds around the non-description +// contents of each bloom-canvas. This is the only part of hideImageDescriptions() that changes markup +// which would otherwise be saved, so it is also the only part the save path needs. It touches nothing +// but the DOM under 'root', so it is safe to run on a detached clone of the page (which is how the +// save path uses it: see removeMarkupFromPageClone in the tools that show image descriptions). +export function unwrapDescribedImages(root: HTMLElement) { for (const describedImage of Array.from( - bodyOfPageIframe.getElementsByClassName("bloom-describedImage"), + root.getElementsByClassName("bloom-describedImage"), )) { for (const child of Array.from(describedImage.children)) { describedImage.parentElement!.appendChild(child); } describedImage.remove(); } - canvasElementManager?.resumeComicEditing(); } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer.tsx b/src/BloomBrowserUI/bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer.tsx index 011d7772007c..59eaf9c55a05 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer.tsx @@ -189,17 +189,20 @@ export class ImpairmentVisualizerControls extends React.Component< } } - public static removeImpairmentVisualizerMarkup() { - const page = ToolboxToolReactAdaptor.getPage(); - if (!page || !page.ownerDocument) return; - ImpairmentVisualizerControls.removeColorBlindnessMarkup(page); - const body = page.ownerDocument.body; + // The classes that drive the cataract and colour-blindness filters live on the page iframe's + // body, which is outside the .bloom-page div and so is never saved. That makes this live-page + // work, unlike removeColorBlindnessMarkup: see ImpairmentVisualizerAdaptor.detachFromPage. + public static removeSimulationClassesFromBody() { + const body = ToolboxToolReactAdaptor.getPage(); + if (!body) return; body.classList.remove("simulateColorBlindness"); body.classList.remove("simulateCataracts"); } // Caller is responsible for guarding against a null page parameter. - private static removeColorBlindnessMarkup(page: HTMLElement) { + // Public because it is also the tool's ITool.removeToolMarkup implementation, which the save + // path runs on a clone of the page. + public static removeColorBlindnessMarkup(page: HTMLElement) { [].slice .call(page.getElementsByClassName("ui-cbOverlay")) .map((x) => x.parentElement.removeChild(x)); @@ -359,8 +362,16 @@ export class ImpairmentVisualizerAdaptor extends ToolboxToolReactAdaptor { this.controlsElement.updateSimulations(undefined); } + // The colour-blindness overlays are the only markup this tool puts inside the page div. (The + // simulateColorBlindness/simulateCataracts classes go on the body, which we never save, so + // removing those is left to removeImpairmentVisualizerMarkup, below.) + public removeToolMarkup(pageOrClone: HTMLElement): void { + ImpairmentVisualizerControls.removeColorBlindnessMarkup(pageOrClone); + } + public detachFromPage() { - ImpairmentVisualizerControls.removeImpairmentVisualizerMarkup(); + super.detachFromPage(); // removeToolMarkup: the overlays + ImpairmentVisualizerControls.removeSimulationClassesFromBody(); } public isExperimental(): boolean { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/motion/motionTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/motion/motionTool.tsx index 2589f48d2188..c4deeb192526 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/motion/motionTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/motion/motionTool.tsx @@ -279,25 +279,34 @@ export class MotionTool extends ToolboxToolReactAdaptor { this.setupResizeObserver(); } + // The start/end rectangles are the one bit of this tool's editing markup that isn't bloom-ui, + // so they would be saved if we didn't take them out. (Their positions are already stored in the + // bloom-canvas's data-initialrect/data-finalrect attributes by updateDataAttributes(), so + // removing the rectangles loses nothing.) We also drop the audio highlight this tool's preview + // leaves behind. + public removeToolMarkup(pageOrClone: HTMLElement): void { + pageOrClone.querySelector("#animationStart")?.remove(); + pageOrClone.querySelector("#animationEnd")?.remove(); + MotionTool.removeCurrentAudioMarkup(pageOrClone); + } + public detachFromPage() { + // This must come first: while a preview is playing, the rectangles have been moved into the + // animation canvas, and cleanupAnimation() is what puts the page back together. if (this.rootControl.state.playing) { this.rootControl.setState({ playing: false }); window.clearTimeout(this.stopPreviewTimeout); this.cleanupAnimation(); } - const page = this.getPage(); - if (page) { - this.removeElt(page.getElementById("animationStart")); - this.removeElt(page.getElementById("animationEnd")); - } + super.detachFromPage(); // removeToolMarkup: the rectangles and the audio highlight + // enhance: if more than one image...do what?? const bloomCanvasToAnimate = this.getBloomCanvasToAnimate(); if (!bloomCanvasToAnimate) { return; } EnableImageEditing(bloomCanvasToAnimate); - this.removeCurrentAudioMarkup(); if (this.observer) { this.observer.disconnect(); } @@ -306,13 +315,11 @@ export class MotionTool extends ToolboxToolReactAdaptor { } } - private removeCurrentAudioMarkup(): void { - const page = this.getPage(); - if (!page) return; - const currentAudioElts = page.getElementsByClassName("ui-audioCurrent"); - if (currentAudioElts.length) { - currentAudioElts[0].classList.remove("ui-audioCurrent"); - } + // Static, and taking the root to work in, so that removeToolMarkup() can use it on a clone. + private static removeCurrentAudioMarkup(pageOrClone: ParentNode): void { + pageOrClone + .querySelector(".ui-audioCurrent") + ?.classList.remove("ui-audioCurrent"); } public id(): string { @@ -903,7 +910,7 @@ export class MotionTool extends ToolboxToolReactAdaptor { if (this.narrationPlayer) { this.narrationPlayer.stopListen(); } - this.removeCurrentAudioMarkup(); + MotionTool.removeCurrentAudioMarkup(page); // stop background music this.getPlayer().pause(); } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/decodableReaderTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/decodableReaderTool.tsx index 2097265ea23f..e94de735a626 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/decodableReaderTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/decodableReaderTool.tsx @@ -2,6 +2,7 @@ import ToolboxToolReactAdaptor from "../../toolboxToolReactAdaptor"; import { DecodableReaderToolControls } from "./DecodableReaderToolControls"; import { beginInitializeDecodableReaderTool } from "../readerTools"; import { getTheOneReaderToolsModel, MarkupType } from "../readerToolsModel"; +import { removeReaderMarkup } from "../removeReaderMarkup"; import { get } from "../../../../utils/bloomApi"; import { isReaderToolEnabledOnCurrentPage } from "../readerToolPageState"; import { renderRoot } from "../../../../utils/reactRender"; @@ -36,7 +37,16 @@ export class DecodableReaderTool extends ToolboxToolReactAdaptor { // usually updateMarkup will do this, unless we are coming from showTool model.doMarkup(); } + // Take our markup off the page we are about to save (a clone), or off the live page when we + // are being detached from it. See removeReaderMarkup. + public removeToolMarkup(pageOrClone: HTMLElement): void { + removeReaderMarkup(pageOrClone); + } + public detachFromPage(): void { + super.detachFromPage(); // takes the markup off the live page + // ...and this stops it coming back: it also resets the model so that further typing is + // not marked up. getTheOneReaderToolsModel().setMarkupType(0); } public updateMarkup() { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/readers/leveledReader/leveledReaderTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/readers/leveledReader/leveledReaderTool.tsx index 17c434c65f0f..b47ba4f0ed5b 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/readers/leveledReader/leveledReaderTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/readers/leveledReader/leveledReaderTool.tsx @@ -4,6 +4,7 @@ import ToolboxToolReactAdaptor from "../../toolboxToolReactAdaptor"; import { isReaderToolEnabledOnCurrentPage } from "../readerToolPageState"; import { beginInitializeLeveledReaderTool } from "../readerTools"; import { getTheOneReaderToolsModel } from "../readerToolsModel"; +import { removeReaderMarkup } from "../removeReaderMarkup"; import { LeveledReaderToolControls } from "./LeveledReaderToolControls"; import $ from "jquery"; @@ -86,9 +87,18 @@ export class LeveledReaderTool extends ToolboxToolReactAdaptor { model.doMarkup(); } + // Take our markup off the page we are about to save (a clone), or off the live page when we + // are being detached from it. See removeReaderMarkup. + public removeToolMarkup(pageOrClone: HTMLElement): void { + removeReaderMarkup(pageOrClone); + } + // this function removes all markup from a page when either that page has been // closed or the tool has been closed. public detachFromPage(): void { + super.detachFromPage(); // takes the markup off the live page + // ...and this stops it coming back: it also resets the model so that further typing is + // not marked up. getTheOneReaderToolsModel().setMarkupType(0); } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/readers/removeReaderMarkup.spec.ts b/src/BloomBrowserUI/bookEdit/toolbox/readers/removeReaderMarkup.spec.ts new file mode 100644 index 000000000000..56f2bb7a08ea --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/toolbox/readers/removeReaderMarkup.spec.ts @@ -0,0 +1,73 @@ +import { describe, it, expect } from "vitest"; +import { removeReaderMarkup } from "./removeReaderMarkup"; + +// The reader tools mark a page that has more text on it than the level allows. That marking is an +// editing aid and must not reach the user's book. Since a save now works from a clone, the cleanup +// has to be something we can point at an element rather than something that reaches into the live +// page frame. + +const pageWith = (inner: string, pageClasses = "bloom-page") => { + const page = document.createElement("div"); + page.className = pageClasses; + page.innerHTML = inner; + return page; +}; + +describe("removeReaderMarkup", () => { + it("takes the too-much-text marking off the page itself", () => { + const page = pageWith( + "

text

", + "bloom-page page-too-many-words-or-sentences", + ); + expect( + page.classList.contains("page-too-many-words-or-sentences"), + ).toBe(true); // sanity + + removeReaderMarkup(page); + + expect( + page.classList.contains("page-too-many-words-or-sentences"), + ).toBe(false); + expect(page.classList.contains("bloom-page")).toBe(true); + }); + + it("takes it off a page nested inside what it is given", () => { + // The save path hands us a clone of the body, so the marked div is a descendant. + const body = document.createElement("div"); + body.appendChild( + pageWith("

x

", "bloom-page page-too-many-words-or-sentences"), + ); + + removeReaderMarkup(body); + + expect( + body.querySelectorAll(".page-too-many-words-or-sentences").length, + ).toBe(0); + }); + + it("leaves the text alone", () => { + // The tools' word and sentence highlighting is painted with the CSS Custom Highlight API, + // so there is nothing of theirs inside the text to clean up -- and nothing here may + // disturb what the user actually wrote. + const html = + '

Just text, with a | in it.

'; + const page = pageWith( + html, + "bloom-page page-too-many-words-or-sentences", + ); + + removeReaderMarkup(page); + + expect(page.innerHTML).toBe(html); + }); + + it("leaves a page that was never marked alone", () => { + const html = '

Just text.

'; + const page = pageWith(html); + + removeReaderMarkup(page); + + expect(page.className).toBe("bloom-page"); + expect(page.innerHTML).toBe(html); + }); +}); diff --git a/src/BloomBrowserUI/bookEdit/toolbox/readers/removeReaderMarkup.ts b/src/BloomBrowserUI/bookEdit/toolbox/readers/removeReaderMarkup.ts new file mode 100644 index 000000000000..88e2abdb86e9 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/toolbox/readers/removeReaderMarkup.ts @@ -0,0 +1,31 @@ +// Take the decodable/leveled reader tools' editing markup off a page — either the live one, when +// the tool is being detached, or the clone we are about to save. +// +// There is exactly one thing to remove: the class the tools put on the .bloom-page div to mark a +// page as having more text on it than the level allows. It is an editing aid and must not be +// stored in the user's book. +// +// Nothing has to be done inside the text itself. The tools' word- and sentence-level highlighting +// is drawn with the CSS Custom Highlight API, which paints ranges without touching the DOM, and +// the hover tip is `bloom-ui`, which the C# save pipeline discards. (Older versions of the markup +// code did wrap each sentence/word/grapheme in a span, which is why removeSynphonyMarkup() still +// unwraps those; the only place that still produces them is the Reader Setup dialog's own word +// list, which is never part of a book.) +// +// removeSynphonyMarkup() cannot do this job in any case, because it reaches into the live page +// frame by id rather than working on an element it is given, so it can only ever clean the page +// the user is looking at. That was fine when saving destroyed the live page anyway; now that we +// save from a clone (BL-13502), the cleanup has to be element-scoped, which is what this is. + +const kTooMuchStuffOnPageClass = "page-too-many-words-or-sentences"; + +export function removeReaderMarkup(pageOrClone: HTMLElement): void { + // The class lives on the .bloom-page div, which may be the element we were given (when a tool + // is detached from the live page) or inside it (when we are cleaning a clone of the body). + if (pageOrClone.classList.contains(kTooMuchStuffOnPageClass)) + pageOrClone.classList.remove(kTooMuchStuffOnPageClass); + for (const marked of Array.from( + pageOrClone.getElementsByClassName(kTooMuchStuffOnPageClass), + )) + marked.classList.remove(kTooMuchStuffOnPageClass); +} diff --git a/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguageTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguageTool.tsx index 76a0e64d7d67..01f93a47708a 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguageTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/signLanguage/signLanguageTool.tsx @@ -1037,6 +1037,7 @@ export class SignLanguageTool extends ToolboxToolReactAdaptor { } public detachFromPage() { + super.detachFromPage(); // this tool has no removeToolMarkup work, but see ITool.detachFromPage this.reactControls.leaveCurrentVideoContext(); // Decided NOT to remove bloom-selected here. It's harmless (only the edit stylesheet // does anything with it) and leaving it allows us to keep the same one selected diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/IAudioRecorder.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/IAudioRecorder.ts index 341595c94fc4..51bb5c92bad5 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/IAudioRecorder.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/IAudioRecorder.ts @@ -14,6 +14,7 @@ export interface IAudioRecorder { setRecordingMode(recordingMode: RecordingMode): Promise; handleImportRecordingClick(): void; removeRecordingSetup: () => void; + undoHighlightingFixes: (pageOrClone: ParentNode) => void; getUpdateMarkupAction: () => Promise<() => void>; setupForRecordingAsync: () => Promise; handleToolHiding: () => void; diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts index 1199dff5aaac..dadf81534b85 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts @@ -105,6 +105,11 @@ const kEnableHighlightClass = "ui-enableHighlight"; // For example, some elements have highlighting prevented at this level // because its content has been broken into child elements, only some of which show the highlight const kDisableHighlightClass = "ui-disableHighlight"; +// Stamped on the ui-enableHighlight spans that fixHighlighting() creates, so undoHighlightingFixes +// can take out OUR spans and leave alone any the book itself contains. An attribute, not a +// JS-side record of the elements, because the undo also has to work on a CLONE of the page, whose +// elements are different objects. +const kTempHighlightAttr = "data-bloom-temp-highlight"; const kAudioSentence = "audio-sentence"; // Even though these can now encompass more than strict sentences, we continue to use this class name for backwards compatability reasons const kAudioSentenceClassSelector = "." + kAudioSentence; const kBloomEditableTextBoxClass = "bloom-editable"; @@ -4997,14 +5002,14 @@ export default class AudioRecording implements IAudioRecorder { ); if (containsNonHighlightText) { - if (!this.nodesToRestoreAfterPlayEnded.has(element.id)) { - // Note: The map could already have the id if you do Play -> Pause -> Play - // We want the modifications to exist during the Pause period, - // and we want the original innerHTML to win, so that's why we need to check - // if the ID exists already and avoid overwriting it. - this.nodesToRestoreAfterPlayEnded.set( + // Remember that we touched this one, and whether the no-highlight class was + // ours to remove -- a book can carry that class itself, and then it is not + // ours to take off. Keep the FIRST answer: on Play -> Pause -> Play the class + // is present the second time round because WE added it. + if (!this.elementsWeFixedHighlightingIn.has(element.id)) { + this.elementsWeFixedHighlightingIn.set( element.id, - element.innerHTML, + !element.classList.contains(kDisableHighlightClass), ); } @@ -5134,27 +5139,87 @@ export default class AudioRecording implements IAudioRecorder { private makeHighlightedSpan(textContent: string) { const newSpan = document.createElement("span"); newSpan.classList.add(kEnableHighlightClass); + newSpan.setAttribute(kTempHighlightAttr, "true"); newSpan.appendChild(document.createTextNode(textContent)); return newSpan; } - private nodesToRestoreAfterPlayEnded = new Map(); + // The audio spans fixHighlighting() has modified in this session, by id, each mapped to + // whether WE added kDisableHighlightClass to it (as opposed to the book already having it). + // Deliberately not a snapshot of what was in them -- see undoHighlightingFixes. + private elementsWeFixedHighlightingIn = new Map(); + + /** + * Take the temporary highlight-segment markup fixHighlighting() added back out, under + * 'pageOrClone'. The caller chooses what to apply it to: the live page (when the page is going + * away, via revertFixHighlighting) or a clone of it (when we are saving the page the user is + * still working on). + * + * This UNDOES the transformation rather than restoring a snapshot of what the element held + * beforehand, and that distinction matters: the user can type into a text box while its audio + * is playing, and playback is exactly when these fixes are in place. Replaying a snapshot taken + * when playback started would throw that typing away -- silently, and into the saved book + * (BL-13502). Unwrapping only what we added cannot: anything else in the element is left where + * it is, including text that arrived after the fix, and including the phrase-delimiter spans + * that removeToolMarkup wraps around a "|" just before calling us. + * + * Scoped to the elements we actually fixed, for the same reason the snapshot version was: an + * older book can legitimately contain ui-enableHighlight spans of its own (HtmlDom.cs even + * generates user-style rules targeting them), and a save must not quietly strip those. + * + * Purely DOM, and idempotent: undoing on a clone leaves the live page's fixes in place, ready + * to be undone again for the next save. See ITool.removeToolMarkup. + */ + public undoHighlightingFixes(pageOrClone: ParentNode) { + this.elementsWeFixedHighlightingIn.forEach( + (weAddedTheNoHighlightClass, id) => { + // Deliberately NOT `querySelector(\`#${id}\`)`. An id that is not a valid CSS + // identifier -- a legacy one starting with a digit, say -- makes that form THROW, and + // this now runs during every save's clone cleanup, where a throw would abort the whole + // page gather and we would post an error string instead of the user's page. Comparing + // the property cannot throw whatever the id looks like. + const element = Array.from( + pageOrClone.querySelectorAll("[id]"), + ).find((candidate) => candidate.id === id); + if (!element) { + console.warn("Can't find element " + id); + return; + } + // Unwrap the spans we wrapped runs of text in: put each one's children back where it + // was and drop it. Only OURS -- selected by the marker fixHighlighting stamps on them + // -- because a book can legitimately contain ui-enableHighlight spans of its own, even + // nested inside a sentence the tool has touched, and a save must not strip those. + for (const span of Array.from( + element.querySelectorAll( + `span.${kEnableHighlightClass}[${kTempHighlightAttr}]`, + ), + )) { + const parent = span.parentNode; + if (!parent) continue; + while (span.firstChild) + parent.insertBefore(span.firstChild, span); + parent.removeChild(span); + // Rejoin the text nodes that leaves adjacent, so the result has the shape the text + // had before rather than a run of separate nodes. + parent.normalize(); + } + // And the class we put on the audio span itself to stop the whole thing highlighting + // -- but only if it was ours to put there. + if (weAddedTheNoHighlightClass) + element.classList.remove(kDisableHighlightClass); + }, + ); + } /** * This function will undo in BloomDesktop the modifications made by fixHighlighting() */ public revertFixHighlighting() { - this.nodesToRestoreAfterPlayEnded.forEach((htmlToRestore, id) => { - const pageDocBody = this.getPageDocBody(); - const element = pageDocBody?.querySelector(`#${id}`); - if (element) { - element.innerHTML = htmlToRestore; - element.classList.remove(kDisableHighlightClass); - } else { - console.warn("Can't find element " + id); - } - }); - this.nodesToRestoreAfterPlayEnded.clear(); + const pageDocBody = this.getPageDocBody(); + if (pageDocBody) { + this.undoHighlightingFixes(pageDocBody); + } + this.elementsWeFixedHighlightingIn.clear(); this.refreshAudioTextHighlights(); } } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts index 29ff89db35a9..c9eb28c4946f 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts @@ -2341,6 +2341,170 @@ describe("audio recording tests", () => { expect(colorSpans[4].innerText).toBe("Three"); }); + describe("- undoHighlightingFixes()", () => { + // It takes fixHighlighting()'s temporary markup back out -- off the live page when the page + // is going away, and off the CLONE we are about to save while the user goes on editing. + // It undoes the transformation rather than restoring a snapshot of what the element held + // before, and that is the point: a save can happen while audio is playing, which is exactly + // when these fixes are in place, so by then the user may have typed. A snapshot would throw + // that typing away, silently, into the saved book (BL-13502). + const fixedUpBox = () => { + SetupIFrameFromHtml( + '

One Two    Three

', + ); + const box1 = getFrameElementById("page", "box1")!; + // The SAME recorder must do the fixing and the undoing, as in production (both go + // through the one theOneAudioRecorder): it only undoes in elements it knows it fixed. + const recording = new AudioRecording(); + recording.fixHighlighting(box1); + const span = box1.querySelector("span")!; + // Sanity: the fix really did happen, so the assertions below aren't watching a no-op. + expect( + span.classList.contains("ui-disableHighlight"), + "test setup: fixHighlighting should have marked the span", + ).toBe(true); + expect( + span.querySelectorAll("span.ui-enableHighlight").length, + "test setup: fixHighlighting should have wrapped the text runs", + ).toBeGreaterThan(0); + // fixHighlighting only re-wraps; it does not change the text. So this is what the + // text should still read after the undo. (Captured rather than written out, because + // the fixture's runs of   are not the plain spaces they look like.) + const textBefore = span.textContent; + return { box1, span, recording, textBefore }; + }; + + // Every test below asserts the undo actually happened, not merely that something survived + // it -- otherwise an undo that did nothing at all would pass most of them. + const expectUndone = (span: Element) => { + expect( + span.querySelectorAll("span.ui-enableHighlight").length, + "the highlight-run spans should be gone", + ).toBe(0); + expect( + span.classList.contains("ui-disableHighlight"), + "the no-highlight marking should be gone", + ).toBe(false); + }; + + it("puts the text back the way it was", () => { + const { box1, span, recording, textBefore } = fixedUpBox(); + + recording.undoHighlightingFixes(box1); + + expectUndone(span); + expect(span.textContent).toBe(textBefore); + // Rejoined, not left as the several adjacent text nodes unwrapping produces. + expect(span.childNodes.length).toBe(1); + }); + + it("keeps text typed while the audio was playing", () => { + const { box1, span, recording } = fixedUpBox(); + span.appendChild(document.createTextNode(" typed later")); + + recording.undoHighlightingFixes(box1); + + expectUndone(span); + expect(span.textContent).toContain("typed later"); + }); + + it("keeps text typed inside one of the highlight runs", () => { + const { box1, span, recording } = fixedUpBox(); + const firstRun = span.querySelector("span.ui-enableHighlight")!; + firstRun.textContent = firstRun.textContent + " inserted"; + + recording.undoHighlightingFixes(box1); + + expectUndone(span); + expect(span.textContent).toContain("inserted"); + }); + + it("leaves the phrase-delimiter spans alone", () => { + // removeToolMarkup enshrouds the vertical bars BEFORE calling us, so its work has to + // survive -- restoring a snapshot would have wiped it out and the bars would show. + const { box1, span, recording } = fixedUpBox(); + const marker = document.createElement("span"); + marker.classList.add("bloom-audio-split-marker"); + marker.textContent = "|"; + span.appendChild(marker); + + recording.undoHighlightingFixes(box1); + + expectUndone(span); + expect( + span.querySelectorAll("span.bloom-audio-split-marker").length, + ).toBe(1); + }); + + it("leaves alone a book's own highlight span nested inside a sentence it fixed", () => { + // The tighter version of the test below. Scoping by element is not enough: a book can + // carry ui-enableHighlight markup of its own INSIDE a sentence the tool has touched, + // and a save must not quietly strip that out of the file. + const { box1, span, recording } = fixedUpBox(); + const fromTheBook = box1.ownerDocument.createElement("span"); + fromTheBook.classList.add("ui-enableHighlight"); + fromTheBook.textContent = "the book's own"; + span.appendChild(fromTheBook); + + recording.undoHighlightingFixes(box1); + + expect( + span.querySelectorAll("span.ui-enableHighlight").length, + "the book's own span should survive", + ).toBe(1); + expect(span.textContent).toContain("the book's own"); + }); + + it("leaves the no-highlight class alone when the book already had it", () => { + SetupIFrameFromHtml( + '

One Two    Three

', + ); + const box1 = getFrameElementById("page", "box1")!; + const recording = new AudioRecording(); + recording.fixHighlighting(box1); + + recording.undoHighlightingFixes(box1); + + expect( + box1 + .querySelector("span")! + .classList.contains("ui-disableHighlight"), + "a class the book brought is not ours to remove", + ).toBe(true); + }); + + it("leaves alone highlight spans it did not put there", () => { + // An older book can legitimately carry ui-enableHighlight spans of its own (HtmlDom.cs + // even generates user-style rules targeting them); a save must not quietly strip those + // just because the Talking Book tool happens to be open. + const { box1, recording } = fixedUpBox(); + const other = box1.ownerDocument.createElement("span"); + other.id = "notOneOfOurs"; + other.innerHTML = + 'from the book'; + box1.appendChild(other); + + recording.undoHighlightingFixes(box1); + + expect( + other.querySelectorAll("span.ui-enableHighlight").length, + "a span we never fixed should be left alone", + ).toBe(1); + }); + + it("can be run more than once", () => { + // Every save undoes on a clone; the live page's fixes stay, to be undone again later. + const { box1, span, recording } = fixedUpBox(); + + recording.undoHighlightingFixes(box1); + const afterFirst = span.textContent; + recording.undoHighlightingFixes(box1); + + expectUndone(span); + expect(span.textContent).toBe(afterFirst); + }); + }); + describe("- fixHighlighting()", () => { const scenarios: ("Check" | "Listen to whole page")[] = [ "Check", @@ -2382,7 +2546,7 @@ describe("audio recording tests", () => { ).toBe(true); expect(childSpan.innerHTML).toBe( - 'One Two  Three   Four    End', + 'One Two  Three   Four    End', ); }); @@ -2401,8 +2565,8 @@ describe("audio recording tests", () => { expect(box1.innerHTML).toBe( "

" + 'One Two  End1.' + - 'Three   End2.' + - 'Four    Five     End3.' + + 'Three   End2.' + + 'Four    Five     End3.' + "

", ); }); @@ -2439,8 +2603,8 @@ describe("audio recording tests", () => { expect(box1.innerHTML).toBe( "

" + 'One Two  End1.' + - 'Three   End2.' + - 'Four    Five     End3.' + + 'Three   End2.' + + 'Four    Five     End3.' + "

", ); }); @@ -2459,7 +2623,7 @@ describe("audio recording tests", () => { // Verification expect(box1.innerHTML).toBe( "

" + - 'Three   End2.' + + 'Three   End2.' + "

", ); }); @@ -2480,7 +2644,7 @@ describe("audio recording tests", () => { // Verification expect(box1.innerHTML).toBe( "

" + - 'Three\u200B \u200BEnd2.' + + 'Three\u200B \u200BEnd2.' + "

", ); }); @@ -3121,7 +3285,7 @@ function getExpectedResultForComplexHtmlFromUser() {

-

              Mientras navegaban,                               Jesús se quedó                             profundamente dormido.

-

         ​ ​   ​ De pronto, una gran                            tormenta se desató. 

+

              Mientras navegaban,                               Jesús se quedó                             profundamente dormido.

+

         ​ ​   ​ De pronto, una gran                            tormenta se desató. 

`; } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookTool.tsx index 18d48657b367..619d5412a3c4 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookTool.tsx @@ -1,5 +1,6 @@ -import { hideImageDescriptions } from "../imageDescription/imageDescriptionUtils"; +import { unwrapDescribedImages } from "../imageDescription/imageDescriptionUtils"; import { kBloomCanvasClass } from "../canvas/canvasElementConstants"; +import { getCanvasElementManager } from "../canvas/canvasElementPageBridge"; import { beginLoadSynphonySettings } from "../readers/readerTools"; import { getTheOneToolbox } from "../toolbox"; import { ToolBox } from "../toolbox"; @@ -136,18 +137,34 @@ export default class TalkingBookTool extends ToolboxToolReactAdaptor { } } + // The markup this tool adds that would otherwise reach the saved HTML: the + // bloom-describedImage wrappers, the visible "|" phrase-delimiter spans, and (while audio is + // playing or paused) the highlight-segment spans fixHighlighting() inserts. Everything else + // removeRecordingSetup() deals with is either bloom-ui (the playback-order controls, the + // recording icon), not in the DOM at all (the ::highlight registry), or purely tool state, so + // it lives in detachFromPage below. + public removeToolMarkup(pageOrClone: HTMLElement): void { + unwrapDescribedImages(pageOrClone); + TalkingBookTool.enshroudPhraseDelimiters(pageOrClone); + getAudioRecorder()?.undoHighlightingFixes(pageOrClone); + } + public detachFromPage() { const audioRecorder = getAudioRecorder(); // not quite sure how this can be called when never initialized, but if // we don't have the object we certainly can't use it. if (audioRecorder) { + // Live-only: takes down the playback-order UI and resets the tool's own state. It also + // calls revertFixHighlighting(), which does the same DOM restoration removeToolMarkup() + // does and then clears the record of it, so the super call below finds nothing left. audioRecorder.removeRecordingSetup(); } - const page = ToolBox.getPage(); - if (page) { - hideImageDescriptions(page); - TalkingBookTool.enshroudPhraseDelimiters(page); - } + // The rest is what hideImageDescriptions() used to do for us here: the + // bloom-showImageDescriptions class is on the body, which is outside the page div that + // removeToolMarkup() gets, and comic editing must not resume until the wrappers are gone. + ToolBox.getPage()?.classList.remove("bloom-showImageDescriptions"); + super.detachFromPage(); + getCanvasElementManager()?.resumeComicEditing(); } // Called whenever the user edits text. diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts index 5b90c7331def..6adb17a6d1bf 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts @@ -100,8 +100,32 @@ export interface ITool { // To guard against certain race conditions, we currently call this again after 600ms. Tools should // allow for this possibility and not repeat any work that was already done. newPageReady(); - detachFromPage(); // called when a page is going away AND before hideTool + // Remove from 'pageOrClone' the markup this tool adds for editing that must not be saved. + // THE SAME METHOD IS USED TWO WAYS, which is why the parameter is named as it is: + // * on every save, with a detached CLONE of the .bloom-page div, so we can save clean HTML + // while the user goes on editing the real page (see getPageContentForSave in + // bloomEditing.ts); + // * on the live .bloom-page div when the page is going away, from detachFromPage(). + // So it must be pure DOM surgery inside 'pageOrClone': it may not reach out to the live + // document, and it may not change this tool's own state (that would be wrong on a save, when + // the tool is still running). Anything live-only — observers, React state, re-enabling image + // editing, clearing caches — belongs in detachFromPage() instead. + // Leave it as the inherited no-op if the markup this tool adds is all either marked bloom-ui or + // ui-resizable-handle, or is a cke_* class, or lives outside the .bloom-page div: the C# save + // pipeline already discards all of those (see HtmlDom.ProcessPageAfterEditing). But make that a + // deliberate decision, not an omission. + removeToolMarkup(pageOrClone: HTMLElement): void; + // Called when a page is going away AND before hideTool. ToolboxToolReactAdaptor's + // implementation calls removeToolMarkup() on the live page, so a tool that has nothing + // live-only to do needs only removeToolMarkup(). OVERRIDE THIS ONLY TO ADD live-only teardown, + // and be sure to call super.detachFromPage() at the point where the markup should come off; + // detachCurrentTool() complains to the console if you forget. + detachFromPage(): void; id(): string; // without trailing "Tool"! + // True if the last call to detachFromPage() reached ToolboxToolReactAdaptor's implementation, + // i.e. removeToolMarkup() was run on the live page. Only detachCurrentTool() should use this; + // it is how we notice a tool that overrode detachFromPage() and forgot to call super. + didRemoveToolMarkupWhileDetaching(): boolean; hasRestoredSettings: boolean; isAlwaysEnabled(): boolean; isExperimental(): boolean; @@ -390,7 +414,7 @@ export class ToolBox { } this.doWhenClosingTool = []; if (currentTool && isToolInitialized(currentTool)) { - currentTool.detachFromPage(); + detachToolFromPage(currentTool); } } // A list of tasks to do when the current tool is closed. This is currently used to @@ -807,7 +831,21 @@ function detachCurrentTool() { } else if (currentTool && isToolInitialized(currentTool)) { // If the toolbox is not available, we still may be able to detach the current tool. // This is what we used to do before we had some extra behavior in the toolbox. - currentTool.detachFromPage(); + detachToolFromPage(currentTool); + } +} + +// Detach one tool from the live page, and complain if it overrode detachFromPage() without calling +// super.detachFromPage(). That mistake is easy to make and its symptom is remote: the tool's markup +// stays on the page and gets saved into the book, but only sometimes and only for that tool. Since +// this runs while we are changing pages, we report rather than throw — losing the page change would +// be worse than the stale markup we are warning about. +function detachToolFromPage(tool: ITool): void { + tool.detachFromPage(); + if (!tool.didRemoveToolMarkupWhileDetaching()) { + console.error( + `${tool.id()}Tool.detachFromPage() did not call super.detachFromPage(), so its removeToolMarkup() never ran on the live page. See ITool.detachFromPage.`, + ); } } @@ -1136,12 +1174,26 @@ function restoreToolboxSettingsWhenPageReady(settings: ToolboxSettings) { }); } -// Remove any markup the toolbox is inserting. Called by a RunJavaScript() in EditingView -// before saving the page. +// Remove any markup the toolbox is inserting. Called when the page is going away (or the tool is +// being switched); it detaches the current tool from the live page, which leaves the page unusable +// for further editing. export function removeToolboxMarkup() { detachCurrentTool(); } +// Strip from 'pageClone' — a detached clone of the page div — any markup the current tool added for +// editing that must not be saved. This runs the very same ITool.removeToolMarkup() that +// detachFromPage() runs on the live page, so the two can't drift apart; the difference is only in +// what we hand it. Everything else about detaching (the doWhenClosingTool tasks that close popups +// and dialogs, each tool's live-only teardown) is deliberately skipped: the user is still on this +// page and still using this tool. +// Called (via the toolbox bundle exports) from the page iframe's getPageContentForSave(). +export function removeToolMarkupFromPageClone(pageClone: HTMLElement): void { + if (currentTool && isToolInitialized(currentTool)) { + currentTool.removeToolMarkup(pageClone); + } +} + function switchTool(newToolName: string): void { // Have Bloom remember which tool is active. (Might be none) postString("editView/saveToolboxSetting", "current\t" + newToolName); diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolboxBootstrap.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolboxBootstrap.ts index d7db8cc854c8..98b152955133 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolboxBootstrap.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolboxBootstrap.ts @@ -4,6 +4,7 @@ import { getTheOneToolbox, applyToolboxStateToUpdatedPage, removeToolboxMarkup, + removeToolMarkupFromPageClone, scheduleMarkupUpdateAfterPaste, updateMarkupAfterUndoOrRedo, } from "./toolbox"; @@ -52,13 +53,18 @@ export interface IToolboxFrameExports { applyToolboxStateToPage(): void; removeToolboxMarkup(): void; + removeToolMarkupFromPageClone(pageClone: HTMLElement): void; setActiveDragActivityTab(tab: number): void; getTheOneAudioRecorderForExportOnly(): IAudioRecorder; simulateBlurOnPageFrameMouseDown(): void; } // each of these exports shows up under this window's toolboxBundle object (see workspaceFrames.ts) -export { removeToolboxMarkup, setActiveDragActivityTab }; +export { + removeToolboxMarkup, + removeToolMarkupFromPageClone, + setActiveDragActivityTab, +}; export { showSetupDialog, initializeReaderSetupDialog, @@ -141,6 +147,7 @@ const toolboxBundle: ToolboxBundleApi = { updateMarkupAfterUndoOrRedo, applyToolboxStateToPage, removeToolboxMarkup, + removeToolMarkupFromPageClone, showSetupDialog, initializeReaderSetupDialog, closeSetupDialog, diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolboxGlobals.d.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolboxGlobals.d.ts index 5a8316ce19a6..5b3526115acf 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolboxGlobals.d.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolboxGlobals.d.ts @@ -27,6 +27,7 @@ declare global { updateMarkupAfterUndoOrRedo: unknown; applyToolboxStateToPage: unknown; removeToolboxMarkup: unknown; + removeToolMarkupFromPageClone: unknown; showSetupDialog: unknown; initializeReaderSetupDialog: unknown; closeSetupDialog: unknown; diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolboxToolReactAdaptor.tsx b/src/BloomBrowserUI/bookEdit/toolbox/toolboxToolReactAdaptor.tsx index 5f282eda3943..d27bf12d05a1 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolboxToolReactAdaptor.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolboxToolReactAdaptor.tsx @@ -57,11 +57,37 @@ export default abstract class ToolboxToolReactAdaptor return false; } public newPageReady() {} - public detachFromPage() {} + // Most tools' editing markup is either marked bloom-ui (which the C# save pipeline strips) or + // lives outside the page div (which is never saved), so they have nothing to remove. See + // ITool.removeToolMarkup for what to do if yours does. + public removeToolMarkup(_pageOrClone: HTMLElement): void {} public configureElements(_container: HTMLElement) {} public finishToolLocalization(_pane: HTMLElement) {} /* eslint-enable @typescript-eslint/no-empty-function */ + private removedToolMarkupWhileDetaching = false; + + /// Take this tool's markup off the live page. A tool that has nothing live-only to clean up + /// needs only to implement removeToolMarkup(); it gets this for free, and the save path gets + /// the identical cleanup by calling the same method on a clone. If you do override this to add + /// live-only teardown, call super.detachFromPage() at the point where the markup should come + /// off — see ITool.detachFromPage. + public detachFromPage(): void { + this.removedToolMarkupWhileDetaching = true; + const bloomPage = ToolboxToolReactAdaptor.getBloomPage(); + if (bloomPage) { + this.removeToolMarkup(bloomPage); + } + } + + // See ITool.didRemoveToolMarkupWhileDetaching. Reading it also resets it, so that each detach + // is judged on its own. + public didRemoveToolMarkupWhileDetaching(): boolean { + const result = this.removedToolMarkupWhileDetaching; + this.removedToolMarkupWhileDetaching = false; + return result; + } + public static getPageFrame(): HTMLIFrameElement { return parent.window.document.getElementById( "page", diff --git a/src/BloomBrowserUI/package.json b/src/BloomBrowserUI/package.json index 403fec8ccd10..0577ea293273 100644 --- a/src/BloomBrowserUI/package.json +++ b/src/BloomBrowserUI/package.json @@ -151,7 +151,7 @@ "calculate-aspect-ratio": "0.1.3", "clsx": "1.2.1", "color-blind": "0.1.1", - "comicaljs": "0.3.106", + "comicaljs": "0.4.1", "contentful": "8.3.7", "filesize": "6.3.0", "global": "4.4.0", diff --git a/src/BloomBrowserUI/pnpm-lock.yaml b/src/BloomBrowserUI/pnpm-lock.yaml index 26b968ce374e..2d50b038b7f7 100644 --- a/src/BloomBrowserUI/pnpm-lock.yaml +++ b/src/BloomBrowserUI/pnpm-lock.yaml @@ -103,8 +103,8 @@ importers: specifier: 0.1.1 version: 0.1.1 comicaljs: - specifier: 0.3.106 - version: 0.3.106 + specifier: 0.4.1 + version: 0.4.1 contentful: specifier: 8.3.7 version: 8.3.7 @@ -4446,10 +4446,10 @@ packages: } engines: { node: ">= 0.8" } - comicaljs@0.3.106: + comicaljs@0.4.1: resolution: { - integrity: sha512-4cwouGkHUmOAVM7125HxCy4L8TF1LRpHcam9vHtl2UhW2A3wGqJRF3+LVPDuFxor4ZlOeyT3vm3UWS/INjT7GA==, + integrity: sha512-5Z3k/o51TDZ7k4bEzLltI7XAWR9pvvhAYAqcLIHW8j4TDlly6lv5lYgDVKrij04wqFCQ4eOtGYJMkjtB1F6b/Q==, } commander@13.1.0: @@ -13821,7 +13821,7 @@ snapshots: dependencies: delayed-stream: 1.0.0 - comicaljs@0.3.106: + comicaljs@0.4.1: dependencies: paper: 0.12.8 diff --git a/src/BloomBrowserUI/utils/bloomApi.ts b/src/BloomBrowserUI/utils/bloomApi.ts index 82ec01847c26..6f0c876df452 100644 --- a/src/BloomBrowserUI/utils/bloomApi.ts +++ b/src/BloomBrowserUI/utils/bloomApi.ts @@ -652,11 +652,18 @@ export function post( // If we one day need to do this with a callback, we will need to think very // hard about possible exceptions during the callback (and the possibility // that the callback is somehow messed up by the page reloading). -export function postThatMightNavigate(urlSuffix: string) { +// The optional value is sent as the body, as text/plain, exactly as postString() does. It is +// there for commands that send the current page's content along so C# can save it without a +// round trip (see collectCurrentPageContent in pageThumbnailList/currentPageContent.ts). +export function postThatMightNavigate(urlSuffix: string, value?: string) { + const config = + value === undefined + ? undefined + : { headers: { "Content-Type": "text/plain" } }; // The internal catch should suppress any errors. In case that fails (which it has), passing // false to wrapAxios further suppresses any error reporting. return wrapAxios( - axios.post(getBloomApiPrefix() + urlSuffix).catch(), + axios.post(getBloomApiPrefix() + urlSuffix, value, config).catch(), false, ); } diff --git a/src/BloomExe/Edit/EditingModel.cs b/src/BloomExe/Edit/EditingModel.cs index 2dd1471f371e..ac120cdc51d7 100644 --- a/src/BloomExe/Edit/EditingModel.cs +++ b/src/BloomExe/Edit/EditingModel.cs @@ -1,4 +1,4 @@ -//#define MEMORYCHECK +//#define MEMORYCHECK using System; using System.Collections.Generic; using System.Diagnostics; @@ -129,16 +129,7 @@ ITemplateFinder sourceCollectionsList (string pageId, string pageContentData) => UpdateBookDomFromBrowserPageContent(pageContentData), // saveBook - () => - { - if (_modifiedPageElement == null) - return; - - CurrentBook.SavePageToDisk(_modifiedPageElement, _nextSaveMustBeFull); - _nextSaveMustBeFull = false; - _pageHasUnsavedDataDerivedChange = false; - PageTemplatesApi.LastSaveTime = DateTime.Now; - }, + SaveBookToDisk, // hidePage () => { @@ -504,9 +495,9 @@ BookSelectionChangedEventArgs bookSelectionChangedEventArgs } } - internal void OnDuplicatePage() + internal void OnDuplicatePage(string pageContentFromBrowser = null) { - DuplicatePage(_pageSelection.CurrentSelection); + DuplicatePage(_pageSelection.CurrentSelection, pageContentFromBrowser); } internal void DuplicateManyPages(IPage page) @@ -522,9 +513,9 @@ internal void DuplicateManyPages(IPage page) } } - internal void DuplicatePage(IPage page) + internal void DuplicatePage(IPage page, string pageContentFromBrowser = null) { - DuplicatePageInternal(page); + DuplicatePageInternal(page, 1, pageContentFromBrowser); } /// @@ -542,7 +533,11 @@ public void DuplicatePageManyTimes(int numberOfTimes) DuplicatePageInternal(_pageSelection.CurrentSelection, numberOfTimes); } - private void DuplicatePageInternal(IPage page, int numberOfTimesToDuplicate = 1) + private void DuplicatePageInternal( + IPage page, + int numberOfTimesToDuplicate = 1, + string pageContentFromBrowser = null + ) { // NB: though there is an api call to do this, it isn't currently used, so we have to measure here. var countString = numberOfTimesToDuplicate.ToString(); @@ -582,16 +577,17 @@ private void DuplicatePageInternal(IPage page, int numberOfTimesToDuplicate = 1) return newPageId; }, () => { }, // wrong state, do nothing - forceFullSave: true + forceFullSave: true, + pageContentFromBrowser: pageContentFromBrowser ); } - internal void OnDeletePage() + internal void OnDeletePage(string pageContentFromBrowser = null) { - DeletePage(_pageSelection.CurrentSelection); + DeletePage(_pageSelection.CurrentSelection, pageContentFromBrowser); } - internal void DeletePage(IPage page) + internal void DeletePage(IPage page, string pageContentFromBrowser = null) { // This can only be called on the UI thread in response to a user button click. // If that ever changed we might need to arrange locking for access to InProcessOfSaving and _tasksToDoAfterSaving. @@ -624,7 +620,8 @@ internal void DeletePage(IPage page) } }, () => { }, // wrong state, do nothing - forceFullSave: true + forceFullSave: true, + pageContentFromBrowser: pageContentFromBrowser ); } @@ -666,9 +663,19 @@ private void OnRelocatePage(RelocatePageInfo info) } /// - /// This is used both to insert pages from the AddPageDialog, and also "paste page" + /// The event handler form of InsertPage, for the AddPageDialog's InsertPage event. The + /// dialog is a separate window, so it has no way to hand us the current page's content; + /// "paste page" calls InsertPage directly and can. /// private void OnInsertPage(object page, PageInsertEventArgs e) + { + InsertPage(page, e, null); + } + + /// + /// This is used both to insert pages from the AddPageDialog, and also "paste page" + /// + private void InsertPage(object page, PageInsertEventArgs e, string pageContentFromBrowser) { SaveThen( () => @@ -719,7 +726,8 @@ private void OnInsertPage(object page, PageInsertEventArgs e) return newPageId; }, () => { }, // wrong state, do nothing - forceFullSave: true + forceFullSave: true, + pageContentFromBrowser: pageContentFromBrowser ); } @@ -1637,15 +1645,33 @@ private void EnsureLevelAttrCorrect() // var idOfFirstPageInTemplateBook = CurrentBook.FindTemplateBook().GetPageByIndex(0).Id; // if (AddNewPageBasedOnTemplate(idOfFirstPageInTemplateBook)) /// - /// Save all the changes to the current page, then reload it (thus restoring any UI stuff that - /// was stripped out by the Save). + /// Save all the changes to the current page, then reload it. + /// + /// The reload used to be needed just to restore the UI markup the Save stripped out. That + /// is no longer true (BL-13502), but it is still doing a second job for these callers, and + /// that is why it stays: the page has to be rebuilt from the book DOM either because C# + /// just changed it (a new topic in the data div, new book settings) or because the browser + /// created elements that have never been through SetupElements (a new origami layout, an + /// imported video, a translation group replaced by a derived field). + /// + /// pageContentFromBrowser, when the caller was able to send it, removes the round trip: + /// we save and navigate in one step rather than asking the browser for the content and + /// waiting for it on another API. Callers that have no browser request to carry it (the + /// PageRefreshEvent handlers) leave it null and get the old path. /// - internal void SavePageAndReloadIt(bool forceFullSave = false) + internal void SavePageAndReloadIt( + bool forceFullSave = false, + string pageContentFromBrowser = null + ) { if (CannotSavePage()) return; _nextSaveMustBeFull |= forceFullSave; - SaveThen(() => _pageSelection.CurrentSelection.Id, () => { }); + SaveThen( + () => _pageSelection.CurrentSelection.Id, + () => { }, + pageContentFromBrowser: pageContentFromBrowser + ); } //invoked from TopicChooserDialog.tsx via API @@ -1660,7 +1686,9 @@ internal void SetTopic(string englishTopicAsKey) internal void SavePageAndReloadIt(ApiRequest request) { - SavePageAndReloadIt(); + // The browser sends the current page's content with this request when it can; see + // saveChangesAndRethinkPage() in bloomEditing.ts. + SavePageAndReloadIt(pageContentFromBrowser: request.GetPageContentFromBrowserOrNull()); request.PostSucceeded(); } @@ -1694,15 +1722,45 @@ private bool CannotSavePage() /// If you are doing this in an API handler, remember that you must retrieve any data in /// the request before calling SaveThen. The Request object can't be used inside doBeforeSaveToDisk, /// since by then the request has been marked completed. + /// The current page's content, when the request that + /// got us here brought it along (see getPageContentForSaveWhenReady() in the browser). Given + /// it, we do the whole thing here and now instead of asking the browser and waiting -- see + /// SavePageInPlaceThen. Everything above still describes what happens; only the number of + /// hops changes. If we turn out not to be in a state to save, we fall back to asking, so + /// passing this is always safe. public void SaveThen( Func doBeforeSaveToDisk, Action doIfNotInRightStateToSave, bool forceFullSave = false, bool skipSaveToDisk = false, Action failureAction = null, - Action doAfterSaveToDisk = null + Action doAfterSaveToDisk = null, + string pageContentFromBrowser = null ) { + if (pageContentFromBrowser != null) + { + // The in-place route does the save itself, in one go, so it has nowhere to put + // these three: they all describe things that happen around a save spread over two + // API calls. No caller combines them with sending content, and quietly ignoring + // them would be a nasty way to find that out. + Guard.Against( + skipSaveToDisk || failureAction != null || doAfterSaveToDisk != null, + "SaveThen: skipSaveToDisk, failureAction and doAfterSaveToDisk are not supported" + + " together with pageContentFromBrowser" + ); + if ( + SavePageInPlaceThen(pageContentFromBrowser, doBeforeSaveToDisk, forceFullSave) + != InPlaceSaveOutcome.Declined + ) + return; + // ONLY Declined may fall through. It means nothing at all happened -- + // doBeforeSaveToDisk has NOT run -- so doing it the long way is safe. Failed means + // the action may already have run (doing it again would duplicate or delete a + // second page), and Refused means this page must not be written at all because an + // external process has replaced the book; the long way would happily write it. + } + _nextSaveMustBeFull |= forceFullSave; if ( !_stateMachine.ToSavePending( @@ -1738,6 +1796,134 @@ public void ReceivePageContent(string pageContentData) _stateMachine.ToSavedAndStripped(pageContentData); } + /// + /// Write out whatever UpdateBookDomFromBrowserPageContent() put into the book DOM: either just + /// the one page that changed, or the whole book if something shared changed. + /// This is the state machine's saveBook action, and also the second half of SavePageInPlace, + /// so both routes make exactly the same decisions. + /// + private void SaveBookToDisk() + { + if (_modifiedPageElement == null) + return; + + CurrentBook.SavePageToDisk(_modifiedPageElement, _nextSaveMustBeFull); + _nextSaveMustBeFull = false; + _pageHasUnsavedDataDerivedChange = false; + PageTemplatesApi.LastSaveTime = DateTime.Now; + } + + /// + /// Save the current page from content the browser has ALREADY gathered — the combined + /// "body <SPLIT-DATA> userCss" string that getPageContentForSave() produces — and leave the + /// browser showing that same page, still editable. + /// + /// This is the Javascript-initiated counterpart of SaveThen(). SaveThen has to ask the browser + /// for the page content and wait for it to arrive through an API, and it always finishes by + /// navigating, because the way it made the browser gather that content stripped the live page + /// of the markup that makes it editable (BL-13502). The browser now gathers the content from a + /// clone without touching the live page, so when Javascript hands us the content we can do the + /// entire save here and now and simply return. + /// + /// It deliberately goes through the same two steps as the SaveThen path — first + /// UpdateBookDomFromBrowserPageContent(), then SaveBookToDisk() — so the same logic decides + /// whether the change is confined to this page or has to be propagated across the book + /// (see NeedToDoFullSave and Book.UpdateDomFromEditedPage). + /// + /// Returns false, having done nothing, if we are not in a position to save. That is a normal + /// outcome, not an error: the user may have started changing pages, or an external process may + /// have replaced the book on disk. + /// + public bool SavePageInPlace(string pageContentData, bool forceFullSave = false) + { + if (CannotSavePage() || !_havePageToSave) + return false; + // An external process has overwritten the book on disk and we are about to discard this + // page in favor of what it wrote; saving now would clobber that. (Same reasoning as + // EditingStateMachine.DiscardInFlightSave, which covers the SaveThen path.) + if (_reloadFromDiskOnLeavingEditTab) + return false; + + _nextSaveMustBeFull |= forceFullSave; + if ( + !_stateMachine.ToSavedInPlace( + pageContentData, + e => + ErrorReport.NotifyUserOfProblem( + e, + LocalizationManager.GetString( + "Errors.CouldNotSavePage", + "Bloom had trouble saving a page. Please report the problem to us. Then quit Bloom, run it again, and check to see if the page you just edited is missing anything. Sorry!" + ) + ) + ) + ) + return false; + + // What we just saved is the new baseline for deciding whether the NEXT save has changed + // anything the rest of the book shares. (For the SaveThen path, the navigation that + // follows a save does this, in EditingView.StartNavigationToEditPage.) + SaveStateForFullSaveDecision(); + // Likewise, the page list would normally be refreshed as part of navigating. + _view?.UpdateThumbnailAsync(_pageSelection.CurrentSelection); + return true; + } + + /// + /// The direct counterpart of SaveThen for a request that arrived from the browser WITH the + /// current page's content: save that content, run doBeforeSaveToDisk (which may change the + /// book, and returns the id of the page to show next), write the book to disk, and navigate + /// there — all synchronously, before we return. + /// + /// SaveThen has to do the same work spread over two API calls and three states, because it + /// must ask the browser for the content and wait for it to come back on another API. Every + /// caller that can hand us the content up front (anything posting from a frame that can + /// reach the editable page: see getPageContentForSaveWhenReady() in bloomEditing.ts) can use this + /// instead and be a plain, straight-line method — including one that can report a failure + /// in its own reply, which SaveThen cannot, since by the time it knows, the request is long + /// since completed. + /// + /// Returns Declined, having done nothing at all, if we were not in a position to save; only + /// then may the caller fall back to asking the browser. If it returns Failed, + /// doBeforeSaveToDisk may already have run and changed the book, so falling back would do it + /// a second time -- see InPlaceSaveOutcome. + /// + /// Private because SaveThen is the way in: pass it pageContentFromBrowser and it comes here + /// when it can and falls back on its own when it can't, so no caller has to get that right. + /// + private InPlaceSaveOutcome SavePageInPlaceThen( + string pageContentData, + Func doBeforeSaveToDisk, + bool forceFullSave = false + ) + { + if (CannotSavePage() || !_havePageToSave) + return InPlaceSaveOutcome.Declined; + // See SavePageInPlace: an external process has replaced the book on disk, so this + // page's content must not be written over what it wrote. Refused, NOT Declined -- + // Declined would send the caller to the ask-the-browser path, which has no such guard + // and would write the page anyway. + if (_reloadFromDiskOnLeavingEditTab) + return InPlaceSaveOutcome.Refused; + + _nextSaveMustBeFull |= forceFullSave; + // Unlike SavePageInPlace there is nothing to do afterwards on success: we do NOT + // refresh the full-save baseline or the thumbnail, because navigating does both for + // us, in EditingView.StartNavigationToEditPage, which this has already started. + return _stateMachine.ToSavedInPlaceThenNavigating( + pageContentData, + doBeforeSaveToDisk, + e => + ErrorReport.NotifyUserOfProblem( + e, + LocalizationManager.GetString( + "Errors.CouldNotSavePage", + "Bloom had trouble saving a page. Please report the problem to us. Then quit Bloom, run it again, and check to see if the page you just edited is missing anything. Sorry!" + ) + ) + ); + } + private SafeXmlElement _modifiedPageElement; /// @@ -2084,17 +2270,43 @@ public bool GetClipboardHasPage() return _pageDivFromCopyPage != null; } - public void CopyPage(IPage page) + public void CopyPage(IPage page, string pageContentFromBrowser = null) { - // need to preserve any typing they've done but not yet saved (BL-4512) + // We have to clone the page div so that if the user changes the page after doing the + // copy, when they paste they get the page as it was, not as it is now. And we have to + // save first, or the clone would miss any typing they have done but not yet saved + // (BL-4512). + Action takeTheSnapshot = () => + { + _pageDivFromCopyPage = (SafeXmlElement)page.GetDivNodeForThisPage().CloneNode(true); + _bookPathFromCopyPage = page.Book.GetPathHtmlFile(); + }; + + // In practice the page being copied is ALWAYS the selected one: the page list only + // opens its context menu on the selected page (see openContextMenu in + // pageThumbnailList.tsx, which bails unless pageId === selectedPageId), and the menu + // button is only rendered there. So we take the in-place branch and copying a page + // does not reload it -- which is the win here. + // + // The navigating branch is kept as a safety net rather than dead weight, because the + // copied page MUST end up selected: a later Paste inserts after the current selection + // (see DeterminePageWhichWouldPrecedeNextInsertion), so were that guarantee ever + // relaxed, copying without selecting would drop the pasted copy somewhere the user + // did not ask for. + var copyingTheSelectedPage = _pageSelection.CurrentSelection?.Id == page.Id; + if ( + copyingTheSelectedPage + && pageContentFromBrowser != null + && SavePageInPlace(pageContentFromBrowser, forceFullSave: true) + ) + { + takeTheSnapshot(); + return; + } SaveThen( () => { - // We have to clone this so that if the user changes the page after doing the copy, - // when they paste they get the page as it was, not as it is now. - _pageDivFromCopyPage = (SafeXmlElement) - page.GetDivNodeForThisPage().CloneNode(true); - _bookPathFromCopyPage = page.Book.GetPathHtmlFile(); + takeTheSnapshot(); return page.Id; }, () => { }, // wrong state, do nothing @@ -2106,7 +2318,7 @@ public void CopyPage(IPage page) /// Paste the previously saved _pageDivFromCopyPage as a new page. /// /// This is NOT the page we are to paste! - public void PastePage(IPage pageToPasteAfter) + public void PastePage(IPage pageToPasteAfter, string pageContentFromBrowser = null) { var templateBook = pageToPasteAfter.Book; // default is to assume it's from the same book bool fromAnotherBook = templateBook.GetPathHtmlFile() != _bookPathFromCopyPage; @@ -2129,7 +2341,8 @@ public void PastePage(IPage pageToPasteAfter) "not used", x => _pageDivFromCopyPage ); - OnInsertPage(pageForPasting, new PageInsertEventArgs(false)); // false => don't need analytics on use of template pages + // false => don't need analytics on use of template pages + InsertPage(pageForPasting, new PageInsertEventArgs(false), pageContentFromBrowser); } public void AdjustPageZoom(int delta) diff --git a/src/BloomExe/Edit/EditingStateMachine.cs b/src/BloomExe/Edit/EditingStateMachine.cs index 9555dff27e07..afd3376e2e77 100644 --- a/src/BloomExe/Edit/EditingStateMachine.cs +++ b/src/BloomExe/Edit/EditingStateMachine.cs @@ -21,6 +21,36 @@ public enum State SavedAndStripped, } +/// +/// What an attempt at an in-place save actually did. The point of the distinction is the third +/// case: a caller that has a fallback (SaveThen) must only use it when nothing happened, because +/// its doBeforeSaveToDisk is usually not something you can afford to do twice -- running it again +/// would duplicate or delete a second page. +/// +public enum InPlaceSaveOutcome +{ + // We were not in a state to save, so nothing was written and doBeforeSaveToDisk did NOT run. + // A normal outcome, not an error: the user may have started changing pages. The caller is free + // to fall back to SaveThen. + Declined, + + // Saved, and (for the ...ThenNavigating form) on the way to the next page. + Saved, + + // We started and something threw. The browser's content may already be in the book DOM and + // doBeforeSaveToDisk may have run and changed the book. The failure has been reported to the + // user; the caller must NOT fall back, or the action happens twice. + Failed, + + // We MUST not write this page at all -- an external process has replaced the book on disk and + // the user's page is about to be discarded in favour of what it wrote. Nothing was written and + // doBeforeSaveToDisk did NOT run, exactly as for Declined; the difference is that the caller + // must NOT fall back to asking the browser, because that path would go ahead and save and so + // clobber the other program's work. Refused and Declined look alike and mean opposite things, + // which is why they are separate values rather than one "didn't save". + Refused, +} + /// /// A state machine to help us reason about the possible states of the editing model, /// manage the valid transitions between them, and ensure that we don't attempt invalid ones. @@ -45,6 +75,14 @@ public class EditingStateMachine // page content) will be discarded on completion rather than merged into the DOM and written to // disk. See DiscardInFlightSave. private bool _discardInFlightSave; + + // Set only while ToSavedInPlaceThenNavigating is running its doBeforeSaveToDisk. In that window + // the browser's content is already in the book DOM, so ToNavigating's "cannot navigate while + // editing" guard does not apply -- there are no unsaved changes left to lose. Some actions do + // navigate: relocating a page raises RelocatePageEvent, and EditingModel.OnRelocatePage + // refreshes the display of the page whose HTML (side, page number) just changed. Under the old + // SaveThen flow that was legal because the action ran while the machine sat in SavedAndStripped. + private bool _runningSaveInPlaceAction; private Action _hidePage; private Action _enableStateTransitions; // arg is (enabled) @@ -102,6 +140,16 @@ public bool ToNoPage() _currentState = State.NoPage; return true; case State.Editing: + if (_runningSaveInPlaceAction) + { + // See _runningSaveInPlaceAction: we have just saved, so the guard below + // (which is about losing unsaved edits) has nothing to protect. This is + // the "action returned null, leave the editor blank" case. + LogTransition("empty page", null); + _hidePage(); + _currentState = State.NoPage; + return true; + } LogError("empty page"); throw new InvalidOperationException("Cannot empty page while editing."); case State.SavePending: @@ -161,6 +209,13 @@ public bool ToNavigating(string pageId) return true; } case State.Editing: + if (_runningSaveInPlaceAction) + { + // See _runningSaveInPlaceAction: we have just saved, so the guard below + // (which is about losing unsaved edits) has nothing to protect. + StartNavigating(pageId); + return true; + } LogError("navigate"); throw new InvalidOperationException("Cannot navigate while editing"); case State.SavePending: @@ -329,6 +384,21 @@ public bool ToSavePending( DoPostSaveAction(null, doBeforeSaveToDisk, failureAction, doAfterSaveToDisk); return true; case State.Editing: + if (_runningSaveInPlaceAction) + { + // We are inside a save-in-place action: the browser's content is already + // merged into the book DOM and _saveBook() is about to run, so there is + // nothing for another save to do. Ignoring it is also what the old flow + // did for free -- the action used to run in SavedAndStripped, where this + // method returns false. Accepting it would be actively harmful: it would + // put us in SavePending, and the ToNavigating that follows the action is + // ignored from there, so the page the action promised to go to would + // never be shown (and the browser's eventual reply would save a second + // time). Reachable from an action that changes the page selection, e.g. + // PageThumbnailList.PageMoved when the relocation is refused. + LogIgnore("save"); + return false; + } _saveActionHandlesSaveBook = saveActionHandlesSaveBook; _doBeforeSaveToDisk = doBeforeSaveToDisk; _failureAction = failureAction; @@ -372,6 +442,174 @@ public bool DiscardInFlightSave() return true; } + /// + /// Save the current page from content the browser gathered on its own initiative, and stay in + /// Editing. + /// + /// This is the transition that exists because the browser can now produce the page content + /// WITHOUT wrecking the live page (it cleans a clone: see getPageContentForSaveWhenReady() in + /// bloomEditing.ts). So, unlike the ToSavePending/ToSavedAndStripped pair, there is no stripped + /// page to recover from, nothing to wait for, and no navigation to do afterwards: the user is + /// still editing the same page when we return. + /// + /// Only legal while Editing. In every other state either there is nothing to save (NoPage), or + /// a save/navigation is already under way and this one would race with it; we return false so + /// the caller can decide what to do about that. + /// + public bool ToSavedInPlace(string pageContentData, Action reportFailure) + { + try + { + switch (_currentState) + { + case State.Editing: + LogTransition("saved in place", _pageId); + if (pageContentData.StartsWith("ERROR:")) + throw new ApplicationException(pageContentData); + _updateBookWithPageContents(_pageId, pageContentData); + _pageIdWeFailedToSave = null; + _saveBook(); + return true; + case State.NoPage: + case State.Navigating: + case State.SavePending: + case State.SavedAndStripped: + LogIgnore("save in place"); + return false; + default: + throw new InvalidOperationException( + "Unknown state In ToSavedInPlace(): " + _currentState.ToString() + ); + } + } + catch (Exception e) + { + // Unlike the ToSavedAndStripped path we don't have to navigate to get out of an + // invalid state: we never left Editing, and the browser still has the intact page. So + // all we owe the user is the report, and the caller a 'false'. + // As there, we only report once per page, so that a page that fails every time doesn't + // lock the user out of Bloom. + if (_pageId != _pageIdWeFailedToSave) + { + _pageIdWeFailedToSave = _pageId; + reportFailure(e); + } + return false; + } + finally + { + UpdateUI(); + } + } + + /// + /// Save the current page from content the browser sent WITH its request, optionally change the + /// book in some way, then go to whichever page doBeforeSaveToDisk names. This is the + /// straight-line form of the whole ToSavePending/ToSavedAndStripped sequence, and it is the + /// reason SavePending can eventually go away: when the browser hands us the content up front + /// there is nothing to wait for, so this is Editing -> Navigating in one step rather than + /// Editing -> SavePending (ask the browser, wait) -> SavedAndStripped -> Navigating. + /// + /// doBeforeSaveToDisk plays exactly the role it plays in ToSavePending: it runs after the + /// browser's content has been merged into the book DOM and before the book is written to disk + /// (so a page it duplicates or deletes already reflects the user's latest edits), and it + /// returns the id of the page to show afterwards. For a caller that only wants to change pages + /// it is simply () => theNewPageId. It is allowed to navigate (see _runningSaveInPlaceAction); + /// if it does, the navigation we do afterwards to its returned page simply supersedes it, or is + /// ignored if it is to the same page. + /// + /// If it fails we report it and do NOT navigate: doing so would throw away the edits we failed + /// to save, and unlike the ToSavedAndStripped path we are not in a broken state we have to + /// escape, since the browser still has the page intact and editable. Note the difference + /// between the two failure-ish outcomes -- see InPlaceSaveOutcome, and be careful to preserve + /// it: Declined means the action never ran and the caller may fall back to SaveThen, whereas + /// Failed means it may have run already and the caller must not run it again. + /// + public InPlaceSaveOutcome ToSavedInPlaceThenNavigating( + string pageContentData, + Func doBeforeSaveToDisk, + Action reportFailure + ) + { + try + { + switch (_currentState) + { + case State.Editing: + LogTransition("saved in place, then navigating", _pageId); + if (pageContentData.StartsWith("ERROR:")) + throw new ApplicationException(pageContentData); + _updateBookWithPageContents(_pageId, pageContentData); + _pageIdWeFailedToSave = null; + RunActionThenSaveAndNavigate(doBeforeSaveToDisk); + return InPlaceSaveOutcome.Saved; + case State.NoPage: + // There is no browser content to merge, but the action can still change the + // book (it may duplicate or delete a page), and that has to reach disk just + // the same. ToSavePending's NoPage branch goes through DoPostSaveAction, which + // runs the action, saves the book, and then navigates -- so we do the same. + RunActionThenSaveAndNavigate(doBeforeSaveToDisk); + return InPlaceSaveOutcome.Saved; + case State.Navigating: + case State.SavePending: + case State.SavedAndStripped: + LogIgnore("save in place then navigate"); + return InPlaceSaveOutcome.Declined; + default: + throw new InvalidOperationException( + "Unknown state In ToSavedInPlaceThenNavigating(): " + + _currentState.ToString() + ); + } + } + catch (Exception e) + { + if (_pageId != _pageIdWeFailedToSave) + { + _pageIdWeFailedToSave = _pageId; + reportFailure(e); + } + return InPlaceSaveOutcome.Failed; + } + finally + { + UpdateUI(); + } + } + + /// + /// The middle of ToSavedInPlaceThenNavigating, from the point where the browser's content is + /// safely in the book DOM: run the caller's action, write the book, and go to the page the + /// action named. Separated out only so that _runningSaveInPlaceAction is obviously scoped to + /// the action, and obviously cleared even if it throws. + /// + private void RunActionThenSaveAndNavigate(Func doBeforeSaveToDisk) + { + _runningSaveInPlaceAction = true; + try + { + var pageIdToGoTo = doBeforeSaveToDisk(); + _saveBook(); + if (pageIdToGoTo == null) + { + // SaveThen's contract: the action returns null to say "leave the editor blank" + // (which is how leaving the edit tab saves). DoPostSaveAction honours that, so we + // must too -- trying to navigate to no page would just leave a broken editor. + ToNoPage(); + return; + } + // Via ToNavigating rather than StartNavigating so that an action which already + // navigated to this very page (as relocating one does) is not made to do it twice. + // While _runningSaveInPlaceAction is set, ToNavigating accepts being called from + // Editing, which is the state we are still in if the action did not navigate. + ToNavigating(pageIdToGoTo); + } + finally + { + _runningSaveInPlaceAction = false; + } + } + /// /// Source: API call providing content of current page will request this after saving and before executing pending action /// (e.g. changing pages) diff --git a/src/BloomExe/Edit/EditingView.cs b/src/BloomExe/Edit/EditingView.cs index 4cd51ba85c10..3e3b6b9ac5d9 100644 --- a/src/BloomExe/Edit/EditingView.cs +++ b/src/BloomExe/Edit/EditingView.cs @@ -392,6 +392,24 @@ public void OnVisibleChanged(bool visible) /// public void OnHideEditTab() { + // Run the page frame's leaving-the-page teardown. Changing pages gets this via + // switchContentPage in workspaceRoot.ts, but leaving the tab does not unload or + // re-navigate the page frame, so nothing there fires and the page we are leaving keeps + // everything the editor had hung on it: the open toolbox tool with its observers and + // any window it had opened, the controls above the page, and the canvas-element + // machinery. Symptoms are a pop-up left on screen behind the new tab, and the toolbox + // staying switched off if the user left with Change Layout on. + // + // This used to happen for free: leaving the tab performs a save, and a save used to + // begin by stripping the live page. That coupling is what BL-13502 removed. + // + // Note we are called from the state machine's transition to NoPage, i.e. AFTER the page + // content has been captured and saved. That matters, because this changes the live + // page, and doing it earlier would put the teardown back into the save path. + _mainBrowser?.RunJavascriptFireAndForget( + "workspaceBundle.getEditablePageBundleExports()?.pageUnloading();" + ); + // Tells the model to prepare for possibly changing the current book, which // currently requires reloading the toolbox. _model.ClearBookForToolboxContent(); diff --git a/src/BloomExe/Edit/PageControlsApi.cs b/src/BloomExe/Edit/PageControlsApi.cs index d06fbe2106e4..a74e1c175996 100644 --- a/src/BloomExe/Edit/PageControlsApi.cs +++ b/src/BloomExe/Edit/PageControlsApi.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Diagnostics; using Bloom.Api; using Newtonsoft.Json; @@ -66,7 +66,7 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) kApiUrlPart + "duplicatePage", request => { - _editingModel.OnDuplicatePage(); + _editingModel.OnDuplicatePage(request.GetPageContentFromBrowserOrNull()); request.PostSucceeded(); }, true @@ -79,7 +79,7 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) request => { // The browser side has already confirmed with the user (BL-16421). - _editingModel.OnDeletePage(); + _editingModel.OnDeletePage(request.GetPageContentFromBrowserOrNull()); request.PostSucceeded(); }, true diff --git a/src/BloomExe/Edit/PageListController.cs b/src/BloomExe/Edit/PageListController.cs index 9b2be121eb49..8436ae3e8521 100644 --- a/src/BloomExe/Edit/PageListController.cs +++ b/src/BloomExe/Edit/PageListController.cs @@ -40,13 +40,23 @@ private void OnPageSelectedChanged(object page, EventArgs e) { if (page == null) return; - if (!_dontForwardSelectionEvent) - { - // The only necessary action after saving is to navigate to the desired page. - // This is achieved by returning the right ID in the trivial doAfterSaving function - // passed as the first argument to SaveThen. - _model.SaveThen(() => (page as Page).Id, () => { }); - } + if (_dontForwardSelectionEvent) + return; + + var pageId = (page as Page).Id; + + // The only necessary action after saving is to go to the desired page, which is what + // returning its ID from the first argument achieves. + // + // When the click brought the outgoing page's content with it, SaveThen saves and goes + // in one step, so we never enter SavePending -- the state in which a further page click + // would be silently discarded. When it didn't, SaveThen asks the browser as it always + // did. + _model.SaveThen( + () => pageId, + () => { }, + pageContentFromBrowser: (e as PageSelectedChangedEventArgs)?.PageContentFromBrowser + ); } public void SetBook(Book.Book book) //review: could do this instead by giving this class the bookselection object diff --git a/src/BloomExe/Edit/PageThumbnailList.cs b/src/BloomExe/Edit/PageThumbnailList.cs index 06eb94464aff..23df285d30fb 100644 --- a/src/BloomExe/Edit/PageThumbnailList.cs +++ b/src/BloomExe/Edit/PageThumbnailList.cs @@ -12,6 +12,21 @@ namespace Bloom.Edit { + /// + /// Carries the outgoing page's content along with a page-selection event, for the case where + /// the browser sent it with the click. Without it we would have to ask the browser for the + /// content and wait for the answer on another API before we could change pages. + /// + public class PageSelectedChangedEventArgs : EventArgs + { + public PageSelectedChangedEventArgs(string pageContentFromBrowser) + { + PageContentFromBrowser = pageContentFromBrowser; + } + + public string PageContentFromBrowser { get; } + } + /// /// Handle a list of page thumbnails (the left column in Edit mode) using an iframe configured by /// pageThumbnailList.pug to load the React component specified in pageThumbnailList.tsx. @@ -88,7 +103,7 @@ public PageThumbnailList() _baseHtml = ReactControl.ReplaceViteDevOrigin(_baseHtml); } - private void InvokePageSelectedChanged(IPage page) + private void InvokePageSelectedChanged(IPage page, string pageContentFromBrowser = null) { EventHandler handler = PageSelectedChanged; if ( @@ -97,7 +112,12 @@ private void InvokePageSelectedChanged(IPage page) page != null ) { - handler(page, null); + handler( + page, + pageContentFromBrowser == null + ? null + : new PageSelectedChangedEventArgs(pageContentFromBrowser) + ); } } @@ -201,10 +221,15 @@ private List UpdateItemsInternal(IEnumerable pages) return result.ToList(); } - internal void PageClicked(IPage page) + /// + /// The user clicked a page in the list. pageContentFromBrowser, when the page list managed + /// to collect it, is the current page's content, so we can save it without asking the + /// browser for it and waiting; null means fall back to that older route. + /// + internal void PageClicked(IPage page, string pageContentFromBrowser = null) { if (Enabled) - InvokePageSelectedChanged(page); + InvokePageSelectedChanged(page, pageContentFromBrowser); } /// @@ -241,7 +266,17 @@ internal bool IsContextMenuCommandEnabled(IPage page, string commandId) } } - internal void ExecuteContextMenuCommand(IPage page, string commandId) + /// + /// Run one of the thumbnail context menu's commands. pageContentFromBrowser, when the page + /// list was able to collect it, is the current page's content; the commands that have to + /// save the current page first can then do so without asking the browser for it and waiting + /// (see EditingModel.SavePageInPlaceThen). + /// + internal void ExecuteContextMenuCommand( + IPage page, + string commandId, + string pageContentFromBrowser = null + ) { if (!IsContextMenuCommandEnabled(page, commandId)) return; @@ -249,20 +284,20 @@ internal void ExecuteContextMenuCommand(IPage page, string commandId) switch (commandId) { case "duplicatePage": - Model.DuplicatePage(page); + Model.DuplicatePage(page, pageContentFromBrowser); break; case "duplicatePageManyTimes": Model.DuplicateManyPages(page); break; case "copyPage": - Model.CopyPage(page); + Model.CopyPage(page, pageContentFromBrowser); break; case "pastePage": - Model.PastePage(page); + Model.PastePage(page, pageContentFromBrowser); break; case "removePage": // The browser side has already confirmed with the user (BL-16421). - Model.DeletePage(page); + Model.DeletePage(page, pageContentFromBrowser); break; case "chooseDifferentLayout": Model.GetEditingBrowser().Focus(); @@ -277,7 +312,11 @@ internal void ExecuteContextMenuCommand(IPage page, string commandId) // This gets invoked by Javascript (via the PageListApi) when it determines that a particular page has been moved. // newIndex is the (zero-based) index that the page is moving to // in the whole list of pages, including the placeholder. - internal void PageMoved(IPage movedPage, int newPageIndex) + internal void PageMoved( + IPage movedPage, + int newPageIndex, + string pageContentFromBrowser = null + ) { // accounts for placeholder. // Enhance: may not be needed in single-column mode, if we ever restore that. @@ -303,7 +342,8 @@ internal void PageMoved(IPage movedPage, int newPageIndex) return movedPage.Id; }, () => { }, // wrong state, do nothing - forceFullSave: true + forceFullSave: true, + pageContentFromBrowser: pageContentFromBrowser ); } diff --git a/src/BloomExe/Edit/SavingWithoutReloading.md b/src/BloomExe/Edit/SavingWithoutReloading.md new file mode 100644 index 000000000000..400b8e9b221d --- /dev/null +++ b/src/BloomExe/Edit/SavingWithoutReloading.md @@ -0,0 +1,366 @@ +# Saving a page without reloading it — what it enables + +## A note on shape: this branch has to survive a long wait + +It will not merge for a while (it is too big a change to risk in the current release), so it is +written to be cheap to merge later rather than to be the most direct expression of each change. +Two rules follow from that, and they are worth keeping if you add to it: + +- **New behaviour goes in new files.** `pageContentDelays.ts`, `niceScrollCleanup.ts`, + `currentPageContent.ts`, `EditingStateMachine`'s new transitions, and the tests for all of them + are additions rather than edits. A new file cannot conflict with anything. +- **Don't reshape existing code to add to it.** What conflicts is a *changed* line, not an added + one — so an extra argument on a call beats hoisting its lambda into a named local, even when the + named local reads a little better on its own. That single choice took `EditingModel.cs` from 130 + changed lines to 37 and removed every reindentation. + +## What changed + +Historically, gathering the current page's content for a save **wrecked the live page**. The +browser stripped the editing markup out of the real DOM (detached the toolbox tool, unmounted the +above-page controls, removed the origami layout mode and text-box labels, killed the niceScroll +bars, and rewrote every `bloom-editable`'s `innerHTML` with CKEditor's cleaned-up data). The page +that was left could be saved but not edited, which is exactly what the `SavedAndStripped` state in +`EditingStateMachine` records, and why **every** save had to end by navigating to some page +(BL-13502). + +That is no longer true: + +- `getBodyContentForSavePage()` (`bookEdit/js/bloomEditing.ts`) now **clones** the body and does all + the stripping on the clone. **Nothing at all is done to the live page** — it is not touched, so + there is nothing to put back. +- Canvas-element editing is no longer turned off and on around the save. `turnOffCanvasElementEditing()` + did three things that affect what gets saved, and `CanvasElementManager.prepareCloneOfBodyForSave()` + now does all three against the clone: Comical's bubble-tail `` (via + `Comical.exportSvgToCopiesOfParents`, added in comicaljs 0.4.1 as the non-destructive counterpart of + `stopEditing()`), the canvas element positions recorded as the current language's alternate (pure + attribute manipulation, so a clone with no layout is fine), and the `bloom-focusedCanvasElement` + class. The rest of what that method does is live-only: the control frame is `bloom-ui` so C# + discards it anyway, `EnableAllImageEditing` only puts `bloom-ui` buttons back, and the listener + removal has no bearing on the HTML. +- CKEditor's cleaned-up text is read from the live editors and written into the clone + (`EditableDivUtils.copyCkEditorDataToClone`) rather than written back over the live editors. +- The scroll bars are cleaned off the clone by our own `removeNiceScrollArtifacts` + (`bookEdit/js/niceScrollCleanup.ts`) instead of by asking the live niceScroll instances to remove + themselves, so a save no longer disturbs the scroll bars the user is looking at. It handles the + inserted rails/cursors, the alignment classes `addScrollbarsToPage()` moved aside (the part that + would otherwise have been real data loss), and the three inline styles niceScroll sets without + recording. The live page still uses bloom-player's `cleanupNiceScroll()` at page setup, since + only that can tear down the instances themselves. +- `ITool` gained **one** new method, `removeToolMarkup(pageOrClone)`, which is used two ways rather + than duplicated: the save path calls it on a clone of the `.bloom-page` div, and + `ToolboxToolReactAdaptor.detachFromPage()` calls it on the live one. A tool with nothing + live-only to clean up implements only `removeToolMarkup` and gets both behaviours; a tool that + does (observers, React state, re-enabling image editing) overrides `detachFromPage` and calls + `super.detachFromPage()` at the point where the markup should come off. `detachCurrentTool()` + logs a console error if an override forgets that `super` call, because the symptom otherwise + shows up much later as tool markup saved into the book. +- We no longer blur the active element while saving, so the user's cursor stays where it was. + +On top of that: + +- `EditingStateMachine.ToSavedInPlace(pageContentData, reportFailure)` — a save that begins and ends + in `Editing`. No `SavePending` wait, no `SavedAndStripped`, no navigation. +- `EditingStateMachine.ToSavedInPlaceThenNavigating(pageContentData, doBeforeSaveToDisk, + reportFailure)` — the same thing for a request that also has to *change* something and then show + another page. `doBeforeSaveToDisk` plays exactly the role it plays in `ToSavePending`: it runs + after the browser's content is in the book DOM and before the book is written to disk, and + returns the page to go to. So the whole `SavePending → SavedAndStripped → Navigating` sequence + collapses into one `Editing → Navigating` step. +- **`EditingModel.SaveThen(..., pageContentFromBrowser)`** — the way in. Given the content it does + the whole save here and now (privately, via `SavePageInPlaceThen`); without it, or if we turn out + not to be in a state to save, it asks the browser exactly as it always did. So a caller opts in + by passing one more argument and needs to know nothing else: in particular it does not have to + know that only a `Declined` outcome may fall back, which is the rule that, got wrong, deletes a + page twice. Both routes reuse `UpdateBookDomFromBrowserPageContent()` and `SaveBookToDisk()`, so + they make exactly the same "just this page vs. full book save" decision. +- `EditingModel.SavePageInPlace(pageContentData)` — save and stay put, for the one caller that + wants no navigation at all (Copy Page). +- API `editView/savePageInPlace`, called by `savePageWithoutReloading()` in `bloomEditing.ts`. The + reply is not sent until the save has finished, so Javascript can `await` it. + +### What has been converted so far + +Everything the **page list frame** initiates. `collectCurrentPageContent()` +(`pageThumbnailList/currentPageContent.ts`) gathers the editable page's content — it can, because +`getEditablePageBundleExports()` reaches across frames — and every one of these sends it along with +its request: + +| Command | Was | Is now | +| --- | --- | --- | +| clicking a page thumbnail | `SaveThen` round trip, then navigate | same `SaveThen`, given the content | +| Duplicate Page (button and context menu) | `SaveThen` round trip, then duplicate, then navigate | ditto | +| Delete Page (button and context menu) | ditto | ditto | +| Paste Page (context menu) | ditto | ditto | +| dragging a page to a new position | ditto | ditto | +| Change Layout, import a video, convert a field to a derived one | `SaveThen` round trip, then reload the page | ditto — and they keep the reload, which is doing a second job for them (§1) | +| **Copy Page** (context menu) | `SaveThen` round trip **and a reload of the page being copied** | `SavePageInPlace` — no navigation at all | + +Copy Page is the first of these to lose its reload entirely: copying doesn't change the page you +are looking at, so with the content in hand there is nothing left to navigate to. The others still +navigate, because they are *going somewhere* (the new page, the next page, the moved page); what +they lose is the round trip, and with it the `SavePending` window in which a second command is +silently dropped. + +Not converted, because the request comes from a separate dialog window that cannot reach the page +frame: Add Page (`AddPageDialog`) and Duplicate Many Times (`duplicateManyDlgBundle`). Both still +use `SaveThen`, which is why it has to stay. + +Everything below is the inventory of what else could be converted, and what that would let us +delete. + +## Why the reload was expensive, not just ugly + +A save-then-reload costs a full page teardown and rebuild: regenerate the page DOM in C#, navigate +the browser, re-run `SetupElements` over every element, re-attach CKEditor to every editable, +re-run the toolbox's `newPageReady` for the current tool, re-measure and re-fit images, re-add +scroll bars. It also throws away everything transient: the cursor position and selection, the +active canvas element and its control frame, scroll position, the Play/Start tab a game page was +on, an in-progress audio playback. Almost every "flicker" complaint about the Edit tab traces back +to a save. + +--- + +## 1. Round trips that collapse into one call + +These are places where Javascript wants "make sure the book on disk is current, then do X". Today +each is: JS posts to an API → C# calls `SaveThen` → C# asks the browser for the content → the +browser answers on a *different* API → the state machine runs the pending action → C# navigates → +the page reloads. Four hops and a reload, to do something the browser could have asked for +directly. + +| Caller | Today | Could become | +| --- | --- | --- | +| ~~`origami.ts`, `bloomVideo.ts`, `canvasControlTextMenuItems.ts`~~ | — | **Done**: all three now call `saveChangesAndRethinkPage()` (`bloomEditing.ts`), which sends the content with the post. They **keep** the reload — see below; what went is the round trip. | +| `EditingViewApi` `editView/setTopic` → `SavePageAndReloadIt()` | Save + full reload to show a changed data-div value | Could carry the content too — the topic chooser runs in the workspace root, so it can reach the page frame. It would have to change from a plain post string to JSON, and it is also used from the Publish tab where there is no editable page at all (the collect just returns nothing and it falls back, which is fine). Small win, so not done yet. | +| `EditingModel.SavePageAndReloadIt` from `PageRefreshEvent` | Save + reload | Stays on `SaveThen`: these are raised inside C# (book settings, and `EditingModel` itself), so there is no browser request to carry the content. | + +### The three converted ones keep their reload, and that is right + +`common/saveChangesAndRethinkPageEvent` reads as "save this page, then show it again", and the +original reason for showing it again — restoring the UI markup the save stripped — is gone. But the +reload is doing a **second** job for these three callers, which is why they keep it: each has just +restructured the page into a state that has 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. This is the `customXmatterPage` lesson (below) applied +before making the mistake rather than after. + +So what they lost is the four-hop round trip, not the reload. Verified by driving Change Layout +mode on and off in a real book: `editView/pageContent` never fires, the page reloads and comes back +fully alive (CKEditor attached, canvas elements present), and text typed but not saved before the +toggle is in the file on disk afterwards. + +`postThatMightNavigate` itself exists (`utils/bloomApi.ts`) only because the post's own page is +about to be navigated out from under it, so the network error has to be swallowed. Calls that stop +navigating can use plain `post`/`postString` and get their errors reported again. + +### Before converting any of these: the reload may be doing a second job + +Check what the caller has just done to the live DOM, because a reload does not only recover from +the old destructive save — it also re-runs the page's whole setup (`SetupElements`, re-attaching +CKEditor to every editable, the toolbox's `newPageReady`, image sizing, scroll bars). A caller that +restructured the page may be relying on that without saying so. + +This is not hypothetical. `customXmatterPage.tsx` posts `editView/jumpToPage` with its **own** +page id — asking to "jump" to where it already is, purely to get a save — right after +`convertXmatterPageToCustom()` rebuilds the cover into canvas elements. Converting it to +`savePageWithoutReloading()` looked ideal on paper (it even fixes a real bug: that handler replies +before the save has happened, so the `await` does not mean what it appears to). But driven live, +the converted cover came back with CKEditor attached to **0 of its 12** editables, where the +standard cover has 9: `convertXmatterPageToCustom()` never attaches editors to the elements it +creates, and the reload had been quietly covering for that. Reverted. + +So: convert, then *drive the real UI* and check the page is still fully alive — editors attached, +tool markup present, images sized. Neither the unit tests nor the typecheck will tell you. + +### What the round trip actually costs — measured, before changing anything + +Do not do this work for speed. Measured on a running Bloom (7-page book, 25 KB page), driving real +thumbnail clicks and watching the API traffic from outside: +`.claude/skills/run-bloom/benchPageChange.mjs` and `benchSaveGather.mjs`. + +| Phase of a page change | median ms from click | +| --- | --- | +| `pageList/pageClicked` acked | 19 | +| `editView/pageContent` complete (old page saved, navigation kicked off) | 167 | +| new page's DOM loaded | 748 | +| new page **editable** | 793 | + +And separately: **gathering the page content in the browser takes 0.7 ms** (median of 15, on 25 KB +of HTML), while a *complete* direct save — `savePageWithoutReloading()`, i.e. gather + POST + merge ++ write to disk + reply — takes **92 ms**. + +So the round trip's whole purpose is to fetch something that costs 0.7 ms to produce. Its window +(19→167 ms) is at most ~56 ms more than doing the same save directly, and even that overstates it, +because the `editView/pageContent` handler also kicks off the navigation before it replies. The +genuinely removable part is the C#→WebView2 dispatch and scheduling: **30–50 ms out of ~790, i.e. +4–6%**. Roughly 80% of a page change is building and setting up the NEW page, which none of this +touches; deleting the save phase entirely would still cap the win at ~21%. The overhead is also +roughly constant while the disk write and page setup grow with the book, so it gets relatively +smaller on real books, not larger. + +The reason to make these changes is the simplification below — fewer hops, fewer states, fewer +things that can interleave — with a small speed bonus, not the other way round. + +Note the existing `PerformanceMeasurement.Measure("Select Page")` in `HandlePageClickedRequest` +cannot answer this: it wraps only the *initiation* (`SaveThen` returns as soon as the browser has +been asked), and it ignores nested measurements, so it cannot be subdivided either. + +## 2. The delay register — now the one gate, not a `requestPageContent` detail + +`addRequestPageContentDelay` / `removeRequestPageContentDelay` / +`wrapWithRequestPageContentDelay` exist because **C# picks the moment to capture the page**, so any +asynchronous DOM work in flight has to register itself and hold the capture off — with a 4-second +cap after which we capture anyway and warn. There are ~10 call sites (image sizing, canvas +background image fitting, clipboard paste, custom xmatter pages, the image gallery dialog…), plus a +rule in `src/BloomBrowserUI/AGENTS.md` telling reviewers to check for it. + +None of that can go while C# still initiates saves. What has changed is that a +**browser**-initiated save is just as capable of catching the page mid-change, and the first +version of this work did exactly that: `collectCurrentPageContent()` gathered synchronously, +straight past the register. A page click landing while an image was still being sized would have +written the half-sized page into the book. + +So the register moved out of `bloomEditing.ts` into its own module, +`bookEdit/js/pageContentDelays.ts`, and gained `whenNoActiveDelays()` — the single gate that +**every** route now waits on: + +| Route | Used by | +| --- | --- | +| `requestPageContent()` | the C#-initiated save; the reason the register exists | +| `getPageContentForSaveWhenReady()` | `savePageWithoutReloading()`, and the page list's commands via `collectCurrentPageContent()` | +| `captureContentForExternalProcessing()` | the off-screen book processor | + +The synchronous `getPageContentForSave()` is no longer exported from the module or across frames, +so there is no longer a way to gather the page without passing the gate. And because the page +list's commands await it, the *command* does not start either: C# is not asked to duplicate, +delete or reorder anything until the page has settled. `pageContentDelays.spec.ts` covers the +waiting, the release, the cap, and that a failed operation cannot leave the gate stuck shut. + +The gate also stopped polling. It used to be two mechanisms — a timeout that `requestPageContent` +armed and `removeRequestPageContentDelay` fired early, plus a separate 50ms poll loop in the +off-screen path. Now removing the last delay releases the waiters directly. + +Javascript-initiated saves could in principle just `await` their own async work instead of using +the register at all — but they cannot know about work someone *else* started, so they wait here +too. Each converted caller is still one fewer place that has to remember the rule. + +## 3. `SaveThen`'s awkward shape + +`SaveThen(doBeforeSaveToDisk, doIfNotInRightStateToSave, forceFullSave, skipSaveToDisk, +failureAction, doAfterSaveToDisk)` has six parameters, four of them callbacks, because the work has +to be chopped into pieces that run at different points of an asynchronous state machine. The +remark on it — *"if you are doing this in an API handler, remember that you must retrieve any data +in the request before calling SaveThen; the Request object can't be used inside +doBeforeSaveToDisk, since by then the request has been marked completed"* — is a direct symptom. + +There are 20 call sites. Several are pure "save, then go to this page": + +- `PageListController.cs:48` — `SaveThen(() => page.Id, () => { })` +- `EditingViewApi.cs:299` — `SaveThen(() => pageId, () => { })` +- `EditingModel.SavePageAndReloadIt` — `SaveThen(() => CurrentSelection.Id, () => { })` + +If the browser sends the page content **with** the request that needs a save, the handler no longer +has to be chopped up around an asynchronous wait. The shape the converted ones use: + +``` +// TS +postThatMightNavigate("edit/pageControls/duplicatePage", + await collectCurrentPageContent("the duplicate command")); +// C# +_editingModel.OnDuplicatePage(request.GetPageContentFromBrowserOrNull()); +// ...which ends up at SaveThen(..., pageContentFromBrowser: content) +``` + +For a handler that has no reason to navigate at all, `SavePageInPlace` is even plainer — save, +do the thing, reply — which is what removes the `doIfNotInRightStateToSave` callback (the handler +can just check the return value), the `doAfterSaveToDisk` callback, and the "don't touch the +request afterwards" hazard. + +Two of the six parameters are there for one caller each and would go away with them: +`skipSaveToDisk` (`collectionClosingEvent` and `OnTabAboutToChange`, which both want to do their own +`CurrentBook.Save()` before some postponed work) and `doAfterSaveToDisk` +(`WorkspaceView.cs:1742` and `CopyrightAndLicenseApi.cs`, which need up-to-date files on disk +before showing a blocking dialog). + +## 4. The websocket dance in the copyright dialog + +`EditingModel.NotifyCopyrightPushedToAllImages` exists, with an explanatory comment, purely because +*"we can't signal completion from the POST response itself, which returns as soon as the save is +initiated, well before the asynchronous post-save action runs."* With `editView/savePageInPlace` +the POST response **is** the completion signal, so this whole websocket event +(`kCopyrightWebSocketEventId_PushedToAllImages`, its sender, and its listener in +`CopyrightAndLicenseDialog.tsx`) can go once that flow is converted. + +## 5. State-machine surface + +If the C#-initiated save ever disappears entirely, these go with it: + +- the `SavePending` and `SavedAndStripped` states, and their `ToSavePending` / + `ToSavedAndStripped` transitions (two overloads); +- `DiscardInFlightSave()` and `_discardInFlightSave`, which exist only because a save can be in + flight for an unbounded time; +- `RequestBrowserToSave()` and the `editView/pageContent` API; +- `enableStateTransitions` — the tab strip is disabled during `SavePending`/`SavedAndStripped` + precisely because the page is unusable during them. An in-place save is synchronous; there is no + window to disable anything in. +- `NavigatingSoSuspendSaving`, and the various "a Save is still in progress, abort" guards such as + `EditingModel.cs:601`. + +That is a long way off — `OnTabAboutToChange` and `collectionClosingEvent` legitimately have to +start a save from C# — but each converted caller shrinks the surface. + +## 6. Smaller things + +- ~~**`getBodyContentForSavePage` is exported cross-frame but nothing calls it.**~~ Done: both it + and `userStylesheetContent` (whose comment still claimed it was *"Called from C# by a + RunJavaScript() in EditingView.CleanHtmlAndCopyToPageDom"*, a method that no longer exists) are + now private to `bloomEditing.ts`. +- **The off-screen book processor** (`BookProcessor.cs`) polls `window.__bloomExternalPageContent` + because there is no live `EditingModel` for the callback API. Now that content-gathering is + side-effect-free, `getPageContentForSave()` can be called directly and its value returned by + `RunJavascriptWithStringResult`, once the caller can wait for the `activeDelays` loop. (Not + urgent; the polling works.) +- **Thumbnail updates.** `SavePageInPlace` refreshes the current page's thumbnail because the + navigation that used to follow a save did it (`EditingView.StartNavigationToEditPage`). If saves + become frequent, that wants debouncing. + +--- + +## Risks to watch when converting callers + +- **niceScroll cleanup is our own code now.** `removeNiceScrollArtifacts` knows what niceScroll and + bloom-player's `addScrollbarsToPage()` leave behind rather than asking them to undo it, so a + change at either end could leave something in the saved page. `niceScrollCleanup.spec.ts` pins + the current expectations, and the module comment records where each item comes from. +- **comicaljs 0.4.1 is required**, for `Comical.exportSvgToCopiesOfParents`. Note that moving from + 0.3.106 to 0.4.x also surfaces four pre-existing type errors in `canvasElementManager/`: 0.3.106's + declarations import `from "bubbleSpec"` (a bare specifier TypeScript cannot resolve), so + `BubbleSpec` silently degraded to `any` in Bloom; 0.4.x emits correct relative imports and the real + types finally apply. They are unrelated to this work but must be fixed to pin 0.4.1. The most + interesting is `CanvasElementResizeAdjustments.ts:161`, `bubbleSpec.spec !== "none"` — `BubbleSpec` + has no `spec` member, so that comparison is always true and a Comical update is forced every time. +- **"We didn't save" and "we tried and failed" are different answers, and the difference is a + page.** `SavePageInPlaceThen` returns `InPlaceSaveOutcome`, and only `Declined` — which + guarantees `doBeforeSaveToDisk` never ran — permits falling back to asking the browser. This is + not hypothetical: the first version returned a plain bool, and when relocating a page threw part + way through, the caller read it as "not saved" and relocated the page a **second** time. + `EditingStateMachineTests` pins all three outcomes. That rule now lives in exactly one place, + inside `SaveThen`, which is the main reason `SavePageInPlaceThen` is private: no caller can get + it wrong because no caller has to know about it. +- **The action is allowed to navigate.** Under `SaveThen` it ran in `SavedAndStripped`, where + `ToNavigating` is legal; it now runs in `Editing`, where `ToNavigating` throws. Relocating a page + does navigate (`OnRelocatePage` refreshes the page whose side and number just changed), so + `_runningSaveInPlaceAction` relaxes that guard for the duration of the action — safely, because + by then the browser's content is already in the book DOM and there is nothing left to lose. Our + own navigation afterwards supersedes the action's, or is ignored when it is to the same page. +- **The context menu runs its command ~100ms after the click** (`HandleContextMenuItemClickedRequest` + defers it so the menu can close). The content we save is therefore gathered slightly *earlier* + than the old path gathered it — at click time rather than 100ms later. Nothing a user can type + into fits in that window, but it is a real difference. +- **Not blurring.** The old code blurred the active element before capturing. If any code relies on + a blur handler to normalize text before it is saved, that normalization no longer happens on save. + CKEditor's `getData()` gives us current text either way, so this is about side effects, not text. +- **`ui-audioCurrent`.** The Talking Book tool deliberately leaves its highlight class on the live + page (BL-15300), so it can reach the saved HTML; `BookData.cs:2091` already defends against that. + Unchanged by this work, but worth knowing when reading the clone-cleanup code. diff --git a/src/BloomExe/Edit/ToolboxView.cs b/src/BloomExe/Edit/ToolboxView.cs index 6ad4e5f388d1..965ae4a805a6 100644 --- a/src/BloomExe/Edit/ToolboxView.cs +++ b/src/BloomExe/Edit/ToolboxView.cs @@ -28,6 +28,12 @@ namespace Bloom.Edit /// ToolBox.registerTool(new MyWonderfulTool()); /// - should implement makeRootElement() to create one div, the react root. /// - the returned root should already have been passed to ReactDOM.render(). + /// - if the tool adds markup to the page for editing that should not be saved into the book, + /// implement removeToolMarkup(pageOrClone). That one method is used BOTH to clean the copy + /// we save (on every save, while the user keeps editing) and to clean the live page when + /// it goes away, so it must be pure DOM surgery inside the element it is handed. Put + /// live-only teardown in detachFromPage(), which must then call super.detachFromPage(). + /// See the comments on ITool in toolbox.ts. /// - Make a new xlf entry with ID EditTab.Toolbox.{UCToolId}.Heading, /// where UCToolId is the capitalized version of your tool Id, e.g., "Music". /// We currently assume the default English value of this will be UCToolId Tool, e.g., "Music Tool" diff --git a/src/BloomExe/web/PageListApi.cs b/src/BloomExe/web/PageListApi.cs index 043ec190579b..ccfaec5812c5 100644 --- a/src/BloomExe/web/PageListApi.cs +++ b/src/BloomExe/web/PageListApi.cs @@ -105,19 +105,30 @@ private void HandlePageClickedRequest(ApiRequest request) { var requestData = DynamicJson.Parse(request.RequiredPostJson()); string pageId = requestData.pageId; + // The page list sends the current page's content with the click when it can, so we can + // save it without asking the browser and waiting. It is absent when there is no page + // to collect from, or collecting threw; then we fall back to asking (see + // PageListController.OnPageSelectedChanged). + string pageContent = requestData.IsDefined("pageContent") + ? requestData.pageContent + : null; var shiftIsDown = (Control.ModifierKeys & Keys.Shift) == Keys.Shift; var label = shiftIsDown ? "Select Page (SHIFT)" : "Select Page"; - using (PerformanceMeasurement.Global?.Measure(label, requestData.detail ?? "")) + // Note this only measures getting the change under way; with the content in hand that + // is now most of the work, but the new page still has to be built and displayed. + using ( + PerformanceMeasurement.Global?.Measure( + label, + requestData.IsDefined("detail") ? requestData.detail : "" + ) + ) { - //using (PerformanceMeasurement.Global.Measure(label, requestData.detail ?? "")) - //{ IPage page = PageFromId(pageId); - //} if (page != null) - PageList.PageClicked(page); + PageList.PageClicked(page, pageContent); } request.PostSucceeded(); @@ -139,15 +150,36 @@ private void HandleContextMenuItemClickedRequest(ApiRequest request) var requestData = DynamicJson.Parse(request.RequiredPostJson()); string pageId = requestData.pageId; string commandId = requestData.commandId; + // See HandlePageClickedRequest: sent when the page list could collect it, so that the + // commands which save the current page first need not ask the browser and wait. + string pageContent = requestData.IsDefined("pageContent") + ? requestData.pageContent + : null; IPage page = PageFromId(pageId); if (page != null) { - // Execute the command asynchronously after a short delay - // The discard operator _ indicates we're intentionally not awaiting this + // The command must not run inline: "Duplicate Many Times" and "Choose Different + // Layout" open MODAL dialogs whose content this same server has to serve, and this + // handler holds the API sync lock until it returns. Running them here would + // deadlock. + // + // The short delay before queueing is deliberate, and is the easy thing to remove by + // mistake. Returning from this handler is not enough on its own: the server thread + // releases the sync lock a moment AFTER we return, while the UI thread is already + // free to pump whatever we queued -- so a dialog could ask for its content while + // the lock is still held. The delay makes that ordering certain rather than merely + // likely. (Removing it during BL-13502 is what brought this to light; the reason + // had never been written down.) + // + // The cost is a small window in which typing would miss the page snapshot that + // came with this request. That is a trade made knowingly: a lost keystroke is + // recoverable, a hung Bloom is not. + // + // The discard operator _ indicates we're intentionally not awaiting this. _ = Task.Run(async () => { - await Task.Delay(100); // 100ms delay to let the UI respond + await Task.Delay(100); // Execute on the UI thread using the form's synchronization context var form = Shell.GetShellOrOtherOpenForm(); @@ -158,7 +190,11 @@ private void HandleContextMenuItemClickedRequest(ApiRequest request) { try { - PageList.ExecuteContextMenuCommand(page, commandId); + PageList.ExecuteContextMenuCommand( + page, + commandId, + pageContent + ); } catch (Exception ex) { @@ -174,7 +210,6 @@ private void HandleContextMenuItemClickedRequest(ApiRequest request) }); } - // Return success immediately without waiting for the command to execute request.PostSucceeded(); } @@ -184,7 +219,11 @@ private void HandlePageMovedRequest(ApiRequest request) string newPageId = requestData.movedPageId; IPage movedPage = PageFromId(newPageId); int newIndex = Convert.ToInt32(requestData.newIndex); // Should come as int, but automatic JSON parsing doesn't know this - PageList.PageMoved(movedPage, newIndex); + // See HandlePageClickedRequest. + string pageContent = requestData.IsDefined("pageContent") + ? requestData.pageContent + : null; + PageList.PageMoved(movedPage, newIndex, pageContent); request.PostSucceeded(); } diff --git a/src/BloomExe/web/controllers/ApiRequest.cs b/src/BloomExe/web/controllers/ApiRequest.cs index f93efc788ec8..350edc1193c1 100644 --- a/src/BloomExe/web/controllers/ApiRequest.cs +++ b/src/BloomExe/web/controllers/ApiRequest.cs @@ -521,6 +521,20 @@ public string GetPostStringOrNull(bool unescape = true) return _requestInfo.GetPostString(unescape); } + /// + /// The current page's content, for a request whose whole body is that content because the + /// browser sent it along so we can save the page without asking for it and waiting (see + /// getPageContentForSaveWhenReady() in bloomEditing.ts and EditingModel.SavePageInPlaceThen). + /// Null if it was not sent, in which case the handler must fall back to SaveThen. + /// + /// Deliberately not unescaped: this is page HTML, and unescaping it would corrupt it. + /// + public string GetPageContentFromBrowserOrNull() + { + var content = GetPostStringOrNull(unescape: false); + return string.IsNullOrEmpty(content) ? null : content; + } + /// /// Get an enum value of type T that was passed as application/json /// diff --git a/src/BloomExe/web/controllers/EditingViewApi.cs b/src/BloomExe/web/controllers/EditingViewApi.cs index 0935efc49de0..45df60fdc7f1 100644 --- a/src/BloomExe/web/controllers/EditingViewApi.cs +++ b/src/BloomExe/web/controllers/EditingViewApi.cs @@ -53,6 +53,27 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) true, true // review. ); + // Save the current page from content the browser gathered on its own initiative, without + // reloading the page. Unlike editView/pageContent (which is the browser answering a save + // that C# started, and always ends in a navigation), this lets Javascript save whenever it + // needs the book on disk to be current and then simply carry on editing the same page. + // The reply is not sent until the save is finished, so Javascript can await it. + // + // It answers whether the save actually happened. It can decline -- the user may have + // started changing pages, or an external process may have replaced the book on disk -- + // and a caller that carries on regardless would be working from a file that does not + // say what it thinks it says. That is not hypothetical: the AI Image Editor saves so + // that the file matches the page it is about to read image sources from. + apiHandler.RegisterEndpointHandler( + "editView/savePageInPlace", + request => + { + var pageContentData = request.RequiredPostString(unescape: false); + request.ReplyWithBoolean(View.Model.SavePageInPlace(pageContentData)); + }, + true, // updates the book DOM, writes files, and refreshes the page list: UI thread + true + ); apiHandler.RegisterEndpointHandler("editView/setTopic", HandleSetTopic, true); apiHandler.RegisterEndpointHandler( "editView/isTextSelected", diff --git a/src/BloomTests/Edit/EditingStateMachineTests.cs b/src/BloomTests/Edit/EditingStateMachineTests.cs new file mode 100644 index 000000000000..f3c97860b26d --- /dev/null +++ b/src/BloomTests/Edit/EditingStateMachineTests.cs @@ -0,0 +1,589 @@ +using System; +using System.Collections.Generic; +using Bloom.Edit; +using NUnit.Framework; + +namespace BloomTests.Edit +{ + /// + /// Tests for EditingStateMachine.ToSavedInPlace, the transition that saves the current page + /// from content the browser gathered on its own initiative and stays in Editing (no stripped + /// page to recover from, so no navigation afterwards). See EditingModel.SavePageInPlace. + /// + [TestFixture] + public class EditingStateMachineTests + { + private List _navigatedTo; + private List _updatedWith; + private int _saveBookCount; + private int _pageSavesRequested; + private List _reportedFailures; + private EditingStateMachine _stateMachine; + + [SetUp] + public void Setup() + { + _navigatedTo = new List(); + _updatedWith = new List(); + _saveBookCount = 0; + _pageSavesRequested = 0; + _reportedFailures = new List(); + _stateMachine = new EditingStateMachine( + navigate: pageId => _navigatedTo.Add(pageId), + requestPageSave: _ => _pageSavesRequested++, + updateBookWithPageContents: (_, data) => _updatedWith.Add(data), + saveBook: () => _saveBookCount++, + hidePage: () => { }, + enableStateTransitions: _ => { } + ); + } + + private void GoToEditing(string pageId) + { + Assert.That( + _stateMachine.ToNavigating(pageId), + Is.True, + "test setup: should be able to start navigating" + ); + Assert.That( + _stateMachine.ToEditing(pageId), + Is.True, + "test setup: should be able to get to Editing" + ); + } + + private bool SaveInPlace(string content) + { + return _stateMachine.ToSavedInPlace(content, e => _reportedFailures.Add(e)); + } + + private InPlaceSaveOutcome SaveInPlaceThenGoTo(string content, string pageId) + { + return SaveInPlaceThenDoAndGoTo(content, () => pageId); + } + + private InPlaceSaveOutcome SaveInPlaceThenDoAndGoTo( + string content, + Func doBeforeSaveToDisk + ) + { + return _stateMachine.ToSavedInPlaceThenNavigating( + content, + doBeforeSaveToDisk, + e => _reportedFailures.Add(e) + ); + } + + [Test] + public void ToSavedInPlace_WhileEditing_UpdatesDomAndSavesWithoutNavigating() + { + GoToEditing("page1"); + _navigatedTo.Clear(); // the navigation that got us here is not what we're testing + + Assert.That(SaveInPlace("bodycss"), Is.True); + + Assert.That(_updatedWith, Is.EqualTo(new[] { "bodycss" })); + Assert.That(_saveBookCount, Is.EqualTo(1)); + Assert.That(_navigatedTo, Is.Empty, "an in-place save must not navigate"); + Assert.That(_reportedFailures, Is.Empty); + } + + [Test] + public void ToSavedInPlace_Twice_BothSaveBecauseWeStayInEditing() + { + GoToEditing("page1"); + + Assert.That(SaveInPlace("first"), Is.True); + Assert.That( + SaveInPlace("second"), + Is.True, + "the first in-place save should have left us in Editing" + ); + + Assert.That(_updatedWith, Is.EqualTo(new[] { "first", "second" })); + Assert.That(_saveBookCount, Is.EqualTo(2)); + } + + [Test] + public void ToSavedInPlace_WhileNavigating_DoesNothing() + { + Assert.That(_stateMachine.ToNavigating("page1"), Is.True); + + Assert.That(SaveInPlace("bodycss"), Is.False); + + Assert.That(_updatedWith, Is.Empty); + Assert.That(_saveBookCount, Is.EqualTo(0)); + Assert.That(_reportedFailures, Is.Empty, "not being ready to save is not a failure"); + } + + [Test] + public void ToSavedInPlace_WhileSavePending_DoesNothing() + { + GoToEditing("page1"); + Assert.That(_stateMachine.ToSavePending(() => "page1"), Is.True); + + Assert.That(SaveInPlace("bodycss"), Is.False); + + Assert.That(_updatedWith, Is.Empty); + Assert.That(_saveBookCount, Is.EqualTo(0)); + } + + [Test] + public void ToSavedInPlace_BrowserReportedError_ReportsAndSavesNothing() + { + GoToEditing("page1"); + _navigatedTo.Clear(); + + Assert.That(SaveInPlace("ERROR: something went wrong in the browser"), Is.False); + + Assert.That(_updatedWith, Is.Empty, "we must not put an error message in the book"); + Assert.That(_saveBookCount, Is.EqualTo(0)); + Assert.That(_reportedFailures.Count, Is.EqualTo(1)); + Assert.That( + _navigatedTo, + Is.Empty, + "we are still in Editing with a good page, so there is nothing to recover from" + ); + } + + [Test] + public void ToSavedInPlace_RepeatedFailureOnSamePage_ReportsOnlyOnce() + { + GoToEditing("page1"); + + SaveInPlace("ERROR: first try"); + SaveInPlace("ERROR: second try"); + + Assert.That( + _reportedFailures.Count, + Is.EqualTo(1), + "a page that always fails must not lock the user out with repeated dialogs" + ); + } + + [Test] + public void ToSavedInPlace_FailureOnDifferentPage_ReportsAgain() + { + GoToEditing("page1"); + SaveInPlace("ERROR: first page"); + Assert.That(_reportedFailures.Count, Is.EqualTo(1), "test setup"); + + // The only way out of Editing is through a save, so use the ordinary + // request-content-from-the-browser route to get to another page. + Assert.That(_stateMachine.ToSavePending(() => "page2"), Is.True, "test setup"); + Assert.That(_stateMachine.ToSavedAndStripped((string)null), Is.True, "test setup"); + Assert.That(_stateMachine.ToEditing("page2"), Is.True, "test setup"); + + SaveInPlace("ERROR: second page"); + + Assert.That(_reportedFailures.Count, Is.EqualTo(2)); + } + + [Test] + public void ToSavedInPlace_AfterFailingThenSucceeding_ReportsAgainIfItFailsAgain() + { + GoToEditing("page1"); + SaveInPlace("ERROR: first try"); + Assert.That(_reportedFailures.Count, Is.EqualTo(1), "test setup"); + + Assert.That(SaveInPlace("good content"), Is.True); + SaveInPlace("ERROR: later try"); + + Assert.That( + _reportedFailures.Count, + Is.EqualTo(2), + "a successful save should clear the 'already reported' memory" + ); + } + + // ToSavedInPlaceThenNavigating: what a page click does when the click brought the outgoing + // page's content with it. See EditingModel.SaveThen's pageContentFromBrowser. + + [Test] + public void ToSavedInPlaceThenNavigating_WhileEditing_SavesThenGoesToTheOtherPage() + { + GoToEditing("page1"); + _navigatedTo.Clear(); + + Assert.That( + SaveInPlaceThenGoTo("bodycss", "page2"), + Is.EqualTo(InPlaceSaveOutcome.Saved) + ); + + Assert.That(_updatedWith, Is.EqualTo(new[] { "bodycss" })); + Assert.That(_saveBookCount, Is.EqualTo(1)); + Assert.That( + _navigatedTo, + Is.EqualTo(new[] { "page2" }), + "should have gone to the clicked page, in the same step" + ); + Assert.That(_reportedFailures, Is.Empty); + } + + [Test] + public void ToSavedInPlaceThenNavigating_NeverEntersSavePending() + { + GoToEditing("page1"); + + Assert.That( + SaveInPlaceThenGoTo("content", "page2"), + Is.EqualTo(InPlaceSaveOutcome.Saved) + ); + + Assert.That( + _stateMachine.SavePending, + Is.False, + "the whole point is that we never wait on the browser, so we never sit in SavePending" + ); + Assert.That(_stateMachine.Navigating, Is.True); + } + + [Test] + public void ToSavedInPlaceThenNavigating_LandsInAStateThatCanAcceptTheNextPageClick() + { + // The bug this avoids: while in SavePending, a further page click is silently dropped. + GoToEditing("page1"); + Assert.That( + SaveInPlaceThenGoTo("content", "page2"), + Is.EqualTo(InPlaceSaveOutcome.Saved) + ); + + // Finish arriving, then click again, as an impatient user would. + Assert.That(_stateMachine.ToEditing("page2"), Is.True); + _navigatedTo.Clear(); + + Assert.That( + SaveInPlaceThenGoTo("more content", "page3"), + Is.EqualTo(InPlaceSaveOutcome.Saved) + ); + Assert.That(_navigatedTo, Is.EqualTo(new[] { "page3" })); + } + + [Test] + public void ToSavedInPlaceThenNavigating_WhileNavigating_DoesNothing() + { + Assert.That(_stateMachine.ToNavigating("page1"), Is.True); + _navigatedTo.Clear(); + + Assert.That( + SaveInPlaceThenGoTo("content", "page2"), + Is.EqualTo(InPlaceSaveOutcome.Declined) + ); + + Assert.That(_updatedWith, Is.Empty); + Assert.That(_saveBookCount, Is.EqualTo(0)); + Assert.That(_navigatedTo, Is.Empty); + } + + [Test] + public void ToSavedInPlaceThenNavigating_FromNoPage_JustGoesThere() + { + // Nothing to save, but the click still means "show me that page". + Assert.That( + SaveInPlaceThenGoTo("content", "page2"), + Is.EqualTo(InPlaceSaveOutcome.Saved) + ); + + Assert.That(_updatedWith, Is.Empty, "there was no page to save"); + Assert.That(_navigatedTo, Is.EqualTo(new[] { "page2" })); + } + + [Test] + public void ToSavedInPlaceThenNavigating_FromNoPage_StillWritesWhatTheActionDid() + { + // There is no browser content to merge here, but the action can still change the book + // -- deleting a page, say -- and that has to reach disk. The request-the-content path + // (ToSavePending -> DoPostSaveAction) saves in this case, so this must too. + Assert.That( + SaveInPlaceThenDoAndGoTo("content", () => "page2"), + Is.EqualTo(InPlaceSaveOutcome.Saved) + ); + + Assert.That( + _saveBookCount, + Is.EqualTo(1), + "whatever the action changed must still be written to disk" + ); + } + + [Test] + public void ToSavedInPlaceThenNavigating_SaveFails_ReportsAndStaysPut() + { + GoToEditing("page1"); + _navigatedTo.Clear(); + + Assert.That( + SaveInPlaceThenGoTo("ERROR: the browser could not gather it", "page2"), + Is.EqualTo(InPlaceSaveOutcome.Failed), + "Failed, not Declined: the caller must not fall back and run the action again" + ); + + Assert.That(_saveBookCount, Is.EqualTo(0)); + Assert.That(_reportedFailures.Count, Is.EqualTo(1)); + Assert.That( + _navigatedTo, + Is.Empty, + "going on to the clicked page would silently discard the edits we failed to save" + ); + } + + // The doBeforeSaveToDisk form: what duplicate/delete/paste/move page do, now that the page + // list sends the current page's content with the command. The action has to see the user's + // latest edits (so it must run AFTER the browser's content goes into the book DOM) and its + // work has to reach disk (so it must run BEFORE the book is written). + // See EditingModel.SavePageInPlaceThen. + + [Test] + public void ToSavedInPlaceThenNavigating_RunsTheActionBetweenTheDomUpdateAndTheDiskSave() + { + GoToEditing("page1"); + _navigatedTo.Clear(); + var domUpdatesWhenActionRan = -1; + var saveBookCountWhenActionRan = -1; + + var result = SaveInPlaceThenDoAndGoTo( + "bodycss", + () => + { + domUpdatesWhenActionRan = _updatedWith.Count; + saveBookCountWhenActionRan = _saveBookCount; + return "theDuplicatedPage"; + } + ); + + Assert.That(result, Is.EqualTo(InPlaceSaveOutcome.Saved)); + Assert.That( + domUpdatesWhenActionRan, + Is.EqualTo(1), + "the action must see the edits the browser just sent us" + ); + Assert.That( + saveBookCountWhenActionRan, + Is.EqualTo(0), + "the action must run before the disk save, so what it does gets written too" + ); + Assert.That(_saveBookCount, Is.EqualTo(1), "and the disk save must still happen"); + Assert.That(_navigatedTo, Is.EqualTo(new[] { "theDuplicatedPage" })); + } + + [Test] + public void ToSavedInPlaceThenNavigating_WrongState_DoesNotRunTheAction() + { + Assert.That(_stateMachine.ToNavigating("page1"), Is.True, "test setup"); + var actionRan = false; + + var result = SaveInPlaceThenDoAndGoTo( + "content", + () => + { + actionRan = true; + return "page2"; + } + ); + + Assert.That(result, Is.EqualTo(InPlaceSaveOutcome.Declined)); + Assert.That( + actionRan, + Is.False, + "the caller falls back to SaveThen when we Decline, so the action must not have " + + "happened already -- it would then happen twice" + ); + } + + [Test] + public void ToSavedInPlaceThenNavigating_SaveFails_DoesNotRunTheAction() + { + GoToEditing("page1"); + var actionRan = false; + + var result = SaveInPlaceThenDoAndGoTo( + "ERROR: the browser could not gather it", + () => + { + actionRan = true; + return "page2"; + } + ); + + Assert.That( + result, + Is.EqualTo(InPlaceSaveOutcome.Failed), + "Failed, not Declined -- see the next test for why the difference matters" + ); + Assert.That( + actionRan, + Is.False, + "deleting or duplicating a page we failed to save would act on stale content" + ); + } + + [Test] + public void ToSavedInPlaceThenNavigating_ActionThrows_ReportsFailedSoTheCallerWillNotRetry() + { + // Found live: relocating a page threw part way through, the caller read the result as + // "not saved, fall back to SaveThen", and the page got relocated a SECOND time. An + // action that has already changed the book must never be offered to the fallback. + GoToEditing("page1"); + var timesActionRan = 0; + + var result = SaveInPlaceThenDoAndGoTo( + "good content", + () => + { + timesActionRan++; + throw new ApplicationException("the action blew up after changing the book"); + } + ); + + Assert.That(timesActionRan, Is.EqualTo(1), "test setup: the action should have run"); + Assert.That( + result, + Is.EqualTo(InPlaceSaveOutcome.Failed), + "Declined here would invite the caller to run the action a second time" + ); + Assert.That(_reportedFailures.Count, Is.EqualTo(1)); + } + + [Test] + public void ToSavedInPlaceThenNavigating_ActionNavigatesToTheSamePage_DoesNotNavigateTwice() + { + // Found live: relocating a page raises RelocatePageEvent, and OnRelocatePage refreshes + // the display of the page whose HTML just changed -- i.e. the action navigates. That + // used to throw "Cannot navigate while editing", because unlike the old SaveThen flow + // (which ran the action in SavedAndStripped) we are still in Editing. It is safe here: + // the browser's content is already in the book DOM, so there is nothing left to lose. + GoToEditing("page1"); + _navigatedTo.Clear(); + + var result = SaveInPlaceThenDoAndGoTo( + "good content", + () => + { + _stateMachine.ToNavigating("theMovedPage"); + return "theMovedPage"; + } + ); + + Assert.That(result, Is.EqualTo(InPlaceSaveOutcome.Saved)); + Assert.That(_reportedFailures, Is.Empty, "an action that navigates is legal here"); + Assert.That(_saveBookCount, Is.EqualTo(1)); + Assert.That( + _navigatedTo, + Is.EqualTo(new[] { "theMovedPage" }), + "the action's navigation and ours are to the same page, so it should happen once" + ); + } + + [Test] + public void ToSavedInPlaceThenNavigating_ActionNavigatesElsewhere_OurTargetWins() + { + GoToEditing("page1"); + _navigatedTo.Clear(); + + var result = SaveInPlaceThenDoAndGoTo( + "good content", + () => + { + _stateMachine.ToNavigating("somewhereTheActionWanted"); + return "whereWeSaidToGo"; + } + ); + + Assert.That(result, Is.EqualTo(InPlaceSaveOutcome.Saved)); + Assert.That( + _navigatedTo, + Is.EqualTo(new[] { "somewhereTheActionWanted", "whereWeSaidToGo" }), + "the page the action named is where we must end up" + ); + } + + [Test] + public void ToSavedInPlaceThenNavigating_ActionAsksForAnotherSave_IgnoresItAndStillNavigates() + { + // An action is allowed to do things that would normally start a save -- changing the + // page selection does, via PageListController.OnPageSelectedChanged. There is nothing + // for that save to do (we have already merged the content and are about to write the + // book), and accepting it would strand us in SavePending, from which the navigation + // this method promises is silently dropped. The old flow got this for free by running + // the action in SavedAndStripped. + GoToEditing("page1"); + _navigatedTo.Clear(); + var nestedSaveAccepted = true; + + var result = SaveInPlaceThenDoAndGoTo( + "good content", + () => + { + nestedSaveAccepted = _stateMachine.ToSavePending(() => + "pageTheNestedSaveWanted" + ); + return "whereWeSaidToGo"; + } + ); + + Assert.That( + nestedSaveAccepted, + Is.False, + "a save requested from inside the action should be refused" + ); + Assert.That( + _pageSavesRequested, + Is.EqualTo(0), + "and it must not have asked the browser for the page again" + ); + Assert.That(result, Is.EqualTo(InPlaceSaveOutcome.Saved)); + Assert.That(_saveBookCount, Is.EqualTo(1), "the book is written exactly once"); + Assert.That( + _navigatedTo, + Is.EqualTo(new[] { "whereWeSaidToGo" }), + "the page we promised to go to must still be shown" + ); + } + + [Test] + public void ToSavedInPlaceThenNavigating_ActionReturnsNull_LeavesTheEditorBlank() + { + // SaveThen's contract: returning null from the action means "show a blank screen", + // which is how leaving the edit tab saves. DoPostSaveAction honours it, so this must + // too -- trying to navigate to no page would leave a broken editor. + GoToEditing("page1"); + _navigatedTo.Clear(); + + var result = SaveInPlaceThenDoAndGoTo("good content", () => null); + + Assert.That(result, Is.EqualTo(InPlaceSaveOutcome.Saved)); + Assert.That(_saveBookCount, Is.EqualTo(1), "it must still write the book"); + Assert.That(_navigatedTo, Is.Empty, "there is no page to go to"); + Assert.That( + _stateMachine.ToNavigating("page2"), + Is.True, + "we should be in NoPage, from which navigating is allowed again" + ); + } + + [Test] + public void ToNoPage_WhileEditingAndNoSaveInPlaceUnderWay_StillThrows() + { + // As with ToNavigating, the relaxation must be scoped to the action. + GoToEditing("page1"); + + Assert.Throws( + () => _stateMachine.ToNoPage(), + "emptying an unsaved page must still be refused" + ); + } + + [Test] + public void ToNavigating_WhileEditingAndNoSaveInPlaceUnderWay_StillThrows() + { + // The relaxation above must be scoped to the action; the ordinary guard against + // leaving a page with unsaved edits has to stay. + GoToEditing("page1"); + + Assert.Throws( + () => _stateMachine.ToNavigating("page2"), + "navigating away from an unsaved page must still be refused" + ); + } + } +}