From 06cda4c2e7a1b0fce1ca3f9bbffa0481a7de1cde Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 23 Jan 2019 09:11:06 +0100 Subject: [PATCH 1/2] Move additional code/methods into `BaseViewer` and have the extending classes override/extend methods as necessary This attempts to provide more "default" methods in the base class, in order to reduce unnecessary duplication and to improve self-documentation of the `BaseViewer` class slightly. The following changes are made (in no particular order): - Have `BaseViewer` implement the `_scrollIntoView` method, and *extend* it as necessary in `PDFViewer`/`PDFSinglePageViewer`. - Simply inline the `BaseViewer._resizeBuffer` method, in `BaseViewer.update`, since there's only one call-site at this point. - Provide a default implementation of `_isScrollModeHorizontal` in `BaseViewer`, and have `PDFSinglePageViewer` override it. - Provide a default implementation of `_getVisiblePages`, and have `PDFViewer` extend it and `PDFSinglePageViewer` override it. --- web/base_viewer.js | 32 ++++++++++++++------------------ web/pdf_single_page_viewer.js | 3 +-- web/pdf_viewer.js | 27 +++++++++------------------ 3 files changed, 24 insertions(+), 38 deletions(-) diff --git a/web/base_viewer.js b/web/base_viewer.js index 8e54b554f..d3f03214c 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -15,9 +15,10 @@ import { CSS_UNITS, DEFAULT_SCALE, DEFAULT_SCALE_VALUE, getGlobalEventBus, - isPortraitOrientation, isValidRotation, MAX_AUTO_SCALE, moveToEndOfArray, - NullL10n, PresentationModeState, RendererType, SCROLLBAR_PADDING, - TextLayerMode, UNKNOWN_SCALE, VERTICAL_PADDING, watchScroll + getVisibleElements, isPortraitOrientation, isValidRotation, MAX_AUTO_SCALE, + moveToEndOfArray, NullL10n, PresentationModeState, RendererType, + SCROLLBAR_PADDING, scrollIntoView, TextLayerMode, UNKNOWN_SCALE, + VERTICAL_PADDING, watchScroll } from './ui_utils'; import { PDFRenderingQueue, RenderingStates } from './pdf_rendering_queue'; import { AnnotationLayerBuilder } from './annotation_layer_builder'; @@ -360,6 +361,7 @@ class BaseViewer { } get _setDocumentViewerElement() { + // In most viewers, e.g. `PDFViewer`, this should return `this.viewer`. throw new Error('Not implemented: _setDocumentViewerElement'); } @@ -545,7 +547,7 @@ class BaseViewer { } _scrollIntoView({ pageDiv, pageSpot = null, pageNumber = null, }) { - throw new Error('Not implemented: _scrollIntoView'); + scrollIntoView(pageDiv, pageSpot); } _setScaleUpdatePages(newScale, newValue, noScroll = false, preset = false) { @@ -784,17 +786,6 @@ class BaseViewer { }); } - /** - * visiblePages is optional; if present, it should be an array of pages and in - * practice its length is going to be numVisiblePages, but this is not - * required. The new size of the buffer depends only on numVisiblePages. - */ - _resizeBuffer(numVisiblePages, visiblePages) { - let suggestedCacheSize = Math.max(DEFAULT_CACHE_SIZE, - 2 * numVisiblePages + 1); - this._buffer.resize(suggestedCacheSize, visiblePages); - } - _updateLocation(firstPage) { let currentScale = this._currentScale; let currentScaleValue = this._currentScaleValue; @@ -835,7 +826,8 @@ class BaseViewer { if (numVisiblePages === 0) { return; } - this._resizeBuffer(numVisiblePages, visiblePages); + const newCacheSize = Math.max(DEFAULT_CACHE_SIZE, 2 * numVisiblePages + 1); + this._buffer.resize(newCacheSize, visiblePages); this.renderingQueue.renderHighestPriority(visible); @@ -857,7 +849,10 @@ class BaseViewer { } get _isScrollModeHorizontal() { - throw new Error('Not implemented: _isScrollModeHorizontal'); + // Used to ensure that pre-rendering of the next/previous page works + // correctly, since Scroll/Spread modes are ignored in Presentation Mode. + return (this.isInPresentationMode ? + false : this._scrollMode === ScrollMode.HORIZONTAL); } get isInPresentationMode() { @@ -903,7 +898,8 @@ class BaseViewer { } _getVisiblePages() { - throw new Error('Not implemented: _getVisiblePages'); + return getVisibleElements(this.container, this._pages, true, + this._isScrollModeHorizontal); } /** diff --git a/web/pdf_single_page_viewer.js b/web/pdf_single_page_viewer.js index e07f555c9..8ef656728 100644 --- a/web/pdf_single_page_viewer.js +++ b/web/pdf_single_page_viewer.js @@ -14,7 +14,6 @@ */ import { BaseViewer } from './base_viewer'; -import { scrollIntoView } from './ui_utils'; import { shadow } from 'pdfjs-lib'; class PDFSinglePageViewer extends BaseViewer { @@ -87,7 +86,7 @@ class PDFSinglePageViewer extends BaseViewer { let previousLocation = this._location; this._ensurePageViewVisible(); - scrollIntoView(pageDiv, pageSpot); + super._scrollIntoView({ pageDiv, pageSpot, pageNumber, }); // Since scrolling is tracked using `requestAnimationFrame`, update the // scroll direction during the next `this._scrollUpdate` invocation. diff --git a/web/pdf_viewer.js b/web/pdf_viewer.js index bc439e995..7bf464728 100644 --- a/web/pdf_viewer.js +++ b/web/pdf_viewer.js @@ -13,8 +13,7 @@ * limitations under the License. */ -import { BaseViewer, ScrollMode } from './base_viewer'; -import { getVisibleElements, scrollIntoView } from './ui_utils'; +import { BaseViewer } from './base_viewer'; import { shadow } from 'pdfjs-lib'; class PDFViewer extends BaseViewer { @@ -22,27 +21,26 @@ class PDFViewer extends BaseViewer { return shadow(this, '_setDocumentViewerElement', this.viewer); } - _scrollIntoView({ pageDiv, pageSpot = null, }) { + _scrollIntoView({ pageDiv, pageSpot = null, pageNumber = null, }) { if (!pageSpot && !this.isInPresentationMode) { const left = pageDiv.offsetLeft + pageDiv.clientLeft; const right = left + pageDiv.clientWidth; const { scrollLeft, clientWidth, } = this.container; - if (this._scrollMode === ScrollMode.HORIZONTAL || + if (this._isScrollModeHorizontal || left < scrollLeft || right > scrollLeft + clientWidth) { pageSpot = { left: 0, top: 0, }; } } - scrollIntoView(pageDiv, pageSpot); + super._scrollIntoView({ pageDiv, pageSpot, pageNumber, }); } _getVisiblePages() { - if (!this.isInPresentationMode) { - return getVisibleElements(this.container, this._pages, true, - this._scrollMode === ScrollMode.HORIZONTAL); + if (this.isInPresentationMode) { + // The algorithm in `getVisibleElements` doesn't work in all browsers and + // configurations (e.g. Chrome) when Presentation Mode is active. + return this._getCurrentVisiblePage(); } - // The algorithm in `getVisibleElements` doesn't work in all browsers and - // configurations when presentation mode is active. - return this._getCurrentVisiblePage(); + return super._getVisiblePages(); } _updateHelper(visiblePages) { @@ -66,13 +64,6 @@ class PDFViewer extends BaseViewer { } this._setCurrentPageNumber(currentId); } - - get _isScrollModeHorizontal() { - // Used to ensure that pre-rendering of the next/previous page works - // correctly, since Scroll/Spread modes are ignored in Presentation Mode. - return (this.isInPresentationMode ? - false : this._scrollMode === ScrollMode.HORIZONTAL); - } } export { From 48e4adf7709caa085926c8830b768b71ac62ca1c Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 23 Jan 2019 09:13:49 +0100 Subject: [PATCH 2/2] Try to simplify the `PDFSinglePageViewer._scrollIntoView` method slightly, by unconditionally ensuring that rendering always occurs --- web/pdf_single_page_viewer.js | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/web/pdf_single_page_viewer.js b/web/pdf_single_page_viewer.js index 8ef656728..c8d3637b4 100644 --- a/web/pdf_single_page_viewer.js +++ b/web/pdf_single_page_viewer.js @@ -39,6 +39,7 @@ class PDFSinglePageViewer extends BaseViewer { super._resetView(); this._previousPageNumber = 1; this._shadowViewer = document.createDocumentFragment(); + this._updateScrollDown = null; } _ensurePageViewVisible() { @@ -82,9 +83,12 @@ class PDFSinglePageViewer extends BaseViewer { if (pageNumber) { // Ensure that `this._currentPageNumber` is correct. this._setCurrentPageNumber(pageNumber); } - let scrolledDown = this._currentPageNumber >= this._previousPageNumber; - let previousLocation = this._location; + const scrolledDown = this._currentPageNumber >= this._previousPageNumber; + this._ensurePageViewVisible(); + // Ensure that rendering always occurs, to avoid showing a blank page, + // even if the current position doesn't change when the page is scrolled. + this.update(); super._scrollIntoView({ pageDiv, pageSpot, pageNumber, }); @@ -92,18 +96,8 @@ class PDFSinglePageViewer extends BaseViewer { // scroll direction during the next `this._scrollUpdate` invocation. this._updateScrollDown = () => { this.scroll.down = scrolledDown; - delete this._updateScrollDown; + this._updateScrollDown = null; }; - // If the scroll position doesn't change as a result of the `scrollIntoView` - // call, ensure that rendering always occurs to avoid showing a blank page. - setTimeout(() => { - if (this._location === previousLocation) { - if (this._updateScrollDown) { - this._updateScrollDown(); - } - this.update(); - } - }, 0); } _getVisiblePages() {