From 1190193d30e7f237139292d040435204ac66990e Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 15 Jan 2021 18:45:12 +0100 Subject: [PATCH] Add previous/next-page functionality that takes scroll/spread-modes into account (issue 11946) - For wrapped scrolling, we unfortunately need to do a fair bit of parsing of the *current* page layout. Compared to e.g. the spread-modes, where we can easily tell how the pages are laid out, with wrapped scrolling we cannot tell without actually checking. In particular documents with varying page sizes require some care, since we need to check all pages on the "row" of the current page are visible and that there aren't any "holes" present. Otherwise, in the general case, there's a risk that we'd skip over pages if we'd simply always advance to the previous/next "row" in wrapped scrolling. - For horizontal scrolling, this patch simply maintains the current behaviour of advancing *one* page at a time. The reason for this is to prevent inconsistent behaviour for the next and previous cases, since those cannot be handled identically. For the next-case, it'd obviously be simple to advance to the first not completely visible page. However for the previous-case, we'd only be able to go back *one* page since it's not possible to (easily) determine the page layout of non-visible pages (documents with varying page sizes being a particular issue). - For vertical scrolling, this patch maintains the current behaviour by default. When spread-modes are being used, we'll now attempt to advance to the next *spread*, rather than just the next page, whenever possible. To prevent skipping over a page, this two-page advance will only apply when both pages of the current spread are visible (to avoid breaking documents with varying page sizes) and when the second page in the current spread is fully visible *horizontally* (to handle larger zoom values). In order to reduce the performance impact of these changes, note that the previous/next-functionality will only call `getVisibleElements` for the scroll/spread-modes where that's necessary and that "normal" vertical scrolling is thus unaffected by these changes. To support these changes, the `getVisibleElements` helper function will now also include the `widthPercent` in addition to the existing `percent` property. The `PDFViewer._updateHelper` method is changed slightly w.r.t. updating the `currentPageNumber` for the non-vertical/non-spread modes, i.e. won't affect "normal" vertical scrolling, since that helped simplify the overall calculation of the page advance. Finally, these new `BaseViewer` methods also allow (some) simplification of previous/next-page functionality in various viewer components. *Please note:* There's one thing that this patch does not attempt to change, namely disabling of the previous/next toolbarButtons respectively the firstPage/lastPage secondaryToolbarButtons. The reason for this is that doing so would add quite a bit of complexity in general, and if for some reason `BaseViewer._getPageAdvance` would get things wrong we could end up incorrectly disabling the buttons. Hence it seemed overall safer to *not* touch this, and accept that the buttons won't be `disabled` despite in some edge-cases no further scrolling being possible. --- test/unit/ui_utils_spec.js | 20 +++-- web/app.js | 12 +-- web/base_viewer.js | 134 ++++++++++++++++++++++++++++++++++ web/pdf_link_service.js | 8 +- web/pdf_presentation_mode.js | 38 ++-------- web/pdf_single_page_viewer.js | 4 + web/pdf_viewer.js | 7 +- web/ui_utils.js | 11 +-- 8 files changed, 178 insertions(+), 56 deletions(-) diff --git a/test/unit/ui_utils_spec.js b/test/unit/ui_utils_spec.js index 927535dbd399bd..b3ca544da40001 100644 --- a/test/unit/ui_utils_spec.js +++ b/test/unit/ui_utils_spec.js @@ -650,11 +650,21 @@ describe("ui_utils", function () { const hiddenWidth = Math.max(0, scrollLeft - viewLeft) + Math.max(0, viewRight - scrollRight); - const visibleArea = - (div.clientHeight - hiddenHeight) * (div.clientWidth - hiddenWidth); - const percent = - ((visibleArea * 100) / div.clientHeight / div.clientWidth) | 0; - views.push({ id: view.id, x: viewLeft, y: viewTop, view, percent }); + + const fractionHeight = + (div.clientHeight - hiddenHeight) / div.clientHeight; + const fractionWidth = + (div.clientWidth - hiddenWidth) / div.clientWidth; + const percent = (fractionHeight * fractionWidth * 100) | 0; + + views.push({ + id: view.id, + x: viewLeft, + y: viewTop, + view, + percent, + widthPercent: (fractionWidth * 100) | 0, + }); } } return { first: views[0], last: views[views.length - 1], views }; diff --git a/web/app.js b/web/app.js index 0512310b301c2b..0152261a94a5e6 100644 --- a/web/app.js +++ b/web/app.js @@ -2829,10 +2829,10 @@ function webViewerLastPage() { } } function webViewerNextPage() { - PDFViewerApplication.page++; + PDFViewerApplication.pdfViewer.nextPage(); } function webViewerPreviousPage() { - PDFViewerApplication.page--; + PDFViewerApplication.pdfViewer.previousPage(); } function webViewerZoomIn() { PDFViewerApplication.zoomIn(); @@ -3351,13 +3351,9 @@ function webViewerKeyDown(evt) { (!turnOnlyIfPageFit || pdfViewer.currentScaleValue === "page-fit") ) { if (turnPage > 0) { - if (PDFViewerApplication.page < PDFViewerApplication.pagesCount) { - PDFViewerApplication.page++; - } + pdfViewer.nextPage(); } else { - if (PDFViewerApplication.page > 1) { - PDFViewerApplication.page--; - } + pdfViewer.previousPage(); } handled = true; } diff --git a/web/base_viewer.js b/web/base_viewer.js index 4060145c94db34..2d0707e17dc969 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -1507,6 +1507,140 @@ class BaseViewer { this.update(); } + /** + * @private + */ + _getPageAdvance(currentPageNumber, previous = false) { + if (this.isInPresentationMode) { + return 1; + } + switch (this._scrollMode) { + case ScrollMode.WRAPPED: { + const { views } = this._getVisiblePages(), + pageLayout = new Map(); + + // Determine the current (visible) page layout. + for (const { id, y, percent, widthPercent } of views) { + if (percent === 0 || widthPercent < 100) { + continue; + } + let yArray = pageLayout.get(y); + if (!yArray) { + pageLayout.set(y, (yArray ||= [])); + } + yArray.push(id); + } + // Find the row of the current page. + for (const yArray of pageLayout.values()) { + const currentIndex = yArray.indexOf(currentPageNumber); + if (currentIndex === -1) { + continue; + } + const numPages = yArray.length; + if (numPages === 1) { + break; + } + // Handle documents with varying page sizes. + if (previous) { + for (let i = currentIndex - 1, ii = 0; i >= ii; i--) { + const currentId = yArray[i], + expectedId = yArray[i + 1] - 1; + if (currentId < expectedId) { + return currentPageNumber - expectedId; + } + } + } else { + for (let i = currentIndex + 1, ii = numPages; i < ii; i++) { + const currentId = yArray[i], + expectedId = yArray[i - 1] + 1; + if (currentId > expectedId) { + return expectedId - currentPageNumber; + } + } + } + // The current row is "complete", advance to the previous/next one. + if (previous) { + const firstId = yArray[0]; + if (firstId < currentPageNumber) { + return currentPageNumber - firstId + 1; + } + } else { + const lastId = yArray[numPages - 1]; + if (lastId > currentPageNumber) { + return lastId - currentPageNumber + 1; + } + } + break; + } + break; + } + case ScrollMode.HORIZONTAL: { + break; + } + case ScrollMode.VERTICAL: { + if (this._spreadMode === SpreadMode.NONE) { + break; // Normal vertical scrolling. + } + const parity = this._spreadMode - 1; + + if (previous && currentPageNumber % 2 !== parity) { + break; // Left-hand side page. + } else if (!previous && currentPageNumber % 2 === parity) { + break; // Right-hand side page. + } + const { views } = this._getVisiblePages(), + expectedId = previous ? currentPageNumber - 1 : currentPageNumber + 1; + + for (const { id, percent, widthPercent } of views) { + if (id !== expectedId) { + continue; + } + if (percent > 0 && widthPercent === 100) { + return 2; + } + break; + } + break; + } + } + return 1; + } + + /** + * Go to the next page, taking scroll/spread-modes into account. + * @returns {boolean} Whether navigation occured. + */ + nextPage() { + const currentPageNumber = this._currentPageNumber, + pagesCount = this.pagesCount; + + if (currentPageNumber >= pagesCount) { + return false; + } + const advance = + this._getPageAdvance(currentPageNumber, /* previous = */ false) || 1; + + this.currentPageNumber = Math.min(currentPageNumber + advance, pagesCount); + return true; + } + + /** + * Go to the previous page, taking scroll/spread-modes into account. + * @returns {boolean} Whether navigation occured. + */ + previousPage() { + const currentPageNumber = this._currentPageNumber; + + if (currentPageNumber <= 1) { + return false; + } + const advance = + this._getPageAdvance(currentPageNumber, /* previous = */ true) || 1; + + this.currentPageNumber = Math.max(currentPageNumber - advance, 1); + return true; + } + initializeScriptingEvents() { if (!this.enableScripting || this._pageOpenPendingSet) { return; diff --git a/web/pdf_link_service.js b/web/pdf_link_service.js index fc7016fd49f31c..74d7bb17fc36b1 100644 --- a/web/pdf_link_service.js +++ b/web/pdf_link_service.js @@ -401,15 +401,11 @@ class PDFLinkService { break; case "NextPage": - if (this.page < this.pagesCount) { - this.page++; - } + this.pdfViewer.nextPage(); break; case "PrevPage": - if (this.page > 1) { - this.page--; - } + this.pdfViewer.previousPage(); break; case "LastPage": diff --git a/web/pdf_presentation_mode.js b/web/pdf_presentation_mode.js index 3a697ed0ccae7c..f9ad45280f2a20 100644 --- a/web/pdf_presentation_mode.js +++ b/web/pdf_presentation_mode.js @@ -138,7 +138,9 @@ class PDFPresentationMode { const totalDelta = this.mouseScrollDelta; this._resetMouseScrollState(); const success = - totalDelta > 0 ? this._goToPreviousPage() : this._goToNextPage(); + totalDelta > 0 + ? this.pdfViewer.previousPage() + : this.pdfViewer.nextPage(); if (success) { this.mouseScrollTimeStamp = currentTime; } @@ -153,32 +155,6 @@ class PDFPresentationMode { ); } - /** - * @private - */ - _goToPreviousPage() { - const page = this.pdfViewer.currentPageNumber; - // If we're at the first page, we don't need to do anything. - if (page <= 1) { - return false; - } - this.pdfViewer.currentPageNumber = page - 1; - return true; - } - - /** - * @private - */ - _goToNextPage() { - const page = this.pdfViewer.currentPageNumber; - // If we're at the last page, we don't need to do anything. - if (page >= this.pdfViewer.pagesCount) { - return false; - } - this.pdfViewer.currentPageNumber = page + 1; - return true; - } - /** * @private */ @@ -315,9 +291,9 @@ class PDFPresentationMode { evt.preventDefault(); if (evt.shiftKey) { - this._goToPreviousPage(); + this.pdfViewer.previousPage(); } else { - this._goToNextPage(); + this.pdfViewer.nextPage(); } } } @@ -422,9 +398,9 @@ class PDFPresentationMode { delta = dy; } if (delta > 0) { - this._goToPreviousPage(); + this.pdfViewer.previousPage(); } else if (delta < 0) { - this._goToNextPage(); + this.pdfViewer.nextPage(); } break; } diff --git a/web/pdf_single_page_viewer.js b/web/pdf_single_page_viewer.js index 20b340e1336d09..a3bee655dd87f6 100644 --- a/web/pdf_single_page_viewer.js +++ b/web/pdf_single_page_viewer.js @@ -121,6 +121,10 @@ class PDFSinglePageViewer extends BaseViewer { _updateScrollMode() {} _updateSpreadMode() {} + + _getPageAdvance() { + return 1; + } } export { PDFSinglePageViewer }; diff --git a/web/pdf_viewer.js b/web/pdf_viewer.js index e7965f37ba38c3..2a70a1de073b14 100644 --- a/web/pdf_viewer.js +++ b/web/pdf_viewer.js @@ -13,6 +13,7 @@ * limitations under the License. */ +import { ScrollMode, SpreadMode } from "./ui_utils.js"; import { BaseViewer } from "./base_viewer.js"; import { shadow } from "pdfjs-lib"; @@ -57,7 +58,11 @@ class PDFViewer extends BaseViewer { if (page.percent < 100) { break; } - if (page.id === currentId) { + if ( + page.id === currentId && + this._scrollMode === ScrollMode.VERTICAL && + this._spreadMode === SpreadMode.NONE + ) { stillFullyVisible = true; break; } diff --git a/web/ui_utils.js b/web/ui_utils.js index d678b19d25ac12..1103a3b4a14d87 100644 --- a/web/ui_utils.js +++ b/web/ui_utils.js @@ -563,17 +563,18 @@ function getVisibleElements({ Math.max(0, top - currentHeight) + Math.max(0, viewBottom - bottom); const hiddenWidth = Math.max(0, left - currentWidth) + Math.max(0, viewRight - right); - const percent = - (((viewHeight - hiddenHeight) * (viewWidth - hiddenWidth) * 100) / - viewHeight / - viewWidth) | - 0; + + const fractionHeight = (viewHeight - hiddenHeight) / viewHeight, + fractionWidth = (viewWidth - hiddenWidth) / viewWidth; + const percent = (fractionHeight * fractionWidth * 100) | 0; + visible.push({ id: view.id, x: currentWidth, y: currentHeight, view, percent, + widthPercent: (fractionWidth * 100) | 0, }); }