Inventory the behaviours, pin the tests, and give caret preservation a seam (BL-6681) - #8153
Open
JohnThomson wants to merge 15 commits into
Open
Inventory the behaviours, pin the tests, and give caret preservation a seam (BL-6681)#8153JohnThomson wants to merge 15 commits into
JohnThomson wants to merge 15 commits into
Conversation
Planning-only commit: no production code changes. Adds the design documents for a long-running project, plus a skill to resume it across sessions. CKEditor 4 (a 2015-era, hand-patched, 1.5 MB vendored copy) is to be replaced by our own code, and Bloom's five poorly-coordinated Undo mechanisms unified into a single ordered stack. Because this will run for many months against a moving master, the plan is deliberately structured to minimise rebase pain: new code in new directories, one-line dispatches into existing files, a dozen small flag-inert PRs rather than one long-lived branch, and deletion commits that are regenerated rather than rebased. docs/retire-ckeditor/ PLAN.md the staged plan (Stages 0-6) and the design decisions REVIEW-NOTES.md review findings already verified, so they aren't relitigated PROGRESS.md live state, so an interrupted session can resume Undo comes first (Stages 1-2) and is independently shippable, so Bloom benefits even if the CKEditor work stalls. Notably, the plan records several things the code currently gets wrong or documents inaccurately: the BL-3125 .bloom-canvas guard in bootstrap() is dead code, the comment claiming ArithmeticTemplate boxes get no editor is contradicted by ckeditableSelector, workspaceRoot's reference to a C# Browser.Undo fallback is stale, and undoStackSize = 0 silently means 20. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Stage 0 of the CKEditor retirement. Documentation only.
BEHAVIOR-INVENTORY.md is the acceptance criteria for the whole project:
sections A-K covering the selection toolbar, formatting commands, paste/drop
filtering, paste transforms, clipboard plumbing, clean-HTML extraction,
selection survival across DOM rewrites, undo/redo, startup races, which fields
get an editor at all, and incidental focus/bubble wiring. Each row cites where
the behaviour lives today and, where the code says so, the ticket behind it.
Scoped deliberately to behaviours *at risk*: those implemented by CKEditor or
in code this project will move. BloomField.ManageField's CKEditor-independent
behaviours stay where they are and are excluded, so the rows that matter aren't
diluted.
Two kinds of row earn a marker. "capture first" rows (all of section C) are
guarantees whose failure is silent, so today's real behaviour must be recorded
before any code changes -- the config string alone doesn't tell us what happens
on a drop, or on a paste from a real web page. "must NOT survive" rows are
current behaviour we intend to remove, listed so nobody faithfully reimplements
a workaround for a problem that will no longer exist.
Four behaviours need a new implementation rather than a port, and the inventory
says why for each: the BL-12357 small-caps transform depends on CKEditor
stamping "cke/id" on its own copies; bloom-preventRemoval currently allows a
deletion and then calls execCommand("undo"), the browser stack we intend to
fence off; the colour panel is superseded by Bloom's own colour dialog; and
caret survival during reader markup should get strictly better, since offset
anchors don't perturb the DOM the way bookmarks do.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…BL-6681)
Stage 0 characterization tests, inventory rows G4/G5. New file only.
EditableDivUtils.makeSelectionIn and getElementSelectionIndex predate CKEditor,
and the retirement plan builds its bookmark-free selection anchors on them
(PLAN.md 4.3). Two reasons to pin them before that work starts:
- makeSelectionIn's divBrCount parameter, which disambiguates a caret position
around <br> elements, has no in-tree caller that passes anything but -1
(readerToolsModel and toolbox both do). So the exact behaviour the new anchors
will depend on is currently exercised by nothing at all.
- getElementSelectionIndex produces only a plain character offset, so the
round-trip established here is the contract the new capture side must preserve.
The tests also record, by needing it, that both functions hard-code
parent.window.document.getElementById("page") and operate on that iframe's
window. That is why the harness has to build such an iframe. The replacement
API should instead take a document/root, which is both testable and usable
outside the page frame.
Verified these can fail, rather than trusting a first-run pass: temporarily
changing the divBrCount-1 expectation from 2 to 99 produced the expected
failure, so the assertions do observe the DOM rather than passing vacuously.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Progress log only. Documents where Stage 0 stopped and why, so the work can be picked up without rediscovering any of it. Two local-environment problems block the rest of Stage 0, and both have traps worth warning about: - node_modules in src/BloomBrowserUI is stale: react-dom 17.0.2 installed where package.json and the lockfile both require 18.3.1, so anything importing react-dom/client fails to resolve. That breaks three spec files at load time and blocks launching Bloom, which is what three of the four remaining Stage 0 items need. - pnpm can't run here: volta's default Node is 22.12.0, pinned pnpm needs 22.13+, and forcing it past that makes pnpm want to purge node_modules. The log warns explicitly against setting CI=true or confirmModulesPurge=false to get through, since that wipes node_modules silently. It also records the workaround that does work for React-free specs: invoking vitest directly rather than through the pnpm launcher. Also records the already-fixed third problem for the benefit of other clones: a stale husky v4 hook hard-coding yarn was running because core.hooksPath was unset, fixed per .githooks/README.md rather than with --no-verify. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Progress log only. My earlier note said to upgrade Node through volta. That was wrong. ReadMe.md's Building section says to install vite-plus (vp) globally, which reads .node-version and the packageManager field and provides the correct node and pnpm, and it says volta was dropped precisely because it does not fully support pnpm -- which is the exact failure hit here. The machine state made this easy to misread: volta was on PATH serving Node 22.12.0, vp was not installed, and the yarn-era volta field is still frozen in the stale output/browser/package.json build copy. So it looks like a volta project until you read the docs. .node-version says 24.13.0 and agrees with devEngines; nothing was reading it. The boxed warning now records the documented sequence (install vp, then ./init.sh) and two traps: don't reach for volta, and don't force pnpm past its purge prompt with CI=true or confirmModulesPurge=false. It also notes that init.sh's closing "pnpm run build" is the one legitimate exception to the "don't run the full pnpm build" rule in AGENTS.md, since it is the documented setup path and there is no dev server running at that point. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Stage 0 prep commit. Extracts the save-selection/restore-selection bracket out of toolbox.ts's keystroke pipeline into markupSelectionPreservation.ts, with the same CKEditor-bookmark implementation it always had. Why, rather than doing this later with the rest of the work: this is the one place where the CKEditor-retirement plan's "integration points are one-line dispatches" does not survive contact with the code. Replacing bookmarks with character-offset anchors would otherwise mean rewriting about a hundred lines of handleKeyboardInput -- interleaved createBookmarks, comment stripping, the async-markup double-bookmark dance for BL-10133, cleanUpNbsps and selectBookmarks -- inside a churn-prone file, in the middle of a months-long project. Doing the mechanical part now, on its own, means the eventual change swaps four function bodies and touches this pipeline not at all. The seam is four functions: boxParticipatesInMarkup, saveSelectionForMarkup, restoreSelectionAfterMarkup, and restoreAndResaveSelectionForMarkup (the asynchronous path's restore-then-record-again). The saved value is deliberately opaque -- typed unknown[] -- because the planned implementation stores a character offset rather than DOM markers. Behaviour is unchanged except for one deliberate difference, commented at the site. The async path previously did createBookmarks unguarded; had the editor reported no selection there it would have thrown, inside an async function nobody awaits, so the pass died half-done via an unhandled rejection with comments already stripped and marker spans possibly left in the DOM. It now abandons the pass cleanly, which is how the first save has always behaved. Also corrects a comment that has been wrong for some time: it claimed ArithmeticTemplate number boxes get no editor "because the logic that invokes WireToCKEditor is looking for classes like bloom-content1". ckeditableSelector in utils/shared.ts explicitly includes .Equation-style, added for that template. The real no-editor case is a box whose computed cursor is not-allowed. Verified: full front-end suite green (591 passed), project typecheck clean, both changed files lint clean. Live verification of caret behaviour during reader markup (inventory rows G1-G3) is still outstanding and comes with the live-Bloom session. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Progress log only. Stage 0's code work is complete; what remains needs a running Bloom. The environment is fixed. react-dom was still 17.0.2 after init.sh, because its backgrounded pnpm install had failed under the wrong Node; a direct vp install in src/BloomBrowserUI repaired it, and the full front-end suite now passes (591 tests). The boxed warning is rewritten around what actually matters going forward: vp is installed and works, but Volta still wins for bare "node" and cannot be outranked by reordering the User PATH, because Windows always puts Machine-scope PATH first. So project commands must go through vp. It also records why removing Volta is a trade rather than pure cleanup: four maintenance worktrees still carry yarn-era volta fields. Also flags the verification gap left by the prep commit: toolboxSpec covers only cleanUpNbsps and removeCommentsFromEditableHtml, not the keystroke pipeline, so inventory rows G1-G3 must be checked against a running Bloom before the PR. That is now the first item in Next actions rather than an afterthought. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Progress log only. The environment is now correct and init.sh runs clean. Three problems were layered, each masking the next, and the intermediate states were misleading enough to be worth recording: - Volta's shim directory was in Machine PATH, and Windows evaluates Machine before User, so vp's User-scope bin could never win. No User-PATH reordering can fix that; the Machine entry had to go. - Removing it broke far more than node. Every binary in %LOCALAPPDATA%\Volta\bin is a shim whose whole body is `volta run "$(basename $0)" "$@"`, so with volta.exe off PATH, pnpm, pnpx, reviewable, nx, nx-cloud and chrome-devtools-mcp all failed with "volta: command not found". That, not init.sh itself, is what reported it could not find volta. Correcting my earlier advice: that directory is NOT harmless just because it contains no node. - vp does not put pnpm on PATH at all (its shims are node, npm, npx, corepack, vpx, vpr); it runs pnpm internally for its own subcommands, but init.sh calls bare pnpm. Fixed with the corepack vp ships, installing a pnpm shim into vp's bin so it honours each package.json's packageManager field. Volta was then restored via User PATH rather than Machine, appended so it falls after vp's bin: node resolves to vp 24.13.0, pnpm to vp 11.5.2, volta remains available at 2.0.2 for the four yarn-era maintenance worktrees, and the volta-installed global tools work again. Nothing had to be uninstalled, so the trade-off noted in the previous entry no longer applies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Stage 0 live verification, plus the harness so it need not be rebuilt. verifyCaretPreservation.mjs types a character mid-word in a bloom-editable and checks where the caret lands after the markup pipeline's 500ms debounce. Run twice independently against a running Bloom, PASS both times: text becomes "housze", the caret sits at offset 5 immediately after the typed character, no cke_bm_* bookmark spans are left in the DOM, and no stray zero-width spaces. That exercises the wiring of all four functions extracted in 2707d98a8 and shows the bookmark lifecycle still balances -- real evidence the prep commit did not break the app's most delicate keystroke code, which has no unit coverage. It deliberately records what is NOT covered, because the gap is easy to forget: with no tool active updateMarkup never runs, so the DOM is unchanged between save and restore. The case bookmarks exist for -- markup rewriting the DOM around the caret -- plus G2's async path (BL-10133, where the prep commit made its one deliberate behaviour change) and G3 longpress all remain open. They need a Decodable or Leveled Reader book: a Basic Book's toolbox offers only Talking Book and "More...", and neither became visible. Two obstacles are recorded in PROGRESS.md because they will recur: a Team Collection that is Disconnected blocks editing its existing books entirely, and the way through is to create a new book rather than to switch collections (which would mean driving a WinForms chooser CDP cannot reach); and toolbox.toolboxIsShowing() gates markup, so the pane must genuinely be open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Docs only, following the rebase onto master. Master's BL-16558 changed the decodable and leveled reader tools so they no longer rewrite the DOM to show violations: they paint ::highlight() pseudo-elements over live Range objects (textHighlightManager.ts, readerHighlights.ts). Talking Book's current-sentence highlight works the same way. That invalidates two of this plan's premises and adds a requirement. - The argument against CKEditor bookmarks gets stronger rather than weaker. It used to be "a marker span briefly confuses the markup routine mid-word". It is now "inserting and removing nodes around the caret is exactly the churn live Ranges cannot survive", i.e. bookmarks are hostile to the architecture the app has just adopted. - Our own restore paths inherit the trap. A Tier 1 undo restores an editable's innerHTML, which rebuilds its text nodes and collapses every Range pointing into them, so the highlights vanish with no error at all. Recorded as an explicit obligation on reinitializePageAfterRestore(): repaint via textHighlightManager and audioTextHighlightManager. This is the same trap BL-16558 had to fix by moving updateMarkup() after cleanUpNbsps and making cleanUpNbsps write innerHTML only when it actually converted something. Adds inventory rows G6/G7 for the highlights, with the acceptance test that toolbox.ts itself prescribes: type in a Leveled Reader book and watch the over-long sentences stay highlighted. Also records that G1 is now verified on both halves -- the automated harness, plus John checking real typing in a decodable reader, which is the case automation could not reach. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment accuracy only; no behaviour change. The comment claimed the guard replaced a case where the old code threw. That overstated it. The old code did restore-then-re-save, both dereferencing getSelection() unguarded, and the restore is still unguarded in restoreSelectionAfterMarkup -- so a null selection there throws exactly where it always did. The guard covers only the narrower case where the restore's getSelection() succeeds and the re-save's immediately afterwards returns null. Raised by the preflight review. Worth fixing rather than leaving: two comments in this same pipeline have already misled this project by overstating what the code does, and one of them is corrected in this very branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Devin's review of this branch was clean -- no bugs, no investigate flags -- but three of its five informational observations were worth acting on. - The committed verification harness hard-coded repoRoot as C:/github/BloomDesktop, so it only ran in the checkout it was written in; anyone else got a module resolution failure before reaching any CDP logic. It now derives the root from import.meta.url, which is the whole point of having committed it. - There are two deliberate behaviour differences in the extraction, 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, since bloomCkEditor is assigned once per div and never cleared, but real -- and this branch already corrects two comments in this same pipeline that misled by overstating, so leaving a third would be poor form. - The inventory pointed at PASTE-DROP-BASELINE.md as though it existed. It says so explicitly now, and why it is deliberately still outstanding. Not acted on: the note that the async early-return leaves the debounce timer handle stale (the pre-extraction early-return did the same, and a stale handle for an expired timeout only costs a pointless clearTimeout), and the note that the new spec relies on parent === window under jsdom (already explained in the spec's header, along with the recommendation that the replacement API take a document instead of hard-coding the page iframe). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
[Claude Opus 5 from John Thomson's machine during preflight] Consulted Devin on 2026-08-05 up to commit Clean — no bugs and no investigate flags. Devin did raise 8 informational observations (its lowest signal tier, not mirrored as threads); three were worth acting on and are fixed in
Not acted on, with reasons: the async early-return leaving the debounce timer handle stale (the pre-extraction early-return did the same, and a stale handle for an expired timeout only costs a pointless An earlier review of |
Progress log only. PR #8153 is open as a draft with the card linked, QA test ideas posted and the Devin consultation logged. Notes the reviewer outcomes, and specifically that three of Devin's *informational* observations were worth acting on -- including that the extraction has two deliberate behaviour differences rather than the one its comment claimed. Also records that longpress (inventory row G3) is now verified by hand, leaving the async markup path (G2) and the new live-Range highlight rows (G6/G7) as the open verification items. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Documentation only. The Current state block still said 'not pushed, no PR yet', which is the first thing a resuming session reads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
JohnThomson
marked this pull request as ready for review
August 5, 2026 20:04
Contributor
Author
|
[Claude Opus 5 from John Thomson's machine during pr-ready-for-human] Devin has since completed a review of the current HEAD |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stage 0 of retiring CKEditor and unifying Undo. Planning, tests and one behaviour-preserving refactor — no user-visible change, nothing behind a flag yet, no CKEditor removed.
Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-6681
Why this is mostly documents
CKEditor 4 (a 2015-era, hand-patched, 1.5 MB vendored copy) is to be replaced by our own code, and Bloom's five poorly-coordinated Undo mechanisms unified into a single ordered stack. That will run for months against a moving
master, so Stage 0 exists to write down what must not break before anything moves, and to prepare the one seam that cannot be done as a late one-line dispatch.docs/retire-ckeditor/PLAN.mddocs/retire-ckeditor/BEHAVIOR-INVENTORY.mddocs/retire-ckeditor/REVIEW-NOTES.mddocs/retire-ckeditor/PROGRESS.mddocs/retire-ckeditor/verifyCaretPreservation.mjs.claude/skills/resume-ckeditor/SKILL.mdThe code changes
bookEdit/toolbox/markupSelectionPreservation.ts(new) — the "save the caret / restore the caret" bracket lifted out oftoolbox.ts's keystroke pipeline, keeping the CKEditor-bookmark implementation exactly as it was. Four functions:boxParticipatesInMarkup,saveSelectionForMarkup,restoreSelectionAfterMarkup,restoreAndResaveSelectionForMarkup. The saved value is typedunknown[]so callers can't peek — the planned replacement stores a character offset, not DOM markers.This is the one place where the plan's "integration points are one-line dispatches" does not survive contact with the code: swapping bookmarks for offset anchors would otherwise mean rewriting ~100 lines of
handleKeyboardInput— interleavedcreateBookmarks, comment stripping, the async double-bookmark dance for BL-10133,cleanUpNbsps,selectBookmarks— in a churn-prone file, mid-project. Doing the mechanical part now means the real change swaps four function bodies and touches the pipeline not at all.bookEdit/js/editableDivUtilsSelectionSpec.ts(new) — 10 characterization tests pinningmakeSelectionIn/getElementSelectionIndex, which the new anchors will be built on. Worth noting why:makeSelectionIn'sdivBrCountparameter has no in-tree caller that passes anything but-1, so the<br>-stepping behaviour the anchors depend on was exercised by nothing at all.One deliberate behaviour difference, commented at the site: on the async path, where the editor reports no selection when re-saving, the pass now abandons cleanly instead of throwing inside an async function nobody awaits (which left comments stripped and marker spans possibly in the DOM via an unhandled rejection). Narrow — the restore itself is still unguarded, so a null selection there throws exactly where it used to.
Two comments corrected
Both had misled, one of them me:
toolbox.tsclaimed ArithmeticTemplate number boxes get no editor "because the logic that invokes WireToCKEditor is looking for classes like bloom-content1".ckeditableSelectorexplicitly includes.Equation-style, added for that very template. The real no-editor case is a box whose computed cursor isnot-allowed.bootstrap()'s BL-3125.bloom-canvasguard is dead code —thisisundefinedin a strict-mode module function, so$(this).find(...)is always empty. Recorded in the plan as "do not port" rather than changed here.Verification
selectAtOffsetfails 9 of its 10 tests.toolbox.tsitself prescribes for this area. Longpress spot-checked too.Still unverified, deliberately: the async-markup path (BL-10133) and the new live-Range highlight rows. Carried in
PROGRESS.md; not blocking, since this branch changes no behaviour there.🤖 Generated with Claude Code
Devin review
This change is