Change most visible page calculation in PDFiumEngine and PDF Viewer.

Currently the calculation for the most visible page in the PDF
viewport and PDFiumEngine compare the first visible page to the page
after it, choosing the page with the highest proportion of its height
that intersects with the viewport. This approach works for single-view,
but not for two-up view, introducing bugs that affect displaying the
page number, traversing the document, and rotating the document.

This CL changes the calculation of the most visible page in
PDFiumEngine and PDF Viewer to return the visible page with the
greatest proportion of its area intersecting with the viewport.

This changes the existing behavior in cases where the most visible
page had the highest proportion of its height intersecting the viewport,
but not area.

Bug: 51472
Change-Id: Ie68c39fbc44543a6a7b6ab023f45c2d5fc74e924
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1739599
Commit-Queue: Jeremy Chinsen <chinsenj@google.com>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#687123}

chrome/browser/resources/pdf/viewport.js

@@ -16,17 +16,23 @@ function clampZoom(factor) {
 }
 
 /**
- * Returns the height of the intersection of two rectangles.
+ * Returns the area of the intersection of two rectangles.
  *
  * @param {!ViewportRect} rect1 the first rect
  * @param {!ViewportRect} rect2 the second rect
- * @return {number} the height of the intersection of the rects
+ * @return {number} the area of the intersection of the rects
  */
