Bug 1550510 - Stop hoisting scrollinfo items inside filters when WR is enabled. r=botond
authorKartikaya Gupta <kgupta@mozilla.com>
Sat, 18 May 2019 00:17:50 +0000
changeset 474426 c0a07a2c5d0263b35ceb271c668a41256b959630
parent 474425 f78634b8bff9ce0b8850e5f9246e7821ded4ef4e
child 474427 366a49e70140c78e213fe86b7616e2c8c88fc906
push id85802
push userkgupta@mozilla.com
push dateSat, 18 May 2019 00:18:24 +0000
treeherderautoland@c0a07a2c5d02 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbotond
bugs1550510, 1527182
milestone68.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 1550510 - Stop hoisting scrollinfo items inside filters when WR is enabled. r=botond In bug 1527182 we made it so that APZ can directly drag-scroll scrollframes that are inside SVG effects, because that's possible with WR on the compositor. However the code changed in that bug was meant to be kept in sync with a second piece of code. The second piece of code controls the generation of ScrollInfo items for scrollframes inside SVG effects - since we can APZ-scroll them with WR, we don't need the scrollinfo item anymore. Producing the scrollinfo item was changing the structure of the APZ tree in terms of where the transform ended up, and was causing badness with untransforming the drag mouse events. This patch adds a test that covers the scenario and also corrects the defect by bringing the two bits of code back in sync. Differential Revision: https://phabricator.services.mozilla.com/D31647
gfx/layers/apz/test/mochitest/helper_bug1550510.html
gfx/layers/apz/test/mochitest/test_group_mouseevents.html
layout/painting/nsDisplayList.cpp
layout/painting/nsDisplayList.h
layout/xul/nsSliderFrame.cpp
new file mode 100644
--- /dev/null
+++ b/gfx/layers/apz/test/mochitest/helper_bug1550510.html
@@ -0,0 +1,64 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <meta charset="utf-8">
+  <meta name="viewport" content="width=device-width; initial-scale=1.0">
+  <title>Dragging the mouse on a scrollbar for a transformed, filtered scrollframe</title>
+  <script type="application/javascript" src="apz_test_native_event_utils.js"></script>
+  <script type="application/javascript" src="apz_test_utils.js"></script>
+  <script src="/tests/SimpleTest/paint_listener.js"></script>
+  <script type="text/javascript">
+
+function* test(testDriver) {
+  var scrollableDiv = document.getElementById("scrollable");
+  scrollableDiv.addEventListener("scroll", () => setTimeout(testDriver, 0), {once: true});
+
+  // Scroll down a small amount (10px). The bug in this case is that the
+  // scrollthumb "jumps" most of the way down the scroll track because with
+  // WR enabled the filter and transform display items combine to generate an
+  // incorrect APZC tree, and the mouse position gets untransformed incorrectly.
+  // Given the scrollable height of 2000px and scrollframe height of 400px,
+  // the scrollthumb should be approximately 80px tall, and dragging it 10px
+  // should scroll approximately 50 pixels. If the bug manifests, it will get
+  // dragged an extra ~150px and scroll to approximately 1250px.
+  var dragFinisher = yield* dragVerticalScrollbar(scrollableDiv, testDriver, 10, 10);
+  if (!dragFinisher) {
+    ok(true, "No scrollbar, can't do this test");
+    return;
+  }
+
+  // the events above might be stuck in APZ input queue for a bit until the
+  // layer is activated, so we wait here until the scroll event listener is
+  // triggered.
+  yield;
+
+  yield* dragFinisher();
+
+  // Flush everything just to be safe
+  yield flushApzRepaints(testDriver);
+
+  // In this case we just want to make sure the scroll position moved from 0
+  // which indicates the thumb dragging worked properly.
+  ok(scrollableDiv.scrollTop < 100, "Scrollbar drag resulted in a scroll position of " + scrollableDiv.scrollTop);
+}
+
+waitUntilApzStable()
+.then(runContinuation(test))
+.then(subtestDone);
+
+  </script>
+</head>
+<body>
+    <div style="position: fixed; left: 100px; top: 100px; width: 400px; height: 600px">
+        <div style="transform: translateY(150px); will-change: transform">
+            <div style="filter: grayscale(80%)">
+                <div id="scrollable" style="height: 400px; overflow-y: auto">
+                    <div style="min-height: 2000px">
+                        yay text
+                    </div>
+                </div>
+            </div>
+        </div>
+    </div>
+</body>
+</html>
--- a/gfx/layers/apz/test/mochitest/test_group_mouseevents.html
+++ b/gfx/layers/apz/test/mochitest/test_group_mouseevents.html
@@ -27,20 +27,23 @@ var subtests = [
   // Test for scrollbar-dragging on a transformed scrollframe inside a fixed-pos item
   {"file": "helper_bug1462961.html"},
   // Scrollbar dragging where we exercise the snapback behaviour by moving the
   // mouse away from the scrollbar during drag
   {"file": "helper_scrollbar_snap_bug1501062.html"},
   // Tests for scrollbar-dragging on scrollframes inside nested transforms
   {"file": "helper_bug1490393.html"},
   {"file": "helper_bug1490393-2.html"},
+  // Scrollbar-dragging on scrollframes inside filters inside transforms
+  {"file": "helper_bug1550510.html"},
 ];
 
 if (isApzEnabled()) {
   SimpleTest.waitForExplicitFinish();
+  SimpleTest.expectAssertions(0, 2); // from helper_bug1550510.html, bug 1232856
   window.onload = function() {
     runSubtestsSeriallyInFreshWindows(subtests)
     .then(SimpleTest.finish, SimpleTest.finish);
   };
 }
 
   </script>
 </head>
