Bug 923431 - Fix the code that adjusts scroll offset while pinching; add tests to go with. r=botond

gfx/layers/ipc/AsyncPanZoomController.cpp

@@ -602,7 +602,7 @@ nsEventStatus AsyncPanZoomController::OnScaleBegin(const PinchGestureInput& aEve
   }
 
   SetState(PINCHING);
-  mLastZoomFocus = aEvent.mFocusPoint;
+  mLastZoomFocus = aEvent.mFocusPoint - mFrameMetrics.mCompositionBounds.TopLeft();
 
   return nsEventStatus_eConsumeNoDefault;
 }
@@ -619,13 +619,14 @@ nsEventStatus AsyncPanZoomController::OnScale(const PinchGestureInput& aEvent) {
     return nsEventStatus_eConsumeNoDefault;
   }
 
-  ScreenToScreenScale spanRatio(aEvent.mCurrentSpan / aEvent.mPreviousSpan);
+  float spanRatio = aEvent.mCurrentSpan / aEvent.mPreviousSpan;
 
   {
     ReentrantMonitorAutoEnter lock(mMonitor);
 
     CSSToScreenScale userZoom = mFrameMetrics.mZoom;
-    ScreenPoint focusPoint = aEvent.mFocusPoint;
+    ScreenPoint focusPoint = aEvent.mFocusPoint - mFrameMetrics.mCompositionBounds.TopLeft();
+    CSSPoint cssFocusPoint = focusPoint / userZoom;
 
     CSSPoint focusChange = (mLastZoomFocus - focusPoint) / userZoom;
     // If displacing by the change in focus point will take us off page bounds,
@@ -641,23 +642,23 @@ nsEventStatus AsyncPanZoomController::OnScale(const PinchGestureInput& aEvent) {
     // When we zoom in with focus, we can zoom too much towards the boundaries
     // that we actually go over them. These are the needed displacements along
     // either axis such that we don't overscroll the boundaries when zooming.
-    gfx::Point neededDisplacement;
+    CSSPoint neededDisplacement;
 
-    bool doScale = (spanRatio > ScreenToScreenScale(1.0) && userZoom < mMaxZoom) ||
+    bool doScale = (spanRatio > 1.0 && userZoom < mMaxZoom) ||
-                   (spanRatio < ScreenToScreenScale(1.0) && userZoom > mMinZoom);
+                   (spanRatio < 1.0 && userZoom > mMinZoom);
 
     if (doScale) {
-      spanRatio.scale = clamped(spanRatio.scale,
+      spanRatio = clamped(spanRatio,
                           mMinZoom.scale / userZoom.scale,
                           mMaxZoom.scale / userZoom.scale);
 
-      switch (mX.ScaleWillOverscroll(spanRatio, focusPoint.x))
+      switch (mX.ScaleWillOverscroll(spanRatio, cssFocusPoint.x))
       {
         case Axis::OVERSCROLL_NONE:
           break;
         case Axis::OVERSCROLL_MINUS:
         case Axis::OVERSCROLL_PLUS:
-          neededDisplacement.x = -mX.ScaleWillOverscrollAmount(spanRatio, focusPoint.x);
+          neededDisplacement.x = -mX.ScaleWillOverscrollAmount(spanRatio, cssFocusPoint.x);
           break;
         case Axis::OVERSCROLL_BOTH:
           // If scaling this way will make us overscroll in both directions, then
@@ -670,13 +671,13 @@ nsEventStatus AsyncPanZoomController::OnScale(const PinchGestureInput& aEvent) {
     }
 
     if (doScale) {
-      switch (mY.ScaleWillOverscroll(spanRatio, focusPoint.y))
+      switch (mY.ScaleWillOverscroll(spanRatio, cssFocusPoint.y))
       {
         case Axis::OVERSCROLL_NONE:
           break;
         case Axis::OVERSCROLL_MINUS:
         case Axis::OVERSCROLL_PLUS:
-          neededDisplacement.y = -mY.ScaleWillOverscrollAmount(spanRatio, focusPoint.y);
+          neededDisplacement.y = -mY.ScaleWillOverscrollAmount(spanRatio, cssFocusPoint.y);
           break;
         case Axis::OVERSCROLL_BOTH:
           doScale = false;
@@ -685,10 +686,10 @@ nsEventStatus AsyncPanZoomController::OnScale(const PinchGestureInput& aEvent) {
     }
 
     if (doScale) {
-      ScaleWithFocus(userZoom * spanRatio, focusPoint);
+      ScaleWithFocus(spanRatio, cssFocusPoint);
 
-      if (neededDisplacement != gfx::Point()) {
+      if (neededDisplacement != CSSPoint()) {
-        ScrollBy(CSSPoint::FromUnknownPoint(neededDisplacement));
+        ScrollBy(neededDisplacement);
       }
 
       ScheduleComposite();
@@ -977,19 +978,14 @@ void AsyncPanZoomController::ScrollBy(const CSSPoint& aOffset) {
   mFrameMetrics.mScrollOffset += aOffset;
 }
 
-void AsyncPanZoomController::ScaleWithFocus(const CSSToScreenScale& aZoom,
+void AsyncPanZoomController::ScaleWithFocus(float aScale,
-                                            const ScreenPoint& aFocus) {
+                                            const CSSPoint& aFocus) {
-  ScreenToScreenScale zoomFactor(aZoom.scale / mFrameMetrics.mZoom.scale);
+  SetZoomAndResolution(CSSToScreenScale(mFrameMetrics.mZoom.scale * aScale));
-  CSSToScreenScale resolution = mFrameMetrics.mZoom;
+  // We want to adjust the scroll offset such that the CSS point represented by aFocus remains
-
+  // at the same position on the screen before and after the change in zoom. The below code
-  SetZoomAndResolution(aZoom);
+  // accomplishes this; see https://bugzilla.mozilla.org/show_bug.cgi?id=923431#c6 for an
-
+  // in-depth explanation of how.
-  // If the new scale is very small, we risk multiplying in huge rounding
+  mFrameMetrics.mScrollOffset = (mFrameMetrics.mScrollOffset + aFocus) - (aFocus / aScale);
-  // errors, so don't bother adjusting the scroll offset.
-  if (resolution.scale >= 0.01f) {
-    zoomFactor.scale -= 1.0;
-    mFrameMetrics.mScrollOffset += aFocus * zoomFactor / resolution;
-  }
 }
 
 bool AsyncPanZoomController::EnlargeDisplayPortAlongAxis(float aSkateSizeMultiplier,

gfx/layers/ipc/AsyncPanZoomController.h

@@ -359,12 +359,10 @@ protected:
   /**
    * Scales the viewport by an amount (note that it multiplies this scale in to
    * the current scale, it doesn't set it to |aScale|). Also considers a focus
-   * point so that the page zooms outward from that point.
+   * point so that the page zooms inward/outward from that point.
-   *
-   * XXX: Fix focus point calculations.
    */
-  void ScaleWithFocus(const mozilla::CSSToScreenScale& aScale,
+  void ScaleWithFocus(float aScale,
-                      const ScreenPoint& aFocus);
+                      const CSSPoint& aFocus);
 
   /**
    * Schedules a composite on the compositor thread. Wrapper for

gfx/layers/ipc/Axis.cpp

@@ -271,12 +271,12 @@ float Axis::DisplacementWillOverscrollAmount(float aDisplacement) {
   }
 }
 
-Axis::Overscroll Axis::ScaleWillOverscroll(ScreenToScreenScale aScale, float aFocus) {
+Axis::Overscroll Axis::ScaleWillOverscroll(float aScale, float aFocus) {
-  float originAfterScale = (GetOrigin() + aFocus) * aScale.scale - aFocus;
+  float originAfterScale = (GetOrigin() + aFocus) - (aFocus / aScale);
 
   bool both = ScaleWillOverscrollBothSides(aScale);
-  bool minus = originAfterScale < GetPageStart() * aScale.scale;
+  bool minus = originAfterScale < GetPageStart();
-  bool plus = (originAfterScale + GetCompositionLength()) > GetPageEnd() * aScale.scale;
+  bool plus = (originAfterScale + (GetCompositionLength() / aScale)) > GetPageEnd();
 
   if ((minus && plus) || both) {
     return OVERSCROLL_BOTH;
@@ -290,12 +290,11 @@ Axis::Overscroll Axis::ScaleWillOverscroll(ScreenToScreenScale aScale, float aFo
   return OVERSCROLL_NONE;
 }
 
-float Axis::ScaleWillOverscrollAmount(ScreenToScreenScale aScale, float aFocus) {
+float Axis::ScaleWillOverscrollAmount(float aScale, float aFocus) {
-  float originAfterScale = (GetOrigin() + aFocus) * aScale.scale - aFocus;
+  float originAfterScale = (GetOrigin() + aFocus) - (aFocus / aScale);
   switch (ScaleWillOverscroll(aScale, aFocus)) {
-  case OVERSCROLL_MINUS: return originAfterScale - GetPageStart() * aScale.scale;
+  case OVERSCROLL_MINUS: return originAfterScale - GetPageStart();
-  case OVERSCROLL_PLUS: return (originAfterScale + GetCompositionLength()) -
+  case OVERSCROLL_PLUS: return originAfterScale + (GetCompositionLength() / aScale) - GetPageEnd();
-                               NS_lround(GetPageEnd() * aScale.scale);
   // Don't handle OVERSCROLL_BOTH. Client code is expected to deal with it.
   default: return 0;
   }
@@ -338,12 +337,12 @@ float Axis::GetPageLength() {
   return GetRectLength(pageRect);
 }
 
-bool Axis::ScaleWillOverscrollBothSides(ScreenToScreenScale aScale) {
+bool Axis::ScaleWillOverscrollBothSides(float aScale) {
   const FrameMetrics& metrics = mAsyncPanZoomController->GetFrameMetrics();
 
   CSSRect cssContentRect = metrics.mScrollableRect;
 
-  CSSToScreenScale scale = metrics.mZoom * aScale;
+  CSSToScreenScale scale(metrics.mZoom.scale * aScale);
   CSSIntRect cssCompositionBounds = RoundedIn(metrics.mCompositionBounds / scale);
 
   return GetRectLength(cssContentRect) < GetRectLength(CSSRect(cssCompositionBounds));

gfx/layers/ipc/Axis.h

@@ -155,7 +155,7 @@ public:
    * scroll offset in such a way that it remains in the same place on the page
    * relative.
    */
-  Overscroll ScaleWillOverscroll(ScreenToScreenScale aScale, float aFocus);
+  Overscroll ScaleWillOverscroll(float aScale, float aFocus);
 
   /**
    * If a scale will overscroll the axis, this returns the amount and in what
@@ -165,7 +165,7 @@ public:
    * scroll offset in such a way that it remains in the same place on the page
    * relative.
    */
-  float ScaleWillOverscrollAmount(ScreenToScreenScale aScale, float aFocus);
+  float ScaleWillOverscrollAmount(float aScale, float aFocus);
 
   /**
    * Checks if an axis will overscroll in both directions by computing the
@@ -174,7 +174,7 @@ public:
    *
    * This gets called by ScaleWillOverscroll().
    */
-  bool ScaleWillOverscrollBothSides(ScreenToScreenScale aScale);
+  bool ScaleWillOverscrollBothSides(float aScale);
 
   float GetOrigin();
   float GetCompositionLength();

gfx/tests/gtest/TestAsyncPanZoomController.cpp

@@ -53,6 +53,11 @@ public:
     ReentrantMonitorAutoEnter lock(mMonitor);
     mFrameMetrics = metrics;
   }
+
+  FrameMetrics GetFrameMetrics() {
+    ReentrantMonitorAutoEnter lock(mMonitor);
+    return mFrameMetrics;
+  }
 };
 
 class TestAPZCTreeManager : public APZCTreeManager {
@@ -115,6 +120,74 @@ TEST(AsyncPanZoomController, Constructor) {
   apzc->SetFrameMetrics(TestFrameMetrics());
 }
 
+TEST(AsyncPanZoomController, Pinch) {
+  nsRefPtr<MockContentController> mcc = new MockContentController();
+  nsRefPtr<TestAsyncPanZoomController> apzc = new TestAsyncPanZoomController(0, mcc);
+
+  FrameMetrics fm;
+  fm.mViewport = CSSRect(0, 0, 980, 480);
+  fm.mCompositionBounds = ScreenIntRect(200, 200, 100, 200);
+  fm.mScrollableRect = CSSRect(0, 0, 980, 1000);
+  fm.mScrollOffset = CSSPoint(300, 300);
+  fm.mZoom = CSSToScreenScale(2.0);
+  apzc->SetFrameMetrics(fm);
+  // the visible area of the document in CSS pixels is x=300 y=300 w=50 h=100
+
+  EXPECT_CALL(*mcc, SendAsyncScrollDOMEvent(_,_,_)).Times(2);
+  EXPECT_CALL(*mcc, RequestContentRepaint(_)).Times(1);
+
+  apzc->HandleInputEvent(PinchGestureInput(PinchGestureInput::PINCHGESTURE_START,
+                                           0,
+                                           ScreenPoint(250, 300),
+                                           10.0,
+                                           10.0));
+  apzc->HandleInputEvent(PinchGestureInput(PinchGestureInput::PINCHGESTURE_SCALE,
+                                           0,
+                                           ScreenPoint(250, 300),
+                                           12.5,
+                                           10.0));
+  apzc->HandleInputEvent(PinchGestureInput(PinchGestureInput::PINCHGESTURE_END,
+                                           0,
+                                           ScreenPoint(250, 300),
+                                           12.5,
+                                           12.5));
+
+  // the visible area of the document in CSS pixels is now x=305 y=310 w=40 h=80
+  fm = apzc->GetFrameMetrics();
+  EXPECT_EQ(fm.mZoom.scale, 2.5f);
+  EXPECT_EQ(fm.mScrollOffset.x, 305);
+  EXPECT_EQ(fm.mScrollOffset.y, 310);
+
+  // part 2 of the test, move to the top-right corner of the page and pinch and
+  // make sure we stay in the correct spot
+  fm.mZoom = CSSToScreenScale(2.0);
+  fm.mScrollOffset = CSSPoint(930, 5);
+  apzc->SetFrameMetrics(fm);
+  // the visible area of the document in CSS pixels is x=930 y=5 w=50 h=100
+
+  apzc->HandleInputEvent(PinchGestureInput(PinchGestureInput::PINCHGESTURE_START,
+                                           0,
+                                           ScreenPoint(250, 300),
+                                           10.0,
+                                           10.0));
+  apzc->HandleInputEvent(PinchGestureInput(PinchGestureInput::PINCHGESTURE_SCALE,
+                                           0,
+                                           ScreenPoint(250, 300),
+                                           5.0,
+                                           10.0));
+  apzc->HandleInputEvent(PinchGestureInput(PinchGestureInput::PINCHGESTURE_END,
+                                           0,
+                                           ScreenPoint(250, 300),
+                                           5.0,
+                                           5.0));
+
+  // the visible area of the document in CSS pixels is now x=880 y=0 w=100 h=200
+  fm = apzc->GetFrameMetrics();
+  EXPECT_EQ(fm.mZoom.scale, 1.0f);
+  EXPECT_EQ(fm.mScrollOffset.x, 880);
+  EXPECT_EQ(fm.mScrollOffset.y, 0);
+}
+
 TEST(AsyncPanZoomController, SimpleTransform) {
   TimeStamp testStartTime = TimeStamp::Now();
   // RefCounted class can't live in the stack