From ae59be181b3b72ab464386c5b50d0e93e4bf8136 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 11 Jan 2019 13:18:36 +0100 Subject: [PATCH 1/3] Clean-up variable definitions in `getVisibleElements` With modern JavaScript supporting block-scoped variables, it's no longer necessary to have large numbers of top-level variable definitions (especially when all variables are, implicitly, set to `undefined`). Besides moving variable definintions, a number of them are also converted to `const` to help ensure that they cannot be *accidentally* modified in the code. Finally, the `views.length` calculation is now cached *once* rather than being re-computed multiple times. --- web/ui_utils.js | 55 +++++++++++++++++++++---------------------------- 1 file changed, 24 insertions(+), 31 deletions(-) diff --git a/web/ui_utils.js b/web/ui_utils.js index 751d9f4fb..cfe3e4104 100644 --- a/web/ui_utils.js +++ b/web/ui_utils.js @@ -413,8 +413,8 @@ function backtrackBeforeAllVisibleElements(index, views, top) { */ function getVisibleElements(scrollEl, views, sortByVisibility = false, horizontal = false) { - let top = scrollEl.scrollTop, bottom = top + scrollEl.clientHeight; - let left = scrollEl.scrollLeft, right = left + scrollEl.clientWidth; + const top = scrollEl.scrollTop, bottom = top + scrollEl.clientHeight; + const left = scrollEl.scrollLeft, right = left + scrollEl.clientWidth; // Throughout this "generic" function, comments will assume we're working with // PDF document pages, which is the most important and complex case. In this @@ -427,27 +427,24 @@ function getVisibleElements(scrollEl, views, sortByVisibility = false, // the border). Adding clientWidth/Height gets us the bottom-right corner of // the padding edge. function isElementBottomAfterViewTop(view) { - let element = view.div; - let elementBottom = + const element = view.div; + const elementBottom = element.offsetTop + element.clientTop + element.clientHeight; return elementBottom > top; } function isElementRightAfterViewLeft(view) { - let element = view.div; - let elementRight = + const element = view.div; + const elementRight = element.offsetLeft + element.clientLeft + element.clientWidth; return elementRight > left; } - let visible = [], view, element; - let currentHeight, viewHeight, viewBottom, hiddenHeight; - let currentWidth, viewWidth, viewRight, hiddenWidth; - let percentVisible; - let firstVisibleElementInd = views.length === 0 ? 0 : + const visible = [], numViews = views.length; + let firstVisibleElementInd = numViews === 0 ? 0 : binarySearchFirstItem(views, horizontal ? isElementRightAfterViewLeft : isElementBottomAfterViewTop); - if (views.length > 0 && !horizontal) { + if (numViews > 0 && !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 @@ -467,15 +464,13 @@ function getVisibleElements(scrollEl, views, sortByVisibility = false, // we pass `right`, without needing the code below that handles the -1 case. let lastEdge = horizontal ? right : -1; - for (let i = firstVisibleElementInd, ii = views.length; i < ii; i++) { - view = views[i]; - element = view.div; - currentWidth = element.offsetLeft + element.clientLeft; - currentHeight = element.offsetTop + element.clientTop; - viewWidth = element.clientWidth; - viewHeight = element.clientHeight; - viewRight = currentWidth + viewWidth; - viewBottom = currentHeight + viewHeight; + for (let i = firstVisibleElementInd; i < numViews; i++) { + const view = views[i], element = view.div; + const currentWidth = element.offsetLeft + element.clientLeft; + const currentHeight = element.offsetTop + element.clientTop; + const viewWidth = element.clientWidth, viewHeight = element.clientHeight; + const viewRight = currentWidth + viewWidth; + const viewBottom = currentHeight + viewHeight; if (lastEdge === -1) { // As commented above, this is only needed in non-horizontal cases. @@ -494,24 +489,22 @@ function getVisibleElements(scrollEl, views, sortByVisibility = false, continue; } - hiddenHeight = Math.max(0, top - currentHeight) + - Math.max(0, viewBottom - bottom); - hiddenWidth = Math.max(0, left - currentWidth) + - Math.max(0, viewRight - right); - percentVisible = ((viewHeight - hiddenHeight) * (viewWidth - hiddenWidth) * - 100 / viewHeight / viewWidth) | 0; - + const hiddenHeight = Math.max(0, top - currentHeight) + + Math.max(0, viewBottom - bottom); + const hiddenWidth = Math.max(0, left - currentWidth) + + Math.max(0, viewRight - right); + const percent = ((viewHeight - hiddenHeight) * (viewWidth - hiddenWidth) * + 100 / viewHeight / viewWidth) | 0; visible.push({ id: view.id, x: currentWidth, y: currentHeight, view, - percent: percentVisible, + percent, }); } - let first = visible[0]; - let last = visible[visible.length - 1]; + const first = visible[0], last = visible[visible.length - 1]; if (sortByVisibility) { visible.sort(function(a, b) { From 9743708a24365a1d488060ace3a49390806a035b Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 11 Jan 2019 18:25:10 +0100 Subject: [PATCH 2/3] 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 From b2235ec9c46c1e7b5a47dd4d5034fcc856ab6fd3 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 12 Jan 2019 11:00:51 +0100 Subject: [PATCH 3/3] Add a unit-test to check that the `sortByVisibility` parameter, in `getVisibleElements`, works correctly --- test/unit/ui_utils_spec.js | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/test/unit/ui_utils_spec.js b/test/unit/ui_utils_spec.js index 84a2bed10..aaab50cd2 100644 --- a/test/unit/ui_utils_spec.js +++ b/test/unit/ui_utils_spec.js @@ -690,6 +690,34 @@ describe('ui_utils', function() { scrollOverDocument(pages, true); }); + it('handles `sortByVisibility` correctly', function() { + const scrollEl = { + scrollTop: 75, + scrollLeft: 0, + clientHeight: 750, + clientWidth: 1500, + }; + const views = makePages([ + [[100, 150]], + [[100, 150]], + [[100, 150]], + ]); + + const visible = getVisibleElements(scrollEl, views); + const visibleSorted = getVisibleElements(scrollEl, views, + /* sortByVisibility = */ true); + + const viewsOrder = [], viewsSortedOrder = []; + for (const view of visible.views) { + viewsOrder.push(view.id); + } + for (const view of visibleSorted.views) { + viewsSortedOrder.push(view.id); + } + expect(viewsOrder).toEqual([0, 1, 2]); + expect(viewsSortedOrder).toEqual([1, 2, 0]); + }); + it('handles views being empty', function() { const scrollEl = { scrollTop: 10,