Bug 1498281: Make flexbox devtools API report actual flex base size (not its min/max-clamped version). r=bradwerth
authorDaniel Holbert <dholbert@cs.stanford.edu>
Thu, 18 Oct 2018 23:45:51 +0000
changeset 490411 48155e26a841cdf691af533a74c958e566c6e2b5
parent 490410 b809ef6dba1b6fb18ac5151e44b1afa18e104c94
child 490412 8641e30ebaf4bc3c719d6e0cacbd5b4ac6c0f980
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewersbradwerth
bugs1498281
milestone64.0a1
Bug 1498281: Make flexbox devtools API report actual flex base size (not its min/max-clamped version). r=bradwerth This patch also updates the expectations in the mochitest test_flex_items.html. Before this commit, the test (incorrectly) expected the mainBaseSize API to return some items' *final sizes*, because that's what our implementation did return, up until now. As of this patch, that API will now return the item's actual flex base size, which means the text expectations need to change. I'm also adding a new flex item to the test, to exercise a formerly-untested scenario. And to accommodate this new item, I'm also doubling the width of the flex container to be sure there's plenty of space. Differential Revision: https://phabricator.services.mozilla.com/D8476
dom/flex/test/chrome/test_flex_items.html
layout/generic/nsFlexContainerFrame.cpp
--- a/dom/flex/test/chrome/test_flex_items.html
+++ b/dom/flex/test/chrome/test_flex_items.html
@@ -4,39 +4,49 @@
 <meta charset="utf-8">
 <script type="text/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
 <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css" />
 <style>
   .container {
     display: flex;
     background-color: grey;
     font: 14px sans-serif;
-    width: 800px;
     height: 50px;
   }
+  .huge {
+    /* This just needs to be large enough so that no shrinking is required
+       inside the flex container that uses this class. */
+    width: 1600px;
+  }
 
   .base        { align-self: baseline; }
   .lastbase    { align-self: last baseline; }
 
   .offset      { margin-top: 10px;
                  margin-bottom: 3px; }
 
   .lime        { background: lime;   }
   .yellow      { background: yellow; }
   .orange      { background: orange; }
   .pink        { background: pink;   }
+  .tan         { background: tan;    }
   .white       { background: white;  }
 
   .crossMinMax { min-height: 40px;
                  max-height: 120px; }
 
   .mainMinMax  { min-width: 120px;
                  max-width: 500px; }
 
   .flexGrow    { flex-grow: 1; }
+  .spacer150   { width: 150px;
+                 box-sizing: border-box;
+                 height: 10px;
+                 border: 1px solid teal; }
+
 </style>
 
 <script>
 "use strict";
 
 SimpleTest.waitForExplicitFinish();
 
 const TEXT_NODE = Node.TEXT_NODE;
