Bug 1435143 - Properly unwrap Maybe<WrScrollId> for the root scroll frame. r=kats

When comparing a Maybe<WrScrollId> to another WrScrollId we need to properly handle the case where Nothing() signifies the root scroll frame. This is equivalent to calling scrollId.valueOr(FrameMetrics::NULL_SCROLL_ID), as was done before WrScrolLId replaced ViewId in the WebRender ScrollingLayersHelper. We also have DisplayListBuilder::TopmostScrollId always return a value instead of a Maybe, since an empty clip stack means that the current scroll id is that of the root scroll frame. Previously Nothing() was not equivalent to WrScrollId { 0 }, which caused the ScrollingLayersHelper to fill the mClipAndScroll value and push another set of clip and scroll nodes onto the WebRender display list builder. MozReview-Commit-ID: CeatZlRXtuI
2018-02-07 10:01:56 +01:00 · 2018-02-07 10:01:56 +01:00 · 90bbf4505c
--- a/gfx/layers/wr/ScrollingLayersHelper.cpp
+++ b/gfx/layers/wr/ScrollingLayersHelper.cpp
@ -124,13 +124,14 @@ ScrollingLayersHelper::BeginItem(nsDisplayItem* aItem,
  // nested ScrollingLayersHelper may rely on things like TopmostScrollId and
  // TopmostClipId, so now we need to push at most two things onto the stack.

-  Maybe<wr::WrScrollId> leafmostId = ids.first;
+  wr::WrScrollId rootId = wr::WrScrollId { 0 };
+  wr::WrScrollId leafmostId = ids.first.valueOr(rootId);
+
  FrameMetrics::ViewID viewId = aItem->GetActiveScrolledRoot()
      ? aItem->GetActiveScrolledRoot()->GetViewId()
      : FrameMetrics::NULL_SCROLL_ID;
-  Maybe<wr::WrScrollId> scrollId =
-      mBuilder->GetScrollIdForDefinedScrollLayer(viewId);
-  MOZ_ASSERT(scrollId.isSome());
+  wr::WrScrollId scrollId =
+      mBuilder->GetScrollIdForDefinedScrollLayer(viewId).valueOr(rootId);

  // If the leafmost ASR is not the same as the item's ASR then we are dealing
  // with a case where the item's clip chain is scrolled by something other than
@ -146,7 +147,7 @@ ScrollingLayersHelper::BeginItem(nsDisplayItem* aItem,
      mBuilder->TopmostScrollId() == scrollId &&
      !mBuilder->TopmostIsClip()) {
    if (auto cs = EnclosingClipAndScroll()) {
-      MOZ_ASSERT(cs->first == *scrollId);
+      MOZ_ASSERT(cs->first == scrollId);
      needClipAndScroll = true;
    }
  }
@ -155,7 +156,7 @@ ScrollingLayersHelper::BeginItem(nsDisplayItem* aItem,
  // the scroll stack
  if (!needClipAndScroll && mBuilder->TopmostScrollId() != scrollId) {
    MOZ_ASSERT(leafmostId == scrollId); // because !needClipAndScroll
-    clips.mScrollId = scrollId;
+    clips.mScrollId = Some(scrollId);
  }
  // And ensure the leafmost clip, if scrolled by that ASR, is at the top of the
  // stack.
@ -174,7 +175,7 @@ ScrollingLayersHelper::BeginItem(nsDisplayItem* aItem,
      clipId = mBuilder->TopmostClipId();
    }

-    clips.mClipAndScroll = Some(std::make_pair(*scrollId, clipId));
+    clips.mClipAndScroll = Some(std::make_pair(scrollId, clipId));
  }

  clips.Apply(mBuilder);
@ -299,7 +300,9 @@ ScrollingLayersHelper::RecurseAndDefineClip(nsDisplayItem* aItem,
  } else {
    MOZ_ASSERT(!ancestorIds.second);
    FrameMetrics::ViewID viewId = aChain->mASR ? aChain->mASR->GetViewId() : FrameMetrics::NULL_SCROLL_ID;
-    auto scrollId = mBuilder->GetScrollIdForDefinedScrollLayer(viewId);
+
+    wr::WrScrollId rootId = wr::WrScrollId { 0 };
+    auto scrollId = mBuilder->GetScrollIdForDefinedScrollLayer(viewId).valueOr(rootId);
    if (mBuilder->TopmostScrollId() == scrollId) {
      if (mBuilder->TopmostIsClip()) {
        // If aChain->mASR is already the topmost scroll layer on the stack, but
@ -328,7 +331,7 @@ ScrollingLayersHelper::RecurseAndDefineClip(nsDisplayItem* aItem,
        // (S, D) for this item. This hunk of code ensures that we define D
        // as a child of C, and when we set the needClipAndScroll flag elsewhere
        // in this file we make sure to set it for this scenario.
-        MOZ_ASSERT(Some(cs->first) == scrollId);
+        MOZ_ASSERT(cs->first == scrollId);
        ancestorIds.first = Nothing();
        ancestorIds.second = cs->second;
      }
--- a/gfx/tests/reftest/1435143-ref.html
+++ b/gfx/tests/reftest/1435143-ref.html
@ -0,0 +1,23 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <style>
+    #green {
+        position: absolute;
+        background: green;
+        border-radius: 1px;
+        transform: translateX(100px);
+    }
+    #text {
+        visibility: hidden;
+    }
+    </style>
+  </head>
+
+  <body>
+    <div id="header">
+      <div id="green"><span id="text">Text.</span></div>
+    </div>
+  </body>
+</html>
+
--- a/gfx/tests/reftest/1435143.html
+++ b/gfx/tests/reftest/1435143.html
@ -0,0 +1,26 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <style>
+    #header {
+        position: fixed;
+    }
+    #green {
+        position: absolute;
+        background: green;
+        border-radius: 1px;
+        transform: translateX(100px);
+    }
+    #text {
+        visibility: hidden;
+    }
+    </style>
+  </head>
+
+  <body>
+    <div id="header">
+      <div id="green"><span id="text">Text.</span></div>
+    </div>
+  </body>
+</html>
+
--- a/gfx/tests/reftest/reftest.list
+++ b/gfx/tests/reftest/reftest.list
@ -10,3 +10,4 @@ fuzzy(100,30) == 1149923.html 1149923-ref.html # use fuzzy due to few distorted
 == 1419528.html 1419528-ref.html
 == 1424673.html 1424673-ref.html
 == 1429411.html 1429411-ref.html
