[css tables] Don't pass table width increase due to % columns to parents

[css tables] Don't pass table width increase due to % columns to parents

A table cell that specifies % width can increase the width of its table,
but shouldn't increase the width of any grandparent table cell.

This makes Blink match FF and Edge.

LayoutTable::layout calls LayoutTable::updateLogicalWidth, which
eventually calls into
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths, which used to
do the percent scaling. This doesn't work because when the table is
inside a table-cell, LayoutTableCell calls the child table's
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths, returning the
scaled width, not the intrinsic width.

This patch moves the scaling to a separate step that's only called in
LayoutTable::updateLogicalWidth, not in
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths.

BUG=178369, 427994
Committed: https://crrev.com/9428cfb16993a2329e87c65da096ca295132ef0f
Cr-Commit-Position: refs/heads/master@{#394715}

[dgrogan, 2016-04-27 02:33:19]

Description was changed from

==========
[css tables] Don't pass table width increase due to % columns to parents

A table cell that specifies % width can increase the width of its table,
but only if the table's parent doesn't shrink-wrap.

This makes us match FF and Edge.

BUG=178369,427994
==========

to

==========
[css tables] Don't pass table width increase due to % columns to parents

A table cell that specifies % width can increase the width of its table,
but shouldn't increase the width of any grandparent table cell.

This makes us match FF and Edge.

LayoutTable::layout calls LayoutTable::updateLogicalWidth, which eventually
calls into TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths, which used
to do the percent scaling. This doesn't work because when the table is inside a
table-cell, LayoutTableCell calls the child table's
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths, returning the scaled
width, not the intrinsic width.

This patch moves the scaling to a separate step that's only called in 
LayoutTable::updateLogicalWidth, not in
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths.

BUG=178369,427994
==========

[dgrogan, 2016-04-27 02:38:54]

Description was changed from

==========
[css tables] Don't pass table width increase due to % columns to parents

A table cell that specifies % width can increase the width of its table,
but shouldn't increase the width of any grandparent table cell.

This makes us match FF and Edge.

LayoutTable::layout calls LayoutTable::updateLogicalWidth, which eventually
calls into TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths, which used
to do the percent scaling. This doesn't work because when the table is inside a
table-cell, LayoutTableCell calls the child table's
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths, returning the scaled
width, not the intrinsic width.

This patch moves the scaling to a separate step that's only called in 
LayoutTable::updateLogicalWidth, not in
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths.

BUG=178369,427994
==========

to

==========
[css tables] Don't pass table width increase due to % columns to parents

A table cell that specifies % width can increase the width of its table,
but shouldn't increase the width of any grandparent table cell.

This makes Blink match FF and Edge.

LayoutTable::layout calls LayoutTable::updateLogicalWidth,
which eventually calls into
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths,
which used to do the percent scaling. This doesn't work
because when the table is inside a table-cell,
LayoutTableCell calls the child table's
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths,
returning the scaled width, not the intrinsic width.

This patch moves the scaling to a separate step that's only
called in LayoutTable::updateLogicalWidth, not in
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths.

BUG=178369,427994
==========

[dgrogan, 2016-04-27 03:02:27]

Description was changed from

==========
[css tables] Don't pass table width increase due to % columns to parents

A table cell that specifies % width can increase the width of its table,
but shouldn't increase the width of any grandparent table cell.

This makes Blink match FF and Edge.

LayoutTable::layout calls LayoutTable::updateLogicalWidth,
which eventually calls into
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths,
which used to do the percent scaling. This doesn't work
because when the table is inside a table-cell,
LayoutTableCell calls the child table's
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths,
returning the scaled width, not the intrinsic width.

This patch moves the scaling to a separate step that's only
called in LayoutTable::updateLogicalWidth, not in
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths.

BUG=178369,427994
==========

to

==========
[css tables] Don't pass table width increase due to % columns to parents

A table cell that specifies % width can increase the width of its table,
but shouldn't increase the width of any grandparent table cell.

This makes Blink match FF and Edge.

LayoutTable::layout calls LayoutTable::updateLogicalWidth, which
eventually calls into
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths, which used to
do the percent scaling. This doesn't work because when the table is
inside a table-cell, LayoutTableCell calls the child table's
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths, returning the
scaled width, not the intrinsic width.

This patch moves the scaling to a separate step that's only called in
LayoutTable::updateLogicalWidth, not in
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths.

BUG=178369,427994
==========

[dgrogan, 2016-04-27 03:14:04]

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

[dgrogan, 2016-04-27 03:14:05]

Morten, could you take a look at this? As mentioned in the rietveld comment, it
still regresses one layout test but I'd love to get your feedback while I look
further into it.

https://codereview.chromium.org/1921973005/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/table/max-width-integer-overflow.html
(left):

https://codereview.chromium.org/1921973005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/table/max-width-integer-overflow.html:1:
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN">
This was testing the earliest ancestor of TableLayoutAlgorithm::tableMaxWidth,
but even when I removed those checks this test didn't fail. It also shows a lot
of red after my change, but so do Edge and FF.

https://codereview.chromium.org/1921973005/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/table/table-size-integer-overflow.html
(right):

https://codereview.chromium.org/1921973005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/table/table-size-integer-overflow.html:1:
<div style="float: left;">
The float makes a difference here but I am not yet sure why.

https://codereview.chromium.org/1921973005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/table/table-size-integer-overflow.html:11:
<div style="clear: left;">Edge 25.10586.0.0 shows the green box as the full
width of the page. So did Blink before Chrome 52. FireFox 48 agrees with Chrome
52.</div>
I am a little worried that this is a bellwether for breaking mobile webview
content, especially if it makes us diverge from webkit, which I haven't tested.

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

https://codereview.chromium.org/1921973005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:299: LayoutUnit
scaledWidth = m_tableLayout->scaledWidthFromPercentColumns() +
bordersPaddingAndSpacingInRowDirection();
fast/table/border-collapsing/cached-change-row-border-width.html, which changes
border width from JS, fails with this patch. I suspect it's because this patch
doesn't use the new borders properly but I haven't dug in yet.

https://codereview.chromium.org/1921973005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp (left):

https://codereview.chromium.org/1921973005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp:228: minWidth
+= m_layoutStruct[i].effectiveMinLogicalWidth;
Your excerpt was instrumental in my understanding of what this loop was doing.
So, thank you again :)

