[css-grid] Fix track sizing algo w/ size restrictions and intrinsic sizes

[css-grid] Fix track sizing algo w/ size restrictions and intrinsic sizes

The current implementation does not comply with what specs say about the
size available for the track sizing algorithm when the grid container is
sized under min/max size restrictions and/or when it has intrinsic sizes.

For the case of columns it was working fine as the intrinsic widths are
properly calculated by computeIntrinsicLogicalWidths() before the layout of
grid items is done. The problem is the rows as the heights depend on the
widths in the CSS Box model.

Apart from that we properly detect from now on when a grid container either
has a definite or indefinite size.

BUG=423743
Committed: https://crrev.com/889d2498a67a461393da302bf244bc35d507e268
Cr-Commit-Position: refs/heads/master@{#358816}

[svillar, 2015-09-01 14:17:13]

svillar@igalia.com changed reviewers:
+ cbiesinger@chromium.org, jfernandez@igalia.com, rego@igalia.com

[svillar, 2015-09-01 14:17:14]

Adding reviewers

[jfernandez, 2015-09-01 23:41:39]

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
File LayoutTests/fast/css-grid-layout/grid-element-change-columns-repaint.html
(right):

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
LayoutTests/fast/css-grid-layout/grid-element-change-columns-repaint.html:19:
width: -webkit-min-content;
Why this change ? Is it related to the issue we are trying to solve ?

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
File LayoutTests/fast/css-grid-layout/grid-item-change-column-repaint.html
(right):

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
LayoutTests/fast/css-grid-layout/grid-item-change-column-repaint.html:19: width:
-webkit-min-content;
Ditto.

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
File
LayoutTests/fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html
(right):

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
LayoutTests/fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html:27:
<div class="sizedToGridArea" data-expected-width="40"
data-expected-height="35">XX XXX XX XXX</div>
I don't get the 'sizedToGridArea' because that's what 'stretch' already does.
You need it because you have set align-items and justify-items to 'start'. Just
wondering the reason behind this.

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
File LayoutTests/fast/css-grid-layout/percent-of-indefinite-track-size.html
(right):

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
LayoutTests/fast/css-grid-layout/percent-of-indefinite-track-size.html:6: width:
-webkit-min-content;
Same question than before.

https://codereview.chromium.org/1317643005/diff/1/Source/core/layout/LayoutGr...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1317643005/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:455: const bool hasDefiniteAvailableSpace =
(direction == ForRows) ? hasDefiniteLogicalHeight() : true;
I don't get why hasDefiniteAvailableSpace is always true when direction is
ForColumns. I asks this specially regarding the flexible tracks size
computation.

[svillar, 2015-09-02 13:25:48]

Thanks for the review!

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
File LayoutTests/fast/css-grid-layout/grid-element-change-columns-repaint.html
(right):

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
LayoutTests/fast/css-grid-layout/grid-element-change-columns-repaint.html:19:
width: -webkit-min-content;
On 2015/09/01 23:41:39, jfernandez wrote:
> Why this change ? Is it related to the issue we are trying to solve ?

Because without my patch, fit-content was behaving like min-content and the test
expects that behavior, i.e. tracks are now maximized to their limits.

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
File LayoutTests/fast/css-grid-layout/grid-item-change-column-repaint.html
(right):

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
LayoutTests/fast/css-grid-layout/grid-item-change-column-repaint.html:19: width:
-webkit-min-content;
On 2015/09/01 23:41:39, jfernandez wrote:
> Ditto.

Same reply.

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
File
LayoutTests/fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html
(right):

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
LayoutTests/fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html:27:
<div class="sizedToGridArea" data-expected-width="40"
data-expected-height="35">XX XXX XX XXX</div>
On 2015/09/01 23:41:39, jfernandez wrote:
> I don't get the 'sizedToGridArea' because that's what 'stretch' already does.
> You need it because you have set align-items and justify-items to 'start'.
Just
> wondering the reason behind this.

Hmm good point, I should have done it for some reason, but I don't remember now.
I agree what I did seems equivalent to not adding anything...

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
File LayoutTests/fast/css-grid-layout/percent-of-indefinite-track-size.html
(right):

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
LayoutTests/fast/css-grid-layout/percent-of-indefinite-track-size.html:6: width:
-webkit-min-content;
On 2015/09/01 23:41:39, jfernandez wrote:
> Same question than before.

Same reply

https://codereview.chromium.org/1317643005/diff/1/Source/core/layout/LayoutGr...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1317643005/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:455: const bool hasDefiniteAvailableSpace =
(direction == ForRows) ? hasDefiniteLogicalHeight() : true;
On 2015/09/01 23:41:39, jfernandez wrote:
> I don't get why hasDefiniteAvailableSpace is always true when direction is
> ForColumns. I asks this specially regarding the flexible tracks size
> computation.

It's explained in the comment I added bellow, for the case of columns
computeIntrinsicLogicalWidths() has been already called, so we always get a
valid available space, something that does not happen for heights (as they
depend on widths, 'auto' means a different thing, etc...)

Regarding flex tracks computation I think you have a point. I'd need to double
check that it's still working fine.

[Manuel Rego, 2015-09-17 11:04:03]

Just another informal review with some questions.

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
File LayoutTests/fast/css-grid-layout/grid-preferred-logical-widths.html (left):

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
LayoutTests/fast/css-grid-layout/grid-preferred-logical-widths.html:46:
.dummyContainer { }
Not sure if it's a good idea to remove this class in this change,
as it's really hard to see the change of behavior in the tests.

Would it be possible to remove it in a follow-up patch?

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
File LayoutTests/fast/css-grid-layout/grid-preferred-logical-widths.html
(right):

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
LayoutTests/fast/css-grid-layout/grid-preferred-logical-widths.html:169: <div
class="dummyContainer min-content grid gridMinContentFixed minWidth70"
data-expected-height="10" data-expected-width="70">
BTW, if you remove the .dummyContainer class you should stop to use it here too.

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
File
LayoutTests/fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html
(right):

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
LayoutTests/fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html:27:
<div class="sizedToGridArea" data-expected-width="40"
data-expected-height="35">XX XXX XX XXX</div>
On 2015/09/02 13:25:48, svillar wrote:
> On 2015/09/01 23:41:39, jfernandez wrote:
> > I don't get the 'sizedToGridArea' because that's what 'stretch' already
does.
> > You need it because you have set align-items and justify-items to 'start'.
> Just
> > wondering the reason behind this.
> 
> Hmm good point, I should have done it for some reason, but I don't remember
now.
> I agree what I did seems equivalent to not adding anything...

You're using "align|justify-items: start;" before, so you need this
if you want them to take 100% of the size.
Not sure why you've used "start" before though.

In addition .sizedToGridArea is the class setting Ahem font.
Probably we should change this at some point and
try to remove the "width|height: 100%;" from .sizedToGridArea
(as stretch should be already doing it)
and rename the class to something related to the Ahem font.

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
LayoutTests/fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html:80:
</div>
This seems a duplicated test comparing it with line 52.

I see that the item will be 20x20 vs 40x40,
but the min-height is 50px,
so it's bigger than the item in both cases.

https://codereview.chromium.org/1317643005/diff/1/Source/core/layout/LayoutGr...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1317643005/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:396: return
style.logicalMinHeight().isMinContent() || style.logicalHeight().isMinContent()
|| style.logicalMaxHeight().isMinContent();
What happens if it's not you, but it's an ancestor
the one sized under a constraint?

https://codereview.chromium.org/1317643005/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:455: const bool hasDefiniteAvailableSpace =
(direction == ForRows) ? hasDefiniteLogicalHeight() : true;
On 2015/09/02 13:25:48, svillar wrote:
> On 2015/09/01 23:41:39, jfernandez wrote:
> > I don't get why hasDefiniteAvailableSpace is always true when direction is
> > ForColumns. I asks this specially regarding the flexible tracks size
> > computation.
> 
> It's explained in the comment I added bellow, for the case of columns
> computeIntrinsicLogicalWidths() has been already called, so we always get a
> valid available space, something that does not happen for heights (as they
> depend on widths, 'auto' means a different thing, etc...)

Probably the comment should be before the variable itself. :-)

