From 4eaa058c168cfbf8548caddb574a4889e8576efd Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 31 Oct 2020 09:54:00 +0100 Subject: [PATCH 1/3] Add early returns to a couple of `PDFLinkService` methods, when there's no active PDF document All of these methods will, in one way or another, cause e.g. scrolling or zooming to occur and consequently they don't really make sense unless there's an active PDF document. Especially since all of these methods end up calling into a `BaseViewer`-instance, which already contains similar early returns in essentially all of it's methods and setters. --- web/pdf_link_service.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/web/pdf_link_service.js b/web/pdf_link_service.js index dd57fa701f1df..9809aa0735f88 100644 --- a/web/pdf_link_service.js +++ b/web/pdf_link_service.js @@ -182,6 +182,9 @@ class PDFLinkService { * @param {string|Array} dest - The named, or explicit, PDF destination. */ async goToDestination(dest) { + if (!this.pdfDocument) { + return; + } let namedDest, explicitDest; if (typeof dest === "string") { namedDest = dest; @@ -206,6 +209,9 @@ class PDFLinkService { * @param {number} pageNumber - The page number. */ goToPage(pageNumber) { + if (!this.pdfDocument) { + return; + } if ( !( Number.isInteger(pageNumber) && @@ -261,6 +267,9 @@ class PDFLinkService { * @param {string} hash */ setHash(hash) { + if (!this.pdfDocument) { + return; + } let pageNumber, dest; if (hash.includes("=")) { const params = parseQueryString(hash); From 911948c5c029726c3bc35a72b27bf4f268cf34f4 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 31 Oct 2020 10:17:28 +0100 Subject: [PATCH 2/3] Also update the browser history when the user *manually* change pages using the pageNumber-input (PR 12493 follow-up) This patch addresses a review comment, which pointed out that we should *also* handle the pageNumber-input, from PR 12493. Given that a user *manually* changing pages using the pageNumber-input, on the toolbar, could be regarded as a pretty strong indication of user-intent w.r.t. navigation in the document, hence I suppose that updating the browser history in this case as well probably won't hurt. --- web/app.js | 2 +- web/base_viewer.js | 16 ++++++++++++++++ web/interfaces.js | 4 ++-- web/pdf_link_service.js | 15 ++++++++------- 4 files changed, 27 insertions(+), 10 deletions(-) diff --git a/web/app.js b/web/app.js index a414c4b7c90f8..731b9eab9a2ac 100644 --- a/web/app.js +++ b/web/app.js @@ -2537,7 +2537,7 @@ function webViewerPageNumberChanged(evt) { // Note that for `` HTML elements, an empty string will // be returned for non-number inputs; hence we simply do nothing in that case. if (evt.value !== "") { - pdfViewer.currentPageLabel = evt.value; + PDFViewerApplication.pdfLinkService.goToPage(evt.value); } // Ensure that the page number input displays the correct value, even if the diff --git a/web/base_viewer.js b/web/base_viewer.js index 1a825078756ad..e9bcd08d96348 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -787,6 +787,22 @@ class BaseViewer { this._scrollIntoView({ pageDiv: pageView.div }); } + /** + * @param {string} label - The page label. + * @returns {number|null} The page number corresponding to the page label, + * or `null` when no page labels exist and/or the input is invalid. + */ + pageLabelToPageNumber(label) { + if (!this._pageLabels) { + return null; + } + const i = this._pageLabels.indexOf(label); + if (i < 0) { + return null; + } + return i + 1; + } + /** * @typedef ScrollPageIntoViewParameters * @property {number} pageNumber - The page number. diff --git a/web/interfaces.js b/web/interfaces.js index f13b11e5f9eb1..62d7b13ebd6a6 100644 --- a/web/interfaces.js +++ b/web/interfaces.js @@ -59,9 +59,9 @@ class IPDFLinkService { async goToDestination(dest) {} /** - * @param {number} pageNumber - The page number. + * @param {number|string} val - The page number, or page label. */ - goToPage(pageNumber) {} + goToPage(val) {} /** * @param dest - The PDF destination object. diff --git a/web/pdf_link_service.js b/web/pdf_link_service.js index 9809aa0735f88..c484d9f5dee91 100644 --- a/web/pdf_link_service.js +++ b/web/pdf_link_service.js @@ -206,12 +206,15 @@ class PDFLinkService { /** * This method will, when available, also update the browser history. * - * @param {number} pageNumber - The page number. + * @param {number|string} val - The page number, or page label. */ - goToPage(pageNumber) { + goToPage(val) { if (!this.pdfDocument) { return; } + const pageNumber = + (typeof val === "string" && this.pdfViewer.pageLabelToPageNumber(val)) || + val | 0; if ( !( Number.isInteger(pageNumber) && @@ -219,9 +222,7 @@ class PDFLinkService { pageNumber <= this.pagesCount ) ) { - console.error( - `PDFLinkService.goToPage: "${pageNumber}" is not a valid page number.` - ); + console.error(`PDFLinkService.goToPage: "${val}" is not a valid page.`); return; } @@ -566,9 +567,9 @@ class SimpleLinkService { async goToDestination(dest) {} /** - * @param {number} pageNumber - The page number. + * @param {number|string} val - The page number, or page label. */ - goToPage(pageNumber) {} + goToPage(val) {} /** * @param dest - The PDF destination object. From 322b1072afa14c243ea443bda5044981747dede0 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 31 Oct 2020 11:24:39 +0100 Subject: [PATCH 3/3] Use optional chaining in `web/pdf_history.js` Since we're now free to use e.g. optional chaining everywhere *except* for the worker, we can thus simplify this code a bit. --- web/pdf_history.js | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/web/pdf_history.js b/web/pdf_history.js index 51e6038fd156d..a4e86774158f9 100644 --- a/web/pdf_history.js +++ b/web/pdf_history.js @@ -290,7 +290,7 @@ class PDFHistory { return; } - if (this._destination && this._destination.page === pageNumber) { + if (this._destination?.page === pageNumber) { // When the new page is identical to the one in `this._destination`, we // don't want to add a potential duplicate entry in the browser history. return; @@ -388,8 +388,7 @@ class PDFHistory { if ( typeof PDFJSDev !== "undefined" && PDFJSDev.test("CHROME") && - window.history.state && - window.history.state.chromecomState + window.history.state?.chromecomState ) { // history.state.chromecomState is managed by chromecom.js. newState.chromecomState = window.history.state.chromecomState; @@ -397,7 +396,7 @@ class PDFHistory { this._updateInternalState(destination, newState.uid); let newUrl; - if (this._updateUrl && destination && destination.hash) { + if (this._updateUrl && destination?.hash) { const baseUrl = document.location.href.split("#")[0]; // Prevent errors in Firefox. if (!baseUrl.startsWith("file://")) { @@ -494,7 +493,7 @@ class PDFHistory { return false; } const [perfEntry] = performance.getEntriesByType("navigation"); - if (!perfEntry || perfEntry.type !== "reload") { + if (perfEntry?.type !== "reload") { return false; } } else { @@ -523,7 +522,7 @@ class PDFHistory { clearTimeout(this._updateViewareaTimeout); this._updateViewareaTimeout = null; } - if (removeTemporary && destination && destination.temporary) { + if (removeTemporary && destination?.temporary) { // When the `destination` comes from the browser history, // we no longer treat it as a *temporary* position. delete destination.temporary; @@ -633,8 +632,7 @@ class PDFHistory { if ( (typeof PDFJSDev !== "undefined" && PDFJSDev.test("CHROME") && - state && - state.chromecomState && + state?.chromecomState && !this._isValidState(state)) || !state ) {