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 draft
authorDaniel Holbert <dholbert@cs.stanford.edu>
Thu, 20 Oct 2016 13:25:28 -0700
changeset 427725 92bc645b0ccb97bb20c3189f528177784195f776
parent 427724 06d4b91875087e77e8a6569b5df9db9b1ade7569
child 427726 5776c3b902f16c148c318d7298e2202427c92853
push id33099
push userdholbert@mozilla.com
push dateThu, 20 Oct 2016 20:25:54 +0000
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
@@ -1062,36 +1062,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,
@@ -1146,24 +1148,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());