Treat percent-height boxes inside auto-height cells as auto

Treat percent-height div inside auto-height cells as auto

This aligns blink with the spec and gecko. We previously used the cell
height to calculate the height of the child element.

BUG=353580
Committed: https://crrev.com/8876584335b48c99cf8df552ef4d8efebb131041
Cr-Commit-Position: refs/heads/master@{#371096}

[dgrogan, 2016-01-21 00:39:40]

Description was changed from

==========
WIP first cut at handling 353580.

Some quirks mode and scrollbar tests still fail.

BUG=353580
==========

to

==========
Treat percent-height div inside auto-height cells as auto

This aligns blink with the spec and gecko. We previously used the cell height to
calculate the height of the child element.

BUG=353580
==========

[dgrogan, 2016-01-21 00:39:59]

Description was changed from

==========
Treat percent-height div inside auto-height cells as auto

This aligns blink with the spec and gecko. We previously used the cell height to
calculate the height of the child element.

BUG=353580
==========

to

==========
Treat percent-height div inside auto-height cells as auto

This aligns blink with the spec and gecko. We previously used the cell
height to calculate the height of the child element.

BUG=353580
==========

[dgrogan, 2016-01-21 01:04:06]

dgrogan@chromium.org changed reviewers:
+ mstensho@opera.com

[dgrogan, 2016-01-21 01:04:06]

Morten, could you review this?

[mstensho (USE GERRIT), 2016-01-21 20:21:33]

https://codereview.chromium.org/1591043002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/table/div-height-inside-auto-table-cell-is-auto.html
(right):

https://codereview.chromium.org/1591043002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/table/div-height-inside-auto-table-cell-is-auto.html:26:
checkLayout('#child');
I suggest that you check #parent instead (which will traverse the entire
subtree). The test results text is dumped into a sibling node of what you
specify here, so currently the text messes up the table layout. The test will
still pass and fail as it should with #child here, but it's less human-friendly
that way. :)

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

https://codereview.chromium.org/1591043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2596: // But FF doesn't
apply this logic (1) in quirks mode or (2) when "this element" is a table.
https://dev.windows.com/en-us/microsoft-edge/tools/screenshots/#http://jsfidd...

Looks like this will make us more incompatible with MSIE (and Edge?), but it's
clearly more spec-compliant to do what you're doing, so let's just go ahead.

BTW: Presto applies this logic also when "this element" is a table. Less
weirdness. Not sure if we should dare to do the same, but if we *don't* want to
do that, we should add a test with a table with percent height inside a table in
strict mode, just to have this behavior covered.

https://codereview.chromium.org/1591043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2597: // Though I can't find
documentation to support either of those.
And I don't think you ever will. Because, tables!!! :) But I think you can
remove this line (move it to the commit message, if you like).

[dgrogan, 2016-01-22 00:08:09]

https://codereview.chromium.org/1591043002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/table/div-height-inside-auto-table-cell-is-auto.html
(right):

https://codereview.chromium.org/1591043002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/table/div-height-inside-auto-table-cell-is-auto.html:26:
checkLayout('#child');
On 2016/01/21 20:21:33, mstensho wrote:
> I suggest that you check #parent instead (which will traverse the entire
> subtree). The test results text is dumped into a sibling node of what you
> specify here, so currently the text messes up the table layout. The test will
> still pass and fail as it should with #child here, but it's less
human-friendly
> that way. :)

Good call. Done.

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

https://codereview.chromium.org/1591043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2596: // But FF doesn't
apply this logic (1) in quirks mode or (2) when "this element" is a table.
On 2016/01/21 20:21:33, mstensho wrote:
>
https://dev.windows.com/en-us/microsoft-edge/tools/screenshots/#http://jsfidd...
> 
> Looks like this will make us more incompatible with MSIE (and Edge?), 

