From 9743708a24365a1d488060ace3a49390806a035b Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 11 Jan 2019 18:25:10 +0100 Subject: [PATCH] Prevent `TypeError: views[index] is undefined` being throw in `getVisibleElements` when the viewer, or all pages, are hidden 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 ` ``` [3] This is an old, pre-exisiting, issue that's not relevant to this patch as such (and it's already being tracked elsewhere). --- test/unit/ui_utils_spec.js | 32 ++++++++++++++++++++++++++++++++ web/app.js | 4 ++++ web/ui_utils.js | 4 +++- 3 files changed, 39 insertions(+), 1 deletion(-) diff --git a/test/unit/ui_utils_spec.js b/test/unit/ui_utils_spec.js index 1bbb175d6..84a2bed10 100644 --- a/test/unit/ui_utils_spec.js +++ b/test/unit/ui_utils_spec.js @@ -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() { diff --git a/web/app.js b/web/app.js index cdff6173a..26da5e818 100644 --- a/web/app.js +++ b/web/app.js @@ -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). diff --git a/web/ui_utils.js b/web/ui_utils.js index cfe3e4104..7c1a6c110 100644 --- a/web/ui_utils.js +++ b/web/ui_utils.js @@ -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