Bug 1490393 - Flush a deferred transform before picking up another if the ASR changes. r=mstange
authorKartikaya Gupta <kgupta@mozilla.com>
Thu, 01 Nov 2018 21:14:50 +0000
changeset 500451 0b39b9d22436f3f463c6c62c69e5ee1737fd2e60
parent 500450 b1e8ab8ec05115d57f8ab2019c7476a117669578
child 500452 1dbaae27c584753c9d22f33265c76e66d044cb2e
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)
reviewersmstange
bugs1490393
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 1490393 - Flush a deferred transform before picking up another if the ASR changes. r=mstange The implementation of deferred transforms did not handle the case where we ended up deferring multiple transform items in a row with different ASRs. In this case, when we encounter the nested transform item that we want to defer, we need to flush the previously-deferred transform item into a WebRenderLayerScrollData item. This patch accomplishes that, and includes a mochitest that exercises the relevant behaviour. Depends on D8110 Differential Revision: https://phabricator.services.mozilla.com/D8111
gfx/layers/apz/test/mochitest/helper_bug1490393-2.html
gfx/layers/apz/test/mochitest/test_group_mouseevents.html
gfx/layers/wr/StackingContextHelper.h
gfx/layers/wr/WebRenderCommandBuilder.cpp
new file mode 100644
--- /dev/null
+++ b/gfx/layers/apz/test/mochitest/helper_bug1490393-2.html
@@ -0,0 +1,63 @@
+<!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 scrollframe inside nested transforms</title>
+  <script type="application/javascript" src="apz_test_native_event_utils.js"></script>
+  <script type="application/javascript" src="apz_test_utils.js"></script>
+  <script type="application/javascript" 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" by an additional 40 pixels (height of the "gap" div)
+  // and the scrollframe scrolls by a corresponding amount. So after doing this
+  // drag we check the scroll position to make sure it hasn't scrolled by
+  // too much.
+  // 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 50px and scroll approximately 250px.
+  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 id="gap" style="min-height: 40px"></div>
+    <div style="height: 400px; transform: translateZ(0)">
+        <div style="height: 100%; overflow-x: auto; overflow-y: hidden; transform: translateZ(0)">
+            <div id="scrollable" style="display: inline-block; height: 100%; overflow-y: auto; transform: translateZ(0)">
+                <div style="min-height: 2000px">Yay text</div>
+            </div>
+            <div style="display: inline-block; width: 2000px; height: 100%;"></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
@@ -24,18 +24,19 @@ var subtests = [
   {'file': 'helper_bug1326290.html'},
   // Test for scrollbar-dragging on a scrollframe inside an SVGEffects
   {'file': 'helper_bug1331693.html'},
   // 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'},
-  // Test for scrollbar-dragging on scrollframes inside nested transforms
-  {'file': 'helper_bug1490393.html'}
+  // Tests for scrollbar-dragging on scrollframes inside nested transforms
+  {'file': 'helper_bug1490393.html'},
+  {'file': 'helper_bug1490393-2.html'}
 ];
 
 if (isApzEnabled()) {
   SimpleTest.waitForExplicitFinish();
   window.onload = function() {
     runSubtestsSeriallyInFreshWindows(subtests)
     .then(SimpleTest.finish, SimpleTest.finish);
   };
--- a/gfx/layers/wr/StackingContextHelper.h
+++ b/gfx/layers/wr/StackingContextHelper.h
@@ -115,17 +115,19 @@ private:
   // there is an invariant here that if mDeferredTransformItem is Nothing(),
   // mDeferredAncestorTransform will also be Nothing(). Note that we
   // can only do this if the nsDisplayTransform items share the same ASR. If
   // we are processing an nsDisplayTransform item with a different ASR than the
   // previously-deferred item, we assume that the previously-deferred transform
   // will get sent to APZ as part of a separate WebRenderLayerScrollData item,
   // and so we don't need to bother with any merging. (The merging probably
   // wouldn't even make sense because the coordinate spaces might be different
-  // in the face of async scrolling).
+  // in the face of async scrolling). This behaviour of forcing a
+  // WebRenderLayerScrollData item to be generated when the ASR changes is
+  // implemented in WebRenderCommandBuilder::CreateWebRenderCommandsFromDisplayList.
   Maybe<nsDisplayTransform*> mDeferredTransformItem;
   Maybe<gfx::Matrix4x4> mDeferredAncestorTransform;
 
   bool mIsPreserve3D;
   bool mRasterizeLocally;
 };
 
 } // namespace layers
--- a/gfx/layers/wr/WebRenderCommandBuilder.cpp
+++ b/gfx/layers/wr/WebRenderCommandBuilder.cpp
@@ -1418,16 +1418,30 @@ WebRenderCommandBuilder::CreateWebRender
       // display item than previously, so we can't squash the display items
       // into the same "layer".
       const ActiveScrolledRoot* asr = item->GetActiveScrolledRoot();
       if (asr != mLastAsr) {
         mLastAsr = asr;
         forceNewLayerData = true;
       }
 
+      // Refer to the comment on StackingContextHelper::mDeferredTransformItem
+      // for an overview of what this is about. This bit of code applies to the
+      // case where we are deferring a transform item, and we then need to defer
+      // another transform with a different ASR. In such a case we cannot just
+      // merge the deferred transforms, but need to force a new
+      // WebRenderLayerScrollData item to flush the old deferred transform, so
+      // that we can then start deferring the new one.
+      if (!forceNewLayerData &&
+          item->GetType() == DisplayItemType::TYPE_TRANSFORM &&
+          aSc.GetDeferredTransformItem() &&
+          (*aSc.GetDeferredTransformItem())->GetActiveScrolledRoot() != asr) {
+        forceNewLayerData = true;
+      }
+
       // If we're going to create a new layer data for this item, stash the
       // ASR so that if we recurse into a sublist they will know where to stop
       // walking up their ASR chain when building scroll metadata.
       if (forceNewLayerData) {
         mAsrStack.push_back(asr);
       }
     }