Col width is not honored when dynamically updated and it would not make table narrower

Col width is not honored when dynamically updated and it would not make table narrower

Increasing the table width by increasing the colgroup width was working
but decreasing the table width by decreasing the colgroup width is not working.

When colgroup width is defined table cell should adjust according to that.
On decreasing colgroup width the cells maxPreferredLogicalWidth was
still set to the earlier value. Setting the setPreferredLogicalWidthsDirty
to true so that cells pref width is calculated again.

I already landed this patch on webkit. Just modified the layout test file and fixed build errors.
http://trac.webkit.org/changeset/162334.

R=jchaffraix@chromium.org
BUG=340252
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168908

[Gurpreet, 2014-02-03 13:24:53]

Please have a look at this patch.

[Julien - ping for review, 2014-02-06 22:28:18]

https://codereview.chromium.org/151993004/diff/20001/LayoutTests/fast/dom/HTM...
File
LayoutTests/fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html
(right):

https://codereview.chromium.org/151993004/diff/20001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html:21:
shouldBe("document.getElementById('colWidth').offsetWidth","100");
Do we really need a custom check instead of using a check-layout.js test?

https://codereview.chromium.org/151993004/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderTableCol.cpp (right):

https://codereview.chromium.org/151993004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderTableCol.cpp:56: } else if (oldStyle &&
oldStyle->width() != style()->width()) {
I don't think this is right: you need to check logical direction so it should be
logicalWidth() here, not width().

This should be covered by a vertical writing mode test case too as it was missed
in the code.

https://codereview.chromium.org/151993004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderTableCol.cpp:58: if (child->isTableSection()) {
We prefer early return so I would expect us to prefer also early continue (to
avoid lost of nested blocks of code).

https://codereview.chromium.org/151993004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderTableCol.cpp:69: }
You are basically dirtying the preferred logical widths for all the cells, which
is totally overblown.

Only the cells in the current column should need their preferred logical widths
recomputed.

[Gurpreet, 2014-02-12 14:06:57]

On 2014/02/06 22:28:18, Julien Chaffraix - PST wrote:
>
https://codereview.chromium.org/151993004/diff/20001/LayoutTests/fast/dom/HTM...
> File
>
LayoutTests/fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html
> (right):
> 
>
https://codereview.chromium.org/151993004/diff/20001/LayoutTests/fast/dom/HTM...
>
LayoutTests/fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html:21:
> shouldBe("document.getElementById('colWidth').offsetWidth","100");
> Do we really need a custom check instead of using a check-layout.js test?
> 
>
https://codereview.chromium.org/151993004/diff/20001/Source/core/rendering/Re...
> File Source/core/rendering/RenderTableCol.cpp (right):
> 
>
https://codereview.chromium.org/151993004/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderTableCol.cpp:56: } else if (oldStyle &&
> oldStyle->width() != style()->width()) {
> I don't think this is right: you need to check logical direction so it should
be
> logicalWidth() here, not width().
> 
> This should be covered by a vertical writing mode test case too as it was
missed
> in the code.
> 
>
https://codereview.chromium.org/151993004/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderTableCol.cpp:58: if (child->isTableSection()) {
> We prefer early return so I would expect us to prefer also early continue (to
> avoid lost of nested blocks of code).
> 
>
https://codereview.chromium.org/151993004/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderTableCol.cpp:69: }
> You are basically dirtying the preferred logical widths for all the cells,
which
> is totally overblown.
> 
> Only the cells in the current column should need their preferred logical
widths
> recomputed.

Hi Julien. Thanks for the review. I have modified the function as suggested and
added new layout test files. Please have a look.

[Julien - ping for review, 2014-02-14 18:18:36]

https://codereview.chromium.org/151993004/diff/110001/LayoutTests/fast/dom/HT...
File
LayoutTests/fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html
(right):

https://codereview.chromium.org/151993004/diff/110001/LayoutTests/fast/dom/HT...
LayoutTests/fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html:34:
<button onclick="changeColWidthUsingWidth();">Click me to test manually. The
column should shrink to 100px.</button>
It's awesome that you thought about manual testing!

Couldn't we just make it automatic using the load event? If needed you can
always force a layout by querying document.body.offsetTop

https://codereview.chromium.org/151993004/diff/110001/Source/core/rendering/R...
File Source/core/rendering/RenderTableCol.cpp (right):

https://codereview.chromium.org/151993004/diff/110001/Source/core/rendering/R...
Source/core/rendering/RenderTableCol.cpp:59: for (RenderTableCol* column =
table->firstColumn()->nextColumn(); column; column = column->nextColumn()) {
The logic skips the first column, is that intentional? Also is that really
correct with column-group (nextColumn returns the next column *or*
column-group)?

https://codereview.chromium.org/151993004/diff/110001/Source/core/rendering/R...
Source/core/rendering/RenderTableCol.cpp:66: for (unsigned i = 0; i <
table->firstBody()->numRows(); i++) {
You can't limit yourself to the first table-body as the other table sections are
impacted by a column change.

[Gurpreet, 2014-02-17 09:32:07]

Changes done. Please have a look.

https://codereview.chromium.org/151993004/diff/110001/Source/core/rendering/R...
File Source/core/rendering/RenderTableCol.cpp (right):

https://codereview.chromium.org/151993004/diff/110001/Source/core/rendering/R...
Source/core/rendering/RenderTableCol.cpp:59: for (RenderTableCol* column =
table->firstColumn()->nextColumn(); column; column = column->nextColumn()) {
On 2014/02/14 18:18:37, Julien Chaffraix - PST wrote:
> The logic skips the first column, is that intentional? Also is that really
> correct with column-group (nextColumn returns the next column *or*
> column-group)?

The firstColumn would give the colgroup instead of col which is not what is
requried here. We want the col object so firstColumn()->nextColumn() would give
the first col and column->nextColumn() would give the next col.

https://codereview.chromium.org/151993004/diff/110001/Source/core/rendering/R...
Source/core/rendering/RenderTableCol.cpp:66: for (unsigned i = 0; i <
table->firstBody()->numRows(); i++) {
On 2014/02/14 18:18:37, Julien Chaffraix - PST wrote:
> You can't limit yourself to the first table-body as the other table sections
are
> impacted by a column change.

Done.

[Gurpreet, 2014-02-17 09:40:43]

https://codereview.chromium.org/151993004/diff/110001/LayoutTests/fast/dom/HT...
File
LayoutTests/fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html
(right):

https://codereview.chromium.org/151993004/diff/110001/LayoutTests/fast/dom/HT...
LayoutTests/fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html:34:
<button onclick="changeColWidthUsingWidth();">Click me to test manually. The
column should shrink to 100px.</button>
On 2014/02/14 18:18:37, Julien Chaffraix - PST wrote:
> It's awesome that you thought about manual testing!
> 
> Couldn't we just make it automatic using the load event? If needed you can
> always force a layout by querying document.body.offsetTop

The col width changes should be done on some manual action or after some time so
that initially the table layout is done with col width as 200px and then changed
to 100px.
I tried with onload event and setTimeout function as 
<script type="text/javascript">
    function runTest() {
        setTimeout(function() {
            var col = document.getElementById('thecol');
            col.style.width="100px";                    
            window.checkLayout("td", document.getElementById("test-output"));
            }, 0);
        }
</script>  
<body onload="runTest()">
But then the expected.txt file is not generated properly.
window.checkLayout("td", document.getElementById("test-output")); doesnot run.

[Gurpreet, 2014-02-17 09:41:20]

Thanks Julien for the review. Added comments and modified the code.

[Julien - ping for review, 2014-02-24 19:25:30]

https://codereview.chromium.org/151993004/diff/170001/LayoutTests/fast/dom/HT...
File
LayoutTests/fast/dom/HTMLTableColElement/resize-table-using-col-height-vertical-writing-mode.html
(right):

https://codereview.chromium.org/151993004/diff/170001/LayoutTests/fast/dom/HT...
LayoutTests/fast/dom/HTMLTableColElement/resize-table-using-col-height-vertical-writing-mode.html:23:
Tests that the height of table cell changes on changing the colgroup height to
new height.
It's the logical width that you are changing, talking about height is right but
confusing as we are thinking in terms of the CSS 3 writing mode's abstract box.

https://codereview.chromium.org/151993004/diff/170001/Source/core/rendering/R...
File Source/core/rendering/RenderTableCol.cpp (right):

https://codereview.chromium.org/151993004/diff/170001/Source/core/rendering/R...
Source/core/rendering/RenderTableCol.cpp:70: }
There is table->topSection() that will give you this answer in a more optimized
manner.

https://codereview.chromium.org/151993004/diff/170001/Source/core/rendering/R...
Source/core/rendering/RenderTableCol.cpp:80: }
Again, you need to invalidate the whole column which spans several sections so
doing that for the first section just doesn't work.

[Gurpreet, 2014-02-25 13:12:56]

Modified code and added comments. Please review.

https://codereview.chromium.org/151993004/diff/170001/LayoutTests/fast/dom/HT...
File
LayoutTests/fast/dom/HTMLTableColElement/resize-table-using-col-height-vertical-writing-mode.html
(right):

https://codereview.chromium.org/151993004/diff/170001/LayoutTests/fast/dom/HT...
LayoutTests/fast/dom/HTMLTableColElement/resize-table-using-col-height-vertical-writing-mode.html:23:
Tests that the height of table cell changes on changing the colgroup height to
new height.
On 2014/02/24 19:25:31, Julien Chaffraix - PST wrote:
> It's the logical width that you are changing, talking about height is right
but
> confusing as we are thinking in terms of the CSS 3 writing mode's abstract
box.

Initially was checking for style()->width() instead of logicalWidth so that
would only do for width related changes only as height test case is already
working without my changes also.

https://codereview.chromium.org/151993004/diff/170001/Source/core/rendering/R...
File Source/core/rendering/RenderTableCol.cpp (right):

https://codereview.chromium.org/151993004/diff/170001/Source/core/rendering/R...
Source/core/rendering/RenderTableCol.cpp:70: }
On 2014/02/24 19:25:31, Julien Chaffraix - PST wrote:
> There is table->topSection() that will give you this answer in a more
optimized
> manner.

Done.

https://codereview.chromium.org/151993004/diff/170001/Source/core/rendering/R...
Source/core/rendering/RenderTableCol.cpp:80: }
On 2014/02/24 19:25:31, Julien Chaffraix - PST wrote:
> Again, you need to invalidate the whole column which spans several sections so
> doing that for the first section just doesn't work.

Done.

[Julien - ping for review, 2014-03-01 00:27:54]

https://codereview.chromium.org/151993004/diff/170001/LayoutTests/fast/dom/HT...
File
LayoutTests/fast/dom/HTMLTableColElement/resize-table-using-col-height-vertical-writing-mode.html
(right):

https://codereview.chromium.org/151993004/diff/170001/LayoutTests/fast/dom/HT...
LayoutTests/fast/dom/HTMLTableColElement/resize-table-using-col-height-vertical-writing-mode.html:23:
Tests that the height of table cell changes on changing the colgroup height to
new height.
On 2014/02/25 13:12:56, Gurpreet wrote:
> On 2014/02/24 19:25:31, Julien Chaffraix - PST wrote:
> > It's the logical width that you are changing, talking about height is right
> but
> > confusing as we are thinking in terms of the CSS 3 writing mode's abstract
> box.
> 
> Initially was checking for style()->width() instead of logicalWidth so that
> would only do for width related changes only as height test case is already
> working without my changes also.

It would be nice to understand why this case was correct in the first place as
it should have had the same issue as the 'width' case.

https://codereview.chromium.org/151993004/diff/260001/LayoutTests/fast/dom/HT...
File
LayoutTests/fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html
(right):

https://codereview.chromium.org/151993004/diff/260001/LayoutTests/fast/dom/HT...
LayoutTests/fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html:34:
<button onclick="changeColWidthUsingWidth();">Click me to test manually. The
column should shrink to 100px.</button>
It would be nicer without a button, using the 'load' event + a forced layout
(document.body.offsetTop) should give the same result but in an automated
fashion.

[Gurpreet, 2014-03-03 11:42:34]

On 2014/03/01 00:27:54, Julien Chaffraix - PST wrote:
>
https://codereview.chromium.org/151993004/diff/170001/LayoutTests/fast/dom/HT...
> File
>
LayoutTests/fast/dom/HTMLTableColElement/resize-table-using-col-height-vertical-writing-mode.html
> (right):
> 
>
https://codereview.chromium.org/151993004/diff/170001/LayoutTests/fast/dom/HT...
>
LayoutTests/fast/dom/HTMLTableColElement/resize-table-using-col-height-vertical-writing-mode.html:23:
> Tests that the height of table cell changes on changing the colgroup height to
> new height.
> On 2014/02/25 13:12:56, Gurpreet wrote:
> > On 2014/02/24 19:25:31, Julien Chaffraix - PST wrote:
> > > It's the logical width that you are changing, talking about height is
right
> > but
> > > confusing as we are thinking in terms of the CSS 3 writing mode's abstract
> > box.
> > 
> > Initially was checking for style()->width() instead of logicalWidth so that
> > would only do for width related changes only as height test case is already
> > working without my changes also.
> 
> It would be nice to understand why this case was correct in the first place as
> it should have had the same issue as the 'width' case.
> 
>
https://codereview.chromium.org/151993004/diff/260001/LayoutTests/fast/dom/HT...
> File
>
LayoutTests/fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html
> (right):
> 
>
https://codereview.chromium.org/151993004/diff/260001/LayoutTests/fast/dom/HT...
>
LayoutTests/fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html:34:
> <button onclick="changeColWidthUsingWidth();">Click me to test manually. The
> column should shrink to 100px.</button>
> It would be nicer without a button, using the 'load' event + a forced layout
> (document.body.offsetTop) should give the same result but in an automated
> fashion.

[Gurpreet, 2014-03-03 11:42:46]

On 2014/03/03 11:42:34, Gurpreet wrote:
> On 2014/03/01 00:27:54, Julien Chaffraix - PST wrote:
> >
>
https://codereview.chromium.org/151993004/diff/170001/LayoutTests/fast/dom/HT...
> > File
> >
>
LayoutTests/fast/dom/HTMLTableColElement/resize-table-using-col-height-vertical-writing-mode.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/151993004/diff/170001/LayoutTests/fast/dom/HT...
> >
>
LayoutTests/fast/dom/HTMLTableColElement/resize-table-using-col-height-vertical-writing-mode.html:23:
> > Tests that the height of table cell changes on changing the colgroup height
to
> > new height.
> > On 2014/02/25 13:12:56, Gurpreet wrote:
> > > On 2014/02/24 19:25:31, Julien Chaffraix - PST wrote:
> > > > It's the logical width that you are changing, talking about height is
> right
> > > but
> > > > confusing as we are thinking in terms of the CSS 3 writing mode's
abstract
> > > box.
> > > 
> > > Initially was checking for style()->width() instead of logicalWidth so
that
> > > would only do for width related changes only as height test case is
already
> > > working without my changes also.
> > 
> > It would be nice to understand why this case was correct in the first place
as
> > it should have had the same issue as the 'width' case.
> > 
> >
>
https://codereview.chromium.org/151993004/diff/260001/LayoutTests/fast/dom/HT...
> > File
> >
>
LayoutTests/fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/151993004/diff/260001/LayoutTests/fast/dom/HT...
> >
>
LayoutTests/fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html:34:
> > <button onclick="changeColWidthUsingWidth();">Click me to test manually. The
> > column should shrink to 100px.</button>
> > It would be nicer without a button, using the 'load' event + a forced layout
> > (document.body.offsetTop) should give the same result but in an automated
> > fashion.

[Gurpreet, 2014-03-03 11:53:12]

On 2014/03/01 00:27:54, Julien Chaffraix - PST wrote:
>
https://codereview.chromium.org/151993004/diff/170001/LayoutTests/fast/dom/HT...
> File
>
LayoutTests/fast/dom/HTMLTableColElement/resize-table-using-col-height-vertical-writing-mode.html
> (right):
> 
>
https://codereview.chromium.org/151993004/diff/170001/LayoutTests/fast/dom/HT...
>
LayoutTests/fast/dom/HTMLTableColElement/resize-table-using-col-height-vertical-writing-mode.html:23:
> Tests that the height of table cell changes on changing the colgroup height to
> new height.
> On 2014/02/25 13:12:56, Gurpreet wrote:
> > On 2014/02/24 19:25:31, Julien Chaffraix - PST wrote:
> > > It's the logical width that you are changing, talking about height is
right
> > but
> > > confusing as we are thinking in terms of the CSS 3 writing mode's abstract
> > box.
> > 
> > Initially was checking for style()->width() instead of logicalWidth so that
> > would only do for width related changes only as height test case is already
> > working without my changes also.
> 
> It would be nice to understand why this case was correct in the first place as
> it should have had the same issue as the 'width' case.

The height case works as when col width changes layout is called and its
layouted as per the new height. But incase of width layout is called but void
AutoTableLayout::recalcColumn(unsigned effCol) 
if (cell->maxPreferredLogicalWidth() > columnLayout.maxLogicalWidth) {
                        columnLayout.maxLogicalWidth =
cell->maxPreferredLogicalWidth();
                        maxContributor = cell;
  }
The cell width which was earlier 200px is as it is and
columnLayout.maxLogicalWidth changes to 100 but since the condition is satisfied
the 
columnLayout.maxLogicalWidth changes back to 200px. I had tried changing the
condition but nothing worked so went ahead with the idea of calculating the
PreferredLogicalWidth again.


>
https://codereview.chromium.org/151993004/diff/260001/LayoutTests/fast/dom/HT...
> File
>
LayoutTests/fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html
> (right):
> 
>
https://codereview.chromium.org/151993004/diff/260001/LayoutTests/fast/dom/HT...
>
LayoutTests/fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html:34:
> <button onclick="changeColWidthUsingWidth();">Click me to test manually. The
> column should shrink to 100px.</button>
> It would be nicer without a button, using the 'load' event + a forced layout
> (document.body.offsetTop) should give the same result but in an automated
> fashion.

As I have mentioned earlier that the col width changes should be done on some
manual action or after some time so
that initially the table layout is done with col width as 200px and then changed
to 100px else without my changes also it will work on chrome incase we do onload
event and call the function changeColWidthUsingWidth().
I tried with onload event and setTimeout function as 
<script type="text/javascript">
    function runTest() {
        setTimeout(function() {
            var col = document.getElementById('thecol');
            col.style.width="100px";                    
            window.checkLayout("td", document.getElementById("test-output"));
            }, 0);
        }
</script>  
<body onload="runTest()">
But then the expected.txt file is not generated properly. On removing setTimeout
expected.txt works fine.
So I feel either we will have to use this button test case or check using
js-test.js instead of check-layout.js incase automated test case is required.

[Gurpreet, 2014-03-03 11:53:52]

Please ignore message #11 and #12.

[Gurpreet, 2014-03-03 11:53:52]

Please ignore message #11 and #12.

[Julien - ping for review, 2014-03-04 19:48:05]

On 2014/03/03 11:53:12, Gurpreet wrote:
> On 2014/03/01 00:27:54, Julien Chaffraix - PST wrote:
> >
>
https://codereview.chromium.org/151993004/diff/170001/LayoutTests/fast/dom/HT...
> > File
> >
>
LayoutTests/fast/dom/HTMLTableColElement/resize-table-using-col-height-vertical-writing-mode.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/151993004/diff/170001/LayoutTests/fast/dom/HT...
> >
>
LayoutTests/fast/dom/HTMLTableColElement/resize-table-using-col-height-vertical-writing-mode.html:23:
> > Tests that the height of table cell changes on changing the colgroup height
to
> > new height.
> > On 2014/02/25 13:12:56, Gurpreet wrote:
> > > On 2014/02/24 19:25:31, Julien Chaffraix - PST wrote:
> > > > It's the logical width that you are changing, talking about height is
> right
> > > but
> > > > confusing as we are thinking in terms of the CSS 3 writing mode's
abstract
> > > box.
> > > 
> > > Initially was checking for style()->width() instead of logicalWidth so
that
> > > would only do for width related changes only as height test case is
already
> > > working without my changes also.
> > 
> > It would be nice to understand why this case was correct in the first place
as
> > it should have had the same issue as the 'width' case.
> 
> The height case works as when col width changes layout is called and its
> layouted as per the new height. But incase of width layout is called but void
> AutoTableLayout::recalcColumn(unsigned effCol) 
> if (cell->maxPreferredLogicalWidth() > columnLayout.maxLogicalWidth) {
>                         columnLayout.maxLogicalWidth =
> cell->maxPreferredLogicalWidth();
>                         maxContributor = cell;
>   }
> The cell width which was earlier 200px is as it is and
> columnLayout.maxLogicalWidth changes to 100 but since the condition is
satisfied
> the 
> columnLayout.maxLogicalWidth changes back to 200px. I had tried changing the
> condition but nothing worked so went ahead with the idea of calculating the
> PreferredLogicalWidth again.
> 
> 
> >
>
https://codereview.chromium.org/151993004/diff/260001/LayoutTests/fast/dom/HT...
> > File
> >
>
LayoutTests/fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/151993004/diff/260001/LayoutTests/fast/dom/HT...
> >
>
LayoutTests/fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html:34:
> > <button onclick="changeColWidthUsingWidth();">Click me to test manually. The
> > column should shrink to 100px.</button>
> > It would be nicer without a button, using the 'load' event + a forced layout
> > (document.body.offsetTop) should give the same result but in an automated
> > fashion.
> 
> As I have mentioned earlier that the col width changes should be done on some
> manual action or after some time so
> that initially the table layout is done with col width as 200px and then
changed
> to 100px else without my changes also it will work on chrome incase we do
onload
> event and call the function changeColWidthUsingWidth().
> I tried with onload event and setTimeout function as 
> <script type="text/javascript">
>     function runTest() {
>         setTimeout(function() {
>             var col = document.getElementById('thecol');
>             col.style.width="100px";                    
>             window.checkLayout("td", document.getElementById("test-output"));
>             }, 0);
>         }
> </script>  
> <body onload="runTest()">
> But then the expected.txt file is not generated properly. On removing
setTimeout
> expected.txt works fine.
> So I feel either we will have to use this button test case or check using
> js-test.js instead of check-layout.js incase automated test case is required.

You can trigger a layout whenever you want, which should solve your issue and
the need to work-around that by doing a round trip through the event loop:

<script type="text/javascript">
    function runTest() {
        var col = document.getElementById('thecol');

        // Force the initial layout so that the column width change below
triggers a new layout.
        col.offsetTop;

        // You can even check that the column width is correct as this is a
pre-condition for this test to work.

        col.style.width="100px";                    
        window.checkLayout("td", document.getElementById("test-output"));
    }
</script>  
<body onload="runTest()">


By the way, 2 of the tests are already passing without the change, not sure if
that was intended. Also those are not DOM tests, they check the rendering of
tables so should be placed into fast/table/.

[Gurpreet, 2014-03-05 11:10:47]

> You can trigger a layout whenever you want, which should solve your issue and
> the need to work-around that by doing a round trip through the event loop:
> 
> <script type="text/javascript">
>     function runTest() {
>         var col = document.getElementById('thecol');
> 
>         // Force the initial layout so that the column width change below
> triggers a new layout.
>         col.offsetTop;
> 
>         // You can even check that the column width is correct as this is a
> pre-condition for this test to work.
> 
>         col.style.width="100px";                    
>         window.checkLayout("td", document.getElementById("test-output"));
>     }
> </script>  
> <body onload="runTest()">
> 
> 
> By the way, 2 of the tests are already passing without the change, not sure if
> that was intended. Also those are not DOM tests, they check the rendering of
> tables so should be placed into fast/table/.

Thanks Julien for the help. I have modified the test cases and its automated
now.
The vertical writing mode as I mentioned earlier is passing without my changes
so shall I remove it ?
Also since height related test case is already passing to shoudl I change
style()->logicalWidth() to style->width() so that only width related it will go
through the code?

[Julien - ping for review, 2014-03-06 00:33:47]

On 2014/03/05 11:10:47, Gurpreet wrote:
> > You can trigger a layout whenever you want, which should solve your issue
and
> > the need to work-around that by doing a round trip through the event loop:
> > 
> > <script type="text/javascript">
> >     function runTest() {
> >         var col = document.getElementById('thecol');
> > 
> >         // Force the initial layout so that the column width change below
> > triggers a new layout.
> >         col.offsetTop;
> > 
> >         // You can even check that the column width is correct as this is a
> > pre-condition for this test to work.
> > 
> >         col.style.width="100px";                    
> >         window.checkLayout("td", document.getElementById("test-output"));
> >     }
> > </script>  
> > <body onload="runTest()">
> > 
> > 
> > By the way, 2 of the tests are already passing without the change, not sure
if
> > that was intended. Also those are not DOM tests, they check the rendering of
> > tables so should be placed into fast/table/.
> 
> Thanks Julien for the help. I have modified the test cases and its automated
> now.
> The vertical writing mode as I mentioned earlier is passing without my changes
> so shall I remove it ?

No, I still think it's an important case to cover (as your original change
didn't think about the vertical writing-mode case).

> Also since height related test case is already passing to shoudl I change
> style()->logicalWidth() to style->width() so that only width related it will
go
> through the code?

Something else is generating the invalidation which is why I said it would be
nice to know what as it's probably not writing-mode aware. That said, it is
still more correct to check logicalWidth than width so no, this should be kept.

[Julien - ping for review, 2014-03-06 00:54:46]

https://codereview.chromium.org/151993004/diff/280001/Source/core/rendering/R...
File Source/core/rendering/RenderTableCol.cpp (right):

https://codereview.chromium.org/151993004/diff/280001/Source/core/rendering/R...
Source/core/rendering/RenderTableCol.cpp:63: }
This span computation code actually doesn't work, totally missed that in
previous reviews. It only works because both test cases have the exact same
layout:
<colgroup>
   <col></col>
