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 4cb8fee0f66a994dd1959295b0ab8483d191890b..c83ff1ec13c02d3407e81042e4ae6d3931b31a3f 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.doRender_ = 1;
 
   window.addEventListener('scroll', this.updateViewport_.bind(this));
   window.addEventListener('resize', this.resize_.bind(this));
@@ -170,9 +172,7 @@ Viewport.prototype = {
    * @private
    * Called when the viewport should be updated.
    */
-  updateViewport_: function() {
-    this.viewportChangedCallback_();
-  },
+  updateViewport_: function() { this.viewportChangedCallback_(); },
 
   /**
    * @private
@@ -221,9 +221,7 @@ Viewport.prototype = {
   /**
    * @type {number} the zoom level of the viewport.
    */
-  get zoom() {
-    return this.zoom_;
-  },
+  get zoom() { return this.zoom_; },
 
   /**
    * @private
@@ -236,6 +234,13 @@ Viewport.prototype = {
     this.beforeZoomCallback_();
     this.allowedToChangeZoom_ = true;
     f();
+    var needsScrollbars = this.documentNeedsScrollbars_(this.zoom_);
+    if (!needsScrollbars.horizontal) {
+      this.pinchCenter_ = {

[wjmaclean, 2016/04/21 14:11:01]

Perhaps add a TODO here: at present the transition between using the window
centre and the gesture centre is liable to be a little 'jarring' ... might be
nice to think of some way of doing a smooth transition, perhaps a weighted
average of the window and gesture centres where the weight transitions smoothly
from one to the other over a preset number of frames?

[bokan, 2016/04/21 15:27:04]

This is the case where we don't have horizontal scrolling so we want to keep the
document centered. In that case, I think using the window center is fine (and
this feels fine to me). Although we probably don't want to do the centering
vertically.

[alessandroa, 2016/04/21 20:39:40]

I've explained today to David how by having the point in the middle of the view
helps us zooming.

+        x: this.window_.innerWidth / 2,
+        y: this.window_.innerHeight / 2
+      };
+    }
     this.allowedToChangeZoom_ = false;
     this.afterZoomCallback_();
   },
@@ -265,6 +270,61 @@ Viewport.prototype = {
   },
 
   /**
+   * @private
+   * Sets the cusotm zoom of the viewport.

[bokan, 2016/04/21 02:24:01]

"cusotm" -> "custom"

[alessandroa, 2016/04/21 20:39:39]

Acknowledged.

+   * Same function as below but for pinch zoom we have some more operations.
+   * @param {number} scaleDelta the zoom delta.
+   * @param {Object} center the pinch center

[bokan, 2016/04/21 18:20:17]

Describe the coordinates of the center value. i.e. "the pinch center in frame
coordinates" or "the pinch center in content coordinates"

[alessandroa, 2016/04/21 20:39:39]

Done.

+   */
+  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, this.zoom_);
+    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};
+    //Update last scale for the current pinch event
+    this.prevScale *= scaleDelta;

[bokan, 2016/04/21 15:22:06]

prevScale is used entirely in the handlers to calculate the delta so it makes
more sense to set it at the end of pinchZoom(). In fact, after calculating
scaleDelta you can set prevScale = ev.scale;

+  },
+
+  /**
+   *  @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
+   *  @param {Object} zoom level
+   *  @return {Objet} the new center in content
+   */
+  frameToContent: function(framePoint, scale) {
+    return {
+      x: (framePoint.x + this.position.x) / scale,

[bokan, 2016/04/21 15:22:06]

scale is always this.zoom_ so remove the argument and just use this.zoom_ here.

[alessandroa, 2016/04/21 20:39:40]

Done.

+      y: (framePoint.y + this.position.y) / scale

[bokan, 2016/04/21 15:22:06]

This doesn't really get you into the content since position.x doesn't take into
account the "gutter". So if you're zoomed out far so that you have ~200px on
each side of the document, this.position.x = 0. I'd consider this an existing
bug since position.y takes into account the top gutter. I suspect this is the
cause of some of the position jumps.

[bokan, 2016/04/21 15:59:02]

You can disregard this comment. While strange, there's a sort of consistency
here, the top gutter is considered part of the "scrollable area" while the side
gutters are not. Strange but...whatever.

+    };
+  },
+
+  /**
    * Sets the zoom to the given zoom level.
    * @param {number} newZoom the zoom level to zoom to.
    */
