Bug 1391770 - Don't allow a faraway second tap to start a one-touch-pinch gesture. r?botond draft
authorKartikaya Gupta <kgupta@mozilla.com>
Sat, 19 Aug 2017 08:50:55 -0400
changeset 649355 1a947d782b4295163b752c607749d247919a10e4
parent 649145 978f68c17245c37283a3635629efff66d2cdcba9
child 727074 e8f2cd7f590f0cdfea14a3a4be6d3c0141ad30f5
push id75026
push userkgupta@mozilla.com
push dateSat, 19 Aug 2017 12:51:14 +0000
reviewersbotond
bugs1391770
milestone57.0a1
Bug 1391770 - Don't allow a faraway second tap to start a one-touch-pinch gesture. r?botond This patch adds a new tolerance pref, which controls how far the second touchdown is allowed to be from the first touchdown when detecting a multi-tap gesture such as double-tap or one-touch-pinch. This stops the one-touch-pinch code from inadvertently triggering when the user does a tap followed by a second tap far away from the first. The default value for the new pref is 5x the touch-start tolerance pref. This seems to provide a reasonable behaviour for me, although this value could probably be tuned. MozReview-Commit-ID: 63aAyGCbvoN
gfx/layers/apz/src/AsyncPanZoomController.cpp
gfx/layers/apz/src/AsyncPanZoomController.h
gfx/layers/apz/src/GestureEventListener.cpp
gfx/layers/apz/src/GestureEventListener.h
gfx/thebes/gfxPrefs.h
mobile/android/app/mobile.js
modules/libpref/init/all.js
--- a/gfx/layers/apz/src/AsyncPanZoomController.cpp
+++ b/gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ -356,16 +356,24 @@ typedef GenericFlingAnimation FlingAnima
  * \li\b apz.popups.enabled
  * Determines whether APZ is used for XUL popup widgets with remote content.
  * Ideally, this should always be true, but it is currently not well tested, and
  * has known issues, so needs to be prefable.
  *
  * \li\b apz.record_checkerboarding
  * Whether or not to record detailed info on checkerboarding events.
  *
+ * \li\b apz.second_tap_tolerance
+ * Constant describing the tolerance in distance we use, multiplied by the
+ * device DPI, within which a second tap is counted as part of a gesture
+ * continuing from the first tap. Making this larger allows the user more
+ * distance between the first and second taps in a "double tap" or "one touch
+ * pinch" gesture.\n
+ * Units: (real-world, i.e. screen) inches
+ *
  * \li\b apz.test.logging_enabled
  * Enable logging of APZ test data (see bug 961289).
  *
  * \li\b apz.touch_move_tolerance
  * See the description for apz.touch_start_tolerance below. This is a similar
  * threshold, except it is used to suppress touchmove events from being delivered
  * to content for NON-scrollable frames (or more precisely, for APZCs where
  * ArePointerEventsConsumable returns false).\n
@@ -827,16 +835,22 @@ AsyncPanZoomController::IsDestroyed() co
 }
 
 /* static */ScreenCoord
 AsyncPanZoomController::GetTouchStartTolerance()
 {
   return (gfxPrefs::APZTouchStartTolerance() * APZCTreeManager::GetDPI());
 }
 
