Bug 1269045 part 1: Adjust flex item "order"-sorting code to treat placeholder frames as <= anything they're compared against, including each other. r=mats
☠☠ backed out by 7c24f4455420 ☠ ☠
authorDaniel Holbert <dholbert@cs.stanford.edu>
Thu, 27 Oct 2016 18:58:25 -0700
changeset 346578 86a391e3e1638407465610b0dc5b5fc4694a0a08
parent 346577 1658a195f669351b5f6caf5dbafdb0b4b12a6c2c
child 346579 7aa8199183fca08ca7d9b309fa01580affb45da7
push id10298
push userraliiev@mozilla.com
push dateMon, 14 Nov 2016 12:33:03 +0000
treeherdermozilla-aurora@7e29173b1641 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats
bugs1269045
milestone52.0a1
Bug 1269045 part 1: Adjust flex item "order"-sorting code to treat placeholder frames as <= anything they're compared against, including each other. r=mats This patch makes the following specific changes: (1) Adds an early-return to both versions of the IsOrderLEQ function, to treat placeholder children as LEQ everything (including each other). This will tend to sort them to the beginning of the child list, which is unimportant but fine. More importantly, though: this means our "order"-sorting code won't reorder placeholders *with respect to each other* (since our sort algorithm is stable). So their painting order won't be affected by the "order" property, which is required by the spec. (2) Drops some nsPlaceholderFrame::GetRealFrameFor() calls -- they're unnecessary, since any placeholder frames will have prompted us to return earlier. One caveat to (2): this patch does leave a few "nsPlaceholderFrame::GetRealFrameFor()" calls in place, *for the moment*. These remaining calls are for handling placeholders that are wrapped, i.e. inside of anonymous flex items. These calls are still needed to avoid assertion-failures (i.e. to get a consistent ordering) at this point, but they'll be removed in a later patch in this same bug, when we stop wrapping placeholders in anonymous flex items. MozReview-Commit-ID: 1R6NW30Kxgv
layout/generic/nsFlexContainerFrame.cpp
--- a/layout/generic/nsFlexContainerFrame.cpp
+++ b/layout/generic/nsFlexContainerFrame.cpp
@@ -1076,36 +1076,38 @@ GetFirstNonAnonBoxDescendant(nsIFrame* a
 bool
 IsOrderLEQWithDOMFallback(nsIFrame* aFrame1,
                           nsIFrame* aFrame2)
 {
   MOZ_ASSERT(aFrame1->IsFlexItem() && aFrame2->IsFlexItem(),
              "this method only intended for comparing flex items");
   MOZ_ASSERT(aFrame1->GetParent() == aFrame2->GetParent(),
              "this method only intended for comparing siblings");
-  bool isInLegacyBox = nsFlexContainerFrame::IsLegacyBox(aFrame1->GetParent());
-
   if (aFrame1 == aFrame2) {
     // Anything is trivially LEQ itself, so we return "true" here... but it's
     // probably bad if we end up actually needing this, so let's assert.
     NS_ERROR("Why are we checking if a frame is LEQ itself?");
     return true;
   }
 
-  // If we've got a placeholder frame, use its out-of-flow frame's 'order' val.
-  {
-    nsIFrame* aRealFrame1 = nsPlaceholderFrame::GetRealFrameFor(aFrame1);
-    nsIFrame* aRealFrame2 = nsPlaceholderFrame::GetRealFrameFor(aFrame2);
-
-    int32_t order1 = GetOrderOrBoxOrdinalGroup(aRealFrame1, isInLegacyBox);
-    int32_t order2 = GetOrderOrBoxOrdinalGroup(aRealFrame2, isInLegacyBox);
-
-    if (order1 != order2) {
-      return order1 < order2;
-    }
+  if (aFrame1->GetType() == nsGkAtoms::placeholderFrame ||
+      aFrame2->GetType() == nsGkAtoms::placeholderFrame) {
+    // Treat placeholders (for abspos/fixedpos frames) as LEQ everything.  This
+    // ensures we don't reorder them w.r.t. one another, which is sufficient to
+    // prevent them from noticeably participating in "order" reordering.
+    return true;
+  }
+
+  bool isInLegacyBox = nsFlexContainerFrame::IsLegacyBox(aFrame1->GetParent());
+
+  int32_t order1 = GetOrderOrBoxOrdinalGroup(aFrame1, isInLegacyBox);
+  int32_t order2 = GetOrderOrBoxOrdinalGroup(aFrame2, isInLegacyBox);
+
+  if (order1 != order2) {
+    return order1 < order2;
   }
 
   // The "order" values are equal, so we need to fall back on DOM comparison.
   // For that, we need to dig through any anonymous box wrapper frames to find
   // the actual frame that corresponds to our child content.
   aFrame1 = GetFirstNonAnonBoxDescendant(aFrame1);
   aFrame2 = GetFirstNonAnonBoxDescendant(aFrame2);
   MOZ_ASSERT(aFrame1 && aFrame2,
@@ -1160,24 +1162,29 @@ IsOrderLEQWithDOMFallback(nsIFrame* aFra
 bool
 IsOrderLEQ(nsIFrame* aFrame1,
            nsIFrame* aFrame2)
 {
   MOZ_ASSERT(aFrame1->IsFlexItem() && aFrame2->IsFlexItem(),
              "this method only intended for comparing flex items");
   MOZ_ASSERT(aFrame1->GetParent() == aFrame2->GetParent(),
              "this method only intended for comparing siblings");
+
+  if (aFrame1->GetType() == nsGkAtoms::placeholderFrame ||
+      aFrame2->GetType() == nsGkAtoms::placeholderFrame) {
+    // Treat placeholders (for abspos/fixedpos frames) as LEQ everything.  This
+    // ensures we don't reorder them w.r.t. one another, which is sufficient to
+    // prevent them from noticeably participating in "order" reordering.
+    return true;
+  }
+
   bool isInLegacyBox = nsFlexContainerFrame::IsLegacyBox(aFrame1->GetParent());
 
-  // If we've got a placeholder frame, use its out-of-flow frame's 'order' val.
-  nsIFrame* aRealFrame1 = nsPlaceholderFrame::GetRealFrameFor(aFrame1);
-  nsIFrame* aRealFrame2 = nsPlaceholderFrame::GetRealFrameFor(aFrame2);
-
-  int32_t order1 = GetOrderOrBoxOrdinalGroup(aRealFrame1, isInLegacyBox);
-  int32_t order2 = GetOrderOrBoxOrdinalGroup(aRealFrame2, isInLegacyBox);
+  int32_t order1 = GetOrderOrBoxOrdinalGroup(aFrame1, isInLegacyBox);
+  int32_t order2 = GetOrderOrBoxOrdinalGroup(aFrame2, isInLegacyBox);
 
   return order1 <= order2;
 }
 
 bool
 nsFlexContainerFrame::IsHorizontal()
 {
   const FlexboxAxisTracker axisTracker(this, GetWritingMode());