[mstensho (USE GERRIT), 2016-04-27 12:19:48]

I have problems applying this patch locally, because it deletes PNG (i.e.
binary) files. Do you have a trick to get around that? If not, maybe you could
add the broken tests to TestExpectations instead of deleting them?

I suspect that the problem with the code changes is that we now limit preferred
width propagation regardless of the ancestry. It seems that we should only apply
the limitation if we're inside another *table* with auto-layout. Table
weirdness!

Sorry. this wasn't in the "spec" excerpt that I sent you (and I had forgotten
all about it), but it was something that I realized later on, after the big
table cleanup in 2007, and it seems that I didn't bother to update the "spec".
Here's an "inspirational" comment from the Presto code:

/* Pay attention to percent-based columns. These are special in the sense that
they don't
   affect maximum width propagation if there is an ancestor table (with
   table-layout:auto). Such are the ways of automatic table layout, and
surprisingly enough,
   all major browser engines have reached a fairly agreeing behavior here.

   Even if percent-based columns should be ignored for maximum width
propagation, they will
   still affect the actual width calculation of the table, i.e. if the
containing block is wide
   enough, then the true table maximum width (maximum_width) will be honored -
it's just that
   it won't be propagated to ancestors. */

https://codereview.chromium.org/1921973005/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/table/table-size-integer-overflow.html
(right):

https://codereview.chromium.org/1921973005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/table/table-size-integer-overflow.html:1:
<div style="float: left;">
On 2016/04/27 03:14:04, dgrogan wrote:
> The float makes a difference here but I am not yet sure why.

An auto-width float is shrink-to-fit, which might affect percentage resolution
inside the table, maybe? But it seems wrong that your change affects the
behavior here. I think it should be limited to nested tables, and not affect how
we lay out a table inside a shrink-to-fit container.

