From 8431cfe4829bc5c27f826329c3938c038f31eaf2 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 3 Oct 2020 17:43:33 +0200 Subject: [PATCH 1/3] Re-name and re-factor the `PDFLinkService.navigateTo` method This modernizes and improves the code, by using `async`/`await` and by extracting the helper function to its own method. To hopefully avoid confusion, given the next patch, the method is also re-named to `goToDestination` to make is slightly clearer what it actually does. --- src/display/annotation_layer.js | 2 +- web/interfaces.js | 4 +- web/pdf_history.js | 4 +- web/pdf_link_service.js | 166 +++++++++++++++++--------------- web/pdf_outline_viewer.js | 2 +- 5 files changed, 94 insertions(+), 84 deletions(-) diff --git a/src/display/annotation_layer.js b/src/display/annotation_layer.js index 8985cbd6f..72944b82b 100644 --- a/src/display/annotation_layer.js +++ b/src/display/annotation_layer.js @@ -344,7 +344,7 @@ class LinkAnnotationElement extends AnnotationElement { link.href = this.linkService.getDestinationHash(destination); link.onclick = () => { if (destination) { - this.linkService.navigateTo(destination); + this.linkService.goToDestination(destination); } return false; }; diff --git a/web/interfaces.js b/web/interfaces.js index b05fbe2a9..7469c9385 100644 --- a/web/interfaces.js +++ b/web/interfaces.js @@ -54,9 +54,9 @@ class IPDFLinkService { set externalLinkEnabled(value) {} /** - * @param dest - The PDF destination object. + * @param {string|Array} dest - The named, or explicit, PDF destination. */ - navigateTo(dest) {} + async goToDestination(dest) {} /** * @param dest - The PDF destination object. diff --git a/web/pdf_history.js b/web/pdf_history.js index 8e2b148ad..097beb64e 100644 --- a/web/pdf_history.js +++ b/web/pdf_history.js @@ -647,7 +647,7 @@ class PDFHistory { this.linkService.rotation = destination.rotation; } if (destination.dest) { - this.linkService.navigateTo(destination.dest); + this.linkService.goToDestination(destination.dest); } else if (destination.hash) { this.linkService.setHash(destination.hash); } else if (destination.page) { @@ -655,7 +655,7 @@ class PDFHistory { this.linkService.page = destination.page; } - // Since `PDFLinkService.navigateTo` is asynchronous, we thus defer the + // Since `PDFLinkService.goToDestination` is asynchronous, we thus defer the // resetting of `this._popStateInProgress` slightly. Promise.resolve().then(() => { this._popStateInProgress = false; diff --git a/web/pdf_link_service.js b/web/pdf_link_service.js index 4b96fbafd..7aba5dc73 100644 --- a/web/pdf_link_service.js +++ b/web/pdf_link_service.js @@ -108,91 +108,98 @@ class PDFLinkService { } /** - * @param {string|Array} dest - The named, or explicit, PDF destination. + * @deprecated */ navigateTo(dest) { - const goToDestination = ({ namedDest, explicitDest }) => { - // Dest array looks like that: - const destRef = explicitDest[0]; - let pageNumber; + console.error( + "Deprecated method: `navigateTo`, use `goToDestination` instead." + ); + this.goToDestination(dest); + } - if (destRef instanceof Object) { - pageNumber = this._cachedPageNumber(destRef); + /** + * @private + */ + _goToDestinationHelper(rawDest, namedDest = null, explicitDest) { + // Dest array looks like that: + const destRef = explicitDest[0]; + let pageNumber; - if (pageNumber === null) { - // Fetch the page reference if it's not yet available. This could - // only occur during loading, before all pages have been resolved. - this.pdfDocument - .getPageIndex(destRef) - .then(pageIndex => { - this.cachePageRef(pageIndex + 1, destRef); - goToDestination({ namedDest, explicitDest }); - }) - .catch(() => { - console.error( - `PDFLinkService.navigateTo: "${destRef}" is not ` + - `a valid page reference, for dest="${dest}".` - ); - }); - return; - } - } else if (Number.isInteger(destRef)) { - pageNumber = destRef + 1; - } else { - console.error( - `PDFLinkService.navigateTo: "${destRef}" is not ` + - `a valid destination reference, for dest="${dest}".` - ); - return; - } - if (!pageNumber || pageNumber < 1 || pageNumber > this.pagesCount) { - console.error( - `PDFLinkService.navigateTo: "${pageNumber}" is not ` + - `a valid page number, for dest="${dest}".` - ); - return; - } + if (destRef instanceof Object) { + pageNumber = this._cachedPageNumber(destRef); - if (this.pdfHistory) { - // Update the browser history before scrolling the new destination into - // view, to be able to accurately capture the current document position. - this.pdfHistory.pushCurrentPosition(); - this.pdfHistory.push({ namedDest, explicitDest, pageNumber }); - } - - this.pdfViewer.scrollPageIntoView({ - pageNumber, - destArray: explicitDest, - ignoreDestinationZoom: this._ignoreDestinationZoom, - }); - }; - - new Promise((resolve, reject) => { - if (typeof dest === "string") { - this.pdfDocument.getDestination(dest).then(destArray => { - resolve({ - namedDest: dest, - explicitDest: destArray, + if (pageNumber === null) { + // Fetch the page reference if it's not yet available. This could + // only occur during loading, before all pages have been resolved. + this.pdfDocument + .getPageIndex(destRef) + .then(pageIndex => { + this.cachePageRef(pageIndex + 1, destRef); + this._goToDestinationHelper(rawDest, namedDest, explicitDest); + }) + .catch(() => { + console.error( + `PDFLinkService._goToDestinationHelper: "${destRef}" is not ` + + `a valid page reference, for dest="${rawDest}".` + ); }); - }); return; } - resolve({ - namedDest: "", - explicitDest: dest, - }); - }).then(data => { - if (!Array.isArray(data.explicitDest)) { - console.error( - `PDFLinkService.navigateTo: "${data.explicitDest}" is` + - ` not a valid destination array, for dest="${dest}".` - ); - return; - } - goToDestination(data); + } else if (Number.isInteger(destRef)) { + pageNumber = destRef + 1; + } else { + console.error( + `PDFLinkService._goToDestinationHelper: "${destRef}" is not ` + + `a valid destination reference, for dest="${rawDest}".` + ); + return; + } + if (!pageNumber || pageNumber < 1 || pageNumber > this.pagesCount) { + console.error( + `PDFLinkService._goToDestinationHelper: "${pageNumber}" is not ` + + `a valid page number, for dest="${rawDest}".` + ); + return; + } + + if (this.pdfHistory) { + // Update the browser history before scrolling the new destination into + // view, to be able to accurately capture the current document position. + this.pdfHistory.pushCurrentPosition(); + this.pdfHistory.push({ namedDest, explicitDest, pageNumber }); + } + + this.pdfViewer.scrollPageIntoView({ + pageNumber, + destArray: explicitDest, + ignoreDestinationZoom: this._ignoreDestinationZoom, }); } + /** + * This method will, when available, also update the browser history. + * + * @param {string|Array} dest - The named, or explicit, PDF destination. + */ + async goToDestination(dest) { + let namedDest, explicitDest; + if (typeof dest === "string") { + namedDest = dest; + explicitDest = await this.pdfDocument.getDestination(dest); + } else { + namedDest = null; + explicitDest = await dest; + } + if (!Array.isArray(explicitDest)) { + console.error( + `PDFLinkService.goToDestination: "${explicitDest}" is not ` + + `a valid destination array, for dest="${dest}".` + ); + return; + } + this._goToDestinationHelper(dest, namedDest, explicitDest); + } + /** * @param {string|Array} dest - The PDF destination object. * @returns {string} The hyperlink to the PDF object. @@ -307,7 +314,7 @@ class PDFLinkService { // Ensure that this parameter is *always* handled last, in order to // guarantee that it won't be overridden (e.g. by the "page" parameter). if ("nameddest" in params) { - this.navigateTo(params.nameddest); + this.goToDestination(params.nameddest); } } else { // Named (or explicit) destination. @@ -323,7 +330,7 @@ class PDFLinkService { } catch (ex) {} if (typeof dest === "string" || isValidExplicitDestination(dest)) { - this.navigateTo(dest); + this.goToDestination(dest); return; } console.error( @@ -394,6 +401,9 @@ class PDFLinkService { this._pagesRefCache[refStr] = pageNum; } + /** + * @private + */ _cachedPageNumber(pageRef) { const refStr = pageRef.gen === 0 ? `${pageRef.num}R` : `${pageRef.num}R${pageRef.gen}`; @@ -510,9 +520,9 @@ class SimpleLinkService { set rotation(value) {} /** - * @param dest - The PDF destination object. + * @param {string|Array} dest - The named, or explicit, PDF destination. */ - navigateTo(dest) {} + async goToDestination(dest) {} /** * @param dest - The PDF destination object. diff --git a/web/pdf_outline_viewer.js b/web/pdf_outline_viewer.js index bb489b7a2..5dbdf1360 100644 --- a/web/pdf_outline_viewer.js +++ b/web/pdf_outline_viewer.js @@ -73,7 +73,7 @@ class PDFOutlineViewer extends BaseTreeViewer { element.href = linkService.getDestinationHash(dest); element.onclick = () => { if (dest) { - linkService.navigateTo(dest); + linkService.goToDestination(dest); } return false; }; From 295716f49646877a023c5f5a39211f2ae29439e4 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 3 Oct 2020 18:08:30 +0200 Subject: [PATCH 2/3] Support adding pages, in addition to regular destinations, to the browser history and use it with thumbnails (issue 12440) While the referenced issue could very well be seen as an edge-case, this patch adds support for updating of the browser history when interacting with the thumbnails in the sidebar (assuming we want to do this). The main reason for adding the history implementation in the first place, was to simplify navigating back to a previous position in the document when named/explicit destinations are used (e.g. when clicking on "links" or when using the outline in the sidebar). As such, it never really crossed by mind to update the browser history when the thumbnails are used. However, a user clicking on thumbnails 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 particular case probably won't hurt. --- web/interfaces.js | 10 ++++++++ web/pdf_history.js | 49 +++++++++++++++++++++++++++++++++++++++ web/pdf_link_service.js | 34 +++++++++++++++++++++++++++ web/pdf_thumbnail_view.js | 2 +- 4 files changed, 94 insertions(+), 1 deletion(-) diff --git a/web/interfaces.js b/web/interfaces.js index 7469c9385..f13b11e5f 100644 --- a/web/interfaces.js +++ b/web/interfaces.js @@ -58,6 +58,11 @@ class IPDFLinkService { */ async goToDestination(dest) {} + /** + * @param {number} pageNumber - The page number. + */ + goToPage(pageNumber) {} + /** * @param dest - The PDF destination object. * @returns {string} The hyperlink to the PDF object. @@ -108,6 +113,11 @@ class IPDFHistory { */ push({ namedDest = null, explicitDest, pageNumber }) {} + /** + * @param {number} pageNumber + */ + pushPage(pageNumber) {} + pushCurrentPosition() {} back() {} diff --git a/web/pdf_history.js b/web/pdf_history.js index 097beb64e..696595a5d 100644 --- a/web/pdf_history.js +++ b/web/pdf_history.js @@ -271,6 +271,55 @@ class PDFHistory { } } + /** + * Push a page to the browser history; generally the `push` method should be + * used instead. + * @param {number} pageNumber + */ + pushPage(pageNumber) { + if (!this._initialized) { + return; + } + if ( + !( + Number.isInteger(pageNumber) && + pageNumber > 0 && + pageNumber <= this.linkService.pagesCount + ) + ) { + console.error( + `PDFHistory.pushPage: "${pageNumber}" is not a valid page number.` + ); + return; + } + + if (this._destination && 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; + } + if (this._popStateInProgress) { + return; + } + + this._pushOrReplaceState({ + hash: `page=${pageNumber}`, + page: pageNumber, + rotation: this.linkService.rotation, + }); + + if (!this._popStateInProgress) { + // Prevent the browser history from updating while the new page is + // being scrolled into view, to avoid potentially inconsistent state. + this._popStateInProgress = true; + // We defer the resetting of `this._popStateInProgress`, to account for + // e.g. zooming occuring when the new page is being navigated to. + Promise.resolve().then(() => { + this._popStateInProgress = false; + }); + } + } + /** * Push the current position to the browser history. */ diff --git a/web/pdf_link_service.js b/web/pdf_link_service.js index 7aba5dc73..434d6c757 100644 --- a/web/pdf_link_service.js +++ b/web/pdf_link_service.js @@ -200,6 +200,35 @@ class PDFLinkService { this._goToDestinationHelper(dest, namedDest, explicitDest); } + /** + * This method will, when available, also update the browser history. + * + * @param {number} pageNumber - The page number. + */ + goToPage(pageNumber) { + if ( + !( + Number.isInteger(pageNumber) && + pageNumber > 0 && + pageNumber <= this.pagesCount + ) + ) { + console.error( + `PDFLinkService.goToPage: "${pageNumber}" is not a valid page number.` + ); + return; + } + + if (this.pdfHistory) { + // Update the browser history before scrolling the new page into view, + // to be able to accurately capture the current document position. + this.pdfHistory.pushCurrentPosition(); + this.pdfHistory.pushPage(pageNumber); + } + + this.pdfViewer.scrollPageIntoView({ pageNumber }); + } + /** * @param {string|Array} dest - The PDF destination object. * @returns {string} The hyperlink to the PDF object. @@ -524,6 +553,11 @@ class SimpleLinkService { */ async goToDestination(dest) {} + /** + * @param {number} pageNumber - The page number. + */ + goToPage(pageNumber) {} + /** * @param dest - The PDF destination object. * @returns {string} The hyperlink to the PDF object. diff --git a/web/pdf_thumbnail_view.js b/web/pdf_thumbnail_view.js index 9e70626df..bf325ec6d 100644 --- a/web/pdf_thumbnail_view.js +++ b/web/pdf_thumbnail_view.js @@ -138,7 +138,7 @@ class PDFThumbnailView { anchor.title = msg; }); anchor.onclick = function () { - linkService.page = id; + linkService.goToPage(id); return false; }; this.anchor = anchor; From b302fd3a6ece8559105c3e1d608872a274db2cf7 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Mon, 12 Oct 2020 12:26:19 +0200 Subject: [PATCH 3/3] Move updating of `this._maxUid` into `PDFHistory._updateInternalState` There's no compelling reason to update this property *manually* in multiple places, since that's error-prone with any future code changes, given that `_updateInternalState` is always called just before anyway. --- web/pdf_history.js | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/web/pdf_history.js b/web/pdf_history.js index 696595a5d..51e6038fd 100644 --- a/web/pdf_history.js +++ b/web/pdf_history.js @@ -143,9 +143,6 @@ class PDFHistory { state.uid, /* removeTemporary = */ true ); - if (this._uid > this._maxUid) { - this._maxUid = this._uid; - } if (destination.rotation !== undefined) { this._initialRotation = destination.rotation; @@ -410,7 +407,6 @@ class PDFHistory { if (shouldReplace) { window.history.replaceState(newState, "", newUrl); } else { - this._maxUid = this._uid; window.history.pushState(newState, "", newUrl); } @@ -534,6 +530,7 @@ class PDFHistory { } this._destination = destination; this._uid = uid; + this._maxUid = Math.max(this._maxUid, uid); // This should always be reset when `this._destination` is updated. this._numPositionUpdates = 0; } @@ -688,9 +685,6 @@ class PDFHistory { state.uid, /* removeTemporary = */ true ); - if (this._uid > this._maxUid) { - this._maxUid = this._uid; - } if (isValidRotation(destination.rotation)) { this.linkService.rotation = destination.rotation;