From 2d6ebc58017a0b70ba5829c49dc25284bfee7216 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sun, 24 Jul 2022 13:14:46 +0200 Subject: [PATCH 1/5] Convert the `OptionalContentConfig` to use *properly* private fields/methods To ensure that this data cannot be directly changed from the outside, use private fields/methods now that those are available. --- src/display/optional_content_config.js | 65 ++++++++++++++------------ 1 file changed, 34 insertions(+), 31 deletions(-) diff --git a/src/display/optional_content_config.js b/src/display/optional_content_config.js index 4f70893a4..a759d28af 100644 --- a/src/display/optional_content_config.js +++ b/src/display/optional_content_config.js @@ -12,6 +12,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + import { objectFromMap, warn } from "../shared/util.js"; class OptionalContentGroup { @@ -23,41 +24,43 @@ class OptionalContentGroup { } class OptionalContentConfig { + #groups = new Map(); + + #order = null; + constructor(data) { this.name = null; this.creator = null; - this._order = null; - this._groups = new Map(); if (data === null) { return; } this.name = data.name; this.creator = data.creator; - this._order = data.order; + this.#order = data.order; for (const group of data.groups) { - this._groups.set( + this.#groups.set( group.id, new OptionalContentGroup(group.name, group.intent) ); } if (data.baseState === "OFF") { - for (const group of this._groups) { + for (const group of this.#groups) { group.visible = false; } } for (const on of data.on) { - this._groups.get(on).visible = true; + this.#groups.get(on).visible = true; } for (const off of data.off) { - this._groups.get(off).visible = false; + this.#groups.get(off).visible = false; } } - _evaluateVisibilityExpression(array) { + #evaluateVisibilityExpression(array) { const length = array.length; if (length < 2) { return true; @@ -67,9 +70,9 @@ class OptionalContentConfig { const element = array[i]; let state; if (Array.isArray(element)) { - state = this._evaluateVisibilityExpression(element); - } else if (this._groups.has(element)) { - state = this._groups.get(element).visible; + state = this.#evaluateVisibilityExpression(element); + } else if (this.#groups.has(element)) { + state = this.#groups.get(element).visible; } else { warn(`Optional content group not found: ${element}`); return true; @@ -95,7 +98,7 @@ class OptionalContentConfig { } isVisible(group) { - if (this._groups.size === 0) { + if (this.#groups.size === 0) { return true; } if (!group) { @@ -103,57 +106,57 @@ class OptionalContentConfig { return true; } if (group.type === "OCG") { - if (!this._groups.has(group.id)) { + if (!this.#groups.has(group.id)) { warn(`Optional content group not found: ${group.id}`); return true; } - return this._groups.get(group.id).visible; + return this.#groups.get(group.id).visible; } else if (group.type === "OCMD") { // Per the spec, the expression should be preferred if available. if (group.expression) { - return this._evaluateVisibilityExpression(group.expression); + return this.#evaluateVisibilityExpression(group.expression); } if (!group.policy || group.policy === "AnyOn") { // Default for (const id of group.ids) { - if (!this._groups.has(id)) { + if (!this.#groups.has(id)) { warn(`Optional content group not found: ${id}`); return true; } - if (this._groups.get(id).visible) { + if (this.#groups.get(id).visible) { return true; } } return false; } else if (group.policy === "AllOn") { for (const id of group.ids) { - if (!this._groups.has(id)) { + if (!this.#groups.has(id)) { warn(`Optional content group not found: ${id}`); return true; } - if (!this._groups.get(id).visible) { + if (!this.#groups.get(id).visible) { return false; } } return true; } else if (group.policy === "AnyOff") { for (const id of group.ids) { - if (!this._groups.has(id)) { + if (!this.#groups.has(id)) { warn(`Optional content group not found: ${id}`); return true; } - if (!this._groups.get(id).visible) { + if (!this.#groups.get(id).visible) { return true; } } return false; } else if (group.policy === "AllOff") { for (const id of group.ids) { - if (!this._groups.has(id)) { + if (!this.#groups.has(id)) { warn(`Optional content group not found: ${id}`); return true; } - if (this._groups.get(id).visible) { + if (this.#groups.get(id).visible) { return false; } } @@ -167,29 +170,29 @@ class OptionalContentConfig { } setVisibility(id, visible = true) { - if (!this._groups.has(id)) { + if (!this.#groups.has(id)) { warn(`Optional content group not found: ${id}`); return; } - this._groups.get(id).visible = !!visible; + this.#groups.get(id).visible = !!visible; } getOrder() { - if (!this._groups.size) { + if (!this.#groups.size) { return null; } - if (this._order) { - return this._order.slice(); + if (this.#order) { + return this.#order.slice(); } - return Array.from(this._groups.keys()); + return [...this.#groups.keys()]; } getGroups() { - return this._groups.size > 0 ? objectFromMap(this._groups) : null; + return this.#groups.size > 0 ? objectFromMap(this.#groups) : null; } getGroup(id) { - return this._groups.get(id) || null; + return this.#groups.get(id) || null; } } From f3d76b42b3553afae0eaa3887f73ffb35e42c245 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sun, 24 Jul 2022 13:14:51 +0200 Subject: [PATCH 2/5] Ensure that `OptionalContentGroup.visible` cannot be modified from the "outside" Given that Optional Content visibility is only intended/supported to be updated via the `OptionalContentConfig.setVisibility`-method, this patch actually enforces that now. Note that this will be used by the next patch in the series, and will help prevent inconsistent state in the `OptionalContentConfig`-class. *Please note:* This patch also uncovered a pre-existing bug, related to iterating through the visibility groups in the constructor, for the `baseState === "OFF"` case. --- src/display/optional_content_config.js | 34 ++++++++++++++++++++------ 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/src/display/optional_content_config.js b/src/display/optional_content_config.js index a759d28af..370ffecf1 100644 --- a/src/display/optional_content_config.js +++ b/src/display/optional_content_config.js @@ -13,14 +13,34 @@ * limitations under the License. */ -import { objectFromMap, warn } from "../shared/util.js"; +import { objectFromMap, unreachable, warn } from "../shared/util.js"; + +const INTERNAL = Symbol("INTERNAL"); class OptionalContentGroup { + #visible = true; + constructor(name, intent) { - this.visible = true; this.name = name; this.intent = intent; } + + /** + * @type {boolean} + */ + get visible() { + return this.#visible; + } + + /** + * @ignore + */ + _setVisible(internal, visible) { + if (internal !== INTERNAL) { + unreachable("Internal method `_setVisible` called."); + } + this.#visible = visible; + } } class OptionalContentConfig { @@ -46,17 +66,17 @@ class OptionalContentConfig { } if (data.baseState === "OFF") { - for (const group of this.#groups) { - group.visible = false; + for (const group of this.#groups.values()) { + group._setVisible(INTERNAL, false); } } for (const on of data.on) { - this.#groups.get(on).visible = true; + this.#groups.get(on)._setVisible(INTERNAL, true); } for (const off of data.off) { - this.#groups.get(off).visible = false; + this.#groups.get(off)._setVisible(INTERNAL, false); } } @@ -174,7 +194,7 @@ class OptionalContentConfig { warn(`Optional content group not found: ${id}`); return; } - this.#groups.get(id).visible = !!visible; + this.#groups.get(id)._setVisible(INTERNAL, !!visible); } getOrder() { From ceb4f8a6ab9e794b5e3757cf33991971d28c0509 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sun, 24 Jul 2022 13:14:54 +0200 Subject: [PATCH 3/5] [api-minor] Add a new method, in `OptionalContentConfig`, to detect the initial Optional Content visibility state This will allow us to improve the `PDFThumbnailView.setImage` handling in the viewer, and thanks to the added caching this should be reasonbly efficient. --- src/display/optional_content_config.js | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/display/optional_content_config.js b/src/display/optional_content_config.js index 370ffecf1..baa4aeb5c 100644 --- a/src/display/optional_content_config.js +++ b/src/display/optional_content_config.js @@ -44,8 +44,12 @@ class OptionalContentGroup { } class OptionalContentConfig { + #cachedHasInitialVisibility = true; + #groups = new Map(); + #initialVisibility = null; + #order = null; constructor(data) { @@ -78,6 +82,12 @@ class OptionalContentConfig { for (const off of data.off) { this.#groups.get(off)._setVisible(INTERNAL, false); } + + // The following code must always run *last* in the constructor. + this.#initialVisibility = new Map(); + for (const [id, group] of this.#groups) { + this.#initialVisibility.set(id, group.visible); + } } #evaluateVisibilityExpression(array) { @@ -195,6 +205,21 @@ class OptionalContentConfig { return; } this.#groups.get(id)._setVisible(INTERNAL, !!visible); + + this.#cachedHasInitialVisibility = null; + } + + get hasInitialVisibility() { + if (this.#cachedHasInitialVisibility !== null) { + return this.#cachedHasInitialVisibility; + } + for (const [id, group] of this.#groups) { + const visible = this.#initialVisibility.get(id); + if (group.visible !== visible) { + return (this.#cachedHasInitialVisibility = false); + } + } + return (this.#cachedHasInitialVisibility = true); } getOrder() { From 3446f15bf374e761a42695f30e5cc9e93a00dcc1 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sun, 24 Jul 2022 13:14:58 +0200 Subject: [PATCH 4/5] Improve how we disable `PDFThumbnailView.setImage` for documents with Optional Content (PR 12170 follow-up) Rather than always disable `PDFThumbnailView.setImage` as soon as user has changed the visibility of the Optional Content, we can utilize the new method added in the previous patch to improve thumbnail performance. Note in particular how, in the old code, even *resetting* of the Optional Content to its default state wouldn't enable `PDFThumbnailView.setImage` again. While slightly unrelated, this patch also removes the `PDFThumbnailViewer._optionalContentConfigPromise`-property since it's completely unused. --- web/base_viewer.js | 1 + web/pdf_page_view.js | 36 ++++++++++++++++++++++++++++++++++++ web/pdf_thumbnail_view.js | 12 +----------- web/pdf_thumbnail_viewer.js | 14 -------------- 4 files changed, 38 insertions(+), 25 deletions(-) diff --git a/web/base_viewer.js b/web/base_viewer.js index 0ecb510ab..2566dba31 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -1824,6 +1824,7 @@ class BaseViewer { return Promise.resolve(null); } if (!this._optionalContentConfigPromise) { + console.error("optionalContentConfigPromise: Not initialized yet."); // Prevent issues if the getter is accessed *before* the `onePageRendered` // promise has resolved; won't (normally) happen in the default viewer. return this.pdfDocument.getOptionalContentConfig(); diff --git a/web/pdf_page_view.js b/web/pdf_page_view.js index bc77234fc..2beca556b 100644 --- a/web/pdf_page_view.js +++ b/web/pdf_page_view.js @@ -99,6 +99,8 @@ const MAX_CANVAS_PIXELS = compatibilityParams.maxCanvasPixels || 16777216; class PDFPageView { #annotationMode = AnnotationMode.ENABLE_FORMS; + #useThumbnailCanvas = true; + /** * @param {PDFPageViewOptions} options */ @@ -174,6 +176,22 @@ class PDFPageView { this.div = div; container?.append(div); + + if (this._isStandalone) { + const { optionalContentConfigPromise } = options; + if (optionalContentConfigPromise) { + // Ensure that the thumbnails always display the *initial* document + // state. + optionalContentConfigPromise.then(optionalContentConfig => { + if ( + optionalContentConfigPromise !== this._optionalContentConfigPromise + ) { + return; + } + this.#useThumbnailCanvas = optionalContentConfig.hasInitialVisibility; + }); + } + } } setPdfPage(pdfPage) { @@ -376,6 +394,16 @@ class PDFPageView { } if (optionalContentConfigPromise instanceof Promise) { this._optionalContentConfigPromise = optionalContentConfigPromise; + + // Ensure that the thumbnails always display the *initial* document state. + optionalContentConfigPromise.then(optionalContentConfig => { + if ( + optionalContentConfigPromise !== this._optionalContentConfigPromise + ) { + return; + } + this.#useThumbnailCanvas = optionalContentConfig.hasInitialVisibility; + }); } const totalRotation = (this.rotation + this.pdfPageRotate) % 360; @@ -1003,6 +1031,14 @@ class PDFPageView { this.div.removeAttribute("data-page-label"); } } + + /** + * For use by the `PDFThumbnailView.setImage`-method. + * @ignore + */ + get thumbnailCanvas() { + return this.#useThumbnailCanvas ? this.canvas : null; + } } export { PDFPageView }; diff --git a/web/pdf_thumbnail_view.js b/web/pdf_thumbnail_view.js index e45c61f7d..2cf3f6c5c 100644 --- a/web/pdf_thumbnail_view.js +++ b/web/pdf_thumbnail_view.js @@ -37,7 +37,6 @@ const THUMBNAIL_WIDTH = 98; // px * The default value is `null`. * @property {IPDFLinkService} linkService - The navigation/linking service. * @property {PDFRenderingQueue} renderingQueue - The rendering queue object. - * @property {function} checkSetImageDisabled * @property {IL10n} l10n - Localization service. * @property {Object} [pageColors] - Overwrites background and foreground colors * with user defined ones in order to improve readability in high contrast @@ -88,7 +87,6 @@ class PDFThumbnailView { optionalContentConfigPromise, linkService, renderingQueue, - checkSetImageDisabled, l10n, pageColors, }) { @@ -109,11 +107,6 @@ class PDFThumbnailView { this.renderTask = null; this.renderingState = RenderingStates.INITIAL; this.resume = null; - this._checkSetImageDisabled = - checkSetImageDisabled || - function () { - return false; - }; const pageWidth = this.viewport.width, pageHeight = this.viewport.height, @@ -356,13 +349,10 @@ class PDFThumbnailView { } setImage(pageView) { - if (this._checkSetImageDisabled()) { - return; - } if (this.renderingState !== RenderingStates.INITIAL) { return; } - const { canvas, pdfPage } = pageView; + const { thumbnailCanvas: canvas, pdfPage } = pageView; if (!canvas) { return; } diff --git a/web/pdf_thumbnail_viewer.js b/web/pdf_thumbnail_viewer.js index 582448253..d54cb043c 100644 --- a/web/pdf_thumbnail_viewer.js +++ b/web/pdf_thumbnail_viewer.js @@ -85,12 +85,6 @@ class PDFThumbnailViewer { this.scroll = watchScroll(this.container, this._scrollUpdated.bind(this)); this._resetView(); - - eventBus._on("optionalcontentconfigchanged", () => { - // Ensure that the thumbnails always render with the *default* optional - // content configuration. - this._setImageDisabled = true; - }); } /** @@ -195,8 +189,6 @@ class PDFThumbnailViewer { this._currentPageNumber = 1; this._pageLabels = null; this._pagesRotation = 0; - this._optionalContentConfigPromise = null; - this._setImageDisabled = false; // Remove the thumbnails from the DOM. this.container.textContent = ""; @@ -220,13 +212,8 @@ class PDFThumbnailViewer { firstPagePromise .then(firstPdfPage => { - this._optionalContentConfigPromise = optionalContentConfigPromise; - const pagesCount = pdfDocument.numPages; const viewport = firstPdfPage.getViewport({ scale: 1 }); - const checkSetImageDisabled = () => { - return this._setImageDisabled; - }; for (let pageNum = 1; pageNum <= pagesCount; ++pageNum) { const thumbnail = new PDFThumbnailView({ @@ -236,7 +223,6 @@ class PDFThumbnailViewer { optionalContentConfigPromise, linkService: this.linkService, renderingQueue: this.renderingQueue, - checkSetImageDisabled, l10n: this.l10n, pageColors: this.pageColors, }); From 37dc0e7d6e152e924419c90c9d09e4802dc2acef Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sun, 24 Jul 2022 15:43:31 +0200 Subject: [PATCH 5/5] Limit the `PDFPageView._isStandalone` handling to the GENERIC viewer This code is completely unnecessary in e.g. the Firefox PDF Viewer. --- web/pdf_page_view.js | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/web/pdf_page_view.js b/web/pdf_page_view.js index 2beca556b..b1f66cb79 100644 --- a/web/pdf_page_view.js +++ b/web/pdf_page_view.js @@ -153,7 +153,12 @@ class PDFPageView { this.renderingState = RenderingStates.INITIAL; this.resume = null; this._renderError = null; - this._isStandalone = !this.renderingQueue?.hasViewer(); + if ( + typeof PDFJSDev === "undefined" || + PDFJSDev.test("!PRODUCTION || GENERIC") + ) { + this._isStandalone = !this.renderingQueue?.hasViewer(); + } this._annotationCanvasMap = null; @@ -177,7 +182,11 @@ class PDFPageView { container?.append(div); - if (this._isStandalone) { + if ( + (typeof PDFJSDev === "undefined" || + PDFJSDev.test("!PRODUCTION || GENERIC")) && + this._isStandalone + ) { const { optionalContentConfigPromise } = options; if (optionalContentConfigPromise) { // Ensure that the thumbnails always display the *initial* document @@ -377,7 +386,11 @@ class PDFPageView { this.loadingIconDiv = document.createElement("div"); this.loadingIconDiv.className = "loadingIcon notVisible"; - if (this._isStandalone) { + if ( + (typeof PDFJSDev === "undefined" || + PDFJSDev.test("!PRODUCTION || GENERIC")) && + this._isStandalone + ) { this.toggleLoadingIconSpinner(/* viewVisible = */ true); } this.loadingIconDiv.setAttribute("role", "img"); @@ -412,7 +425,11 @@ class PDFPageView { rotation: totalRotation, }); - if (this._isStandalone) { + if ( + (typeof PDFJSDev === "undefined" || + PDFJSDev.test("!PRODUCTION || GENERIC")) && + this._isStandalone + ) { docStyle.setProperty("--scale-factor", this.viewport.scale); }