https://codereview.chromium.org/1921973005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/table/table-size-integer-overflow.html:11:
<div style="clear: left;">Edge 25.10586.0.0 shows the green box as the full
width of the page. So did Blink before Chrome 52. FireFox 48 agrees with Chrome
52.</div>
On 2016/04/27 03:14:04, dgrogan wrote:
> I am a little worried that this is a bellwether for breaking mobile webview
> content, especially if it makes us diverge from webkit, which I haven't
tested.

Diverging from Edge for table layout certainly doesn't sound too good, no.

I wonder if this one fails now, too:

<!DOCTYPE html>
<p>There should be a blue square below.</p>
<div style="float:left;">
    <table cellspacing="0" cellpadding="0" style="background:blue;">
        <td style="width:20%;">
            <div style="width:40px; height:200px;"></div>
        </td>
        <td></td>
    </table>
</div>

I have problems applying your patch, due to changes in binary files, so I
haven't tested.

https://codereview.chromium.org/1921973005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/TableLayoutAlgorithm.h (right):

https://codereview.chromium.org/1921973005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/TableLayoutAlgorithm.h:46: virtual
LayoutUnit scaledWidthFromPercentColumns() = 0;
I don't think it's strictly necessary to make this one pure virtual. Having it
return LayoutUnit() instead would eliminate the need for modifying the fixed
table layout implementation.

[dgrogan, 2016-04-27 18:10:17]

Thanks for the feedback. I don't have a trick for getting around the png patch
issue. patchset 4 uses TestExpectations like you suggested and it patches in
fine. That's the only difference in patchset 4.

[dgrogan, 2016-04-27 20:24:06]

https://codereview.chromium.org/1921973005/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/table/table-size-integer-overflow.html
(right):

https://codereview.chromium.org/1921973005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/table/table-size-integer-overflow.html:1:
<div style="float: left;">
On 2016/04/27 12:19:48, mstensho wrote:
> On 2016/04/27 03:14:04, dgrogan wrote:
> > The float makes a difference here but I am not yet sure why.
> 
> An auto-width float is shrink-to-fit, which might affect percentage resolution
> inside the table, maybe? But it seems wrong that your change affects the
> behavior here. I think it should be limited to nested tables, and not affect
how
> we lay out a table inside a shrink-to-fit container.

Yeah, if that matches blink's current behavior and Edge, then I agree we should
stick to it.

https://codereview.chromium.org/1921973005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/table/table-size-integer-overflow.html:11:
<div style="clear: left;">Edge 25.10586.0.0 shows the green box as the full
width of the page. So did Blink before Chrome 52. FireFox 48 agrees with Chrome
52.</div>
On 2016/04/27 12:19:48, mstensho wrote:
> On 2016/04/27 03:14:04, dgrogan wrote:
> > I am a little worried that this is a bellwether for breaking mobile webview
> > content, especially if it makes us diverge from webkit, which I haven't
> tested.
> 
> Diverging from Edge for table layout certainly doesn't sound too good, no.
> 
> I wonder if this one fails now, too:
> 
> <!DOCTYPE html>
> <p>There should be a blue square below.</p>
> <div style="float:left;">
>     <table cellspacing="0" cellpadding="0" style="background:blue;">
>         <td style="width:20%;">
>             <div style="width:40px; height:200px;"></div>
>         </td>
>         <td></td>
>     </table>
> </div>
> 
> I have problems applying your patch, due to changes in binary files, so I
> haven't tested.

Yeah, this one we also change behavior from Edge to FF.

https://codereview.chromium.org/1921973005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/TableLayoutAlgorithm.h (right):

https://codereview.chromium.org/1921973005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/TableLayoutAlgorithm.h:46: virtual
LayoutUnit scaledWidthFromPercentColumns() = 0;
On 2016/04/27 12:19:48, mstensho wrote:
> I don't think it's strictly necessary to make this one pure virtual. Having it
> return LayoutUnit() instead would eliminate the need for modifying the fixed
> table layout implementation.

Good call.

