Bug 1502010 - Tighten up the ArePointerEventsConsumable checks. r=botond
☠☠ backed out by f3849030a93c ☠ ☠
authorKartikaya Gupta <kgupta@mozilla.com>
Mon, 26 Nov 2018 19:03:20 +0000
changeset 504522 67c5cdc1e812495104949f8c5e8ebe83a450719f
parent 504521 f324774fb8637b4c725b8edbc96aea80b2f1a040
child 504523 e9e34c0b62ac841810517ebb5c5a3dd4c7dd439a
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbotond
bugs1502010
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1502010 - Tighten up the ArePointerEventsConsumable checks. r=botond This patch tries to reduce the false-positive cases where ArePointerEventsConsumable returns true even though the input events won't actually result in panning. It does this by ascertaining the direction of panning (if possible) in the current input block and checking to see if panning can actually occur in that direction. Previously it would just check if panning could occur without taking into account the actual pan direction of the input events. Differential Revision: https://phabricator.services.mozilla.com/D12824
gfx/layers/apz/src/AsyncPanZoomController.cpp
gfx/layers/apz/src/AsyncPanZoomController.h
gfx/layers/apz/src/InputBlockState.cpp
gfx/layers/apz/src/InputBlockState.h
gfx/layers/apz/src/InputQueue.cpp
--- a/gfx/layers/apz/src/AsyncPanZoomController.cpp
+++ b/gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ -950,38 +950,57 @@ AsyncPanZoomController::GetSecondTapTole
 }
 
 /* static */AsyncPanZoomController::PinchLockMode AsyncPanZoomController::GetPinchLockMode()
 {
   return static_cast<PinchLockMode>(gfxPrefs::APZPinchLockMode());
 }
 
 bool
