[CSSGridLayout] Skip items spanning flex tracks in track sizing algo

[CSSGridLayout] Skip items spanning flex tracks in track sizing algo

Section 10.4 of the specs forces us to ignore items spanning tracks with
flex sizing functions when resolving the content-based track sizing
functions. Items with span < 2 are not affected by this rule (as they will
belong to a single track). This way the algorithm ensures that min-content
and max-content restrictions are fulfilled before distributing the extra
space.

BUG=392200
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183807

[svillar, 2014-08-07 10:52:48]

Sending for review

[cbiesinger, 2014-08-12 01:21:08]

this looks plausible, but I don't think I understand grid well enough to really
review this, unfortunately

[svillar, 2014-08-26 09:12:51]

@jchaffraix, @esprehn: anyone?

[Julien - ping for review, 2014-10-10 18:09:16]

https://codereview.chromium.org/448993002/diff/1/LayoutTests/fast/css-grid-la...
File
LayoutTests/fast/css-grid-layout/flex-and-content-sized-resolution-columns.html
(right):

https://codereview.chromium.org/448993002/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/flex-and-content-sized-resolution-columns.html:96:
shouldBeEqualToString("window.getComputedStyle(" + gridId + ",
'').getPropertyValue('grid-template-columns')", columnValue);
Shouldn't this be window.gridElement? (gridId is defined automatically but I
don't like to rely our bad behavior)

https://codereview.chromium.org/448993002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/448993002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:719: if (gridItemSpan > 1 &&
(trackSize.minTrackBreadth().isFlex() || trackSize.maxTrackBreadth().isFlex()))
While this will work and remove any grid item spanning a flexible track,
wouldn't it be better filter them out? (this code will be called 4 times per
grid item)

I am thinking that aligning our algorithm with what the algorithm's
specification may help us solve all these discrepancies in our implementation
based on the older specification.

[svillar, 2014-10-13 06:52:32]

https://codereview.chromium.org/448993002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/448993002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:719: if (gridItemSpan > 1 &&
(trackSize.minTrackBreadth().isFlex() || trackSize.maxTrackBreadth().isFlex()))
On 2014/10/10 18:09:16, Julien Chaffraix - PST wrote:
> While this will work and remove any grid item spanning a flexible track,
> wouldn't it be better filter them out? (this code will be called 4 times per
> grid item)

That's true.

> I am thinking that aligning our algorithm with what the algorithm's
> specification may help us solve all these discrepancies in our implementation
> based on the older specification.

Do you mean replacing it by a new version based on the rewritten algorithm?

[svillar, 2014-10-13 14:38:44]

https://codereview.chromium.org/448993002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/448993002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:719: if (gridItemSpan > 1 &&
(trackSize.minTrackBreadth().isFlex() || trackSize.maxTrackBreadth().isFlex()))
On 2014/10/13 06:52:32, svillar wrote:
> On 2014/10/10 18:09:16, Julien Chaffraix - PST wrote:
> > While this will work and remove any grid item spanning a flexible track,
> > wouldn't it be better filter them out? (this code will be called 4 times per
> > grid item)
> 
> That's true.

BTW doing this would involve an extra hash lookup per item as we need the
cachedGridCoordinate to filter items out. BUT if we wait for issue 592803003 to
land (with the std::pair -> class change) this won't be an issue at all. Another
reason to replace the pair with a class :).

[Julien - ping for review, 2014-10-13 19:43:49]

https://codereview.chromium.org/448993002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/448993002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:719: if (gridItemSpan > 1 &&
(trackSize.minTrackBreadth().isFlex() || trackSize.maxTrackBreadth().isFlex()))
On 2014/10/13 06:52:32, svillar wrote:
> On 2014/10/10 18:09:16, Julien Chaffraix - PST wrote:
> > While this will work and remove any grid item spanning a flexible track,
> > wouldn't it be better filter them out? (this code will be called 4 times per
> > grid item)
> 
> That's true.
> 
> > I am thinking that aligning our algorithm with what the algorithm's
> > specification may help us solve all these discrepancies in our
implementation
> > based on the older specification.
> 
> Do you mean replacing it by a new version based on the rewritten algorithm?

I would go with incremental changes for that as it's going to be difficult to
land a rewritten algorithm. That means probably starting to rework the way we
iterate and some of our datastructure.

What concerns me is how much bugs we will have due to shoe-horning our current
algorithm into the new one's.

https://codereview.chromium.org/448993002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:719: if (gridItemSpan > 1 &&
(trackSize.minTrackBreadth().isFlex() || trackSize.maxTrackBreadth().isFlex()))
On 2014/10/13 14:38:44, svillar wrote:
> On 2014/10/13 06:52:32, svillar wrote:
> > On 2014/10/10 18:09:16, Julien Chaffraix - PST wrote:
> > > While this will work and remove any grid item spanning a flexible track,
> > > wouldn't it be better filter them out? (this code will be called 4 times
per
> > > grid item)
> > 
> > That's true.
> 
> BTW doing this would involve an extra hash lookup per item as we need the
> cachedGridCoordinate to filter items out. BUT if we wait for issue 592803003
to
> land (with the std::pair -> class change) this won't be an issue at all.
Another
> reason to replace the pair with a class :).

Another argument for the class, I am officially overruled :)

It's LGTM'ed so I am looking forward to rebasing this change on top of it.

[svillar, 2014-10-14 06:29:13]