[dgrogan, 2016-04-28 01:04:32]

My comments here are as much notes to me as anything else. But take a look if
you'd like; I always like your feedback.

https://codereview.chromium.org/1921973005/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/table/inner-percent-width-affects-outer-floated-div.html
(right):

https://codereview.chromium.org/1921973005/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/table/inner-percent-width-affects-outer-floated-div.html:2:
<p>There should be a blue square below. Inner table's percent-driven width
This is just a little different than your example from yesterday: the background
is on the floated div instead of the table.

https://codereview.chromium.org/1921973005/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/table/inner-percent-width-doesnt-affect-ancestor-columns.html
(right):

https://codereview.chromium.org/1921973005/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/table/inner-percent-width-doesnt-affect-ancestor-columns.html:1:
<!DOCTYPE html>
My first-cut naive approach causes us to still fail this test even though FF and
IE both pass.

https://codereview.chromium.org/1921973005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp (right):

https://codereview.chromium.org/1921973005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp:182: return
!table->containingBlock()->isTableCell();
This is the naive first cut. It doesn't even implement everything you mentioned:
"they don't affect maximum width propagation if there is an _ancestor table
(with table-layout:auto)_."

My goal here is to match Edge/FF where they agree and match Blink's old behavior
when Edge/FF disagree. I probably won't get it exactly right when this patch
lands but hopefully I can iterate. I suspect that the sparsely-tested logic in
shouldScaleColumnsForSelf is similar to what this will look like.

https://codereview.chromium.org/1921973005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp:233:
LayoutUnit TableLayoutAlgorithmAuto::scaledWidthFromPercentColumns()
Ignore that this is a separate function for now. I will return the logic to
computeIntrinsicLogicalWidths, which will cache an m_scaledWidth.
scaledWidthFromPercentColumns will then just return m_scaledWidth.

[mstensho (USE GERRIT), 2016-04-28 09:18:05]

https://codereview.chromium.org/1921973005/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/table/inner-percent-width-affects-outer-floated-div.html
(right):

https://codereview.chromium.org/1921973005/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/table/inner-percent-width-affects-outer-floated-div.html:2:
<p>There should be a blue square below. Inner table's percent-driven width
On 2016/04/28 01:04:31, dgrogan wrote:
> This is just a little different than your example from yesterday: the
background
> is on the floated div instead of the table.

Even better, I think. Then we actually test the result of the shrink-to-fit
operation more directly.

https://codereview.chromium.org/1921973005/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/table/inner-percent-width-doesnt-affect-ancestor-columns.html
(right):

https://codereview.chromium.org/1921973005/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/table/inner-percent-width-doesnt-affect-ancestor-columns.html:1:
<!DOCTYPE html>
On 2016/04/28 01:04:31, dgrogan wrote:
> My first-cut naive approach causes us to still fail this test even though FF
and
> IE both pass.

All thanks to the blue DIV which prevents the inner table from finding the outer
TD, I guess.

https://codereview.chromium.org/1921973005/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/table/inner-percent-width-doesnt-affect-ancestor-columns.html:2:
This should be identical to <a
href="inner-percent-width-doesnt-affect-outer-columns.html">inner-percent-width-doesnt-affect-outer-columns.html</a>
Better copy the pass condition text from there instead. :)

https://codereview.chromium.org/1921973005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp (right):

https://codereview.chromium.org/1921973005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp:182: return
!table->containingBlock()->isTableCell();
On 2016/04/28 01:04:31, dgrogan wrote:
> This is the naive first cut. It doesn't even implement everything you
mentioned:
> "they don't affect maximum width propagation if there is an _ancestor table
> (with table-layout:auto)_."
> 
> My goal here is to match Edge/FF where they agree and match Blink's old
behavior
> when Edge/FF disagree. I probably won't get it exactly right when this patch
> lands but hopefully I can iterate. I suspect that the sparsely-tested logic in
> shouldScaleColumnsForSelf is similar to what this will look like.

I see. This approach should in theory not cause any regressions, but we're
dealing with tables here, so who knows... :)

Here's a gem for you (for later, if you want to fix this in iterations):

