Inventory the behaviours, pin the tests, and give caret preservation a seam (BL-6681)

.claude/skills/resume-ckeditor/SKILL.md

@@ -0,0 +1,51 @@
+---
+name: resume-ckeditor
+description: Resume the long-running "retire CKEditor / unify Undo" project (BL-6681). Use when the user says "/resume-ckeditor", "resume the ckeditor work", "continue retiring ckeditor", or picks that project back up after an interruption (sleep, token exhaustion, days off).
+---
+
+# 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.
+
+## 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.
+
+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.
+
+## 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.
+- 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.

docs/retire-ckeditor/REVIEW-NOTES.md

@@ -0,0 +1,98 @@
+# Review notes on the CKEditor-retirement plan
+
+Round 1 review by Fable (Claude), 2026-08-04, against the first draft of [PLAN.md](PLAN.md).
+Every finding below was independently verified against the source before being accepted or
+rejected. This file exists so a later session knows *why* the plan says what it says, and
+doesn't re-litigate settled points.
+
+## Accepted — factual corrections to the draft
+
+| Finding | Verification | Where it landed |
+| --- | --- | --- |
+| The draft said `Equation-style` / ArithmeticTemplate boxes get no editor. They **do**: `utils/shared.ts:16-19` includes `.Equation-style[contenteditable='true']` in `ckeditableSelector`, added for that template. The real no-editor case is `attachToCkEditor`'s `cursor: not-allowed` early return (`bloomEditing.ts:1952`). The comment at `toolbox.ts:1530-1537` claiming otherwise is stale. | Confirmed in `shared.ts` | §2 "dead or misleading code", Stage 0 inventory |
+| The draft called the toolbox undo "reader-setup changes". It is really a **per-editable text-typing** undo: `{html, text, caretOffset}` seeded on focus (`readerToolsModel.ts:557-568`) and pushed inside `doMarkup` (:753-764), gated on `shouldHandleUndo()` (:570). | Confirmed by reading both ranges | §3 table |
+| Consequently `handleUndo` consulting toolbox **before** CKEditor is deliberate, not arbitrary — when a reader tool is active it must shadow CKEditor's undo, which would restore stale markup. | Follows from the above | §3, Stage 1 rationale |
+| Origami's undo is a jQuery `clone(true)` — DOM *plus handlers and data* — not an innerHTML snapshot, and it is safe partly because layout mode strips `contentEditable` (`origami.ts:132`). So it is not a drop-in precedent for innerHTML restore. | Confirmed at `origami.ts:262-294`, :132 | §3 table, Risk 2, Stage 4 |
+| `deleteCanvasElement`'s background-image branch already records an image undo (`CanvasElementManager.ts:2755-2770`), so wrapping the delete would double-record. | Confirmed | §4.8, Stage 2b |
+| Support-file cleanup runs only from `Book.BringBookUpToDate` (`Book.cs:1112`) and publish/upload paths (`BookStorage.cs:2623-2632`), **not** on page save — so the audio-file risk is much smaller than the draft assumed. | Confirmed by tracing `CleanupUnusedSupportFiles` call sites | Risk 7 (downgraded and scoped) |
+| The cross-frame export is `getWorkspaceBundleExports`, not `getWorkspaceExports`. | Confirmed (`origami.ts:204`) | §4.2 |
+| `ISelectionAnchor` keyed on `editableId` won't work: ordinary `.bloom-editable` divs have no `id` (only talking-book assigns them, `audioRecording.ts:1380, 3681`). | Confirmed | §4.3 — structural locator instead |
+| The draft overstated the existing selection machinery. `makeSelectionIn` is the *consume* side; `getElementSelectionIndex` returns only an offset and in-tree callers pass `divBrCount = -1`. The capture side is new code. | Confirmed (`editableDivUtils.ts:30-46`, `readerToolsModel.ts:587-592`) | §4.3 |
+
+## Accepted — design changes
+
+1. **Undo entries must be data, not closures.** The page iframe's JS context dies on same-page
+   reloads too (ctrl+wheel zoom `bloomEditing.ts:1268`, origami exit `origami.ts:193`), so
+   page-id-scoped clearing alone leaves entries closing over a dead document. Snapshot entries
+   are now pure data interpreted at undo time; closures are permitted only for workspace-owned
+   operations; clearing also keys on page-frame unload/load. → §4.1
+2. **Native browser undo must be actively fenced.** Plain typing feeds Chromium's undo stack and
+   Ctrl+Z inside an editable triggers it; today CKEditor intercepts that. The new editor must
+   `preventDefault()` on `beforeinput` with `inputType` `historyUndo`/`historyRedo`. This is
+   also the replacement for `BloomField.PreventRemovalOfSomeElements`'s `execCommand("undo")`
+   (`BloomField.ts:810-825`) — block the deletion rather than undoing it. → §4.4, Risk 3
+3. **Re-cut Stages 1–2.** The draft spent Stage 1 converting three mechanisms that already work
+   (delivering nothing the user asked for) while forcing in-place snapshot restore to mature on a
+   page full of live CKEditor instances. The decisive detail: restoring innerHTML orphans
+   `div.bloomCkEditor` (`BloomField.ts:419`), and because `doCkEditorCleanup` iterates that
+   expando (`editableDivUtils.ts:350`) and `getBodyContentForSavePage` calls it
+   (`bloomEditing.ts:1483`), the **save path would silently skip cleanup** for restored divs.
+   So: Stage 1 now wraps all four mechanisms as legacy providers with no conversions; delete-page
+   and delete-canvas-element move to Stage 2; `PageSnapshot` lands in Stage 3; conversions become
+   optional Stage 4 cleanups.
+4. **Delete canvas element uses an inverse-op / narrow subtree, not a page snapshot**, reusing
+   the existing `refreshCanvasElementEditing` path. Two things flagged to verify first: Comical
+   bubble-family re-linking, and restoring a drag-activity target. → Stage 2b
+5. **`runUndoable` needs nesting semantics from day one** (depth counter, outermost wins). → §4.8
+6. **Delete-page capture must happen inside the `SaveThen` callback** (`EditingModel.cs:590-624`),
+   and restore must re-raise `_pageListChangedEvent` / `InvokeContentsChanged` and navigate, not
+   just renumber. The front-end entry is pushed by C#, since the page frame is being torn down.
+   → Stage 2a
+7. **The toolbox keystroke pipeline is the one place "one-line dispatch" fails**
+   (`toolbox.ts:1509-1607`, ~100 lines of the app's most delicate code). Added a mechanical
+   behaviour-preserving prep commit in Stage 0 that extracts the save/restore-selection bracket,
+   so the eventual change swaps a function body. → §5.4
+8. **The flag is a `localStorage` dev switch.** A URL parameter is not zero-touch: the page
+   iframe's `src` comes from C# via `switchContentPage`. → §5.5
+9. **`canUndo()` must stay synchronous and O(1)** — C# polls it on a timer with a reentrancy
+   guard (`WebView2Browser.cs:963-996`); expensive work there makes the button flicker. → §4.1
+
+## Accepted — missed dependencies now in scope
+
+- `WireToCKEditor` services the draft omitted: the BL-13779 `change`-event `data-user-deleted`
+  tracking (`BloomField.ts:244-252`); BL-11745 qtip z-order juggling on focus/blur (:345-366);
+  `selectionChange` → `EnsureCaretNotInsideLineBreakSpan` (:259-261); the SetupLink hyperlink
+  command and button (:368-415). → §2 table rows 8–11, Stage 3 `BloomTextEditor.ts` /
+  `FormatToolbar.tsx`
+- **BL-12357 small caps cannot be ported unchanged.** `restoreHtmlMarkupIfNecessary`
+  (`BloomField.ts:425-456`) detects CKEditor-internal copies via `dataTransfer.getData("cke/id")`
+  and compensates for CKEditor's own span-stripping. With CKEditor gone, nothing stamps `cke/id`
+  and nothing strips the spans, so the transform is meaningless as written and the problem may
+  vanish. Verify against the repro. → Stage 3
+- **The second paste path**: `pasteImpl`'s replace-whole-content branch for a canvas element that
+  is selected but not being text-edited (`bloomEditing.ts:1792-1842`), plus the C#-initiated
+  `pasteClipboard` entry point as distinct from the DOM `paste` event. → Stage 3 `pasteHandler.ts`
+- **jsdom cannot test the `beforeinput` layer** — no native editing behaviour, no
+  `getTargetRanges()`. `typingTransactions.ts` and `keyCommands.ts` need the live-WebView2 CDP
+  harness. → §7
+- `readerSetup.ui.ts:454`'s `execCommand` is in the reader-setup dialog, not the page frame —
+  scoped out. → Stage 3 `keyCommands.ts`
+
+## Accepted — cuts
+
+- **Stage 1c (convert reader-tools undo) deleted.** It is a text-typing undo, so converting it
+  would require typing transactions to exist first — out of order. It becomes a legacy provider
+  and is deleted in Stage 4/5 when shared-stack snapshots subsume it.
+- **Byte-budget accounting on the stack** — dropped; a count cap (~50) suffices.
+- **`ISelectionAnchor.focusOffset` / range support** — deferred; every identified consumer needs
+  only a caret. Added the reviewer's neat alternative for the snapshot case: inject a caret
+  marker into the captured HTML *string*, which has none of the bookmark-span downsides.
+- **Open question "delete-page depth"** — answered in the plan rather than asked: keep every
+  deletion in the session, capped ~10.
+
+## Confirmed as already correct
+
+The Stage 0 behaviour inventory (which would itself have caught the `Equation-style` error), the
+dev harness page, doing `inlineFormat` first, consolidating the C# scrubbers into
+`LegacyCkEditorCleanup` rather than deleting them, and keeping Stage 6 as a separately-reviewed
+pass.

docs/retire-ckeditor/verifyCaretPreservation.mjs

@@ -0,0 +1,183 @@
+// BL-6681 Stage 0 live-verification harness: does the caret survive the markup pipeline?
+// Inventory rows G1/G2 (docs/retire-ckeditor/BEHAVIOR-INVENTORY.md).
+//
+// USAGE (needs a running Bloom on the Edit tab with a text box on the current page):
+//   node .github/skills/bloom-automation/launcherControl.mjs --ensure-running --wait-ready --json
+//   node docs/retire-ckeditor/verifyCaretPreservation.mjs <cdpPort>
+//
+// WHAT IT COVERS. In toolbox.ts's handleKeyboardInput, the
+// saveSelectionForMarkup / restoreSelectionAfterMarkup bracket runs on EVERY keystroke in an
+// editable (after the 500ms debounce), regardless of whether a tool is active — the tool check
+// only gates the updateMarkup call between them. So typing mid-word and checking the caret
+// afterwards exercises the wiring of the seam extracted in commit 2707d98a8, and confirms the
+// bookmark spans are consumed rather than left in the DOM.
+//
+// WHAT IT DOES *NOT* COVER, and how to extend it. With no tool active, updateMarkup never runs,
+// so the DOM is unchanged between save and restore — which is the easy half. The case bookmarks
+// actually exist for is markup rewriting the DOM around the caret. To reach it you need a book
+// whose toolbox offers a reader tool: a **Decodable Reader** or **Leveled Reader** book, not a
+// Basic Book. In a Basic Book the toolbox offers only Talking Book and "More...", both of which
+// stayed invisible even after toggling #pure-toggle-right, and no audio-sentence spans appeared,
+// so updateMarkup did not run. Also remember toolbox.toolboxIsShowing() gates markup, so the
+// toolbox pane has to be genuinely open, not merely toggled.
+import { createRequire } from "node:module";
+import path from "node:path";
+import url from "node:url";
+
+// Derive the repo root from this file's own location (docs/retire-ckeditor/), so the harness works
+// from any checkout or worktree rather than only the one it was written in.
+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");
+
+const cdpPort = process.argv[2] || "8091";
+const browser = await chromium.connectOverCDP(`http://localhost:${cdpPort}`);
+
+const mainPage = (() => {
+    for (const c of browser.contexts())
+        for (const p of c.pages())
+            if (
+                p.url().includes("/bloom/") &&
+                !p.url().includes("toolboxcontent")
+            )
+                return p;
+    return undefined;
+})();
+if (!mainPage) throw new Error("no Bloom main page");
+
+const pageFrame = mainPage.frames().find((f) => f.name() === "page");
+if (!pageFrame) {
+    console.log(
+        "FRAMES:",
+        mainPage.frames().map((f) => `${f.name()}|${f.url().slice(0, 80)}`),
+    );
+    throw new Error("no 'page' frame");
+}
+console.log("page frame url:", pageFrame.url().slice(0, 90));
+
+// Pick the first visible bloom-editable we can type into.
+const info = await pageFrame.evaluate(() => {
+    const eds = Array.from(
+        document.querySelectorAll(
+            ".bloom-editable.bloom-visibility-code-on[contenteditable='true']",
+        ),
+    );
+    return eds.map((e, i) => ({
+        i,
+        lang: e.getAttribute("lang"),
+        cls: (e.className || "")
+            .split(" ")
+            .filter((c) => c.startsWith("bloom-content"))
+            .join(","),
+        text: (e.textContent || "").trim().slice(0, 40),
+        hasCkEditor: !!e.bloomCkEditor,
+    }));
+});
+console.log("editables:", JSON.stringify(info, null, 1));
+
+const target = info.find((e) => e.hasCkEditor);
+if (!target) {
+    console.log(
+        "RESULT: no editable has a CKEditor attached — cannot exercise the pipeline here",
+    );
+    await browser.close();
+    process.exit(3);
+}
+console.log(`using editable index ${target.i} (lang=${target.lang})`);
+
+const sel = `.bloom-editable.bloom-visibility-code-on[contenteditable='true']`;
+
+// --- Step 1: put a known word in the box -------------------------------------------------
+await pageFrame.evaluate(
+    ({ s, idx }) => {
+        const ed = document.querySelectorAll(s)[idx];
+        const p =
+            ed.querySelector("p") ||
+            ed.appendChild(document.createElement("p"));
+        p.textContent = "house";
+    },
+    { s: sel, idx: target.i },
+);
+await mainPage.waitForTimeout(300);
+
+// --- Step 2: click in, then place the caret between "hous" and "e" (offset 4) ------------
+await pageFrame.locator(sel).nth(target.i).click();
+await mainPage.waitForTimeout(400);
+
+const placed = await pageFrame.evaluate(
+    ({ s, idx }) => {
+        const ed = document.querySelectorAll(s)[idx];
+        const textNode = ed.querySelector("p").firstChild;
+        const r = document.createRange();
+        r.setStart(textNode, 4);
+        r.setEnd(textNode, 4);
+        const g = window.getSelection();
+        g.removeAllRanges();
+        g.addRange(r);
+        return { text: ed.textContent, anchorOffset: g.anchorOffset };
+    },
+    { s: sel, idx: target.i },
+);
+console.log("caret placed:", JSON.stringify(placed));
+if (placed.anchorOffset !== 4) {
+    console.log("RESULT: FAILED SETUP — could not place caret at offset 4");
+    await browser.close();
+    process.exit(4);
+}
+
+// --- Step 3: type a character mid-word, then wait out the 500ms markup debounce ----------
+await mainPage.keyboard.type("z");
+console.log("typed 'z' at offset 4; waiting 1500ms for the markup pass...");
+await mainPage.waitForTimeout(1500);
+
+// --- Step 4: observe where the caret ended up and what the text is -----------------------
+const after = await pageFrame.evaluate(
+    ({ s, idx }) => {
+        const ed = document.querySelectorAll(s)[idx];
+        const g = window.getSelection();
+        // Character offset of the caret within the editable, counting text only.
+        let offset = -1;
+        if (g && g.anchorNode && ed.contains(g.anchorNode)) {
+            const r = g.getRangeAt(0).cloneRange();
+            r.setStart(ed, 0);
+            offset = r.toString().length;
+        }
+        return {
+            text: ed.textContent,
+            html: ed.innerHTML,
+            caretCharOffset: offset,
+            anchorIsInsideEditable: !!(
+                g &&
+                g.anchorNode &&
+                ed.contains(g.anchorNode)
+            ),
+            leftoverBookmarkSpans:
+                ed.querySelectorAll("[id^='cke_bm_']").length,
+            zeroWidthSpaces: (ed.innerHTML.match(/\u200B/g) || []).length,
+        };
+    },
+    { s: sel, idx: target.i },
+);
+console.log("AFTER:", JSON.stringify(after, null, 1));
+
+// Caret at offset 4 in "house" sits between "hous" and "e", so typing "z" gives "housze"
+// and the caret should end up at offset 5, immediately after the character just typed.
+const pass =
+    after.text === "housze" &&
+    after.caretCharOffset === 5 &&
+    after.leftoverBookmarkSpans === 0 &&
+    after.zeroWidthSpaces === 0;
+console.log(
+    pass
+        ? "RESULT: PASS — text 'housze', caret at offset 5, no leftover bookmark spans, no ZWSP"
+        : `RESULT: FAIL — expected text 'housze' caret 5 bookmarks 0 zwsp 0; got text '${after.text}' caret ${after.caretCharOffset} bookmarks ${after.leftoverBookmarkSpans} zwsp ${after.zeroWidthSpaces}`,
+);
+
+await browser.close();