From 167045d0005f79596c1d6c8c4a8b4724f3cd9984 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Wed, 17 Jun 2026 14:32:22 -0400 Subject: [PATCH] Show library-aware section headers in the item tree for multi-row selections https://github.com/zotero/zotero/pull/5954#issuecomment-4724330966 --- .../content/zotero/collectionViewItemTree.jsx | 126 +++++++++++------- chrome/content/zotero/itemTree.jsx | 5 +- chrome/content/zotero/itemTreeRow.js | 55 +++++++- chrome/locale/en-US/zotero/zotero.ftl | 35 +++++ scss/components/_item-tree.scss | 52 ++++++-- test/tests/collectionViewItemTreeTest.js | 110 +++++++++++++-- 6 files changed, 307 insertions(+), 76 deletions(-) diff --git a/chrome/content/zotero/collectionViewItemTree.jsx b/chrome/content/zotero/collectionViewItemTree.jsx index dad021e11e..62d104a4a4 100644 --- a/chrome/content/zotero/collectionViewItemTree.jsx +++ b/chrome/content/zotero/collectionViewItemTree.jsx @@ -43,7 +43,7 @@ const React = require('react'); const ReactDOM = require('react-dom'); const ItemTree = require('zotero/itemTree'); const { ItemTreeRowProvider } = ItemTree; -const { LibraryHeaderItemTreeRow } = require('zotero/itemTreeRow'); +const { LibraryHeaderItemTreeRow, SpacerItemTreeRow } = require('zotero/itemTreeRow'); const { OS } = ChromeUtils.importESModule("chrome://zotero/content/osfile.mjs"); const { ZOTERO_CONFIG } = ChromeUtils.importESModule('resource://zotero/config.mjs'); @@ -140,23 +140,80 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider { let lastLibraryID = null; let seenFirstHeader = false; for (let row of this._rows) { - if (row.type == 'library-header') { + if (row.type == 'library-header' || row.type == 'spacer') { continue; } if (row.level == 0 && row.ref.libraryID !== lastLibraryID) { lastLibraryID = row.ref.libraryID; - let header = new LibraryHeaderItemTreeRow(Zotero.Libraries.get(lastLibraryID)); - // Every header except the first gets a gap above it separating it from - // the previous library's items (see _updateLibraryHeaderHeights and CSS) - header.hasSectionGap = seenFirstHeader; + let library = Zotero.Libraries.get(lastLibraryID); + // Every header except the first gets a blank spacer row above it, for one + // row of whitespace separating its section from the previous library's + if (seenFirstHeader) { + newRows.push(new SpacerItemTreeRow(library)); + } seenFirstHeader = true; - newRows.push(header); + let { label, iconName } = this._sectionHeaders.get(lastLibraryID) || {}; + newRows.push(new LibraryHeaderItemTreeRow(library, label, iconName)); } newRows.push(row); } this._rows = newRows; } + /** + * Compute the section header label (and, for single-library selections, icon) for + * each library in the selection. Returns a Map of libraryID -> { label, iconName }. + * + * Across libraries, each header is the library name plus what's selected in it + * ("My Library (2 collections selected)"); within a single library, one header + * summarizes the selection ("2 collections selected"). Labels are resolved here (and + * cached on the rows) so the sticky header can render them synchronously. + */ + async _computeSectionHeaders(collectionTreeRows) { + let grouped = this._groupedByLibrary; + // Group the selected rows by library, preserving selection order + let byLibrary = new Map(); + for (let row of collectionTreeRows) { + let libraryID = row.ref.libraryID; + if (!byLibrary.has(libraryID)) { + byLibrary.set(libraryID, []); + } + byLibrary.get(libraryID).push(row); + } + let entries = []; + for (let [libraryID, rows] of byLibrary) { + let count = rows.length; + let library = Zotero.Libraries.getName(libraryID); + let id; + if (rows.every(row => row.isRecentlyRead())) { + id = grouped ? 'items-section-library-recently-read' : null; + } + else if (rows.every(row => row.isLibrary(true))) { + id = grouped ? 'items-section-library' : null; + } + else if (rows.every(row => row.isCollection())) { + id = grouped ? 'items-section-library-collections' : 'items-section-collections-selected'; + } + else if (rows.every(row => row.isSearch())) { + id = grouped ? 'items-section-library-searches' : 'items-section-searches-selected'; + } + else { + id = grouped ? 'items-section-library-sources' : 'items-section-sources-selected'; + } + // Cross-library headers use the library icon; the single-library summary + // header has no icon, to set it apart from a sticky library header + entries.push({ libraryID, id, args: { library, count }, iconName: grouped ? undefined : null }); + } + let labels = await document.l10n.formatValues( + entries.map(e => (e.id ? { id: e.id, args: e.args } : { id: 'items-section-library', args: e.args })) + ); + let map = new Map(); + entries.forEach((e, i) => { + map.set(e.libraryID, { label: labels[i], iconName: e.iconName }); + }); + return map; + } + /** * Set new collectionTreeRows and refresh items. * This handles the data/model logic; UI orchestration stays in ItemTree. @@ -200,6 +257,14 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider { } this._groupedByLibrary = !collectionTreeRows[0].isFeedsOrFeed() && this._libraryOrder.size > 1; + // A section header is shown above each library's items whenever more than one row + // is selected (a single header for a single-library selection; one per library + // across libraries). Feeds are never grouped or headed. + this._showSectionHeaders = !collectionTreeRows[0].isFeedsOrFeed() + && collectionTreeRows.length > 1; + this._sectionHeaders = this._showSectionHeaders + ? await this._computeSectionHeaders(collectionTreeRows) + : new Map(); // Set ID based on visibilityGroup const visibilityGroup = collectionTreeRows[0].visibilityGroup || 'default'; @@ -311,8 +376,8 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider { var skipChildren; for (let i = 0; i < this._rows.length; i++) { let row = this._rows[i]; - // Don't copy library header rows -- they're reinserted after sorting - if (row.type == 'library-header') { + // Don't copy library header or spacer rows -- they're reinserted after sorting + if (row.type == 'library-header' || row.type == 'spacer') { continue; } // Top-level items @@ -407,7 +472,7 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider { this.refreshRowMap(); Zotero.debug(`Refreshed open parents in ${new Date() - t} ms`); - if (this._groupedByLibrary) { + if (this._showSectionHeaders) { this._insertLibraryHeaders(); this.refreshRowMap(); } @@ -1040,10 +1105,6 @@ class CollectionViewItemTree extends ItemTree { } async handleRowModelUpdate(rows, options = {}) { - // Update header row heights before super renders: super invalidates the - // windowed list, and _renderItem reads the custom-height map at render time, - // so the map and offsets have to be set first for the taller rows to paint - this._updateLibraryHeaderHeights(); const completed = await super.handleRowModelUpdate(rows, options); if (completed) { await this._updateIntroText(); @@ -1051,35 +1112,6 @@ class CollectionViewItemTree extends ItemTree { return completed; } - /** - * Each library header after the first is given extra height so that, with the - * header content bottom-aligned (see _item-tree.scss), there's a gap above it - * separating it from the previous library's items. The first header gets no extra - * height (no gap at the top of the list). The windowed list positions every row at - * index * baseRowHeight unless an override is given, so feed it the current header - * indices on each model update. - */ - _updateLibraryHeaderHeights() { - if (!this.tree?._jsWindow || !this.tree._rowHeight) { - return; - } - // _groupedByLibrary and the row array live on the row provider, not the tree - let provider = this.rowProvider; - let customRowHeights = []; - if (provider._groupedByLibrary) { - // Headers with a section gap are made taller; bottom-aligned content (CSS) - // turns the extra height into a gap above the heading - let headerHeight = Math.round(this.tree._rowHeight * 1.5); - let rows = provider._rows; - for (let i = 0; i < rows.length; i++) { - if (rows[i].type == 'library-header' && rows[i].hasSectionGap) { - customRowHeights.push([i, headerHeight]); - } - } - } - this.tree.updateCustomRowHeights(customRowHeights); - } - async notify(action, type, ids, extraData) { // If a collection with subcollections is deleted/restored, ids will include subcollections // though they are not showing in itemTree. @@ -1126,8 +1158,9 @@ class CollectionViewItemTree extends ItemTree { * @returns {Boolean} */ isSelectable(index, selectAll=false) { - // Library header rows are never selectable - if (this.getRow(index)?.type == 'library-header') { + // Library header and spacer rows are never selectable + let type = this.getRow(index)?.type; + if (type == 'library-header' || type == 'spacer') { return false; } // Every listed item is selectable individually. There are exceptions @@ -1196,7 +1229,8 @@ class CollectionViewItemTree extends ItemTree { * Start a drag using HTML 5 Drag and Drop */ onDragStart(event, index) { - if (this.getRow(index)?.type == 'library-header') { + let type = this.getRow(index)?.type; + if (type == 'library-header' || type == 'spacer') { event.preventDefault(); return false; } diff --git a/chrome/content/zotero/itemTree.jsx b/chrome/content/zotero/itemTree.jsx index 114e50e0e8..c8cc71fc40 100644 --- a/chrome/content/zotero/itemTree.jsx +++ b/chrome/content/zotero/itemTree.jsx @@ -1424,6 +1424,9 @@ var ItemTree = class ItemTree extends LibraryTree { multiSelect: this.props.multiSelect, + stickySectionHeaders: true, + isSectionHeader: index => this.getRow(index)?.type == 'library-header', + onSelectionChange: this._handleSelectionChange.bind(this), isSelectable: this.isSelectable.bind(this), getParentIndex: this.getParentIndex.bind(this), @@ -2189,7 +2192,7 @@ var ItemTree = class ItemTree extends LibraryTree { div.classList.toggle('last-highlighted', this._highlightedRows.has(rowData.id) && !this._highlightedRows.has(nextRowID)); div.classList.toggle('annotation-row', row.type === 'annotation'); div.classList.toggle('library-header-row', row.type === 'library-header'); - div.classList.toggle('section-gap', !!row.hasSectionGap); + div.classList.toggle('spacer-row', row.type === 'spacer'); if (row.type !== 'annotation') { div.classList.remove('tight'); } diff --git a/chrome/content/zotero/itemTreeRow.js b/chrome/content/zotero/itemTreeRow.js index a8c765311f..d999e84c28 100644 --- a/chrome/content/zotero/itemTreeRow.js +++ b/chrome/content/zotero/itemTreeRow.js @@ -636,8 +636,13 @@ class SearchItemTreeRow extends ItemTreeRow { * items list displays a multi-library selection. Wraps a Zotero.Library. */ class LibraryHeaderItemTreeRow extends ItemTreeRow { - constructor(library) { + constructor(library, label, iconName) { super(library, 0, false); + // Label and icon are computed from the selection (see + // CollectionViewItemTreeRow._computeSectionHeaders); fall back to the library's + // own name/icon when not provided + this._label = label; + this._iconName = iconName; } get type() { @@ -645,7 +650,7 @@ class LibraryHeaderItemTreeRow extends ItemTreeRow { } getDisplayTitle() { - return Zotero.Libraries.getName(this.ref.libraryID); + return this._label ?? Zotero.Libraries.getName(this.ref.libraryID); } getField(field) { @@ -656,23 +661,62 @@ class LibraryHeaderItemTreeRow extends ItemTreeRow { } getIcon() { - let icon = getCSSIcon(this.ref.libraryType == 'group' ? 'library-group' : 'library'); + // An explicit null icon name means render no icon (e.g. the single-library + // summary header), distinct from undefined (use the library's own icon) + if (this._iconName === null) { + return null; + } + let iconName = this._iconName + ?? (this.ref.libraryType == 'group' ? 'library-group' : 'library'); + let icon = getCSSIcon(iconName); icon.classList.add('icon-item-type'); return icon; } renderRow(div, _index, _columns, _rowData, _renderCtx) { - // Single cell with the library icon and name, spanning the row + // Single cell with an optional icon and the label, spanning the row let span = document.createElement('span'); span.className = 'cell primary library-header'; let textSpan = document.createElement('span'); textSpan.className = 'cell-text'; textSpan.textContent = this.getDisplayTitle(); - span.append(this.getIcon(), textSpan); + let icon = this.getIcon(); + if (icon) { + span.append(icon, textSpan); + } + else { + // No icon, but reserve its space so the label still lines up with item titles + let spacer = document.createElement('span'); + spacer.className = 'library-header-icon-spacer'; + span.append(spacer, textSpan); + } div.appendChild(span); } } +/** + * A blank, non-selectable row providing one row of whitespace above a library header + * (see CollectionViewItemTreeRow._insertLibraryHeaders). Kept separate from the header + * row so the header stays a uniform height and pins flush to the top when sticky. + */ +class SpacerItemTreeRow extends ItemTreeRow { + constructor(library) { + super(library, 0, false, 'spacer-' + library.libraryID); + } + + get type() { + return 'spacer'; + } + + getField() { + return ''; + } + + renderRow(_div, _index, _columns, _rowData, _renderCtx) { + // Intentionally empty -- the row's height alone provides the whitespace + } +} + ItemTreeRow.create = function (ref, level, isOpen) { if (ref instanceof Zotero.Collection) return new CollectionItemTreeRow(ref, level, isOpen); if (ref instanceof Zotero.Search) return new SearchItemTreeRow(ref, level, isOpen); @@ -688,4 +732,5 @@ module.exports.FileItemTreeRow = FileItemTreeRow; module.exports.AnnotationItemTreeRow = AnnotationItemTreeRow; module.exports.CollectionItemTreeRow = CollectionItemTreeRow; module.exports.SearchItemTreeRow = SearchItemTreeRow; +module.exports.SpacerItemTreeRow = SpacerItemTreeRow; module.exports.LibraryHeaderItemTreeRow = LibraryHeaderItemTreeRow; diff --git a/chrome/locale/en-US/zotero/zotero.ftl b/chrome/locale/en-US/zotero/zotero.ftl index 2f8451e910..75843a759d 100644 --- a/chrome/locale/en-US/zotero/zotero.ftl +++ b/chrome/locale/en-US/zotero/zotero.ftl @@ -195,6 +195,41 @@ collections-menu-show-recently-read = .label = Show { recently-read } item-menu-remove-from-recently-read = .label = Remove from { recently-read }… + +# Item list section headers for a multiple-row collection-tree selection within one library +items-section-collections-selected = + { $count -> + [one] { $count } collection selected + *[other] { $count } collections selected + } +items-section-searches-selected = + { $count -> + [one] { $count } saved search selected + *[other] { $count } saved searches selected + } +items-section-sources-selected = + { $count -> + [one] { $count } source selected + *[other] { $count } sources selected + } +# Item list section headers for a selection spanning multiple libraries, one per library +items-section-library-collections = + { $count -> + [one] { $library } ({ $count } collection selected) + *[other] { $library } ({ $count } collections selected) + } +items-section-library-searches = + { $count -> + [one] { $library } ({ $count } saved search selected) + *[other] { $library } ({ $count } saved searches selected) + } +items-section-library-sources = + { $count -> + [one] { $library } ({ $count } source selected) + *[other] { $library } ({ $count } sources selected) + } +items-section-library-recently-read = { $library } ({ recently-read }) +items-section-library = { $library } collections-menu-rename = .label = Rename edit-saved-search = Edit Saved Search diff --git a/scss/components/_item-tree.scss b/scss/components/_item-tree.scss index 683ead15ac..6bf78cbb12 100644 --- a/scss/components/_item-tree.scss +++ b/scss/components/_item-tree.scss @@ -25,6 +25,14 @@ padding-inline-start: 8px; padding-inline-end: calc(8px + var(--scrollbar-width, 0px)); box-sizing: border-box; + // A clear divider below the column headers (the default faint border is lost + // against the white section-header/spacer rows below it). The header already + // carries a second 1px line via ::after, so drop that one to avoid doubling. + border-bottom: var(--material-panedivider); + + &::after { + border-bottom: none; + } .cell.hasAttachment, .cell.numNotes { @@ -75,7 +83,8 @@ .virtualized-table, .drag-image-container { .row { - &.odd:not(.selected) { + // Header and spacer rows are always the pane background, never striped + &.odd:not(.selected):not(.library-header-row):not(.spacer-row) { background-color: var(--material-stripe); } @@ -215,17 +224,14 @@ } } - // Section header above each library's items in a cross-library selection. - // A hairline below the heading separates it from its own items. The first - // heading sits flush at the top; every later heading carries extra height - // (via customRowHeights) that, with bottom-aligned content, becomes a gap - // above it separating it from the previous library's items. + // Section header above each library's items in a cross-library selection. These + // rows are pinned to the top while their section is scrolled (see the sticky + // section header support in virtualized-table), so they're a uniform height with + // top-aligned content. The background matches the pane so the items below stripe + // against it; square corners keep the on-scroll hairline from curving at the edges. .library-header-row { - border-bottom: var(--material-panedivider); - - &.section-gap { - align-items: flex-end; - } + background-color: var(--material-background); + border-radius: 0; .cell.library-header { display: flex; @@ -235,12 +241,34 @@ color: var(--fill-secondary); .icon-item-type { - margin-inline-end: 6px; + margin-inline-end: 4px; opacity: 0.8; } + + // Stands in for a missing icon (single-library summary header) so the + // label still aligns with the item titles below it + .library-header-icon-spacer { + width: 16px; + margin-inline-end: 4px; + flex-shrink: 0; + } } } + // The divider under a section header shows only while its section is scrolled + // under the pinned copy (see the "stuck" state in virtualized-table). Drawn as a + // box-shadow rather than a border so toggling it doesn't shift the row's content. + .virtualized-table-sticky-section-header.stuck .library-header-row { + box-shadow: inset 0 -1px 0 var(--color-panedivider); + } + + // Blank whitespace row above each library header (except the first); matches the + // pane background so it reads as empty space rather than a striped row + .spacer-row { + background-color: var(--material-background); + pointer-events: none; + } + .annotation-row { .cell { font-size: $font-size-small; diff --git a/test/tests/collectionViewItemTreeTest.js b/test/tests/collectionViewItemTreeTest.js index 51bc5aad9e..f8d08f35d1 100644 --- a/test/tests/collectionViewItemTreeTest.js +++ b/test/tests/collectionViewItemTreeTest.js @@ -2582,6 +2582,12 @@ describe("CollectionViewItemTree", function () { }); describe("Library grouping", function () { + // Fluent wraps interpolated values in bidi isolation marks; strip them for + // plain-text comparisons + function stripBidi(str) { + return str.replace(/[⁦-⁩]/g, ''); + } + async function selectMultipleCollections(collections) { await cv.selectByID("C" + collections[0].id); await waitForItemsLoad(win); @@ -2625,21 +2631,99 @@ describe("CollectionViewItemTree", function () { // Header rows aren't selectable assert.isFalse(view.isSelectable(userHeaderRow)); - // The first library header sits flush at the top (default height); each - // later header is taller, giving a gap above it that separates sections - let tree = view.tree; - assert.notProperty(tree._customRowHeightMap, String(userHeaderRow), - "First library header should use the default height (no gap at top)"); - assert.isAbove(tree._customRowHeightMap[groupHeaderRow], tree._rowHeight, - "Later library headers should be taller, for a gap above the heading"); - assert.notProperty(tree._customRowHeightMap, String(item1Row), - "Item rows should use the default height"); + // Each cross-library header is the library name plus what's selected in it + assert.equal(stripBidi(view.getRow(groupHeaderRow).getDisplayTitle()), + Zotero.Libraries.getName(group.libraryID) + " (1 collection selected)"); + + // A blank spacer row sits above every header except the first, for whitespace + // separating the sections + assert.notEqual(view.getRow(userHeaderRow - 1)?.type, 'spacer', + "No spacer above the first header"); + assert.equal(view.getRow(groupHeaderRow - 1).type, 'spacer', + "Spacer row above a later header"); + assert.isFalse(view.isSelectable(groupHeaderRow - 1), + "Spacer rows aren't selectable"); await selectLibrary(win); await group.eraseTx(); }); - it("shouldn't group when all items are in a single library", async function () { + it("should pin the section header of the library scrolled to the top", async function () { + let group = await createGroup(); + let collection1 = await createDataObject('collection'); + let collection2 = await createDataObject('collection', { libraryID: group.libraryID }); + await createDataObject('item', { collections: [collection1.id] }); + // Enough group items below the group header that it can be scrolled to the top + await Zotero.DB.executeTransaction(async function () { + for (let i = 0; i < 60; i++) { + let item = createUnsavedDataObject( + 'item', { libraryID: group.libraryID, collections: [collection2.id] } + ); + await item.save(); + } + }); + + await cv.expandLibrary(group.libraryID); + await selectMultipleCollections([collection1, collection2]); + + let view = zp.itemsView; + let tree = view.tree; + let body = tree._jsWindow.targetElement; + let groupHeaderRow = view.getRowIndexByID("L" + group.libraryID); + + // At the top of the list, the first (user library) header is pinned + body.scrollTop = 0; + tree._updateStickySectionHeader(); + assert.include(tree._stickyHeader.textContent, + Zotero.Libraries.getName(Zotero.Libraries.userLibraryID)); + + // The pinned header lines up horizontally with the real header row + let userHeaderRow = view.getRowIndexByID("L" + Zotero.Libraries.userLibraryID); + let realIcon = tree._jsWindow.getElementByIndex(userHeaderRow).querySelector('.icon-item-type'); + let stickyIcon = tree._stickyHeaderContent.querySelector('.icon-item-type'); + assert.equal( + stickyIcon.getBoundingClientRect().left, + realIcon.getBoundingClientRect().left, + "Pinned header icon should align with the real header icon" + ); + + // Scrolling the group header to the top pins the group library header instead + body.scrollTop = tree._jsWindow._getItemPosition(groupHeaderRow); + tree._updateStickySectionHeader(); + assert.include(tree._stickyHeader.textContent, Zotero.Libraries.getName(group.libraryID)); + + await selectLibrary(win); + await group.eraseTx(); + }); + + it("should restart row striping at each section header", async function () { + let group = await createGroup(); + let collection1 = await createDataObject('collection'); + let collection2 = await createDataObject('collection', { libraryID: group.libraryID }); + // Two user-library items so the group's first item falls on an odd absolute index + await createDataObject('item', { collections: [collection1.id] }); + await createDataObject('item', { collections: [collection1.id] }); + await createDataObject('item', { libraryID: group.libraryID, collections: [collection2.id] }); + + await cv.expandLibrary(group.libraryID); + await selectMultipleCollections([collection1, collection2]); + + let view = zp.itemsView; + let tree = view.tree; + let groupHeaderRow = view.getRowIndexByID("L" + group.libraryID); + let firstGroupItemRow = groupHeaderRow + 1; + // The group's first item is at an odd absolute index, but striping restarts at + // the header, so it gets the unstriped (even) shade + assert.equal(firstGroupItemRow % 2, 1, "Setup: first group item at an odd index"); + let elem = tree._jsWindow.getElementByIndex(firstGroupItemRow); + assert.isTrue(elem.classList.contains('even'), "First item of a section is unstriped"); + assert.isFalse(elem.classList.contains('odd')); + + await selectLibrary(win); + await group.eraseTx(); + }); + + it("should show one summary header but not group for a single-library multi-selection", async function () { let collection1 = await createDataObject('collection'); let collection2 = await createDataObject('collection'); let item1 = await createDataObject('item', { collections: [collection1.id] }); @@ -2648,9 +2732,11 @@ describe("CollectionViewItemTree", function () { await selectMultipleCollections([collection1, collection2]); let view = zp.itemsView; + // One library -> a single summary header, but not grouped into sections assert.isFalse(view.rowProvider._groupedByLibrary); - assert.isFalse(view.getRowIndexByID("L" + Zotero.Libraries.userLibraryID), - "No library header should be shown for a single-library selection"); + let headerRow = view.getRowIndexByID("L" + Zotero.Libraries.userLibraryID); + assert.isNumber(headerRow, "A summary header should be shown"); + assert.equal(stripBidi(view.getRow(headerRow).getDisplayTitle()), "2 collections selected"); assert.isNumber(view.getRowIndexByID(item1.id)); assert.isNumber(view.getRowIndexByID(item2.id));