Bug 1420737 - Fix merge algorithm to handle more complex z-index changes. r=mstange

layout/painting/RetainedDisplayListBuilder.cpp

@@ -306,13 +306,13 @@ void UpdateASR(nsDisplayItem* aItem,
  *
  * The basic algorithm is:
  *
- * For-each item in the new list:
+ * For-each item i in the new list:
  *     If the item has a matching item in the old list:
- *         Remove items from the bottom of the old list until we reach the matching item:
+ *         Remove items from the start of the old list up until we reach an item that also exists in the new list (leaving the matched item in place):
  *             Add valid items to the merged list, destroy invalid items.
- *         Destroy the matching item from the old list.
+ *     Add i into the merged list.
- *     Add the item from the new list into the merged list.
+ *     If the start of the old list matches i, remove and destroy it, otherwise mark the old version of i as used.
- * Add all remaining valid items from the old list into the merged list.
+ * Add all remaining valid items from the old list into the merged list, skipping over (and destroying) any that are marked as used.
  *
  * If any item has a child display list, then we recurse into the merge
  * algorithm once we match up the new/old versions (if present).
@@ -320,7 +320,7 @@ void UpdateASR(nsDisplayItem* aItem,
  * Example 1:
  *
  * Old List: A,B,C,D
- * New List: A,D
+ * Modified List: A,D
  * Invalidations: C,D
  *
  * We first match the A items, and add the new one to the merged list.
@@ -330,10 +330,23 @@ void UpdateASR(nsDisplayItem* aItem,
  *
  * Merged List: A,B,D
  *
- * Example 2:
+ * Example 2 (layout/reftests/retained-dl-zindex-1.html):
  *
  * Old List: A, B
- * New List, B, A
+ * Modified List: B, A
+ * Invalidations: A
+ *
+ * In this example A has been explicitly moved to the back.
+ *
+ * We match the B items, but don't copy A since it's invalid, and then add the
+ * new B into the merged list. We then add A, and we're done.
+ *
+ * Merged List: B, A
+ *
+ * Example 3:
+ *
+ * Old List: A, B
+ * Modified List: B, A
  * Invalidations: -
  *
  * This can happen because a prior merge might have changed the ordering
@@ -343,6 +356,28 @@ void UpdateASR(nsDisplayItem* aItem,
  * and then add the new B into the merged list. We then add A, and we're done.
  *
  * Merged List: B, A
+ *
+ * Example 4 (layout/reftests/retained-dl-zindex-2.html):
+ *
+ * Element A has two elements covering it (B and C), that don't intersect each
+ * other. We then move C to the back.
+ *
+ * The correct initial ordering has B and C after A, in any order.
+ *
+ * Old List: A, B, C
+ * Modified List: C, A
+ * Invalidations: C
+ *
+ * We match the C items, but don't add anything from the old list because A is present
+ * in both lists. We add C to the merged list, and mark the old version of C as reused.
+ *
+ * We then match A, add the new version the merged list and delete the old version.
+ *
+ * We then process the remainder of the old list, B is added (since it is valid,
+ * and hasn't been mark as reused), C is destroyed since it's marked as reused and
+ * is already present in the merged list.
+ *
+ * Merged List: C, A, B
  */
 void
 RetainedDisplayListBuilder::MergeDisplayLists(nsDisplayList* aNewList,
@@ -407,18 +442,17 @@ RetainedDisplayListBuilder::MergeDisplayLists(nsDisplayList* aNewList,
       // The new item has a matching counterpart in the old list that we haven't yet reached,
       // so copy all valid items from the old list into the merged list until we get to the
       // matched item.
-      if (!oldItem->IsReused()) {
       nsDisplayItem* old = nullptr;
-        while ((old = aOldList->RemoveBottom()) && !IsSameItem(newItem, old)) {
+      while ((old = aOldList->GetBottom()) && old != oldItem) {
         if (IsAnyAncestorModified(old->FrameForInvalidation())) {
           // The old item is invalid, discard it.
           oldListLookup.Remove({ old->Frame(), old->GetPerFrameKey() });
+          aOldList->RemoveBottom();
           old->Destroy(&mBuilder);
         } else if (newListLookup.Get({ old->Frame(), old->GetPerFrameKey() })) {
-            // The old item is also in the new list, but we haven't got to it yet.
+          // This old item is also in the new list, but we haven't got to it yet.
-            // Mark that we've found it, and we'll deal with it when we get to the new
+          // Stop now, and we'll deal with it when we get to the new entry.
-            // entry.
+          break;
-            old->SetReused(true);
         } else {
           // Recurse into the child list (without a matching new list) to
           // ensure that we find and remove any invalidated items.
@@ -430,16 +464,27 @@ RetainedDisplayListBuilder::MergeDisplayLists(nsDisplayList* aNewList,
             UpdateASR(old, containerASRForChildren);
             old->UpdateBounds(&mBuilder);
           }
+          aOldList->RemoveBottom();
           ReuseItem(old);
         }
       }
-        MOZ_ASSERT(old && IsSameItem(newItem, old));
+      bool destroy = false;
-        MOZ_ASSERT(old == oldItem);
+      if (old == oldItem) {
+        // If we advanced the old list until the matching item then we can pop
+        // the matching item off the old list and make sure we clean it up.
+        aOldList->RemoveBottom();
+        destroy = true;
+      } else {
+        // If we didn't get to the matching item, then mark the old item
+        // as being reused (since we're adding the new version to the new
+        // list now) so that we don't add it twice at the end.
+        oldItem->SetReused(true);
       }
 
       // Recursively merge any child lists, destroy the old item and add
       // the new one to the list.
-      if (oldItem->GetType() == DisplayItemType::TYPE_LAYER_EVENT_REGIONS &&
+      if (destroy &&
+          oldItem->GetType() == DisplayItemType::TYPE_LAYER_EVENT_REGIONS &&
           !IsAnyAncestorModified(oldItem->FrameForInvalidation())) {
         // Event regions items don't have anything interesting other than
         // the lists of regions and frames, so we have no need to use the
@@ -459,7 +504,9 @@ RetainedDisplayListBuilder::MergeDisplayLists(nsDisplayList* aNewList,
           newItem->UpdateBounds(&mBuilder);
         }
 
+        if (destroy) {
           oldItem->Destroy(&mBuilder);
+        }
         UseItem(newItem);
       }
     } else {
@@ -471,7 +518,8 @@ RetainedDisplayListBuilder::MergeDisplayLists(nsDisplayList* aNewList,
 
   // Reuse the remaining valid items from the old display list.
   while (nsDisplayItem* old = aOldList->RemoveBottom()) {
-    if (!IsAnyAncestorModified(old->FrameForInvalidation())) {
+    if (!IsAnyAncestorModified(old->FrameForInvalidation()) &&
+        !old->IsReused()) {
       if (old->GetChildren()) {
         // We are calling MergeDisplayLists() to ensure that the display items
         // with modified or deleted children will be correctly handled.

layout/reftests/display-list/reftest.list

@@ -8,6 +8,8 @@ skip-if(!retainedDisplayList) == retained-dl-scroll-out-of-view-1.html retained-
 skip-if(!retainedDisplayList) == retained-dl-displayport-1.html retained-dl-displayport-1-ref.html
 skip-if(!retainedDisplayList) == retained-dl-prerender-transform-1.html retained-dl-prerender-transform-1-ref.html
 == retained-dl-wrap-list.html retained-dl-wrap-list-ref.html
+== retained-dl-zindex-1.html retained-dl-zindex-1-ref.html
+== retained-dl-zindex-2.html retained-dl-zindex-2-ref.html
 fuzzy(1,235200) == 1413073.html 1413073-ref.html
 == 1416291.html 1416291-ref.html
 == 1417601-1.html 1417601-1-ref.html

layout/reftests/display-list/retained-dl-zindex-1-ref.html

@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+div {
+  width: 200px;
+  height: 200px;
+  position:relative;
+  will-change: transform;
+}
+#one {
+  top:120px;
+  background-color:blue;
+}
+#two {
+  top: -200px;
+  left: 100px;
+  background-color:green;
+  z-index: -1;
+}
+</style>
+</head>
+<body>
+  <div id="one"></div>
+  <div id="two"></div>
+</body>
+</html>

layout/reftests/display-list/retained-dl-zindex-1.html

@@ -0,0 +1,35 @@
+<!DOCTYPE html>
+<html class="reftest-wait">
+<head>
+<style>
+div {
+  width: 200px;
+  height: 200px;
+  position:relative;
+  will-change: transform;
+}
+#one {
+  top:120px;
+  background-color:blue;
+}
+#two {
+  top: -200px;
+  left: 100px;
+  background-color:green;
+  z-index: 1;
+}
+</style>
+</head>
+<body>
+  <div id="one"></div>
+  <div id="two"></div>
+</body>
+<script>
+function doTest() {
+  document.getElementById("two").style.zIndex = -1;
+  document.documentElement.removeAttribute("class");
+}
+
+window.addEventListener("MozReftestInvalidate", doTest);
+</script>
+</html>

layout/reftests/display-list/retained-dl-zindex-2-ref.html

@@ -0,0 +1,33 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+div {
+  width: 200px;
+  height: 200px;
+  position:relative;
+  will-change:transform;
+}
+#one {
+  top:120px;
+  background-color:blue;
+}
+#two {
+  top: -200px;
+  left: 100px;
+  background-color:green;
+}
+#three {
+  top: -180px;
+  left: 100px;
+  background-color:red;
+  z-index: -1;
+}
+</style>
+</head>
+<body>
+  <div id="one"></div>
+  <div id="two"></div>
+  <div id="three"></div>
+</body>
+</html>

layout/reftests/display-list/retained-dl-zindex-2.html

@@ -0,0 +1,41 @@
+<!DOCTYPE html>
+<html class="reftest-wait">
+<head>
+<style>
+div {
+  width: 200px;
+  height: 200px;
+  position:relative;
+  will-change:transform;
+}
+#one {
+  top:120px;
+  background-color:blue;
+}
+#two {
+  top: -200px;
+  left: 100px;
+  background-color:green;
+}
+#three {
+  top: -180px;
+  left: 100px;
+  background-color:red;
+  z-index: 1;
+}
+</style>
+</head>
+<body>
+  <div id="one"></div>
+  <div id="two"></div>
+  <div id="three"></div>
+</body>
+<script>
+function doTest() {
+  document.getElementById("three").style.zIndex = -1;
+  document.documentElement.removeAttribute("class");
+}
+
+window.addEventListener("MozReftestInvalidate", doTest);
+</script>
+</html>