1425599 - [css-grid] Unintended gaps appearing in CSS Grid layout

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[davidh]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171128222554

Steps to reproduce:

Created a css grid layout that loads items into grid (as user scrolls). https://codepen.io/svdavidh/pen/mpeWVy


Actual results:

Randomly, unintended gaps/spacing will appear which throws off the grid cell sizing


Expected results:

No gaps should appear in the layout

Comment 1

[davidh]

Attachment: Screenshot of codepen with described gap

Comment 2

[Daniel Holbert [:dholbert]]

Thanks for the bug report!

I can't immediately reproduce the issue, locally.  (I do see something a *little* similar, but when I see blanks, there are *entire cells* missing at the bottom/right, and they fill in once I scroll past them -- and that happens in Chrome as well, so I think that's just part of how the demo works right now.  I haven't been able to trigger gaps between cells like your screenshot shows.)

Could you perhaps create a simplified demo that doesn't change when the user scrolls?  (e.g. maybe a version of the codepen that runs your current "onscroll" code N times up-front, and then you can scroll down a bit and see some brokenness without triggering JS to run again)

That would make it easier to be sure Firefox developers/bug-triagers are looking at the same content that you are (rather than a different page from having scrolled a different number of times).

(Also: maybe it's just my network connection [or some temporary issue on the server end], but at the moment, lorempixel.com URLs seem to take *super* long (tens of seconds) to load for me, which makes the testcase hard to work with. You might consider using https://placehold.it/100/100 instead for your placeholder-image, if that works on your end, too.)

Comment 3

[Daniel Holbert [:dholbert]]

OK, I've triggered this now -- I'll attach a testcase that reproduces it without needing any JS or specific user interaction.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: (placeholder image)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: screenshot of testcase in Firefox

Here's a screenshot of the testcase in Firefox. Note the gap towards the lower-left.

In Chrome, those gaps are closed -- that bottom-left content is snapped up to the boxes directly above them.

Mats, could you take a look?

Comment 7

[Mats Palmgren (inactive)]

Right, so this appears to be a somewhat recent spec change:
https://github.com/w3c/csswg-drafts/issues/1729
https://github.com/w3c/csswg-drafts/commit/afae499a6c8f42bd60e6624294ce50aadcb648e4
We currently implement what the spec said before that change.

Comment 8

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 9

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-grid] Change the track sizing algorithm for spanning items so that it accumulates individual item contributions to the plan by max() rather than incrementing the planned size directly

This patch changes the Track Sizing Algorithm for span>1 to follow
the updated spec text.  It also fixes an existing bug when copying
back the results from the "intrinsic max-sizing" step:
https://searchfox.org/mozilla-central/rev/a70da6775d5341a9cca86bf1572a5e3534909153/layout/generic/nsGridContainerFrame.cpp#1218,1426,4372-4373,4394
We copied back the base size as the limit for all tracks, not
just the tracks spanned by items in this particular span group,
so any groups with a larger span would never see any tracks
with an unconstrained limit.

Admittedly, this patch and the resulting code isn't very readable.
I just wanted to post it to (hopefully) make it a little clearer
what the issues are.  Feel free to ignore reviewing this part if
you want and instead review the patches that follows + the rollup
patch at the end if that's easier.

So, next I'll post a series of patches that gradually rewrites
this code to make it easier to understand.

(I also noted that the existing spec references in the code are
a bit out of date.  For example, step 2.1 .. 2.6 is now numbered
3.1 ... 3.6 in the spec.  I'll fix those in a follow-up bug.)

Comment 10

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid] Factor out the min-sizing parts of the track sizing for spanned items to a templated method (idempotent change)

Comment 11

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid] Factor out most of the max-sizing parts of the track sizing for spanned items to a templated method (idempotent change)

Comment 12

[Mats Palmgren (inactive)]

Attachment: part 4 - [css-grid] Factor out the starting base/limit size to a templated method (idempotent change)

Comment 13

[Mats Palmgren (inactive)]

Attachment: part 5 - [css-grid] Make CollectGrowable a templated method so that it works with either base/limit sizes (idempotent change)

Comment 14

[Mats Palmgren (inactive)]