https://codereview.chromium.org/1317643005/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:487: }
In order to try to simplify this "if-else",
would it be possible to modify the freeSpace variable
and always call maximizeTracks()?

[svillar, 2015-09-17 14:05:44]

Thanks for the reviews. I'll update the patch with some of the suggestions.

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
File LayoutTests/fast/css-grid-layout/grid-preferred-logical-widths.html (left):

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
LayoutTests/fast/css-grid-layout/grid-preferred-logical-widths.html:46:
.dummyContainer { }
On 2015/09/17 11:04:02, Manuel Rego wrote:
> Not sure if it's a good idea to remove this class in this change,
> as it's really hard to see the change of behavior in the tests.
> 
> Would it be possible to remove it in a follow-up patch?

I agree with you. Let's do it later.

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
File LayoutTests/fast/css-grid-layout/grid-preferred-logical-widths.html
(right):

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
LayoutTests/fast/css-grid-layout/grid-preferred-logical-widths.html:169: <div
class="dummyContainer min-content grid gridMinContentFixed minWidth70"
data-expected-height="10" data-expected-width="70">
On 2015/09/17 11:04:02, Manuel Rego wrote:
> BTW, if you remove the .dummyContainer class you should stop to use it here
too.

Acknowledged.

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
File
LayoutTests/fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html
(right):

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
LayoutTests/fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html:27:
<div class="sizedToGridArea" data-expected-width="40"
data-expected-height="35">XX XXX XX XXX</div>
On 2015/09/17 11:04:02, Manuel Rego wrote:
> On 2015/09/02 13:25:48, svillar wrote:
> > On 2015/09/01 23:41:39, jfernandez wrote:
> > > I don't get the 'sizedToGridArea' because that's what 'stretch' already
> does.
> > > You need it because you have set align-items and justify-items to 'start'.
> > Just
> > > wondering the reason behind this.
> > 
> > Hmm good point, I should have done it for some reason, but I don't remember
> now.
> > I agree what I did seems equivalent to not adding anything...
> 
> You're using "align|justify-items: start;" before, so you need this
> if you want them to take 100% of the size.
> Not sure why you've used "start" before though.
> 
> In addition .sizedToGridArea is the class setting Ahem font.
> Probably we should change this at some point and
> try to remove the "width|height: 100%;" from .sizedToGridArea
> (as stretch should be already doing it)
> and rename the class to something related to the Ahem font.

