Advanced search: Show subcollections in submenus

The Collection value menu was a flat list of every collection in the
library, with subcollections set apart by an indent (or hyphens before
606d8f19ba). Build it with Utilities.Internal.createMenuForTarget()
instead, using the new 'filter' feature to limit to collections. The new
FAYT helper preserves matching on subcollections.

If a subcollection is selected, open the path down to the subcollection
when opening the menu. (As of fx153, macOS renders popups as native
menus, which can't be opened to a submenu, so revert to non-native menus
there.) A collection in a submenu can't be a menulist's selected item,
so the condition holds the value and sets the menulist's label and icon
itself, with the collection's path as a tooltip.

chrome/content/zotero/elements/zoteroSearch.js

@@ -1182,21 +1182,7 @@
 			switch (conditionName) {
 				case 'collection':
 				{
-					let rows = [];
-
-					var libraryID = this.parent.search.libraryID;
-
-					let cols = Zotero.Collections.getByLibrary(libraryID, true);
-					for (let col of cols) {
-						rows.push({
-							name: Zotero.Utilities.trimInternal(col.name),
-							value: 'C' + col.key,
-							image: Zotero.Collection.prototype.treeViewImage,
-							level: col.level
-						});
-					}
-
-					this.createValueMenu(rows);
+					this.createCollectionValueMenu(this.parent.search.libraryID);
 					break;
 				}
 				case 'savedSearch':
@@ -1398,6 +1384,7 @@
 
 		createValueMenu(rows) {
 			let valueMenu = this.querySelector('#valuemenu');
+			valueMenu.removeAttribute('tooltiptext');
 
 			while (valueMenu.hasChildNodes()) {
 				valueMenu.removeChild(valueMenu.firstChild);
@@ -1409,16 +1396,12 @@
 					menuitem.className = 'menuitem-iconic';
 					menuitem.setAttribute('image', row.image);
 				}
-				// Indent nested rows (subcollections)
-				if (row.level) {
-					menuitem.style.setProperty('--nesting-level', row.level);
-				}
 			}
 			valueMenu.selectedIndex = 0;
 			
 			if (this.value) {
 				valueMenu.value = this.value;
-				// If the value isn't in the menu (e.g., a collection from another
+				// If the value isn't in the menu (e.g., a saved search from another
 				// library after a library change), fall back to the first item
 				if (!valueMenu.selectedItem) {
 					valueMenu.selectedIndex = 0;
@@ -1426,6 +1409,91 @@
 			}
 		}
 
+		// Subcollections are shown in submenus, which the menulist can't select from, so
+		// the selection is kept in this.value (see showSelectedCollection())
+		createCollectionValueMenu(libraryID) {
+			let valueMenu = this.querySelector('#valuemenu');
+			valueMenu.removeAllItems();
+			let menupopup = valueMenu.appendChild(document.createXULElement('menupopup'));
+			// macOS renders a menulist's popup as a native menu, which can't be opened to a
+			// submenu and ignores CSS. Opt this one out so the path to the selected
+			// collection can be shown.
+			menupopup.setAttribute('nonnative', 'true');
+			
+			// If the stored collection isn't in this library (e.g., after a library change),
+			// select the first one
+			let selected = (this.value?.startsWith('C')
+					&& Zotero.Collections.getByLibraryAndKey(libraryID, this.value.substr(1)))
+				|| Zotero.Collections.getByLibrary(libraryID)[0];
+			
+			Zotero.Utilities.Internal.createMenuForTarget(
+				Zotero.Libraries.get(libraryID),
+				menupopup,
+				selected?.treeViewID,
+				(event, collection) => this.onCollectionSelected(event, collection),
+				null,
+				{ filter: target => target.objectType == 'collection' }
+			);
+			this.showSelectedCollection(selected);
+
+			// Open the submenus down to the selected collection
+			menupopup.addEventListener('popupshown', (event) => {
+				if (event.target != menupopup) {
+					return;
+				}
+				let item = menupopup.querySelector(`menuitem[checked]`);
+				let menus = [];
+				for (let menu = item?.closest('menu'); menu && menupopup.contains(menu);
+					menu = menu.parentElement?.closest('menu')) {
+					menus.unshift(menu);
+				}
+				let openNext = (index) => {
+					let menu = menus[index];
+					if (!menu) {
+						return;
+					}
+					menu.menupopup.addEventListener(
+						'popupshown', () => openNext(index + 1), { once: true }
+					);
+					menu.open = true;
+				};
+				// Wait for the outer popup to finish its own popupshown handling
+				setTimeout(() => openNext(0));
+			});
+		}
+
+		onCollectionSelected(event, collection) {
+			this.showSelectedCollection(collection);
+			// Clicking the row of a collection that has subcollections selects it without
+			// closing the menu or firing a command event
+			if (event.target.localName == 'menu') {
+				let valueMenu = this.querySelector('#valuemenu');
+				valueMenu.menupopup.hidePopup();
+				valueMenu.dispatchEvent(new Event('command', { bubbles: true }));
+			}
+		}
+
+		// The condition's value, since a collection in a submenu can't be the menulist's
+		// selected item and so can't be read back off the control
+		showSelectedCollection(collection) {
+			this.value = collection ? 'C' + collection.key : '';
+			let valueMenu = this.querySelector('#valuemenu');
+			// A top-level collection can still be the selected item, which lets the popup
+			// open positioned on it. One in a submenu can't, so set the label and icon
+			// directly instead.
+			valueMenu.selectedItem = null;
+			if (collection) {
+				valueMenu.value = collection.treeViewID;
+				valueMenu.setAttribute('label', collection.name);
+				valueMenu.setAttribute('image', collection.treeViewImage);
+				let names = [];
+				for (let c = collection; c; c = c.parentID && Zotero.Collections.get(c.parentID)) {
+					names.unshift(c.name);
+				}
+				valueMenu.setAttribute('tooltiptext', names.join(' \u203A '));
+			}
+		}
+
 		initWithParentAndCondition(parent, condition) {
 			this.parent = parent;
 			this.conditionID = condition.id;
@@ -1487,6 +1555,10 @@
 			if (this._valueMenuPending) {
 				return this.value;
 			}
+			// The collection menu can't always hold the selection (see showSelectedCollection())
+			if (this.selectedCondition == 'collection') {
+				return this.value;
+			}
 			let valueField = this.querySelector('#valuefield');
 			if (!valueField.hidden) {
 				return valueField.value;
@@ -1546,16 +1618,12 @@
 				value = this.querySelector('#value-date-age').value;
 			}
 
-			// Handle special C1234 and S5678 form for
-			// collections and searches
-			else if (condition == 'collection' || condition == 'savedSearch') {
-				var letter = this.querySelector('#valuemenu').value.substr(0, 1);
-				if (letter == 'C') {
-					condition = 'collection';
-				}
-				else if (letter == 'S') {
-					condition = 'savedSearch';
-				}
+			// Values take the special C1234/S5678 form. A collection can be in a submenu,
+			// which a menulist can't select from, so its selection is kept on the condition.
+			else if (condition == 'collection') {
+				value = this.value.substr(1);
+			}
+			else if (condition == 'savedSearch') {
 				value = this.querySelector('#valuemenu').value.substr(1);
 			}
 

scss/elements/_zoteroSearch.scss

@@ -320,11 +320,6 @@ zoterosearch {
 		max-height: 16px;
 	}
 	
-	// Indent subcollections below their parents, icon included
-	#valuemenu menuitem > .menu-icon {
-		margin-inline-start: calc(var(--nesting-level, 0) * 16px);
-	}
-
 	#condition-tooltips hbox > label
 	{
 		font-weight: 600;

test/tests/advancedSearchTest.js

@@ -1319,7 +1319,7 @@ describe("Advanced Search", function () {
 		});
 
 		describe("Collection", function () {
-			it("should show only collections", async function () {
+			it("should show only collections, with subcollections in submenus", async function () {
 				var col1 = await createDataObject('collection', { name: "A" });
 				var col2 = await createDataObject('collection', { name: "C", parentID: col1.id });
 				var col3 = await createDataObject('collection', { name: "D", parentID: col2.id });
@@ -1348,32 +1348,104 @@ describe("Advanced Search", function () {
 				}
 				
 				assert.isFalse(valueMenu.hidden);
-				// Only the collections, with the saved searches no longer mixed in
-				assert.equal(valueMenu.itemCount, 4);
-				// Subcollections are indented via a margin on the icon
-				function getIndent(menuitem) {
-					return win.getComputedStyle(menuitem.querySelector('.menu-icon'))
-						.marginInlineStart;
-				}
-				var valueMenuItem = valueMenu.getItemAtIndex(1);
-				assert.equal(valueMenuItem.getAttribute('label'), col2.name);
-				assert.equal(valueMenuItem.getAttribute('value'), "C" + col2.key);
-				assert.equal(getIndent(valueMenuItem), '16px');
-				valueMenuItem = valueMenu.getItemAtIndex(2);
-				assert.equal(valueMenuItem.getAttribute('label'), col3.name);
-				assert.equal(valueMenuItem.getAttribute('value'), "C" + col3.key);
-				assert.equal(getIndent(valueMenuItem), '32px');
-				var values = [];
-				for (let i = 0; i < valueMenu.itemCount; i++) {
-					values.push(valueMenu.getItemAtIndex(i).getAttribute('value'));
-				}
+				// Only the two top-level collections
+				assert.equal(valueMenu.itemCount, 2);
+				var col1Menu = valueMenu.getItemAtIndex(0);
+				assert.equal(col1Menu.getAttribute('label'), col1.name);
+				assert.equal(valueMenu.getItemAtIndex(1).getAttribute('label'), col4.name);
+				
+				// Subcollections are nested below their parents
+				var col2Menu = col1Menu.menupopup.querySelector(`menu[value="${col2.treeViewID}"]`);
+				assert.equal(col2Menu.getAttribute('label'), col2.name);
+				var col3Item = col2Menu.menupopup
+					.querySelector(`menuitem[value="${col3.treeViewID}"]`);
+				assert.equal(col3Item.getAttribute('label'), col3.name);
+				
+				var values = [...valueMenu.querySelectorAll('menu, menuitem')]
+					.map(node => node.getAttribute('value'));
 				assert.notInclude(values, "S" + search1.key);
 				assert.notInclude(values, "S" + search2.key);
 				
+				// Selecting a subcollection shows it on the menulist and stores its key
+				col3Item.doCommand();
+				assert.equal(valueMenu.label, col3.name);
+				// The path is out of the way in a tooltip, since the menulist rarely has room
+				assert.equal(
+					valueMenu.getAttribute('tooltiptext'),
+					`${col1.name} \u203A ${col2.name} \u203A ${col3.name}`
+				);
+				assert.isTrue(col3Item.hasAttribute('checked'));
+				assert.equal(searchCondition.getConditionData().value, col3.key);
+				
 				await Zotero.Collections.erase([col1.id, col2.id, col3.id, col4.id]);
 				await Zotero.Searches.erase([search1.id, search2.id]);
 			});
 			
+			it("should select a subcollection in a submenu by typing its name", async function () {
+				var col1 = await createDataObject('collection', { name: "A" });
+				var col2 = await createDataObject('collection', { name: "Deep", parentID: col1.id });
+				
+				var s = new Zotero.Search();
+				s.libraryID = Zotero.Libraries.userLibraryID;
+				s.addCondition('title', 'is', '');
+				pane.search = s;
+				
+				var searchCondition = conditions.firstChild;
+				var conditionsMenu = searchCondition.querySelector('#conditionsmenu');
+				var valueMenu = searchCondition.querySelector('#valuemenu');
+				
+				// Select 'Collection' condition
+				for (let i = 0; i < conditionsMenu.itemCount; i++) {
+					let menuitem = conditionsMenu.getItemAtIndex(i);
+					if (menuitem.value == 'collection') {
+						menuitem.click();
+						break;
+					}
+				}
+				assert.equal(valueMenu.label, col1.name);
+				
+				// "Deep" is in a submenu, so the menulist's own find-as-you-type can't reach it
+				valueMenu.dispatchEvent(new win.KeyboardEvent('keydown', {
+					key: 'd',
+					bubbles: true,
+					cancelable: true
+				}));
+				assert.equal(valueMenu.label, col2.name);
+				assert.equal(searchCondition.getConditionData().value, col2.key);
+				
+				await Zotero.Collections.erase([col1.id, col2.id]);
+			});
+			
+			it("should keep matching by name after the menu is rebuilt", async function () {
+				var col1 = await createDataObject('collection', { name: "Apple" });
+				var col2 = await createDataObject('collection', { name: "Banana" });
+				
+				var s = new Zotero.Search();
+				s.libraryID = Zotero.Libraries.userLibraryID;
+				s.addCondition('collection', 'is', col1.key);
+				pane.search = s;
+				
+				var searchCondition = conditions.firstChild;
+				var conditionsMenu = searchCondition.querySelector('#conditionsmenu');
+				var valueMenu = searchCondition.querySelector('#valuemenu');
+				
+				// Switching to another condition and back replaces the menu
+				conditionsMenu.querySelector('menuitem[value="title"]').doCommand();
+				conditionsMenu.querySelector('menuitem[value="collection"]').doCommand();
+				
+				for (let ch of 'banana') {
+					valueMenu.dispatchEvent(new win.KeyboardEvent('keydown', {
+						key: ch,
+						bubbles: true,
+						cancelable: true
+					}));
+				}
+				assert.equal(valueMenu.label, col2.name);
+				assert.equal(searchCondition.getConditionData().value, col2.key);
+				
+				await Zotero.Collections.erase([col1.id, col2.id]);
+			});
+			
 			it("should update when the library is changed", async function () {
 				var group = await getGroup();
 				var groupLibraryID = group.libraryID;
@@ -1398,14 +1470,9 @@ describe("Advanced Search", function () {
 						break;
 					}
 				}
-				for (let i = 0; i < valueMenu.itemCount; i++) {
-					let menuitem = valueMenu.getItemAtIndex(i);
-					if (menuitem.getAttribute('value') == "C" + collection1.key) {
-						menuitem.click();
-						break;
-					}
-				}
-				assert.equal(valueMenu.value, "C" + collection1.key);
+				valueMenu.querySelector(`menuitem[value="${collection1.treeViewID}"]`).doCommand();
+				assert.equal(valueMenu.label, collection1.name);
+				assert.equal(searchCondition.getConditionData().value, collection1.key);
 				
 				// Switch to the group library in the collection tree, which changes
 				// the search library and re-renders the conditions
@@ -1414,13 +1481,14 @@ describe("Advanced Search", function () {
 				var values = [];
 				searchCondition = conditions.firstChild;
 				valueMenu = searchCondition.querySelector('#valuemenu');
-				assert.equal(valueMenu.value, "C" + collection2.key);
+				assert.equal(valueMenu.label, collection2.name);
+				assert.equal(searchCondition.getConditionData().value, collection2.key);
 				for (let i = 0; i < valueMenu.itemCount; i++) {
 					let menuitem = valueMenu.getItemAtIndex(i);
 					values.push(menuitem.getAttribute('value'));
 				}
-				assert.notInclude(values, "C" + collection1.key);
-				assert.include(values, "C" + collection2.key);
+				assert.notInclude(values, collection1.treeViewID);
+				assert.include(values, collection2.treeViewID);
 				
 				await selectLibrary(win);