From 6a486afd2f7ae658278ad5acea0a4dd6c8de274a Mon Sep 17 00:00:00 2001 From: John Thomson Date: Fri, 9 Oct 2026 15:39:02 -0500 Subject: [PATCH] Save a page without reloading it; save from a snapshot the browser volunteers (BL-13502) Saving used to strip the editing markup out of the live page (CKEditor, the toolbox tool, the canvas-element machinery), so every save had to end by reloading the page, and C# had to ask the browser for the page and wait for the answer on another API, which is why everything that saved first was split around an asynchronous gap with two extra editing states to wait in. - The page is gathered from a clone of the .bloom-page element and the live page is never touched, so a save leaves the user where they were, still editing. Each toolbox tool takes its markup off whichever element it is given (the clone, or the live page when it is detached) through one method, removeToolMarkup. Editor chrome is stripped in the browser only. Bloom Tables (removeTableEditingMarkupFromClone) and Comical's drawing surfaces are cleaned on the copy too. - The browser volunteers the page: a MutationObserver posts the gathered page to editView/pageSnapshot 25 ms after each change settles, and once as it loads. C# stores it per page load (PageSnapshot) and every save uses it synchronously. No request carries the page; the commands that restructure the page and reload it (Change Layout, importing a video, unlocking and relocking the original-copyright sentence) send the snapshot at once first (saveChangesAndRethinkPage, postAfterSendingSnapshot). - Asynchronous work that belongs in the page registers in the delay register; every gather waits for it, and the browser tells C# when the page is busy, so a save waits briefly for the snapshot of the finished page. Snapshots and busy notices go one at a time, in order; failures are reported, not retried. - The SavePending / SavedAndStripped states, the ask-the-browser API and the shutdown cancel-and-reissue are gone. Leaving the Edit tab and closing the collection are straight-line code. MergeCurrentPageThenSave and SaveCurrentPageAndBook make the same what-to-write decision, and C# compares the merged page with the book, ignoring attribute order (SafeXmlNode.GetXmlIgnoringAttributeOrder), so an untouched page writes nothing. - Things that made untouched pages look edited are fixed: qTip attributes, Comical's regenerated SVG ids, a text-measuring scratch element, attribute order, the cover's editing-only ?transparent=yes parameter, missing-image alt text on optional branding images, a stray space when BookData merges classes, and the original-copyright sentence compared before being locked again. - The page list's Duplicate Many Times and Choose Different Layout dialogs open in the browser, so context-menu commands run inside their request, in order. - The Talking Book highlight leaves long runs of spaces unpainted itself, instead of changing the DOM. - Requires bloom-player 2.20.3-alpha.1 (bloom-player#441), so a save leaves a game being played alone and writes the authored positions. - Xml_DoesNotProvide_ThreadSafety no longer depends on luck, and CI runs it. - E2E tests: typing survives every way of leaving a page, quitting included; looking at pages does not rewrite the book; Change Layout keeps layout and typing; no editor leftovers in the saved book; Duplicate Page Many Times. Master's table and original-copyright specs adjusted to the new saving. src/BloomExe/Edit/SavingWithoutReloading.md explains the design and the measurements; docs/save-without-reload/HANDOFF.md is for continuing the work elsewhere and goes before merging. Co-Authored-By: Claude Opus 5.5 Co-Authored-By: Claude Opus 5 Co-Authored-By: Claude Opus 5 (1M context) Co-Authored-By: Claude Fable 5.1 --- .../run-bloom/ai-image-editor-driving.md | 21 +- .prettierignore | 5 + .../localization/en/BloomLowPriority.xlf | 5 + docs/save-without-reload/HANDOFF.md | 133 ++ .../bookEdit/StyleEditor/StyleEditor.ts | 10 + .../bookEdit/aiImageEditor/AGENTS.md | 12 +- .../aiImageEditorOverlay.test.ts | 12 +- .../bookEdit/duplicateManyDialog.entry.tsx | 4 - .../bookEdit/duplicateManyDialog.tsx | 19 +- src/BloomBrowserUI/bookEdit/editablePage.ts | 75 +- .../bookEdit/js/AbovePageControls.tsx | 2 +- .../bookEdit/js/bloomEditing.ts | 393 +++--- src/BloomBrowserUI/bookEdit/js/bloomImages.ts | 20 +- .../bookEdit/js/bloomImagesSpec.ts | 52 + src/BloomBrowserUI/bookEdit/js/bloomVideo.ts | 4 +- .../CanvasElementAlternates.test.ts | 76 ++ .../CanvasElementAlternates.ts | 18 +- ...anvasElementBackgroundImageManager.test.ts | 6 +- .../CanvasElementBackgroundImageManager.ts | 5 +- .../CanvasElementBubbleLevelUtils.ts | 3 +- .../CanvasElementClipboard.test.ts | 9 +- .../CanvasElementClipboard.ts | 2 +- .../CanvasElementFactories.ts | 3 +- .../CanvasElementManager.ts | 52 +- .../CanvasElementResizeAdjustments.test.ts | 52 + .../CanvasElementResizeAdjustments.ts | 19 +- .../bookEdit/js/editableDivUtils.ts | 52 +- .../bookEdit/js/editableDivUtilsSpec.ts | 72 +- .../bookEdit/js/editorChromeCleanup.spec.ts | 144 ++ .../bookEdit/js/editorChromeCleanup.ts | 59 + .../bookEdit/js/imageTargetResolution.ts | 78 +- .../bookEdit/js/niceScrollCleanup.spec.ts | 136 ++ .../bookEdit/js/niceScrollCleanup.ts | 88 ++ src/BloomBrowserUI/bookEdit/js/origami.ts | 5 +- .../js/pageContentCapturePolicy.test.ts | 2 +- .../bookEdit/js/pageContentDelays.spec.ts | 195 +++ .../bookEdit/js/pageContentDelays.ts | 128 ++ .../bookEdit/js/pageSnapshot.spec.ts | 662 ++++++++++ .../bookEdit/js/pageSnapshot.ts | 327 +++++ .../bookEdit/js/tableEditing.test.ts | 48 +- .../bookEdit/js/tableEditing.ts | 50 +- .../pageControls/pageControls.tsx | 15 +- .../pageThumbnailList/pageThumbnailList.tsx | 66 +- .../textContextMenu/TextContextMenu.tsx | 7 +- .../bookEdit/textContextMenu/noIndent.ts | 4 +- .../canvas/canvasControlTextMenuItems.ts | 9 +- .../bookEdit/toolbox/canvas/canvasTool.tsx | 1 + .../toolbox/canvas/customXmatterPage.tsx | 6 +- .../bookEdit/toolbox/games/GameTool.tsx | 11 +- .../games/undoPrepareActivityContract.spec.ts | 110 ++ .../imageDescription/imageDescription.tsx | 19 +- .../imageDescription/imageDescriptionUtils.ts | 12 +- .../impairmentVisualizer.tsx | 21 +- .../bookEdit/toolbox/motion/motionTool.tsx | 18 +- .../decodableReader/decodableReaderTool.tsx | 8 + .../leveledReader/leveledReaderTool.tsx | 8 + .../readers/removeReaderMarkup.spec.ts | 73 ++ .../toolbox/readers/removeReaderMarkup.ts | 22 + .../toolbox/signLanguage/signLanguageTool.tsx | 1 + .../talkingBook/audioHighlightManager.ts | 66 +- .../toolbox/talkingBook/audioRecording.ts | 235 +--- .../toolbox/talkingBook/audioRecordingSpec.ts | 311 ++--- .../toolbox/talkingBook/talkingBookTool.tsx | 22 +- .../bookEdit/toolbox/toolbox.ts | 47 +- .../bookEdit/toolbox/toolboxBootstrap.ts | 9 +- .../bookEdit/toolbox/toolboxGlobals.d.ts | 1 + .../toolbox/toolboxToolReactAdaptor.tsx | 24 +- src/BloomBrowserUI/bookEdit/workspaceRoot.ts | 10 + .../pageChooser/PageChooserDialog.tsx | 58 +- .../react_components/numberChooserDialog.tsx | 15 +- src/BloomBrowserUI/utils/bloomApi.ts | 14 + src/BloomBrowserUI/vite.config.mts | 1 - src/BloomE2E/AUTOMATION-DEBT.md | 9 + src/BloomE2E/fixtures/bloomTest.ts | 21 + src/BloomE2E/fixtures/launchBloom.ts | 141 ++ src/BloomE2E/helpers/bookHtml.ts | 111 ++ src/BloomE2E/helpers/bookMaking.ts | 66 + src/BloomE2E/helpers/origami.ts | 26 +- src/BloomE2E/helpers/pageList.ts | 100 +- src/BloomE2E/helpers/pageThumbnails.ts | 73 +- .../tests/change-layout-keeps-typing.spec.ts | 81 ++ src/BloomE2E/tests/copy-page.spec.ts | 8 +- .../tests/duplicate-page-many-times.spec.ts | 80 ++ src/BloomE2E/tests/duplicate-page.spec.ts | 6 +- .../tests/original-copyright-sentence.spec.ts | 4 + .../tests/saved-book-stays-clean.spec.ts | 169 +++ src/BloomE2E/tests/tables-extended.spec.ts | 11 +- .../typing-survives-leaving-the-page.spec.ts | 160 +++ src/BloomExe/Book/Book.cs | 86 +- src/BloomExe/Book/BookData.cs | 13 +- src/BloomExe/Book/BookProcessor.cs | 13 +- src/BloomExe/Book/BookStorage.cs | 11 +- src/BloomExe/Book/HtmlDom.cs | 56 +- src/BloomExe/Edit/EditingModel.cs | 1155 +++++++++-------- src/BloomExe/Edit/EditingStateMachine.cs | 589 +++------ src/BloomExe/Edit/EditingView.cs | 23 +- src/BloomExe/Edit/PageListController.cs | 20 +- src/BloomExe/Edit/PageSnapshot.cs | 182 +++ src/BloomExe/Edit/PageThumbnailList.cs | 40 +- src/BloomExe/Edit/SavingWithoutReloading.md | 572 ++++++++ src/BloomExe/Edit/ToolboxView.cs | 6 + src/BloomExe/Event.cs | 65 +- src/BloomExe/SafeXml/SafeXmlNode.cs | 41 + src/BloomExe/Shell.cs | 165 ++- .../TeamCollection/TeamCollectionManager.cs | 126 +- src/BloomExe/Workspace/WorkspaceView.cs | 19 +- src/BloomExe/web/PageListApi.cs | 58 +- src/BloomExe/web/ReactControl.cs | 1 - .../web/controllers/AddOrChangePageApi.cs | 61 +- .../web/controllers/AiImageEditorApi.cs | 88 +- .../web/controllers/CopyrightAndLicenseApi.cs | 2 +- src/BloomExe/web/controllers/E2eTestingApi.cs | 16 + .../web/controllers/EditingViewApi.cs | 145 ++- .../web/controllers/PageTemplatesApi.cs | 7 - .../web/controllers/SignLanguageApi.cs | 67 +- src/BloomTests/Book/BookDataTests.cs | 34 + src/BloomTests/Book/BookTests.cs | 223 +++- src/BloomTests/Book/HtmlDomTests.cs | 40 - src/BloomTests/Edit/EditingModelTests.cs | 105 ++ .../Edit/EditingStateMachineTests.cs | 649 ++++++--- src/BloomTests/Edit/PageSnapshotTests.cs | 231 ++++ src/BloomTests/SafeXml/SafeXmlTests.cs | 118 +- 122 files changed, 7669 insertions(+), 2706 deletions(-) create mode 100644 docs/save-without-reload/HANDOFF.md delete mode 100644 src/BloomBrowserUI/bookEdit/duplicateManyDialog.entry.tsx create mode 100644 src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementAlternates.test.ts create mode 100644 src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementResizeAdjustments.test.ts create mode 100644 src/BloomBrowserUI/bookEdit/js/editorChromeCleanup.spec.ts create mode 100644 src/BloomBrowserUI/bookEdit/js/editorChromeCleanup.ts 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/js/pageSnapshot.spec.ts create mode 100644 src/BloomBrowserUI/bookEdit/js/pageSnapshot.ts create mode 100644 src/BloomBrowserUI/bookEdit/toolbox/games/undoPrepareActivityContract.spec.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/BloomE2E/tests/change-layout-keeps-typing.spec.ts create mode 100644 src/BloomE2E/tests/duplicate-page-many-times.spec.ts create mode 100644 src/BloomE2E/tests/saved-book-stays-clean.spec.ts create mode 100644 src/BloomE2E/tests/typing-survives-leaving-the-page.spec.ts create mode 100644 src/BloomExe/Edit/PageSnapshot.cs create mode 100644 src/BloomExe/Edit/SavingWithoutReloading.md create mode 100644 src/BloomTests/Edit/PageSnapshotTests.cs diff --git a/.claude/skills/run-bloom/ai-image-editor-driving.md b/.claude/skills/run-bloom/ai-image-editor-driving.md index 9aa5bb9248aa..64d4fc908006 100644 --- a/.claude/skills/run-bloom/ai-image-editor-driving.md +++ b/.claude/skills/run-bloom/ai-image-editor-driving.md @@ -62,15 +62,20 @@ Right-click the canvas element → **"Edit with AI…"**. Loading the iframe dir `aiImageEditorOverlay.ts`'s postMessage handler, so the commit + current-page save path wouldn't run. **The overlay does not appear on the same tick as the click.** The menu command posts -`aiImageEditor/saveThenLaunch`, which makes C# save the current page — reloading the `page` -frame — and then, *once the reloaded page reports back*, call +`aiImageEditor/saveThenLaunch`, which makes C# save the current page and then call `workspaceBundle.openAiImageEditor(...)` in the shell (BL-16682; the saved book DOM has to be -current or the editor opens with an empty "Image to Edit" slot). So the wait spans a whole page -load: **wait for the overlay**, e.g. `await page.waitForSelector("#ai-image-editor-overlay iframe")`, -and don't re-use any content-frame handle taken before the click — that frame is gone. - -Note also that the overlay itself belongs to the **shell**, not the `page` frame, so it survives -page reloads: a commit saves the page immediately and the ✕ keeps working across that reload. +current or the editor opens with an empty "Image to Edit" slot). **Wait for the overlay**, e.g. +`await page.waitForSelector("#ai-image-editor-overlay iframe")`. + +That save used to end in a navigation, so the wait spanned a whole page load and any content-frame +handle taken before the click was stale. Since BL-13502 a save does not navigate: the page frame +stays put and its handles stay good. Waiting for the overlay is still the right thing — the save +and the round trip take a moment — but the page is no longer replaced underneath you. (Continuing +to wait for a page load that no longer happens is precisely what once left the editor never +opening; see the comment on `HandleSaveThenLaunch`.) + +Note also that the overlay itself belongs to the **shell**, not the `page` frame, so it survives a +reload when one does happen. (It does *not* survive a reload of the whole workspace root, which `EditingView.StartNavigationToEditPage` does once `MemoryUtils.SystemIsShortOfMemory()` — Bloom's own private bytes past ~2GB, i.e. a long session on a big book. That closes the overlay like any diff --git a/.prettierignore b/.prettierignore index 97b97fe00b1a..bd3f6facf240 100644 --- a/.prettierignore +++ b/.prettierignore @@ -16,3 +16,8 @@ src/BloomBrowserUI/bookEdit/OverflowChecker/*Fixture.html # leave a dirty working tree, and prettier outright fails on the book .htm files. Leave the # whole tree alone so the fetched fixtures match what Bloom writes. output/testing-inputs/ + +# The overflow checker's layout fixtures: the whitespace between their inline elements is +# part of what OverflowChecker.browser.spec.ts measures, and prettier's reflow changes it +# enough to turn its answers around. +src/BloomBrowserUI/bookEdit/OverflowChecker/*Fixture.html diff --git a/DistFiles/localization/en/BloomLowPriority.xlf b/DistFiles/localization/en/BloomLowPriority.xlf index 57c8e0b0ce76..fbf7ee6ec319 100644 --- a/DistFiles/localization/en/BloomLowPriority.xlf +++ b/DistFiles/localization/en/BloomLowPriority.xlf @@ -406,6 +406,11 @@ ID: ImageUtils.ShrinkingImagesFailed Shown as a toast when the one-time shrinking of an old book's oversized pictures fails. The book still works; the pictures are just left large, and Bloom retries later. "Bloom" is a product name and must not be translated. + + Bloom is saving your book + ID: Shell.SavingBeforeWindowsShutsDown + Shown by Windows itself, next to Bloom's name on the screen Windows displays when an application is slow to let it shut down, restart or log off. It appears only if Bloom takes more than a few seconds to finish saving. "Bloom" is a product name and must not be translated. + {0} used of {1} account credits ID: AiImageEditor.Credits.AccountCreditsUsed diff --git a/docs/save-without-reload/HANDOFF.md b/docs/save-without-reload/HANDOFF.md new file mode 100644 index 000000000000..1ecf6861c626 --- /dev/null +++ b/docs/save-without-reload/HANDOFF.md @@ -0,0 +1,133 @@ +# BL-13502 handoff: saving a page without reloading it + +For an agent continuing PR #8209 (branch `BL-13502-save-without-reload`, base `master`) on another +machine, most likely to answer a human reviewer. Everything here is either not in the code or easy +to miss there. Delete this folder before the PR merges. + +## Where things are + +- **The PR:** https://github.com/BloomBooks/BloomDesktop/pull/8209. Its description (between the + `preflight-narrative` markers) is the current summary of what the PR does, why, the risks, e2e + coverage and Notion cards. Keep it current if the PR changes. +- **The card:** https://issues.bloomlibrary.org/youtrack/issue/BL-13502. It carries the tester's + notes (a comment marked `test-ideas`). +- **The design:** `src/BloomExe/Edit/SavingWithoutReloading.md`. Read this first. +- **The preflight report** (for John, with his decisions so far): + https://bloombooks.github.io/dev-process-artifacts/deciders/BloomDesktop-BL-13502-save-without-reload.html, + in the `BloomBooks/dev-process-artifacts` repo at + `deciders/BloomDesktop-BL-13502-save-without-reload.html`. +- **The e2e plan** this PR's tests came from: a PR comment, + https://github.com/BloomBooks/BloomDesktop/pull/8209#issuecomment-6025653325. +- **Devin:** https://devinreview.com/BloomBooks/BloomDesktop/pull/8209. Every finding has been + mirrored to the PR and answered; nothing is open. + +## Decisions John has made (don't reopen them without a reason) + +- **Every save uses the snapshot the browser volunteers.** No request carries the page. A + snapshot means the page is idle unless it carries `stillBusyWith`. +- **No retries.** A failed snapshot post is reported to the user, once per page, and is not + retried: "Failure should be a crash." +- **Only the `.bloom-page` element is gathered and sent.** C# wraps it in a trivial body. Nothing + reads the body element's attributes. +- **A click, command or exit within about 25 ms of a keystroke** saves the snapshot from before + that keystroke. This is accepted: nobody can click that fast. +- **The wait for a busy page** is a plain sleep with a two-second cap (`PageSnapshot.WaitUntilIdle`). + The log names the work that was still running if the cap is reached. +- **Attribute order is ignored** when deciding whether a page changed + (`SafeXmlNode.GetXmlIgnoringAttributeOrder`). It is a generic XML helper, which is why it lives + in `SafeXmlNode`, not `HtmlDom`. +- **Optional branding images that are missing get no "missing image" alt text** + (`isBrandingImage` in `bloomImages.ts`). That text was being saved into book-wide data. +- **Table editing markup is stripped from the saved copy by Bloom's own copy of bloom-table's + list** (`removeTableEditingMarkupFromClone` in `tableEditing.ts`). bloom-table's + `removeTableEditingArtifacts` also ends Paint Format and Border Brush mode, which a save that runs + on every change must not do, and it doesn't export its markup-only part. A unit test compares the + two. If bloom-table ever exports that part, call it instead. +- **Any request that makes C# save the page and then reload it** must first send the page: + `saveChangesAndRethinkPage` or `postAfterSendingSnapshot` in `pageSnapshot.ts`. A feature merged + from master that posts such a request directly will lose the last moments of typing. The + original-copyright sentence's unlock and relock needed this when master was merged on 10-09. +- **The flaky test fix is in this PR.** `Xml_DoesNotProvide_ThreadSafety` was flaky on master; + John asked for the fix to go in here. +- **Left alone on purpose:** bloom-player's `prepareActivity` leaves `touch-action: none` on + draggables, so playing a game page and then saving writes it into the book. It predates this + PR, and the tester's notes say not to report it. +- **Comments say why the code is as it is now.** No history, except where old books can still + contain the older DOM. +- **Fixed waits in e2e tests** are allowed only for a specific, known interval. The one in use is + `waitForPageToSettle`, which waits for the overflow checker's 1000 ms timer and then for the + page to have nothing left to send. + +## Running things on this branch + +- **C#:** build and test through `build/agent-dotnet.sh`, never bare `dotnet`. It builds into an + isolated folder. +- **Bloom:** start it through the run-bloom skill. The launcher script is + `.claude/skills/run-bloom/launcherControl.mjs`, not `.github/skills/...`. +- **E2E tests (`src/BloomE2E`)** run the `Bloom.exe` in `output/Debug`, not the isolated build. So: + 1. Rebuild with `launcherControl.mjs --restart --wait-ready`. + 2. Quit that Bloom with `launcherControl.mjs --quit-bloom`; it blocks a second Bloom. Vite stays + up. + 3. Run `node build/get-testing-inputs.mjs` once. + 4. Run the specs with `BLOOM_E2E_VITE_PORT=` and + `BLOOM_AUTOMATION_MONITOR=headless`: + `pnpm exec playwright test tests/ --reporter=line`. + + The fixture refuses a build older than any C# source. + After merging master, also run `pnpm run build:pageSizes` in `src/content`. It writes + `output/browser/pageSizesLookup.json`, which C# needs (about 30 C# tests fail without it). The + full front-end build would make it too, but agents must not run that. The commit hook reformats C# files + (csharpier), which can make the build look stale after a commit: rebuild, then rerun. +- **The new specs:** + - `typing-survives-leaving-the-page` + - `saved-book-stays-clean` + - `change-layout-keeps-typing` + - `duplicate-page-many-times` + + They have 16 tests, all passing. +- **The older specs that touch this code:** + - `duplicate-page` + - `copy-page` + - `workspace-tabs` + - `toolbox-tools` + - `talking-book-paste` + - `rotate-and-flip-images` + - `capture-book-page` + + All 28 of their tests pass. +- **E2E tests that type and then leave the page** need `waitForBloomToHaveTyping` in between, as + a person's pause would provide. Master's specs, written before this PR, may not have it; on + 10-09, `original-copyright-sentence.spec.ts` needed it. +- **Quitting Bloom from a test:** `quitAndRestart` in the fixture posts `WM_CLOSE` to Bloom's + visible windows. In `--dont-disturb` mode the main window has a hidden owner, so `taskkill` + without `/F` and `CloseMainWindow` both miss it. +- **CI:** the only PR workflow is `pr-automation`, which just starts Devin. On 2026-10-09 GitHub + started no run for the pushes that day. If Devin has no job for the head commit, start it by + loading the review page (see the `devin-review` skill). + +## Notion test cards + +Database "Test Case Runs", `38c4bb19-df12-8123-8bc8-e65b962cb12f`. Token: the +`BLOOM_TESTCASE_NOTION` user environment variable, the secret of the internal integration +`test-management-access`. Use the REST API directly (Notion-Version `2022-06-28`). + +The cards this PR automates are all set to Automation = `PR Pending`: + +- #659 Quit Without Losing Changes +- #663 Clean Saved Book HTML +- #841 Typing Survives Leaving the Page +- #842 Looking at Pages Does Not Rewrite the Book +- #843 Change Layout Keeps Typing +- #844 Duplicate Page Many Times + +When the PR merges, set them to `Automated`. #366 notes that #843 covers part of it. #810's Many +Times step moved to #844. Ask John before creating or changing cards. + +## Conventions for posting + +- **Attribution:** comments and replies posted under John's account start with a model tag, e.g. + `[Claude Opus 5.5 from John Thomson's machine]`. +- **Devin:** never post an `@devin` mention. +- **Human reviewers:** a fix you make at a reviewer's suggestion gets a short reply saying what + changed, then the thread is resolved. Disagreeing with a human goes to John first. +- **Commits** end with the `Co-Authored-By` line for the model in use. diff --git a/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts b/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts index 685b5f4c35ba..d073dd471da9 100644 --- a/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts +++ b/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts @@ -44,6 +44,7 @@ import { RenderCanvasElementRoot } from "./CanvasElementFormatPage"; import { CanvasElementManager } from "../js/canvasElementManager/CanvasElementManager"; import { kCanvasElementSelector } from "../toolbox/canvas/canvasElementConstants"; import { getPageIFrame } from "../../utils/shared"; +import { getEditablePageBundleExports } from "../js/workspaceFrames"; // Controls the CSS text-align value // Note: CSS text-align W3 standard does not specify "start" or "end", but Firefox/Chrome/Edge do support it. @@ -587,6 +588,15 @@ export default class StyleEditor { if (styleSheet == null) { return null; } + if (create) { + // A caller asking us to create the rule is about to change it through the CSSOM, which + // the page watcher cannot see (see notePageContentMayHaveChanged). Every such change + // passes through here. Saying so before the change is fine: the watcher reads the page + // after a delay, and the caller's edits are synchronous. + // Through the page frame's exports rather than a direct import, because this class is + // also used from the toolbox frame, and it is the PAGE frame that watches. + getEditablePageBundleExports()?.notePageContentMayHaveChanged(); + } let ruleList: CSSRuleList = styleSheet.cssRules; if (ruleList == null) { diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/AGENTS.md b/src/BloomBrowserUI/bookEdit/aiImageEditor/AGENTS.md index 59bd487b8b61..3f10db88f09a 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/AGENTS.md +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/AGENTS.md @@ -38,12 +38,12 @@ So: is there specifically so the two halves cannot disagree about which commit results belong to the page being edited. -The overlay is in the top window because a page save reloads the page iframe, which would tear -down an overlay hosted there — the same reason the image-gallery and copyright/license dialogs -live up there (see the comments on those commands in `canvasControlRegistry.ts`). Note that this -is *not* enough on its own: a save can also reload the whole workspace root, so C# waits for the -page to come back before opening the overlay. `AiImageEditorApi.HandleSaveThenLaunch` explains -that in full. +The overlay is in the top window, like the image-gallery and copyright/license dialogs (see the +comments on those commands in `canvasControlRegistry.ts`): plenty of operations replace the page +iframe underneath it. Opening it first saves the page, because everything the editor is told about +the book is read from the saved file; a save no longer reloads the page (BL-13502), so the editor +opens as soon as the save returns, and does not open at all if the save did not happen. +`AiImageEditorApi.HandleSaveThenLaunch` explains that in full. ## Analytics: Bloom forwards the AI Image Editor's events and knows nothing about them diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.test.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.test.ts index f41f0a5cd095..01bf464bcaf7 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.test.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.test.ts @@ -9,9 +9,11 @@ import { beforeEach, describe, expect, test, vi } from "vitest"; // - The edit target. C# hands over the page id and file name of the image the user // right-clicked (it survived a page save, which reloaded the page frame), and the overlay // matches that against the book image list to fill the "Image to Edit" slot (BL-16682). -// - NOT saving after a commit. A current-page swap lives only in the live page, like a pasted -// picture, and is saved by the normal page save when the user moves on. Saving at once -// would reload the page frame and discard the undo the swap registered (BL-16330). +// - NOT saving after a commit. A current-page swap lives in the live page DOM only, like an +// image pasted or chosen from the gallery, and is persisted the same way: by the normal page +// save when the user moves on (BL-16330). A retry from this still-open overlay would read a +// stale oldSrc, which the page frame handles by remembering what it already swapped rather +// than by saving here. const post = vi.fn(); const postJson = vi.fn(); @@ -376,7 +378,7 @@ describe("aiImageEditorOverlay: the live page is NOT saved after a commit", () = }; expect(ack.ok).toBe(true); expect(ack.error).toBeUndefined(); - // Nothing landed on this page, so nothing to save. + // We never save from here at all -- see the note at the top of this file. expect(postThatMightNavigate).not.toHaveBeenCalled(); expect(applyAiImageEditorReplacements).not.toHaveBeenCalled(); postMessageToEditor.mockRestore(); @@ -402,7 +404,7 @@ describe("aiImageEditorOverlay: the live page is NOT saved after a commit", () = expect(ack.ok).toBe(false); expect(ack.error).toContain("not available"); expect(ack.error).toContain("other pages were made"); - // Nothing landed, so nothing to save. + // We never save from here at all -- see the note at the top of this file. expect(postThatMightNavigate).not.toHaveBeenCalled(); postMessageToEditor.mockRestore(); }); diff --git a/src/BloomBrowserUI/bookEdit/duplicateManyDialog.entry.tsx b/src/BloomBrowserUI/bookEdit/duplicateManyDialog.entry.tsx deleted file mode 100644 index 31367e91b391..000000000000 --- a/src/BloomBrowserUI/bookEdit/duplicateManyDialog.entry.tsx +++ /dev/null @@ -1,4 +0,0 @@ -import { bootstrapReactComponent } from "../utils/entryPointBootstrap"; -import { DuplicateManyDialog } from "./duplicateManyDialog"; - -bootstrapReactComponent(DuplicateManyDialog); diff --git a/src/BloomBrowserUI/bookEdit/duplicateManyDialog.tsx b/src/BloomBrowserUI/bookEdit/duplicateManyDialog.tsx index 583ffb3bfe17..d9bde147e087 100644 --- a/src/BloomBrowserUI/bookEdit/duplicateManyDialog.tsx +++ b/src/BloomBrowserUI/bookEdit/duplicateManyDialog.tsx @@ -3,9 +3,11 @@ import { IBloomDialogEnvironmentParams } from "../react_components/BloomDialog/B import { useL10n } from "../react_components/l10nHooks"; import { NumberChooserDialog } from "../react_components/numberChooserDialog"; import { postData } from "../utils/bloomApi"; -import { WireUpForWinforms } from "../utils/WireUpWinform"; +import { ShowEditViewDialog } from "./workspaceRoot"; export const DuplicateManyDialog: React.FunctionComponent<{ + // The page the dialog was opened for. C# duplicates only if it is still the current page. + pageId: string; dialogEnvironment?: IBloomDialogEnvironmentParams; }> = (props) => { const title = useL10n( @@ -26,6 +28,7 @@ export const DuplicateManyDialog: React.FunctionComponent<{ const clickHandler = (value: number) => { postData("editView/duplicatePageMany", { numberOfTimes: value, + pageId: props.pageId, }); }; return ( @@ -40,4 +43,16 @@ export const DuplicateManyDialog: React.FunctionComponent<{ ); }; -WireUpForWinforms(DuplicateManyDialog); +// Shown in the Edit tab, from the page list's context menu, so nothing reaches C# until the user +// clicks OK. (A C# modal opened from the page list's request would hold the API lock.) +export function showDuplicateManyDialog(pageId: string) { + ShowEditViewDialog( + , + ); +} diff --git a/src/BloomBrowserUI/bookEdit/editablePage.ts b/src/BloomBrowserUI/bookEdit/editablePage.ts index 50331cb1adc7..ccc8bb1300d6 100644 --- a/src/BloomBrowserUI/bookEdit/editablePage.ts +++ b/src/BloomBrowserUI/bookEdit/editablePage.ts @@ -17,6 +17,12 @@ import { } from "./js/canvasElementManager/CanvasElementManager"; import { kCanvasElementSelector } from "./toolbox/canvas/canvasElementConstants"; import { renderDragActivityTabControl } from "./js/AbovePageControls"; +import { + getPageLoadId, + isSnapshotStreamIdle, + notePageContentMayHaveChanged, + startWatchingPageForSnapshots, +} from "./js/pageSnapshot"; import { tableHistoryManager } from "bloom-table"; import { getCkeditorChangeOrder, @@ -39,8 +45,20 @@ function getPageId(): string { // This notification lets the C# know that this partciular page is ready to edit. // It is important that this does not get pulled into any other compiled bundle, // since it will generate errors when loaded into any page that does not have a .bloom-page. +// +// The load id goes with it: once C# answers, it accepts snapshots only from this load (see +// getPageLoadId()), so the page snapshot waits for that answer before it starts. +let pageDomLoadedAnswered: Promise | undefined; +function tellCSharpPageDomLoaded(): Promise { + if (!pageDomLoadedAnswered) + pageDomLoadedAnswered = postJson("editView/pageDomLoaded", { + pageId: getPageId(), + loadId: getPageLoadId(), + }); + return pageDomLoadedAnswered; +} document.addEventListener("DOMContentLoaded", () => { - postString("editView/pageDomLoaded", getPageId()); + void tellCSharpPageDomLoaded(); }); // This allows strong typing to be done for exported functions. @@ -54,8 +72,14 @@ document.addEventListener("DOMContentLoaded", () => { // but I think it is unwise. It is so easy for an extra file to get imported into another bundle, // and then it will bring this along, with disastrous results. export interface IPageFrameExports { - requestPageContent(): void; + // The combined "page userCss" string that a save needs, gathered without + // disturbing the live page. + getPageContentForSaveWhenReady(): Promise; pageUnloading(): void; + // See pageSnapshot.ts. + notePageContentMayHaveChanged(): void; + // See pageSnapshot.ts. For the e2e suite. + isSnapshotStreamIdle(): boolean; copySelection(): void; cutSelection(): void; pasteClipboard(): void; @@ -121,12 +145,10 @@ export interface IPageFrameExports { } // This exports the functions that should be accessible from other IFrames or from C#. -// For example, workspaceBundle.getEditablePageBundleExports().requestPageContent() can be called. +// For example, workspaceBundle.getEditablePageBundleExports().pageUnloading() can be called. import { - getBodyContentForSavePage, - requestPageContent, + getPageContentForSaveWhenReady, captureContentForExternalProcessing, - userStylesheetContent, pageUnloading, topBarButtonClick, copySelection, @@ -140,9 +162,11 @@ import { changeImageByElement, imageOperationCanUndo, imageOperationUndo, +} from "./js/bloomEditing"; +import { addRequestPageContentDelay, removeRequestPageContentDelay, -} from "./js/bloomEditing"; +} from "./js/pageContentDelays"; import { showGamePromptDialog } from "./toolbox/games/GameTool"; // Called from the AI Image Editor overlay in the top window, which owns the session but // cannot touch this page itself; see aiImageEditorPageCommands.ts and aiImageEditorOverlay.ts. @@ -156,11 +180,11 @@ import type { } from "./aiImageEditor/aiImageEditorShared"; import type { IPageMetrics } from "./js/imageTargetResolution"; export { - getBodyContentForSavePage, - requestPageContent, + getPageContentForSaveWhenReady, captureContentForExternalProcessing, - userStylesheetContent, pageUnloading, + notePageContentMayHaveChanged, + isSnapshotStreamIdle, topBarButtonClick, copySelection, cutSelection, @@ -182,7 +206,7 @@ export { getAiImageEditorPageMetrics, }; import { origamiCanUndo, origamiUndo } from "./js/origami"; -import { postString } from "../utils/bloomApi"; +import { postJson } from "../utils/bloomApi"; export { origamiCanUndo, origamiUndo }; const styleSheets = [ @@ -436,11 +460,11 @@ window["PasteImageCredits"] = () => { //UnlockOriginalCredits() is called from the hint bubble on the credits page's //generated original-copyright sentence. See BookCopyrightAndLicense.SetOriginalCopyrightNoticeHint(). window["UnlockOriginalCredits"] = () => { - EditableDivUtils.unlockOriginalCredits(); + void EditableDivUtils.unlockOriginalCredits(); }; window["RelockOriginalCredits"] = () => { - EditableDivUtils.relockOriginalCredits(); + void EditableDivUtils.relockOriginalCredits(); }; $(document).ready(() => { @@ -456,6 +480,13 @@ $(document).ready(() => { // in the live editor, which never reads this flag. window.__bloomEditablePageReady = true; + // Start sending the page's content to C# (see pageSnapshot.ts), but not until C# has accepted + // this load, or it would refuse what we send. (The notice normally went at DOMContentLoaded; + // asking again just returns that post.) + void tellCSharpPageDomLoaded().then(() => + startWatchingPageForSnapshots(getPageContentForSaveWhenReady), + ); + // If the user clicks outside of the page thumbnail context menu, we want to close it. // Since it is currently a winforms menu, we do that by sending a message // back to c#-land. We have a similar listener in the pageThumbnailList itself. @@ -473,11 +504,10 @@ export function SayHello() { // Legacy global exposure: mimic old webpack window["editablePageBundle"] contract used by other iframes / C# // NOTE: Keep this as a minimal curated surface: only expose functions intentionally callable cross-frame. interface EditablePageBundleApi { - requestPageContent: typeof requestPageContent; captureContentForExternalProcessing: typeof captureContentForExternalProcessing; - getBodyContentForSavePage: typeof getBodyContentForSavePage; - userStylesheetContent: typeof userStylesheetContent; + getPageContentForSaveWhenReady: typeof getPageContentForSaveWhenReady; pageUnloading: typeof pageUnloading; + notePageContentMayHaveChanged: typeof notePageContentMayHaveChanged; copySelection: typeof copySelection; cutSelection: typeof cutSelection; pasteClipboard: typeof pasteClipboard; @@ -512,6 +542,7 @@ interface EditablePageBundleApi { e2eCanExpandActiveCanvasElementToFillSpace: typeof e2eCanExpandActiveCanvasElementToFillSpace; e2eOverrideCanExpandToFillSpace: typeof e2eOverrideCanExpandToFillSpace; e2eClearCanExpandToFillSpaceOverride: typeof e2eClearCanExpandToFillSpaceOverride; + isSnapshotStreamIdle: typeof isSnapshotStreamIdle; SayHello: typeof SayHello; renderDragActivityTabControl: typeof renderDragActivityTabControl; showGamePromptDialog: typeof showGamePromptDialog; @@ -537,8 +568,8 @@ declare global { // the finished page onto window.__bloomExternalPageContent. // 3. C# polls window.__bloomExternalPageContent until it is non-empty and reads it back. // - // Why globals + polling, rather than posting to the editView/pageContent API the way the live - // editor's requestPageContent() does: + // Why globals + polling, rather than posting to the editView/pageSnapshot API the way the + // live editor does: // - That API feeds the live EditingModel; reusing it off-screen would corrupt the real // editor's state. We want the same page-cleanup output, delivered out-of-band. // - C#'s JS runner on this path (RunJavascriptWithStringResult_Sync_Dangerous) is @@ -551,17 +582,16 @@ declare global { // // Step 1's flag: set in $(document).ready below; read in BookProcessor.ProcessOnePage. __bloomEditablePageReady?: boolean; - // Step 2/3's mailbox: the combined "bodyuserCss" string, or "ERROR: ". + // Step 2/3's mailbox: the combined "pageuserCss" string, or "ERROR: ". __bloomExternalPageContent?: string; } } window.editablePageBundle = { - requestPageContent, captureContentForExternalProcessing, - getBodyContentForSavePage, - userStylesheetContent, + getPageContentForSaveWhenReady, pageUnloading, + notePageContentMayHaveChanged, copySelection, cutSelection, pasteClipboard, @@ -596,6 +626,7 @@ window.editablePageBundle = { e2eCanExpandActiveCanvasElementToFillSpace, e2eOverrideCanExpandToFillSpace, e2eClearCanExpandToFillSpaceOverride, + isSnapshotStreamIdle, SayHello, renderDragActivityTabControl, showGamePromptDialog, diff --git a/src/BloomBrowserUI/bookEdit/js/AbovePageControls.tsx b/src/BloomBrowserUI/bookEdit/js/AbovePageControls.tsx index d0d4a918199d..2635d636068d 100644 --- a/src/BloomBrowserUI/bookEdit/js/AbovePageControls.tsx +++ b/src/BloomBrowserUI/bookEdit/js/AbovePageControls.tsx @@ -50,7 +50,7 @@ export function updateAbovePageControls( export function resetAbovePageControls(): void { currentState = defaultState; // Off-screen (e.g. process-book) there is no workspace frame, so this is a no-op there. - // (Reached via removeEditingDebris() in the shared extractAndStripPageContentForSave() save path.) + // (Reached via pageUnloading() in bloomEditing.ts.) tryGetWorkspaceBundleExports()?.setToolboxEnabled(true); const container = document.getElementsByClassName( diff --git a/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts b/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts index 25582ca732aa..cfee03a93b87 100644 --- a/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts +++ b/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts @@ -20,8 +20,10 @@ import { } from "./bloomVideo"; import { SetupWidgetEditing } from "./bloomWidgets"; import { setupOrigami, cleanupOrigami } from "./origami"; -import { SetupTableEditing, TeardownTableEditing } from "./tableEditing"; -import { removeTableEditingArtifacts } from "bloom-table"; +import { + SetupTableEditing, + removeTableEditingMarkupFromClone, +} from "./tableEditing"; import theOneLocalizationManager from "../../lib/localizationManager/localizationManager"; import StyleEditor from "../StyleEditor/StyleEditor"; import OverflowChecker from "../OverflowChecker/OverflowChecker"; @@ -29,6 +31,13 @@ import BloomField from "../bloomField/BloomField"; import BloomNotices from "./bloomNotices"; import BloomSourceBubbles from "../sourceBubbles/BloomSourceBubbles"; import BloomHintBubbles from "./BloomHintBubbles"; +import { + addRequestPageContentDelay, + getActiveDelayIds, + removeRequestPageContentDelay, + whenNoActiveDelays, + wrapWithRequestPageContentDelay, +} from "./pageContentDelays"; import { CanvasElementManager, initializeCanvasElementManager, @@ -61,7 +70,7 @@ import { showInvisibles, hideInvisibles } from "./showInvisibles"; //import promise = require('es6-promise'); //promise.Promise.polyfill(); import axios from "axios"; -import { post, postBoolean, postJson, postString } from "../../utils/bloomApi"; +import { post, postBoolean, postJson } from "../../utils/bloomApi"; import { showRequestStringDialog } from "../../react_components/RequestStringDialog"; import { hookupLinkHandler } from "../../utils/linkHandler"; @@ -77,6 +86,9 @@ import { } from "./pageContentCapturePolicy"; import { setupDragActivityTabControl } from "../toolbox/games/GameTool"; import { addScrollbarsToPage, cleanupNiceScroll } from "bloom-player"; +import { removeNiceScrollArtifacts } from "./niceScrollCleanup"; +import { removeEditorChromeFromClone } from "./editorChromeCleanup"; +import { sendSnapshotNow, stopWatchingPageForSnapshots } from "./pageSnapshot"; import { setupBookLinkGrids } from "./linkGrid"; import { fitImageOverTextSplits } from "./autoFitImageOverTextSplits"; import PlaceholderProvider from "./PlaceholderProvider"; @@ -174,6 +186,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(); } @@ -1353,204 +1368,130 @@ 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 the editing-only markup out of 'clonedPage', the detached copy we are about to save. +function removeEditingDebrisFromClone(clonedPage: 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 = clonedPage.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); - removeTableEditingArtifacts(document); - TeardownTableEditing(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; - } -} - -// 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. -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(); + for (const textLabel of Array.from( + marginBox.getElementsByClassName("textBox-identifier"), + )) { + textLabel.remove(); } + removeTransientVideoTimestampParams(clonedPage); + removeTableEditingMarkupFromClone(clonedPage); + removeEditorChromeFromClone(clonedPage); } -// 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 element + user stylesheet combined with the delimiter that C# splits +// on. Shared by the live editor's gathers (getPageContentForSaveWhenReady) and the off-screen +// capture path (captureContentForExternalProcessing), so the cleanup steps and the delimiter can't +// drift between them. // -// 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 -// navigates to the next page afterwards. Don't call this from a context where the page must stay -// live and editable afterward. -function extractAndStripPageContentForSave(): string { - // Record how much of the page each image slot covers, while the page is still laid out. - // That is the only record of it: the saved HTML otherwise says nothing about how big - // anything ends up on screen, so without this the AI image editor could not tell what size - // an image on any page but the open one ought to be. Never throws out: a missing size hint - // must not cost the user their page. - try { - recordFractionOfPageOnImageSlots(document.body); - } catch (e) { - console.error("recordFractionOfPageOnImageSlots failed: ", e); - } - - // 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(); - const content = getBodyContentForSavePage(); +// Deliberately NOT exported: every caller should come through getPageContentForSaveWhenReady() (or +// the off-screen path, which does its own waiting), so that nobody can gather the page while +// asynchronous work that belongs in it is still running. It is also deliberately synchronous, so +// that no other event handler can run part way through capturing the page. +function getPageContentForSave(): string { + const content = getPageHtmlForSave(); const userStylesheet = userStylesheetContent(); // (We tossed up whether to use a JSON object instead of a delimiter, but combining two strings is // simpler: HTML needs escaping to live in JSON, which we'd then have to undo in C#.) return content + "" + userStylesheet; } -function requestPageContentInternal() { - if (requestPageContentTimeout !== null) { - clearTimeout(requestPageContentTimeout); - } - requestPageContentTimeout = null; - try { - postString("editView/pageContent", extractAndStripPageContentForSave()); - } catch (e) { - postString( - "editView/pageContent", - "ERROR: " + - e.message + - "\n" + - e.stack + - "\n\n" + - `document ${document ? "exists" : "does not exist"}` + - "\n" + - "body.innerHTML: " + - document?.body?.innerHTML, - ); - } +// 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. Nothing is awaited between the two, 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(); } -// 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() { - if (hadOrigamiWhenWeLoadedThePage && !hasOrigami(document.body)) { +// Produce the HTML of the .bloom-page element as it should be saved. All the cleanup is done on a +// CLONE, so the live page is untouched and stays editable without a reload (BL-13502). +// +// 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 getPageHtmlForSave() { + const livePage = document.querySelector(".bloom-page"); + if (!livePage) { throw new Error( - "getBodyContentForSavePage(): The page had origami when it loaded, but it doesn't now (check before cleanup). BL-13120", + "getPageHtmlForSave(): there is no .bloom-page to save.", ); } - - const canvasElementEditingOn = - theOneCanvasElementManager.isCanvasElementEditingOn; - if (canvasElementEditingOn) { - theOneCanvasElementManager.turnOffCanvasElementEditing(); - } - // Active element should be forced to blur - if (document.activeElement instanceof HTMLElement) { - document.activeElement.blur(); + if (hadOrigamiWhenWeLoadedThePage && !hasOrigami(livePage)) { + throw new Error( + "getPageHtmlForSave(): The page had origami when it loaded, but it doesn't now (check before cleanup). BL-13120", + ); } - const editableDivs = ( - Array.from(document.querySelectorAll("div.bloom-editable")) - ); + // Deliberately do NOT blur the active element: that would throw the user's cursor out of the + // box they are typing in on every snapshot. CKEditor's getData() gives the up-to-date text + // without a blur. - // 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); + // Only the page element is saved (C# keeps nothing else), so only it is copied and cleaned. + // Everything the editor puts outside it -- CKEditor's toolbars, qTip's bubbles, menus, scratch + // elements -- never reaches C# at all. + const clonedPage = livePage.cloneNode(true) as HTMLElement; + cleanCloneOfPageForSave(livePage, clonedPage); - if (hadOrigamiWhenWeLoadedThePage && !hasOrigami(document.body)) { + if (hadOrigamiWhenWeLoadedThePage && !hasOrigami(clonedPage)) { throw new Error( - "getBodyContentForSavePage(): The page had origami when it loaded, but it doesn't now (check after cleanup). BL-13120", + "getPageHtmlForSave(): The page had origami when it loaded, but it doesn't now (check after cleanup). BL-13120", ); } - const result = document.body.innerHTML; + return clonedPage.outerHTML; +} - if (canvasElementEditingOn) { - theOneCanvasElementManager.turnOnCanvasElementEditing(); +// Do all the "strip the editing markup" work on 'clonedPage', a detached deep copy of 'livePage'. +// Nothing here may touch the live page. +function cleanCloneOfPageForSave( + livePage: HTMLElement, + clonedPage: HTMLElement, +) { + // Record how much of the page each image slot covers, measured on the live page (the clone + // has no layout) and written into the clone. That is the only record of it: the saved HTML + // otherwise says nothing about how big anything ends up on screen, so without this the AI + // image editor could not tell what size an image on any page but the open one ought to be. + // Never throws out: a missing size hint must not cost the user their page. + try { + recordFractionOfPageOnImageSlots(livePage, clonedPage); + } catch (e) { + console.error("recordFractionOfPageOnImageSlots failed: ", e); } - return result; + // 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 must come BEFORE the tool cleanup below: getData() reports what the live editors hold, + // tool markup included, so the tools must clean the text CKEditor gave us. Otherwise a tool + // whose cleanup reaches inside an editable (today, the Talking Book tool's phrase-delimiter + // spans and audio highlighting) would have its work overwritten. + EditableDivUtils.copyCkEditorDataToClone(livePage, clonedPage); + + // 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 on: otherwise this 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), whose balloon data a save must leave as it found it. + if (theOneCanvasElementManager.isCanvasElementEditingOn) { + theOneCanvasElementManager.prepareCloneOfPageForSave(clonedPage); + } + + // 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()?.removeToolMarkupFromPageClone(clonedPage); + + removeNiceScrollArtifacts(clonedPage); + + removeEditingDebrisFromClone(clonedPage); } // Resize each text canvas element (bloom-canvas-element) to fit its content -- growing or shrinking @@ -1603,24 +1544,24 @@ function resizeCanvasElementsToFitContent(): void { } // Used by the off-screen "process whole book" path (C# BookProcessor, driven by the -// external/process-book API). It gathers the same page content that requestPageContent() would save -// (via the shared extractAndStripPageContentForSave()), but instead of posting it to the editView/pageContent -// API (which feeds the LIVE EditingModel and would corrupt the live editor's state), it stashes the -// combined result on window.__bloomExternalPageContent for the C# caller to poll. Like -// requestPageContent(), it first waits for any in-flight async DOM work (activeDelays) to finish, so -// browser-based measurements (image sizing, canvas-element layout, etc.) are complete before we capture -// the page. Unlike the live save, it is a background job with nobody waiting at the keyboard, so it -// waits longer (kExternalCaptureMaxWaitMs), and if the one piece of work that must not be captured -// half-done, the background image conversion, is still pending at the cap, it reports an ERROR instead -// of capturing (see externalCaptureErrorForPendingWork); the C# caller then fails the page rather than +// external/process-book API). It gathers the same page content a save would (via the shared +// getPageContentForSave()), but instead of posting it to an API that feeds the LIVE EditingModel, +// it stashes the result on window.__bloomExternalPageContent for the C# caller to poll. It first +// waits on whenNoActiveDelays(), longer than the live editor since nobody is waiting at the +// keyboard (kExternalCaptureMaxWaitMs), and if the one piece of work that must not be captured +// half-done, the background image conversion, is still pending at the cap, it reports an ERROR +// instead of capturing (see externalCaptureErrorForPendingWork); the C# caller then fails the page +// rather than // saving a picture that can neither be cropped nor deleted (BL-16870). 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. +// elements to fit their content (see resizeCanvasElementsToFitContent), since that auto-height +// adjustment is otherwise deferred on a timer the wait loop does not track. export function captureContentForExternalProcessing( fitImageTextSplits?: boolean, ): void { window.__bloomExternalPageContent = undefined; + // This off-screen page is a full editing page, so it may have started sending snapshots. C# + // refuses them (wrong load id), but they are wasted work. + stopWatchingPageForSnapshots(); // Optionally auto-fit image/text origami pages so the fitted split persists into the saved HTML. // This handles two-pane image-above-text and image-left-of-text (image in the first pane), plus @@ -1648,44 +1589,29 @@ export function captureContentForExternalProcessing( } } - const start = Date.now(); - const finish = () => { + void whenNoActiveDelays(kExternalCaptureMaxWaitMs).then(() => { + // If the wait ran out, decide whether what is still pending may be captured half-done. + const stillPending = getActiveDelayIds(); + if (stillPending.length > 0) { + const error = externalCaptureErrorForPendingWork(stillPending); + if (error) { + window.__bloomExternalPageContent = error; + return; + } + } 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) { - finish(); - return; - } - if (Date.now() - start > kExternalCaptureMaxWaitMs) { - const error = externalCaptureErrorForPendingWork(activeDelays); - if (error) { - window.__bloomExternalPageContent = error; - return; - } - console.warn( - `captureContentForExternalProcessing: Maximum wait time (${kExternalCaptureMaxWaitMs}ms) exceeded with active delay(s): [${activeDelays.join( - ", ", - )}]. Proceeding anyway.`, - ); - finish(); - return; - } - 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. +const userStylesheetContent = () => { const ss = Array.from(document.styleSheets).find( (s) => s.title === "userModifiedStyles", ) as CSSStyleSheet | undefined; @@ -1695,12 +1621,29 @@ export const userStylesheetContent = () => { .join("\n"); }; +// pageUnloading() can be called twice on the same page: leaving the Edit tab runs it (from +// EditingView.OnHideEditTab), and coming back runs it again from switchContentPage() before the +// new page replaces this document. A second run detaches the current tool again, and +// detachToolFromPage() then falsely reports the tool for not calling super.detachFromPage(). +let thisPageHasBeenUnloaded = false; + export const pageUnloading = () => { + if (thisPageHasBeenUnloaded) return; + thisPageHasBeenUnloaded = true; + stopWatchingPageForSnapshots(); // It's just possible that 'theOneCanvasElementManager' hasn't been initialized. // If not, just ignore this, since it's a no-op at this point anyway. if (theOneCanvasElementManager) { theOneCanvasElementManager.cleanUp(); } + // Shut the open toolbox tool down, releasing whatever it was holding on the page we are leaving + // (observers, listeners, any UI it opened such as a colour picker); the counterpart of the + // newPageReady() the tool gets for the next page. Saving never touches the live page, so + // nothing else detaches the tool, and without this every page change would leak its hooks. + getToolboxBundleExports()?.removeToolboxMarkup(); + // Unmount the React root for the controls above the page and re-enable the toolbox (the + // Change Layout toggle disables it). + resetAbovePageControls(); }; export function topBarButtonClick(button: { command: string }) { @@ -2080,6 +2023,24 @@ export function attachToCkEditor(element) { $("body").addClass("hideAllCKEditors"); const ckedit = CKEDITOR.inline(element); + // Until this editor is ready, a gather reads the box from the DOM rather than from getData(), + // and they differ: SetupElements puts an empty

