diff --git a/chrome/content/zotero/elements/zoteroSearch.js b/chrome/content/zotero/elements/zoteroSearch.js index fedad7b73b..45740f1921 100644 --- a/chrome/content/zotero/elements/zoteroSearch.js +++ b/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); } diff --git a/scss/elements/_zoteroSearch.scss b/scss/elements/_zoteroSearch.scss index 7eca317668..14291627d5 100644 --- a/scss/elements/_zoteroSearch.scss +++ b/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; diff --git a/test/tests/advancedSearchTest.js b/test/tests/advancedSearchTest.js index 5fe3990f4b..279a278c5f 100644 --- a/test/tests/advancedSearchTest.js +++ b/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);