From 4ef253208ea79160fe4358e0c95068dc0b6e4538 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Wed, 12 Aug 2026 15:29:12 -0500 Subject: [PATCH 01/10] Correct two wrong claims about Undo, and record Stage 1's deferred edits (BL-6681) Both corrections came from spending the "worth a moment's check" that Stage 1 itself asked for, and both would have caused real damage if left. 1. readerToolsModel.redo() is NOT unreachable. decodableReaderTool.tsx:170 calls it. So there are two existing Redos, not one, and Stage 5's "deleted regardless" would have silently removed a working Ctrl+Y / Ctrl+Shift+Z for reader-tool typing. 2. Section 3's ordering table describes the *button* path only. handleUndo() has exactly one caller -- topBarButtonClick, i.e. the toolbar Undo button. There is no Ctrl+Z handler in the workspace frame at all, and C#'s UndoCommand.Implementer is an empty lambda that exists only so the button's Enabled can be set. Ctrl+Z is claimed in the page frame by origami (layout mode), by the reader tools (whenever a markup type is active -- they preventDefault), then by CKEditor, then by native contenteditable undo. Three things follow, all now written down: - The deliberate reader-tools-before-CKEditor precedence is enforced for the keyboard by that preventDefault, not by handleUndo's ordering. The plan reached the right conclusion (wrap the providers and get it for free) from a wrong argument. - Stage 1 is behaviour-neutral because it changes only the button path, not because it preserves an ordering. So "one consistent Undo stack" arrives for the button now and for the keystroke only in Stages 3-4. Better to be straight about that than to over-claim. - Redo cannot be a workspace-frame keydown handler, which is what Stage 1 assumed: keyboard events inside the page iframe never reach the parent document, and typing is exactly when Redo is wanted. That is why both existing handlers are in the page frame. Adds DEFERRED-EDITS.md, the ledger of edits to existing files that a finished stage of new code is waiting on. The project's defence against rebase pain is that new code goes in new files and edits to existing files land late; the cost is that the reasoning for those edits can go stale in between. Writing each one down at design time -- what it is, why it is safe, and what proves it worked -- makes landing it mechanical. Its trigger is deliberately NOT "after Stage 0 merges": under the no-merging constraint that is months away, and waiting would leave Stage 1 unreachable and so unverifiable for the whole period. Stage 0's commits are the base of BL-6681-ckeditor and Stage 1's PR targets that branch, so the review-independence the deferral was buying is already there. Also makes the project resumable cold, on another machine with none of this session's context. That needs saying explicitly because nothing here exists on master -- not the plan, not the code, not even the resume skill -- so a fresh clone has no /resume-ckeditor at all until it checks out a project branch. PROGRESS.md gains a "How to resume" section with that bootstrap, an authoritative branch table naming the working tip, and a master-sync log; the resume skill is rewritten for the new topology, since its old advice ("you may well be on master with nothing in flight", "don't keep a long-lived branch") is now exactly backwards. Stage 0's four remaining verification items are retargeted off the branch under review, where pushing would restart the review for purely additive work. Also fixes citation drift found while checking: origami.ts:139-146 -> :137, and the origamiCanUndo/origamiUndo range -> :277-294. Co-Authored-By: Claude Opus 5 --- .claude/skills/resume-ckeditor/SKILL.md | 117 ++++++++++---- docs/retire-ckeditor/DEFERRED-EDITS.md | 179 +++++++++++++++++++++ docs/retire-ckeditor/PLAN.md | 45 +++++- docs/retire-ckeditor/PROGRESS.md | 197 +++++++++++++++++++----- 4 files changed, 458 insertions(+), 80 deletions(-) create mode 100644 docs/retire-ckeditor/DEFERRED-EDITS.md diff --git a/.claude/skills/resume-ckeditor/SKILL.md b/.claude/skills/resume-ckeditor/SKILL.md index 18be2d51a584..d6e607d899b0 100644 --- a/.claude/skills/resume-ckeditor/SKILL.md +++ b/.claude/skills/resume-ckeditor/SKILL.md @@ -5,47 +5,100 @@ description: Resume the long-running "retire CKEditor / unify Undo" project (BL- # Resume the CKEditor-retirement project (BL-6681) -This is a deliberately long-running project spanning many sessions and many rebases against a -moving `master`. Its state lives in `docs/retire-ckeditor/`, not in any session's memory. +This is a deliberately long-running project spanning many sessions and months of a moving `master`. +Its state lives in `docs/retire-ckeditor/`, not in any session's memory. + +## If you are on a fresh clone or a different computer + +**None of this project exists on `master`** — nothing may merge there until a `Version6.5` branch is +cut (see the ground rules), so the plan, the progress log, the code, and *this skill file* all live +only on the project's own branches. A checkout of `master` has no `/resume-ckeditor` at all. + +So the first move is always: + +```sh +git fetch origin +git branch -r | grep BL-6681 # see what exists +git checkout BL-6681-stage1-undostack # or whatever PROGRESS.md's branch table calls the tip +``` + +Then read `docs/retire-ckeditor/PROGRESS.md`, whose **branch table** is authoritative for which +branch is which and which one is the current working tip. That table changes as stages advance; +trust it over this file, and over any branch name you remember. ## Do this, in order 1. **Read the state.** - - `docs/retire-ckeditor/PROGRESS.md` — the live log, current phase, and next actions. Start here. - - `docs/retire-ckeditor/PLAN.md` — the staged plan (Stages 0–6). Authoritative for *what* to - do. §11 records what BL-6681 itself asks for, and what on it is already obsolete. - - `docs/retire-ckeditor/REVIEW-NOTES.md` — findings already verified and decisions already - made. **Do not re-litigate anything settled there.** If you think a settled point is wrong, - say so explicitly to the user rather than quietly changing course. - - `docs/retire-ckeditor/BEHAVIOR-INVENTORY.md` if it exists — the behaviours that must survive. - -2. **Orient in git.** `git status`, `git log --oneline -15`, and note the current branch. Because - the plan deliberately avoids long-lived branches, you may well be on `master` with nothing in - flight; that is the normal resting state between stages, not a sign something was lost. - -3. **Check for drift.** Other work lands on `master` continuously. Before continuing a stage, - confirm the files it touches still look the way the plan assumes — the plan cites specific - `file:line` locations, and those move. If a citation has gone stale, fix the plan text as part - of the work; a plan nobody trusts is worse than no plan. - -4. **Continue from the next unchecked item** in PROGRESS.md's "Next actions". Confirm with the - user which stage to work on if more than one is plausible. + - `docs/retire-ckeditor/PROGRESS.md` — the live log, the branch table, the master-sync log, and + the next actions. **Start here.** + - `docs/retire-ckeditor/PLAN.md` — the staged plan (Stages 0–6). Authoritative for *what* to do. + **§5 is the branch strategy and it is not optional reading** — the no-merging constraint makes + several natural instincts (rebase onto master, land the stage on master) actively wrong. §11 + records what BL-6681 itself asks for, and what on it is already obsolete. + - `docs/retire-ckeditor/REVIEW-NOTES.md` — findings already verified and decisions already made. + **Do not re-litigate anything settled there.** If you think a settled point is wrong, say so + explicitly to the user rather than quietly changing course. + - `docs/retire-ckeditor/DEFERRED-EDITS.md` — edits to *existing* files that finished new code is + waiting on, each with why it is safe and what proves it worked. Check whether any are now due. + - `docs/retire-ckeditor/BEHAVIOR-INVENTORY.md` — the behaviours that must survive. + +2. **Orient in git.** `git status`, `git log --oneline -15`, `git branch -vv | grep BL-6681`. You + should be on one of the project's branches, not `master`. Several live branches at once is the + normal state here, not a sign something went wrong — read PROGRESS.md's branch table. + +3. **Check drift, and sync if due.** `master` moves ~17 commits a day, of which ~2 touch this + project's files. PLAN.md §5.3 gives the procedure and §5.1 the measured numbers. In short: + + ```sh + git log ..origin/master --oneline -- \ + src/BloomBrowserUI/bookEdit/js/bloomEditing.ts \ + src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts \ + src/BloomBrowserUI/bookEdit/bloomField/BloomField.ts \ + src/BloomBrowserUI/lib/ckeditor + ``` + + where `` is the newest row of PROGRESS.md's master-sync table. Sync weekly, and + always before starting a new stage. **Merge into the integration branch; never rebase it.** + + Separately, the plan cites specific `file:line` locations and those drift. If a citation has gone + stale, fix the plan text as part of the work — a plan nobody trusts is worse than no plan. + +4. **Continue from the next unchecked item** in PROGRESS.md's "Next actions". Confirm with the user + which stage to work on if more than one is plausible. 5. **Before ending the session** (or when you sense you are running low on context), update - `PROGRESS.md`: what you did, what you learned, the branch/PR, and a revised "Next actions". - Do this even if the work is half-finished — especially then. + `PROGRESS.md`: what you did, what you learned, the branch state, and a revised "Next actions". + Do this even if the work is half-finished — especially then. Then **push**, so the work survives a + move to another machine. ## Ground rules for this project -- **New code goes in new files** (`bookEdit/undo/`, `bookEdit/textEditor/`). Edits to existing - files should be one-line dispatches wherever possible, and as late in the plan as possible. - This is the project's whole defence against rebase pain. -- **Don't keep a long-lived branch.** Each stage is designed to be its own small, green, - flag-inert PR onto `master`. -- **Deletion commits (Stage 5) are regenerated, never rebased.** If one conflicts, throw it away - and redo it mechanically. +- **Nothing merges to `master` until a `Version6.5` branch is cut** (manager's decision, + 2026-08-06), which happens once 6.5 is mostly finished. Everything below follows from that; + PLAN.md §5 is the full treatment. +- **One long-lived integration branch, `BL-6681-ckeditor`,** is the project's trunk and the only + branch that merges `master` in. Each stage is a short-lived branch off it, PR'd *into* it and + **squash-merged**, so integration carries roughly one commit per stage. Delete a stage branch once + merged and cut the next fresh, or sibling branches re-apply changes already in. + - Note this **reverses** the project's original "never keep a long-lived branch, land small PRs + promptly" rule. If you find that rule quoted anywhere, it is historical. +- **Merge, never rebase, the integration branch or any pushed/reviewed branch.** Rebasing rewrites + reviewed commits, discards their review threads, needs a force-push, and re-resolves the same + conflict on every replay. An unreviewed, unpushed stage branch may still be rebased onto + integration freely. +- **After every master sync, run the nightly by hand:** + `gh workflow run nightly.yml --ref BL-6681-ckeditor`. It is schedule-only and master-only, and it + is the only thing that runs the full C# suite and the visual-regression suite — a branch that never + merges otherwise goes months with neither. +- **New code goes in new files** (`bookEdit/undo/`, `bookEdit/textEditor/`). Edits to existing files + should be one-line dispatches wherever possible, and as late in the plan as possible. This is the + main reason a months-long branch is survivable; record any that must wait in DEFERRED-EDITS.md. +- **Deletion commits (Stage 5) are regenerated, never reconciled.** If one conflicts with an incoming + master change, throw it away and redo it mechanically against the new state. +- **Every stage boundary must be a state that could ship as-is** — green, flag-inert, no half-applied + dispatch. The merge date is set by someone else and may move. - Build and test through the wrappers, never bare `dotnet`/`vite` — `build/agent-dotnet.sh` and `build/agent-vite.sh` — because the developer usually has a Bloom running via `./go.sh`. See `AGENTS.md`. Never run the full `pnpm build`. -- To see a change in the running Bloom, just edit the source (the Vite dev server pushes it in) - and observe via the `run-bloom` skill. No build. +- To see a change in the running Bloom, just edit the source (the Vite dev server pushes it in) and + observe via the `run-bloom` skill. No build. diff --git a/docs/retire-ckeditor/DEFERRED-EDITS.md b/docs/retire-ckeditor/DEFERRED-EDITS.md new file mode 100644 index 000000000000..5425de61da15 --- /dev/null +++ b/docs/retire-ckeditor/DEFERRED-EDITS.md @@ -0,0 +1,179 @@ +# Deferred edits to existing files + +The project's defence against rebase pain is that new code goes in new files and edits to existing +files land as late as possible (PLAN.md §5). This file is the ledger of edits that a completed stage +of *new* code is waiting on — written down at the moment the new code was designed, while the +reasoning is fresh, so that landing them later is mechanical rather than a re-derivation. + +**Every entry states what the edit is, why it is safe, and what proves it worked.** Delete an entry +when its edit lands. + +Line citations are as of the commit that added the entry. They drift; the surrounding code is quoted +so the right place is still findable. + +--- + +## Stage 1 — activate the one undo stack + +**New code (already landed, inert):** `src/BloomBrowserUI/bookEdit/undo/` — `undoTypes.ts`, +`UndoStack.ts`, `legacyUndoProviders.ts`, `runUndoable.ts` and their specs. Nothing imports them +yet, so the bundle is unchanged in behaviour and very nearly unchanged in size. + +**Why they were deferred:** these edits are all in `workspaceRoot.ts`, which the Stage 0 PR (#8153) +does not touch but which sits next to code that PR does touch, so waiting until Stage 0 merged kept +the two reviews independent. + +**They are no longer blocked (2026-08-06).** Under the no-merging constraint (PLAN.md §5) Stage 0 +will not reach `master` for months — deferring until then would leave Stage 1 unverifiable for the +whole period, which is much worse than the review-independence it was buying. And the reason has +gone anyway: Stage 0's commits are the base of the integration branch `BL-6681-ckeditor`, and Stage +1's PR targets that branch, so its diff shows only Stage 1's own changes. **Apply these on the Stage +1 branch.** Every file they touch had zero commits on `master` in the 30 days measured in §5.1, so +the integration risk is as low as it gets. + +### 1a. Register the legacy providers, once + +In `workspaceRoot.ts`, alongside the other module-level imports: + +```ts +import { registerLegacyUndoProviders } from "./undo/legacyUndoProviders"; +import { theOneUndoStack } from "./undo/UndoStack"; + +registerLegacyUndoProviders(); +``` + +Module-level is right: `workspaceRoot` is loaded once per edit-tab session, and the providers only +reach across frames when consulted, so nothing needs the frames to exist yet. + +*Safe because:* registration does no work. **Do not call it twice** — each mechanism would be +consulted twice, harmless but confusing. + +### 1b. `handleUndo()` becomes a delegation + +**Scope, so the verification below is not over-claimed:** `handleUndo` has exactly one caller — +`topBarButtonClick` (`bloomEditing.ts:1633-1648`), i.e. the toolbar Undo button. There is no Ctrl+Z +handler in the workspace frame, and C#'s `UndoCommand.Implementer` is an empty lambda +(`WebView2Browser.cs:890`) existing only to make the button's `Enabled` settable. Ctrl+Z is claimed in +the *page* frame by origami, by the reader tools, or by CKEditor. So these edits change the **button** +path only; the keyboard path is untouched, which is both why they are safe and why "one consistent +Undo" is not yet true for the keystroke. See PLAN.md §3's correction. + + +Replace the body of `handleUndo()` (`workspaceRoot.ts:97-126`) with: + +```ts +export function handleUndo(): void { + theOneUndoStack.undo(); +} +``` + +The four-way if-chain being deleted is reproduced exactly by the providers, in the same order, in +`legacyUndoProviders.registerLegacyUndoProviders()`. Two comments in the deleted body should move +rather than die, because they record *why* the order is what it is — they are already carried in +`legacyUndoProviders.ts`, so check them across before deleting. + +**One comment must not move: it is wrong.** `workspaceRoot.ts:125` says "*See also Browser.Undo; if +all else fails we ask the C# browser object to Undo*". There is no such fallback in the WebView2 +code — the Undo button's enabled state comes purely from `canUndo()` returning `"yes"`. Delete it. + +### 1c. `canUndo()` becomes a delegation + +Replace the body of `canUndo()` (`workspaceRoot.ts:248-266`) with: + +```ts +//Called by c# using workspaceBundle.canUndo() +export function canUndo(): string { + return theOneUndoStack.canUndo() ? "yes" : "fail"; +} +``` + +Keep the `"yes"`/`"fail"` strings: that is the contract with `WebView2Browser.CanUndoAsync`, which +polls it on a timer. Changing it is a separate, C#-touching change and not worth bundling in. + +*Watch for:* the old `canUndo` guarded the toolbox call as `toolboxWindow.canUndo && +toolboxWindow.canUndo()` while `handleUndo` did not. `toolboxUndoProvider` keeps that asymmetry +deliberately (a throw in a timer-polled function fires repeatedly), and says so. + +### 1d. Tell the stack when the page changes + +`UndoStack.setCurrentPageId()` exists but nothing calls it, so page-scoped entries are never +discarded. The hook point is the `load` handler already inside `switchContentPage` +(`workspaceRoot.ts:163-172`), which is where the new page's DOM first exists: + +```ts +const handler = () => { + handlerCalled = true; + iframe.removeEventListener("load", handler); + theOneUndoStack.setCurrentPageId(getCurrentPageIdFromPageFrame()); + doWhenToolboxLoaded(...); +}; +``` + +The page id lives on the current page element as `data-page-id` — the same source +`ImageUndoManager` uses for exactly this purpose (`ImageUndoManager.ts:154-161`, +`clearImageOperationUndoOnPageChange`). Reuse that, don't invent a second notion of page identity. + +Note the 1500 ms fallback below it: the `load` event sometimes never fires, and `handler` is called +on a timer instead. `setCurrentPageId` is idempotent for an unchanged id, so being called twice or +late is harmless — but it means an entry pushed in that window could be attributed to the previous +page. Nothing pushes automatically in Stage 1, so this cannot bite yet; it must be re-examined when +Stage 3 starts recording typing. + +**Also needed, and not covered by the above:** `clearPageScopedEntries()` on a page-frame reload +that keeps the *same* page — ctrl+wheel zoom (`bloomEditing.ts:1259-1276`, whose own comment says +"Zooming re-loads the page") and leaving Change Layout mode (`origami.ts:193`). `switchContentPage` is not involved in either, so this needs its own hook. +`pageUnloading()` (already called at `workspaceRoot.ts:138`) is the candidate; confirm it runs on +same-page reloads before relying on it. + +### 1e. Expose Redo (Ctrl+Y only) — and it cannot live in the workspace frame + +There is **no Redo plumbing in C# at all** — no `RedoCommand`, no `SetEditingCommands` parameter, +nothing in the `updateEditButtons` payload, no icon, no XLF entry. So a Redo *button* is where the +real cost is, and it is deliberately out of scope. Ctrl+Y is JS-only and needs none of it. + +Add to `workspaceRoot.ts`: + +```ts +export function handleRedo(): void { + theOneUndoStack.redo(); +} +``` + +**But do not bind Ctrl+Y in the workspace frame.** Keyboard events inside the page iframe are +delivered to that iframe's document and never reach the parent, so a workspace-frame handler would +fire only when focus is outside the page — which is the opposite of when Redo is wanted. This is why +*both* existing Ctrl+Y handlers are in the page frame: origami's on `html` (`origami.ts:137`) and the +reader tools' on each editable (`decodableReaderTool.tsx:158-178`). So the new binding goes in the +page frame too, and calls `getWorkspaceBundleExports().handleRedo()`. + +**Both existing handlers `preventDefault()` and win where they apply**, so the new one must be the +last resort, not the first: + +- In Change Layout mode, origami's handler claims Ctrl+Y. Leave it — it is the only Redo for layout + changes until Stage 4 converts it, and that conversion must retire the handler in the *same* + commit or its Redo breaks in between. +- In any editable while a reader tool is active (`currentMarkupType !== None`), the reader tools' + handler claims Ctrl+Z *and* Ctrl+Y and returns false. Also leave it. + +*Verify:* press Ctrl+Y in Change Layout mode, in a reader-tool text box, and in an ordinary text box, +and confirm exactly one redo happens in each — not two, and not none. + +### 1f. Expose the cross-frame push + +`IWorkspaceExports` (`workspaceRoot.ts:14-55`) and the global exposure object at the bottom of the +file both need whatever Stage 2 pushes with. **Do not export `push(entry)` across frames** — that +would hand page-frame code the ability to put a page-frame closure on the stack, which is exactly +the failure `undoTypes.ts` documents at length. Export a function taking *data* and let the +workspace frame build the entry. Design it with Stage 2's first real caller, not before. + +### Proof it worked + +- `pnpm test` green; `bookEdit/undo` specs green (31 tests). +- **The point of Stage 1 is that nothing changes**, so the verification is behavioural, in a + running Bloom, comparing against the same gestures before the edits: + - In Change Layout mode: make a layout change, Ctrl+Z undoes it; Ctrl+Y redoes it. + - With the Decodable Reader tool open: type, then Undo — the reader-tools undo runs, not + CKEditor's (this is the deliberate precedence that would be easiest to lose). + - On an image: change its copyright, then Undo. + - In a text box with no tool active: type, then Undo — CKEditor's undo runs. + - The Undo button's enabled state tracks all four, since C# polls `canUndo()` on a timer. diff --git a/docs/retire-ckeditor/PLAN.md b/docs/retire-ckeditor/PLAN.md index 36d5fca5af02..52008ce46e15 100644 --- a/docs/retire-ckeditor/PLAN.md +++ b/docs/retire-ckeditor/PLAN.md @@ -115,13 +115,34 @@ asynchronously after `CKEDITOR.inline()` returns: | Mechanism | What it really is | Notes | | --- | --- | --- | -| `origamiCanUndo`/`origamiUndo` (`origami.ts:262-294`) | A stack of **jQuery `clone(true)` copies of `.marginBox`** — DOM plus attached handlers and data — restored with `replaceWith` | Only while Change Layout mode is active. Has its **own** `keydown.origami` Ctrl+Z/Ctrl+Y handler on `html` (`origami.ts:139-146`), and its own Redo. Safe today partly *because* layout mode strips `contentEditable` (`origami.ts:132`), so there are no live CKEditor instances to orphan. | +| `origamiCanUndo`/`origamiUndo` (`origami.ts:277-294`) | A stack of **jQuery `clone(true)` copies of `.marginBox`** — DOM plus attached handlers and data — restored with `replaceWith` | Only while Change Layout mode is active. Has its **own** `keydown.origami` Ctrl+Z/Ctrl+Y handler on `html` (`origami.ts:137`), and its own Redo. Safe today partly *because* layout mode strips `contentEditable` (`origami.ts:132`), so there are no live CKEditor instances to orphan. | | `toolboxWindow.canUndo/undo` → `readerToolsModel` | A per-editable **text-typing** undo: `{html, text, caretOffset}` snapshots, seeded on focus (`noteFocus`, :557-568, from `decodableReaderTool.tsx:155`) and pushed on every markup-changing keystroke inside `doMarkup` (:753-764) | Gated on `shouldHandleUndo()` — `currentMarkupType !== None` (:570). It is consulted *before* CKEditor **deliberately**: when a reader tool is active it must shadow CKEditor's undo, which would restore stale decodable/leveled markup. Not "reader-setup changes". | | `imageOperationCanUndo`/`imageOperationUndo` (`ImageUndoManager.ts`) | Restores an image's `src` / copyright / crop | Clean two-phase prepare/commit; already page-id-scoped; gated on the active element being an image container. | | `ckeditorCanUndo`/`ckeditorUndo` | `CKEDITOR.currentInstance.undoManager`, **per editable div** | An "implementation secret". Ordering across boxes is already wrong. | | Browser-native undo | Invisible | Called directly in `BloomField.PreventRemovalOfSomeElements` (`BloomField.ts:810-825`); also fed implicitly by every `document.execCommand("insertHTML"/"formatBlock"/"justify*"/"insertText")` in `bloomEditing.ts` and `GamePromptDialog.tsx`, and by plain typing in any contenteditable. | -Two corrections to the folklore: +**Correction, verified 2026-08-06 — the table above is the *button* path, not the keyboard path.** +`handleUndo()` has exactly one caller: `topBarButtonClick` (`bloomEditing.ts:1633-1648`), reached +when the user clicks the toolbar Undo button. There is **no Ctrl+Z handler anywhere in the workspace +frame**, and C#'s `UndoCommand.Implementer` is an empty lambda (`WebView2Browser.cs:890`) that exists +only so the button's `Enabled` can be set. So Ctrl+Z is handled entirely in the **page** frame, by +whichever of these claims it first: + +| Ctrl+Z handler | Where | When it wins | +| --- | --- | --- | +| `keydown.origami` on `html` | page frame (`origami.ts:137`) | Change Layout mode only | +| per-editable `keydown` in the reader tools | page frame (`decodableReaderTool.tsx:158-178`) | any editable, whenever `currentMarkupType !== None`; `preventDefault`s and returns false | +| CKEditor's own keystroke handling | inside each editable | otherwise | +| browser-native contenteditable undo | — | when nothing above claims it | + +Two consequences the plan depended on and got half right. First, the deliberate +reader-tools-before-CKEditor precedence is enforced for the keyboard by that `preventDefault`, not by +`handleUndo`'s ordering — so with a reader tool active, Ctrl+Z in a text box never reaches the shared +stack at all. Second, that is *why* Stage 1 is behaviour-neutral: it changes only the button path. +The keyboard path is not unified until those page-frame handlers are converted (Stages 3–4), and +until then a single consistent Undo exists for the button but not for the keystroke. + +Two further corrections to the folklore: - `workspaceRoot.ts:125`'s "*See also Browser.Undo; if all else fails we ask the C# browser object to Undo*" is **stale** — no such fallback exists in the WebView2 code. The Undo button's enabled state comes purely from `workspaceBundle.canUndo()` returning `"yes"` @@ -942,13 +963,21 @@ Exit criteria: inventory reviewed; `pnpm test` green; prep commit demonstrably b clearForPage / clearOnPageFrameReload. Index-based with truncate-on-push (§4.1), count-bounded, `canUndo` and `canRedo` both O(1). - `workspaceRoot.canUndo`/`handleUndo` become thin delegations (two small edits, one file). Redo - needs no C# counterpart — it is reached only by Ctrl+Y (§10 q1), so it stays entirely in JS. + needs no C# counterpart — it is reached only by Ctrl+Y (§10 q1), so it stays entirely in JS. **But + it cannot be a workspace-frame keydown handler:** keyboard events inside the page iframe never + reach the parent document, and typing is exactly when the user wants Redo. It has to be registered + in the page frame (as both existing Ctrl+Y handlers are) and call across. See DEFERRED-EDITS.md 1e. - **Wrap all four existing mechanisms as legacy providers in their current priority order.** - No conversions, no behaviour change. This preserves the deliberate reader-tools-before-CKEditor - precedence (§3) for free. Redo has no legacy providers to wrap — origami's is the only Redo that - exists, and it keeps working via its own handler until Stage 4 converts it. (Note - `readerToolsModel.redo()` at `:609` appears to be **unreachable** — nothing exports or calls it; - worth a moment's check, but it is deleted in Stage 5 regardless.) + No conversions, no behaviour change. **Note precisely what that order governs**, which §3's + correction spells out: `handleUndo` is reached only from the top-bar Undo button, so wrapping it + reproduces the *button* path exactly and leaves the keyboard path — which is handled per-context in + the page frame and never enters `handleUndo` — untouched. Behaviour-neutrality holds, but not + because the ordering is preserved; because the keyboard path was never in scope. +- Redo has no legacy providers to wrap, and there are **two** existing Redos, not one: origami's and + the reader tools'. Both keep working via their own page-frame handlers until converted. + (**Correction, verified 2026-08-06:** the earlier claim that `readerToolsModel.redo()` is + unreachable was wrong — `decodableReaderTool.tsx:170` calls it. Stage 5 must **not** delete it + blind; doing so would silently remove a working Ctrl+Y/Ctrl+Shift+Z for reader-tool typing.) - `runUndoable(label, fn)` with the nesting semantics of §4.13. Rationale for doing *no* conversions here: the four existing mechanisms are contextually diff --git a/docs/retire-ckeditor/PROGRESS.md b/docs/retire-ckeditor/PROGRESS.md index 4efd95235107..e4f9a7ba505b 100644 --- a/docs/retire-ckeditor/PROGRESS.md +++ b/docs/retire-ckeditor/PROGRESS.md @@ -2,9 +2,26 @@ **Ticket:** BL-6681. **Plan:** [PLAN.md](PLAN.md). **Settled decisions:** [REVIEW-NOTES.md](REVIEW-NOTES.md). -To resume after an interruption, issue **`/resume-ckeditor`** (see -`.claude/skills/resume-ckeditor/SKILL.md`). Equivalent plain-English prompt: -*"Read docs/retire-ckeditor/PROGRESS.md and continue from the next unchecked item."* +## How to resume + +Issue **`/resume-ckeditor`** (`.claude/skills/resume-ckeditor/SKILL.md`). Equivalent plain-English +prompt: *"Read docs/retire-ckeditor/PROGRESS.md and continue from the next unchecked item."* + +**On a fresh clone or a different computer, check out a project branch first.** Nothing here exists +on `master` — not the plan, not the code, not even the skill file — because nothing may merge until +the 6.5 branch is cut. So `/resume-ckeditor` does not exist until you do this: + +```sh +git fetch origin +git branch -r | grep BL-6681 # what exists +git checkout BL-6681-stage1-undostack # the current tip; see the branch table below +cd src/BloomBrowserUI && vp install # if node_modules is absent or stale +``` + +The **branch table** below is authoritative for which branch is the working tip — it changes as +stages advance, so trust it over any branch name you remember. If `node` is not 24.13.0 or `pnpm` +not 11.5.2, read the boxed toolchain warning further down **before** touching the toolchain: the +answer is `vp`, never Volta, and the intermediate states are all misleading. ## Current state @@ -16,14 +33,18 @@ To resume after an interruption, issue **`/resume-ckeditor`** (see > [PLAN.md](PLAN.md) is rewritten around a long-lived integration branch. Read §5 before doing any > branch work; the short version is the table below. +Stage 0's PR is reviewed-ready and awaiting a human; its card is in *Ready For Code Review*, the QA +test-ideas comment is posted, and Devin is clean against HEAD `6bd49463`. Stage 1's new code is +written and inert. + **Branch topology** — one integration branch tracks `master`; each stage is a short-lived branch off it, PR'd into it and **squash-merged**, so integration carries one commit per stage: | Branch | What | State | | --- | --- | --- | -| **`BL-6681-ckeditor`** | The project's trunk. The only branch that merges `master` in. Eventually one PR into `master`. | Created 2026-08-06 at Stage 0's HEAD; synced to master `9b6ba1cd9` | -| `BL-6681-stage0-inventory` | PR [#8153](https://github.com/BloomBooks/BloomDesktop/pull/8153) — docs, characterization tests, the `toolbox.ts` seam | Ready for review, awaiting a human. Left targeting `master` on purpose (§5.6) | -| `BL-6681-stage1-undostack` | `bookEdit/undo/` — the one undo stack, new files only | Local, green, inert; to be PR'd into `BL-6681-ckeditor` | +| **`BL-6681-ckeditor`** | The project's trunk. The only branch that merges `master` in. Eventually one PR into `master`. | Pushed. Created 2026-08-06 at Stage 0's HEAD; synced to master `9b6ba1cd9` | +| **`BL-6681-stage1-undostack`** | ← **the working tip.** `bookEdit/undo/` — the one undo stack, new files only, nothing imports it | Pushed, green, inert. No PR yet; when there is one it targets `BL-6681-ckeditor`, not master | +| `BL-6681-stage0-inventory` | PR [#8153](https://github.com/BloomBooks/BloomDesktop/pull/8153) — docs, characterization tests, the `toolbox.ts` seam | Pushed; ready for review, awaiting a human. Left targeting `master` on purpose (§5.6). **Don't push more to it** — it would restart the review | **Master-sync log** (§5.3 — record every sync here so the next drift check has a start point): @@ -417,6 +438,8 @@ dispatches become "*also* start the new editor" rather than "skip CKEditor". the *same commit* that converts its entry, or its Redo breaks in between. - Incidental: `readerToolsModel.redo()` (`:609`) appears **unreachable** — nothing exports or calls it. Noted in Stage 1; deleted in Stage 5 regardless. + **← WRONG. Corrected 2026-08-06; see that day's entry. It is called from + `decodableReaderTool.tsx:170`.** §10 is retitled from "Open questions" to "Decisions", with reasoning kept inline so a later session doesn't reopen settled ground. **One genuinely open item remains**, and it blocks nothing: whether @@ -629,6 +652,106 @@ comment asks for, and it covers the case automation could not reach (a reader to running). Combined with the automated harness result, the prep commit is verified on both halves. **G2 (async path / BL-10133) and G3 (longpress) remain unverified.** +### 2026-08-05 (later still) — Stage 0 preflighted; PR #8153 open as draft + +**PR:** https://github.com/BloomBooks/BloomDesktop/pull/8153 (draft). Branch pushed, card linked, +QA test-ideas comment posted, Devin consultation logged. + +Reviewer outcomes at HEAD `178269d78`: + +| Reviewer | Outcome | +| --- | --- | +| Local review (light, 1 subagent) | Clean — no correctness problems. It mutation-tested the new spec (neutering `selectAtOffset` fails 9 of 10 tests) and raised one accuracy note, which was fixed. | +| Devin | **Re-review clean** — 0 bugs, 0 investigate flags, 8 informational. Three informational items acted on; the rest declined with reasons, recorded in the PR consultation log. | +| CI (`pr-automation`) | pass | +| CodeRabbit | see the run's final report | + +**Three things Devin's informational tier caught that were worth fixing** — a reminder that the +lowest-signal tier is not always noise: + +1. The committed harness hard-coded `repoRoot = "C:/github/BloomDesktop"`, so it only ran in the + checkout it was written in. Now derived from `import.meta.url`. +2. **The extraction has two deliberate behaviour differences, not one.** + `restoreSelectionAfterMarkup` re-reads the editor and no-ops if it has gone, where the old code + sat inside `if (ckeditorOfThisBox)` and would have thrown. Unreachable in practice + (`bloomCkEditor` is assigned once and never cleared) but real, and now documented on the + function. Notable because this branch already corrects two *other* comments in the same pipeline + that misled by overstating — a third would have been poor form. +3. The inventory pointed at `PASTE-DROP-BASELINE.md` as if it existed. + +**G3 (longpress) is now verified too** — John spot-checked it manually and reports it basically +works. So of the G rows, G1 and G3 are verified, and **G2 (async markup path / BL-10133) is the one +still open**, along with the new G6/G7 highlight rows. + + +### 2026-08-06 — Stage 1's new code written; two plan claims about Undo found wrong + +Stage 0's PR (#8153) is waiting on a human reviewer, so Stage 1's *new* code was written in parallel +on a **temporary local branch `BL-6681-stage1-undostack`** (branched off Stage 0's HEAD, so the docs +are in scope). New files only — nothing imports them, so the change is behaviourally inert and the +two reviews stay independent. The edits to existing files that would activate it are written up in +the new **[DEFERRED-EDITS.md](DEFERRED-EDITS.md)**, to land after Stage 0 merges. *(Superseded a few +hours later by the no-merging constraint — see the next entry. The edits are no longer blocked, +because Stage 0's commits became the integration branch's base.)* + +**New:** `src/BloomBrowserUI/bookEdit/undo/` — `undoTypes.ts` (`IUndoEntry`, +`ILegacyUndoProvider`, `kMaxUndoEntries`), `UndoStack.ts` (index-based with truncate-on-push, O(1) +`canUndo`/`canRedo`, count bound, page scoping, lazy redo capture, legacy-provider arbitration, +`runUndoable` scope plumbing), `legacyUndoProviders.ts` (the four wrappers), `runUndoable.ts`, and +specs. **31 tests**, typecheck clean, no new lint warnings. + +**Mutation-tested rather than trusted**, since they passed first run — three separate mutations, each +caught by exactly the test that should catch it: dropping truncate-on-push failed 1 test; disabling +the nested-push suppression failed 6; making `keepOnly` recompute the index unconditionally failed 1. + +**Two claims in the plan turned out to be wrong, both found by spending the "worth a moment's check" +the plan itself asked for.** Both are corrected in PLAN.md at the point of error, not only here. + +1. **`readerToolsModel.redo()` is *not* unreachable** — `decodableReaderTool.tsx:170` calls it. So + there are **two** existing Redos, not one, and Stage 5's "deleted regardless" would have silently + removed a working Ctrl+Y/Ctrl+Shift+Z for reader-tool typing. +2. **§3's ordering table describes the *button* path only.** `handleUndo()` has exactly one caller, + `topBarButtonClick` (`bloomEditing.ts:1633-1648`) — the toolbar Undo button. There is **no Ctrl+Z + handler in the workspace frame at all**, and C#'s `UndoCommand.Implementer` is an empty lambda + (`WebView2Browser.cs:890`) that exists only so the button's `Enabled` can be set. Ctrl+Z is + claimed in the *page* frame, by whichever gets it first: origami's `keydown.origami` + (`origami.ts:137`, layout mode), the reader tools' per-editable handler + (`decodableReaderTool.tsx:158-178`, whenever `currentMarkupType !== None` — it `preventDefault`s), + then CKEditor, then native contenteditable undo. + + Three consequences, all recorded: + - The deliberate reader-tools-before-CKEditor precedence is enforced for the keyboard by that + `preventDefault`, **not** by `handleUndo`'s ordering. §3 said the ordering was what preserved + it, so wrapping the providers "for free" was the right conclusion reached by a wrong argument. + - Stage 1 is behaviour-neutral for a different reason than the plan gave: it changes only the + button path. So "one consistent Undo stack" arrives for the button now and for the keystroke + only when the page-frame handlers are converted (Stages 3–4). Worth being straight about. + - **Redo cannot be a workspace-frame keydown handler**, which is what Stage 1 assumed. Keyboard + events inside the page iframe never reach the parent document, and typing is precisely when + Redo is wanted — which is why both existing handlers are in the page frame. Rewritten as + DEFERRED-EDITS.md 1e, including that the new binding must be the last resort behind the two + that `preventDefault`. + +**Design points settled while writing it, so they are not reopened:** + +- **Legacy providers are consulted before our own entries.** That reproduces today's button path + exactly. It does *not* give true chronological order across the boundary (an entry recorded here + followed by a legacy operation undoes in the right order; the reverse does not), which was already + true between the old mechanisms. Documented on `UndoStack.undo` rather than fixed: inventing + cross-mechanism sequencing for a state we are deleting is not worth it. +- **`push` is deliberately not exposed across frames.** Handing page-frame code a way to put a + page-frame *closure* on the stack is the exact failure §4.1 exists to prevent, so the cross-frame + surface must take data and let the workspace frame build the entry. Designed with Stage 2's first + real caller, not before (DEFERRED-EDITS.md 1f). +- **`setCurrentPageId` is not enough on its own.** Ctrl+wheel zoom and leaving Change Layout mode + reload the page frame *without* changing page, so `clearPageScopedEntries()` needs its own hook; + `pageUnloading()` is the candidate, unverified. Harmless in Stage 1 (nothing pushes yet), and it + must be settled before Stage 3 records typing. +- Page identity comes from `data-page-id`, reusing what `ImageUndoManager` already does + (`ImageUndoManager.ts:154-161`) rather than inventing a second notion of it. + +Incidental drift fixed while checking citations: `origami.ts:139-146` → `:137`, and the +`origamiCanUndo`/`origamiUndo` range → `:277-294`. ### 2026-08-06 (later) — the no-merging constraint, and §5 rewritten around it John's manager ruled that **nothing from this project may merge to `master` until a `Version6.5` @@ -700,8 +823,14 @@ Stage 1 branch now, and DEFERRED-EDITS.md's trigger is updated to say so. ## Next actions -All of Stage 0's code work is done. What remains needs a **running Bloom** — do it in one session -(`run-bloom` skill), on branch `BL-6681-stage0-inventory`: +**Work is on hold as of 2026-08-06** at John's request. Everything below is pushed; nothing is +half-applied, and both branches are green with a clean working tree. + +### Stage 0's remainder — four items, all needing a running Bloom + +Do these in one session (`run-bloom` skill). **Not on `BL-6681-stage0-inventory`** — that branch is +under human review, and pushing to it would restart the review for work that is purely additive. +Branch off **`BL-6681-ckeditor`** instead (the files below are new; nothing conflicts). 1. **Finish rows G1–G3.** The seam's wiring is verified; the DOM-rewriting case is not (see the 2026-08-05 entry). Create a book from the **Decodable Reader** template, open the toolbox, @@ -720,7 +849,26 @@ All of Stage 0's code work is done. What remains needs a **running Bloom** — d way — it should fail before any fix. 4. **Page-reload timing baseline** (§4.11) with the performance-log feature. -Then: open the Stage 0 PR via `preflight`. +### Stage 1 — branch `BL-6681-stage1-undostack`, off `BL-6681-ckeditor` + +The new files are written, tested (31 tests) and inert. What remains: + +5. **Apply [DEFERRED-EDITS.md](DEFERRED-EDITS.md) §1a–1f** — no longer blocked on Stage 0 merging (see + the 2026-08-06 (later) entry), and they should not wait, or Stage 1 stays unverifiable for months. + Then run the five behavioural checks listed there in a running Bloom. Until they land, Stage 1's + code is unreachable and nothing about Undo has changed. +6. Decide where `clearPageScopedEntries()` hangs off (see 1d) — needs a running Bloom to confirm + `pageUnloading()` fires on a same-page reload. +7. Then PR the branch **into `BL-6681-ckeditor`**, not master, and squash-merge it (§5.2). + +**Standing chores while the branch is long-lived** (§5.3, §5.5): + +- **Weekly:** `git checkout BL-6681-ckeditor && git merge origin/master`, then record the master SHA + and the watchlist-commit count in the sync table at the top of this file. +- **After every sync:** `gh workflow run nightly.yml --ref BL-6681-ckeditor` — otherwise the branch + gets no full C# suite and no visual-regression coverage at all, for months. +- Set `git config rerere.enabled true` once, so a conflict resolved in one merge is reapplied in the + next. Note: launching Bloom uses `./go.sh`. If it fails with missing types like `PodcastUtilities` or `IDevice` (CS0246), this worktree lacks its C# dependencies — run `./init.sh` (see `AGENTS.md`). @@ -729,34 +877,3 @@ Later, not Stage 0: - Before designing `clipboard.ts`, read PR #8140 and `origin/BL-16459-clipboard-failure-reporting`. - Optional, offered but not done: comment on **BL-13502** that undo is another reason to want save/reload decoupled. - -### 2026-08-05 (later still) — Stage 0 preflighted; PR #8153 open as draft - -**PR:** https://github.com/BloomBooks/BloomDesktop/pull/8153 (draft). Branch pushed, card linked, -QA test-ideas comment posted, Devin consultation logged. - -Reviewer outcomes at HEAD `178269d78`: - -| Reviewer | Outcome | -| --- | --- | -| Local review (light, 1 subagent) | Clean — no correctness problems. It mutation-tested the new spec (neutering `selectAtOffset` fails 9 of 10 tests) and raised one accuracy note, which was fixed. | -| Devin | **Re-review clean** — 0 bugs, 0 investigate flags, 8 informational. Three informational items acted on; the rest declined with reasons, recorded in the PR consultation log. | -| CI (`pr-automation`) | pass | -| CodeRabbit | see the run's final report | - -**Three things Devin's informational tier caught that were worth fixing** — a reminder that the -lowest-signal tier is not always noise: - -1. The committed harness hard-coded `repoRoot = "C:/github/BloomDesktop"`, so it only ran in the - checkout it was written in. Now derived from `import.meta.url`. -2. **The extraction has two deliberate behaviour differences, not one.** - `restoreSelectionAfterMarkup` re-reads the editor and no-ops if it has gone, where the old code - sat inside `if (ckeditorOfThisBox)` and would have thrown. Unreachable in practice - (`bloomCkEditor` is assigned once and never cleared) but real, and now documented on the - function. Notable because this branch already corrects two *other* comments in the same pipeline - that misled by overstating — a third would have been poor form. -3. The inventory pointed at `PASTE-DROP-BASELINE.md` as if it existed. - -**G3 (longpress) is now verified too** — John spot-checked it manually and reports it basically -works. So of the G rows, G1 and G3 are verified, and **G2 (async markup path / BL-10133) is the one -still open**, along with the new G6/G7 highlight rows. From 1f93ab90eea5b04d8f21f2086912808c0da6640e Mon Sep 17 00:00:00 2001 From: John Thomson Date: Wed, 12 Aug 2026 15:29:16 -0500 Subject: [PATCH 02/10] Add the one undo stack, inert (BL-6681) Stage 1 of retiring CKEditor: the single undo entry point that the four existing mechanisms will be folded into, plus the transaction wrapper that keeps one user gesture to one undo entry. New files only. Nothing imports them, so this is behaviourally inert -- the edits to workspaceRoot.ts that would activate it are specified in docs/retire-ckeditor/DEFERRED-EDITS.md. - undoTypes.ts: IUndoEntry, ILegacyUndoProvider, kMaxUndoEntries. The long comment on IUndoEntry earns its length: it is the rule that shapes the whole design. The page iframe's JS context dies not only on page change but on same-page reloads (ctrl+wheel zoom, leaving Change Layout mode), so an entry built by page-frame code would later mutate a detached document or throw. Entries are therefore built in the workspace frame out of pure data. - UndoStack.ts: index-based with truncate-on-push, so Redo is possible without a second structure. canUndo/canRedo are O(1) because C# polls canUndo on a timer to set the button's enabled state, and anything that walks entries there makes the button flicker. Bounded by entry count. Page-scoped entries are discarded on page change, while entries with no pageId (deleting a page) survive. Redo state is captured lazily at undo time rather than at commit time, so nothing extra is paid per keystroke -- the same trick origamiUndo already uses. - legacyUndoProviders.ts: wrappers, not conversions, in exactly the order handleUndo used. Each carries the reason its mechanism behaves as it does, so that when it is deleted the reasoning does not go with it. - runUndoable.ts: depth-counted scopes, outermost wins, nested pushes dropped. Not speculative: deleting a canvas element whose content is a background image already records an image undo, so wrapping the delete naively would leave two entries and the first Ctrl+Z would half-undo the gesture. 31 tests. They passed first run, which for new code is a reason for suspicion rather than satisfaction, so I mutation-tested three of the load-bearing behaviours: removing truncate-on-push failed 1 test, disabling nested-push suppression failed 6, and making keepOnly recompute the index unconditionally failed 1 -- each caught by exactly the test meant to catch it. Co-Authored-By: Claude Opus 5 --- src/BloomBrowserUI/bookEdit/undo/UndoStack.ts | 274 ++++++++++++ .../bookEdit/undo/UndoStackSpec.ts | 410 ++++++++++++++++++ .../bookEdit/undo/legacyUndoProviders.ts | 113 +++++ .../bookEdit/undo/runUndoable.ts | 47 ++ .../bookEdit/undo/runUndoableSpec.ts | 143 ++++++ src/BloomBrowserUI/bookEdit/undo/undoTypes.ts | 106 +++++ 6 files changed, 1093 insertions(+) create mode 100644 src/BloomBrowserUI/bookEdit/undo/UndoStack.ts create mode 100644 src/BloomBrowserUI/bookEdit/undo/UndoStackSpec.ts create mode 100644 src/BloomBrowserUI/bookEdit/undo/legacyUndoProviders.ts create mode 100644 src/BloomBrowserUI/bookEdit/undo/runUndoable.ts create mode 100644 src/BloomBrowserUI/bookEdit/undo/runUndoableSpec.ts create mode 100644 src/BloomBrowserUI/bookEdit/undo/undoTypes.ts diff --git a/src/BloomBrowserUI/bookEdit/undo/UndoStack.ts b/src/BloomBrowserUI/bookEdit/undo/UndoStack.ts new file mode 100644 index 000000000000..a829375f7795 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/undo/UndoStack.ts @@ -0,0 +1,274 @@ +// The one undo stack (BL-6681, PLAN.md 4.1 / 4.2). +// +// Lives in the workspace frame, because the page iframe is destroyed on every page change and +// reload while the workspace frame is not. Deliberately free of DOM and jQuery dependencies so it +// can be unit-tested directly; everything frame-specific lives in legacyUndoProviders.ts or in the +// factories that build entries. + +import { ILegacyUndoProvider, IUndoEntry, kMaxUndoEntries } from "./undoTypes"; + +/** + * An index-based undo/redo stack, plus the arbitration between it and Bloom's pre-existing undo + * mechanisms. + * + * Index-based rather than pop-based because Redo is in scope: `undo()` steps the index back, + * `redo()` steps it forward, and any new push truncates everything above the index — so typing + * after an undo discards the redo branch, which is what every editor does. + */ +export class UndoStack { + private entries: IUndoEntry[] = []; + + /** + * Index of the entry that the *next* undo would apply; -1 when there is nothing to undo. + * Entries above it are the redo branch. + */ + private currentIndex = -1; + + /** Consulted before our own entries, in registration order. See {@link canUndo}. */ + private legacyProviders: ILegacyUndoProvider[] = []; + + /** The page entries are being recorded against. Set by whoever notices page changes. */ + private currentPageId: string | undefined; + + /** + * Labels of the `runUndoable` scopes currently open, outermost first. + * Non-empty means a push should be folded into the outermost scope rather than added. + */ + private openScopeLabels: string[] = []; + + /** Whether the outermost open scope has already claimed an entry. See {@link push}. */ + private pushedInOutermostScope = false; + + /** True while an undo or redo is being applied, to stop a re-entrant one interleaving. */ + private applying = false; + + /** + * Add an adapter for one of the pre-existing undo mechanisms. + * + * Order matters and is the caller's responsibility: providers are consulted in the order + * registered, which must reproduce the order `workspaceRoot.handleUndo` uses today. + */ + public registerLegacyProvider(provider: ILegacyUndoProvider): void { + this.legacyProviders.push(provider); + } + + /** Drop all legacy providers. For tests; also what Stage 5's deletions leave behind. */ + public clearLegacyProviders(): void { + this.legacyProviders = []; + } + + /** + * Record an undoable step. + * + * If a `runUndoable` scope is open this does *not* add a second entry — one user gesture must + * produce exactly one entry, however many layers of code it passes through. The outermost + * scope wins: the first push inside it is kept and relabelled with the scope's label, and + * later pushes within the same scope are ignored. See PLAN.md 4.13. + */ + public push(entry: IUndoEntry): void { + if (this.openScopeLabels.length > 0) { + if (this.pushedInOutermostScope) { + // A nested operation recording its own undo. Deliberately dropped: undoing the + // outermost operation already covers it, and keeping both would make the first + // Ctrl+Z half-undo the gesture. + return; + } + this.pushedInOutermostScope = true; + entry.label = this.openScopeLabels[0]; + } + + // Anything the user had undone is now unreachable: they have taken a different branch. + this.entries.length = this.currentIndex + 1; + + this.entries.push(entry); + if (this.entries.length > kMaxUndoEntries) { + this.entries.shift(); + } + this.currentIndex = this.entries.length - 1; + } + + /** + * Whether anything can be undone. + * + * Cheap and synchronous by contract: C# polls this on a timer to set the Undo button's enabled + * state (`WebView2Browser.UpdateEditButtonsAsync`), so it must not walk entries or touch + * layout. + * + * Legacy providers are consulted before our own entries, which reproduces today's behaviour + * exactly. See the note on {@link undo} about what that ordering does and does not guarantee. + */ + public canUndo(): boolean { + return ( + this.legacyProviders.some((p) => p.canUndo()) || + this.currentIndex >= 0 + ); + } + + /** Whether anything can be redone. O(1); false at a redo floor (an entry with no `redo`). */ + public canRedo(): boolean { + const next = this.entries[this.currentIndex + 1]; + return !!next?.redo; + } + + /** + * Undo one step. + * + * Order: each legacy provider that has something to undo, in registration order, then our own + * entries. That is exactly what `workspaceRoot.handleUndo` did before this class existed, so + * adopting the stack changes nothing while the stack is empty. + * + * What that ordering does *not* give us is true chronological order across the boundary: if a + * user does an operation recorded here and then one still handled by a legacy provider, the + * legacy one is undone first — which happens to be right — but in the other order it is wrong. + * That was already true between the old mechanisms (they were consulted in a fixed order too), + * and it stops being possible as each provider is converted. It is not worth inventing + * cross-mechanism sequencing for a state we are deleting. + */ + public undo(): void | Promise { + if (this.applying) { + return; + } + const provider = this.legacyProviders.find((p) => p.canUndo()); + if (provider) { + provider.undo(); + return; + } + if (this.currentIndex < 0) { + return; + } + const entry = this.entries[this.currentIndex]; + this.currentIndex--; + entry.prepareRedo?.(); + return this.apply(() => entry.undo()); + } + + /** + * Redo the step that was last undone. + * + * Legacy providers take no part: the only pre-existing Redo is origami's, which keeps using + * its own Ctrl+Y handler until it is converted. + */ + public redo(): void | Promise { + if (this.applying || !this.canRedo()) { + return; + } + const entry = this.entries[this.currentIndex + 1]; + this.currentIndex++; + return this.apply(() => entry.redo!()); + } + + /** + * Note which page we are on, discarding entries that belonged to a previous one. + * + * Page-scoped entries capture state within a page, so they are meaningless once the user has + * moved on; entries with no `pageId` (deleting a page) deliberately survive. + */ + public setCurrentPageId(pageId: string | undefined): void { + if (pageId === this.currentPageId) { + return; + } + this.currentPageId = pageId; + this.keepOnly((e) => e.pageId === undefined || e.pageId === pageId); + } + + /** The page id entries are currently being recorded against. */ + public getCurrentPageId(): string | undefined { + return this.currentPageId; + } + + /** + * Discard every page-scoped entry, keeping the ones that survive a page change. + * + * Called when the page frame reloads *without* the page changing — ctrl+wheel zoom and leaving + * origami layout mode both do that. The page id is the same, so `setCurrentPageId` would not + * notice, but the captured state is just as stale: the elements it describes have been rebuilt. + */ + public clearPageScopedEntries(): void { + this.keepOnly((e) => e.pageId === undefined); + } + + /** Discard everything. Used when leaving the edit tab, and by tests. */ + public clear(): void { + this.entries = []; + this.currentIndex = -1; + } + + /** How many entries are held. Tests and diagnostics only — not part of the undo contract. */ + public getEntryCount(): number { + return this.entries.length; + } + + /** The label of the entry the next undo would apply, or undefined. For tooltips and tests. */ + public peekUndoLabel(): string | undefined { + return this.entries[this.currentIndex]?.label; + } + + /** The label of the entry the next redo would apply, or undefined. */ + public peekRedoLabel(): string | undefined { + return this.entries[this.currentIndex + 1]?.label; + } + + /** + * Open a `runUndoable` scope. Call `endUndoableScope` in a `finally`. + * + * Only `runUndoable` should call this; it is public because it lives in another module. + */ + public beginUndoableScope(label: string): void { + if (this.openScopeLabels.length === 0) { + this.pushedInOutermostScope = false; + } + this.openScopeLabels.push(label); + } + + /** Close the innermost `runUndoable` scope. */ + public endUndoableScope(): void { + this.openScopeLabels.pop(); + } + + /** Whether a `runUndoable` scope is currently open. */ + public isInUndoableScope(): boolean { + return this.openScopeLabels.length > 0; + } + + /** Run an entry's undo/redo, holding the re-entrancy guard until it finishes. */ + private apply(action: () => void | Promise): void | Promise { + this.applying = true; + let result: void | Promise; + try { + result = action(); + } catch (e) { + this.applying = false; + throw e; + } + if (!result) { + this.applying = false; + return; + } + return result.finally(() => { + this.applying = false; + }); + } + + /** Filter entries, keeping `currentIndex` pointing at the same entry it did before. */ + private keepOnly(predicate: (entry: IUndoEntry) => boolean): void { + const kept: IUndoEntry[] = []; + let newIndex = -1; + for (let i = 0; i < this.entries.length; i++) { + if (!predicate(this.entries[i])) { + continue; + } + kept.push(this.entries[i]); + if (i <= this.currentIndex) { + newIndex = kept.length - 1; + } + } + this.entries = kept; + this.currentIndex = newIndex; + } +} + +/** + * The one stack. A singleton because C# and the other frames reach undo through a single + * function pair on the workspace bundle, and because "one consistent Undo stack" is the point. + */ +export const theOneUndoStack = new UndoStack(); diff --git a/src/BloomBrowserUI/bookEdit/undo/UndoStackSpec.ts b/src/BloomBrowserUI/bookEdit/undo/UndoStackSpec.ts new file mode 100644 index 000000000000..79b27707c326 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/undo/UndoStackSpec.ts @@ -0,0 +1,410 @@ +// Tests for the one undo stack (BL-6681). See docs/retire-ckeditor/PLAN.md 4.1. +// +// These are specification tests, not characterization tests: the stack is new code, so each case +// pins a decision the plan made rather than recording what some existing code happens to do. + +import { describe, it, expect, beforeEach } from "vitest"; +import { UndoStack } from "./UndoStack"; +import { ILegacyUndoProvider, IUndoEntry, kMaxUndoEntries } from "./undoTypes"; + +/** A minimal entry that appends its label to `log` when undone or redone. */ +function makeEntry( + label: string, + log: string[], + options?: { pageId?: string; canRedo?: boolean }, +): IUndoEntry { + const entry: IUndoEntry = { + label, + pageId: options && "pageId" in options ? options.pageId : "page1", + kind: "custom", + undo: () => { + log.push(`undo ${label}`); + }, + }; + // Redo is optional by design (an entry without it is a redo floor), so tests must be able to + // create entries both ways. + if (options?.canRedo !== false) { + entry.redo = () => { + log.push(`redo ${label}`); + }; + } + return entry; +} + +/** A legacy-mechanism adapter whose availability the test controls. */ +function makeProvider( + name: string, + log: string[], + available: () => boolean, +): ILegacyUndoProvider { + return { + name, + canUndo: available, + undo: () => { + log.push(`legacy ${name}`); + }, + }; +} + +describe("UndoStack", () => { + let stack: UndoStack; + let log: string[]; + + beforeEach(() => { + stack = new UndoStack(); + log = []; + }); + + describe("basic undo and redo", () => { + it("has nothing to undo or redo when empty", () => { + expect(stack.canUndo()).toBe(false); + expect(stack.canRedo()).toBe(false); + }); + + it("undoes the most recent entry first", () => { + stack.push(makeEntry("first", log)); + stack.push(makeEntry("second", log)); + expect(stack.canUndo()).toBe(true); + + stack.undo(); + stack.undo(); + + expect(log).toEqual(["undo second", "undo first"]); + expect(stack.canUndo()).toBe(false); + }); + + it("redoes in the reverse order of undoing", () => { + stack.push(makeEntry("first", log)); + stack.push(makeEntry("second", log)); + stack.undo(); + stack.undo(); + expect(stack.canRedo()).toBe(true); + + stack.redo(); + stack.redo(); + + expect(log).toEqual([ + "undo second", + "undo first", + "redo first", + "redo second", + ]); + expect(stack.canRedo()).toBe(false); + expect(stack.canUndo()).toBe(true); + }); + + it("does nothing when asked to undo or redo past the end", () => { + stack.push(makeEntry("only", log)); + stack.undo(); + expect(log).toEqual(["undo only"]); + + stack.undo(); // nothing left + stack.redo(); + stack.redo(); // nothing left to redo either + + expect(log).toEqual(["undo only", "redo only"]); + }); + }); + + describe("the redo branch", () => { + it("is discarded by a new push, so the new entry is what gets undone", () => { + stack.push(makeEntry("first", log)); + stack.push(makeEntry("second", log)); + stack.undo(); + // Sanity check: "second" is undone and would otherwise be redoable. + expect(log).toEqual(["undo second"]); + expect(stack.canRedo()).toBe(true); + + stack.push(makeEntry("third", log)); + + expect(stack.canRedo()).toBe(false); + expect(stack.getEntryCount()).toBe(2); // first, third — "second" is gone + stack.undo(); + expect(log).toEqual(["undo second", "undo third"]); + }); + + it("stops at an entry that cannot redo, rather than skipping it", () => { + stack.push(makeEntry("noRedo", log, { canRedo: false })); + stack.undo(); + + expect(stack.canRedo()).toBe(false); + stack.redo(); + + expect(log).toEqual(["undo noRedo"]); + }); + }); + + describe("lazy redo capture", () => { + it("calls prepareRedo immediately before undo, not at push time", () => { + const entry = makeEntry("captured", log); + entry.prepareRedo = () => { + log.push("prepareRedo"); + }; + + stack.push(entry); + // The whole point: pushing costs nothing extra. This is what keeps Redo cheap on the + // common path (every typing transaction). + expect(log).toEqual([]); + + stack.undo(); + + expect(log).toEqual(["prepareRedo", "undo captured"]); + }); + }); + + describe("page scoping", () => { + it("discards entries for other pages when the page changes", () => { + stack.setCurrentPageId("page1"); + stack.push(makeEntry("onPage1", log)); + stack.push(makeEntry("alsoPage1", log)); + expect(stack.getEntryCount()).toBe(2); + + stack.setCurrentPageId("page2"); + + expect(stack.getEntryCount()).toBe(0); + expect(stack.canUndo()).toBe(false); + }); + + it("keeps entries with no page id, so deleting a page stays undoable", () => { + stack.setCurrentPageId("page1"); + stack.push(makeEntry("pageScoped", log)); + stack.push(makeEntry("deletePage", log, { pageId: undefined })); + + stack.setCurrentPageId("page2"); + + expect(stack.getEntryCount()).toBe(1); + expect(stack.peekUndoLabel()).toBe("deletePage"); + stack.undo(); + expect(log).toEqual(["undo deletePage"]); + }); + + it("keeps the undo position pointing at the same entry after filtering", () => { + stack.setCurrentPageId("page1"); + stack.push(makeEntry("survives", log, { pageId: undefined })); + stack.push(makeEntry("dropped", log)); + stack.push(makeEntry("alsoSurvives", log, { pageId: undefined })); + stack.undo(); // undoes alsoSurvives; it is now the redo entry + expect(stack.peekUndoLabel()).toBe("dropped"); + + stack.setCurrentPageId("page2"); + + // "dropped" is gone, so the next undo is "survives" and the redo branch is intact. + expect(stack.peekUndoLabel()).toBe("survives"); + expect(stack.peekRedoLabel()).toBe("alsoSurvives"); + }); + + it("discards page-scoped entries on a page-frame reload that keeps the same page", () => { + stack.setCurrentPageId("page1"); + stack.push(makeEntry("pageScoped", log)); + stack.push(makeEntry("deletePage", log, { pageId: undefined })); + + // Ctrl+wheel zoom and leaving Change Layout mode both rebuild the page frame without + // changing page, so setCurrentPageId would not notice, but the captured DOM is stale. + stack.clearPageScopedEntries(); + + expect(stack.getEntryCount()).toBe(1); + expect(stack.peekUndoLabel()).toBe("deletePage"); + }); + + it("does not discard anything when told the page id it already has", () => { + stack.setCurrentPageId("page1"); + stack.push(makeEntry("onPage1", log)); + + stack.setCurrentPageId("page1"); + + expect(stack.getEntryCount()).toBe(1); + }); + }); + + describe("bounding", () => { + it("drops the oldest entry rather than growing without limit", () => { + for (let i = 0; i < kMaxUndoEntries + 5; i++) { + stack.push(makeEntry(`entry${i}`, log)); + } + + expect(stack.getEntryCount()).toBe(kMaxUndoEntries); + expect(stack.peekUndoLabel()).toBe( + `entry${kMaxUndoEntries + 5 - 1}`, + ); + // Undoing all the way down must stop cleanly at the truncated end. + for (let i = 0; i < kMaxUndoEntries; i++) { + stack.undo(); + } + expect(stack.canUndo()).toBe(false); + expect(log.length).toBe(kMaxUndoEntries); + expect(log[log.length - 1]).toBe("undo entry5"); + }); + }); + + describe("legacy providers", () => { + it("consults them in registration order, before our own entries", () => { + stack.registerLegacyProvider( + makeProvider("origami", log, () => true), + ); + stack.registerLegacyProvider( + makeProvider("toolbox", log, () => true), + ); + stack.push(makeEntry("ours", log)); + + stack.undo(); + + expect(log).toEqual(["legacy origami"]); + }); + + it("falls through to the next provider, and then to our entries", () => { + let origamiHasSomething = true; + stack.registerLegacyProvider( + makeProvider("origami", log, () => origamiHasSomething), + ); + stack.push(makeEntry("ours", log)); + + stack.undo(); + expect(log).toEqual(["legacy origami"]); + + origamiHasSomething = false; + stack.undo(); + + expect(log).toEqual(["legacy origami", "undo ours"]); + }); + + it("reports canUndo when only a legacy provider has something", () => { + expect(stack.canUndo()).toBe(false); // sanity check: nothing yet + stack.registerLegacyProvider( + makeProvider("image", log, () => true), + ); + + expect(stack.canUndo()).toBe(true); + }); + + it("takes no part in redo", () => { + stack.registerLegacyProvider( + makeProvider("origami", log, () => true), + ); + + // Origami keeps its own Ctrl+Y handler until it is converted, so the shared stack must + // not claim to be able to redo on its behalf. + expect(stack.canRedo()).toBe(false); + stack.redo(); + expect(log).toEqual([]); + }); + }); + + describe("undoable scopes (runUndoable's mechanism)", () => { + it("keeps only the first entry pushed in a scope, relabelled with the scope label", () => { + stack.beginUndoableScope("Delete canvas element"); + stack.push(makeEntry("inner image undo", log)); + stack.push(makeEntry("another inner push", log)); + stack.endUndoableScope(); + + expect(stack.getEntryCount()).toBe(1); + expect(stack.peekUndoLabel()).toBe("Delete canvas element"); + }); + + it("treats a nested scope as part of the outer one", () => { + stack.beginUndoableScope("outer"); + stack.beginUndoableScope("inner"); + stack.push(makeEntry("pushed by inner", log)); + stack.endUndoableScope(); + stack.push(makeEntry("pushed by outer", log)); + stack.endUndoableScope(); + + expect(stack.getEntryCount()).toBe(1); + expect(stack.peekUndoLabel()).toBe("outer"); + }); + + it("starts a fresh claim for each new outermost scope", () => { + stack.beginUndoableScope("first gesture"); + stack.push(makeEntry("a", log)); + stack.endUndoableScope(); + stack.beginUndoableScope("second gesture"); + stack.push(makeEntry("b", log)); + stack.endUndoableScope(); + + expect(stack.getEntryCount()).toBe(2); + expect(stack.peekUndoLabel()).toBe("second gesture"); + }); + + it("records normally again once the scope is closed", () => { + stack.beginUndoableScope("gesture"); + stack.push(makeEntry("a", log)); + stack.push(makeEntry("b", log)); + stack.endUndoableScope(); + expect(stack.isInUndoableScope()).toBe(false); + + stack.push(makeEntry("afterwards", log)); + + expect(stack.getEntryCount()).toBe(2); + expect(stack.peekUndoLabel()).toBe("afterwards"); + }); + }); + + describe("asynchronous entries", () => { + it("waits for an async undo before allowing another", async () => { + let release: () => void = () => { + throw new Error("test bug: release called before it was set"); + }; + const slow: IUndoEntry = { + label: "slow", + pageId: "page1", + kind: "custom", + undo: () => + new Promise((resolve) => { + release = () => { + log.push("undo slow finished"); + resolve(); + }; + }), + }; + stack.push(slow); + stack.push(makeEntry("fast", log)); + + // "fast" is on top and is synchronous, so this completes before returning. + const inFlight = stack.undo(); + expect(log).toEqual(["undo fast"]); + expect(inFlight).toBeUndefined(); + + const slowPromise = stack.undo(); + // While that is in flight a second undo must be ignored rather than interleaved. + stack.undo(); + expect(log).toEqual(["undo fast"]); + + release(); + await slowPromise; + + expect(log).toEqual(["undo fast", "undo slow finished"]); + }); + + it("releases the guard when an undo throws, so undo is not wedged", () => { + const bad: IUndoEntry = { + label: "bad", + pageId: "page1", + kind: "custom", + undo: () => { + throw new Error("boom"); + }, + }; + stack.push(makeEntry("good", log)); + stack.push(bad); + + expect(() => stack.undo()).toThrow("boom"); + + // The stack must still work; a broken entry must not disable Undo for the session. + stack.undo(); + expect(log).toEqual(["undo good"]); + }); + }); + + describe("clear", () => { + it("discards everything, including entries that survive page changes", () => { + stack.push(makeEntry("pageScoped", log)); + stack.push(makeEntry("deletePage", log, { pageId: undefined })); + expect(stack.canUndo()).toBe(true); // sanity check + + stack.clear(); + + expect(stack.getEntryCount()).toBe(0); + expect(stack.canUndo()).toBe(false); + expect(stack.canRedo()).toBe(false); + }); + }); +}); diff --git a/src/BloomBrowserUI/bookEdit/undo/legacyUndoProviders.ts b/src/BloomBrowserUI/bookEdit/undo/legacyUndoProviders.ts new file mode 100644 index 000000000000..2292b9cb0e3d --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/undo/legacyUndoProviders.ts @@ -0,0 +1,113 @@ +// Adapters that let Bloom's four pre-existing undo mechanisms take part in the one stack +// (BL-6681, PLAN.md 6 Stage 1). +// +// This is deliberately a wrapping exercise and not a conversion. Registering these in the order +// below reproduces exactly what workspaceRoot.handleUndo() / canUndo() did before the stack +// existed, so the stack can become the single entry point with no behaviour change at all. +// +// Know what that order governs. handleUndo() has one caller: the toolbar Undo button. Ctrl+Z is +// claimed in the *page* frame — by origami in Change Layout mode, by the reader tools in any +// editable while a markup tool is active (they preventDefault), and otherwise by CKEditor — and +// never enters handleUndo() at all. So this file unifies the button, and the keyboard stays as it +// was until those page-frame handlers are converted. See PLAN.md 3's correction. +// +// Why no conversions yet: the four mechanisms are contextually exclusive in practice (origami only +// in Change Layout mode, the reader undo only with an active markup tool, image undo only on an +// image container), so their relative ordering only starts to matter once text edits share the +// stack, which is Stage 3. Converting them now would mean maturing the riskiest new machinery — +// in-place snapshot restore — on a page full of live CKEditor instances. +// +// Each function here disappears when its mechanism is converted (Stages 3 and 4), and the last one +// out takes this file with it. + +import { + getEditablePageBundleExports, + getToolboxBundleExports, +} from "../js/workspaceFrames"; +import { theOneUndoStack, UndoStack } from "./UndoStack"; +import { ILegacyUndoProvider } from "./undoTypes"; + +/** + * Origami's own stack of jQuery `clone(true)` copies of `.marginBox`. + * + * Only ever has anything while Change Layout mode is active. It also has its own Ctrl+Z/Ctrl+Y + * handler bound to `html`, and its own Redo, both of which keep working independently until it is + * converted — which must happen in a single commit with retiring that handler, or its Redo breaks + * in between. + */ +export const origamiUndoProvider: ILegacyUndoProvider = { + name: "origami", + canUndo: () => !!getEditablePageBundleExports()?.origamiCanUndo(), + undo: () => getEditablePageBundleExports()?.origamiUndo(), +}; + +/** + * The toolbox's per-editable text-typing undo (`readerToolsModel`). + * + * Despite living in the toolbox, this is a text undo, not a "reader setup" undo: it snapshots + * `{html, text, caretOffset}` per editable and pushes on markup-changing keystrokes. It is + * consulted *before* CKEditor on purpose — while a reader tool is active it must shadow CKEditor's + * undo, which would otherwise restore stale decodable/leveled markup. + * + * It is also the one legacy mechanism with a Redo (`readerToolsModel.redo`), reached by + * Ctrl+Y/Ctrl+Shift+Z from the reader tools' own per-editable handler. The shared stack does not + * offer it: that handler keeps working, and this adapter covers only the button's undo. + * + * `canUndo` is called through an existence check because the old `canUndo()` did the same, and it + * is polled on a timer by C#: a throw here would fire repeatedly. `undo` is not, also matching the + * old code, since it only runs just after `canUndo()` returned true. + */ +export const toolboxUndoProvider: ILegacyUndoProvider = { + name: "toolbox", + canUndo: () => { + const toolbox = getToolboxBundleExports(); + return !!toolbox?.canUndo?.(); + }, + undo: () => getToolboxBundleExports()?.undo(), +}; + +/** + * `ImageUndoManager` — restores an image's src, copyright or crop. + * + * The cleanest of the four: a two-phase prepare/commit, already scoped by page id, and gated on + * the active element being an image container. A good candidate to convert early, since it depends + * on nothing in Stage 3. + */ +export const imageUndoProvider: ILegacyUndoProvider = { + name: "image", + canUndo: () => !!getEditablePageBundleExports()?.imageOperationCanUndo(), + undo: () => { + getEditablePageBundleExports()?.imageOperationUndo(); + }, +}; + +/** + * CKEditor's own per-editable undo manager — the mechanism this whole project exists to replace. + * + * Its ordering across boxes is already wrong (each editable has its own stack, so undo follows + * focus rather than time). We reproduce that rather than fix it: fixing it here would be a + * behaviour change in the commit whose whole value is being behaviour-neutral, and Stage 3 removes + * the mechanism. + */ +export const ckeditorUndoProvider: ILegacyUndoProvider = { + name: "ckeditor", + canUndo: () => !!getEditablePageBundleExports()?.ckeditorCanUndo(), + undo: () => getEditablePageBundleExports()?.ckeditorUndo(), +}; + +/** + * Register all four, in the order `workspaceRoot.handleUndo()` used. + * + * Call once, as the workspace frame sets up. Registering twice would double-consult each + * mechanism, which is harmless for `canUndo` but would be confusing, so callers should not. + * + * @param stack defaults to the one real stack; a parameter only so tests need not use a singleton. + */ +export function registerLegacyUndoProviders( + stack: UndoStack = theOneUndoStack, +): void { + stack.registerLegacyProvider(origamiUndoProvider); + stack.registerLegacyProvider(toolboxUndoProvider); + stack.registerLegacyProvider(imageUndoProvider); + stack.registerLegacyProvider(ckeditorUndoProvider); +} diff --git a/src/BloomBrowserUI/bookEdit/undo/runUndoable.ts b/src/BloomBrowserUI/bookEdit/undo/runUndoable.ts new file mode 100644 index 000000000000..1e084a836f31 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/undo/runUndoable.ts @@ -0,0 +1,47 @@ +// One user gesture, one undo entry (BL-6681, PLAN.md 4.13). +// +// Wrapping an operation in runUndoable() says "everything in here is one undoable step". It exists +// because nesting is not hypothetical: deleting a canvas element whose content is a background +// image already records an image undo of its own, so wrapping the delete naively would leave two +// entries for one gesture and the first Ctrl+Z would half-undo it. Call sites accrete, so the +// semantics are fixed here from the start rather than patched in when a bug turns up. + +import { theOneUndoStack, UndoStack } from "./UndoStack"; + +/** + * Run `operation` as a single undoable step, however many nested operations record undos inside it. + * + * The outermost scope wins: the first entry pushed inside it is kept and takes `label` as its + * label, and any further pushes within the scope are dropped. `label` is what the user would call + * the whole gesture — "Delete canvas element" — not what the innermost layer of code calls it. + * + * Works for a synchronous or an asynchronous operation: if `operation` returns a promise the scope + * stays open until it settles, and the promise is passed through. Two *independent* asynchronous + * undoables must not overlap in time — the scope depth is global, so an unrelated operation + * starting while another is awaiting would be treated as nested. Every intended use is a single + * user gesture, so overlap does not arise; it is written down because it would be invisible. + * + * @param stack defaults to the one real stack; a parameter only so tests need not use a singleton. + */ +export function runUndoable( + label: string, + operation: () => T, + stack: UndoStack = theOneUndoStack, +): T { + stack.beginUndoableScope(label); + let result: T; + try { + result = operation(); + } catch (e) { + stack.endUndoableScope(); + throw e; + } + const promise = result as unknown as Promise | undefined; + if (typeof promise?.finally === "function") { + return promise.finally(() => { + stack.endUndoableScope(); + }) as unknown as T; + } + stack.endUndoableScope(); + return result; +} diff --git a/src/BloomBrowserUI/bookEdit/undo/runUndoableSpec.ts b/src/BloomBrowserUI/bookEdit/undo/runUndoableSpec.ts new file mode 100644 index 000000000000..c8efd1ae5a9d --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/undo/runUndoableSpec.ts @@ -0,0 +1,143 @@ +// Tests for runUndoable (BL-6681). See docs/retire-ckeditor/PLAN.md 4.13. +// +// The case that motivates all of this: deleting a canvas element whose content is a background +// image already records an image undo of its own, so one gesture must not leave two entries. + +import { describe, it, expect, beforeEach } from "vitest"; +import { runUndoable } from "./runUndoable"; +import { UndoStack } from "./UndoStack"; +import { IUndoEntry } from "./undoTypes"; + +function makeEntry(label: string, log: string[]): IUndoEntry { + return { + label, + pageId: "page1", + kind: "custom", + undo: () => { + log.push(`undo ${label}`); + }, + }; +} + +describe("runUndoable", () => { + let stack: UndoStack; + let log: string[]; + + beforeEach(() => { + stack = new UndoStack(); + log = []; + }); + + it("produces one entry for a gesture whose inner code also records an undo", () => { + runUndoable( + "Delete canvas element", + () => { + // What deleteCanvasElement's background-image branch does today. + stack.push(makeEntry("image operation", log)); + stack.push(makeEntry("element removal", log)); + }, + stack, + ); + + expect(stack.getEntryCount()).toBe(1); + expect(stack.peekUndoLabel()).toBe("Delete canvas element"); + stack.undo(); + // The kept entry is the *first* one pushed, relabelled — not a new synthetic entry. + expect(log).toEqual(["undo image operation"]); + }); + + it("returns the operation's value", () => { + const result = runUndoable("compute", () => 42, stack); + + expect(result).toBe(42); + }); + + it("closes the scope so later work records normally", () => { + runUndoable("gesture", () => stack.push(makeEntry("a", log)), stack); + expect(stack.isInUndoableScope()).toBe(false); + + stack.push(makeEntry("later", log)); + + expect(stack.getEntryCount()).toBe(2); + expect(stack.peekUndoLabel()).toBe("later"); + }); + + it("closes the scope even when the operation throws", () => { + expect(() => + runUndoable( + "gesture that fails", + () => { + stack.push(makeEntry("recorded before the failure", log)); + throw new Error("boom"); + }, + stack, + ), + ).toThrow("boom"); + + // A leaked scope would silently swallow every later undo entry, which is far worse than + // the original failure and much harder to diagnose. + expect(stack.isInUndoableScope()).toBe(false); + expect(stack.getEntryCount()).toBe(1); + }); + + it("treats a nested runUndoable as part of the outer gesture", () => { + runUndoable( + "outer gesture", + () => { + runUndoable( + "inner gesture", + () => stack.push(makeEntry("inner", log)), + stack, + ); + stack.push(makeEntry("outer", log)); + }, + stack, + ); + + expect(stack.getEntryCount()).toBe(1); + expect(stack.peekUndoLabel()).toBe("outer gesture"); + }); + + it("holds the scope open across an await, and passes the promise through", async () => { + let resolveInner: () => void = () => { + throw new Error("test bug: resolve called before it was set"); + }; + const gate = new Promise((resolve) => { + resolveInner = resolve; + }); + + const promise = runUndoable( + "async gesture", + async () => { + stack.push(makeEntry("first", log)); + await gate; + stack.push(makeEntry("after the await", log)); + return "done"; + }, + stack, + ); + + // Sanity check: we really are mid-operation, with the scope still open. + expect(stack.isInUndoableScope()).toBe(true); + resolveInner(); + const result = await promise; + + expect(result).toBe("done"); + expect(stack.isInUndoableScope()).toBe(false); + expect(stack.getEntryCount()).toBe(1); + expect(stack.peekUndoLabel()).toBe("async gesture"); + }); + + it("closes the scope when an async operation rejects", async () => { + const promise = runUndoable( + "async gesture that fails", + async () => { + throw new Error("async boom"); + }, + stack, + ); + + await expect(promise).rejects.toThrow("async boom"); + expect(stack.isInUndoableScope()).toBe(false); + }); +}); diff --git a/src/BloomBrowserUI/bookEdit/undo/undoTypes.ts b/src/BloomBrowserUI/bookEdit/undo/undoTypes.ts new file mode 100644 index 000000000000..b96a9b6dada0 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/undo/undoTypes.ts @@ -0,0 +1,106 @@ +// The contract for Bloom's single undo stack (BL-6681). +// +// Today Bloom has five poorly-coordinated undo mechanisms; see docs/retire-ckeditor/PLAN.md 3. +// This file defines the one entry type they will all eventually become, plus the adapter interface +// that lets the old mechanisms take part before they are converted. Nothing here touches the DOM, +// so it can be unit-tested and imported from any frame. + +/** + * How an entry restores. + * + * - `custom` — the entry carries its own `undo()`. Fully supported now. + * - `pageSnapshot` / `subtreeSnapshot` — the entry carries captured HTML and restores it through + * the tiered restore paths of PLAN.md 4.11. Declared here so the kind field is stable, but no + * snapshot entries exist until Stage 3; the stack treats them exactly like any other entry (it + * just calls `undo()`), so the factory that builds one owns the restore logic. + */ +export type UndoEntryKind = "pageSnapshot" | "subtreeSnapshot" | "custom"; + +/** + * One undoable step. + * + * ## The rule that shapes this interface: an entry must not close over page-frame objects + * + * The page iframe's JS context dies not only when the user changes page but on same-page + * *reloads* — ctrl+wheel zoom regenerates the page, leaving origami layout mode posts + * `saveChangesAndRethinkPageEvent`, and several tools navigate. A function object created in that + * frame dies with it, so an entry built by page-frame code becomes a live grenade: `undo()` would + * mutate a detached document, or simply throw. + * + * So entries are **built in the workspace frame** (which survives), out of **pure data** — HTML + * strings, indices, ids. Anything an entry needs from the page frame it must re-acquire *inside* + * `undo()` via `getEditablePageBundleExports()`. Page-frame code that wants to record an undo + * therefore sends a *description* of what happened across the frame boundary and lets the + * workspace frame build the entry; it never sends a closure. See PLAN.md 4.1 and 4.2. + */ +export interface IUndoEntry { + /** Human-readable, e.g. "Delete canvas element". For tooltips and logging, not identity. */ + label: string; + + /** + * The page this entry belongs to, or `undefined` if it survives a page change. + * + * `undefined` is for workspace-owned operations — deleting a page being the main one, where + * the whole point is that the page is gone. Everything else is page-scoped and is discarded + * when the user moves to another page, because its captured state would no longer mean + * anything. + */ + pageId: string | undefined; + + /** Which restore strategy this entry represents. See {@link UndoEntryKind}. */ + kind: UndoEntryKind; + + /** Reverse the operation. May be async (a restore that has to wait for the page frame). */ + undo(): void | Promise; + + /** + * Re-apply the operation. Optional, so Redo can arrive one entry kind at a time: an entry + * with no `redo` acts as a redo floor (`canRedo()` is false when the next entry can't redo). + * That lets the one case needing real C# work — redoing a page deletion — be deferred without + * holding up the rest. + */ + redo?(): void | Promise; + + /** + * Capture whatever `redo()` will need, called by the stack immediately before `undo()` runs. + * + * Capturing the "after" state lazily like this is what keeps Redo nearly free: nothing extra + * is paid on the common path (every typing transaction), only when the user actually undoes. + * Bloom already does exactly this — `origamiUndo` stashes a fresh clone before stepping its + * index back. + */ + prepareRedo?(): void; +} + +/** + * An adapter round one of Bloom's pre-existing undo mechanisms. + * + * Stage 1 wraps all of them rather than converting any, so that the single entry point can land + * with no behaviour change at all: the stack consults these in exactly the order + * `workspaceRoot.handleUndo` used to. Each one disappears as its mechanism is converted to push + * real {@link IUndoEntry}s, and the last one to go takes this interface with it. + */ +export interface ILegacyUndoProvider { + /** Identifies the provider in logs and test failures, e.g. "origami". */ + name: string; + + /** + * Whether this mechanism has something to undo *right now*. + * + * Must be cheap and synchronous: C# polls the aggregate `canUndo` on a timer to decide + * whether the Undo button is enabled, so anything that walks a stack or forces layout here + * makes the button flicker. + */ + canUndo(): boolean; + + /** Undo one step. Only called when `canUndo()` has just returned true. */ + undo(): void; +} + +/** + * How many entries the stack keeps. + * + * Bounded by count rather than bytes: the worst case is ~50 page-HTML strings, which is + * single-digit MB. Revisit only if something proves byte accounting is needed. + */ +export const kMaxUndoEntries = 50; From f9238303180d87a5c2cd2f7f8764a0745789cf45 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Mon, 7 Sep 2026 09:54:11 -0500 Subject: [PATCH 03/10] Activate the one undo stack: the Undo button and Ctrl+Y go through it (BL-6681) Stage 1's DEFERRED-EDITS 1a-1e. workspaceRoot.handleUndo() and canUndo() become delegations to theOneUndoStack, whose four legacy providers reproduce the old if-chain in the same order. The button path is the only thing this changes; Ctrl+Z is still claimed per-context in the page frame. Three things differ from the deferred-edits text as written, all because master moved or the premise was wrong: - BL-16558 (on master since) made handleUndo call updateMarkupAfterUndoOrRedo() after the reader-tools and CKEditor undos, because both rewrite an editable's innerHTML and so detach the highlights painted over it. The toolbox and ckeditor providers now do the same, and legacyUndoProvidersSpec pins it. - Redo is bound in the PAGE frame (undo/redoKeyBinding.ts, one call from editablePage.ts), as the last resort: it acts only when nothing earlier in the bubble claimed Ctrl+Y and the stack has something to redo, so origami's and the reader tools' handlers, and CKEditor's own redo, keep winning until converted. The workspace bundle grows canRedo()/handleRedo() for it. - Page identity is the .bloom-page element's id, not data-page-id: nothing in Bloom sets that attribute (only ImageUndoManagerSpec does), so the check in ImageUndoManager that reads it never fires. pageFrameUndoHooks.ts clears page-scoped entries whenever switchContentPage runs -- the one route C# uses to navigate the page frame, same page or not -- and records the id on load. Also corrected: ctrl+wheel zoom no longer reloads the page (it is a CSS transform via setZoom), so the comments citing it as a same-page reload are fixed; the real same-page reloads are leaving Change Layout mode, importing a video and changing the topic. 52 undo tests green (21 new), typecheck clean, no new lint warnings. --- src/BloomBrowserUI/bookEdit/editablePage.ts | 6 + src/BloomBrowserUI/bookEdit/undo/UndoStack.ts | 7 +- .../bookEdit/undo/legacyUndoProviders.ts | 23 ++- .../bookEdit/undo/legacyUndoProvidersSpec.ts | 139 ++++++++++++++++++ .../bookEdit/undo/pageFrameUndoHooks.ts | 47 ++++++ .../bookEdit/undo/pageFrameUndoHooksSpec.ts | 93 ++++++++++++ .../bookEdit/undo/redoKeyBinding.ts | 65 ++++++++ .../bookEdit/undo/redoKeyBindingSpec.ts | 111 ++++++++++++++ src/BloomBrowserUI/bookEdit/undo/undoTypes.ts | 10 +- src/BloomBrowserUI/bookEdit/workspaceRoot.ts | 95 +++++------- 10 files changed, 532 insertions(+), 64 deletions(-) create mode 100644 src/BloomBrowserUI/bookEdit/undo/legacyUndoProvidersSpec.ts create mode 100644 src/BloomBrowserUI/bookEdit/undo/pageFrameUndoHooks.ts create mode 100644 src/BloomBrowserUI/bookEdit/undo/pageFrameUndoHooksSpec.ts create mode 100644 src/BloomBrowserUI/bookEdit/undo/redoKeyBinding.ts create mode 100644 src/BloomBrowserUI/bookEdit/undo/redoKeyBindingSpec.ts diff --git a/src/BloomBrowserUI/bookEdit/editablePage.ts b/src/BloomBrowserUI/bookEdit/editablePage.ts index c88eafcf3748..bbec0d0191c9 100644 --- a/src/BloomBrowserUI/bookEdit/editablePage.ts +++ b/src/BloomBrowserUI/bookEdit/editablePage.ts @@ -17,6 +17,8 @@ import { } from "./js/canvasElementManager/CanvasElementManager"; import { kCanvasElementSelector } from "./toolbox/canvas/canvasElementConstants"; import { renderDragActivityTabControl } from "./js/AbovePageControls"; +import { installRedoKeyBinding } from "./undo/redoKeyBinding"; +import { tryGetWorkspaceBundleExports } from "./js/workspaceFrames"; function getPageId(): string { const page = document.querySelector(".bloom-page"); @@ -383,6 +385,10 @@ $(document).ready(() => { // in the live editor, which never reads this flag. window.__bloomEditablePageReady = true; + // Ctrl+Y reaches the one undo stack's Redo from here, as the last resort behind the handlers + // that already claim it (see undo/redoKeyBinding.ts). Null in the off-screen context. + installRedoKeyBinding(document, tryGetWorkspaceBundleExports); + // 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. diff --git a/src/BloomBrowserUI/bookEdit/undo/UndoStack.ts b/src/BloomBrowserUI/bookEdit/undo/UndoStack.ts index a829375f7795..5ed1241c9b1d 100644 --- a/src/BloomBrowserUI/bookEdit/undo/UndoStack.ts +++ b/src/BloomBrowserUI/bookEdit/undo/UndoStack.ts @@ -179,9 +179,10 @@ export class UndoStack { /** * Discard every page-scoped entry, keeping the ones that survive a page change. * - * Called when the page frame reloads *without* the page changing — ctrl+wheel zoom and leaving - * origami layout mode both do that. The page id is the same, so `setCurrentPageId` would not - * notice, but the captured state is just as stale: the elements it describes have been rebuilt. + * Called whenever the page frame is about to navigate (see pageFrameUndoHooks.ts). It exists + * for the reloads that keep the *same* page — leaving origami layout mode, importing a video, + * changing the topic — where `setCurrentPageId` would see no change, but the captured state is + * just as stale: the elements it describes have been rebuilt. */ public clearPageScopedEntries(): void { this.keepOnly((e) => e.pageId === undefined); diff --git a/src/BloomBrowserUI/bookEdit/undo/legacyUndoProviders.ts b/src/BloomBrowserUI/bookEdit/undo/legacyUndoProviders.ts index 2292b9cb0e3d..4353a6091380 100644 --- a/src/BloomBrowserUI/bookEdit/undo/legacyUndoProviders.ts +++ b/src/BloomBrowserUI/bookEdit/undo/legacyUndoProviders.ts @@ -63,7 +63,18 @@ export const toolboxUndoProvider: ILegacyUndoProvider = { const toolbox = getToolboxBundleExports(); return !!toolbox?.canUndo?.(); }, - undo: () => getToolboxBundleExports()?.undo(), + undo: () => { + const toolbox = getToolboxBundleExports(); + if (!toolbox) { + return; + } + toolbox.undo(); + // The reader tools' undo restores a saved innerHTML, which replaces the text nodes their + // highlights are painted over. Nothing else will notice: unlike Ctrl+Z, a click on the + // Undo button produces no keystroke in the page, so the usual keyup markup update never + // happens and the highlights would stay dead. (BL-16558) + toolbox.updateMarkupAfterUndoOrRedo(); + }, }; /** @@ -92,7 +103,15 @@ export const imageUndoProvider: ILegacyUndoProvider = { export const ckeditorUndoProvider: ILegacyUndoProvider = { name: "ckeditor", canUndo: () => !!getEditablePageBundleExports()?.ckeditorCanUndo(), - undo: () => getEditablePageBundleExports()?.ckeditorUndo(), + undo: () => { + getEditablePageBundleExports()?.ckeditorUndo(); + // As for the toolbox provider: this undo replaces the content of an editable, and there + // is no keystroke to trigger the markup update that repaints the tools' highlights over + // the new text nodes. (ckeditorUndo calls the undoManager directly rather than the undo + // command, so the afterCommandExec handler in attachToCkEditor does not see this one.) + // (BL-16558) + getToolboxBundleExports()?.updateMarkupAfterUndoOrRedo(); + }, }; /** diff --git a/src/BloomBrowserUI/bookEdit/undo/legacyUndoProvidersSpec.ts b/src/BloomBrowserUI/bookEdit/undo/legacyUndoProvidersSpec.ts new file mode 100644 index 000000000000..de8934cb592b --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/undo/legacyUndoProvidersSpec.ts @@ -0,0 +1,139 @@ +// Tests for the adapters round Bloom's pre-existing undo mechanisms (BL-6681). +// +// What these pin is not the mechanisms themselves but the arbitration that workspaceRoot.handleUndo +// used to do inline, and which is easy to lose in a refactor: the order the four are consulted in, +// and the BL-16558 rule that an undo which rewrites an editable's innerHTML must be followed by a +// markup update, because it has just detached every highlight painted over that box. + +import { describe, it, expect, beforeEach, vi } from "vitest"; +import { UndoStack } from "./UndoStack"; + +// The providers reach the other frames through workspaceFrames; there are no frames in jsdom. +const page = { + origamiCanUndo: vi.fn(() => false), + origamiUndo: vi.fn(), + imageOperationCanUndo: vi.fn(() => false), + imageOperationUndo: vi.fn(() => true), + ckeditorCanUndo: vi.fn(() => false), + ckeditorUndo: vi.fn(), +}; +const toolbox = { + canUndo: vi.fn(() => false) as (() => boolean) | undefined, + undo: vi.fn(), + updateMarkupAfterUndoOrRedo: vi.fn(), +}; +let pageAvailable = true; +let toolboxAvailable = true; +vi.mock("../js/workspaceFrames", () => ({ + getEditablePageBundleExports: () => (pageAvailable ? page : null), + getToolboxBundleExports: () => (toolboxAvailable ? toolbox : null), +})); + +import { + ckeditorUndoProvider, + imageUndoProvider, + origamiUndoProvider, + registerLegacyUndoProviders, + toolboxUndoProvider, +} from "./legacyUndoProviders"; + +describe("legacyUndoProviders", () => { + beforeEach(() => { + vi.clearAllMocks(); + page.origamiCanUndo.mockReturnValue(false); + page.imageOperationCanUndo.mockReturnValue(false); + page.ckeditorCanUndo.mockReturnValue(false); + toolbox.canUndo = vi.fn(() => false); + pageAvailable = true; + toolboxAvailable = true; + }); + + it("registers the four in the order handleUndo consulted them", () => { + const stack = new UndoStack(); + // Make every mechanism claim to have something, so the order alone decides. + page.origamiCanUndo.mockReturnValue(true); + toolbox.canUndo = vi.fn(() => true); + page.imageOperationCanUndo.mockReturnValue(true); + page.ckeditorCanUndo.mockReturnValue(true); + registerLegacyUndoProviders(stack); + + stack.undo(); + expect(page.origamiUndo).toHaveBeenCalledTimes(1); + expect(toolbox.undo).not.toHaveBeenCalled(); + + page.origamiCanUndo.mockReturnValue(false); + stack.undo(); + expect(toolbox.undo).toHaveBeenCalledTimes(1); + expect(page.imageOperationUndo).not.toHaveBeenCalled(); + + toolbox.canUndo = vi.fn(() => false); + stack.undo(); + expect(page.imageOperationUndo).toHaveBeenCalledTimes(1); + expect(page.ckeditorUndo).not.toHaveBeenCalled(); + + page.imageOperationCanUndo.mockReturnValue(false); + stack.undo(); + expect(page.ckeditorUndo).toHaveBeenCalledTimes(1); + }); + + describe("the markup update after an undo that rewrites an editable (BL-16558)", () => { + it("follows the reader tools' undo", () => { + toolboxUndoProvider.undo(); + expect(toolbox.undo).toHaveBeenCalledTimes(1); + expect(toolbox.updateMarkupAfterUndoOrRedo).toHaveBeenCalledTimes( + 1, + ); + }); + + it("follows CKEditor's undo", () => { + ckeditorUndoProvider.undo(); + expect(page.ckeditorUndo).toHaveBeenCalledTimes(1); + expect(toolbox.updateMarkupAfterUndoOrRedo).toHaveBeenCalledTimes( + 1, + ); + }); + + it("does not follow origami's or the image undo, which rewrite no editable", () => { + origamiUndoProvider.undo(); + imageUndoProvider.undo(); + expect(toolbox.updateMarkupAfterUndoOrRedo).not.toHaveBeenCalled(); + }); + + it("survives CKEditor's undo running with no toolbox frame", () => { + toolboxAvailable = false; + expect(() => ckeditorUndoProvider.undo()).not.toThrow(); + expect(page.ckeditorUndo).toHaveBeenCalledTimes(1); + }); + }); + + describe("canUndo", () => { + it("is false for every provider when the frames are not there yet", () => { + pageAvailable = false; + toolboxAvailable = false; + for (const p of [ + origamiUndoProvider, + toolboxUndoProvider, + imageUndoProvider, + ckeditorUndoProvider, + ]) { + expect(p.canUndo(), p.name).toBe(false); + } + }); + + it("tolerates a toolbox bundle with no canUndo, because C# polls it on a timer", () => { + toolbox.canUndo = undefined; + expect(toolboxUndoProvider.canUndo()).toBe(false); + }); + + it("reports each mechanism's own answer", () => { + page.origamiCanUndo.mockReturnValue(true); + expect(origamiUndoProvider.canUndo()).toBe(true); + toolbox.canUndo = vi.fn(() => true); + expect(toolboxUndoProvider.canUndo()).toBe(true); + page.imageOperationCanUndo.mockReturnValue(true); + expect(imageUndoProvider.canUndo()).toBe(true); + page.ckeditorCanUndo.mockReturnValue(true); + expect(ckeditorUndoProvider.canUndo()).toBe(true); + }); + }); +}); diff --git a/src/BloomBrowserUI/bookEdit/undo/pageFrameUndoHooks.ts b/src/BloomBrowserUI/bookEdit/undo/pageFrameUndoHooks.ts new file mode 100644 index 000000000000..d4106b3daf45 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/undo/pageFrameUndoHooks.ts @@ -0,0 +1,47 @@ +// Keeping the one undo stack informed about the page frame's lifetime (BL-6681). +// +// The stack lives in the workspace frame and outlives the page frame, so it has to be told when the +// page frame is replaced: page-scoped entries describe elements that are about to be rebuilt and +// mean nothing afterwards. Both hooks are called from workspaceRoot.switchContentPage, which is the +// one route C# uses to navigate the page frame (EditingView.cs) -- whether to a different page or +// to a rebuilt copy of the same one (leaving Change Layout mode, importing a video, changing the +// topic). That is why "navigating" clears rather than waiting to see whether the id changes. + +import { getBloomPageElement } from "../../utils/shared"; +import { theOneUndoStack, UndoStack } from "./UndoStack"; + +/** + * The id of the page currently loaded in the page frame, or undefined if there is none (nothing + * loaded yet, or about:blank). + * + * A page's identity is the `id` attribute of its `.bloom-page` element -- the same thing the page + * frame itself reports to C# (editablePage.ts, getPageId). It is NOT `data-page-id`: nothing in + * Bloom sets that attribute (only ImageUndoManagerSpec does), so the check in + * `ImageUndoManager.clearImageOperationUndoOnPageChange` that reads it is comparing undefined with + * undefined and never fires. That manager gets away with it because it lives in the page frame and + * dies with the page; this stack does not, so it has to get this right. + */ +export function getCurrentPageIdFromPageFrame(): string | undefined { + return getBloomPageElement()?.id || undefined; +} + +/** + * The page frame is about to navigate. Everything scoped to the page it is showing is now stale, + * whether or not the next page has the same id. + */ +export function pageFrameNavigating(stack: UndoStack = theOneUndoStack): void { + stack.clearPageScopedEntries(); +} + +/** + * The page frame has loaded (or, on the 1500 ms fallback in switchContentPage, is assumed to have). + * Records which page entries are now being made against. + * + * Idempotent for an unchanged id, so being called twice, or late, is harmless. Being called EARLY + * is not quite: an entry pushed before this runs is attributed to whatever id was current, which + * could be the previous page. Nothing pushes automatically yet; this has to be looked at again when + * typing starts recording entries (Stage 3). + */ +export function pageFrameLoaded(stack: UndoStack = theOneUndoStack): void { + stack.setCurrentPageId(getCurrentPageIdFromPageFrame()); +} diff --git a/src/BloomBrowserUI/bookEdit/undo/pageFrameUndoHooksSpec.ts b/src/BloomBrowserUI/bookEdit/undo/pageFrameUndoHooksSpec.ts new file mode 100644 index 000000000000..feea9f53d9a2 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/undo/pageFrameUndoHooksSpec.ts @@ -0,0 +1,93 @@ +// Tests for the hooks that tell the one undo stack about the page frame's lifetime (BL-6681). + +import { describe, it, expect, beforeEach, vi } from "vitest"; +import { UndoStack } from "./UndoStack"; +import { IUndoEntry } from "./undoTypes"; + +// The hooks find the page through utils/shared, which reaches into the page iframe. There is no +// iframe in jsdom, so stand in for that one lookup. +let pageElement: HTMLElement | null = null; +vi.mock("../../utils/shared", () => ({ + getBloomPageElement: () => pageElement, +})); + +import { + getCurrentPageIdFromPageFrame, + pageFrameLoaded, + pageFrameNavigating, +} from "./pageFrameUndoHooks"; + +function entry(label: string, pageId: string | undefined): IUndoEntry { + return { label, pageId, kind: "custom", undo: () => {} }; +} + +function makePage(id: string): HTMLElement { + const div = document.createElement("div"); + div.className = "bloom-page"; + div.id = id; + return div; +} + +describe("pageFrameUndoHooks", () => { + let stack: UndoStack; + + beforeEach(() => { + stack = new UndoStack(); + pageElement = null; + }); + + describe("getCurrentPageIdFromPageFrame", () => { + it("reads the .bloom-page element's id", () => { + pageElement = makePage("page-abc"); + expect(getCurrentPageIdFromPageFrame()).toBe("page-abc"); + }); + + it("is undefined when there is no page, or the page has no id", () => { + expect(getCurrentPageIdFromPageFrame()).toBeUndefined(); + pageElement = makePage(""); + expect(getCurrentPageIdFromPageFrame()).toBeUndefined(); + }); + }); + + describe("pageFrameNavigating", () => { + it("drops page-scoped entries and keeps the ones that survive a page change", () => { + stack.setCurrentPageId("page-1"); + stack.push(entry("delete page", undefined)); + stack.push(entry("typing", "page-1")); + expect(stack.getEntryCount()).toBe(2); // sanity + + pageFrameNavigating(stack); + + expect(stack.getEntryCount()).toBe(1); + expect(stack.peekUndoLabel()).toBe("delete page"); + }); + + it("clears even when the page id is not going to change", () => { + // Leaving Change Layout mode rebuilds the same page under its own id; the rebuilt + // elements are new, so the old entries are just as stale as after a real page change. + stack.setCurrentPageId("page-1"); + stack.push(entry("typing", "page-1")); + pageElement = makePage("page-1"); + + pageFrameNavigating(stack); + pageFrameLoaded(stack); + + expect(stack.getEntryCount()).toBe(0); + expect(stack.getCurrentPageId()).toBe("page-1"); + }); + }); + + describe("pageFrameLoaded", () => { + it("records the loaded page's id on the stack", () => { + pageElement = makePage("page-2"); + pageFrameLoaded(stack); + expect(stack.getCurrentPageId()).toBe("page-2"); + }); + + it("records undefined when no page is loaded", () => { + stack.setCurrentPageId("page-2"); + pageFrameLoaded(stack); + expect(stack.getCurrentPageId()).toBeUndefined(); + }); + }); +}); diff --git a/src/BloomBrowserUI/bookEdit/undo/redoKeyBinding.ts b/src/BloomBrowserUI/bookEdit/undo/redoKeyBinding.ts new file mode 100644 index 000000000000..452a269510c8 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/undo/redoKeyBinding.ts @@ -0,0 +1,65 @@ +// Ctrl+Y in the page frame reaches the one undo stack's Redo (BL-6681). +// +// Why the binding is in the PAGE frame although the stack lives in the workspace frame: keyboard +// events inside the page iframe are delivered to that iframe's document and never reach the parent, +// so a workspace-frame handler would fire only when focus is outside the page -- which is the +// opposite of when Redo is wanted. This is also why both pre-existing Ctrl+Y handlers are in the +// page frame: origami's on `html` (origami.ts) and the reader tools' on each editable +// (decodableReaderTool.tsx). +// +// Why it is the LAST resort and not the first: those two handlers claim the keystroke where they +// apply (the reader tools' one returns false, which stops propagation; origami's fires but does not +// prevent the default), and CKEditor's own redo command handles Ctrl+Y inside a box with anything +// on its per-box stack. Until those mechanisms are converted (Stages 3-4) they must keep winning. +// So this handler sits at the document, in the bubble phase, and acts only when nothing earlier +// claimed the event AND the shared stack actually has something to redo. When it does not, the +// keystroke falls through untouched to whatever would have handled it before this existed. +// +// There is no Redo button and no C# involvement: Redo is JS-only by decision (PLAN.md 10). + +/** The part of the workspace bundle this binding needs. Kept small so a test can fake it. */ +export interface IRedoTarget { + canRedo(): boolean; + handleRedo(): void; +} + +/** Whether this keydown is the Redo gesture: Ctrl+Y with no other modifier. */ +export function isRedoKeystroke(e: KeyboardEvent): boolean { + return ( + e.ctrlKey && + !e.altKey && + !e.metaKey && + !e.shiftKey && + (e.key === "y" || e.key === "Y") + ); +} + +/** + * Listen for Ctrl+Y on `doc` and redo through the workspace bundle when it has something to redo. + * + * `getTarget` is called per keystroke rather than once, because the workspace bundle is reached + * across frames and may legitimately be absent (the off-screen page-processing context loads a + * page with no workspace root). A null target means "do nothing", not an error. + */ +export function installRedoKeyBinding( + doc: Document, + getTarget: () => IRedoTarget | null, +): void { + doc.addEventListener("keydown", (e: KeyboardEvent) => { + if (!isRedoKeystroke(e)) { + return; + } + // Someone earlier in the bubble already claimed this keystroke (e.g. the reader tools' + // handler, when a markup tool is active). Not ours. + if (e.defaultPrevented) { + return; + } + const target = getTarget(); + if (!target?.canRedo()) { + // Nothing of ours to redo: leave the keystroke to CKEditor's redo or the browser's. + return; + } + e.preventDefault(); + target.handleRedo(); + }); +} diff --git a/src/BloomBrowserUI/bookEdit/undo/redoKeyBindingSpec.ts b/src/BloomBrowserUI/bookEdit/undo/redoKeyBindingSpec.ts new file mode 100644 index 000000000000..c5440fb7f571 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/undo/redoKeyBindingSpec.ts @@ -0,0 +1,111 @@ +// Tests for the page-frame Ctrl+Y binding (BL-6681). See redoKeyBinding.ts for why it exists and +// why it must be the last resort rather than the first. + +import { describe, it, expect, beforeEach } from "vitest"; +import { + installRedoKeyBinding, + IRedoTarget, + isRedoKeystroke, +} from "./redoKeyBinding"; + +function keydown(init: KeyboardEventInit): KeyboardEvent { + return new KeyboardEvent("keydown", { + bubbles: true, + cancelable: true, + ...init, + }); +} + +describe("redoKeyBinding", () => { + describe("isRedoKeystroke", () => { + it("is Ctrl+Y, either case", () => { + expect(isRedoKeystroke(keydown({ key: "y", ctrlKey: true }))).toBe( + true, + ); + expect(isRedoKeystroke(keydown({ key: "Y", ctrlKey: true }))).toBe( + true, + ); + }); + + it("is not Y alone, Ctrl+Z, or Ctrl+Y with another modifier", () => { + expect(isRedoKeystroke(keydown({ key: "y" }))).toBe(false); + expect(isRedoKeystroke(keydown({ key: "z", ctrlKey: true }))).toBe( + false, + ); + expect( + isRedoKeystroke( + keydown({ key: "y", ctrlKey: true, shiftKey: true }), + ), + ).toBe(false); + expect( + isRedoKeystroke( + keydown({ key: "y", ctrlKey: true, altKey: true }), + ), + ).toBe(false); + }); + }); + + describe("installRedoKeyBinding", () => { + let doc: Document; + let editable: HTMLElement; + let target: IRedoTarget; + let canRedo: boolean; + let redoCalls: number; + + beforeEach(() => { + doc = document.implementation.createHTMLDocument("page"); + editable = doc.createElement("div"); + doc.body.appendChild(editable); + canRedo = false; + redoCalls = 0; + target = { + canRedo: () => canRedo, + handleRedo: () => { + redoCalls++; + }, + }; + installRedoKeyBinding(doc, () => target); + }); + + it("redoes, and claims the keystroke, when the stack has something to redo", () => { + canRedo = true; + const e = keydown({ key: "y", ctrlKey: true }); + editable.dispatchEvent(e); + expect(redoCalls).toBe(1); + expect(e.defaultPrevented).toBe(true); + }); + + it("leaves the keystroke alone when there is nothing to redo", () => { + // This is what lets CKEditor's redo, and the browser's, keep working until converted. + canRedo = false; + const e = keydown({ key: "y", ctrlKey: true }); + editable.dispatchEvent(e); + expect(redoCalls).toBe(0); + expect(e.defaultPrevented).toBe(false); + }); + + it("defers to a handler earlier in the bubble that already claimed the keystroke", () => { + canRedo = true; + // Stand-in for the reader tools' per-editable handler, which prevents the default. + editable.addEventListener("keydown", (e) => e.preventDefault()); + const e = keydown({ key: "y", ctrlKey: true }); + editable.dispatchEvent(e); + expect(redoCalls).toBe(0); + }); + + it("ignores keystrokes that are not Ctrl+Y", () => { + canRedo = true; + editable.dispatchEvent(keydown({ key: "y" })); + editable.dispatchEvent(keydown({ key: "z", ctrlKey: true })); + expect(redoCalls).toBe(0); + }); + + it("does nothing when there is no workspace bundle to reach", () => { + const bare = document.implementation.createHTMLDocument("bare"); + installRedoKeyBinding(bare, () => null); + const e = keydown({ key: "y", ctrlKey: true }); + bare.body.dispatchEvent(e); + expect(e.defaultPrevented).toBe(false); + }); + }); +}); diff --git a/src/BloomBrowserUI/bookEdit/undo/undoTypes.ts b/src/BloomBrowserUI/bookEdit/undo/undoTypes.ts index b96a9b6dada0..b5c944ddf310 100644 --- a/src/BloomBrowserUI/bookEdit/undo/undoTypes.ts +++ b/src/BloomBrowserUI/bookEdit/undo/undoTypes.ts @@ -22,10 +22,12 @@ export type UndoEntryKind = "pageSnapshot" | "subtreeSnapshot" | "custom"; * ## The rule that shapes this interface: an entry must not close over page-frame objects * * The page iframe's JS context dies not only when the user changes page but on same-page - * *reloads* — ctrl+wheel zoom regenerates the page, leaving origami layout mode posts - * `saveChangesAndRethinkPageEvent`, and several tools navigate. A function object created in that - * frame dies with it, so an entry built by page-frame code becomes a live grenade: `undo()` would - * mutate a detached document, or simply throw. + * *reloads* — leaving origami layout mode posts `saveChangesAndRethinkPageEvent`, importing a + * video and changing the topic rebuild the page under its own id, and several tools navigate. (An + * earlier draft cited ctrl+wheel zoom too; that is now a CSS transform, `workspaceRoot.setZoom`, + * and reloads nothing.) A function object created in that frame dies with it, so an entry built by + * page-frame code becomes a live grenade: `undo()` would mutate a detached document, or simply + * throw. * * So entries are **built in the workspace frame** (which survives), out of **pure data** — HTML * strings, indices, ids. Anything an entry needs from the page frame it must re-acquire *inside* diff --git a/src/BloomBrowserUI/bookEdit/workspaceRoot.ts b/src/BloomBrowserUI/bookEdit/workspaceRoot.ts index c864174008ae..b2251e22413b 100644 --- a/src/BloomBrowserUI/bookEdit/workspaceRoot.ts +++ b/src/BloomBrowserUI/bookEdit/workspaceRoot.ts @@ -10,6 +10,17 @@ import { import { postJson } from "../utils/bloomApi"; import "../modified_libraries/jquery-ui/jquery-ui-1.10.3.custom.min.js"; //for dialog() import $ from "jquery"; +import { theOneUndoStack } from "./undo/UndoStack"; +import { registerLegacyUndoProviders } from "./undo/legacyUndoProviders"; +import { + pageFrameLoaded, + pageFrameNavigating, +} from "./undo/pageFrameUndoHooks"; + +// The one undo stack (BL-6681) arbitrates between Bloom's pre-existing undo mechanisms until +// they are converted. Registering them is all it takes; this module is loaded once per edit-tab +// session, and the providers only reach across frames when consulted. +registerLegacyUndoProviders(); export interface IWorkspaceExports { showDialog( @@ -53,6 +64,9 @@ export interface IWorkspaceExports { showBookSettingsDialog(initiallySelectedPageKey?: string): void; showImageGalleryDialog(img: HTMLElement, searchLang: string): void; openAiImageEditor(target: IAiImageEditorTarget): void; + // Redo has no button and no C# side; the page frame's Ctrl+Y binding reaches it here. + canRedo(): boolean; + handleRedo(): void; } export function SayHello() { @@ -101,45 +115,22 @@ export { showAdjustTimingsDialog as showAdjustTimingsDialogFromWorkspaceRoot }; // Local alias so we have an in-scope identifier for legacy global exposure typing. const showAdjustTimingsDialogFromWorkspaceRoot = showAdjustTimingsDialog; +// The top bar's Undo button (via topBarButtonClick in the page frame) ends up here. Everything +// about WHICH mechanism gets to undo -- origami, the reader tools, image operations, CKEditor, +// in that order and for the reasons recorded there -- lives in undo/legacyUndoProviders.ts, and +// the stack's own entries come after them. See docs/retire-ckeditor/PLAN.md 3 and 6 (Stage 1). export function handleUndo(): void { - // First see if origami is active and knows about something we can undo. - // (Origami undo works only while the origami tool is active.) - const contentWindow = getEditablePageBundleExports(); - if (contentWindow && contentWindow.origamiCanUndo()) { - contentWindow.origamiUndo(); - return; - } - // Undoing changes made by commands and dialogs in the toolbox can't be undone using - // ckeditor, and has its own mechanism. Look next to see whether we know about any Undos there. - const toolboxWindow = getToolboxBundleExports(); - if (toolboxWindow && toolboxWindow.canUndo()) { - toolboxWindow.undo(); - // The reader tools' undo restores a saved innerHTML, which replaces the text nodes - // their highlights are painted over. Nothing else will notice: unlike Ctrl+Z, a click - // on this button produces no keystroke in the page, so the usual keyup markup update - // never happens and the highlights would stay dead. (BL-16558) - toolboxWindow.updateMarkupAfterUndoOrRedo(); - return; - } - // In an ideal world, we would have all undo information stored in the order of the operations. - // But since ckeditor and image operations handle undo differently, we don't have that ordering. - // And each textbox has its own ckeditor instance, so their undo stacks are already separate. - // The canUndoImageOperation check verifies that we are on a canvas element that contains an image, - // which makes things work similarly to having multiple textboxes on a page. However, multiple image - // boxes will operate on a single undo stack unlike mutiple textboxes. - // Because they are independent, and operational only the the proper context, it doesn't really - // matter in which order we check for undo operations. - if (contentWindow && contentWindow.imageOperationCanUndo()) { - contentWindow.imageOperationUndo(); - } else if (contentWindow && contentWindow.ckeditorCanUndo()) { - contentWindow.ckeditorUndo(); - // As above: this undo replaces the content of an editable, and there is no keystroke - // to trigger the markup update that repaints the tools' highlights over the new text - // nodes. (We call ckeditor's undoManager directly rather than its undo command, so the - // afterCommandExec handler in attachToCkEditor doesn't see this one.) - toolboxWindow?.updateMarkupAfterUndoOrRedo(); - } - // See also Browser.Undo; if all else fails we ask the C# browser object to Undo. + void theOneUndoStack.undo(); +} + +// Ctrl+Y, from the page frame's binding (undo/redoKeyBinding.ts). There is no Redo button. +export function handleRedo(): void { + void theOneUndoStack.redo(); +} + +// Whether Ctrl+Y would do anything. O(1): the page frame asks on every Ctrl+Y keydown. +export function canRedo(): boolean { + return theOneUndoStack.canRedo(); } // We need this update to maintain relative paths to images for the thumbnails. (BL-15906) @@ -150,6 +141,9 @@ export function switchThumbnailPage(newSource: string) { } export function switchContentPage(newSource: string) { + // Whatever undo entries were scoped to the page being shown are about to describe elements + // that no longer exist. This runs before the try below on purpose: it touches no frame. + pageFrameNavigating(); try { const editablePageBundle = getEditablePageBundleExports(); if (editablePageBundle?.pageUnloading) { @@ -181,6 +175,7 @@ export function switchContentPage(newSource: string) { const handler = () => { handlerCalled = true; iframe.removeEventListener("load", handler); + pageFrameLoaded(); doWhenToolboxLoaded((toolboxFrameExports: IToolboxFrameExports) => { toolboxFrameExports.applyToolboxStateToPage(); }); @@ -262,24 +257,10 @@ export function doWhenToolboxLoaded( } } -//Called by c# using workspaceBundle.canUndo() +//Called by c# using workspaceBundle.canUndo(), polled on a timer to set the Undo button's +// enabled state (WebView2Browser.CanUndoAsync). "yes"/"fail" is that contract; keep it. export function canUndo(): string { - // See comments on handleUndo() - const contentWindow = getEditablePageBundleExports(); - if (contentWindow && contentWindow.origamiCanUndo()) { - return "yes"; - } - const toolboxWindow = getToolboxBundleExports(); - if (toolboxWindow && toolboxWindow.canUndo && toolboxWindow.canUndo()) { - return "yes"; - } - if (contentWindow && contentWindow.imageOperationCanUndo()) { - return "yes"; - } - if (contentWindow && contentWindow.ckeditorCanUndo()) { - return "yes"; - } - return "fail"; //can't undo in Javascript, possibly something in C# can? + return theOneUndoStack.canUndo() ? "yes" : "fail"; } //noinspection JSUnusedGlobalSymbols @@ -423,6 +404,8 @@ export function setZoom(zoom: number): void { interface WorkspaceBundleApi { SayHello: typeof SayHello; handleUndo: typeof handleUndo; + handleRedo: typeof handleRedo; + canRedo: typeof canRedo; switchThumbnailPage: typeof switchThumbnailPage; switchContentPage: typeof switchContentPage; showDialog: typeof showDialog; @@ -467,6 +450,8 @@ window.workspaceBundle = { // simple exports SayHello, handleUndo, + handleRedo, + canRedo, switchThumbnailPage, switchContentPage, showDialog, From ac09a5878e53b344cf9fa893d2bb3ab77f5f7094 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Mon, 7 Sep 2026 10:18:29 -0500 Subject: [PATCH 04/10] Record the 2026-09-07 sync, Stage 1 verification and BL-13502 assessment (BL-6681) PROGRESS.md: the master-sync row, the Version6.5 finding (the merge window may now be open; John's call), the live-verification results, the two pre-existing reader-tools bugs the harness exposed, what BL-13502 changes for the plan, and revised next actions. PLAN.md: the zoom premise and the 5.1 drift table were wrong and are corrected. DEFERRED-EDITS.md: entries 1a-1e are landed; the verification checklist is ticked with what proved each item. docs/retire-ckeditor/liveChecks/: the four CDP harnesses that produced those results, kept so later stages can show a mechanism moved onto the stack and nothing else did. They attribute each gesture by wrapping the cross-frame entry points and listening to CKEditor's afterCommandExec, not by looking at the text. --- docs/retire-ckeditor/DEFERRED-EDITS.md | 194 ++++------------- docs/retire-ckeditor/PLAN.md | 26 ++- docs/retire-ckeditor/PROGRESS.md | 197 +++++++++++++++--- docs/retire-ckeditor/liveChecks/README.md | 45 ++++ docs/retire-ckeditor/liveChecks/cdp.mjs | 78 +++++++ docs/retire-ckeditor/liveChecks/gotoBook.mjs | 36 ++++ docs/retire-ckeditor/liveChecks/verifyCk.mjs | 102 +++++++++ .../liveChecks/verifyCommon.mjs | 177 ++++++++++++++++ .../liveChecks/verifyImage.mjs | 88 ++++++++ .../liveChecks/verifyOrigami.mjs | 114 ++++++++++ .../liveChecks/verifyReader.mjs | 100 +++++++++ 11 files changed, 968 insertions(+), 189 deletions(-) create mode 100644 docs/retire-ckeditor/liveChecks/README.md create mode 100644 docs/retire-ckeditor/liveChecks/cdp.mjs create mode 100644 docs/retire-ckeditor/liveChecks/gotoBook.mjs create mode 100644 docs/retire-ckeditor/liveChecks/verifyCk.mjs create mode 100644 docs/retire-ckeditor/liveChecks/verifyCommon.mjs create mode 100644 docs/retire-ckeditor/liveChecks/verifyImage.mjs create mode 100644 docs/retire-ckeditor/liveChecks/verifyOrigami.mjs create mode 100644 docs/retire-ckeditor/liveChecks/verifyReader.mjs diff --git a/docs/retire-ckeditor/DEFERRED-EDITS.md b/docs/retire-ckeditor/DEFERRED-EDITS.md index 5425de61da15..def87291ccd9 100644 --- a/docs/retire-ckeditor/DEFERRED-EDITS.md +++ b/docs/retire-ckeditor/DEFERRED-EDITS.md @@ -15,165 +15,47 @@ so the right place is still findable. ## Stage 1 — activate the one undo stack -**New code (already landed, inert):** `src/BloomBrowserUI/bookEdit/undo/` — `undoTypes.ts`, -`UndoStack.ts`, `legacyUndoProviders.ts`, `runUndoable.ts` and their specs. Nothing imports them -yet, so the bundle is unchanged in behaviour and very nearly unchanged in size. - -**Why they were deferred:** these edits are all in `workspaceRoot.ts`, which the Stage 0 PR (#8153) -does not touch but which sits next to code that PR does touch, so waiting until Stage 0 merged kept -the two reviews independent. - -**They are no longer blocked (2026-08-06).** Under the no-merging constraint (PLAN.md §5) Stage 0 -will not reach `master` for months — deferring until then would leave Stage 1 unverifiable for the -whole period, which is much worse than the review-independence it was buying. And the reason has -gone anyway: Stage 0's commits are the base of the integration branch `BL-6681-ckeditor`, and Stage -1's PR targets that branch, so its diff shows only Stage 1's own changes. **Apply these on the Stage -1 branch.** Every file they touch had zero commits on `master` in the 30 days measured in §5.1, so -the integration risk is as low as it gets. - -### 1a. Register the legacy providers, once - -In `workspaceRoot.ts`, alongside the other module-level imports: - -```ts -import { registerLegacyUndoProviders } from "./undo/legacyUndoProviders"; -import { theOneUndoStack } from "./undo/UndoStack"; - -registerLegacyUndoProviders(); -``` - -Module-level is right: `workspaceRoot` is loaded once per edit-tab session, and the providers only -reach across frames when consulted, so nothing needs the frames to exist yet. - -*Safe because:* registration does no work. **Do not call it twice** — each mechanism would be -consulted twice, harmless but confusing. - -### 1b. `handleUndo()` becomes a delegation - -**Scope, so the verification below is not over-claimed:** `handleUndo` has exactly one caller — -`topBarButtonClick` (`bloomEditing.ts:1633-1648`), i.e. the toolbar Undo button. There is no Ctrl+Z -handler in the workspace frame, and C#'s `UndoCommand.Implementer` is an empty lambda -(`WebView2Browser.cs:890`) existing only to make the button's `Enabled` settable. Ctrl+Z is claimed in -the *page* frame by origami, by the reader tools, or by CKEditor. So these edits change the **button** -path only; the keyboard path is untouched, which is both why they are safe and why "one consistent -Undo" is not yet true for the keystroke. See PLAN.md §3's correction. - - -Replace the body of `handleUndo()` (`workspaceRoot.ts:97-126`) with: - -```ts -export function handleUndo(): void { - theOneUndoStack.undo(); -} -``` - -The four-way if-chain being deleted is reproduced exactly by the providers, in the same order, in -`legacyUndoProviders.registerLegacyUndoProviders()`. Two comments in the deleted body should move -rather than die, because they record *why* the order is what it is — they are already carried in -`legacyUndoProviders.ts`, so check them across before deleting. - -**One comment must not move: it is wrong.** `workspaceRoot.ts:125` says "*See also Browser.Undo; if -all else fails we ask the C# browser object to Undo*". There is no such fallback in the WebView2 -code — the Undo button's enabled state comes purely from `canUndo()` returning `"yes"`. Delete it. - -### 1c. `canUndo()` becomes a delegation - -Replace the body of `canUndo()` (`workspaceRoot.ts:248-266`) with: - -```ts -//Called by c# using workspaceBundle.canUndo() -export function canUndo(): string { - return theOneUndoStack.canUndo() ? "yes" : "fail"; -} -``` - -Keep the `"yes"`/`"fail"` strings: that is the contract with `WebView2Browser.CanUndoAsync`, which -polls it on a timer. Changing it is a separate, C#-touching change and not worth bundling in. - -*Watch for:* the old `canUndo` guarded the toolbox call as `toolboxWindow.canUndo && -toolboxWindow.canUndo()` while `handleUndo` did not. `toolboxUndoProvider` keeps that asymmetry -deliberately (a throw in a timer-polled function fires repeatedly), and says so. - -### 1d. Tell the stack when the page changes - -`UndoStack.setCurrentPageId()` exists but nothing calls it, so page-scoped entries are never -discarded. The hook point is the `load` handler already inside `switchContentPage` -(`workspaceRoot.ts:163-172`), which is where the new page's DOM first exists: - -```ts -const handler = () => { - handlerCalled = true; - iframe.removeEventListener("load", handler); - theOneUndoStack.setCurrentPageId(getCurrentPageIdFromPageFrame()); - doWhenToolboxLoaded(...); -}; -``` - -The page id lives on the current page element as `data-page-id` — the same source -`ImageUndoManager` uses for exactly this purpose (`ImageUndoManager.ts:154-161`, -`clearImageOperationUndoOnPageChange`). Reuse that, don't invent a second notion of page identity. - -Note the 1500 ms fallback below it: the `load` event sometimes never fires, and `handler` is called -on a timer instead. `setCurrentPageId` is idempotent for an unchanged id, so being called twice or -late is harmless — but it means an entry pushed in that window could be attributed to the previous -page. Nothing pushes automatically in Stage 1, so this cannot bite yet; it must be re-examined when -Stage 3 starts recording typing. - -**Also needed, and not covered by the above:** `clearPageScopedEntries()` on a page-frame reload -that keeps the *same* page — ctrl+wheel zoom (`bloomEditing.ts:1259-1276`, whose own comment says -"Zooming re-loads the page") and leaving Change Layout mode (`origami.ts:193`). `switchContentPage` is not involved in either, so this needs its own hook. -`pageUnloading()` (already called at `workspaceRoot.ts:138`) is the candidate; confirm it runs on -same-page reloads before relying on it. - -### 1e. Expose Redo (Ctrl+Y only) — and it cannot live in the workspace frame - -There is **no Redo plumbing in C# at all** — no `RedoCommand`, no `SetEditingCommands` parameter, -nothing in the `updateEditButtons` payload, no icon, no XLF entry. So a Redo *button* is where the -real cost is, and it is deliberately out of scope. Ctrl+Y is JS-only and needs none of it. - -Add to `workspaceRoot.ts`: - -```ts -export function handleRedo(): void { - theOneUndoStack.redo(); -} -``` - -**But do not bind Ctrl+Y in the workspace frame.** Keyboard events inside the page iframe are -delivered to that iframe's document and never reach the parent, so a workspace-frame handler would -fire only when focus is outside the page — which is the opposite of when Redo is wanted. This is why -*both* existing Ctrl+Y handlers are in the page frame: origami's on `html` (`origami.ts:137`) and the -reader tools' on each editable (`decodableReaderTool.tsx:158-178`). So the new binding goes in the -page frame too, and calls `getWorkspaceBundleExports().handleRedo()`. - -**Both existing handlers `preventDefault()` and win where they apply**, so the new one must be the -last resort, not the first: - -- In Change Layout mode, origami's handler claims Ctrl+Y. Leave it — it is the only Redo for layout - changes until Stage 4 converts it, and that conversion must retire the handler in the *same* - commit or its Redo breaks in between. -- In any editable while a reader tool is active (`currentMarkupType !== None`), the reader tools' - handler claims Ctrl+Z *and* Ctrl+Y and returns false. Also leave it. - -*Verify:* press Ctrl+Y in Change Layout mode, in a reader-tool text box, and in an ordinary text box, -and confirm exactly one redo happens in each — not two, and not none. +**New code:** `src/BloomBrowserUI/bookEdit/undo/` — `undoTypes.ts`, `UndoStack.ts`, +`legacyUndoProviders.ts`, `runUndoable.ts`, `pageFrameUndoHooks.ts`, `redoKeyBinding.ts` and their +specs. + +**Entries 1a–1e landed 2026-09-07** (commit `f92383031` on `BL-6681-stage1-undostack`). What they +did, briefly, so this file still explains the shape of `workspaceRoot.ts`: + +- `registerLegacyUndoProviders()` is called once at module level in `workspaceRoot.ts`. +- `handleUndo()` and `canUndo()` are delegations to `theOneUndoStack`; the four-way if-chain, its + order, and the BL-16558 markup-update calls now live in `undo/legacyUndoProviders.ts`. The stale + "*See also Browser.Undo*" comment is gone. +- `switchContentPage()` calls `pageFrameNavigating()` before touching the old frame and + `pageFrameLoaded()` in its load handler (`undo/pageFrameUndoHooks.ts`). One hook covers same-page + reloads too, because every page-frame navigation C# makes goes through `switchContentPage`. +- `handleRedo()` / `canRedo()` are exported on the workspace bundle; Ctrl+Y is bound in the page frame + by `undo/redoKeyBinding.ts`, installed from `editablePage.ts`'s ready handler, as the last resort + behind origami's and the reader tools' handlers and CKEditor's own redo. + +Three things differed from the entries as originally written — recorded in PROGRESS.md +(2026-09-07): master's BL-16558 had changed `handleUndo`; `data-page-id` is never set, so the page id +is `.bloom-page`'s `id`; and ctrl+wheel zoom no longer reloads the page. ### 1f. Expose the cross-frame push -`IWorkspaceExports` (`workspaceRoot.ts:14-55`) and the global exposure object at the bottom of the -file both need whatever Stage 2 pushes with. **Do not export `push(entry)` across frames** — that -would hand page-frame code the ability to put a page-frame closure on the stack, which is exactly -the failure `undoTypes.ts` documents at length. Export a function taking *data* and let the -workspace frame build the entry. Design it with Stage 2's first real caller, not before. +`IWorkspaceExports` (`workspaceRoot.ts`) and the global exposure object at the bottom of the file +both need whatever Stage 2 pushes with. **Do not export `push(entry)` across frames** — that would +hand page-frame code the ability to put a page-frame closure on the stack, which is exactly the +failure `undoTypes.ts` documents at length. Export a function taking *data* and let the workspace +frame build the entry. Design it with Stage 2's first real caller, not before. ### Proof it worked -- `pnpm test` green; `bookEdit/undo` specs green (31 tests). -- **The point of Stage 1 is that nothing changes**, so the verification is behavioural, in a - running Bloom, comparing against the same gestures before the edits: - - In Change Layout mode: make a layout change, Ctrl+Z undoes it; Ctrl+Y redoes it. - - With the Decodable Reader tool open: type, then Undo — the reader-tools undo runs, not - CKEditor's (this is the deliberate precedence that would be easiest to lose). - - On an image: change its copyright, then Undo. - - In a text box with no tool active: type, then Undo — CKEditor's undo runs. - - The Undo button's enabled state tracks all four, since C# polls `canUndo()` on a timer. +- [x] `pnpm test` green; `bookEdit/undo` specs green (52 tests, 21 of them added with the edits). +- [x] **The point of Stage 1 is that nothing changes**, so the verification is behavioural, in a + running Bloom. Done 2026-09-07 with the harnesses in `liveChecks/` (see its README); results in + PROGRESS.md under that date: + - [x] Change Layout mode: a split, Ctrl+Z undoes it, Ctrl+Y redoes it, each exactly once (origami's + own handler; ours declined). The Undo button reaches `origamiUndo` through the stack. + - [x] Decodable Reader tool active: type, Undo button — the reader-tools undo runs (`tb=1`), not + CKEditor's (`ck=0`), and the markup update follows (`markup=1`). + - [x] Image: an undoable copyright change, Undo button — `imageOperationUndo` runs (`img=1`). + - [x] Text box with no reader tool: type, Undo button — CKEditor's undo runs (`ck=1`); Ctrl+Y runs + CKEditor's redo exactly once and ours declines. + - [x] The Undo button's enabled state tracked `canUndo()` in every case. diff --git a/docs/retire-ckeditor/PLAN.md b/docs/retire-ckeditor/PLAN.md index 52008ce46e15..87d160449efb 100644 --- a/docs/retire-ckeditor/PLAN.md +++ b/docs/retire-ckeditor/PLAN.md @@ -163,10 +163,16 @@ nobody enumerated). **Use snapshots as the default entry type, with inverse-op e snapshot is too blunt.** The critical constraint, which shapes the contract: the page iframe's JS context dies not only -on page *change* but on same-page **reloads** — ctrl+wheel zoom regenerates the page -(`bloomEditing.ts:1268`), origami exit posts `saveChangesAndRethinkPageEvent` -(`origami.ts:193`), and several tools navigate. An entry that closes over page-frame DOM or -functions therefore becomes a live grenade: `undo()` would mutate a detached document or throw. +on page *change* but on same-page **reloads** — origami exit posts `saveChangesAndRethinkPageEvent` +(`origami.ts:193`), importing a video and changing the topic rebuild the page under its own id, and +several tools navigate. (An earlier draft also cited ctrl+wheel zoom; **that is stale** — zoom is a +CSS transform now, `EditingView.SetZoom` → `workspaceBundle.setZoom`, and reloads nothing. Corrected +2026-09-07.) An entry that closes over page-frame DOM or functions therefore becomes a live grenade: +`undo()` would mutate a detached document or throw. + +**Every one of those reloads goes through `workspaceRoot.switchContentPage`** — it is the only route +C# uses to navigate the page frame (`EditingView.cs`, three call sites). So one hook there covers +same-page reloads and page changes alike; see `bookEdit/undo/pageFrameUndoHooks.ts`. So **snapshot entries must be pure data**, interpreted at undo time by a restore function that re-acquires the current page frame via `getEditablePageBundleExports()`: @@ -754,13 +760,19 @@ And the risk is concentrated — four paths are 74% of it: | 1 each | `editableDivUtils.ts`, `canvasElementManager/CanvasElementManager.ts` | | **0** | `workspaceRoot.ts`, `origami.ts`, `ImageUndoManager.ts`, `editablePage.ts` | +> **Correction (2026-09-07):** the zero row was measured with the wrong path for `workspaceRoot.ts` +> (it is `bookEdit/workspaceRoot.ts`, not `bookEdit/js/`). Re-measured over the following month +> (2026-08-06 → 09-07): `workspaceRoot.ts` **5** commits — BL-16558 changed `handleUndo` itself — +> `editablePage.ts` **3**, `origami.ts` and `ImageUndoManager.ts` genuinely 0. So Stage 1's +> integration risk was low, not zero, and the BL-16558 change had to be folded into the legacy +> providers. **When measuring drift, get the paths from `git ls-tree`, not from memory.** + Three things follow directly: - **1.7 commits a day is a weekly sync, not a daily one.** A month between syncs would mean ~50 commits to reconcile at once, which is what made the one Stage 0 rebase painful. -- **Stage 1's integration risk is near zero** — every file its deferred edits touch is in the - zero-commit row. Stages 3 and 6 are where the cost lands, because that is where - `bloomEditing.ts` and `toolbox.ts` are. +- **Stage 1's integration risk is low** (not zero — see the correction above). Stages 3 and 6 are + where the cost lands, because that is where `bloomEditing.ts` and `toolbox.ts` are. - **`lib/ckeditor/` is still being actively patched** — 4 commits in 30 days, to the library we are deleting. Each is a behaviour somebody needed. Stage 5 must diff that directory against the project's start point and account for every change, rather than deleting a directory assumed diff --git a/docs/retire-ckeditor/PROGRESS.md b/docs/retire-ckeditor/PROGRESS.md index e4f9a7ba505b..d73561397014 100644 --- a/docs/retire-ckeditor/PROGRESS.md +++ b/docs/retire-ckeditor/PROGRESS.md @@ -33,17 +33,28 @@ answer is `vp`, never Volta, and the intermediate states are all misleading. > [PLAN.md](PLAN.md) is rewritten around a long-lived integration branch. Read §5 before doing any > branch work; the short version is the table below. +> ## ⚠ `Version6.5` has been cut (2026-09-04) — the merge window may be open +> +> The constraint below was "nothing merges to `master` until a `Version6.5` branch is cut". That +> branch now exists (`origin/Version6.5`, first commit 2026-09-04; master is 160 commits past it). +> Master's `AGENTS.md` carries a temporary header saying ordinary new work should target +> `Version6.5`, not `master`, during the transition — which is about 6.5 fixes. This project is 6.6 +> work, so `master` is presumably now its correct target, and the integration branch could open its +> PR. **Nothing has been merged or retargeted; that is John's call**, and it changes §5's economics +> (stage PRs could go straight to master again). Raised in the 2026-09-07 entry. + Stage 0's PR is reviewed-ready and awaiting a human; its card is in *Ready For Code Review*, the QA -test-ideas comment is posted, and Devin is clean against HEAD `6bd49463`. Stage 1's new code is -written and inert. +test-ideas comment is posted, and Devin is clean against HEAD `6bd49463`. **Stage 1 is live and +verified in a running Bloom** (2026-09-07): the Undo button and Ctrl+Y go through the one stack, and +four live checks show each legacy mechanism is reached exactly as before. It has no PR yet. **Branch topology** — one integration branch tracks `master`; each stage is a short-lived branch off it, PR'd into it and **squash-merged**, so integration carries one commit per stage: | Branch | What | State | | --- | --- | --- | -| **`BL-6681-ckeditor`** | The project's trunk. The only branch that merges `master` in. Eventually one PR into `master`. | Pushed. Created 2026-08-06 at Stage 0's HEAD; synced to master `9b6ba1cd9` | -| **`BL-6681-stage1-undostack`** | ← **the working tip.** `bookEdit/undo/` — the one undo stack, new files only, nothing imports it | Pushed, green, inert. No PR yet; when there is one it targets `BL-6681-ckeditor`, not master | +| **`BL-6681-ckeditor`** | The project's trunk. The only branch that merges `master` in. Eventually one PR into `master`. | Pushed. Synced to master `f0d9f1472` (2026-09-07) | +| **`BL-6681-stage1-undostack`** | ← **the working tip.** `bookEdit/undo/` — the one undo stack, **active**: `handleUndo`/`canUndo` delegate to it, Ctrl+Y bound in the page frame | Pushed (rebased onto integration 2026-09-07 — allowed: unreviewed, no PR). Green: 52 undo tests, full suite, typecheck. Live-verified. No PR yet; when there is one it targets `BL-6681-ckeditor`, not master | | `BL-6681-stage0-inventory` | PR [#8153](https://github.com/BloomBooks/BloomDesktop/pull/8153) — docs, characterization tests, the `toolbox.ts` seam | Pushed; ready for review, awaiting a human. Left targeting `master` on purpose (§5.6). **Don't push more to it** — it would restart the review | **Master-sync log** (§5.3 — record every sync here so the next drift check has a start point): @@ -51,6 +62,7 @@ it, PR'd into it and **squash-merged**, so integration carries one commit per st | Date | Merged `master` at | Watchlist commits in that range | | --- | --- | --- | | 2026-08-06 | `9b6ba1cd9` | **0** of 51 — clean merge, nothing of ours touched | +| 2026-09-07 | `f0d9f1472` | **11** of 433 — one conflict, `toolbox.ts` (BL-16717 made bookmarks conditional inside the extracted seam); resolved by teaching the seam. Nightly run [34134257000](https://github.com/BloomBooks/BloomDesktop/actions/runs/34134257000) triggered | All of PLAN.md §10 is decided except the Stage-5 legacy-cleanup lifetime, which blocks nothing. @@ -76,6 +88,8 @@ Stage 0 checklist (PLAN.md §6): - [x] **G1 verified, both halves.** Automated: `verifyCaretPreservation.mjs` PASS (caret at the right offset, bookmarks consumed, no ZWSP). Manual, by John: decodable reader open, "real typing seems fine" — the case automation couldn't reach, and the check `toolbox.ts` itself prescribes. + **2026-09-07: the automated harness now also passes with the Decodable Reader tool active** + (markup running), on the post-BL-16717 code. - [ ] **G2** (async markup path / BL-10133 — where the prep commit made its one deliberate behaviour change) and **G3** (longpress) still unverified - [ ] **G6/G7** (new, from BL-16558): reader and Talking Book highlights are live Ranges and must @@ -821,10 +835,137 @@ has to reconcile) or close it as superseded by the integration PR, which contain the whole period. Stage 0's commits are the integration branch's base, so the edits can land on the Stage 1 branch now, and DEFERRED-EDITS.md's trigger is updated to say so. +### 2026-09-07 — master sync, Stage 1 activated and live-verified, BL-13502 assessed + +Autonomous session (John mostly unavailable). Everything below is pushed. + +**Master sync (§5.3).** Merged `origin/master` `f0d9f1472` into `BL-6681-ckeditor`: 433 commits, 11 +on the watchlist, one conflict — `toolbox.ts`, in exactly the region the Stage 0 prep commit +extracted. Master's **BL-16717** (ligature glyphs vanishing) made the CKEditor bookmark *conditional*: +it is only taken when a tool is active or the box has a comment/nbsp to clean up, because the bookmark +span splits the text node. Rather than let that logic grow back inline, the seam learned it: +`saveSelectionForMarkup(editableDiv, boxMightBeRewritten)` records nothing when nothing can move the +caret, and `restoreSelectionAfterMarkup` no-ops on such a record. The decision is made *before* the +record, as master did, because the bookmark span itself contains an nbsp. Master's +`mergeAdjacentTextNodes` sweep stays in the pipeline after the restore — it is about backspace and +long-press splits too, so it must survive the anchor swap. Full suite 794 green; nightly triggered. + +Two things to know about that merge: + +- **The pre-commit hook reformatted three of master's own files** that were staged as part of the + merge (`crowdin.yml`, `aiImageEditorOverlay.test.ts`, `SIL-Niger/branding.less`) — master's copies + don't satisfy this repo's prettier. Restoring master's bytes needs a `--no-verify` commit, which I + did not do without asking. Harmless noise; **John: say the word and it's one commit.** +- **§5.1's drift table was wrong for `workspaceRoot.ts`** — measured with the wrong path. It had 5 + commits, one of them BL-16558 changing `handleUndo` itself (see below). Corrected in PLAN.md. + +**`Version6.5` exists.** See the box at the top. Not acted on. + +**Stage 1 activated** (`f92383031`): DEFERRED-EDITS 1a–1e applied, three ways different from how they +were written: + +1. **BL-16558** (master, 2026-08) had made `handleUndo` call `updateMarkupAfterUndoOrRedo()` after the + reader-tools and CKEditor undos, because both rewrite an editable's innerHTML and so detach the + `::highlight()` Ranges painted over it. The toolbox and ckeditor providers now do the same; + `legacyUndoProvidersSpec.ts` pins it, and the order of the four. +2. **Page identity is `.bloom-page`'s `id`, not `data-page-id`.** Nothing in Bloom sets + `data-page-id` — only `ImageUndoManagerSpec` does — so the check in + `ImageUndoManager.clearImageOperationUndoOnPageChange` compares undefined with undefined and never + fires (harmless there: the manager dies with the page frame). The plan's "reuse what + ImageUndoManager does" would have reproduced a dead check. *Worth a small card of its own.* +3. **Ctrl+wheel zoom no longer reloads the page** — `EditingView.SetZoom` → `workspaceBundle.setZoom`, + a CSS transform. The plan cited zoom as the canonical same-page reload in §4.1, §4.11 and 1d; all + corrected. The real same-page reloads (leaving Change Layout mode, importing a video, changing the + topic) **all go through `workspaceRoot.switchContentPage`** — the only route C# uses to navigate + the page frame (`EditingView.cs`, three sites) — so 1d's question ("does `pageUnloading` fire on a + same-page reload?") is moot: `pageFrameNavigating()` clears page-scoped entries in + `switchContentPage` before the frame is touched, and `pageFrameLoaded()` records the id on load. + +Redo is bound in the **page** frame (`undo/redoKeyBinding.ts`, one call from `editablePage.ts`), at +the document, bubble phase, acting only when nothing earlier claimed Ctrl+Y *and* the stack has +something to redo — so origami's and the reader tools' handlers and CKEditor's own redo keep winning +until converted. The workspace bundle grew `canRedo()`/`handleRedo()`. + +**Live verification — four harnesses, kept in `docs/retire-ckeditor/liveChecks/`** (see its README). +Each wraps the cross-frame entry points and CKEditor's `afterCommandExec`, so a gesture is attributed +by counters rather than by "the text changed back". Results against Bloom launched from this worktree +(English Books collection): + +| Check | Result | +| --- | --- | +| Decodable Reader tool active: Undo button → reader-tools undo only (`tb=1 ck=0 markup=1`, no CKEditor command); our Ctrl+Y binding never fires | **PASS** | +| Basic Book, no reader tool: Undo button → CKEditor undo only (`ck=1 tb=0 markup=1`); Ctrl+Z/Ctrl+Y run CKEditor's commands exactly once; our binding declines; round trip restores the text | **PASS** (7/7) | +| Change Layout mode: Undo button → `origamiUndo` (`ori=1`); origami's Ctrl+Z/Ctrl+Y fire once; ours declines | **PASS** (6/6) | +| Undoable copyright change on an image (`changeImageByElement`, the dialog's entry point): Undo button → `imageOperationUndo` (`img=1`), copyright restored | **PASS** (4/4) | +| Undo button enabled state tracks `canUndo()` in all four | **PASS** (observed each time) | + +**Two pre-existing bugs the harness exposed** — both in the reader-tools mechanism, both caused by +the very things this project removes, neither introduced here (the provider calls exactly what the +old `handleUndo` called). Recorded as expected failures A4/A6/A7 in `verifyReader.mjs`: + +- **The reader-tools undo restores a snapshot containing a stale CKEditor bookmark span.** + `readerToolsModel.doMarkup` snapshots `innerHTML` while the `cke_bm_*` span is in the DOM, so + undoing restores it: after one undo the box read `"…on sun\u00a0"` with a + `` inside, and they accumulate (a page had + two after two runs). The snapshot's `text` also carries the nbsp, so the "is this the current state" + comparison in `undo()` fails and it steps back one fewer level than intended. This is the + "mid-word bookmark bug" of the inventory's ✗ rows made concrete, and it is content corruption, not + just wrong analysis. +- **Ctrl+Z with a reader tool active runs TWO undos.** The reader tools' per-editable handler + `return false`s, which stops *propagation* — but CKEditor's keystroke handler is on the same + element, so it fires regardless (`ckCmds: ["undo"]` observed alongside the reader undo). Ctrl+Y + likewise runs both redos, and the round trip does not restore the typed text (`" pot"` was lost). + §3's "the reader tools claim Ctrl+Z" is therefore only half true: they act, but so does CKEditor. + *Should be reproduced on master and filed; it is user-visible today.* + +**BL-13502 (`origin/BL-13502-save-without-reload`, PR #8209, draft, 22 commits, 20 behind master) +assessed** — likely to merge before us, and it matters to us more than expected: + +- **It removes the in-flight-save hazard entirely.** `SavePending`, `SavedAndStripped`, + `RequestBrowserToSave`, `editView/pageContent` and `DiscardInFlightSave` are gone; the browser + *volunteers* the page (`pageSnapshot.ts`, a body `MutationObserver` + 25 ms debounce) and C# saves + synchronously from the last snapshot. So §4.11's "sharp edge" and risk 5 evaporate — and a Tier 1 + innerHTML restore needs *no* save integration at all: the observer sees it and posts within ~50 ms. +- **Name clash.** Their `pageSnapshot.ts` / `PageSnapshot.cs` mean "the last content the browser + posted for saving". Our Stage 3 `PageSnapshot` (an undo entry kind) must be renamed — `undoSnapshot` + or similar — before it is written. +- **A new CKEditor dependency to inventory.** The save path now clones the body and copies CKEditor's + cleaned data into the clone (`EditableDivUtils.copyCkEditorDataToClone`) instead of writing it back + over the live editors. REVIEW-NOTES' "restored divs silently skip cleanup" concern changes shape but + does not go away; Stage 3/5 must give that function a no-CKEditor path. Add to BEHAVIOR-INVENTORY + once it merges. +- **Stage 2a's citations will be wrong.** `SaveThen` is now `MergeCurrentPageThenSave`, delete-page + receives its content from the page list and the "capture inside the SaveThen callback" reasoning + changes. **Do Stage 2a after BL-13502 merges**, re-deriving from the new code. +- **It answers the Stage 0 timing item.** Their `SavingWithoutReloading.md` measured a page change at + ~790 ms, ~80% of it building the new page; gather is 0.4–0.7 ms; and "one keystroke produces ~9 + MutationObserver batches because CKEditor does a lot of DOM work per key". Adopt those numbers as + the baseline (with their `benchPageChange.mjs`) rather than re-measuring; the ~9 batches per key is + a ready-made before/after metric for Stage 6. +- Also touches `editablePage.ts` (the ready handler where our one-liner went), `toolbox.ts`, + `bloomEditing.ts`, `origami.ts` and `decodableReaderTool.tsx` — expect a small conflict at the + next sync after it lands; nothing structural. + +**Deliberately left for later:** the paste/drop baseline (needs synthetic `ClipboardEvent`s with real +web-page payloads; feasible over CDP, not started), and the handler-accumulation repro (now easy: +`editablePageBundle.SetupElements` is exported cross-frame, so calling it twice on the page and +counting `document` keydown listeners via `DOMDebugger.getEventListeners` is the whole repro). + ## Next actions -**Work is on hold as of 2026-08-06** at John's request. Everything below is pushed; nothing is -half-applied, and both branches are green with a clean working tree. +Everything below is pushed; nothing is half-applied, and both branches are green with a clean +working tree. Bloom can be launched from this worktree with the `run-bloom` skill; the live checks in +`docs/retire-ckeditor/liveChecks/` drive it. + +### Decisions John needs to make + +- **The merge window.** `Version6.5` exists. Does the project now target `master` (6.6)? If so, §5 + could go back to "stage PRs straight to master" — cheaper than the integration branch — and the + integration branch's first PR could open now. Nothing done pending the answer. +- **Restore the three hook-reformatted master files** in the sync merge (needs one `--no-verify` + commit)? Or leave the noise. +- **File the two reader-tools bugs** found above (stale bookmark span in undo snapshots; Ctrl+Z runs + two undos), after a master repro? And the dead `data-page-id` check in `ImageUndoManager`? ### Stage 0's remainder — four items, all needing a running Bloom @@ -832,34 +973,38 @@ Do these in one session (`run-bloom` skill). **Not on `BL-6681-stage0-inventory` under human review, and pushing to it would restart the review for work that is purely additive. Branch off **`BL-6681-ckeditor`** instead (the files below are new; nothing conflicts). -1. **Finish rows G1–G3.** The seam's wiring is verified; the DOM-rewriting case is not (see the - 2026-08-05 entry). Create a book from the **Decodable Reader** template, open the toolbox, - activate the reader tool, and re-run `node docs/retire-ckeditor/verifyCaretPreservation.mjs - ` — it already reports the span counts that show whether markup ran. Then G2 (async - path / BL-10133, where the prep commit made its one deliberate behaviour change) and G3 - (longpress). Remember: a disconnected Team Collection blocks editing existing books, so make a - new one; and `toolboxIsShowing()` gates markup, so the pane must genuinely be open. +1. ~~Finish G1~~ (done 2026-09-07, harness passes with the reader tool active). **G2** (async + markup path / BL-10133 — Talking Book tool, where the prep commit made its one deliberate + behaviour change) is still unverified; type in a box with the Talking Book tool active and check + the caret. G3 is verified. 2. **Capture the paste/drop baseline** → `PASTE-DROP-BASELINE.md`, rows C1–C7. Use a **real web-page clipboard payload**, not hand-written tidy HTML. Do it before any further code change — this is the row-set whose failure is silent. Include **C7 (drop)**, the row CKEditor has been covering invisibly. -3. **Handler-accumulation repro** (§4.10): drive `refreshCanvasElementEditing` repeatedly and watch - for duplicate `document` keydown handlers via CDP `DOMDebugger.getEventListeners`; F6 is the - likeliest visible symptom. File its own card if it reproduces. Add the X4 listener-leak test either - way — it should fail before any fix. -4. **Page-reload timing baseline** (§4.11) with the performance-log feature. +3. **Handler-accumulation repro** (§4.10): `editablePageBundle.SetupElements(page)` is exported + cross-frame, so call it twice on the current page and count `document` keydown listeners via CDP + `DOMDebugger.getEventListeners`; F6 is the likeliest visible symptom. File its own card if it + reproduces. Add the X4 listener-leak test either way — it should fail before any fix. +4. ~~Page-reload timing baseline~~ — adopt BL-13502's measurements (see the 2026-09-07 entry) once it + merges; re-run its `benchPageChange.mjs` on our branch only if something looks off. ### Stage 1 — branch `BL-6681-stage1-undostack`, off `BL-6681-ckeditor` -The new files are written, tested (31 tests) and inert. What remains: +Active, tested (52 tests) and live-verified. What remains: + +5. ~~Apply DEFERRED-EDITS 1a–1e~~ — done 2026-09-07. 1f (the cross-frame push) waits for Stage 2's + first caller by design. +6. ~~Where `clearPageScopedEntries()` hangs off~~ — settled: `switchContentPage`, which every + page-frame navigation goes through. +7. **PR the branch into `BL-6681-ckeditor`** (or into `master`, if John opens the window — see the + decisions above) and run `preflight` on it. Then squash-merge and delete the branch (§5.2). + +### Stage 2 — after the Stage 1 PR -5. **Apply [DEFERRED-EDITS.md](DEFERRED-EDITS.md) §1a–1f** — no longer blocked on Stage 0 merging (see - the 2026-08-06 (later) entry), and they should not wait, or Stage 1 stays unverifiable for months. - Then run the five behavioural checks listed there in a running Bloom. Until they land, Stage 1's - code is unreachable and nothing about Undo has changed. -6. Decide where `clearPageScopedEntries()` hangs off (see 1d) — needs a running Bloom to confirm - `pageUnloading()` fires on a same-page reload. -7. Then PR the branch **into `BL-6681-ckeditor`**, not master, and squash-merge it (§5.2). +- **2b (undo delete canvas element) first**, not 2a: it is pure front-end, and 2a's C# citations are + about to be invalidated by BL-13502. Design 1f (the data-not-closure cross-frame push) with it. +- **2a (undo delete page) after BL-13502 merges**, re-derived from `MergeCurrentPageThenSave`. +- Rename our planned `PageSnapshot` entry kind before Stage 3 (BL-13502 owns that name). **Standing chores while the branch is long-lived** (§5.3, §5.5): diff --git a/docs/retire-ckeditor/liveChecks/README.md b/docs/retire-ckeditor/liveChecks/README.md new file mode 100644 index 000000000000..3d92498f8d48 --- /dev/null +++ b/docs/retire-ckeditor/liveChecks/README.md @@ -0,0 +1,45 @@ +# Live checks for the undo work (BL-6681) + +Scripts that drive a running Bloom over CDP and say which undo mechanism a gesture actually reached. +They exist because Stage 1's whole claim is "nothing changed", and jsdom cannot see the frames; each +later stage moves a mechanism onto the shared stack, and these are what shows it moved and nothing +else did. Kept in the repo, like `../verifyCaretPreservation.mjs`, so the next session does not +rebuild them. + +## Running + +```sh +node .github/skills/bloom-automation/launcherControl.mjs --ensure-running --wait-ready --json +node docs/retire-ckeditor/liveChecks/gotoBook.mjs "Decodable highlight" # any Decodable Reader book +node docs/retire-ckeditor/liveChecks/verifyReader.mjs +node docs/retire-ckeditor/liveChecks/gotoBook.mjs "A house for mouse" # any Basic Book with an image +node docs/retire-ckeditor/liveChecks/verifyCk.mjs +node docs/retire-ckeditor/liveChecks/verifyOrigami.mjs +node docs/retire-ckeditor/liveChecks/verifyImage.mjs +``` + +Ports come from `output/bloom-launcher.json`; set `BLOOM_CDP_PORT` / `BLOOM_HTTP_PORT` if Bloom was +started another way. Each script exits non-zero if any check failed. They type into and restore a +text box, so use a test book, and the collection must not be a disconnected Team Collection (its +books cannot be checked out, so Edit is disabled). + +## What each proves + +| Script | Book | Proves | +| --- | --- | --- | +| `verifyReader.mjs` | Decodable/Leveled Reader, tool active | Undo button → reader-tools undo only, with the BL-16558 markup update; our Ctrl+Y never fires while the reader tool claims it | +| `verifyCk.mjs` | Basic Book | Undo button → CKEditor undo only; Ctrl+Z/Ctrl+Y run CKEditor's commands exactly once; our Ctrl+Y declines | +| `verifyOrigami.mjs` | Basic Book (customPage) | Undo button → origamiUndo; origami's own Ctrl+Z/Ctrl+Y fire once; ours declines | +| `verifyImage.mjs` | any page with an image | Undo button → imageOperationUndo after an undoable copyright change | + +How attribution works: `verifyCommon.mjs` wraps the cross-frame functions the workspace bundle +reaches each mechanism through, and listens to CKEditor's `afterCommandExec`, so a gesture is +attributed by counters, not by "the text changed back". + +## Expected failures, on purpose + +`verifyReader.mjs` A4, A6 and A7 fail today and record two pre-existing problems (PROGRESS.md, +2026-09-07): the reader-tools undo restores a snapshot that still contains a CKEditor bookmark span, +and Ctrl+Z with a reader tool active runs *both* the reader-tools undo and CKEditor's, which breaks +the Ctrl+Y round trip. They should start passing when Stage 3 replaces both mechanisms; until then a +run of `verifyReader.mjs` is green when only those three fail. diff --git a/docs/retire-ckeditor/liveChecks/cdp.mjs b/docs/retire-ckeditor/liveChecks/cdp.mjs new file mode 100644 index 000000000000..5b47b62b182d --- /dev/null +++ b/docs/retire-ckeditor/liveChecks/cdp.mjs @@ -0,0 +1,78 @@ +// CDP plumbing shared by the live checks in this directory (BL-6681). +// +// Connects Playwright to the running Bloom's WebView2. The ports come from the dev launcher's +// status API (`launcherControl.mjs --status --json`), or from BLOOM_CDP_PORT / BLOOM_HTTP_PORT if +// you started Bloom some other way. +import { createRequire } from "node:module"; +import { execFileSync } from "node:child_process"; +import path from "node:path"; +import url from "node:url"; + +export const repoRoot = path.resolve( + path.dirname(url.fileURLToPath(import.meta.url)), + "../../..", +); +const ctDir = path.join( + repoRoot, + "src/BloomBrowserUI/react_components/component-tester", +); +const req = createRequire(path.join(ctDir, "package.json")); +const { chromium } = req("playwright"); + +/** + * The running Bloom's ports. Asked of the dev launcher (`launcherControl.mjs --status --json`, which + * reads output/bloom-launcher.json and queries the launcher's control API) unless overridden. + */ +export function ports() { + let fromLauncher = {}; + if (!process.env.BLOOM_CDP_PORT || !process.env.BLOOM_HTTP_PORT) { + try { + const out = execFileSync( + "node", + [ + path.join( + repoRoot, + ".github/skills/bloom-automation/launcherControl.mjs", + ), + "--status", + "--json", + ], + { encoding: "utf8", stdio: ["ignore", "pipe", "ignore"] }, + ); + const status = JSON.parse(out).status || {}; + fromLauncher = { + cdpPort: status.cdpPort, + httpPort: status.httpPort, + }; + } catch { + // no launcher; rely on the environment + } + } + return { + cdpPort: process.env.BLOOM_CDP_PORT || fromLauncher.cdpPort, + httpPort: process.env.BLOOM_HTTP_PORT || fromLauncher.httpPort, + }; +} + +export async function connect() { + const { cdpPort } = ports(); + if (!cdpPort) + throw new Error( + "no CDP port: start Bloom with launcherControl.mjs or set BLOOM_CDP_PORT", + ); + const browser = await chromium.connectOverCDP( + `http://localhost:${cdpPort}`, + ); + const pages = browser.contexts().flatMap((c) => c.pages()); + return { browser, pages }; +} + +/** Bloom's main (workspace) page, as opposed to the toolbox content page. */ +export function mainPage(pages) { + const p = pages.find( + (p) => + p.url().includes("/bloom/") && !p.url().includes("toolboxcontent"), + ); + if (!p) throw new Error("no Bloom main page over CDP"); + return p; +} diff --git a/docs/retire-ckeditor/liveChecks/gotoBook.mjs b/docs/retire-ckeditor/liveChecks/gotoBook.mjs new file mode 100644 index 000000000000..6b89ffe044ea --- /dev/null +++ b/docs/retire-ckeditor/liveChecks/gotoBook.mjs @@ -0,0 +1,36 @@ +// Open a book by title in the Edit tab of the running Bloom: switch to the Collections tab, click +// the book, switch to Edit. Usage: node gotoBook.mjs "A house for mouse" +import { execFileSync } from "node:child_process"; +import path from "node:path"; +import { connect, mainPage, ports, repoRoot } from "./cdp.mjs"; +const title = process.argv[2]; +if (!title) throw new Error("usage: node gotoBook.mjs "); +const { httpPort } = ports(); +const sw = path.join( + repoRoot, + ".github/skills/bloom-automation/switchWorkspaceTab.mjs", +); +const tab = (name) => + execFileSync( + "node", + [sw, "--http-port", String(httpPort), "--tab", name, "--json"], + { stdio: "ignore" }, + ); +tab("collection"); +await new Promise((r) => setTimeout(r, 2500)); +{ + const { browser, pages } = await connect(); + const p = mainPage(pages); + const loc = p.getByText(title, { exact: true }); + const count = await loc.count(); + if (count === 0) + throw new Error( + `no book titled '${title}' visible in the collection tab`, + ); + await loc.first().click(); + await p.waitForTimeout(1500); + await browser.close(); +} +tab("edit"); +await new Promise((r) => setTimeout(r, 4000)); +console.log("opened", title, "in Edit"); diff --git a/docs/retire-ckeditor/liveChecks/verifyCk.mjs b/docs/retire-ckeditor/liveChecks/verifyCk.mjs new file mode 100644 index 000000000000..34877b86e9bd --- /dev/null +++ b/docs/retire-ckeditor/liveChecks/verifyCk.mjs @@ -0,0 +1,102 @@ +// Live check B (BL-6681 Stage 1): with no reader tool, the Undo button reaches CKEditor's undo; +// Ctrl+Y redoes exactly once (CKEditor's) and our binding declines. +// +// Needs Bloom on the Edit tab of a book whose toolbox has no reader tool (a Basic Book). Restores +// the text it typed into. +import { setup } from "./verifyCommon.mjs"; +const h = await setup(); +await h.gotoContentPage(); +await h.instrument(); +console.log("active tool:", await h.activeTool()); +const originalHtml = await h.readHtml(); +const before = await h.readText(); +const norm = (s) => s.replace(/​/g, "").replace(/ /g, " "); +console.log( + "button disabled before typing?", + await h.undoButtonDisabled(), + "canUndo:", + await h.canUndo(), +); + +// --- button path --- +await h.clickEnd(); +await h.p.keyboard.type(" dog"); +await h.p.waitForTimeout(1500); +const typed = await h.readText(); +h.record( + "B1 typing landed", + norm(typed) === norm(before) + " dog", + `before='${before}' after=${JSON.stringify(typed)}`, +); +h.record( + "B2 canUndo=yes after typing", + (await h.canUndo()) === "yes", + `button disabled=${await h.undoButtonDisabled()}`, +); +await h.resetCalls(); +await h.calls(); +await h.pressUndoButton(); +const c = await h.calls(); +h.record( + "B3 Undo button -> CKEditor undo only (ck=1, tb=0, markup=1)", + c.ck === 1 && c.tb === 0 && c.markup === 1, + JSON.stringify(c), +); +const afterUndo = await h.readText(); +h.record( + "B4 CKEditor undo changed the text back", + afterUndo !== typed, + `text now ${JSON.stringify(afterUndo)}`, +); +console.log( + "canUndo after undo:", + await h.canUndo(), + "button disabled:", + await h.undoButtonDisabled(), +); + +// --- keyboard path --- +await h.writeHtml(originalHtml); +await h.p.waitForTimeout(300); +await h.clickEnd(); +await h.p.keyboard.type(" cat"); +await h.p.waitForTimeout(1500); +const typed2 = await h.readText(); +await h.resetCalls(); +await h.calls(); +await h.p.keyboard.press("Control+z"); +await h.p.waitForTimeout(700); +const afterZ = await h.readText(); +const cz = await h.calls(); +await h.p.keyboard.press("Control+y"); +await h.p.waitForTimeout(700); +const afterY = await h.readText(); +const cy = await h.calls(); +console.log( + `Ctrl+Z: text=${JSON.stringify(afterZ)} calls=${JSON.stringify(cz)}`, +); +console.log( + `Ctrl+Y: text=${JSON.stringify(afterY)} calls=${JSON.stringify(cy)}`, +); +h.record( + "B5 Ctrl+Z ran CKEditor's undo (its command), not ours", + cz.ckCmds.includes("undo") && cz.ck === 0, + JSON.stringify(cz), +); +h.record( + "B6 Ctrl+Y ran CKEditor's redo exactly once and our binding declined", + cy.ckCmds.filter((n) => n === "redo").length === 1 && cy.redo === 0, + JSON.stringify(cy), +); +h.record( + "B7 Ctrl+Z/Ctrl+Y round trip restores the typed text", + norm(afterY) === norm(typed2), + `typed=${JSON.stringify(typed2)} y=${JSON.stringify(afterY)}`, +); + +// --- restore --- +await h.writeHtml(originalHtml); +await h.p.waitForTimeout(500); +console.log("restored text:", JSON.stringify(await h.readText())); +h.summary(); +await h.browser.close(); diff --git a/docs/retire-ckeditor/liveChecks/verifyCommon.mjs b/docs/retire-ckeditor/liveChecks/verifyCommon.mjs new file mode 100644 index 000000000000..5c9ddf032e51 --- /dev/null +++ b/docs/retire-ckeditor/liveChecks/verifyCommon.mjs @@ -0,0 +1,177 @@ +// Shared pieces of the undo live checks (BL-6681). See README.md in this directory. +// +// The one idea worth knowing: every check ATTRIBUTES each gesture to the mechanism(s) that actually +// ran, by wrapping the cross-frame entry points the workspace bundle reaches the mechanisms through +// (page bundle: ckeditorUndo / imageOperationUndo / origamiUndo; toolbox bundle: undo / +// updateMarkupAfterUndoOrRedo; workspace bundle: handleRedo) and by listening to CKEditor's own +// afterCommandExec. "The text changed back" is not evidence of WHICH undo ran; the counters are. +import { connect, mainPage } from "./cdp.mjs"; + +export const editableSel = + ".bloom-editable.bloom-visibility-code-on[contenteditable='true']"; + +export async function setup() { + const { browser, pages } = await connect(); + const p = mainPage(pages); + const results = []; + const record = (name, pass, detail) => { + results.push({ name, pass, detail }); + console.log(`${pass ? "PASS" : "FAIL"}: ${name} — ${detail}`); + }; + const h = { + browser, + p, + results, + record, + pf: undefined, + idx: -1, + pageFrame: () => p.frames().find((f) => f.name() === "page"), + toolboxFrame: () => p.frames().find((f) => f.name() === "toolbox"), + pageListFrame: () => p.frames().find((f) => f.name() === "pageList"), + }; + h.instrument = async () => { + await p.evaluate(() => { + const wb = window.workspaceBundle; + const px = wb.getEditablePageBundleExports(); + const tx = wb.getToolboxBundleExports(); + window.__calls = { + ck: 0, + tb: 0, + img: 0, + ori: 0, + markup: 0, + redo: 0, + }; + const wrap = (obj, name, key) => { + const o = obj[name]; + if (o.__wrapped) return; + const w = function (...a) { + window.__calls[key]++; + return o.apply(this, a); + }; + w.__wrapped = true; + obj[name] = w; + }; + wrap(px, "ckeditorUndo", "ck"); + wrap(px, "imageOperationUndo", "img"); + wrap(px, "origamiUndo", "ori"); + wrap(tx, "undo", "tb"); + wrap(tx, "updateMarkupAfterUndoOrRedo", "markup"); + wrap(wb, "handleRedo", "redo"); + }); + await h.pf.evaluate(() => { + window.__ckCmds = []; + if (typeof CKEDITOR === "undefined") return; + for (const k in CKEDITOR.instances) { + const ed = CKEDITOR.instances[k]; + if (ed.__instrumented) continue; + ed.__instrumented = true; + ed.on("afterCommandExec", (e) => + window.__ckCmds.push(e.data.name), + ); + } + }); + }; + h.calls = async () => { + const c = await p.evaluate(() => ({ ...window.__calls })); + c.ckCmds = await h.pf.evaluate(() => (window.__ckCmds || []).splice(0)); + return c; + }; + h.resetCalls = () => + p.evaluate(() => { + for (const k in window.__calls) window.__calls[k] = 0; + }); + h.canUndo = () => p.evaluate(() => window.workspaceBundle.canUndo()); + // The top bar's Undo button; C# enables it from canUndo() on a timer. + h.undoButtonDisabled = () => + p.evaluate(() => { + const b = Array.from(document.querySelectorAll("button")).find( + (e) => + /undo/i.test(e.innerHTML) || + /undo/i.test(e.getAttribute("aria-label") || ""), + ); + return b ? b.disabled : "not found"; + }); + h.readText = () => + h.pf.evaluate( + ({ s, idx }) => document.querySelectorAll(s)[idx].textContent, + { s: editableSel, idx: h.idx }, + ); + h.readHtml = () => + h.pf.evaluate( + ({ s, idx }) => document.querySelectorAll(s)[idx].innerHTML, + { s: editableSel, idx: h.idx }, + ); + h.writeHtml = (html) => + h.pf.evaluate( + ({ s, idx, html }) => { + document.querySelectorAll(s)[idx].innerHTML = html; + }, + { s: editableSel, idx: h.idx, html }, + ); + h.clickEnd = async () => { + await h.pf.locator(editableSel).nth(h.idx).click(); + await p.waitForTimeout(300); + await p.keyboard.press("End"); + }; + h.activeTool = () => + h + .toolboxFrame() + ?.evaluate(() => + document + .querySelector("h3.ui-accordion-header-active") + ?.getAttribute("data-toolid"), + ); + // What the top bar's Undo button does (topBarButtonClick -> handleUndo), minus the WinForms click. + h.pressUndoButton = async () => { + await p.evaluate(() => window.workspaceBundle.handleUndo()); + await p.waitForTimeout(800); + }; + // Find a content page (not xmatter) with a CKEditor'd content editable, starting at thumb 1. + h.gotoContentPage = async () => { + const thumbs = h.pageListFrame().locator(".gridItem"); + const n = await thumbs.count(); + for (let t = 1; t < n; t++) { + await thumbs.nth(t).click(); + await p.waitForTimeout(3000); + h.pf = h.pageFrame(); + const info = await h.pf.evaluate((s) => { + const page = document.querySelector(".bloom-page"); + return { + xmatter: /bloom-frontMatter|bloom-backMatter/.test( + page?.className || "", + ), + eds: Array.from(document.querySelectorAll(s)).map((e) => ({ + ck: !!e.bloomCkEditor, + c1: e.classList.contains("bloom-content1"), + text: e.textContent.trim().slice(0, 30), + })), + }; + }, editableSel); + if (!info.xmatter) { + h.idx = info.eds.findIndex((e) => e.ck && e.c1); + if (h.idx >= 0) { + console.log( + `using thumb ${t}, editable ${h.idx}:`, + JSON.stringify(info.eds[h.idx]), + ); + return; + } + } + } + throw new Error("no content page with a CKEditor'd content editable"); + }; + h.summary = () => { + const failed = results.filter((r) => !r.pass); + console.log( + "\nSUMMARY:", + results.length - failed.length, + "passed,", + failed.length, + "failed", + ); + for (const r of failed) console.log(" FAILED:", r.name, "—", r.detail); + process.exitCode = failed.length ? 1 : 0; + }; + return h; +} diff --git a/docs/retire-ckeditor/liveChecks/verifyImage.mjs b/docs/retire-ckeditor/liveChecks/verifyImage.mjs new file mode 100644 index 000000000000..115e363a3452 --- /dev/null +++ b/docs/retire-ckeditor/liveChecks/verifyImage.mjs @@ -0,0 +1,88 @@ +// Live check D (BL-6681 Stage 1): an undoable image operation -- changeImageByElement with +// undoable="true", the same entry point the copyright dialog's completion uses -- is undone by the +// Undo button through the stack's image provider. +// +// Needs Bloom on the Edit tab; finds the first page with an image. Restores the copyright. +import { setup } from "./verifyCommon.mjs"; +const h = await setup(); +const thumbs = h.pageListFrame().locator(".gridItem"); +const n = await thumbs.count(); +let found = false; +for (let t = 1; t < n && !found; t++) { + await thumbs.nth(t).click(); + await h.p.waitForTimeout(3000); + h.pf = h.pageFrame(); + found = await h.pf.evaluate( + () => + !!document.querySelector( + ".bloom-canvas img, .bloom-imageContainer img", + ), + ); + console.log(`thumb ${t}: image? ${found}`); +} +if (!found) throw new Error("no page with an image"); +h.idx = 0; +await h.instrument(); +const sel = ".bloom-canvas img, .bloom-imageContainer img"; +const readCopyright = () => + h.pf.evaluate((s) => { + const img = document.querySelector(s); + const c = img.closest(".bloom-canvas, .bloom-imageContainer"); + return { + imgCopyright: img.getAttribute("data-copyright"), + containerCopyright: c.getAttribute("data-copyright"), + }; + }, sel); +const before = await readCopyright(); +console.log("before:", JSON.stringify(before), "canUndo:", await h.canUndo()); +// The image undo is gated on the active canvas element containing an image, so make it active +// first. The Comical canvas sits over the image and intercepts pointer events exactly as it does +// for a real user's click; a forced click lands at the image's position the same way. +await h.pf.locator(sel).first().click({ force: true }); +await h.p.waitForTimeout(700); +await h.pf.evaluate((s) => { + const img = document.querySelector(s); + const target = img.closest(".bloom-canvas") + ? img + : img.closest(".bloom-imageContainer"); + window.editablePageBundle.changeImageByElement(target, { + src: img.getAttribute("src"), + copyright: "Copyright © 2026, Stage 1 verification", + creator: "verifyImage.mjs", + license: "cc-by", + undoable: "true", + }); +}, sel); +await h.p.waitForTimeout(800); +const changed = await readCopyright(); +h.record( + "D1 the copyright change applied", + (changed.imgCopyright || changed.containerCopyright || "").includes( + "Stage 1 verification", + ), + JSON.stringify(changed), +); +h.record( + "D2 canUndo=yes via the image undo", + (await h.canUndo()) === "yes", + `button disabled=${await h.undoButtonDisabled()}`, +); +await h.resetCalls(); +await h.calls(); +await h.pressUndoButton(); +const c = await h.calls(); +const after = await readCopyright(); +h.record( + "D3 Undo button -> imageOperationUndo through the stack (img=1, others 0)", + c.img === 1 && c.ck === 0 && c.tb === 0 && c.ori === 0, + JSON.stringify(c), +); +h.record( + "D4 the copyright was restored", + after.imgCopyright === before.imgCopyright && + after.containerCopyright === before.containerCopyright, + `after=${JSON.stringify(after)}`, +); +console.log("canUndo after:", await h.canUndo()); +h.summary(); +await h.browser.close(); diff --git a/docs/retire-ckeditor/liveChecks/verifyOrigami.mjs b/docs/retire-ckeditor/liveChecks/verifyOrigami.mjs new file mode 100644 index 000000000000..6ff6f6a8682e --- /dev/null +++ b/docs/retire-ckeditor/liveChecks/verifyOrigami.mjs @@ -0,0 +1,114 @@ +// Live check C (BL-6681 Stage 1): Change Layout mode. Origami's own Ctrl+Z/Ctrl+Y keep working and +// fire once; the Undo button reaches origamiUndo through the stack. +// +// Needs Bloom on the Edit tab of a book whose content pages are customPages (a Basic Book). Leaves +// the layout as it found it; leaving layout mode saves and reloads the page. +import { setup } from "./verifyCommon.mjs"; +const h = await setup(); +await h.gotoContentPage(); +await h.instrument(); +const splitCount = () => + h.pf.evaluate( + () => document.querySelectorAll(".marginBox .split-pane").length, + ); +const inLayoutMode = () => + h.pf.evaluate( + () => !!document.querySelector(".marginBox.origami-layout-mode"), + ); +const toggleLabel = () => + h.pf.locator("label[for=changeLayoutToggle], .onoffswitch-label").first(); +console.log( + "toggle present:", + await h.pf.locator("#changeLayoutToggle").count(), + "layout mode:", + await inLayoutMode(), + "splits:", + await splitCount(), +); +if ((await h.pf.locator("#changeLayoutToggle").count()) === 0) + throw new Error("no Change Layout toggle on this page"); +if (!(await inLayoutMode())) { + await toggleLabel().click(); + await h.p.waitForTimeout(1200); +} +h.record( + "C1 entered Change Layout mode", + await inLayoutMode(), + `layout=${await inLayoutMode()}`, +); +const splits0 = await splitCount(); +console.log("canUndo in layout mode before any change:", await h.canUndo()); +// Make a layout change. The split buttons only show on hover, so click at the DOM level (origami +// binds them with jQuery click). +await h.pf.evaluate(() => + document + .querySelector( + ".split-pane-component-inner .button.add-bottom, .split-pane-component-inner .button.add-right", + ) + .click(), +); +await h.p.waitForTimeout(800); +const splits1 = await splitCount(); +h.record( + "C2 a split was made", + splits1 > splits0, + `splits ${splits0} -> ${splits1}`, +); +h.record( + "C3 canUndo=yes via origami", + (await h.canUndo()) === "yes", + `button disabled=${await h.undoButtonDisabled()}`, +); +// Keyboard: Ctrl+Z (origami's handler on html), then Ctrl+Y. Ours must not double it. +await h.pf.locator("html").click({ position: { x: 5, y: 5 }, force: true }); +await h.resetCalls(); +await h.calls(); +await h.p.keyboard.press("Control+z"); +await h.p.waitForTimeout(800); +const splitsZ = await splitCount(); +const cz = await h.calls(); +await h.p.keyboard.press("Control+y"); +await h.p.waitForTimeout(800); +const splitsY = await splitCount(); +const cy = await h.calls(); +console.log(`Ctrl+Z: splits=${splitsZ} calls=${JSON.stringify(cz)}`); +console.log(`Ctrl+Y: splits=${splitsY} calls=${JSON.stringify(cy)}`); +h.record( + "C4 Ctrl+Z undid the split once (origami), no CKEditor command", + splitsZ === splits0 && cz.ckCmds.length === 0, + `splits=${splitsZ}`, +); +h.record( + "C5 Ctrl+Y redid the split exactly once (origami); our binding declined", + splitsY === splits1 && cy.redo === 0, + `splits=${splitsY} redo=${cy.redo}`, +); +// Button path: Undo button -> stack -> origami provider. +await h.resetCalls(); +await h.calls(); +await h.pressUndoButton(); +const cb = await h.calls(); +const splitsB = await splitCount(); +h.record( + "C6 Undo button -> origamiUndo through the stack (ori=1, others 0)", + cb.ori === 1 && + cb.tb === 0 && + cb.ck === 0 && + cb.img === 0 && + splitsB === splits0, + `calls=${JSON.stringify(cb)} splits=${splitsB}`, +); +// Leave layout mode (this saves and reloads the page). +await toggleLabel().click(); +await h.p.waitForTimeout(3500); +h.pf = h.pageFrame(); +console.log( + "layout mode after leaving:", + await inLayoutMode(), + "splits:", + await splitCount(), + "canUndo:", + await h.canUndo(), +); +h.summary(); +await h.browser.close(); diff --git a/docs/retire-ckeditor/liveChecks/verifyReader.mjs b/docs/retire-ckeditor/liveChecks/verifyReader.mjs new file mode 100644 index 000000000000..58c58763ca3b --- /dev/null +++ b/docs/retire-ckeditor/liveChecks/verifyReader.mjs @@ -0,0 +1,100 @@ +// Live check A (BL-6681 Stage 1): with a reader tool active, the Undo button reaches the +// reader-tools undo, not CKEditor's, and the keyboard path is unchanged. +// +// Needs Bloom on the Edit tab of a Decodable (or Leveled) Reader book with that tool active. +// Restores the text it typed into, but does leave the page "edited" as far as Bloom is concerned. +// +// A4, A6 and A7 FAIL today, and are expected to: they observe two pre-existing problems, recorded +// in PROGRESS.md (2026-09-07), that this project exists to remove. Re-run after Stage 3 expecting +// them to pass. +import { setup } from "./verifyCommon.mjs"; +const h = await setup(); +await h.gotoContentPage(); +await h.instrument(); +console.log("active tool:", await h.activeTool()); +const originalHtml = await h.readHtml(); +const before = await h.readText(); +console.log( + "button disabled before typing?", + await h.undoButtonDisabled(), + "canUndo:", + await h.canUndo(), +); + +// --- button path --- +await h.clickEnd(); +await h.p.keyboard.type(" sun"); +await h.p.waitForTimeout(1500); // the 500 ms markup debounce, with margin +const typed = await h.readText(); +h.record( + "A1 typing landed", + typed === before + " sun", + `before='${before}' after='${typed}'`, +); +h.record( + "A2 canUndo=yes after typing", + (await h.canUndo()) === "yes", + `button disabled=${await h.undoButtonDisabled()}`, +); +await h.resetCalls(); +await h.calls(); +await h.pressUndoButton(); +const c = await h.calls(); +h.record( + "A3 Undo button -> reader-tools undo only (tb=1, ck=0, markup=1, no CKEditor command)", + c.tb === 1 && c.ck === 0 && c.markup === 1 && c.ckCmds.length === 0, + JSON.stringify(c), +); +const afterUndo = await h.readText(); +const afterUndoHtml = await h.readHtml(); +h.record( + "A4 (pre-existing) reader-tools undo reverted the text and left no bookmark span", + afterUndo === before && !/cke_bm_/.test(afterUndoHtml), + `text now ${JSON.stringify(afterUndo)}; bookmark spans in html: ${(afterUndoHtml.match(/cke_bm_/g) || []).length}`, +); + +// --- keyboard path: Ctrl+Z, Ctrl+Y in the box; which mechanisms run? --- +await h.writeHtml(originalHtml); +await h.clickEnd(); +await h.p.keyboard.type(" pot"); +await h.p.waitForTimeout(1500); +const typed2 = await h.readText(); +await h.resetCalls(); +await h.calls(); +await h.p.keyboard.press("Control+z"); +await h.p.waitForTimeout(700); +const afterZ = await h.readText(); +const cz = await h.calls(); +await h.p.keyboard.press("Control+y"); +await h.p.waitForTimeout(700); +const afterY = await h.readText(); +const cy = await h.calls(); +console.log( + `Ctrl+Z: text=${JSON.stringify(afterZ)} calls=${JSON.stringify(cz)}`, +); +console.log( + `Ctrl+Y: text=${JSON.stringify(afterY)} calls=${JSON.stringify(cy)}`, +); +h.record( + "A5 our Redo binding never fires while the reader tool claims Ctrl+Y", + cy.redo === 0, + `redo calls=${cy.redo}`, +); +h.record( + "A6 (pre-existing) CKEditor's own undo/redo commands did NOT also run on Ctrl+Z/Y", + cz.ckCmds.length === 0 && cy.ckCmds.length === 0, + `ckCmds z=${JSON.stringify(cz.ckCmds)} y=${JSON.stringify(cy.ckCmds)}`, +); +const strip = (s) => s.replace(/[​  ]+$/, ""); +h.record( + "A7 (pre-existing) Ctrl+Z/Ctrl+Y round trip restores the typed text", + strip(afterY) === strip(typed2), + `typed='${typed2}' y='${afterY}'`, +); + +// --- restore the book text --- +await h.writeHtml(originalHtml); +await h.p.waitForTimeout(500); +console.log("restored text:", JSON.stringify(await h.readText())); +h.summary(); +await h.browser.close(); From 4a413d0d5a2f199d0621774cece1b2ad775734da Mon Sep 17 00:00:00 2001 From: John Thomson Date: Mon, 7 Sep 2026 10:24:19 -0500 Subject: [PATCH 05/10] Reproduce the edit-handler accumulation and record the G2 attempt (BL-6681) liveChecks/handlerAccumulation.mjs shows the document-level and per-editable edit key handlers each fire once more per extra SetupElements run (1 -> 2 -> 3), confirming PLAN.md 4.10's code-reading finding; it doubles as the inventory's X4 test, failing until the signal-scoped teardown lands. activateTool.mjs switches tools through ToolBox.activateToolFromId, and the harnesses now ask the ToolBox which tool is current instead of trusting the accordion's header classes, which lag. G2 stays open: with the Talking Book tool current, typing produced no audio-sentence markup, so the async path was not exercised. --- docs/retire-ckeditor/PROGRESS.md | 51 ++++++++-- docs/retire-ckeditor/liveChecks/README.md | 5 + .../liveChecks/activateTool.mjs | 30 ++++++ .../liveChecks/handlerAccumulation.mjs | 95 +++++++++++++++++++ .../liveChecks/verifyCommon.mjs | 16 ++-- 5 files changed, 180 insertions(+), 17 deletions(-) create mode 100644 docs/retire-ckeditor/liveChecks/activateTool.mjs create mode 100644 docs/retire-ckeditor/liveChecks/handlerAccumulation.mjs diff --git a/docs/retire-ckeditor/PROGRESS.md b/docs/retire-ckeditor/PROGRESS.md index d73561397014..b65a6ad94631 100644 --- a/docs/retire-ckeditor/PROGRESS.md +++ b/docs/retire-ckeditor/PROGRESS.md @@ -946,10 +946,40 @@ assessed** — likely to merge before us, and it matters to us more than expecte `bloomEditing.ts`, `origami.ts` and `decodableReaderTool.tsx` — expect a small conflict at the next sync after it lands; nothing structural. -**Deliberately left for later:** the paste/drop baseline (needs synthetic `ClipboardEvent`s with real -web-page payloads; feasible over CDP, not started), and the handler-accumulation repro (now easy: -`editablePageBundle.SetupElements` is exported cross-frame, so calling it twice on the page and -counting `document` keydown listeners via `DOMDebugger.getEventListeners` is the whole repro). +**Handler accumulation (§4.10, inventory X4) — reproduced.** `liveChecks/handlerAccumulation.mjs` +stubs `document.execCommand` in the page frame, dispatches Ctrl+R (the handler on `document`) and F7 +(the per-editable handler), and counts how many handlers fired; then calls the cross-frame +`editablePageBundle.SetupElements(page)` again, as `refreshCanvasElementEditing` does on a subtree: + +| | Ctrl+R handlers fired | F7 handlers fired | +| --- | --- | --- | +| page as loaded | 1 | 1 | +| after one extra `SetupElements` | 2 | 2 | +| after two | 3 | 3 | + +So the code-reading finding of 2026-08-04 is real, not theoretical. (Counting native listeners over +CDP would have shown nothing: jQuery multiplexes all its handlers behind one native listener, which +is why the plan's suggested `DOMDebugger.getEventListeners` check was the wrong instrument.) In real +use the trigger is anything that calls `refreshCanvasElementEditing` — adding or duplicating a canvas +element — after which F6 wraps the selection in `` twice. *Ready to file as its own card.* + +**G2 (async markup path) attempted, still open.** With the Talking Book tool verified current +(`getCurrentTool().id() === "talkingBook"`, `isUpdateMarkupAsync() === true`, toolbox showing), typing +into a Basic Book text box produced **no `audio-sentence` markup at all** — and neither did calling +`updateMarkupAsync()` and applying its result directly. So the async branch of the keystroke pipeline +was not exercised, and the caret harness's PASS in that state proves nothing about G2. The tool has +some gating of its own (probably the box must be its current recording div, or the page must be one +it has set up) that I did not chase. **To close G2:** find what makes the Talking Book tool mark up a +box on typing (start in `audioRecording.ts`'s `updateMarkupAsync`), get spans to appear, then re-run +`verifyCaretPreservation.mjs` and check the caret *and* that the spans are there. + +**Left for later:** the paste/drop baseline (needs synthetic `ClipboardEvent`s with real web-page +payloads; feasible over CDP, not started). + +**A harness lesson:** the jQuery-UI accordion's `h3.ui-accordion-header-active` class is not a reliable +"which tool is active" signal — it said Canvas Tool while the Talking Book panel was plainly open. +`toolboxBundle.getTheOneToolbox().getCurrentTool().id()` is; the harnesses now use that, and +`liveChecks/activateTool.mjs` switches tools through `activateToolFromId`. ## Next actions @@ -975,16 +1005,17 @@ Branch off **`BL-6681-ckeditor`** instead (the files below are new; nothing conf 1. ~~Finish G1~~ (done 2026-09-07, harness passes with the reader tool active). **G2** (async markup path / BL-10133 — Talking Book tool, where the prep commit made its one deliberate - behaviour change) is still unverified; type in a box with the Talking Book tool active and check - the caret. G3 is verified. + behaviour change) is still unverified: the tool did not mark up the box on typing in the + 2026-09-07 attempt (see that entry), so first work out what makes it mark up, then re-run + `verifyCaretPreservation.mjs` and confirm `audio-sentence` spans appear alongside the caret check. + G3 is verified. 2. **Capture the paste/drop baseline** → `PASTE-DROP-BASELINE.md`, rows C1–C7. Use a **real web-page clipboard payload**, not hand-written tidy HTML. Do it before any further code change — this is the row-set whose failure is silent. Include **C7 (drop)**, the row CKEditor has been covering invisibly. -3. **Handler-accumulation repro** (§4.10): `editablePageBundle.SetupElements(page)` is exported - cross-frame, so call it twice on the current page and count `document` keydown listeners via CDP - `DOMDebugger.getEventListeners`; F6 is the likeliest visible symptom. File its own card if it - reproduces. Add the X4 listener-leak test either way — it should fail before any fix. +3. ~~Handler-accumulation repro~~ — **reproduced 2026-09-07** (`liveChecks/handlerAccumulation.mjs`, + 1 → 2 → 3 handlers). Remaining: file its card (John's call), and keep that script as the X4 + listener-leak test — it fails today and should pass once §4.10's signal-scoped teardown lands. 4. ~~Page-reload timing baseline~~ — adopt BL-13502's measurements (see the 2026-09-07 entry) once it merges; re-run its `benchPageChange.mjs` on our branch only if something looks off. diff --git a/docs/retire-ckeditor/liveChecks/README.md b/docs/retire-ckeditor/liveChecks/README.md index 3d92498f8d48..5ed556a9a536 100644 --- a/docs/retire-ckeditor/liveChecks/README.md +++ b/docs/retire-ckeditor/liveChecks/README.md @@ -31,6 +31,11 @@ books cannot be checked out, so Edit is disabled). | `verifyCk.mjs` | Basic Book | Undo button → CKEditor undo only; Ctrl+Z/Ctrl+Y run CKEditor's commands exactly once; our Ctrl+Y declines | | `verifyOrigami.mjs` | Basic Book (customPage) | Undo button → origamiUndo; origami's own Ctrl+Z/Ctrl+Y fire once; ours declines | | `verifyImage.mjs` | any page with an image | Undo button → imageOperationUndo after an undoable copyright change | +| `handlerAccumulation.mjs` | any book with a text box | Stage 0 / inventory X4: edit key handlers accumulate when `SetupElements` runs again (fails today by design — it is the repro) | + +Helpers: `gotoBook.mjs ""` opens a book in Edit; `activateTool.mjs <toolId>` (e.g. +`talkingBook`, `decodableReader`) switches the toolbox tool through the ToolBox's own +`activateToolFromId`. How attribution works: `verifyCommon.mjs` wraps the cross-frame functions the workspace bundle reaches each mechanism through, and listens to CKEditor's `afterCommandExec`, so a gesture is diff --git a/docs/retire-ckeditor/liveChecks/activateTool.mjs b/docs/retire-ckeditor/liveChecks/activateTool.mjs new file mode 100644 index 000000000000..5151fee16a8e --- /dev/null +++ b/docs/retire-ckeditor/liveChecks/activateTool.mjs @@ -0,0 +1,30 @@ +// Activate a toolbox tool by id in the running Bloom, e.g. `node activateTool.mjs talkingBook`. +// Uses the toolbox bundle's own ToolBox.activateToolFromId, so it does exactly what a click on the +// accordion header does (including newPageReady for the new tool). +import { connect, mainPage } from "./cdp.mjs"; +const toolId = process.argv[2]; +if (!toolId) + throw new Error( + "usage: node activateTool.mjs <toolId> (e.g. talkingBook, decodableReader)", + ); +const { browser, pages } = await connect(); +const p = mainPage(pages); +const tb = p.frames().find((f) => f.name() === "toolbox"); +const result = await tb.evaluate((id) => { + const toolbox = window.toolboxBundle.getTheOneToolbox(); + toolbox.activateToolFromId(id); + return toolbox.getCurrentTool()?.id(); +}, toolId); +await p.waitForTimeout(1500); +const active = await tb.evaluate(() => + window.toolboxBundle.getTheOneToolbox().getCurrentTool()?.id(), +); +console.log( + "requested:", + toolId, + "current tool now:", + active, + "(immediately after call:", + result + ")", +); +await browser.close(); diff --git a/docs/retire-ckeditor/liveChecks/handlerAccumulation.mjs b/docs/retire-ckeditor/liveChecks/handlerAccumulation.mjs new file mode 100644 index 000000000000..f7e3b3e457b1 --- /dev/null +++ b/docs/retire-ckeditor/liveChecks/handlerAccumulation.mjs @@ -0,0 +1,95 @@ +// Stage 0 repro for PLAN.md 4.10 / inventory X4: do edit key handlers accumulate when +// SetupElements runs again on a page that already has them? +// +// SetupElements(container) is already called re-entrantly on subtrees (CanvasElementManager's +// refreshCanvasElementEditing, imageDescription.tsx), and it calls AddEditKeyHandlers(container), +// which attaches per-editable jQuery keydown handlers AND two handlers on `document` (Ctrl+Space +// clear-formatting, Ctrl+R/L/E justify). jQuery multiplexes many handlers behind one native +// listener, so counting native listeners over CDP shows nothing; instead this counts what the +// handlers DO: it stubs document.execCommand in the page frame and dispatches the keystrokes. +// +// Needs Bloom on the Edit tab of any book with a content text box. Changes nothing in the book +// (execCommand is stubbed for the duration and restored afterwards). +import { setup, editableSel } from "./verifyCommon.mjs"; +const h = await setup(); +await h.gotoContentPage(); + +const countCommands = async (label) => + h.pf.evaluate( + ({ s, idx, label }) => { + const ed = document.querySelectorAll(s)[idx]; + const real = document.execCommand; + const calls = []; + document.execCommand = (cmd, ui, value) => { + calls.push(cmd); + return true; + }; + try { + // Ctrl+R -> justifyright, from the handler on `document`. + ed.dispatchEvent( + new KeyboardEvent("keydown", { + key: "r", + ctrlKey: true, + bubbles: true, + cancelable: true, + }), + ); + const justify = calls.filter( + (c) => c === "justifyright", + ).length; + calls.length = 0; + // F7 -> formatBlock H1, from the per-editable handler. + ed.dispatchEvent( + new KeyboardEvent("keydown", { + key: "F7", + bubbles: true, + cancelable: true, + }), + ); + const formatBlock = calls.filter( + (c) => c === "formatBlock", + ).length; + return { + label, + justifyRightHandlersFired: justify, + f7HandlersFired: formatBlock, + }; + } finally { + document.execCommand = real; + } + }, + { s: editableSel, idx: h.idx, label }, + ); + +const before = await countCommands("as loaded"); +console.log(JSON.stringify(before)); +// Re-run SetupElements on the page, as the canvas-element refresh paths do on a subtree. +await h.pf.evaluate(() => { + window.editablePageBundle.SetupElements( + document.querySelector(".bloom-page"), + ); +}); +await h.p.waitForTimeout(500); +const after1 = await countCommands("after 1 extra SetupElements"); +console.log(JSON.stringify(after1)); +await h.pf.evaluate(() => { + window.editablePageBundle.SetupElements( + document.querySelector(".bloom-page"), + ); +}); +await h.p.waitForTimeout(500); +const after2 = await countCommands("after 2 extra SetupElements"); +console.log(JSON.stringify(after2)); + +h.record( + "X4 as loaded, each keystroke runs its handler exactly once", + before.justifyRightHandlersFired === 1 && before.f7HandlersFired === 1, + JSON.stringify(before), +); +h.record( + "X4 handlers do NOT accumulate when SetupElements runs again (known to fail: PLAN.md 4.10)", + after2.justifyRightHandlersFired === 1 && after2.f7HandlersFired === 1, + `document-level Ctrl+R handlers: ${before.justifyRightHandlersFired} -> ${after1.justifyRightHandlersFired} -> ${after2.justifyRightHandlersFired}; per-editable F7 handlers: ${before.f7HandlersFired} -> ${after1.f7HandlersFired} -> ${after2.f7HandlersFired}`, +); +h.summary(); +await h.browser.close(); diff --git a/docs/retire-ckeditor/liveChecks/verifyCommon.mjs b/docs/retire-ckeditor/liveChecks/verifyCommon.mjs index 5c9ddf032e51..81ab38a7f597 100644 --- a/docs/retire-ckeditor/liveChecks/verifyCommon.mjs +++ b/docs/retire-ckeditor/liveChecks/verifyCommon.mjs @@ -114,14 +114,16 @@ export async function setup() { await p.waitForTimeout(300); await p.keyboard.press("End"); }; + // Which tool the toolbox considers current. Ask the ToolBox itself: the jQuery-UI accordion's + // header classes lag behind (and once mislabelled the Canvas tool as active while the Talking + // Book panel was plainly open). h.activeTool = () => - h - .toolboxFrame() - ?.evaluate(() => - document - .querySelector("h3.ui-accordion-header-active") - ?.getAttribute("data-toolid"), - ); + h.toolboxFrame()?.evaluate(() => { + const tool = window.toolboxBundle + .getTheOneToolbox() + .getCurrentTool(); + return tool ? tool.id() : undefined; + }); // What the top bar's Undo button does (topBarButtonClick -> handleUndo), minus the WinForms click. h.pressUndoButton = async () => { await p.evaluate(() => window.workspaceBundle.handleUndo()); From 007346606742e414008d5fe6a60ccf135a28f3bc Mon Sep 17 00:00:00 2001 From: John Thomson <john_thomson@sil.org> Date: Mon, 7 Sep 2026 10:33:12 -0500 Subject: [PATCH 06/10] Capture the paste/drop baseline, and the paste-filter bypass it exposed (BL-6681) PASTE-DROP-BASELINE.md records what today's pasteFilter lets through for inventory rows C1-C7, paste and drop alike, from synthetic ClipboardEvent / DragEvent payloads that go through CKEditor's clipboard plugin and Bloom's paste transforms exactly as real ones do. Every row matches the inventory except C5: spans arrive with every attribute and style, because BloomField.restoreHtmlMarkupIfNecessary (BL-12357) tests dataTransfer's cke/id to detect an internal copy, and CKEditor stamps every transfer with one. When the payload contains a styled span that handler replaces the filtered HTML with the full clipboard HTML, so tables, iframes, images and divs with ids reach the book; the same payload without the span is filtered correctly. liveChecks/pasteFilterBypass.mjs is the repro; the inventory's C5/C7 rows and PROGRESS.md are updated, with the fix (test the transfer type) noted. --- docs/retire-ckeditor/BEHAVIOR-INVENTORY.md | 4 +- docs/retire-ckeditor/PASTE-DROP-BASELINE.md | 185 ++++++++++++++++++ docs/retire-ckeditor/PROGRESS.md | 41 ++-- docs/retire-ckeditor/liveChecks/README.md | 2 + .../liveChecks/pasteDropBaseline.mjs | 156 +++++++++++++++ .../liveChecks/pasteFilterBypass.mjs | 99 ++++++++++ 6 files changed, 474 insertions(+), 13 deletions(-) create mode 100644 docs/retire-ckeditor/PASTE-DROP-BASELINE.md create mode 100644 docs/retire-ckeditor/liveChecks/pasteDropBaseline.mjs create mode 100644 docs/retire-ckeditor/liveChecks/pasteFilterBypass.mjs diff --git a/docs/retire-ckeditor/BEHAVIOR-INVENTORY.md b/docs/retire-ckeditor/BEHAVIOR-INVENTORY.md index 3a77441197b4..c44655a5c748 100644 --- a/docs/retire-ckeditor/BEHAVIOR-INVENTORY.md +++ b/docs/retire-ckeditor/BEHAVIOR-INVENTORY.md @@ -81,9 +81,9 @@ of CKEditor's colour panel (§4.5), which supersedes A7 and A9 rather than repro | C2 | Pasting nested **`<div>`s** does not produce divs — importantly, a div copied from another Bloom book must not arrive with its **id**, which would duplicate an id in this book | `pasteFilter` | BL-3899 | ⚠ capture, then unit + live | | C3 | Pasting an **`<iframe>`**, `<script>`, `<style>`, `<object>` or `<embed>` produces none of them | `pasteFilter` | | ⚠ capture, then unit | | C4 | Pasting an **`<img>`** does not embed the image in the text | `pasteFilter` | | ⚠ capture, then live | -| C5 | Pasting arbitrary styled `<span>` soup from a real web page keeps only `font-variant` and `color` | `pasteFilter` | BL-4775, BL-12357 | ⚠ capture, then unit | +| C5 | Pasting arbitrary styled `<span>` soup from a real web page keeps only `font-variant` and `color` — **intended, and NOT what happens today.** Captured 2026-09-07 (PASTE-DROP-BASELINE.md): every span attribute and style survives, because `BloomField.restoreHtmlMarkupIfNecessary` (BL-12357) mistakes every paste for an internal one and restores the unfiltered HTML — **and with it any table/iframe/img/div in the same paste (C1–C4 defeated).** The replacement must implement the intended rule. | `pasteFilter`, undone by BL-12357's `cke/id` test | BL-4775, BL-12357 | captured ✓; unit for the new sanitizer | | C6 | Pasting `<a href>` keeps the link; other attributes on it are dropped | `pasteFilter` `a[!href]` | | ⚠ capture, then unit | -| C7 | **Dropping** any of C1–C6 from outside Bloom is filtered the same way as pasting | CKEditor's clipboard plugin routes `drop` through the same filter, `ckeditor.js:622` | | ⚠ capture, then live | +| C7 | **Dropping** any of C1–C6 from outside Bloom is filtered the same way as pasting | CKEditor's clipboard plugin routes `drop` through the same filter, `ckeditor.js:622` | | captured ✓ 2026-09-07: drop matches paste in every row (including the C5 bypass) | | C8 | Dragging a canvas element from the toolbox onto a page still works (Bloom's own internal drag, custom `text/x-bloom-canvas-element` type) | `CanvasElementManager.ts:2069-2088` | BL-7958 (Linux) | live | | C9 | Pasting **plain** text with several lines produces several paragraphs | `reconstituteParagraphsOnPlainTextPaste` | BL-9961 | unit | | C10 | Bloom's own code can still write markup the paste filter would reject (audio spans with ids, `bloom-linebreak`, canvas elements) — the filter applies **only** at the clipboard/drop boundary | `config.allowedContent = true` alongside a restrictive `pasteFilter` | BL-3899 / BL-3976 (the first fix filtered everything and broke this) | unit | diff --git a/docs/retire-ckeditor/PASTE-DROP-BASELINE.md b/docs/retire-ckeditor/PASTE-DROP-BASELINE.md new file mode 100644 index 000000000000..8ae4b3781725 --- /dev/null +++ b/docs/retire-ckeditor/PASTE-DROP-BASELINE.md @@ -0,0 +1,185 @@ +# Paste / drop baseline — what today's filter lets through + +Captured 2026-09-07 by `liveChecks/pasteDropBaseline.mjs` against Bloom on branch `BL-6681-stage1-undostack` (CKEditor 4 with Bloom's `config.js` pasteFilter). This is the behaviour the replacement sanitizer (PLAN.md 4.8) must reproduce; rows are inventory C1–C7. + +Each row: the box started as `<p>Start end</p>` with the caret after "Start "; the event carried both `text/html` and `text/plain`. + +## C1 — a table + +**Input HTML** + +```html +<table border="1" style="border-collapse:collapse"><thead><tr><th>Name</th><th>Age</th></tr></thead><tbody><tr><td>Ann</td><td>3</td></tr><tr><td>Bob</td><td>5</td></tr></tbody></table> +``` + +**After paste** (event was handled (defaultPrevented)) + +```html +<p>Start NameAge</p><p>Ann3</p><p>Bob5</p><p>end</p> +``` + +**After drop** (event was handled (defaultPrevented)) + +```html +<p>StNameAge</p><p>Ann3</p><p>Bob5</p><p>art end</p> +``` + +## C2 — nested divs with an id (as copied from another Bloom book) + +**Input HTML** + +```html +<div id="i7a3b2c1" class="bloom-editable bloom-content1" lang="en" style="color:red"><div class="inner"><p>Nested <b>bold</b> text</p><p>Second para</p></div></div> +``` + +**After paste** (event was handled (defaultPrevented)) + +```html +<p>Start Nested <strong>bold</strong> text</p><p>Second para</p><p>end</p> +``` + +**After drop** (event was handled (defaultPrevented)) + +```html +<p>StNested <strong>bold</strong> text</p><p>Second para</p><p>art end</p> +``` + +## C3 — iframe, script, style, object, embed + +**Input HTML** + +```html +<p>Before</p><iframe src="https://example.com/"></iframe><script>window.__pwned=1</script><style>p{color:red}</style><object data="movie.swf"></object><embed src="movie.mp4"><p>After</p> +``` + +**After paste** (event was handled (defaultPrevented)) + +```html +<p>Start Before</p><p>After</p><p>end</p> +``` + +**After drop** (event was handled (defaultPrevented)) + +```html +<p>StBefore</p><p>After</p><p>art end</p> +``` + +## C4 — an inline image + +**Input HTML** + +```html +<p>Picture: <img src="https://example.com/a.png" alt="alt text" width="40" height="40"> end</p> +``` + +**After paste** (event was handled (defaultPrevented)) + +```html +<p>Start Picture: end</p><p>end</p> +``` + +**After drop** (event was handled (defaultPrevented)) + +```html +<p>StPicture: end</p><p>art end</p> +``` + +## C5 — styled span soup from a web page + +**Input HTML** + +```html +<p><span style="font-family:Arial,sans-serif;font-size:14pt;color:#ff0000;font-variant:small-caps;background:yellow;font-weight:bold;letter-spacing:2px">Soup</span> <span class="x" data-foo="1" title="t">plain span</span> <span style="text-decoration:underline">underlined</span></p> +``` + +**After paste** (event was handled (defaultPrevented)) + +```html +<p>Start <span style="font-family:Arial,sans-serif;font-size:14pt;color:#ff0000;font-variant:small-caps;background:yellow;font-weight:bold;letter-spacing:2px">Soup</span> <span class="x" data-foo="1" title="t">plain span</span> <span style="text-decoration:underline">underlined</span></p><p>end</p> +``` + +**After drop** (event was handled (defaultPrevented)) + +```html +<p>St<span style="font-family:Arial,sans-serif;font-size:14pt;color:#ff0000;font-variant:small-caps;background:yellow;font-weight:bold;letter-spacing:2px">Soup</span> <span class="x" data-foo="1" title="t">plain span</span> <span style="text-decoration:underline">underlined</span></p><p>art end</p> +``` + +## C6 — a link with extra attributes + +**Input HTML** + +```html +<p>See <a href="https://example.com/page" target="_blank" rel="noopener" class="lnk" id="l1" title="t" style="color:blue" onclick="alert(1)">this link</a>.</p> +``` + +**After paste** (event was handled (defaultPrevented)) + +```html +<p>Start See <a data-cke-saved-href="https://example.com/page" href="https://example.com/page">this link</a>.</p><p>end</p> +``` + +**After drop** (event was handled (defaultPrevented)) + +```html +<p>StSee <a data-cke-saved-href="https://example.com/page" href="https://example.com/page">this link</a>.</p><p>art end</p> +``` + +## C-mixed — a realistic web-page fragment (heading, list, bold/italic, sup) + +**Input HTML** + +```html +<h2 class="title">Heading</h2><ul><li>One <em>two</em></li><li><strong>Three</strong></li></ul><p>H<sub>2</sub>O and E=mc<sup>2</sup>, <u>under</u>, <s>struck</s>, <code>code</code>.</p> +``` + +**After paste** (event was handled (defaultPrevented)) + +```html +<p>Start Heading</p><p>One <em>two</em></p><p><strong>Three</strong></p><p>H2O and E=mc<sup>2</sup>, <u>under</u>, struck, code.</p><p>end</p> +``` + +**After drop** (event was handled (defaultPrevented)) + +```html +<p>StHeading</p><p>One <em>two</em></p><p><strong>Three</strong></p><p>H2O and E=mc<sup>2</sup>, <u>under</u>, struck, code.</p><p>art end</p> +``` + +## Findings + +Read the outputs as **live DOM**, not the saved form: `data-cke-saved-href` / `data-cke-saved-src` +are CKEditor bookkeeping that its `getData()` removes on save. + +1. **Rows C1, C2, C3, C4, C6 and the mixed fragment behave as the inventory says** — blocks collapse + to `<p>`s, tables/divs/iframes/scripts/images/headings/lists vanish leaving their text, the div's + `id` is gone, the link keeps only `href`, `sub`/`s`/`code` are dropped while `em`/`strong`/`sup`/`u` + survive. **Drop (C7) matches paste in every row**, differing only in where the content lands (the + drop point, mid-word here, versus the caret). + +2. **Row C5 does NOT behave as the inventory says.** The spans arrive with *everything* — `class`, + `data-*`, `title`, and the whole `style` (font-family, font-size, background, font-weight, + letter-spacing…), not just `font-variant` and `color`. The filter itself is fine: applied directly, + `pasteFilter` reduces the same soup to `<span style="color:#ff0000; font-variant:small-caps">` and + `<span>`. What undoes it is Bloom's own paste handler, one step later + (`liveChecks/pasteFilterBypass.mjs` logs both): + + `BloomField.restoreHtmlMarkupIfNecessary` (BL-12357) exists to put spans back when the paste came + from *inside* CKEditor. It decides that by `dataTransfer.getData("cke/id")` — but CKEditor 4.5's + `dataTransfer` wrapper assigns an id to **every** transfer, external ones included (the logged ids + are `cke-…`, its generated form). So the test is always true, and whenever the clipboard HTML + contains `<span style=` the handler replaces the **filtered** `dataValue` with the **full** + `text/html` from the clipboard. + +3. **That bypass is not limited to spans.** With a styled span anywhere in the payload, a table, an + `<iframe>`, an `<img>` and a `<div id="dup-id">` all reached the box intact; the identical payload + without the span was filtered to `cell red div text`. Since almost any real web page carries styled + spans, **the BL-3899 guarantee is effectively off for web-page pastes today.** This is the + silent-failure case §4.8 warned about, already in production rather than a future risk. + + *Caveat:* captured with synthetic `ClipboardEvent`s. A real Chrome/Word clipboard also carries + `<!--StartFragment-->` markers, which take the handler's other branch — still the unfiltered + fragment. Confirm once by pasting a real web-page table with coloured text into a Bloom box. + +**Consequences for the plan:** the new sanitizer (Stage 3, `pasteSanitizer.ts`) must implement the +*intended* C5 (`span{font-variant,color}`), not today's behaviour; D8's "detect an internal copy" needs +a test that actually works (`getTransferType() === DATA_TRANSFER_INTERNAL`, or `sourceEditor`); and +this should be fixed on master independently of the project — it is a one-line condition. diff --git a/docs/retire-ckeditor/PROGRESS.md b/docs/retire-ckeditor/PROGRESS.md index b65a6ad94631..e94fcd731823 100644 --- a/docs/retire-ckeditor/PROGRESS.md +++ b/docs/retire-ckeditor/PROGRESS.md @@ -80,9 +80,12 @@ Stage 0 checklist (PLAN.md §6): - [x] Environment unblocked: `vp`/`volta` PATH untangled, `init.sh` clean, `output/browser` repopulated. Full front-end suite green: **591 passed**. - [x] `toolbox.ts` selection-bracket prep commit (`2707d98a8`) — §5.7.3 done -- [ ] Capture the paste/drop baseline (rows C1–C7, incl. **C7 drop**): needs a running Bloom -- [ ] Handler-accumulation repro (§4.10) + the X4 listener-leak test: needs a running Bloom -- [ ] Page-reload timing baseline (§4.11): needs a running Bloom +- [x] **Paste/drop baseline captured** (2026-09-07, `PASTE-DROP-BASELINE.md`, C1–C7 incl. drop) — and + it found that **the paste filter is bypassed whenever the payload contains a styled span** + (BL-12357's `cke/id` test is always true), letting tables/iframes/images/divs into the book. +- [x] Handler-accumulation repro (§4.10) — **reproduced** 2026-09-07 (1 → 2 → 3 handlers); + `liveChecks/handlerAccumulation.mjs` is the X4 test, failing until §4.10 lands +- [~] Page-reload timing baseline (§4.11) — superseded by BL-13502's measurements (see 2026-09-07) - [x] Rebased onto `origin/master` (was 64 behind; one conflict in `toolbox.ts`, resolved). Now 0 behind. Typecheck clean, 63 tests green. - [x] **G1 verified, both halves.** Automated: `verifyCaretPreservation.mjs` PASS (caret at the right @@ -973,8 +976,20 @@ it has set up) that I did not chase. **To close G2:** find what makes the Talkin box on typing (start in `audioRecording.ts`'s `updateMarkupAsync`), get spans to appear, then re-run `verifyCaretPreservation.mjs` and check the caret *and* that the spans are there. -**Left for later:** the paste/drop baseline (needs synthetic `ClipboardEvent`s with real web-page -payloads; feasible over CDP, not started). +**Paste/drop baseline captured (§4.8, rows C1–C7)** — `liveChecks/pasteDropBaseline.mjs` → +`PASTE-DROP-BASELINE.md`, by dispatching synthetic `paste` and `drop` events carrying `text/html`, +which go through CKEditor's clipboard plugin and Bloom's paste transforms exactly as real ones do. +Every row except C5 behaves as the inventory says, and drop matches paste throughout. **C5 does not, +and the reason is a real bug:** `BloomField.restoreHtmlMarkupIfNecessary` (BL-12357) tests +`dataTransfer.getData("cke/id")` to detect an internal CKEditor copy, but CKEditor assigns an id to +*every* transfer, so the test is always true — and when the pasted HTML contains `<span style=` it +replaces CKEditor's *filtered* HTML with the *full* clipboard HTML. `liveChecks/pasteFilterBypass.mjs` +shows the same payload with and without one styled span: without, `table/iframe/img/div#id` are all +stripped; with, **all four reach the book**. So the BL-3899 guarantee is effectively off for +web-page pastes today. Full write-up in `PASTE-DROP-BASELINE.md` ▸ Findings. **This is worth a card +of its own, ahead of anything else found today** — it is a one-condition fix on master +(`getTransferType() === DATA_TRANSFER_INTERNAL`), and it should be confirmed once with a real +clipboard paste, since the capture used synthetic events. **A harness lesson:** the jQuery-UI accordion's `h3.ui-accordion-header-active` class is not a reliable "which tool is active" signal — it said Canvas Tool while the Talking Book panel was plainly open. @@ -994,8 +1009,13 @@ working tree. Bloom can be launched from this worktree with the `run-bloom` skil integration branch's first PR could open now. Nothing done pending the answer. - **Restore the three hook-reformatted master files** in the sync merge (needs one `--no-verify` commit)? Or leave the noise. -- **File the two reader-tools bugs** found above (stale bookmark span in undo snapshots; Ctrl+Z runs - two undos), after a master repro? And the dead `data-page-id` check in `ImageUndoManager`? +- **File the bugs found today** — in priority order: (1) the **paste-filter bypass** (BL-12357's + `cke/id` test admits every paste containing a styled span; tables/iframes/images/divs get in) — + confirm with a real clipboard first; (2) Ctrl+Z with a reader tool active runs two undos and breaks + Ctrl+Y; (3) the reader-tools undo restores stale `cke_bm_` bookmark spans; (4) edit key handlers + accumulate on every `SetupElements` re-run (F6 double-wraps); (5) the dead `data-page-id` check in + `ImageUndoManager`. All reproduced on this branch; (2)–(5) are untouched by our changes, and (1) is + in code we have not modified at all. ### Stage 0's remainder — four items, all needing a running Bloom @@ -1009,10 +1029,9 @@ Branch off **`BL-6681-ckeditor`** instead (the files below are new; nothing conf 2026-09-07 attempt (see that entry), so first work out what makes it mark up, then re-run `verifyCaretPreservation.mjs` and confirm `audio-sentence` spans appear alongside the caret check. G3 is verified. -2. **Capture the paste/drop baseline** → `PASTE-DROP-BASELINE.md`, rows C1–C7. Use a **real web-page - clipboard payload**, not hand-written tidy HTML. Do it before any further code change — this is the - row-set whose failure is silent. Include **C7 (drop)**, the row CKEditor has been covering - invisibly. +2. ~~Capture the paste/drop baseline~~ — done 2026-09-07 with synthetic events; **remaining:** one + manual confirmation with a real clipboard (copy a web-page table containing coloured text into a + Bloom box) that the styled-span bypass happens for real pastes too, then file it. 3. ~~Handler-accumulation repro~~ — **reproduced 2026-09-07** (`liveChecks/handlerAccumulation.mjs`, 1 → 2 → 3 handlers). Remaining: file its card (John's call), and keep that script as the X4 listener-leak test — it fails today and should pass once §4.10's signal-scoped teardown lands. diff --git a/docs/retire-ckeditor/liveChecks/README.md b/docs/retire-ckeditor/liveChecks/README.md index 5ed556a9a536..fbaba303d43f 100644 --- a/docs/retire-ckeditor/liveChecks/README.md +++ b/docs/retire-ckeditor/liveChecks/README.md @@ -32,6 +32,8 @@ books cannot be checked out, so Edit is disabled). | `verifyOrigami.mjs` | Basic Book (customPage) | Undo button → origamiUndo; origami's own Ctrl+Z/Ctrl+Y fire once; ours declines | | `verifyImage.mjs` | any page with an image | Undo button → imageOperationUndo after an undoable copyright change | | `handlerAccumulation.mjs` | any book with a text box | Stage 0 / inventory X4: edit key handlers accumulate when `SetupElements` runs again (fails today by design — it is the repro) | +| `pasteDropBaseline.mjs` | Basic Book | Stage 0 / inventory C1–C7: writes `../PASTE-DROP-BASELINE.md`, what paste and drop let through today | +| `pasteFilterBypass.mjs` | Basic Book | The bug the baseline found: a styled span in the payload makes BL-12357's handler restore the unfiltered HTML (fails today by design) | Helpers: `gotoBook.mjs "<title>"` opens a book in Edit; `activateTool.mjs <toolId>` (e.g. `talkingBook`, `decodableReader`) switches the toolbox tool through the ToolBox's own diff --git a/docs/retire-ckeditor/liveChecks/pasteDropBaseline.mjs b/docs/retire-ckeditor/liveChecks/pasteDropBaseline.mjs new file mode 100644 index 000000000000..437112e2d7a7 --- /dev/null +++ b/docs/retire-ckeditor/liveChecks/pasteDropBaseline.mjs @@ -0,0 +1,156 @@ +// Stage 0 capture for inventory rows C1-C7 (BL-6681, PLAN.md 4.8): what does TODAY's paste/drop +// filtering actually let through? Writes docs/retire-ckeditor/PASTE-DROP-BASELINE.md. +// +// Why synthetic events: the point is the filter, not the OS clipboard. A `paste` ClipboardEvent +// carrying a DataTransfer with text/html goes through exactly the code a real paste does - +// CKEditor's clipboard plugin reads evt.data.$.clipboardData, applies pasteFilter, fires its own +// `paste` event that BloomField's transforms hook - and a `drop` DragEvent carrying the same +// DataTransfer goes through the plugin's drop handler (C7). What it does NOT cover is anything that +// reads the real clipboard from C# (the Paste button), which is a separate entry point. +// +// Needs Bloom on the Edit tab of a book with a content text box (a Basic Book). Restores the box. +import { writeFileSync } from "node:fs"; +import path from "node:path"; +import { setup, editableSel } from "./verifyCommon.mjs"; +import { repoRoot } from "./cdp.mjs"; + +const rows = [ + { + id: "C1", + name: "a table", + html: `<table border="1" style="border-collapse:collapse"><thead><tr><th>Name</th><th>Age</th></tr></thead><tbody><tr><td>Ann</td><td>3</td></tr><tr><td>Bob</td><td>5</td></tr></tbody></table>`, + text: "Name\tAge\nAnn\t3\nBob\t5", + }, + { + id: "C2", + name: "nested divs with an id (as copied from another Bloom book)", + html: `<div id="i7a3b2c1" class="bloom-editable bloom-content1" lang="en" style="color:red"><div class="inner"><p>Nested <b>bold</b> text</p><p>Second para</p></div></div>`, + text: "Nested bold text\nSecond para", + }, + { + id: "C3", + name: "iframe, script, style, object, embed", + html: `<p>Before</p><iframe src="https://example.com/"></iframe><script>window.__pwned=1</script><style>p{color:red}</style><object data="movie.swf"></object><embed src="movie.mp4"><p>After</p>`, + text: "Before\nAfter", + }, + { + id: "C4", + name: "an inline image", + html: `<p>Picture: <img src="https://example.com/a.png" alt="alt text" width="40" height="40"> end</p>`, + text: "Picture: end", + }, + { + id: "C5", + name: "styled span soup from a web page", + html: `<p><span style="font-family:Arial,sans-serif;font-size:14pt;color:#ff0000;font-variant:small-caps;background:yellow;font-weight:bold;letter-spacing:2px">Soup</span> <span class="x" data-foo="1" title="t">plain span</span> <span style="text-decoration:underline">underlined</span></p>`, + text: "Soup plain span underlined", + }, + { + id: "C6", + name: "a link with extra attributes", + html: `<p>See <a href="https://example.com/page" target="_blank" rel="noopener" class="lnk" id="l1" title="t" style="color:blue" onclick="alert(1)">this link</a>.</p>`, + text: "See this link.", + }, + { + id: "C-mixed", + name: "a realistic web-page fragment (heading, list, bold/italic, sup)", + html: `<h2 class="title">Heading</h2><ul><li>One <em>two</em></li><li><strong>Three</strong></li></ul><p>H<sub>2</sub>O and E=mc<sup>2</sup>, <u>under</u>, <s>struck</s>, <code>code</code>.</p>`, + text: "Heading\nOne two\nThree\nH2O and E=mc2, under, struck, code.", + }, +]; + +const h = await setup(); +await h.gotoContentPage(); +const originalHtml = await h.readHtml(); + +async function applyEvent(kind, row) { + await h.writeHtml("<p>Start end</p>"); + await h.p.waitForTimeout(200); + await h.pf.locator(editableSel).nth(h.idx).click(); + await h.p.waitForTimeout(300); + // Put the caret after "Start " so the pasted content lands mid-paragraph. + await h.pf.evaluate( + ({ s, idx }) => { + const ed = document.querySelectorAll(s)[idx]; + const t = ed.querySelector("p").firstChild; + const r = document.createRange(); + r.setStart(t, 6); + r.collapse(true); + const sel = window.getSelection(); + sel.removeAllRanges(); + sel.addRange(r); + }, + { s: editableSel, idx: h.idx }, + ); + const dispatched = await h.pf.evaluate( + ({ s, idx, kind, html, text }) => { + const ed = document.querySelectorAll(s)[idx]; + const dt = new DataTransfer(); + dt.setData("text/html", html); + dt.setData("text/plain", text); + let evt; + if (kind === "paste") { + evt = new ClipboardEvent("paste", { + clipboardData: dt, + bubbles: true, + cancelable: true, + }); + } else { + const rect = ed.getBoundingClientRect(); + evt = new DragEvent("drop", { + dataTransfer: dt, + bubbles: true, + cancelable: true, + clientX: rect.left + 20, + clientY: rect.top + 10, + }); + } + const target = kind === "paste" ? document.activeElement || ed : ed; + const notCancelled = target.dispatchEvent(evt); + return { + targetIsEditable: ed.contains(target) || target === ed, + defaultPrevented: evt.defaultPrevented, + notCancelled, + }; + }, + { s: editableSel, idx: h.idx, kind, html: row.html, text: row.text }, + ); + await h.p.waitForTimeout(1200); + const result = await h.readHtml(); + return { dispatched, result }; +} + +const out = []; +out.push("# Paste / drop baseline — what today's filter lets through\n"); +out.push( + `Captured ${new Date().toISOString().slice(0, 10)} by \`liveChecks/pasteDropBaseline.mjs\` against Bloom on branch \`BL-6681-stage1-undostack\` (CKEditor 4 with Bloom's \`config.js\` pasteFilter). This is the behaviour the replacement sanitizer (PLAN.md 4.8) must reproduce; rows are inventory C1–C7.\n`, +); +out.push( + 'Each row: the box started as `<p>Start end</p>` with the caret after "Start "; the event carried both `text/html` and `text/plain`.\n', +); +for (const row of rows) { + out.push(`## ${row.id} — ${row.name}\n`); + out.push("**Input HTML**\n\n```html\n" + row.html + "\n```\n"); + for (const kind of ["paste", "drop"]) { + const { dispatched, result } = await applyEvent(kind, row); + console.log( + row.id, + kind, + JSON.stringify(dispatched), + "=>", + result.slice(0, 160), + ); + const unchanged = result === "<p>Start end</p>"; + out.push( + `**After ${kind}** (event ${dispatched.defaultPrevented ? "was handled (defaultPrevented)" : "was NOT defaultPrevented"}${unchanged ? "; box unchanged — nothing was inserted" : ""})\n\n\`\`\`html\n${result}\n\`\`\`\n`, + ); + } +} +await h.writeHtml(originalHtml); +const outPath = path.join( + repoRoot, + "docs/retire-ckeditor/PASTE-DROP-BASELINE.md", +); +writeFileSync(outPath, out.join("\n"), "utf8"); +console.log("wrote", outPath); +await h.browser.close(); diff --git a/docs/retire-ckeditor/liveChecks/pasteFilterBypass.mjs b/docs/retire-ckeditor/liveChecks/pasteFilterBypass.mjs new file mode 100644 index 000000000000..a10087338c8b --- /dev/null +++ b/docs/retire-ckeditor/liveChecks/pasteFilterBypass.mjs @@ -0,0 +1,99 @@ +// Repro for the paste-filter bypass found while capturing the C1-C7 baseline (BL-6681, 2026-09-07). +// +// Bloom's pasteFilter (config.js) reduces external HTML to a small vocabulary. But +// BloomField.restoreHtmlMarkupIfNecessary (BL-12357) decides "this paste came from inside CKEditor, +// so put the spans back" by testing dataTransfer.getData("cke/id") -- and CKEditor's dataTransfer +// wrapper assigns an id to EVERY transfer, external ones included (the ids logged here start +// "cke-", CKEditor's generated form). So whenever the pasted HTML contains "<span style=", the +// handler replaces the FILTERED html with the FULL clipboard html, and tables, iframes, images and +// divs with ids all reach the book. The control payload (no styled span) is filtered correctly. +// +// Logs the pasted HTML after CKEditor's filter (priority 9) and after Bloom's handler (priority 11), +// so the point of failure is visible. Needs Bloom on the Edit tab of a book with a content text box. +// Restores the box. Exits non-zero while the bug is present. +import { setup, editableSel } from "./verifyCommon.mjs"; +const h = await setup(); +await h.gotoContentPage(); +const orig = await h.readHtml(); + +const run = async (label, html) => { + await h.writeHtml("<p>Start end</p>"); + await h.pf.locator(editableSel).nth(h.idx).click(); + await h.p.waitForTimeout(300); + await h.pf.evaluate( + ({ s, idx, html }) => { + const ed = document.querySelectorAll(s)[idx]; + const editor = ed.bloomCkEditor; + window.__log = []; + if (!editor.__diag) { + editor.__diag = true; + editor.on( + "paste", + (e) => + window.__log.push({ + at: "after CKEditor's pasteFilter (priority 9)", + ckeId: e.data.dataTransfer.getData("cke/id"), + transferType: + e.data.dataTransfer.getTransferType(editor), + dataValue: String(e.data.dataValue), + }), + null, + null, + 9, + ); + editor.on( + "paste", + (e) => + window.__log.push({ + at: "after Bloom's paste handler (priority 11)", + dataValue: String(e.data.dataValue), + }), + null, + null, + 11, + ); + } + const dt = new DataTransfer(); + dt.setData("text/html", html); + dt.setData("text/plain", "x"); + ed.dispatchEvent( + new ClipboardEvent("paste", { + clipboardData: dt, + bubbles: true, + cancelable: true, + }), + ); + }, + { s: editableSel, idx: h.idx, html }, + ); + await h.p.waitForTimeout(1200); + console.log("=== " + label); + for (const l of await h.pf.evaluate(() => window.__log)) + console.log(" ", JSON.stringify(l)); + const final = await h.readHtml(); + console.log(" final:", JSON.stringify(final)); + return final; +}; + +const hostile = `<table border="1"><tr><td>cell <span style="color:red">red</span></td></tr></table><iframe src="https://example.com/"></iframe><img src="https://example.com/a.png"><div id="dup-id"><p>div text</p></div>`; +const withSpan = await run( + "table + iframe + img + div#id, WITH a styled span", + hostile, +); +const control = await run( + "the same, WITHOUT the styled span", + hostile.replace(`<span style="color:red">red</span>`, "red"), +); +h.record( + "control: without a styled span the filter holds (no table/iframe/img/div)", + !/<(table|iframe|img|div)/.test(control), + control, +); +h.record( + "with a styled span the filter still holds (FAILS today: BL-12357's cke/id test admits everything)", + !/<(table|iframe|img|div)/.test(withSpan), + withSpan.slice(0, 200), +); +await h.writeHtml(orig); +h.summary(); +await h.browser.close(); From 495cee770f41049e0300500c8f945b1a24f27aed Mon Sep 17 00:00:00 2001 From: John Thomson <john_thomson@sil.org> Date: Mon, 7 Sep 2026 10:34:39 -0500 Subject: [PATCH 07/10] Record the integration-branch nightly result (BL-6681) --- docs/retire-ckeditor/PROGRESS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/retire-ckeditor/PROGRESS.md b/docs/retire-ckeditor/PROGRESS.md index e94fcd731823..9ac6b9280e95 100644 --- a/docs/retire-ckeditor/PROGRESS.md +++ b/docs/retire-ckeditor/PROGRESS.md @@ -62,7 +62,7 @@ it, PR'd into it and **squash-merged**, so integration carries one commit per st | Date | Merged `master` at | Watchlist commits in that range | | --- | --- | --- | | 2026-08-06 | `9b6ba1cd9` | **0** of 51 — clean merge, nothing of ours touched | -| 2026-09-07 | `f0d9f1472` | **11** of 433 — one conflict, `toolbox.ts` (BL-16717 made bookmarks conditional inside the extracted seam); resolved by teaching the seam. Nightly run [34134257000](https://github.com/BloomBooks/BloomDesktop/actions/runs/34134257000) triggered | +| 2026-09-07 | `f0d9f1472` | **11** of 433 — one conflict, `toolbox.ts` (BL-16717 made bookmarks conditional inside the extracted seam); resolved by teaching the seam. Nightly [34134257000](https://github.com/BloomBooks/BloomDesktop/actions/runs/34134257000): TS, C#, visual-regression and React suites **green**; BloomE2E failed 2 of 47 (Test Case 356 gear positioning — in active development in another worktree; 170 publish talking-book languages), neither near undo, and master's own nightlies have failed daily this week | All of PLAN.md §10 is decided except the Stage-5 legacy-cleanup lifetime, which blocks nothing. From 8e70aa588e16fe0bc7cee7acc1de291407a09fc5 Mon Sep 17 00:00:00 2001 From: John Thomson <john_thomson@sil.org> Date: Mon, 7 Sep 2026 11:48:38 -0500 Subject: [PATCH 08/10] Route the Undo button to the workspace frame's stack, and fix what review found (BL-6681) The Undo button never reached workspaceRoot's handleUndo: C# calls topBarButtonClick in the PAGE frame, which imported handleUndo from ../workspaceRoot, so that module -- and "the one" stack, with its own providers -- was executing in the page frame too, and the button undid from that copy. Neutral while both were empty, wrong the moment anything is pushed. topBarButtonClick now calls getWorkspaceBundleExports().handleUndo(), the import is gone, and the provider registration runs only in the top frame (Vite still bundles workspaceRoot into a chunk the page and toolbox bundles import, so its top level runs in every frame). The live checks now press Undo through the page frame's real entry point, which is how the bug was missed. Devin's three findings, all in new code: undo()/redo() now move currentIndex transactionally, so a failing entry stays the next thing to undo instead of being skipped and offered as Redo; runUndoable holds pushes until the outermost scope closes and keeps the gesture's own (depth-1) entry, falling back to the first inner one, so an inner layer recording first no longer wins; and the Ctrl+Y binding stands down in Change Layout mode, where origami redoes without claiming the event. A once-only load listener records the page id even when switchContentPage's 1500 ms fallback ran first. 69 undo/toolbox tests green (11 new), typecheck and lint clean, and all four live checks pass on a freshly launched Bloom. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --- docs/retire-ckeditor/PROGRESS.md | 39 +++++ .../liveChecks/verifyCommon.mjs | 11 +- .../bookEdit/js/bloomEditing.ts | 9 +- src/BloomBrowserUI/bookEdit/undo/UndoStack.ts | 110 ++++++++++---- .../bookEdit/undo/UndoStackSpec.ts | 138 +++++++++++++++++- .../bookEdit/undo/pageFrameUndoHooks.ts | 16 +- .../bookEdit/undo/redoKeyBinding.ts | 15 ++ .../bookEdit/undo/redoKeyBindingSpec.ts | 11 ++ .../bookEdit/undo/runUndoableSpec.ts | 14 +- src/BloomBrowserUI/bookEdit/workspaceRoot.ts | 20 ++- 10 files changed, 339 insertions(+), 44 deletions(-) diff --git a/docs/retire-ckeditor/PROGRESS.md b/docs/retire-ckeditor/PROGRESS.md index 9ac6b9280e95..20b33167a473 100644 --- a/docs/retire-ckeditor/PROGRESS.md +++ b/docs/retire-ckeditor/PROGRESS.md @@ -996,6 +996,45 @@ clipboard paste, since the capture used synthetic events. `toolboxBundle.getTheOneToolbox().getCurrentTool().id()` is; the harnesses now use that, and `liveChecks/activateTool.mjs` switches tools through `activateToolFromId`. +### 2026-09-07 (later) — Stage 1 preflighted: PR #8317; two stacks found and fixed + +**PR:** https://github.com/BloomBooks/BloomDesktop/pull/8317 (draft, into `BL-6681-ckeditor`). Devin +and a read-only local review both ran; every finding was acted on the same day. Two of them matter +beyond this PR: + +1. **There were two "one" stacks.** The Undo button does not call the workspace bundle's + `handleUndo`: C# runs `getEditablePageBundleExports().topBarButtonClick({command:"undo"})` in the + **page** frame, and `bloomEditing.ts` imported `handleUndo` from `../workspaceRoot` — so + `workspaceRoot`'s module (with `theOneUndoStack` and the module-level provider registration) was + also executing in the page frame, and the button undid from *that* copy. Neutral while both were + empty, wrong the moment anything is pushed. **Fixed:** `topBarButtonClick` now calls + `getWorkspaceBundleExports().handleUndo()`, the import is gone, and registration runs only when + `window.parent === window`. The production build confirms why the guard is needed: Vite still puts + `workspaceRoot` in a chunk (`requiresSubscriptionBundle-main.js`) that the page and toolbox bundles + import, so its top level runs in all three frames regardless. **Lesson for the harnesses:** the + live checks had called `workspaceBundle.handleUndo()` directly and so could not see this; + `pressUndoButton` now goes through the page frame's `topBarButtonClick`, the real entry point. +2. **Devin's three bugs, all real, all fixed:** a failed undo/redo moved `currentIndex` anyway (now + transactional — the failing entry stays the next thing to undo, tests for sync and async failure + both ways); `runUndoable` kept the *first* push inside a scope rather than the outer gesture's own + entry (now: pushes are held while a scope is open and, when the outermost closes, the first + depth-1 push wins, else the first inner one — with the discipline that an inner operation records + inside its own `runUndoable`); and Ctrl+Y in Change Layout mode would have fired origami's redo + *and* ours once the stack held anything (the binding now stands down when + `.marginBox.origami-layout-mode` is present, until Stage 4 retires origami's handler). Also a + dedicated once-only `load` listener records the page id even when the 1500 ms fallback ran first. + +**Observed, not chased — the reader-tools undo arms itself in books without a reader tool.** In "A +house for mouse" (Basic Book, toolbox shows only Canvas/Talking Book/Settings), after this session +had earlier opened a Decodable Reader book and re-run `SetupElements` on this page, typing in a text +box made `toolboxBundle.canUndo()` true, so the Undo button ran the reader-tools undo instead of +CKEditor's — with the Canvas tool active as well as the Talking Book tool. `doMarkup` (which pushes +the model's undo snapshots) is only reachable from the decodable/leveled tools' own keyup handlers, +so those handlers were attached to this page's editables somehow. Pre-existing (the old +`handleUndo` used the same order and the same `toolbox.canUndo()`), and possibly an artefact of the +session's own harness runs, so it needs a repro from a fresh launch before it is filed. Stage 3 +removes both mechanisms anyway. + ## Next actions Everything below is pushed; nothing is half-applied, and both branches are green with a clean diff --git a/docs/retire-ckeditor/liveChecks/verifyCommon.mjs b/docs/retire-ckeditor/liveChecks/verifyCommon.mjs index 81ab38a7f597..fd2e08c43d1a 100644 --- a/docs/retire-ckeditor/liveChecks/verifyCommon.mjs +++ b/docs/retire-ckeditor/liveChecks/verifyCommon.mjs @@ -124,9 +124,16 @@ export async function setup() { .getCurrentTool(); return tool ? tool.id() : undefined; }); - // What the top bar's Undo button does (topBarButtonClick -> handleUndo), minus the WinForms click. + // What the top bar's Undo button does, minus the click itself: C# runs + // getEditablePageBundleExports().topBarButtonClick({command:"undo"}) -- in the PAGE frame -- + // which must reach the workspace frame's one stack. Going through the page frame here, rather + // than calling workspaceBundle.handleUndo() directly, is what caught the two-stacks bug. h.pressUndoButton = async () => { - await p.evaluate(() => window.workspaceBundle.handleUndo()); + await p.evaluate(() => + window.workspaceBundle + .getEditablePageBundleExports() + .topBarButtonClick({ command: "undo" }), + ); await p.waitForTimeout(800); }; // Find a content page (not xmatter) with a CKEditor'd content editable, starting at thumb 1. diff --git a/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts b/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts index 30ab1554988c..06172d100719 100644 --- a/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts +++ b/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts @@ -52,6 +52,7 @@ import "../../lib/long-press/jquery.longpress.js"; import { doWhenWorkspaceBundleLoaded, getToolboxBundleExports, + getWorkspaceBundleExports, } from "./workspaceFrames"; import { showInvisibles, hideInvisibles } from "./showInvisibles"; @@ -75,7 +76,6 @@ import { setupBookLinkGrids } from "./linkGrid"; import { fitImageOverTextSplits } from "./autoFitImageOverTextSplits"; import PlaceholderProvider from "./PlaceholderProvider"; import { initChoiceWidgetsForEditing } from "./simpleComprehensionQuiz"; -import { handleUndo } from "../workspaceRoot"; import { setupPageLayoutMenu } from "../toolbox/canvas/customXmatterPage"; import { setupTextContextMenu } from "../textContextMenu/TextContextMenu"; import { resetAbovePageControls } from "./AbovePageControls"; @@ -1647,7 +1647,12 @@ export function topBarButtonClick(button: { command: string }) { cutSelection(); break; case "undo": - handleUndo(); + // Across the frame boundary, deliberately. This function runs in the PAGE frame + // (C# calls it through getEditablePageBundleExports), but the one undo stack lives + // in the workspace frame. Importing handleUndo from ../workspaceRoot here would run + // a second copy of that module in this frame, with its own empty stack, and the + // button would undo from the wrong one (BL-6681). + getWorkspaceBundleExports().handleUndo(); break; // We don't handle paste this way. We need code on the C# side to decide if we have // an image on the clipboard. So we shortcut a roundtrip to client and server by just diff --git a/src/BloomBrowserUI/bookEdit/undo/UndoStack.ts b/src/BloomBrowserUI/bookEdit/undo/UndoStack.ts index 5ed1241c9b1d..2113f9ed342a 100644 --- a/src/BloomBrowserUI/bookEdit/undo/UndoStack.ts +++ b/src/BloomBrowserUI/bookEdit/undo/UndoStack.ts @@ -32,12 +32,15 @@ export class UndoStack { /** * Labels of the `runUndoable` scopes currently open, outermost first. - * Non-empty means a push should be folded into the outermost scope rather than added. + * Non-empty means a push is held until the outermost scope closes. See {@link push}. */ private openScopeLabels: string[] = []; - /** Whether the outermost open scope has already claimed an entry. See {@link push}. */ - private pushedInOutermostScope = false; + /** + * Entries pushed while a scope was open, with the scope depth each arrived at, in order. One + * of them is recorded when the outermost scope closes; see {@link endUndoableScope}. + */ + private heldPushes: { entry: IUndoEntry; depth: number }[] = []; /** True while an undo or redo is being applied, to stop a re-entrant one interleaving. */ private applying = false; @@ -60,23 +63,21 @@ export class UndoStack { /** * Record an undoable step. * - * If a `runUndoable` scope is open this does *not* add a second entry — one user gesture must - * produce exactly one entry, however many layers of code it passes through. The outermost - * scope wins: the first push inside it is kept and relabelled with the scope's label, and - * later pushes within the same scope are ignored. See PLAN.md 4.13. + * If a `runUndoable` scope is open the entry is not recorded yet but *held*: one user gesture + * must produce exactly one entry, however many layers of code it passes through, and which of + * the held entries that is can only be decided once the whole gesture has run. See + * {@link endUndoableScope} for the rule, and PLAN.md 4.13. */ public push(entry: IUndoEntry): void { if (this.openScopeLabels.length > 0) { - if (this.pushedInOutermostScope) { - // A nested operation recording its own undo. Deliberately dropped: undoing the - // outermost operation already covers it, and keeping both would make the first - // Ctrl+Z half-undo the gesture. - return; - } - this.pushedInOutermostScope = true; - entry.label = this.openScopeLabels[0]; + this.heldPushes.push({ entry, depth: this.openScopeLabels.length }); + return; } + this.record(entry); + } + /** Actually add an entry, truncating any redo branch. */ + private record(entry: IUndoEntry): void { // Anything the user had undone is now unreachable: they have taken a different branch. this.entries.length = this.currentIndex + 1; @@ -137,9 +138,20 @@ export class UndoStack { return; } const entry = this.entries[this.currentIndex]; + const indexBefore = this.currentIndex; this.currentIndex--; - entry.prepareRedo?.(); - return this.apply(() => entry.undo()); + // If the entry fails to undo, the index goes back where it was: the failed entry stays the + // next thing to undo (so the user can retry, or see that it is stuck), instead of being + // silently skipped and offered as a Redo of something that never happened. + return this.apply( + () => { + entry.prepareRedo?.(); + return entry.undo(); + }, + () => { + this.currentIndex = indexBefore; + }, + ); } /** @@ -153,8 +165,16 @@ export class UndoStack { return; } const entry = this.entries[this.currentIndex + 1]; + const indexBefore = this.currentIndex; this.currentIndex++; - return this.apply(() => entry.redo!()); + // As in undo(): a redo that fails leaves the index where it was, so the same entry is + // still the next Redo rather than being treated as done. + return this.apply( + () => entry.redo!(), + () => { + this.currentIndex = indexBefore; + }, + ); } /** @@ -216,14 +236,39 @@ export class UndoStack { */ public beginUndoableScope(label: string): void { if (this.openScopeLabels.length === 0) { - this.pushedInOutermostScope = false; + this.heldPushes = []; } this.openScopeLabels.push(label); } - /** Close the innermost `runUndoable` scope. */ + /** + * Close the innermost `runUndoable` scope. Closing the *outermost* one records exactly one of + * the entries pushed while it was open, labelled with the outermost scope's label: + * + * - the first entry the outermost operation pushed **itself** (at depth 1), if it pushed one — + * that entry describes the whole gesture, which is what a single Ctrl+Z must reverse; or + * - failing that, the first entry pushed by anything nested inside it, since a scope that + * records nothing of its own is just a wrapper saying "these inner steps are one gesture". + * + * "First push wins" alone would be wrong: an inner layer usually runs, and pushes, *before* the + * outer operation gets to record its own entry, and keeping the inner one would leave an undo + * that reverses only part of the gesture (an image reverting to a placeholder, say, but not the + * canvas element coming back). The corollary is a discipline for inner layers: an operation + * that records its own undo does so inside its own `runUndoable`, so that its push sits at + * depth 2 or more when it happens inside a larger gesture. See PLAN.md 4.13. + */ public endUndoableScope(): void { + const label = this.openScopeLabels[0]; this.openScopeLabels.pop(); + if (this.openScopeLabels.length > 0 || this.heldPushes.length === 0) { + return; + } + const chosen = + this.heldPushes.find((held) => held.depth === 1) ?? + this.heldPushes[0]; + this.heldPushes = []; + chosen.entry.label = label; + this.record(chosen.entry); } /** Whether a `runUndoable` scope is currently open. */ @@ -231,23 +276,38 @@ export class UndoStack { return this.openScopeLabels.length > 0; } - /** Run an entry's undo/redo, holding the re-entrancy guard until it finishes. */ - private apply(action: () => void | Promise<void>): void | Promise<void> { + /** + * Run an entry's undo/redo, holding the re-entrancy guard until it finishes, and calling + * `onFailure` (after releasing the guard) if it throws or rejects. The failure itself is still + * propagated to the caller. + */ + private apply( + action: () => void | Promise<void>, + onFailure: () => void, + ): void | Promise<void> { this.applying = true; let result: void | Promise<void>; try { result = action(); } catch (e) { this.applying = false; + onFailure(); throw e; } if (!result) { this.applying = false; return; } - return result.finally(() => { - this.applying = false; - }); + return result.then( + () => { + this.applying = false; + }, + (e) => { + this.applying = false; + onFailure(); + throw e; + }, + ); } /** Filter entries, keeping `currentIndex` pointing at the same entry it did before. */ diff --git a/src/BloomBrowserUI/bookEdit/undo/UndoStackSpec.ts b/src/BloomBrowserUI/bookEdit/undo/UndoStackSpec.ts index 79b27707c326..91937ca05159 100644 --- a/src/BloomBrowserUI/bookEdit/undo/UndoStackSpec.ts +++ b/src/BloomBrowserUI/bookEdit/undo/UndoStackSpec.ts @@ -312,6 +312,45 @@ describe("UndoStack", () => { expect(stack.peekUndoLabel()).toBe("outer"); }); + it("keeps the outer operation's own entry even when an inner scope pushed first", () => { + // The inner layer usually runs, and records, before the outer operation gets to + // record its own entry. The outer entry describes the whole gesture, so it must win. + stack.beginUndoableScope("outer"); + stack.beginUndoableScope("inner"); + stack.push(makeEntry("inner", log)); + stack.endUndoableScope(); + stack.push(makeEntry("outer", log)); + stack.endUndoableScope(); + + stack.undo(); + expect(log).toEqual(["undo outer"]); + }); + + it("falls back to the first inner entry when the outer scope records nothing itself", () => { + stack.beginUndoableScope("outer wrapper"); + stack.beginUndoableScope("inner a"); + stack.push(makeEntry("a", log)); + stack.endUndoableScope(); + stack.beginUndoableScope("inner b"); + stack.push(makeEntry("b", log)); + stack.endUndoableScope(); + stack.endUndoableScope(); + + expect(stack.getEntryCount()).toBe(1); + expect(stack.peekUndoLabel()).toBe("outer wrapper"); + stack.undo(); + expect(log).toEqual(["undo a"]); + }); + + it("records nothing while the scope is still open", () => { + stack.beginUndoableScope("gesture"); + stack.push(makeEntry("a", log)); + expect(stack.getEntryCount()).toBe(0); + expect(stack.canUndo()).toBe(false); + stack.endUndoableScope(); + expect(stack.getEntryCount()).toBe(1); + }); + it("starts a fresh claim for each new outermost scope", () => { stack.beginUndoableScope("first gesture"); stack.push(makeEntry("a", log)); @@ -338,6 +377,97 @@ describe("UndoStack", () => { }); }); + describe("a failing undo or redo", () => { + function failingEntry( + label: string, + options: { + failUndo?: boolean; + failRedo?: boolean; + failPrepare?: boolean; + }, + ): IUndoEntry { + return { + label, + pageId: "page1", + kind: "custom", + prepareRedo: () => { + if (options.failPrepare) throw new Error("prepare failed"); + }, + undo: () => { + if (options.failUndo) throw new Error("undo failed"); + log.push(`undo ${label}`); + }, + redo: () => { + if (options.failRedo) throw new Error("redo failed"); + log.push(`redo ${label}`); + }, + }; + } + + it("leaves a synchronously failing entry as the next thing to undo", () => { + stack.push(makeEntry("a", log)); + stack.push(failingEntry("b", { failUndo: true })); + expect(stack.peekUndoLabel()).toBe("b"); // sanity + + expect(() => stack.undo()).toThrow("undo failed"); + + expect(stack.peekUndoLabel()).toBe("b"); + expect(stack.canRedo()).toBe(false); + expect(log).toEqual([]); + }); + + it("does the same when prepareRedo is what fails", () => { + stack.push(failingEntry("b", { failPrepare: true })); + expect(() => stack.undo()).toThrow("prepare failed"); + expect(stack.peekUndoLabel()).toBe("b"); + expect(stack.canRedo()).toBe(false); + }); + + it("leaves an asynchronously failing entry as the next thing to undo", async () => { + const entry: IUndoEntry = { + label: "async b", + pageId: "page1", + kind: "custom", + undo: () => Promise.reject(new Error("async undo failed")), + redo: () => {}, + }; + stack.push(makeEntry("a", log)); + stack.push(entry); + + await expect(stack.undo()).rejects.toThrow("async undo failed"); + + expect(stack.peekUndoLabel()).toBe("async b"); + expect(stack.canRedo()).toBe(false); + // The guard is released, so the user can retry; it fails again, harmlessly, and the + // entry is still where it was. + await expect(stack.undo()).rejects.toThrow("async undo failed"); + expect(stack.peekUndoLabel()).toBe("async b"); + }); + + it("leaves a failing redo as the next thing to redo", async () => { + stack.push(failingEntry("b", { failRedo: true })); + stack.undo(); + expect(stack.peekRedoLabel()).toBe("b"); // sanity + + expect(() => stack.redo()).toThrow("redo failed"); + + expect(stack.peekRedoLabel()).toBe("b"); + expect(stack.canUndo()).toBe(false); + + const asyncEntry: IUndoEntry = { + label: "async c", + pageId: "page1", + kind: "custom", + undo: () => {}, + redo: () => Promise.reject(new Error("async redo failed")), + }; + stack.push(asyncEntry); + stack.undo(); + await expect(stack.redo()).rejects.toThrow("async redo failed"); + expect(stack.peekRedoLabel()).toBe("async c"); + }); + }); + describe("asynchronous entries", () => { it("waits for an async undo before allowing another", async () => { let release: () => void = () => { @@ -388,9 +518,13 @@ describe("UndoStack", () => { expect(() => stack.undo()).toThrow("boom"); - // The stack must still work; a broken entry must not disable Undo for the session. + // The guard is released, so the stack still works: new work can be recorded and + // undone. (The broken entry itself stays put as the next thing to undo — see + // "a failing undo or redo" — rather than being skipped.) + stack.push(makeEntry("later", log)); stack.undo(); - expect(log).toEqual(["undo good"]); + expect(log).toEqual(["undo later"]); + expect(stack.peekUndoLabel()).toBe("bad"); }); }); diff --git a/src/BloomBrowserUI/bookEdit/undo/pageFrameUndoHooks.ts b/src/BloomBrowserUI/bookEdit/undo/pageFrameUndoHooks.ts index d4106b3daf45..527693caad89 100644 --- a/src/BloomBrowserUI/bookEdit/undo/pageFrameUndoHooks.ts +++ b/src/BloomBrowserUI/bookEdit/undo/pageFrameUndoHooks.ts @@ -34,13 +34,17 @@ export function pageFrameNavigating(stack: UndoStack = theOneUndoStack): void { } /** - * The page frame has loaded (or, on the 1500 ms fallback in switchContentPage, is assumed to have). - * Records which page entries are now being made against. + * The page frame has loaded. Records which page entries are now being made against. * - * Idempotent for an unchanged id, so being called twice, or late, is harmless. Being called EARLY - * is not quite: an entry pushed before this runs is attributed to whatever id was current, which - * could be the previous page. Nothing pushes automatically yet; this has to be looked at again when - * typing starts recording entries (Stage 3). + * Called from switchContentPage's load handler, and also from a dedicated once-only load listener + * there, because that handler can be run early by a 1500 ms fallback (reading the page that is + * still showing) and then unregistered — without the second listener the id would stay stale for + * the rest of the page. Idempotent for an unchanged id, so running twice is harmless. + * + * Note the id recorded here is only used to notice page CHANGES (`setCurrentPageId` discards + * entries scoped to other pages). Entries carry the page id their pusher gave them, and every + * navigation clears page-scoped entries anyway (`pageFrameNavigating`), so a briefly stale id here + * cannot mis-stamp or wrongly keep an entry. */ export function pageFrameLoaded(stack: UndoStack = theOneUndoStack): void { stack.setCurrentPageId(getCurrentPageIdFromPageFrame()); diff --git a/src/BloomBrowserUI/bookEdit/undo/redoKeyBinding.ts b/src/BloomBrowserUI/bookEdit/undo/redoKeyBinding.ts index 452a269510c8..5808884fffc6 100644 --- a/src/BloomBrowserUI/bookEdit/undo/redoKeyBinding.ts +++ b/src/BloomBrowserUI/bookEdit/undo/redoKeyBinding.ts @@ -15,6 +15,12 @@ // claimed the event AND the shared stack actually has something to redo. When it does not, the // keystroke falls through untouched to whatever would have handled it before this existed. // +// One case defaultPrevented cannot catch: origami's handler runs origamiRedo() without claiming +// the event, so in Change Layout mode a Ctrl+Y that reaches us with something on the shared stack +// would redo twice. We therefore also stand down whenever the page is in Change Layout mode, which +// origami itself signals with the `origami-layout-mode` class on `.marginBox`. This goes away when +// Stage 4 converts origami's undo onto the shared stack and retires its handler. +// // There is no Redo button and no C# involvement: Redo is JS-only by decision (PLAN.md 10). /** The part of the workspace bundle this binding needs. Kept small so a test can fake it. */ @@ -34,6 +40,11 @@ export function isRedoKeystroke(e: KeyboardEvent): boolean { ); } +/** Whether the page is in Change Layout mode, where origami owns Ctrl+Z and Ctrl+Y. */ +export function isInChangeLayoutMode(doc: Document): boolean { + return !!doc.querySelector(".marginBox.origami-layout-mode"); +} + /** * Listen for Ctrl+Y on `doc` and redo through the workspace bundle when it has something to redo. * @@ -54,6 +65,10 @@ export function installRedoKeyBinding( if (e.defaultPrevented) { return; } + // Origami claims Ctrl+Y in Change Layout mode without preventing the default; see above. + if (isInChangeLayoutMode(doc)) { + return; + } const target = getTarget(); if (!target?.canRedo()) { // Nothing of ours to redo: leave the keystroke to CKEditor's redo or the browser's. diff --git a/src/BloomBrowserUI/bookEdit/undo/redoKeyBindingSpec.ts b/src/BloomBrowserUI/bookEdit/undo/redoKeyBindingSpec.ts index c5440fb7f571..76a55dc6b5ea 100644 --- a/src/BloomBrowserUI/bookEdit/undo/redoKeyBindingSpec.ts +++ b/src/BloomBrowserUI/bookEdit/undo/redoKeyBindingSpec.ts @@ -93,6 +93,17 @@ describe("redoKeyBinding", () => { expect(redoCalls).toBe(0); }); + it("stands down in Change Layout mode, where origami redoes without claiming the key", () => { + canRedo = true; + const marginBox = doc.createElement("div"); + marginBox.className = "marginBox origami-layout-mode"; + doc.body.appendChild(marginBox); + const e = keydown({ key: "y", ctrlKey: true }); + editable.dispatchEvent(e); + expect(redoCalls).toBe(0); + expect(e.defaultPrevented).toBe(false); + }); + it("ignores keystrokes that are not Ctrl+Y", () => { canRedo = true; editable.dispatchEvent(keydown({ key: "y" })); diff --git a/src/BloomBrowserUI/bookEdit/undo/runUndoableSpec.ts b/src/BloomBrowserUI/bookEdit/undo/runUndoableSpec.ts index c8efd1ae5a9d..f891f0966bcc 100644 --- a/src/BloomBrowserUI/bookEdit/undo/runUndoableSpec.ts +++ b/src/BloomBrowserUI/bookEdit/undo/runUndoableSpec.ts @@ -32,8 +32,13 @@ describe("runUndoable", () => { runUndoable( "Delete canvas element", () => { - // What deleteCanvasElement's background-image branch does today. - stack.push(makeEntry("image operation", log)); + // An inner layer that records its own undo does so in its own runUndoable, which + // is what lets the outer gesture's entry take precedence over it. + runUndoable( + "image operation", + () => stack.push(makeEntry("image operation", log)), + stack, + ); stack.push(makeEntry("element removal", log)); }, stack, @@ -42,8 +47,9 @@ describe("runUndoable", () => { expect(stack.getEntryCount()).toBe(1); expect(stack.peekUndoLabel()).toBe("Delete canvas element"); stack.undo(); - // The kept entry is the *first* one pushed, relabelled — not a new synthetic entry. - expect(log).toEqual(["undo image operation"]); + // The kept entry is the gesture's own, relabelled — not the inner layer's, and not a new + // synthetic entry. + expect(log).toEqual(["undo element removal"]); }); it("returns the operation's value", () => { diff --git a/src/BloomBrowserUI/bookEdit/workspaceRoot.ts b/src/BloomBrowserUI/bookEdit/workspaceRoot.ts index b2251e22413b..a5358914744e 100644 --- a/src/BloomBrowserUI/bookEdit/workspaceRoot.ts +++ b/src/BloomBrowserUI/bookEdit/workspaceRoot.ts @@ -18,9 +18,18 @@ import { } from "./undo/pageFrameUndoHooks"; // The one undo stack (BL-6681) arbitrates between Bloom's pre-existing undo mechanisms until -// they are converted. Registering them is all it takes; this module is loaded once per edit-tab -// session, and the providers only reach across frames when consulted. -registerLegacyUndoProviders(); +// they are converted. Registering them is all it takes, and the providers only reach across +// frames when consulted. +// +// Only in the top (workspace) frame, though. Vite puts this module in a chunk shared with other +// code, so its top level also runs inside the page and toolbox iframes, each of which would +// otherwise get a live copy of "the one" stack with its own providers. Nothing in those frames may +// use that copy -- the Undo button's page-frame entry point (topBarButtonClick) reaches the real +// stack through getWorkspaceBundleExports() for exactly this reason -- and leaving it unregistered +// makes sure of it: an accidental use would find an empty stack with no providers. +if (window.parent === window) { + registerLegacyUndoProviders(); +} export interface IWorkspaceExports { showDialog( @@ -182,6 +191,11 @@ export function switchContentPage(newSource: string) { }; iframe.removeEventListener("load", handler); iframe.addEventListener("load", handler); + // Separately from the handler above, which the 1500 ms fallback below can run early (against + // the page that is still there) and then unregister: the undo stack must learn the id of the + // page that ACTUALLY loads, so it listens for the real load on its own. Idempotent, so running + // twice when the load does fire in time is harmless. + iframe.addEventListener("load", () => pageFrameLoaded(), { once: true }); iframe.src = newSource; updateWorkspaceUrlParam("pageSrc", newSource); // When we don't already have a video (either a new page, or it has been deleted), From 448e5d035fc6e92e4fd2c8abdeaa1ba2074463e4 Mon Sep 17 00:00:00 2001 From: John Thomson <john_thomson@sil.org> Date: Mon, 7 Sep 2026 11:57:00 -0500 Subject: [PATCH 09/10] Survive a page change during an in-flight undo or an open undoable scope (BL-6681) Two edges Devin's second round found in the transactional index and the held pushes, both about navigation racing an asynchronous operation: - A failing async undo restored the numeric index it had before, but clearPageScopedEntries may have dropped and renumbered entries meanwhile, so the index could point past the end and canUndo would advertise an entry that was not there. The rollback is now by identity: the failed entry becomes the next to undo (or redo) if it still exists; if navigation removed it, the index keepOnly computed is already right. - Pushes held by an open runUndoable scope were not filtered by keepOnly, so an async gesture awaiting across a page change would record its old-page entry when the scope closed. They are now filtered by the same predicate. Two tests pin both. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --- src/BloomBrowserUI/bookEdit/undo/UndoStack.ts | 52 ++++++++++++++----- .../bookEdit/undo/UndoStackSpec.ts | 48 +++++++++++++++++ 2 files changed, 86 insertions(+), 14 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/undo/UndoStack.ts b/src/BloomBrowserUI/bookEdit/undo/UndoStack.ts index 2113f9ed342a..427d062b94b1 100644 --- a/src/BloomBrowserUI/bookEdit/undo/UndoStack.ts +++ b/src/BloomBrowserUI/bookEdit/undo/UndoStack.ts @@ -138,19 +138,16 @@ export class UndoStack { return; } const entry = this.entries[this.currentIndex]; - const indexBefore = this.currentIndex; this.currentIndex--; - // If the entry fails to undo, the index goes back where it was: the failed entry stays the - // next thing to undo (so the user can retry, or see that it is stuck), instead of being - // silently skipped and offered as a Redo of something that never happened. + // If the entry fails to undo, it becomes the next thing to undo again (so the user can + // retry, or see that it is stuck), instead of being silently skipped and offered as a Redo + // of something that never happened. return this.apply( () => { entry.prepareRedo?.(); return entry.undo(); }, - () => { - this.currentIndex = indexBefore; - }, + () => this.makeNextToUndo(entry), ); } @@ -165,18 +162,36 @@ export class UndoStack { return; } const entry = this.entries[this.currentIndex + 1]; - const indexBefore = this.currentIndex; this.currentIndex++; - // As in undo(): a redo that fails leaves the index where it was, so the same entry is - // still the next Redo rather than being treated as done. + // As in undo(): a redo that fails is still the next Redo rather than being treated as done. return this.apply( () => entry.redo!(), - () => { - this.currentIndex = indexBefore; - }, + () => this.makeNextToRedo(entry), ); } + /** + * After a failed undo, point the index back at `entry` — by identity, not by the number it had + * before. An asynchronous undo can be in flight while the page changes, and `keepOnly` may have + * dropped entries (including this one) and renumbered the rest meanwhile; restoring the old + * number would then point past the end, and `canUndo` would advertise an entry that is not + * there. If the entry is gone, the index `keepOnly` computed is already right. + */ + private makeNextToUndo(entry: IUndoEntry): void { + const i = this.entries.indexOf(entry); + if (i >= 0) { + this.currentIndex = i; + } + } + + /** The redo counterpart of {@link makeNextToUndo}. */ + private makeNextToRedo(entry: IUndoEntry): void { + const i = this.entries.indexOf(entry); + if (i >= 0) { + this.currentIndex = i - 1; + } + } + /** * Note which page we are on, discarding entries that belonged to a previous one. * @@ -310,8 +325,17 @@ export class UndoStack { ); } - /** Filter entries, keeping `currentIndex` pointing at the same entry it did before. */ + /** + * Filter entries, keeping `currentIndex` pointing at the same entry it did before. + * + * Pushes held by an open `runUndoable` scope are filtered too: an asynchronous gesture can be + * awaiting while the page changes, and without this its held entry, scoped to the page just + * left, would be recorded when the scope closes and later undone against the new page. + */ private keepOnly(predicate: (entry: IUndoEntry) => boolean): void { + this.heldPushes = this.heldPushes.filter((held) => + predicate(held.entry), + ); const kept: IUndoEntry[] = []; let newIndex = -1; for (let i = 0; i < this.entries.length; i++) { diff --git a/src/BloomBrowserUI/bookEdit/undo/UndoStackSpec.ts b/src/BloomBrowserUI/bookEdit/undo/UndoStackSpec.ts index 91937ca05159..8200451f888c 100644 --- a/src/BloomBrowserUI/bookEdit/undo/UndoStackSpec.ts +++ b/src/BloomBrowserUI/bookEdit/undo/UndoStackSpec.ts @@ -468,6 +468,54 @@ describe("UndoStack", () => { }); }); + describe("a page change during an in-flight or held operation", () => { + it("does not leave a dangling index when the failing entry was cleared meanwhile", async () => { + let reject: (e: Error) => void = () => { + throw new Error("test bug: reject called before it was set"); + }; + const slowFailing: IUndoEntry = { + label: "slow failing", + pageId: "page1", + kind: "custom", + undo: () => + new Promise<void>((_, rej) => { + reject = rej; + }), + redo: () => {}, + }; + stack.setCurrentPageId("page1"); + stack.push(makeEntry("a", log)); + stack.push(slowFailing); + + const pending = stack.undo() as Promise<void>; + // The user changes page while the undo is still in flight. + stack.clearPageScopedEntries(); + expect(stack.getEntryCount()).toBe(0); // sanity + reject(new Error("too late")); + await expect(pending).rejects.toThrow("too late"); + + // A numeric rollback would have pointed past the end here. + expect(stack.canUndo()).toBe(false); + expect(stack.peekUndoLabel()).toBeUndefined(); + expect(() => stack.undo()).not.toThrow(); + }); + + it("drops a held page-scoped push when the page is left before the scope closes", () => { + stack.setCurrentPageId("page1"); + stack.beginUndoableScope("async gesture"); + stack.push(makeEntry("old page work", log)); + stack.push(makeEntry("delete page", log, { pageId: undefined })); + // Navigation happens while the gesture is still awaiting. + stack.clearPageScopedEntries(); + stack.endUndoableScope(); + + // The page-scoped entry is gone; the one that survives page changes was kept. + expect(stack.getEntryCount()).toBe(1); + stack.undo(); + expect(log).toEqual(["undo delete page"]); + }); + }); + describe("asynchronous entries", () => { it("waits for an async undo before allowing another", async () => { let release: () => void = () => { From ecff075476f025dde31c97cb2a36592bbcbe8b62 Mon Sep 17 00:00:00 2001 From: John Thomson <john_thomson@sil.org> Date: Mon, 7 Sep 2026 12:03:12 -0500 Subject: [PATCH 10/10] Never record an entry for a page that is no longer current (BL-6681) Devin's third round: a push that arrives AFTER a page change -- an asynchronous gesture on the old page finishing late -- misses keepOnly, which ran at navigation time, and would be recorded and later undone against the new page. record() now refuses an entry scoped to any page but the current one; entries with no page id (deleting a page) are unaffected. A test drives the late push both bare and inside a scope. Also logs the second round's two fixes in PROGRESS.md. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --- docs/retire-ckeditor/PROGRESS.md | 4 +++ src/BloomBrowserUI/bookEdit/undo/UndoStack.ts | 16 +++++++++++- .../bookEdit/undo/UndoStackSpec.ts | 25 +++++++++++++++++++ 3 files changed, 44 insertions(+), 1 deletion(-) diff --git a/docs/retire-ckeditor/PROGRESS.md b/docs/retire-ckeditor/PROGRESS.md index 20b33167a473..47b71cea2ad7 100644 --- a/docs/retire-ckeditor/PROGRESS.md +++ b/docs/retire-ckeditor/PROGRESS.md @@ -1023,6 +1023,10 @@ beyond this PR: *and* ours once the stack held anything (the binding now stands down when `.marginBox.origami-layout-mode` is present, until Stage 4 retires origami's handler). Also a dedicated once-only `load` listener records the page id even when the 1500 ms fallback ran first. + Devin's second round added two navigation races on top of the first fix, both real and fixed: + the failure rollback now restores by entry identity (a page change during an in-flight async + undo may have dropped and renumbered entries), and `keepOnly` also filters the pushes held by + an open `runUndoable` scope. **Observed, not chased — the reader-tools undo arms itself in books without a reader tool.** In "A house for mouse" (Basic Book, toolbox shows only Canvas/Talking Book/Settings), after this session diff --git a/src/BloomBrowserUI/bookEdit/undo/UndoStack.ts b/src/BloomBrowserUI/bookEdit/undo/UndoStack.ts index 427d062b94b1..361668464087 100644 --- a/src/BloomBrowserUI/bookEdit/undo/UndoStack.ts +++ b/src/BloomBrowserUI/bookEdit/undo/UndoStack.ts @@ -76,8 +76,22 @@ export class UndoStack { this.record(entry); } - /** Actually add an entry, truncating any redo branch. */ + /** + * Actually add an entry, truncating any redo branch. + * + * An entry scoped to a page other than the current one is dropped instead. That is the last + * line of defence for a push that arrives *after* a page change — an asynchronous gesture on + * the old page finishing late — which `keepOnly` (run at navigation time) could not have seen. + * Undoing such an entry would apply the old page's data to whatever page is showing now. + */ private record(entry: IUndoEntry): void { + if ( + entry.pageId !== undefined && + this.currentPageId !== undefined && + entry.pageId !== this.currentPageId + ) { + return; + } // Anything the user had undone is now unreachable: they have taken a different branch. this.entries.length = this.currentIndex + 1; diff --git a/src/BloomBrowserUI/bookEdit/undo/UndoStackSpec.ts b/src/BloomBrowserUI/bookEdit/undo/UndoStackSpec.ts index 8200451f888c..83b4ae14a7a2 100644 --- a/src/BloomBrowserUI/bookEdit/undo/UndoStackSpec.ts +++ b/src/BloomBrowserUI/bookEdit/undo/UndoStackSpec.ts @@ -500,6 +500,31 @@ describe("UndoStack", () => { expect(() => stack.undo()).not.toThrow(); }); + it("refuses an entry scoped to a page that is no longer current", () => { + // The push that arrives AFTER navigation, from an async gesture finishing late on the + // old page: keepOnly has already run, so record() itself has to turn it away. + stack.setCurrentPageId("page1"); + stack.push(makeEntry("on page 1", log)); + stack.setCurrentPageId("page2"); + expect(stack.getEntryCount()).toBe(0); // sanity: the page-1 entry went with the page + + stack.push( + makeEntry("late, from page 1", log, { pageId: "page1" }), + ); + expect(stack.getEntryCount()).toBe(0); + + // A late push inside a scope is turned away the same way when the scope closes... + stack.beginUndoableScope("late gesture"); + stack.push(makeEntry("late and scoped", log, { pageId: "page1" })); + stack.endUndoableScope(); + expect(stack.getEntryCount()).toBe(0); + + // ...while entries for the current page, and ones that survive page changes, record. + stack.push(makeEntry("on page 2", log, { pageId: "page2" })); + stack.push(makeEntry("delete page", log, { pageId: undefined })); + expect(stack.getEntryCount()).toBe(2); + }); + it("drops a held page-scoped push when the page is left before the scope closes", () => { stack.setCurrentPageId("page1"); stack.beginUndoableScope("async gesture");