Make synthetic pinch zooms accurate

This patch makes synthetic pinch zoom gestures zoom to the correct
scale. We do this by fixing two bugs in the scale gesture detector.

The first is error due to touch slop. Slop is the amount that the touch
points must move before the zoom gesture is activated. Unfortunately,
the gesture recognizer drops the entire event that crosses the slop
threshold.

The second issue is similar but has to do with the minimum scaling
span. The "span" is the distance between the two fingers in the pinch
gesture. The minimum scaling span is the span at which point making
the span smaller doesn't change the zoom level. Similarly to slop,
the event that crosses this threshold is entirely dropped. At high
speeds this resulted in losing a significant amount of delta.

This CL fixes these issues by modifying the prev_span_ field on the
first event that initiates a scale to be at the threshold, thus
contributing the delta beyond it.

I moved the reset and end that happens when a zoom out crosses
the min span threshold to happen after we send an update, which allows
us to send the final delta, clamped to the min span, before ending the
gesture. I also start a pinch-zoom if the initial span is large enough
but the first update causes us to cross this threshold. This means a
single update can cuase a PinchBegin, PinchUpdate, PinchEnd.

It also fixes existing tests that didn't expect that the first touch
move that initiates a pinch zoom would also include a gesture pinch
update.

Bug: 610021
Change-Id: I22e2ff8d4b89f5da67ab102658175dbb5f2b2cec
Reviewed-on: https://chromium-review.googlesource.com/784213
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525348}

third_party/WebKit/LayoutTests/inspector-protocol/input/dispatchTouchEvent-expected.txt

@@ -192,13 +192,13 @@ Dispatching event:
     touchPoints : [
         [0] : {
             id : <number>
-            x : 120
-            y : 130
+            x : 25
+            y : 36
         }
         [1] : {
             id : <number>
-            x : 200
-            y : 100
+            x : 101
+            y : 202
         }
     ]
     type : touchMove
@@ -207,8 +207,8 @@ Dispatching event:
 type: touchmove
 ----Touches----
 id: 0
-pageX: 120
-pageY: 130
+pageX: 25
+pageY: 36
 radiusX: 1
 radiusY: 1
 rotationAngle: 0
@@ -224,15 +224,15 @@ force: 1
 type: touchmove
 ----Touches----
 id: 0
-pageX: 120
-pageY: 130
+pageX: 25
+pageY: 36
 radiusX: 1
 radiusY: 1
 rotationAngle: 0
 force: 1
 id: 1
-pageX: 200
-pageY: 100
+pageX: 101
+pageY: 202
 radiusX: 1
 radiusY: 1
 rotationAngle: 0
@@ -243,8 +243,8 @@ Dispatching event:
     touchPoints : [
         [0] : {
             id : <number>
-            x : 200
-            y : 300
+            x : 103
+            y : 203
         }
     ]
     type : touchMove
@@ -253,15 +253,15 @@ Dispatching event:
 type: touchmove
 ----Touches----
 id: 0
-pageX: 120
-pageY: 130
+pageX: 25
+pageY: 36
 radiusX: 1
 radiusY: 1
 rotationAngle: 0
 force: 1
 id: 1
-pageX: 200
-pageY: 300
+pageX: 103
+pageY: 203
 radiusX: 1
 radiusY: 1
 rotationAngle: 0
@@ -270,8 +270,8 @@ force: 1
 type: touchend
 ----Touches----
 id: 0
-pageX: 200
-pageY: 300
+pageX: 103
+pageY: 203
 radiusX: 1
 radiusY: 1
 rotationAngle: 0

third_party/WebKit/LayoutTests/inspector-protocol/input/dispatchTouchEvent.js

@@ -114,20 +114,20 @@
   await dispatchEvent({
     type: 'touchMove',
     touchPoints: [{
-      x: 120,
-      y: 130,
+      x: 25,
+      y: 36,
       id: 0
     }, {
-      x: 200,
-      y: 100,
+      x: 101,
+      y: 202,
       id: 1
     }]
   });
   await dispatchEvent({
     type: 'touchMove',
     touchPoints: [{
-      x: 200,
-      y: 300,
+      x: 103,
+      y: 203,
       id: 1
     }]
   });

