From ffeba9c6306927552da2435d4709af57e6bd452b Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Tue, 23 Jun 2015 17:16:21 +0200 Subject: [PATCH] Move the page switching code into `set currentPageNumber` in `PDFViewer` instead of placing it in the `pagechange` event handler The reason that this code can be moved is that the `if (this.loading && page === 1)` check, in the `pagechange` event handler in viewer.js, is never satisfied since `this.loading` is not defined in that scope. This *could* be considered a regression from PR 5295, since prior to that `this.loading` was using the `PDFViewerApplication` scope (or `PDFView` as it were). However, I don't think that we need to fix that since we've been shipping this code in no less than *three* Firefox releases (uplifted in https://bugzilla.mozilla.org/show_bug.cgi?id=1084158), without breaking the world. An explanation of why the `pagechange` code works, despite `this.loading === undefined`, is that `set currentPageNumber` (in `PDFViewer`) returns early whenever `this.pdfDocument` isn't set. This check is, for all intents and purposes, functionally equivalent to checking `PDFViewerApplication.loading`. Hence we can move the page switching code into `PDFViewer`, and also remove `PDFViewerApplication.loading` since it's not used any more. (The `this.loading` property was added in PR 686, which was before the current viewer even existed.) *Note:* The changes in this patch should also be beneficial to the viewer `components`, since requiring every implementer to provide their own `pagechange` event handler just to get `PDFViewer.currentPageNumber` to actually work seems like an unnecessary complication. --- web/pdf_viewer.js | 10 ++++++++-- web/viewer.js | 13 ------------- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/web/pdf_viewer.js b/web/pdf_viewer.js index 94e3f9fc7..75dcf32a1 100644 --- a/web/pdf_viewer.js +++ b/web/pdf_viewer.js @@ -137,6 +137,12 @@ var PDFViewer = (function pdfViewer() { this._currentPageNumber = val; event.pageNumber = val; this.container.dispatchEvent(event); + + // Check if the caller is `PDFViewer_update`, to avoid breaking scrolling. + if (this.updateInProgress) { + return; + } + this.scrollPageIntoView(val); }, /** @@ -566,7 +572,7 @@ var PDFViewer = (function pdfViewer() { }; }, - update: function () { + update: function PDFViewer_update() { var visible = this._getVisiblePages(); var visiblePages = visible.views; if (visiblePages.length === 0) { @@ -581,7 +587,7 @@ var PDFViewer = (function pdfViewer() { this.renderingQueue.renderHighestPriority(visible); - var currentId = this.currentPageNumber; + var currentId = this._currentPageNumber; var firstPage = visible.first; for (var i = 0, ii = visiblePages.length, stillFullyVisible = false; diff --git a/web/viewer.js b/web/viewer.js index 32ed42843..ea09d868a 100644 --- a/web/viewer.js +++ b/web/viewer.js @@ -526,7 +526,6 @@ var PDFViewerApplication = { } var self = this; - self.loading = true; self.downloadComplete = false; var passwordNeeded = function passwordNeeded(updatePassword, reason) { @@ -543,7 +542,6 @@ var PDFViewerApplication = { getDocumentProgress).then( function getDocumentCallback(pdfDocument) { self.load(pdfDocument, scale); - self.loading = false; }, function getDocumentError(exception) { var message = exception && exception.message; @@ -571,7 +569,6 @@ var PDFViewerApplication = { message: message }; self.error(loadingErrorMessage, moreInfo); - self.loading = false; } ); @@ -1822,16 +1819,6 @@ window.addEventListener('pagechange', function pagechange(evt) { Stats.add(page, pageView.stats); } } - - // checking if the this.page was called from the updateViewarea function - if (evt.updateInProgress) { - return; - } - // Avoid scrolling the first page during loading - if (this.loading && page === 1) { - return; - } - PDFViewerApplication.pdfViewer.scrollPageIntoView(page); }, true); function handleMouseWheel(evt) {