@@ -281,16 +341,12 @@ Viewport.prototype = {
   /**
    * @type {number} the width of scrollbars in the viewport in pixels.
    */
-  get scrollbarWidth() {
-    return this.scrollbarWidth_;
-  },
+  get scrollbarWidth() { return this.scrollbarWidth_; },
 
   /**
    * @type {Viewport.FittingType} the fitting type the viewport is currently in.
    */
-  get fittingType() {
-    return this.fittingType_;
-  },
+  get fittingType() { return this.fittingType_; },
 
   /**
    * @private
@@ -456,14 +512,26 @@ Viewport.prototype = {
       this.updateViewport_();
     }.bind(this));
   },
+  /**
+   * @private
+   * Computes vector between two points.
+   * @param {Object} First Point
+   * @param {Object} Second Point
+   * @return {Object} The vector
+   */
+  vectorDistance: function(p1, p2) {

[wjmaclean, 2016/04/21 14:11:01]

Maybe call the vectorDelta instead of invoking the notion of distance, since
this really isn't a distance?

[alessandroa, 2016/04/21 20:39:40]

Done.

+    var vector = {
+      x: p2.x - p1.x,
+      y: p2.y - p1.y
+    };
+    return vector;
+  },
 
   /**
    * Zoom the viewport so that a page consumes the entire viewport. Also scrolls
    * the viewport to the top of the current page.
    */
-  fitToPage: function() {
-    this.fitToPageInternal_(true);
-  },
+  fitToPage: function() { this.fitToPageInternal_(true); },
 
   /**
    * Zoom out to the next predefined zoom level.
@@ -498,6 +566,74 @@ Viewport.prototype = {
   },
 
   /**
+   * Pinch zoom event handler
+   * @param {ev} the pinch event
+   */
+  pinchZoom: function(ev) {
+    this.mightZoom_(function() {
+      if (ev.additionalEvent == 'pinchin')

[bokan, 2016/04/21 15:22:06]

Add a comment explaining why we render only on pinchin

+        this.doRender_ = 1;

[bokan, 2016/04/21 02:24:01]

Should doRender be true/false rather than a numeric value?

[wjmaclean, 2016/04/21 14:11:01]

Agreed, seems like doRender should be a Boolean.

[alessandroa, 2016/04/21 20:39:40]

True :)

+      else
+        this.doRender_ = 0;
+
+      var scaleDelta = ev.scale / this.prevScale;
+      this.pinchPanVector_ = this.vectorDistance(ev.center, this.pinch_start_);
+
+      var needsScrollbars = this.documentNeedsScrollbars_(
+          this.clampScale(this.zoom_ * scaleDelta));
+      // We must change the new content in center because now we have fake

[wjmaclean, 2016/04/21 14:11:01]

I'm never comfortable with this term 'fake scrollbars'. We either have
scrollbars or we don't, so please explain what you mean by fake :-)

[bokan, 2016/04/21 15:27:04]

Unrelated to this, but fun anecdote anyway, did you know that the scrollbars in
the PDF viewer are actually caused by keeping an invisible DIV behind the plugin
and growing it as the plugin zooms so that it causes scrollbars? In that sense,
they really are fake :)

[alessandroa, 2016/04/21 20:39:40]

That's pretty much my definition of fake .. 

+      // scrollbars

[bokan, 2016/04/21 15:22:06]

This comment should elaborate on what we're doing and why. Something along the
lines of: "If there's no horizontal scrolling, keep the content centered so the
user can't zoom in on the non-content area". The changeCenter_ variable is
likewise vague. It might be more descriptive as "keepContentCentered_" or
similar.

[alessandroa, 2016/04/21 20:39:40]

Acknowledged.

+      if (this.changeCenter_ && needsScrollbars.horizontal) {
+        this.oldCenterInContent =
+            this.frameToContent(this.containerCenter(ev.center), this.zoom_);
+        this.changeCenter_ = false;
+      }
+
+      this.pinchCenter_ = ev.center;
+      this.setPinchZoomInternal_(scaleDelta, this.containerCenter(ev.center));
+      this.updateViewport_();
+    }.bind(this));
+  },
+
+  pinchZoomStart: function(ev) {
+    //We render the document once before pinch.
+    //By doing so we ensure the image_data_ in the plugin is the correct one

[bokan, 2016/04/21 02:24:01]

Shouldn't doRender_ be = 1 then? Or I'm misunderstanding the comment...

[wjmaclean, 2016/04/21 14:11:01]

I would have thought that we are continuing to use the rendering (texture) from
before the pinch started, which (if everything is working correctly) should be
completely up to date. Is that correct?

[bokan, 2016/04/21 15:27:04]

If so, this comment is misleading.

[alessandroa, 2016/04/21 20:39:40]

Yes. We are using the texture from before the pinch start and that means we do
not want a render => this.doRender_ = false is ok :)