-AsyncPanZoomController::ArePointerEventsConsumable(TouchBlockState* aBlock, uint32_t aTouchPoints) {
-  if (aTouchPoints == 0) {
+AsyncPanZoomController::ArePointerEventsConsumable(TouchBlockState* aBlock, const MultiTouchInput& aInput) {
+  uint32_t touchPoints = aInput.mTouches.Length();
+  if (touchPoints == 0) {
     // Cant' do anything with zero touch points
     return false;
   }
 
-  // This logic is simplified, erring on the side of returning true
-  // if we're not sure. It's safer to pretend that we can consume the
-  // event and then not be able to than vice-versa.
+  // This logic is simplified, erring on the side of returning true if we're
+  // not sure. It's safer to pretend that we can consume the event and then
+  // not be able to than vice-versa. But at the same time, we should try hard
+  // to return an accurate result, because returning true can trigger a
+  // pointercancel event to web content, which can break certain features
+  // that are using touch-action and handling the pointermove events.
+  //
   // We could probably enhance this logic to determine things like "we're
   // not pannable, so we can only zoom in, and the zoom is already maxed
   // out, so we're not zoomable either" but no need for that at this point.
 
-  bool pannable = aBlock->GetOverscrollHandoffChain()->CanBePanned(this);
+  bool pannableX = aBlock->TouchActionAllowsPanningX() &&
+      aBlock->GetOverscrollHandoffChain()->CanScrollInDirection(this, ScrollDirection::eHorizontal);
+  bool pannableY = aBlock->TouchActionAllowsPanningY() &&
+      aBlock->GetOverscrollHandoffChain()->CanScrollInDirection(this, ScrollDirection::eVertical);
+
+  bool pannable;
+
+  Maybe<ScrollDirection> panDirection = aBlock->GetBestGuessPanDirection(aInput);
+  if (panDirection == Some(ScrollDirection::eVertical)) {
+    pannable = pannableY;
+  } else if (panDirection == Some(ScrollDirection::eHorizontal)) {
+    pannable = pannableX;
+  } else {
+    // If we don't have a guessed pan direction, err on the side of returning true.
+    pannable = pannableX || pannableY;
+  }
+
   bool zoomable = mZoomConstraints.mAllowZoom;
-
-  pannable &= (aBlock->TouchActionAllowsPanningX() || aBlock->TouchActionAllowsPanningY());
   zoomable &= (aBlock->TouchActionAllowsPinchZoom());
 
   // XXX once we fix bug 1031443, consumable should be assigned
-  // pannable || zoomable if aTouchPoints > 1.
-  bool consumable = (aTouchPoints == 1 ? pannable : zoomable);
+  // pannable || zoomable if touchPoints > 1.
+  bool consumable = (touchPoints == 1 ? pannable : zoomable);
   if (!consumable) {
     return false;
   }
 
   return true;
 }
 
 nsEventStatus AsyncPanZoomController::HandleDragEvent(const MouseInput& aEvent,
--- a/gfx/layers/apz/src/AsyncPanZoomController.h
+++ b/gfx/layers/apz/src/AsyncPanZoomController.h
@@ -1210,22 +1210,22 @@ private:
 public:
   /**
    * Flush a repaint request if one is needed, without throttling it with the
    * paint throttler.
    */
   void FlushRepaintForNewInputBlock();
 
   /**
-   * Given the number of touch points in an input event and touch block they
-   * belong to, check if the event can result in a panning/zooming behavior.
+   * Given an input event and the touch block it belongs to, check if the
+   * event can lead to a panning/zooming behavior.
    * This is primarily used to figure out when to dispatch the pointercancel
    * event for the pointer events spec.
    */
-  bool ArePointerEventsConsumable(TouchBlockState* aBlock, uint32_t aTouchPoints);
+  bool ArePointerEventsConsumable(TouchBlockState* aBlock, const MultiTouchInput& aInput);
 
   /**
    * Clear internal state relating to touch input handling.
    */
   void ResetTouchInputState();
 
   /**
    * Gets a ref to the input queue that is shared across the entire tree manager.
--- a/gfx/layers/apz/src/InputBlockState.cpp
+++ b/gfx/layers/apz/src/InputBlockState.cpp
@@ -1,16 +1,17 @@
 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "InputBlockState.h"
 
+#include "APZUtils.h"
 #include "AsyncPanZoomController.h"         // for AsyncPanZoomController
 #include "ScrollAnimationPhysics.h"         // for kScrollSeriesTimeoutMs
 #include "gfxPrefs.h"                       // for gfxPrefs
 #include "mozilla/MouseEvents.h"
 #include "mozilla/Telemetry.h"              // for Telemetry
 #include "mozilla/layers/IAPZCTreeManager.h" // for AllowedTouchBehavior
 #include "OverscrollHandoffState.h"
 #include "QueuedInput.h"
@@ -904,16 +905,39 @@ TouchBlockState::UpdateSlopState(const M
       // this block
       TBS_LOG("%p exiting slop\n", this);
       mInSlop = false;
     }
   }
   return mInSlop;
 }
 
+Maybe<ScrollDirection>
+TouchBlockState::GetBestGuessPanDirection(const MultiTouchInput& aInput)
+{
+  if (aInput.mType != MultiTouchInput::MULTITOUCH_MOVE ||
+      aInput.mTouches.Length() != 1) {
+    return Nothing();
+  }
+  ScreenPoint vector = aInput.mTouches[0].mScreenPoint - mSlopOrigin;
+  double angle = atan2(vector.y, vector.x); // range [-pi, pi]
+  angle = fabs(angle); // range [0, pi]
+
+  double angleThreshold = TouchActionAllowsPanningXY()
+      ? gfxPrefs::APZAxisLockAngle()
+      : gfxPrefs::APZAllowedDirectPanAngle();
+  if (apz::IsCloseToHorizontal(angle, angleThreshold)) {
+    return Some(ScrollDirection::eHorizontal);
+  }
+  if (apz::IsCloseToVertical(angle, angleThreshold)) {
+    return Some(ScrollDirection::eVertical);
+  }
+  return Nothing();
+}
+
 uint32_t
 TouchBlockState::GetActiveTouchCount() const
 {
   return mTouchCounter.GetActiveTouchCount();
 }
 
 KeyboardBlockState::KeyboardBlockState(const RefPtr<AsyncPanZoomController>& aTargetApzc)
   : InputBlockState(aTargetApzc, TargetConfirmationFlags{true})
--- a/gfx/layers/apz/src/InputBlockState.h
+++ b/gfx/layers/apz/src/InputBlockState.h
@@ -480,16 +480,23 @@ public:
    * the slop area is - if this is true the slop area is larger.
    * @return true iff the provided event is a touchmove in the slop area and
    *         so should not be sent to content.
    */
   bool UpdateSlopState(const MultiTouchInput& aInput,
                        bool aApzcCanConsumeEvents);
 
   /**
+   * Based on the slop origin and the given input event, return a best guess
+   * as to the pan direction of this touch block. Returns Nothing() if no guess
+   * can be made.
+   */
+  Maybe<ScrollDirection> GetBestGuessPanDirection(const MultiTouchInput& aInput);
+
+  /**
    * Returns the number of touch points currently active.
    */
   uint32_t GetActiveTouchCount() const;
 
   void DispatchEvent(const InputData& aEvent) const override;
   bool MustStayActive() override;
   const char* Type() override;
 
--- a/gfx/layers/apz/src/InputQueue.cpp
+++ b/gfx/layers/apz/src/InputQueue.cpp
@@ -146,17 +146,17 @@ InputQueue::ReceiveTouchInput(const RefP
   nsEventStatus result = nsEventStatus_eIgnore;
 
   // XXX calling ArePointerEventsConsumable on |target| may be wrong here if
   // the target isn't confirmed and the real target turns out to be something
   // else. For now assume this is rare enough that it's not an issue.
   if (block->IsDuringFastFling()) {
     INPQ_LOG("dropping event due to block %p being in fast motion\n", block);
     result = nsEventStatus_eConsumeNoDefault;
-  } else if (target && target->ArePointerEventsConsumable(block, aEvent.mTouches.Length())) {
+  } else if (target && target->ArePointerEventsConsumable(block, aEvent)) {
     if (block->UpdateSlopState(aEvent, true)) {
       INPQ_LOG("dropping event due to block %p being in slop\n", block);
       result = nsEventStatus_eConsumeNoDefault;
     } else {
       result = nsEventStatus_eConsumeDoDefault;
     }
   } else if (block->UpdateSlopState(aEvent, false)) {
     INPQ_LOG("dropping event due to block %p being in mini-slop\n", block);