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, 27 Oct 2016 14:08:44 -0700
changeset 430449 9e23a4bef1c1c3bd5cc417ca7ad4a2a011a87ffc
parent 430448 ed237d1108c363653089ebd061795843a122ca7b
child 430450 5fe8fd86ad37e4055026819f633d36a34533f429
push id33838
push userdholbert@mozilla.com
push dateThu, 27 Oct 2016 21:10:05 +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
@@ -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());