Yeah, that's it. The patch is a bit old, so I forgot the rationale behind some
decisions.

https://codereview.chromium.org/1317643005/diff/1/Source/core/layout/LayoutGr...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1317643005/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:396: return
style.logicalMinHeight().isMinContent() || style.logicalHeight().isMinContent()
|| style.logicalMaxHeight().isMinContent();
On 2015/09/17 11:04:03, Manuel Rego wrote:
> What happens if it's not you, but it's an ancestor
> the one sized under a constraint?

On the one hand the specs explicitly mention "When the grid container is
sized..." and on the other inspecting the ancestors to make a decision would be
weird, for example, what if I have:

max-content
|_min-content
  |_auto <- this is the grid container

am I under min-content or max-content or neither?.

Apart from that, I also decided not to check the ancestors because this specific
behavior is totally different to how "normal" boxes are sized under those
constraints so I think it makes sense to limit it to the grid container.

https://codereview.chromium.org/1317643005/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:455: const bool hasDefiniteAvailableSpace =
(direction == ForRows) ? hasDefiniteLogicalHeight() : true;
On 2015/09/17 11:04:02, Manuel Rego wrote:
> On 2015/09/02 13:25:48, svillar wrote:
> > On 2015/09/01 23:41:39, jfernandez wrote:
> > > I don't get why hasDefiniteAvailableSpace is always true when direction is
> > > ForColumns. I asks this specially regarding the flexible tracks size
> > > computation.
> > 
> > It's explained in the comment I added bellow, for the case of columns
> > computeIntrinsicLogicalWidths() has been already called, so we always get a
> > valid available space, something that does not happen for heights (as they
> > depend on widths, 'auto' means a different thing, etc...)
> 
> Probably the comment should be before the variable itself. :-)