</colgroup>

The following are allowed too and will be mishandled by your code:

<colgroup span="5"/>
<col/>

or

<colgroup>
   <col span="3"/>
   <col/>
</colgroup>

The rule is that <colgroup> is only allowed to have a |span| attribute if it
doesn't have <col> children (see HTML5
http://www.whatwg.org/specs/web-apps/current-work/multipage/tabular-data.html...).

I don't think we have some code to do this computation and I fear that we don't
want to spread such tricky code around (RenderTable already has some column
information but we can't reuse it). My take is to revert back to the original
approach to invalidate all the cells but put a FIXME that explains that we could
do better.

The previous cases should be tested as they would have been broken by this
change.

[Gurpreet, 2014-03-07 07:46:17]

Thanks Julien for your time. I have made the changes as per your suggestion.

[Julien - ping for review, 2014-03-10 18:04:40]

lgtm but don't land without the appropriate testing:
- a combinaison of colgroup / col with span
- several table sections (<thead>, <tbody>, ...)

Why is adding these tests important? Tables are known to be under-tested and
your test are ensuring we don't regress them in the future!

[Gurpreet, 2014-03-11 10:30:45]

The CQ bit was checked by k.gurpreet@samsung.com

[commit-bot: I haz the power, 2014-03-11 10:30:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/k.gurpreet@samsung.com/151993004/320001

[commit-bot: I haz the power, 2014-03-11 11:39:23]

Change committed as 168908