+== 1435143.html 1435143-ref.html
--- a/gfx/webrender_bindings/WebRenderAPI.cpp
+++ b/gfx/webrender_bindings/WebRenderAPI.cpp
@ -1311,15 +1311,15 @@ DisplayListBuilder::TopmostClipId()
  return Nothing();
 }

-Maybe<wr::WrScrollId>
+wr::WrScrollId
 DisplayListBuilder::TopmostScrollId()
 {
  for (auto it = mClipStack.crbegin(); it != mClipStack.crend(); it++) {
    if (it->is<wr::WrScrollId>()) {
-      return Some(it->as<wr::WrScrollId>());
+      return it->as<wr::WrScrollId>();
    }
  }
-  return Nothing();
+  return wr::WrScrollId { 0 };
 }

 bool
--- a/gfx/webrender_bindings/WebRenderAPI.h
+++ b/gfx/webrender_bindings/WebRenderAPI.h
@ -435,7 +435,7 @@ public:
  // is empty.
  Maybe<wr::WrClipId> TopmostClipId();
  // Same as TopmostClipId() but for scroll layers.
-  Maybe<wr::WrScrollId> TopmostScrollId();
+  wr::WrScrollId TopmostScrollId();
  // If the topmost item on the stack is a clip or a scroll layer
  bool TopmostIsClip();

--- a/gfx/webrender_bindings/WebRenderTypes.h
+++ b/gfx/webrender_bindings/WebRenderTypes.h
@ -771,6 +771,10 @@ struct WrScrollId {
  bool operator==(const WrScrollId& other) const {
    return id == other.id;
  }
+
+  bool operator!=(const WrScrollId& other) const {
+    return id != other.id;
+  }
 };

 // Corresponds to a clip id for a position:sticky clip in webrender. Similar