Improved Pinch-Zoom for PDF.

chrome/browser/resources/pdf/viewport.js

Index: chrome/browser/resources/pdf/viewport.js
diff --git a/chrome/browser/resources/pdf/viewport.js b/chrome/browser/resources/pdf/viewport.js
index 18fb6c2ad403f4c8a49956f43a37bc776eaece6e..152a87b1116e3386714a88615ceb376b10dea307 100644
--- a/chrome/browser/resources/pdf/viewport.js
+++ b/chrome/browser/resources/pdf/viewport.js
@@ -49,6 +49,8 @@ function Viewport(window,
   this.fittingType_ = Viewport.FittingType.NONE;
   this.defaultZoom_ = defaultZoom;
   this.topToolbarHeight_ = topToolbarHeight;
+  this.prevScale = 1;
+  this.pinchPhase_ = Viewport.PinchPhase.PINCH_NONE;
 
   window.addEventListener('scroll', this.updateViewport_.bind(this));
   window.addEventListener('resize', this.resize_.bind(this));
@@ -65,6 +67,19 @@ Viewport.FittingType = {
 };
 
 /**
+ * Enumeration of pinch states.
+ * This should match PinchPhase enum in pdf/out_of_process_instance.h
+ * @enum {number}
+ */
+Viewport.PinchPhase = {
+  PINCH_NONE: 0,
+  PINCH_START: 1,
+  PINCH_IN_UPDATE: 2,

[bokan, 2016/10/17 22:20:16]

There's some disagreement in terminology on which direction "in" and "out" are.
One camp (which I fall into) says "pinch-in" is the gesture that makes things on
the page larger (i.e. "zoom-in"), thereas the other camp says "pinch-in" is the
gesture that makes things smaller (i.e. your fingers are literally pinching-in).
Please document which terminology we're using here.

[Kevin McNee - google account, 2016/10/24 21:11:47]

Yeah, I was naming these to be consistent with Hammer.js, but I've been getting
confused myself. I've changed these to PINCH_UPDATE_ZOOM_OUT and
PINCH_UPDATE_ZOOM_IN to make them unambiguous. When we get the pinchin/pinchout
information from Hammer.js, I've included a comment to explain that Hammer.js
uses pinchin to mean zooming out.

Done.

+  PINCH_OUT_UPDATE: 3,
+  PINCH_END: 4
+};
+
+/**
  * The increment to scroll a page by in pixels when up/down/left/right arrow
  * keys are pressed. Usually we just let the browser handle scrolling on the
  * window when these keys are pressed but in certain cases we need to simulate
@@ -227,6 +242,28 @@ Viewport.prototype = {
   },
 
   /**
+   * @type {Viewport.PinchPhase} the phase of the current pinch gesture for
+   *    the viewport.
+   */
+  get pinchPhase() {
+    return this.pinchPhase_;
+  },
+
+  /**
+   * @type {Object} the panning caused by the current pinch gesture.
+   */
+  get pinchPanVector() {
+    return this.pinchPanVector_;
+  },
+
+  /**
+   * @type {Object} the center of the current pinch gesture.
+   */
+  get pinchCenter() {
+    return this.pinchCenter_;
+  },
+
+  /**
    * @private
    * Used to wrap a function that might perform zooming on the viewport. This is
    * required so that we can notify the plugin that zooming is in progress
@@ -266,6 +303,64 @@ Viewport.prototype = {
   },
 
   /**
+   * @private
+   * Sets the zoom of the viewport.
+   * Same as setZoomInternal_ but for pinch zoom we have some more operations.
+   * @param {number} scaleDelta the zoom delta.
+   * @param {Object} center the pinch center in content coordinates
+   */
+  setPinchZoomInternal_: function(scaleDelta, center) {
+    if (!this.allowedToChangeZoom_) {
+      throw 'Called Viewport.setZoomInternal_ without calling ' +
+          'Viewport.mightZoom_.';
+    }
+    this.zoom_ = this.clampScale(this.zoom_ * scaleDelta);
+
+    var newCenterInContent = this.frameToContent(center);
+    var delta = {
+      x: (newCenterInContent.x - this.oldCenterInContent.x),
+      y: (newCenterInContent.y - this.oldCenterInContent.y)
+    };
+
+    // Record the scroll position (relative to the pinch center).
+    var currentScrollPos = {
+      x: this.position.x - delta.x * this.zoom_,
+      y: this.position.y - delta.y * this.zoom_
+    };
+
+    this.contentSizeChanged_();
+    // Scroll to the scaled scroll position.
+    this.position = {
+      x: currentScrollPos.x,
+      y: currentScrollPos.y
+    };
+  },
+
+  /**
+   *  @private
+   *  Makes sure that the scale level doesn't get out of the limits.
+   *  @param {number} scale the new scale level
+   *  @return {number} the scale clamped in the limit
+   */
+  clampScale: function(scale) {
+    return Math.min(5, Math.max(0.25, scale));
+  },
+
+  /**
+   *  @private
+   *  Gets the new center in content.
+   *  @param {Object} pinch center

[bokan, 2016/10/17 22:20:15]

This comment block is wrong, this converts a point from "frame" to "content"
coordinates. i.e. "window" to "page", and the parameters seem to be out of date.

[Kevin McNee - google account, 2016/10/24 21:11:47]

Done.

+   *  @param {Object} zoom level
+   *  @return {Objet} the new center in content
+   */
+  frameToContent: function(framePoint) {
+    return {
+      x: (framePoint.x + this.position.x) / this.zoom_,
+      y: (framePoint.y + this.position.y) / this.zoom_
+    };
+  },
+
+  /**
    * Sets the zoom to the given zoom level.
    * @param {number} newZoom the zoom level to zoom to.
    */
@@ -457,6 +552,19 @@ Viewport.prototype = {
       this.updateViewport_();
     }.bind(this));
   },
