Treat <table> widths of 0 as auto.

Don't scale columns in an inner table when the outer table has width:0px

The bug has an old repro that depends on whether the inner table
is floated. This no longer seems to have an impact on our behavior.

BUG=244182
Committed: https://crrev.com/2d44230a5d4a9f92e5fc9fb2777847f594f37311
Cr-Commit-Position: refs/heads/master@{#374799}

[dgrogan, 2016-02-09 03:18:01]

Patchset #1 (id:1) has been deleted

[dgrogan, 2016-02-09 03:27:18]

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

[dgrogan, 2016-02-09 03:27:19]

Morten, could you review this?

I am not sure this is the right approach to fix this problem so if you have a
better suggestion I am all ears. The old proposed patch
(https://codereview.chromium.org/15650011/) seemed to just be hacking away on
the problem though, adding more complexity deep in the bowels of table layout.

BTW, IE and FF agree on the correct behavior:
https://dev.windows.com/en-us/microsoft-edge/tools/screenshots/#https://jsfid...

https://codereview.chromium.org/1682453004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (left):

https://codereview.chromium.org/1682453004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:275: if
((styleLogicalWidth.isSpecified() && styleLogicalWidth.isPositive()) ||
styleLogicalWidth.isIntrinsic()) {
With the above change this clause is always true.

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

https://codereview.chromium.org/1682453004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:276:
mutableStyle()->setLogicalWidth(Length());
I'm not sure how much of a no-no this is, to mutate the style during layout.
Perhaps I should instead modify whatever transforms a negative specified width
into auto to do the same for a zero specified width? Do you know where that is?
I've not been able to find it yet.

[dgrogan, 2016-02-09 03:28:00]

Description was changed from

==========
Treat <table> widths of 0px as auto.

Checking for isSpecified() && isPositive() multiple places is fragile.
A missing one caused this bug.

The original bug had a test case that depended on whether the inner table
was floated. This no longer seems to have an impact on our behavior.

BUG=244182
==========

to

==========
Treat <table> widths of 0px as auto.

Checking for isSpecified() && isPositive() multiple places is fragile.
A missing one caused this bug.

The bug has an old test case that depends on whether the inner table
was floated. This no longer seems to have an impact on our behavior.

BUG=244182
==========

[dgrogan, 2016-02-09 03:31:28]

Description was changed from

==========
Treat <table> widths of 0px as auto.

Checking for isSpecified() && isPositive() multiple places is fragile.
A missing one caused this bug.

The bug has an old test case that depends on whether the inner table
was floated. This no longer seems to have an impact on our behavior.

BUG=244182
==========

to

==========
Treat <table> widths of 0 as auto.

Checking for isSpecified() && isPositive() multiple places is fragile.
A missing one caused this bug.

The bug has an old test case that depends on whether the inner table
was floated. This no longer seems to have an impact on our behavior.

BUG=244182
==========

[mstensho (USE GERRIT), 2016-02-09 10:52:46]

I think the original CL - https://codereview.chromium.org/15650011/ - was closer
to what we want.

We probably only want to change the behavior for auto-layout tables, and leave
fixed-layout ones alone.

To a large extent, we already treat 0 as auto; e.g:

<table style="width:0;">
    <tr>
        <td>This text should be on one line.</td>
    </tr>
</table>

Now see what happens if we specify width:1px on the table instead. So there
already exists a mechanism to treat 0 as auto, but it's bypassed by
shouldScaleColumns(), which just looks at computed style.

(BTW: someone ought to teach shouldScaleColumns() about useful things like early
returns, instead of setting |table| to 0 to exit from the loop)

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

https://codereview.chromium.org/1682453004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:276:
mutableStyle()->setLogicalWidth(Length());
On 2016/02/09 03:27:19, dgrogan wrote:
> I'm not sure how much of a no-no this is, to mutate the style during layout.
> Perhaps I should instead modify whatever transforms a negative specified width
> into auto to do the same for a zero specified width? Do you know where that
is?
> I've not been able to find it yet.

I think this is bad. We should really leave computed style alone. Negative
width? Do we have those? The parser shouldn't have allowed that, since it would
be a spec violation.

Non-positive widths as in zero, sure - but negative widths???

[dgrogan, 2016-02-10 02:47:56]

Good point about not affecting fixed-layout tables. I tried to make a
fixed-layout test that patchset 1 fails but couldn't: the
TableLayoutAlgorithmFixed object is already in place and blindly requests the
table's width, which is still 0.

Anyway, thanks for the feedback. Take another look? I put off refactoring
shouldScaleColumns until this fix is in.

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

https://codereview.chromium.org/1682453004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:276:
mutableStyle()->setLogicalWidth(Length());
On 2016/02/09 10:52:45, mstensho wrote:
> On 2016/02/09 03:27:19, dgrogan wrote:
> > I'm not sure how much of a no-no this is, to mutate the style during layout.
> > Perhaps I should instead modify whatever transforms a negative specified
width
> > into auto to do the same for a zero specified width? Do you know where that
> is?
> > I've not been able to find it yet.
> 
> I think this is bad. We should really leave computed style alone. Negative
> width? Do we have those? The parser shouldn't have allowed that, since it
would
> be a spec violation.
> 
> Non-positive widths as in zero, sure - but negative widths???

No, we don't have negative widths, which is why I added the ASSERT. Some code
below checks for negative widths though...

https://codereview.chromium.org/1682453004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:298: if
((styleMaxLogicalWidth.isSpecified() && !styleMaxLogicalWidth.isNegative()) ||
styleMaxLogicalWidth.isIntrinsic()) {
Like here: !styleMaxLogicalWidth.isNegative().

[dgrogan, 2016-02-10 02:55:30]

Description was changed from

==========
Treat <table> widths of 0 as auto.

Checking for isSpecified() && isPositive() multiple places is fragile.
A missing one caused this bug.

The bug has an old test case that depends on whether the inner table
was floated. This no longer seems to have an impact on our behavior.

BUG=244182
==========

to

==========
Don't scale columns in an inner table when the outer table has width:0px

The bug has an old repro that depends on whether the inner table
is floated. This no longer seems to have an impact on our behavior.

BUG=244182
==========

[mstensho (USE GERRIT), 2016-02-10 10:04:09]

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

https://codereview.chromium.org/1682453004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:298: if
((styleMaxLogicalWidth.isSpecified() && !styleMaxLogicalWidth.isNegative()) ||
styleMaxLogicalWidth.isIntrinsic()) {
On 2016/02/10 02:47:56, dgrogan wrote:
> Like here: !styleMaxLogicalWidth.isNegative().

o_O

We should probably turn that into an ASSERT some day.

https://codereview.chromium.org/1682453004/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/table/zero-width-on-table-is-auto.html
(right):

https://codereview.chromium.org/1682453004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/table/zero-width-on-table-is-auto.html:10:
<table id="outerTable" style="width:0px;" class="yellow"
data-expected-width="200">
Can you add a pass condition text in front of this? "There should be no red" or
something like that.

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

https://codereview.chromium.org/1682453004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp:199: if
(cell->colSpan() > 1 || tableWidth.isAuto() || (tableWidth.isSpecified() &&
tableWidth.isZero()))
I think this is the behavior we want.

The reason why <table style="width:0;"> is treated as auto on a general basis
can be found in LayoutTable::updateLogicalWidth():

    if ((styleLogicalWidth.isSpecified() && styleLogicalWidth.isPositive()) ||
styleLogicalWidth.isIntrinsic()) {

So maybe we could add this as a method to LayoutTable?

bool LayoutTable::isLogicalWidthAuto() { ... return
(!styleLogicalWidth.isSpecified() || !styleLogicalWidth.isPositive()) &&
!styleLogicalWidth.isIntrinsic(); }

[dgrogan, 2016-02-10 18:38:26]

https://codereview.chromium.org/1682453004/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/table/zero-width-on-table-is-auto.html
(right):

https://codereview.chromium.org/1682453004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/table/zero-width-on-table-is-auto.html:10:
<table id="outerTable" style="width:0px;" class="yellow"
data-expected-width="200">
On 2016/02/10 10:04:09, mstensho wrote:
> Can you add a pass condition text in front of this? "There should be no red"
or
> something like that.

Done.

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

https://codereview.chromium.org/1682453004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp:199: if
(cell->colSpan() > 1 || tableWidth.isAuto() || (tableWidth.isSpecified() &&
tableWidth.isZero()))
On 2016/02/10 10:04:09, mstensho wrote:
> I think this is the behavior we want.
> 
> The reason why <table style="width:0;"> is treated as auto on a general basis
> can be found in LayoutTable::updateLogicalWidth():
> 
>     if ((styleLogicalWidth.isSpecified() && styleLogicalWidth.isPositive()) ||
> styleLogicalWidth.isIntrinsic()) {
> 
> So maybe we could add this as a method to LayoutTable?
> 
> bool LayoutTable::isLogicalWidthAuto() { ... return
> (!styleLogicalWidth.isSpecified() || !styleLogicalWidth.isPositive()) &&
> !styleLogicalWidth.isIntrinsic(); }

Done.

[mstensho (USE GERRIT), 2016-02-10 19:50:50]

lgtm

[dgrogan, 2016-02-10 20:25:42]

The CQ bit was checked by dgrogan@chromium.org

[commit-bot: I haz the power, 2016-02-10 20:27:08]

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

[commit-bot: I haz the power, 2016-02-10 22:25:27]

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

[commit-bot: I haz the power, 2016-02-10 22:25:28]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dgrogan, 2016-02-10 22:30:05]

The CQ bit was checked by dgrogan@chromium.org

[commit-bot: I haz the power, 2016-02-10 22:34:59]

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

[commit-bot: I haz the power, 2016-02-11 00:13:14]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-02-16 22:33:03]

Description was changed from

==========
Don't scale columns in an inner table when the outer table has width:0px

The bug has an old repro that depends on whether the inner table
is floated. This no longer seems to have an impact on our behavior.

BUG=244182
==========

to

==========
Don't scale columns in an inner table when the outer table has width:0px

The bug has an old repro that depends on whether the inner table
is floated. This no longer seems to have an impact on our behavior.

BUG=244182

Committed: https://crrev.com/2d44230a5d4a9f92e5fc9fb2777847f594f37311
Cr-Commit-Position: refs/heads/master@{#374799}
==========

[commit-bot: I haz the power, 2016-02-16 22:33:04]

Patchset 3 (id:??) landed as
https://crrev.com/2d44230a5d4a9f92e5fc9fb2777847f594f37311
Cr-Commit-Position: refs/heads/master@{#374799}