PDF: Adding horizontal scroll offset for internal navigation.

Document links or bookmarks may have horizontal scroll offsets defined
in the PDF. Currently the horizontal scroll offsets are not taken into
account during navigation of document links or bookmarks. This CL adds
the horizontal scroll offset for document links and bookmarks. Tests
have been modified to validate the scroll value.

Bug: 982176
Change-Id: I80d227b1455db49d35294835dd6ed07f6154ca9c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1803941Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Kevin Babbitt <kbabbitt@microsoft.com>
Commit-Queue: Ankit Kumar 🌪️ <ankk@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#699439}

chrome/browser/resources/pdf/elements/viewer-bookmark.js

@@ -7,8 +7,9 @@
  *
  * The bookmark may point at a location in the PDF or a URI.
  * If it points at a location, |page| indicates which 0-based page it leads to.
- * Optionally, |y| is the y position in that page, in pixel coordinates.
- * If it points at an URI, |uri| is the target for that bookmark.
+ * Optionally, |x| is the x position in that page, |y| is the y position in that
+ * page, in pixel coordinates. If it points at an URI, |uri| is the target for
+ * that bookmark.
  *
  * |children| is an array of the |Bookmark|s that are below this in a table of
  * contents tree
@@ -16,6 +17,7 @@
  * @typedef {{
  *   title: string,
  *   page: (number | undefined),
+ *   x: (number | undefined),
  *   y: (number | undefined),
  *   uri: (string | undefined),
  *   children: !Array<!Bookmark>
@@ -81,10 +83,11 @@ Polymer({
   /** @private */
   onClick_: function() {
     if (this.bookmark.hasOwnProperty('page')) {
-      if (this.bookmark.hasOwnProperty('y')) {
+      if (this.bookmark.hasOwnProperty('x') &&
+          this.bookmark.hasOwnProperty('y')) {
         this.fire('change-page-and-xy', {
           page: this.bookmark.page,
-          x: 0,
+          x: this.bookmark.x,
           y: this.bookmark.y,
           origin: 'bookmark'
         });

chrome/test/data/pdf/bookmarks_test.js

@@ -29,18 +29,22 @@ var tests = [
 
     // Check bookmark fields.
     chrome.test.assertEq(0, firstBookmark.page);
+    chrome.test.assertEq(133, firstBookmark.x);
     chrome.test.assertEq(667, firstBookmark.y);
     chrome.test.assertEq(undefined, firstBookmark.uri);
 
     chrome.test.assertEq(1, firstNestedBookmark.page);
+    chrome.test.assertEq(133, firstNestedBookmark.x);
     chrome.test.assertEq(667, firstNestedBookmark.y);
     chrome.test.assertEq(undefined, firstNestedBookmark.uri);
 
     chrome.test.assertEq(2, secondBookmark.page);
+    chrome.test.assertEq(133, secondBookmark.x);
     chrome.test.assertEq(667, secondBookmark.y);
     chrome.test.assertEq(undefined, secondBookmark.uri);
 
     chrome.test.assertEq(undefined, uriBookmark.page);
+    chrome.test.assertEq(undefined, uriBookmark.x);
     chrome.test.assertEq(undefined, uriBookmark.y);
     chrome.test.assertEq('http://www.chromium.org', uriBookmark.uri);
 
@@ -103,9 +107,9 @@ var tests = [
       chrome.test.assertEq(expectedEvent.uri, lastUriNavigation);
     }
 
-    testTapTarget(rootBookmarks[0].$.item, {page: 0, x: 0, y: 667})
-    testTapTarget(subBookmarks[0].$.item, {page: 1, x: 0, y: 667})
-    testTapTarget(rootBookmarks[1].$.item, {page: 2, x: 0, y: 667})
+    testTapTarget(rootBookmarks[0].$.item, {page: 0, x: 133, y: 667})
+    testTapTarget(subBookmarks[0].$.item, {page: 1, x: 133, y: 667})
+    testTapTarget(rootBookmarks[1].$.item, {page: 2, x: 133, y: 667})
     testTapTarget(rootBookmarks[2].$.item, {uri: "http://www.chromium.org"})
 
     chrome.test.succeed();

pdf/pdfium/accessibility_unittest.cc

@@ -358,6 +358,10 @@ class NavigationEnabledTestClient : public TestClient {
     disposition_ = disposition;
   }
 
+  void ScrollToX(int x_in_screen_coords) override {
+    x_scroll_offset_ = x_in_screen_coords;
+  }
+
   void ScrollToY(int y_in_screen_coords, bool compensate_for_toolbar) override {
     y_scroll_offset_ = y_in_screen_coords;
     compensate_for_toolbar_ = compensate_for_toolbar;
@@ -365,12 +369,14 @@ class NavigationEnabledTestClient : public TestClient {
 
   const std::string& url() const { return url_; }
   WindowOpenDisposition disposition() const { return disposition_; }
+  int x_scroll_offset() const { return x_scroll_offset_; }
   int y_scroll_offset() const { return y_scroll_offset_; }
   bool compensate_for_toolbar() const { return compensate_for_toolbar_; }
 
  private:
   std::string url_;
   WindowOpenDisposition disposition_ = WindowOpenDisposition::UNKNOWN;
+  int x_scroll_offset_ = 0;
   int y_scroll_offset_ = 0;
   bool compensate_for_toolbar_ = false;
 };
@@ -401,6 +407,7 @@ TEST_F(AccessibilityTest, TestInternalLinkClickActionHandling) {
   action_data.page_index = 0;
   action_data.link_index = 1;
   engine->HandleAccessibilityAction(action_data);
+  EXPECT_EQ(266, client.x_scroll_offset());
   EXPECT_EQ(1159, client.y_scroll_offset());
   EXPECT_TRUE(client.compensate_for_toolbar());
   EXPECT_TRUE(client.url().empty());

pdf/pdfium/pdfium_engine.cc

@@ -1219,17 +1219,18 @@ bool PDFiumEngine::NavigateToLinkDestination(
 
     if (disposition == WindowOpenDisposition::CURRENT_TAB) {
       pp::Rect page_rect(GetPageScreenRect(target.page));
+      int x = position_.x() + page_rect.x();
+      x += target.x_in_pixels.value_or(0) * current_zoom_;
       int y = position_.y() + page_rect.y();
-      if (target.y_in_pixels)
-        y += target.y_in_pixels.value() * current_zoom_;
+      y += target.y_in_pixels.value_or(0) * current_zoom_;
 
+      client_->ScrollToX(x);
       client_->ScrollToY(y, /*compensate_for_toolbar=*/true);
     } else {
       std::string parameters = base::StringPrintf("#page=%d", target.page + 1);
-      if (target.y_in_pixels) {
-        parameters += base::StringPrintf(
-            "&zoom=100,0,%d", static_cast<int>(target.y_in_pixels.value()));
-      }
+      parameters += base::StringPrintf(
+          "&zoom=100,%d,%d", static_cast<int>(target.x_in_pixels.value_or(0)),
+          static_cast<int>(target.y_in_pixels.value_or(0)));
 
       client_->NavigateTo(parameters, disposition);
     }
@@ -2082,8 +2083,10 @@ pp::VarDictionary PDFiumEngine::TraverseBookmarks(FPDF_BOOKMARK bookmark,
 
       base::Optional<gfx::PointF> xy =
           pages_[page_index]->GetPageXYTarget(dest);
-      if (xy)
+      if (xy) {
+        dict.Set(pp::Var("x"), pp::Var(static_cast<int>(xy.value().x())));
         dict.Set(pp::Var("y"), pp::Var(static_cast<int>(xy.value().y())));
+      }
     }
   } else {
     // Extract URI for bookmarks linking to an external page.

pdf/pdfium/pdfium_page.cc

@@ -625,8 +625,11 @@ PDFiumPage::Area PDFiumPage::GetDestinationTarget(FPDF_DEST destination,
   target->page = page_index;
 
   base::Optional<gfx::PointF> xy = GetPageXYTarget(destination);
-  if (xy)
-    target->y_in_pixels = TransformPageToScreenXY(xy.value()).y();
+  if (xy) {
+    gfx::PointF point = TransformPageToScreenXY(xy.value());
+    target->x_in_pixels = point.x();
+    target->y_in_pixels = point.y();
+  }
 
   return DOCLINK_AREA;
 }

pdf/pdfium/pdfium_page.h

@@ -91,7 +91,8 @@ class PDFiumPage {
 
     // Valid for DOCLINK_AREA only.
     int page;
-    // Valid for DOCLINK_AREA only. From the top of the page.
+    // Valid for DOCLINK_AREA only. From the top-left of the page.
+    base::Optional<float> x_in_pixels;
     base::Optional<float> y_in_pixels;
   };