+/* static */ScreenCoord
+AsyncPanZoomController::GetSecondTapTolerance()
+{
+  return (gfxPrefs::APZSecondTapTolerance() * APZCTreeManager::GetDPI());
+}
+
 /* static */AsyncPanZoomController::AxisLockMode AsyncPanZoomController::GetAxisLockMode()
 {
   return static_cast<AxisLockMode>(gfxPrefs::APZAxisLockMode());
 }
 
 bool
 AsyncPanZoomController::ArePointerEventsConsumable(TouchBlockState* aBlock, uint32_t aTouchPoints) {
   if (aTouchPoints == 0) {
--- a/gfx/layers/apz/src/AsyncPanZoomController.h
+++ b/gfx/layers/apz/src/AsyncPanZoomController.h
@@ -110,16 +110,22 @@ public:
    * Constant describing the tolerance in distance we use, multiplied by the
    * device DPI, before we start panning the screen. This is to prevent us from
    * accidentally processing taps as touch moves, and from very short/accidental
    * touches moving the screen.
    * Note: It's an abuse of the 'Coord' class to use it to represent a 2D
    *       distance, but it's the closest thing we currently have.
    */
   static ScreenCoord GetTouchStartTolerance();
+  /**
+   * Same as GetTouchStartTolerance, but the tolerance for how close the second
+   * tap has to be to the first tap in order to be counted as part of a multi-tap
+   * gesture (double-tap or one-touch-pinch).
+   */
+  static ScreenCoord GetSecondTapTolerance();
 
   AsyncPanZoomController(uint64_t aLayersId,
                          APZCTreeManager* aTreeManager,
                          const RefPtr<InputQueue>& aInputQueue,
                          GeckoContentController* aController,
                          GestureBehavior aGestures = DEFAULT_GESTURES);
 
   // --------------------------------------------------------------------------
--- a/gfx/layers/apz/src/GestureEventListener.cpp
+++ b/gfx/layers/apz/src/GestureEventListener.cpp
@@ -173,17 +173,30 @@ nsEventStatus GestureEventListener::Hand
     mTouchStartPosition = mLastTouchInput.mTouches[0].mLocalScreenPoint;
 
     if (sLongTapEnabled) {
       CreateLongTapTimeoutTask();
     }
     CreateMaxTapTimeoutTask();
     break;
   case GESTURE_FIRST_SINGLE_TOUCH_UP:
-    SetState(GESTURE_SECOND_SINGLE_TOUCH_DOWN);
+    if (SecondTapIsFar()) {
+      // If the second tap goes down far away from the first, then bail out
+      // of the gesture.
+      CancelLongTapTimeoutTask();
+      CancelMaxTapTimeoutTask();
+      mSingleTapSent = Nothing();
+      SetState(GESTURE_NONE);
+    } else {
+      // Otherwise, reset the touch start position so that, if this turns into
+      // a one-touch-pinch gesture, it uses the second tap's down position as
+      // the focus, rather than the first tap's.
+      mTouchStartPosition = mLastTouchInput.mTouches[0].mLocalScreenPoint;
+      SetState(GESTURE_SECOND_SINGLE_TOUCH_DOWN);
+    }
     break;
   default:
     NS_WARNING("Unhandled state upon single touch start");
     SetState(GESTURE_NONE);
     break;
   }
 
   return nsEventStatus_eIgnore;
@@ -235,22 +248,35 @@ nsEventStatus GestureEventListener::Hand
     NS_WARNING("Unhandled state upon multitouch start");
     SetState(GESTURE_NONE);
     break;
   }
 
   return rv;
 }
 
