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

Prevent TypeError: views[index] is undefined being throw in getVisibleElements when the viewer, or all pages, are hidden

Browse Source 
Previously a couple of different attempts at fixing this problem has been rejected, given how *crucial* this code is for the correct function of the viewer, since no one has thus far provided any evidence that the problem actually affects the default viewer[1] nor an example using the viewer components directly (without another library on top).
The fact that none of the prior patches contained even a *simple* unit-test probably contributed to the unwillingness of a reviewer to sign off on the suggested changes.

However, it turns out that it's possible to create a reduced test-case, using the default viewer, that demonstrates the error[2]. Since this utilizes a hidden `<iframe>`, please note that this error will thus affect Firefox as well.
Note that while errors are thrown when the hidden `<iframe>` loads, the default viewer doesn't break completely since rendering does start working once the `<iframe>` becomes visible (although the errors do break the initial Toolbar state).

Before making any changes here, I carefully read through not just the immediately relevant code but also the rendering code in the viewer (given it's dependence on `getVisibleElements`). After concluding that the changes should be safe in general, the default viewer was tested without any issues found. (The above being much easier with significant prior experience of working with the viewer code.)
Finally the patch also adds new unit-tests, one of which explicitly triggers the relevant code-path and will thus fail with the current `master` branch.

This patch also makes `PDFViewerApplication` slightly more robust against errors during document opening, to ensure that viewer/document initialization always completes as expected.
Please keep in mind that even though this patch prevents an error in `getVisibleElements`, it's still not possible to set the initial position/zoom level/sidebar view etc. when the viewer is hidden since rendering and scrolling is completely dependent[3] on being able to actually access the DOM elements.

---
[1] And hence the PDF Viewer that's built-in to Firefox.

[2] Copy the HTML code below and save it as `iframe.html`, and place the file in the `web/` folder. Then start the server, with `gulp server`, and navigate to http://localhost:8888/web/iframe.html

```html
<!DOCTYPE html>
<html>
  <head>
    <title>Iframe test</title>

    <script>
      window.onload = function() {
        const button = document.getElementById('button1');
        const frame = document.getElementById('frame1');

        button.addEventListener('click', function(evt) {
          frame.hidden = !frame.hidden;
        });
      };
    </script>
  </head>

  <body>
    <button id="button1">Toggle iframe</button>
    <br>
    <iframe id="frame1" width="800" height="600" src="http://localhost:8888/web/viewer.html" hidden="true"></iframe>
  </body>
</html>
```

[3] This is an old, pre-exisiting, issue that's not relevant to this patch as such (and it's already being tracked elsewhere).
This commit is contained in:
Jonas Jenwald 2019-01-11 18:25:10 +01:00
parent ae59be181b
commit 9743708a24
3 changed files with 39 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

32
test/unit/ui_utils_spec.js
View File

@ -690,6 +690,38 @@ describe('ui_utils', function() {
scrollOverDocument(pages, true);
});
it('handles views being empty', function() {
const scrollEl = {
scrollTop: 10,
scrollLeft: 0,
clientHeight: 750,
clientWidth: 1500,
};
const views = [];
expect(getVisibleElements(scrollEl, views)).toEqual({
first: undefined, last: undefined, views: [],
});
});
it('handles all views being hidden (without errors)', function() {
const scrollEl = {
scrollTop: 100000,
scrollLeft: 0,
clientHeight: 750,
clientWidth: 1500,
};
const views = makePages([
[[100, 150]],
[[100, 150]],
[[100, 150]],
]);
expect(getVisibleElements(scrollEl, views)).toEqual({
first: undefined, last: undefined, views: [],
});
});
// This sub-suite is for a notionally internal helper function for
// getVisibleElements.
describe('backtrackBeforeAllVisibleElements', function() {

4
web/app.js
View File

@ -996,6 +996,10 @@ let PDFViewerApplication = {
pdfViewer.currentScaleValue = pdfViewer.currentScaleValue;
// Re-apply the initial document location.
this.setInitialView(hash);
}).catch(() => {
// Ensure that the document is always completely initialized,
// even if there are any errors thrown above.
this.setInitialView();
}).then(function() {
// At this point, rendering of the initial page(s) should always have
// started (and may even have completed).

4
web/ui_utils.js
View File

@ -444,7 +444,9 @@ function getVisibleElements(scrollEl, views, sortByVisibility = false,
binarySearchFirstItem(views, horizontal ? isElementRightAfterViewLeft :
isElementBottomAfterViewTop);
if (numViews > 0 && !horizontal) {
// Please note the return value of the `binarySearchFirstItem` function when
// no valid element is found (hence the `firstVisibleElementInd` check below).
if (numViews > 0 && firstVisibleElementInd < numViews && !horizontal) {
// In wrapped scrolling (or vertical scrolling with spreads), with some page
// sizes, isElementBottomAfterViewTop doesn't satisfy the binary search
// condition: there can be pages with bottoms above the view top between