Lightbox 2.0: add basic pinch zoom #12219

cathyxz · 2017-11-28T00:50:00Z

This PR adds basic pinch zoom to amp-lightbox-viewer via image-viewer, tested on Android/Chrome (iOS testing in progress).

Some small tweaks were made to the behavior of PinchRecognizer and SwipeXYRecognizer because the default implementation keeps causing pinches to be miscategorized as swipes. Comments inline.

TODOs:

iOS testing.
fix tests. (Forgot that GestureRecognizers have lovely and elaborate tests).

Added a event.preventDefault() for iOS safari. The pinch zoom is now pretty smooth. =)

cathyxz · 2017-11-28T00:51:20Z

src/gesture-recognizers.js

@@ -691,42 +693,44 @@ export class PinchRecognizer extends GestureRecognizer {
  /** @override */
  onTouchStart(e) {
    const touches = e.touches;


Should only stop tracking this series if touches are more than 2. If there has been one touch, PinchRecognizer should wait for a second touch, since practically speaking, when you pinch a phone, the touches very rarely happen simultaneously.

cathyxz · 2017-11-28T00:52:26Z

src/gesture-recognizers.js

  }

  /** @override */
  onTouchMove(e) {
    const touches = e.touches;
-    if (!touches || touches.length != 2) {
-      return false;


Instead of returning false, which causes PinchRecognizer to stop tracking this event series, we return true, but do not act on the touch event unless the number of touches is explicitly 2.

cathyxz · 2017-11-28T00:54:54Z

src/gesture-recognizers.js

+        const dx2 = this.lastX2_ - this.startX2_;
+        const dy2 = this.lastY2_ - this.startY2_;
+        // Fingers should move in opposite directions and go over the threshold.
+        if (dx1 * dx2 <= 0 && dy1 * dy2 <= 0) {


The two boundaries here were tweaked from 8 to 3 and 10 respectively.

Lowered the standards for registering a signalReady from 8 to 3. Empirically on the android tester, most pinch zooms here registered a delta of 3-8. Another possibility is to look at the total distance and to see if it is larger than 8.

Increased the standard for dropping this event series from 8 to 10, since swipes are usually a lot more generous than pinches.
This will be finalized after iOS testing.

Empirical values should be sufficient, especially when distinguishing between two gestures. Let's move these thresholds from magic literals into named magic constants at the module level, like:

/** * Threshold in pixels for ... */ const PINCH_THRESHOLD = 4;

cathyxz · 2017-11-28T00:55:47Z

src/image-viewer.js

@@ -339,17 +340,25 @@ export class ImageViewer {
      const deltaX = this.viewerBox_.width / 2 - e.data.clientX;
      const deltaY = this.viewerBox_.height / 2 - e.data.clientY;
      this.onZoom_(newScale, deltaX, deltaY, true).then(() => {
-        return this.onZoomRelease_(0, 0, 0, 0, 0, 0);
+        return this.onZoomRelease_();


Refactored this, since DoubleTap zoom and Pinch zoom only needs a subset of the onZoomRelease functionality necessary for TapZoom.

cathyxz · 2017-11-28T00:57:55Z

src/image-viewer.js

+ * @param {number} deltaY
+ *  @param {number} dir
+ * @private
+ */


This is just a wrapper for naming conventions. We could also take it out and use onZoomInc. I'm not actually sure why it's called onZoomInc. Maybe onZoomEvent / onZoomAction would be more appropriate? Feedback welcome. =)

Yeah onZoomInc is probably not the best name. I generally prefer to avoid abbreviations in variable and function names for this reason. I guess it was onZoomIncrement?
Maybe a better name could describe the action that the function accomplishes. zoomToPoint? zoomInstant? Naming is hard : P

cathyxz · 2017-11-28T00:58:45Z

src/image-viewer.js

+   * released.
+   * @return {!Promise}
+   * @private
+   */


Basically refactored out this shared portion so that DoubleTap zoom and Pinch zoom can use this without unnecessary variables.

cathyxz · 2017-11-28T23:49:06Z

/to @aghassemi @cvializ

cvializ

Looks awesome! I just have a few thoughts and questions

cvializ · 2017-11-29T00:55:45Z

src/gesture-recognizers.js

-    const touches = e.changedTouches || e.touches;
-    if (touches && touches.length == 1) {
+    const touches = e.touches;
+    if (!touches || touches.length != 1) {


I think this might be clearer with touches && touches.length == 1 and a return false; at the end of the function. Thinking about what touches.length != 1 might mean takes a little more effort to reason about the condition.

cvializ · 2017-11-29T00:56:07Z

src/gesture-recognizers.js

-    const touches = e.changedTouches || e.touches;
-    if (touches && touches.length == 1) {
+    const touches = e.touches;
+    if (!touches || touches.length != 1) {


Ditto the above

cvializ · 2017-11-29T00:57:47Z

src/gesture-recognizers.js

@@ -271,8 +273,10 @@ class SwipeRecognizer extends GestureRecognizer {

  /** @override */
  onTouchMove(e) {
-    const touches = e.changedTouches || e.touches;


Does changing this from e.changedTouches || e.touches; to just touches = e.touches; change the behavior? I notice the the TapzoomRecognizer#onTouchMove still uses e.changedTouches. Can you help me understand the different cases?

So changedTouches refers to all touches involved in this event. Aka touches that moved, initiated, or were removed. touches refers to all contact points with the screen. This changes the functionality of swipe so that it only works when one finger is on the screen--basically no matter how many changedTouches there are, if more than one touch is on the screen, then we don't register a swipe. This was done because a large number of pinches were aggressively misread as swipes.

For TapZoom, I'm not sure if we consider one finger on the screen (not moving), and a second finger moving a TapZoom or a Pinch. If we assign that to Pinch and not TapZoom, then we can make the same change to TapZoom.

Cool, that makes sense. My initial impression is that one stationary finger and one moving finger should be a pinch zoom and not a tap zoom. macOS seems to require both fingers moving, but iOS's default behavior seems to allow one finger stationary or both fingers moving to pinch zoom

That makes sense. I've gone ahead and updated TapZoom to const touches = e.touches as well.

cvializ · 2017-11-29T00:58:00Z

src/gesture-recognizers.js

@@ -512,7 +516,9 @@ export class TapzoomRecognizer extends GestureRecognizer {
  /** @override */
  onTouchMove(e) {
    const touches = e.changedTouches || e.touches;
-    if (touches && touches.length == 1) {
+    if (!touches || touches.length != 1) {


Ditto the above

cvializ · 2017-11-29T01:12:28Z

src/gesture-recognizers.js

+        const dx2 = this.lastX2_ - this.startX2_;
+        const dy2 = this.lastY2_ - this.startY2_;
+        // Fingers should move in opposite directions and go over the threshold.
+        if (dx1 * dx2 <= 0 && dy1 * dy2 <= 0) {


Empirical values should be sufficient, especially when distinguishing between two gestures. Let's move these thresholds from magic literals into named magic constants at the module level, like:

/** * Threshold in pixels for ... */ const PINCH_THRESHOLD = 4;

cvializ · 2017-11-29T01:18:26Z

src/image-viewer.js

+ * @param {number} centerClientY
+ * @param {number} deltaX
+ * @param {number} deltaY
+ *  @param {number} dir


nit: the whitespace in this comment is incorrect. The stars should be at the same indentation as the first star in the jsdoc comment opener.

cvializ · 2017-11-29T02:24:47Z

src/image-viewer.js

+ * @param {number} deltaY
+ *  @param {number} dir
+ * @private
+ */


Yeah onZoomInc is probably not the best name. I generally prefer to avoid abbreviations in variable and function names for this reason. I guess it was onZoomIncrement?
Maybe a better name could describe the action that the function accomplishes. zoomToPoint? zoomInstant? Naming is hard : P

aghassemi · 2017-11-29T17:42:23Z

src/gesture-recognizers.js

+        const dx2 = this.lastX2_ - this.startX2_;
+        const dy2 = this.lastY2_ - this.startY2_;
+        // Fingers should move in opposite directions and go over the threshold.
+        if (dx1 * dx2 <= 0 && dy1 * dy2 <= 0) {


aghassemi · 2017-11-29T17:47:04Z

src/image-viewer.js

            e.data.deltaX, e.data.deltaY, e.data.velocityY, e.data.velocityY);
      }
    });
+    gestures.onGesture(PinchRecognizer, e => {
+      // To disable iOS 10 Safari default pinch zoom
+      event.preventDefault();


I suggest removing disableTouchZoom from enterOverlayMode (a new PR might be better, maybe an issue to do it later).

Reasoning is:
1- the old disableTouchZoom don't work on iOS anymore
2- Disabling touchzoom for every overlay is not the right thing to do. It only makes sense for image viewer and similar cases not every lightbox.

Opened #12254 as a separate PR.

aghassemi · 2017-11-29T17:51:17Z

src/image-viewer.js

+  onTapZoom_(centerClientX, centerClientY, deltaX, deltaY) {
+    const dir = Math.abs(deltaY) > Math.abs(deltaX) ?
+      Math.sign(deltaY) : Math.sign(-deltaX);
+    this.onZoomInc_(centerClientX, centerClientY, deltaX, deltaY, dir);


made me think of Monster Inc :). Mayeb a different name? performZoom, applyZoom?

cathyxz · 2017-11-29T23:03:35Z

@cvializ @aghassemi PTAL at your leisure. =)

cvializ

LGTM!!

* Add basic pinch zoom * Correct if / else chain in GestureRecognizers for clarity * Reindent comments and rename onZoomInc to zoomToPoint * Pull out threshold constants

googlebot added the cla: yes label Nov 28, 2017

cathyxz commented Nov 28, 2017

View reviewed changes

cathyxz force-pushed the feature/lightbox-pinch-zoom branch 2 times, most recently from b2b37fc to 2079d7e Compare November 28, 2017 23:15

Add basic pinch zoom

6e2c52e

cathyxz force-pushed the feature/lightbox-pinch-zoom branch from 2079d7e to 6e2c52e Compare November 28, 2017 23:22

aghassemi requested review from aghassemi and cvializ November 28, 2017 23:49

cvializ reviewed Nov 29, 2017

View reviewed changes

aghassemi reviewed Nov 29, 2017

View reviewed changes

cathyxz mentioned this pull request Nov 29, 2017

remove disableTouchZoom from enterOverlayMode #12251

Closed

cathyxz added 3 commits November 29, 2017 10:43

Correct if / else chain in GestureRecognizers for clarity

c4af6df

Reindent comments and rename onZoomInc to zoomToPoint

762b23e

Pull out threshold constants

57bc8b7

aghassemi approved these changes Nov 30, 2017

View reviewed changes

cvializ approved these changes Nov 30, 2017

View reviewed changes

cvializ merged commit f7d7a03 into ampproject:master Dec 1, 2017

cathyxz deleted the feature/lightbox-pinch-zoom branch December 1, 2017 22:06

ghost pushed a commit that referenced this pull request Dec 6, 2017

Lightbox 2.0: add basic pinch zoom (#12219)

5262d86

* Add basic pinch zoom * Correct if / else chain in GestureRecognizers for clarity * Reindent comments and rename onZoomInc to zoomToPoint * Pull out threshold constants

cathyxz mentioned this pull request Dec 6, 2017

Lightbox 2.0: image zoom/pan functionality #9977

Closed

4 tasks

ampprojectbot added the PR use: In Beta / Experimental label Dec 6, 2017

aghassemi mentioned this pull request Dec 11, 2017

Status Update 2017-12-11 #12402

Closed

mattwomple mentioned this pull request Mar 7, 2018

implement pinch zoom on <amp-image-lightbox> #9891

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lightbox 2.0: add basic pinch zoom #12219

Lightbox 2.0: add basic pinch zoom #12219

cathyxz commented Nov 28, 2017 •

edited

cathyxz Nov 28, 2017

cathyxz Nov 28, 2017

cathyxz Nov 28, 2017 •

edited

cvializ Nov 29, 2017

aghassemi Nov 29, 2017

cathyxz Nov 28, 2017

cathyxz Nov 28, 2017

cvializ Nov 29, 2017

cathyxz Nov 28, 2017

cathyxz commented Nov 28, 2017

cvializ left a comment

cvializ Nov 29, 2017

cvializ Nov 29, 2017

cvializ Nov 29, 2017

cathyxz Nov 29, 2017

cvializ Nov 29, 2017

cathyxz Nov 29, 2017

cvializ Nov 29, 2017

cvializ Nov 29, 2017

cvializ Nov 29, 2017

cvializ Nov 29, 2017

aghassemi Nov 29, 2017

aghassemi Nov 29, 2017

cathyxz Nov 29, 2017 •

edited

aghassemi Nov 29, 2017

cathyxz commented Nov 29, 2017

cvializ left a comment

Lightbox 2.0: add basic pinch zoom #12219

Lightbox 2.0: add basic pinch zoom #12219

Conversation

cathyxz commented Nov 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cathyxz Nov 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cathyxz commented Nov 28, 2017

cvializ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cathyxz Nov 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cathyxz commented Nov 29, 2017

cvializ left a comment

Choose a reason for hiding this comment

cathyxz commented Nov 28, 2017 •

edited

cathyxz Nov 28, 2017 •

edited

cathyxz Nov 29, 2017 •

edited