From 227e130c848de7696be70f1bc0d194345f6d54b7 Mon Sep 17 00:00:00 2001 From: Kartikaya Gupta Date: Mon, 11 May 2020 16:56:57 +0000 Subject: [PATCH] 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 --- .../apz/public/GeckoContentController.h | 5 ++-- gfx/layers/apz/src/APZCTreeManager.cpp | 17 ++++++----- gfx/layers/apz/src/AsyncPanZoomController.cpp | 7 +++-- gfx/layers/apz/src/GestureEventListener.cpp | 18 ++++++------ gfx/layers/apz/test/gtest/APZTestCommon.h | 6 ++-- gfx/layers/apz/util/APZCCallbackHelper.cpp | 1 + widget/InputData.cpp | 5 ---- widget/InputData.h | 28 ++++++++----------- widget/cocoa/nsChildView.mm | 4 --- 9 files changed, 39 insertions(+), 52 deletions(-) diff --git a/gfx/layers/apz/public/GeckoContentController.h b/gfx/layers/apz/public/GeckoContentController.h index 35327365ca23..c60779508b40 100644 --- a/gfx/layers/apz/public/GeckoContentController.h +++ b/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. diff --git a/gfx/layers/apz/src/APZCTreeManager.cpp b/gfx/layers/apz/src/APZCTreeManager.cpp index 5194592cad3f..e0417e1fe663 100644 --- a/gfx/layers/apz/src/APZCTreeManager.cpp +++ b/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(), - 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); diff --git a/gfx/layers/apz/src/AsyncPanZoomController.cpp b/gfx/layers/apz/src/AsyncPanZoomController.cpp index 0312f0b75cdb..19ee4dc71f9e 100644 --- a/gfx/layers/apz/src/AsyncPanZoomController.cpp +++ b/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 diff --git a/gfx/layers/apz/src/GestureEventListener.cpp b/gfx/layers/apz/src/GestureEventListener.cpp index 4895929945c9..6349eafc98b5 100644 --- a/gfx/layers/apz/src/GestureEventListener.cpp +++ b/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(); + 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(); 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; diff --git a/gfx/layers/apz/test/gtest/APZTestCommon.h b/gfx/layers/apz/test/gtest/APZTestCommon.h index a61b61448518..a00cce551816 100644 --- a/gfx/layers/apz/test/gtest/APZTestCommon.h +++ b/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(), 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; diff --git a/gfx/layers/apz/util/APZCCallbackHelper.cpp b/gfx/layers/apz/util/APZCCallbackHelper.cpp index e51fa8be32ab..29826a68aa5e 100644 --- a/gfx/layers/apz/util/APZCCallbackHelper.cpp +++ b/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; diff --git a/widget/InputData.cpp b/widget/InputData.cpp index 949f694a007d..0964d3654acd 100644 --- a/widget/InputData.cpp +++ b/widget/InputData.cpp @@ -554,11 +554,6 @@ PinchGestureInput::PinchGestureInput( bool PinchGestureInput::TransformToLocal( const ScreenToParentLayerMatrix4x4& aTransform) { - if (mFocusPoint == BothFingersLifted()) { - // Special value, no transform required. - mLocalFocusPoint = BothFingersLifted(); - return true; - } Maybe point = UntransformBy(aTransform, mFocusPoint); if (!point) { return false; diff --git a/widget/InputData.h b/widget/InputData.h index a7fdc4236519..bea126e738e6 100644 --- a/widget/InputData.h +++ b/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 - static gfx::PointTyped BothFingersLifted() { - return gfx::PointTyped{-1, -1}; - } }; /** diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm index 01f462ac19f4..b8dbb971188b 100644 --- a/widget/cocoa/nsChildView.mm +++ b/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(); - } - // FIXME: bug 1525793 -- this may need to handle zooming or not on a per-document basis. if (StaticPrefs::apz_allow_zooming()) { mGeckoChild->DispatchAPZInputEvent(event);