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/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 **`
`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 **`

After

+``` + +**After paste** (event was handled (defaultPrevented)) + +```html +

Start Before

After

end

+``` + +**After drop** (event was handled (defaultPrevented)) + +```html +

StBefore

After

art end

+``` + +## C4 — an inline image + +**Input HTML** + +```html +

Picture: alt text end

+``` + +**After paste** (event was handled (defaultPrevented)) + +```html +

Start Picture: end

end

+``` + +**After drop** (event was handled (defaultPrevented)) + +```html +

StPicture: end

art end

+``` + +## C5 — styled span soup from a web page + +**Input HTML** + +```html +

Soup plain span underlined

+``` + +**After paste** (event was handled (defaultPrevented)) + +```html +

Start Soup plain span underlined

end

+``` + +**After drop** (event was handled (defaultPrevented)) + +```html +

StSoup plain span underlined

art end

+``` + +## C6 — a link with extra attributes + +**Input HTML** + +```html +

See this link.

+``` + +**After paste** (event was handled (defaultPrevented)) + +```html +

Start See this link.

end

+``` + +**After drop** (event was handled (defaultPrevented)) + +```html +

StSee this link.

art end

+``` + +## C-mixed — a realistic web-page fragment (heading, list, bold/italic, sup) + +**Input HTML** + +```html +

Heading

  • One two
  • Three

H2O and E=mc2, under, struck, code.

+``` + +**After paste** (event was handled (defaultPrevented)) + +```html +

Start Heading

One two

Three

H2O and E=mc2, under, struck, code.

end

+``` + +**After drop** (event was handled (defaultPrevented)) + +```html +

StHeading

One two

Three

H2O and E=mc2, under, struck, code.

art end

+``` + +## 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 `

`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 `` and + ``. 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 ``, an `` and a `

` 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 + `` 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/PLAN.md b/docs/retire-ckeditor/PLAN.md index 36d5fca5af02..87d160449efb 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"` @@ -142,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()`: @@ -733,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 @@ -942,13 +975,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..47b71cea2ad7 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,20 +33,36 @@ 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. +> ## ⚠ `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 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`. | 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. 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): | 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 [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. @@ -47,14 +80,19 @@ 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 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 @@ -417,6 +455,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 +669,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` @@ -698,29 +838,275 @@ 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. + +**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. + +**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 `` — 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. -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. - -Then: open the Stage 0 PR via `preflight`. +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 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 + +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 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: 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~~ — 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. +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` + +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 + +- **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): + +- **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 +1115,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. diff --git a/docs/retire-ckeditor/liveChecks/README.md b/docs/retire-ckeditor/liveChecks/README.md new file mode 100644 index 000000000000..fbaba303d43f --- /dev/null +++ b/docs/retire-ckeditor/liveChecks/README.md @@ -0,0 +1,52 @@ +# 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 | +| `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 ""` 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 +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/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/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 <book title>"); +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/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/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(); 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..fd2e08c43d1a --- /dev/null +++ b/docs/retire-ckeditor/liveChecks/verifyCommon.mjs @@ -0,0 +1,186 @@ +// 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"); + }; + // 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(() => { + const tool = window.toolboxBundle + .getTheOneToolbox() + .getCurrentTool(); + return tool ? tool.id() : undefined; + }); + // 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 + .getEditablePageBundleExports() + .topBarButtonClick({ command: "undo" }), + ); + 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(); 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/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 new file mode 100644 index 000000000000..361668464087 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/undo/UndoStack.ts @@ -0,0 +1,373 @@ +// 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 is held until the outermost scope closes. See {@link push}. + */ + private openScopeLabels: string[] = []; + + /** + * 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; + + /** + * 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 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) { + this.heldPushes.push({ entry, depth: this.openScopeLabels.length }); + return; + } + this.record(entry); + } + + /** + * 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; + + 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<void> { + 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--; + // 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.makeNextToUndo(entry), + ); + } + + /** + * 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<void> { + if (this.applying || !this.canRedo()) { + return; + } + const entry = this.entries[this.currentIndex + 1]; + this.currentIndex++; + // As in undo(): a redo that fails is still the next Redo rather than being treated as done. + return this.apply( + () => entry.redo!(), + () => 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. + * + * 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 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); + } + + /** 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.heldPushes = []; + } + this.openScopeLabels.push(label); + } + + /** + * 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. */ + public isInUndoableScope(): boolean { + return this.openScopeLabels.length > 0; + } + + /** + * 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.then( + () => { + this.applying = false; + }, + (e) => { + this.applying = false; + onFailure(); + throw e; + }, + ); + } + + /** + * 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++) { + 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..83b4ae14a7a2 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/undo/UndoStackSpec.ts @@ -0,0 +1,617 @@ +// 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("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)); + 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("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("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("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"); + 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 = () => { + throw new Error("test bug: release called before it was set"); + }; + const slow: IUndoEntry = { + label: "slow", + pageId: "page1", + kind: "custom", + undo: () => + new Promise<void>((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 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 later"]); + expect(stack.peekUndoLabel()).toBe("bad"); + }); + }); + + 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..4353a6091380 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/undo/legacyUndoProviders.ts @@ -0,0 +1,132 @@ +// 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: () => { + 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(); + }, +}; + +/** + * `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(); + // 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(); + }, +}; + +/** + * 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/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..527693caad89 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/undo/pageFrameUndoHooks.ts @@ -0,0 +1,51 @@ +// 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. Records which page entries are now being made against. + * + * 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/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..5808884fffc6 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/undo/redoKeyBinding.ts @@ -0,0 +1,80 @@ +// 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. +// +// 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. */ +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") + ); +} + +/** 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. + * + * `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; + } + // 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. + 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..76a55dc6b5ea --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/undo/redoKeyBindingSpec.ts @@ -0,0 +1,122 @@ +// 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("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" })); + 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/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<T>( + 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<unknown> | 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..f891f0966bcc --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/undo/runUndoableSpec.ts @@ -0,0 +1,149 @@ +// 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", + () => { + // 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, + ); + + expect(stack.getEntryCount()).toBe(1); + expect(stack.peekUndoLabel()).toBe("Delete canvas element"); + stack.undo(); + // 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", () => { + 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<void>((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..b5c944ddf310 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/undo/undoTypes.ts @@ -0,0 +1,108 @@ +// 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* — 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* + * `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<void>; + + /** + * 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<void>; + + /** + * 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; diff --git a/src/BloomBrowserUI/bookEdit/workspaceRoot.ts b/src/BloomBrowserUI/bookEdit/workspaceRoot.ts index c864174008ae..a5358914744e 100644 --- a/src/BloomBrowserUI/bookEdit/workspaceRoot.ts +++ b/src/BloomBrowserUI/bookEdit/workspaceRoot.ts @@ -10,6 +10,26 @@ 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, 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( @@ -53,6 +73,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 +124,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 +150,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,12 +184,18 @@ export function switchContentPage(newSource: string) { const handler = () => { handlerCalled = true; iframe.removeEventListener("load", handler); + pageFrameLoaded(); doWhenToolboxLoaded((toolboxFrameExports: IToolboxFrameExports) => { toolboxFrameExports.applyToolboxStateToPage(); }); }; 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), @@ -262,24 +271,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 +418,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 +464,8 @@ window.workspaceBundle = { // simple exports SayHello, handleUndo, + handleRedo, + canRedo, switchThumbnailPage, switchContentPage, showDialog,