-function getIntersectionHeight(rect1, rect2) {
-  return Math.max(
-      0,
-      Math.min(rect1.y + rect1.height, rect2.y + rect2.height) -
-          Math.max(rect1.y, rect2.y));
+function getIntersectionArea(rect1, rect2) {
+  const left = Math.max(rect1.x, rect2.x);
+  const top = Math.max(rect1.y, rect2.y);
+  const right = Math.min(rect1.x + rect1.width, rect2.x + rect2.width);
+  const bottom = Math.min(rect1.y + rect1.height, rect2.y + rect2.height);
+
+  if (left >= right || top >= bottom) {
+    return 0;
+  }
+
+  return (right - left) * (bottom - top);
 }
 
 /**
@@ -487,7 +493,9 @@ class ViewportImpl {
   }
 
   /**
-   * Get the which page is at a given y position.
+   * Get the page at a given y position. If there are multiple pages
+   * overlapping the given y-coordinate, return the page with the smallest
+   * index.
    *
    * @param {number} y the y-coordinate to get the page at.
    * @return {number} the index of a page overlapping the given y-coordinate.
@@ -521,6 +529,39 @@ class ViewportImpl {
     return 0;
   }
 
+  /**
+   * Assuming there are at most two pages overlapping |y| in the current layout,
+   * return the highest index of the pages at a given y position. Returns the
+   * last index of the document if the |y| is greater than the total height of
+   * the document.
+   *
+   * @param {number} y the y-coordinate to get the page at.
+   * @return {number} the highest index of the pages overlapping the given
+   *     y-coordinate.
+   * @private
+   */
+  getLastPageAtY_(y) {
+    const firstPage = this.getPageAtY_(y);
+
+    if (firstPage == 0 && this.pageDimensions_[firstPage].y < y) {
+      return this.pageDimensions_.length - 1;
+    }
+
+    if (firstPage % 2 != 0 || firstPage + 1 >= this.pageDimensions_.length) {
+      return firstPage;
+    }
+
+    // If the next page overlaps |y| then the next page is located to the
+    // right of |firstPage|, implying the document is in two-up view. Check
+    // the next page to see if it has this property, returning
+    // |firstPage| + 1 if true.
+    const nextPageY = this.pageDimensions_[firstPage + 1].y;
+    const nextPageBottom =
+        this.pageDimensions_[firstPage + 1].height + nextPageY;
+
+    return (nextPageY <= y && nextPageBottom > y) ? firstPage + 1 : firstPage;
+  }
+
   /** @override */
   isPointInsidePage(point) {
     const zoom = this.zoom;
@@ -544,35 +585,48 @@ class ViewportImpl {
   }
 
   /**
-   * Returns the page with the greatest proportion of its height in the current
+   * Returns the page with the greatest proportion of its area in the current
    * viewport.
    *
    * @return {number} the index of the most visible page.
    */
   getMostVisiblePage() {
-    const firstVisiblePage = this.getPageAtY_(this.position.y / this.zoom);
-    if (firstVisiblePage == this.pageDimensions_.length - 1) {
-      return firstVisiblePage;
-    }
-
     const viewportRect = {
       x: this.position.x / this.zoom,
       y: this.position.y / this.zoom,
       width: this.size.width / this.zoom,
       height: this.size.height / this.zoom
     };
-    const firstVisiblePageVisibility =
-        getIntersectionHeight(
-            this.pageDimensions_[firstVisiblePage], viewportRect) /
-        this.pageDimensions_[firstVisiblePage].height;
-    const nextPageVisibility =
-        getIntersectionHeight(
-            this.pageDimensions_[firstVisiblePage + 1], viewportRect) /
-        this.pageDimensions_[firstVisiblePage + 1].height;
-    if (nextPageVisibility > firstVisiblePageVisibility) {
-      return firstVisiblePage + 1;
+
+    const firstVisiblePage = this.getPageAtY_(viewportRect.y);
+    const lastPossibleVisiblePage =
+        this.getLastPageAtY_(viewportRect.y + viewportRect.height);
+    if (firstVisiblePage === lastPossibleVisiblePage) {
+      return firstVisiblePage;
     }
-    return firstVisiblePage;
+
+    let mostVisiblePage = firstVisiblePage;
+    let largestIntersection = 0;
+
+    for (let i = firstVisiblePage; i < lastPossibleVisiblePage + 1; i++) {
+      const pageArea =
+          this.pageDimensions_[i].width * this.pageDimensions_[i].height;
+
+      // TODO(thestig): check whether we can remove this check.
+      if (pageArea <= 0) {
+        continue;
+      }
+
+      const pageIntersectionArea =
+          getIntersectionArea(this.pageDimensions_[i], viewportRect) / pageArea;
+
+      if (pageIntersectionArea > largestIntersection) {
+        mostVisiblePage = i;
+        largestIntersection = pageIntersectionArea;
+      }
+    }
+
+    return mostVisiblePage;
   }
 
   /**

chrome/test/data/pdf/test_util.js

@@ -98,6 +98,16 @@ function MockDocumentDimensions(width, height) {
       height: h
     });
   };
+  this.addPageForTwoUpView = function(x, y, w, h) {
+    this.width = Math.max(this.width, 2 * w);
+    this.height = Math.max(this.height, y + h);
+    this.pageDimensions.push({
+      x: x,
+      y: y,
+      width: w,
+      height: h
+    });
+  };
   this.reset = function() {
     this.width = 0;
     this.height = 0;

chrome/test/data/pdf/viewport_test.js

@@ -214,8 +214,16 @@ var tests = [
     chrome.test.assertEq(1, viewport.getMostVisiblePage());
 
     // Scrolled just over half way through the first page with 2x zoom.
+    // Despite having a larger intersection height, the proportional
+    // intersection area for the second page is less than the proportional
+    // intersection area for the first page.
     viewport.setZoom(2);
     mockWindow.scrollTo(0, 151);
+    chrome.test.assertEq(0, viewport.getMostVisiblePage());
+
+    // After scrolling further down, we reach a point where proportionally
+    // more area of the second page intersects with the viewport than the first.
+    mockWindow.scrollTo(0, 170);
     chrome.test.assertEq(1, viewport.getMostVisiblePage());
 
     // Zoomed out with the entire document visible.
@@ -225,6 +233,45 @@ var tests = [
     chrome.test.succeed();
   },
 
+  function testGetMostVisiblePageForTwoUpView() {
+    var mockWindow = new MockWindow(400, 500);
+    var viewport = new ViewportImpl(
+        mockWindow, new MockSizer(), function() {}, function() {},
+        function() {}, function() {}, 0, 1, 0);
+
+    var documentDimensions = new MockDocumentDimensions(100, 100);
+
+    documentDimensions.addPageForTwoUpView(100, 0, 300, 400);
+    documentDimensions.addPageForTwoUpView(400, 0, 400, 300);
+    documentDimensions.addPageForTwoUpView(0, 400, 400, 250);
+    documentDimensions.addPageForTwoUpView(400, 400, 200, 400);
+    documentDimensions.addPageForTwoUpView(50, 800, 350, 200);
+    viewport.setDocumentDimensions(documentDimensions);
+    viewport.setZoom(1);
+
+    // Scrolled to the start of the first page.
+    mockWindow.scrollTo(0, 0);
+    chrome.test.assertEq(0, viewport.getMostVisiblePage());
+
+    // Scrolled such that only the first and third pages are visible.
+    mockWindow.scrollTo(0, 200);
+    chrome.test.assertEq(2, viewport.getMostVisiblePage());
+
+    // Scrolled such that only the second and fourth pages are visible.
+    mockWindow.scrollTo(400, 200);
+    chrome.test.assertEq(3, viewport.getMostVisiblePage());
+
+    // Scroll such that first to fourth pages are visible.
+    mockWindow.scrollTo(200, 200);
+    chrome.test.assertEq(3, viewport.getMostVisiblePage());
+
+    // Zoomed out with the entire document visible.
+    viewport.setZoom(0.25);
+    mockWindow.scrollTo(0, 0);
+    chrome.test.assertEq(0, viewport.getMostVisiblePage());
+    chrome.test.succeed();
+  },
+
   function testFitToWidth() {
     var mockWindow = new MockWindow(100, 100);
     var mockSizer = new MockSizer();

pdf/draw_utils/coordinates.cc

@@ -37,6 +37,31 @@ pp::Rect GetBottomGapBetweenRects(int page_rect_bottom,
                   bottom_rect.bottom() - page_rect_bottom);
 }
 
+int GetMostVisiblePage(const std::vector<IndexedPage>& visible_pages,
+                       const pp::Rect& visible_screen) {
+  if (visible_pages.empty())
+    return -1;
+
+  int most_visible_page_index = visible_pages.front().index;
+  double most_visible_page_area = 0.0;
+  for (const auto& visible_page : visible_pages) {
+    double page_area = static_cast<double>(visible_page.rect.size().GetArea());
+    // TODO(thestig): Check whether we can remove this check.
+    if (page_area <= 0.0)
+      continue;
+
+    pp::Rect screen_intersect = visible_screen.Intersect(visible_page.rect);
+    double intersect_area =
+        static_cast<double>(screen_intersect.size().GetArea()) / page_area;
+    if (intersect_area > most_visible_page_area) {
+      most_visible_page_index = visible_page.index;
+      most_visible_page_area = intersect_area;
+    }
+  }
+
+  return most_visible_page_index;
+}
+
 PageInsetSizes GetPageInsetsForTwoUpView(
     size_t page_index,
     size_t num_of_pages,

pdf/draw_utils/coordinates.h

@@ -5,7 +5,9 @@
 #ifndef PDF_DRAW_UTILS_COORDINATES_H_
 #define PDF_DRAW_UTILS_COORDINATES_H_
 
-#include "stddef.h"
+#include <stddef.h>
+
+#include <vector>
 
 #include "ppapi/cpp/rect.h"
 
@@ -26,6 +28,13 @@ struct PageInsetSizes {
   int bottom;
 };
 
+// Struct for sending a page's pp::Rect object along with its corresponding
+// index in the PDF document.
+struct IndexedPage {
+  int index;
+  pp::Rect rect;
+};
+
 // Given a right page's |bottom_gap|, reduce it to only the part of |bottom_gap|
 // that is directly below the right page by translating |bottom_gap| to |page_x|
 // and halving its width. This avoids over-drawing empty space on the already
@@ -48,6 +57,14 @@ void ExpandDocumentSize(const pp::Size& rect_size, pp::Size* doc_size);
 pp::Rect GetBottomGapBetweenRects(int page_rect_bottom,
                                   const pp::Rect& bottom_rect);
 
+// Given |visible_pages| and |visible_screen| in the same coordinates, return
+// the index of the page in |visible_pages| which has the largest proportion of
+// its area intersecting with |visible_screen|. If there is a tie, return the
+// page with the lower index. Returns -1 if |visible_pages| is empty. Returns
+// first page in |visible_pages| if no page intersects with |visible_screen|.
+int GetMostVisiblePage(const std::vector<IndexedPage>& visible_pages,
+                       const pp::Rect& visible_screen);
+
 // Given |page_index|, and |num_of_pages|, return the configuration of
 // |single_view_insets| and |horizontal_separator| for the current page in
 // two-up view.

pdf/draw_utils/coordinates_unittest.cc

@@ -93,6 +93,40 @@ TEST(CoordinateTest, GetBottomGapBetweenRects) {
   CompareRect({0, 0, 0, 0}, GetBottomGapBetweenRects(1400, {0, 10, 300, 500}));
 }
 
+TEST(CoordinateTest, GetMostVisiblePage) {
+  // Test a single-view layout.
+  std::vector<IndexedPage> visible_pages = {
+      {0, {0, 0, 50, 100}}, {1, {0, 100, 100, 100}}, {2, {0, 200, 100, 200}}};
+  EXPECT_EQ(0, GetMostVisiblePage(visible_pages, {0, 0, 100, 100}));
+  EXPECT_EQ(1, GetMostVisiblePage(visible_pages, {0, 100, 100, 100}));
+  EXPECT_EQ(0, GetMostVisiblePage(visible_pages, {0, 50, 100, 100}));
+  EXPECT_EQ(1, GetMostVisiblePage(visible_pages, {0, 51, 100, 100}));
+  EXPECT_EQ(2, GetMostVisiblePage(visible_pages, {0, 180, 100, 100}));
+  EXPECT_EQ(1, GetMostVisiblePage(visible_pages, {0, 160, 100, 100}));
+  EXPECT_EQ(0, GetMostVisiblePage(visible_pages, {0, 77, 50, 50}));
+  EXPECT_EQ(1, GetMostVisiblePage(visible_pages, {0, 85, 50, 50}));
+  EXPECT_EQ(0, GetMostVisiblePage(visible_pages, {0, 0, 400, 400}));
+
+  // Test a two-up view layout.
+  visible_pages = {{0, {100, 0, 300, 400}},
+                   {1, {400, 0, 400, 300}},
+                   {2, {0, 400, 400, 250}},
+                   {3, {400, 400, 200, 400}},
+                   {4, {50, 800, 350, 200}}};
+  EXPECT_EQ(0, GetMostVisiblePage(visible_pages, {0, 0, 400, 500}));
+  EXPECT_EQ(2, GetMostVisiblePage(visible_pages, {0, 200, 400, 500}));
+  EXPECT_EQ(3, GetMostVisiblePage(visible_pages, {400, 200, 400, 500}));
+  EXPECT_EQ(3, GetMostVisiblePage(visible_pages, {200, 200, 400, 500}));
+  EXPECT_EQ(0, GetMostVisiblePage(visible_pages, {0, 0, 1600, 2000}));
+
+  // Test case where no pages intersect with the viewport.
+  EXPECT_EQ(0, GetMostVisiblePage(visible_pages, {1000, 2000, 150, 200}));
+
+  // Test empty vector.
+  std::vector<IndexedPage> empty_pages;
+  EXPECT_EQ(-1, GetMostVisiblePage(empty_pages, {100, 200, 300, 400}));
+}
+
 TEST(CoordinateTest, GetPageInsetsForTwoUpView) {
   // Page is on the left side and isn't the last page in the document.
   CompareInsetSizes(kLeftInsets,

pdf/pdfium/pdfium_engine.cc

@@ -2570,18 +2570,15 @@ void PDFiumEngine::CalculateVisiblePages() {
   // screen coordinates.
   form_highlights_.clear();
 
-  int most_visible_page = visible_pages_.empty() ? -1 : visible_pages_.front();
-  // Check if the next page is more visible than the first one.
-  if (most_visible_page != -1 && !pages_.empty() &&
-      most_visible_page < static_cast<int>(pages_.size()) - 1) {
-    pp::Rect rc_first =
-        visible_rect.Intersect(GetPageScreenRect(most_visible_page));
-    pp::Rect rc_next =
-        visible_rect.Intersect(GetPageScreenRect(most_visible_page + 1));
-    if (rc_next.height() > rc_first.height())
-      most_visible_page++;
+  std::vector<draw_utils::IndexedPage> visible_pages_rects;
+  visible_pages_rects.reserve(visible_pages_.size());
+  for (int visible_page_index : visible_pages_) {
+    visible_pages_rects.push_back(
+        {visible_page_index, pages_[visible_page_index]->rect()});
   }
 
+  int most_visible_page =
+      draw_utils::GetMostVisiblePage(visible_pages_rects, GetVisibleRect());
   SetCurrentPage(most_visible_page);
 }