--- a/layout/painting/nsDisplayList.cpp
+++ b/layout/painting/nsDisplayList.cpp
@@ -2354,16 +2354,25 @@ void nsDisplayListBuilder::ExitSVGEffect
   mSVGEffectsBuildingDepth--;
   MOZ_ASSERT(mSVGEffectsBuildingDepth >= 0);
   MOZ_ASSERT(mScrollInfoItemsForHoisting);
   if (mSVGEffectsBuildingDepth == 0) {
     mScrollInfoItemsForHoisting = nullptr;
   }
 }
 
+bool nsDisplayListBuilder::ShouldBuildScrollInfoItemsForHoisting() const {
+  /*
+   * Note: if changing the conditions under which scroll info layers
+   * are created, make a corresponding change to
+   * ScrollFrameWillBuildScrollInfoLayer() in nsSliderFrame.cpp.
+   */
+  return !gfxVars::UseWebRender() && mSVGEffectsBuildingDepth > 0;
+}
+
 void nsDisplayListBuilder::AppendNewScrollInfoItemForHoisting(
     nsDisplayScrollInfoLayer* aScrollInfoItem) {
   MOZ_ASSERT(ShouldBuildScrollInfoItemsForHoisting());
   MOZ_ASSERT(mScrollInfoItemsForHoisting);
   mScrollInfoItemsForHoisting->AppendToTop(aScrollInfoItem);
 }
 
 void nsDisplayListBuilder::BuildCompositorHitTestInfoIfNeeded(
--- a/layout/painting/nsDisplayList.h
+++ b/layout/painting/nsDisplayList.h
@@ -1650,24 +1650,17 @@ class nsDisplayListBuilder {
 
   void RemoveFromWillChangeBudget(nsIFrame* aFrame);
 
   void ClearWillChangeBudget();
 
   void EnterSVGEffectsContents(nsDisplayList* aHoistedItemsStorage);
   void ExitSVGEffectsContents();
 
-  /**
-   * Note: if changing the conditions under which scroll info layers
-   * are created, make a corresponding change to
-   * ScrollFrameWillBuildScrollInfoLayer() in nsSliderFrame.cpp.
-   */
-  bool ShouldBuildScrollInfoItemsForHoisting() const {
-    return mSVGEffectsBuildingDepth > 0;
-  }
+  bool ShouldBuildScrollInfoItemsForHoisting() const;
 
   void AppendNewScrollInfoItemForHoisting(
       nsDisplayScrollInfoLayer* aScrollInfoItem);
 
   /**
    * A helper class to install/restore nsDisplayListBuilder::mPreserves3DCtx.
    *
    * mPreserves3DCtx is used by class AutoAccumulateTransform &
--- a/layout/xul/nsSliderFrame.cpp
+++ b/layout/xul/nsSliderFrame.cpp
@@ -928,16 +928,21 @@ class AsyncScrollbarDragStarter final : 
 };
 
 static bool UsesSVGEffects(nsIFrame* aFrame) {
   return aFrame->StyleEffects()->HasFilters() ||
          nsSVGIntegrationUtils::UsingMaskOrClipPathForFrame(aFrame);
 }
 
 static bool ScrollFrameWillBuildScrollInfoLayer(nsIFrame* aScrollFrame) {
+  /*
+   * Note: if changing the conditions in this function, make a corresponding
+   * change to nsDisplayListBuilder::ShouldBuildScrollInfoItemsForHoisting()
+   * in nsDisplayList.cpp.
+   */
   if (gfx::gfxVars::UseWebRender()) {
     // If WebRender is enabled, even scrollframes enclosed in SVG effects can
     // be drag-scrolled by APZ.
     return false;
   }
   nsIFrame* current = aScrollFrame;
   while (current) {
     if (UsesSVGEffects(current)) {