Attachment: part 6 - [css-grid] Make the size distribution methods templated with the intent of merging them in a later patch (idempotent change)

Comment 15

[Mats Palmgren (inactive)]

Attachment: part 7 - [css-grid] Remove the 'limits' copy of track sizes since they are no longer needed (idempotent change)

Comment 16

[Mats Palmgren (inactive)]

Attachment: part 8 - [css-grid] Factor out the fit-content clamping function from DistributeToTrackLimits and pass it as a param instead (idempotent change)

Comment 17

[Mats Palmgren (inactive)]

Attachment: part 9 - [css-grid] Merge DistributeToTrackLimits/Bases (idempotent change)

Comment 18

[Mats Palmgren (inactive)]

Attachment: part 10 - [css-grid] Make MarkExcludedTracks a static method since it doesn't use 'this' (idempotent change)

Comment 19

[Mats Palmgren (inactive)]

Attachment: part 11 - [css-grid] Hoist the marking of excluded tracks out from GrowSelectedTracksUnlimited to a separate method (idempotent change)

Comment 20

[Mats Palmgren (inactive)]

Attachment: part 12 - [css-grid] Merge CopyPlanToBase/Limits into a templated method instead (idempotent change)

Comment 21

[Mats Palmgren (inactive)]

Attachment: part 13 - [css-grid] Merge Grow[Base|Limits]ForSpanningItems into a templated method instead (idempotent change)

Comment 22

[Mats Palmgren (inactive)]

Attachment: part 14 - [css-grid] Use iterators instead of an array + start/end index for the item data (idempotent change)

Comment 23

[Mats Palmgren (inactive)]

Attachment: part 15 - [css-grid] Test reference fixes + more tests.

Comment 24

[Mats Palmgren (inactive)]

Attachment: Rollup patch for part 1..14 above

Comment 25

[Mats Palmgren (inactive)]

Attachment: Rollup patch for part 1..14 above (wdiff)

Comment 26

[Mats Palmgren (inactive)]

FYI, I'm not entirely happy with having an array of TrackSize:s
for both "plan" and "itemplan".  I think that can be optimized
by using a single array to minimize memory use / cache pressure.
That's better as a follow-up bug though.

I think we should uplift this to the v60 branch too since it's
an ESR version (IIUC) and it would be nice to have this fix there.
These patches only touches nsGridContainerFrame.cpp (apart from
tests), so it obviously only affects Grid layout which minimizes
any regression risks.

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 8958169 [details] [diff] [review]
part 2 - [css-grid] Factor out the min-sizing parts of the track sizing for spanned items to a templated method (idempotent change)

Review of attachment 8958169 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, a few nits:

::: layout/generic/nsGridContainerFrame.cpp
@@ +1174,5 @@
> +        case TrackSizingPhase::eMaxContentMinimums:
> +        case TrackSizingPhase::eMaxContentMaximums:
> +          return mMaxContentContribution;
> +      }
> +      MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE("Unexpected phase");

Do you need this MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE(...) hint to work around a silly compiler on one of our platforms?  If it's not strictly necessary, I'd much prefer that we remove it... It's harmless at the moment sine it is indeed unreachable, but it's potentially dangerous if it becomes reachable, later due to a subsequent refactoring/tweak.

We only have this macro for cases where our compilers really are too stupid to figure out that code is unreachable -- and I don't think that's the case here.

At least on my machine, the compiler (clang 7) can figure out on its own that any code after the switch statement would be unreachable, because the compiler has "enum class" support, and you're using "enum class" here for strictness, and you're correctly handling all the values of that enum class.

Also: if I naively add a new enum value ("eSomethingElse") without handling it in this switch statement, I get stricter error output if I remove this hint -- 2 build warnings, as opposed to just 1 if I leave the hint in.  (And stricter seems better here.)

@@ +4245,5 @@
> +  }
> +  if (updatedBase) {
> +    CopyPlanToBase(aPlan);
> +  }
> +  return updatedBase;

Maybe name this "didUpdateBase", to make it a bit less ambiguous? (Right now "updatedBase" sounds like it might store the updated base value, or something like that.)

Comment 28

[Daniel Holbert [:dholbert]]

Comment on attachment 8958171 [details] [diff] [review]
part 3 - [css-grid] Factor out most of the max-sizing parts of the track sizing for spanned items to a templated method (idempotent change)

Review of attachment 8958171 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: layout/generic/nsGridContainerFrame.cpp
@@ +4292,5 @@
> +                                aFunctions, aPercentageBasis);
> +      }
> +    }
> +  }
> +  return true;

(Observation, probably no action needed): it looks like this function (GrowLimitForSpanningItems) always returns true, and the callers don't use that value anyway... Though I suppose that won't matter in the end, because I don't see this function (or any unconditional "return true") in the final rollup. So I assume a later patch in the series gets rid of this problem -- hence, this is probably fine.

Comment 29

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #27)
> Do you need this MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE(...) hint to work
> around a silly compiler on one of our platforms?

Yes, "error: control reaches end of non-void function [-Werror=return-type]"
on Try... gcc I guess.  It compiled fine for me locally (clang) without it.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=170cb288e97880956af12302b88175503662d062&selectedJob=167270388

> Maybe name this "didUpdateBase", to make it a bit less ambiguous? (Right now
> "updatedBase" sounds like it might store the updated base value, or
> something like that.)

I renamed it 'needToUpdateSizes' in a later patch since it changed
meaning slightly.

Comment 30

[Daniel Holbert [:dholbert]]

Attachment: hackaround to demonstrate how you can avoid MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE [applies on top of "part 2"]