[alessandroa, 2016/04/21 20:39:40]

Old comment. Sorry!

+    this.doRender_ = 0;

[bokan, 2016/04/21 18:20:17]

This isn't inside a mightZoom though so we won't send the "viewport" message to
the plugin until we call pinchZoom but there we set doRender so this will get
clobbered. Either remove this line or maybe we need pinchZoomStart to also use
mightZoom_.

[alessandroa, 2016/04/21 20:39:40]

True. I removed the doRender_ as we don't send the message to the plugin. If you
remember by sending both pinchStart and pinch we will get the same event twice
in the plugin and we don't want that :)

+    this.prevScale = 1;
+    this.oldCenterInContent =
+        this.frameToContent(this.containerCenter(ev.center), this.zoom_);
+
+    var needsScrollbars = this.documentNeedsScrollbars_(this.zoom_);
+    if (!needsScrollbars.horizontal)
+      this.changeCenter_ = true;
+    else
+      this.changeCenter_ = false;
+    // We keep track of begining of the pinch.
+    // By doing so we will be able to compute the pan distance.
+    this.pinch_start_ = ev.center;

[bokan, 2016/04/21 15:22:06]

More descriptive name please. pinch_start_ -> first_pinch_center_in_frame_

[alessandroa, 2016/04/21 20:39:40]

Acknowledged.

+  },
+
+  pinchZoomEnd: function(ev) {
+    this.mightZoom_(function() {
+      // We want to render the document on pinch end
+      this.doRender_ = 1;
+      var scaleDelta = ev.scale / this.prevScale;
+      this.pinchCenter_ = ev.center;
+
+      this.setPinchZoomInternal_(scaleDelta, this.containerCenter(ev.center));
+      this.updateViewport_();
+    }.bind(this));
+  },
+
+  containerCenter: function(pageCenter) {

[bokan, 2016/04/21 15:22:06]

This function would be better described as "frameToPluginCoordinate". The
argument should just be called "coordinateInFrame".

container.getBoundingClientRect().{left|top} will always be 0 today but I think
its good to keep this if that ever changes. Unless you meant this function to
convert into the "content" (i.e. non-grey gutter), in which case this is not
what that does.

[alessandroa, 2016/04/21 20:39:40]

Acknowledged.

+    var container = $('plugin');
+    return {
+      x: pageCenter.x - container.getBoundingClientRect().left,
+      y: pageCenter.y - container.getBoundingClientRect().top
+    };
+  },
+
+  /**
    * Go to the given page index.
    * @param {number} page the index of the page to go to. zero-based.
    */