<!DOCTYPE html>
<p>There should apparently be a square below.</p>
<div style="display:table;">
    <div id="stf" style="position:absolute; background:blue;">
        <table cellspacing="0" cellpadding="0">
            <td style="width:1%;">
                <div style="width:150px; height:150px;"></div>
            </td>
        </table>
    </div>
</div>

In Edge (and Presto) #stf becomes a square, because of the outer table, which
prevents propagation of scaled percentage-width columns from the inner table to
the #stf in the middle!

The outer table isn't even in the containing block chain, because of the
absolutely positioned element, so this is pretty weird.

[dgrogan, 2016-04-29 20:37:59]

Patchset #8 (id:140001) has been deleted

[dgrogan, 2016-04-29 20:56:11]

Patchset #8 (id:160001) has been deleted

[dgrogan, 2016-04-29 21:03:56]

dgrogan@chromium.org changed reviewers:
+ wangxianzhu@chromium.org

[dgrogan, 2016-04-29 21:03:57]

Xianzhu, this patch causes
fast/table/border-collapsing/cached-change-row-border-width.html to fail. I
don't know enough about painting to know if this is trivial, but it sure doesn't
look like it. Would you mind taking a look at the patch and let me know if you
see any suspect changes?

[Xianzhu, 2016-04-29 21:05:50]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-29 21:06:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1921973005/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1921973005/180001

[Xianzhu, 2016-04-29 21:06:51]

On 2016/04/29 21:03:57, dgrogan wrote:
> Xianzhu, this patch causes
> fast/table/border-collapsing/cached-change-row-border-width.html to fail. I
> don't know enough about painting to know if this is trivial, but it sure
doesn't
> look like it. Would you mind taking a look at the patch and let me know if you
> see any suspect changes?

I scheduled a dry run so that I can examine the result. (I suggest you also do
this (or git cl try) for each uploaded patch set.)

[Xianzhu, 2016-04-29 21:26:16]

On 2016/04/29 21:06:51, Xianzhu wrote:
> On 2016/04/29 21:03:57, dgrogan wrote:
> > Xianzhu, this patch causes
> > fast/table/border-collapsing/cached-change-row-border-width.html to fail. I
> > don't know enough about painting to know if this is trivial, but it sure
> doesn't
> > look like it. Would you mind taking a look at the patch and let me know if
you
> > see any suspect changes?
> 
> I scheduled a dry run so that I can examine the result. (I suggest you also do
> this (or git cl try) for each uploaded patch set.)

It seems that the layout of the test changed with your patch. You can run the
following test (put under fast/table/border-collapsing) as a layout test with
and without your change to see if the layout change in the dumped layout tree is
expected. If yes, then you are safe to rebaseline the paint invalidation test.

<link rel="stylesheet" href="resources/cached.css">
<table style="border-collapse:collapse; border:1px solid blue">
  <tr style="border:4px solid yellow" id="row">
    <td style="border:2px solid lime"/>
  </tr>
  <tr>
    <td/>
  </tr>
</table>

[commit-bot: I haz the power, 2016-04-29 22:25:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-29 22:25:14]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dgrogan, 2016-04-29 23:20:40]

The layout doesn't seem to have changed with the patch. For both before and
after I get:
layer at (0,0) size 800x600
  LayoutView at (0,0) size 800x600