into an empty editable, which getData() + // reports as empty, as the book on disk does. So the delay register holds gathers until + // instanceReady; otherwise an untouched page would look changed and an early save would write + //

into boxes the user left empty. + const ckEditorDelayId = "attachToCkEditor " + ckedit.id; + addRequestPageContentDelay(ckEditorDelayId); + let ckEditorDelayReleased = false; + const releaseCkEditorDelay = () => { + if (ckEditorDelayReleased) return; + ckEditorDelayReleased = true; + removeRequestPageContentDelay(ckEditorDelayId); + }; + // (instanceReady is not on CKEditor’s TypeScript type; toolbox.ts declares it the same way.) + if ((ckedit as { instanceReady?: boolean }).instanceReady) + releaseCkEditorDelay(); + else ckedit.on("instanceReady", releaseCkEditorDelay); + // Record the div of the edit box for use later in positioning the format bar. mapCkeditDiv[ckedit.id] = element; diff --git a/src/BloomBrowserUI/bookEdit/js/bloomImages.ts b/src/BloomBrowserUI/bookEdit/js/bloomImages.ts index c47af7f5d624..0095c37009b5 100644 --- a/src/BloomBrowserUI/bookEdit/js/bloomImages.ts +++ b/src/BloomBrowserUI/bookEdit/js/bloomImages.ts @@ -24,11 +24,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"; @@ -1311,9 +1308,22 @@ export function getDisplayNameFromImageUrl(rawImageUrl: string): string { } } +// The same test as EpubMaker.IsBranding in C#: marked as branding itself, or inside a data-book +// element that holds branding, such as "outside-back-cover-branding-bottom-html". +export function isBrandingImage(img: HTMLElement): boolean { + return ( + img.classList.contains("branding") || + !!img.closest("[data-book*='branding']") + ); +} + // Instead of "missing", we want to show it in the right ui language. We also want the text // to indicate that it might not be missing, just didn't load (this happens on slow machines) function SetAlternateTextOnImages(element) { + // Every branding image is optional, so one that is not there is not worth a message. Its alt + // comes from the branding and is book-wide data: changing it here would get saved, and make + // every visit to the page look like an edit to the whole book. + if (isBrandingImage(element)) return; const rawImageUrl = GetRawImageUrl(element); if (rawImageUrl.length > 0) { if (isPlaceHolderImage(rawImageUrl)) { diff --git a/src/BloomBrowserUI/bookEdit/js/bloomImagesSpec.ts b/src/BloomBrowserUI/bookEdit/js/bloomImagesSpec.ts index bfd3db0cc8b6..16817137e9f7 100644 --- a/src/BloomBrowserUI/bookEdit/js/bloomImagesSpec.ts +++ b/src/BloomBrowserUI/bookEdit/js/bloomImagesSpec.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import { getDisplayNameFromImageUrl, + isBrandingImage, normalizeCoverImageDesignation, } from "./bloomImages"; @@ -204,3 +205,54 @@ describe("getDisplayNameFromImageUrl", () => { ).toBe("ภาพ.jpg"); }); }); + +// Branding images get no missing-image alt text, because that alt would be saved into book-wide data. +describe("isBrandingImage", () => { + function imageIn(html: string): HTMLElement { + const page = document.createElement("div"); + page.innerHTML = html; + const img = page.querySelector("#img"); + if (!img) throw new Error("The test markup has no #img"); + return img as HTMLElement; + } + + it("recognises an image marked as branding", () => { + expect( + isBrandingImage( + imageIn(``), + ), + ).toBe(true); + }); + + it("recognises an unmarked image inside a branding block", () => { + expect( + isBrandingImage( + imageIn( + `
+
+
`, + ), + ), + ).toBe(true); + }); + + it("does not count a picture in the book", () => { + expect( + isBrandingImage( + imageIn( + `
`, + ), + ), + ).toBe(false); + }); + + it("does not count an image in other book-wide data", () => { + expect( + isBrandingImage( + imageIn( + `
`, + ), + ), + ).toBe(false); + }); +}); diff --git a/src/BloomBrowserUI/bookEdit/js/bloomVideo.ts b/src/BloomBrowserUI/bookEdit/js/bloomVideo.ts index 3a4436980031..c11de7cc2fa3 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 "./pageSnapshot"; // 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. @@ -201,7 +201,7 @@ export function doVideoCommand( // Makes sure the page gets saved with a reference to the new video, // and incidentally that everything gets updated to be consistent with the // new state of things. - postThatMightNavigate("common/saveChangesAndRethinkPageEvent"); + void saveChangesAndRethinkPage(); } }); } else if (command === "record") { diff --git a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementAlternates.test.ts b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementAlternates.test.ts new file mode 100644 index 000000000000..c30417c8ec7a --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementAlternates.test.ts @@ -0,0 +1,76 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +// Every test passes the language explicitly, so the collection settings this module would +// otherwise consult are never reached. +import { saveStateOfCanvasElementAsCurrentLangAlternate } from "./CanvasElementAlternates"; + +const kLang = "xyz"; + +function makeCanvasElement(dataBubble: string | null): HTMLElement { + const el = document.createElement("div"); + el.className = "bloom-canvas-element"; + el.setAttribute("style", "left: 10px; top: 20px;"); + if (dataBubble !== null) el.setAttribute("data-bubble", dataBubble); + const editable = document.createElement("div"); + editable.className = "bloom-editable"; + editable.setAttribute("lang", kLang); + el.appendChild(editable); + return el; +} + +function alternateOn(canvasElement: HTMLElement): string | null { + return canvasElement + .getElementsByClassName("bloom-editable")[0] + .getAttribute("data-bubble-alternate"); +} + +describe("saveStateOfCanvasElementAsCurrentLangAlternate", () => { + beforeEach(() => { + vi.spyOn(console, "warn").mockImplementation(() => {}); + }); + + it("records the alternate when the bubble data is readable", () => { + // Bloom stores this JSON with backticks standing in for the quotes. + const el = makeCanvasElement("{`version`:`1.0`,`tails`:[{`tipX`:1}]}"); + + saveStateOfCanvasElementAsCurrentLangAlternate(el, kLang); + + const written = alternateOn(el); + expect(written).not.toBeNull(); + expect(written).toContain("`lang`:`" + kLang + "`"); + expect(written).toContain("tipX"); + }); + + it("does not lose the whole page when a canvas element has no bubble data", () => { + // The real hazard: this runs inside the clone gather, so throwing here does not merely + // skip one alternate, it aborts gathering the page -- and then the page cannot be saved at + // all. A missing attribute used to throw, because JSON.parse("") is an error. + const el = makeCanvasElement(null); + + expect(() => + saveStateOfCanvasElementAsCurrentLangAlternate(el, kLang), + ).not.toThrow(); + expect(alternateOn(el)).toBeNull(); + }); + + it("does not lose the whole page when the bubble data is malformed", () => { + const el = makeCanvasElement("{this is not json"); + + expect(() => + saveStateOfCanvasElementAsCurrentLangAlternate(el, kLang), + ).not.toThrow(); + expect(alternateOn(el)).toBeNull(); + }); + + it("leaves an existing alternate alone rather than replacing it with an empty one", () => { + // Recording an alternate with no tails would claim this language's copy has none, and the + // user would lose them on switching to it. Skipping is the conservative choice. + const el = makeCanvasElement("{broken"); + const editable = el.getElementsByClassName("bloom-editable")[0]; + editable.setAttribute("data-bubble-alternate", "{`lang`:`xyz`}"); + + saveStateOfCanvasElementAsCurrentLangAlternate(el, kLang); + + expect(alternateOn(el)).toBe("{`lang`:`xyz`}"); + }); +}); diff --git a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementAlternates.ts b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementAlternates.ts index bfa40064e2a8..b99a94516bcb 100644 --- a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementAlternates.ts +++ b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementAlternates.ts @@ -39,7 +39,23 @@ export const saveStateOfCanvasElementAsCurrentLangAlternate = ( ).find((e) => e.getAttribute("lang") === canvasElementLang); if (editable) { const bubbleData = canvasElement.getAttribute("data-bubble") ?? ""; - const bubbleDataObj = JSON.parse(bubbleData.replace(/`/g, '"')); + // A missing or unreadable data-bubble must not cost the user the whole page: this runs + // inside the clone gather, so a throw would abort gathering, and so saving, the page. + // (A missing attribute throws too, since JSON.parse("") is an error.) Skipping leaves any + // existing alternate alone; recording one with no tails would lose this language's tails. + let bubbleDataObj: { tails?: object[] }; + try { + bubbleDataObj = JSON.parse(bubbleData.replace(/`/g, '"')); + } catch (e) { + console.warn( + "Not recording a canvas-element alternate for lang " + + canvasElementLang + + ": its data-bubble could not be read (" + + bubbleData.slice(0, 60) + + ")", + ); + return; + } const alternate = { lang: canvasElementLang, style: canvasElement.getAttribute("style") ?? "", diff --git a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementBackgroundImageManager.test.ts b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementBackgroundImageManager.test.ts index 4e7060668456..a0b772d5186b 100644 --- a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementBackgroundImageManager.test.ts +++ b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementBackgroundImageManager.test.ts @@ -1,7 +1,7 @@ import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; -// A faithful stand-in for the requestPageContent delay bookkeeping in bloomEditing.ts, which we -// can't import here (it pulls in the whole editing world). What matters for this suite is the +// A faithful stand-in for the page-content delay register in pageContentDelays.ts, modelling the +// save it gates. What matters for this suite is the // ordering the real code has: a save requested while delays are active is captured // SYNCHRONOUSLY, the instant the last delay is released, before any promise continuation of // the work that held the delay gets to run. vi.hoisted so the mock factory below (which vitest @@ -32,7 +32,7 @@ const saveTracker = vi.hoisted(() => ({ }, })); -vi.mock("../bloomEditing", () => ({ +vi.mock("../pageContentDelays", () => ({ wrapWithRequestPageContentDelay: async ( fn: () => Promise, _delayId: string, diff --git a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementBackgroundImageManager.ts b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementBackgroundImageManager.ts index 51240c582b91..50c5b0e45d38 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 { @@ -322,7 +322,8 @@ 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, as the minLevel computation above does; else this is 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..893ce4d7a064 100644 --- a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementBubbleLevelUtils.ts +++ b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementBubbleLevelUtils.ts @@ -28,7 +28,8 @@ 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, as the minLevel computation above does; else this is 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 e9e8a4362c6f..ecba76daf003 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 51bd7bc3a1a2..e88665e37efe 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 4e6021e284b9..5698cde7f4fb 100644 --- a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementFactories.ts +++ b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementFactories.ts @@ -938,7 +938,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 43b08709a754..668c3556925c 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, @@ -2608,6 +2610,52 @@ export class CanvasElementManager { ); } + // Put into 'clonedPage', a detached copy of the live .bloom-page, what + // turnOffCanvasElementEditing() would put into the page, leaving the live page still editable. + // Only three of the things that method does affect what gets saved: + // * Comical's editing becomes the that draws the bubbles without Javascript. + // exportSvgToCopiesOfParents writes it into the copy without disturbing the live editing. + // * Canvas element positions are recorded as the current language's alternate. That only + // reads and writes attributes, so it works on a clone, which has no layout. + // * The bloom-focusedCanvasElement class comes off; it is not bloom-ui, so C# would keep it. + // The rest (control frame, image editing buttons, listeners) is bloom-ui or live-only. + public prepareCloneOfPageForSave(clonedPage: HTMLElement): void { + const liveBloomCanvases = this.getAllBloomCanvasesOnPage(); + const clonedBloomCanvases = Array.from( + clonedPage.getElementsByClassName(kBloomCanvasClass), + ) as HTMLElement[]; + if (liveBloomCanvases.length !== clonedBloomCanvases.length) { + throw new Error( + `prepareCloneOfPageForSave(): 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( + clonedPage.getElementsByClassName("bloom-focusedCanvasElement"), + ).forEach((element) => + element.classList.remove("bloom-focusedCanvasElement"), + ); + + // Comical takes one canvas out of each bloom-canvas's copy, but a drawing surface it has + // forgotten can be another (see removeOrphanedDrawingSurfaces), and must not be saved. + Array.from( + clonedPage.querySelectorAll("canvas.comical-generated"), + ).forEach((surface) => surface.remove()); + } + /** * Take out any drawing surface Comical left behind. * diff --git a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementResizeAdjustments.test.ts b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementResizeAdjustments.test.ts new file mode 100644 index 000000000000..05a1d999f2b3 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementResizeAdjustments.test.ts @@ -0,0 +1,52 @@ +import { describe, it, expect } from "vitest"; +import { BubbleSpec } from "comicaljs"; +import { comicalDrawsSomethingFor } from "./CanvasElementResizeAdjustments"; + +// A plain text box: style "none", no tails, nothing chosen for its colours. +function plainTextBox(extra: Partial = {}): BubbleSpec { + return { version: "1.0", style: "none", tails: [], level: 1, ...extra }; +} + +describe("comicalDrawsSomethingFor", () => { + it("is false for a plain text box, which Comical draws nothing for", () => { + expect(comicalDrawsSomethingFor(plainTextBox())).toBe(false); + }); + + it("is true for any bubble style", () => { + expect( + comicalDrawsSomethingFor(plainTextBox({ style: "speech" })), + ).toBe(true); + expect( + comicalDrawsSomethingFor(plainTextBox({ style: "caption" })), + ).toBe(true); + }); + + it("is true for a plain text box with a tail", () => { + const tail = { tipX: 1, tipY: 2, midpointX: 3, midpointY: 4 }; + expect(comicalDrawsSomethingFor(plainTextBox({ tails: [tail] }))).toBe( + true, + ); + }); + + it("is true for a plain text box with a background colour, which Comical paints in a box", () => { + expect( + comicalDrawsSomethingFor( + plainTextBox({ backgroundColors: ["#ffff00"] }), + ), + ).toBe(true); + }); + + it("is false for a plain text box whose background is explicitly transparent", () => { + expect( + comicalDrawsSomethingFor( + plainTextBox({ backgroundColors: ["transparent"] }), + ), + ).toBe(false); + }); + + it("is true for a plain text box with an outer border colour", () => { + expect( + comicalDrawsSomethingFor(plainTextBox({ outerBorderColor: "red" })), + ).toBe(true); + }); +}); diff --git a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementResizeAdjustments.ts b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementResizeAdjustments.ts index 5a58121ce008..99050d518784 100644 --- a/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementResizeAdjustments.ts +++ b/src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementResizeAdjustments.ts @@ -1,7 +1,7 @@ // Stateless helpers for resizing/repositioning canvas children when the // underlying bloom-canvas changes size. -import { Bubble, Comical, TailSpec } from "comicaljs"; +import { Bubble, BubbleSpec, Comical, TailSpec } from "comicaljs"; import { getImageFromCanvasElement, isPlaceHolderImage } from "../bloomImages"; import { kBackgroundImageClass, @@ -10,6 +10,19 @@ import { import { pxToNumber } from "../../toolbox/canvas/canvasElementCssUtils"; import { adjustCanvasElementAlternates } from "./CanvasElementAlternates"; +// Whether Comical draws anything for this canvas element, and so has something to redraw when it +// moves or changes size. Style "none" (a plain text box) has no outline, but Comical still paints +// a box sized from the text box with the spec's background and outer border colours. So only a +// plain text box with no tails and neither colour has nothing to redraw. +export function comicalDrawsSomethingFor(bubbleSpec: BubbleSpec): boolean { + if (bubbleSpec.style !== "none") return true; + if (bubbleSpec.tails?.length) return true; + if (bubbleSpec.outerBorderColor) return true; + return (bubbleSpec.backgroundColors ?? []).some( + (color) => color !== "transparent", + ); +} + function updateBloomCanvasSizeData(bloomCanvas: HTMLElement): void { bloomCanvas.setAttribute( // originally data-imgSizeBasedOn, but that is technically invalid @@ -156,9 +169,7 @@ export function adjustCanvasElementChildrenIfSizeChanged( let reposition = true; const bubbleSpec = Bubble.getBubbleSpec(child); needComicalUpdate = - needComicalUpdate || - (!!bubbleSpec.tails && bubbleSpec.tails.length > 0) || - bubbleSpec.spec !== "none"; + needComicalUpdate || comicalDrawsSomethingFor(bubbleSpec); 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 d3f9c2484a0c..f86eaccaaf27 100644 --- a/src/BloomBrowserUI/bookEdit/js/editableDivUtils.ts +++ b/src/BloomBrowserUI/bookEdit/js/editableDivUtils.ts @@ -1,6 +1,10 @@ /// -import { get, postString, postThatMightNavigate } from "../../utils/bloomApi"; +import { get, postString } from "../../utils/bloomApi"; import $ from "jquery"; +import { + postAfterSendingSnapshot, + saveChangesAndRethinkPage, +} from "./pageSnapshot"; interface qtipInterface extends JQuery { qtip(options: string): JQuery; @@ -281,9 +285,8 @@ export class EditableDivUtils { // to hand the text over: it stores the current wording in the data div, stops generating the // sentence, and rebuilds the credits page with that spot as an ordinary editable field. // The server saves the page and reloads it, so we have nothing to change here ourselves. - // The reload can cut the request off, which must not be reported as an error. - public static unlockOriginalCredits() { - postThatMightNavigate( + public static unlockOriginalCredits(): Promise { + return postAfterSendingSnapshot( "copyrightAndLicense/unlockOriginalCopyrightNotice", ); } @@ -291,8 +294,8 @@ export class EditableDivUtils { // Close the sentence about the original book again. The wording the user has just typed is // theirs from now on; saving the page stores it in the data div, and the page comes back // with that spot read-only, as it is on every other visit. - public static relockOriginalCredits() { - postThatMightNavigate("common/saveChangesAndRethinkPageEvent"); + public static relockOriginalCredits(): Promise { + return saveChangesAndRethinkPage(); } public static pasteImageCredits() { @@ -404,6 +407,43 @@ export class EditableDivUtils { return bookmarksForEachEditable; } + // The non-destructive counterpart of doCkEditorCleanup(): reads CKEditor's data from the live + // editors and writes it into the corresponding divs of a detached CLONE, because writing it + // back into the live divs disturbs the running editors. + // 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 why it is used exactly as it comes (BL-16843). + 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 = ckeditorOfThisBox.getData(); + 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/editableDivUtilsSpec.ts b/src/BloomBrowserUI/bookEdit/js/editableDivUtilsSpec.ts index 6b7765bf36fe..f3e60be7eeaa 100644 --- a/src/BloomBrowserUI/bookEdit/js/editableDivUtilsSpec.ts +++ b/src/BloomBrowserUI/bookEdit/js/editableDivUtilsSpec.ts @@ -1,11 +1,15 @@ import { describe, it, expect, beforeEach, vi } from "vitest"; import $ from "jquery"; import { EditableDivUtils } from "./editableDivUtils"; -import { postThatMightNavigate } from "../../utils/bloomApi"; +import { + postAfterSendingSnapshot, + saveChangesAndRethinkPage, +} from "./pageSnapshot"; -vi.mock("../../utils/bloomApi", async (importOriginal) => ({ +vi.mock("./pageSnapshot", async (importOriginal) => ({ ...((await importOriginal()) as object), - postThatMightNavigate: vi.fn(), + postAfterSendingSnapshot: vi.fn(), + saveChangesAndRethinkPage: vi.fn(), })); describe("EditableDivUtils Tests", () => { @@ -223,6 +227,52 @@ describe("EditableDivUtils Tests", () => { expect(div.innerHTML).toEqual(ckEditorData); }); + // copyCkEditorDataToClone runs on every save gather of a page with a text box. These give the + // live box a stand-in editor; without one the function returns early and none of it runs. + const pageWithEditor = (liveHtml: string, editorData: string) => { + const live = document.createElement("div"); + live.innerHTML = `
${liveHtml}
`; + const liveBox = live.querySelector("div.bloom-editable")!; + (liveBox as HTMLElement & { bloomCkEditor?: object }).bloomCkEditor = { + getData: () => editorData, + }; + const clone = live.cloneNode(true) as HTMLElement; + return { live, liveBox, clone }; + }; + + it("copyCkEditorDataToClone writes the editor's data into the clone and leaves the live page alone", () => { + const { live, liveBox, clone } = pageWithEditor( + "

typeddebris

", + "

typed

", + ); + const liveBefore = live.innerHTML; + + EditableDivUtils.copyCkEditorDataToClone(live, clone); + + expect(clone.querySelector("div.bloom-editable")!.innerHTML).toBe( + "

typed

", + ); + expect(live.innerHTML, "the live page must not change").toBe( + liveBefore, + ); + expect(liveBox.innerHTML).toBe("

typeddebris

"); + }); + + it("copyCkEditorDataToClone keeps U+200B word breaks that ckeditor's getData() reports", () => { + const zwsp = String.fromCharCode(0x200b); + const wordsWithBreak = `

