Bug 1632293 - Replace the BothFingersLifted() special case behaviour with a new pinch input type. r=tnikkel,botond

The BothFingersLifted() mechanism was a bit of a hack to handle the case that
with touch inputs, a user might be able to lift just one finger to transition
from a pinch to a pan or other kind of gesture. This isn't possible with
other kinds of pinch gestures, such as on trackpads or with mousewheels.
So instead of tracking that via special-case behaviour of mFocusPoint, or
adding an extra bool to the PinchGestureInput class, it seems cleaner to
separate the concerns by adding a new type of input event that explicitly
covers the "finger lifted" touch scenario. This patch implements this change,
and removes the BothFingersLifted() machinery.

Differential Revision: https://phabricator.services.mozilla.com/D74415

gfx/layers/apz/public/GeckoContentController.h

@@ -90,8 +90,9 @@ class GeckoContentController {
    * however it wishes. Note that this function is not called if the pinch is
    * prevented by content calling preventDefault() on the touch events, or via
    * use of the touch-action property.
-   * @param aType One of PINCHGESTURE_START, PINCHGESTURE_SCALE, or
-   *        PINCHGESTURE_END, indicating the phase of the pinch.
+   * @param aType One of PINCHGESTURE_START, PINCHGESTURE_SCALE,
+   *        PINCHGESTURE_FINGERLIFTED, or PINCHGESTURE_END, indicating the phase
+   *        of the pinch.
    * @param aGuid The guid of the APZ that is detecting the pinch. This is
    *        generally the root APZC for the layers id.
    * @param aSpanChange For the START or END event, this is always 0.

gfx/layers/apz/src/APZCTreeManager.cpp

@@ -2284,15 +2284,14 @@ void APZCTreeManager::SynthesizePinchGestureFromMouseWheel(
                                 oldSpan,
                                 newSpan,
                                 aWheelInput.modifiers};
-  PinchGestureInput pinchEnd{
-      PinchGestureInput::PINCHGESTURE_END,
-      aWheelInput.mTime,
-      aWheelInput.mTimeStamp,
-      ExternalPoint(0, 0),
-      PinchGestureInput::BothFingersLifted<ScreenPixel>(),
-      newSpan,
-      newSpan,
-      aWheelInput.modifiers};
+  PinchGestureInput pinchEnd{PinchGestureInput::PINCHGESTURE_END,
+                             aWheelInput.mTime,
+                             aWheelInput.mTimeStamp,
+                             ExternalPoint(0, 0),
+                             focusPoint,
+                             newSpan,
+                             newSpan,
+                             aWheelInput.modifiers};
 
   mInputQueue->ReceiveInputEvent(aTarget, confFlags, pinchStart, nullptr);
   mInputQueue->ReceiveInputEvent(aTarget, confFlags, pinchScale1, nullptr);

gfx/layers/apz/src/AsyncPanZoomController.cpp

@@ -1265,6 +1265,7 @@ nsEventStatus AsyncPanZoomController::HandleGestureEvent(
         case PinchGestureInput::PINCHGESTURE_SCALE:
           rv = OnScale(pinchGestureInput);
           break;
+        case PinchGestureInput::PINCHGESTURE_FINGERLIFTED:
         case PinchGestureInput::PINCHGESTURE_END:
           rv = OnScaleEnd(pinchGestureInput);
           break;
@@ -1751,8 +1752,8 @@ nsEventStatus AsyncPanZoomController::OnScaleEnd(
 
   mPinchEventBuffer.clear();
 
-  // Non-negative focus point would indicate that one finger is still down
-  if (aEvent.mLocalFocusPoint != PinchGestureInput::BothFingersLifted()) {
+  if (aEvent.mType == PinchGestureInput::PINCHGESTURE_FINGERLIFTED) {
+    // One finger is still down, so transition to a TOUCHING state
     if (mZoomConstraints.mAllowZoom) {
       mPanDirRestricted = false;
       StartTouch(aEvent.mLocalFocusPoint, aEvent.mTime);
@@ -1763,7 +1764,7 @@ nsEventStatus AsyncPanZoomController::OnScaleEnd(
       StartPanning(ToExternalPoint(aEvent.mScreenOffset, aEvent.mFocusPoint));
     }
   } else {
-    // Otherwise, handle the fingers being lifted.
+    // Otherwise, handle the gesture being completely done.
 
     // Some of the code paths below, like ScrollSnap() or HandleEndOfPan(),
     // may start an animation, but otherwise we want to end up in the NOTHING

gfx/layers/apz/src/GestureEventListener.cpp

@@ -480,16 +480,19 @@ nsEventStatus GestureEventListener::HandleInputTouchEnd() {
     case GESTURE_PINCH:
       if (mTouches.Length() < 2) {
         SetState(GESTURE_NONE);
-        ScreenPoint point = PinchGestureInput::BothFingersLifted<ScreenPixel>();
+        PinchGestureInput::PinchGestureType type =
+            PinchGestureInput::PINCHGESTURE_END;
+        ScreenPoint point;
         if (mTouches.Length() == 1) {
           // As user still keeps one finger down the event's focus point should
           // contain meaningful data.
+          type = PinchGestureInput::PINCHGESTURE_FINGERLIFTED;
           point = mTouches[0].mScreenPoint;
         }
-        PinchGestureInput pinchEvent(
-            PinchGestureInput::PINCHGESTURE_END, mLastTouchInput.mTime,
-            mLastTouchInput.mTimeStamp, mLastTouchInput.mScreenOffset, point,
-            1.0f, 1.0f, mLastTouchInput.modifiers);
+        PinchGestureInput pinchEvent(type, mLastTouchInput.mTime,
+                                     mLastTouchInput.mTimeStamp,
+                                     mLastTouchInput.mScreenOffset, point, 1.0f,
+                                     1.0f, mLastTouchInput.modifiers);
         mAsyncPanZoomController->HandleGestureEvent(pinchEvent);
       }
 
@@ -499,11 +502,10 @@ nsEventStatus GestureEventListener::HandleInputTouchEnd() {
 
     case GESTURE_ONE_TOUCH_PINCH: {
       SetState(GESTURE_NONE);
-      ScreenPoint point = PinchGestureInput::BothFingersLifted<ScreenPixel>();
       PinchGestureInput pinchEvent(
           PinchGestureInput::PINCHGESTURE_END, mLastTouchInput.mTime,
-          mLastTouchInput.mTimeStamp, mLastTouchInput.mScreenOffset, point,
-          1.0f, 1.0f, mLastTouchInput.modifiers);
+          mLastTouchInput.mTimeStamp, mLastTouchInput.mScreenOffset,
+          ScreenPoint(), 1.0f, 1.0f, mLastTouchInput.modifiers);
       mAsyncPanZoomController->HandleGestureEvent(pinchEvent);
 
       rv = nsEventStatus_eConsumeNoDefault;

gfx/layers/apz/test/gtest/APZTestCommon.h

@@ -869,10 +869,8 @@ void APZCTesterBase::PinchWithPinchInput(
   mcc->AdvanceBy(TIME_BETWEEN_PINCH_INPUT);
 
   actualStatus = aTarget->ReceiveInputEvent(
-      CreatePinchGestureInput(
-          PinchGestureInput::PINCHGESTURE_END,
-          PinchGestureInput::BothFingersLifted<ScreenPixel>(), 10.0 * aScale,
-          10.0 * aScale, mcc->Time()),
+      CreatePinchGestureInput(PinchGestureInput::PINCHGESTURE_END, aSecondFocus,
+                              10.0 * aScale, 10.0 * aScale, mcc->Time()),
       nullptr);
   if (aOutEventStatuses) {
     (*aOutEventStatuses)[2] = actualStatus;

gfx/layers/apz/util/APZCCallbackHelper.cpp

@@ -882,6 +882,7 @@ void APZCCallbackHelper::NotifyPinchGesture(
     case PinchGestureInput::PINCHGESTURE_SCALE:
       msg = eMagnifyGestureUpdate;
       break;
+    case PinchGestureInput::PINCHGESTURE_FINGERLIFTED:
     case PinchGestureInput::PINCHGESTURE_END:
       msg = eMagnifyGesture;
       break;

widget/InputData.cpp

@@ -554,11 +554,6 @@ PinchGestureInput::PinchGestureInput(
 
 bool PinchGestureInput::TransformToLocal(
     const ScreenToParentLayerMatrix4x4& aTransform) {
-  if (mFocusPoint == BothFingersLifted<ScreenPixel>()) {
-    // Special value, no transform required.
-    mLocalFocusPoint = BothFingersLifted();
-    return true;
-  }
   Maybe<ParentLayerPoint> point = UntransformBy(aTransform, mFocusPoint);
   if (!point) {
     return false;

widget/InputData.h

@@ -440,6 +440,13 @@ class PinchGestureInput : public InputData {
     PinchGestureType, (
       PINCHGESTURE_START,
       PINCHGESTURE_SCALE,
+      // The FINGERLIFTED state is used when a touch-based pinch gesture is
+      // terminated by lifting one of the two fingers. The position of the
+      // finger that's still down is populated as the focus point.
+      PINCHGESTURE_FINGERLIFTED,
+      // The END state is used when the pinch gesture is completely terminated.
+      // In this state, the focus point should not be relied upon for having
+      // meaningful data.
       PINCHGESTURE_END
   ));
   // clang-format on
@@ -463,9 +470,10 @@ class PinchGestureInput : public InputData {
   // point is implementation-specific, but can for example be the midpoint
   // between the very first and very last touch. This is in device pixels and
   // are the coordinates on the screen of this midpoint.
-  // For PINCHGESTURE_END events, this instead will hold the coordinates of
-  // the remaining finger, if there is one. If there isn't one then it will
-  // store |BothFingersLifted()|.
+  // For PINCHGESTURE_END events, this may hold the last known focus point or
+  // just be empty; in any case for END events it should not be relied upon.
+  // For PINCHGESTURE_FINGERLIFTED events, this holds the point of the finger
+  // that is still down.
   ScreenPoint mFocusPoint;
 
   // The screen offset of the root widget. This can be changing along with
@@ -485,20 +493,6 @@ class PinchGestureInput : public InputData {
   ScreenCoord mPreviousSpan;
 
   bool mHandledByAPZ;
-
-  // A special value for mFocusPoint used in PINCHGESTURE_END events to
-  // indicate that both fingers have been lifted. If only one finger has
-  // been lifted, the coordinates of the remaining finger are expected to
-  // be stored in mFocusPoint.
-  // For pinch events that were not triggered by touch gestures, the
-  // value of mFocusPoint in a PINCHGESTURE_END event is always expected
-  // to be this value.
-  // For convenience, we allow retrieving this value in any coordinate system.
-  // Since it's a special value, no conversion is needed.
-  template <typename Units = ParentLayerPixel>
-  static gfx::PointTyped<Units> BothFingersLifted() {
-    return gfx::PointTyped<Units>{-1, -1};
-  }
 };
 
 /**

widget/cocoa/nsChildView.mm

@@ -2849,10 +2849,6 @@ NSEvent* gLastDragMouseDownEvent = nil;  // [strong]
                           100.0 * (1.0 - [anEvent magnification]),
                           nsCocoaUtils::ModifiersForEvent(anEvent)};
 
-  if (pinchGestureType == PinchGestureInput::PINCHGESTURE_END) {
-    event.mFocusPoint = PinchGestureInput::BothFingersLifted<ScreenPixel>();
-  }
-
   // FIXME: bug 1525793 -- this may need to handle zooming or not on a per-document basis.
   if (StaticPrefs::apz_allow_zooming()) {
     mGeckoChild->DispatchAPZInputEvent(event);