Yeah, Edge shows the red square :(

> 
> BTW: Presto applies this logic also when "this element" is a table. Less
> weirdness. Not sure if we should dare to do the same, but if we *don't* want
to
> do that, we should add a test with a table with percent height inside a table
in
> strict mode, just to have this behavior covered.

I asked Boris on the bug why FF treats nested tables differently. Maybe it's in
some corner of the spec, a bug in FF, or just historical. What do you think of
using his answer as input to decide what to do?

https://codereview.chromium.org/1591043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2597: // Though I can't find
documentation to support either of those.
On 2016/01/21 20:21:33, mstensho wrote:
> And I don't think you ever will. Because, tables!!! :) But I think you can
> remove this line (move it to the commit message, if you like).

Done.

[mstensho (USE GERRIT), 2016-01-22 10:15:37]

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

https://codereview.chromium.org/1591043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2596: // But FF doesn't
apply this logic (1) in quirks mode or (2) when "this element" is a table.
On 2016/01/22 00:08:09, dgrogan wrote:
> On 2016/01/21 20:21:33, mstensho wrote:
> >
>
https://dev.windows.com/en-us/microsoft-edge/tools/screenshots/#http://jsfidd...
> > 
> > Looks like this will make us more incompatible with MSIE (and Edge?), 
> 
> Yeah, Edge shows the red square :(

I still think we take the chance and land this change.

> > BTW: Presto applies this logic also when "this element" is a table. Less
> > weirdness. Not sure if we should dare to do the same, but if we *don't* want
> to
> > do that, we should add a test with a table with percent height inside a
table
> in
> > strict mode, just to have this behavior covered.
> 
> I asked Boris on the bug why FF treats nested tables differently. Maybe it's
in
> some corner of the spec, a bug in FF, or just historical. What do you think of
> using his answer as input to decide what to do?

Where's his answer?

I don't think this is written in any spec, not even in dusty corners. :) This is
probably something FF did in order to be compatible with IE (or even Netscape)
on some site(s).

Regarding the strict behavior of Presto, I don't think we should make that
change right now, even if it's Right(TM). One step at a time. :) Maybe add a
TODO? I suggested that you add a test for this (table in table), but maybe
that's not necessary, since this is just some weirdness that we're not sure is
necessary.

[dgrogan, 2016-01-22 17:41:48]

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

https://codereview.chromium.org/1591043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2596: // But FF doesn't
apply this logic (1) in quirks mode or (2) when "this element" is a table.
> > > BTW: Presto applies this logic also when "this element" is a table. Less
> > > weirdness. Not sure if we should dare to do the same, but if we *don't*
want
> > to
> > > do that, we should add a test with a table with percent height inside a
> table
> > in
> > > strict mode, just to have this behavior covered.
> > 
> > I asked Boris on the bug why FF treats nested tables differently. Maybe it's
> in
> > some corner of the spec, a bug in FF, or just historical. What do you think
of
> > using his answer as input to decide what to do?
> 
> Where's his answer?

His answer for now is "don't know, will ask around"
https://code.google.com/p/chromium/issues/detail?id=353580#c10 


> I don't think this is written in any spec, not even in dusty corners. :) This
is
> probably something FF did in order to be compatible with IE (or even Netscape)
> on some site(s).
> 
> Regarding the strict behavior of Presto, I don't think we should make that
> change right now, even if it's Right(TM). One step at a time. :) Maybe add a
> TODO? I suggested that you add a test for this (table in table), but maybe
> that's not necessary, since this is just some weirdness that we're not sure is
> necessary.

TODO added. It's obliquely covered by an existing layout test
(fast/table/table-in-table-percent-width.html) but I agree that if we decide to
bake it into the platform it should have a dedicated layout test.

[mstensho (USE GERRIT), 2016-01-22 19:47:16]

lgtm to land this (unless you want to wait for more feedback from bzbarsky).

[dgrogan, 2016-01-22 21:32:07]

On 2016/01/22 19:47:16, mstensho wrote:
> lgtm to land this (unless you want to wait for more feedback from bzbarsky).

I'll handle the table issue, if necessary, in a separate patch. Thanks for the
review.

[dgrogan, 2016-01-22 21:32:22]

The CQ bit was checked by dgrogan@chromium.org

[commit-bot: I haz the power, 2016-01-22 21:33:25]

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

[commit-bot: I haz the power, 2016-01-23 00:15:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-23 00:16:49]

Description was changed from

==========
Treat percent-height div inside auto-height cells as auto

This aligns blink with the spec and gecko. We previously used the cell
height to calculate the height of the child element.

BUG=353580
==========

to

==========
Treat percent-height div inside auto-height cells as auto

This aligns blink with the spec and gecko. We previously used the cell
height to calculate the height of the child element.

BUG=353580

Committed: https://crrev.com/8876584335b48c99cf8df552ef4d8efebb131041
Cr-Commit-Position: refs/heads/master@{#371096}
==========

[commit-bot: I haz the power, 2016-01-23 00:16:49]

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