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

@@ -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);
 	},
 
 	/**

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();

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(
+				'<!DOCTYPE html><html><body></body></html>', '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');