a${zwsp}b

`; + const { live, clone } = pageWithEditor( + `

a${zwsp}b${zwsp}

`, + wordsWithBreak, + ); + + EditableDivUtils.copyCkEditorDataToClone(live, clone); + + expect(clone.querySelector("div.bloom-editable")!.innerHTML).toBe( + wordsWithBreak, + ); + }); + it("doCkEditorCleanup keeps U+200B word breaks that ckeditor's getData() reports", () => { const zwsp = String.fromCharCode(0x200b); const wordsWithBreak = `

a${zwsp}b

`; @@ -729,12 +779,12 @@ describe("EditableDivUtils.unlockOriginalCredits", () => { }); // All the work happens on the server, which stores the current wording, stops generating - // the sentence, and reloads the page with an editable field in its place. All this side - // has to get right is the endpoint name. - it("asks the server to hand the notice over to the user", () => { - EditableDivUtils.unlockOriginalCredits(); + // the sentence, and reloads the page with an editable field in its place. The server saves + // the page first, so it has to have the page as it is now. + it("asks the server to hand the notice over to the user, once it has the page", () => { + void EditableDivUtils.unlockOriginalCredits(); - expect(postThatMightNavigate).toHaveBeenCalledWith( + expect(postAfterSendingSnapshot).toHaveBeenCalledWith( "copyrightAndLicense/unlockOriginalCopyrightNotice", ); }); @@ -742,10 +792,8 @@ describe("EditableDivUtils.unlockOriginalCredits", () => { // The server saves the page, which stores what the user typed, and reloads it with the // sentence read-only again. it("relocking asks the server to save and reload the page", () => { - EditableDivUtils.relockOriginalCredits(); + void EditableDivUtils.relockOriginalCredits(); - expect(postThatMightNavigate).toHaveBeenCalledWith( - "common/saveChangesAndRethinkPageEvent", - ); + expect(saveChangesAndRethinkPage).toHaveBeenCalled(); }); }); diff --git a/src/BloomBrowserUI/bookEdit/js/editorChromeCleanup.spec.ts b/src/BloomBrowserUI/bookEdit/js/editorChromeCleanup.spec.ts new file mode 100644 index 000000000000..90d42160c4be --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/editorChromeCleanup.spec.ts @@ -0,0 +1,144 @@ +import { describe, it, expect } from "vitest"; +import { removeEditorChromeFromClone } from "./editorChromeCleanup"; + +// A clone of an xmatter page as it looks once the editor has finished waking up on it, with what +// CKEditor and qTip added inside it. (Their toolbars and bubbles go in the body, outside the page, +// so they are never in the clone.) Wrapped in a div so the tests can find the page in it. +function makeClonedBodyWithChrome(): HTMLElement { + const body = document.createElement("div"); + body.innerHTML = ` +
+
+
+ +
+ +
+
+
+