ui/aura/gestures/gesture_recognizer_unittest.cc

@@ -1906,7 +1906,7 @@ TEST_F(GestureRecognizerTest, AsynchronousGestureRecognition) {
   EXPECT_FALSE(queued_delegate->scroll_end());
 
   // Move the second touch-point enough so that it is considered a pinch. This
-  // should generate both SCROLL_BEGIN and PINCH_BEGIN gestures.
+  // should generate SCROLL_BEGIN, PINCH_BEGIN, and PINCH_UPDATE gestures.
   queued_delegate->Reset();
   delegate->Reset();
   ui::TouchEvent move(
@@ -1969,7 +1969,7 @@ TEST_F(GestureRecognizerTest, AsynchronousGestureRecognition) {
   EXPECT_TRUE(queued_delegate->scroll_update());
   EXPECT_FALSE(queued_delegate->scroll_end());
   EXPECT_TRUE(queued_delegate->pinch_begin());
-  EXPECT_FALSE(queued_delegate->pinch_update());
+  EXPECT_TRUE(queued_delegate->pinch_update());
   EXPECT_FALSE(queued_delegate->pinch_end());
 }
 
@@ -2024,9 +2024,8 @@ TEST_F(GestureRecognizerTest, GestureEventPinchFromScroll) {
       ui::ET_TOUCH_MOVED, gfx::Point(95, 201), tes.Now(),
       ui::PointerDetails(ui::EventPointerType::POINTER_TYPE_TOUCH, kTouchId1));
   DispatchEventUsingWindowDispatcher(&move3);
-  EXPECT_2_EVENTS(delegate->events(),
-                  ui::ET_GESTURE_SCROLL_UPDATE,
-                  ui::ET_GESTURE_PINCH_BEGIN);
+  EXPECT_3_EVENTS(delegate->events(), ui::ET_GESTURE_SCROLL_UPDATE,
+                  ui::ET_GESTURE_PINCH_BEGIN, ui::ET_GESTURE_PINCH_UPDATE);
   EXPECT_EQ(gfx::Rect(10, 10, 85, 191).ToString(),
             delegate->bounding_box().ToString());
 
@@ -2087,10 +2086,10 @@ TEST_F(GestureRecognizerTest, GestureEventPinchFromScrollFromPinch) {
   DispatchEventUsingWindowDispatcher(&press2);
   EXPECT_FALSE(delegate->pinch_begin());
 
-  // Touch move triggers pinch begin.
+  // Touch move triggers pinch begin and update.
   tes.SendScrollEvent(event_sink(), 130, 230, kTouchId1, delegate.get());
   EXPECT_TRUE(delegate->pinch_begin());
-  EXPECT_FALSE(delegate->pinch_update());
+  EXPECT_TRUE(delegate->pinch_update());
 
   // Touch move triggers pinch update.
   tes.SendScrollEvent(event_sink(), 160, 200, kTouchId1, delegate.get());
@@ -2166,10 +2165,9 @@ TEST_F(GestureRecognizerTest, GestureEventPinchFromTap) {
       ui::ET_TOUCH_MOVED, gfx::Point(65, 201), tes.Now(),
       ui::PointerDetails(ui::EventPointerType::POINTER_TYPE_TOUCH, kTouchId1));
   DispatchEventUsingWindowDispatcher(&move3);
-  EXPECT_3_EVENTS(delegate->events(),
-                  ui::ET_GESTURE_SCROLL_BEGIN,
-                  ui::ET_GESTURE_SCROLL_UPDATE,
-                  ui::ET_GESTURE_PINCH_BEGIN);
+  EXPECT_4_EVENTS(delegate->events(), ui::ET_GESTURE_SCROLL_BEGIN,
+                  ui::ET_GESTURE_SCROLL_UPDATE, ui::ET_GESTURE_PINCH_BEGIN,
+                  ui::ET_GESTURE_PINCH_UPDATE);
   EXPECT_EQ(gfx::Rect(10, 10, 55, 191).ToString(),
             delegate->bounding_box().ToString());
 
@@ -2511,9 +2509,8 @@ TEST_F(GestureRecognizerTest, PinchScrollWithPreventDefaultedRelease) {
 
   delegate->Reset();
   delegate->ReceivedAck();
-  EXPECT_2_EVENTS(delegate->events(),
-                  ui::ET_GESTURE_SCROLL_UPDATE,
-                  ui::ET_GESTURE_PINCH_BEGIN);
+  EXPECT_3_EVENTS(delegate->events(), ui::ET_GESTURE_SCROLL_UPDATE,
+                  ui::ET_GESTURE_PINCH_BEGIN, ui::ET_GESTURE_PINCH_UPDATE);
 
   // Ack the first release. Although the release is processed, it should still
   // generate a pinch-end event.
@@ -3802,10 +3799,9 @@ TEST_F(GestureRecognizerTest, CancelAllActiveTouches) {
       ui::ET_TOUCH_MOVED, gfx::Point(350, 300), tes.Now(),
       ui::PointerDetails(ui::EventPointerType::POINTER_TYPE_TOUCH, kTouchId2));
   DispatchEventUsingWindowDispatcher(&move);
-  EXPECT_3_EVENTS(delegate->events(),
-                  ui::ET_GESTURE_SCROLL_BEGIN,
-                  ui::ET_GESTURE_SCROLL_UPDATE,
-                  ui::ET_GESTURE_PINCH_BEGIN);
+  EXPECT_4_EVENTS(delegate->events(), ui::ET_GESTURE_SCROLL_BEGIN,
+                  ui::ET_GESTURE_SCROLL_UPDATE, ui::ET_GESTURE_PINCH_BEGIN,
+                  ui::ET_GESTURE_PINCH_UPDATE);
   EXPECT_EQ(2, handler->touch_pressed_count());
   delegate->Reset();
   handler->Reset();
@@ -4223,7 +4219,7 @@ TEST_F(GestureRecognizerTest, PinchAlternatelyConsumedTest) {
   EXPECT_TRUE(delegate->scroll_begin());
   EXPECT_TRUE(delegate->scroll_update());
   EXPECT_TRUE(delegate->pinch_begin());
-  EXPECT_FALSE(delegate->pinch_update());
+  EXPECT_TRUE(delegate->pinch_update());
   delegate->Reset();
 
   const float expected_scales[] = {1.5f, 1.2f, 1.125f};
@@ -4370,10 +4366,9 @@ TEST_F(GestureRecognizerWithSwitchTest, GestureEventSmallPinchDisabled) {
       ui::PointerDetails(ui::EventPointerType::POINTER_TYPE_TOUCH, kTouchId1));
   DispatchEventUsingWindowDispatcher(&move1);
 
-  EXPECT_3_EVENTS(delegate->events(),
-                  ui::ET_GESTURE_SCROLL_BEGIN,
-                  ui::ET_GESTURE_SCROLL_UPDATE,
-                  ui::ET_GESTURE_PINCH_BEGIN);
+  EXPECT_4_EVENTS(delegate->events(), ui::ET_GESTURE_SCROLL_BEGIN,
+                  ui::ET_GESTURE_SCROLL_UPDATE, ui::ET_GESTURE_PINCH_BEGIN,
+                  ui::ET_GESTURE_PINCH_UPDATE);
 
   // No pinch update occurs, as kCompensateForUnstablePinchZoom is on and
   // |min_pinch_update_span_delta| was nonzero, and this is a very small pinch.
@@ -4413,10 +4408,9 @@ TEST_F(GestureRecognizerTest, GestureEventSmallPinchEnabled) {
       ui::PointerDetails(ui::EventPointerType::POINTER_TYPE_TOUCH, kTouchId1));
   DispatchEventUsingWindowDispatcher(&move1);
 
-  EXPECT_3_EVENTS(delegate->events(),
-                  ui::ET_GESTURE_SCROLL_BEGIN,
-                  ui::ET_GESTURE_SCROLL_UPDATE,
-                  ui::ET_GESTURE_PINCH_BEGIN);
+  EXPECT_4_EVENTS(delegate->events(), ui::ET_GESTURE_SCROLL_BEGIN,
+                  ui::ET_GESTURE_SCROLL_UPDATE, ui::ET_GESTURE_PINCH_BEGIN,
+                  ui::ET_GESTURE_PINCH_UPDATE);
 
   delegate->Reset();
   ui::TouchEvent move2(

ui/events/gesture_detection/gesture_provider.cc

@@ -230,7 +230,9 @@ class GestureProvider::GestureListenerImpl : public ScaleGestureListener,
                const MotionEvent& e) override {
     if (ignore_multitouch_zoom_events_ && !detector.InAnchoredScaleMode())
       return false;
+    bool first_scale = false;
     if (!pinch_event_sent_) {
+      first_scale = true;
       Send(CreateGesture(ET_GESTURE_PINCH_BEGIN,
                          e.GetPointerId(),
                          e.GetToolType(),
@@ -259,8 +261,13 @@ class GestureProvider::GestureListenerImpl : public ScaleGestureListener,
       // For historical reasons, Chrome has instead adopted a scale factor
       // computation that is invariant to the focal distance, where
       // the scale delta remains constant if the touch velocity is constant.
-      float dy =
-          (detector.GetCurrentSpanY() - detector.GetPreviousSpanY()) * 0.5f;
+      // Note: Because we calculate the scale here manually based on the
+      // y-span, but the scale factor accounts for slop in the first previous
+      // span, we manaully reproduce the behavior here for previous span y.
+      float prev_y = first_scale
+                         ? config_.gesture_detector_config.touch_slop * 2
+                         : detector.GetPreviousSpanY();
+      float dy = (detector.GetCurrentSpanY() - prev_y) * 0.5f;
       scale = std::pow(scale > 1 ? 1.0f + kDoubleTapDragZoomSpeed
                                  : 1.0f - kDoubleTapDragZoomSpeed,
                        std::abs(dy));

ui/events/gesture_detection/scale_gesture_detector.cc

@@ -21,6 +21,12 @@ namespace {
 
 const float kScaleFactor = .5f;
 
+// Using a small epsilon when comparing slop distances allows pixel
+// perfect slop determination when using fractional DPI coordinates
+// (assuming the slop region and DPI scale are reasonably
+// proportioned).
+const float kSlopEpsilon = .05f;
+
 }  // namespace
 
 // Note: These constants were taken directly from the default (unscaled)
@@ -56,14 +62,8 @@ ScaleGestureDetector::ScaleGestureDetector(const Config& config,
       event_before_or_above_starting_gesture_event_(false) {
   DCHECK(listener_);
 
-  // Using a small epsilon when comparing slop distances allows pixel
-  // perfect slop determination when using fractional DPI coordinates
-  // (assuming the slop region and DPI scale are reasonably
-  // proportioned).
-  const float kSlopEpsilon = .05f;
-
-  span_slop_ = config.span_slop + kSlopEpsilon;
-  min_span_ = config.min_scaling_span + kSlopEpsilon;
+  span_slop_ = config.span_slop;
+  min_span_ = config.min_scaling_span;
 }
 
 ScaleGestureDetector::~ScaleGestureDetector() {}
@@ -178,8 +178,7 @@ bool ScaleGestureDetector::OnTouchEvent(const MotionEvent& event) {
   const bool was_in_progress = in_progress_;
   focus_x_ = focus_x;
   focus_y_ = focus_y;
-  if (!InAnchoredScaleMode() && in_progress_ &&
-      (span < min_span_ || config_changed)) {
+  if (!InAnchoredScaleMode() && in_progress_ && config_changed) {
     listener_->OnScaleEnd(*this, event);
     ResetScaleWithSpan(span);
   }
@@ -190,11 +189,22 @@ bool ScaleGestureDetector::OnTouchEvent(const MotionEvent& event) {
   }
 
   const float min_span = InAnchoredScaleMode() ? span_slop_ : min_span_;
-  if (!in_progress_ && span >= min_span &&
-      (was_in_progress || std::abs(span - initial_span_) > span_slop_)) {
+  bool span_exceeds_min_span = span >= min_span + kSlopEpsilon ||
+                               initial_span_ >= min_span + kSlopEpsilon;
+  if (!in_progress_ && span_exceeds_min_span &&
+      (was_in_progress ||
+       std::abs(span - initial_span_) > span_slop_ + kSlopEpsilon)) {
+    float zoom_sign = span > initial_span_ ? 1 : -1;
+
     prev_span_x_ = curr_span_x_ = span_x;
     prev_span_y_ = curr_span_y_ = span_y;
-    prev_span_ = curr_span_ = span;
+    curr_span_ = span;
+
+    // To ensure we don't lose any delta when the first event crosses the min
+    // and slop thresholds, the prev_span on the first update will be the point
+    // at which zooming would have started.
+    prev_span_ = std::max(initial_span_ + zoom_sign * span_slop_, min_span);
+
     prev_time_ = curr_time_;
     in_progress_ = listener_->OnScaleBegin(*this, event);
   }
@@ -207,9 +217,8 @@ bool ScaleGestureDetector::OnTouchEvent(const MotionEvent& event) {
 
     bool update_prev = true;
 
-    if (in_progress_) {
+    if (in_progress_)
       update_prev = listener_->OnScale(*this, event);
-    }
 
     if (update_prev) {
       prev_span_x_ = curr_span_x_;
@@ -219,6 +228,12 @@ bool ScaleGestureDetector::OnTouchEvent(const MotionEvent& event) {
     }
   }
 
+  if (!InAnchoredScaleMode() && in_progress_ &&
+      span < min_span_ + kSlopEpsilon) {
+    listener_->OnScaleEnd(*this, event);
+    ResetScaleWithSpan(span);
+  }
+
   return true;
 }
 
@@ -245,20 +260,28 @@ float ScaleGestureDetector::GetPreviousSpanX() const { return prev_span_x_; }
 float ScaleGestureDetector::GetPreviousSpanY() const { return prev_span_y_; }
 
 float ScaleGestureDetector::GetScaleFactor() const {
+  float curr_span = curr_span_;
   if (InAnchoredScaleMode()) {
     // Drag is moving up; the further away from the gesture start, the smaller
     // the span should be, the closer, the larger the span, and therefore the
     // larger the scale.
     const bool scale_up = (event_before_or_above_starting_gesture_event_ &&
-                           (curr_span_ < prev_span_)) ||
+                           (curr_span < prev_span_)) ||
                           (!event_before_or_above_starting_gesture_event_ &&
-                           (curr_span_ > prev_span_));
+                           (curr_span > prev_span_));
     const float span_diff =
-        (std::abs(1.f - (curr_span_ / prev_span_)) * kScaleFactor);
+        (std::abs(1.f - (curr_span / prev_span_)) * kScaleFactor);
     return prev_span_ <= 0 ? 1.f
                            : (scale_up ? (1.f + span_diff) : (1.f - span_diff));
   }
-  return prev_span_ > 0 ? curr_span_ / prev_span_ : 1;
+
+  // If this will be the last update because this event crossed the min
+  // threshold, calculate the update as if the event stopped right at the
+  // boundary.
+  if (curr_span < min_span_ + kSlopEpsilon)
+    curr_span = min_span_;
+
+  return prev_span_ > 0 ? curr_span / prev_span_ : 1;
 }
 
 base::TimeDelta ScaleGestureDetector::GetTimeDelta() const {