From 31d9e89165757c4739a9024a4f8cba9077659721 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Mon, 8 Jun 2026 12:55:13 -0400 Subject: [PATCH 001/465] Fix item tree focus checks broken by view-specific tree IDs The item tree's DOM id carries a view-specific suffix (e.g. "item-tree-main-default", "item-tree-main-recentlyRead"), but three call sites compared against a single hardcoded "item-tree-main": - Collection highlighting on Ctrl/Option (zoteroPane.js) -- match on the "item-tree-main" prefix to cover all views. This restores highlighting in Recently Read, where it silently failed. - Focusing the items list after Add Item by Identifier (lookup.js) -- use the current view's tree id instead of a literal that resolved to null and threw. - Shift-Tab from the item tree to the toolbar (zoteroPane.js) -- key the actionsMap on the current view's tree id. Add a test confirming focus lands on the items list after a lookup. https://forums.zotero.org/discussion/130968/collection-of-selected-papers-is-not-highlighted-in-recently-read-panel --- chrome/content/zotero/lookup.js | 3 ++- chrome/content/zotero/zoteroPane.js | 7 +++++-- test/tests/lookupTest.js | 16 ++++++++++++++++ 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/chrome/content/zotero/lookup.js b/chrome/content/zotero/lookup.js index 6abc11f2ba..cd7cbbcb72 100644 --- a/chrome/content/zotero/lookup.js +++ b/chrome/content/zotero/lookup.js @@ -147,7 +147,8 @@ var Zotero_Lookup = new function () { // Send the focus to the item tree after the popup closes ZoteroPane.lastFocusedElement = null; document.getElementById("zotero-lookup-panel").hidePopup(); - document.getElementById("item-tree-main-default").focus(); + // The item tree's DOM id has a view-specific suffix, so use the current view's id + document.getElementById(ZoteroPane.itemsView.id).focus(); } return false; }; diff --git a/chrome/content/zotero/zoteroPane.js b/chrome/content/zotero/zoteroPane.js index 5a48549b52..7ec522dac0 100644 --- a/chrome/content/zotero/zoteroPane.js +++ b/chrome/content/zotero/zoteroPane.js @@ -438,7 +438,8 @@ var ZoteroPane = new function () { itemTree.addEventListener("keydown", (event) => { let actionsMap = { - 'item-tree-main-default': { + // The item tree's DOM id has a view-specific suffix, so key on the current view's id + [ZoteroPane.itemsView?.id]: { ShiftTab: () => document.getElementById('zotero-tb-toggle-item-pane-stacked') } }; @@ -1066,7 +1067,9 @@ var ZoteroPane = new function () { else { enableHighlight = !event.shiftKey && !event.metaKey && event.key == "Control" && !event.altKey; } - let isItemTreeFocused = document.activeElement.id == "item-tree-main-default"; + // The item tree's DOM id has a view-specific suffix (e.g. "item-tree-main-default", + // "item-tree-main-recentlyRead"), so match on the prefix to cover all views + let isItemTreeFocused = document.activeElement.id.startsWith("item-tree-main"); // Only highlight collections when itemTree is focused to try to avoid // conflicts with other shortcuts if (enableHighlight && isItemTreeFocused) { diff --git a/test/tests/lookupTest.js b/test/tests/lookupTest.js index 245c0cfe53..da594dfa23 100644 --- a/test/tests/lookupTest.js +++ b/test/tests/lookupTest.js @@ -49,6 +49,22 @@ describe("Add Item by Identifier", function () { }); }); + it("should focus the items list after adding items", async function () { + // Make sure the items list is non-empty (and therefore focusable) + await createDataObject('item'); + await waitForItemsLoad(win); + // Stub the lookup itself so the test doesn't hit external services + var stub = sinon.stub(win.Zotero_Lookup, "addItemsFromIdentifier").resolves([{}]); + try { + var textbox = win.document.getElementById("zotero-lookup-textbox"); + await win.Zotero_Lookup.accept(textbox); + assert.equal(win.document.activeElement.id, win.ZoteroPane.itemsView.id); + } + finally { + stub.restore(); + } + }); + it.skip("should add a DOI with an open-access PDF"); // e.g., arXiv From dcf5010415a41c4aaa38e88d18f9cc03823f2d55 Mon Sep 17 00:00:00 2001 From: Abe Jellinek <1770299+AbeJellinek@users.noreply.github.com> Date: Mon, 8 Jun 2026 15:43:32 -0400 Subject: [PATCH 002/465] Info box: Optimize rendering of many creator rows (#5939) --- .../content/zotero/elements/editableText.js | 72 +++++- chrome/content/zotero/elements/itemBox.js | 239 +++++++++++------- 2 files changed, 216 insertions(+), 95 deletions(-) diff --git a/chrome/content/zotero/elements/editableText.js b/chrome/content/zotero/elements/editableText.js index 22bf14ade1..2c8dcb131c 100644 --- a/chrome/content/zotero/elements/editableText.js +++ b/chrome/content/zotero/elements/editableText.js @@ -38,9 +38,7 @@ class EditableText extends XULElementBase { _input; - - _resizeObserver; - + _ignoredWindowInactiveBlur = false; _focusMousedownEvent = false; @@ -76,7 +74,30 @@ }); return span; } - + + /** + * A single ResizeObserver shared by every EditableText in the document. + * When inputs resize, recompute the 'overflowing' state for all affected + * fields at once, batching layout reads before writes: + * toggling 'overflowing' makes the layout dirty, and measuring the width + * requires a clean layout, so interleaving the two would force a reflow + * per measurement, which adds up to multiple seconds of layout + * calculations on outlier items with thousands of creators. + * + * See {@link #batchSizeToContent()}. + */ + static _resizeObserver = new ResizeObserver((entries) => { + let editableTexts = entries + .map(entry => entry.target.closest('editable-text')) + .filter(editableText => editableText?._input); + // Read phase: measure everything first + let overflowing = editableTexts.map(editableText => editableText._isOverflowing()); + // Write phase: apply the class changes in one batch + for (let i = 0; i < editableTexts.length; i++) { + editableTexts[i].classList.toggle('overflowing', overflowing[i]); + } + }); + get noWrap() { return this.hasAttribute('nowrap'); } @@ -199,6 +220,27 @@ sizeToContent = () => { this.style.maxWidth = this._getContentWidth() + 'px'; }; + + /** + * Size multiple elements to their content in a single batch. + * + * sizeToContent() reads layout and then updates layout styles. + * Calling it once per element interleaves those reads and writes, + * forcing a full synchronous reflow on every call. That's extremely + * slow, especially if it occurs in a loop that also adds new elements + * to the DOM on each iteration, as in the InfoBox. + * Measuring everything first and only then applying the widths collapses + * that to a single reflow regardless of how many elements are sized. + * + * @param {EditableText[]} elements + */ + static batchSizeToContent(elements) { + elements = [...elements]; + let widths = elements.map(el => el._getContentWidth()); + for (let i = 0; i < elements.length; i++) { + elements[i].style.maxWidth = widths[i] + 'px'; + } + } attributeChangedCallback(name) { if (name === 'value' || name === 'dir') { @@ -211,6 +253,13 @@ this.render(); } + destroy() { + // Stop the shared observer from holding a reference to our input + if (this._input) { + EditableText._resizeObserver.unobserve(this._input); + } + } + render() { let autocompleteParams = this.autocomplete; let autocompleteEnabled = !this.multiline && !!autocompleteParams; @@ -245,6 +294,7 @@ this.removeEventListener('keydown', this._captureAutocompleteKeydown, true); } + let oldInput = this._input; let focused = this.focused; let selectionStart = this._input?.selectionStart; let selectionEnd = this._input?.selectionEnd; @@ -268,10 +318,12 @@ this._input.setSelectionRange(selectionStart, selectionEnd, selectionDirection); } - this._resizeObserver?.disconnect(); + if (oldInput) { + EditableText._resizeObserver.unobserve(oldInput); + } + // Only nowrap fields can overflow horizontally; textareas wrap if (this.noWrap) { - this._resizeObserver = new ResizeObserver(this._handleInputResize); - this._resizeObserver.observe(this._input); + EditableText._resizeObserver.observe(this._input); } } this._input.readOnly = this.readOnly; @@ -497,10 +549,10 @@ } }; - _handleInputResize = () => { + _isOverflowing() { // Very small floating-point-error allowance const EPSILON = 0.001; - this.classList.toggle('overflowing', + return ( // We're overflowing if the field can scroll at least a pixel this._input.scrollLeftMax > 0 // But sometimes it can scroll a *sub*pixel, and every single @@ -513,7 +565,7 @@ && this._getContentWidth() > this._input.getBoundingClientRect().width + EPSILON ) ); - }; + } focus(options) { // If the window isn't active, the focus event won't fire yet, diff --git a/chrome/content/zotero/elements/itemBox.js b/chrome/content/zotero/elements/itemBox.js index e87a96916e..6acae0df39 100644 --- a/chrome/content/zotero/elements/itemBox.js +++ b/chrome/content/zotero/elements/itemBox.js @@ -67,6 +67,9 @@ this._selectFieldValue = null; this._selectFieldSelection = null; this._addCreatorRow = false; + this._addingCreatorRowsInBulk = false; + this._needsUnsavedCreatorRemoval = false; + this._pendingCreatorSizing = []; this._switchedModeOfCreator = null; this._popupNode = null; @@ -548,6 +551,11 @@ let rowLabel = document.createElement("div"); rowLabel.className = "meta-label"; rowLabel.setAttribute('fieldname', fieldName); + // Augment the fieldname attribute with a class so querySelectors for + // label elements use fast indexed class lookups + if (fieldName) { + rowLabel.classList.add(`meta-label-${fieldName}`); + } let valueElement = this.createFieldValueElement( val, fieldName @@ -771,7 +779,7 @@ // or at beginning var field = this.getTitleField(); if (!field) { - field = this._infoTable.querySelector('[fieldName="itemType"]'); + field = this._infoTable.querySelector('.meta-label-itemType'); } if (field) { this._firstRowBeforeCreators = field.closest(".meta-row").nextSibling; @@ -780,52 +788,55 @@ this._firstRowBeforeCreators = this._infoTable.firstChild; } - this._creatorCount = 0; - var num = this.item.numCreators(); - if (num > 0) { - // Limit number of creators display - var max = Math.min(num, this._initialVisibleCreators); - // If only 1 or 2 more, just display - if (num < max + 3 || this._displayAllCreators) { - max = num; - } - for (let i = 0; i < max; i++) { - let data = this.item.getCreator(i); - this.addCreatorRow(data, data.creatorTypeID, false); - } - if (this._draggedCreator) { - this._draggedCreator = false; - // Block hover effects on creators, enable them back on first mouse movement. - // See comment in creatorDragPlaceholder() for explanation - for (let label of document.querySelectorAll(".meta-label[fieldname^='creator-']")) { - label.closest(".meta-row").classList.add("noHover"); + let max; + this._addCreatorRowsBulk(() => { + this._creatorCount = 0; + var num = this.item.numCreators(); + if (num > 0) { + // Limit number of creators display + max = Math.min(num, this._initialVisibleCreators); + // If only 1 or 2 more, just display + if (num < max + 3 || this._displayAllCreators) { + max = num; + } + for (let i = 0; i < max; i++) { + let data = this.item.getCreator(i); + this.addCreatorRow(data, data.creatorTypeID, false); + } + if (this._draggedCreator) { + this._draggedCreator = false; + // Block hover effects on creators, enable them back on first mouse movement. + // See comment in creatorDragPlaceholder() for explanation + for (let creatorValue of document.querySelectorAll(".creator-type-value")) { + creatorValue.closest(".meta-row").classList.add("noHover"); + } + let removeHoverBlock = () => { + let noHoverRows = document.querySelectorAll('.noHover'); + noHoverRows.forEach(el => el.classList.remove('noHover')); + document.removeEventListener('mousemove', removeHoverBlock); + }; + document.addEventListener('mousemove', removeHoverBlock); + } + + // Additional creators not displayed + if (num > max) { + this.addMoreCreatorsRow(num - max); + } + else { + // If we didn't start with creators truncated, + // don't truncate for as long as we're viewing + // this item, so that added creators aren't + // immediately hidden + this._displayAllCreators = true; } - let removeHoverBlock = () => { - let noHoverRows = document.querySelectorAll('.noHover'); - noHoverRows.forEach(el => el.classList.remove('noHover')); - document.removeEventListener('mousemove', removeHoverBlock); - }; - document.addEventListener('mousemove', removeHoverBlock); } - - // Additional creators not displayed - if (num > max) { - this.addMoreCreatorsRow(num - max); + else if (this.editable && Zotero.CreatorTypes.itemTypeHasCreators(this.item.itemTypeID)) { + // Add default row + this.addCreatorRow(false, false, false); } - else { - // If we didn't start with creators truncated, - // don't truncate for as long as we're viewing - // this item, so that added creators aren't - // immediately hidden - this._displayAllCreators = true; - } - } - else if (this.editable && Zotero.CreatorTypes.itemTypeHasCreators(this.item.itemTypeID)) { - // Add default row - this.addCreatorRow(false, false, false); - } - - + }); + + if (this._showCreatorTypeGuidance) { let creatorTypeLabels = this.querySelectorAll(".creator-type-label"); this._id("zotero-author-guidance").show({ @@ -834,9 +845,6 @@ this._showCreatorTypeGuidance = false; } - this._ensureButtonsFocusable(); - this._updateCreatorButtonsStatus(); - // Set focus on the last focused field this._restoreFieldFocus(); @@ -869,7 +877,7 @@ // If rowIDs are provided, always update them if (rowIDs?.length > 0) { for (let rowID of rowIDs) { - let rowElem = this._infoTable.querySelector(`[data-custom-row-id="${CSS.escape(rowID)}"]`); + let rowElem = this._infoTable.querySelector(`.meta-row[data-custom-row-id="${CSS.escape(rowID)}"]`); if (!rowElem) continue; this.updateCustomRowData(rowElem); } @@ -887,7 +895,7 @@ // Add rows that are in the target rows but not in the current rows for (let row of targetRows) { - let rowElem = this._infoTable.querySelector(`[data-custom-row-id="${CSS.escape(row.rowID)}"]`); + let rowElem = this._infoTable.querySelector(`.meta-row[data-custom-row-id="${CSS.escape(row.rowID)}"]`); if (rowElem) { // If the row is already in the table, and not already updated, update it if (!rowIDs?.includes(row.rowID)) { @@ -984,7 +992,7 @@ } case "end": default: { - let dateAddedRow = this._infoTable.querySelector(".meta-label[fieldname=dateAdded]")?.parentElement; + let dateAddedRow = this._infoTable.querySelector(".meta-label-dateAdded")?.parentElement; if (dateAddedRow) { this._infoTable.insertBefore(rowElem, dateAddedRow); } @@ -1063,7 +1071,7 @@ var row = document.createElement('div'); row.className = "meta-row"; var labelWrapper = document.createElement('div'); - labelWrapper.className = "meta-label"; + labelWrapper.className = "meta-label meta-label-itemType"; labelWrapper.setAttribute("fieldname", "itemType"); var label = this.createLabelElement({ id: "itembox-field-itemType-label", @@ -1150,6 +1158,39 @@ return row; } + _addCreatorRowsBulk(fn) { + this._addingCreatorRowsInBulk = true; + // Remove unsaved creator row in the first addCreatorRow() invocation + this._needsUnsavedCreatorRemoval = true; + try { + fn(); + } + finally { + this._addingCreatorRowsInBulk = false; + this._needsUnsavedCreatorRemoval = false; + this._finishCreatorRowChanges(); + } + } + + /** + * Perform final work after adding one or more creator rows: + * - Size added name fields to their content + * - Ensure button focusability + * - Update hidden/disabled status of each button + */ + _finishCreatorRowChanges() { + if (this._addingCreatorRowsInBulk) { + return; + } + let fieldsToSize = this._pendingCreatorSizing; + this._pendingCreatorSizing = []; + if (fieldsToSize.length) { + customElements.get("editable-text").batchSizeToContent(fieldsToSize); + } + this._ensureButtonsFocusable(); + this._updateCreatorButtonsStatus(); + } + addCreatorRow(creatorData, creatorTypeIDOrName, unsaved, before) { // getCreatorFields(), switchCreatorMode() and handleCreatorAutoCompleteSelect() // may need need to be adjusted if this DOM structure changes @@ -1237,7 +1278,7 @@ fieldName, ) ); - + lastNameElem.classList.add("creator-last-name"); lastNameElem.placeholder = this._defaultLastName; fieldName = 'creator-' + rowIndex + '-firstName'; var firstNameElem = firstlast.appendChild( @@ -1246,6 +1287,7 @@ fieldName, ) ); + firstNameElem.classList.add("creator-first-name"); firstNameElem.placeholder = this._defaultFirstName; if (fieldMode > 0) { firstlast.lastChild.hidden = true; @@ -1319,8 +1361,18 @@ this._creatorCount++; - // Delete existing unsaved creator row if any - this.removeUnsavedCreatorRow(); + // Delete existing unsaved creator row, if any. + // During a bulk add, this only needs to run once, on the first row, rather than + // repeating the slow removeUnsavedCreatorRow() procedure for every row in the loop. + if (this._addingCreatorRowsInBulk) { + if (this._needsUnsavedCreatorRemoval) { + this._needsUnsavedCreatorRemoval = false; + this.removeUnsavedCreatorRow(); + } + } + else { + this.removeUnsavedCreatorRow(); + } // If this creator row's type was just switched, remove ".show-on-hover" to avoid buttons appearing // and then immediately disappearing when the css rule kicks in if the row is hovered. @@ -1335,8 +1387,6 @@ } let row = this.addDynamicRow(rowLabel, rowData, before); - this._ensureButtonsFocusable(); - /** * Events handling creator drag-drop reordering */ @@ -1385,10 +1435,14 @@ this.switchCreatorMode(rowData.parentNode, 0, true, false, rowIndex); } - lastNameElem.sizeToContent(); - firstNameElem.sizeToContent(); + // Queue the name fields to be sized to their content. The actual sizing is batched in + // _finishCreatorRowChanges() so that all fields added in one operation are measured and + // resized together, which is many orders of magnitude faster than sizing each field + // individually. + this._pendingCreatorSizing.push(lastNameElem, firstNameElem); if (!this.editable) { + this._finishCreatorRowChanges(); return; } @@ -1407,11 +1461,14 @@ // Focus unsaved empty creator row if (unsaved) { rowData.setAttribute("unsaved", true); + // Mirror the unsaved attribute with a class so we never have to match on [unsaved=true] + rowData.classList.add("unsaved-creator"); lastNameElem.focus(); } - // Refresh creator buttons status, e.g. to disable + button of a row that just added - // a new creator - this._updateCreatorButtonsStatus(); + + // Finalize sizing/button state. A no-op during a bulk add, which finalizes once at the + // end (see _addCreatorRowsBulk()). + this._finishCreatorRowChanges(); } addMoreCreatorsRow(num) { @@ -1856,7 +1913,7 @@ } removeUnsavedCreatorRow(onlyIfEmpty = false) { - let unsavedCreatorData = this._infoTable.querySelector(".creator-type-value[unsaved=true]"); + let unsavedCreatorData = this._infoTable.querySelector(".creator-type-value.unsaved-creator"); if (!unsavedCreatorData) return; let { firstName, lastName } = this.getCreatorFields(unsavedCreatorData.parentNode); let isEmpty = firstName == "" && lastName == ""; @@ -1864,7 +1921,7 @@ unsavedCreatorData.closest(".meta-row").remove(); this._creatorCount--; - this._updateCreatorButtonsStatus(); + this._finishCreatorRowChanges(); } dateTimeFromUTC(valueText) { @@ -2102,7 +2159,7 @@ this._forceRenderAll(); } } - if (event.key == "Escape" && row.querySelector(".creator-type-value[unsaved=true]")) { + if (event.key == "Escape" && row.querySelector(".creator-type-value.unsaved-creator")) { // Escape on an unsaved row deletes it and focuses previous creator event.stopPropagation(); row.previousElementSibling.querySelector("editable-text").focus(); @@ -2336,13 +2393,13 @@ // Make sure that irrelevant creators +/- buttons are disabled _updateCreatorButtonsStatus() { - let creatorValues = [...this.querySelectorAll(".creator-type-value")]; + let creatorValues = this.querySelectorAll(".creator-type-value"); let row; for (let creatorValue of creatorValues) { row = creatorValue.closest(".meta-row"); let { lastName, firstName } = this.getCreatorFields(row); let isEmpty = lastName == "" && firstName == ""; - let isNextRowUnsavedCreator = row.nextSibling?.querySelector(".creator-type-value[unsaved=true]"); + let isNextRowUnsavedCreator = row.nextSibling?.querySelector(".creator-type-value.unsaved-creator"); let isDefaultEmptyRow = isEmpty && creatorValues.length == 1; if (!this.editable) { @@ -2359,26 +2416,38 @@ } getCreatorFields(row) { - var typeID = row.querySelector('[typeid]').getAttribute('typeid'); + var typeID = row.querySelector('.meta-label').getAttribute('typeid'); var [label1, label2] = row.querySelectorAll('editable-text'); - var fieldMode = row.querySelector('[fieldMode]')?.getAttribute('fieldMode'); - let isUnsavedRow = !!row.querySelector("[unsaved=true]"); - // Calculate the index this row will occupy after the new row (if it exists) is saved. - // This is used for focus management. - let creatorsData = [...this.querySelectorAll(".creator-type-value")]; - let position = creatorsData.findIndex(node => node.parentNode == row); - if (position == -1) { - position = null; - } - var fields = { + var fieldMode = label1?.getAttribute('fieldMode'); + let isUnsavedRow = !!row.querySelector(".creator-type-value.unsaved-creator"); + let position; + + let fields = { lastName: label1.value.trim(), firstName: label2.value.trim(), fieldMode: fieldMode ? parseInt(fieldMode) : 0, creatorTypeID: parseInt(typeID), - position: position, isUnsaved: isUnsavedRow }; - + Object.defineProperty(fields, 'position', { + // Calculate the index this row will occupy after the new row (if it exists) is saved. + // This is used for focus management. + // (We compute this lazily, since the procedure is relatively slow and most callers + // don't need it. Needs to be a lambda to avoid aliasing `this`.) + get: () => { + if (position === undefined) { + let creatorsData = [...this.querySelectorAll(".creator-type-value")]; + position = creatorsData.findIndex(node => node.parentNode == row); + if (position == -1) { + position = null; + } + } + return position; + }, + set: () => { + throw new Error('position is read-only'); + }, + }); return fields; } @@ -2410,7 +2479,7 @@ */ async swapNames(_event) { var row = this._popupNode.closest('.meta-row'); - var typeBox = row.querySelector('[fieldname]'); + var typeBox = row.querySelector('.meta-label'); var creatorIndex = parseInt(typeBox.getAttribute('fieldname').split('-')[1]); var fields = this.getCreatorFields(row); var lastName = fields.lastName; @@ -2437,7 +2506,7 @@ var row = this._popupNode.closest('.meta-row'); let label = row.querySelector('.meta-label'); var creatorIndex = parseInt(label.getAttribute('fieldname').split('-')[1]); - let [lastName, firstName] = [...row.querySelectorAll("editable-text")]; + let [lastName, firstName] = row.querySelectorAll("editable-text"); lastName.value = Zotero.Utilities.capitalizeName(lastName.value); firstName.value = Zotero.Utilities.capitalizeName(firstName.value); var fields = this.getCreatorFields(row); @@ -2478,8 +2547,8 @@ // after creator is dropped, the hover effect often stays at // the row's old location. To workaround that, set noHover class to block all // hover effects on creator rows and then remove it on the first mouse movement in refresh(). - for (let label of document.querySelectorAll(".meta-label[fieldname^='creator-']")) { - label.closest(".meta-row").classList.add("noHover"); + for (let creatorValue of document.querySelectorAll(".creator-type-value")) { + creatorValue.closest(".meta-row").classList.add("noHover"); } // Un-hide the moved creator row this.querySelector(".drag-hidden-creator").classList.remove("drag-hidden-creator"); @@ -2920,7 +2989,7 @@ var index = parseInt(typeBox.getAttribute('fieldname').split('-')[1]); var item = this.item; var exists = item.hasCreatorAt(index); - var fieldMode = row.querySelector("[fieldMode]").getAttribute("fieldMode"); + var fieldMode = row.querySelector(".creator-last-name").getAttribute("fieldMode"); var moreCreators = item.numCreators() > index + 1; @@ -3018,7 +3087,7 @@ this._clearSavedFieldFocus(); } // If user moves focus outside of empty unsaved creator row, remove it. - let unsavedCreatorRow = this.querySelector(".creator-type-value[unsaved=true]")?.closest(".meta-row"); + let unsavedCreatorRow = this.querySelector(".creator-type-value.unsaved-creator")?.closest(".meta-row"); // But not if these parent components receive focus which happens when menus are opened if (["zotero-view-item", "main-window"].includes(focused.id) || !unsavedCreatorRow) return; let focusLeftUnsavedCreatorRow = !unsavedCreatorRow.contains(focused); From fa3e0f683f392b9d0caae7cb3fc3d68b07e65e2c Mon Sep 17 00:00:00 2001 From: abaevbog Date: Tue, 9 Jun 2026 09:35:30 -0700 Subject: [PATCH 003/465] Citation dialog: display preview of the citation (#5916) In Add/Edit Citation mode, display a preview of the citation in the bottom section. The section can be hidden/displayed via the toggle in the right corner. Remove io.preview from editor instance, so that citation dialog knows not to show the preview even if the preference is set. A minor refactor to have resizeWindow() resolve when the animation is fully over, and clear minHeight on window in list mode before resizing, restoring it when resizing animation is done, same as in library mode. It allows us to fully expand the window in list mode before showing the preview. Fixes: zotero#5910 --- .../zotero/integration/citationDialog.js | 151 +++++++++++++++--- .../zotero/integration/citationDialog.xhtml | 13 +- chrome/content/zotero/xpcom/editorInstance.js | 8 - chrome/content/zotero/xpcom/integration.js | 9 +- chrome/locale/en-US/zotero/integration.ftl | 3 + .../20/universal/dialog-citation-preview.svg | 3 + defaults/preferences/zotero.js | 1 + scss/components/_citationDialog.scss | 47 +++++- test/tests/citationDialogTest.js | 2 + 9 files changed, 198 insertions(+), 39 deletions(-) create mode 100644 chrome/skin/default/zotero/20/universal/dialog-citation-preview.svg diff --git a/chrome/content/zotero/integration/citationDialog.js b/chrome/content/zotero/integration/citationDialog.js index 1dff8c0b15..c35d1365bb 100644 --- a/chrome/content/zotero/integration/citationDialog.js +++ b/chrome/content/zotero/integration/citationDialog.js @@ -131,6 +131,8 @@ async function onLoad() { await IOManager.toggleDialogMode(initialMode); // most of IO handling relies on currentLayout being defined so it must follow setInitialDialogMode IOManager.init(); + // set the text of the citation preview + CitationPreview.update(); // explicitly focus bubble input so one can begin typing right away _id("bubble-input").refocusInput(); // wait to call functions that rely on io.getItems() or io.sort() till all cited data is loaded @@ -258,8 +260,9 @@ async function setDialogType(type) { _id("keepSorted").disabled = !io.sortable || !DIALOG_STATE.isCitingItems(); _id("keepSorted").checked = !_id("keepSorted").disabled && !io.citation.properties.unsorted; if (DIALOG_STATE.isCitingItems()) { - _id("settings-button").hidden = !io.sortable; _id("keepSorted").disabled = !io.sortable; + _id("keepSorted").parentElement.hidden = !io.sortable; + CitationPreview.update(); if (!DIALOG_STATE.loaded) { _id("keepSorted").checked = io.sortable && !io.citation.properties.unsorted; } @@ -596,14 +599,16 @@ class LibraryLayout extends Layout { IOManager.updateBubbleInput(); } + // Resolves once the resize animation has fully completed async resizeWindow() { await Helpers.smoothResizingPromise; let bubbleInputHeight = Helpers.getSearchRowHeight(); let suggestedItemsHeight = _id("library-other-items").getBoundingClientRect().height; let minTableHeight = 400; + let citationPreview = _id("citation-preview").getBoundingClientRect().height; let bottomHeight = _id("bottom-area-wrapper").getBoundingClientRect().height; - let minHeight = bubbleInputHeight + suggestedItemsHeight + bottomHeight + minTableHeight; + let minHeight = bubbleInputHeight + suggestedItemsHeight + citationPreview + bottomHeight + minTableHeight; let targetWidth = Math.max(window.innerWidth, this.MIN_WIDTH); let targetHeight = Math.max(minHeight, lastSetWindowHeight); @@ -612,13 +617,16 @@ class LibraryLayout extends Layout { if (needsResize) { doc.documentElement.style.removeProperty('min-height'); ignoreWindowResizing = true; - Helpers.smoothResize(targetWidth, targetHeight, { - onComplete: () => { - _id("bubble-input").refocusInput(); - doc.documentElement.style.minHeight = `${minHeight}px`; - document.documentElement.setAttribute("dialog-layout", this.type); - ignoreWindowResizing = false; - }, + await new Promise((resolve) => { + Helpers.smoothResize(targetWidth, targetHeight, { + onComplete: () => { + _id("bubble-input").refocusInput(); + doc.documentElement.style.minHeight = `${minHeight}px`; + document.documentElement.setAttribute("dialog-layout", this.type); + ignoreWindowResizing = false; + resolve(); + }, + }); }); } // ensure dialog-layout and min-height is set even if window does not need resizing @@ -1092,6 +1100,7 @@ class ListLayout extends Layout { IOManager.updateBubbleInput(); } + // Resolves only once the resize animation has fully completed async resizeWindow() { await Helpers.smoothResizingPromise; let bubbleInputHeight = Helpers.getSearchRowHeight(); @@ -1112,11 +1121,12 @@ class ListLayout extends Layout { marginOfError = Zotero.isWin ? 6 : 2; } - // height of the bottom section + // height of citation preview (0 when hidden) and the bottom section + let citationPreview = _id("citation-preview").getBoundingClientRect().height; let bottomHeight = _id("bottom-area-wrapper").getBoundingClientRect().height; // set min height and resize the window - let autoHeight = bubbleInputHeight + sectionsHeight + sectionsWrapperPadding + bottomHeight + marginOfError; + let autoHeight = bubbleInputHeight + sectionsHeight + sectionsWrapperPadding + citationPreview + bottomHeight + marginOfError; // window.resizeTo(X,Y) resizes the window so that it's outerHeight == Y. On mac and windows, // innerHeight and outerHeight are the same. On linux, the outerHeight > innerHeight, perhaps // outerHeight there includes chrome, borders, etc. This difference is accounted for below, so that the dialog @@ -1124,24 +1134,39 @@ class ListLayout extends Layout { if (Zotero.isLinux) { autoHeight += (window.outerHeight - window.innerHeight); } - let minHeight = bubbleInputHeight + bottomHeight; - doc.documentElement.style.minHeight = `${minHeight}px`; + let minHeight = bubbleInputHeight + citationPreview + bottomHeight; // cap window height at the height last set by the user autoHeight = Math.min(autoHeight, lastSetWindowHeight); let targetWidth = Math.min(window.innerWidth, this.MIN_WIDTH); + + // Skip the resize animation if the window is already at the target size. + let needsResize = Math.round(window.innerWidth) !== Math.round(targetWidth) || Math.round(window.innerHeight) !== Math.round(autoHeight); + if (!needsResize) { + doc.documentElement.style.minHeight = `${minHeight}px`; + document.documentElement.setAttribute("dialog-layout", this.type); + return; + } + + // Clear the min-height floor so the window can animate freely (including shrinking); + // it's restored to the new value in onComplete below. + doc.documentElement.style.removeProperty("min-height"); ignoreWindowResizing = true; - // Timeout is required likely to allow minHeight update to settle - setTimeout(() => { - Helpers.smoothResize(targetWidth, autoHeight, { - onComplete: () => { - _id("bubble-input").refocusInput(); - document.documentElement.setAttribute("dialog-layout", this.type); - ignoreWindowResizing = false; - }, - }); - }, 10); + // Timeout is required likely to allow the min-height removal to settle + await new Promise((resolve) => { + setTimeout(() => { + Helpers.smoothResize(targetWidth, autoHeight, { + onComplete: () => { + _id("bubble-input").refocusInput(); + doc.documentElement.style.minHeight = `${minHeight}px`; + document.documentElement.setAttribute("dialog-layout", this.type); + ignoreWindowResizing = false; + resolve(); + }, + }); + }, 10); + }); } _markRoundedCorners() { @@ -1224,6 +1249,7 @@ const IOManager = { }); _id("includeComments").addEventListener("click", () => this._toggleIncludeComments()); + _id("display-preview-button").addEventListener("click", () => this._toggleDisplayPreview()); // open settings popup on btn click _id("settings-button").addEventListener("click", event => _id("settings-popup").openPopup(event.target, "before_end")); @@ -1267,10 +1293,12 @@ const IOManager = { let isInitialModeSetting = currentLayout === undefined; currentLayout = newMode === "library" ? libraryLayout : listLayout; + // Reflect visibility of the citation preview after the layout switch. + CitationPreview.update(); + // Wait for window resize before running search to avoid stutter with large libraries if (!isInitialModeSetting) { await currentLayout.resizeWindow(); - await Helpers.smoothResizingPromise; } // do not show View menubar with itemTree-specific options in list mode @@ -1317,6 +1345,7 @@ const IOManager = { }; }), DIALOG_STATE.type); _id("accept-button").disabled = !CitationDataManager.items.length; + CitationPreview.update(); }, async addItemsToCitation(items, { noInputRefocus, index } = { index: null }) { @@ -1394,6 +1423,9 @@ const IOManager = { doc.querySelector("guidance-panel").setAttribute("x", Math.round(width / 2)); IOManager.showFirstRunDialog(); } + // Render the preview before refreshing the list so resizeWindow measures its real height; + // otherwise the debounced render lands after the resize and overflows the window. + await CitationPreview.render(); // Always refresh items list to make sure the opened and selected items are up to date await currentLayout.refreshItemsList(); if (!noInputRefocus) { @@ -1862,6 +1894,31 @@ const IOManager = { Zotero.Prefs.set("integration.annotationDialogIncludeComments", includeComments); }, + async _toggleDisplayPreview() { + let newShown = !Zotero.Prefs.get("integration.citationPreviewShown"); + Zotero.Prefs.set("integration.citationPreviewShown", newShown); + // Reflect the pressed state right away, since revealing the preview is deferred until resize + _id("display-preview-button").setAttribute("aria-pressed", newShown ? "true" : "false"); + let preview = _id("citation-preview"); + if (newShown) { + // Lay the preview out off-flow via .measuring (real height, but invisible and not + // pushing the list around) so resizeWindow grows the window to fit it; then drop it + // into view once there's room, avoiding a momentary squeeze of the list. + await CitationPreview.render(); + preview.classList.add("measuring"); + preview.hidden = false; + await currentLayout.resizeWindow(); + preview.classList.remove("measuring"); + CitationPreview.update(); + } + else { + // Hide first, then shrink -- freeing the space before the window contracts looks clean. + preview.classList.remove("measuring"); + CitationPreview.update(); + currentLayout.resizeWindow(); + } + }, + // Return focus to where it was before click moved focus. // If it's not possible, refocus the last input in bubble-input so that // focus is not just lost. @@ -1939,6 +1996,52 @@ const IOManager = { } }; +// Manages the citation preview shown in the bottom area of both layouts. +const CitationPreview = { + // Lazily create _renderDebounced on first use + get _renderDebounced() { + delete CitationPreview._renderDebounced; + CitationPreview._renderDebounced = Zotero.Utilities.debounce(() => CitationPreview.render(), 250); + return CitationPreview._renderDebounced; + }, + + // The rendered text is kept in sync with the cited items even while the preview is hidden, + // so it can be measured and revealed instantly when toggled on. + update() { + let prefShown = Zotero.Prefs.get("integration.citationPreviewShown"); + let isCitingItems = DIALOG_STATE.isCitingItems(); + let hasPreview = !!io.preview; + let shouldShow = isCitingItems && prefShown && hasPreview; + _id("citation-preview").hidden = !shouldShow; + let isEmpty = !CitationDataManager.items.length; + _id("citation-preview-empty").hidden = !isEmpty; + _id("citation-preview-content").hidden = isEmpty; + if (isEmpty) { + _id("citation-preview-content").innerHTML = ""; + } + // The toggle button only makes sense while citing items with a backing + // preview function. It reflects the pref directly. + let toggleBtn = _id("display-preview-button"); + toggleBtn.hidden = !(isCitingItems && hasPreview); + toggleBtn.setAttribute("aria-pressed", prefShown ? "true" : "false"); + if (!isEmpty) { + CitationPreview._renderDebounced(); + } + }, + + async render() { + if (!DIALOG_STATE.isCitingItems()) return; + if (!CitationDataManager.items.length) return; + if (!io.preview) return; + + CitationDataManager.updateCitationObject(); + let html = await io.preview("html"); + // Re-check after the await in case the user cleared items + if (!CitationDataManager.items.length) return; + _id("citation-preview-content").innerHTML = html; + }, +}; + // Representation of a single entry in the citation. class BubbleItem { // Can be created from either Zotero.Item or citation item from io.citation.citationItems diff --git a/chrome/content/zotero/integration/citationDialog.xhtml b/chrome/content/zotero/integration/citationDialog.xhtml index 722af2ae97..8116cc596f 100644 --- a/chrome/content/zotero/integration/citationDialog.xhtml +++ b/chrome/content/zotero/integration/citationDialog.xhtml @@ -105,6 +105,14 @@
+
+
+
+
+
+
+
+
-
+ + +
@@ -127,7 +137,6 @@
-
diff --git a/chrome/content/zotero/xpcom/editorInstance.js b/chrome/content/zotero/xpcom/editorInstance.js index 4d10d37834..bc70e51a1a 100644 --- a/chrome/content/zotero/xpcom/editorInstance.js +++ b/chrome/content/zotero/xpcom/editorInstance.js @@ -1226,14 +1226,6 @@ class EditorInstance { // Otherwise returns `undefined` which makes this function to be }, - /** - * Execute a callback with a preview of the given citation - * @return {Promise} A promise resolved with the previewed citation string - */ - preview: async function () { - // Zotero.debug('CI: preview'); - }, - /** * Sort the citationItems within citation (depends on this.citation.properties.unsorted) * @return {Promise} A promise resolved with the previewed citation string diff --git a/chrome/content/zotero/xpcom/integration.js b/chrome/content/zotero/xpcom/integration.js index ab5235c61d..a474dde978 100644 --- a/chrome/content/zotero/xpcom/integration.js +++ b/chrome/content/zotero/xpcom/integration.js @@ -1564,7 +1564,7 @@ Zotero.Integration.Session.prototype.cite = async function (field, addNote=false this.updateFromDocument(FORCE_CITATIONS_FALSE).then(() => this.citationsByItemID); } - var previewFn = async function (citation) { + var previewFn = async function (citation, format) { let idx = await fieldIndexPromise; await citationsByItemIDPromise; @@ -1580,7 +1580,7 @@ Zotero.Integration.Session.prototype.cite = async function (field, addNote=false let citationsPost = citations.slice(sliceIdx); let citationID = citation.citationID; try { - var result = this.style.previewCitationCluster(citation, citationsPre, citationsPost, "rtf"); + var result = this.style.previewCitationCluster(citation, citationsPre, citationsPost, format || "rtf"); } catch(e) { throw e; } finally { @@ -1811,10 +1811,11 @@ Zotero.Integration.CitationEditInterface = function (items, sortable, fieldIndex Zotero.Integration.CitationEditInterface.prototype = { /** * Execute a callback with a preview of the given citation + * @param {String} [format] Override the default output format (e.g. "html" for use in citation dialog) * @return {Promise} A promise resolved with the previewed citation string */ - preview: function () { - return this.previewFn(this.citation); + preview: function (format) { + return this.previewFn(this.citation, format); }, /** diff --git a/chrome/locale/en-US/zotero/integration.ftl b/chrome/locale/en-US/zotero/integration.ftl index 36b8d0b9a0..fdc2d14062 100644 --- a/chrome/locale/en-US/zotero/integration.ftl +++ b/chrome/locale/en-US/zotero/integration.ftl @@ -49,6 +49,9 @@ integration-citationDialog-lib-message-annotations = { $search -> *[other] No selected or open items with annotations } integration-citationDialog-settings-keepSorted = Keep sources sorted +integration-citationDialog-preview-empty = Preview +integration-citationDialog-btn-displayPreview = + .title = Display citation preview integration-citationDialog-btn-settings = .title = { general-open-settings } integration-citationDialog-mode-library = Library diff --git a/chrome/skin/default/zotero/20/universal/dialog-citation-preview.svg b/chrome/skin/default/zotero/20/universal/dialog-citation-preview.svg new file mode 100644 index 0000000000..5a35d2c913 --- /dev/null +++ b/chrome/skin/default/zotero/20/universal/dialog-citation-preview.svg @@ -0,0 +1,3 @@ + + + diff --git a/defaults/preferences/zotero.js b/defaults/preferences/zotero.js index 054b7bfd43..3c66fa7089 100644 --- a/defaults/preferences/zotero.js +++ b/defaults/preferences/zotero.js @@ -153,6 +153,7 @@ pref("extensions.zotero.integration.upgradeTemplateDelayedOn", 0); pref("extensions.zotero.integration.dontPromptMendeleyImport", false); pref("extensions.zotero.integration.citationDialogMode", "last-used"); pref("extensions.zotero.integration.annotationDialogIncludeComments", true); +pref("extensions.zotero.integration.citationPreviewShown", true); // Connector settings pref("extensions.zotero.httpServer.enabled", true); diff --git a/scss/components/_citationDialog.scss b/scss/components/_citationDialog.scss index df915ef9f3..84810ec27f 100644 --- a/scss/components/_citationDialog.scss +++ b/scss/components/_citationDialog.scss @@ -110,6 +110,11 @@ .divider { border-bottom: 1px solid var(--color-panedivider); margin: 0; + + &.subtle { + border-bottom-color: var(--fill-quinary); + margin: 0 8px; + } } .add-all { @@ -547,8 +552,42 @@ } } + #citation-preview { + padding-top: 6px; + + // While the preview is being toggled on, lay it out off-flow so the window can grow to + // fit it before it's revealed: absolute keeps a real, measurable height without pushing + // the list around, and visibility:hidden keeps it invisible until there's room for it. + // inset-inline: 0 matches the in-flow width (body is the flow parent), so the measured + // height is accurate. + &.measuring { + position: absolute; + inset-inline: 0; + visibility: hidden; + } + + #citation-preview-wrapper { + padding: 2px 12px 8px 12px; + min-height: 20px; + max-height: 160px; + overflow-y: auto; + -moz-window-dragging: no-drag; + font-family: "Times New Roman"; + + #citation-preview-content { + color: var(--fill-primary); + font-size: 1.1rem; + } + + #citation-preview-empty { + padding-top: 2px; + color: var(--fill-secondary); + font-family: $font-family-base; + } + } + } + #bottom-area-wrapper { - border-top: var(--material-panedivider); padding: 4px 8px; .segmented-switch { @@ -647,6 +686,12 @@ height: 28px; gap: 8px; -moz-window-dragging: no-drag; + #display-preview-button { + @include svgicon("dialog-citation-preview", "universal", "20"); + &[aria-pressed="true"] { + background-color: var(--fill-quinary); + } + } #settings-button { @include svgicon("dialog-options", "universal", "16"); } diff --git a/test/tests/citationDialogTest.js b/test/tests/citationDialogTest.js index 2c8c809fe0..d3e9dd605e 100644 --- a/test/tests/citationDialogTest.js +++ b/test/tests/citationDialogTest.js @@ -13,6 +13,7 @@ describe("Citation Dialog", function () { getItems() { return []; }, + preview: () => {}, allCitedDataLoadedPromise: Promise.resolve(), }; let dialog, win, doc, IOManager, CitationDataManager, SearchHandler; @@ -661,6 +662,7 @@ describe("Citation Dialog", function () { getItems() { return new Zotero.Promise(() => {}); }, + preview: () => {}, allCitedDataLoadedPromise: new Zotero.Promise(() => {}), }; From 3e7030a642db231193183acc937fb73578bb45df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adomas=20Ven=C4=8Dkauskas?= Date: Wed, 10 Jun 2026 12:31:08 +0300 Subject: [PATCH 004/465] Update Word for Windows submodule --- app/modules/zotero-word-for-windows-integration | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/modules/zotero-word-for-windows-integration b/app/modules/zotero-word-for-windows-integration index 72e8364775..27b4aea971 160000 --- a/app/modules/zotero-word-for-windows-integration +++ b/app/modules/zotero-word-for-windows-integration @@ -1 +1 @@ -Subproject commit 72e83647756b759ec80923922218fff8bf8b0343 +Subproject commit 27b4aea971dd2c10e4e484f48ded60385a6839c2 From fde4086e08c248b376744a771bb46af71fc86f17 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Tue, 9 Jun 2026 11:31:01 -0400 Subject: [PATCH 005/465] Show file-access error when storage directory can't be cleared on download createDirectoryForItem() wipes and recreates the item's storage directory before moving in a downloaded file. If removeDir() failed (e.g., a locked file on Windows), the error bubbled up to zfs.js and became the generic sync error. Route it through checkFileAccessError() instead, so the user gets the actionable locked-file message and a Show Parent Directory button. https://forums.zotero.org/discussion/132097/ --- chrome/content/zotero/xpcom/storage/storageLocal.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/chrome/content/zotero/xpcom/storage/storageLocal.js b/chrome/content/zotero/xpcom/storage/storageLocal.js index 518ec58303..0598ec72a8 100644 --- a/chrome/content/zotero/xpcom/storage/storageLocal.js +++ b/chrome/content/zotero/xpcom/storage/storageLocal.js @@ -663,7 +663,14 @@ Zotero.Sync.Storage.Local = { throw new Error("Downloaded file not found"); } - await Zotero.Attachments.createDirectoryForItem(item); + try { + await Zotero.Attachments.createDirectoryForItem(item); + } + catch (e) { + Zotero.File.checkFileAccessError( + e, Zotero.Attachments.getStorageDirectory(item).path, 'create' + ); + } var filename = item.attachmentFilename; if (!filename) { From 4e3baf09a08f32eb4aee55e09918a7c6ab66305a Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Wed, 10 Jun 2026 15:02:19 -0400 Subject: [PATCH 006/465] Add okButtonLabel support to FilePicker Maps to nsIFilePicker.okButtonLabel, which customizes the label of the button used to accept the dialog, where supported by the platform. --- chrome/content/zotero/modules/filePicker.mjs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/chrome/content/zotero/modules/filePicker.mjs b/chrome/content/zotero/modules/filePicker.mjs index 47f07568ec..441d255766 100644 --- a/chrome/content/zotero/modules/filePicker.mjs +++ b/chrome/content/zotero/modules/filePicker.mjs @@ -121,7 +121,8 @@ FilePicker.prototype.filterAllowURLs = 0x80; FilePicker.prototype.filterAudio = 0x100; FilePicker.prototype.filterVideo = 0x200; -['addToRecentDocs', 'defaultExtension', 'defaultString', 'displayDirectory', 'filterIndex'].forEach((prop) => { +['addToRecentDocs', 'defaultExtension', 'defaultString', 'displayDirectory', 'filterIndex', + 'okButtonLabel'].forEach((prop) => { /** * @name FilePicker#addToRecentDocs * @type Boolean @@ -154,6 +155,12 @@ FilePicker.prototype.filterVideo = 0x200; * @desc The (0-based) index of the filter which is currently selected in the file picker dialog. * Set this to choose a particular filter to be selected by default. */ + /** + * @name FilePicker#okButtonLabel + * @type String + * @desc A custom label for the button the user uses to accept the file picker, if supported + * by the platform. This should be set before calling show(). + */ Object.defineProperty(FilePicker.prototype, prop, { // TODO: Others get: function () { From de1bf1cec1855400043d6b8edfa7b003736a35ad Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Wed, 10 Jun 2026 15:02:19 -0400 Subject: [PATCH 007/465] Update Mac Word plugin install flow for macOS 27 Golden Gate On macOS 27 and later, the installer gets access to the Word startup folder via a folder-selection dialog rather than an OS permission prompt, so support an adjusted banner message and add strings for the folder dialog. --- app/modules/zotero-word-for-mac-integration | 2 +- chrome/content/zotero/zoteroPane.js | 9 ++++++++- chrome/locale/en-US/zotero/zotero.ftl | 4 ++++ 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/app/modules/zotero-word-for-mac-integration b/app/modules/zotero-word-for-mac-integration index 084f16d78b..51faa4b21e 160000 --- a/app/modules/zotero-word-for-mac-integration +++ b/app/modules/zotero-word-for-mac-integration @@ -1 +1 @@ -Subproject commit 084f16d78b4924b15ad24c83dd5d8aaaeb9998d8 +Subproject commit 51faa4b21e1a433a6c0f69a4bdfc5a7882341f23 diff --git a/chrome/content/zotero/zoteroPane.js b/chrome/content/zotero/zoteroPane.js index 7ec522dac0..943686cb0a 100644 --- a/chrome/content/zotero/zoteroPane.js +++ b/chrome/content/zotero/zoteroPane.js @@ -6586,12 +6586,19 @@ var ZoteroPane = new function () { * before the installer displays the "scary" OS prompt to access other application data. * @returns {Promise} Object with either install, dismiss or remindLater set to true. */ - this.showMacWordPluginInstallWarning = function () { + this.showMacWordPluginInstallWarning = function (options = {}) { return new Promise((resolve) => { const panel = document.getElementById('mac-word-plugin-install-container'); + const message = document.querySelector('#mac-word-plugin-install-banner .message'); const action = document.getElementById('mac-word-plugin-install-action'); const remind = document.getElementById('mac-word-plugin-install-remind-later'); const dontAskAgain = document.getElementById('mac-word-plugin-install-dont-ask-again'); + + // On macOS 27 (Golden Gate) and later, the user allows the installation in a + // folder-selection dialog rather than approving an OS permission prompt + message.dataset.l10nId = options.folderAccess + ? 'mac-word-plugin-install-folder-message' + : 'mac-word-plugin-install-message'; // TODO: Replace with ftl string dontAskAgain.label = Zotero.getString('general.dontAskAgain'); diff --git a/chrome/locale/en-US/zotero/zotero.ftl b/chrome/locale/en-US/zotero/zotero.ftl index df65a30cbf..42cd1e48d8 100644 --- a/chrome/locale/en-US/zotero/zotero.ftl +++ b/chrome/locale/en-US/zotero/zotero.ftl @@ -862,12 +862,16 @@ text-action-paste-and-search = .label = Paste and Search mac-word-plugin-install-message = Zotero needs access to Word data to install the Word plugin. +mac-word-plugin-install-folder-message = { -app-name } needs access to Word’s startup folder to install the Word plugin. mac-word-plugin-install-action-button = .label = Install Word plugin mac-word-plugin-install-remind-later-button = .label = { general-remind-me-later } mac-word-plugin-install-dont-ask-again-button = .label = { general-dont-ask-again } +mac-word-plugin-install-folder-dialog-title = Install the plugin in the Word startup folder +mac-word-plugin-install-folder-dialog-button = Install +mac-word-plugin-install-wrong-folder-selected = The suggested folder must be selected. Please try again without choosing a different folder. file-renaming-banner-message = { -app-name } now automatically keeps attachment filenames in sync as you make changes to items. file-renaming-banner-documentation-link = { general-learn-more } From 06e16c297ce754ee9eb019f741e836a54f48cea9 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Wed, 10 Jun 2026 22:15:46 -0400 Subject: [PATCH 008/465] Fix file-change detection in all libraries after the first one synced The local-file-change watcher added in f21e1b2d32 accumulated changed item keys globally but was drained separately by each library's storage engine, with the drained keys filtered to that library. The first library to file-sync (normally My Library) consumed all pending events, and keys belonging to other libraries were silently discarded, so files modified on disk in group libraries were never marked for upload. The initial and periodic full-scan fallbacks on Windows and Linux were likewise global, so only the first library ever received them, and changes made in other libraries while Zotero was closed were never detected at all. The sync runner now drains the watcher once per sync session and immediately runs the modification check on the changed items across all libraries, recording any changes in the database, and the per-library storage engines skip the check entirely unless the watcher reports that the library needs a full scan: - On all platforms, a library that has never been scanned gets one full scan, which also gives libraries one recovery scan for changes dropped by affected releases. - On Windows/Linux, where the watchers only capture events while Zotero is running, each library gets a full scan on its first file sync of the session, on every manual sync, and daily during background syncs (instead of the previous 3-hour interval, which dated from when scans were the primary detection mechanism). - On macOS, libraries scanned since the last FSEvents journal discontinuity are tracked in a pref, since the journal -- and therefore the validity of previous scans -- survives restarts. Also: - Check FSEvents event flags and fall back to full scans when events were dropped or coalesced (MustScanSubDirs/UserDropped/KernelDropped/ EventIdsWrapped), and skip HistoryDone sentinel events - Disable the watcher for the session and fall back to legacy scanning on backend errors, including when the inotify watch limit is reached, instead of continuing with silently incomplete coverage - Prune scan records for deleted libraries, since SQLite can reuse a deleted library's libraryID https://forums.zotero.org/discussion/132120/ --- .../zotero/xpcom/storage/fileChangeWatcher.js | 236 ++++++++++++++++-- .../storage/fileChangeWatcher_fsevents.js | 33 ++- .../storage/fileChangeWatcher_inotify.js | 23 +- .../xpcom/storage/fileChangeWatcher_rdcw.js | 17 +- .../zotero/xpcom/storage/storageEngine.js | 35 +-- .../content/zotero/xpcom/sync/syncRunner.js | 3 + test/tests/fileChangeWatcherTest.js | 192 +++++++++++++- 7 files changed, 452 insertions(+), 87 deletions(-) diff --git a/chrome/content/zotero/xpcom/storage/fileChangeWatcher.js b/chrome/content/zotero/xpcom/storage/fileChangeWatcher.js index e4bdc7a8a1..2c7efc6b0e 100644 --- a/chrome/content/zotero/xpcom/storage/fileChangeWatcher.js +++ b/chrome/content/zotero/xpcom/storage/fileChangeWatcher.js @@ -34,6 +34,21 @@ * Windows -- ReadDirectoryChangesW (live recursive directory watch) * Linux -- inotify (live per-directory watches) * + * The storage directory is shared by all libraries, so backend events are drained once per sync + * session via snapshot(), which maps the changed item keys to items across all libraries and + * immediately runs the modification check on them, recording any changes in the database. The + * per-library storage engines then only need to ask needsFullScan() whether the watcher can be + * relied on for their library or a full scan is required -- because the library hasn't been + * scanned since a point where events may have been missed (watcher startup on the live-watcher + * platforms, or a backend fallback signal such as a missing event journal baseline, a buffer + * overflow, or an error). As insurance against silently dropped events, the live-watcher + * platforms also do a full scan on every manual sync and daily on background syncs. + * + * On macOS the set of libraries scanned since the last journal discontinuity is persisted to a + * pref, since the FSEvents journal survives restarts and full scans would otherwise be repeated + * (or, worse, skipped) after a restart. An absent pref means no library has been scanned, so + * every library gets one full scan the first time this code runs. + * * On unsupported platforms or on error, falls back to the existing scan logic. */ Zotero.Sync.Storage.FileChangeWatcher = { @@ -46,12 +61,20 @@ Zotero.Sync.Storage.FileChangeWatcher = { // Key validation pattern _keyPattern: /^[A-Z0-9]{8}$/, - // For live watcher backends (RDCW and inotify), track whether the first call has happened - // (returns null to trigger a full scan) and enforce periodic full scans every - // _MAX_WATCHER_AGE ms - _liveWatcherFirstCall: true, - _liveWatcherLastFullScanTime: 0, - _MAX_WATCHER_AGE: 10800000, // 3 hours -- matches storageEngine.maxCheckAge + // Whether snapshot() has been called since init + _snapshotTaken: false, + + // macOS: libraries fully scanned since the last event journal discontinuity (persisted) + _scannedLibraries: null, + + // Windows/Linux: libraryID -> time of the last full scan this session + _lastFullScan: {}, + + _SCANNED_LIBRARIES_PREF: 'sync.storage.watcher.scannedLibraries', + + // How long watcher results can be relied on for background syncs on the live-watcher + // platforms before a full scan is done as insurance against silently dropped events + _MAX_WATCHER_AGE: 86400000, // 24 hours init() { try { @@ -101,14 +124,139 @@ Zotero.Sync.Storage.FileChangeWatcher = { Zotero.logError(e); Zotero.debug("FileChangeWatcher: " + backendName + " init failed -- " + "falling back to legacy scanning"); + return; + } + + this._snapshotTaken = false; + this._lastFullScan = {}; + this._loadScannedLibraries(); + }, + + /** + * Drain changed item keys from the backend, map them to items across all libraries, and run + * the modification check on them, marking any changed files for upload in the database + * + * Called once per sync session, before the per-library storage engines run. + */ + async snapshot() { + if (!this.available) { + return; + } + this._snapshotTaken = true; + this._pruneScannedLibraries(); + let keys; + try { + keys = this.getChangedItemKeys(); + } + catch (e) { + Zotero.logError(e); + keys = null; + } + if (!keys) { + // Backend signaled fallback -- events may have been missed for any library, so + // require a full scan of each library + this._requireFullScans(); + return; + } + if (!keys.size) { + return; + } + try { + // Map keys to items across all libraries -- a key can match items in multiple + // libraries, and checkForUpdatedFiles() filters out unchanged files by mtime/hash + let itemIDsByLibrary = {}; + await Zotero.Utilities.Internal.forEachChunkAsync( + [...keys], + 500, + async (chunk) => { + let rows = await Zotero.DB.queryAsync( + "SELECT libraryID, itemID FROM items WHERE key IN (" + + chunk.map(() => '?').join(',') + ")", + chunk + ); + for (let row of rows) { + if (!itemIDsByLibrary[row.libraryID]) { + itemIDsByLibrary[row.libraryID] = []; + } + itemIDsByLibrary[row.libraryID].push(row.itemID); + } + } + ); + for (let libraryID in itemIDsByLibrary) { + await Zotero.Sync.Storage.Local.checkForUpdatedFiles( + parseInt(libraryID), itemIDsByLibrary[libraryID] + ); + } + } + catch (e) { + // Don't lose changes if the check failed + Zotero.logError(e); + this._requireFullScans(); } }, /** - * Get the set of item keys whose storage files have changed since the last call to this method. + * Whether a library needs a full modification scan instead of relying on watcher results * - * @return {Set|null} Set of 8-char item keys, or null to signal that the caller should fall - * back to a full scan + * The caller should call recordFullScan() once the scan has completed. + * + * @param {Integer} libraryID + * @param {Boolean} background - Whether this is a background sync -- on the live-watcher + * platforms, manual syncs always do a full scan as a user-triggered recovery mechanism + * @return {Boolean} + */ + needsFullScan(libraryID, background) { + if (!this.available) { + return true; + } + let reason = null; + if (!this._snapshotTaken) { + reason = "no snapshot taken"; + } + else if (this._backend == 'fsevents') { + if (!this._scannedLibraries.has(libraryID)) { + reason = "library not scanned since last event journal reset"; + } + } + else if (!background) { + reason = "manual sync"; + } + else { + let lastFullScan = this._lastFullScan[libraryID] || 0; + if (!lastFullScan) { + reason = "library not yet scanned this session"; + } + else if (Date.now() - lastFullScan > this._MAX_WATCHER_AGE) { + reason = "daily refresh"; + } + } + if (!reason) { + return false; + } + Zotero.debug("FileChangeWatcher: Full scan needed for library " + libraryID + + " -- " + reason); + return true; + }, + + /** + * Record that a full modification scan of a library was completed + * + * @param {Integer} libraryID + */ + recordFullScan(libraryID) { + this._lastFullScan[libraryID] = Date.now(); + this._scannedLibraries.add(libraryID); + if (this._backend == 'fsevents') { + this._saveScannedLibraries(); + } + }, + + /** + * Drain the set of item keys whose storage files have changed since the last drain from the + * platform backend + * + * @return {Set|null} Set of 8-char item keys, or null to signal that events may have been + * missed and callers should fall back to full scans */ getChangedItemKeys() { if (!this.available) { @@ -125,8 +273,13 @@ Zotero.Sync.Storage.FileChangeWatcher = { } } catch (e) { + // A backend exception means the watcher can no longer be trusted (e.g., the + // inotify watch limit was reached), so disable it and let the legacy scan logic + // take over for the rest of the session Zotero.logError(e); - Zotero.debug("FileChangeWatcher: getChangedItemKeys() failed -- signaling fallback"); + Zotero.debug("FileChangeWatcher: getChangedItemKeys() failed -- disabling for " + + "this session and falling back to legacy scanning"); + this.close(); } return null; }, @@ -145,6 +298,7 @@ Zotero.Sync.Storage.FileChangeWatcher = { } this._backend = null; this.available = false; + this._snapshotTaken = false; }, // @@ -152,23 +306,57 @@ Zotero.Sync.Storage.FileChangeWatcher = { // /** - * For live watcher backends (RDCW and inotify), check whether we should return null to - * trigger a full scan (first call or periodic refresh). - * - * @return {boolean} true if getChangedItemKeys should return null + * Remove scan records for libraries that no longer exist, both to keep the pref from + * accumulating cruft and because SQLite can reuse the libraryID of a deleted library for a + * new one, which shouldn't inherit the old library's scan record */ - _liveWatcherNeedsFullScan() { - if (this._liveWatcherFirstCall) { - this._liveWatcherFirstCall = false; - this._liveWatcherLastFullScanTime = Date.now(); - return true; + _pruneScannedLibraries() { + let libraryIDs = new Set(Zotero.Libraries.getAll().map(library => library.libraryID)); + let pruned = false; + for (let libraryID of this._scannedLibraries) { + if (!libraryIDs.has(libraryID)) { + this._scannedLibraries.delete(libraryID); + pruned = true; + } } - if (Date.now() - this._liveWatcherLastFullScanTime - > this._MAX_WATCHER_AGE) { - this._liveWatcherLastFullScanTime = Date.now(); - return true; + for (let libraryID in this._lastFullScan) { + if (!libraryIDs.has(parseInt(libraryID))) { + delete this._lastFullScan[libraryID]; + } } - return false; + if (pruned && this._backend == 'fsevents') { + this._saveScannedLibraries(); + } + }, + + _requireFullScans() { + this._scannedLibraries.clear(); + this._lastFullScan = {}; + if (this._backend == 'fsevents') { + this._saveScannedLibraries(); + } + }, + + _loadScannedLibraries() { + this._scannedLibraries = new Set(); + if (this._backend != 'fsevents') { + return; + } + try { + let scanned = Zotero.Prefs.get(this._SCANNED_LIBRARIES_PREF); + if (scanned) { + this._scannedLibraries = new Set(JSON.parse(scanned)); + } + } + catch (e) { + Zotero.logError(e); + } + }, + + _saveScannedLibraries() { + Zotero.Prefs.set( + this._SCANNED_LIBRARIES_PREF, JSON.stringify([...this._scannedLibraries]) + ); }, /** diff --git a/chrome/content/zotero/xpcom/storage/fileChangeWatcher_fsevents.js b/chrome/content/zotero/xpcom/storage/fileChangeWatcher_fsevents.js index a1052bfc84..1d08890f50 100644 --- a/chrome/content/zotero/xpcom/storage/fileChangeWatcher_fsevents.js +++ b/chrome/content/zotero/xpcom/storage/fileChangeWatcher_fsevents.js @@ -198,20 +198,43 @@ Object.assign(Zotero.Sync.Storage.FileChangeWatcher, { } let sinceEventId = ctypes.UInt64(savedIdStr); + // kFSEventStreamEventFlag flags indicating that events were dropped or coalesced, + // so changes may be missing from the replay + const MUST_SCAN_SUBDIRS = 0x01; + const USER_DROPPED = 0x02; + const KERNEL_DROPPED = 0x04; + const EVENT_IDS_WRAPPED = 0x08; + // Sentinel event marking the end of historical events -- not a file change + const HISTORY_DONE = 0x10; + let changedKeys = new Set(); + let droppedEvents = false; let storageRoot = this._storageRoot; let keyPattern = this._keyPattern; let callbackFn = this._FSEventStreamCallbackType.ptr(function ( _streamRef, _info, numEvents, eventPaths, - _eventFlags, _eventIds + eventFlags, _eventIds ) { let n = Number(numEvents); let StringArray = ctypes.ArrayType(ctypes.char.ptr, n); let paths = ctypes.cast( eventPaths, StringArray.ptr ).contents; + let FlagsArray = ctypes.ArrayType(ctypes.uint32_t, n); + let flags = ctypes.cast( + eventFlags, FlagsArray.ptr + ).contents; for (let i = 0; i < n; i++) { + let f = flags[i]; + if (f & (MUST_SCAN_SUBDIRS | USER_DROPPED + | KERNEL_DROPPED | EVENT_IDS_WRAPPED)) { + droppedEvents = true; + continue; + } + if (f & HISTORY_DONE) { + continue; + } let p = paths[i].readString(); if (!p.startsWith(storageRoot)) continue; let relative = p.substring(storageRoot.length); @@ -272,6 +295,14 @@ Object.assign(Zotero.Sync.Storage.FileChangeWatcher, { "sync.storage.watcher.fsEventsEventID", newEventId.toString() ); + if (droppedEvents) { + // The full scans triggered by the fallback cover everything up to now, so the + // advanced event ID baseline above remains valid + Zotero.debug("FileChangeWatcher: FSEvents reported dropped or coalesced events" + + " -- signaling full scan"); + return null; + } + return this._returnKeys(changedKeys); }, diff --git a/chrome/content/zotero/xpcom/storage/fileChangeWatcher_inotify.js b/chrome/content/zotero/xpcom/storage/fileChangeWatcher_inotify.js index 4f0e0ff076..f05d30806e 100644 --- a/chrome/content/zotero/xpcom/storage/fileChangeWatcher_inotify.js +++ b/chrome/content/zotero/xpcom/storage/fileChangeWatcher_inotify.js @@ -28,8 +28,9 @@ * inotify backend for FileChangeWatcher (Linux) * * Like the Windows RDCW backend, inotify is a live watcher -- it only captures events from the - * moment watches are set up. The first getChangedItemKeys() call returns null to trigger a full - * scan. Periodic full scans every 3 hours ensure no changes are missed permanently. + * moment watches are set up, accumulating changes between getChangedItemKeys() calls. The core + * watcher compensates by requiring a full scan of each library after watcher startup and every + * 3 hours. * * Note: errno values are read from ctypes.errno, which Mozilla ctypes captures immediately after * each default_abi call. Using an explicit __errno_location() call would be unreliable because @@ -124,7 +125,6 @@ Object.assign(Zotero.Sync.Storage.FileChangeWatcher, { this._inotifyWdToKey = new Map(); this._inotifyAccumulatedKeys = new Set(); this._inotifySubdirsSetUp = false; - this._liveWatcherFirstCall = true; }, /** @@ -150,9 +150,11 @@ Object.assign(Zotero.Sync.Storage.FileChangeWatcher, { ); if (wd < 0) { if (ctypes.errno === this._ENOSPC) { - Zotero.debug("FileChangeWatcher: inotify watch limit reached after " - + count + " directories -- some changes may be missed"); - break; + // Watch coverage would be silently incomplete, so give up on the watcher + // entirely -- the thrown error makes the core watcher disable itself and + // fall back to legacy scanning + throw new Error("inotify watch limit reached after " + count + + " directories"); } continue; } @@ -270,18 +272,9 @@ Object.assign(Zotero.Sync.Storage.FileChangeWatcher, { let ok = this._inotifyDrainEvents(); - // First call or periodic refresh -- signal full scan - if (this._liveWatcherNeedsFullScan()) { - Zotero.debug("FileChangeWatcher: Live watcher signaling full scan" - + " (first call or periodic refresh)"); - this._inotifyAccumulatedKeys.clear(); - return null; - } - // Overflow -- signal full scan if (!ok) { this._inotifyAccumulatedKeys.clear(); - this._liveWatcherLastFullScanTime = Date.now(); return null; } diff --git a/chrome/content/zotero/xpcom/storage/fileChangeWatcher_rdcw.js b/chrome/content/zotero/xpcom/storage/fileChangeWatcher_rdcw.js index 97e2a13fe9..9dd252c710 100644 --- a/chrome/content/zotero/xpcom/storage/fileChangeWatcher_rdcw.js +++ b/chrome/content/zotero/xpcom/storage/fileChangeWatcher_rdcw.js @@ -31,9 +31,9 @@ * Unlike the previous USN Change Journal backend, this works without admin privileges -- it only * needs read access to the storage directory itself. * - * This is a live watcher (like inotify on Linux): the first getChangedItemKeys() call returns null - * to trigger a full scan, then accumulates changes between calls. Periodic full scans every 3 hours - * via _liveWatcherNeedsFullScan() ensure no changes are missed permanently. + * This is a live watcher (like inotify on Linux): it only captures events from the moment the + * watch is set up, accumulating changes between getChangedItemKeys() calls. The core watcher + * compensates by requiring a full scan of each library after watcher startup and every 3 hours. * * Approach -- overlapped I/O polling: * 1. Init: Open storage directory with FILE_FLAG_OVERLAPPED, create an event handle, issue the @@ -202,7 +202,6 @@ Object.assign(Zotero.Sync.Storage.FileChangeWatcher, { this._overlapped.hEvent = this._eventHandle; this._rdcwAccumulatedKeys = new Set(); - this._liveWatcherFirstCall = true; // ---- Issue first ReadDirectoryChangesW call ---- @@ -324,7 +323,6 @@ Object.assign(Zotero.Sync.Storage.FileChangeWatcher, { Zotero.debug("FileChangeWatcher: RDCW buffer overflow" + " -- signaling full scan"); this._rdcwAccumulatedKeys.clear(); - this._liveWatcherLastFullScanTime = Date.now(); // Re-arm for future notifications try { this._rdcwArm(); @@ -349,7 +347,6 @@ Object.assign(Zotero.Sync.Storage.FileChangeWatcher, { Zotero.debug("FileChangeWatcher: RDCW returned 0 bytes" + " -- signaling full scan"); this._rdcwAccumulatedKeys.clear(); - this._liveWatcherLastFullScanTime = Date.now(); try { this._rdcwArm(); } @@ -374,14 +371,6 @@ Object.assign(Zotero.Sync.Storage.FileChangeWatcher, { } } - // First call or periodic refresh -- signal full scan - if (this._liveWatcherNeedsFullScan()) { - Zotero.debug("FileChangeWatcher: Live watcher signaling full scan" - + " (first call or periodic refresh)"); - this._rdcwAccumulatedKeys.clear(); - return null; - } - let keys = this._rdcwAccumulatedKeys; this._rdcwAccumulatedKeys = new Set(); return this._returnKeys(keys); diff --git a/chrome/content/zotero/xpcom/storage/storageEngine.js b/chrome/content/zotero/xpcom/storage/storageEngine.js index 76ec88502a..664711f5b4 100644 --- a/chrome/content/zotero/xpcom/storage/storageEngine.js +++ b/chrome/content/zotero/xpcom/storage/storageEngine.js @@ -146,38 +146,15 @@ Zotero.Sync.Storage.Engine.prototype.start = async function () { Zotero.debug("No file editing access -- skipping file modification check for " + this.library.name); } - // Use file change watcher to check only files that actually changed on disk - // - // Note: File-sync downloads also generate filesystem events, so recently downloaded files will - // appear here on the next sync cycle, but checkForUpdatedFiles() will see that the mtime/hash - // match the synced values and skip them. + // If the file change watcher is active, files that actually changed on disk were already + // checked and marked in the database when the sync runner took the watcher snapshot at the + // start of file syncing, so the scan can be skipped entirely unless this library needs a + // full scan (not yet scanned, watcher fallback, daily refresh, or manual sync) else if (Zotero.Sync.Storage.FileChangeWatcher.available) { - let changedKeys = Zotero.Sync.Storage.FileChangeWatcher.getChangedItemKeys(); - if (changedKeys) { - if (changedKeys.size > 0) { - let keysArray = Array.from(changedKeys); - let itemIDs = []; - // Batch to avoid hitting SQLite parameter limit - await Zotero.Utilities.Internal.forEachChunkAsync( - keysArray, 500, async function (chunk) { - let ids = await Zotero.DB.columnQueryAsync( - "SELECT itemID FROM items WHERE libraryID=? AND key IN (" - + chunk.map(() => '?').join(',') + ")", - [libraryID, ...chunk] - ); - itemIDs.push(...ids); - } - ); - if (itemIDs.length) { - await this.local.checkForUpdatedFiles(libraryID, itemIDs); - } - } - // else: no changes detected, skip scan entirely - } - else { - // Watcher returned null (first run or error) -- full scan + if (Zotero.Sync.Storage.FileChangeWatcher.needsFullScan(libraryID, this.background)) { this.local.lastFullFileCheck[libraryID] = new Date().getTime(); await this.local.checkForUpdatedFiles(libraryID); + Zotero.Sync.Storage.FileChangeWatcher.recordFullScan(libraryID); } } // If this is a background sync, it's not the first sync of the session, the library has had diff --git a/chrome/content/zotero/xpcom/sync/syncRunner.js b/chrome/content/zotero/xpcom/sync/syncRunner.js index 7ac415f658..2c4e5dbbdc 100644 --- a/chrome/content/zotero/xpcom/sync/syncRunner.js +++ b/chrome/content/zotero/xpcom/sync/syncRunner.js @@ -696,6 +696,9 @@ Zotero.Sync.Runner_Module = function (options = {}) { */ var _doFileSync = async function (libraries, options) { Zotero.debug("Starting file syncing"); + // Drain file change events and run the modification check on the changed files across + // all libraries + await Zotero.Sync.Storage.FileChangeWatcher.snapshot(); var resyncLibraries = [] for (let libraryID of libraries) { _stopCheck(); diff --git a/test/tests/fileChangeWatcherTest.js b/test/tests/fileChangeWatcherTest.js index 30383abb2a..764a31ae9e 100644 --- a/test/tests/fileChangeWatcherTest.js +++ b/test/tests/fileChangeWatcherTest.js @@ -400,6 +400,194 @@ describe("Zotero.Sync.Storage.FileChangeWatcher", function () { }); }); + function resetWatcherState() { + watcher._snapshotTaken = false; + watcher._scannedLibraries = new Set(); + watcher._lastFullScan = {}; + Zotero.Prefs.clear(watcher._SCANNED_LIBRARIES_PREF); + } + + function markLibrariesScanned(...libraryIDs) { + for (let libraryID of libraryIDs) { + watcher._scannedLibraries.add(libraryID); + watcher._lastFullScan[libraryID] = Date.now(); + } + } + + describe("snapshots", function () { + var savedAvailable, savedBackend; + var stub; + + beforeEach(function () { + savedAvailable = watcher.available; + savedBackend = watcher._backend; + watcher.available = true; + resetWatcherState(); + }); + + afterEach(function () { + if (stub) { + stub.restore(); + stub = null; + } + watcher.available = savedAvailable; + watcher._backend = savedBackend; + resetWatcherState(); + }); + + it("should require a full scan of a library that hasn't been scanned", async function () { + stub = sinon.stub(watcher, 'getChangedItemKeys').returns(new Set()); + markLibrariesScanned(1); + await watcher.snapshot(); + assert.isFalse(watcher.needsFullScan(1, true)); + assert.isTrue(watcher.needsFullScan(2, true)); + // Once a full scan has been recorded, watcher results can be relied on + watcher.recordFullScan(2); + assert.isFalse(watcher.needsFullScan(2, true)); + }); + + it("should require a full scan of every library after a backend fallback", async function () { + stub = sinon.stub(watcher, 'getChangedItemKeys').returns(null); + markLibrariesScanned(1, 2); + await watcher.snapshot(); + assert.isTrue(watcher.needsFullScan(1, true)); + assert.isTrue(watcher.needsFullScan(2, true)); + }); + + it("should prune scan records for deleted libraries", async function () { + stub = sinon.stub(watcher, 'getChangedItemKeys').returns(new Set()); + let userLibraryID = Zotero.Libraries.userLibraryID; + markLibrariesScanned(userLibraryID, 99999); + await watcher.snapshot(); + assert.isFalse(watcher.needsFullScan(userLibraryID, true)); + assert.isTrue(watcher.needsFullScan(99999, true)); + }); + + it("should require a full scan on a manual sync with a live watcher", async function () { + stub = sinon.stub(watcher, 'getChangedItemKeys').returns(new Set()); + markLibrariesScanned(1); + await watcher.snapshot(); + watcher._backend = 'rdcw'; + assert.isTrue(watcher.needsFullScan(1, false)); + assert.isFalse(watcher.needsFullScan(1, true)); + // The FSEvents journal is trusted on manual syncs + watcher._backend = 'fsevents'; + assert.isFalse(watcher.needsFullScan(1, false)); + }); + }); + + describe("multi-library file syncs", function () { + var apiKey = Zotero.Utilities.randomString(24); + var server, httpd, port, baseURL; + + beforeEach(async function () { + Zotero.HTTP.mock = sinon.FakeXMLHttpRequest; + server = sinon.fakeServer.create(); + server.autoRespond = true; + + ({ httpd, port } = await startHTTPServer()); + baseURL = `http://localhost:${port}/`; + + await Zotero.Users.setCurrentUserID(1); + await Zotero.Users.setCurrentUsername("testuser"); + }); + + afterEach(async function () { + Zotero.HTTP.mock = null; + await new Promise(resolve => httpd.stop(resolve)); + resetWatcherState(); + }); + + function makeEngine(libraryID) { + const { ConcurrentCaller } = ChromeUtils.importESModule( + "resource://zotero/concurrentCaller.mjs" + ); + var caller = new ConcurrentCaller(1); + caller.setLogger(msg => Zotero.debug(msg)); + + var client = new Zotero.Sync.APIClient({ + baseURL, + apiVersion: ZOTERO_CONFIG.API_VERSION, + apiKey, + caller, + background: true + }); + + return new Zotero.Sync.Storage.Engine({ + libraryID, + controller: new Zotero.Sync.Storage.Mode.ZFS({ + apiClient: client + }), + background: true, + stopOnError: false + }); + } + + it("should detect an externally modified group file when My Library syncs first", async function () { + var group = await createGroup(); + group.libraryVersion = 5; + await group.saveTx(); + group.storageVersion = 5; + await group.saveTx(); + + var item = await importFileAttachment('test.png', { libraryID: group.libraryID }); + + // Mark as in sync, with the file mtime in the past + var path = await item.getFilePathAsync(); + var mtime = (Math.floor(new Date().getTime() / 1000) * 1000) - 2000; + await OS.File.setDates(path, null, mtime); + item.attachmentSyncedModificationTime = mtime; + item.attachmentSyncedHash = await item.attachmentHash; + item.attachmentSyncState = "in_sync"; + await item.saveTx({ skipAll: true }); + + // Modify the file externally + await Zotero.File.putContentsAsync(path, Zotero.Utilities.randomString()); + + // Simulate a watcher backend that reports the group file change on the first + // drain and nothing after that + var savedAvailable = watcher.available; + var stub = sinon.stub(watcher, 'getChangedItemKeys'); + stub.returns(new Set()); + stub.onFirstCall().returns(new Set([item.key])); + watcher.available = true; + // Simulate the steady state, with both libraries previously scanned + resetWatcherState(); + markLibrariesScanned(Zotero.Libraries.userLibraryID, group.libraryID); + + var spy = sinon.spy(Zotero.Sync.Storage.Local, 'checkForUpdatedFiles'); + try { + // Drain events once per session and then file-sync My Library first and the + // group second, as the sync runner does + await watcher.snapshot(); + await makeEngine(Zotero.Libraries.userLibraryID).start(); + await makeEngine(group.libraryID).start(); + } + finally { + stub.restore(); + spy.restore(); + watcher.available = savedAvailable; + } + + // The group library should have been checked with the changed item rather than + // via a full scan -- checkForUpdatedFiles() drains the passed array, so just + // check that one was passed + var call = spy.getCalls().find(c => c.args[0] == group.libraryID); + assert.ok(call, "checkForUpdatedFiles() should be called for the group library"); + assert.isArray( + call.args[1], + "checkForUpdatedFiles() should be passed the changed group items" + ); + assert.equal( + item.attachmentSyncState, + Zotero.Sync.Storage.Local.SYNC_STATE_TO_UPLOAD, + "Group file modified on disk should be marked for upload" + ); + + await group.eraseTx(); + }); + }); + describe("ReadDirectoryChangesW backend (Windows)", function () { before(async function () { if (!Zotero.isWin) { @@ -423,7 +611,6 @@ describe("Zotero.Sync.Storage.FileChangeWatcher", function () { }); it("should initialize successfully"); - it("should return null on first call (no persistent journal)"); it("should return an empty set when no files have changed"); it("should detect a modified attachment file"); it("should detect changes across multiple items"); @@ -431,7 +618,6 @@ describe("Zotero.Sync.Storage.FileChangeWatcher", function () { it("should detect a new file added to a storage directory"); it("should re-arm overlapped I/O after draining notifications"); it("should return null on buffer overflow and re-arm"); - it("should return null for periodic full scan after max age"); it("should only report valid 8-char item keys"); }); @@ -460,14 +646,12 @@ describe("Zotero.Sync.Storage.FileChangeWatcher", function () { }); it("should initialize successfully"); - it("should return null on first call (no persistent journal)"); it("should return an empty set when no files have changed"); it("should detect a modified attachment file"); it("should detect changes across multiple items"); it("should not report the same changes twice"); it("should detect a new file added to a storage directory"); it("should auto-watch newly created storage subdirectories"); - it("should return null for periodic full scan after max age"); it("should only report valid 8-char item keys"); }); From 982c00aaf6817a23684a6929801ef1cc19623e11 Mon Sep 17 00:00:00 2001 From: Abe Jellinek <1770299+AbeJellinek@users.noreply.github.com> Date: Thu, 11 Jun 2026 11:54:42 -0400 Subject: [PATCH 009/465] Add by Identifier: Stay open with input, don't clear unless submitted (#5949) --- chrome/content/zotero/lookup.js | 67 ++++++++++++++++++++++++-- chrome/content/zotero/zoteroPane.xhtml | 1 + 2 files changed, 63 insertions(+), 5 deletions(-) diff --git a/chrome/content/zotero/lookup.js b/chrome/content/zotero/lookup.js index cd7cbbcb72..1a890b990a 100644 --- a/chrome/content/zotero/lookup.js +++ b/chrome/content/zotero/lookup.js @@ -28,6 +28,9 @@ * @namespace */ var Zotero_Lookup = new function () { + this._button = null; + this._accepted = false; + /** * Performs a lookup by DOI, PMID, or ISBN on the given textBox value * and adds any items it can. @@ -41,7 +44,6 @@ var Zotero_Lookup = new function () { * @param toggleProgress {function} - Callback to toggle progress on/off * @returns {Promise} */ - this._button = null; this.addItemsFromIdentifier = async function (textBox, childItem, toggleProgress) { var identifiers = Zotero.Utilities.extractIdentifiers(textBox.value); if (!identifiers.length) { @@ -137,6 +139,8 @@ var Zotero_Lookup = new function () { * Try a lookup and hide popup if successful */ this.accept = async function (textBox) { + this._accepted = true; + let newItems = await Zotero_Lookup.addItemsFromIdentifier( textBox, false, @@ -150,6 +154,10 @@ var Zotero_Lookup = new function () { // The item tree's DOM id has a view-specific suffix, so use the current view's id document.getElementById(ZoteroPane.itemsView.id).focus(); } + else { + // Hide on failure too + document.getElementById("zotero-lookup-panel").hidePopup(); + } return false; }; @@ -164,6 +172,11 @@ var Zotero_Lookup = new function () { }; this.onFocusOut = function (event) { + // Ignore focus loss caused by the window being deactivated + if (Services.focus.activeWindow !== window) { + return; + } + // If the lookup popup was triggered by the lookup button, // we want to return there on focus out. So we check // (1) that we came from a button and (2) that @@ -183,14 +196,53 @@ var Zotero_Lookup = new function () { }; - /** - * Focuses the field - */ this.onShown = function (event) { // Ignore context menu if (event.originalTarget.id != 'zotero-lookup-panel') return; + + this._accepted = false; + // Focus the field this.getActivePanel().querySelector('textarea').focus(); + + // Add handlers to dismiss the popup when contextually appropriate. + // We set noautohide="true" so the popup doesn't lose input when + // switching windows (e.g., so you can build a long list of identifiers + // copied from another app), but that means we need to manually handle + // closing on click outside (_onMouseDown) and closing on a window + // switch when there's no content (_onBlur) + window.addEventListener('mousedown', this._onMouseDown, { capture: true }); + document.getElementById('zotero-lookup-panel').addEventListener('blur', this._onBlur, { capture: true }); + }; + + + /** + * Hide on a click outside the panel + */ + this._onMouseDown = (event) => { + // Ignore clicks inside the panel or any popup (e.g. its context menu); + // only a click outside dismisses it + if (event.target.closest("panel, menupopup")) { + return; + } + document.getElementById("zotero-lookup-panel").hidePopup(); + // Prevent the toolbar button's own handlers from triggering, so the + // popup doesn't immediately reopen + if (document.getElementById("zotero-tb-lookup").contains(event.target)) { + event.preventDefault(); + event.stopPropagation(); + } + }; + + + this._onBlur = () => { + if (Services.focus.activeWindow === window) { + return; + } + let textBox = document.getElementById('zotero-lookup-textbox'); + if (textBox.value.trim() === '') { + document.getElementById('zotero-lookup-panel').hidePopup(); + } }; @@ -201,7 +253,12 @@ var Zotero_Lookup = new function () { // Ignore context menu if (event.originalTarget.id != 'zotero-lookup-panel') return; - document.getElementById("zotero-lookup-textbox").value = ""; + window.removeEventListener('mousedown', this._onMouseDown, { capture: true }); + document.getElementById('zotero-lookup-panel').removeEventListener('blur', this._onBlur, { capture: true }); + + if (this._accepted) { + document.getElementById("zotero-lookup-textbox").value = ""; + } Zotero_Lookup.setShowProgress(false); // Revert to single-line when closing diff --git a/chrome/content/zotero/zoteroPane.xhtml b/chrome/content/zotero/zoteroPane.xhtml index 62de1383fd..a15a042f6e 100644 --- a/chrome/content/zotero/zoteroPane.xhtml +++ b/chrome/content/zotero/zoteroPane.xhtml @@ -1278,6 +1278,7 @@ /> Date: Thu, 11 Jun 2026 15:05:31 +0300 Subject: [PATCH 010/465] Add SDT support --- chrome/content/zotero/xpcom/fulltext.js | 23 +- .../content/zotero/xpcom/pdfWorker/manager.js | 262 +++++------ chrome/content/zotero/xpcom/reader.js | 21 + chrome/content/zotero/xpcom/sdt.js | 426 ++++++++++++++++++ chrome/content/zotero/zotero.mjs | 1 + document-worker | 2 +- js-build/build.js | 2 +- js-build/document-worker.js | 41 +- test/tests/fulltextTest.js | 17 + test/tests/sdtTest.js | 344 ++++++++++++++ 10 files changed, 979 insertions(+), 160 deletions(-) create mode 100644 chrome/content/zotero/xpcom/sdt.js create mode 100644 test/tests/sdtTest.js diff --git a/chrome/content/zotero/xpcom/fulltext.js b/chrome/content/zotero/xpcom/fulltext.js index 4fcefe2b61..5cca7e7344 100644 --- a/chrome/content/zotero/xpcom/fulltext.js +++ b/chrome/content/zotero/xpcom/fulltext.js @@ -378,15 +378,22 @@ Zotero.Fulltext = Zotero.FullText = new function () { } var item = await Zotero.Items.getAsync(itemID); var linkMode = item.attachmentLinkMode; - // If file is stored outside of Zotero, create a directory for the item - // in the storage directory and save the cache file there - if (linkMode == Zotero.Attachments.LINK_MODE_LINKED_FILE) { - var parentDirPath = await Zotero.Attachments.createDirectoryForItem(item); - } - else { - var parentDirPath = PathUtils.parent(filePath); - } + // If the file is stored outside of Zotero, the cache file is saved in + // the item's storage directory + var parentDirPath = linkMode == Zotero.Attachments.LINK_MODE_LINKED_FILE + ? Zotero.Attachments.getStorageDirectory(item).path + : PathUtils.parent(filePath); var cacheFilePath = OS.Path.join(parentDirPath, this.fulltextCacheFile); + if (linkMode == Zotero.Attachments.LINK_MODE_LINKED_FILE) { + // Create only if missing -- don't use createDirectoryForItem(), + // which deletes and recreates the directory and would destroy + // other files stored there (e.g., the SDT cache) + await Zotero.File.createDirectoryIfMissingAsync(parentDirPath); + // Remove any previous cache file, which createDirectoryForItem() + // did implicitly, so that a failed re-extraction below can't + // leave a replaced file's old text in place + await IOUtils.remove(cacheFilePath, { ignoreAbsent: true }); + } try { var { text, diff --git a/chrome/content/zotero/xpcom/pdfWorker/manager.js b/chrome/content/zotero/xpcom/pdfWorker/manager.js index b95c5bb57f..3c903b960b 100644 --- a/chrome/content/zotero/xpcom/pdfWorker/manager.js +++ b/chrome/content/zotero/xpcom/pdfWorker/manager.js @@ -23,10 +23,16 @@ ***** END LICENSE BLOCK ***** */ -const WORKER_URL = 'chrome://zotero/content/xpcom/pdfWorker/worker.js'; -const CMAPS_URL = 'resource://zotero/reader/pdf/web/cmaps/'; -const STANDARD_FONTS_URL = 'resource://zotero/reader/pdf/web/standard_fonts/'; -const WASM_URL = 'resource://zotero/reader/pdf/web/wasm/'; +const WORKER_URL = 'resource://zotero/document-worker/worker.js'; +const ASSETS_URL = 'resource://zotero/document-worker/'; +const READER_PDF_ASSETS_URL = 'resource://zotero/reader/pdf/web/'; + +function getAssetURL(path) { + if (path.startsWith('cmaps/') || path.startsWith('standard_fonts/')) { + return READER_PDF_ASSETS_URL + path; + } + return ASSETS_URL + path; +} class PDFWorker { constructor() { @@ -80,67 +86,58 @@ class PDFWorker { }); } + /** + * Log and rethrow a worker error wrapped with the action name and + * optional context, restoring the error name (e.g., 'PasswordException') + * from the worker error JSON + */ + _throwWorkerError(action, e, details = {}) { + let error = new Error(`Worker action '${action}' failed: ${JSON.stringify({ ...details, error: e.message })}`); + try { + error.name = JSON.parse(e.message).name; + } + catch (e) { + Zotero.logError(e); + } + Zotero.logError(error); + throw error; + } + _init() { if (this._worker) return; this._worker = new Worker(WORKER_URL); this._worker.addEventListener('message', async (event) => { let message = event.data; - if (message.responseID) { - let { resolve, reject } = this._waitingPromises[message.responseID]; + if ('responseID' in message) { + let promise = this._waitingPromises[message.responseID]; + if (!promise) { + Zotero.debug(`Received response from PDF worker for unknown request ${message.responseID}`); + return; + } delete this._waitingPromises[message.responseID]; - if (message.data) { - resolve(message.data); + let { resolve, reject } = promise; + if ('error' in message) { + reject(new Error(JSON.stringify(message.error))); } else { - reject(new Error(JSON.stringify(message.error))); + resolve(message.data); } return; } - if (message.id) { + if ('id' in message) { let respData = null; try { - if (message.action === 'FetchBuiltInCMap') { + if (message.action === 'FetchData') { let response = await Zotero.HTTP.request( 'GET', - CMAPS_URL + message.data + '.bcmap', - { responseType: 'arraybuffer' } - ); - respData = { - isCompressed: true, - cMapData: new Uint8Array(response.response) - }; - } - } - catch (e) { - Zotero.debug('Failed to fetch CMap data:'); - Zotero.debug(e); - } - try { - if (message.action === 'FetchStandardFontData') { - let response = await Zotero.HTTP.request( - 'GET', - STANDARD_FONTS_URL + message.data, + getAssetURL(message.data), { responseType: 'arraybuffer' } ); respData = new Uint8Array(response.response); } } catch (e) { - Zotero.debug('Failed to fetch standard font data:'); - Zotero.debug(e); - } - try { - if (message.action === 'FetchWasm') { - let response = await Zotero.HTTP.request( - 'GET', - WASM_URL + message.data, - { responseType: 'arraybuffer' } - ); - respData = new Uint8Array(response.response); - } - } - catch (e) { - Zotero.debug('Failed to fetch wasm data:'); + Zotero.debug(`Failed to fetch data (${message.data}):`); Zotero.debug(e); } try { @@ -158,11 +155,14 @@ class PDFWorker { Zotero.debug('Failed to save rendered annotation:'); Zotero.logError(e); } - this._worker.postMessage({ responseID: event.data.id, data: respData }); + this._worker.postMessage( + { responseID: event.data.id, data: respData }, + respData instanceof Uint8Array ? [respData.buffer] : [] + ); } }); this._worker.addEventListener('error', (event) => { - Zotero.logError(`PDF Web Worker error (${event.filename}:${event.lineno}): ${event.message}`); + Zotero.logError(`Document worker error (${event.filename}:${event.lineno}): ${event.message}`); }); } @@ -229,23 +229,12 @@ class PDFWorker { buf = new Uint8Array(buf).buffer; try { - var res = await this._query('export', { + var res = await this._query('pdf.writeAnnotations', { buf, annotations, password }, [buf]); } catch (e) { - let error = new Error(`Worker 'export' failed: ${JSON.stringify({ - annotations, - error: e.message - })}`); - try { - error.name = JSON.parse(e.message).name; - } - catch (e) { - Zotero.logError(e); - } - Zotero.logError(error); - throw error; + this._throwWorkerError('pdf.writeAnnotations', e, { annotations }); } await IOUtils.write(path, new Uint8Array(res.buf)); @@ -331,23 +320,12 @@ class PDFWorker { buf = new Uint8Array(buf).buffer; try { - var { imported, deleted, buf: modifiedBuf } = await this._query('import', { + var { imported, deleted, buf: modifiedBuf } = await this._query('pdf.importAnnotations', { buf, existingAnnotations, password, transfer }, [buf]); } catch (e) { - let error = new Error(`Worker 'import' failed: ${JSON.stringify({ - existingAnnotations, - error: e.message - })}`); - try { - error.name = JSON.parse(e.message).name; - } - catch (e) { - Zotero.logError(e); - } - Zotero.logError(error); - throw error; + this._throwWorkerError('pdf.importAnnotations', e, { existingAnnotations }); } let ids = []; @@ -404,17 +382,12 @@ class PDFWorker { let buf = await IOUtils.read(pdfPath); buf = new Uint8Array(buf).buffer; try { - var annotations = await this._query('importCitavi', { + var annotations = await this._query('pdf.importCitaviAnnotations', { buf, citaviAnnotations, password }, [buf]); } catch (e) { - let error = new Error(`Worker 'importCitavi' failed: ${JSON.stringify({ - citaviAnnotations, - error: e.message - })}`); - Zotero.logError(error); - throw error; + this._throwWorkerError('pdf.importCitaviAnnotations', e, { citaviAnnotations }); } return annotations; }, isPriority); @@ -438,17 +411,12 @@ class PDFWorker { let buf = await IOUtils.read(pdfPath); buf = new Uint8Array(buf).buffer; try { - var annotations = await this._query('importMendeley', { + var annotations = await this._query('pdf.importMendeleyAnnotations', { buf, mendeleyAnnotations, password }, [buf]); } catch (e) { - let error = new Error(`Worker 'importMendeley' failed: ${JSON.stringify({ - mendeleyAnnotations, - error: e.message - })}`); - Zotero.logError(error); - throw error; + this._throwWorkerError('pdf.importMendeleyAnnotations', e, { mendeleyAnnotations }); } return annotations; }, isPriority); @@ -507,20 +475,12 @@ class PDFWorker { buf = new Uint8Array(buf).buffer; try { - var { buf: modifiedBuf } = await this._query('deletePages', { + var { buf: modifiedBuf } = await this._query('pdf.deletePages', { buf, pageIndexes, password }, [buf]); } catch (e) { - let error = new Error(`Worker 'deletePages' failed: ${JSON.stringify({ error: e.message })}`); - try { - error.name = JSON.parse(e.message).name; - } - catch (e) { - Zotero.logError(e); - } - Zotero.logError(error); - throw error; + this._throwWorkerError('pdf.deletePages', e); } // Delete annotations from deleted pages @@ -605,20 +565,12 @@ class PDFWorker { buf = new Uint8Array(buf).buffer; try { - var { buf: modifiedBuf } = await this._query('rotatePages', { + var { buf: modifiedBuf } = await this._query('pdf.rotatePages', { buf, pageIndexes, degrees, password }, [buf]); } catch (e) { - let error = new Error(`Worker 'rotatePages' failed: ${JSON.stringify({ error: e.message })}`); - try { - error.name = JSON.parse(e.message).name; - } - catch (e) { - Zotero.logError(e); - } - Zotero.logError(error); - throw error; + this._throwWorkerError('pdf.rotatePages', e); } await IOUtils.write(path, new Uint8Array(modifiedBuf)); @@ -657,20 +609,12 @@ class PDFWorker { buf = new Uint8Array(buf).buffer; try { - var result = await this._query('getFulltext', { + var result = await this._query('pdf.getFulltext', { buf, maxPages, password }, [buf]); } catch (e) { - let error = new Error(`Worker 'getFullText' failed: ${JSON.stringify({ error: e.message })}`); - try { - error.name = JSON.parse(e.message).name; - } - catch (e) { - Zotero.logError(e); - } - Zotero.logError(error); - throw error; + this._throwWorkerError('pdf.getFulltext', e); } Zotero.debug(`Extracted full text for item ${attachment.libraryKey} in ${new Date() - t} ms`); @@ -679,6 +623,56 @@ class PDFWorker { }, isPriority); } + /** + * Get structured document text for a PDF, EPUB, or snapshot attachment + * + * @param {Integer} itemID Attachment item id + * @param {Boolean} [isPriority] + * @param {String} [password] + * @returns {Promise} + */ + async getStructuredDocumentText(itemID, isPriority, password) { + return this._enqueue(async () => { + let attachment = await Zotero.Items.getAsync(itemID); + if (!(attachment.isPDFAttachment() + || attachment.isEPUBAttachment() + || attachment.isSnapshotAttachment())) { + throw new Error('Item must be a PDF, EPUB, or snapshot attachment'); + } + + let path = await attachment.getFilePathAsync(); + if (!path) { + return null; + } + + let sourceHash = await attachment.attachmentHash; + if (!sourceHash) { + throw new Error('Attachment is missing an MD5 hash'); + } + + Zotero.debug(`Getting structured document text from item ${attachment.libraryKey}`); + let t = new Date(); + let buf = await IOUtils.read(path); + buf = new Uint8Array(buf).buffer; + + try { + var result = await this._query('getStructuredDocumentText', { + buf, + contentType: attachment.attachmentContentType, + password, + sourceHash, + }, [buf]); + } + catch (e) { + this._throwWorkerError('getStructuredDocumentText', e); + } + + Zotero.debug(`Extracted structured document text for item ${attachment.libraryKey} in ${new Date() - t} ms`); + + return result; + }, isPriority); + } + /** * Get data for recognizer-server * @@ -703,18 +697,10 @@ class PDFWorker { buf = new Uint8Array(buf).buffer; try { - var result = await this._query('getRecognizerData', { buf, password }, [buf]); + var result = await this._query('pdf.getRecognizerData', { buf, password }, [buf]); } catch (e) { - let error = new Error(`Worker 'getRecognizerData' failed: ${JSON.stringify({ error: e.message })}`); - try { - error.name = JSON.parse(e.message).name; - } - catch (e) { - Zotero.logError(e); - } - Zotero.logError(error); - throw error; + this._throwWorkerError('pdf.getRecognizerData', e); } Zotero.debug(`Extracted PDF recognizer data for item ${attachment.libraryKey} in ${new Date() - t} ms`); @@ -759,18 +745,10 @@ class PDFWorker { let { libraryID } = attachment; try { - var result = await this._query('renderAnnotations', { libraryID, buf, annotations, password }, [buf]); + var result = await this._query('pdf.renderAnnotations', { libraryID, buf, annotations, password }, [buf]); } catch (e) { - let error = new Error(`Worker 'renderAnnotations' failed: ${JSON.stringify({ error: e.message })}`); - try { - error.name = JSON.parse(e.message).name; - } - catch (e) { - Zotero.logError(e); - } - Zotero.logError(error); - throw error; + this._throwWorkerError('pdf.renderAnnotations', e); } Zotero.debug(`Rendered ${annotations.length} PDF annotation(s) ${attachment.libraryKey} in ${new Date() - t} ms`); @@ -802,18 +780,10 @@ class PDFWorker { buf = new Uint8Array(buf).buffer; try { - var result = await this._query('hasAnnotations', { buf, password }, [buf]); + var result = await this._query('pdf.hasAnnotations', { buf, password }, [buf]); } catch (e) { - let error = new Error(`Worker 'hasAnnotations' failed: ${JSON.stringify({ error: e.message })}`); - try { - error.name = JSON.parse(e.message).name; - } - catch (e) { - Zotero.logError(e); - } - Zotero.logError(error); - throw error; + this._throwWorkerError('pdf.hasAnnotations', e); } return result.hasAnnotations; diff --git a/chrome/content/zotero/xpcom/reader.js b/chrome/content/zotero/xpcom/reader.js index b4cecf3d70..0473486643 100644 --- a/chrome/content/zotero/xpcom/reader.js +++ b/chrome/content/zotero/xpcom/reader.js @@ -256,6 +256,7 @@ class ReaderInstance { readAloudVoices: this._getReadAloudVoices(), readAloudEnabledVoices: await this._getReadAloudEnabledVoices(), readAloudRemoteInterface: this._getReadAloudRemoteInterface(this._iframeWindow), + getSDTPack: this._createGetSDTPack(this._iframeWindow), loggedIn: Zotero.Sync.Runner.enabled, onOpenContextMenu: () => { // Functions can only be passed over wrappedJSObject (we call back onClick for context menu items) @@ -1133,6 +1134,26 @@ class ReaderInstance { return state; } + // Returns the function passed to the reader as options.getSDTPack, which + // resolves with the SDT pack for the displayed attachment, generating it + // if necessary. The reader decides when to pull (currently at init, so + // the pack is ready when a feature needs it), and a reader build without + // SDT support never triggers extraction. The pack bytes are passed by + // value, so a held pack can't be affected by a later regeneration and + // always matches the document the reader is displaying. + _createGetSDTPack(targetWindow) { + if (!Zotero.SDT || !this.itemID || this._isTransient()) { + return null; + } + // Wrap the return value in a child window Promise to avoid + // permissions errors (as in _getReadAloudRemoteInterface()). + // getPack() never rejects + return () => new targetWindow.Promise(async (resolve) => { + let result = await Zotero.SDT.getPack(this.itemID, { isPriority: true }); + resolve(Cu.cloneInto(result, targetWindow)); + }); + } + _isTransient() { return false; } diff --git a/chrome/content/zotero/xpcom/sdt.js b/chrome/content/zotero/xpcom/sdt.js new file mode 100644 index 0000000000..93e0225e4c --- /dev/null +++ b/chrome/content/zotero/xpcom/sdt.js @@ -0,0 +1,426 @@ +/* + ***** BEGIN LICENSE BLOCK ***** + + Copyright © 2026 Corporation for Digital Scholarship + Vienna, Virginia, USA + http://digitalscholar.org/ + + This file is part of Zotero. + + Zotero is free software: you can redistribute it and/or modify + it under the terms of the GNU Affero General Public License as published by + the Free Software Foundation, either version 3 of the License, or + (at your option) any later version. + + Zotero is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU Affero General Public License for more details. + + You should have received a copy of the GNU Affero General Public License + along with Zotero. If not, see . + + ***** END LICENSE BLOCK ***** +*/ + +const SDT_CACHE_FILE_NAME = '.zotero-sdt-cache'; +const DOCUMENT_WORKER_METADATA_URL = 'resource://zotero/document-worker/metadata.json'; + +Zotero.SDT = new function () { + // Per-item in-flight generation, so that concurrent getPack() calls share + // one extraction and two generations can never race on the same cache file + let _generating = new Map(); + let _sourceHashCache = new Map(); + // Source hash of the last password-required failure per item, so that a + // password-protected file isn't re-extracted until the file changes + let _passwordFailures = new Map(); + let _module = null; + let _moduleErrorLogged = false; + let _documentWorkerMetadata = null; + let _documentWorkerMetadataErrorLogged = false; + + // Load the bundled SDT module lazily, so that a missing or broken + // resource degrades to an 'unavailable' result instead of breaking + // Zotero startup. Load failures aren't cached, so a load is retried + // on the next call (require() itself caches successful loads) + function _getModule() { + if (!_module) { + try { + _module = require('resource://zotero/document-worker/structured-document-text.js'); + } + catch (e) { + if (!_moduleErrorLogged) { + _moduleErrorLogged = true; + Zotero.logError(e); + } + return null; + } + } + return _module; + } + + /** + * Get the structured document text pack for a PDF, EPUB, or snapshot + * attachment, generating and caching it if necessary. The returned bytes + * are owned by the caller -- a later regeneration can't affect them. + * + * If the cached pack was produced by an older (but still readable) + * processor version, it's returned as is and a regeneration is started in + * the background, so that processor bumps don't block consumers. + * + * @param {Integer} itemID + * @param {Object} [options] + * @param {Boolean} [options.isPriority] - Put a needed extraction at the + * front of the worker queue (for user-initiated requests) + * @returns {Promise} { ok: true, bytes: ArrayBuffer, packVersion, + * schemaMajorVersion }, or { ok: false, reason: 'unavailable' | + * 'password-required' | 'failed' } + */ + this.getPack = async function (itemID, options = {}) { + try { + if (!_getModule() || !(await _getDocumentWorkerMetadata())) { + return { ok: false, reason: 'unavailable' }; + } + let context = await _getAttachmentContext(itemID); + if (!context.ok) { + return { ok: false, reason: context.reason }; + } + let cache = await _readValidCache(context, { allowStaleProcessorVersion: true }); + if (cache.ok) { + if (cache.staleProcessorVersion) { + _generate(context, {}).catch(e => Zotero.logError(e)); + } + return _makeResult(cache); + } + return await _generate(context, options); + } + catch (e) { + Zotero.logError(e); + return { ok: false, reason: 'failed' }; + } + }; + + /** + * Ensure that a current pack is cached for an attachment, generating or + * regenerating it if necessary, without returning it. For warming up the + * cache (e.g., at import time), so that later getPack() calls are hits. + * + * Unlike getPack(), which returns a stale-processor pack immediately and + * regenerates in the background, this resolves only once the cache is + * fully current. + * + * @param {Integer} itemID + * @param {Object} [options] - See getPack() + * @returns {Promise} - Whether a current pack is cached + */ + this.ensure = async function (itemID, options = {}) { + try { + if (!_getModule() || !(await _getDocumentWorkerMetadata())) { + return false; + } + let context = await _getAttachmentContext(itemID); + if (!context.ok) { + return false; + } + let cache = await _readValidCache(context, {}); + if (cache.ok) { + return true; + } + let result = await _generate(context, options); + return result.ok; + } + catch (e) { + Zotero.logError(e); + return false; + } + }; + + /** + * Get a parsed pack reader for in-process consumers + * + * @param {Integer} itemID + * @param {Object} [options] - See getPack() + * @returns {Promise} + */ + this.getReader = async function (itemID, options = {}) { + let result = await this.getPack(itemID, options); + if (!result.ok) { + return null; + } + return _openPack(new Uint8Array(result.bytes)); + }; + + async function _readValidCache({ sourceHash, cachePath, processorType }, options) { + let bytes; + try { + bytes = await IOUtils.read(cachePath); + } + catch (e) { + if (e.name === 'NotFoundError') { + return { ok: false, reason: 'missing' }; + } + throw e; + } + return _validateBytes(bytes, sourceHash, processorType, options); + } + + // Validate a pack held in memory, so that the validated bytes can't be + // affected by concurrent file changes + async function _validateBytes(bytes, sourceHash, processorType, options = {}) { + let documentWorkerMetadata = await _getDocumentWorkerMetadata(); + let expectedProcessorVersion = documentWorkerMetadata + && documentWorkerMetadata.SDT_PROCESSOR_VERSIONS[processorType]; + if (!expectedProcessorVersion) { + return { ok: false, reason: 'unavailable' }; + } + try { + // openStructuredDocumentTextPack() validates the pack magic and + // version itself, so a corrupt or unsupported-pack-version cache + // throws here and is reported as 'invalid-cache'. The schema major + // version is content semantics that the module deliberately + // doesn't validate, so check it here + let reader = await _openPack(bytes); + let { header } = reader; + if (header.packVersion !== documentWorkerMetadata.SDT_PACK_VERSION + || _getSchemaMajorVersion(header.schemaVersion) + !== _getSchemaMajorVersion(documentWorkerMetadata.SDT_SCHEMA_VERSION)) { + return { ok: false, reason: 'unsupported-version' }; + } + + let metadata = await reader.getMetadata(); + if (metadata.source?.hash !== sourceHash) { + return { ok: false, reason: 'stale-source' }; + } + if (metadata.processor?.type !== processorType) { + return { ok: false, reason: 'stale-processor' }; + } + let staleProcessorVersion = metadata.processor?.version !== expectedProcessorVersion; + if (staleProcessorVersion + && !(options.allowStaleProcessorVersion + && _isPositiveInteger(metadata.processor?.version))) { + return { ok: false, reason: 'stale-processor' }; + } + return { ok: true, bytes, header, staleProcessorVersion }; + } + catch (e) { + return { ok: false, reason: 'invalid-cache', error: e }; + } + } + + function _makeResult({ bytes, header }) { + return { + ok: true, + // IOUtils.read() and the worker both produce exactly-sized buffers + bytes: bytes.buffer, + packVersion: header.packVersion, + schemaMajorVersion: _getSchemaMajorVersion(header.schemaVersion), + }; + } + + function _generate(context, options) { + let key = _getItemKey(context.item); + if (_generating.has(key)) { + return _generating.get(key); + } + let promise = _generateUnqueued(context, options) + .finally(() => _generating.delete(key)); + _generating.set(key, promise); + return promise; + } + + async function _generateUnqueued({ item, sourceHash, cachePath, processorType }, options) { + try { + if (_passwordFailures.get(_getItemKey(item)) === sourceHash) { + return { ok: false, reason: 'password-required' }; + } + let t = new Date(); + let result = await Zotero.PDFWorker.getStructuredDocumentText( + item.id, + !!options.isPriority + ); + if (!result?.buf) { + return { ok: false, reason: 'failed' }; + } + let bytes = new Uint8Array(result.buf); + // Validate against the file's current hash rather than the one + // captured above, since the file can change while the extraction + // job waits in the worker queue and the worker stamps the hash it + // computes at processing time + let currentHash = await _getSourceHash(item) || sourceHash; + let cache = await _validateBytes(bytes, currentHash, processorType); + if (!cache.ok) { + Zotero.debug(`Generated SDT pack for item ${item.libraryKey} is unusable: ${cache.reason}`); + return { ok: false, reason: 'failed' }; + } + // Don't use Zotero.Attachments.createDirectoryForItem() here -- it + // deletes and recreates the directory, which would destroy other + // files stored there (e.g., the full-text cache of a linked file) + await Zotero.File.createDirectoryIfMissingAsync(PathUtils.parent(cachePath)); + await IOUtils.write(cachePath, bytes, { tmpPath: `${cachePath}.tmp` }); + Zotero.debug( + `Generated SDT pack for item ${item.libraryKey} in ${new Date() - t} ms ` + + `(${bytes.byteLength} bytes)` + ); + return _makeResult(cache); + } + catch (e) { + if (_isPasswordError(e)) { + _passwordFailures.set(_getItemKey(item), sourceHash); + return { ok: false, reason: 'password-required' }; + } + Zotero.logError(e); + return { ok: false, reason: 'failed' }; + } + } + + async function _openPack(bytes) { + let SDT = _getModule(); + let pako = require('pako'); + let source = { + byteLength: bytes.byteLength, + read: async (offset, length) => bytes.buffer.slice( + bytes.byteOffset + offset, + bytes.byteOffset + offset + length + ), + }; + return SDT.openStructuredDocumentTextPack(source, { + inflate: b => pako.inflateRaw(b), + }); + } + + async function _getDocumentWorkerMetadata() { + if (_documentWorkerMetadata) { + return _documentWorkerMetadata; + } + try { + let metadata = JSON.parse( + await Zotero.File.getContentsFromURLAsync(DOCUMENT_WORKER_METADATA_URL) + ); + _validateDocumentWorkerMetadata(metadata); + _documentWorkerMetadata = metadata; + return metadata; + } + catch (e) { + if (!_documentWorkerMetadataErrorLogged) { + _documentWorkerMetadataErrorLogged = true; + Zotero.logError(e); + } + return null; + } + } + + function _validateDocumentWorkerMetadata(metadata) { + let SDT = _getModule(); + if (!metadata || typeof metadata !== 'object') { + throw new Error('Invalid document-worker metadata'); + } + if (typeof metadata.SDT_SCHEMA_VERSION !== 'string' + || !_isPositiveInteger(metadata.SDT_PACK_VERSION) + || !metadata.SDT_PROCESSOR_VERSIONS + || typeof metadata.SDT_PROCESSOR_VERSIONS !== 'object') { + throw new Error('Invalid document-worker metadata'); + } + for (let processorType of ['pdf', 'epub', 'snapshot']) { + if (!_isPositiveInteger(metadata.SDT_PROCESSOR_VERSIONS[processorType])) { + throw new Error('Invalid document-worker processor metadata'); + } + } + if (!SDT + || metadata.SDT_SCHEMA_VERSION !== SDT.SDT_SCHEMA_VERSION + || metadata.SDT_PACK_VERSION !== SDT.SDT_PACK_VERSION) { + throw new Error('Document-worker metadata does not match the bundled SDT reader'); + } + } + + function _getCachePath(item) { + return PathUtils.join(Zotero.Attachments.getStorageDirectory(item).path, SDT_CACHE_FILE_NAME); + } + + async function _getAttachmentContext(itemID) { + // getAsync() returns false, not null, for a nonexistent item + let item = await Zotero.Items.getAsync(itemID); + if (!item || !item.isAttachment()) { + return { ok: false, reason: 'unavailable' }; + } + // The type checks exclude linked-URL attachments, and _getSourceHash() + // returns null when the attachment has no readable file + let processorType = _getProcessorType(item); + if (!processorType) { + return { ok: false, reason: 'unavailable' }; + } + let sourceHash = await _getSourceHash(item); + if (!sourceHash) { + return { ok: false, reason: 'unavailable' }; + } + return { + ok: true, + item, + sourceHash, + processorType, + cachePath: _getCachePath(item), + }; + } + + function _getProcessorType(item) { + if (item.isPDFAttachment()) { + return 'pdf'; + } + if (item.isEPUBAttachment()) { + return 'epub'; + } + if (item.isSnapshotAttachment()) { + return 'snapshot'; + } + return null; + } + + function _getSchemaMajorVersion(schemaVersion) { + return Number(String(schemaVersion).split('.')[0]); + } + + function _getItemKey(item) { + return `${item.libraryID}/${item.key}`; + } + + function _isPositiveInteger(value) { + return Number.isInteger(value) && value > 0; + } + + async function _getSourceHash(item) { + try { + // attachmentHash reads and hashes the whole file, so reuse the + // result as long as the file's path, size, and mtime are unchanged + let path = await item.getFilePathAsync(); + if (!path) { + return null; + } + let { size, lastModified } = await IOUtils.stat(path); + let cached = _sourceHashCache.get(item.id); + if (cached + && cached.path === path + && cached.size === size + && cached.mtime === lastModified) { + return cached.hash; + } + let hash = await item.attachmentHash; + if (hash) { + _sourceHashCache.set(item.id, { path, size, mtime: lastModified, hash }); + } + return hash; + } + catch (e) { + if (e.name !== 'NotFoundError') { + Zotero.logError(e); + } + return null; + } + } + + function _isPasswordError(e) { + // Password-protected documents aren't supported, but the failure is + // classified as 'password-required' so that the reason can be shown + // to the user. (PDFWorker restores the error name from the worker + // error JSON.) + return e?.name === 'PasswordException'; + } +}; diff --git a/chrome/content/zotero/zotero.mjs b/chrome/content/zotero/zotero.mjs index c8dc7cefdc..6dc8648269 100644 --- a/chrome/content/zotero/zotero.mjs +++ b/chrome/content/zotero/zotero.mjs @@ -117,6 +117,7 @@ const xpcomFilesLocal = [ 'pluginAPI/menuManager', 'pluginAPI/itemPaneManager', 'pluginAPI/itemTreeManager', + 'sdt', 'reader', 'progressQueue', 'progressQueueDialog', diff --git a/document-worker b/document-worker index fd642b3828..05287e4b8d 160000 --- a/document-worker +++ b/document-worker @@ -1 +1 @@ -Subproject commit fd642b38287f1e59aaf8e02c3132da6d3daa39c1 +Subproject commit 05287e4b8d9f2e1fd3fc81b02fbd864b9cc010e1 diff --git a/js-build/build.js b/js-build/build.js index a2e074d371..580b97d2b4 100644 --- a/js-build/build.js +++ b/js-build/build.js @@ -53,4 +53,4 @@ if (require.main === module) { onError(err); } })(); -} \ No newline at end of file +} diff --git a/js-build/document-worker.js b/js-build/document-worker.js index 3bdc7c3e50..52f81d2d83 100644 --- a/js-build/document-worker.js +++ b/js-build/document-worker.js @@ -7,16 +7,21 @@ const exec = util.promisify(require('child_process').exec); const { getSignatures, writeSignatures, onSuccess, onError } = require('./utils'); const { buildsURL } = require('./config'); +const sharedAssetDirs = ['cmaps', 'standard_fonts']; +const requiredFiles = ['worker.js', 'metadata.json', 'structured-document-text.js']; + async function getDocumentWorker(signatures) { const t1 = Date.now(); const modulePath = path.join(__dirname, '..', 'document-worker'); + const targetDir = path.join(__dirname, '..', 'build', 'resource', 'document-worker'); const { stdout } = await exec('git rev-parse HEAD', { cwd: modulePath }); const hash = stdout.trim(); - if (!('document-worker' in signatures) || signatures['document-worker'].hash !== hash) { - const targetDir = path.join(__dirname, '..', 'build', 'chrome', 'content', 'zotero', 'xpcom', 'pdfWorker'); + if (!('document-worker' in signatures) + || signatures['document-worker'].hash !== hash + || !(await isBuildReady(targetDir))) { try { const filename = hash + '.zip'; const tmpDir = path.join(__dirname, '..', 'tmp', 'builds', 'document-worker'); @@ -26,21 +31,35 @@ async function getDocumentWorker(signatures) { await fs.ensureDir(targetDir); await fs.ensureDir(tmpDir); + // Skip the shared asset directories, which are served from the + // reader build instead (see the cleanup loop below) await exec( `cd ${tmpDir}` + ` && (test -f ${filename} || curl -f ${url} -o ${filename})` - + ` && unzip -o ${filename} worker.js -d ${targetDir}` + + ` && unzip -o ${filename} -d ${targetDir} -x ${sharedAssetDirs.map(dir => `'${dir}/*'`).join(' ')}` ); + let missingFiles = await getMissingFiles(targetDir); + if (missingFiles.length) { + throw new Error(`Downloaded document-worker build is missing ${missingFiles.join(', ')}`); + } } catch (e) { console.error(e); await exec('npm ci', { cwd: modulePath }); await exec('npm run build', { cwd: modulePath }); - await fs.copy(path.join(modulePath, 'build', 'worker.js'), path.join(targetDir, 'worker.js')); + await fs.copy(path.join(modulePath, 'build'), targetDir); + let missingFiles = await getMissingFiles(targetDir); + if (missingFiles.length) { + throw new Error(`Local document-worker build is missing ${missingFiles.join(', ')}`); + } } signatures['document-worker'] = { hash }; } + for (let dir of sharedAssetDirs) { + await fs.remove(path.join(targetDir, dir)); + } + const t2 = Date.now(); return { @@ -51,6 +70,20 @@ async function getDocumentWorker(signatures) { }; } +async function isBuildReady(targetDir) { + return !(await getMissingFiles(targetDir)).length; +} + +async function getMissingFiles(targetDir) { + let missingFiles = []; + for (let file of requiredFiles) { + if (!(await fs.pathExists(path.join(targetDir, file)))) { + missingFiles.push(file); + } + } + return missingFiles; +} + module.exports = getDocumentWorker; if (require.main === module) { diff --git a/test/tests/fulltextTest.js b/test/tests/fulltextTest.js index 29e2b9b435..d346a1c63b 100644 --- a/test/tests/fulltextTest.js +++ b/test/tests/fulltextTest.js @@ -110,6 +110,23 @@ describe("Zotero.FullText", function () { assert.isTrue(await OS.File.exists(OS.Path.join(storageDir, '.zotero-ft-cache'))); assert.isFalse(await OS.File.exists(OS.Path.join(storageDir, filename))); }); + + it("should preserve the SDT cache when reindexing a linked attachment", async function () { + var file = OS.Path.join(getTestDataDirectory().path, 'test.pdf'); + var linkedFile = OS.Path.join(await getTempDirectory(), 'test.pdf'); + await OS.File.copy(file, linkedFile); + var item = await Zotero.Attachments.linkFromFile({ file: linkedFile }); + + // The full-text cache of a linked file shares the item's + // storage directory with the SDT cache, so reindexing must + // not recreate the directory and destroy it + var storageDir = Zotero.Attachments.getStorageDirectory(item).path; + var sdtCacheFile = OS.Path.join(storageDir, '.zotero-sdt-cache'); + await Zotero.File.putContentsAsync(sdtCacheFile, 'test'); + + assert.isTrue(await Zotero.Fulltext.indexPDF(linkedFile, item.id)); + assert.isTrue(await OS.File.exists(sdtCacheFile)); + }); }); }); diff --git a/test/tests/sdtTest.js b/test/tests/sdtTest.js new file mode 100644 index 0000000000..a181dab074 --- /dev/null +++ b/test/tests/sdtTest.js @@ -0,0 +1,344 @@ +describe("Zotero.SDT", function () { + const SDT_CACHE_FILE_NAME = '.zotero-sdt-cache'; + const TEST_PDF_HASH = 'e54589353710950c4b7ff70829a60036'; + const SDT_PACK_MAGIC = [0x89, 0x53, 0x44, 0x54, 0x0d, 0x0a, 0x1a, 0x0a]; + const TEST_SDT_PACK_BASE64 = 'iVNEVA0KGgoBAQAAGAAAAHMAAABGAAAAAAAAADMAAAAAAAAAAQAAAB3MsQ7CIBQF0H+5MzWXtpSW1V9wcsPySF2EQGtiGv7daHLmcyKXtEqtqcCd2D9Z4JBDhMJbSn2mF5xuCsHvci3idwlw6NlPHXVHfSPd34XkHQo1HWWVX7b5usFBzGjmZTCD1VwM1/FhY7Sc+8VP5DChtS+rVipITE8tVrKKrlbKTFGyUiowVNJRyklMSs1RslICsZPz80pS80qCEvPSU5WsoqMNYnWiDWNja2N1lPJLS3Iy80CisbUAY2BgYKhWKqksSFWyUipILEpML0osyFDSUSpJrShRslIKSS0uUQh2CVFITkzOSNVTqgUA'; + const STALE_SDT_PACK_BASE64 = 'iVNEVA0KGgoBAQAAGAAAAGAAAABGAAAAAAAAADMAAAAAAAAAAQAAAIXMMQ6DMBBE0btMbaIxBcW2uQIVnYUXkSa2dk2kCPnuEVwgX6/+J6qVVd2LQU60b1UIat4Q8FHzV3lDYg/IqenTNDXNEIwcp4FxYJxJuT1ILgjwctiq12xPvkPAP6H3H6tWKkhMTy1WsoquVspMUbJSKjBU0lHKSUxKzVGyUgKxk/PzSlLzSoIS89JTlayiow1idaINY2NrY3WU8ktLcjLzQKKxtQBjYGBgqFYqqSxIVbJSKkgsSkwvSizIUNJRKkmtKFGyUgpJLS5RCHYJUUhOTM5I1VOqBQA='; + const STALE_PROCESSOR_VERSION_SDT_PACK_BASE64 = 'iVNEVA0KGgoBAQAAGAAAAHMAAABGAAAAAAAAADMAAAAAAAAAAQAAAB3MsQ7CIBQF0H+5MzW3tEDL6i84uWF5pC7SADUxTf/daHLmc2AreZFac4E/0D6bwGOLCQpvKfWZX/D6VIihybVIaBLhoaltx75jfyP934XkHQo172WRX7aGusJDzGimeTCD6zkbLuPDpeQ46TlYcrA4zy+rVipITE8tVrKKrlbKTFGyUiowVNJRyklMSs1RslICsZPz80pS80qCEvPSU5WsoqMNYnWiDWNja2N1lPJLS3Iy80CisbUAY2BgYKhWKqksSFWyUipILEpML0osyFDSUSpJrShRslIKSS0uUQh2CVFITkzOSNVTqgUA'; + const WRONG_PROCESSOR_TYPE_SDT_PACK_BASE64 = 'iVNEVA0KGgoBAQAAGAAAAHMAAABGAAAAAAAAADMAAAAAAAAAAQAAAB3MsQ6DIBQF0H+5szYXFRXW/kKnbojP2KUQwCaN4d8bm5z5nIgpeMk5JNgT5RsFFhKPBQ0+kvIrvGFVbbC6IvckrsgKi47d2FK1VA/S/t1IPtEghyN5ubbd5f3a9KBn0+t+UjSaflimbZs4d8aNZD+i1h+rVipITE8tVrKKrlbKTFGyUiowVNJRyklMSs1RslICsZPz80pS80qCEvPSU5WsoqMNYnWiDWNja2N1lPJLS3Iy80CisbUAY2BgYKhWKqksSFWyUipILEpML0osyFDSUSpJrShRslIKSS0uUQh2CVFITkzOSNVTqgUA'; + + it("should return a valid cached pack", async function () { + let item = await importFileAttachment('test.pdf'); + await writeTestSDTCache(item); + + let result = await getValidPack(item); + + assert.equal(result.packVersion, 1); + assert.equal(result.schemaMajorVersion, 1); + assertPackMagic(result); + }); + + it("should generate the pack when missing", async function () { + let item = await importFileAttachment('test.pdf'); + let cachePath = getSDTCachePath(item); + await OS.File.remove(cachePath, { ignoreAbsent: true }); + + let workerStub = stubStructuredDocumentTextWorker(); + try { + let result = await getValidPack(item); + assert.isTrue(workerStub.calledOnce); + assert.equal(workerStub.firstCall.args[0], item.id); + assert.isTrue(await OS.File.exists(cachePath)); + assertPackMagic(result); + + // A second call should hit the cache + await getValidPack(item); + assert.isTrue(workerStub.calledOnce); + } + finally { + workerStub.restore(); + } + }); + + it("should share a single generation between concurrent getPack() calls", async function () { + let item = await importFileAttachment('test.pdf'); + await OS.File.remove(getSDTCachePath(item), { ignoreAbsent: true }); + + let unblockWorker; + let workerBlocked = new Promise((resolve) => { + unblockWorker = resolve; + }); + let workerStub = sinon.stub(Zotero.PDFWorker, 'getStructuredDocumentText') + .callsFake(async () => { + await workerBlocked; + return { buf: getTestSDTPackBuffer() }; + }); + try { + // One generation per item at a time -- this is also what keeps + // concurrent generations from racing on the cache file write + let promise1 = Zotero.SDT.getPack(item.id); + let promise2 = Zotero.SDT.getPack(item.id); + await waitForStubCall(workerStub); + unblockWorker(); + + let [result1, result2] = await Promise.all([promise1, promise2]); + assert.isTrue(result1.ok, result1.reason); + assert.isTrue(result2.ok, result2.reason); + assert.isTrue(workerStub.calledOnce); + } + finally { + unblockWorker(); + workerStub.restore(); + } + }); + + it("should regenerate a stale pack", async function () { + let item = await importFileAttachment('test.pdf'); + await writeTestSDTCache(item, getStaleSDTPackBytes()); + + let workerStub = stubStructuredDocumentTextWorker(); + try { + await getValidPack(item); + assert.isTrue(workerStub.calledOnce); + } + finally { + workerStub.restore(); + } + }); + + it("should return a stale-processor pack and regenerate it in the background", async function () { + let item = await importFileAttachment('test.pdf'); + await writeTestSDTCache(item, getStaleProcessorVersionSDTPackBytes()); + + let unblockWorker; + let workerBlocked = new Promise((resolve) => { + unblockWorker = resolve; + }); + let workerStub = sinon.stub(Zotero.PDFWorker, 'getStructuredDocumentText') + .callsFake(async () => { + await workerBlocked; + return { buf: getTestSDTPackBuffer() }; + }); + try { + // The old pack is returned immediately while regeneration is + // still blocked in the worker + let result = await getValidPack(item); + assert.deepEqual( + new Uint8Array(result.bytes), + getStaleProcessorVersionSDTPackBytes() + ); + await waitForStubCall(workerStub); + + unblockWorker(); + await waitForCacheBytes(item, getTestSDTPackBytes()); + + // The next call returns the fresh pack without re-extracting + result = await getValidPack(item); + assert.deepEqual(new Uint8Array(result.bytes), getTestSDTPackBytes()); + assert.isTrue(workerStub.calledOnce); + } + finally { + unblockWorker(); + workerStub.restore(); + } + }); + + it("should regenerate a pack with the wrong processor type", async function () { + let item = await importFileAttachment('test.pdf'); + await writeTestSDTCache(item, getWrongProcessorTypeSDTPackBytes()); + + let workerStub = stubStructuredDocumentTextWorker(); + try { + await getValidPack(item); + assert.isTrue(workerStub.calledOnce); + } + finally { + workerStub.restore(); + } + }); + + it("should regenerate a pack with an incompatible schema major version", async function () { + let item = await importFileAttachment('test.pdf'); + await writeTestSDTCache(item, getIncompatibleSchemaMajorSDTPackBytes()); + + let workerStub = stubStructuredDocumentTextWorker(); + try { + let result = await getValidPack(item); + assert.isTrue(workerStub.calledOnce); + assert.equal(result.schemaMajorVersion, 1); + } + finally { + workerStub.restore(); + } + }); + + it("should retry generation after a transient failure", async function () { + let item = await importFileAttachment('test.pdf'); + await OS.File.remove(getSDTCachePath(item), { ignoreAbsent: true }); + + let workerStub = sinon.stub(Zotero.PDFWorker, 'getStructuredDocumentText'); + workerStub.onFirstCall().rejects(new Error('Transient extraction failure')); + workerStub.callsFake(async () => ({ buf: getTestSDTPackBuffer() })); + try { + let result = await Zotero.SDT.getPack(item.id); + assert.isFalse(result.ok); + assert.equal(result.reason, 'failed'); + + await getValidPack(item); + assert.isTrue(workerStub.calledTwice); + } + finally { + workerStub.restore(); + } + }); + + it("shouldn't re-extract a password-protected file until it changes", async function () { + let item = await importFileAttachment('test.pdf'); + await OS.File.remove(getSDTCachePath(item), { ignoreAbsent: true }); + + let error = new Error('Password required'); + error.name = 'PasswordException'; + let workerStub = sinon.stub(Zotero.PDFWorker, 'getStructuredDocumentText') + .rejects(error); + try { + let result = await Zotero.SDT.getPack(item.id); + assert.isFalse(result.ok); + assert.equal(result.reason, 'password-required'); + + result = await Zotero.SDT.getPack(item.id); + assert.isFalse(result.ok); + assert.equal(result.reason, 'password-required'); + assert.isTrue(workerStub.calledOnce); + } + finally { + workerStub.restore(); + } + }); + + it("should regenerate a stale-processor pack before resolving ensure()", async function () { + let item = await importFileAttachment('test.pdf'); + await writeTestSDTCache(item, getStaleProcessorVersionSDTPackBytes()); + + let workerStub = stubStructuredDocumentTextWorker(); + try { + // Unlike getPack(), ensure() doesn't return early with the old + // pack -- once it resolves, the cache must already be current + assert.isTrue(await Zotero.SDT.ensure(item.id)); + assert.isTrue(workerStub.calledOnce); + assert.deepEqual( + await IOUtils.read(getSDTCachePath(item)), + getTestSDTPackBytes() + ); + } + finally { + workerStub.restore(); + } + }); + + it("should return false from ensure() when generation fails", async function () { + let item = await importFileAttachment('test.pdf'); + await OS.File.remove(getSDTCachePath(item), { ignoreAbsent: true }); + + let workerStub = sinon.stub(Zotero.PDFWorker, 'getStructuredDocumentText') + .rejects(new Error('Extraction failure')); + try { + assert.isFalse(await Zotero.SDT.ensure(item.id)); + } + finally { + workerStub.restore(); + } + }); + + it("should return unavailable for unsupported items", async function () { + let item = await importFileAttachment('test.txt'); + let result = await Zotero.SDT.getPack(item.id); + assert.isFalse(result.ok); + assert.equal(result.reason, 'unavailable'); + }); + + it("should generate and open a pack with the real document worker", async function () { + // Cold worker startup fetches the wasm runtime and segmentation models + this.timeout(120000); + + let item = await importFileAttachment('test.pdf'); + let cachePath = getSDTCachePath(item); + await OS.File.remove(cachePath, { ignoreAbsent: true }); + + // getPack() succeeds only if the worker-produced pack parses with the + // bundled SDT module, matches its schema major version, and is + // stamped with the source file's hash, so this catches version + // drift between the document-worker and the bundled module + let result = await getValidPack(item); + assert.isTrue(await OS.File.exists(cachePath)); + assertPackMagic(result); + + // getReader() should return a parsed pack from the cache without + // re-extracting + let reader = await Zotero.SDT.getReader(item.id); + assert.isOk(reader); + let metadata = await reader.getMetadata(); + assert.equal(metadata.source.hash, TEST_PDF_HASH); + }); + + function getSDTCachePath(item) { + return OS.Path.join(Zotero.Attachments.getStorageDirectory(item).path, SDT_CACHE_FILE_NAME); + } + + async function writeTestSDTCache(item, bytes = getTestSDTPackBytes()) { + // The fixture packs embed the hash of test.pdf, so they have to be + // regenerated if the test PDF ever changes + assert.equal(await item.attachmentHash, TEST_PDF_HASH, + 'fixture pack source hash should match test.pdf'); + let cachePath = getSDTCachePath(item); + await OS.File.writeAtomic(cachePath, bytes, { tmpPath: `${cachePath}.tmp` }); + return cachePath; + } + + async function getValidPack(item) { + let result = await Zotero.SDT.getPack(item.id); + assert.isTrue(result.ok, result.reason); + return result; + } + + function assertPackMagic(result) { + assert.deepEqual(Array.from(new Uint8Array(result.bytes, 0, 8)), SDT_PACK_MAGIC); + } + + async function waitForStubCall(stub) { + while (!stub.called) { + await Zotero.Promise.delay(5); + } + } + + async function waitForCacheBytes(item, expected) { + let cachePath = getSDTCachePath(item); + while (true) { + let bytes = await IOUtils.read(cachePath); + if (bytes.length === expected.length && bytes.every((b, i) => b === expected[i])) { + return; + } + await Zotero.Promise.delay(10); + } + } + + function stubStructuredDocumentTextWorker() { + return sinon.stub(Zotero.PDFWorker, 'getStructuredDocumentText') + .callsFake(async () => ({ buf: getTestSDTPackBuffer() })); + } + + function getTestSDTPackBuffer() { + let bytes = getTestSDTPackBytes(); + return bytes.buffer; + } + + function getStaleSDTPackBytes() { + return decodeBase64Bytes(STALE_SDT_PACK_BASE64); + } + + function getStaleProcessorVersionSDTPackBytes() { + return decodeBase64Bytes(STALE_PROCESSOR_VERSION_SDT_PACK_BASE64); + } + + function getWrongProcessorTypeSDTPackBytes() { + return decodeBase64Bytes(WRONG_PROCESSOR_TYPE_SDT_PACK_BASE64); + } + + function getIncompatibleSchemaMajorSDTPackBytes() { + let bytes = getTestSDTPackBytes(); + bytes[9] = 2; + return bytes; + } + + function getTestSDTPackBytes() { + return decodeBase64Bytes(TEST_SDT_PACK_BASE64); + } + + function decodeBase64Bytes(base64) { + let binary = atob(base64); + let bytes = new Uint8Array(binary.length); + for (let i = 0; i < binary.length; i++) { + bytes[i] = binary.charCodeAt(i); + } + return bytes; + } +}); From 55434d14a1796384587361a02d1f437167a5ff27 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Fri, 12 Jun 2026 11:16:22 -0400 Subject: [PATCH 011/465] Encrypt API key and WebDAV password using OS keychain (#5897) Wraps the values stored in nsILoginManager with OSKeyStore, which derives its master key from Keychain on macOS, DPAPI on Windows, and libsecret on Linux. A copy of the profile alone is no longer enough to extract these credentials. Existing plaintext entries are mirrored once per session to a new "(encrypted)" realm but preserved in the original realm so a user can still downgrade to a release that doesn't know about encryption. Active credential changes (sign in, sign out, password change) write to the encrypted realm only and remove the legacy entry. A future version can clear any remaining legacy entries on startup. Patches MOZ_APP_BASENAME in the bundled runtime so the keychain master key is labeled "Zotero Encrypted Storage" rather than "Firefox Encrypted Storage", with a check_line guard so a future Mozilla change to the OSKeyStore label format fails the build instead of silently rebranding the entry. Also fixes check_line to take an explicit file argument. --- app/scripts/fetch_xulrunner | 8 +- app/scripts/utils.sh | 1 + chrome/content/zotero/xpcom/osKeyStore.js | 117 ++++++++++++++++++ chrome/content/zotero/xpcom/storage/webdav.js | 92 +++++++++++--- chrome/content/zotero/xpcom/sync/syncLocal.js | 83 +++++++++++-- chrome/content/zotero/zotero.mjs | 1 + chrome/locale/en-US/zotero/zotero.ftl | 20 +++ 7 files changed, 296 insertions(+), 26 deletions(-) create mode 100644 chrome/content/zotero/xpcom/osKeyStore.js diff --git a/app/scripts/fetch_xulrunner b/app/scripts/fetch_xulrunner index 7ec257f506..6ba631c7a8 100755 --- a/app/scripts/fetch_xulrunner +++ b/app/scripts/fetch_xulrunner @@ -133,6 +133,12 @@ function modify_omni { rm actors/AudioPlayback{Parent,Child}.sys.mjs replace_line 'BROWSER_CHROME_URL:.+' 'BROWSER_CHROME_URL: "chrome:\/\/zotero\/content\/zoteroPane.xhtml",' modules/AppConstants.sys.mjs + # Used by OSKeyStore as the master-key label, visible in macOS Keychain Access. + # Verify that OSKeyStore still derives the label from MOZ_APP_BASENAME, so a + # future Mozilla change to a hardcoded string doesn't silently rebrand the + # keychain entry back to "Firefox Encrypted Storage". + replace_line 'MOZ_APP_BASENAME: "Firefox"' 'MOZ_APP_BASENAME: "Zotero"' modules/AppConstants.sys.mjs + check_line 'STORE_LABEL: AppConstants\.MOZ_APP_BASENAME \+ " Encrypted Storage"' modules/OSKeyStore.sys.mjs # https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/internals/preferences.html # @@ -460,7 +466,7 @@ function modify_omni { chrome/toolkit/content/global/commonDialog.xhtml # commonDialog.css link is split across multiple lines, so we have to do a weird substitution, # so check the one-line global.css to make sure the format hasn't changed - check_line '' + check_line '' chrome/toolkit/content/global/commonDialog.xhtml replace_line 'chrome:\/\/global\/skin\/commonDialog.css"' \ 'chrome:\/\/global\/skin\/commonDialog.css"\/> &1 exit 1 diff --git a/chrome/content/zotero/xpcom/osKeyStore.js b/chrome/content/zotero/xpcom/osKeyStore.js new file mode 100644 index 0000000000..24e8a1efcb --- /dev/null +++ b/chrome/content/zotero/xpcom/osKeyStore.js @@ -0,0 +1,117 @@ +/* + ***** BEGIN LICENSE BLOCK ***** + + Copyright © 2026 Corporation for Digital Scholarship + Vienna, Virginia, USA + https://www.zotero.org + + This file is part of Zotero. + + Zotero is free software: you can redistribute it and/or modify + it under the terms of the GNU Affero General Public License as published by + the Free Software Foundation, either version 3 of the License, or + (at your option) any later version. + + Zotero is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU Affero General Public License for more details. + + You should have received a copy of the GNU Affero General Public License + along with Zotero. If not, see . + + ***** END LICENSE BLOCK ***** +*/ + +// Wrapper around Mozilla's OSKeyStore, which derives an encryption key from +// platform-native key storage (Keychain on macOS, DPAPI on Windows, libsecret +// on Linux). Encrypted values are returned with a versioned prefix so callers +// can distinguish them from legacy plaintext values previously written to +// nsILoginManager. +Zotero.OSKeyStore = { + _prefix: 'oskv1:', + _module: null, + + _load: function () { + if (this._module === null) { + try { + let { OSKeyStore } = ChromeUtils.importESModule( + "resource://gre/modules/OSKeyStore.sys.mjs" + ); + this._module = OSKeyStore; + } + catch (e) { + Zotero.logError(e); + this._module = false; + } + } + return this._module; + }, + + get available() { + return !!this._load(); + }, + + isEncrypted: function (value) { + return typeof value == 'string' && value.startsWith(this._prefix); + }, + + // Show an alert when an active write of new credentials fails (e.g., keychain unavailable) + alertSaveFailed: function () { + let win = Services.wm.getMostRecentWindow('zotero:main'); + if (!win) { + return; + } + Zotero.alert( + win, + Zotero.getString('general-error'), + Zotero.getString('os-keystore-save-failed') + ); + }, + + // Show a one-shot alert when migration of an existing legacy plaintext entry + // fails. The caller falls back to using the legacy value, so the user isn't + // blocked, but show an alert so the keychain issue can be reported and + // addressed before a future version drops the legacy fallback. + alertMigrateFailed: function () { + if (this._migrateAlertShown) { + return; + } + this._migrateAlertShown = true; + let win = Services.wm.getMostRecentWindow('zotero:main'); + if (!win) { + return; + } + Zotero.alert( + win, + Zotero.getString('general-error'), + Zotero.getString('os-keystore-migrate-failed') + ); + }, + + // Returns prefixed ciphertext. Throws if OSKeyStore is unavailable so we + // don't silently store plaintext when a caller expects encryption. + encrypt: async function (plaintext) { + let mod = this._load(); + if (!mod) { + throw new Error("OSKeyStore unavailable"); + } + let ciphertext = await mod.encrypt(plaintext); + return this._prefix + ciphertext; + }, + + // Returns the plaintext, or the input unchanged if it doesn't carry our + // prefix (legacy plaintext). Throws if the value is prefixed but decryption + // fails -- e.g. keychain locked, user canceled the unlock prompt, profile + // copied to a different OS user, ciphertext corrupted. + decrypt: async function (value) { + if (!this.isEncrypted(value)) { + return value; + } + let mod = this._load(); + if (!mod) { + throw new Error("OSKeyStore unavailable but stored value is encrypted"); + } + return mod.decrypt(value.slice(this._prefix.length)); + } +}; diff --git a/chrome/content/zotero/xpcom/storage/webdav.js b/chrome/content/zotero/xpcom/storage/webdav.js index 263ac3bd52..4883b95564 100644 --- a/chrome/content/zotero/xpcom/storage/webdav.js +++ b/chrome/content/zotero/xpcom/storage/webdav.js @@ -214,7 +214,8 @@ Zotero.Sync.Storage.Mode.WebDAV.prototype = { }, _loginManagerHost: 'chrome://zotero', - _loginManagerRealm: 'Zotero Storage Server', + _loginManagerRealm: 'Zotero Storage Server (encrypted)', + _loginManagerRealmLegacy: 'Zotero Storage Server', get defaultError() { @@ -238,14 +239,41 @@ Zotero.Sync.Storage.Mode.WebDAV.prototype = { } Zotero.debug('Getting WebDAV password'); + + // Prefer the legacy realm during the transition window: an older version + // may have written a fresh value there after we migrated. Mirror it to + // the encrypted realm but keep the legacy entry so a downgrade can still + // read it. The legacy realm will be cleared in a future version once + // downgrades are unlikely. + var legacyLogins = await Services.logins.searchLoginsAsync({ + origin: this._loginManagerHost, + httpRealm: this._loginManagerRealmLegacy, + }); + for (let i = 0; i < legacyLogins.length; i++) { + if (legacyLogins[i].username == username) { + let password = legacyLogins[i].password; + if (!this._mirroredPassword) { + try { + Zotero.debug("Mirroring plaintext WebDAV password to encrypted storage"); + await this._writeEncryptedPassword(username, password); + this._mirroredPassword = true; + } + catch (e) { + Zotero.logError(e); + Zotero.OSKeyStore.alertMigrateFailed(); + } + } + return password; + } + } + var logins = await Services.logins.searchLoginsAsync({ origin: this._loginManagerHost, httpRealm: this._loginManagerRealm, }); - // Find user from returned array of nsILoginInfo objects for (var i = 0; i < logins.length; i++) { if (logins[i].username == username) { - return logins[i].password; + return Zotero.OSKeyStore.decrypt(logins[i].password); } } @@ -270,22 +298,43 @@ Zotero.Sync.Storage.Mode.WebDAV.prototype = { return; } - if (password == (await this.getPassword())) { - Zotero.debug("WebDAV password hasn't changed"); - return; + // Skip the write if the password hasn't changed. This is an optimization, + // not a correctness requirement -- if we can't read the existing value + // (e.g. keychain locked), proceed with the write anyway. + try { + if (password == (await this.getPassword())) { + Zotero.debug("WebDAV password hasn't changed"); + return; + } + } + catch (e) { + Zotero.logError(e); } this._basicAuthHeader = false; this._digestParams = null; + try { + await this._writeEncryptedPassword(username, password); + } + catch (e) { + Zotero.OSKeyStore.alertSaveFailed(); + throw e; + } + + // Drop any leftover plaintext entry from the legacy realm var logins = await Services.logins.searchLoginsAsync({ origin: this._loginManagerHost, - httpRealm: this._loginManagerRealm + httpRealm: this._loginManagerRealmLegacy }); - for (var i = 0; i < logins.length; i++) { - Zotero.debug('Clearing WebDAV passwords'); - if (logins[i].httpRealm == this._loginManagerRealm) { - Services.logins.removeLogin(logins[i]); + for (let i = 0; i < logins.length; i++) { + if (logins[i].httpRealm == this._loginManagerRealmLegacy) { + try { + Services.logins.removeLogin(logins[i]); + } + catch (e) { + Zotero.logError(e); + } } break; } @@ -306,13 +355,26 @@ Zotero.Sync.Storage.Mode.WebDAV.prototype = { } break; } + }, + + async _writeEncryptedPassword(username, password) { + // Remove any existing entries in the encrypted realm for this user + var logins = await Services.logins.searchLoginsAsync({ + origin: this._loginManagerHost, + httpRealm: this._loginManagerRealm + }); + for (let i = 0; i < logins.length; i++) { + if (logins[i].username == username) { + Services.logins.removeLogin(logins[i]); + } + } if (password) { - Zotero.debug('Setting WebDAV password'); - var nsLoginInfo = new Components.Constructor("@mozilla.org/login-manager/loginInfo;1", + let storedValue = await Zotero.OSKeyStore.encrypt(password); + let nsLoginInfo = new Components.Constructor("@mozilla.org/login-manager/loginInfo;1", Components.interfaces.nsILoginInfo, "init"); - var loginInfo = new nsLoginInfo(this._loginManagerHost, null, - this._loginManagerRealm, username, password, "", ""); + let loginInfo = new nsLoginInfo(this._loginManagerHost, null, + this._loginManagerRealm, username, storedValue, "", ""); await Services.logins.addLoginAsync(loginInfo); } }, diff --git a/chrome/content/zotero/xpcom/sync/syncLocal.js b/chrome/content/zotero/xpcom/sync/syncLocal.js index d5a50384fd..900889c002 100644 --- a/chrome/content/zotero/xpcom/sync/syncLocal.js +++ b/chrome/content/zotero/xpcom/sync/syncLocal.js @@ -30,7 +30,8 @@ if (!Zotero.Sync.Data) { Zotero.Sync.Data.Local = { _syncQueueIntervals: [0.5, 1, 4, 16, 16, 16, 16, 16, 16, 16, 64], // hours _loginManagerHost: 'chrome://zotero', - _loginManagerRealm: 'Zotero Web API', + _loginManagerRealm: 'Zotero Web API (encrypted)', + _loginManagerRealmLegacy: 'Zotero Web API', _lastSyncTime: null, _lastClassicSyncTime: null, @@ -45,12 +46,35 @@ Zotero.Sync.Data.Local = { /** * @return {Promise} */ - getAPIKey: function () { + getAPIKey: async function () { + // Prefer the legacy realm during the transition window: an older version + // may have written a fresh value there after we migrated, and we want + // to use the most recent value. Mirror it to the encrypted realm but + // keep the legacy entry so a downgrade can still read it. The legacy + // realm will be cleared in a future version once downgrades are + // unlikely. + var legacyLogin = this._getLegacyAPIKeyLoginInfo(); + if (legacyLogin) { + let apiKey = legacyLogin.password; + if (!this._mirroredAPIKey) { + try { + Zotero.debug("Mirroring plaintext API key to encrypted storage"); + await this._writeEncryptedAPIKey(apiKey); + this._mirroredAPIKey = true; + } + catch (e) { + Zotero.logError(e); + Zotero.OSKeyStore.alertMigrateFailed(); + } + } + return apiKey; + } var login = this._getAPIKeyLoginInfo(); - return login - ? login.password - // Fallback to old username/password - : this._getAPIKeyFromLogin(); + if (login) { + return Zotero.OSKeyStore.decrypt(login.password); + } + // Fallback to old username/password + return this._getAPIKeyFromLogin(); }, @@ -58,8 +82,7 @@ Zotero.Sync.Data.Local = { * Check for an API key or a legacy username/password (which may or may not be valid) */ hasCredentials: function () { - var login = this._getAPIKeyLoginInfo(); - if (login) { + if (this._getAPIKeyLoginInfo() || this._getLegacyAPIKeyLoginInfo()) { return true; } // If no API key, check for legacy login @@ -70,6 +93,7 @@ Zotero.Sync.Data.Local = { setAPIKey: async function (apiKey) { var oldLoginInfo = this._getAPIKeyLoginInfo(); + var legacyLoginInfo = this._getLegacyAPIKeyLoginInfo(); // Clear old login if ((!apiKey || apiKey === "")) { @@ -77,10 +101,31 @@ Zotero.Sync.Data.Local = { Zotero.debug("Clearing old API key"); Services.logins.removeLogin(oldLoginInfo); } + if (legacyLoginInfo) { + Services.logins.removeLogin(legacyLoginInfo); + } Zotero.Notifier.trigger('delete', 'api-key', []); return; } + try { + await this._writeEncryptedAPIKey(apiKey); + } + catch (e) { + Zotero.OSKeyStore.alertSaveFailed(); + throw e; + } + // Drop any leftover plaintext entry from the legacy realm + if (legacyLoginInfo) { + Services.logins.removeLogin(legacyLoginInfo); + } + Zotero.Notifier.trigger('modify', 'api-key', []); + }, + + + _writeEncryptedAPIKey: async function (apiKey) { + var oldLoginInfo = this._getAPIKeyLoginInfo(); + var storedValue = await Zotero.OSKeyStore.encrypt(apiKey); var nsLoginInfo = new Components.Constructor("@mozilla.org/login-manager/loginInfo;1", Components.interfaces.nsILoginInfo, "init"); var loginInfo = new nsLoginInfo( @@ -88,7 +133,7 @@ Zotero.Sync.Data.Local = { null, this._loginManagerRealm, 'API Key', - apiKey, + storedValue, '', '' ); @@ -100,7 +145,6 @@ Zotero.Sync.Data.Local = { Zotero.debug("Replacing API key"); Services.logins.modifyLogin(oldLoginInfo, loginInfo); } - Zotero.Notifier.trigger('modify', 'api-key', []); }, @@ -428,6 +472,25 @@ Zotero.Sync.Data.Local = { }, + /** + * @return {nsILoginInfo|false} + */ + _getLegacyAPIKeyLoginInfo: function () { + try { + var logins = Services.logins.findLogins( + this._loginManagerHost, + null, + this._loginManagerRealmLegacy + ); + } + catch (e) { + Zotero.logError(e); + return false; + } + return logins.length ? logins[0] : false; + }, + + _getAPIKeyFromLogin: async function () { let username = Zotero.Prefs.get('sync.server.username'); if (username) { diff --git a/chrome/content/zotero/zotero.mjs b/chrome/content/zotero/zotero.mjs index 6dc8648269..3ec8d7a7a0 100644 --- a/chrome/content/zotero/zotero.mjs +++ b/chrome/content/zotero/zotero.mjs @@ -113,6 +113,7 @@ const xpcomFilesLocal = [ 'mime', 'notifier', 'fileHandlers', + 'osKeyStore', 'plugins', 'pluginAPI/menuManager', 'pluginAPI/itemPaneManager', diff --git a/chrome/locale/en-US/zotero/zotero.ftl b/chrome/locale/en-US/zotero/zotero.ftl index 42cd1e48d8..c2b108c0c6 100644 --- a/chrome/locale/en-US/zotero/zotero.ftl +++ b/chrome/locale/en-US/zotero/zotero.ftl @@ -25,6 +25,12 @@ delete-or-backspace = [macos] Delete *[other] Backspace } +-os-name = + { PLATFORM() -> + [macos] macOS + [windows] Windows + *[other] Linux + } general-print = Print general-remove = Remove @@ -904,3 +910,17 @@ plugins-blocked-plugin = .message = This plugin has been disabled by { -app-name }. data-dir-unsupported-storage = This can happen if the { -app-name } data directory is in a cloud storage folder (OneDrive, Dropbox, etc.) or on a network share. + +os-keystore-save-failed = + { PLATFORM() -> + [macos] { -app-name } couldn’t access the { -os-name } Keychain to securely save your credentials. Make sure your Keychain is accessible and try again. + [windows] { -app-name } couldn’t securely save your credentials. Try again or restart { -app-name }. + *[other] { -app-name } couldn’t access your { -os-name } keyring to securely save your credentials. Make sure a keyring service is running and try again. + } + +os-keystore-migrate-failed = + { PLATFORM() -> + [macos] { -app-name } couldn’t access the { -os-name } Keychain to encrypt your stored credentials. Your credentials remain stored unencrypted on disk. Make sure your Keychain is accessible and restart { -app-name }. + [windows] { -app-name } couldn’t encrypt your stored credentials. Your credentials remain stored unencrypted on disk. Restart { -app-name } and try again. + *[other] { -app-name } couldn’t access your { -os-name } keyring to encrypt your stored credentials. Your credentials remain stored unencrypted on disk. Make sure a keyring service is running and restart { -app-name }. + } From 632f11db749c38ebb3e9201a2d4f311bfca0ddc2 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Fri, 12 Jun 2026 11:07:34 -0400 Subject: [PATCH 012/465] Fix corrupted-login-manager warning never appearing The once-per-minute rate limit from e40fff6a7d (5.0.91) was inverted, and the last-error time was never initialized, so the alert hasn't appeared since 2020. --- chrome/content/zotero/xpcom/sync/syncLocal.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/chrome/content/zotero/xpcom/sync/syncLocal.js b/chrome/content/zotero/xpcom/sync/syncLocal.js index 900889c002..3e4e1ab34a 100644 --- a/chrome/content/zotero/xpcom/sync/syncLocal.js +++ b/chrome/content/zotero/xpcom/sync/syncLocal.js @@ -458,7 +458,8 @@ Zotero.Sync.Data.Local = { } catch (e) { Zotero.logError(e); - if (this._lastLoginManagerErrorTime > Date.now() - 60000) { + if (!this._lastLoginManagerErrorTime + || this._lastLoginManagerErrorTime < Date.now() - 60000) { let msg = Zotero.getString('sync.error.loginManagerCorrupted1', Zotero.appName) + "\n\n" + Zotero.getString('sync.error.loginManagerCorrupted2', Zotero.appName); Zotero.alert(null, Zotero.getString('general.error'), msg); From cd39445b94db9f8f5bd0fbad3f4f6c6233fa723b Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Fri, 12 Jun 2026 11:07:34 -0400 Subject: [PATCH 013/465] Automatically repair unusable login manager Zotero never sets a primary password, so if one is set on the NSS key database, it was either corrupted or copied in from a Firefox profile, and stored logins can never be decrypted, since there's no primary-password prompt. This previously made it impossible to save credentials without manually deleting cert9.db, key4.db, and logins.json from the profile directory. If reading or saving credentials fails and a primary password is set, clear stored logins and reset the key database so that credentials can be saved again. Saving the API key or WebDAV password is retried automatically, so logging in completes without a manual fix, and if previously stored credentials are lost to a reset, show a one-time alert prompting the user to log in again. --- chrome/content/zotero/xpcom/storage/webdav.js | 15 ++++- chrome/content/zotero/xpcom/sync/syncLocal.js | 64 ++++++++++++++++++- chrome/locale/en-US/zotero/zotero.ftl | 2 + test/tests/syncLocalTest.js | 35 ++++++++++ 4 files changed, 112 insertions(+), 4 deletions(-) diff --git a/chrome/content/zotero/xpcom/storage/webdav.js b/chrome/content/zotero/xpcom/storage/webdav.js index 4883b95564..a3cf95e4a6 100644 --- a/chrome/content/zotero/xpcom/storage/webdav.js +++ b/chrome/content/zotero/xpcom/storage/webdav.js @@ -318,8 +318,19 @@ Zotero.Sync.Storage.Mode.WebDAV.prototype = { await this._writeEncryptedPassword(username, password); } catch (e) { - Zotero.OSKeyStore.alertSaveFailed(); - throw e; + // If the write failed because the key database was unusable, reset the login + // manager and retry + if (!Zotero.Sync.Data.Local.repairLoginManager()) { + Zotero.OSKeyStore.alertSaveFailed(); + throw e; + } + try { + await this._writeEncryptedPassword(username, password); + } + catch (e) { + Zotero.OSKeyStore.alertSaveFailed(); + throw e; + } } // Drop any leftover plaintext entry from the legacy realm diff --git a/chrome/content/zotero/xpcom/sync/syncLocal.js b/chrome/content/zotero/xpcom/sync/syncLocal.js index 3e4e1ab34a..dd07bd97b9 100644 --- a/chrome/content/zotero/xpcom/sync/syncLocal.js +++ b/chrome/content/zotero/xpcom/sync/syncLocal.js @@ -73,6 +73,16 @@ Zotero.Sync.Data.Local = { if (login) { return Zotero.OSKeyStore.decrypt(login.password); } + // If the login manager had to be reset, the stored API key is gone, so tell the user + // to log in again + if (this._loginManagerRepaired && !this._loginManagerRepairAlertShown) { + this._loginManagerRepairAlertShown = true; + Zotero.alert( + null, + Zotero.getString('general-error'), + Zotero.getString('login-manager-reset') + ); + } // Fallback to old username/password return this._getAPIKeyFromLogin(); }, @@ -112,8 +122,19 @@ Zotero.Sync.Data.Local = { await this._writeEncryptedAPIKey(apiKey); } catch (e) { - Zotero.OSKeyStore.alertSaveFailed(); - throw e; + // If the write failed because the key database was unusable, reset the login + // manager and retry, so that logging in works without a manual fix + if (!this.repairLoginManager()) { + Zotero.OSKeyStore.alertSaveFailed(); + throw e; + } + try { + await this._writeEncryptedAPIKey(apiKey); + } + catch (e) { + Zotero.OSKeyStore.alertSaveFailed(); + throw e; + } } // Drop any leftover plaintext entry from the legacy realm if (legacyLoginInfo) { @@ -445,6 +466,40 @@ Zotero.Sync.Data.Local = { }, + /** + * Reset the login manager if the NSS key database is unusable + * + * Zotero never sets a primary password, so if one is set on the key database, it was either + * corrupted or copied in from a Firefox profile, and stored logins can never be decrypted, + * since there's no primary-password prompt. Clear stored logins and reset the key database + * so that credentials can be saved again. + * + * @return {Boolean} - True if the login manager was reset + */ + repairLoginManager: function () { + try { + let token = Cc["@mozilla.org/security/pk11tokendb;1"] + .getService(Ci.nsIPK11TokenDB) + .getInternalKeyToken(); + if (!token.hasPassword) { + return false; + } + Zotero.debug("Primary password set on NSS key database -- resetting login manager", 1); + Services.logins.removeAllLogins(); + token.reset(); + if (token.needsUserInit) { + token.initPassword(""); + } + } + catch (e) { + Zotero.logError(e); + return false; + } + this._loginManagerRepaired = true; + return true; + }, + + /** * @return {nsILoginInfo|false} */ @@ -458,6 +513,11 @@ Zotero.Sync.Data.Local = { } catch (e) { Zotero.logError(e); + // If the key database was unusable, reset the login manager so that credentials + // can be saved again + if (this.repairLoginManager()) { + return false; + } if (!this._lastLoginManagerErrorTime || this._lastLoginManagerErrorTime < Date.now() - 60000) { let msg = Zotero.getString('sync.error.loginManagerCorrupted1', Zotero.appName) + "\n\n" diff --git a/chrome/locale/en-US/zotero/zotero.ftl b/chrome/locale/en-US/zotero/zotero.ftl index c2b108c0c6..cb5a9df4ee 100644 --- a/chrome/locale/en-US/zotero/zotero.ftl +++ b/chrome/locale/en-US/zotero/zotero.ftl @@ -911,6 +911,8 @@ plugins-blocked-plugin = data-dir-unsupported-storage = This can happen if the { -app-name } data directory is in a cloud storage folder (OneDrive, Dropbox, etc.) or on a network share. +login-manager-reset = { -app-name } was unable to read your saved login information, so it has been reset. Please log in again in the { preferences-pane-account } pane of the { -app-name } settings. + os-keystore-save-failed = { PLATFORM() -> [macos] { -app-name } couldn’t access the { -os-name } Keychain to securely save your credentials. Make sure your Keychain is accessible and try again. diff --git a/test/tests/syncLocalTest.js b/test/tests/syncLocalTest.js index 79ec9d6f17..fd52a816d5 100644 --- a/test/tests/syncLocalTest.js +++ b/test/tests/syncLocalTest.js @@ -19,6 +19,41 @@ describe("Zotero.Sync.Data.Local", function () { assert.strictEqual(await Zotero.Sync.Data.Local.getAPIKey(apiKey), ""); }) }) + + + describe("#repairLoginManager()", function () { + it("should reset a key database with a primary password set", async function () { + var token = Components.classes["@mozilla.org/security/pk11tokendb;1"] + .getService(Components.interfaces.nsIPK11TokenDB) + .getInternalKeyToken(); + + // No-op if no primary password is set + assert.isFalse(Zotero.Sync.Data.Local.repairLoginManager()); + + if (token.needsUserInit) { + token.initPassword("repair-test"); + } + else { + token.changePassword("", "repair-test"); + } + try { + assert.isTrue(token.hasPassword); + assert.isTrue(Zotero.Sync.Data.Local.repairLoginManager()); + assert.isFalse(token.hasPassword); + + // Credentials should be saveable and readable again + var apiKey = Zotero.Utilities.randomString(24); + await Zotero.Sync.Data.Local.setAPIKey(apiKey); + assert.equal(await Zotero.Sync.Data.Local.getAPIKey(), apiKey); + } + finally { + if (token.hasPassword) { + token.changePassword("repair-test", ""); + } + await Zotero.Sync.Data.Local.setAPIKey(""); + } + }) + }) describe("#checkUser()", function () { From c72d80f22bc8931c066eb724b8701382fef838a5 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Fri, 12 Jun 2026 12:57:51 -0400 Subject: [PATCH 014/465] Update Word for Windows submodule --- app/modules/zotero-word-for-windows-integration | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/modules/zotero-word-for-windows-integration b/app/modules/zotero-word-for-windows-integration index 27b4aea971..c0aa6e4bef 160000 --- a/app/modules/zotero-word-for-windows-integration +++ b/app/modules/zotero-word-for-windows-integration @@ -1 +1 @@ -Subproject commit 27b4aea971dd2c10e4e484f48ded60385a6839c2 +Subproject commit c0aa6e4bef039d94e17e81cb28b1fe9170c45b96 From 40f949ba9491d832cfccf7292dfbb2f030647adf Mon Sep 17 00:00:00 2001 From: Bogdan Abaev Date: Mon, 1 Jun 2026 09:05:57 -0700 Subject: [PATCH 015/465] Citation dialog: allow accepting while details popup is open - allow to accept the dialog via cmd/ctrl+Enter from inside of a panel - enable click-through on item details panel, so one can click on the "Accept" button without having to close the popup first These two measures allow one to add a bubble, modify the citation via item details popup, and accept it with one less step (without having to close the popup first) --- chrome/content/zotero/integration/citationDialog.xhtml | 2 +- .../zotero/integration/citationDialog/keyboardHandler.mjs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/chrome/content/zotero/integration/citationDialog.xhtml b/chrome/content/zotero/integration/citationDialog.xhtml index 8116cc596f..3e31708baf 100644 --- a/chrome/content/zotero/integration/citationDialog.xhtml +++ b/chrome/content/zotero/integration/citationDialog.xhtml @@ -142,7 +142,7 @@
- +
diff --git a/chrome/content/zotero/integration/citationDialog/keyboardHandler.mjs b/chrome/content/zotero/integration/citationDialog/keyboardHandler.mjs index d5e82f99da..0928ac5cd8 100644 --- a/chrome/content/zotero/integration/citationDialog/keyboardHandler.mjs +++ b/chrome/content/zotero/integration/citationDialog/keyboardHandler.mjs @@ -49,8 +49,8 @@ export class CitationDialogKeyboardHandler { // lower-level components captureKeydown(event) { let cmdOrCtrl = Zotero.isMac ? event.metaKey : event.ctrlKey; - // Cmd/Ctrl-Enter will always accept the dialog regardless of the target (unless within a panel) - if (event.key == "Enter" && cmdOrCtrl && !event.target.closest("panel")) { + // Cmd/Ctrl-Enter will always accept the dialog regardless of the target + if (event.key == "Enter" && cmdOrCtrl) { this.doc.dispatchEvent(new CustomEvent("dialog-accepted")); event.stopPropagation(); event.preventDefault(); From 0f2d3e103d4d640a568537587a52e136393e8612 Mon Sep 17 00:00:00 2001 From: Bogdan Abaev Date: Mon, 1 Jun 2026 09:16:20 -0700 Subject: [PATCH 016/465] Citation dialog: show placeholder after a new bubble is added (#5932) After the first bubble is added, the focused input gets a placeholder indicating that typing a number will add it as a page to the just-added bubble. The placeholder is truncated if it's too close to the edge in multi-item citations. Also add a tip to the item details popup explaining that locators can be typed into the main input field, with a link to the documentation. The tip stops appearing once a typed locator has been used. --------- Co-authored-by: Dan Stillman --- chrome/content/zotero/elements/bubbleInput.js | 96 ++++++++++++++++++- .../zotero/integration/citationDialog.js | 30 +++++- .../zotero/integration/citationDialog.xhtml | 7 +- .../citationDialog/popupHandler.mjs | 3 + chrome/locale/en-US/zotero/integration.ftl | 6 ++ defaults/preferences/zotero.js | 1 + scss/components/_citationDialog.scss | 8 ++ scss/elements/_bubbleInput.scss | 2 +- 8 files changed, 143 insertions(+), 10 deletions(-) diff --git a/chrome/content/zotero/elements/bubbleInput.js b/chrome/content/zotero/elements/bubbleInput.js index 6f93259659..7492c8cae8 100644 --- a/chrome/content/zotero/elements/bubbleInput.js +++ b/chrome/content/zotero/elements/bubbleInput.js @@ -36,6 +36,7 @@ this._body = this.querySelector('.bubble-input.body'); this._body.addEventListener('click', this._onBodyClick.bind(this)); this._lastFocusedInput = null; + this.showJustAddedPlaceholder = false; Utils.init(this); DragDropHandler.init(this); @@ -123,11 +124,10 @@ if (isOnlyInput) { document.l10n.setAttributes(this._body.firstChild, `integration-citationDialog-single-input-${dialogType}`); } - // otherwise, add a regular aria descriptions and placeholders to all inputs + // otherwise, set default placeholders for all inputs, with special handling + // of the last input after a bubble is added with visible placeholder else { - for (let input of [...this.querySelectorAll(".input")]) { - document.l10n.setAttributes(input, `integration-citationDialog-input-${dialogType}`); - } + Utils.setupInputPlaceholders(this.showJustAddedPlaceholder, dialogType); } // If any two inputs end up next to each other (e.g. after bubble is deleted), // have them merged @@ -343,6 +343,13 @@ if (!Utils.isInputEmpty(input) || !this.contains(event.relatedTarget)) { this._lastFocusedInput = input; } + // Collapse a placeholder input back to regular size once focus leaves. + // The placeholder attributes are reset on the next refresh() + if (input.classList.contains("just-added-placeholder")) { + input.classList.remove("just-added-placeholder"); + input.removeAttribute("title"); + input.style.minWidth = ""; + } }); return input; } @@ -604,6 +611,87 @@ return spanWidth; }, + getTextWidth(text) { + let span = document.createElement("span"); + span.classList = "input"; + span.innerText = text; + this.bubbleInput._body.appendChild(span); + let spanWidth = span.getBoundingClientRect().width; + span.remove(); + return spanWidth; + }, + + // Return the longest prefix of `text` such that prefix + "…" fits within maxWidth, + // or the original text if it already fits. Used for placeholder truncation, since + // text-overflow:ellipsis doesn't work on in Firefox chrome. + truncateToWidth(text, maxWidth) { + if (this.getTextWidth(text) <= maxWidth) return text; + let ellipsis = "…"; + for (let i = text.length - 1; i > 0; i--) { + let candidate = text.slice(0, i) + ellipsis; + if (this.getTextWidth(candidate) <= maxWidth) { + return candidate; + } + } + return ellipsis; + }, + + // Set aria-descriptions and placeholders on every input in the bubble-input. + // For the just-added input (the last one, when showJustAddedPlaceholder is set), the + // placeholder is truncated to the space remaining on its line and passed to Fluent as + // a variable. + setupInputPlaceholders(showJustAddedPlaceholder, dialogType) { + let allInputs = [...this.bubbleInput.querySelectorAll(".input")]; + let lastInput = allInputs[allInputs.length - 1]; + let bodyRight = this.bubbleInput._body.getBoundingClientRect().right; + for (let input of allInputs) { + let isJustAdded = showJustAddedPlaceholder && input === lastInput; + if (!isJustAdded) { + // If the just-added placeholder was dismissed (e.g. by right-arrow at the + // end of the input) while the input is still focused and empty, keep the + // visible placeholder but fall back to the default search prompt, since + // searching still works. It collapses when the input loses focus (see the + // blur handler in _createInputElem()) + if (input.classList.contains("just-added-placeholder") + && document.activeElement == input && !input.value) { + let placeholder = this.truncateToWidth( + " " + Zotero.getString("integration-citationDialog-search-for-items"), + parseFloat(input.style.minWidth) || Infinity); + document.l10n.setAttributes(input, "integration-citationDialog-just-added-input-citation", { placeholder, title: "" }); + continue; + } + input.classList.remove("just-added-placeholder"); + document.l10n.setAttributes(input, `integration-citationDialog-input-${dialogType}`); + // Clear any stale title and min-width left over from a previous just-added state + input.removeAttribute("title"); + input.style.minWidth = ""; + } + if (isJustAdded && !input.classList.contains("just-added-placeholder")) { + input.classList.add("just-added-placeholder"); + // Leading NBSP gives a small visual gap between the cursor and the placeholder + // text (CSS padding/text-indent on input or ::placeholder both move the cursor too) + let fullPlaceholder = " " + Zotero.getString("integration-citationDialog-just-added-input-placeholder"); + let availableWidth = bodyRight - input.getBoundingClientRect().left - 20; + let placeholderWidth = this.getTextWidth(fullPlaceholder); + let placeholder, title, minWidth; + if (availableWidth >= placeholderWidth) { + placeholder = fullPlaceholder; + title = ""; + minWidth = placeholderWidth; + } + else { + placeholder = this.truncateToWidth(fullPlaceholder, availableWidth); + title = fullPlaceholder; + minWidth = availableWidth; + } + // min-width keeps the placeholder visible when the input is empty, but + // lets the input grow to fit content the user types beyond the placeholder. + input.style.minWidth = minWidth + 'px'; + document.l10n.setAttributes(input, "integration-citationDialog-just-added-input-citation", { placeholder, title }); + } + } + }, + // If a bubble is removed between two inputs we need to combine them combineNeighboringInputs(startingNode) { let node = startingNode; diff --git a/chrome/content/zotero/integration/citationDialog.js b/chrome/content/zotero/integration/citationDialog.js index c35d1365bb..f0100c65fc 100644 --- a/chrome/content/zotero/integration/citationDialog.js +++ b/chrome/content/zotero/integration/citationDialog.js @@ -1206,6 +1206,7 @@ class ListLayout extends Layout { const IOManager = { sectionExpandedStatus: {}, _skipInputAcceptOnEnterUntil: 0, + _timesItemsAdded: 0, // most essential IO functionality that is added immediately on load preInit() { @@ -1406,7 +1407,12 @@ const IOManager = { // If no locator is provided, record which bubbles were just added. // If a locator is typed next, these bubbles will receive it. this._justAddedBubbles = bubbleItems; + // Only show the placeholder guidance on the first add -- after + // that, the user presumably knows about the shortcut + _id("bubble-input").showJustAddedPlaceholder = DIALOG_STATE.isCitingItems() + && this._timesItemsAdded < 1; } + this._timesItemsAdded++; await CitationDataManager.addItems({ bubbleItems, index }); // Refresh the itemTree if in library mode if (currentLayout.type == "library") { @@ -1695,6 +1701,9 @@ const IOManager = { input.value = ""; input.dispatchEvent(new Event('input', { bubbles: true })); this.updateBubbleInput(); + // The typed-locator shortcut has been used, so stop showing the tip + // about it in the item details popup + Zotero.Prefs.set("integration.citationDialogShowLocatorTip", false); return; } } @@ -1735,9 +1744,14 @@ const IOManager = { _deleteItem(dialogReferenceID) { CitationDataManager.deleteItem({ dialogReferenceID }); + // If the citation is emptied, show the placeholder guidance again on the next add + if (!CitationDataManager.items.length) { + this._timesItemsAdded = 0; + } if (currentLayout.type == "library") { libraryLayout.refreshItemsView(); } + this._clearJustAddedBubbles(); this.updateBubbleInput(); // Always refresh items list to make sure the opened and selected items are up to date currentLayout.refreshItemsList(); @@ -1835,9 +1849,14 @@ const IOManager = { bubbleItem.label = "page"; } IOManager._hideLoadingSpinner(); - // Clear the input and update bubbles + // Clear the input and update bubbles. The placeholder stays, since both of its + // suggestions still apply -- typed digits keep appending to the locator, and + // any other input starts a search input.value = ""; IOManager.updateBubbleInput(); + // The typed-locator shortcut has been used, so stop showing the tip + // about it in the item details popup + Zotero.Prefs.set("integration.citationDialogShowLocatorTip", false); // Disable Enter on input from accepting the dialog for the next 500ms; // If one intends to confirmed the numeric locator by pressing Enter (via _handleInputEnter), // we ensure that the Enter keypress won't happen right after when the locator is added to @@ -1851,11 +1870,14 @@ const IOManager = { // and Enter is presses, just-added bubbles get that locator. _clearJustAddedBubbles(event) { if (!this._justAddedBubbles) return; - // on keydown, only proceed if it's an arrow key - let navigationKeys = ["ArrowUp", "ArrowDown", "ArrowRight", "ArrowLeft"]; - if (event && event.type == "keydown" && !navigationKeys.includes(event.key)) return; + // On keydown, only proceed for left/right arrows, which move to another + // reference (e.g. to explicitly search for a year). Up/down arrows just move + // the list selection while focus remains in the input, so locator entry + // stays active. + if (event && event.type == "keydown" && !["ArrowLeft", "ArrowRight"].includes(event.key)) return; // clear just added bubbles and update bubble input to reflect that this._justAddedBubbles = null; + _id("bubble-input").showJustAddedPlaceholder = false; this.updateBubbleInput(); }, diff --git a/chrome/content/zotero/integration/citationDialog.xhtml b/chrome/content/zotero/integration/citationDialog.xhtml index 3e31708baf..a1122cd14d 100644 --- a/chrome/content/zotero/integration/citationDialog.xhtml +++ b/chrome/content/zotero/integration/citationDialog.xhtml @@ -31,7 +31,9 @@ xmlns="http://www.w3.org/1999/xhtml" xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" drawintitlebar-platforms="mac,win" - resizable="false"> + resizable="false" + windowtype="zotero:citation-dialog" + > @@ -154,6 +156,9 @@ +
+ +
diff --git a/chrome/content/zotero/integration/citationDialog/popupHandler.mjs b/chrome/content/zotero/integration/citationDialog/popupHandler.mjs index 1d1ce61f34..22e165f528 100644 --- a/chrome/content/zotero/integration/citationDialog/popupHandler.mjs +++ b/chrome/content/zotero/integration/citationDialog/popupHandler.mjs @@ -99,6 +99,9 @@ export class CitationDialogPopupsHandler { popup.style.top = `${bubbleRect.bottom + 10}px`; this._getNode("#itemDetails .show").hidden = !this.bubbleItem.item.id; + // Hide the typed-locator tip once the shortcut has been used + this._getNode("#itemDetails .details-locator-info").hidden + = !Zotero.Prefs.get("integration.citationDialogShowLocatorTip"); let topLevelItem = this.bubbleItem.item.topLevelItem; // Add header and fill inputs with their values diff --git a/chrome/locale/en-US/zotero/integration.ftl b/chrome/locale/en-US/zotero/integration.ftl index fdc2d14062..b7653c4892 100644 --- a/chrome/locale/en-US/zotero/integration.ftl +++ b/chrome/locale/en-US/zotero/integration.ftl @@ -30,6 +30,7 @@ integration-citationDialog-section-cited = { $count -> integration-citationDialog-details-suffix = Suffix integration-citationDialog-details-prefix = Prefix integration-citationDialog-details-suppressAuthor = Omit Author +integration-citationDialog-details-locator-info = Tip: You can also type page numbers and other locators directly into the main field. Learn more integration-citationDialog-details-includeComments = Include Comments integration-citationDialog-details-remove = { general-remove } integration-citationDialog-details-done = @@ -77,6 +78,11 @@ integration-citationDialog-aria-bubble = integration-citationDialog-single-input-citation = .placeholder = { integration-citationDialog-search-for-items } .aria-description = Press Tab to select items to add to this citation. Press Escape to discard the changes and close the dialog. +integration-citationDialog-just-added-input-placeholder = Type “10-15” to cite pages, or search for items +integration-citationDialog-just-added-input-citation = + .placeholder = { $placeholder } + .title = { $title } + .aria-description = { integration-citationDialog-general-instructions } integration-citationDialog-input-citation = .placeholder = { integration-citationDialog-search-for-items } .aria-description = { integration-citationDialog-general-instructions } diff --git a/defaults/preferences/zotero.js b/defaults/preferences/zotero.js index 3c66fa7089..e9caa7242b 100644 --- a/defaults/preferences/zotero.js +++ b/defaults/preferences/zotero.js @@ -152,6 +152,7 @@ pref("extensions.zotero.integration.keepAddCitationDialogRaised", false); pref("extensions.zotero.integration.upgradeTemplateDelayedOn", 0); pref("extensions.zotero.integration.dontPromptMendeleyImport", false); pref("extensions.zotero.integration.citationDialogMode", "last-used"); +pref("extensions.zotero.integration.citationDialogShowLocatorTip", true); pref("extensions.zotero.integration.annotationDialogIncludeComments", true); pref("extensions.zotero.integration.citationPreviewShown", true); diff --git a/scss/components/_citationDialog.scss b/scss/components/_citationDialog.scss index 84810ec27f..298e4675bb 100644 --- a/scss/components/_citationDialog.scss +++ b/scss/components/_citationDialog.scss @@ -786,6 +786,14 @@ height: 24px; // same height as input } } + .details-locator-info { + padding-inline-start: 5px; + grid-column: 2; + // pull closer to the locator input above + margin-top: -4px; + color: var(--fill-secondary); + font-size: .93rem; + } } .buttons { display: flex; diff --git a/scss/elements/_bubbleInput.scss b/scss/elements/_bubbleInput.scss index 2a72c9c675..905dc08706 100644 --- a/scss/elements/_bubbleInput.scss +++ b/scss/elements/_bubbleInput.scss @@ -52,7 +52,7 @@ bubble-input { // inputs initially occupy no width so they don't misalign bubbles at the // start of each line. On focus, set their width for the cursor to appear // and offset it by negative margin to avoid bubbles shifting - &.empty:not(.full-width):focus { + &.empty:not(.full-width):not(.just-added-placeholder):focus { min-width: 1px; margin-inline-start: -1px; } From cd29a818b0681065b7dc80998877791f85dda381 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Thu, 11 Jun 2026 15:05:08 -0400 Subject: [PATCH 017/465] Don't let hover background override selected item background --- scss/components/_citationDialog.scss | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/scss/components/_citationDialog.scss b/scss/components/_citationDialog.scss index 298e4675bb..d3c5e764cc 100644 --- a/scss/components/_citationDialog.scss +++ b/scss/components/_citationDialog.scss @@ -456,7 +456,10 @@ &.selected { background-color: var(--selected-item-background); border-radius: 0; - + } + // selected background wins over the hover effect + &.selected:not([disabled]):hover { + background-color: var(--selected-item-background); } &.selected-first { border-top-left-radius: 5px; From d2f1c56250cfbaeaef61dc3726d0ccfcaa0265ea Mon Sep 17 00:00:00 2001 From: Bogdan Abaev Date: Mon, 25 Sep 2023 17:05:02 -0400 Subject: [PATCH 018/465] keep search conditionIDs in arithmetic sequence When conditions are removed, shift conditionIDs so that conditionIDs always go in increments of 1 (0, 1, 2 ...). It prevents conditionIDs from conflicting with each other when conditions are rearranged. Fixes: #3434 --- .../content/zotero/elements/zoteroSearch.js | 13 ++++++++++-- chrome/content/zotero/xpcom/data/search.js | 20 ++++++++++++++++-- test/tests/searchTest.js | 21 +++++++++++++++++++ 3 files changed, 50 insertions(+), 4 deletions(-) diff --git a/chrome/content/zotero/elements/zoteroSearch.js b/chrome/content/zotero/elements/zoteroSearch.js index a73b56e1b7..0150c160a5 100644 --- a/chrome/content/zotero/elements/zoteroSearch.js +++ b/chrome/content/zotero/elements/zoteroSearch.js @@ -145,11 +145,20 @@ var conditionsBox = this.querySelector('#conditions'); this.search.removeCondition(id); - + let found = false; for (var i = 0, len = conditionsBox.childNodes.length; i < len; i++) { if (conditionsBox.childNodes[i].conditionID == id) { conditionsBox.removeChild(conditionsBox.childNodes[i]); - break; + found = true; + i--; + len--; + continue; + } + // When a condition is removed, ids of remaining conditions + // are shifted to remain in arithmetic sequence. The conditionID + // of the nodes need to be updated accordingly + if (found) { + conditionsBox.childNodes[i].conditionID--; } } diff --git a/chrome/content/zotero/xpcom/data/search.js b/chrome/content/zotero/xpcom/data/search.js index d66ee0d3d9..8110841918 100644 --- a/chrome/content/zotero/xpcom/data/search.js +++ b/chrome/content/zotero/xpcom/data/search.js @@ -482,7 +482,20 @@ Zotero.Search.prototype.removeCondition = function (searchConditionID) { throw new Error('Invalid searchConditionID ' + searchConditionID + ' in removeCondition()'); } - delete this._conditions[searchConditionID]; + searchConditionID = String(searchConditionID); + // Decrement the id of all conditions following + // the condition to be deleted. It ensures that + // all conditions remain in stict arithmetic sequence and prevents + // conditionIDs from colliding + let conditionIDs = Object.keys(this._conditions); + let conditionIndex = conditionIDs.indexOf(searchConditionID); + for (let i = conditionIndex + 1; i < conditionIDs.length; i++) { + let conditionID = conditionIDs[i]; + this._conditions[conditionID - 1] = this._conditions[conditionID]; + this._conditions[conditionID - 1].id = conditionID - 1; + } + // After all conditions are shifted, delete the last, empty one + delete this._conditions[this._maxSearchConditionID]; this._maxSearchConditionID--; this._markFieldChange('conditions', this._conditions); this._changed.conditions = true; @@ -864,7 +877,10 @@ Zotero.Search.prototype.fromJSON = function (json, options = {}) { this.name = json.name; } - Object.keys(this.getConditions()).forEach(id => this.removeCondition(id)); + // Remove all conditions + while (Object.keys(this._conditions).length) { + this.removeCondition(Object.keys(this._conditions)[0]); + } for (let i = 0; i < json.conditions.length; i++) { let condition = json.conditions[i]; this.addCondition( diff --git a/test/tests/searchTest.js b/test/tests/searchTest.js index cc296eb5b5..4dc950174e 100644 --- a/test/tests/searchTest.js +++ b/test/tests/searchTest.js @@ -216,6 +216,27 @@ describe("Zotero.Search", function () { var matches = await s.search(); assert.lengthOf(matches, 0); }); + + it("should have same result after the same search conditions is removed and added", async function () { + var itemOne = await createDataObject('item', { title: "One" }); + var itemTwo = await createDataObject('item', { title: "Two" }); + + var s = new Zotero.Search(); + s.libraryID = itemOne.libraryID; + s.addCondition("joinMode", "any"); + // Match both collections + s.addCondition('title', 'contains', 'One'); + s.addCondition('title', 'contains', 'Two'); + var matches = await s.search(); + assert.sameMembers(matches, [itemOne.id, itemTwo.id]); + + // Remove the first condition and add it again + s.removeCondition(1); + s.addCondition('title', 'contains', 'One'); + matches = await s.search(); + // Result should be the same + assert.sameMembers(matches, [itemOne.id, itemTwo.id]); + }); }); describe("tag", function () { From 00527332c4e3ec9ddc80a2de8bc962bf7a678b40 Mon Sep 17 00:00:00 2001 From: Abe Jellinek Date: Fri, 12 Dec 2025 10:35:03 -0500 Subject: [PATCH 019/465] Move Advanced Search and saved search editing to the main window (#5658) --------- Co-authored-by: Dan Stillman --- chrome/content/zotero/advancedSearch.js | 194 ------------- chrome/content/zotero/advancedSearch.xhtml | 58 ---- .../content/zotero/collectionViewItemTree.jsx | 3 + chrome/content/zotero/customElements.js | 2 + .../zotero/elements/advancedSearchDeck.js | 83 ++++++ .../zotero/elements/advancedSearchPane.js | 209 ++++++++++++++ .../zotero/elements/quickSearchTextbox.js | 75 ++++- .../content/zotero/elements/zoteroSearch.js | 85 ++---- chrome/content/zotero/libraryTree.js | 6 +- chrome/content/zotero/searchDialog.js | 70 ----- chrome/content/zotero/searchDialog.xhtml | 43 --- .../content/zotero/xpcom/collectionTreeRow.js | 57 +++- chrome/content/zotero/xpcom/data/search.js | 5 +- chrome/content/zotero/zoteroPane.js | 256 +++++++++++------- chrome/content/zotero/zoteroPane.xhtml | 7 +- chrome/locale/en-US/zotero/searchbox.dtd | 6 - chrome/locale/en-US/zotero/zotero.dtd | 2 - chrome/locale/en-US/zotero/zotero.ftl | 22 +- .../default/zotero/20/universal/maximize.svg | 3 + .../default/zotero/20/universal/minimize.svg | 3 + scss/_zotero.scss | 2 +- scss/components/_advancedSearch.scss | 26 -- scss/components/_menu.scss | 1 - scss/components/_toolbar.scss | 4 +- scss/elements/_advancedSearchPane.scss | 54 ++++ scss/elements/_quickSearchTextbox.scss | 106 +++++++- scss/elements/_zoteroSearch.scss | 34 ++- test/tests/advancedSearchTest.js | 195 ++++++++++--- test/tests/collectionViewItemTreeTest.js | 41 --- test/tests/searchTest.js | 3 + test/tests/zoteroPaneTest.js | 79 +++--- 31 files changed, 1016 insertions(+), 718 deletions(-) delete mode 100644 chrome/content/zotero/advancedSearch.js delete mode 100644 chrome/content/zotero/advancedSearch.xhtml create mode 100644 chrome/content/zotero/elements/advancedSearchDeck.js create mode 100644 chrome/content/zotero/elements/advancedSearchPane.js delete mode 100644 chrome/content/zotero/searchDialog.js delete mode 100644 chrome/content/zotero/searchDialog.xhtml create mode 100644 chrome/skin/default/zotero/20/universal/maximize.svg create mode 100644 chrome/skin/default/zotero/20/universal/minimize.svg delete mode 100644 scss/components/_advancedSearch.scss create mode 100644 scss/elements/_advancedSearchPane.scss diff --git a/chrome/content/zotero/advancedSearch.js b/chrome/content/zotero/advancedSearch.js deleted file mode 100644 index b28c441147..0000000000 --- a/chrome/content/zotero/advancedSearch.js +++ /dev/null @@ -1,194 +0,0 @@ -/* - ***** BEGIN LICENSE BLOCK ***** - - Copyright © 2009 Center for History and New Media - George Mason University, Fairfax, Virginia, USA - http://zotero.org - - This file is part of Zotero. - - Zotero is free software: you can redistribute it and/or modify - it under the terms of the GNU Affero General Public License as published by - the Free Software Foundation, either version 3 of the License, or - (at your option) any later version. - - Zotero is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - GNU Affero General Public License for more details. - - You should have received a copy of the GNU Affero General Public License - along with Zotero. If not, see . - - ***** END LICENSE BLOCK ***** -*/ - - -import CollectionViewItemTree from 'zotero/collectionViewItemTree'; -import { COLUMNS } from 'zotero/itemTreeColumns'; - - -var ZoteroAdvancedSearch = new function () { - this.onLoad = onLoad; - this.search = search; - this.clear = clear; - this.onItemActivate = onItemActivate; - - this.itemsView = false; - this._loadedDeferred = Zotero.Promise.defer(); - - var _searchBox; - var _libraryID; - var _searchCounter = 0; - - async function onLoad() { - _searchBox = document.getElementById('zotero-search-box'); - - // Set font size from pref - var sbc = document.getElementById('zotero-search-box-container'); - Zotero.UIProperties.registerRoot(sbc); - - _searchBox.onLibraryChange = this.onLibraryChange; - var io = window.arguments[0]; - - io.dataIn.search.loadPrimaryData() - .then(function () { - _searchBox.search = io.dataIn.search; - }); - - var elem = document.getElementById('zotero-items-tree'); - const columns = COLUMNS.map((column) => { - column = Object.assign({}, column); - column.hidden = !['title', 'firstCreator', 'year', 'hasAttachment'].includes(column.dataKey); - return column; - }); - this.itemsView = await CollectionViewItemTree.init(elem, { - id: "advanced-search", - dragAndDrop: true, - columnPicker: true, - onActivate: this.onItemActivate.bind(this), - columns, - }); - - await this.itemsView.changeCollectionTreeRow({ - id: 'advanced-search-' + _searchCounter++, - ref: _searchBox.search, - visibilityGroup: 'default', - isSearchMode: () => true, - getItems: async () => [], - isLibrary: () => false, - isCollection: () => false, - isSearch: () => true, - isPublications: () => false, - isDuplicates: () => false, - isFeed: () => false, - isFeeds: () => false, - isFeedsOrFeed: () => false, - isRecentlyRead: () => false, - isSortable: () => true, - isShare: () => false, - isTrash: () => false, - isSearch: () => true - }); - - // Focus the first field in the window - Services.focus.moveFocus(window, null, Services.focus.MOVEFOCUS_FORWARD, 0); - this._loadedDeferred.resolve(); - } - - this.onUnload = function () { - this.itemsView.unregister(); - } - - function search() { - _searchBox.updateSearch(); - _searchBox.active = true; - - return this.itemsView.changeCollectionTreeRow({ - id: 'advanced-search-' + _searchCounter++, - ref: _searchBox.search, - visibilityGroup: 'default', - isSearchMode: () => true, - isSearch: () => true, - getItems: async function () { - await Zotero.Libraries.get(_libraryID).waitForDataLoad('item'); - - var search = _searchBox.search.clone(); - search.libraryID = _libraryID; - var ids = await search.search(); - return Zotero.Items.get(ids); - } - }); - } - - - function clear() { - this.itemsView.changeCollectionTreeRow(null); - - var s = new Zotero.Search(); - // Don't clear the selected library - s.libraryID = _searchBox.search.libraryID; - s.addCondition('title', 'contains', ''); - _searchBox.search = s; - _searchBox.active = false; - } - - - this.save = async function () { - _searchBox.updateSearch(); - - var promptService = Services.prompt; - - var libraryID = _searchBox.search.libraryID; - - var searches = await Zotero.Searches.getAll(libraryID); - var prefix = Zotero.getString('pane.collections.untitled'); - var name = Zotero.Utilities.Internal.getNextName( - prefix, - searches.map(s => s.name).filter(n => n.startsWith(prefix)) - ); - - name = { value: name }; - var result = promptService.prompt(window, - Zotero.getString('pane.collections.newSavedSeach'), - Zotero.getString('pane.collections.savedSearchName'), name, "", {}); - - if (!result) { - return; - } - - if (!name.value) { - name.value = 'untitled'; - } - - var s = _searchBox.search.clone(); - s.name = name.value; - await s.saveTx(); - - window.close(); - }; - - - this.onLibraryChange = function (libraryID) { - _libraryID = libraryID; - var library = Zotero.Libraries.get(libraryID); - var isEditable = library.editable && library.libraryType != 'publications'; - document.getElementById('zotero-search-save').disabled = !isEditable; - } - - - function onItemActivate(event, items) - { - var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"] - .getService(Components.interfaces.nsIWindowMediator); - - var lastWin = wm.getMostRecentWindow("navigator:browser"); - - if (!lastWin) { - return; - } - - lastWin.ZoteroPane.selectItems(items.map(item => item.id)); - lastWin.focus(); - } -} diff --git a/chrome/content/zotero/advancedSearch.xhtml b/chrome/content/zotero/advancedSearch.xhtml deleted file mode 100644 index ac90733b78..0000000000 --- a/chrome/content/zotero/advancedSearch.xhtml +++ /dev/null @@ -1,58 +0,0 @@ - - - - - - - - - %zoteroDTD; - - %searchboxDTD; -]> - - - - - - - - - - - - - - - - - diff --git a/chrome/content/zotero/xpcom/collectionTreeRow.js b/chrome/content/zotero/xpcom/collectionTreeRow.js index 38ded96839..86d3cb656f 100644 --- a/chrome/content/zotero/xpcom/collectionTreeRow.js +++ b/chrome/content/zotero/xpcom/collectionTreeRow.js @@ -464,7 +464,8 @@ Zotero.CollectionTreeRow.prototype.getSearchObject = async function () { } s2.setScope(s, includeScopeChildren); - if (this.searchText) { + // Add Quick Search unless advanced search is enabled + if (this.searchText && !this.advancedSearch) { let cond = 'quicksearch-' + (this.searchMode || Zotero.Prefs.get('search.quicksearch-mode')); s2.addCondition(cond, 'contains', this.searchText); @@ -476,8 +477,37 @@ Zotero.CollectionTreeRow.prototype.getSearchObject = async function () { } } - this._cachedSearch = s2; - return s2; + let s3; + if (this.advancedSearch) { + if (this.advancedSearch.libraryID === null) { + // A library-less search (Feeds pseudo-library) can't be clone()d + s3 = new Zotero.Search(); + s3.fromJSON(this.advancedSearch.toJSON()); + } + else { + s3 = this.advancedSearch.clone(); + } + // Show matches in the trash too. includeDeleted has to be set on the + // scope searches as well, since a search's scope defines the superset + // of possible results. Special condition - unaffected by joinMode. + if (!this.isTrash()) { + if (s == this.ref) { + // Don't modify a saved search object + s = s.clone(this.ref.libraryID); + s2.setScope(s, includeScopeChildren); + } + s.addCondition('includeDeleted', 'true'); + s2.addCondition('includeDeleted', 'true'); + } + s3.addCondition('includeDeleted', 'true'); + s3.setScope(s2, includeScopeChildren); + } + else { + s3 = s2; + } + + this._cachedSearch = s3; + return s3; }; Zotero.CollectionTreeRow.prototype.getChildTags = function () { @@ -537,6 +567,23 @@ Zotero.CollectionTreeRow.prototype.setSearch = function (searchText, mode = null return true; } +Zotero.CollectionTreeRow.prototype.setAdvancedSearch = function (advancedSearch) { + this.clearCache(); + if (!advancedSearch) { + this.advancedSearch = undefined; + } + else if (this.ref.libraryID === undefined) { + // Feeds pseudo-library -- leave the library unset so that the search + // spans all feed libraries + this.advancedSearch = new Zotero.Search(); + this.advancedSearch.fromJSON(advancedSearch.toJSON()); + } + else { + this.advancedSearch = advancedSearch.clone(this.ref.libraryID); + } + return true; +}; + Zotero.CollectionTreeRow.prototype.setTags = function (tags) { let oldTags = this.tags instanceof Set ? this.tags : new Set(this.tags || []); let newTags = tags instanceof Set ? new Set(tags) : new Set(tags || []); @@ -570,8 +617,8 @@ Zotero.CollectionTreeRow.prototype.isSearchMode = function () { return true; } - // Quicksearch - if (this.searchText != '') { + // Search filters + if (this.advancedSearch || this.searchText != '') { return true; } diff --git a/chrome/content/zotero/xpcom/data/search.js b/chrome/content/zotero/xpcom/data/search.js index 8110841918..edde327cd7 100644 --- a/chrome/content/zotero/xpcom/data/search.js +++ b/chrome/content/zotero/xpcom/data/search.js @@ -597,8 +597,9 @@ Zotero.Search.prototype.search = async function (asTempTable) { // Run a subsearch to define the superset of possible results if (this._scope) { - // If subsearch has post-search filter, run and insert ids into temp table - if (this._scope.hasPostSearchFilter()) { + // If subsearch has post-search filter or a recursive scope, + // run and insert ids into temp table + if (this._scope.hasPostSearchFilter() || this._scope._scope) { var ids = await this._scope.search(); if (!ids) { return []; diff --git a/chrome/content/zotero/zoteroPane.js b/chrome/content/zotero/zoteroPane.js index 943686cb0a..37d7f866fd 100644 --- a/chrome/content/zotero/zoteroPane.js +++ b/chrome/content/zotero/zoteroPane.js @@ -364,7 +364,7 @@ var ZoteroPane = new function () { 'zotero-tb-add': { ArrowNext: () => document.getElementById("zotero-tb-lookup"), ArrowPrevious: () => null, - Tab: () => document.getElementById("zotero-tb-search")._searchModePopup.flattenedTreeParentNode.focus(), + Tab: () => document.getElementById("zotero-tb-search").focus(), ShiftTab: () => { if (collectionsPane.getAttribute("collapsed")) { return document.getElementById('zotero-tb-sync'); @@ -378,7 +378,7 @@ var ZoteroPane = new function () { 'zotero-tb-lookup': { ArrowNext: () => document.getElementById("zotero-tb-attachment-add"), ArrowPrevious: () => document.getElementById("zotero-tb-add"), - Tab: () => document.getElementById("zotero-tb-search")._searchModePopup.flattenedTreeParentNode.focus(), + Tab: () => document.getElementById("zotero-tb-search").focus(), ShiftTab: () => document.getElementById('zotero-tb-collections-search').click(), Enter: () => Zotero_Lookup.showPanel(event.target), ' ': () => Zotero_Lookup.showPanel(event.target) @@ -386,13 +386,13 @@ var ZoteroPane = new function () { 'zotero-tb-attachment-add': { ArrowNext: () => document.getElementById("zotero-tb-note-add"), ArrowPrevious: () => document.getElementById("zotero-tb-lookup"), - Tab: () => document.getElementById("zotero-tb-search")._searchModePopup.flattenedTreeParentNode.focus(), + Tab: () => document.getElementById("zotero-tb-search").focus(), ShiftTab: () => document.getElementById('zotero-tb-collections-search').click() }, 'zotero-tb-note-add': { ArrowNext: () => null, ArrowPrevious: () => document.getElementById("zotero-tb-attachment-add"), - Tab: () => document.getElementById("zotero-tb-search")._searchModePopup.flattenedTreeParentNode.focus(), + Tab: () => document.getElementById("zotero-tb-search").focus(), ShiftTab: () => document.getElementById('zotero-tb-collections-search').click() }, 'zotero-tb-search-dropmarker': { @@ -403,9 +403,7 @@ var ZoteroPane = new function () { }, 'zotero-tb-search-textbox': { Tab: () => document.getElementById("zotero-tb-toggle-item-pane-stacked"), - ShiftTab: () => { - document.getElementById("zotero-tb-search")._searchModePopup.flattenedTreeParentNode.focus(); - } + ShiftTab: () => document.getElementById("zotero-tb-search").focus() }, 'zotero-tb-toggle-item-pane-stacked': { Tab: () => itemTree.querySelector(".virtualized-table"), @@ -1554,59 +1552,9 @@ var ZoteroPane = new function () { this.loadURI(Zotero.Groups.addGroupURL); } - - this.newSearch = async function () { - if (Zotero.DB.inTransaction()) { - await Zotero.DB.waitForTransaction(); - } - - var libraryID = this.getSelectedLibraryID(); - - var s = new Zotero.Search(); - s.libraryID = libraryID; - s.addCondition('title', 'contains', ''); - - var searches = await Zotero.Searches.getAll(libraryID) - var prefix = Zotero.getString('pane.collections.untitled'); - var name = Zotero.Utilities.Internal.getNextName( - prefix, - searches.map(s => s.name).filter(n => n.startsWith(prefix)) - ); - - var io = { dataIn: { search: s, name }, dataOut: null }; - window.openDialog('chrome://zotero/content/searchDialog.xhtml','','chrome,modal,centerscreen',io); - if (!io.dataOut) { - return false; - } - s.fromJSON(io.dataOut.json); - await s.saveTx(); - return s.id; - }; - this.setVirtual = function (libraryID, type, show, select) { return this.collectionsView.toggleVirtualCollection(libraryID, type, show, select); }; - - this.openAdvancedSearchWindow = function () { - var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"] - .getService(Components.interfaces.nsIWindowMediator); - var enumerator = wm.getEnumerator('zotero:search'); - while (enumerator.hasMoreElements()) { - var win = enumerator.getNext(); - } - - if (win) { - win.focus(); - return; - } - - var s = new Zotero.Search(); - s.libraryID = this.getSelectedLibraryID(); - s.addCondition('title', 'contains', ''); - - var io = {dataIn: {search: s}, dataOut: null}; - window.openDialog('chrome://zotero/content/advancedSearch.xhtml', '', 'chrome,dialog=no,centerscreen', io); - }; this.initItemsTree = async function () { try { @@ -1688,7 +1636,7 @@ var ZoteroPane = new function () { this.tagSelector.handleResize(); } if (this.collectionsView) { - this.collectionsView.updateHeight(); + this.collectionsView.updateHeightDebounced(); } }, 100); @@ -1756,7 +1704,6 @@ var ZoteroPane = new function () { } }; - this.onCollectionSelected = Zotero.serial(async function () { var collectionTreeRow = this.getCollectionTreeRow(); if (!collectionTreeRow) { @@ -1772,13 +1719,43 @@ var ZoteroPane = new function () { return; } + let advancedSearchDeck = document.getElementById('zotero-advanced-search-pane-deck'); + if (this.itemsView.collectionTreeRow?.isSearch() + && advancedSearchDeck.state === 'open' + && advancedSearchDeck.selectedSearchType === 'saved') { + let result = Services.prompt.confirmEx(window, + Zotero.getString('saved-search-close-confirmation-title'), + Zotero.getString('saved-search-close-confirmation-body'), + Ci.nsIPromptService.BUTTON_POS_0_DEFAULT + | Ci.nsIPrompt.BUTTON_TITLE_SAVE * Ci.nsIPrompt.BUTTON_POS_0 + | Ci.nsIPrompt.BUTTON_TITLE_CANCEL * Ci.nsIPrompt.BUTTON_POS_1 + | Ci.nsIPrompt.BUTTON_TITLE_DONT_SAVE * Ci.nsIPrompt.BUTTON_POS_2, + null, null, null, + null, {}); + switch (result) { + case 0: + await advancedSearchDeck.pane.save(); + return; + case 1: + this.collectionsView.selection.selectEventsSuppressed = true; + try { + await this.collectionsView.selectByID(this.itemsView.collectionTreeRow.id); + } + finally { + this.collectionsView.selection.selectEventsSuppressed = false; + } + return; + case 2: + await advancedSearchDeck.pane.cancel(); + break; + } + } + // Rename tab Zotero_Tabs.rename('zotero-pane', collectionTreeRow.getName()); - let type = Zotero.Libraries.get(collectionTreeRow.ref.libraryID).libraryType; - // Clear quick search and tag selector when switching views - document.getElementById('zotero-tb-search-textbox').value = ""; + document.getElementById('zotero-tb-search').onCollectionSelected(); if (ZoteroPane.tagSelector) { ZoteroPane.tagSelector.clearTagSelection(); } @@ -1788,6 +1765,7 @@ var ZoteroPane = new function () { collectionTreeRow.setTags(ZoteroPane.tagSelector.getTagSelection()); } + this._refreshAdvancedSearchPane(collectionTreeRow); this._updateEnabledActionsForRow(collectionTreeRow); // If item data not yet loaded for library, load it now. @@ -1815,6 +1793,125 @@ var ZoteroPane = new function () { Zotero.Prefs.set('lastViewedFolder', collectionTreeRow.id); }); + + + /** + * @param {Zotero.CollectionTreeRow} [collectionTreeRow] - During collection selection, + * the newly selected row, which isn't in the items view yet + */ + this._refreshAdvancedSearchPane = function (collectionTreeRow) { + let deck = document.getElementById('zotero-advanced-search-pane-deck'); + + deck.pane.refresh(); + + let search = deck.state === 'closed' || deck.selectedSearchType !== 'temporary' || !deck.pane.active + ? null + : deck.pane.search; + if (collectionTreeRow) { + collectionTreeRow.setAdvancedSearch(search); + return undefined; + } + // Apply via the items view, whose row can be a different object from the + // collection tree's current row + return this.itemsView.setFilter('advanced-search', search); + }; + + + /** + * @param {'open' | 'collapsed' | 'closed'} state + */ + this.setAdvancedSearchState = async function (state) { + let deck = document.getElementById('zotero-advanced-search-pane-deck'); + let oldState = deck.state; + deck.selectedSearchType = 'temporary'; + deck.state = state; + + let advancedSearchPane = deck.pane; + + document.getElementById('zotero-tb-search').updateMode(); + let refreshPromise; + if (state === 'open' && oldState === 'collapsed' + || state === 'collapsed' && oldState === 'open') { + // State change only causes visual refresh - update the tree height + this.itemsView.updateHeight(); + } + else { + // State change changes displayed items - refresh the tree + refreshPromise = this._refreshAdvancedSearchPane(); + } + + // Update the pane state synchronously, so that a state change initiated + // while the refresh below is pending doesn't see a stale search + if (state === 'closed') { + advancedSearchPane.search = null; + } + else if (state === 'open') { + Zotero_Tabs.select('zotero-pane'); + advancedSearchPane.focus(); + } + + await refreshPromise; + }; + + + /** + * @param {'open' | 'collapsed' | 'closed'} state + */ + this.toggleAdvancedSearchState = async function (state) { + let deck = document.getElementById('zotero-advanced-search-pane-deck'); + if (state === deck.state && deck.selectedSearchType !== 'saved') { + // If we're trying to open the pane, and it's already open but not focused, + // focus it + if (state === 'open' && !deck.pane.matches(':focus-within')) { + deck.pane.focus(); + return; + } + + // Flip the state + switch (state) { + case 'open': + state = 'closed'; + break; + case 'collapsed': + case 'closed': + state = 'open'; + break; + } + } + await this.setAdvancedSearchState(state); + }; + + + /** + * @param {'open' | 'closed'} state + */ + this.setSavedSearchEditorState = async function (state) { + let collectionTreeRow = this.getCollectionTreeRow(); + if (state === 'open' && !collectionTreeRow.isSearch()) { + throw new Error('Cannot show saved search editor outside search row'); + } + + let deck = document.getElementById('zotero-advanced-search-pane-deck'); + deck.selectedSearchType = 'saved'; + deck.state = state; + + if (state === 'open') { + deck.pane.search = collectionTreeRow.ref; + } + + document.getElementById('zotero-tb-search').updateMode(); + let refreshPromise = this._refreshAdvancedSearchPane(); + if (state === 'open') { + deck.pane.focus(); + } + await refreshPromise; + }; + + + this.openAdvancedSearchWindow = function () { + Zotero.debug(`ZoteroPane.openAdvancedSearchWindow() is deprecated -- use ZoteroPane.toggleAdvancedSearchState() instead`); + this.toggleAdvancedSearchState('open'); + }; /** @@ -1828,7 +1925,6 @@ var ZoteroPane = new function () { "menu_noteAdd", "cmd_zotero_newCollection", - "cmd_zotero_newSavedSearch", "cmd_zotero_import", "cmd_zotero_importFromClipboard", @@ -2580,28 +2676,7 @@ var ZoteroPane = new function () { this.collectionsView.startEditing(row); } else { - let s = row.ref.clone(); - let groups = []; - // Promises don't work in the modal dialog, so get the group name here, if - // applicable, and pass it in. We only need the group that this search belongs - // to, if any, since the library drop-down is disabled for saved searches. - if (Zotero.Libraries.get(s.libraryID).libraryType == 'group') { - groups.push(Zotero.Groups.getByLibraryID(s.libraryID)); - } - var io = { - dataIn: { - search: s, - name: row.getName(), - groups: groups - }, - dataOut: null - }; - window.openDialog('chrome://zotero/content/searchDialog.xhtml','','chrome,modal,centerscreen',io); - if (io.dataOut) { - row.ref.fromJSON(io.dataOut.json); - await row.ref.saveTx(); - Zotero_Tabs.rename("zotero-pane", row.ref.name); - } + this.setSavedSearchEditorState('open'); } } }; @@ -3274,10 +3349,6 @@ var ZoteroPane = new function () { id: "newCollection", command: "cmd_zotero_newCollection" }, - { - id: "newSavedSearch", - command: "cmd_zotero_newSavedSearch" - }, { id: "newSubcollection", oncommand: () => { @@ -3564,8 +3635,7 @@ var ZoteroPane = new function () { show.push( 'sync', 'sep1', - 'newCollection', - 'newSavedSearch' + 'newCollection' ); } // Only show "Show Duplicates", "Show Unfiled Items", and "Show Retracted" if rows are hidden @@ -6784,7 +6854,7 @@ var ZoteroPane = new function () { if (ZoteroPane.itemsView) { // Need to immediately rerender the items here without any debouncing // since tree height will have changed - ZoteroPane.itemsView._updateHeight(); + ZoteroPane.itemsView.updateHeight(); } ZoteroContextPane.update(); Zotero_Tabs.updateSidebarLayout(); @@ -6970,7 +7040,7 @@ var ZoteroPane = new function () { var collectionsPaneWidth = collectionsPane.getBoundingClientRect().width; tagSelector.style.maxWidth = collectionsPaneWidth + 'px'; if (ZoteroPane.itemsView) { - ZoteroPane.itemsView.updateHeight(); + ZoteroPane.itemsView.updateHeightDebounced(); } this.handleTagSelectorResize(); diff --git a/chrome/content/zotero/zoteroPane.xhtml b/chrome/content/zotero/zoteroPane.xhtml index a15a042f6e..9f4d1cf0c2 100644 --- a/chrome/content/zotero/zoteroPane.xhtml +++ b/chrome/content/zotero/zoteroPane.xhtml @@ -102,7 +102,7 @@ - + @@ -116,7 +116,6 @@ - @@ -428,7 +427,7 @@ key="key_find" accesskey="&findCmd.accesskey;" oncommand="ZoteroStandalone.currentReader.toggleFindPopup({ open: true });"/>