Sakurai/pdf.js
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Reduce the value of UPDATE_VIEWAREA_TIMEOUT and simplify the 'popstate' event handler to avoid subtle bugs

Browse Source 
When testing the new `PDFHistory` implementation in practice, I felt that the current value of `UPDATE_VIEWAREA_TIMEOUT` is too large to be truly useful.
The purpose of the timeout is to attempt to address (the PDF.js part of) https://bugzilla.mozilla.org/show_bug.cgi?id=1153393, and it's currently fairly easy for the user e.g. close the browser before the timeout had a change to finish.

Obviously, the timeout is a best-effort solution, but with the current value of `UPDATE_VIEWAREA_TIMEOUT` it's not as useful as one would want.
Please note that lowering it shouldn't be a problem, since it still prevents the browser history from updating at *every* 'updateviewarea' event or during (quick) scrolling, which is all that's really needed to not impact the UX negatively.

---

Furthermore, with this lower timeout, we can also simplify the part of the 'popstate' event handler that attempted to update the browser history with the current position before moving back. In most cases, the current position will now already exist in the history, and this *greatly* decreases the complexity of this code path.

The main impetus for this change though, is that I unfortunately found that given the asynchronous nature of updating the browser history, there is some *edge* cases where that code could cause history corruption.
In practice, the user could thus get "stuck" at a particular history entry and not be able to move back. I haven't got any reliable STR for this, since it's so difficult to trigger, but it involved navigating around in a document such that a number of destinations are added to the browser history and then changing the rotation before going back/forward in the history.

Rather that attempting to patch this code, and making it even more difficult to understand than it already is or adding more asynchronous behaviour, by far the easiest solution is to remove it and simply rely on the (lowered) `UPDATE_VIEWAREA_TIMEOUT` instead.
This commit is contained in:
Jonas Jenwald 2017-09-07 12:19:59 +02:00
parent b7c4d788ed
commit 938dffb06b
1 changed files with 1 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

30
web/pdf_history.js
View File

@ -21,7 +21,7 @@ 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
const UPDATE_VIEWAREA_TIMEOUT = 1000; // milliseconds
/**
* @typedef {Object} PDFHistoryOptions
@ -493,34 +493,6 @@ class PDFHistory {
});
}
// This case corresponds to navigation backwards in the browser history.
if (state.uid < this._currentUid && this._position && this._destination) {
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;
window.history.back();
return;
}
}
// Navigate to the new destination.
let destination = state.destination;
this._updateInternalState(destination, state.uid,