From 2dd2a7db45faa700480462910d6833f51cedef0c Mon Sep 17 00:00:00 2001 From: John Thomson Date: Thu, 13 Aug 2026 12:59:43 -0500 Subject: [PATCH 1/4] Re-mark up the page after an Undo or Redo (BL-16558) Since the reader and Talking Book highlights became ::highlight() pseudo-elements painted over live Ranges, an undo silently kills them: it does not edit text in place, it writes a saved snapshot over the whole editable, rebuilding every text node in the box. Only a markup pass can re-create the Ranges, and three separate things stopped one from running. Measured against a running Bloom (driving the Edit tab over CDP and inspecting CSS.highlights, checking each Range for collapsed / getClientRects), with the Leveled Reader tool on a page holding an over-long sentence: - the top bar's Undo button left the highlight permanently dead; - so did a Ctrl+Z that restored a range selection (select a word, type over it, undo); - a plain Ctrl+Z blanked it for about half a second. Talking Book turned out to be largely self-healing, because the MutationObserver added for BL-15300 repairs its highlights in place, so the reader tools were where this showed. Its markup was equally stale after a top bar Undo, though, since nothing re-ran it. The three causes were: nothing triggers a markup pass when the undo came from the top bar button (there is no keystroke); the pass gives up when the selection is not collapsed, and an undo restores whatever selection its snapshot was taken with; and jquery.longpress sets isLongPressEvaluating on EVERY keydown and clears it on keyup, so Ctrl+Z, which does its work on the keydown, always found the flag set. So every undo path we own now calls one entry point, updateMarkupAfterUndoOrRedo(): ckeditor's own undo/redo commands (via a new afterCommandExec hook), the top bar button's two text-changing branches, and the reader tools' Ctrl+Z/Ctrl+Y interception. handlePageEditing() takes a trigger - "editing", "paste" or "undoOrRedo" - in place of a retry count, and the trigger is what the undo-specific behavior hangs off: skip the longpress flag, tolerate a range selection, retry when there is no usable selection yet (the top bar button has no keystroke to bring a second chance), and run at once on a private timer so the keyup that ends Ctrl+Z cannot cancel the update it just asked for. A held Ctrl+Z auto-repeats, so those immediate passes are spaced at least 150ms apart rather than re-marking the page on every repeat. Not unit-tested, for the reason already recorded in handlePageEditing: exercising it needs a live ckeditor instance on the editable plus the parent window's page iframe and a real Selection, none of which stand up in jsdom. Verified in the running app instead - after the change, top bar Undo, Ctrl+Z, Ctrl+Z over a selection and Ctrl+Y all keep the highlight alive, with sampling every 10ms finding it dead at most for a sub-frame blip. Known limitation, unchanged here: on Ctrl+Z with the Talking Book tool open, the keyup that follows bumps keydownEventCounter, so the undo's own (async) markup action is discarded and the keyup's pass does the work 500ms later, as it does today. Making the undo pass immune to that would risk applying markup computed from pre-keystroke text - it is built from a clone taken before the await - which is exactly what that counter guards against. Co-Authored-By: Claude Opus 5 (1M context) --- .../bookEdit/js/bloomEditing.ts | 12 ++ .../decodableReader/decodableReaderTool.tsx | 9 +- .../bookEdit/toolbox/toolbox.ts | 146 +++++++++++++++--- .../bookEdit/toolbox/toolboxBootstrap.ts | 5 +- .../bookEdit/toolbox/toolboxGlobals.d.ts | 1 + src/BloomBrowserUI/bookEdit/workspaceRoot.ts | 10 ++ 6 files changed, 157 insertions(+), 26 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts b/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts index 02b969a3f39a..82429ecac618 100644 --- a/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts +++ b/src/BloomBrowserUI/bookEdit/js/bloomEditing.ts @@ -2023,6 +2023,18 @@ export function attachToCkEditor(element) { updateCkEditorButtonStatus(editor); }); + // Ctrl+Z and Ctrl+Y (and Ctrl+Shift+Z) are handled by ckeditor's own undo plugin, which + // runs them as the "undo" and "redo" commands. Undo writes a whole saved snapshot over the + // editable, so the active tool's markup no longer matches the text and, worse, the tools' + // ::highlight() Ranges are left pointing at text nodes that no longer exist. Tell the + // toolbox, which knows how to put both right. See updateMarkupAfterUndoOrRedo(). + ckedit.on("afterCommandExec", (evt) => { + const commandName = evt.data.name; + if (commandName === "undo" || commandName === "redo") { + getToolboxBundleExports()?.updateMarkupAfterUndoOrRedo(); + } + }); + // hide the toolbar when ckeditor starts ckedit.on("instanceReady", (evt) => { const editor = evt["editor"]; diff --git a/src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/decodableReaderTool.tsx b/src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/decodableReaderTool.tsx index b6332124bd4f..2097265ea23f 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/decodableReaderTool.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/readers/decodableReader/decodableReaderTool.tsx @@ -5,7 +5,10 @@ import { getTheOneReaderToolsModel, MarkupType } from "../readerToolsModel"; import { get } from "../../../../utils/bloomApi"; import { isReaderToolEnabledOnCurrentPage } from "../readerToolPageState"; import { renderRoot } from "../../../../utils/reactRender"; -import { isLongPressEvaluating } from "../../toolbox"; +import { + isLongPressEvaluating, + updateMarkupAfterUndoOrRedo, +} from "../../toolbox"; import StyleEditor from "../../../StyleEditor/StyleEditor"; import $ from "jquery"; @@ -171,6 +174,10 @@ export class DecodableReaderTool extends ToolboxToolReactAdaptor { } else { getTheOneReaderToolsModel().undo(); } + // Both of those restore a saved innerHTML, which replaces the text + // nodes our highlights are painted over, so the markup has to be + // redone before they can paint anything again (BL-16558). + updateMarkupAfterUndoOrRedo(); return false; } } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts index 72281f5f7f91..8aa1f8148e00 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts @@ -35,6 +35,22 @@ let savedSettings: ToolboxSettings = {}; let keypressTimer: ReturnType | null = null; +// The pending markup update asked for by an undo or redo, if any. It deliberately does NOT +// share keypressTimer: every keystroke cancels that one, and Ctrl+Z ends with a keyup, which +// would therefore throw away the very update the undo just asked for. +let undoRedoMarkupTimer: ReturnType | null = null; + +// When the last undo/redo markup pass began, and how close together we will let them be. +// A single undo asks for its markup at once, because the whole point is to repaint the +// highlights it just detached before anyone sees them missing. But holding Ctrl+Z down +// auto-repeats the undo, and each repeat asks for another pass; with no delay to coalesce +// them, that is a full re-markup of the page per repeat, which is what the 500ms typing +// throttle exists to avoid. So the first one runs immediately and any that pile in behind it +// wait their turn. (A burst is bounded anyway - ckeditor's undo stack is 20 deep - but 20 +// re-markups in half a second while ckeditor is mid-burst is still worth not doing.) +let lastUndoRedoMarkupStartTime = 0; +const minMillisecondsBetweenUndoRedoMarkups = 150; + // This variable stores all the ids of the enabled tools, so // that the React toolbox settings can initially check the // checkboxes that correspond to the enabled tools @@ -279,10 +295,7 @@ export class ToolBox { const isPasteShortcut = (event.ctrlKey || event.metaKey) && event.keyCode === 86; if (isPasteShortcut) { - setTimeout( - () => handlePageEditing(maxPasteMarkupUpdateRetries), - 0, - ); + setTimeout(() => handlePageEditing("paste"), 0); } }) .keyup((event) => { @@ -1397,8 +1410,8 @@ function beginAddTool( } let keydownEventCounter = 0; -const retryDelayForPasteMarkupUpdateInMilliseconds = 100; -const maxPasteMarkupUpdateRetries = 3; +const retryDelayForMarkupUpdateInMilliseconds = 100; +const maxMarkupUpdateRetries = 3; export function scheduleMarkupUpdateAfterPaste(): void { // AI thinks we might need this to "allow the DOM to settle" even before we do the @@ -1408,17 +1421,46 @@ export function scheduleMarkupUpdateAfterPaste(): void { // that is hard to reproduce reliably. I'd rather have a timeout that we don't // need than have the markup occasionally not update, let alone somehow have // the markup update somehow mess up the paste. So I decided to leave it in. - setTimeout(() => handlePageEditing(maxPasteMarkupUpdateRetries), 0); + setTimeout(() => handlePageEditing("paste"), 0); } -// Handle edits to the page: mainly triggered by key up, but also by paste. +// Call this whenever an Undo or Redo has replaced the content of an editable, from wherever +// that Undo was initiated (Ctrl+Z in the text, the Undo button in the top bar, the reader +// tools' own undo stack). +// +// It matters more than it looks. Undo does not edit the text in place: it writes a whole +// saved snapshot over the editable, which builds new text nodes for everything in the box. +// The tools' highlights are ::highlight() pseudo-elements painted over live Ranges into +// those text nodes (see textHighlightManager.ts), so every highlight in that box dies at +// that moment - it stays in the registry but paints nothing, and only a markup pass can put +// it back. Left to itself, the markup pass either doesn't happen at all (a click on the top +// bar's Undo button produces no keystroke) or happens and gives up (see the three places +// below where "undoOrRedo" is treated differently), which is why the Leveled and Decodable +// Reader highlights could stay gone until the user typed something (BL-16558). +export function updateMarkupAfterUndoOrRedo(): void { + handlePageEditing("undoOrRedo"); +} + +// What asked for a markup update. It decides several things below, because an undo is not +// just another edit: it is a single deliberate action, and it can leave the page in states +// that we would rather not do the markup in but have no choice about. +type MarkupUpdateTrigger = "editing" | "paste" | "undoOrRedo"; + +// Handle edits to the page: mainly triggered by key up, but also by paste and by undo/redo. // For various reasons a single paste may cause this to get called several times, but the 500ms // delay should prevent us from doing the markup more than once per paste. // Similarly, since updating the markup is fairly costly, it's good not to do it on every keystroke // while the user is typing rapidly. -function handlePageEditing( - remainingRetriesForInvalidSelectionState: number = 0, -): void { +function handlePageEditing(trigger: MarkupUpdateTrigger = "editing"): void { + const isUndoOrRedo = trigger === "undoOrRedo"; + // Typing gets no retries because the next keystroke brings another update along anyway. + // The other two have no such fallback: a paste can leave the selection temporarily in a + // state we can't mark up in, and an undo can leave it with no selection at all (e.g. + // readerToolsModel.undo() restores the html but returns before makeSelectionIn() when it + // has no saved offset). An undo from the top bar button has no keystroke behind it, so + // giving up on the first look would leave the highlights dead with no second chance. + const remainingRetriesForInvalidSelectionState = + trigger === "editing" ? 0 : maxMarkupUpdateRetries; // BL-599: "Unresponsive script" while typing in text. // The function setTimeout() returns an integer, not a timer object, and therefore it does not have a member // function called "clearTimeout." Because of this, the jQuery method $.isFunction(keypressTimer.clearTimeout) @@ -1431,7 +1473,32 @@ function handlePageEditing( // this.keypressTimer.clearTimeout(); //} const counterValueThatIdentifiesThisKeyDown = ++keydownEventCounter; - if (keypressTimer) clearTimeout(keypressTimer); + // An undo waits on its own timer rather than the shared one, because every keystroke + // cancels the shared one - and Ctrl+Z ends with a keyup, which would therefore throw away + // the very update the undo just asked for. These two put the choice in one place so the + // rest of the method doesn't have to care which timer it is. Replacing whatever that same + // timer was already waiting for is what makes a burst of events do the markup just once. + const cancelPendingUpdate = (): void => { + if (isUndoOrRedo) { + if (undoRedoMarkupTimer) clearTimeout(undoRedoMarkupTimer); + undoRedoMarkupTimer = null; + return; + } + if (keypressTimer) clearTimeout(keypressTimer); + keypressTimer = null; + }; + const scheduleMainTask = ( + task: () => void, + delayMilliseconds: number, + ): void => { + cancelPendingUpdate(); + if (isUndoOrRedo) { + undoRedoMarkupTimer = setTimeout(task, delayMilliseconds); + } else { + keypressTimer = setTimeout(task, delayMilliseconds); + } + }; + cancelPendingUpdate(); // Not sure we need this now the method is triggered by keyup. If it is triggered by keydown, // we have a problem: // If we don't do this check, then the last keydown from autorepeat during longpress will @@ -1442,7 +1509,11 @@ function handlePageEditing( // I'm leaving it in for now because the method might get called on a keyup connected with using // a key in longpress to select one of the options, and in that case, we don't want to do the markup // (until the keyup from the original key, of course). - if (window?.top?.[isLongPressEvaluating]) { + // Undo/redo is exempt: longpress sets that flag on EVERY keydown (see its onKeyDown) and + // clears it on keyup, so Ctrl+Z, which does its work on the keydown, always finds it set, + // and honoring it here meant the undo never got its markup update at all. Ctrl+Z can't be + // a longpress in any case: longpress is about holding down a letter key on its own. + if (!isUndoOrRedo && window?.top?.[isLongPressEvaluating]) { return; } // If this was making DOM changes that we want to save, we would want to try to use @@ -1458,6 +1529,9 @@ function handlePageEditing( // (perhaps temporarily) invalid for doing the markup, we'll try again a few times with a // shorter delay, and if it still isn't valid, we'll just give up until the next keyup or paste. const mainTask = async (remainingRetries: number) => { + if (isUndoOrRedo) { + lastUndoRedoMarkupStartTime = Date.now(); + } const page: HTMLIFrameElement = ( parent.window.document.getElementById("page") ); @@ -1468,21 +1542,29 @@ function handlePageEditing( const active = anchor ? $(anchor).closest("div").get(0) : null; + // A selection that covers a range of text, rather than being a simple insertion point, + // normally makes us leave the markup alone until the user does something simpler. + // Undo/redo is the exception: it restores whatever selection its snapshot was taken + // with, and the markup (and with it the highlights) has to be brought up to date + // regardless. The bookmark machinery below preserves a range just as it does an + // insertion point. + const selectionIsRange = !!( + selection && + (selection.rangeCount > 1 || + (selection.rangeCount === 1 && + !selection.getRangeAt(0).collapsed)) + ); const selectionStateIsInvalidForMarkup = - !active || - (selection && - (selection.rangeCount > 1 || - (selection.rangeCount === 1 && - !selection.getRangeAt(0).collapsed))); + !active || (selectionIsRange && !isUndoOrRedo); if (selectionStateIsInvalidForMarkup) { // Copilot suggested that there are some cases after a paste where the selection // is only temporarily a range, so it's worth trying again a few times. // This callback can also be canceled by a new keypress etc. if (remainingRetries > 0) { - keypressTimer = setTimeout( + scheduleMainTask( () => mainTask(remainingRetries - 1), - retryDelayForPasteMarkupUpdateInMilliseconds, + retryDelayForMarkupUpdateInMilliseconds, ); } return; // don't even try to adjust markup while there is some complex selection @@ -1503,7 +1585,8 @@ function handlePageEditing( // It would be great if we didn't have settle for using window.top, // but the other player here (jquery.longpress.js) is in a totally different // context currently, so my other attempts to share a boolean failed. - if (window.top[isLongPressEvaluating]) { + // (Undo/redo is exempt, for the reason given where this flag is tested above.) + if (!isUndoOrRedo && window.top[isLongPressEvaluating]) { return; } @@ -1630,11 +1713,26 @@ function handlePageEditing( } } // clear this value to prevent unnecessary calls to clearTimeout() for timeouts that have already expired. - keypressTimer = null; + if (isUndoOrRedo) { + undoRedoMarkupTimer = null; + } else { + keypressTimer = null; + } }; - keypressTimer = setTimeout( + scheduleMainTask( () => mainTask(remainingRetriesForInvalidSelectionState), - 500, + // An undo has already put the restored text in the DOM, and unlike typing it is a + // single deliberate action, so there is nothing to wait for. Doing it at once (rather + // than after the usual 500ms of quiet) keeps the highlights, which the undo has just + // detached from the text, from visibly blinking off. The only reason to wait at all is + // to keep an auto-repeating Ctrl+Z from re-marking the page on every repeat. + isUndoOrRedo + ? Math.max( + 0, + minMillisecondsBetweenUndoRedoMarkups - + (Date.now() - lastUndoRedoMarkupStartTime), + ) + : 500, ); } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolboxBootstrap.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolboxBootstrap.ts index 9fb1578f1e49..d7db8cc854c8 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolboxBootstrap.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolboxBootstrap.ts @@ -5,6 +5,7 @@ import { applyToolboxStateToUpdatedPage, removeToolboxMarkup, scheduleMarkupUpdateAfterPaste, + updateMarkupAfterUndoOrRedo, } from "./toolbox"; import { simulateBlurOnPageFrameMouseDown } from "../../utils/menuCloseOnBlur"; import { getTheOneReaderToolsModel } from "./readers/readerToolsModel"; @@ -43,6 +44,7 @@ export interface IToolboxFrameExports { getTheOneToolbox(): ToolBox; scheduleMarkupUpdateAfterPaste(): void; + updateMarkupAfterUndoOrRedo(): void; canUndo(): boolean; undo(): void; @@ -71,7 +73,7 @@ export { activateLongPressFor } from "../js/bloomEditing"; export { TalkingBookTool }; // one function is called by CSharp. export { getTheOneToolbox }; -export { scheduleMarkupUpdateAfterPaste }; +export { scheduleMarkupUpdateAfterPaste, updateMarkupAfterUndoOrRedo }; // Import the functions we're re-exporting so we can use them in the bundle import { @@ -136,6 +138,7 @@ ToolBox.registerTool(new SettingsTool()); const toolboxBundle: ToolboxBundleApi = { getTheOneToolbox, scheduleMarkupUpdateAfterPaste, + updateMarkupAfterUndoOrRedo, applyToolboxStateToPage, removeToolboxMarkup, showSetupDialog, diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolboxGlobals.d.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolboxGlobals.d.ts index a3c5234bf56d..5a8316ce19a6 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolboxGlobals.d.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolboxGlobals.d.ts @@ -24,6 +24,7 @@ declare global { interface ToolboxBundleApi { getTheOneToolbox: () => ToolboxApi | undefined; scheduleMarkupUpdateAfterPaste: unknown; + updateMarkupAfterUndoOrRedo: unknown; applyToolboxStateToPage: unknown; removeToolboxMarkup: unknown; showSetupDialog: unknown; diff --git a/src/BloomBrowserUI/bookEdit/workspaceRoot.ts b/src/BloomBrowserUI/bookEdit/workspaceRoot.ts index febdbb3065cc..68189ec7df23 100644 --- a/src/BloomBrowserUI/bookEdit/workspaceRoot.ts +++ b/src/BloomBrowserUI/bookEdit/workspaceRoot.ts @@ -107,6 +107,11 @@ export function handleUndo(): void { 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. @@ -121,6 +126,11 @@ export function handleUndo(): void { 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. } From 8ff980defa034a998868144c6bd0b1e62a1012b6 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Thu, 13 Aug 2026 13:12:21 -0500 Subject: [PATCH 2/4] Only make an undo wait behind a pass that did the markup (BL-16558) Devin's review of the previous commit noticed that lastUndoRedoMarkupStartTime was stamped at the top of mainTask, before we know whether the pass will do anything. An attempt that bails because the selection isn't usable - and each of its 100ms retries - therefore counted as "the last markup pass", so a genuine undo arriving within 150ms waited its turn behind work that never happened. Stamp it once we are past every early return instead, which is also what the throttle is for: spacing out real re-markups of the page during an auto-repeating Ctrl+Z. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts index 8aa1f8148e00..a94013f63084 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts @@ -40,7 +40,8 @@ let keypressTimer: ReturnType | null = null; // would therefore throw away the very update the undo just asked for. let undoRedoMarkupTimer: ReturnType | null = null; -// When the last undo/redo markup pass began, and how close together we will let them be. +// When the last undo/redo markup pass that actually got as far as doing the markup began, and +// how close together we will let them be. // A single undo asks for its markup at once, because the whole point is to repaint the // highlights it just detached before anyone sees them missing. But holding Ctrl+Z down // auto-repeats the undo, and each repeat asks for another pass; with no delay to coalesce @@ -1529,9 +1530,6 @@ function handlePageEditing(trigger: MarkupUpdateTrigger = "editing"): void { // (perhaps temporarily) invalid for doing the markup, we'll try again a few times with a // shorter delay, and if it still isn't valid, we'll just give up until the next keyup or paste. const mainTask = async (remainingRetries: number) => { - if (isUndoOrRedo) { - lastUndoRedoMarkupStartTime = Date.now(); - } const page: HTMLIFrameElement = ( parent.window.document.getElementById("page") ); @@ -1590,6 +1588,13 @@ function handlePageEditing(trigger: MarkupUpdateTrigger = "editing"): void { return; } + // Past every reason to give up, so this pass is really going to do the markup. That is + // what the next undo/redo has to wait behind; an attempt that bailed above (and each of + // its retries) did no work and must not make one wait. + if (isUndoOrRedo) { + lastUndoRedoMarkupStartTime = Date.now(); + } + // the hard thing about all this is preserving the user's insertion point while we change the actual // html out from under them to add/remove markup. // ckeditor specific discussion: http://stackoverflow.com/questions/16835365/set-cursor-to-specific-position-in-ckeditor From 659c8061d0ebbe4ee436bb6f8f27100fb6d1d22e Mon Sep 17 00:00:00 2001 From: John Thomson Date: Thu, 13 Aug 2026 13:26:27 -0500 Subject: [PATCH 3/4] Stamp the undo throttle only where the markup is certain (BL-16558) Devin's re-review pointed out that the previous commit moved the timestamp past the early returns but not past the two guards after them: the pass still does nothing if the caret isn't in a bloom-editable, or if that box has no ckeditor attached (some arithmetic template boxes). So put the stamp where the markup is actually about to happen, and say so. Behaviour difference is small either way - at worst a following undo's repaint waited up to 150ms behind a pass that did nothing - but the comment claimed more than the placement delivered. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts index a94013f63084..26c20f9b1299 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts @@ -1588,13 +1588,6 @@ function handlePageEditing(trigger: MarkupUpdateTrigger = "editing"): void { return; } - // Past every reason to give up, so this pass is really going to do the markup. That is - // what the next undo/redo has to wait behind; an attempt that bailed above (and each of - // its retries) did no work and must not make one wait. - if (isUndoOrRedo) { - lastUndoRedoMarkupStartTime = Date.now(); - } - // the hard thing about all this is preserving the user's insertion point while we change the actual // html out from under them to add/remove markup. // ckeditor specific discussion: http://stackoverflow.com/questions/16835365/set-cursor-to-specific-position-in-ckeditor @@ -1628,6 +1621,13 @@ function handlePageEditing(trigger: MarkupUpdateTrigger = "editing"): void { if (!ckeditorSelection) { return; // may be changing pages? } + // We are now certainly going to do the markup, which is the work the next + // undo/redo has to wait behind. Anything that gave up before this point - an + // unusable selection and each of its retries, no editable under the caret, a + // box with no ckeditor - did none of that work and must not make one wait. + if (isUndoOrRedo) { + lastUndoRedoMarkupStartTime = Date.now(); + } // there is also createBookmarks2(), which avoids actually inserting anything. That has the // advantage that changing a character in the middle of a word will allow the entire word to // be evaluated by the markup routine. However, testing shows that the cursor then doesn't From 3c857036a8423478da38dd724b82610a03a2fe83 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Thu, 13 Aug 2026 13:41:21 -0500 Subject: [PATCH 4/4] Use function syntax for the two new helpers (BL-16558) src/BloomBrowserUI/AGENTS.md asks for typescript "function" syntax over const foo = () => functions; Devin's review caught that the two helpers added to handlePageEditing were arrow constants. (The pre-existing mainTask above them is left alone.) Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts index 26c20f9b1299..203bfa8ac3e7 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts @@ -1479,7 +1479,7 @@ function handlePageEditing(trigger: MarkupUpdateTrigger = "editing"): void { // the very update the undo just asked for. These two put the choice in one place so the // rest of the method doesn't have to care which timer it is. Replacing whatever that same // timer was already waiting for is what makes a burst of events do the markup just once. - const cancelPendingUpdate = (): void => { + function cancelPendingUpdate(): void { if (isUndoOrRedo) { if (undoRedoMarkupTimer) clearTimeout(undoRedoMarkupTimer); undoRedoMarkupTimer = null; @@ -1487,18 +1487,18 @@ function handlePageEditing(trigger: MarkupUpdateTrigger = "editing"): void { } if (keypressTimer) clearTimeout(keypressTimer); keypressTimer = null; - }; - const scheduleMainTask = ( + } + function scheduleMainTask( task: () => void, delayMilliseconds: number, - ): void => { + ): void { cancelPendingUpdate(); if (isUndoOrRedo) { undoRedoMarkupTimer = setTimeout(task, delayMilliseconds); } else { keypressTimer = setTimeout(task, delayMilliseconds); } - }; + } cancelPendingUpdate(); // Not sure we need this now the method is triggered by keyup. If it is triggered by keydown, // we have a problem: