The intention here is to land simple track sizing and grid item reflow to be able to use reftests as regression test for the earlier work on grid item placement. Basically just support explicit 'px' sizes and nothing else. This code will be throw-away (mostly) until we implement real track sizing per spec.

As I said above, this is just a first primitive version of track sizing so that we can use reftests with explicit "px" sizes on everything, but that's enough for testing grid placement for now. Basically this just implements 11.4 "Initialize Track Sizes" in: http://dev.w3.org/csswg/css-grid/#algo-init So, don't be too nit-picky on this one, there will be opportunities for that in future patches...

This calculates the containing block for each grid item's grid area and reflows the child frame in it. Just normal flow items for now. (Abs.pos. items will be implemented in bug 1107783)

A few tests for the code added in this bug.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c98209e27e79 (don't worry about the OSX build failures, that's bug 1142006)

Comment on attachment 8575968 [details] [diff] [review] part 1, [css-grid] Implement primitive grid track sizing. Review of attachment 8575968 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following: ::: layout/generic/nsGridContainerFrame.cpp @@ +686,5 @@ > + const nsStyleCoord& aMinCoord, > + const nsStyleCoord& aMaxCoord, > + TrackSize* aTrackSize) > +{ > + MOZ_ASSERT(aPercentageBasis != NS_UNCONSTRAINEDSIZE); This should be NS_ASSERTION -- non-fatal -- or else you'll make Jesse's debug builds abort (in a non-ignorable way) when he fuzzes this code using absurdly-large sizes. (Which essentially means he can't fuzz-test this code, which is bad.) (I know this is throwaway code, but I expect that things like preconditions are somewhat likely to stick around.) @@ +724,5 @@ > + nsTArray<TrackSize>& aResults) > +{ > + MOZ_ASSERT(aResults.Length() >= aMinSizingFunctions.Length()); > + MOZ_ASSERT(aResults.Length() >= aMaxSizingFunctions.Length()); > + const size_t len = aMinSizingFunctions.Length(); These asserts are sort of redundant, because aMinSizingFunctions and aMaxSizingFunctions should presumably have the equal lengths. (And checking aResults.Length() separately against 2 things that are equal is redundant.) Though -- it is worth asserting that they do in fact have equal length. (since we're about to index into one, using "i" that runs up to the length of the other one, so they better have the same length!) So I'd suggest just replacing the first assertion with MOZ_ASSERT(aMinSizingFunctions.Length() == aMaxSizingFunctions.Length());

Comment on attachment 8575972 [details] [diff] [review] part 2, [css-grid] Implement grid item containing block calculations and reflow (initial version). Review of attachment 8575972 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following: ::: layout/generic/nsGridContainerFrame.cpp @@ +765,5 @@ > void > +nsGridContainerFrame::LineRange::ToPositionAndLength( > + const nsTArray<TrackSize>& aTrackSizes, nscoord* aPos, nscoord* aLength) const > +{ > + MOZ_ASSERT(mStart != 0 && Extent() > 0); This needs a message (particularly since 0 is a legitimate value for mStart to have). Maybe "Only call on definite LineRanges", or "Only call after this LineRanges has been made definite"? @@ +777,5 @@ > + > + nscoord length = 0; > + uint32_t extent = Extent(); > + while (extent--) { > + length += aTrackSizes[i++].mBase; We need to assert/prove somewhere that this code isn't going to walk off the end of aTrackSizes. I'd suggest adding this at the beginning of the function (before we start indexing into aTrackSizes): const uint32_t end = mEnd - 1; // convert to 0-based idx of 1st invalid track MOZ_ASSERT(end <= aTrackSizes.Length(), "aTrackSizes isn't large enough"); ...and then replace this second loop with: for (; i < end; ++i) { length += aTrackSizes[i].mBase; } @@ +816,5 @@ > + cb += gridOrigin; > + nsHTMLReflowState reflowState(pc, aReflowState, child, cb.Size(wm)); > + const LogicalMargin margin = reflowState.ComputedLogicalMargin(); > + cb.IStart(wm) += margin.IStart(wm); > + cb.BStart(wm) += margin.BStart(wm); This is a bit deceptive -- "cb" is the containing block -- and in reality, the containing block is *not* affected by the child's margin. I'd prefer: LogicalPoint childPos = cb.Origin(wm); childPos.I(wm) += margin.IStart(wm); childPos.B(wm) += margin.BStart(wm); and then use childPos instead of "cb.Origin(wm)" lower down, in ReflowChild and FinishReflowChild. @@ +817,5 @@ > + nsHTMLReflowState reflowState(pc, aReflowState, child, cb.Size(wm)); > + const LogicalMargin margin = reflowState.ComputedLogicalMargin(); > + cb.IStart(wm) += margin.IStart(wm); > + cb.BStart(wm) += margin.BStart(wm); > + if (reflowState.ComputedBSize() == NS_UNCONSTRAINEDSIZE) { Nit: this is inconsistent w/ ::Reflow on whether we check ComputedBSize() against NS_UNCONSTRAINEDSIZE vs. NS_AUTOHEIGHT. This should probably be NS_AUTOHEIGHT, for consistency with other usages I've seen for computed 'height'; though we may need to rename that value (or just remove it) in this brave new world of logical directions. (Feel free to ignore this if you're just considering this section to be throwaway code.) @@ +821,5 @@ > + if (reflowState.ComputedBSize() == NS_UNCONSTRAINEDSIZE) { > + LogicalMargin bp = reflowState.ComputedLogicalBorderPadding(); > + bp.ApplySkipSides(child->GetLogicalSkipSides()); > + nscoord bSize = cb.BSize(wm) - bp.BStartEnd(wm) - margin.BStartEnd(wm); > + reflowState.SetComputedBSize(std::max(bSize, 0)); It looks like this is assuming/implementing "align-self:stretch", I think? (for auto-height grid items) Might be worth mentioning that in a comment; otherwise it's unclear why we'd be sizing auto-height children to fill their containing block. @@ +823,5 @@ > + bp.ApplySkipSides(child->GetLogicalSkipSides()); > + nscoord bSize = cb.BSize(wm) - bp.BStartEnd(wm) - margin.BStartEnd(wm); > + reflowState.SetComputedBSize(std::max(bSize, 0)); > + } > + nsHTMLReflowMetrics childSize(aReflowState); This should take "reflowState", not "aReflowState"! (aReflowState is for the grid; reflowState is for the child) Probably worth naming those variables more-differently, too, so they're easier to distinguish. e.g. "aGridRefowState" (or "aGridRS"?) vs. "childReflowState" perhaps. @@ +824,5 @@ > + nscoord bSize = cb.BSize(wm) - bp.BStartEnd(wm) - margin.BStartEnd(wm); > + reflowState.SetComputedBSize(std::max(bSize, 0)); > + } > + nsHTMLReflowMetrics childSize(aReflowState); > + nsReflowStatus status; Similar to above: we have aStatus (for the grid) & status (for the child) here. One or both should probably be renamed to avoid inevitable confusion/bugs. @@ +863,5 @@ > + WritingMode wm = aReflowState.GetWritingMode(); > + const nscoord computedBSize = aReflowState.ComputedBSize(); > + const nscoord computedISize = aReflowState.ComputedISize(); > + LogicalSize percentageBasis(wm, computedISize, > + computedBSize == NS_AUTOHEIGHT ? 0 : computedBSize); Nit: this is > 80 chars. (feel free to leave if you like, since this is throwaway code) ::: layout/generic/nsGridContainerFrame.h @@ +321,5 @@ > + > + /** > + * Reflow and place our children. > + */ > + void ReflowChildren(const mozilla::LogicalRect& aContentArea, I think you meant to remove the "mozilla::" prefix here? (otherwise I'm not sure why you have "typedef mozilla::LogicalRect LogicalRect;" up above this.) Alternately, leave this as-is & drop the typedef.

(In reply to Daniel Holbert [:dholbert] from comment #7) > ::: layout/generic/nsGridContainerFrame.cpp > @@ +765,5 @@ > > void > > +nsGridContainerFrame::LineRange::ToPositionAndLength( [...] (In reply to Daniel Holbert [:dholbert] from comment #7) > I'd suggest adding this at the beginning of the function (before we start > indexing into aTrackSizes): > const uint32_t end = mEnd - 1; // convert to 0-based idx of 1st invalid track Er, sorry, "invalid track" was wrong there. (I was thinking in terms of grid-bounds, but this is a LineRange). Better suggestion: const uint32_t end = mEnd - 1; // 0-based idx of track after this LineRange

(In reply to Daniel Holbert [:dholbert] from comment #7) > It looks like this is assuming/implementing "align-self:stretch", I think? > (for auto-height grid items) I think it's 'justify-self' actually. I added a XXX comment. (I implemented this bit to be able to easily compare layout with Chrome) Addressed the other nits. Final Try run is green for the record: https://treeherder.mozilla.org/#/jobs?repo=try&revision=494e37b67e37

(In reply to Mats Palmgren (:mats) from comment #9) > (In reply to Daniel Holbert [:dholbert] from comment #7) > > It looks like this is assuming/implementing "align-self:stretch", I think? > > (for auto-height grid items) > > I think it's 'justify-self' actually. I added a XXX comment. > (I implemented this bit to be able to easily compare layout with Chrome) I'm pretty sure it's "align-self", actually. "align-self" is for block/cross-axis alignment/stretching[1] (which is what you're doing here). In contrast, "justify-self" is for inline/main-axis alignment/stretching[2]. [1] http://dev.w3.org/csswg/css-align/#align-self-property [2] http://dev.w3.org/csswg/css-align/#justify-self-property

Hmm, the spec says "The 'align-self' property applies along the grid’s column axis." [1] and the "column axis" is the "vertical axis" (assuming default Western writing mode). So that's not what I'm doing here, I'm stretching the height of the grid item to align it with the container's horizontal axis (row axis). Maybe I'm misunderstanding what "applies along" means in this context.

(In reply to Mats Palmgren (:mats) from comment #11) > Hmm, the spec says "The 'align-self' property applies along the grid’s > column axis." [1] > and the "column axis" is the "vertical axis" (assuming default Western > writing mode). Right -- and the code I'm talking about is stretching the item's height (which *is* its vertical/column-axis size). The other "align-self" values give you non-stretchy options to align the item vertically -- e.g. at the top or bottom of its row. This is alignment in the column-axis, within the bounds established by the row. Does that make sense?

In other words: when the spec says "Aligns the box within its parent along the block/column/cross axis" ...it means: "establish the box's *position & size* in the block/column/cross-axis" (i.e. its y-position & height) (I think you're reading it as being about the position *within the fixed bounds of its column*, i.e. its position between column-lines, i.e. its x-position/width. That's the opposite of what it's supposed to mean, though. I'm pretty confident on this, based on its meaning in flexbox [and the assumption that it means the same thing in grid & flexbox]. It's entirely possible that the spec language can be clarified here -- if you can think of a way to phrase this less ambiguously, definitely email www-style.)

OK, now I see what you mean. I was thinking more in terms of applying alignment to a set of grid items to make them align, more from the container's POV I guess. But yeah, the property applies to the item itself of course, that makes sense. Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/94b4fb5fa88e https://hg.mozilla.org/integration/mozilla-inbound/rev/ff6df2d098cb https://hg.mozilla.org/integration/mozilla-inbound/rev/5841f1273594 https://hg.mozilla.org/integration/mozilla-inbound/rev/41827d04d683

https://hg.mozilla.org/mozilla-central/rev/94b4fb5fa88e https://hg.mozilla.org/mozilla-central/rev/ff6df2d098cb https://hg.mozilla.org/mozilla-central/rev/5841f1273594 https://hg.mozilla.org/mozilla-central/rev/41827d04d683