offsetWidth on a table cell with fixed width is wrong

offsetWidth on a table cell with fixed width is wrong

TableCell's width is layout dependent, even when specified
via width: Npx in CSS.

Hopefully there are not other cases which are missing from this
optimization.  This code is very fragile as written.
We should make this a white-list instead of a black-list
for this optimization.

BUG=290399
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158467

[pdr, 2013-09-27 19:23:49]

LGTM, and +1 to making this optimization more robust in the future.

[eseidel, 2013-09-27 19:24:18]

Layout reviewers, activate!

This is for a stable-blocker.

Please carefully consider if there are other block-types whose width is
layout-dependent.

Another fix would be to just disable this optimization entirely for stable, and
ship it in a later Chrome.

[commit-bot: I haz the power, 2013-09-27 19:28:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eseidel@chromium.org/25086004/1

[eseidel, 2013-09-27 19:34:11]

Committed patchset #1 manually as r158467 (presubmit successful).

[eae, 2013-09-27 19:56:56]

https://codereview.chromium.org/25086004/diff/1/Source/core/rendering/RenderB...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/25086004/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBox.cpp:425: || isTableCell();
This should probably be isTableCell() && !table->isFixedLayout()

[Julien - ping for review, 2013-09-27 20:03:54]

https://codereview.chromium.org/25086004/diff/1/Source/core/rendering/RenderB...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/25086004/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBox.cpp:425: || isTableCell();
On 2013/09/27 19:56:56, eae wrote:
> This should probably be isTableCell() && !table->isFixedLayout()

I don't think so. Fixed table layout will also force the table cells' widths.

Note that I think that anything that depend on a cell's size should also trigger
a layout (table, row, row-group, column and column-group).

[eae, 2013-09-27 20:05:27]

On 2013/09/27 20:03:54, Julien Chaffraix wrote:
>
https://codereview.chromium.org/25086004/diff/1/Source/core/rendering/RenderB...
> File Source/core/rendering/RenderBox.cpp (right):
> 
>
https://codereview.chromium.org/25086004/diff/1/Source/core/rendering/RenderB...
> Source/core/rendering/RenderBox.cpp:425: || isTableCell();
> On 2013/09/27 19:56:56, eae wrote:
> > This should probably be isTableCell() && !table->isFixedLayout()
> 
> I don't think so. Fixed table layout will also force the table cells' widths.
> 
> Note that I think that anything that depend on a cell's size should also
trigger
> a layout (table, row, row-group, column and column-group).

Hmm, you might be right.

[eseidel, 2013-09-27 20:13:52]

I tried:
<table><tbody style="width: 10px"><td style="font:
Ahem;">FAIL</td><tbody></table>
<script>
if (window.testRunner)
	testRunner.dumpAsText();
var width = document.getElementsByTagName('tbody')[0].offsetWidth;
document.body.textContent = (width == 36 ? "PASS" : "FAIL") + " (" + width +
"px)";
</script>

and

<table style="width: 10px"><td style="font: Ahem;">FAIL</td></table>
<script>
if (window.testRunner)
	testRunner.dumpAsText();
var width = document.getElementsByTagName('table')[0].offsetWidth;
document.body.textContent = (width == 36 ? "PASS" : "FAIL") + " (" + width +
"px)";
</script>

And didn't repro the bug.

But I'm happy to write a different patch.

[eseidel, 2013-09-27 20:39:49]

I've attempted to clean up the code a bit more in
https://codereview.chromium.org/25048006

It's now pretty clear that this is turned on for all of RenderBlockFlow, which
is probably much more than is safe.