(In reply to Mats Palmgren (:mats) from comment #29)
> (In reply to Daniel Holbert [:dholbert] from comment #27)
> > Do you need this MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE(...) hint to work
> > around a silly compiler on one of our platforms?
> 
> Yes, "error: control reaches end of non-void function [-Werror=return-type]"
> on Try... gcc I guess.  It compiled fine for me locally (clang) without it.

Thanks - I can reproduce that error locally with gcc (if I remove the hint, and add ac_add_options --enable-warnings-as-errors for good measure to be sure I notice the warning).

I investigated a bit, because it struck me as odd that it *only* happens for this one function, and doesn't happen for several other functions in this patch series that also switch across a "TrackSizingPhase" enum value (with unreachable empty code after the switch statement).

It turns out, GCC only warns if TrackSizingPhase is a function-parameter -- it goes away if we promote it to be a template parameter (which it is in all other usages where we switch over it, AFAICT). We can convert it to be a template-param here, because all of the callsites known the value statically at compile-time.  See attached patch, which fixes the issue for me.

Would you mind fixing this via the attached patch or similar? (at the end of the stack if you like, to avoid bitrotting yourself)  It seems like a net win, by making us use this type more consistently as a template-param, and also by avoiding reliance on the MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE footgun.

Comment 31

[Daniel Holbert [:dholbert]]

Comment on attachment 8958172 [details] [diff] [review]
part 4 - [css-grid] Factor out the starting base/limit size to a templated method (idempotent change)

Review of attachment 8958172 [details] [diff] [review]:
-----------------------------------------------------------------

Faint r- on part 4, since it's not clear to me that it's precisely idempotent/no-behavior-change.

::: layout/generic/nsGridContainerFrame.cpp
@@ +1221,5 @@
>                                   SizingConstraint            aConstraint,
>                                   const LineRange&            aRange,
>                                   const GridItemInfo&         aGridItem);
> +  
> +  // Helper method that returns the track size to use in §11.5.1.2

Drop whitespace on the blank line right before this comment.

(I double-checked the final rollup & this whitespace is present there, too.)

@@ +1236,5 @@
> +      case TrackSizingPhase::eMaxContentMaximums:
> +        if (aSize.mLimit == NS_UNCONSTRAINEDSIZE) {
> +          return aSize.mBase;
> +        }
> +        return aSize.mLimit;

The NS_UNCONSTRAINEDSIZE special case here seems a bit odd, given the "(idempotent change)" in the commit message.

I don't see this special case in the code that's being removed/replaced by this patch...  That replaced code (in  ResetBasePlan) seems to unconditionally use "aSize.mBase":
  aPlan[i].mBase = aSizes[i].mBase;


Perhaps add a bit more explanation in the commit message and/or alongside this code? And/or: if there is "NS_UNCONSTRAINEDSIZE-->choose-mBase-or-mLimit" special-casing *elsewhere* in the file that's being obsoleted by this change, then maybe that old special-casing should be removed here as well?

Comment 32

[Daniel Holbert [:dholbert]]

Comment on attachment 8958173 [details] [diff] [review]
part 5 - [css-grid] Make CollectGrowable a templated method so that it works with either base/limit sizes (idempotent change)

Review of attachment 8958173 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGridContainerFrame.cpp
@@ +1258,5 @@
>      const uint32_t start = aRange.mStart;
>      const uint32_t end = aRange.mEnd;
>      for (uint32_t i = start; i < end; ++i) {
> +      const TrackSize& sz = mSizes[i];
> +      space -= StartSizeInDistribution<phase>(sz);

As in part 4, it's unclear to me why this change is "idempotent".

Could you clarify why the switch from "sz.mBase" to this function call (which now maybe returns mLimit) isn't going to change behavior?

@@ -1265,4 @@
>        if (space <= 0) {
>          return 0;
>        }
> -      if ((sz.mState & aSelector) && !sz.IsFrozen()) {

You're removing the "!sz.IsFrozen()" dependence here, but it's not obvious to me what the significance of that change is, and why that wouldn't make a difference on behavior. (given that this is labeled as an idempotent patch)  Assuming this is nonetheless a correct change: maybe clarify via the commit message and/or a code comment?

Comment 33

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #30)
> It turns out, GCC only warns if TrackSizingPhase is a function-parameter --
> it goes away if we promote it to be a template parameter 

Bah, using a template param was definitely my intention.
Good catch, thanks. (I'll fix it as a separate patch at the end.)

Comment 34

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #31)
> > +      case TrackSizingPhase::eMaxContentMaximums:
> > +        if (aSize.mLimit == NS_UNCONSTRAINEDSIZE) {
> > +          return aSize.mBase;
> > +        }
> > +        return aSize.mLimit;
> 
> The NS_UNCONSTRAINEDSIZE special case here seems a bit odd, given the
> "(idempotent change)" in the commit message.

The call from GrowBase... is using a e*Minimums phase, so isn't affected.
The call from GrowLimit... used to pass in 'aSizes', which comes from 'limits'
in both its callers, which we setup here, in part 1:
+        nsTArray<TrackSize> limits(mSizes);
+        for (TrackSize& sz : limits) {
           if (sz.mLimit == NS_UNCONSTRAINEDSIZE) {
             // use mBase as the planned limit
           } else {
             sz.mBase = sz.mLimit;
           }
         }

So, the new code is initializing directly from mSizes instead of via 'limits'
(which is removed in a later part).

We have always used "plan.mBase" to mean a planned new base or limit size,
even though the name is a bit misleading.  Perhaps this is another good
reason to have a dedicated new struct instead of reusing TrackSize in this
code (so we could name it mPlannedSize).  I'll address this in the follow-up
bug to merge plan/itemplan.

> "NS_UNCONSTRAINEDSIZE-->choose-mBase-or-mLimit" special-casing *elsewhere*
> in the file that's being obsoleted by this change, then maybe that old
> special-casing should be removed here as well?

That code is removed in part 7, after all dependencies on 'limits' are gone.

Comment 35

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #32)
> > +      const TrackSize& sz = mSizes[i];
> > +      space -= StartSizeInDistribution<phase>(sz);
> 
> As in part 4, it's unclear to me why this change is "idempotent".

Same answer as for part 4.

> > -      if ((sz.mState & aSelector) && !sz.IsFrozen()) {
> 
> You're removing the "!sz.IsFrozen()" dependence here, but it's not obvious
> to me what the significance of that change is, and why that wouldn't make a
> difference on behavior.

Yeah, I could probably have removed that already in part 1, but I only
realized it here that we only use this bit for "itemplans", never on
mSizes.

Comment 36

[Daniel Holbert [:dholbert]]

Comment on attachment 8958172 [details] [diff] [review]
part 4 - [css-grid] Factor out the starting base/limit size to a templated method (idempotent change)

Review of attachment 8958172 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on part 4, though:
 - see whitespace nit at beginning of comment 31
 - Ideally, add a note in the commit message mentioning that the e*Maximums cases aren't exercised until a later patch in this queue. (That'll make it clearer to code-archaeologists why this patch is idempotent/no-behavior-change -- since the new code in those cases is different from the code that we're replacing here.)

Comment 37

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #36)
> Ideally, add a note in the commit message mentioning that the e*Maximums cases aren't exercised

(Er, sorry, this wasn't quite correct -- the e*Maximums cases are indeed exercised, but their callers have a similar NS_UNCONSTRAINEDSIZE check up a few stack-levels, as you noted.  Not sure if there's a way to easily distill that in the commit message. If you feel up to it, it might be nice, but up to you.)

Comment 38

[Daniel Holbert [:dholbert]]

Comment on attachment 8958173 [details] [diff] [review]
part 5 - [css-grid] Make CollectGrowable a templated method so that it works with either base/limit sizes (idempotent change)

Review of attachment 8958173 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 39

[Daniel Holbert [:dholbert]]

Comment on attachment 8958175 [details] [diff] [review]
part 6 - [css-grid] Make the size distribution methods templated with the intent of merging them in a later patch (idempotent change)

Review of attachment 8958175 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 40

[Daniel Holbert [:dholbert]]

Comment on attachment 8958177 [details] [diff] [review]
part 7 - [css-grid] Remove the 'limits' copy of track sizes since they are no longer needed (idempotent change)

Review of attachment 8958177 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 41

[Daniel Holbert [:dholbert]]

Comment on attachment 8958178 [details] [diff] [review]
part 8 - [css-grid] Factor out the fit-content clamping function from DistributeToTrackLimits and pass it as a param instead (idempotent change)

Review of attachment 8958178 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 42

[Daniel Holbert [:dholbert]]

Comment on attachment 8958179 [details] [diff] [review]
part 9 - [css-grid] Merge DistributeToTrackLimits/Bases (idempotent change)

Review of attachment 8958179 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 43

[Daniel Holbert [:dholbert]]

Comment on attachment 8958183 [details] [diff] [review]
part 10 - [css-grid] Make MarkExcludedTracks a static method since it doesn't use 'this' (idempotent change)

Review of attachment 8958183 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 44

[Daniel Holbert [:dholbert]]

Comment on attachment 8958186 [details] [diff] [review]
part 11 - [css-grid] Hoist the marking of excluded tracks out from GrowSelectedTracksUnlimited to a separate method (idempotent change)

Review of attachment 8958186 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, one nit:

::: layout/generic/nsGridContainerFrame.cpp
@@ +1412,5 @@
>    }
>  
>    /**
> +   * Mark all tracks in aGrowableTracks with an eSkipGrowUnlimited bit if
> +   * they *shouldn't* grow unlimited in §11.5.1.3

I think "11.5.1.3" is wrong here.

I think you really mean §11.5.1, step 2.3, right? (the step labeled "Distribute space beyond growth limits")

(Right now you're missing the "Step 2" qualifier.  Without that, the comment is implicitly pointing at an unrelated "11.5.1 Step 3" ["Update the tracks' affected sizes"]).

Comment 45

[Daniel Holbert [:dholbert]]

Comment on attachment 8958187 [details] [diff] [review]
part 12 - [css-grid] Merge CopyPlanToBase/Limits into a templated method instead (idempotent change)

Review of attachment 8958187 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGridContainerFrame.cpp
@@ +1295,5 @@
>        const auto& sz = mSizes[i];
>        plan.mBase = StartSizeInDistribution<phase>(sz);
> +      if (phase != TrackSizingPhase::eMaxContentMaximums) {
> +        MOZ_ASSERT(!(sz.mState & TrackSize::eInfinitelyGrowable));
> +      }

Two things:

(1) Perhaps the "if" condition should just be folded into the MOZ_ASSERT condition?  the "if" here isn't guarding any code besides the assertion.  (Fine as-is too, if you prefer).

(2) Please add some sort of brief explanatory message to the MOZ_ASSERT.

@@ -4508,5 @@
>            spanGroupStartIndex, spanGroupEndIndex, fitContentClamper);
> -
> -        for (size_t j = 0, len = mSizes.Length(); j < len; ++j) {
> -          TrackSize& sz = itemPlan[j];
> -          sz.mState &= ~(TrackSize::eFrozen | TrackSize::eSkipGrowUnlimited);

I'm not sure if it's a problem, but FWIW: I don't see where this ^^ removed bit-clearing line is recreated in the new code in this patch.

Do we still clear those bits somehow? Or are we just leaving them now and it doesn't matter?  (I checked the rollup diff, too, and I don't see any new code that'd clear these bits there either.)

@@ -4515,5 @@
> -              mSizes[j].mState |= TrackSize::eInfinitelyGrowable;
> -            }
> -            mSizes[j].mLimit = plan[j].mBase;
> -          }
> -          plan[j].mState &= ~(TrackSize::eModified);

Same here (though this one matters less, perhaps, because this eModified flag was only introduced in an earlier patch in this bug).

Comment 46

[Daniel Holbert [:dholbert]]

Comment on attachment 8958188 [details] [diff] [review]
part 13 - [css-grid] Merge Grow[Base|Limits]ForSpanningItems into a templated method instead (idempotent change)

Review of attachment 8958188 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 47

[Daniel Holbert [:dholbert]]

Comment on attachment 8958190 [details] [diff] [review]
part 14 - [css-grid] Use iterators instead of an array + start/end index for the item data (idempotent change)

Review of attachment 8958190 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGridContainerFrame.cpp
@@ +4439,5 @@
> +    const auto end = step2Items.end();
> +    for (; spanGroupStart != end; spanGroupStart = spanGroupEnd) {
> +      while (spanGroupEnd != end &&
> +             !Step2ItemData::IsSpanLessThan(*spanGroupStart, *spanGroupEnd)) {
> +        ++spanGroupEnd;

Could you add a comment (or expand the existing comment) above the new version of this code, to help the reader form a mental model about what's going on? I looked at it for a minute and couldn't immediately sort out what's going on.

In particular, it's confusing to me that we've got two different "end" variables, one of which we're incrementing and which frequently is equal to the "start" variable. (Not that the code needs rewriting - it just needs a bit of guidance to orient the reader about what's going on.)

Also: a comment just before the "while" loop would be helpful, too, to explain what that loop is doing...  (It looks like it's something simple that could be easily summarized, but it's non-obvious what it is, from just looking at this few lines of code.)

Comment 48

[Daniel Holbert [:dholbert]]

Comment on attachment 8958168 [details] [diff] [review]
part 1 - [css-grid] Change the track sizing algorithm for spanning items so that it accumulates individual item contributions to the plan by max() rather than incrementing the planned size directly

Review of attachment 8958168 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 49

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #44)
> I think you really mean §11.5.1, step 2.3, right? (the step labeled
> "Distribute space beyond growth limits")

Yup, I added the missing 2. and "Distribute space beyond
growth limits" for clarity.

Comment 50

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #45)
> (1) Perhaps the "if" condition should just be folded into the MOZ_ASSERT
> condition?  the "if" here isn't guarding any code besides the assertion. 

Fixed.

> (2) Please add some sort of brief explanatory message to the MOZ_ASSERT.

Fixed.

> > -          sz.mState &= ~(TrackSize::eFrozen | TrackSize::eSkipGrowUnlimited);
> 
> I'm not sure if it's a problem, but FWIW: I don't see where this ^^ removed
> bit-clearing line is recreated in the new code in this patch.

Those bits are only set in the "itemplan" array during GrowSizeForSpanningItems.
We don't copy them anywhere else, and we reset them at the start of
DistributeToTrackSizes in the call to InitializeItemPlan, i.e.
      plan.mState = sz.mState;


> Same here (though this one matters less, perhaps, because this eModified
> flag was only introduced in an earlier patch in this bug).

This bit is only set on "plan".  It's reset at the start of
GrowSizeForSpanningItems in the call to InitializePlan.

(using a dedicated structure for itemplan/plan in the future will hopefully
make this clearer)

Comment 51

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #47)
> @@ +4439,5 @@
> > +    const auto end = step2Items.end();
> > +    for (; spanGroupStart != end; spanGroupStart = spanGroupEnd) {
> > +      while (spanGroupEnd != end &&
> > +             !Step2ItemData::IsSpanLessThan(*spanGroupStart, *spanGroupEnd)) {
> > +        ++spanGroupEnd;
> 
> Could you add a comment (or expand the existing comment) above the new
> version of this code, to help the reader form a mental model about what's
> going on? I looked at it for a minute and couldn't immediately sort out
> what's going on.

The inner loop is replaced in bug 1445229 by:
  spanGroupEnd = spanGroupStart + perSpanData[span].mItemCountWithSameSpan
which hopefully is self-explanatory.  I'll try to add a comment before
the outer loop explaining the use of iterators in that patch.
Basically, we need to run each sizing phase on each subset of items that
has the same span length.

Comment 52

[Daniel Holbert [:dholbert]]

OK, that sounds good. Thanks!

Comment 53

[Daniel Holbert [:dholbert]]

Comment on attachment 8958187 [details] [diff] [review]
part 12 - [css-grid] Merge CopyPlanToBase/Limits into a templated method instead (idempotent change)

Review of attachment 8958187 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 54

[Daniel Holbert [:dholbert]]

Comment on attachment 8958190 [details] [diff] [review]
part 14 - [css-grid] Use iterators instead of an array + start/end index for the item data (idempotent change)

Review of attachment 8958190 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 55

[Daniel Holbert [:dholbert]]

(I assume you've got a followup patch locally for comment 33, to remove the MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE by going from param to template-param.  Up to you whether that needs review; rs=me if it's trivial / approximately like my hackaround.)

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccecf0d22e69
part 1 - [css-grid] Change the track sizing algorithm for spanning items so that it accumulates individual item contributions to the plan by max() rather than incrementing the planned size directly.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/bff0cf63bb77
part 2 - [css-grid] Factor out the min-sizing parts of the track sizing for spanned items to a templated method (idempotent change).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/035a8f610e64
part 3 - [css-grid] Factor out most of the max-sizing parts of the track sizing for spanned items to a templated method (idempotent change).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/a21fbd58159b
part 4 - [css-grid] Factor out the starting base/limit size to a templated method (idempotent change).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/03fbcda33f10
part 5 - [css-grid] Make CollectGrowable a templated method so that it works with either base/limit sizes (idempotent change).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/d94a08198cc9
part 6 - [css-grid] Make the size distribution methods templated with the intent of merging them in a later patch (idempotent change).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a271f8b8d43
part 7 - [css-grid] Remove the 'limits' copy of track sizes since they are no longer needed (idempotent change).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/47858fdcb6d0
part 8 - [css-grid] Factor out the fit-content clamping function from DistributeToTrackLimits and pass it as a param instead (idempotent change).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/020c8add8312
part 9 - [css-grid] Merge DistributeToTrackLimits/Bases (idempotent change).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc82584a8bf8
part 10 - [css-grid] Make MarkExcludedTracks a static method since it doesn't use 'this' (idempotent change).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2b41c222095
part 11 - [css-grid] Hoist the marking of excluded tracks out from GrowSelectedTracksUnlimited to a separate method (idempotent change).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3bfae184ffe
part 12 - [css-grid] Merge CopyPlanToBase/Limits into a templated method instead (idempotent change).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcc155783561
part 13 - [css-grid] Merge Grow[Base|Limits]ForSpanningItems into a templated method instead (idempotent change).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b212971662e
part 14 - [css-grid] Use iterators instead of an array + start/end index for the item data (idempotent change).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/78842ca8c212
part 15 - [css-grid] Test reference fixes + more tests.
https://hg.mozilla.org/integration/mozilla-inbound/rev/40a01f11c5c3
part 16 - [css-grid] Make SizeContributionForPhase a template.  rs=dholbert

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/949cac525749
[css-grid] Follow-up bustage fix for stupid compiler warnings.  r=me  CLOSED TREE

Comment 58

[Mats Palmgren (inactive)]

The above is for a bunch of Visual C++ warnings about: 
'nsGridContainerFrame::Tracks::StartSizeInDistribution<4>': not all control paths return a value

I'm hoping MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE cures that...

Comment 59

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/ccecf0d22e69
https://hg.mozilla.org/mozilla-central/rev/bff0cf63bb77
https://hg.mozilla.org/mozilla-central/rev/035a8f610e64
https://hg.mozilla.org/mozilla-central/rev/a21fbd58159b
https://hg.mozilla.org/mozilla-central/rev/03fbcda33f10
https://hg.mozilla.org/mozilla-central/rev/d94a08198cc9
https://hg.mozilla.org/mozilla-central/rev/6a271f8b8d43
https://hg.mozilla.org/mozilla-central/rev/47858fdcb6d0
https://hg.mozilla.org/mozilla-central/rev/020c8add8312
https://hg.mozilla.org/mozilla-central/rev/fc82584a8bf8
https://hg.mozilla.org/mozilla-central/rev/c2b41c222095
https://hg.mozilla.org/mozilla-central/rev/b3bfae184ffe
https://hg.mozilla.org/mozilla-central/rev/bcc155783561
https://hg.mozilla.org/mozilla-central/rev/5b212971662e
https://hg.mozilla.org/mozilla-central/rev/78842ca8c212
https://hg.mozilla.org/mozilla-central/rev/40a01f11c5c3
https://hg.mozilla.org/mozilla-central/rev/949cac525749

Comment 60

[Mats Palmgren (inactive)]

Comment on attachment 8958168 [details] [diff] [review]
part 1 - [css-grid] Change the track sizing algorithm for spanning items so that it accumulates individual item contributions to the plan by max() rather than incrementing the planned size directly

Approval Request Comment
[Feature/Bug causing the regression]:the bug was in the CSS Grid specification
[User impact if declined]:wrong Grid layout in some cases.  Given that v60 is an ESR, I'd like to fix this bug to avoid potential web-compat issues
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:not yet
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:all revisions in comment 59
[Is the change risky?]:no
[Why is the change risky/not risky?]:nsGridContainerFrame.cpp is the only changed file (apart from tests) so it's guaranteed that the changes won't affect anything but Grid layout
[String changes made/needed]:none

Comment 61

[Julien Cristau [:jcristau]]

Comment on attachment 8958168 [details] [diff] [review]
part 1 - [css-grid] Change the track sizing algorithm for spanning items so that it accumulates individual item contributions to the plan by max() rather than incrementing the planned size directly

I guess we're early enough in beta that it makes sense to take these css grid changes.  beta60+.

Comment 62

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/44425feb2d20
https://hg.mozilla.org/releases/mozilla-beta/rev/934d0c726031
https://hg.mozilla.org/releases/mozilla-beta/rev/2405110bef04
https://hg.mozilla.org/releases/mozilla-beta/rev/a2d9cdb47e62
https://hg.mozilla.org/releases/mozilla-beta/rev/7a68f2b6d545
https://hg.mozilla.org/releases/mozilla-beta/rev/fc18102d61a7
https://hg.mozilla.org/releases/mozilla-beta/rev/8d563458696d
https://hg.mozilla.org/releases/mozilla-beta/rev/58331302df76
https://hg.mozilla.org/releases/mozilla-beta/rev/6dba9c6e951f
https://hg.mozilla.org/releases/mozilla-beta/rev/c8c86f719155
https://hg.mozilla.org/releases/mozilla-beta/rev/f542fec596d9
https://hg.mozilla.org/releases/mozilla-beta/rev/ce0dfee9798f
https://hg.mozilla.org/releases/mozilla-beta/rev/d4d1f62c6211
https://hg.mozilla.org/releases/mozilla-beta/rev/5d5e3495dbe7
https://hg.mozilla.org/releases/mozilla-beta/rev/a37d1635609a
https://hg.mozilla.org/releases/mozilla-beta/rev/d67280b77e23
https://hg.mozilla.org/releases/mozilla-beta/rev/b4987640fade