LayoutBox::hasDefiniteLogicalHeight() should not consider abspos boxes as definite

LayoutBox::hasDefiniteLogicalHeight() should not consider abspos boxes as definite

An abspos box does not itself have a definite height as per the
CSS spec; it only does so with respect to resolving a child's
percentage size. This is handled correctly by computePercentageHeight.
The grid spec, however, requires checking whether the grid's size
itself is definite:
https://drafts.csswg.org/css-grid/#valdef-grid-template-columns-percentage

So, this change makes this code correct.

There is also some code simplification that I'm doing in this change.

This code was introduced in Feb 2015 in https://codereview.chromium.org/906323003

R=leviw@chromium.org,ojan@chromium.org
BUG=476840

[Manuel Rego, 2015-04-24 05:55:17]

rego@igalia.com changed reviewers:
+ rego@igalia.com

[Manuel Rego, 2015-04-24 05:55:17]

Non-owner LGTM. Thanks for the fix.

Regarding the test I think it'd be nice to also add a case for
"flex-direction: column;". Despite or not being failing right now, it'd
increase coverage for the future. As the width is also definite for 
absolutely positioned elements.
On top of that, I've noted down a few nits in the test.

BTW, I'd add a test for grid in a follow-up patch.

https://codereview.chromium.org/1081433005/diff/1/LayoutTests/css3/flexbox/pe...
File LayoutTests/css3/flexbox/percentage-height-in-abspos-expected.html (right):

https://codereview.chromium.org/1081433005/diff/1/LayoutTests/css3/flexbox/pe...
LayoutTests/css3/flexbox/percentage-height-in-abspos-expected.html:10:
background-color: grey;
Again this doesn't seem needed.

https://codereview.chromium.org/1081433005/diff/1/LayoutTests/css3/flexbox/pe...
LayoutTests/css3/flexbox/percentage-height-in-abspos-expected.html:14:
background-color: red;
Nit: in the -expected file this is not needed as you know it's never 
going to be red.

https://codereview.chromium.org/1081433005/diff/1/LayoutTests/css3/flexbox/pe...
LayoutTests/css3/flexbox/percentage-height-in-abspos-expected.html:21:
flex-direction: column;
Nit: we don't need a flexbox here

https://codereview.chromium.org/1081433005/diff/1/LayoutTests/css3/flexbox/pe...
File LayoutTests/css3/flexbox/percentage-height-in-abspos.html (right):

https://codereview.chromium.org/1081433005/diff/1/LayoutTests/css3/flexbox/pe...
LayoutTests/css3/flexbox/percentage-height-in-abspos.html:10: background-color:
grey;
Don't get what you need this.

https://codereview.chromium.org/1081433005/diff/1/LayoutTests/css3/flexbox/pe...
LayoutTests/css3/flexbox/percentage-height-in-abspos.html:38: Tests that
percentage heights get resolved correctly when the flexbox is

Nit: this is a common description for a check-layout.js test. However 
for ref-tests the description usually explains how to visually check if 
the test passes or not. Things like you shouldn't see any red, etc.

[mstensho (USE GERRIT), 2015-04-24 07:22:33]

mstensho@opera.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2015-04-24 07:22:34]

lgtm, once rego's issues are addressed.

https://codereview.chromium.org/1081433005/diff/1/Source/core/layout/LayoutBo...
File Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1081433005/diff/1/Source/core/layout/LayoutBo...
Source/core/layout/LayoutBox.cpp:4340: bool
LayoutBox::hasDefiniteLogicalHeight() const
Note (not an issue that should be handled in this CL):

This may make sense for flexbox and grid (which, after all, are the only
callers), but for regular block layout, it seems strange to consider height:auto
on position:absolute as definite, unless both the top and bottom properties are
non-auto.

Maybe it's just the method names that need to be changed (or add some
documentation).

[Manuel Rego, 2015-04-24 07:35:10]

https://codereview.chromium.org/1081433005/diff/1/Source/core/layout/LayoutBo...
File Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1081433005/diff/1/Source/core/layout/LayoutBo...
Source/core/layout/LayoutBox.cpp:4340: bool
LayoutBox::hasDefiniteLogicalHeight() const
On 2015/04/24 07:22:33, mstensho wrote:
> This may make sense for flexbox and grid (which, after all, are the only
> callers), but for regular block layout, it seems strange to consider
height:auto
> on position:absolute as definite, unless both the top and bottom properties
are
> non-auto.

