[unseasoned-pdf] Enable cross-frame scrolling

Enables scrolling within the unseasoned PDF viewer's plugin frame,
instead of within the extension frame. Notably, this fixes autoscroll
behavior, since the plugin frame now handles its own scrolling.

This required fixes for a number of would-be regressions:

1. Some keyboard shortcuts require suppressing the default scroll
   behavior using preventDefault(), in order to behave the same as the
   shipping Pepper version. Note that there are still some pre-existing
   regressions in the unseasoned PDF viewer; these will be fixed as part
   of crbug.com/1279550.
2. Tickmarks are set using WebLocalFrame::SetTickmarks() again, instead
   of with a Mojo IPC; this had been changed in crbug.com/1199999.
3. Scrolling uses WebLocalFrame::GetScrollOffset() again, instead of an
   "updateScroll" message; this had been changed in crbug.com/1101598 to
   support the sidenav panel.

Fixed: 1271262
Change-Id: Ia98054a81766d5a0ce87820288dcc74f958d6b3c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3294022
Reviewed-by: Daniel Hosseinian <dhoss@chromium.org>
Commit-Queue: K. Moon <kmoon@chromium.org>
Cr-Commit-Position: refs/heads/main@{#951352}

chrome/browser/resources/pdf/controller.js

@@ -214,6 +214,8 @@ export class PluginController {
           (message, transfer) => {
             this.unseasonedDelayedMessages_.push({message, transfer});
           };
+
+      this.viewport_.setRemoteContent(this.unseasonedPlugin_);
     }
   }
 