-bool GestureEventListener::MoveDistanceIsLarge()
+bool GestureEventListener::MoveDistanceExceeds(ScreenCoord aThreshold) const
 {
   const ParentLayerPoint start = mLastTouchInput.mTouches[0].mLocalScreenPoint;
   ParentLayerPoint delta = start - mTouchStartPosition;
   ScreenPoint screenDelta = mAsyncPanZoomController->ToScreenCoordinates(delta, start);
-  return (screenDelta.Length() > AsyncPanZoomController::GetTouchStartTolerance());
+  return (screenDelta.Length() > aThreshold);
+}
+
+bool GestureEventListener::MoveDistanceIsLarge() const
+{
+  return MoveDistanceExceeds(AsyncPanZoomController::GetTouchStartTolerance());
+}
+
+bool GestureEventListener::SecondTapIsFar() const
+{
+  // Allow a little more room here, because the is actually lifting their finger
+  // off the screen before replacing it, and that tends to have more error than
+  // wiggling the finger while on the screen.
+  return MoveDistanceExceeds(AsyncPanZoomController::GetSecondTapTolerance());
 }
 
 nsEventStatus GestureEventListener::HandleInputTouchMove()
 {
   nsEventStatus rv = nsEventStatus_eIgnore;
 
   switch (mState) {
   case GESTURE_NONE:
--- a/gfx/layers/apz/src/GestureEventListener.h
+++ b/gfx/layers/apz/src/GestureEventListener.h
@@ -140,17 +140,19 @@ private:
   nsEventStatus HandleInputTouchEnd();
   nsEventStatus HandleInputTouchMove();
   nsEventStatus HandleInputTouchCancel();
   void HandleInputTimeoutLongTap();
   void HandleInputTimeoutMaxTap(bool aDuringFastFling);
 
   void TriggerSingleTapConfirmedEvent();
 
-  bool MoveDistanceIsLarge();
+  bool MoveDistanceExceeds(ScreenCoord aThreshold) const;
+  bool MoveDistanceIsLarge() const;
+  bool SecondTapIsFar() const;
 
   /**
    * Returns current vertical span, counting from the where the user first put
    * her finger down.
    */
   ParentLayerCoord GetYSpanFromStartPoint();
 
   /**
--- a/gfx/thebes/gfxPrefs.h
+++ b/gfx/thebes/gfxPrefs.h
@@ -328,16 +328,17 @@ private:
   DECL_GFX_PREF(Live, "apz.overscroll.stop_distance_threshold", APZOverscrollStopDistanceThreshold, float, 5.0f);
   DECL_GFX_PREF(Live, "apz.overscroll.stop_velocity_threshold", APZOverscrollStopVelocityThreshold, float, 0.01f);
   DECL_GFX_PREF(Live, "apz.overscroll.stretch_factor",         APZOverscrollStretchFactor, float, 0.5f);
   DECL_GFX_PREF(Live, "apz.paint_skipping.enabled",            APZPaintSkipping, bool, true);
   DECL_GFX_PREF(Live, "apz.peek_messages.enabled",             APZPeekMessages, bool, true);
   DECL_GFX_PREF(Live, "apz.popups.enabled",                    APZPopupsEnabled, bool, false);
   DECL_GFX_PREF(Live, "apz.printtree",                         APZPrintTree, bool, false);
   DECL_GFX_PREF(Live, "apz.record_checkerboarding",            APZRecordCheckerboarding, bool, false);
+  DECL_GFX_PREF(Live, "apz.second_tap_tolerance",              APZSecondTapTolerance, float, 0.5f);
   DECL_GFX_PREF(Live, "apz.test.fails_with_native_injection",  APZTestFailsWithNativeInjection, bool, false);
   DECL_GFX_PREF(Live, "apz.test.logging_enabled",              APZTestLoggingEnabled, bool, false);
   DECL_GFX_PREF(Live, "apz.touch_move_tolerance",              APZTouchMoveTolerance, float, 0.1f);
   DECL_GFX_PREF(Live, "apz.touch_start_tolerance",             APZTouchStartTolerance, float, 1.0f/4.5f);
   DECL_GFX_PREF(Live, "apz.velocity_bias",                     APZVelocityBias, float, 0.0f);
   DECL_GFX_PREF(Live, "apz.velocity_relevance_time_ms",        APZVelocityRelevanceTime, uint32_t, 150);
   DECL_GFX_PREF(Live, "apz.x_skate_highmem_adjust",            APZXSkateHighMemAdjust, float, 0.0f);
   DECL_GFX_PREF(Live, "apz.x_skate_size_multiplier",           APZXSkateSizeMultiplier, float, 1.5f);
--- a/mobile/android/app/mobile.js
+++ b/mobile/android/app/mobile.js
@@ -557,16 +557,17 @@ pref("apz.fling_curve_function_y1", "0.4
 pref("apz.fling_curve_function_x2", "0.05");
 pref("apz.fling_curve_function_y2", "1.00");
 pref("apz.fling_curve_threshold_inches_per_ms", "0.01");
 // apz.fling_friction and apz.fling_stopped_threshold are currently ignored by Fennec.
 pref("apz.fling_friction", "0.004");
 pref("apz.fling_stopped_threshold", "0.0");
 pref("apz.max_velocity_inches_per_ms", "0.07");
 pref("apz.overscroll.enabled", true);
+pref("apz.second_tap_tolerance", "0.3");
 pref("apz.touch_move_tolerance", "0.03");
 pref("apz.touch_start_tolerance", "0.06");
 
 #ifdef NIGHTLY_BUILD
 // Temporary fix of Bug 1390145 for Fennec Nightly
 pref("apz.frame_delay.enabled", false);
 #endif
 
--- a/modules/libpref/init/all.js
+++ b/modules/libpref/init/all.js
@@ -737,16 +737,17 @@ pref("apz.popups.enabled", false);
 // Whether to print the APZC tree for debugging
 pref("apz.printtree", false);
 
 #ifdef NIGHTLY_BUILD
 pref("apz.record_checkerboarding", true);
 #else
 pref("apz.record_checkerboarding", false);
 #endif
+pref("apz.second_tap_tolerance", "0.5");
 pref("apz.test.logging_enabled", false);
 pref("apz.touch_start_tolerance", "0.1");
 pref("apz.touch_move_tolerance", "0.1");
 pref("apz.velocity_bias", "0.0");
 pref("apz.velocity_relevance_time_ms", 150);
 pref("apz.x_skate_highmem_adjust", "0.0");
 pref("apz.y_skate_highmem_adjust", "0.0");
 pref("apz.x_skate_size_multiplier", "1.25");