From 67ed2f8d16f757f8b6306ae4501f98f48d485cfa Mon Sep 17 00:00:00 2001 From: Abe Jellinek <1770299+AbeJellinek@users.noreply.github.com> Date: Mon, 17 Aug 2026 17:14:47 -0400 Subject: [PATCH] Fixes after review - Fix action absorbed into previous step not being undoable after history is cleared - Fix redo still showing as active after step is revised - Fix text input check including radio/checkbox inputs (and don't hardcode input types at all) --- chrome/content/zotero/xpcom/undoHistory.js | 72 ++++++++------ test/tests/readerTest.js | 83 ++++++++++++++++ test/tests/undoHistoryTest.js | 106 +++++++++++++++++++++ 3 files changed, 234 insertions(+), 27 deletions(-) diff --git a/chrome/content/zotero/xpcom/undoHistory.js b/chrome/content/zotero/xpcom/undoHistory.js index cc0aee0f1f..b9d91c3ce2 100644 --- a/chrome/content/zotero/xpcom/undoHistory.js +++ b/chrome/content/zotero/xpcom/undoHistory.js @@ -106,7 +106,7 @@ Zotero.UndoHistory = { */ registerProvider(providerID, { libraryID, undo, redo, reveal, window }) { this._providers.set(providerID, - { libraryID, undo, redo, reveal, window, lastStepID: 0 }); + { libraryID, undo, redo, reveal, window, seenSteps: new Map() }); }, /** @@ -124,12 +124,14 @@ Zotero.UndoHistory = { /** * Bring a provider's entries in line with the provider's own history. * - * Steps are ordered oldest first and identified by an id that increases + * Steps are ordered oldest first and identified by an ID that increases * monotonically, plus a revision that changes when a step absorbs a later * change (e.g., continued typing in an annotation comment). Steps the - * provider no longer has are dropped; steps newer than any we've seen are - * pushed onto the undo stack; and a step whose revision changed moves back - * to the top of the stack, since it now covers the most recent change. + * provider no longer has are dropped. A step we haven't seen, or one whose + * revision changed since we last saw it, covers a new change, so it goes + * to the top of the undo stack. A step reported with a revision we've + * already seen is left alone, so entries discarded in the meantime (e.g. + * by a clear()) aren't brought back. * * @param {String} providerID * @param {Object} history @@ -141,34 +143,36 @@ Zotero.UndoHistory = { if (!provider || !this.isEnabled()) { return; } - let stepIDs = new Set([...undoSteps, ...redoSteps].map(step => step.id)); + let steps = [...undoSteps, ...redoSteps]; + let stepIDs = new Set(steps.map(step => step.id)); this._filterEntries( entry => entry.providerID !== providerID || stepIDs.has(entry.stepID) ); - let added = false; + let changed = false; for (let step of undoSteps) { - if (step.id > provider.lastStepID) { - this._undoStack.push({ - providerID, - stepID: step.id, - revision: step.revision, - libraryID: provider.libraryID, - action: step.action, - actionArgs: step.actionArgs || null - }); - added = true; + if (provider.seenSteps.get(step.id) === step.revision) { continue; } let index = this._undoStack.findIndex( entry => entry.providerID === providerID && entry.stepID === step.id); - if (index !== -1 && this._undoStack[index].revision !== step.revision) { - let [entry] = this._undoStack.splice(index, 1); - entry.revision = step.revision; - this._undoStack.push(entry); + if (index === -1) { + this._undoStack.push({ + providerID, + stepID: step.id, + libraryID: provider.libraryID, + action: step.action, + actionArgs: step.actionArgs || null + }); } + else { + this._undoStack.push(...this._undoStack.splice(index, 1)); + } + changed = true; } - provider.lastStepID = Math.max(provider.lastStepID, ...stepIDs); - if (added) { + provider.seenSteps = new Map(steps.map(step => [step.id, step.revision])); + if (changed) { + // Any new change invalidates redo, including one absorbed into a + // step we already had an entry for this._redoStack = []; this._trimUndoStack(); } @@ -306,7 +310,7 @@ Zotero.UndoHistory = { if (!this._getProviderForWindow(focusedWindow)) { return true; } - return this._isTextBoxFocused(focusedWindow); + return this._isTextBoxFocused(focusedWindow, cmd); } let el = doc.commandDispatcher.focusedElement; if (!el) return false; @@ -314,6 +318,15 @@ Zotero.UndoHistory = { && !this._getProviderForWindow(el.contentWindow)) { return true; } + return this._elementSupportsCommand(el, cmd); + }, + + /** + * @param {Element} el + * @param {String} cmd + * @return {Boolean} + */ + _elementSupportsCommand(el, cmd) { let controllers; try { controllers = el.controllers; @@ -357,12 +370,17 @@ Zotero.UndoHistory = { * editing owns undo/redo for it * * @param {Window} win + * @param {String} cmd * @return {Boolean} */ - _isTextBoxFocused(win) { + _isTextBoxFocused(win, cmd) { let el = win.document.activeElement; - return !!el - && (el.isContentEditable || ['input', 'textarea'].includes(el.localName)); + if (!el) { + return false; + } + // A contenteditable's undo/redo is handled by an editing controller on + // the window rather than one of its own + return el.isContentEditable || this._elementSupportsCommand(el, cmd); }, /** diff --git a/test/tests/readerTest.js b/test/tests/readerTest.js index 733da17d83..fe024a524d 100644 --- a/test/tests/readerTest.js +++ b/test/tests/readerTest.js @@ -276,6 +276,58 @@ describe("Reader", function () { assert.isFalse(Zotero.UndoHistory.canRedo()); }); + it('should keep recording annotation edits after the undo history is cleared', async function () { + let attachment = await importFileAttachment('test.pdf'); + let reader = await Zotero.Reader.open(attachment.itemID); + try { + await reader._initPromise; + let annotationManager = reader._internalReader._annotationManager; + annotationManager._skipAnnotationSavingDebounce = true; + Zotero.UndoHistory.clear(); + + let annotation = annotationManager.addAnnotation( + Components.utils.cloneInto({ + type: 'highlight', + color: '#ffd400', + sortIndex: '00000|003305|00000', + position: { + pageIndex: 0, + rects: [[0, 0, 100, 100]] + }, + text: 'test' + }, reader._iframeWindow) + ); + await waitForItemEvent('add'); + + let editComment = async (comment) => { + annotationManager.updateAnnotations( + Components.utils.cloneInto([{ id: annotation.id, comment }], reader._iframeWindow) + ); + await waitForItemEvent('modify'); + }; + // The second edit is joined into the first one's history step, + // as continued typing in a comment is + await editComment('a'); + await editComment('ab'); + + // A sync applying remote changes discards the history + Zotero.UndoHistory.clear(); + assert.isFalse(Zotero.UndoHistory.canUndo()); + + // Typing in the same comment is joined into that step again, but + // it's still a change we haven't recorded, so it has to be undoable + await editComment('abc'); + assert.isTrue(Zotero.UndoHistory.canUndo()); + assert.isTrue(await Zotero.UndoHistory.undo()); + await waitForItemEvent('modify'); + // The reader's step reverts the last change joined into it + assert.equal(attachment.getAnnotations()[0].annotationComment, 'ab'); + } + finally { + await cleanupReaders(reader); + } + }); + it('should select the reader tab when undoing an annotation change from the library', async function () { let attachment = await importFileAttachment('test.pdf'); let reader = await Zotero.Reader.open(attachment.itemID); @@ -400,6 +452,37 @@ describe("Reader", function () { } }); + it('should not leave undo to text editing while a reader checkbox has focus', async function () { + let attachment = await importFileAttachment('test.pdf'); + let reader = await Zotero.Reader.open(attachment.itemID); + try { + await reader._initPromise; + await reader._internalReader._primaryView.initializedPromise; + reader._internalReader.toggleFindPopup( + Components.utils.cloneInto({ open: true }, reader._iframeWindow) + ); + let doc = reader._iframeWindow.document; + // The find popup focuses its own text box when it opens + let checkbox = await waitForCallback(() => doc.getElementById('highlight-all')); + await Zotero.Promise.delay(200); + checkbox.focus(); + await Zotero.Promise.delay(100); + // Focus only moves into the reader while its window is active + if (win.document.commandDispatcher.focusedWindow !== reader._iframeWindow + || doc.activeElement !== checkbox) { + Zotero.debug("Skipping test -- reader checkbox couldn't be focused"); + this.skip(); + } + + // A checkbox has no text editing component, so there's nothing + // for native text editing to undo + assert.isFalse(Zotero.UndoHistory.hasNativeUndo(win.document)); + } + finally { + await cleanupReaders(reader); + } + }); + it('should reopen a reader whose tab closes during a notifier transaction', async function () { let reader, reopenedReader; let title = Zotero.Promise.defer(); diff --git a/test/tests/undoHistoryTest.js b/test/tests/undoHistoryTest.js index 3f3c00563a..8fccf938c9 100644 --- a/test/tests/undoHistoryTest.js +++ b/test/tests/undoHistoryTest.js @@ -1737,6 +1737,24 @@ describe("Zotero.UndoHistory", function () { assert.equal(collection.name, 'Modified'); }); + it("should record a change absorbed into a step after history was cleared", async function () { + provider.addStep(); + provider.addStep(); + // As a sync that applied remote changes does + Zotero.UndoHistory.clear(); + assert.isFalse(Zotero.UndoHistory.canUndo()); + + // A change joined into a step we've already seen is still a change + // we haven't recorded, so it has to become undoable + provider.reviseNewestStep(); + assert.isTrue(Zotero.UndoHistory.canUndo()); + assert.isTrue(await Zotero.UndoHistory.undo()); + assert.lengthOf(provider.undoSteps, 1); + + // The step from before the clear stays discarded + assert.isFalse(await Zotero.UndoHistory.undo()); + }); + it("should clear the redo stack when a provider records a new step", async function () { let collection = await createDataObject('collection', { name: 'Original' }); Zotero.UndoHistory.clear(); @@ -1750,6 +1768,24 @@ describe("Zotero.UndoHistory", function () { assert.isFalse(Zotero.UndoHistory.canRedo()); }); + it("should clear the redo stack when a provider revises a step", async function () { + let collection = await createDataObject('collection', { name: 'Original' }); + Zotero.UndoHistory.clear(); + + provider.addStep(); + provider.addStep(); + collection.name = 'Modified'; + await collection.saveTx({ undoAction: 'undo-action-rename-collection' }); + await Zotero.UndoHistory.undo(); + assert.equal(collection.name, 'Original'); + assert.isTrue(Zotero.UndoHistory.canRedo()); + + // The provider's newest step now covers a further change, so + // redoing the rename over it is no longer valid + provider.reviseNewestStep(); + assert.isFalse(Zotero.UndoHistory.canRedo()); + }); + it("should drop entries for steps the provider no longer has", async function () { let firstStep = provider.addStep(); provider.addStep(); @@ -1808,6 +1844,76 @@ describe("Zotero.UndoHistory", function () { }); }); + describe("deferring to native text editing", function () { + var providerID, htmlDoc; + + // A provider frame with the given element focused within it + function createFrame(activeElement) { + let frame = { + document: { activeElement }, + get parent() { + return frame; + } + }; + Zotero.UndoHistory.registerProvider(providerID, { + libraryID: Zotero.Libraries.userLibraryID, + undo: () => true, + redo: () => true, + window: frame + }); + return frame; + } + + // A main window document with focus inside the given frame + function createDocument(focusedWindow) { + return { + defaultView: {}, + commandDispatcher: { focusedWindow, focusedElement: null } + }; + } + + function createInput(type) { + let input = htmlDoc.createElement('input'); + if (type) { + input.type = type; + } + return input; + } + + before(function () { + htmlDoc = new DOMParser().parseFromString( + '', 'text/html'); + }); + + beforeEach(function () { + providerID = 'provider-' + Zotero.Utilities.randomString(); + }); + + afterEach(function () { + Zotero.UndoHistory.unregisterProvider(providerID); + }); + + function assertDefers(el, shouldDefer) { + let doc = createDocument(createFrame(el)); + let desc = el.localName === 'input' ? `input[type=${el.type}]` : el.localName; + assert.equal(Zotero.UndoHistory.hasNativeUndo(doc), shouldDefer, desc); + assert.equal(Zotero.UndoHistory.hasNativeRedo(doc), shouldDefer, desc); + } + + it("should defer for a focused text box", function () { + for (let type of ['', 'text', 'search', 'password', 'number']) { + assertDefers(createInput(type), true); + } + assertDefers(htmlDoc.createElement('textarea'), true); + }); + + it("should not defer for a focused input that holds no text", function () { + for (let type of ['checkbox', 'radio', 'button', 'range', 'file']) { + assertDefers(createInput(type), false); + } + }); + }); + describe("step limit pref", function () { afterEach(function () { Zotero.Prefs.clear('undoHistory.steps');