From 36111a1f40ae800556ab4196f4e7f299939d9e22 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 21 Jun 2018 17:55:59 +0200 Subject: [PATCH 1/8] [Regression] Remove instances of `Element.classList.toggle()` with *two* parameters, since browser support is limited The Secondary Toolbar buttons for, not to mention the actual toggling of, Scroll/Spread modes are currently completely broken in older browsers (such as IE11). As a follow-up, it'd probably be a good idea to try and find a *feature complete* `classList` polyfill that could be used instead, but this patch at least addresses the immediate regression. Please refer to the compatibility information in https://developer.mozilla.org/en-US/docs/Web/API/Element/classList#Browser_compatibility --- web/base_viewer.js | 15 +++++++++++--- web/secondary_toolbar.js | 42 ++++++++++++++++++++++++++++------------ 2 files changed, 42 insertions(+), 15 deletions(-) diff --git a/web/base_viewer.js b/web/base_viewer.js index e084512b0..2eee52ac9 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -1035,9 +1035,18 @@ class BaseViewer { } _updateScrollModeClasses() { - const mode = this.scrollMode, { classList, } = this.viewer; - classList.toggle('scrollHorizontal', mode === ScrollMode.HORIZONTAL); - classList.toggle('scrollWrapped', mode === ScrollMode.WRAPPED); + const { scrollMode, viewer, } = this; + + if (scrollMode === ScrollMode.HORIZONTAL) { + viewer.classList.add('scrollHorizontal'); + } else { + viewer.classList.remove('scrollHorizontal'); + } + if (scrollMode === ScrollMode.WRAPPED) { + viewer.classList.add('scrollWrapped'); + } else { + viewer.classList.remove('scrollWrapped'); + } } setSpreadMode(mode) { diff --git a/web/secondary_toolbar.js b/web/secondary_toolbar.js index a44be0642..233f5f6fa 100644 --- a/web/secondary_toolbar.js +++ b/web/secondary_toolbar.js @@ -190,23 +190,41 @@ class SecondaryToolbar { _bindScrollModeListener(buttons) { this.eventBus.on('scrollmodechanged', function(evt) { - buttons.scrollVerticalButton.classList.toggle('toggled', - evt.mode === ScrollMode.VERTICAL); - buttons.scrollHorizontalButton.classList.toggle('toggled', - evt.mode === ScrollMode.HORIZONTAL); - buttons.scrollWrappedButton.classList.toggle('toggled', - evt.mode === ScrollMode.WRAPPED); + buttons.scrollVerticalButton.classList.remove('toggled'); + buttons.scrollHorizontalButton.classList.remove('toggled'); + buttons.scrollWrappedButton.classList.remove('toggled'); + + switch (evt.mode) { + case ScrollMode.VERTICAL: + buttons.scrollVerticalButton.classList.add('toggled'); + break; + case ScrollMode.HORIZONTAL: + buttons.scrollHorizontalButton.classList.add('toggled'); + break; + case ScrollMode.WRAPPED: + buttons.scrollWrappedButton.classList.add('toggled'); + break; + } }); } _bindSpreadModeListener(buttons) { this.eventBus.on('spreadmodechanged', function(evt) { - buttons.spreadNoneButton.classList.toggle('toggled', - evt.mode === SpreadMode.NONE); - buttons.spreadOddButton.classList.toggle('toggled', - evt.mode === SpreadMode.ODD); - buttons.spreadEvenButton.classList.toggle('toggled', - evt.mode === SpreadMode.EVEN); + buttons.spreadNoneButton.classList.remove('toggled'); + buttons.spreadOddButton.classList.remove('toggled'); + buttons.spreadEvenButton.classList.remove('toggled'); + + switch (evt.mode) { + case SpreadMode.NONE: + buttons.spreadNoneButton.classList.add('toggled'); + break; + case SpreadMode.ODD: + buttons.spreadOddButton.classList.add('toggled'); + break; + case SpreadMode.EVEN: + buttons.spreadEvenButton.classList.add('toggled'); + break; + } }); } From 9b0ed6f821219ee07ca8684b7a99b6dd3b4d03c3 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 21 Jun 2018 18:15:04 +0200 Subject: [PATCH 2/8] Remove all pages from the DOM at once, rather than using a loop, in `PDFViewer._regroupSpreads` There's no good reason to iterate through an arbitrary number of DOM elements this way, since a document could contain thousands of pages, when everything can be easily removed at once; compare with e.g. `BaseViewer._resetView` and `PDFThumbnailViewer._resetView`. Furthermore given that it's a `PDFViewer` instance, the `this.viewer` property can be accessed directly. Besides, `_setDocumentViewerElement` only exists as a helper method for `setDocument` in the base class and none of this code applies for `PDFSinglePageViewer` instances either. --- web/pdf_viewer.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/web/pdf_viewer.js b/web/pdf_viewer.js index 2a6c46ca8..99757087d 100644 --- a/web/pdf_viewer.js +++ b/web/pdf_viewer.js @@ -88,13 +88,13 @@ class PDFViewer extends BaseViewer { } _regroupSpreads() { - const container = this._setDocumentViewerElement, pages = this._pages; - while (container.firstChild) { - container.firstChild.remove(); - } + const viewer = this.viewer, pages = this._pages; + // Temporarily remove all the pages from the DOM. + viewer.textContent = ''; + if (this.spreadMode === SpreadMode.NONE) { for (let i = 0, iMax = pages.length; i < iMax; ++i) { - container.appendChild(pages[i].div); + viewer.appendChild(pages[i].div); } } else { const parity = this.spreadMode - 1; @@ -103,10 +103,10 @@ class PDFViewer extends BaseViewer { if (spread === null) { spread = document.createElement('div'); spread.className = 'spread'; - container.appendChild(spread); + viewer.appendChild(spread); } else if (i % 2 === parity) { spread = spread.cloneNode(false); - container.appendChild(spread); + viewer.appendChild(spread); } spread.appendChild(pages[i].div); } From da52dff04b90e48a2e17d3ebc891bf677c3ed1ec Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 21 Jun 2018 19:53:13 +0200 Subject: [PATCH 3/8] Add validation of the argument in the `BaseViewer.{setScrollMode, setSpreadMode}` methods Since all the other "public" methods validate the arguments, these (new) ones really ought to do the same. --- web/base_viewer.js | 49 +++++++++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 18 deletions(-) diff --git a/web/base_viewer.js b/web/base_viewer.js index 2eee52ac9..2ff541905 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -181,7 +181,9 @@ class BaseViewer { if (this.removePageBorders) { this.viewer.classList.add('removePageBorders'); } - this._updateScrollModeClasses(); + if (this.scrollMode !== ScrollMode.VERTICAL) { + this._updateScrollModeClasses(); + } } get pagesCount() { @@ -1018,20 +1020,26 @@ class BaseViewer { } setScrollMode(mode) { - if (mode !== this.scrollMode) { - this.scrollMode = mode; - this._updateScrollModeClasses(); - this.eventBus.dispatch('scrollmodechanged', { mode, }); - const pageNumber = this._currentPageNumber; - // Non-numeric scale modes can be sensitive to the scroll orientation. - // Call this before re-scrolling to the current page, to ensure that any - // changes in scale don't move the current page. - if (isNaN(this._currentScaleValue)) { - this._setScale(this._currentScaleValue, this.isInPresentationMode); - } - this.scrollPageIntoView({ pageNumber, }); - this.update(); + if (mode === this.scrollMode) { + return; } + if (!Number.isInteger(mode) || !Object.values(ScrollMode).includes(mode)) { + throw new Error(`Invalid scroll mode: ${mode}`); + } + + this.scrollMode = mode; + this._updateScrollModeClasses(); + this.eventBus.dispatch('scrollmodechanged', { mode, }); + + const pageNumber = this._currentPageNumber; + // Non-numeric scale modes can be sensitive to the scroll orientation. + // Call this before re-scrolling to the current page, to ensure that any + // changes in scale don't move the current page. + if (isNaN(this._currentScaleValue)) { + this._setScale(this._currentScaleValue, this.isInPresentationMode); + } + this.scrollPageIntoView({ pageNumber, }); + this.update(); } _updateScrollModeClasses() { @@ -1050,11 +1058,16 @@ class BaseViewer { } setSpreadMode(mode) { - if (mode !== this.spreadMode) { - this.spreadMode = mode; - this.eventBus.dispatch('spreadmodechanged', { mode, }); - this._regroupSpreads(); + if (mode === this.spreadMode) { + return; } + if (!Number.isInteger(mode) || !Object.values(SpreadMode).includes(mode)) { + throw new Error(`Invalid spread mode: ${mode}`); + } + + this.spreadMode = mode; + this.eventBus.dispatch('spreadmodechanged', { mode, }); + this._regroupSpreads(); } _regroupSpreads() {} From 8bd12442985b084dde154dbf7b0f60448c9b26e3 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 21 Jun 2018 20:30:58 +0200 Subject: [PATCH 4/8] Move `_updateScrollModeClasses` from `BaseViewer` to `PDFViewer` Given that this method is a no-op in `PDFSinglePageViewer`, similar to `_regroupSpreads`, let's improve the general code structure by simply moving the method. --- web/base_viewer.js | 17 ++++------------- web/pdf_viewer.js | 15 +++++++++++++++ 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/web/base_viewer.js b/web/base_viewer.js index 2ff541905..11f5432aa 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -1043,18 +1043,7 @@ class BaseViewer { } _updateScrollModeClasses() { - const { scrollMode, viewer, } = this; - - if (scrollMode === ScrollMode.HORIZONTAL) { - viewer.classList.add('scrollHorizontal'); - } else { - viewer.classList.remove('scrollHorizontal'); - } - if (scrollMode === ScrollMode.WRAPPED) { - viewer.classList.add('scrollWrapped'); - } else { - viewer.classList.remove('scrollWrapped'); - } + // No-op in the base class. } setSpreadMode(mode) { @@ -1070,7 +1059,9 @@ class BaseViewer { this._regroupSpreads(); } - _regroupSpreads() {} + _regroupSpreads() { + // No-op in the base class. + } } export { diff --git a/web/pdf_viewer.js b/web/pdf_viewer.js index 99757087d..79c3e055b 100644 --- a/web/pdf_viewer.js +++ b/web/pdf_viewer.js @@ -87,6 +87,21 @@ class PDFViewer extends BaseViewer { }); } + _updateScrollModeClasses() { + const { scrollMode, viewer, } = this; + + if (scrollMode === ScrollMode.HORIZONTAL) { + viewer.classList.add('scrollHorizontal'); + } else { + viewer.classList.remove('scrollHorizontal'); + } + if (scrollMode === ScrollMode.WRAPPED) { + viewer.classList.add('scrollWrapped'); + } else { + viewer.classList.remove('scrollWrapped'); + } + } + _regroupSpreads() { const viewer = this.viewer, pages = this._pages; // Temporarily remove all the pages from the DOM. From 6a086fa0b9431c7f997a8edd666d9616d6d22c8c Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 21 Jun 2018 20:54:57 +0200 Subject: [PATCH 5/8] Refactor `setScrollMode`/`setSpreadMode`, in the viewer classes, such that they are no-ops in `PDFSinglePageViewer` instances Since the Scroll/Spread modes doesn't make (any) sense in `PDFSinglePageViewer` instances, the general structure of these methods can be improved to reflect that. --- web/base_viewer.js | 22 ---------------------- web/pdf_viewer.js | 30 ++++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 22 deletions(-) diff --git a/web/base_viewer.js b/web/base_viewer.js index 11f5432aa..d259ef4d3 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -1020,26 +1020,10 @@ class BaseViewer { } setScrollMode(mode) { - if (mode === this.scrollMode) { - return; - } if (!Number.isInteger(mode) || !Object.values(ScrollMode).includes(mode)) { throw new Error(`Invalid scroll mode: ${mode}`); } - this.scrollMode = mode; - this._updateScrollModeClasses(); - this.eventBus.dispatch('scrollmodechanged', { mode, }); - - const pageNumber = this._currentPageNumber; - // Non-numeric scale modes can be sensitive to the scroll orientation. - // Call this before re-scrolling to the current page, to ensure that any - // changes in scale don't move the current page. - if (isNaN(this._currentScaleValue)) { - this._setScale(this._currentScaleValue, this.isInPresentationMode); - } - this.scrollPageIntoView({ pageNumber, }); - this.update(); } _updateScrollModeClasses() { @@ -1047,16 +1031,10 @@ class BaseViewer { } setSpreadMode(mode) { - if (mode === this.spreadMode) { - return; - } if (!Number.isInteger(mode) || !Object.values(SpreadMode).includes(mode)) { throw new Error(`Invalid spread mode: ${mode}`); } - this.spreadMode = mode; - this.eventBus.dispatch('spreadmodechanged', { mode, }); - this._regroupSpreads(); } _regroupSpreads() { diff --git a/web/pdf_viewer.js b/web/pdf_viewer.js index 79c3e055b..10d11a4c6 100644 --- a/web/pdf_viewer.js +++ b/web/pdf_viewer.js @@ -87,6 +87,26 @@ class PDFViewer extends BaseViewer { }); } + setScrollMode(mode) { + if (mode === this.scrollMode) { + return; + } + super.setScrollMode(mode); + + this.eventBus.dispatch('scrollmodechanged', { mode, }); + this._updateScrollModeClasses(); + + const pageNumber = this._currentPageNumber; + // Non-numeric scale modes can be sensitive to the scroll orientation. + // Call this before re-scrolling to the current page, to ensure that any + // changes in scale don't move the current page. + if (isNaN(this._currentScaleValue)) { + this._setScale(this._currentScaleValue, this.isInPresentationMode); + } + this.scrollPageIntoView({ pageNumber, }); + this.update(); + } + _updateScrollModeClasses() { const { scrollMode, viewer, } = this; @@ -102,6 +122,16 @@ class PDFViewer extends BaseViewer { } } + setSpreadMode(mode) { + if (mode === this.spreadMode) { + return; + } + super.setSpreadMode(mode); + + this.eventBus.dispatch('spreadmodechanged', { mode, }); + this._regroupSpreads(); + } + _regroupSpreads() { const viewer = this.viewer, pages = this._pages; // Temporarily remove all the pages from the DOM. From 05f682cd4be78b7985dd630a708f17ea752a76dd Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 21 Jun 2018 21:45:03 +0200 Subject: [PATCH 6/8] [Regression] Ensure that pre-rendering of the next/previous page works correctly in Presentation Mode, when horizontal scrolling was enabled Note how in `BaseViewer.forceRendering` the Scroll mode is used to determine how pre-rendering will work. Currently this is broken in Presentation Mode, if horizontal scrolling was enabled prior to entering fullscreen. Furthermore, there's a few additional cases where the `this.scrollMode === ScrollMode.HORIZONTAL` check is pointless either in Presentation Mode or when a `PDFSinglePageViewer` instance is used. --- web/base_viewer.js | 18 +++++++++++------- web/pdf_single_page_viewer.js | 5 +++++ web/pdf_viewer.js | 9 ++++++++- 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/web/base_viewer.js b/web/base_viewer.js index d259ef4d3..79c65c9d4 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -597,11 +597,11 @@ class BaseViewer { if (!currentPage) { return; } - let hPadding = (this.isInPresentationMode || this.removePageBorders) ? - 0 : SCROLLBAR_PADDING; - let vPadding = (this.isInPresentationMode || this.removePageBorders) ? - 0 : VERTICAL_PADDING; - if (this.scrollMode === ScrollMode.HORIZONTAL) { + const noPadding = (this.isInPresentationMode || this.removePageBorders); + let hPadding = noPadding ? 0 : SCROLLBAR_PADDING; + let vPadding = noPadding ? 0 : VERTICAL_PADDING; + + if (!noPadding && this._isScrollModeHorizontal) { const temp = hPadding; hPadding = vPadding; vPadding = temp; @@ -834,6 +834,10 @@ class BaseViewer { this.container.focus(); } + get _isScrollModeHorizontal() { + throw new Error('Not implemented: _isScrollModeHorizontal'); + } + get isInPresentationMode() { return this.presentationModeState === PresentationModeState.FULLSCREEN; } @@ -906,8 +910,8 @@ class BaseViewer { forceRendering(currentlyVisiblePages) { let visiblePages = currentlyVisiblePages || this._getVisiblePages(); - let scrollAhead = this.scrollMode === ScrollMode.HORIZONTAL ? - this.scroll.right : this.scroll.down; + let scrollAhead = (this._isScrollModeHorizontal ? + this.scroll.right : this.scroll.down); let pageView = this.renderingQueue.getHighestPriority(visiblePages, this._pages, scrollAhead); diff --git a/web/pdf_single_page_viewer.js b/web/pdf_single_page_viewer.js index 94fb7823e..4a446f457 100644 --- a/web/pdf_single_page_viewer.js +++ b/web/pdf_single_page_viewer.js @@ -142,6 +142,11 @@ class PDFSinglePageViewer extends BaseViewer { location: this._location, }); } + + get _isScrollModeHorizontal() { + // The Scroll/Spread modes are never used in `PDFSinglePageViewer`. + return shadow(this, '_isScrollModeHorizontal', false); + } } export { diff --git a/web/pdf_viewer.js b/web/pdf_viewer.js index 10d11a4c6..78a2ce3a5 100644 --- a/web/pdf_viewer.js +++ b/web/pdf_viewer.js @@ -23,7 +23,7 @@ class PDFViewer extends BaseViewer { } _scrollIntoView({ pageDiv, pageSpot = null, }) { - if (!pageSpot) { + if (!pageSpot && !this.isInPresentationMode) { const left = pageDiv.offsetLeft + pageDiv.clientLeft; const right = left + pageDiv.clientWidth; const { scrollLeft, clientWidth, } = this.container; @@ -87,6 +87,13 @@ class PDFViewer extends BaseViewer { }); } + 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); + } + setScrollMode(mode) { if (mode === this.scrollMode) { return; From d3cb5e7117fbd112b90fb79e92dba630fdb79f29 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 23 Jun 2018 10:30:22 +0200 Subject: [PATCH 7/8] Don't attempt to modify the DOM and/or trigger rendering when changing Scroll/Spread modes without a PDF document being loaded --- web/pdf_viewer.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/web/pdf_viewer.js b/web/pdf_viewer.js index 78a2ce3a5..6e42db779 100644 --- a/web/pdf_viewer.js +++ b/web/pdf_viewer.js @@ -103,6 +103,9 @@ class PDFViewer extends BaseViewer { this.eventBus.dispatch('scrollmodechanged', { mode, }); this._updateScrollModeClasses(); + if (!this.pdfDocument) { + return; + } const pageNumber = this._currentPageNumber; // Non-numeric scale modes can be sensitive to the scroll orientation. // Call this before re-scrolling to the current page, to ensure that any @@ -140,6 +143,9 @@ class PDFViewer extends BaseViewer { } _regroupSpreads() { + if (!this.pdfDocument) { + return; + } const viewer = this.viewer, pages = this._pages; // Temporarily remove all the pages from the DOM. viewer.textContent = ''; From bfbe2b411c3aa0925fa6b0c35a1eee3614c9ddf6 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 23 Jun 2018 10:43:07 +0200 Subject: [PATCH 8/8] Simplify the `_setScale` call when changing Scroll modes Since the current page will be explicitly scrolled into view *directly* afterwards anyway (compare with e.g. the `pagesRotation` code), trying to maintain the current position when re-applying the zoom level during changing of Scroll modes is redundant. --- web/pdf_viewer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/pdf_viewer.js b/web/pdf_viewer.js index 6e42db779..0a7db67ce 100644 --- a/web/pdf_viewer.js +++ b/web/pdf_viewer.js @@ -111,7 +111,7 @@ class PDFViewer extends BaseViewer { // Call this before re-scrolling to the current page, to ensure that any // changes in scale don't move the current page. if (isNaN(this._currentScaleValue)) { - this._setScale(this._currentScaleValue, this.isInPresentationMode); + this._setScale(this._currentScaleValue, true); } this.scrollPageIntoView({ pageNumber, }); this.update();