Bug 1557424 - Re-clamp the scroll offset if the composition bounds changes. r=kats a=jcristau
authorBotond Ballo <botond@mozilla.com>
Fri, 07 Jun 2019 22:17:04 +0000
changeset 536829 e94a4d5cd811f4b766af8ba4970452bab29b3f4c
parent 536828 f2aa60a336fa6dfcec12da4e2e132c34d3973fb4
child 536830 eb71282f69a503677fe8a8fecabe38212149fbe9
push id2082
push userffxbld-merge
push dateMon, 01 Jul 2019 08:34:18 +0000
treeherdermozilla-release@2fb19d0466d2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskats, jcristau
bugs1557424, 1551582
milestone68.0
Bug 1557424 - Re-clamp the scroll offset if the composition bounds changes. r=kats a=jcristau This is similar to bug 1551582, but concerns a main-thread change to the composition bounds rather than the scrollable rect; either can result in the scroll range shrinking and therefore a need to re-clamp the scroll offset. Differential Revision: https://phabricator.services.mozilla.com/D34211
gfx/layers/apz/src/AsyncPanZoomController.cpp
gfx/layers/apz/test/gtest/TestTreeManager.cpp
--- a/gfx/layers/apz/src/AsyncPanZoomController.cpp
+++ b/gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ -4583,23 +4583,28 @@ void AsyncPanZoomController::NotifyLayer
       // viewport size got changed (e.g. due to orientation change, or content
       // changing the meta-viewport tag).
       Metrics().SetZoom(aLayerMetrics.GetZoom());
       mCompositedZoom = aLayerMetrics.GetZoom();
       Metrics().SetDevPixelsPerCSSPixel(
           aLayerMetrics.GetDevPixelsPerCSSPixel());
     }
     bool scrollableRectChanged = false;
+    bool compositionBoundsChanged = false;
     if (!Metrics().GetScrollableRect().IsEqualEdges(
             aLayerMetrics.GetScrollableRect())) {
       Metrics().SetScrollableRect(aLayerMetrics.GetScrollableRect());
       needContentRepaint = true;
       scrollableRectChanged = true;
     }
-    Metrics().SetCompositionBounds(aLayerMetrics.GetCompositionBounds());
+    if (!Metrics().GetCompositionBounds().IsEqualEdges(
+            aLayerMetrics.GetCompositionBounds())) {
+      Metrics().SetCompositionBounds(aLayerMetrics.GetCompositionBounds());
+      compositionBoundsChanged = true;
+    }
     Metrics().SetRootCompositionSize(aLayerMetrics.GetRootCompositionSize());
     Metrics().SetPresShellResolution(aLayerMetrics.GetPresShellResolution());
     Metrics().SetCumulativeResolution(aLayerMetrics.GetCumulativeResolution());
     mScrollMetadata.SetHasScrollgrab(aScrollMetadata.GetHasScrollgrab());
     mScrollMetadata.SetLineScrollAmount(aScrollMetadata.GetLineScrollAmount());
     mScrollMetadata.SetPageScrollAmount(aScrollMetadata.GetPageScrollAmount());
     mScrollMetadata.SetSnapInfo(ScrollSnapInfo(aScrollMetadata.GetSnapInfo()));
     // The scroll clip can differ between layers associated a given scroll
@@ -4677,20 +4682,20 @@ void AsyncPanZoomController::NotifyLayer
       // top displayport margin is zero) to the bottom of a page, which will
       // result in a displayport that doesn't extend upwards at all.
       // Note that even if the CancelAnimation call above requested a repaint
       // this is fine because we already have repaint request deduplication.
       needContentRepaint = true;
       // Since the main-thread scroll offset changed we should trigger a
       // recomposite to make sure it becomes user-visible.
       ScheduleComposite();
