[css-grid] Fix definite/indefinite size detection on block axis

[css-grid] Fix definite/indefinite size detection on block axis

LayoutBox::hasDefiniteLogicalHeight() was wrong as
it was not taking into account several cases
where the height is definite/indefinite.
E.g. it was not checking the special situations for flex items,
that despite of having height "auto" can definite sometimes.

As the method was buggy, this patch is directly removing it
and using LayoutBox::percentageLogicalHeightIsResolvable() instead.

Add new test to check that the flexbox case works as expected.

BUG=617876
TEST=fast/css-grid-layout/flex-item-grid-container-percentage-rows.html

Committed: https://crrev.com/f8393437ca2c05d3334011444260adac7e82530c
Cr-Commit-Position: refs/heads/master@{#398680}

[Manuel Rego, 2016-06-06 11:08:48]

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

[jfernandez, 2016-06-06 14:07:52]

non-owner LGTM.

[cbiesinger1, 2016-06-06 16:46:02]

cbiesinger@google.com changed reviewers:
+ cbiesinger@google.com

[cbiesinger1, 2016-06-06 16:46:02]

The necessity for this function makes me sad; it duplicates some logic from
computePercentageLogicalHeight :( But I don't know how to avoid it for the grid
case. See comment below though.

https://codereview.chromium.org/2037383002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2037383002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:4295: return false;
This is not correct, if a grid is a flex item it can have a definite height even
with height:auto. You'll have to do something like this:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...

And compare to -1

[cbiesinger, 2016-06-06 16:48:00]

Description was changed from

==========
[css-grid] Fix definite/indefinite size detection for abspos elements

We were considering that any abspos element has a definite size,
and that's not true. On the block axis that's only true
if the offset properties (top and bottom) are non-auto.
Fixed LayoutBox::hasDefiniteLogicalHeight() to check this properly.

BUG=538513
TEST=fast/css-grid-layout/positioned-grid-container-percentage-tracks.html
==========

to

==========
[css-grid] Fix definite/indefinite size detection for abspos elements

We were considering that any abspos element has a definite size,
and that's not true. On the block axis that's only true
if the offset properties (top and bottom) are non-auto.
Fixed LayoutBox::hasDefiniteLogicalHeight() to check this properly.

BUG=538513
TEST=fast/css-grid-layout/positioned-grid-container-percentage-tracks.html
==========

[cbiesinger, 2016-06-06 16:48:01]

cbiesinger@chromium.org changed reviewers:
- cbiesinger@google.com

[Manuel Rego, 2016-06-07 08:31:30]

Description was changed from

==========
[css-grid] Fix definite/indefinite size detection for abspos elements

We were considering that any abspos element has a definite size,
and that's not true. On the block axis that's only true
if the offset properties (top and bottom) are non-auto.
Fixed LayoutBox::hasDefiniteLogicalHeight() to check this properly.

BUG=538513
TEST=fast/css-grid-layout/positioned-grid-container-percentage-tracks.html
==========

to

==========
[css-grid] Fix definite/indefinite size detection

LayoutBox::hasDefiniteLogicalHeight() was wrong as
it was not taking into account several cases
where the height is definite/indefinite:
1) It considers that any abspos element has a definite size,
   this is only true if the element has a fixed height or
   the offset properties (top and bottom) are non-auto.
2) It was not checking the special situations for flex items,
   that despite of having height "auto" can definite sometimes.

As the method was buggy, this patch is directly removing it
and using LayoutBox::percentageLogicalHeightIsResolvable() instead.
It also introduces a small fix on percentageLogicalHeightIsResolvable()
for the first case described above (1).

Add new tests to check that both cases work as expected.

BUG=538513,617876
TEST=fast/css-grid-layout/positioned-grid-container-percentage-tracks.html
TEST=fast/css-grid-layout/flex-item-grid-container-percentage-rows.html
==========

[Manuel Rego, 2016-06-07 09:05:56]

