Issue #1485 - Fix incorrect grid cell sizing to min/max space.

There were actually two separate logical errors in this method:

The first part is that "origSizes.isSome()" is simply a bogus
requirement for applying min/max-sizes here. I'm still keeping
the optimization of not needlessly copying the mSizes array
(as originally intended) since it's a quite common case.

The second bug is that min/max-sizes were only applied under
the "if (fr != 0.0f)" block. This is bogus since the calculated
'fr' value depends on 'aAvailableSize' which might change by
applying min/max-sizes and thus 'fr' could become non-zero in
the second round.
To fix, this patch just moves "applyMinMax" block out one level.

3 files changed, 77 insertions, 29 deletions

diff --git a/layout/generic/nsGridContainerFrame.cpp b/layout/generic/nsGridContainerFrame.cpp
index 959061e334..f771c9d7c5 100644
--- a/layout/generic/nsGridContainerFrame.cpp
+++ b/layout/generic/nsGridContainerFrame.cpp
@@ -4822,14 +4822,14 @@ nsGridContainerFrame::Tracks::StretchFlexibleTracks(
                                          : ri->ComputedMaxISize();
   }
   Maybe<nsTArray<TrackSize>> origSizes;
+  bool applyMinMax = (minSize != 0 || maxSize != NS_UNCONSTRAINEDSIZE) &&
+                     aAvailableSize == NS_UNCONSTRAINEDSIZE;
   // We iterate twice at most.  The 2nd time if the grid size changed after
   // applying a min/max-size (can only occur if aAvailableSize is indefinite).
   while (true) {
     float fr = FindUsedFlexFraction(aState, aGridItems, flexTracks,
                                     aFunctions, aAvailableSize);
     if (fr != 0.0f) {
-      bool applyMinMax = (minSize != 0 || maxSize != NS_UNCONSTRAINEDSIZE) &&
-                         aAvailableSize == NS_UNCONSTRAINEDSIZE;
       for (uint32_t i : flexTracks) {
         float flexFactor = aFunctions.MaxSizingFor(i).GetFlexFractionValue();
         nscoord flexLength = NSToCoordRound(flexFactor * fr);
@@ -4841,36 +4841,36 @@ nsGridContainerFrame::Tracks::StretchFlexibleTracks(
           base = flexLength;
         }
       }
-      if (applyMinMax && origSizes.isSome()) {
-        // https://drafts.csswg.org/css-grid/#algo-flex-tracks
-        // "If using this flex fraction would cause the grid to be smaller than
-        // the grid container’s min-width/height (or larger than the grid
-        // container’s max-width/height), then redo this step, treating the free
-        // space as definite [...]"
-        nscoord newSize = 0;
-        for (auto& sz : mSizes) {
-          newSize += sz.mBase;
-        }
-        const auto sumOfGridGaps = SumOfGridGaps();
-        newSize += sumOfGridGaps;
-        if (newSize > maxSize) {
-          aAvailableSize = maxSize;
-        } else if (newSize < minSize) {
-          aAvailableSize = minSize;
-        }
-        if (aAvailableSize != NS_UNCONSTRAINEDSIZE) {
-          // Reset min/max-size to ensure 'applyMinMax' becomes false next time.
-          minSize = 0;
-          maxSize = NS_UNCONSTRAINEDSIZE;
-          aAvailableSize = std::max(0, aAvailableSize - sumOfGridGaps);
-          // Restart with the original track sizes and definite aAvailableSize.
+    }
+    if (applyMinMax) {
+      applyMinMax = false;
+      // https://drafts.csswg.org/css-grid/#algo-flex-tracks
+      // "If using this flex fraction would cause the grid to be smaller than
+      // the grid container’s min-width/height (or larger than the grid
+      // container’s max-width/height), then redo this step, treating the free
+      // space as definite [...]"
+      nscoord newSize = 0;
+      for (auto& sz : mSizes) {
+        newSize += sz.mBase;
+      }
+      const auto sumOfGridGaps = SumOfGridGaps();
+      newSize += sumOfGridGaps;
+      if (newSize > maxSize) {
+        aAvailableSize = maxSize;
+      } else if (newSize < minSize) {
+        aAvailableSize = minSize;
+      }
+      if (aAvailableSize != NS_UNCONSTRAINEDSIZE) {
+        aAvailableSize = std::max(0, aAvailableSize - sumOfGridGaps);
+        // Restart with the original track sizes and definite aAvailableSize.
+        if (origSizes.isSome()) {
           mSizes = Move(*origSizes);
           origSizes.reset();
-          if (aAvailableSize == 0) {
-            break; // zero available size wouldn't change any sizes though...
-          }
-          continue;
+        } // else, no mSizes[].mBase were changed above so it's still correct
+        if (aAvailableSize == 0) {
+          break; // zero available size wouldn't change any sizes though...
         }
+        continue;
       }
     }
     break;
diff --git a/layout/reftests/css-grid/grid-max-sizing-flex-007-ref.html b/layout/reftests/css-grid/grid-max-sizing-flex-007-ref.html
index c5392d32cd..b17a1cc02e 100644
--- a/layout/reftests/css-grid/grid-max-sizing-flex-007-ref.html
+++ b/layout/reftests/css-grid/grid-max-sizing-flex-007-ref.html
@@ -107,4 +107,28 @@
   <div class="item"></div>
 </div>
 
+<pre>The first 6 grids should look the same:</pre>
+<div class="grid rows" style="grid: 1fr / 30px; height:83px">
+  <div class="item"></div>
+</div>
+<div class="grid rows" style="grid: 10px 1fr / 30px; height:83px">
+  <div class="item" style="grid-row:span 2"></div>
+</div>
+<div class="grid rows" style="grid: 1fr / 30px; height:83px">
+  <div class="item"></div>
+</div>
+<div class="grid rows" style="grid: 1fr 1fr / 30px; height:83px">
+  <div class="item" style="grid-row:span 2"><div style="height:90px"></div></div>
+</div>
+<div class="grid rows" style="grid: 1fr auto / 30px; height:83px">
+  <div class="item" style="grid-row:span 2"><div style="height:90px"></div></div>
+</div>
+<div class="grid rows" style="grid: 10px 1fr / 30px; height:83px">
+  <div class="item" style="grid-row:span 2"><div style="height:90px"></div></div>
+</div>
+<div class="grid rows" style="grid: 1fr 1fr / 30px; grid-row-gap:10px; height:83px">
+  <div class="item" style="grid-row:span 2"><div style="height:40px"></div></div>
+  <div class="item"><div style="height:40px"></div></div>
+</div>
+
 </body></html>
diff --git a/layout/reftests/css-grid/grid-max-sizing-flex-007.html b/layout/reftests/css-grid/grid-max-sizing-flex-007.html
index ac9dcc77c4..a2f39e95be 100644
--- a/layout/reftests/css-grid/grid-max-sizing-flex-007.html
+++ b/layout/reftests/css-grid/grid-max-sizing-flex-007.html
@@ -105,4 +105,28 @@
   <div class="item"></div>
 </div>
 
+<pre>The first 6 grids should look the same:</pre>
+<div class="grid rows" style="grid: 1fr / 30px; min-height:83px">
+  <div class="item"></div>
+</div>
+<div class="grid rows" style="grid: 10px 1fr / 30px; min-height:83px">
+  <div class="item" style="grid-row:span 2"></div>
+</div>
+<div class="grid rows" style="grid: 1fr / 30px; max-height:30px; min-height:83px">
+  <div class="item"></div>
+</div>
+<div class="grid rows" style="grid: 1fr 1fr / 30px; max-height:83px">
+  <div class="item" style="grid-row:span 2"><div style="height:90px"></div></div>
+</div>
+<div class="grid rows" style="grid: 1fr auto / 30px; max-height:83px">
+  <div class="item" style="grid-row:span 2"><div style="height:90px"></div></div>
+</div>
+<div class="grid rows" style="grid: 10px 1fr / 30px; max-height:83px">
+  <div class="item" style="grid-row:span 2"><div style="height:90px"></div></div>
+</div>
+<div class="grid rows" style="grid: 1fr 1fr / 30px; grid-row-gap:10px; max-height:83px">
+  <div class="item" style="grid-row:span 2"><div style="height:40px"></div></div>
+  <div class="item"><div style="height:40px"></div></div>
+</div>
+
 </body></html>