https://codereview.chromium.org/448993002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/448993002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:719: if (gridItemSpan > 1 &&
(trackSize.minTrackBreadth().isFlex() || trackSize.maxTrackBreadth().isFlex()))
On 2014/10/13 19:43:48, Julien Chaffraix - PST wrote:
> On 2014/10/13 06:52:32, svillar wrote:
> > On 2014/10/10 18:09:16, Julien Chaffraix - PST wrote:
> > > While this will work and remove any grid item spanning a flexible track,
> > > wouldn't it be better filter them out? (this code will be called 4 times
per
> > > grid item)
> > 
> > That's true.
> > 
> > > I am thinking that aligning our algorithm with what the algorithm's
> > > specification may help us solve all these discrepancies in our
> implementation
> > > based on the older specification.
> > 
> > Do you mean replacing it by a new version based on the rewritten algorithm?
> 
> I would go with incremental changes for that as it's going to be difficult to
> land a rewritten algorithm. That means probably starting to rework the way we
> iterate and some of our datastructure.
> 
> What concerns me is how much bugs we will have due to shoe-horning our current
> algorithm into the new one's.

Yeah but note that this step was already present in the old algorithm (as it
should be as they're supposed to produce identical results) so we aren't at that
point yet, just polishing the final bits of the "old" implementation.

[svillar, 2014-10-15 15:05:34]

@jchaffraix: I uploaded a new version which pre-filters spanning items crossing
flex tracks even before sorting them. If you don't raise any objection I'll land
it tomorrow.

[Julien - ping for review, 2014-10-15 16:04:58]

lgtm.

In the future, please don't highjack unwanted LGTM (I was talking about the
other change) :-/

https://codereview.chromium.org/448993002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/448993002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:719: if (gridItemSpan > 1 &&
(trackSize.minTrackBreadth().isFlex() || trackSize.maxTrackBreadth().isFlex()))
On 2014/10/14 06:29:13, svillar wrote:
> On 2014/10/13 19:43:48, Julien Chaffraix - PST wrote:
> > On 2014/10/13 06:52:32, svillar wrote:
> > > On 2014/10/10 18:09:16, Julien Chaffraix - PST wrote:
> > > > While this will work and remove any grid item spanning a flexible track,
> > > > wouldn't it be better filter them out? (this code will be called 4 times
> per
> > > > grid item)
> > > 
> > > That's true.
> > > 
> > > > I am thinking that aligning our algorithm with what the algorithm's
> > > > specification may help us solve all these discrepancies in our
> > implementation
> > > > based on the older specification.
> > > 
> > > Do you mean replacing it by a new version based on the rewritten
algorithm?
> > 
> > I would go with incremental changes for that as it's going to be difficult
to
> > land a rewritten algorithm. That means probably starting to rework the way
we
> > iterate and some of our datastructure.
> > 
> > What concerns me is how much bugs we will have due to shoe-horning our
current
> > algorithm into the new one's.
> 
> Yeah but note that this step was already present in the old algorithm (as it
> should be as they're supposed to produce identical results) so we aren't at
that
> point yet, just polishing the final bits of the "old" implementation.

100% agreed! I was just mentioning that as it may be better to align ourself
sooner rather than later.

https://codereview.chromium.org/448993002/diff/70001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/448993002/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:723: const GridTrackSize& trackSize =
gridTrackSize(direction, trackPosition.toInt());
Too bad we have this call, else we could have moved this function to
GridItemWithSpan.

https://codereview.chromium.org/448993002/diff/90001/LayoutTests/fast/css-gri...
File
LayoutTests/fast/css-grid-layout/flex-and-content-sized-resolution-columns.html
(right):

https://codereview.chromium.org/448993002/diff/90001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/flex-and-content-sized-resolution-columns.html:101:
checkColumns("gridMinMaxFixedFlexAndMaxContentAndAuto", "60px 20px 20px");
I have a hard time saying whether any of these results are correct based on the
current spec's algorithm, which is super convoluted when it comes to growth
limits.

[svillar, 2014-10-16 06:36:30]

First of all sorry for the missunderstanding. It was not my intention at all to
highjack the LGTM, I just didn't understand properly what you meant.

https://codereview.chromium.org/448993002/diff/70001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/448993002/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:723: const GridTrackSize& trackSize =
gridTrackSize(direction, trackPosition.toInt());
On 2014/10/15 16:04:58, Julien Chaffraix - PST wrote:
> Too bad we have this call, else we could have moved this function to
> GridItemWithSpan.

Acknowledged.

https://codereview.chromium.org/448993002/diff/90001/LayoutTests/fast/css-gri...
File
LayoutTests/fast/css-grid-layout/flex-and-content-sized-resolution-columns.html
(right):

https://codereview.chromium.org/448993002/diff/90001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/flex-and-content-sized-resolution-columns.html:101:
checkColumns("gridMinMaxFixedFlexAndMaxContentAndAuto", "60px 20px 20px");
On 2014/10/15 16:04:58, Julien Chaffraix - PST wrote:
> I have a hard time saying whether any of these results are correct based on
the
> current spec's algorithm, which is super convoluted when it comes to growth
> limits.

Ideally we should have w3c tests for this kind of stuff. From my side all I can
say is that I try to compute the result first and then I code the test, so
should there be an error is a human mistake not algorithm's :)

[svillar, 2014-10-16 09:51:29]

The CQ bit was checked by svillar@igalia.com

[commit-bot: I haz the power, 2014-10-16 09:51:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/448993002/90001

[commit-bot: I haz the power, 2014-10-16 12:07:25]

Committed patchset #3 (id:90001) as 183807