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..203bfa8ac3e7 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts @@ -35,6 +35,23 @@ 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 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 +// 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 +296,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 +1411,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 +1422,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 +1474,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. + function cancelPendingUpdate(): void { + if (isUndoOrRedo) { + if (undoRedoMarkupTimer) clearTimeout(undoRedoMarkupTimer); + undoRedoMarkupTimer = null; + return; + } + if (keypressTimer) clearTimeout(keypressTimer); + keypressTimer = null; + } + function 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 +1510,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 @@ -1468,21 +1540,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 +1583,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; } @@ -1540,6 +1621,13 @@ function handlePageEditing( 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 @@ -1630,11 +1718,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. }