Group attachment and annotation conditions into Advanced Search submenus (#5981)

chrome/content/zotero/elements/zoteroSearch.js

@@ -884,7 +884,13 @@
 				<popupset id="condition-tooltips"/>
 				
 				<menulist id="conditionsmenu" oncommand="this.closest('zoterosearchcondition').onConditionSelected(event.target.value); event.stopPropagation()" native="true">
-					<menupopup onpopupshown="this.closest('zoterosearchcondition').revealSelectedCondition()">
+					<menupopup onpopupshown="if (event.target == this) this.closest('zoterosearchcondition').revealSelectedCondition()">
+						<menu id="attachment-conditions-menu">
+							<menupopup/>
+						</menu>
+						<menu id="annotation-conditions-menu">
+							<menupopup/>
+						</menu>
 						<menu id="more-conditions-menu" label="&zotero.general.more;">
 							<menupopup/>
 						</menu>
@@ -930,6 +936,8 @@
 			// Build conditions menu
 			var conditionsMenu = this.querySelector('#conditionsmenu');
 			var moreConditionsMenu = this.querySelector('#more-conditions-menu');
+			var attachmentConditionsMenu = this.querySelector('#attachment-conditions-menu');
+			var annotationConditionsMenu = this.querySelector('#annotation-conditions-menu');
 			var conditions = Zotero.SearchConditions.getStandardConditions();
 
 			// Cache the (alphabetically sorted) condition list and set up
@@ -939,20 +947,45 @@
 			this._typeAheadTime = 0;
 			conditionsMenu.addEventListener('keydown', event => this.handleConditionKeyDown(event), true);
 
+			// Label the submenus and seed the top-level entries with them, so the
+			// headings sort alphabetically alongside the primary conditions
+			attachmentConditionsMenu.setAttribute(
+				'label', Zotero.getString('search-conditions-submenu-attachment')
+			);
+			annotationConditionsMenu.setAttribute(
+				'label', Zotero.getString('search-conditions-submenu-annotation')
+			);
+			let topLevelEntries = [
+				{ label: attachmentConditionsMenu.getAttribute('label'), node: attachmentConditionsMenu },
+				{ label: annotationConditionsMenu.getAttribute('label'), node: annotationConditionsMenu },
+			];
+
 			for (let condition of conditions) {
 				let menuitem;
-				if (this.isPrimaryCondition(condition.name)) {
+				let submenu = this.getConditionSubmenu(condition.name);
+				// Attachment- and annotation-level conditions go in their own submenus,
+				// with a short label since the submenu heading supplies the context
+				if (submenu == 'attachment' || submenu == 'annotation') {
+					let parentMenu = submenu == 'attachment'
+						? attachmentConditionsMenu
+						: annotationConditionsMenu;
+					menuitem = parentMenu.appendItem(
+						Zotero.getString('search-conditions-short-' + condition.name),
+						condition.name
+					);
+				}
+				else if (this.isPrimaryCondition(condition.name)) {
 					menuitem = document.createXULElement('menuitem');
 					menuitem.setAttribute('label', condition.localized);
 					menuitem.setAttribute('value', condition.name);
-					moreConditionsMenu.before(menuitem);
+					topLevelEntries.push({ label: condition.localized, node: menuitem });
 				}
 				else {
 					menuitem = moreConditionsMenu.appendItem(
 						condition.localized, condition.name
 					);
 				}
-				
+
 				var baseFields = null;
 				try {
 					baseFields = Zotero.ItemFields.getTypeFieldsFromBase(condition.name);
@@ -1011,6 +1044,15 @@
 					menuitem.setAttribute('tooltip', condition.name + '-tooltip');
 				}
 			}
+
+			// Insert the top-level conditions and the two submenus alphabetically,
+			// before the catch-all "More" submenu
+			let collation = Zotero.getLocaleCollation();
+			topLevelEntries.sort((a, b) => collation.compareString(1, a.label, b.label));
+			for (let entry of topLevelEntries) {
+				moreConditionsMenu.before(entry.node);
+			}
+
 			conditionsMenu.selectedIndex = 0;
 		}
 
@@ -1024,22 +1066,34 @@
 				case 'date':
 				case 'dateAdded':
 				case 'dateModified':
-				case 'lastRead':
 				case 'itemType':
-				case 'fileTypeID':
 				case 'publicationTitle':
 				case 'tag':
 				case 'note':
+					return true;
+			}
+
+			return false;
+		}
+
+		// Conditions that live in the Attachment or Annotation submenu rather than
+		// at the top level of the conditions menu. Returns 'attachment', 'annotation',
+		// or null.
+		getConditionSubmenu(condition) {
+			switch (condition) {
 				case 'fulltextContent':
+				case 'fileTypeID':
+				case 'lastRead':
+					return 'attachment';
 				case 'annotationText':
 				case 'annotationComment':
 				case 'annotationType':
 				case 'annotationColor':
 				case 'annotationAuthor':
-					return true;
+					return 'annotation';
 			}
-			
-			return false;
+
+			return null;
 		}
 
 		async onConditionSelected(conditionName, reload) {

chrome/locale/en-US/zotero/zotero.ftl

@@ -1004,6 +1004,22 @@ search-conditions-annotationAuthor = Annotation Author
 search-conditions-anyField = Any Field
 search-conditions-titleCreatorYear = Title, Creator, Year
 
+# Submenu headings grouping the attachment- and annotation-level conditions
+search-conditions-submenu-attachment = Attachment
+search-conditions-submenu-annotation = Annotation
+
+# Labels for conditions shown within the Attachment and Annotation submenus,
+# where the submenu heading already supplies the "Attachment"/"Annotation"
+# context. The full names above are still shown once a condition is selected.
+search-conditions-short-fulltextContent = Content
+search-conditions-short-fileTypeID = File Type
+search-conditions-short-lastRead = Last Read
+search-conditions-short-annotationText = Text
+search-conditions-short-annotationComment = Comment
+search-conditions-short-annotationType = Type
+search-conditions-short-annotationColor = Color
+search-conditions-short-annotationAuthor = Author
+
 find-pdf-files-added = { $count ->
     [one] { $count } file added
     *[other] { $count } files added

test/tests/advancedSearchTest.js

@@ -629,6 +629,54 @@ describe("Advanced Search", function () {
 			assert.isFalse(row.querySelector('#valuemenu').hidden);
 		});
 
+		it("should place attachment and annotation conditions in their submenus", function () {
+			var s = new Zotero.Search();
+			s.libraryID = Zotero.Libraries.userLibraryID;
+			s.addCondition('title', 'is', '');
+			pane.search = s;
+
+			var row = conditions.firstChild;
+			var attachmentMenu = row.querySelector('#attachment-conditions-menu');
+			var annotationMenu = row.querySelector('#annotation-conditions-menu');
+
+			// An attachment-level condition is in the Attachment submenu rather than at
+			// the top level of the conditions menu
+			assert.isFalse(row.isPrimaryCondition('fileTypeID'));
+			assert.ok(attachmentMenu.querySelector('menuitem[value="fileTypeID"]'));
+
+			// An annotation-level condition is in the Annotation submenu
+			assert.isFalse(row.isPrimaryCondition('annotationColor'));
+			assert.ok(annotationMenu.querySelector('menuitem[value="annotationColor"]'));
+
+			// Selecting one shows its full name as the closed-menu label
+			row.onConditionSelected('annotationColor');
+			assert.equal(row.selectedCondition, 'annotationColor');
+			assert.equal(
+				row.querySelector('#conditionsmenu').getAttribute('label'),
+				Zotero.SearchConditions.getLocalizedName('annotationColor')
+			);
+		});
+
+		it("should sort the submenus alphabetically among the top-level conditions", function () {
+			var s = new Zotero.Search();
+			s.libraryID = Zotero.Libraries.userLibraryID;
+			s.addCondition('title', 'is', '');
+			pane.search = s;
+
+			var row = conditions.firstChild;
+			var popup = row.querySelector('#conditionsmenu > menupopup');
+
+			// Top-level entries (including the Attachment/Annotation submenus) are in
+			// alphabetical order, with the catch-all "More" submenu kept last
+			var labels = [...popup.children]
+				.filter(node => node.id != 'more-conditions-menu')
+				.map(node => node.getAttribute('label'));
+			var collation = Zotero.getLocaleCollation();
+			var sorted = [...labels].sort((a, b) => collation.compareString(1, a, b));
+			assert.deepEqual(labels, sorted);
+			assert.equal(popup.lastElementChild.id, 'more-conditions-menu');
+		});
+
 		it("should keep an edited value when switching to another text condition", async function () {
 			var s = new Zotero.Search();
 			s.libraryID = Zotero.Libraries.userLibraryID;