The title

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

ab

`; + + removeEditorChromeFromClone(body); + + expect(body.querySelector("#cke_bm_71S")).not.toBeNull(); + expect(body.querySelector("p")!.textContent).toBe("ab"); + }); +}); diff --git a/src/BloomBrowserUI/bookEdit/js/editorChromeCleanup.ts b/src/BloomBrowserUI/bookEdit/js/editorChromeCleanup.ts new file mode 100644 index 000000000000..41a0398f52ae --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/editorChromeCleanup.ts @@ -0,0 +1,59 @@ +// Strip, from a CLONE of the .bloom-page element, the chrome that only exists because the page is +// being edited -- so that what we hand C# is the page, not the editor. +// +// This is the one place the editor's chrome is removed before saving; C# +// (HtmlDom.ProcessPageAfterEditing) relies on it. Doing it here also keeps chrome out of what the +// page snapshot compares and sends (see pageSnapshot.ts), where it would make untouched pages look +// edited and cause a save on the way out. +// +// CKEditor's toolbars and qTip's bubbles are appended to the body, outside the page, so they never +// reach the clone. What is inside it: bloom-ui elements, resize handles, cke_ classes, and qTip's +// bookkeeping attributes, which churn between runs (the number in "qtip-0" depends on the order +// bubbles happen to be created). Books saved in the past may still contain those attributes. +// +// Nothing here may touch the live page; the caller passes a detached deep copy of it. +export function removeEditorChromeFromClone(clonedPage: HTMLElement) { + for (const element of Array.from( + clonedPage.querySelectorAll(".bloom-ui, .ui-resizable-handle"), + )) { + element.remove(); + } + + // Only qtip-* values, so that an aria-describedby someone put there on purpose survives. + for (const element of Array.from( + clonedPage.querySelectorAll( + "[aria-describedby], [data-hasqtip], [ariasecondary-describedby]", + ), + )) { + if (element.getAttribute("aria-describedby")?.startsWith("qtip-")) + element.removeAttribute("aria-describedby"); + if ( + element + .getAttribute("ariasecondary-describedby") + ?.startsWith("qtip-") + ) + element.removeAttribute("ariasecondary-describedby"); + element.removeAttribute("data-hasqtip"); + } + + // The ids paper.js leaves on the SVG Comical draws for speech bubbles. The SVG itself IS saved + // (the reader has no Comical to redraw bubbles), but these ids get a fresh GUID on every + // redraw, so any page with a bubble would look edited on every visit. Safe to drop: nothing + // references them (no url(#...) or href="#..."), and they are not even unique. + for (const element of Array.from( + clonedPage.querySelectorAll("svg.comical-generated [id]"), + )) { + element.removeAttribute("id"); + } + + // The classes CKEditor adds to each editable it attaches to (cke_editable, cke_focus, ...). + for (const element of Array.from( + clonedPage.querySelectorAll("[class*='cke_']"), + )) { + const kept = Array.from(element.classList).filter( + (c) => !c.startsWith("cke_"), + ); + if (kept.length === 0) element.removeAttribute("class"); + else element.setAttribute("class", kept.join(" ")); + } +} diff --git a/src/BloomBrowserUI/bookEdit/js/imageTargetResolution.ts b/src/BloomBrowserUI/bookEdit/js/imageTargetResolution.ts index d3872149a763..ba1100ef645b 100644 --- a/src/BloomBrowserUI/bookEdit/js/imageTargetResolution.ts +++ b/src/BloomBrowserUI/bookEdit/js/imageTargetResolution.ts @@ -307,7 +307,7 @@ export function getElementThatDeterminesImageSlotSize( // Writes each image slot's share of its page onto the slot, so that the size it wants can be // worked out later for a page nobody has open. Called as a page is saved (see -// extractAndStripPageContentForSave in bloomEditing.ts): the ordinary Edit-tab save, and the +// cleanCloneOfPageForSave in bloomEditing.ts): the ordinary Edit-tab save, and the // off-screen pass over every page that "Update Book" runs on demand and that Bloom runs by itself // when the AI image editor is launched on a book that has not had it (BL-16852). // @@ -322,42 +322,60 @@ export function getElementThatDeterminesImageSlotSize( // passes above put it right in one go -- and a page-size change makes the book due for that pass // again, so the next AI image editor launch re-measures every slot at the new size. Deliberately // not re-measured any more eagerly than that. -export function recordFractionOfPageOnImageSlots(pageRoot: Element): void { +// +// The measurements come from pageRoot, which must be laid out. The attribute is written to the +// corresponding slot under writeTo (default pageRoot); the live editor's save passes its detached +// clone so the live page is not changed. writeTo must then be an untouched copy of pageRoot, so +// that the Nth slot in each is the same slot. +export function recordFractionOfPageOnImageSlots( + pageRoot: Element, + writeTo: Element = pageRoot, +): void { const pageElement = pageRoot.closest(".bloom-page") ?? pageRoot.querySelector(".bloom-page"); if (!pageElement) return; const page = getOpenPageMetrics(pageElement); if (!page) return; - Array.from(pageRoot.querySelectorAll("." + kImageContainerClass)).forEach( - (slot) => { - // Bloom injects controls into the live page; they are not slots, and a save strips - // them anyway. - if (slot.closest(".bloom-ui")) return; - // A Bloom Games target holds a copy of its draggable's content, so its picture is - // not separately editable and the AI image editor is never offered it, nor even - // counted when the page's slots are numbered (C#'s IsSlotInsideGameTarget and - // imageSlotsOnPage, which look for the same ancestor attribute). - if (slot.parentElement?.closest(kGameTargetSelector)) return; - const container = slot as HTMLElement; - // The value always goes ON the container, because that is what C# enumerates - // (EnumerateBookImages), but for a canvas background it is the canvas that gets - // measured. - const box = getElementThatDeterminesImageSlotSize(container); - if ( - !isUsableLength(box.offsetWidth) || - !isUsableLength(box.offsetHeight) - ) { - return; - } - const width = roundFraction(box.offsetWidth / page.widthPx); - const height = roundFraction(box.offsetHeight / page.heightPx); - container.setAttribute( - kFractionOfPageAttribute, - `${width},${height}`, - ); - }, + const liveSlots = Array.from( + pageRoot.querySelectorAll("." + kImageContainerClass), ); + const targetSlots = + writeTo === pageRoot + ? liveSlots + : Array.from(writeTo.querySelectorAll("." + kImageContainerClass)); + if (targetSlots.length !== liveSlots.length) { + throw new Error( + `recordFractionOfPageOnImageSlots(): the copy has ${targetSlots.length} image slots but the live page has ${liveSlots.length}. The copy must be an untouched copy of the live page.`, + ); + } + liveSlots.forEach((slot, index) => { + // Bloom injects controls into the live page; they are not slots, and a save strips + // them anyway. + if (slot.closest(".bloom-ui")) return; + // A Bloom Games target holds a copy of its draggable's content, so its picture is + // not separately editable and the AI image editor is never offered it, nor even + // counted when the page's slots are numbered (C#'s IsSlotInsideGameTarget and + // imageSlotsOnPage, which look for the same ancestor attribute). + if (slot.parentElement?.closest(kGameTargetSelector)) return; + const container = slot as HTMLElement; + // The value always goes ON the container, because that is what C# enumerates + // (EnumerateBookImages), but for a canvas background it is the canvas that gets + // measured. + const box = getElementThatDeterminesImageSlotSize(container); + if ( + !isUsableLength(box.offsetWidth) || + !isUsableLength(box.offsetHeight) + ) { + return; + } + const width = roundFraction(box.offsetWidth / page.widthPx); + const height = roundFraction(box.offsetHeight / page.heightPx); + targetSlots[index].setAttribute( + kFractionOfPageAttribute, + `${width},${height}`, + ); + }); } // Reads back what recordFractionOfPageOnImageSlots wrote. Null for anything that is not two 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..a7ae65cc3214 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/niceScrollCleanup.ts @@ -0,0 +1,88 @@ +import { kSelectorForPotentialNiceScrollElements } from "bloom-player"; + +// The elements niceScroll inserts. Each rail contains a cursor (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. + * + * Unlike bloom-player's cleanupNiceScroll(), which asks each live niceScroll instance to remove + * itself, this works on any root, including a detached clone, so saving leaves the live page alone. + * + * There are three kinds of leftovers: + * + * 1. The .nicescroll-rails divs (each containing a .nicescroll-cursors div). niceScroll appends + * them to the nearest positioned or scrollable ancestor, which can be inside the page (e.g. an + * origami split-pane component). Rails appended to the body are outside the page, so never saved. + * + * 2. Classes addScrollbarsToPage() changed, because niceScroll does not work with the display:flex + * of vertical alignment: bloom-vertical-align-center/-bottom moved aside to a "-removed" marker + * on the translationGroup, and scrolling-bubble added to a canvas element's editable. Saving + * these would silently lose the user's vertical alignment choice. + * + * 3. Inline styles niceScroll sets on the box it scrolls without recording them for restoring: + * overflow-x/-y, outline, and a pixel width (a Chrome workaround it does not always undo, + * BL-14052). (position:relative is set only in the two-argument niceScroll() form, which + * bloom-player does not use.) + */ +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 remove the class it selects on, so copy it 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 (nothing in Bloom sets one). Checking + // for it means we can't blank an author's inline width on a box niceScroll never touched. + if (!scrollBox.style.overflowY) { + continue; + } + // Longhands named explicitly: whether clearing a shorthand clears its longhands varies + // between CSSOM implementations (jsdom, where our tests run, does not). + 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 65554359a80f..f1ffee1fc251 100644 --- a/src/BloomBrowserUI/bookEdit/js/origami.ts +++ b/src/BloomBrowserUI/bookEdit/js/origami.ts @@ -2,7 +2,8 @@ import { SetupImage } from "./bloomImages"; import { kBloomCanvasClass } from "../toolbox/canvas/canvasElementPageBridge"; import "../../lib/split-pane/split-pane.js"; import TextBoxProperties from "../TextBoxProperties/TextBoxProperties"; -import { post, postThatMightNavigate } from "../../utils/bloomApi"; +import { post } from "../../utils/bloomApi"; +import { saveChangesAndRethinkPage } from "./pageSnapshot"; import { theOneCanvasElementManager } from "./canvasElementManager/CanvasElementManager"; import { FeatureStatus, @@ -209,7 +210,7 @@ function changeLayoutModeToggleClickHandler() { const toggleTransitionLength = 450; setTimeout(() => { $("html").off("keydown.origami"); - postThatMightNavigate("common/saveChangesAndRethinkPageEvent"); + void saveChangesAndRethinkPage(); }, toggleTransitionLength); } } diff --git a/src/BloomBrowserUI/bookEdit/js/pageContentCapturePolicy.test.ts b/src/BloomBrowserUI/bookEdit/js/pageContentCapturePolicy.test.ts index 39a23a0edf5b..a596aa5b47b3 100644 --- a/src/BloomBrowserUI/bookEdit/js/pageContentCapturePolicy.test.ts +++ b/src/BloomBrowserUI/bookEdit/js/pageContentCapturePolicy.test.ts @@ -32,7 +32,7 @@ describe("externalCaptureErrorForPendingWork (BL-16870)", () => { }); test("waits far longer than the live editor's 4 seconds but stays under the C# poll timeout", () => { - // The live editor's cap is 4000 ms (bloomEditing.ts kMaxWaitTimeMs); BookProcessor polls for + // The live editor's cap is 4000 ms (pageContentDelays.ts kMaxWaitTimeMs); BookProcessor polls for // our answer for 60000 ms. A regression in either direction would either give a slow image // no more grace than the live save does, or have C# time out before the browser answers. expect(kExternalCaptureMaxWaitMs).toBeGreaterThan(4000); diff --git a/src/BloomBrowserUI/bookEdit/js/pageContentDelays.spec.ts b/src/BloomBrowserUI/bookEdit/js/pageContentDelays.spec.ts new file mode 100644 index 000000000000..bdabe4a73f89 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/pageContentDelays.spec.ts @@ -0,0 +1,195 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { + addRequestPageContentDelay, + getActiveDelayIds, + kMaxWaitTimeMs, + onDelayRegisterChanged, + removeRequestPageContentDelay, + whenNoActiveDelays, + wrapWithRequestPageContentDelay, +} from "./pageContentDelays"; + +// The gate that keeps a save from reading a page that is still being changed. Everything that +// gathers page content waits on whenNoActiveDelays(), so if this is wrong, half-finished work +// (an image still being sized, a paste still in progress) gets written into the user's book. + +// Has the promise settled? Attaches a callback and then lets the microtask queue drain, which is +// enough for a promise that is already resolved (or resolves synchronously from a call we just +// made) and not enough for one still waiting on a timer. +const isResolved = async (p: Promise): Promise => { + let resolved = false; + void p.then(() => { + resolved = true; + }); + for (let i = 0; i < 5; i++) await Promise.resolve(); + return resolved; +}; + +describe("pageContentDelays", () => { + beforeEach(() => { + vi.useFakeTimers(); + // Sanity check: nothing left over from another test, or the assertions below are meaningless. + expect(getActiveDelayIds()).toEqual([]); + }); + + afterEach(() => { + vi.useRealTimers(); + if (getActiveDelayIds().length) + throw new Error( + "test leaked delays: " + getActiveDelayIds().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(getActiveDelayIds()).toEqual(["theWork"]); + expect(await isResolved(gate)).toBe(false); + + releaseTheWork!(); + await wrapped; + + expect(await isResolved(gate)).toBe(true); + expect(getActiveDelayIds()).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(getActiveDelayIds()).toEqual([]); + expect(await isResolved(whenNoActiveDelays())).toBe(true); + }); + + it("tells a listener when the register becomes busy, and when it empties, but not in between", () => { + // The page snapshot relays these to C#, which waits for the idle one before a save that + // uses the snapshot. Every add and remove would be noise; the transitions are the signal. + const heard: (string | undefined)[] = []; + const unsubscribe = onDelayRegisterChanged((busyWith) => + heard.push(busyWith), + ); + + addRequestPageContentDelay("sizing an image"); + addRequestPageContentDelay("settling a paste"); + expect(heard).toEqual(["sizing an image"]); + + removeRequestPageContentDelay("sizing an image"); + expect(heard, "still busy with the paste").toEqual(["sizing an image"]); + removeRequestPageContentDelay("settling a paste"); + expect(heard).toEqual(["sizing an image", undefined]); + + unsubscribe(); + addRequestPageContentDelay("later work"); + removeRequestPageContentDelay("later work"); + expect(heard, "nothing more after unsubscribing").toEqual([ + "sizing an image", + undefined, + ]); + }); + it("tells a listener that subscribes while the register is already busy", () => { + // The page snapshot subscribes after bootstrap(), when load-time work has usually already + // registered; a listener that heard only transitions would miss all of it. + addRequestPageContentDelay("sizing an image"); + const heard: (string | undefined)[] = []; + const unsubscribe = onDelayRegisterChanged((busyWith) => + heard.push(busyWith), + ); + + expect(heard).toEqual(["sizing an image"]); + + removeRequestPageContentDelay("sizing an image"); + expect(heard).toEqual(["sizing an image", undefined]); + unsubscribe(); + }); + it("complains about, and ignores, a removal of something never registered", () => { + const error = vi.spyOn(console, "error").mockImplementation(() => {}); + addRequestPageContentDelay("realWork"); + + removeRequestPageContentDelay("neverRegistered"); + + expect(error).toHaveBeenCalled(); + expect(getActiveDelayIds()).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..9fec56823a20 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/pageContentDelays.ts @@ -0,0 +1,128 @@ +// The register of asynchronous work that must finish before the page can be saved, and the gate +// every page-content-gathering path waits on. +// +// Much of the editor changes the DOM asynchronously (sizing an image, fitting a canvas element's +// background, pasting, building a custom xmatter page); a page read while one of those is half done +// is what gets written into the book. So such work registers here for its duration (preferably via +// wrapWithRequestPageContentDelay), and every route that gathers page content waits on +// whenNoActiveDelays() first: getPageContentForSaveWhenReady in bloomEditing.ts (used by the page +// snapshot) and the off-screen book processor (captureContentForExternalProcessing). C#, which +// saves from the snapshot, is told when the register is busy (see onDelayRegisterChanged). + +// Upper bound (not a fixed wait) on how long we wait for in-flight async DOM work to finish before +// gathering anyway; generous to give slow computers with complex pages headroom. +export const kMaxWaitTimeMs = 4000; + +const activeDelays: string[] = []; + +// Callbacks waiting for activeDelays to empty; see whenNoActiveDelays(). +const delayWaiters: (() => void)[] = []; + +// Told when the register goes from empty to busy (with describeBusyRegister()) and back to empty +// (with undefined). See onDelayRegisterChanged. +const registerListeners: ((busyWith: string | undefined) => void)[] = []; + +// Be told when the register becomes busy and when it empties again (only those transitions; the +// listener needs to know WHETHER the page is busy). Returns a function that unsubscribes. +// +// What the busy notice says the page is busy with is only a clue for the log: it is the work +// registered at that moment, not a running record, which would cost a request per add and remove. +// +// If the register is already busy, the listener is told at once: the page snapshot subscribes +// after bootstrap(), by which time load-time work (image sizing, CKEditor attaching) has usually +// registered. +export function onDelayRegisterChanged( + listener: (busyWith: string | undefined) => void, +): () => void { + registerListeners.push(listener); + if (activeDelays.length > 0) listener(describeBusyRegister()); + return () => { + const index = registerListeners.indexOf(listener); + if (index >= 0) registerListeners.splice(index, 1); + }; +} + +// Register asynchronous work whose results belong in the saved page. The caller must pass the same +// id to removeRequestPageContentDelay when the work finishes -- see wrapWithRequestPageContentDelay, +// which does that for you. IDs do not need to be unique; the same ID can be added multiple times. +export function addRequestPageContentDelay(id: string): void { + activeDelays.push(id); + if (activeDelays.length === 1) { + const busyWith = describeBusyRegister(); + registerListeners.forEach((listener) => listener(busyWith)); + } +} + +function describeBusyRegister(): string { + return activeDelays.join(", "); +} + +// Deregister work, releasing anyone waiting if this was the last of it. +export function removeRequestPageContentDelay(id: string): void { + const index = activeDelays.indexOf(id); + if (index === -1) { + console.error( + `removeRequestPageContentDelay: ID "${id}" not found in active delays. Active delays: [${activeDelays.join( + ", ", + )}]`, + ); + return; + } + activeDelays.splice(index, 1); + + if (activeDelays.length === 0) { + // Take the list before calling anyone, so that a waiter which starts new work (and so + // registers a new delay) does not get released a second time by that work finishing. + delayWaiters.splice(0).forEach((release) => release()); + registerListeners.forEach((listener) => listener(undefined)); + } +} + +// Run some asynchronous work with its delay registered for the duration, whether it succeeds or +// throws. Prefer this to the add/remove pair: a delay that is never removed blocks every save for +// kMaxWaitTimeMs and then gets overridden anyway. +export async function wrapWithRequestPageContentDelay( + fn: () => Promise, + delayId: string, +): Promise { + addRequestPageContentDelay(delayId); + try { + return await fn(); + } finally { + removeRequestPageContentDelay(delayId); + } +} + +// Resolves once no registered work is outstanding: immediately if there is none, otherwise as soon +// as the last of it finishes, and after maxWaitMs regardless (saving a slightly stale page beats +// blocking the user forever). The off-screen capture, with nobody waiting at the keyboard, passes a +// longer cap and then checks getActiveDelayIds() to decide whether it can capture what is pending. +export function whenNoActiveDelays( + maxWaitMs: number = kMaxWaitTimeMs, +): 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 ${maxWaitMs}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(); + }, maxWaitMs); + }); +} + +// What is currently registered. For tests, diagnostics, and the off-screen capture's decision +// about what it may capture half-done (see whenNoActiveDelays). +export function getActiveDelayIds(): string[] { + return [...activeDelays]; +} diff --git a/src/BloomBrowserUI/bookEdit/js/pageSnapshot.spec.ts b/src/BloomBrowserUI/bookEdit/js/pageSnapshot.spec.ts new file mode 100644 index 000000000000..a8b1ea4a0d15 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/pageSnapshot.spec.ts @@ -0,0 +1,662 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { + notePageContentMayHaveChanged, + startWatchingPageForSnapshots, + stopWatchingPageForSnapshots, + quietMsForTests, + sendSnapshotNow, + postAfterSendingSnapshot, + getPageLoadId, +} from "./pageSnapshot"; +import { + addRequestPageContentDelay, + removeRequestPageContentDelay, + whenNoActiveDelays, +} from "./pageContentDelays"; + +const posted: Array<{ url: string; body: string }> = []; + +// Lets a test hold a POST open, to check that a second one never starts alongside it. +let postHook: (() => Promise) | undefined; + +// What C# answers. A real post resolves to the axios response, and this endpoint answers with a +// boolean, so `{ data: true }` is an ordinary success. `{ data: false }` is a refusal: the snapshot +// was for a page load it is not showing. And `undefined` -- no response at all -- is what a FAILED +// post looks like, because postStringQuietly goes through wrapAxios, which turns a rejected request +// into a resolved promise carrying nothing. +let postReply: unknown = { data: true }; + +const reported: string[] = []; +vi.mock("../../lib/errorHandler", () => ({ + reportError: (message: string) => reported.push(message), +})); + +vi.mock("../../utils/bloomApi", () => ({ + postStringQuietly: (url: string, body: string) => { + posted.push({ url, body }); + return postHook ? postHook() : Promise.resolve(postReply); + }, + postThatMightNavigate: (url: string) => { + posted.push({ url, body: "" }); + return Promise.resolve(); + }, +})); + +// The page as the gather would report it. Tests change this to simulate the user editing. +let contentToReport = ""; +const gather = () => Promise.resolve(contentToReport); + +function setUpPage(pageId = "page-1") { + document.body.innerHTML = `