Acknowledged.

https://codereview.chromium.org/1317643005/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:487: }
On 2015/09/17 11:04:03, Manuel Rego wrote:
> In order to try to simplify this "if-else",
> would it be possible to modify the freeSpace variable
> and always call maximizeTracks()?

If you mean the whole if-else thing the answer is no, because as you can see
there are some branches that lead to no track maximization (like being under
min-content and min-size being a fixed length).

If you mean the setBaseSize(growthLimit()) ones then the answer is, yes it can
be done, but setting the base size to the growth limit directly is much more
efficient than distributing that free space.

[svillar, 2015-09-17 15:37:41]

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
File LayoutTests/fast/css-grid-layout/grid-preferred-logical-widths.html (left):

https://codereview.chromium.org/1317643005/diff/1/LayoutTests/fast/css-grid-l...
LayoutTests/fast/css-grid-layout/grid-preferred-logical-widths.html:46:
.dummyContainer { }
On 2015/09/17 14:05:44, svillar wrote:
> On 2015/09/17 11:04:02, Manuel Rego wrote:
> > Not sure if it's a good idea to remove this class in this change,
> > as it's really hard to see the change of behavior in the tests.
> > 
> > Would it be possible to remove it in a follow-up patch?
> 
> I agree with you. Let's do it later.

Actually I tried, and the diff is not better so I'll leave it as it is.

[svillar, 2015-09-17 15:44:08]