On 2016/06/06 16:46:02, cbiesinger1 wrote:
> The necessity for this function makes me sad; it duplicates some logic from
> computePercentageLogicalHeight :( But I don't know how to avoid it for the
grid
> case. See comment below though.

Yeah these 2 functions hasDefiniteLogicalWidth() and hasDefiniteLogicalHeight()
are really a pain and I believe both are wrong.

We're getting rid of hasDefiniteLogicalWidth() in
https://codereview.chromium.org/2033033002/

And the new version of this patch is getting rid of hasDefiniteLogicalHeight()
and using percentageLogicalHeightIsResolvable() instead.
I've updated the description accordingly too.

https://codereview.chromium.org/2037383002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2037383002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:4295: return false;
On 2016/06/06 16:46:02, cbiesinger1 wrote:
> This is not correct, if a grid is a flex item it can have a definite height
even
> with height:auto. You'll have to do something like this:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
> 
> And compare to -1

Yes, this is wrong for those cases.
But it was already failing before this patch,
I've reported the bug #617876 to track this.

[cbiesinger, 2016-06-07 18:03:13]

On 2016/06/07 09:05:56, Manuel Rego wrote:
> Yes, this is wrong for those cases.
> But it was already failing before this patch,
> I've reported the bug #617876 to track this.

True. Thanks for filing the bug. lgtm.

[cbiesinger, 2016-06-07 18:05:56]

https://codereview.chromium.org/2037383002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/2037383002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:237: if
(!firstChildBox() || firstChildBox()->percentageLogicalHeightIsResolvable() ||
isLayoutView()) {
Actually, wait, I don't think this is equivalent? Doesn't this try to check if
the current height is non-auto (basically), but the new code does something
different?

[cbiesinger, 2016-06-07 18:08:58]

On 2016/06/07 18:05:56, cbiesinger wrote:
>
https://codereview.chromium.org/2037383002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):
> 
>
https://codereview.chromium.org/2037383002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:237: if
> (!firstChildBox() || firstChildBox()->percentageLogicalHeightIsResolvable() ||
> isLayoutView()) {
> Actually, wait, I don't think this is equivalent? Doesn't this try to check if
> the current height is non-auto (basically), but the new code does something
> different?

Oh... to see if the box has a definite height, you check if a (any) child's
percentage can be resolved. Smart. But what if the first child is an abspos box?
Isn't that wrong then, because those heights get resolved against the containing
block, not necessarily the parent?

[Manuel Rego, 2016-06-07 20:13:01]

On 2016/06/07 18:08:58, cbiesinger wrote:
> On 2016/06/07 18:05:56, cbiesinger wrote:
> >
>
https://codereview.chromium.org/2037383002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2037383002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:237: if
> > (!firstChildBox() || firstChildBox()->percentageLogicalHeightIsResolvable()
||
> > isLayoutView()) {
> > Actually, wait, I don't think this is equivalent? Doesn't this try to check
if
> > the current height is non-auto (basically), but the new code does something
> > different?
> 
> Oh... to see if the box has a definite height, you check if a (any) child's
> percentage can be resolved. Smart. But what if the first child is an abspos
box?
> Isn't that wrong then, because those heights get resolved against the
containing
> block, not necessarily the parent?

Yes you're right, this won't work for that case.
I've uploaded a new version using firstInFlowChildBox() instead of
firstChildBox().

[cbiesinger, 2016-06-07 20:21:12]

lgtm with some nits.

I don't think this part of the description is still correct:
> It also introduces a small fix on percentageLogicalHeightIsResolvable()
> for the first case described above (1).

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

https://codereview.chromium.org/2037383002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:237: if
(!firstInFlowChildBox() ||
firstInFlowChildBox()->percentageLogicalHeightIsResolvable() || isLayoutView())
{
Can you add a comment here explaining the firstInFlowChildBox() logic, so it's
clearer to future readers? Same in LayoutGrid

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

https://codereview.chromium.org/2037383002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2734: } else if
(hasOverrideContainingBlockLogicalHeight() &&
!isOutOfFlowPositionedWithSpecifiedHeight) {
BTW, why is this change necessary? Seems like an unrelated fix?

[Manuel Rego, 2016-06-08 12:10:25]

Thanks for the review.

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

https://codereview.chromium.org/2037383002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:237: if
(!firstInFlowChildBox() ||
firstInFlowChildBox()->percentageLogicalHeightIsResolvable() || isLayoutView())
{
On 2016/06/07 20:21:12, cbiesinger wrote:
> Can you add a comment here explaining the firstInFlowChildBox() logic, so it's
> clearer to future readers? Same in LayoutGrid

Yeah, I'll add a comment.

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

https://codereview.chromium.org/2037383002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2734: } else if
(hasOverrideContainingBlockLogicalHeight() &&
!isOutOfFlowPositionedWithSpecifiedHeight) {
On 2016/06/07 20:21:12, cbiesinger wrote:
> BTW, why is this change necessary? Seems like an unrelated fix?

It's actually fixing bug #538513.

That's why the description says:
> 1) It considers that any abspos element has a definite size,
>    this is only true if the element has a fixed height or
>    the offset properties (top and bottom) are non-auto.
> [...]
> It also introduces a small fix on percentageLogicalHeightIsResolvable()
> for the first case described above (1).

Anyway now that this patch has evolved so much,
probably I can move this to a separated CL.
So this would only fix bug #617876 and get rid of 
LayoutBox::hasDefiniteLogicalHeight().

[cbiesinger, 2016-06-08 15:51:01]

On 2016/06/08 12:10:25, Manuel Rego wrote:
>
https://codereview.chromium.org/2037383002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):
> 
>
https://codereview.chromium.org/2037383002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutBox.cpp:2734: } else if
> (hasOverrideContainingBlockLogicalHeight() &&
> !isOutOfFlowPositionedWithSpecifiedHeight) {
> On 2016/06/07 20:21:12, cbiesinger wrote:
> > BTW, why is this change necessary? Seems like an unrelated fix?
> 
> It's actually fixing bug #538513.
> 
> That's why the description says:
> > 1) It considers that any abspos element has a definite size,
> >    this is only true if the element has a fixed height or
> >    the offset properties (top and bottom) are non-auto.
> > [...]
> > It also introduces a small fix on percentageLogicalHeightIsResolvable()
> > for the first case described above (1).
> 
> Anyway now that this patch has evolved so much,
> probably I can move this to a separated CL.
> So this would only fix bug #617876 and get rid of 
> LayoutBox::hasDefiniteLogicalHeight().

oh I see -- I was confused because that's in computePercentageLogicalHeight, not
in percentageLogicalHeightIsResolvable. Makes sense,  but a different CL seems
better. Thanks!

[Manuel Rego, 2016-06-08 19:59:54]

Description was changed from

==========
[css-grid] Fix definite/indefinite size detection

LayoutBox::hasDefiniteLogicalHeight() was wrong as
it was not taking into account several cases
where the height is definite/indefinite:
1) It considers that any abspos element has a definite size,
   this is only true if the element has a fixed height or
   the offset properties (top and bottom) are non-auto.
2) It was not checking the special situations for flex items,
   that despite of having height "auto" can definite sometimes.

As the method was buggy, this patch is directly removing it
and using LayoutBox::percentageLogicalHeightIsResolvable() instead.
It also introduces a small fix on percentageLogicalHeightIsResolvable()
for the first case described above (1).

Add new tests to check that both cases work as expected.

BUG=538513,617876
TEST=fast/css-grid-layout/positioned-grid-container-percentage-tracks.html
TEST=fast/css-grid-layout/flex-item-grid-container-percentage-rows.html
==========

to

==========
[css-grid] Fix definite/indefinite size detection on block axis

LayoutBox::hasDefiniteLogicalHeight() was wrong as
it was not taking into account several cases
where the height is definite/indefinite.
E.g. it was not checking the special situations for flex items,
that despite of having height "auto" can definite sometimes.

As the method was buggy, this patch is directly removing it
and using LayoutBox::percentageLogicalHeightIsResolvable() instead.

Add new test to check that the flexbox case works as expected.

BUG=617876
TEST=fast/css-grid-layout/flex-item-grid-container-percentage-rows.html
==========

[Manuel Rego, 2016-06-08 20:02:48]

On 2016/06/08 15:51:01, cbiesinger wrote:
> On 2016/06/08 12:10:25, Manuel Rego wrote:
> >
>
https://codereview.chromium.org/2037383002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2037383002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/LayoutBox.cpp:2734: } else if
> > (hasOverrideContainingBlockLogicalHeight() &&
> > !isOutOfFlowPositionedWithSpecifiedHeight) {
> > On 2016/06/07 20:21:12, cbiesinger wrote:
> > > BTW, why is this change necessary? Seems like an unrelated fix?
> > 
> > It's actually fixing bug #538513.
> > 
> > That's why the description says:
> > > 1) It considers that any abspos element has a definite size,
> > >    this is only true if the element has a fixed height or
> > >    the offset properties (top and bottom) are non-auto.
> > > [...]
> > > It also introduces a small fix on percentageLogicalHeightIsResolvable()
> > > for the first case described above (1).
> > 
> > Anyway now that this patch has evolved so much,
> > probably I can move this to a separated CL.
> > So this would only fix bug #617876 and get rid of 
> > LayoutBox::hasDefiniteLogicalHeight().
> 
> oh I see -- I was confused because that's in computePercentageLogicalHeight,
not
> in percentageLogicalHeightIsResolvable. Makes sense,  but a different CL seems
> better. Thanks!

Ups yeah, I didn't realize that the description was talking about the wrong
method,
my fault. :-(

Anyway I'm moving that part to a different CL.

[Manuel Rego, 2016-06-08 20:16:32]

The CQ bit was checked by rego@igalia.com

[Manuel Rego, 2016-06-08 20:16:32]

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/2037383002/#ps60001
(title: "Patch for landing")

[commit-bot: I haz the power, 2016-06-08 20:17:05]

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

[commit-bot: I haz the power, 2016-06-08 21:25:36]

Description was changed from

==========
[css-grid] Fix definite/indefinite size detection on block axis

LayoutBox::hasDefiniteLogicalHeight() was wrong as
it was not taking into account several cases
where the height is definite/indefinite.
E.g. it was not checking the special situations for flex items,
that despite of having height "auto" can definite sometimes.

As the method was buggy, this patch is directly removing it
and using LayoutBox::percentageLogicalHeightIsResolvable() instead.

Add new test to check that the flexbox case works as expected.

BUG=617876
TEST=fast/css-grid-layout/flex-item-grid-container-percentage-rows.html
==========

to

==========
[css-grid] Fix definite/indefinite size detection on block axis

LayoutBox::hasDefiniteLogicalHeight() was wrong as
it was not taking into account several cases
where the height is definite/indefinite.
E.g. it was not checking the special situations for flex items,
that despite of having height "auto" can definite sometimes.

As the method was buggy, this patch is directly removing it
and using LayoutBox::percentageLogicalHeightIsResolvable() instead.

Add new test to check that the flexbox case works as expected.

BUG=617876
TEST=fast/css-grid-layout/flex-item-grid-container-percentage-rows.html
==========

[commit-bot: I haz the power, 2016-06-08 21:25:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-08 21:27:59]

Description was changed from

==========
[css-grid] Fix definite/indefinite size detection on block axis

LayoutBox::hasDefiniteLogicalHeight() was wrong as
it was not taking into account several cases
where the height is definite/indefinite.
E.g. it was not checking the special situations for flex items,
that despite of having height "auto" can definite sometimes.

As the method was buggy, this patch is directly removing it
and using LayoutBox::percentageLogicalHeightIsResolvable() instead.

Add new test to check that the flexbox case works as expected.

BUG=617876
TEST=fast/css-grid-layout/flex-item-grid-container-percentage-rows.html
==========

to

==========
[css-grid] Fix definite/indefinite size detection on block axis

LayoutBox::hasDefiniteLogicalHeight() was wrong as
it was not taking into account several cases
where the height is definite/indefinite.
E.g. it was not checking the special situations for flex items,
that despite of having height "auto" can definite sometimes.

As the method was buggy, this patch is directly removing it
and using LayoutBox::percentageLogicalHeightIsResolvable() instead.

Add new test to check that the flexbox case works as expected.

BUG=617876
TEST=fast/css-grid-layout/flex-item-grid-container-percentage-rows.html

Committed: https://crrev.com/f8393437ca2c05d3334011444260adac7e82530c
Cr-Commit-Position: refs/heads/master@{#398680}
==========

[commit-bot: I haz the power, 2016-06-08 21:28:00]

Patchset 4 (id:??) landed as
https://crrev.com/f8393437ca2c05d3334011444260adac7e82530c
Cr-Commit-Position: refs/heads/master@{#398680}