@@ -262,6 +264,12 @@ export class PluginController {
    * @param {number} y
    */
   updateScroll(x, y) {
+    if (this.unseasonedPlugin_) {
+      // Ignore "local" scroll events in unseasoned mode, as these are synthetic
+      // events generated by the remote scrolling implementation.
+      return;
+    }
+
     this.postMessage_({type: 'updateScroll', x, y});
   }
 
@@ -491,9 +499,11 @@ export class PluginController {
     let url;
     if (this.unseasonedPlugin_) {
       assert(this.unseasonedPlugin_ === this.plugin_);
+      this.viewport_.setRemoteContent(this.unseasonedPlugin_);
       this.unseasonedPlugin_.postMessage(
           {type: 'loadArray', dataToLoad: data}, [data]);
     } else {
+      this.viewport_.setContent(this.plugin_);
       url = URL.createObjectURL(new Blob([data]));
       this.plugin_.setAttribute('src', url);
       this.plugin_.setAttribute('has-edits', '');
@@ -567,6 +577,13 @@ export class PluginController {
       case 'scrollBy':
         this.viewport_.scrollBy(/** @type {!Point} */ (messageData));
         break;
+      case 'syncScrollFromRemote':
+        this.viewport_.syncScrollFromRemote(
+            /** @type {!Point} */ (messageData));
+        break;
+      case 'ackScrollToRemote':
+        this.viewport_.ackScrollToRemote();
+        break;
       case 'saveData':
         this.saveData_(/** @type {!SaveDataMessageData} */ (messageData));
         break;

chrome/browser/resources/pdf/pdf_internal_plugin_wrapper.js

@@ -17,7 +17,24 @@ if (parentOrigin === 'chrome-untrusted://print') {
   parentOrigin = 'chrome://print';
 }
 
-plugin.addEventListener('message', e => channel.port1.postMessage(e.data));
+// Plugin-to-parent message handlers. All messages are passed through, but some
+// messages may affect this frame, too.
+let isFormFieldFocused = false;
+plugin.addEventListener('message', e => {
+  switch (e.data.type) {
+    case 'formFocusChange':
+      // TODO(crbug.com/1279516): Ideally, the plugin would just consume
+      // interesting keyboard events first.
+      isFormFieldFocused = /** @type {{focused:boolean}} */ (e.data).focused;
+      break;
+  }
+
+  channel.port1.postMessage(e.data);
+});
+
+// Parent-to-plugin message handlers. Most messages are passed through, but some
+// messages (with handlers that `return` immediately) are meant only for this
+// frame, not the plugin.
 channel.port1.onmessage = e => {
   switch (e.data.type) {
     case 'loadArray':
@@ -26,16 +43,54 @@ channel.port1.onmessage = e => {
       }
       plugin.src = URL.createObjectURL(new Blob([e.data.dataToLoad]));
       plugin.setAttribute('has-edits', '');
-      break;
+      return;
 
-    default:
-      plugin.postMessage(e.data);
+    case 'syncScrollToRemote':
+      channel.port1.postMessage({type: 'ackScrollToRemote'});
+      window.scrollTo(e.data.x, e.data.y);
+      return;
+
+    case 'updateSize':
+      sizer.style.width = `${e.data.width}px`;
+      sizer.style.height = `${e.data.height}px`;
+      return;
+
+    case 'viewport':
+      // Snoop on "viewport" message to support real RTL scrolling in Print
+      // Preview.
+      // TODO(crbug.com/1158670): Support real RTL scrolling in the PDF viewer.
+      if (parentOrigin === 'chrome://print' && e.data.layoutOptions) {
+        switch (e.data.layoutOptions.direction) {
+          case 1:
+            document.dir = 'rtl';
+            break;
+          case 2:
+            document.dir = 'ltr';
+            break;
+          default:
+            document.dir = '';
+            break;
+        }
+      }
       break;
   }
+
+  plugin.postMessage(e.data);
 };
+
+// Entangle parent-child message channel.
 window.parent.postMessage(
     {type: 'connect', token: srcUrl.href}, parentOrigin, [channel.port2]);
 
+// Forward "scroll" events back to the parent frame's `Viewport`.
+window.addEventListener('scroll', () => {
+  channel.port1.postMessage({
+    type: 'syncScrollFromRemote',
+    x: window.scrollX,
+    y: window.scrollY,
+  });
+});
+
 /**
  * Relays gesture events to the parent frame.
  * @param {!Event} e The gesture event.
@@ -60,20 +115,28 @@ document.addEventListener('keydown', e => {
   // Only forward potential shortcut keys.
   switch (e.key) {
     case ' ':
+      // Preventing Space happens in the "keypress" event handler.
+      break;
+
     case 'ArrowLeft':
     case 'ArrowRight':
+      // Don't prevent ArrowLeft/ArrowRight in form fields.
+      if (!isFormFieldFocused) {
+        e.preventDefault();
+      }
       break;
+
     default:
       if (e.ctrlKey || e.metaKey) {
+        // Take over Ctrl+A, but not other shortcuts, such as zoom or print.
+        if (e.key === 'a') {
+          e.preventDefault();
+        }
         break;
       }
       return;
   }
 
-  // Take over Ctrl+A, but not other shortcuts, such as zoom or print.
-  if (e.key === 'a') {
-    e.preventDefault();
-  }
   channel.port1.postMessage({
     type: 'sendKeyEvent',
     keyEvent: {
@@ -87,3 +150,17 @@ document.addEventListener('keydown', e => {
     },
   });
 });
+
+// Suppress extra scroll by preventing the default "keypress" handler for Space.
+// TODO(crbug.com/1279429): Ideally would prevent "keydown" instead, but this
+// doesn't work when a plugin element has focus.
+document.addEventListener('keypress', e => {
+  switch (e.key) {
+    case ' ':
+      // Don't prevent Space in form fields.
+      if (!isFormFieldFocused) {
+        e.preventDefault();
+      }
+      break;
+  }
+});

chrome/browser/resources/pdf/viewport.js

@@ -1636,7 +1636,7 @@ class ScrollContent {
   }
 
   /**
-   * Sets the contents, scrolling locally.
+   * Sets the contents, switching to scrolling locally.
    * @param {?Node} content The new contents, or null to clear.
    */
   setContent(content) {
@@ -1659,7 +1659,7 @@ class ScrollContent {
   }
 
   /**
-   * Sets the contents, scrolling remotely.
+   * Sets the contents, switching to scrolling remotely.
    * @param {!UnseasonedPdfPluginElement} content The new contents.
    */
   setRemoteContent(content) {
@@ -1785,8 +1785,9 @@ class ScrollContent {
    */
   scrollTo(x, y) {
     if (this.unseasonedPlugin_) {
-      // To match real scrollTo(), update scroll position immediately, but fire
-      // the scroll event later (in `ackScrollToRemote()`).
+      // To match the DOM's scrollTo() behavior, update the scroll position
+      // immediately, but fire the scroll event later (when the remote side
+      // triggers `ackScrollToRemote()`).
       // TODO(crbug.com/1277228): Can get NaN if zoom calculations divide by 0.
       this.scrollLeft_ = Number.isNaN(x) ? 0 : x;
       this.scrollTop_ = Number.isNaN(y) ? 0 : y;

pdf/pdf_view_web_plugin.cc

@@ -58,9 +58,11 @@
 #include "third_party/blink/public/platform/web_url_error.h"
 #include "third_party/blink/public/platform/web_url_request.h"
 #include "third_party/blink/public/platform/web_url_response.h"
+#include "third_party/blink/public/platform/web_vector.h"
 #include "third_party/blink/public/web/web_associated_url_loader.h"
 #include "third_party/blink/public/web/web_associated_url_loader_options.h"
 #include "third_party/blink/public/web/web_document.h"
+#include "third_party/blink/public/web/web_element.h"
 #include "third_party/blink/public/web/web_frame.h"
 #include "third_party/blink/public/web/web_frame_widget.h"
 #include "third_party/blink/public/web/web_local_frame.h"
@@ -80,6 +82,7 @@
 #include "ui/events/blink/blink_event_util.h"
 #include "ui/events/keycodes/keyboard_codes.h"
 #include "ui/gfx/geometry/point.h"
+#include "ui/gfx/geometry/point_f.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/skia_conversions.h"
 #include "ui/gfx/geometry/vector2d_f.h"
@@ -164,6 +167,15 @@ class BlinkContainerWrapper final : public PdfViewWebPlugin::ContainerWrapper {
     container_->ReportFindInPageSelection(identifier, index);
   }
 
+  void ReportFindInPageTickmarks(
+      const std::vector<gfx::Rect>& tickmarks) override {
+    blink::WebLocalFrame* frame = GetFrame();
+    if (frame) {
+      frame->SetTickmarks(blink::WebElement(),
+                          blink::WebVector<gfx::Rect>(tickmarks));
+    }
+  }
+
   float DeviceScaleFactor() override {
     // Do not reply on the device scale returned by
     // `container_->DeviceScaleFactor()`, since it doesn't always reflect the
@@ -173,6 +185,12 @@ class BlinkContainerWrapper final : public PdfViewWebPlugin::ContainerWrapper {
     return widget->GetOriginalScreenInfo().device_scale_factor;
   }
 
+  gfx::PointF GetScrollPosition() override {
+    // Note that `blink::WebLocalFrame::GetScrollOffset()` actually returns a
+    // scroll position (a point relative to the top-left corner).
+    return GetFrame()->GetScrollOffset();
+  }
+
   void SetReferrerForRequest(blink::WebURLRequest& request,
                              const blink::WebURL& referrer_url) override {
     GetFrame()->SetReferrerForRequest(request, referrer_url);
@@ -433,6 +451,13 @@ void PdfViewWebPlugin::UpdateGeometry(const gfx::Rect& window_rect,
     return;
 
   OnViewportChanged(window_rect, container_wrapper_->DeviceScaleFactor());
+
+  gfx::PointF scroll_position = container_wrapper_->GetScrollPosition();
+  if (client_->IsUseZoomForDSFEnabled()) {
+    // Convert back to CSS pixels.
+    scroll_position.Scale(1.0f / device_scale());
+  }
+  UpdateScroll(scroll_position);
 }
 
 void PdfViewWebPlugin::UpdateFocus(bool focused,
@@ -904,21 +929,12 @@ void PdfViewWebPlugin::NotifyFindResultsChanged(int total, bool final_result) {
   container_wrapper_->ReportFindInPageMatchCount(find_identifier_, total,
                                                  final_result);
 }
+
 void PdfViewWebPlugin::NotifyFindTickmarks(
     const std::vector<gfx::Rect>& tickmarks) {
-  auto* service = GetPdfService();
-  if (!service)
-    return;
-
-  if (!find_remote_) {
-    mojo::PendingRemote<pdf::mojom::PdfFindInPage> pending_find_remote;
-    service->GetPdfFindInPage(&pending_find_remote);
-    if (!pending_find_remote)
-      return;
-
-    find_remote_.Bind(std::move(pending_find_remote));
-  }
-  find_remote_->SetTickmarks(tickmarks);
+  // TODO(crbug.com/1278476): Clean up `PdfFindInPage::SetTickmarks()` once
+  // plugin frame scrolling is stable.
+  container_wrapper_->ReportFindInPageTickmarks(tickmarks);
 }
 
 void PdfViewWebPlugin::SetContentRestrictions(int content_restrictions) {

pdf/pdf_view_web_plugin.h

@@ -43,7 +43,9 @@ struct WebAssociatedURLLoaderOptions;
 }  // namespace blink
 
 namespace gfx {
+class PointF;
 class Range;
+class Rect;
 }  // namespace gfx
 
 namespace printing {
@@ -85,9 +87,16 @@ class PdfViewWebPlugin final : public PdfViewPluginBase,
     // Notify the web plugin container about the selected find result in plugin.
     virtual void ReportFindInPageSelection(int identifier, int index) = 0;
 
+    // Notify the web plugin container about find result tickmarks.
+    virtual void ReportFindInPageTickmarks(
+        const std::vector<gfx::Rect>& tickmarks) = 0;
+
     // Returns the device scale factor.
     virtual float DeviceScaleFactor() = 0;
 
+    // Gets the scroll position.
+    virtual gfx::PointF GetScrollPosition() = 0;
+
     // Calls underlying WebLocalFrame::SetReferrerForRequest().
     virtual void SetReferrerForRequest(blink::WebURLRequest& request,
                                        const blink::WebURL& referrer_url) = 0;

pdf/pdf_view_web_plugin_unittest.cc

@@ -7,12 +7,14 @@
 #include <memory>
 #include <string>
 #include <utility>
+#include <vector>
 
 #include "base/location.h"
 #include "base/memory/raw_ptr.h"
 #include "base/run_loop.h"
 #include "base/strings/string_piece.h"
 #include "base/test/bind.h"
+#include "base/test/values_test_util.h"
 #include "base/time/time.h"
 #include "cc/paint/paint_canvas.h"
 #include "cc/test/pixel_comparator.h"
@@ -158,8 +160,15 @@ class FakeContainerWrapper : public PdfViewWebPlugin::ContainerWrapper {
 
   MOCK_METHOD(void, ReportFindInPageSelection, (int, int), (override));
 
+  MOCK_METHOD(void,
+              ReportFindInPageTickmarks,
+              (const std::vector<gfx::Rect>&),
+              (override));
+
   float DeviceScaleFactor() override { return device_scale_; }
 
+  MOCK_METHOD(gfx::PointF, GetScrollPosition, (), (override));
+
   MOCK_METHOD(void,
               SetReferrerForRequest,
               (blink::WebURLRequest&, const blink::WebURL&),
@@ -300,6 +309,24 @@ class PdfViewWebPluginTest : public PdfViewWebPluginWithoutInitializeTest {
     return std::make_unique<NiceMock<TestPDFiumEngine>>(plugin_.get());
   }
 
+  void SetDocumentDimensions(const gfx::Size& dimensions) {
+    EXPECT_CALL(*engine_ptr_, ApplyDocumentLayout)
+        .WillRepeatedly(Return(dimensions));
+    plugin_->OnMessage(base::test::ParseJson(R"({
+      "type": "viewport",
+      "userInitiated": false,
+      "zoom": 1,
+      "layoutOptions": {
+        "direction": 2,
+        "defaultPageOrientation": 0,
+        "twoUpViewEnabled": false,
+      },
+      "xOffset": 0,
+      "yOffset": 0,
+      "pinchPhase": 0,
+    })"));
+  }
+
   void UpdatePluginGeometry(float device_scale, const gfx::Rect& window_rect) {
     UpdatePluginGeometryWithoutWaiting(device_scale, window_rect);
 
@@ -457,6 +484,30 @@ TEST_F(PdfViewWebPluginTest,
   }
 }
 
+TEST_F(PdfViewWebPluginTest, UpdateGeometryScrollsUseZoomForDSFEnabled) {
+  EXPECT_CALL(*client_ptr_, IsUseZoomForDSFEnabled)
+      .WillRepeatedly(Return(true));
+  SetDocumentDimensions({100, 200});
+
+  EXPECT_CALL(*wrapper_ptr_, GetScrollPosition)
+      .WillRepeatedly(Return(gfx::PointF(4.0f, 6.0f)));
+  EXPECT_CALL(*engine_ptr_, ScrolledToXPosition(4));
+  EXPECT_CALL(*engine_ptr_, ScrolledToYPosition(6));
+  UpdatePluginGeometryWithoutWaiting(2.0f, gfx::Rect(3, 4, 5, 6));
+}
+
+TEST_F(PdfViewWebPluginTest, UpdateGeometryScrollsUseZoomForDSFDisabled) {
+  EXPECT_CALL(*client_ptr_, IsUseZoomForDSFEnabled)
+      .WillRepeatedly(Return(false));
+  SetDocumentDimensions({100, 200});
+
+  EXPECT_CALL(*wrapper_ptr_, GetScrollPosition)
+      .WillRepeatedly(Return(gfx::PointF(2.0f, 3.0f)));
+  EXPECT_CALL(*engine_ptr_, ScrolledToXPosition(4));
+  EXPECT_CALL(*engine_ptr_, ScrolledToYPosition(6));
+  UpdatePluginGeometryWithoutWaiting(2.0f, gfx::Rect(3, 4, 5, 6));
+}
+
 class PdfViewWebPluginTestUseZoomForDSF
     : public PdfViewWebPluginTest,
       public testing::WithParamInterface<bool> {
@@ -918,4 +969,15 @@ TEST_F(PdfViewWebPluginTest, CaretChangeUseZoomForDSFDisabled) {
   EXPECT_EQ(gfx::Rect(23, 10, 15, 20), plugin_->GetPluginCaretBounds());
 }
 
+TEST_F(PdfViewWebPluginTest, NotifyNumberOfFindResultsChanged) {
+  plugin_->StartFind("x", /*case_sensitive=*/false, /*identifier=*/123);
+
+  const std::vector<gfx::Rect> tickmarks = {gfx::Rect(1, 2), gfx::Rect(3, 4)};
+  plugin_->UpdateTickMarks(tickmarks);
+
+  EXPECT_CALL(*wrapper_ptr_, ReportFindInPageTickmarks(tickmarks));
+  EXPECT_CALL(*wrapper_ptr_, ReportFindInPageMatchCount(123, 5, true));
+  plugin_->NotifyNumberOfFindResultsChanged(/*total=*/5, /*final_result=*/true);
+}
+
 }  // namespace chrome_pdf