I've moved the code to a new function which computes the space to distribute.
maximizeTracks() also have 2 new special cases, one for infinity (meaning grow
till the limits) and another one for 0 (meaning don't do anything)

[jfernandez, 2015-09-17 15:57:23]

lgtm

The code is much clearer now.

[Manuel Rego, 2015-09-18 10:52:08]

Great changes, the code looks much better now!

https://codereview.chromium.org/1317643005/diff/1/Source/core/layout/LayoutGr...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1317643005/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:396: return
style.logicalMinHeight().isMinContent() || style.logicalHeight().isMinContent()
|| style.logicalMaxHeight().isMinContent();
On 2015/09/17 14:05:44, svillar wrote:
> On 2015/09/17 11:04:03, Manuel Rego wrote:
> > What happens if it's not you, but it's an ancestor
> > the one sized under a constraint?
> 
> On the one hand the specs explicitly mention "When the grid container is
> sized..." and on the other inspecting the ancestors to make a decision would
be
> weird, for example, what if I have:
> 
> max-content
> |_min-content
>   |_auto <- this is the grid container
> 
> am I under min-content or max-content or neither?.
> 
> Apart from that, I also decided not to check the ancestors because this
specific
> behavior is totally different to how "normal" boxes are sized under those
> constraints so I think it makes sense to limit it to the grid container.

Sorry but I'm not still 100% sure about this. :-/

The thing is that for width, we take into account
the sizing constraints of the ancestors.

So, with this patch we'll have a different behavior
for widths than for heights
(when the constraint comes from an ancestor).
Example:
http://jsbin.com/wiroyeduro/1/edit?html,css,output

Maybe we should clarify the meaning of
"is sized under a XXX constraint" in the specs.

https://codereview.chromium.org/1317643005/diff/20001/LayoutTests/fast/css-gr...
File
LayoutTests/fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html
(right):

https://codereview.chromium.org/1317643005/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html:14:

Can't we do the same than for the width test here?
That means remove the alignment "start" and
not use the sizedToGridArea class.

[svillar, 2015-09-18 12:01:20]

https://codereview.chromium.org/1317643005/diff/1/Source/core/layout/LayoutGr...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1317643005/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:396: return
style.logicalMinHeight().isMinContent() || style.logicalHeight().isMinContent()
|| style.logicalMaxHeight().isMinContent();
On 2015/09/18 10:52:07, Manuel Rego wrote:
> On 2015/09/17 14:05:44, svillar wrote:
> > On 2015/09/17 11:04:03, Manuel Rego wrote:
> > > What happens if it's not you, but it's an ancestor
> > > the one sized under a constraint?
> > 
> > On the one hand the specs explicitly mention "When the grid container is
> > sized..." and on the other inspecting the ancestors to make a decision would
> be
> > weird, for example, what if I have:
> > 
> > max-content
> > |_min-content
> >   |_auto <- this is the grid container
> > 
> > am I under min-content or max-content or neither?.
> > 
> > Apart from that, I also decided not to check the ancestors because this
> specific
> > behavior is totally different to how "normal" boxes are sized under those
> > constraints so I think it makes sense to limit it to the grid container.
> 
> Sorry but I'm not still 100% sure about this. :-/
> 
> The thing is that for width, we take into account
> the sizing constraints of the ancestors.
> 
> So, with this patch we'll have a different behavior
> for widths than for heights
> (when the constraint comes from an ancestor).
> Example:
> http://jsbin.com/wiroyeduro/1/edit?html,css,output
> 
> Maybe we should clarify the meaning of
> "is sized under a XXX constraint" in the specs.

Well in this particular example the fact that height and width are different is
because auto is different. For widths auto means ~ fill the container (as
min-content is 0 then the width is 0) while for heights auto means fit contents
(as the row is 100px tall that's the height of the grid).

https://codereview.chromium.org/1317643005/diff/20001/LayoutTests/fast/css-gr...
File
LayoutTests/fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html
(right):

https://codereview.chromium.org/1317643005/diff/20001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html:14:

On 2015/09/18 10:52:08, Manuel Rego wrote:
> Can't we do the same than for the width test here?
> That means remove the alignment "start" and
> not use the sizedToGridArea class.

I guess so.

[Manuel Rego, 2015-09-22 10:12:34]

https://codereview.chromium.org/1317643005/diff/1/Source/core/layout/LayoutGr...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1317643005/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:396: return
style.logicalMinHeight().isMinContent() || style.logicalHeight().isMinContent()
|| style.logicalMaxHeight().isMinContent();
On 2015/09/18 12:01:20, svillar wrote:
> On 2015/09/18 10:52:07, Manuel Rego wrote:
> > On 2015/09/17 14:05:44, svillar wrote:
> > > On 2015/09/17 11:04:03, Manuel Rego wrote:
> > > > What happens if it's not you, but it's an ancestor
> > > > the one sized under a constraint?
> > > 
> > > On the one hand the specs explicitly mention "When the grid container is
> > > sized..." and on the other inspecting the ancestors to make a decision
would
> > be
> > > weird, for example, what if I have:
> > > 
> > > max-content
> > > |_min-content
> > >   |_auto <- this is the grid container
> > > 
> > > am I under min-content or max-content or neither?.
> > > 
> > > Apart from that, I also decided not to check the ancestors because this
> > specific
> > > behavior is totally different to how "normal" boxes are sized under those
> > > constraints so I think it makes sense to limit it to the grid container.
> > 
> > Sorry but I'm not still 100% sure about this. :-/
> > 
> > The thing is that for width, we take into account
> > the sizing constraints of the ancestors.
> > 
> > So, with this patch we'll have a different behavior
> > for widths than for heights
> > (when the constraint comes from an ancestor).
> > Example:
> > http://jsbin.com/wiroyeduro/1/edit?html,css,output
> > 
> > Maybe we should clarify the meaning of
> > "is sized under a XXX constraint" in the specs.
> 
> Well in this particular example the fact that height and width are different
is
> because auto is different. For widths auto means ~ fill the container (as
> min-content is 0 then the width is 0) while for heights auto means fit
contents
> (as the row is 100px tall that's the height of the grid).

My question is why the row is 100px tall and not 0px tall.

For example imagine that we don't have any item:
http://jsbin.com/kusore/1/edit?html,css,output

Sorry but I'm still not sure about all this,
but this kind of checks inside LayoutGrid
make me wonder if we're doing the right thing.

[cbiesinger, 2015-09-30 15:49:35]

https://codereview.chromium.org/1317643005/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1317643005/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:394: static bool
heightSizedUnderMinContentConstraint(const ComputedStyle& style)
I'm somewhat skeptical about this design -- does this mean it is not possible to
calculate a min-content and max-content width for a given grid? "sized under a
min-content constraint" is not so much a feature of a given element, it's an
aspect of what's being calculated right now, isn't it?

[svillar, 2015-10-01 14:56:04]

On 2015/09/30 15:49:35, cbiesinger wrote:
>
https://codereview.chromium.org/1317643005/diff/20001/Source/core/layout/Layo...
> File Source/core/layout/LayoutGrid.cpp (right):
> 
>
https://codereview.chromium.org/1317643005/diff/20001/Source/core/layout/Layo...
> Source/core/layout/LayoutGrid.cpp:394: static bool
> heightSizedUnderMinContentConstraint(const ComputedStyle& style)
> I'm somewhat skeptical about this design -- does this mean it is not possible
to
> calculate a min-content and max-content width for a given grid? "sized under a
> min-content constraint" is not so much a feature of a given element, it's an
> aspect of what's being calculated right now, isn't it?

Actually it is possible, both are already properly calculated and stored in
minPreferredLogicalWidth and maxPreferredLogicalWidth. This bug is more about
heights.

Anyway I was not happy with the design either, so I'm uploading soon a new patch
with a much better design and integration with the rest of layout Blink
infrastructure.

[svillar, 2015-10-08 09:34:07]

svillar@igalia.com changed reviewers:
+ esprehn@chromium.org

[svillar, 2015-10-08 09:34:08]

Could you guys review the last patch which is a totally different approach?

[Manuel Rego, 2015-10-09 14:38:35]

Just a couple of inline comments.


Also, I've been testing this example:
http://jsbin.com/kusore/1/edit?html,css,output

And it seems it's not working as expected yet
(unless I made some mistake in the rebase).

https://codereview.chromium.org/1317643005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1317643005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:507: if (hasLineIfEmpty())
{
Do we need this if "direction == ForColumns"?

Otherwise, we could move it inside the previous if.

https://codereview.chromium.org/1317643005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:567: freeSpace -=
increment;
We're changing this regarding the previous code
that was subtracting "baseSize".

"baseSize" was already subtracted from the "freeSpace"
so probably this version is right.
However it seems not directly related with the rest of the patch.

[jfernandez, 2015-10-19 10:46:22]

Some additional comments on this new version of the patch.

https://codereview.chromium.org/1317643005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1317643005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:334: void
LayoutGrid::updateLogicalHeightDependentGridTracks(const LayoutUnit&
trackBasedLogicalHeight, GridSizingData& sizingData, LayoutUnit&
availableSpaceForRows)
Which are those dependent tracks ? This function will recompute logical height
of all row tracks, even those with a fixed size. Is that what we want ? 

If we really want to update all row tracks, shouldn't we change the function
name accordingly ?

https://codereview.chromium.org/1317643005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:426: if
(logicalHeightLength.isMaxContent())
Current LayoutBox: logic doesn't implement the new min/max-content logic and it
has a FIXME stating the following:

"// FIXME(cbiesinger): The css-sizing spec is considering changing what
min-content/max-content should resolve to.
// If that happens, this code will have to change."

Are we sure we want to do this for Grid ? 

If we want to add this logic to grid, shouldn't we consider move/refactor some
code to LayoutBox ?

https://codereview.chromium.org/1317643005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:567: freeSpace -=
increment;
Wow, that changes seems like a bug fix, but why we haven't detected it so far ?

[svillar, 2015-10-19 12:14:20]

Thanks for both reviews. I'm uploading a new patch soon, which is an improved
version of this one.

https://codereview.chromium.org/1317643005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1317643005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:334: void
LayoutGrid::updateLogicalHeightDependentGridTracks(const LayoutUnit&
trackBasedLogicalHeight, GridSizingData& sizingData, LayoutUnit&
availableSpaceForRows)
On 2015/10/19 10:46:22, jfernandez wrote:
> Which are those dependent tracks ? This function will recompute logical height
> of all row tracks, even those with a fixed size. Is that what we want ? 
> 
> If we really want to update all row tracks, shouldn't we change the function
> name accordingly ? 

We need to rerun the algorithm, and it'll indeed update all row tracks, you
cannot just update a subset of them (updating fixed size tracks is ultra-cheap
BTW). Also it does not happen always as you can see, actually only in cases
where you have indefinite sizes with min|max-content restrictions as far as I
have observed.

https://codereview.chromium.org/1317643005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:426: if
(logicalHeightLength.isMaxContent())
On 2015/10/19 10:46:22, jfernandez wrote:
> Current LayoutBox: logic doesn't implement the new min/max-content logic and
it
> has a FIXME stating the following:

Yes it does implement the min/max-content, it's just they behave the same for
normal blocks.

> "// FIXME(cbiesinger): The css-sizing spec is considering changing what
> min-content/max-content should resolve to.
> // If that happens, this code will have to change."
> 
> Are we sure we want to do this for Grid ? 
> 
> If we want to add this logic to grid, shouldn't we consider move/refactor some
> code to LayoutBox ?

No because the behavior, as I said is totally different for grid. For "normal"
blocks min-content=max-content=fit-content for heights.

That's the reason why I need to add a custom behavior for grids.

https://codereview.chromium.org/1317643005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:507: if (hasLineIfEmpty())
{
On 2015/10/09 14:38:35, Manuel Rego wrote:
> Do we need this if "direction == ForColumns"?
> 
> Otherwise, we could move it inside the previous if.

Will do that.

https://codereview.chromium.org/1317643005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:567: freeSpace -=
increment;
On 2015/10/19 10:46:22, jfernandez wrote:
> Wow, that changes seems like a bug fix, but why we haven't detected it so far
?

Because we need very specific tests for that, as the remaining freeSpace will
only be used by the alignment logic, it won't be used by the track sizing
algorithm anymore.

[svillar, 2015-10-26 15:40:45]

@cbiesinger, @esprehn could you please take a look? The code changed quite a lot
since the first iterations.

[cbiesinger, 2015-10-26 22:22:35]

lgtm

https://codereview.chromium.org/1317643005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutGrid.h (right):

https://codereview.chromium.org/1317643005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGrid.h:100: LayoutUnit
computeIntrinsicLogicalContentHeightUsing(const Length& logicalHeightLength,
LayoutUnit intrinsicContentHeight, LayoutUnit borderAndPadding) const override;
override -> final

[esprehn, 2015-10-27 05:16:18]

On 2015/10/26 at 15:40:45, svillar wrote:
> @cbiesinger, @esprehn could you please take a look? The code changed quite a
lot since the first iterations.

Sure, I'll review this tomorrow.

[svillar, 2015-10-29 09:58:18]

On 2015/10/27 05:16:18, esprehn wrote:
> On 2015/10/26 at 15:40:45, svillar wrote:
> > @cbiesinger, @esprehn could you please take a look? The code changed quite a
> lot since the first iterations.
> 
> Sure, I'll review this tomorrow.

Are you finally reviewing it Elliot?

[esprehn, 2015-10-29 10:50:23]

Yes I will, sorry for the delay!

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[svillar, 2015-10-29 15:15:45]

On 29/10/15 11:50, Elliott Sprehn wrote:
> Yes I will, sorry for the delay!
> 

Awesome! many thanks for taking care

BR

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[svillar, 2015-11-02 10:03:56]

The minimum size for contenteditable grids should not be part of the intrinsic
size. Also it should be applied to any kind of grid (not only the ones with
intrinsic heights).

[svillar, 2015-11-10 08:42:46]

The CQ bit was checked by svillar@igalia.com

[svillar, 2015-11-10 08:42:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
jfernandez@igalia.com, cbiesinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1317643005/#ps160001
(title: "Patch for landing. Removed unused params")

[commit-bot: I haz the power, 2015-11-10 08:43:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1317643005/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1317643005/160001

[commit-bot: I haz the power, 2015-11-10 10:50:03]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-11-10 10:51:09]

Patchset 9 (id:??) landed as
https://crrev.com/889d2498a67a461393da302bf244bc35d507e268
Cr-Commit-Position: refs/heads/master@{#358816}