-    } else if (scrollableRectChanged) {
+    } else if (scrollableRectChanged || compositionBoundsChanged) {
       // Even if we didn't accept a new scroll offset from content, the
-      // scrollable rect may have changed in a way that makes our local
-      // scroll offset out of bounds, so re-clamp it.
+      // scrollable rect or composition bounds may have changed in a way that
+      // makes our local scroll offset out of bounds, so re-clamp it.
       ClampAndSetScrollOffset(Metrics().GetScrollOffset());
       ClampCompositedScrollOffset();
     }
   }
 
   if (smoothScrollRequested) {
     // A smooth scroll has been requested for animation on the compositor
     // thread.  This flag will be reset by the main thread when it receives
--- a/gfx/layers/apz/test/gtest/TestTreeManager.cpp
+++ b/gfx/layers/apz/test/gtest/TestTreeManager.cpp
@@ -110,18 +110,18 @@ TEST_F(APZCTreeManagerTester, Bug1198900
                        ScrollWheelInput::SCROLLMODE_INSTANT,
                        ScrollWheelInput::SCROLLDELTA_PIXEL, origin, 0, 10,
                        false, WheelDeltaAdjustmentStrategy::eNone);
   uint64_t blockId;
   manager->ReceiveInputEvent(swi, nullptr, &blockId);
   manager->ContentReceivedInputBlock(blockId, /* preventDefault= */ true);
 }
 
-// This test checks that APZ clamps the scroll offset it composites even if
-// the main thread fails to do so. (The main thread will always clamp its
+// The next two tests check that APZ clamps the scroll offset it composites even
+// if the main thread fails to do so. (The main thread will always clamp its
 // scroll offset internally, but it may not send APZ the clamped version for
 // scroll offset synchronization reasons.)
 TEST_F(APZCTreeManagerTester, Bug1551582) {
   // The simple layer tree has a scrollable rect of 500x500 and a composition
   // bounds of 200x200, leading to a scroll range of (0,0,300,300).
   CreateSimpleScrollingLayer();
   ScopedLayerTreeRegistration registration(manager, LayersId{0}, root, mcc);
   manager->UpdateHitTestingTree(LayersId{0}, root, false, LayersId{0}, 0);
@@ -146,8 +146,40 @@ TEST_F(APZCTreeManagerTester, Bug1551582
     aMetrics.SetScrollableRect(CSSRect(0, 0, 400, 400));
   });
   manager->UpdateHitTestingTree(LayersId{0}, root, false, LayersId{0}, 0);
 
   // Check that APZ has clamped the scroll offset to (200,200) for us.
   compositedScrollOffset = apzc->GetCompositedScrollOffset();
   EXPECT_EQ(CSSPoint(200, 200), compositedScrollOffset);
 }
+TEST_F(APZCTreeManagerTester, Bug1557424) {
+  // The simple layer tree has a scrollable rect of 500x500 and a composition
+  // bounds of 200x200, leading to a scroll range of (0,0,300,300).
+  CreateSimpleScrollingLayer();
+  ScopedLayerTreeRegistration registration(manager, LayersId{0}, root, mcc);
+  manager->UpdateHitTestingTree(LayersId{0}, root, false, LayersId{0}, 0);
+
+  // Simulate the main thread scrolling to the end of the scroll range.
+  ModifyFrameMetrics(root, [](FrameMetrics& aMetrics) {
+    aMetrics.SetScrollOffset(CSSPoint(300, 300));
+    aMetrics.SetScrollGeneration(1);
+    aMetrics.SetScrollOffsetUpdateType(FrameMetrics::eMainThread);
+  });
+  manager->UpdateHitTestingTree(LayersId{0}, root, false, LayersId{0}, 0);
+
+  // Sanity check.
+  RefPtr<TestAsyncPanZoomController> apzc = ApzcOf(root);
+  CSSPoint compositedScrollOffset = apzc->GetCompositedScrollOffset();
+  EXPECT_EQ(CSSPoint(300, 300), compositedScrollOffset);
+
+  // Simulate the main thread expanding the composition bounds to 300x300 (and
+  // thereby shrinking the scroll range to (0,0,200,200) without sending a new
+  // scroll offset update for the clamped scroll position (200,200).
+  ModifyFrameMetrics(root, [](FrameMetrics& aMetrics) {
+    aMetrics.SetCompositionBounds(ParentLayerRect(0, 0, 300, 300));
+  });
+  manager->UpdateHitTestingTree(LayersId{0}, root, false, LayersId{0}, 0);
+
+  // Check that APZ has clamped the scroll offset to (200,200) for us.
+  compositedScrollOffset = apzc->GetCompositedScrollOffset();
+  EXPECT_EQ(CSSPoint(200, 200), compositedScrollOffset);
+}