Bug 1345873 - Part 1: Make flex-item sorting code treat placeholders as having the default value of 'order' / 'box-ordinal-group'. r=mats, a=lizzard
authorDaniel Holbert <dholbert@cs.stanford.edu>
Thu, 06 Apr 2017 15:14:00 -0400
changeset 379485 15eeffc957863531c7fc42ef8b0a39406b0a195e
parent 379484 dd6de9fcdc8f7e2dc5b3c00ee586fbac3c1c40ab
child 379486 3972c6e42982804732a7bd52bc693e04f5f2186f
push id1419
push userjlund@mozilla.com
push dateMon, 10 Apr 2017 20:44:07 +0000
treeherdermozilla-release@5e6801b73ef6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats, lizzard
bugs1345873
milestone53.0
Bug 1345873 - Part 1: Make flex-item sorting code treat placeholders as having the default value of 'order' / 'box-ordinal-group'. r=mats, a=lizzard This should allow their flex-item siblings to reorder themselves as-needed for honoring their own 'order' values, without unnecessarily reordering the placeholders. MozReview-Commit-ID: aCBQER5r2G
layout/generic/nsFlexContainerFrame.cpp
layout/style/test/mochitest.ini
layout/style/test/test_flexbox_order_abspos.html
--- a/layout/generic/nsFlexContainerFrame.cpp
+++ b/layout/generic/nsFlexContainerFrame.cpp
@@ -1014,16 +1014,21 @@ BuildStrutInfoFromCollapsedItems(const F
 }
 
 // Convenience function to get either the "order" or the "box-ordinal-group"
 // property-value for a flex item (depending on whether the container is a
 // modern flex container or a legacy box).
 static int32_t
 GetOrderOrBoxOrdinalGroup(nsIFrame* aFlexItem, bool aIsLegacyBox)
 {
+  if (aFlexItem->GetType() == nsGkAtoms::placeholderFrame) {
+    // Always treat placeholders as having the default value, which is
+    // 1 for (legacy) 'box-ordinal-group' and 0 for 'order'.
+    return aIsLegacyBox ? 1 : 0;
+  }
   if (aIsLegacyBox) {
     // We'll be using mBoxOrdinal, which has type uint32_t. However, the modern
     // 'order' property (whose functionality we're co-opting) has type int32_t.
     // So: if we happen to have a uint32_t value that's greater than INT32_MAX,
     // we clamp it rather than letting it overflow. Chances are, this is just
     // an author using BIG_VALUE anyway, so the clamped value should be fine.
     // (particularly since sufficiently-huge values are busted in Chrome/WebKit
     // per https://bugs.chromium.org/p/chromium/issues/detail?id=599645 )
@@ -1104,24 +1109,16 @@ IsOrderLEQWithDOMFallback(nsIFrame* aFra
              "this method only intended for comparing siblings");
   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 (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;
-  }
-
   const bool isInLegacyBox = IsLegacyBox(aFrame1->GetParent());
 
   int32_t order1 = GetOrderOrBoxOrdinalGroup(aFrame1, isInLegacyBox);
   int32_t order2 = GetOrderOrBoxOrdinalGroup(aFrame2, isInLegacyBox);
 
   if (order1 != order2) {
     return order1 < order2;
   }
@@ -1137,18 +1134,20 @@ IsOrderLEQWithDOMFallback(nsIFrame* aFra
 
 
   // Special case:
   // If either frame is for generated content from ::before or ::after, then
   // we can't use nsContentUtils::PositionIsBefore(), since that method won't
   // recognize generated content as being an actual sibling of other nodes.
   // We know where ::before and ::after nodes *effectively* insert in the DOM
   // tree, though (at the beginning & end), so we can just special-case them.
-  nsIAtom* pseudo1 = aFrame1->StyleContext()->GetPseudo();
-  nsIAtom* pseudo2 = aFrame2->StyleContext()->GetPseudo();
+  nsIAtom* pseudo1 =
+    nsPlaceholderFrame::GetRealFrameFor(aFrame1)->StyleContext()->GetPseudo();
+  nsIAtom* pseudo2 =
+    nsPlaceholderFrame::GetRealFrameFor(aFrame2)->StyleContext()->GetPseudo();
 
   if (pseudo1 == nsCSSPseudoElements::before ||
       pseudo2 == nsCSSPseudoElements::after) {
     // frame1 is ::before and/or frame2 is ::after => frame1 is LEQ frame2.
     return true;
   }
   if (pseudo1 == nsCSSPseudoElements::after ||
       pseudo2 == nsCSSPseudoElements::before) {
@@ -1182,24 +1181,16 @@ 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;
-  }
-
   const bool isInLegacyBox = IsLegacyBox(aFrame1->GetParent());
 
   int32_t order1 = GetOrderOrBoxOrdinalGroup(aFrame1, isInLegacyBox);
   int32_t order2 = GetOrderOrBoxOrdinalGroup(aFrame2, isInLegacyBox);
 
   return order1 <= order2;
 }
 
--- a/layout/style/test/mochitest.ini
+++ b/layout/style/test/mochitest.ini
@@ -191,16 +191,17 @@ skip-if = stylo # bug 1332969
 [test_exposed_prop_accessors.html]
 [test_extra_inherit_initial.html]
 [test_flexbox_child_display_values.xhtml]
 [test_flexbox_flex_grow_and_shrink.html]
 [test_flexbox_flex_shorthand.html]
 [test_flexbox_layout.html]
 support-files = flexbox_layout_testcases.js
 [test_flexbox_order.html]
+[test_flexbox_order_abspos.html]
 [test_flexbox_order_table.html]
 [test_flexbox_reflow_counts.html]
 [test_font_face_parser.html]
 [test_font_family_parsing.html]
 [test_font_feature_values_parsing.html]
 [test_font_loading_api.html]
 support-files =
   BitPattern.woff
copy from layout/style/test/test_flexbox_order.html
copy to layout/style/test/test_flexbox_order_abspos.html
--- a/layout/style/test/test_flexbox_order.html
+++ b/layout/style/test/test_flexbox_order_abspos.html
@@ -1,16 +1,16 @@
 <!DOCTYPE HTML>
 <html>
 <!--
-https://bugzilla.mozilla.org/show_bug.cgi?id=666041
+https://bugzilla.mozilla.org/show_bug.cgi?id=1345873
 -->
 <head>
   <meta charset="utf-8">
-  <title>Test for Bug 666041</title>
+  <title>Test for Bug 1345873</title>
   <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
   <script type="application/javascript" src="/tests/SimpleTest/WindowSnapshot.js"></script>
   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
   <style type="text/css">
 
     div.ref {
       display: none;
       height: 30px;
@@ -39,30 +39,46 @@ https://bugzilla.mozilla.org/show_bug.cg
     div#flexContainer {
       display: flex;
       width: 100px;
       height: 30px;
     }
     div#flexContainerParent {
       display: none;
     }
+    .abs {
+      position: absolute !important;
+      width: 15px  !important;
+      height: 15px !important;
+    }
   </style>
 </head>
 <body>
-<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=666041">Mozilla Bug 666041</a>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1345873">Mozilla Bug 1345873</a>
 <div id="display">
 
   <!-- Reference cases (display:none; only shown during initRefSnapshots) -->
   <div id="references">
     <div class="ref" id="abc"><refA></refA><refB></refB><refC></refC></div>
-    <div class="ref" id="acb"><refA></refA><refC></refC><refB></refB></div>
-    <div class="ref" id="bac"><refB></refB><refA></refA><refC></refC></div>
-    <div class="ref" id="bca"><refB></refB><refC></refC><refA></refA></div>
-    <div class="ref" id="cab"><refC></refC><refA></refA><refB></refB></div>
-    <div class="ref" id="cba"><refC></refC><refB></refB><refA></refA></div>
+    <div class="ref" id="Abc">
+      <refA class="abs"></refA><refB></refB><refC></refC></div>
+    <div class="ref" id="Bac">
+      <refB class="abs"></refB><refA></refA><refC></refC></div>
+    <div class="ref" id="Bca">
+      <refB class="abs"></refB><refC></refC><refA></refA></div>
+    <div class="ref" id="Cab">
+      <refC class="abs"></refC><refA></refA><refB></refB></div>
+    <div class="ref" id="ABc">
+      <refA class="abs"></refA><refB class="abs"></refB><refC></refC></div>
+    <div class="ref" id="ACb">
+      <refA class="abs"></refA><refC class="abs"></refC><refB></refB></div>
+    <div class="ref" id="BCa">
+      <refB class="abs"></refB><refC class="abs"></refC><refA></refA></div>
+    <div class="ref" id="ABC">
+      <refA class="abs"></refA><refB class="abs"></refB><refC class="abs"></refC></div>
   </div>
 
   <div id="flexContainerParent">
     <!-- The flex container that we'll be testing
          (its parent is display:none initially) -->
     <div id="flexContainer">
       <div id="a"></div>
       <div id="b"></div>
@@ -70,69 +86,70 @@ https://bugzilla.mozilla.org/show_bug.cg
     </div>
   </div>
 
 </div>
 <pre id="test">
 <script type="application/javascript;version=1.7">
 "use strict";
 
-/** Test for Bug 666041 **/
+/** Test for Bug 1345873 **/
 
 /* This testcase ensures that we honor the "order" property when ordering
  * flex items within a flex container.
  *
  * Note: The items in this testcase don't overlap, so this testcase does _not_
  * test paint ordering.  It only tests horizontal ordering in a flex container.
  */
 
 // DATA
 // ----
 
 // This will store snapshots of our reference divs
 let gRefSnapshots = {};
 
 // These are the sets of 'order' values that we'll test.
-// The first three values in each array are the 'order' values that we'll
-// assign to elements a, b, and c (respectively).  The final value in each
-// array is the ID of the expected reference rendering.
-let gOrderTestcases = [
-  // The 6 basic permutations:
-  [ 1, 2, 3, "abc"],
-  [ 1, 3, 2, "acb"],
-  [ 2, 1, 3, "bac"],
-  [ 2, 3, 1, "cab"],
-  [ 3, 1, 2, "bca"],
-  [ 3, 2, 1, "cba"],
+// * The first three values in each array are the 'order' values that we'll
+// assign to elements a, b, and c (respectively).
+// * The next value is a string containing the concatenated IDs of any
+// elements that should be absolutely positioned.
+// * The final value in each array is the ID of the expected reference
+// rendering. (By convention, in those IDs, capital = abspos)
+var gOrderTestcases = [
+  // Just one child is abspos:
+  [ 1, 2, 3, "a", "Abc"],
+  [ 1, 2, 3, "b", "Bac"],
+  [ 1, 2, 3, "c", "Cab"],
+  [ 2, 3, 1, "b", "Bca"],
+  [ 3, 1, 1, "b", "Bca"],
 
-  // Test negative values
-  [ 1, -5, -2, "bca"],
-  [ -50, 0, -2, "acb"],
+  // Two children are abspos:
+  // (Note: "order" doesn't influence position or paint order for abspos
+  // children - only for (in-flow) flex items.)
+  [ 1, 2, 3, "ab", "ABc"],
+  [ 2, 1, 3, "ab", "ABc"],
+  [ 1, 2, 3, "ac", "ACb"],
+  [ 3, 2, 1, "ac", "ACb"],
+  [ 3, 2, 1, "bc", "BCa"],
 
-  // Non-integers should be ignored.
-  // (So, they'll leave their div with the initial 'order' value, which is 0.)
-  [ 1,   1.5, 2,   "bac"],
-  [ 2.5, 3.4, 1,   "abc"],
-  [ 0.5, 1,   1.5, "acb"],
-
-  // Decimal values that happen to be equal to integers (e.g. "3.0") are still
-  // <numbers>, and are _not_ <integers>.
-  //  Source: http://www.w3.org/TR/CSS21/syndata.html#value-def-integer
-  // (So, they'll leave their div with the initial 'order' value, which is 0.)
-  // (NOTE: We have to use quotes around "3.0" and "2.0" to be sure JS doesn't
-  // coerce them into integers before we get a chance to set them in CSS.)
-  [ "3.0", "2.0", "1.0", "abc"],
-  [ 3, "2.0", 1, "bca"],
+  // All three children are abspos:
+  // (Rendering always the same regardless of "order" values)
+  [ 1, 2, 3, "abc", "ABC"],
+  [ 3, 1, 2, "abc", "ABC"],
+  [ 3, 2, 1, "abc", "ABC"],
 ];
 
 // FUNCTIONS
 // ---------
 
 function initRefSnapshots() {
-  let refIds = ["abc", "acb", "bac", "bca", "cab", "cba"];
+  let refIds = ["abc",
+                "Abc", "Bac", "Bca", "Cab",
+                "ABc", "ACb", "BCa",
+                "ABC"];
   for (let id of refIds) {
     let elem = document.getElementById(id);
     elem.style.display = "block";
     gRefSnapshots[id] = snapshotWindow(window, false);
     elem.style.display = "";
   }
 }
 
@@ -144,29 +161,35 @@ function complainIfSnapshotsDiffer(aSnap
     todo(false, "TESTCASE: " + compareResult[1]);
     todo(false, "REFERENCE: "+ compareResult[2]);
   }
 }
 
 function runOrderTestcase(aOrderTestcase) {
   // Sanity-check
   ok(Array.isArray(aOrderTestcase), "expecting testcase to be an array");
-  is(aOrderTestcase.length, 4, "expecting testcase to have 4 elements");
+  is(aOrderTestcase.length, 5, "expecting testcase to have 5 elements");
 
   document.getElementById("a").style.order = aOrderTestcase[0];
   document.getElementById("b").style.order = aOrderTestcase[1];
   document.getElementById("c").style.order = aOrderTestcase[2];
 
+  let idsToMakeAbspos = aOrderTestcase[3].split("");
+  for (let absPosId of idsToMakeAbspos) {
+    document.getElementById(absPosId).classList.add("abs");
+  }
+
   let snapshot = snapshotWindow(window, false);
-  complainIfSnapshotsDiffer(snapshot, gRefSnapshots[aOrderTestcase[3]],
+  complainIfSnapshotsDiffer(snapshot, gRefSnapshots[aOrderTestcase[4]],
                             aOrderTestcase);
 
   // Clean up
   for (let id of ["a", "b", "c"]) {
     document.getElementById(id).style.order = "";
+    document.getElementById(id).classList.remove("abs");
   }
 }
 
 // Main Function
 function main() {
   initRefSnapshots();
 
   // un-hide the flex container's parent