hello

`; +} + +function changeThePage(text: string) { + document.querySelector(".bloom-page p")!.textContent = text; +} + +// startWatching... sends the page as it has loaded, after the usual quiet time. This lets that +// post go out -- releasing the gather first, for a test whose gather the test holds -- and then +// forgets it, so that a test counts only what happens after loading. +async function letTheLoadedPageBeSent(releaseGather?: () => void) { + vi.advanceTimersByTime(quietMsForTests); + await vi.runAllTicks(); + releaseGather?.(); + await vi.runAllTicks(); + await Promise.resolve(); // the gather's await + await Promise.resolve(); // the post's await + await Promise.resolve(); + posted.length = 0; +} + +// A MutationObserver delivers its callback in a microtask, and the module then waits kQuietMs. +// This walks both forward. +async function letTheSnapshotHappen() { + await Promise.resolve(); // let the observer fire + vi.advanceTimersByTime(quietMsForTests); + await vi.runAllTicks(); + await Promise.resolve(); // the gather's await + await Promise.resolve(); // the post's await +} + +describe("pageSnapshot", () => { + beforeEach(() => { + vi.useFakeTimers(); + posted.length = 0; + reported.length = 0; + contentToReport = ""; + postHook = undefined; + postReply = { data: true }; + setUpPage(); + }); + + afterEach(() => { + stopWatchingPageForSnapshots(); + vi.useRealTimers(); + document.body.innerHTML = ""; + }); + + it("posts the page once when it has loaded, and then nothing while nobody changes it", async () => { + // Loading can itself change the page -- after a change of page size, images and canvas + // elements are laid out afresh -- and some of that happens before we start watching, where + // the observer cannot see it. So the page as loaded is always sent; C# decides whether it + // differs from the book. + contentToReport = "the untouched page"; + startWatchingPageForSnapshots(gather); + vi.advanceTimersByTime(quietMsForTests); + await vi.runAllTicks(); + await Promise.resolve(); + await Promise.resolve(); + expect(posted.length, "the loaded page is sent").toBe(1); + expect(posted[0].body).toBe("the untouched page"); + + vi.advanceTimersByTime(quietMsForTests * 5); + await vi.runAllTicks(); + expect(posted.length, "and nothing more while nobody changes it").toBe( + 1, + ); + }); + + it("sends what loading changed in that one post, not a post per change", async () => { + // Image sizing and canvas-element layout finish after bootstrap() and mutate the page; + // those results belong in the book, but they should arrive as the settled page, once. + startWatchingPageForSnapshots(gather); + changeThePage("a late load-time fix-up"); + await Promise.resolve(); + changeThePage("and another"); + contentToReport = "the settled page"; + await letTheSnapshotHappen(); + expect(posted.map((p) => p.body)).toEqual(["the settled page"]); + + changeThePage("a mutation that does not change the saved form"); + await letTheSnapshotHappen(); + expect( + posted.length, + "mutations that do not change the page's saved form are not sent", + ).toBe(1); + }); + + it("posts the content, with the page id, once the page has been changed and settles", async () => { + startWatchingPageForSnapshots(gather); + await letTheLoadedPageBeSent(); + contentToReport = "edited content"; + changeThePage("goodbye"); + await letTheSnapshotHappen(); + + expect(posted.length).toBe(1); + expect(posted[0].body).toBe("edited content"); + expect(posted[0].url).toContain("editView/pageSnapshot"); + expect(posted[0].url).toContain("pageId=page-1"); + }); + + it("does not post again when the content has not actually changed", async () => { + startWatchingPageForSnapshots(gather); + await letTheLoadedPageBeSent(); + contentToReport = "same every time"; + changeThePage("a"); + await letTheSnapshotHappen(); + expect(posted.length, "sanity: the first change posts").toBe(1); + + // Tools constantly add and remove editing decorations, which the gather strips. Those + // mutations must not produce a stream of identical posts. + changeThePage("b"); + await letTheSnapshotHappen(); + + expect(posted.length).toBe(1); + }); + + it("stops posting once the page is unloaded", async () => { + startWatchingPageForSnapshots(gather); + await letTheLoadedPageBeSent(); + contentToReport = "first"; + changeThePage("a"); + await letTheSnapshotHappen(); + expect(posted.length, "sanity: it was posting before we stopped").toBe( + 1, + ); + + stopWatchingPageForSnapshots(); + contentToReport = "second"; + changeThePage("b"); + await letTheSnapshotHappen(); + + expect(posted.length).toBe(1); + }); + + it("waits for the page to be quiet rather than posting per change", async () => { + startWatchingPageForSnapshots(gather); + await letTheLoadedPageBeSent(); + contentToReport = "typed a word"; + + // Three changes in quick succession, as typing produces. + changeThePage("a"); + await Promise.resolve(); + vi.advanceTimersByTime(quietMsForTests / 4); + changeThePage("ab"); + await Promise.resolve(); + vi.advanceTimersByTime(quietMsForTests / 4); + changeThePage("abc"); + await letTheSnapshotHappen(); + + expect( + posted.length, + "the debounce should collapse a burst of changes into one snapshot", + ).toBe(1); + expect(posted[0].body).toBe("typed a word"); + }); + + it("never has two posts in flight at once", async () => { + // HTTP does not promise that two outstanding POSTs arrive in the order they were sent, so + // an older snapshot could land after a newer one and C# would keep the older content. That + // needs a machine slow enough for a post to still be in flight when the next keystroke's + // snapshot comes round -- so it must be enforced, not left to timing. + startWatchingPageForSnapshots(gather); + await letTheLoadedPageBeSent(); + + let inFlight = 0; + let maxInFlight = 0; + let releasePost: () => void = () => {}; + postHook = () => + new Promise((resolve) => { + inFlight++; + maxInFlight = Math.max(maxInFlight, inFlight); + releasePost = () => { + inFlight--; + resolve({ data: true }); // an ordinary successful post + }; + }); + + contentToReport = "first"; + changeThePage("a"); + await Promise.resolve(); + vi.advanceTimersByTime(quietMsForTests); + await vi.runAllTicks(); + await Promise.resolve(); + expect(inFlight, "sanity: a post is outstanding").toBe(1); + + // More edits arrive while that post is still outstanding. + contentToReport = "second"; + changeThePage("b"); + await Promise.resolve(); + vi.advanceTimersByTime(quietMsForTests * 3); + await vi.runAllTicks(); + await Promise.resolve(); + + expect( + maxInFlight, + "a second post must not start while one is outstanding", + ).toBe(1); + + // Once it completes, the newer content still gets sent. + releasePost(); + await vi.runAllTicks(); + await Promise.resolve(); + vi.advanceTimersByTime(quietMsForTests); + await vi.runAllTicks(); + await Promise.resolve(); + releasePost(); + await vi.runAllTicks(); + await Promise.resolve(); + + expect( + posted.map((p) => p.body), + "the later edit must still reach C#, just after the first post finished", + ).toEqual(["first", "second"]); + }); + + it("takes another snapshot when the page changes while one is being gathered", async () => { + let release: (value: string) => void = () => {}; + let gatherCount = 0; + const slowGather = () => { + gatherCount++; + return new Promise((resolve) => { + release = resolve; + }); + }; + startWatchingPageForSnapshots(slowGather); + await letTheLoadedPageBeSent(() => release("the loaded page")); + expect(gatherCount, "sanity: the loaded page was gathered").toBe(1); + + changeThePage("a"); + await Promise.resolve(); + vi.advanceTimersByTime(quietMsForTests); + await vi.runAllTicks(); + expect(gatherCount, "sanity: a snapshot gather started").toBe(2); + + // While that gather is outstanding, the user types again. That change is not in what the + // gather is about to hand us, so it must not be silently dropped. + changeThePage("b"); + await Promise.resolve(); + + release("first content"); + await vi.runAllTicks(); + await Promise.resolve(); + await Promise.resolve(); + expect(posted.length, "sanity: the first gather posted").toBe(1); + + vi.advanceTimersByTime(quietMsForTests); + await vi.runAllTicks(); + expect( + gatherCount, + "the change that landed mid-gather must trigger another snapshot, not be dropped", + ).toBe(3); + }); + + // A gather that behaves like the real one in the respect these tests are about: it waits for + // the delay register to empty before it reads the page. + const gatherWhenIdle = async () => { + await whenNoActiveDelays(); + return contentToReport; + }; + + // Lets queued posts and the gathers behind them run to completion. + async function letEverythingSettle() { + await vi.runAllTicks(); + for (let i = 0; i < 8; i++) await Promise.resolve(); + } + + const kinds = () => + posted.map((p) => (p.url.includes("busy=true") ? "busy" : "snapshot")); + + it("tells C# when work begins, and sends the page once the work is done", async () => { + // A save C# makes from the snapshot meanwhile would miss the work, so it waits for the + // snapshot that follows; that snapshot is also what tells it the page is idle. + contentToReport = "before the work"; + startWatchingPageForSnapshots(gatherWhenIdle); + await letTheLoadedPageBeSent(); + + addRequestPageContentDelay("sizing an image"); + await letEverythingSettle(); + expect(kinds()).toEqual(["busy"]); + expect(posted[0].url).toContain("editView/pageSnapshot"); + expect(posted[0].body).toBe("sizing an image"); + + // The work changes the page; the snapshot for that waits for the work to finish. + contentToReport = "after the work"; + changeThePage("after the work"); + await letTheSnapshotHappen(); + expect(kinds(), "no snapshot while the page is busy").toEqual(["busy"]); + + removeRequestPageContentDelay("sizing an image"); + await letEverythingSettle(); + expect(kinds()).toEqual(["busy", "snapshot"]); + expect(posted[1].body).toBe("after the work"); + }); + + it("sends the page after work finishes even if the work changed nothing, because that is what says idle", async () => { + contentToReport = "unchanged"; + startWatchingPageForSnapshots(gatherWhenIdle); + await letTheLoadedPageBeSent(); + + addRequestPageContentDelay("settling a paste"); + await letEverythingSettle(); + removeRequestPageContentDelay("settling a paste"); + await letEverythingSettle(); + + expect(kinds()).toEqual(["busy", "snapshot"]); + expect(posted[1].body).toBe("unchanged"); + + // Having said so once, an unchanged page is not sent again. + changeThePage("a mutation that does not change the saved form"); + await letTheSnapshotHappen(); + expect(posted.length).toBe(2); + }); + + it("reports work that was already registered when watching began", async () => { + addRequestPageContentDelay("sizing an image"); + startWatchingPageForSnapshots(gatherWhenIdle); + await letEverythingSettle(); + + expect(kinds()).toEqual(["busy"]); + expect(posted[0].body).toBe("sizing an image"); + + removeRequestPageContentDelay("sizing an image"); + await letTheLoadedPageBeSent(); + }); + + it("marks a page read just before work began as still busy", async () => { + // The gather has read the page, and before we get to post it, work begins and we tell C# + // the page is busy. C# takes a snapshot to mean idle unless it says otherwise, so this one + // must say so itself, or a save could go ahead without the work. + let release: (value: string) => void = () => {}; + const heldGather = () => + new Promise((resolve) => { + release = resolve; + }); + startWatchingPageForSnapshots(heldGather); + await letTheLoadedPageBeSent(() => release("the loaded page")); + + changeThePage("typed"); + await letTheSnapshotHappen(); + addRequestPageContentDelay("sizing an image"); + release("typed, read before the work"); + await letEverythingSettle(); + expect(kinds()).toEqual(["busy", "snapshot"]); + expect(posted[1].body).toBe("typed, read before the work"); + expect(posted[1].url).toContain("stillBusyWith=sizing%20an%20image"); + + removeRequestPageContentDelay("sizing an image"); + release("after the work"); + await letEverythingSettle(); + expect(kinds()).toEqual(["busy", "snapshot", "snapshot"]); + expect(posted[2].body).toBe("after the work"); + expect(posted[2].url).not.toContain("stillBusyWith"); + }); + + it("still sends the page when work outlasts the gather's wait, marked as still busy", async () => { + // Work that never deregisters (a bug elsewhere) must not stop the page from being sent: + // the gather gives up waiting (kMaxWaitTimeMs) and reads the page anyway. + contentToReport = "first"; + startWatchingPageForSnapshots(gatherWhenIdle); + await letTheLoadedPageBeSent(); + + addRequestPageContentDelay("work that never finishes"); + await letEverythingSettle(); + contentToReport = "typed"; + changeThePage("typed"); + await letTheSnapshotHappen(); + vi.advanceTimersByTime(4000); // the register's cap + await letEverythingSettle(); + expect(kinds()).toEqual(["busy", "snapshot"]); + expect(posted[1].body).toBe("typed"); + expect(posted[1].url).toContain("stillBusyWith="); + removeRequestPageContentDelay("work that never finishes"); + await letEverythingSettle(); + }); + + it("sends busy notices and snapshots one at a time, in the order they arose", async () => { + // They share one unsynchronised endpoint. Two in flight could be processed in either + // order, and a busy notice cleared by a snapshot from before the work would let a save go + // ahead in the middle of it. + startWatchingPageForSnapshots(gather); + await letTheLoadedPageBeSent(); + + let releasePost: () => void = () => {}; + postHook = () => + new Promise((resolve) => { + releasePost = () => resolve({ data: true }); + }); + contentToReport = "typed"; + changeThePage("typed"); + await letTheSnapshotHappen(); + expect(kinds(), "sanity: the snapshot is in flight").toEqual([ + "snapshot", + ]); + + addRequestPageContentDelay("sizing an image"); + await letEverythingSettle(); + expect( + kinds(), + "the busy notice waits for the snapshot to be answered", + ).toEqual(["snapshot"]); + + releasePost(); + await letEverythingSettle(); + expect(kinds()).toEqual(["snapshot", "busy"]); + + releasePost(); + removeRequestPageContentDelay("sizing an image"); + await letEverythingSettle(); + releasePost(); + await letEverythingSettle(); + expect(kinds()).toEqual(["snapshot", "busy", "snapshot"]); + }); + + it("reports a post that fails, once per page, and does not retry it", async () => { + // Posting a string to localhost should never fail. If it does, the user needs to know + // their changes may not be saved; offering it again on a timer would only hide that. + contentToReport = "first"; + startWatchingPageForSnapshots(gather); + await letTheLoadedPageBeSent(); + + postReply = undefined; // no response at all: wrapAxios swallowed a failed request + contentToReport = "typed"; + changeThePage("typed"); + await letTheSnapshotHappen(); + await letEverythingSettle(); + expect(posted.length).toBe(1); + expect(reported.length).toBe(1); + expect(reported[0]).toContain("could not keep track of your changes"); + + vi.advanceTimersByTime(60000); + await letEverythingSettle(); + expect(posted.length, "no retry").toBe(1); + + // A mutation that leaves the saved form as it was: C# never got that form, so it is + // still sent, rather than skipped as already delivered. + postReply = { data: true }; + changeThePage("a mutation that does not change the saved form"); + await letTheSnapshotHappen(); + await letEverythingSettle(); + expect(posted.map((p) => p.body)).toEqual(["typed", "typed"]); + + postReply = undefined; + contentToReport = "typed more"; + changeThePage("typed more"); + await letTheSnapshotHappen(); + await letEverythingSettle(); + expect(posted.length, "a later change is still sent").toBe(3); + expect(reported.length, "and the user is told only once").toBe(1); + }); + + it("sends the page at once when asked, and resolves only after C# has answered", async () => { + // saveChangesAndRethinkPage restructures the page and then asks C# to save and reload it + // straight away, before the usual quiet time is up. + contentToReport = "first"; + startWatchingPageForSnapshots(gather); + await letTheLoadedPageBeSent(); + + let releasePost: () => void = () => {}; + postHook = () => + new Promise((resolve) => { + releasePost = () => resolve({ data: true }); + }); + contentToReport = "a new origami layout"; + let done = false; + const sending = sendSnapshotNow().then((ok) => { + done = ok; + }); + await letEverythingSettle(); + expect(posted.map((p) => p.body)).toEqual(["a new origami layout"]); + expect(done, "not until C# has answered").toBe(false); + + releasePost(); + await sending; + expect(done).toBe(true); + }); + + it("posts a request that reloads the page only once C# has the page as it is now", async () => { + contentToReport = "first"; + startWatchingPageForSnapshots(gather); + await letTheLoadedPageBeSent(); + + contentToReport = "just typed"; + await postAfterSendingSnapshot("some/reloadingRequest"); + expect(posted.map((p) => p.body)).toEqual(["just typed", ""]); + expect(posted[1].url).toBe("some/reloadingRequest"); + + // If C# did not get the page, reloading would lose the change, so the request is not sent. + posted.length = 0; + postReply = { data: false }; + contentToReport = "typed again"; + await postAfterSendingSnapshot("some/reloadingRequest"); + expect(posted.map((p) => p.url)).not.toContain("some/reloadingRequest"); + }); + + it("says so when asked to send the page at once and the post fails", async () => { + // The caller is about to have C# save and reload the page; if C# did not get it, the + // reload would rebuild the page without the change that prompted it. + contentToReport = "first"; + startWatchingPageForSnapshots(gather); + await letTheLoadedPageBeSent(); + + postReply = undefined; // no response: the post failed + contentToReport = "a new origami layout"; + expect(await sendSnapshotNow()).toBe(false); + expect(reported.length).toBe(1); + }); + + it("says so when asked to send the page at once and it cannot be read", async () => { + startWatchingPageForSnapshots(() => + Promise.reject(new Error("no marginBox")), + ); + expect(await sendSnapshotNow()).toBe(false); + expect(reported.length).toBe(1); + expect(posted.length).toBe(0); + }); + + it("drops a message C# refuses, without reporting it or offering it again", async () => { + // C# accepts our load before we send anything, so a refusal means it has moved on from + // this page, and nobody wants the message any more. + contentToReport = "first"; + startWatchingPageForSnapshots(gather); + await letTheLoadedPageBeSent(); + + postReply = { data: false }; + contentToReport = "typed"; + changeThePage("typed"); + await letTheSnapshotHappen(); + vi.advanceTimersByTime(60000); + await letEverythingSettle(); + + expect(posted.length).toBe(1); + expect(reported.length).toBe(0); + }); + + it("reports a gather that throws, once per page, instead of losing the edits silently", async () => { + // If the gather throws and nobody says so, C# concludes there is nothing to save and the + // user's work disappears without a word. + contentToReport = "fine"; + startWatchingPageForSnapshots(gather); + await letTheLoadedPageBeSent(); + + const exploding = () => Promise.reject(new Error("no marginBox")); + stopWatchingPageForSnapshots(); + startWatchingPageForSnapshots(exploding); + await letTheLoadedPageBeSent(); + + changeThePage("one"); + await letTheSnapshotHappen(); + changeThePage("two"); + await letTheSnapshotHappen(); + + expect(reported.length).toBe(1); + expect(reported[0]).toContain("could not keep track of your changes"); + expect(posted.length).toBe(0); + }); + it("stamps every post with the id of this page load", async () => { + // C# refuses a snapshot that does not carry the load it is currently showing, so that a + // post overtaking a reload of the same page cannot be merged over what the reload built. + contentToReport = "before"; + startWatchingPageForSnapshots(gather); + await letTheLoadedPageBeSent(); + + contentToReport = "after"; + changeThePage("after"); + await letTheSnapshotHappen(); + + expect(posted.length).toBe(1); + expect(posted[0].url).toContain( + "loadId=" + encodeURIComponent(getPageLoadId()), + ); + expect(getPageLoadId()).not.toBe(""); + }); + + it("posts when told the content changed in a way it cannot observe", async () => { + // The user's style definitions are gathered too, but they are changed through the CSSOM -- + // setProperty, deleteRule, insertRule -- which mutates no DOM node, so a MutationObserver + // cannot see it. Changing a style's size or colour without touching the text would + // otherwise produce no snapshot at all, and leaving the tab would write the old styles. + contentToReport = "first"; + startWatchingPageForSnapshots(gather); + await letTheLoadedPageBeSent(); + expect(posted.length, "sanity: nothing posted yet").toBe(0); + + // The style editor changed a rule. Nothing in the page changed. + contentToReport = "first, but with bigger type"; + notePageContentMayHaveChanged(); + vi.advanceTimersByTime(quietMsForTests); + await vi.runAllTicks(); + await Promise.resolve(); + await Promise.resolve(); + + expect(posted.map((p) => p.body)).toEqual([ + "first, but with bigger type", + ]); + }); + + it("posts nothing when told of a change that turns out not to be one", async () => { + // Callers are told to err towards saying so, which is only safe because an unchanged page + // costs nothing. + contentToReport = "first"; + startWatchingPageForSnapshots(gather); + await letTheLoadedPageBeSent(); + + notePageContentMayHaveChanged(); + vi.advanceTimersByTime(quietMsForTests); + await vi.runAllTicks(); + await Promise.resolve(); + await Promise.resolve(); + + expect(posted.length).toBe(0); + }); +}); diff --git a/src/BloomBrowserUI/bookEdit/js/pageSnapshot.ts b/src/BloomBrowserUI/bookEdit/js/pageSnapshot.ts new file mode 100644 index 000000000000..9af619ad6ab2 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/js/pageSnapshot.ts @@ -0,0 +1,327 @@ +import { postStringQuietly, postThatMightNavigate } from "../../utils/bloomApi"; +import { reportError } from "../../lib/errorHandler"; +import { onDelayRegisterChanged } from "./pageContentDelays"; + +// Keep C# supplied with the current content of the page being edited, so that a save can take it +// synchronously (see PageSnapshot.cs) instead of asking the browser for it and waiting. Gathering +// the page is cheap (~0.7 ms) and works on a clone, so we can afford to do it after every change. +// +// We post the page once as soon as it has loaded, whether or not anyone touches it, because loading +// can itself change the page (after a change of page size or appearance, image sizing and +// canvas-element layout recompute), some of it before the observer starts. C# +// (Book.UpdateDomFromEditedPage) decides whether a snapshot actually changes the book, so a page +// the user only looked at writes nothing to disk. After that we post only when a gather differs +// from the last thing we sent; tools constantly add and remove decorations that the gather strips. +// +// The same endpoint carries a BUSY notice: the page has asynchronous work whose results belong in +// the saved page (see pageContentDelays.ts). C# then waits, for a bounded time, for the next +// snapshot, which we send as soon as the work is done even if the content did not change, because +// that snapshot is what tells C# the page is idle again. +// +// Everything goes out one at a time, in order (see postInOrder), so C# needs no sequence numbers. + +const kApi = "editView/pageSnapshot"; + +// Identifies THIS load of THIS page. A module-level constant is the right scope: the page frame +// gets a fresh document, and so a fresh module, on every page load. +// +// The snapshot endpoint is deliberately unsynchronised (a keystroke should not queue behind a +// save), so a post sent just before a navigation can arrive after C# has moved on. Change Layout, +// importing a video and changing the topic reload the SAME page id, and without the load id a +// snapshot of the pre-reload page would be merged over what the reload built. +const pageLoadId = + Date.now().toString(36) + "-" + Math.random().toString(36).slice(2, 10); + +/** + * Identifies this load of this page. Sent with the "page is ready" notification and with every + * snapshot, so C# can ignore anything from a load it has superseded. + */ +export function getPageLoadId(): string { + return pageLoadId; +} + +// How long the page must be quiet before we take a snapshot. Small on purpose: it bounds how much +// typing an exit could lose. It only has to coalesce the ~nine MutationObserver batches CKEditor +// produces per keystroke; below about 25 ms the lag is dominated by the POST anyway. The cost, one +// cheap localhost POST per keystroke, is measured in Edit/SavingWithoutReloading.md. +const kQuietMs = 25; + +let observer: MutationObserver | undefined; +let timer: number | undefined; +// What we last handed to postInOrder: the content C# will hold once the queue has drained. +let lastPosted: string | undefined; +let pageIdBeingWatched: string | undefined; +// Passed in rather than imported, so this module does not depend on bloomEditing (which depends on +// it) and a test can drive it without a real page. +let gatherPageContent: (() => Promise) | undefined; +// What the delay register said the page was busy with when it became busy; undefined when idle. +let busyWith: string | undefined; +// Set when a busy spell ends: C# is waiting for a snapshot to tell it the page is idle, so the next +// one must be sent even if the page's saved form did not change. +let snapshotOwed = false; +let unsubscribeFromDelayRegister: (() => void) | undefined; +// So that a page which fails every time reports once rather than on every keystroke. +let pageWeReportedAFailureFor: string | undefined; +// The tail of the queue of posts; see postInOrder. +let postQueue: Promise = Promise.resolve(); +// Posts queued or in flight, and gathers under way; see isSnapshotStreamIdle. +let postsPending = 0; +let gathersPending = 0; +// Whether C# took the latest snapshot we queued; see sendSnapshotNow. +let lastSnapshotTaken: Promise = Promise.resolve(true); + +// Tell the user, at most once for this page; this is why snapshot posts use postStringQuietly. +// There is no retry: posting to localhost should never fail, and if it does, something is badly +// wrong and the user needs to know their work may not be saved. +function reportFailureOncePerPage( + pageId: string, + message: string, + stack: string | undefined, +): void { + if (pageWeReportedAFailureFor === pageId) return; + pageWeReportedAFailureFor = pageId; + reportError(message, stack); +} + +function currentPageId(): string | undefined { + return document.querySelector(".bloom-page")?.id || undefined; +} + +// Send one message to C#, after every message sent before it has been answered. Two POSTs in +// flight to the unsynchronised endpoint could be processed in either order: an older snapshot +// could overwrite a newer one, or a busy notice could be cleared by a snapshot taken before the +// work began. +// +// C# answers false for a load it is no longer showing (we start only once it has accepted this +// load; see startWatchingPageForSnapshots), so nobody wants the message and we drop it. +// +// Resolves to whether C# took the message. +function postInOrder( + pageId: string, + url: string, + body: string, +): Promise { + postsPending++; + const taken = postQueue.then(async () => { + let reply: unknown; + try { + reply = await postStringQuietly(url, body); + } catch { + reply = undefined; + } + // wrapAxios turns a rejected request into a resolved promise carrying nothing, so no + // response is what failure looks like. + if (!reply) { + reportFailureOncePerPage( + pageId, + "Bloom could not keep track of your changes to this page: the request to save them did not get through.", + undefined, + ); + // C# does not hold this content, so the next gather must not skip it as already sent + // (unless something newer has been queued since). + if (lastPosted === body) lastPosted = undefined; + return false; + } + return (reply as { data?: unknown }).data !== false; + }); + postQueue = taken.then(() => { + postsPending--; + }); + return taken; +} + +// Resolves to false only if the page could not be read (which has been reported). +async function takeSnapshot(): Promise { + const pageId = pageIdBeingWatched; + const gather = gatherPageContent; + if (!pageId || !gather) return true; + let content: string; + gathersPending++; + try { + // Waits for the delay register to empty (see pageContentDelays). + content = await gather(); + } catch (error) { + // Gathering can legitimately throw (the BL-13120 origami guard, a missing marginBox, the + // canvas-element count checks). If we stayed quiet, C# would save whatever it last + // received and the user's edits would be dropped without a word; the global + // unhandledrejection handler is commented out in lib/errorHandler.ts, so nothing else + // would report it. + reportFailureOncePerPage( + pageId, + "Bloom could not keep track of your changes to this page: " + + (error instanceof Error ? error.message : String(error)), + error instanceof Error ? error.stack : undefined, + ); + return false; + } finally { + gathersPending--; + } + + // The page may have been unloaded, or navigated, while we were waiting. + if (pageIdBeingWatched !== pageId) return true; + if (content === lastPosted && !snapshotOwed) return true; + lastPosted = content; + snapshotOwed = false; + let url = snapshotUrl(pageId); + // The page can be busy even so: the gather gave up waiting for the work (see kMaxWaitTimeMs), + // or the work began just after the gather read the page. C# takes a snapshot to mean idle + // unless it says otherwise, so say so in the same message; a separate busy notice afterwards + // would leave a moment in which a save took this for the finished page. + if (busyWith !== undefined) + url += "&stillBusyWith=" + encodeURIComponent(busyWith); + lastSnapshotTaken = postInOrder(pageId, url, content); + return true; +} + +/** + * Send C# the page as it is now, without waiting for the page to be quiet, and resolve once C# + * has answered. For a request that makes C# save a page the caller has only just changed. Resolves + * to false if C# does not have the page as it is now: it could not be read, or the post failed + * (both reported to the user), or C# has moved on from this page. + */ +export async function sendSnapshotNow(): Promise { + if (!(await takeSnapshot())) return false; + return lastSnapshotTaken; +} + +// The delay register has gone busy or idle. A save C# makes from the snapshot cannot wait for the +// register the way a gather here does, so we tell it; it waits a bounded time for the snapshot +// that follows (PageSnapshot.WaitUntilIdle), logging what the page was busy with if none comes. +function handleDelayRegisterChange(nowBusyWith: string | undefined): void { + const pageId = pageIdBeingWatched; + if (!pageId) return; + busyWith = nowBusyWith; + if (nowBusyWith !== undefined) { + void postInOrder( + pageId, + snapshotUrl(pageId) + "&busy=true", + nowBusyWith, + ); + } else { + snapshotOwed = true; + void takeSnapshot(); + } +} + +function snapshotUrl(pageId: string): string { + return `${kApi}?pageId=${encodeURIComponent(pageId)}&loadId=${encodeURIComponent( + pageLoadId, + )}`; +} + +function scheduleSnapshot(): void { + if (timer !== undefined) window.clearTimeout(timer); + timer = window.setTimeout(() => { + timer = undefined; + void takeSnapshot(); + }, kQuietMs); +} + +/** + * Tell the watcher that the saved form of the page may have changed in a way it cannot see. + * + * The MutationObserver watches the body. The user's style definitions are gathered too, but they + * live in a