layer at (0,0) size 800x600
  LayoutBlockFlow {HTML} at (0,0) size 800x600
    LayoutBlockFlow {BODY} at (8,8) size 784x584
      LayoutTable {TABLE} at (0,0) size 60x103 [border: (2px solid #0000FF)]
        LayoutTableSection {TBODY} at (2,2) size 56x100
          LayoutTableRow {TR} at (0,0) size 56x50 [border: (4px solid #FFFF00)]
            LayoutTableCell {TD} at (0,22) size 56x6 [border: (2px solid
#00FF00)] [r=0 c=0 rs=1 cs=1]
          LayoutTableRow {TR} at (0,50) size 56x50
            LayoutTableCell {TD} at (0,73) size 56x4 [border: (2px none
#000000)] [r=1 c=0 rs=1 cs=1]

The test passes if I remove the new call to
bordersPaddingAndSpacingInRowDirection() but I'm not yet sure why. Any ideas?
I'll of course keep digging. Thanks for your help.

https://codereview.chromium.org/1921973005/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):

https://codereview.chromium.org/1921973005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:299: LayoutUnit
scaledWidth = m_tableLayout->scaledWidthFromPercentColumns() +
bordersPaddingAndSpacingInRowDirection();
This one.

[Xianzhu, 2016-04-30 00:06:26]

On 2016/04/29 23:20:40, dgrogan wrote:
> The layout doesn't seem to have changed with the patch. For both before and
> after I get:
> layer at (0,0) size 800x600
>   LayoutView at (0,0) size 800x600
> layer at (0,0) size 800x600
>   LayoutBlockFlow {HTML} at (0,0) size 800x600
>     LayoutBlockFlow {BODY} at (8,8) size 784x584
>       LayoutTable {TABLE} at (0,0) size 60x103 [border: (2px solid #0000FF)]
>         LayoutTableSection {TBODY} at (2,2) size 56x100
>           LayoutTableRow {TR} at (0,0) size 56x50 [border: (4px solid
#FFFF00)]
>             LayoutTableCell {TD} at (0,22) size 56x6 [border: (2px solid
> #00FF00)] [r=0 c=0 rs=1 cs=1]
>           LayoutTableRow {TR} at (0,50) size 56x50
>             LayoutTableCell {TD} at (0,73) size 56x4 [border: (2px none
> #000000)] [r=1 c=0 rs=1 cs=1]
> 
> The test passes if I remove the new call to
> bordersPaddingAndSpacingInRowDirection() but I'm not yet sure why. Any ideas?
> I'll of course keep digging. Thanks for your help.
> 
>
https://codereview.chromium.org/1921973005/diff/180001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):
> 
>
https://codereview.chromium.org/1921973005/diff/180001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutTable.cpp:299: LayoutUnit
> scaledWidth = m_tableLayout->scaledWidthFromPercentColumns() +
> bordersPaddingAndSpacingInRowDirection();
> This one.

I found that borderBoxRect of the td is different with (0,0,54,50) and without
your change (0,0,52,50).

[dgrogan, 2016-05-04 01:54:14]

Ah, quite right. The table size IS different with my change, but only for the
original test, not the simplified one.

But I think we've uncovered a latent bug. When the original test changes border
sizes LayoutTableRow::styleDidChange is called but doesn't mark the table's
preferred widths dirty. This causes TableLayoutAlgorithmAuto::layout to subtract
the _new_ border width from m_table->logicalWidth(), which was calculated with
the _original_ border width in LayoutTable::computePreferredLogicalWidths().

I'll dig more tomorrow but thoughts in the meantime are welcome.

[dgrogan, 2016-05-18 13:49:18]

Morten, this is ready for a detailed review now that its dependency landed.

[mstensho (USE GERRIT), 2016-05-18 19:50:18]

https://codereview.chromium.org/1921973005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp (right):

https://codereview.chromium.org/1921973005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp:36: ,
m_scaledWidthFromPercentColumns(0)
No need to explicitly initialize LayoutUnit to 0. Actually it's kind of bad to
do it, since we'd perform saturation checks and stuff. :)

https://codereview.chromium.org/1921973005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp:267: if
(shouldScaleColumnsForParent(m_table)) {
Should probably check |scaleColumnsForSelf| first, since this may be expensive,
given a deep enough tree. Maybe we should also check if
m_scaledWidthFromPercentColumns is different from maxWidth before (potentially)
walking to the root.

https://codereview.chromium.org/1921973005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp:274:
LayoutUnit TableLayoutAlgorithmAuto::scaledWidthFromPercentColumns()
Make this inline?

[dgrogan, 2016-05-18 22:28:33]

https://codereview.chromium.org/1921973005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp (right):

https://codereview.chromium.org/1921973005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp:36: ,
m_scaledWidthFromPercentColumns(0)
On 2016/05/18 19:50:17, mstensho wrote:
> No need to explicitly initialize LayoutUnit to 0. Actually it's kind of bad to
> do it, since we'd perform saturation checks and stuff. :)

Done.

https://codereview.chromium.org/1921973005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp:267: if
(shouldScaleColumnsForParent(m_table)) {
On 2016/05/18 19:50:17, mstensho wrote:
> Should probably check |scaleColumnsForSelf| first, since this may be
expensive,
> given a deep enough tree. Maybe we should also check if
> m_scaledWidthFromPercentColumns is different from maxWidth before
(potentially)
> walking to the root.

Done. Well, I changed to checking to see if m_scaledWidthFromPercentColumns >
maxWidth, since that's the only way it could have an effect.

https://codereview.chromium.org/1921973005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp:274:
LayoutUnit TableLayoutAlgorithmAuto::scaledWidthFromPercentColumns()
On 2016/05/18 19:50:17, mstensho wrote:
> Make this inline?

Done.

[dgrogan, 2016-05-18 22:46:43]

Patchset #12 (id:260001) has been deleted

[mstensho (USE GERRIT), 2016-05-19 05:30:13]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2016-05-19 05:30:14]

lgtm

https://codereview.chromium.org/1921973005/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.h (right):

https://codereview.chromium.org/1921973005/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.h:57: LayoutUnit
scaledWidthFromPercentColumns() override
Duh, I somehow missed the fact that this one was virtual (but really, how would
it work, otherwise...). Still fine to make it "inline", of course, but I
shouldn't really have bothered to ask you to make this change. :)

[commit-bot: I haz the power, 2016-05-19 05:30:41]

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

[commit-bot: I haz the power, 2016-05-19 05:33:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-19 05:33:27]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dgrogan, 2016-05-19 06:19:32]

Thanks for the review and for sticking with it for so long.

[dgrogan, 2016-05-19 06:19:46]

The CQ bit was checked by dgrogan@chromium.org

[dgrogan, 2016-05-19 06:19:47]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com
Link to the patchset: https://codereview.chromium.org/1921973005/#ps300001
(title: "merge ToT")

[commit-bot: I haz the power, 2016-05-19 06:20:07]

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

[commit-bot: I haz the power, 2016-05-19 08:29:44]

Committed patchset #13 (id:300001)

[commit-bot: I haz the power, 2016-05-19 08:30:52]

Description was changed from

==========
[css tables] Don't pass table width increase due to % columns to parents

A table cell that specifies % width can increase the width of its table,
but shouldn't increase the width of any grandparent table cell.

This makes Blink match FF and Edge.

LayoutTable::layout calls LayoutTable::updateLogicalWidth, which
eventually calls into
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths, which used to
do the percent scaling. This doesn't work because when the table is
inside a table-cell, LayoutTableCell calls the child table's
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths, returning the
scaled width, not the intrinsic width.

This patch moves the scaling to a separate step that's only called in
LayoutTable::updateLogicalWidth, not in
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths.

BUG=178369,427994
==========

to

==========
[css tables] Don't pass table width increase due to % columns to parents

A table cell that specifies % width can increase the width of its table,
but shouldn't increase the width of any grandparent table cell.

This makes Blink match FF and Edge.

LayoutTable::layout calls LayoutTable::updateLogicalWidth, which
eventually calls into
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths, which used to
do the percent scaling. This doesn't work because when the table is
inside a table-cell, LayoutTableCell calls the child table's
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths, returning the
scaled width, not the intrinsic width.

This patch moves the scaling to a separate step that's only called in
LayoutTable::updateLogicalWidth, not in
TableLayoutAlgorithmAuto::computeIntrinsicLogicalWidths.

BUG=178369,427994

Committed: https://crrev.com/9428cfb16993a2329e87c65da096ca295132ef0f
Cr-Commit-Position: refs/heads/master@{#394715}
==========

[commit-bot: I haz the power, 2016-05-19 08:30:53]

Patchset 13 (id:??) landed as
https://crrev.com/9428cfb16993a2329e87c65da096ca295132ef0f
Cr-Commit-Position: refs/heads/master@{#394715}