@@ -113,28 +123,32 @@ function runTests() {
   let lbElemBoundingRect = lbElem.getBoundingClientRect();
   ok(line.lastBaselineOffset > containerHeight - lbElemBoundingRect.bottom &&
      line.lastBaselineOffset < containerHeight - lbElemBoundingRect.top,
      "Line lastBaselineOffset should land somewhere within the element" +
      "that determines it.");
 
   let expectedValues = [
     { crossMinSize: 0 },
-    { mainBaseSize: lbElemBoundingRect.width,
+    { mainBaseSize: 100,
       mainDeltaSize: 0 },
     { crossMinSize: 40,
       crossMaxSize: 120,
       mainDeltaSize: 0 },
     { mainMinSize: 120,
       mainMaxSize: 500,
       mainDeltaSize: 0 },
-    { mainBaseSize:  5, /* XXXdholbert should be 10, fixing in bug 1498281 */
+    { mainMinSize: 120,
+      mainMaxSize: 500,
+      mainBaseSize: 150,
+      mainDeltaSize: 0 },
+    { mainBaseSize:  10,
       mainMaxSize:   5,
       mainDeltaSize: 0 },
-    { mainBaseSize: 15, /* XXXdholbert should be 10, fixing in bug 1498281 */
+    { mainBaseSize: 10,
       mainMinSize:  15,
       mainDeltaSize: 0 },
     { mainBaseSize: 50,
       mainMaxSize: 10 },
     { mainDeltaSize: 0 },
     { /* final item is anonymous flex item */ },
   ];
 
@@ -171,21 +185,26 @@ function runTests() {
           "flex-grow item should have expected mainDeltaSize.");
 
   SimpleTest.finish();
 }
 </script>
 </head>
 
 <body onLoad="runTests();">
-  <div id="wrapper" class="container">
+  <div id="wrapper" class="container huge">
     <div class="lime base flexGrow">one line (first)</div>
     <div class="yellow lastbase" style="width: 100px">one line (last)</div>
     <div class="orange offset lastbase crossMinMax">two<br/>lines and offset (last)</div>
     <div class="pink offset base mainMinMax">offset (first)</div>
+    <!-- Inflexible item w/ content-derived flex base size, which has min/max
+         but doesn't violate them: -->
+    <div class="tan mainMinMax">
+      <div class="spacer150"></div>
+    </div>
     <!-- Inflexible item that is trivially clamped to smaller max-width: -->
     <div style="flex: 0 0 10px; max-width: 5px"></div>
     <!-- Inflexible item that is trivially clamped to larger min-width: -->
     <div style="flex: 0 0 10px; min-width: 15px"></div>
     <!-- XXXdholbert should create a second container here w/ specific
          hardcoded flex base sizes so we can predict & check mainDeltaSize
          for this item: -->
     <!-- Item that wants to grow but is trivially clamped to max-width
--- a/layout/generic/nsFlexContainerFrame.cpp
+++ b/layout/generic/nsFlexContainerFrame.cpp
@@ -2621,16 +2621,29 @@ FlexLine::FreezeOrRestoreEachFlexibleSiz
 }
 
 void
 FlexLine::ResolveFlexibleLengths(nscoord aFlexContainerMainSize,
                                  ComputedFlexLineInfo* aLineInfo)
 {
   MOZ_LOG(gFlexContainerLog, LogLevel::Debug, ("ResolveFlexibleLengths\n"));
 
+  // Before we start resolving sizes: if we have an aLineInfo structure to fill
+  // out, we inform it of each item's base size, and we initialize the "delta"
+  // for each item to 0. (And if the flex algorithm wants to grow or shrink the
+  // item, we'll update this delta further down.)
+  if (aLineInfo) {
+    uint32_t itemIndex = 0;
+    for (FlexItem* item = mItems.getFirst(); item; item = item->getNext(),
+                                                   ++itemIndex) {
+      aLineInfo->mItems[itemIndex].mMainBaseSize = item->GetFlexBaseSize();
+      aLineInfo->mItems[itemIndex].mMainDeltaSize = 0;
+    }
+  }
+
   // Determine whether we're going to be growing or shrinking items.
   const bool isUsingFlexGrow =
     (mTotalOuterHypotheticalMainSize < aFlexContainerMainSize);
 
   // Do an "early freeze" for flex items that obviously can't flex in the
   // direction we've chosen:
   FreezeItemsEarly(isUsingFlexGrow, aLineInfo);
 
@@ -2666,31 +2679,16 @@ FlexLine::ResolveFlexibleLengths(nscoord
     nscoord availableFreeSpace = spaceAvailableForFlexItemsContentBoxes;
     for (FlexItem* item = mItems.getFirst(); item; item = item->getNext()) {
       if (!item->IsFrozen()) {
         item->SetMainSize(item->GetFlexBaseSize());
       }
       availableFreeSpace -= item->GetMainSize();
     }
 
-    // If we have an aLineInfo structure to fill out, and this is the
-    // first time through the loop, capture these sizes as mainBaseSizes.
-    // We only care about the first iteration, because additional
-    // iterations will only reset item base sizes to these values.
-    // We also set a 0 mainDeltaSize. This will be modified later if
-    // the item is stretched or shrunk.
-    if (aLineInfo && (iterationCounter == 0)) {
-      uint32_t itemIndex = 0;
-      for (FlexItem* item = mItems.getFirst(); item; item = item->getNext(),
-                                                     ++itemIndex) {
-        aLineInfo->mItems[itemIndex].mMainBaseSize = item->GetMainSize();
-        aLineInfo->mItems[itemIndex].mMainDeltaSize = 0;
-      }
-    }
-
     MOZ_LOG(gFlexContainerLog, LogLevel::Debug,
            (" available free space = %d\n", availableFreeSpace));
 
 
     // The sign of our free space should agree with the type of flexing
     // (grow/shrink) that we're doing (except if we've had integer overflow;
     // then, all bets are off). Any disagreement should've made us use the
     // other type of flexing, or should've been resolved in FreezeItemsEarly.