+  /**
+   * @private
+   * Computes vector between two points.
+   * @param {Object} First Point
+   * @param {Object} Second Point
+   * @return {Object} The vector
+   */
+  vectorDelta: function(p1, p2) {
+    return {
+      x: p2.x - p1.x,
+      y: p2.y - p1.y
+    };
+  },
 
   /**
    * Zoom the viewport so that a page consumes the entire viewport. Also scrolls
@@ -499,6 +607,82 @@ Viewport.prototype = {
   },
 
   /**
+   * Pinch zoom event handler
+   * @param {ev} the pinch event
+   */
+  pinchZoom: function(ev) {
+    this.mightZoom_(function() {
+      this.pinchPhase_ = (ev.additionalEvent == 'pinchin') ?
+                         Viewport.PinchPhase.PINCH_IN_UPDATE :
+                         Viewport.PinchPhase.PINCH_OUT_UPDATE;
+
+      var scaleDelta = ev.scale / this.prevScale;
+      this.pinchPanVector_ =
+          this.vectorDelta(ev.center, this.first_pinch_center_in_frame_);
+
+      var needsScrollbars = this.documentNeedsScrollbars_(
+          this.clampScale(this.zoom_ * scaleDelta));
+      // If there's no horizontal scrolling, keep the content centered so the
+      // user can't zoom in on the non-content area.

[bokan, 2016/10/17 22:20:16]

The if block attached to this comment is for the case where we *are* centering.
Perhaps this comment was meant with the if block below: |if
(!needsScrollbars.horizontal)|. Perhaps the two blocks would be more natural as
an if/else but I'll leave the decision to you.

[Kevin McNee - google account, 2016/10/24 21:11:47]

Done.

+      if (this.keepContentCentered_ && needsScrollbars.horizontal) {
+        this.oldCenterInContent =
+            this.frameToContent(this.frameToPluginCoordinate(ev.center));
+        this.keepContentCentered_ = false;
+      }
+
+      this.pinchCenter_ = ev.center;
+      // TODO(mcnee) Come up with a less jarring way of scaling
+      // when we don't have horizontal scrolling. We want to keep
+      // the document centered, but this causes an awkward transition
+      // when we start using the gesture center.

[bokan, 2016/10/17 22:20:16]

IIRC this didn't actually feel that unnatural, but if you want to experiment
with something you think might work better that sgtm :)

[Kevin McNee - google account, 2016/10/24 21:11:47]

Yeah, after playing with this a bit, I guess it's not as bad as I thought. I
changed the wording of the TODO to be more open-ended.

Acknowledged.

+      if (!needsScrollbars.horizontal) {
+        this.pinchCenter_ = {
+          x: this.window_.innerWidth / 2,
+          y: this.window_.innerHeight / 2
+        };
+      }
+      this.setPinchZoomInternal_(
+          scaleDelta, this.frameToPluginCoordinate(ev.center));
+      this.updateViewport_();
+      this.prevScale = ev.scale;
+    }.bind(this));
+  },
+
+  pinchZoomStart: function(ev) {
+    this.pinchPhase_ = Viewport.PinchPhase.PINCH_START;
+    this.prevScale = 1;
+    this.oldCenterInContent =
+        this.frameToContent(this.frameToPluginCoordinate(ev.center));
+
+    var needsScrollbars = this.documentNeedsScrollbars_(this.zoom_);
+    this.keepContentCentered_ = (!needsScrollbars.horizontal);

[bokan, 2016/10/17 22:20:15]

Nit: no need for parentheses.

[Kevin McNee - google account, 2016/10/24 21:11:47]

Done.

+    // We keep track of begining of the pinch.
+    // By doing so we will be able to compute the pan distance.
+    this.first_pinch_center_in_frame_ = ev.center;
+  },
+
+  pinchZoomEnd: function(ev) {
+    this.mightZoom_(function() {
+      this.pinchPhase_ = Viewport.PinchPhase.PINCH_END;
+      var scaleDelta = ev.scale / this.prevScale;
+      this.pinchCenter_ = ev.center;
+
+      this.setPinchZoomInternal_(
+          scaleDelta, this.frameToPluginCoordinate(ev.center));
+      this.updateViewport_();
+    }.bind(this));
+    this.pinchPhase_ = Viewport.PinchPhase.PINCH_NONE;
+  },
+
+  frameToPluginCoordinate: function(coordinateInFrame) {
+    var container = $('plugin');
+    return {
+      x: coordinateInFrame.x - container.getBoundingClientRect().left,
+      y: coordinateInFrame.y - container.getBoundingClientRect().top
+    };
+  },
+
+  /**
    * Go to the given page index.
    * @param {number} page the index of the page to go to. zero-based.
    */