From 6cb77d65809babc94f96e7758789b29da8769a91 Mon Sep 17 00:00:00 2001 From: Xiliang Chen Date: Thu, 21 Jan 2016 11:39:30 +1300 Subject: [PATCH 1/2] fix outline may jump to previous page issue --- web/pdf_viewer.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/web/pdf_viewer.js b/web/pdf_viewer.js index 46ee55d99..a771cf636 100644 --- a/web/pdf_viewer.js +++ b/web/pdf_viewer.js @@ -627,7 +627,13 @@ var PDFViewer = (function pdfViewer() { pageView.viewport.convertToViewportPoint(x + width, y + height) ]; var left = Math.min(boundingRect[0][0], boundingRect[1][0]); - var top = Math.min(boundingRect[0][1], boundingRect[1][1]); + // Some pdf generator will generate a large top value (e.g. 10000) + // for outline destination + // which exceeds the hight of the page + // Therefore we have to ensure top is not less 0 + // otherwise viewer will scroll to previous page + // See PR 6903 and bug 874482 for more discussion + var top = Math.max(Math.min(boundingRect[0][1], boundingRect[1][1]), 0); scrollIntoView(pageView.div, { left: left, top: top }); }, From 076e25f1ca6b540396c68a684248820bea1a6b1e Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Mon, 20 Jun 2016 12:30:05 +0200 Subject: [PATCH 2/2] Prevent destinations with bad left/top values from scrolling the wrong page into view (bug 874482) There are PDF generators which create destinations with e.g. too large top values, which cause the wrong page to be scrolled into view because the offset becomes negative. By ignoring negative offsets, we can prevent this issue, and get a similar behaviour as in Adobe Reader. However, since we're also using `PDFViewer_scrollPageIntoView` in more cases than just when links (in the document/outline) are clicked, the patch adds a way to allow the caller to opt-out of this behaviour. In e.g. the following situations, I think that we still want to be able to allow negative offsets: when restoring a position from the `ViewHistory`, when the `viewBookmark` button is used to obtain a link to the current position, or when maintaining the current position on zooming. Rather than adding another parameter to `PDFViewer_scrollPageIntoView`, I've changed the signature to take an parameter object instead. To maintain backwards compatibility, I've added fallback code enclosed in a `GENERIC` preprocessor tag. Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=874482. --- web/app.js | 2 +- web/pdf_find_controller.js | 2 +- web/pdf_link_service.js | 11 +++++-- web/pdf_viewer.js | 60 +++++++++++++++++++++++++++++--------- 4 files changed, 57 insertions(+), 18 deletions(-) diff --git a/web/app.js b/web/app.js index f44783022..accfb1e99 100644 --- a/web/app.js +++ b/web/app.js @@ -1187,7 +1187,7 @@ var PDFViewerApplication = { this.forceRendering(); - this.pdfViewer.scrollPageIntoView(pageNumber); + this.pdfViewer.currentPageNumber = pageNumber; }, requestPresentationMode: function pdfViewRequestPresentationMode() { diff --git a/web/pdf_find_controller.js b/web/pdf_find_controller.js index bfe8027c5..2ee51e6b2 100644 --- a/web/pdf_find_controller.js +++ b/web/pdf_find_controller.js @@ -308,7 +308,7 @@ var PDFFindController = (function PDFFindControllerClosure() { // If the page is selected, scroll the page into view, which triggers // rendering the page, which adds the textLayer. Once the textLayer is // build, it will scroll onto the selected match. - this.pdfViewer.scrollPageIntoView(index + 1); + this.pdfViewer.currentPageNumber = index + 1; } var page = this.pdfViewer.getPageView(index); diff --git a/web/pdf_link_service.js b/web/pdf_link_service.js index 8263bc1a2..045913a56 100644 --- a/web/pdf_link_service.js +++ b/web/pdf_link_service.js @@ -115,7 +115,10 @@ var PDFLinkService = (function PDFLinkServiceClosure() { 'Trying to navigate to a non-existent page.'); return; } - self.pdfViewer.scrollPageIntoView(pageNumber, dest); + self.pdfViewer.scrollPageIntoView({ + pageNumber: pageNumber, + destArray: dest, + }); if (self.pdfHistory) { // Update the browsing history. @@ -241,7 +244,11 @@ var PDFLinkService = (function PDFLinkServiceClosure() { } } if (dest) { - this.pdfViewer.scrollPageIntoView(pageNumber || this.page, dest); + this.pdfViewer.scrollPageIntoView({ + pageNumber: pageNumber || this.page, + destArray: dest, + allowNegativeOffset: true, + }); } else if (pageNumber) { this.page = pageNumber; // simple page } diff --git a/web/pdf_viewer.js b/web/pdf_viewer.js index a771cf636..9ce56ec5f 100644 --- a/web/pdf_viewer.js +++ b/web/pdf_viewer.js @@ -459,7 +459,11 @@ var PDFViewer = (function pdfViewer() { dest = [null, { name: 'XYZ' }, this._location.left, this._location.top, null]; } - this.scrollPageIntoView(page, dest); + this.scrollPageIntoView({ + pageNumber: page, + destArray: dest, + allowNegativeOffset: true, + }); } this._setScaleDispatchEvent(newScale, newValue, preset); @@ -532,16 +536,38 @@ var PDFViewer = (function pdfViewer() { }, /** - * Scrolls page into view. - * @param {number} pageNumber - * @param {Array} dest - (optional) original PDF destination array: - * + * @typedef ScrollPageIntoViewParameters + * @param {number} pageNumber - The page number. + * @param {Array} destArray - (optional) The original PDF destination array, + * in the format: + * @param {boolean} allowNegativeOffset - (optional) Allow negative page + * offsets. The default value is `false`. */ - scrollPageIntoView: function PDFViewer_scrollPageIntoView(pageNumber, - dest) { + + /** + * Scrolls page into view. + * @param {ScrollPageIntoViewParameters} params + */ + scrollPageIntoView: function PDFViewer_scrollPageIntoView(params) { if (!this.pdfDocument) { return; } +//#if GENERIC + if (arguments.length > 1 || typeof params === 'number') { + console.warn('Call of scrollPageIntoView() with obsolete signature.'); + var paramObj = {}; + if (typeof params === 'number') { + paramObj.pageNumber = params; // pageNumber argument was found. + } + if (arguments[1] instanceof Array) { + paramObj.destArray = arguments[1]; // destArray argument was found. + } + params = paramObj; + } +//#endif + var pageNumber = params.pageNumber || 0; + var dest = params.destArray || null; + var allowNegativeOffset = params.allowNegativeOffset || false; if (this.isInPresentationMode || !dest) { this._setCurrentPageNumber(pageNumber, /* resetCurrentPageView */ true); @@ -549,6 +575,11 @@ var PDFViewer = (function pdfViewer() { } var pageView = this._pages[pageNumber - 1]; + if (!pageView) { + console.error('PDFViewer_scrollPageIntoView: ' + + 'Invalid "pageNumber" parameter.'); + return; + } var x = 0, y = 0; var width = 0, height = 0, widthScale, heightScale; var changeOrientation = (pageView.rotation % 180 === 0 ? false : true); @@ -627,14 +658,15 @@ var PDFViewer = (function pdfViewer() { pageView.viewport.convertToViewportPoint(x + width, y + height) ]; var left = Math.min(boundingRect[0][0], boundingRect[1][0]); - // Some pdf generator will generate a large top value (e.g. 10000) - // for outline destination - // which exceeds the hight of the page - // Therefore we have to ensure top is not less 0 - // otherwise viewer will scroll to previous page - // See PR 6903 and bug 874482 for more discussion - var top = Math.max(Math.min(boundingRect[0][1], boundingRect[1][1]), 0); + var top = Math.min(boundingRect[0][1], boundingRect[1][1]); + if (!allowNegativeOffset) { + // Some bad PDF generators will create destinations with e.g. top values + // that exceeds the page height. Ensure that offsets are not negative, + // to prevent a previous page from becoming visible (fixes bug 874482). + left = Math.max(left, 0); + top = Math.max(top, 0); + } scrollIntoView(pageView.div, { left: left, top: top }); },