Change the getVisibleElements helper function to take a parameter object

Given the number of parameters, and the fact that many of them are booleans, the call-sites are no longer particularly easy to read and understand. Furthermore, this slightly improves the formatting of the JSDoc-comment, since it needed updating as part of these changes anyway.

Finally, this removes an unnecessary `numViews === 0` check from `getVisibleElements`, since that should be *very* rare and more importantly that the `binarySearchFirstItem` function already has a fast-path for that particular case.

test/unit/ui_utils_spec.js

@@ -639,11 +639,11 @@ describe("ui_utils", function () {
     // This function takes a fixed layout of pages and compares the system under
     // test to the slower implementation above, for a range of scroll viewport
     // sizes and positions.
-    function scrollOverDocument(pages, horizontally = false, rtl = false) {
+    function scrollOverDocument(pages, horizontal = false, rtl = false) {
       const size = pages.reduce(function (max, { div }) {
         return Math.max(
           max,
-          horizontally
+          horizontal
             ? Math.abs(div.offsetLeft + div.clientLeft + div.clientWidth)
             : div.offsetTop + div.clientTop + div.clientHeight
         );
@@ -657,7 +657,7 @@ describe("ui_utils", function () {
         // iteration; again, this is just to test an interesting range of cases
         // without slowing the tests down to check every possible case.
         for (let j = i + 5; j < size; j += j - i) {
-          const scroll = horizontally
+          const scrollEl = horizontal
             ? {
                 scrollTop: 0,
                 scrollLeft: i,
@@ -671,8 +671,14 @@ describe("ui_utils", function () {
                 clientWidth: 10000,
               };
           expect(
-            getVisibleElements(scroll, pages, false, horizontally, rtl)
+            getVisibleElements({
-          ).toEqual(slowGetVisibleElements(scroll, pages));
+              scrollEl,
+              views: pages,
+              sortByVisibility: false,
+              horizontal,
+              rtl,
+            })
+          ).toEqual(slowGetVisibleElements(scrollEl, pages));
         }
       }
     }
@@ -734,7 +740,7 @@ describe("ui_utils", function () {
           [30, 10],
         ],
       ]);
-      scrollOverDocument(pages, true);
+      scrollOverDocument(pages, /* horizontal = */ true);
     });
 
     it("works with horizontal scrolling with RTL-documents", function () {
@@ -745,7 +751,7 @@ describe("ui_utils", function () {
           [-30, 10],
         ],
       ]);
-      scrollOverDocument(pages, true, true);
+      scrollOverDocument(pages, /* horizontal = */ true, /* rtl = */ true);
     });
 
     it("handles `sortByVisibility` correctly", function () {
@@ -757,12 +763,12 @@ describe("ui_utils", function () {
       };
       const views = makePages([[[100, 150]], [[100, 150]], [[100, 150]]]);
 
-      const visible = getVisibleElements(scrollEl, views);
+      const visible = getVisibleElements({ scrollEl, views });
-      const visibleSorted = getVisibleElements(
+      const visibleSorted = getVisibleElements({
         scrollEl,
         views,
-        /* sortByVisibility = */ true
+        sortByVisibility: true,
-      );
+      });
 
       const viewsOrder = [],
         viewsSortedOrder = [];
@@ -785,7 +791,7 @@ describe("ui_utils", function () {
       };
       const views = [];
 
-      expect(getVisibleElements(scrollEl, views)).toEqual({
+      expect(getVisibleElements({ scrollEl, views })).toEqual({
         first: undefined,
         last: undefined,
         views: [],
@@ -801,7 +807,7 @@ describe("ui_utils", function () {
       };
       const views = makePages([[[100, 150]], [[100, 150]], [[100, 150]]]);
 
-      expect(getVisibleElements(scrollEl, views)).toEqual({
+      expect(getVisibleElements({ scrollEl, views })).toEqual({
         first: undefined,
         last: undefined,
         views: [],

web/base_viewer.js

@@ -1076,13 +1076,13 @@ class BaseViewer {
   }
 
   _getVisiblePages() {
-    return getVisibleElements(
+    return getVisibleElements({
-      this.container,
+      scrollEl: this.container,
-      this._pages,
+      views: this._pages,
-      true,
+      sortByVisibility: true,
-      this._isScrollModeHorizontal,
+      horizontal: this._isScrollModeHorizontal,
-      this._isScrollModeHorizontal && this._isContainerRtl
+      rtl: this._isScrollModeHorizontal && this._isContainerRtl,
-    );
+    });
   }
 
   /**

web/pdf_thumbnail_viewer.js

@@ -81,7 +81,10 @@ class PDFThumbnailViewer {
    * @private
    */
   _getVisibleThumbs() {
-    return getVisibleElements(this.container, this._thumbnails);
+    return getVisibleElements({
+      scrollEl: this.container,
+      views: this._thumbnails,
+    });
   }
 
   scrollThumbnailIntoView(pageNumber) {

web/ui_utils.js

@@ -411,6 +411,21 @@ function backtrackBeforeAllVisibleElements(index, views, top) {
   return index;
 }
 
+/**
+ * @typedef {Object} GetVisibleElementsParameters
+ * @property {HTMLElement} scrollEl - A container that can possibly scroll.
+ * @property {Array} views - Objects with a `div` property that contains an
+ *   HTMLElement, which should all be descendants of `scrollEl` satisfying the
+ *   relevant layout assumptions.
+ * @property {boolean} sortByVisibility - If `true`, the returned elements are
+ *   sorted in descending order of the percent of their padding box that is
+ *   visible. The default value is `false`.
+ * @property {boolean} horizontal - If `true`, the elements are assumed to be
+ *   laid out horizontally instead of vertically. The default value is `false`.
+ * @property {boolean} rtl - If `true`, the `scrollEl` container is assumed to
+ *   be in right-to-left mode. The default value is `false`.
+ */
+
 /**
  * Generic helper to find out what elements are visible within a scroll pane.
  *
@@ -428,23 +443,16 @@ function backtrackBeforeAllVisibleElements(index, views, top) {
  * rendering canvas. Earlier and later refer to index in `views`, not page
  * layout.)
  *
- * @param scrollEl {HTMLElement} - a container that can possibly scroll
+ * @param {GetVisibleElementsParameters}
- * @param views {Array} - objects with a `div` property that contains an
- *   HTMLElement, which should all be descendents of `scrollEl` satisfying the
- *   above layout assumptions
- * @param sortByVisibility {boolean} - if true, the returned elements are sorted
- *   in descending order of the percent of their padding box that is visible
- * @param horizontal {boolean} - if true, the elements are assumed to be laid
- *   out horizontally instead of vertically
  * @returns {Object} `{ first, last, views: [{ id, x, y, view, percent }] }`
  */
-function getVisibleElements(
+function getVisibleElements({
   scrollEl,
   views,
   sortByVisibility = false,
   horizontal = false,
-  rtl = false
+  rtl = false,
-) {
+}) {
   const top = scrollEl.scrollTop,
     bottom = top + scrollEl.clientHeight;
   const left = scrollEl.scrollLeft,
@@ -475,15 +483,12 @@ function getVisibleElements(
 
   const visible = [],
     numViews = views.length;
-  let firstVisibleElementInd =
+  let firstVisibleElementInd = binarySearchFirstItem(
-    numViews === 0
+    views,
-      ? 0
+    horizontal
-      : binarySearchFirstItem(
+      ? isElementNextAfterViewHorizontally
-          views,
+      : isElementBottomAfterViewTop
-          horizontal
+  );
-            ? isElementNextAfterViewHorizontally
-            : isElementBottomAfterViewTop
-        );
 
   // Please note the return value of the `binarySearchFirstItem` function when
   // no valid element is found (hence the `firstVisibleElementInd` check below).