This is in the spec (http://dev.w3.org/csswg/css-sizing-3/#definite):
"Additionally, the size of the containing block of an absolutely 
positioned element is always definite with respect to that element."

So, if that should work different for regular blocks, it probably needs
to be addressed on the CSS WG first.

[mstensho (USE GERRIT), 2015-04-24 07:40:03]

https://codereview.chromium.org/1081433005/diff/1/Source/core/layout/LayoutBo...
File Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1081433005/diff/1/Source/core/layout/LayoutBo...
Source/core/layout/LayoutBox.cpp:4340: bool
LayoutBox::hasDefiniteLogicalHeight() const
On 2015/04/24 07:35:10, Manuel Rego wrote:
> On 2015/04/24 07:22:33, mstensho wrote:
> > This may make sense for flexbox and grid (which, after all, are the only
> > callers), but for regular block layout, it seems strange to consider
> height:auto
> > on position:absolute as definite, unless both the top and bottom properties
> are
> > non-auto.
> 
> This is in the spec (http://dev.w3.org/csswg/css-sizing-3/#definite):
> "Additionally, the size of the containing block of an absolutely 
> positioned element is always definite with respect to that element."

That's the size of THE CONTAINING BLOCK of an absolutely positioned element. Not
the size of the absolutely positioned element itself. And even then, it's only
considered definite with respect to the abspos element.

[ojan, 2015-04-24 18:14:31]

+tabatkins for the spec question.

https://codereview.chromium.org/1081433005/diff/1/Source/core/layout/LayoutBo...
File Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1081433005/diff/1/Source/core/layout/LayoutBo...
Source/core/layout/LayoutBox.cpp:4340: bool
LayoutBox::hasDefiniteLogicalHeight() const
On 2015/04/24 at 07:40:03, mstensho wrote:
> On 2015/04/24 07:35:10, Manuel Rego wrote:
> > On 2015/04/24 07:22:33, mstensho wrote:
> > > This may make sense for flexbox and grid (which, after all, are the only
> > > callers), but for regular block layout, it seems strange to consider
> > height:auto
> > > on position:absolute as definite, unless both the top and bottom
properties
> > are
> > > non-auto.
> > 
> > This is in the spec (http://dev.w3.org/csswg/css-sizing-3/#definite):
> > "Additionally, the size of the containing block of an absolutely 
> > positioned element is always definite with respect to that element."
> 
> That's the size of THE CONTAINING BLOCK of an absolutely positioned element.
Not the size of the absolutely positioned element itself. And even then, it's
only considered definite with respect to the abspos element.

Yeah, I think the comment in the code below is confused WRT this. This is
querying on the child, not on the containing block.

As to the test case in question, it's not clear to me how to read the spec text
and it's not clear to me what behavior we want. But if we do want to treat
abspos in this way, we'd need to make the if statement below include checks as
to whether the absolutely positioned element's top/bottom are non-auto. If one
or both of them is auto, then the element sizes to it's content (i.e. does not
have a definite size).

[mstensho (USE GERRIT), 2015-05-26 20:38:42]

Can we get some progress here, please?

[cbiesinger, 2015-05-27 21:31:30]

Ojan - maybe we can get this landed for now because it solves real issues, and
separately discuss what the right thing is?

I don't believe that the way flexbox calls this function causes actual bugs. The
only other caller is grid which we're not shipping, I believe.

[ojan, 2015-05-28 03:20:42]

On 2015/05/27 at 21:31:30, cbiesinger wrote:
> Ojan - maybe we can get this landed for now because it solves real issues, and
separately discuss what the right thing is?

I'm OK with that modulo the issue below.

> I don't believe that the way flexbox calls this function causes actual bugs.
The only other caller is grid which we're not shipping, I believe.

Can you explain why this doesn't cause bugs in flexbox? If I have a
position:absolute with none of top/right/left/bottom set, isn't it auto sized?
In that case, shouldn't this function return false?

[mstensho (USE GERRIT), 2015-07-06 18:28:08]

mstensho@opera.com changed reviewers:
- mstensho@opera.com

[cbiesinger, 2015-07-29 18:31:35]

On 2015/05/28 03:20:42, ojan wrote:
> On 2015/05/27 at 21:31:30, cbiesinger wrote:
> > Ojan - maybe we can get this landed for now because it solves real issues,
and
> separately discuss what the right thing is?
> 
> I'm OK with that modulo the issue below.
> 
> > I don't believe that the way flexbox calls this function causes actual bugs.
> The only other caller is grid which we're not shipping, I believe.
> 
> Can you explain why this doesn't cause bugs in flexbox? If I have a
> position:absolute with none of top/right/left/bottom set, isn't it auto sized?
> In that case, shouldn't this function return false?

It won't cause bugs in Flexbox because we only call it on actual flex items, not
on any out-of-flow boxes.

(argh, I forgot to reply to this sooner :( )

[cbiesinger, 2015-08-04 17:44:55]

On 2015/07/29 18:31:35, cbiesinger wrote:
> On 2015/05/28 03:20:42, ojan wrote:
> > On 2015/05/27 at 21:31:30, cbiesinger wrote:
> > > Ojan - maybe we can get this landed for now because it solves real issues,
> and
> > separately discuss what the right thing is?
> > 
> > I'm OK with that modulo the issue below.
> > 
> > > I don't believe that the way flexbox calls this function causes actual
bugs.
> > The only other caller is grid which we're not shipping, I believe.
> > 
> > Can you explain why this doesn't cause bugs in flexbox? If I have a
> > position:absolute with none of top/right/left/bottom set, isn't it auto
sized?
> > In that case, shouldn't this function return false?
> 
> It won't cause bugs in Flexbox because we only call it on actual flex items,
not
> on any out-of-flow boxes.
> 
> (argh, I forgot to reply to this sooner :( )

Although maybe I should just switch to
percentageLogicalHeightIsResolvableFromBlock

[cbiesinger, 2015-08-04 23:35:31]

OK, in https://codereview.chromium.org/1258693005/ I will fix the flexbox issue
in a different way.

I will use this one to fix hasDefiniteHeight, because I agree with Ojan that it
is incorrect as it stands.

[cbiesinger, 2015-08-04 23:35:33]

The CQ bit was checked by cbiesinger@chromium.org

[cbiesinger, 2015-08-04 23:35:37]

The CQ bit was unchecked by cbiesinger@chromium.org

[cbiesinger, 2015-08-10 21:44:45]

Manuel - can you help me write a grid testcase? I don't know grid well enough to
understand what this function is used for. (The patch is otherwise updated)

Thanks!

[Manuel Rego, 2015-09-23 10:22:02]

One possible example for grid it might be:
http://jsbin.com/jaseto/1/edit?html,css,output

Right now we're detecting the height as indefinite,
so the percentage rows are treated as "auto".
That's why they don't take the right height.

Even if we fix LayoutBox::hasDefiniteLogicalHeight() and
we detect it properly as definite in this example.
Because of we're using computeContentLogicalHeight()
in LayoutGrid::computeUsedBreadthOfSpecifiedLength()
and that's not taking into account this issue.

Anyway I've some questions/doubts
about the new version of the patch.

https://codereview.chromium.org/1081433005/diff/80001/Source/core/layout/Layo...
File Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1081433005/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutBox.cpp:4270: if (length.isIntrinsicOrAuto())
I think this still has the original problem.

If the grid is abspos and has "height: auto;"
it'll identified as indefinite.
However, if top and bottom are non-auto
the height would be definite.

https://codereview.chromium.org/1081433005/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutBox.cpp:4280: if (length.isMaxSizeNone())
I don't get what the max-height has to do in order
to check if it's definite or not.
Specially if "max-height: none;" implies
that the size is definite.

[Manuel Rego, 2015-10-02 07:46:57]

On 2015/09/23 10:22:02, Manuel Rego wrote:
> One possible example for grid it might be:
> http://jsbin.com/jaseto/1/edit?html,css,output
> 
> Right now we're detecting the height as indefinite,
> so the percentage rows are treated as "auto".
> That's why they don't take the right height.
> 
> Even if we fix LayoutBox::hasDefiniteLogicalHeight() and
> we detect it properly as definite in this example.
> Because of we're using computeContentLogicalHeight()
> in LayoutGrid::computeUsedBreadthOfSpecifiedLength()
> and that's not taking into account this issue.

I've provided a different patch fixing this use case in CL:
https://codereview.chromium.org/1383003002/

[cbiesinger, 2015-10-02 16:16:43]

https://codereview.chromium.org/1081433005/diff/80001/Source/core/layout/Layo...
File Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1081433005/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutBox.cpp:4270: if (length.isIntrinsicOrAuto())
On 2015/09/23 10:22:02, Manuel Rego wrote:
> I think this still has the original problem.
> 
> If the grid is abspos and has "height: auto;"
> it'll identified as indefinite.
> However, if top and bottom are non-auto
> the height would be definite.

I don't believe that's a correct interpretation of the CSS spec. At least,
https://drafts.csswg.org/css-sizing-3/#definite does not indicate that this
would be a definite size?

[Manuel Rego, 2015-11-05 08:26:03]

On 2015/10/02 16:16:43, cbiesinger wrote:
>
https://codereview.chromium.org/1081433005/diff/80001/Source/core/layout/Layo...
> File Source/core/layout/LayoutBox.cpp (right):
> 
>
https://codereview.chromium.org/1081433005/diff/80001/Source/core/layout/Layo...
> Source/core/layout/LayoutBox.cpp:4270: if (length.isIntrinsicOrAuto())
> On 2015/09/23 10:22:02, Manuel Rego wrote:
> > I think this still has the original problem.
> > 
> > If the grid is abspos and has "height: auto;"
> > it'll identified as indefinite.
> > However, if top and bottom are non-auto
> > the height would be definite.
> 
> I don't believe that's a correct interpretation of the CSS spec. At least,
> https://drafts.csswg.org/css-sizing-3/#definite does not indicate that this
> would be a definite size?

According to Tab this should be considered a definite size:
https://lists.w3.org/Archives/Public/www-style/2015Nov/0074.html

[Manuel Rego, 2016-04-11 08:05:31]

@cbiesinger we should close this as the patch in
https://codereview.chromium.org/1383003002/
has already landed.