From 28ce3b61854f433dd7bf231609795279af63f12b Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 14 Jul 2017 14:24:32 +0200 Subject: [PATCH 1/6] Rip out the current implementation of `PDFHistory` The current implementation of `PDFHistory` contains a number of smaller bugs, which are *very* difficult to address without breaking other parts of its code. Possibly the main issue with the current implementation, is that I wrote it quite some time ago, and at the time my understanding of the various edge-cases the code has to deal with was quite limited. Currently `PDFHistory` may, despite most of those cases being fixed, in certain edge-cases lock-up the browser history, essentially preventing the user from navigating back/forward. Hence rather than trying to iterate on `PDFHistory` to make it better, the only viable approach is unfortunately rip it out in its entirety and re-write it from scratch. --- web/app.js | 63 ++---- web/interfaces.js | 9 +- web/pdf_history.js | 417 ++-------------------------------------- web/pdf_link_service.js | 14 -- 4 files changed, 31 insertions(+), 472 deletions(-) diff --git a/web/app.js b/web/app.js index 99103b77e..9eeb542d1 100644 --- a/web/app.js +++ b/web/app.js @@ -89,7 +89,6 @@ const DefaultExternalServices = { let PDFViewerApplication = { initialBookmark: document.location.hash.substring(1), - initialDestination: null, initialized: false, fellback: false, appConfig: null, @@ -931,20 +930,11 @@ let PDFViewerApplication = { if (!PDFJS.disableHistory && !this.isViewerEmbedded) { // The browsing history is only enabled when the viewer is standalone, // i.e. not when it is embedded in a web page. - if (!this.viewerPrefs['showPreviousViewOnLoad']) { - this.pdfHistory.clearHistoryState(); - } - this.pdfHistory.initialize(this.documentFingerprint); - - if (this.pdfHistory.initialDestination) { - this.initialDestination = this.pdfHistory.initialDestination; - } else if (this.pdfHistory.initialBookmark) { - this.initialBookmark = this.pdfHistory.initialBookmark; - } + let resetHistory = !this.viewerPrefs['showPreviousViewOnLoad']; + this.pdfHistory.initialize(id, resetHistory); } let initialParams = { - destination: this.initialDestination, bookmark: this.initialBookmark, hash: null, }; @@ -991,14 +981,12 @@ let PDFViewerApplication = { }).then(() => { // For documents with different page sizes, once all pages are resolved, // ensure that the correct location becomes visible on load. - if (!initialParams.destination && !initialParams.bookmark && - !initialParams.hash) { + if (!initialParams.bookmark && !initialParams.hash) { return; } if (pdfViewer.hasEqualPageSizes) { return; } - this.initialDestination = initialParams.destination; this.initialBookmark = initialParams.bookmark; pdfViewer.currentScaleValue = pdfViewer.currentScaleValue; @@ -1141,12 +1129,8 @@ let PDFViewerApplication = { this.isInitialViewSet = true; this.pdfSidebar.setInitialView(sidebarView); - if (this.initialDestination) { - this.pdfLinkService.navigateTo(this.initialDestination); - this.initialDestination = null; - } else if (this.initialBookmark) { + if (this.initialBookmark) { this.pdfLinkService.setHash(this.initialBookmark); - this.pdfHistory.push({ hash: this.initialBookmark, }, true); this.initialBookmark = null; } else if (storedHash) { this.pdfLinkService.setHash(storedHash); @@ -1787,10 +1771,6 @@ function webViewerUpdateViewarea(evt) { PDFViewerApplication.appConfig.secondaryToolbar.viewBookmarkButton.href = href; - // Update the current bookmark in the browsing history. - PDFViewerApplication.pdfHistory.updateCurrentBookmark(location.pdfOpenParams, - location.pageNumber); - // Show/hide the loading indicator in the page number input element. let currentPage = PDFViewerApplication.pdfViewer.getPageView(PDFViewerApplication.page - 1); @@ -1814,16 +1794,14 @@ function webViewerResize() { } function webViewerHashchange(evt) { - if (PDFViewerApplication.pdfHistory.isHashChangeUnlocked) { - let hash = evt.hash; - if (!hash) { - return; - } - if (!PDFViewerApplication.isInitialViewSet) { - PDFViewerApplication.initialBookmark = hash; - } else { - PDFViewerApplication.pdfLinkService.setHash(hash); - } + let hash = evt.hash; + if (!hash) { + return; + } + if (!PDFViewerApplication.isInitialViewSet) { + PDFViewerApplication.initialBookmark = hash; + } else { + PDFViewerApplication.pdfLinkService.setHash(hash); } } @@ -2277,23 +2255,6 @@ function webViewerKeyDown(evt) { } } - if (cmd === 2) { // alt-key - switch (evt.keyCode) { - case 37: // left arrow - if (isViewerInPresentationMode) { - PDFViewerApplication.pdfHistory.back(); - handled = true; - } - break; - case 39: // right arrow - if (isViewerInPresentationMode) { - PDFViewerApplication.pdfHistory.forward(); - handled = true; - } - break; - } - } - if (ensureViewerFocused && !pdfViewer.containsElement(curElement)) { // The page container is not focused, but a page navigation key has been // pressed. Change the focus to the viewer container to make sure that diff --git a/web/interfaces.js b/web/interfaces.js index 69cd55d5f..48c7b5a9d 100644 --- a/web/interfaces.js +++ b/web/interfaces.js @@ -73,10 +73,13 @@ class IPDFLinkService { * @interface */ class IPDFHistory { - forward() {} + initialize() {} + + push() {} + back() {} - push(params) {} - updateNextHashParam(hash) {} + + forward() {} } /** diff --git a/web/pdf_history.js b/web/pdf_history.js index bcc495880..e4dd680a0 100644 --- a/web/pdf_history.js +++ b/web/pdf_history.js @@ -1,4 +1,4 @@ -/* Copyright 2012 Mozilla Foundation +/* Copyright 2017 Mozilla Foundation * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -12,415 +12,24 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -/* globals chrome */ import { getGlobalEventBus } from './dom_events'; -function PDFHistory(options) { - this.linkService = options.linkService; - this.eventBus = options.eventBus || getGlobalEventBus(); +class PDFHistory { + constructor({ linkService, eventBus, }) { + this.linkService = linkService; + this.eventBus = eventBus || getGlobalEventBus(); + } - this.initialized = false; - this.initialDestination = null; - this.initialBookmark = null; + initialize(fingerprint, resetHistory = false) {} + + push() {} + + back() {} + + forward() {} } -PDFHistory.prototype = { - /** - * @param {string} fingerprint - */ - initialize: function pdfHistoryInitialize(fingerprint) { - this.initialized = true; - this.reInitialized = false; - this.allowHashChange = true; - this.historyUnlocked = true; - this.isViewerInPresentationMode = false; - - this.previousHash = window.location.hash.substring(1); - this.currentBookmark = ''; - this.currentPage = 0; - this.updatePreviousBookmark = false; - this.previousBookmark = ''; - this.previousPage = 0; - this.nextHashParam = ''; - - this.fingerprint = fingerprint; - this.currentUid = this.uid = 0; - this.current = {}; - - var state = window.history.state; - if (this._isStateObjectDefined(state)) { - // This corresponds to navigating back to the document - // from another page in the browser history. - if (state.target.dest) { - this.initialDestination = state.target.dest; - } else { - this.initialBookmark = state.target.hash; - } - this.currentUid = state.uid; - this.uid = state.uid + 1; - this.current = state.target; - } else { - // This corresponds to the loading of a new document. - if (state && state.fingerprint && - this.fingerprint !== state.fingerprint) { - // Reinitialize the browsing history when a new document - // is opened in the web viewer. - this.reInitialized = true; - } - this._pushOrReplaceState({ fingerprint: this.fingerprint, }, true); - } - - var self = this; - window.addEventListener('popstate', function pdfHistoryPopstate(evt) { - if (!self.historyUnlocked) { - return; - } - if (evt.state) { - // Move back/forward in the history. - self._goTo(evt.state); - return; - } - - // If the state is not set, then the user tried to navigate to a - // different hash by manually editing the URL and pressing Enter, or by - // clicking on an in-page link (e.g. the "current view" link). - // Save the current view state to the browser history. - - // Note: In Firefox, history.null could also be null after an in-page - // navigation to the same URL, and without dispatching the popstate - // event: https://bugzilla.mozilla.org/show_bug.cgi?id=1183881 - - if (self.uid === 0) { - // Replace the previous state if it was not explicitly set. - var previousParams = (self.previousHash && self.currentBookmark && - self.previousHash !== self.currentBookmark) ? - { hash: self.currentBookmark, page: self.currentPage, } : - { page: 1, }; - replacePreviousHistoryState(previousParams, function() { - updateHistoryWithCurrentHash(); - }); - } else { - updateHistoryWithCurrentHash(); - } - }); - - - function updateHistoryWithCurrentHash() { - self.previousHash = window.location.hash.slice(1); - self._pushToHistory({ hash: self.previousHash, }, false, true); - self._updatePreviousBookmark(); - } - - function replacePreviousHistoryState(params, callback) { - // To modify the previous history entry, the following happens: - // 1. history.back() - // 2. _pushToHistory, which calls history.replaceState( ... ) - // 3. history.forward() - // Because a navigation via the history API does not immediately update - // the history state, the popstate event is used for synchronization. - self.historyUnlocked = false; - - // Suppress the hashchange event to avoid side effects caused by - // navigating back and forward. - self.allowHashChange = false; - window.addEventListener('popstate', rewriteHistoryAfterBack); - history.back(); - - function rewriteHistoryAfterBack() { - window.removeEventListener('popstate', rewriteHistoryAfterBack); - window.addEventListener('popstate', rewriteHistoryAfterForward); - self._pushToHistory(params, false, true); - history.forward(); - } - function rewriteHistoryAfterForward() { - window.removeEventListener('popstate', rewriteHistoryAfterForward); - self.allowHashChange = true; - self.historyUnlocked = true; - callback(); - } - } - - function pdfHistoryBeforeUnload() { - var previousParams = self._getPreviousParams(null, true); - if (previousParams) { - var replacePrevious = (!self.current.dest && - self.current.hash !== self.previousHash); - self._pushToHistory(previousParams, false, replacePrevious); - self._updatePreviousBookmark(); - } - // Remove the event listener when navigating away from the document, - // since 'beforeunload' prevents Firefox from caching the document. - window.removeEventListener('beforeunload', pdfHistoryBeforeUnload); - } - - window.addEventListener('beforeunload', pdfHistoryBeforeUnload); - - window.addEventListener('pageshow', function pdfHistoryPageShow(evt) { - // If the entire viewer (including the PDF file) is cached in - // the browser, we need to reattach the 'beforeunload' event listener - // since the 'DOMContentLoaded' event is not fired on 'pageshow'. - window.addEventListener('beforeunload', pdfHistoryBeforeUnload); - }); - - self.eventBus.on('presentationmodechanged', function(e) { - self.isViewerInPresentationMode = e.active; - }); - }, - - clearHistoryState: function pdfHistory_clearHistoryState() { - this._pushOrReplaceState(null, true); - }, - - _isStateObjectDefined: function pdfHistory_isStateObjectDefined(state) { - return (state && state.uid >= 0 && - state.fingerprint && this.fingerprint === state.fingerprint && - state.target && state.target.hash) ? true : false; - }, - - _pushOrReplaceState: function pdfHistory_pushOrReplaceState(stateObj, - replace) { - // history.state.chromecomState is managed by chromecom.js. - if (typeof PDFJSDev !== 'undefined' && PDFJSDev.test('CHROME') && - window.history.state && 'chromecomState' in window.history.state) { - stateObj = stateObj || {}; - stateObj.chromecomState = window.history.state.chromecomState; - } - if (replace) { - if (typeof PDFJSDev === 'undefined' || - PDFJSDev.test('GENERIC || CHROME')) { - window.history.replaceState(stateObj, '', document.URL); - } else { - window.history.replaceState(stateObj, ''); - } - } else { - if (typeof PDFJSDev === 'undefined' || - PDFJSDev.test('GENERIC || CHROME')) { - window.history.pushState(stateObj, '', document.URL); - } else { - window.history.pushState(stateObj, ''); - } - if (typeof PDFJSDev !== 'undefined' && PDFJSDev.test('CHROME') && - top === window) { - chrome.runtime.sendMessage('showPageAction'); - } - } - }, - - get isHashChangeUnlocked() { - if (!this.initialized) { - return true; - } - return this.allowHashChange; - }, - - _updatePreviousBookmark: function pdfHistory_updatePreviousBookmark() { - if (this.updatePreviousBookmark && - this.currentBookmark && this.currentPage) { - this.previousBookmark = this.currentBookmark; - this.previousPage = this.currentPage; - this.updatePreviousBookmark = false; - } - }, - - updateCurrentBookmark: function pdfHistoryUpdateCurrentBookmark(bookmark, - pageNum) { - if (this.initialized) { - this.currentBookmark = bookmark.substring(1); - this.currentPage = pageNum | 0; - this._updatePreviousBookmark(); - } - }, - - updateNextHashParam: function pdfHistoryUpdateNextHashParam(param) { - if (this.initialized) { - this.nextHashParam = param; - } - }, - - push: function pdfHistoryPush(params, isInitialBookmark) { - if (!(this.initialized && this.historyUnlocked)) { - return; - } - if (params.dest && !params.hash) { - params.hash = (this.current.hash && this.current.dest && - this.current.dest === params.dest) ? - this.current.hash : - this.linkService.getDestinationHash(params.dest).split('#')[1]; - } - if (params.page) { - params.page |= 0; - } - if (isInitialBookmark) { - var target = window.history.state.target; - if (!target) { - // Invoked when the user specifies an initial bookmark, - // thus setting initialBookmark, when the document is loaded. - this._pushToHistory(params, false); - this.previousHash = window.location.hash.substring(1); - } - this.updatePreviousBookmark = this.nextHashParam ? false : true; - if (target) { - // If the current document is reloaded, - // avoid creating duplicate entries in the history. - this._updatePreviousBookmark(); - } - return; - } - if (this.nextHashParam) { - if (this.nextHashParam === params.hash) { - this.nextHashParam = null; - this.updatePreviousBookmark = true; - return; - } - this.nextHashParam = null; - } - - if (params.hash) { - if (this.current.hash) { - if (this.current.hash !== params.hash) { - this._pushToHistory(params, true); - } else { - if (!this.current.page && params.page) { - this._pushToHistory(params, false, true); - } - this.updatePreviousBookmark = true; - } - } else { - this._pushToHistory(params, true); - } - } else if (this.current.page && params.page && - this.current.page !== params.page) { - this._pushToHistory(params, true); - } - }, - - _getPreviousParams: function pdfHistory_getPreviousParams(onlyCheckPage, - beforeUnload) { - if (!(this.currentBookmark && this.currentPage)) { - return null; - } else if (this.updatePreviousBookmark) { - this.updatePreviousBookmark = false; - } - if (this.uid > 0 && !(this.previousBookmark && this.previousPage)) { - // Prevent the history from getting stuck in the current state, - // effectively preventing the user from going back/forward in - // the history. - // - // This happens if the current position in the document didn't change - // when the history was previously updated. The reasons for this are - // either: - // 1. The current zoom value is such that the document does not need to, - // or cannot, be scrolled to display the destination. - // 2. The previous destination is broken, and doesn't actally point to a - // position within the document. - // (This is either due to a bad PDF generator, or the user making a - // mistake when entering a destination in the hash parameters.) - return null; - } - if ((!this.current.dest && !onlyCheckPage) || beforeUnload) { - if (this.previousBookmark === this.currentBookmark) { - return null; - } - } else if (this.current.page || onlyCheckPage) { - if (this.previousPage === this.currentPage) { - return null; - } - } else { - return null; - } - var params = { hash: this.currentBookmark, page: this.currentPage, }; - if (this.isViewerInPresentationMode) { - params.hash = null; - } - return params; - }, - - _stateObj: function pdfHistory_stateObj(params) { - return { fingerprint: this.fingerprint, uid: this.uid, target: params, }; - }, - - _pushToHistory: function pdfHistory_pushToHistory(params, - addPrevious, overwrite) { - if (!this.initialized) { - return; - } - if (!params.hash && params.page) { - params.hash = ('page=' + params.page); - } - if (addPrevious && !overwrite) { - var previousParams = this._getPreviousParams(); - if (previousParams) { - var replacePrevious = (!this.current.dest && - this.current.hash !== this.previousHash); - this._pushToHistory(previousParams, false, replacePrevious); - } - } - this._pushOrReplaceState(this._stateObj(params), - (overwrite || this.uid === 0)); - this.currentUid = this.uid++; - this.current = params; - this.updatePreviousBookmark = true; - }, - - _goTo: function pdfHistory_goTo(state) { - if (!(this.initialized && this.historyUnlocked && - this._isStateObjectDefined(state))) { - return; - } - if (!this.reInitialized && state.uid < this.currentUid) { - var previousParams = this._getPreviousParams(true); - if (previousParams) { - this._pushToHistory(this.current, false); - this._pushToHistory(previousParams, false); - this.currentUid = state.uid; - window.history.back(); - return; - } - } - this.historyUnlocked = false; - - if (state.target.dest) { - this.linkService.navigateTo(state.target.dest); - } else { - this.linkService.setHash(state.target.hash); - } - this.currentUid = state.uid; - if (state.uid > this.uid) { - this.uid = state.uid; - } - this.current = state.target; - this.updatePreviousBookmark = true; - - var currentHash = window.location.hash.substring(1); - if (this.previousHash !== currentHash) { - this.allowHashChange = false; - } - this.previousHash = currentHash; - - this.historyUnlocked = true; - }, - - back: function pdfHistoryBack() { - this.go(-1); - }, - - forward: function pdfHistoryForward() { - this.go(1); - }, - - go: function pdfHistoryGo(direction) { - if (this.initialized && this.historyUnlocked) { - var state = window.history.state; - if (direction === -1 && state && state.uid > 0) { - window.history.back(); - } else if (direction === 1 && state && state.uid < (this.uid - 1)) { - window.history.forward(); - } - } - }, -}; - export { PDFHistory, }; diff --git a/web/pdf_link_service.js b/web/pdf_link_service.js index cb592cc9b..79f5e33fa 100644 --- a/web/pdf_link_service.js +++ b/web/pdf_link_service.js @@ -115,14 +115,6 @@ class PDFLinkService { pageNumber, destArray: explicitDest, }); - - if (this.pdfHistory) { // Update the browsing history, if enabled. - this.pdfHistory.push({ - dest: explicitDest, - hash: namedDest, - page: pageNumber, - }); - } }; new Promise((resolve, reject) => { @@ -190,9 +182,6 @@ class PDFLinkService { } // borrowing syntax from "Parameters for Opening PDF Files" if ('nameddest' in params) { - if (this.pdfHistory) { - this.pdfHistory.updateNextHashParam(params.nameddest); - } this.navigateTo(params.nameddest); return; } @@ -270,9 +259,6 @@ class PDFLinkService { } catch (ex) {} if (typeof dest === 'string' || isValidExplicitDestination(dest)) { - if (this.pdfHistory) { - this.pdfHistory.updateNextHashParam(dest); - } this.navigateTo(dest); return; } From 0c4985546a9ebc3d5849fd564267899c355ab15e Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 12 Aug 2017 16:06:20 +0200 Subject: [PATCH 2/6] Add a `waitOnEventOrTimeout` helper function that allows waiting for an event or a timeout, whichever occurs first --- test/unit/ui_utils_spec.js | 117 ++++++++++++++++++++++++++++++++++++- web/ui_utils.js | 60 ++++++++++++++++++- 2 files changed, 175 insertions(+), 2 deletions(-) diff --git a/test/unit/ui_utils_spec.js b/test/unit/ui_utils_spec.js index 44517a91a..58f97e9d1 100644 --- a/test/unit/ui_utils_spec.js +++ b/test/unit/ui_utils_spec.js @@ -14,7 +14,8 @@ */ import { - binarySearchFirstItem, EventBus, getPDFFileNameFromURL + binarySearchFirstItem, EventBus, getPDFFileNameFromURL, waitOnEventOrTimeout, + WaitOnType } from '../../web/ui_utils'; import { createObjectURL, isNodeJS } from '../../src/shared/util'; @@ -259,4 +260,118 @@ describe('ui_utils', function() { expect(count).toEqual(2); }); }); + + describe('waitOnEventOrTimeout', function() { + let eventBus; + + beforeAll(function(done) { + eventBus = new EventBus(); + done(); + }); + + afterAll(function() { + eventBus = null; + }); + + it('should reject invalid parameters', function(done) { + let invalidTarget = waitOnEventOrTimeout({ + target: 'window', + name: 'DOMContentLoaded', + }).then(function() { + throw new Error('Should reject invalid parameters.'); + }, function(reason) { + expect(reason instanceof Error).toEqual(true); + }); + + let invalidName = waitOnEventOrTimeout({ + target: eventBus, + name: '', + }).then(function() { + throw new Error('Should reject invalid parameters.'); + }, function(reason) { + expect(reason instanceof Error).toEqual(true); + }); + + let invalidDelay = waitOnEventOrTimeout({ + target: eventBus, + name: 'pagerendered', + delay: -1000, + }).then(function() { + throw new Error('Should reject invalid parameters.'); + }, function(reason) { + expect(reason instanceof Error).toEqual(true); + }); + + Promise.all([invalidTarget, invalidName, invalidDelay]).then(done, + done.fail); + }); + + it('should resolve on event, using the DOM', function(done) { + if (isNodeJS()) { + pending('Document in not supported in Node.js.'); + } + let button = document.createElement('button'); + + let buttonClicked = waitOnEventOrTimeout({ + target: button, + name: 'click', + delay: 10000, + }); + // Immediately dispatch the expected event. + button.click(); + + buttonClicked.then(function(type) { + expect(type).toEqual(WaitOnType.EVENT); + done(); + }, done.fail); + }); + + it('should resolve on timeout, using the DOM', function(done) { + if (isNodeJS()) { + pending('Document in not supported in Node.js.'); + } + let button = document.createElement('button'); + + let buttonClicked = waitOnEventOrTimeout({ + target: button, + name: 'click', + delay: 10, + }); + // Do *not* dispatch the event, and wait for the timeout. + + buttonClicked.then(function(type) { + expect(type).toEqual(WaitOnType.TIMEOUT); + done(); + }, done.fail); + }); + + it('should resolve on event, using the EventBus', function(done) { + let pageRendered = waitOnEventOrTimeout({ + target: eventBus, + name: 'pagerendered', + delay: 10000, + }); + // Immediately dispatch the expected event. + eventBus.dispatch('pagerendered'); + + pageRendered.then(function(type) { + expect(type).toEqual(WaitOnType.EVENT); + done(); + }, done.fail); + }); + + it('should resolve on timeout, using the EventBus', function(done) { + let pageRendered = waitOnEventOrTimeout({ + target: eventBus, + name: 'pagerendered', + delay: 10, + }); + // Do *not* dispatch the event, and wait for the timeout. + + pageRendered.then(function(type) { + expect(type).toEqual(WaitOnType.TIMEOUT); + done(); + }, done.fail); + }); + }); }); diff --git a/web/ui_utils.js b/web/ui_utils.js index df6702ca4..a670d6702 100644 --- a/web/ui_utils.js +++ b/web/ui_utils.js @@ -13,7 +13,7 @@ * limitations under the License. */ -import { PDFJS } from 'pdfjs-lib'; +import { createPromiseCapability, PDFJS } from 'pdfjs-lib'; const CSS_UNITS = 96.0 / 72.0; const DEFAULT_SCALE_VALUE = 'auto'; @@ -453,6 +453,62 @@ function cloneObj(obj) { return result; } +const WaitOnType = { + EVENT: 'event', + TIMEOUT: 'timeout', +}; + +/** + * @typedef {Object} WaitOnEventOrTimeoutParameters + * @property {Object} target - The event target, can for example be: + * `window`, `document`, a DOM element, or an {EventBus} instance. + * @property {string} name - The name of the event. + * @property {number} delay - The delay, in milliseconds, after which the + * timeout occurs (if the event wasn't already dispatched). + */ + +/** + * Allows waiting for an event or a timeout, whichever occurs first. + * Can be used to ensure that an action always occurs, even when an event + * arrives late or not at all. + * + * @param {WaitOnEventOrTimeoutParameters} + * @returns {Promise} A promise that is resolved with a {WaitOnType} value. + */ +function waitOnEventOrTimeout({ target, name, delay = 0, }) { + if (typeof target !== 'object' || !(name && typeof name === 'string') || + !(Number.isInteger(delay) && delay >= 0)) { + return Promise.reject( + new Error('waitOnEventOrTimeout - invalid paramaters.')); + } + let capability = createPromiseCapability(); + + function handler(type) { + if (target instanceof EventBus) { + target.off(name, eventHandler); + } else { + target.removeEventListener(name, eventHandler); + } + + if (timeout) { + clearTimeout(timeout); + } + capability.resolve(type); + } + + let eventHandler = handler.bind(null, WaitOnType.EVENT); + if (target instanceof EventBus) { + target.on(name, eventHandler); + } else { + target.addEventListener(name, eventHandler); + } + + let timeoutHandler = handler.bind(null, WaitOnType.TIMEOUT); + let timeout = setTimeout(timeoutHandler, delay); + + return capability.promise; +} + /** * Promise that is resolved when DOM window becomes visible. */ @@ -618,4 +674,6 @@ export { normalizeWheelEventDelta, animationStarted, localized, + WaitOnType, + waitOnEventOrTimeout, }; From 388851e37b0492442a95d0dcdfde545bb2bcbf75 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sun, 13 Aug 2017 17:11:49 +0200 Subject: [PATCH 3/6] Add a `isDestsEqual` helper function, to allow comparing explicit destinations, in `pdf_history.js` --- test/unit/clitests.json | 1 + test/unit/jasmine-boot.js | 1 + test/unit/pdf_history_spec.js | 45 +++++++++++++++++++++++++++++++++++ web/pdf_history.js | 37 ++++++++++++++++++++++++++++ 4 files changed, 84 insertions(+) create mode 100644 test/unit/pdf_history_spec.js diff --git a/test/unit/clitests.json b/test/unit/clitests.json index 3c98a6f0a..46b115ada 100644 --- a/test/unit/clitests.json +++ b/test/unit/clitests.json @@ -17,6 +17,7 @@ "murmurhash3_spec.js", "node_stream_spec.js", "parser_spec.js", + "pdf_history.js", "primitives_spec.js", "stream_spec.js", "type1_parser_spec.js", diff --git a/test/unit/jasmine-boot.js b/test/unit/jasmine-boot.js index 7dfa7321d..1405f9990 100644 --- a/test/unit/jasmine-boot.js +++ b/test/unit/jasmine-boot.js @@ -64,6 +64,7 @@ function initializePDFJS(callback) { 'pdfjs-test/unit/murmurhash3_spec', 'pdfjs-test/unit/network_spec', 'pdfjs-test/unit/parser_spec', + 'pdfjs-test/unit/pdf_history_spec', 'pdfjs-test/unit/primitives_spec', 'pdfjs-test/unit/stream_spec', 'pdfjs-test/unit/type1_parser_spec', diff --git a/test/unit/pdf_history_spec.js b/test/unit/pdf_history_spec.js new file mode 100644 index 000000000..1b2930e18 --- /dev/null +++ b/test/unit/pdf_history_spec.js @@ -0,0 +1,45 @@ +/* Copyright 2017 Mozilla Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { isDestsEqual } from '../../web/pdf_history'; + +describe('pdf_history', function() { + describe('isDestsEqual', function() { + let firstDest = [{ num: 1, gen: 0, }, { name: 'XYZ', }, 0, 375, null]; + let secondDest = [{ num: 5, gen: 0, }, { name: 'XYZ', }, 0, 375, null]; + let thirdDest = [{ num: 1, gen: 0, }, { name: 'XYZ', }, 750, 0, null]; + let fourthDest = [{ num: 1, gen: 0, }, { name: 'XYZ', }, 0, 375, 1.0]; + let fifthDest = [{ gen: 0, num: 1, }, { name: 'XYZ', }, 0, 375, null]; + + it('should reject non-equal destination arrays', function() { + expect(isDestsEqual(firstDest, undefined)).toEqual(false); + expect(isDestsEqual(firstDest, [1, 2, 3, 4, 5])).toEqual(false); + + expect(isDestsEqual(firstDest, secondDest)).toEqual(false); + expect(isDestsEqual(firstDest, thirdDest)).toEqual(false); + expect(isDestsEqual(firstDest, fourthDest)).toEqual(false); + }); + + it('should accept equal destination arrays', function() { + expect(isDestsEqual(firstDest, firstDest)).toEqual(true); + expect(isDestsEqual(firstDest, fifthDest)).toEqual(true); + + let firstDestCopy = firstDest.slice(); + expect(firstDest).not.toBe(firstDestCopy); + + expect(isDestsEqual(firstDest, firstDestCopy)).toEqual(true); + }); + }); +}); diff --git a/web/pdf_history.js b/web/pdf_history.js index e4dd680a0..ef1a7b33a 100644 --- a/web/pdf_history.js +++ b/web/pdf_history.js @@ -30,6 +30,43 @@ class PDFHistory { forward() {} } +function isDestsEqual(firstDest, secondDest) { + function isEntryEqual(first, second) { + if (typeof first !== typeof second) { + return false; + } + if (first instanceof Array || second instanceof Array) { + return false; + } + if (first !== null && typeof first === 'object' && second !== null) { + if (Object.keys(first).length !== Object.keys(second).length) { + return false; + } + for (var key in first) { + if (!isEntryEqual(first[key], second[key])) { + return false; + } + } + return true; + } + return first === second || (Number.isNaN(first) && Number.isNaN(second)); + } + + if (!(firstDest instanceof Array && secondDest instanceof Array)) { + return false; + } + if (firstDest.length !== secondDest.length) { + return false; + } + for (let i = 0, ii = firstDest.length; i < ii; i++) { + if (!isEntryEqual(firstDest[i], secondDest[i])) { + return false; + } + } + return true; +} + export { PDFHistory, + isDestsEqual, }; From 133d06e9a492e24c25336bf895a52d3a703d2385 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sun, 16 Jul 2017 13:39:39 +0200 Subject: [PATCH 4/6] Re-write `PDFHistory` from scratch This patch completely re-implements `PDFHistory` to get rid of various bugs currently present, and to hopefully make maintenance slightly easier. Most of the interface is similar to the existing one, but it should be somewhat simplified. The new implementation should be more robust against failure, compared to the old one. Previously, it was too easy to end up in a state which basically caused the browser history to lock-up, preventing the user from navigating back/forward. (In the new implementation, the browser history should not be updated rather than breaking if things go wrong.) Given that the code has to deal with various edge-cases, it's still not as simple as I would have liked, but it should now be somewhat easier to deal with. The main source of complication in the code is actually that we allow the user to change the hash of a already loaded document (we'll no longer try to navigate back-and-forth in this case, since the next commit contains a workaround). In the new code, there's also *a lot* more comments (perhaps too many?) to attempt to explain the logic. This is something that the old implementation was serverly lacking, which is a one of the reasons why it was so difficult to maintain. One particular thing to note is that the new code uses the `pagehide` event rather than `beforeunload`, since the latter seems to be a bad idea based on https://bugzilla.mozilla.org/show_bug.cgi?id=1336763. --- web/app.js | 12 +- web/interfaces.js | 13 +- web/pdf_history.js | 448 +++++++++++++++++++++++++++++++++++++++- web/pdf_link_service.js | 7 + 4 files changed, 471 insertions(+), 9 deletions(-) diff --git a/web/app.js b/web/app.js index 9eeb542d1..caeeb37da 100644 --- a/web/app.js +++ b/web/app.js @@ -932,10 +932,14 @@ let PDFViewerApplication = { // i.e. not when it is embedded in a web page. let resetHistory = !this.viewerPrefs['showPreviousViewOnLoad']; this.pdfHistory.initialize(id, resetHistory); + + if (this.pdfHistory.initialBookmark) { + this.initialBookmark = this.pdfHistory.initialBookmark; + } } let initialParams = { - bookmark: this.initialBookmark, + bookmark: null, hash: null, }; let storePromise = store.getMultiple({ @@ -969,9 +973,11 @@ let PDFViewerApplication = { sidebarView, }; }).then(({ hash, sidebarView, }) => { - this.setInitialView(hash, { sidebarView, }); + initialParams.bookmark = this.initialBookmark; initialParams.hash = hash; + this.setInitialView(hash, { sidebarView, }); + // Make all navigation keys work on document load, // unless the viewer is embedded in a web page. if (!this.isViewerEmbedded) { @@ -1800,7 +1806,7 @@ function webViewerHashchange(evt) { } if (!PDFViewerApplication.isInitialViewSet) { PDFViewerApplication.initialBookmark = hash; - } else { + } else if (!PDFViewerApplication.pdfHistory.popStateInProgress) { PDFViewerApplication.pdfLinkService.setHash(hash); } } diff --git a/web/interfaces.js b/web/interfaces.js index 48c7b5a9d..04c6d97ed 100644 --- a/web/interfaces.js +++ b/web/interfaces.js @@ -73,9 +73,18 @@ class IPDFLinkService { * @interface */ class IPDFHistory { - initialize() {} + /** + * @param {string} fingerprint - The PDF document's unique fingerprint. + * @param {boolean} resetHistory - (optional) Reset the browsing history. + */ + initialize(fingerprint, resetHistory = false) {} - push() {} + /** + * @param {Object} params + */ + push({ namedDest, explicitDest, pageNumber, }) {} + + pushCurrentPosition() {} back() {} diff --git a/web/pdf_history.js b/web/pdf_history.js index ef1a7b33a..45616b1de 100644 --- a/web/pdf_history.js +++ b/web/pdf_history.js @@ -13,21 +13,461 @@ * limitations under the License. */ +import { parseQueryString, waitOnEventOrTimeout } from './ui_utils'; import { getGlobalEventBus } from './dom_events'; +// Heuristic value used when force-resetting `this._blockHashChange`. +const HASH_CHANGE_TIMEOUT = 1000; // milliseconds + +/** + * @typedef {Object} PDFHistoryOptions + * @property {IPDFLinkService} linkService - The navigation/linking service. + * @property {EventBus} eventBus - The application event bus. + */ + +/** + * @typedef {Object} PushParameters + * @property {string} namedDest - (optional) The named destination. If absent, + * a stringified version of `explicitDest` is used. + * @property {Array} explicitDest - The explicit destination array. + * @property {number} pageNumber - The page to which the destination points. + */ + +function getCurrentHash() { + return document.location.hash; +} + +function parseCurrentHash(linkService) { + let hash = unescape(getCurrentHash()).substring(1); + let params = parseQueryString(hash); + + let page = params.page | 0; + if (!(Number.isInteger(page) && page > 0 && page <= linkService.pagesCount)) { + page = null; + } + return { hash, page, }; +} + class PDFHistory { + /** + * @param {PDFHistoryOptions} options + */ constructor({ linkService, eventBus, }) { this.linkService = linkService; this.eventBus = eventBus || getGlobalEventBus(); + + this.initialized = false; + this.initialBookmark = null; + + this._boundEvents = Object.create(null); + this._isViewerInPresentationMode = false; + + // Ensure that we don't miss a 'presentationmodechanged' event, by + // registering the listener immediately. + this.eventBus.on('presentationmodechanged', (evt) => { + this._isViewerInPresentationMode = evt.active || evt.switchInProgress; + }); } - initialize(fingerprint, resetHistory = false) {} + /** + * Initialize the history for the PDF document, using either the current + * browser history entry or the document hash, whichever is present. + * @param {string} fingerprint - The PDF document's unique fingerprint. + * @param {boolean} resetHistory - (optional) Reset the browsing history. + */ + initialize(fingerprint, resetHistory = false) { + if (!fingerprint || typeof fingerprint !== 'string') { + console.error( + 'PDFHistory.initialize: The "fingerprint" must be a non-empty string.'); + return; + } + let reInitialized = this.initialized && this.fingerprint !== fingerprint; + this.fingerprint = fingerprint; - push() {} + if (!this.initialized) { + this._bindEvents(); + } + let state = window.history.state; - back() {} + this.initialized = true; + this.initialBookmark = null; - forward() {} + this._popStateInProgress = false; + this._blockHashChange = 0; + this._currentHash = getCurrentHash(); + + this._currentUid = this._uid = 0; + this._destination = null; + this._position = null; + + if (!this._isValidState(state) || resetHistory) { + let { hash, page, } = parseCurrentHash(this.linkService); + + if (!hash || reInitialized || resetHistory) { + // Ensure that the browser history is reset on PDF document load. + this._pushOrReplaceState(null, /* forceReplace = */ true); + return; + } + // Ensure that the browser history is initialized correctly when + // the document hash is present on PDF document load. + this._pushOrReplaceState({ hash, page, }, /* forceReplace = */ true); + return; + } + + // The browser history contains a valid entry, ensure that the history is + // initialized correctly on PDF document load. + let destination = state.destination; + this._updateInternalState(destination, state.uid); + + if (destination.dest) { + this.initialBookmark = JSON.stringify(destination.dest); + + // If the history is updated, e.g. through the user changing the hash, + // before the initial destination has become visible, then we do *not* + // want to potentially add `this._position` to the browser history. + this._destination.page = null; + } else if (destination.hash) { + this.initialBookmark = destination.hash; + } else if (destination.page) { + // Fallback case; shouldn't be necessary, but better safe than sorry. + this.initialBookmark = `page=${destination.page}`; + } + } + + /** + * Push an internal destination to the browser history. + * @param {PushParameters} + */ + push({ namedDest, explicitDest, pageNumber, }) { + if (!this.initialized) { + return; + } + if ((namedDest && typeof namedDest !== 'string') || + !(explicitDest instanceof Array) || + !(Number.isInteger(pageNumber) && + pageNumber > 0 && pageNumber <= this.linkService.pagesCount)) { + console.error('PDFHistory.push: Invalid parameters.'); + return; + } + + let hash = namedDest || JSON.stringify(explicitDest); + if (!hash) { + // The hash *should* never be undefined, but if that were to occur, + // avoid any possible issues by not updating the browser history. + return; + } + + let forceReplace = false; + if (this._destination && + (this._destination.hash === hash || + isDestsEqual(this._destination.dest, explicitDest))) { + // When the new destination is identical to `this._destination`, and + // its `page` is undefined, replace the current browser history entry. + // NOTE: This can only occur if `this._destination` was set either: + // - through the document hash being specified on load. + // - through the user changing the hash of the document. + if (this._destination.page) { + return; + } + forceReplace = true; + } + if (this._popStateInProgress && !forceReplace) { + return; + } + + this._pushOrReplaceState({ + dest: explicitDest, + hash, + page: pageNumber, + }, forceReplace); + } + + /** + * Push the current position to the browser history. + */ + pushCurrentPosition() { + if (!this.initialized || this._popStateInProgress) { + return; + } + this._tryPushCurrentPosition(); + } + + /** + * Go back one step in the browser history. + * NOTE: Avoids navigating away from the document, useful for "named actions". + */ + back() { + if (!this.initialized || this._popStateInProgress) { + return; + } + let state = window.history.state; + if (this._isValidState(state) && state.uid > 0) { + window.history.back(); + } + } + + /** + * Go forward one step in the browser history. + * NOTE: Avoids navigating away from the document, useful for "named actions". + */ + forward() { + if (!this.initialized || this._popStateInProgress) { + return; + } + let state = window.history.state; + if (this._isValidState(state) && state.uid < (this._uid - 1)) { + window.history.forward(); + } + } + + /** + * @returns {boolean} Indicating if the user is currently moving through the + * browser history, useful e.g. for skipping the next 'hashchange' event. + */ + get popStateInProgress() { + return this.initialized && + (this._popStateInProgress || this._blockHashChange > 0); + } + + /** + * @private + */ + _pushOrReplaceState(destination, forceReplace = false) { + let shouldReplace = forceReplace || !this._destination; + let newState = { + fingerprint: this.fingerprint, + uid: shouldReplace ? this._currentUid : this._uid, + destination, + }; + + if (typeof PDFJSDev !== 'undefined' && PDFJSDev.test('CHROME') && + window.history.state && window.history.state.chromecomState) { + // history.state.chromecomState is managed by chromecom.js. + newState.chromecomState = window.history.state.chromecomState; + } + this._updateInternalState(destination, newState.uid); + + if (shouldReplace) { + if (typeof PDFJSDev !== 'undefined' && + PDFJSDev.test('FIREFOX || MOZCENTRAL')) { + // Providing the third argument causes a SecurityError for file:// URLs. + window.history.replaceState(newState, ''); + } else { + window.history.replaceState(newState, '', document.URL); + } + } else { + if (typeof PDFJSDev !== 'undefined' && + PDFJSDev.test('FIREFOX || MOZCENTRAL')) { + // Providing the third argument causes a SecurityError for file:// URLs. + window.history.pushState(newState, ''); + } else { + window.history.pushState(newState, '', document.URL); + } + } + + if (typeof PDFJSDev !== 'undefined' && PDFJSDev.test('CHROME') && + top === window) { + // eslint-disable-next-line no-undef + chrome.runtime.sendMessage('showPageAction'); + } + } + + /** + * @private + */ + _tryPushCurrentPosition() { + if (!this._position) { + return; + } + let position = this._position; + + if (!this._destination) { + this._pushOrReplaceState(position); + return; + } + if (this._destination.hash === position.hash) { + return; // The current document position has not changed. + } + if (!this._destination.page) { + // `this._destination` was set through the user changing the hash of + // the document. Do not add `this._position` to the browser history, + // to avoid "flooding" it with lots of (nearly) identical entries, + // since we cannot ensure that the document position has changed. + return; + } + + let forceReplace = false; + if (this._destination.page === position.first || + this._destination.page === position.page) { + // When the `page` of `this._destination` is still visible, do not + // update the browsing history when `this._destination` either: + // - contains an internal destination, since in this case we + // cannot ensure that the document position has actually changed. + // - was set through the user changing the hash of the document. + if (this._destination.dest || !this._destination.first) { + return; + } + // To avoid "flooding" the browser history, replace the current entry. + forceReplace = true; + } + this._pushOrReplaceState(position, forceReplace); + } + + /** + * @private + */ + _isValidState(state) { + if (!state) { + return false; + } + if (state.fingerprint !== this.fingerprint) { + // This should only occur in viewers with support for opening more than + // one PDF document, e.g. the GENERIC viewer. + return false; + } + if (!Number.isInteger(state.uid) || state.uid < 0) { + return false; + } + if (state.destination === null || typeof state.destination !== 'object') { + return false; + } + return true; + } + + /** + * @private + */ + _updateInternalState(destination, uid) { + this._destination = destination; + this._currentUid = uid; + this._uid = this._currentUid + 1; + } + + /** + * @private + */ + _updateViewarea({ location, }) { + this._position = { + hash: this._isViewerInPresentationMode ? + `page=${location.pageNumber}` : location.pdfOpenParams.substring(1), + page: this.linkService.page, + first: location.pageNumber, + }; + + if (this._popStateInProgress) { + return; + } + } + + /** + * @private + */ + _popState({ state, }) { + let newHash = getCurrentHash(), hashChanged = this._currentHash !== newHash; + this._currentHash = newHash; + + if (!state || + (typeof PDFJSDev !== 'undefined' && PDFJSDev.test('CHROME') && + state.chromecomState && !this._isValidState(state))) { + // This case corresponds to the user changing the hash of the document. + this._currentUid = this._uid; + + let { hash, page, } = parseCurrentHash(this.linkService); + this._pushOrReplaceState({ hash, page, }, /* forceReplace */ true); + return; + } + if (!this._isValidState(state)) { + // This should only occur in viewers with support for opening more than + // one PDF document, e.g. the GENERIC viewer. + return; + } + + // Prevent the browser history from updating until the new destination, + // as stored in the browser history, has been scrolled into view. + this._popStateInProgress = true; + + if (hashChanged) { + // When the hash changed, implying that the 'popstate' event will be + // followed by a 'hashchange' event, then we do *not* want to update the + // browser history when handling the 'hashchange' event (in web/app.js) + // since that would *overwrite* the new destination navigated to below. + // + // To avoid accidentally disabling all future user-initiated hash changes, + // if there's e.g. another 'hashchange' listener that stops the event + // propagation, we make sure to always force-reset `this._blockHashChange` + // after `HASH_CHANGE_TIMEOUT` milliseconds have passed. + this._blockHashChange++; + waitOnEventOrTimeout({ + target: window, + name: 'hashchange', + delay: HASH_CHANGE_TIMEOUT, + }).then(() => { + this._blockHashChange--; + }); + } + + // This case corresponds to navigation backwards in the browser history. + if (state.uid < this._currentUid && this._position && this._destination) { + if (this._destination.page && + this._destination.page !== this._position.first && + this._destination.page !== this._position.page) { + // If the `page` of the `this._destination` is no longer visible, + // push the `this._position` to the browser history before moving back. + this._pushOrReplaceState(this._destination); + this._pushOrReplaceState(this._position); + + // After `window.history.back()`, we must not enter this block on the + // resulting 'popstate' event, since that may cause an infinite loop. + this._currentUid = state.uid; + + window.history.back(); + return; + } + } + + // Navigate to the new destination. + let destination = state.destination; + this._updateInternalState(destination, state.uid); + + if (destination.dest) { + this.linkService.navigateTo(destination.dest); + } else if (destination.hash) { + this.linkService.setHash(destination.hash); + } else if (destination.page) { + // Fallback case; shouldn't be necessary, but better safe than sorry. + this.linkService.page = destination.page; + } + + // Since `PDFLinkService.navigateTo` is asynchronous, we thus defer the + // resetting of `this._popStateInProgress` slightly. + Promise.resolve().then(() => { + this._popStateInProgress = false; + }); + } + + /** + * @private + */ + _bindEvents() { + let { _boundEvents, eventBus, } = this; + + _boundEvents.updateViewarea = this._updateViewarea.bind(this); + _boundEvents.popState = this._popState.bind(this); + _boundEvents.pageHide = (evt) => { + // Attempt to push the `this._position` into the browser history when + // navigating away from the document. This is *only* done if the history + // is currently empty, since otherwise an existing browser history entry + // will end up being overwritten (given that new entries cannot be pushed + // into the browser history when the 'unload' event has already fired). + if (!this._destination) { + this._tryPushCurrentPosition(); + } + }; + + eventBus.on('updateviewarea', _boundEvents.updateViewarea); + window.addEventListener('popstate', _boundEvents.popState); + window.addEventListener('pagehide', _boundEvents.pageHide); + } } function isDestsEqual(firstDest, secondDest) { diff --git a/web/pdf_link_service.js b/web/pdf_link_service.js index 79f5e33fa..e024f6992 100644 --- a/web/pdf_link_service.js +++ b/web/pdf_link_service.js @@ -111,6 +111,13 @@ class PDFLinkService { return; } + if (this.pdfHistory) { + // Update the browser history before scrolling the new destination into + // view, to be able to accurately capture the current document position. + this.pdfHistory.pushCurrentPosition(); + this.pdfHistory.push({ namedDest, explicitDest, pageNumber, }); + } + this.pdfViewer.scrollPageIntoView({ pageNumber, destArray: explicitDest, From 0d58bb5512c73c40642735cf84997bbb8dd3742b Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 20 Jul 2017 15:57:26 +0200 Subject: [PATCH 5/6] Temporarily add the current position to the browser history when the viewer is idle This patch attempts to address an issue in the old `PDFHistory` implementation, where the current position wouldn't be correctly saved when the browser was closed. In theory this *should* already be working, however as the discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=1153393 showed, it seems that both `pagehide` and `beforeunload` arrive to late to successfully update the history during closing. Hence a timeout is used to *temporarily* add the current position to the browser history when the viewer is idle. Note that we need to take care not to update the browser history too often, since that would render the viewer more or unusable. Furthermore, if the timeout is *too* long it may end up effectively disable this whole functionality. The `UPDATE_VIEWAREA_TIMEOUT` constant is thus a heuristic value, which we may need to tweak taking the above into account. --- web/pdf_history.js | 76 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 65 insertions(+), 11 deletions(-) diff --git a/web/pdf_history.js b/web/pdf_history.js index 45616b1de..8c4a208a2 100644 --- a/web/pdf_history.js +++ b/web/pdf_history.js @@ -13,11 +13,13 @@ * limitations under the License. */ -import { parseQueryString, waitOnEventOrTimeout } from './ui_utils'; +import { cloneObj, parseQueryString, waitOnEventOrTimeout } from './ui_utils'; import { getGlobalEventBus } from './dom_events'; // Heuristic value used when force-resetting `this._blockHashChange`. const HASH_CHANGE_TIMEOUT = 1000; // milliseconds +// Heuristic value used when adding a temporary position to the browser history. +const UPDATE_VIEWAREA_TIMEOUT = 2000; // milliseconds /** * @typedef {Object} PDFHistoryOptions @@ -117,8 +119,8 @@ class PDFHistory { // The browser history contains a valid entry, ensure that the history is // initialized correctly on PDF document load. let destination = state.destination; - this._updateInternalState(destination, state.uid); - + this._updateInternalState(destination, state.uid, + /* removeTemporary = */ true); if (destination.dest) { this.initialBookmark = JSON.stringify(destination.dest); @@ -275,16 +277,25 @@ class PDFHistory { /** * @private */ - _tryPushCurrentPosition() { + _tryPushCurrentPosition(temporary = false) { if (!this._position) { return; } let position = this._position; + if (temporary) { + position = cloneObj(this._position); + position.temporary = true; + } if (!this._destination) { this._pushOrReplaceState(position); return; } + if (this._destination.temporary) { + // Always replace a previous *temporary* position. + this._pushOrReplaceState(position, /* forceReplace = */ true); + return; + } if (this._destination.hash === position.hash) { return; // The current document position has not changed. } @@ -337,7 +348,12 @@ class PDFHistory { /** * @private */ - _updateInternalState(destination, uid) { + _updateInternalState(destination, uid, removeTemporary = false) { + if (removeTemporary && destination && destination.temporary) { + // When the `destination` comes from the browser history, + // we no longer treat it as a *temporary* position. + delete destination.temporary; + } this._destination = destination; this._currentUid = uid; this._uid = this._currentUid + 1; @@ -347,6 +363,11 @@ class PDFHistory { * @private */ _updateViewarea({ location, }) { + if (this._updateViewareaTimeout) { + clearTimeout(this._updateViewareaTimeout); + this._updateViewareaTimeout = null; + } + this._position = { hash: this._isViewerInPresentationMode ? `page=${location.pageNumber}` : location.pdfOpenParams.substring(1), @@ -357,6 +378,30 @@ class PDFHistory { if (this._popStateInProgress) { return; } + + if (UPDATE_VIEWAREA_TIMEOUT > 0) { + // When closing the browser, a 'pagehide' event will be dispatched which + // *should* allow us to push the current position to the browser history. + // In practice, it seems that the event is arriving too late in order for + // the session history to be successfully updated. + // (For additional details, please refer to the discussion in + // https://bugzilla.mozilla.org/show_bug.cgi?id=1153393.) + // + // To workaround this we attempt to *temporarily* add the current position + // to the browser history only when the viewer is *idle*, + // i.e. when scrolling and/or zooming does not occur. + // + // PLEASE NOTE: It's absolutely imperative that the browser history is + // *not* updated too often, since that would render the viewer more or + // less unusable. Hence the use of a timeout to delay the update until + // the viewer has been idle for `UPDATE_VIEWAREA_TIMEOUT` milliseconds. + this._updateViewareaTimeout = setTimeout(() => { + if (!this._popStateInProgress) { + this._tryPushCurrentPosition(/* temporary = */ true); + } + this._updateViewareaTimeout = null; + }, UPDATE_VIEWAREA_TIMEOUT); + } } /** @@ -408,14 +453,23 @@ class PDFHistory { // This case corresponds to navigation backwards in the browser history. if (state.uid < this._currentUid && this._position && this._destination) { - if (this._destination.page && - this._destination.page !== this._position.first && - this._destination.page !== this._position.page) { + let shouldGoBack = false; + + if (this._destination.temporary) { + // If the `this._destination` contains a *temporary* position, always + // push the `this._position` to the browser history before moving back. + this._pushOrReplaceState(this._position); + shouldGoBack = true; + } else if (this._destination.page && + this._destination.page !== this._position.first && + this._destination.page !== this._position.page) { // If the `page` of the `this._destination` is no longer visible, // push the `this._position` to the browser history before moving back. this._pushOrReplaceState(this._destination); this._pushOrReplaceState(this._position); - + shouldGoBack = true; + } + if (shouldGoBack) { // After `window.history.back()`, we must not enter this block on the // resulting 'popstate' event, since that may cause an infinite loop. this._currentUid = state.uid; @@ -427,8 +481,8 @@ class PDFHistory { // Navigate to the new destination. let destination = state.destination; - this._updateInternalState(destination, state.uid); - + this._updateInternalState(destination, state.uid, + /* removeTemporary = */ true); if (destination.dest) { this.linkService.navigateTo(destination.dest); } else if (destination.hash) { From 0ac1fba2f9f6e3f40935dbf1e1747aeb0258a6ab Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 11 Aug 2017 13:54:30 +0200 Subject: [PATCH 6/6] Prevent `PDFHistory._tryPushCurrentPosition()` from effectively becoming a no-op if the document hash contains an invalid/non-existent destination By using the (heuristic) `POSITION_UPDATED_THRESHOLD` constant, we can ensure that the current document position will be added to the browser history when a sufficiently "large" number of `updateviewarea` events have been dispatched. --- web/pdf_history.js | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/web/pdf_history.js b/web/pdf_history.js index 8c4a208a2..7d90fe001 100644 --- a/web/pdf_history.js +++ b/web/pdf_history.js @@ -18,6 +18,8 @@ import { getGlobalEventBus } from './dom_events'; // Heuristic value used when force-resetting `this._blockHashChange`. const HASH_CHANGE_TIMEOUT = 1000; // milliseconds +// Heuristic value used when adding the current position to the browser history. +const POSITION_UPDATED_THRESHOLD = 50; // Heuristic value used when adding a temporary position to the browser history. const UPDATE_VIEWAREA_TIMEOUT = 2000; // milliseconds @@ -63,12 +65,16 @@ class PDFHistory { this._boundEvents = Object.create(null); this._isViewerInPresentationMode = false; + this._isPagesLoaded = false; - // Ensure that we don't miss a 'presentationmodechanged' event, by - // registering the listener immediately. + // Ensure that we don't miss either a 'presentationmodechanged' or a + // 'pagesloaded' event, by registering the listeners immediately. this.eventBus.on('presentationmodechanged', (evt) => { this._isViewerInPresentationMode = evt.active || evt.switchInProgress; }); + this.eventBus.on('pagesloaded', (evt) => { + this._isPagesLoaded = !!evt.pagesCount; + }); } /** @@ -97,6 +103,7 @@ class PDFHistory { this._popStateInProgress = false; this._blockHashChange = 0; this._currentHash = getCurrentHash(); + this._numPositionUpdates = 0; this._currentUid = this._uid = 0; this._destination = null; @@ -299,7 +306,9 @@ class PDFHistory { if (this._destination.hash === position.hash) { return; // The current document position has not changed. } - if (!this._destination.page) { + if (!this._destination.page && + (POSITION_UPDATED_THRESHOLD <= 0 || + this._numPositionUpdates <= POSITION_UPDATED_THRESHOLD)) { // `this._destination` was set through the user changing the hash of // the document. Do not add `this._position` to the browser history, // to avoid "flooding" it with lots of (nearly) identical entries, @@ -357,6 +366,8 @@ class PDFHistory { this._destination = destination; this._currentUid = uid; this._uid = this._currentUid + 1; + // This should always be reset when `this._destination` is updated. + this._numPositionUpdates = 0; } /** @@ -379,6 +390,19 @@ class PDFHistory { return; } + if (POSITION_UPDATED_THRESHOLD > 0 && this._isPagesLoaded && + this._destination && !this._destination.page) { + // If the current destination was set through the user changing the hash + // of the document, we will usually not try to push the current position + // to the browser history; see `this._tryPushCurrentPosition()`. + // + // To prevent `this._tryPushCurrentPosition()` from effectively being + // reduced to a no-op in this case, we will assume that the position + // *did* in fact change if the 'updateviewarea' event was dispatched + // more than `POSITION_UPDATED_THRESHOLD` times. + this._numPositionUpdates++; + } + if (UPDATE_VIEWAREA_TIMEOUT > 0) { // When closing the browser, a 'pagehide' event will be dispatched which // *should* allow us to push the current position to the browser history.