Item tree selection changes and fixes

- Fixes selection events always being debounced
- Fixes some failing tests
- Ensures Select All command selects search matching children of
collapsed parents. Adds tests for this case

chrome/content/zotero/collectionTree.jsx

@@ -159,8 +159,7 @@ var CollectionTree = class CollectionTree extends LibraryTree {
 		return true;
 	}
 	
-	_handleSelectionChange = (shouldDebounce) => {
-		let selection = this.selection;
+	_handleSelectionChange = (selection, shouldDebounce) => {
 		let treeRow = this.getRow(selection.focused);
 		// If selection changed (by click on a different row) and we are editing
 		// commit the edit
@@ -2028,7 +2027,7 @@ var CollectionTree = class CollectionTree extends LibraryTree {
 	
 	isSelectable = index => {
 		let treeRow = this.getRow(index);
-		return !(treeRow.isSeparator() || treeRow.isHeader());
+		return treeRow && !(treeRow.isSeparator() || treeRow.isHeader());
 	}
 
 	_closeContainer(row, skipMap) {

chrome/content/zotero/components/virtualized-table.jsx

@@ -79,6 +79,7 @@ class TreeSelection {
 	 * @param shouldDebounce {Boolean} Whether the update to the tree should be debounced
 	 */
 	toggleSelect(index, shouldDebounce) {
+		if (!this._tree.props.isSelectable(index)) return;
 		index = Math.max(0, index);
 		if (this.selected.has(index)) {
 			this.selected.delete(index);
@@ -112,6 +113,7 @@ class TreeSelection {
 	 * @returns {boolean} False if nothing to select and select handlers won't be called
 	 */
 	select(index, shouldDebounce) {
+		if (!this._tree.props.isSelectable(index)) return;
 		index = Math.max(0, index);
 		if (this.selected.size == 1 && this._focused == index && this.pivot == index) {
 			return false;
@@ -140,7 +142,9 @@ class TreeSelection {
 			this.selected = new Set();
 		}
 		for (let i = from; i <= to; i++) {
-			this.selected.add(i);
+			if (this._tree.props.isSelectable(i)) {
+				this.selected.add(i);
+			}
 		}
 	}
 
@@ -166,6 +170,8 @@ class TreeSelection {
 	 * @param shouldDebounce {Boolean} Whether the update to the tree should be debounced
 	 */
 	shiftSelect(index, shouldDebounce) {
+		if (!this._tree.props.isSelectable(index)) return;
+		
 		index = Math.max(0, index);
 		let from = Math.min(index, this.pivot);
 		let to = Math.max(index, this.pivot);
@@ -542,21 +548,7 @@ class VirtualizedTable extends React.Component {
 
 		case "a":
 			// i.e. if CTRL/CMD pressed down
-			if (movePivot) {
-				// Do not select unselectable (disabled) rows
-				for (let i = 0; i < this.props.getRowCount(); i++) {
-					if (this.props.isSelectable(i)) {
-						this.selection.selected.add(i);
-					} else {
-						this.selection.selected.delete(i);
-					}
-				}
-				if (this.selection.selectEventsSuppressed) break;
-				this.invalidate();
-				if (!this.selection.selectEventsSuppressed) {
-					this.props.onSelectionChange(this, false);
-				}
-			}
+			if (movePivot) this.selection.rangedSelect(0, this.props.getRowCount()-1);
 			break;
 			
 		case " ":

chrome/content/zotero/itemTree.jsx

@@ -1911,7 +1911,9 @@ var ItemTree = class ItemTree extends LibraryTree {
 	}
 
 	isSelectable = (index) => {
-		return !this._searchMode || this._searchItemIDs.has(this.getRow(index).id);
+		if (!this._searchMode || this.collectionTreeRow.isPublications()) return true;
+		let row = this.getRow(index);
+		return row && this._searchItemIDs.has(row.id);
 	}
 	
 	isContainer = (index) => {
@@ -2573,8 +2575,7 @@ var ItemTree = class ItemTree extends LibraryTree {
 	//
 	// //////////////////////////////////////////////////////////////////////////////
 
-	_handleSelectionChange = (shouldDebounce) => {
-		let selection = this.selection;
+	_handleSelectionChange = (selection, shouldDebounce) => {
 		// Update aria-activedescendant on the tree
 		if (this.collectionTreeRow.isDuplicates() && selection.count == 1) {
 			var itemID = this.getRow(selection.focused).ref.id;
@@ -3112,7 +3113,7 @@ var ItemTree = class ItemTree extends LibraryTree {
 			Zotero.logError(e);
 		}
 
-		this.ensureRowsAreVisible(Array.from(this.selection.selected.keys()));
+		this.ensureRowsAreVisible(Array.from(this.selection.selected));
 
 		if (unsuppress) {
 			this.selection.selectEventsSuppressed = false;

chrome/content/zotero/xpcom/progressQueueDialog.js

@@ -83,7 +83,7 @@ Zotero.ProgressQueueDialog = function (progressQueue) {
 	};
 	
 	function _onWindowLoaded() {
-		var rootElement = document.getElementById('zotero-progress');
+		var rootElement = _progressWindow.document.getElementById('zotero-progress');
 		Zotero.setFontSize(rootElement);
 		
 		_progressIndicator = _progressWindow.document.getElementById('progress-indicator');

test/tests/itemTreeTest.js

@@ -31,7 +31,45 @@ describe("Zotero.ItemTree", function() {
 		item.deleted = true;
 		await item.saveTx();
 		assert.isFalse(itemsView.getRowIndexByID(itemID));
-	})
+	});
+	
+	describe("when performing a quick search", function () {
+		let parentItem, match, nonMatch;
+		let selectAllEvent = {key: 'a'};
+		before(async function () {
+			parentItem = await createDataObject('item');
+			match = await importFileAttachment('test.png', { title: 'find-me', parentItemID: parentItem.id });
+			nonMatch = await importFileAttachment('test.png', { title: 'not-a-result', parentItemID: parentItem.id });
+			if (Zotero.isMac) {
+				selectAllEvent.metaKey = true;
+			} else {
+				selectAllEvent.ctrlKey = true;
+			}
+		});
+
+		it("should not select non-matching children when issuing a Select All command", async function () {
+			var quicksearch = win.document.getElementById('zotero-tb-search');
+			quicksearch.value = match.getField('title');
+			quicksearch.doCommand();
+			await itemsView._refreshPromise;
+			itemsView.tree._onKeyDown(selectAllEvent);
+
+			var selected = itemsView.getSelectedItems(true);
+			assert.lengthOf(selected, 1);
+			assert.equal(selected[0], match.id);
+		});
+
+		it("should expand collapsed parents with matching children when issuing a Select All command", async function () {
+			itemsView.collapseAllRows();
+			var selected = itemsView.getSelectedItems(true);
+			assert.lengthOf(selected, 0);
+			
+			itemsView.tree._onKeyDown(selectAllEvent);
+			selected = itemsView.getSelectedItems(true);
+			assert.lengthOf(selected, 1);
+			assert.equal(selected[0], match.id);
+		});
+	});
 	
 	describe("#selectItem()", function () {
 		/**
@@ -183,8 +221,10 @@ describe("Zotero.ItemTree", function() {
 				skipSelect: true
 			});
 			
-			// No select events should have occurred
-			assert.equal(win.ZoteroPane.itemSelected.callCount, 0);
+			// itemSelected should have been called once (from 'selectEventsSuppressed = false'
+			// in notify()) as a no-op
+			assert.equal(win.ZoteroPane.itemSelected.callCount, 1);
+			assert.isFalse(win.ZoteroPane.itemSelected.returnValues[0].value());
 			
 			// Existing item should still be selected
 			selected = itemsView.getSelectedItems(true);
@@ -274,8 +314,10 @@ describe("Zotero.ItemTree", function() {
 			item.setField('title', 'no select on modify');
 			yield item.saveTx();
 			
-			// No select events should have occurred
-			assert.equal(win.ZoteroPane.itemSelected.callCount, 0);
+			// itemSelected should have been called once (from 'selectEventsSuppressed = false'
+			// in notify()) as a no-op
+			assert.equal(win.ZoteroPane.itemSelected.callCount, 1);
+			assert.isFalse(win.ZoteroPane.itemSelected.returnValues[0].value());
 			
 			// Modified item should not be selected
 			assert.lengthOf(itemsView.getSelectedItems(), 0);