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

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

In the example on the attached URL, a simple table with one column and one row is used. The width of this table is controlled by the width of the single col element. If the col width is dynamically updated to a wider width than the original width, the table resizes just fine, but if the col width is updated to a narrower width than the original width, the table will not get narrower than the original width.

The expected behavior is that the table width would shrink as well as grow as the col width changes. 

The same behavior was observed with both an auto and fixed layout table.

This behavior was observed in Chrome 23.0.1271.97 and Safari 5.1.7 on Windows 7 as well as Chrome 23.0.1271.97 and Safari 6.0.2 on OS X 10.8.
The expected behavior was observed in Firefox 17.0.1, IE 9.0.8112.16421 and Opera 12.11.1661 on Windows 7 as well as Firefox 17.0.1 on OS X 10.8.

Created attachment 212119 [details]
Patch

[Build Bot]

Comment on attachment 212119 [details]
Patch

Attachment 212119 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1925265

New failing tests:
tables/mozilla/bugs/bug113235-1.html
fast/text/international/bidi-layout-across-linebreak.html
fast/table/border-collapsing/003.html
fast/repaint/table-cell-collapsed-border.html
fast/table/border-collapsing/003-vertical.html
tables/mozilla_expected_failures/marvin/x_colgroup_width_px.xml
tables/mozilla/bugs/bug14159-1.html

[Build Bot]

Created attachment 212124 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 212119 [details]
Patch

Attachment 212119 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2015072

New failing tests:
tables/mozilla/bugs/bug113235-1.html
fast/text/international/bidi-layout-across-linebreak.html
fast/table/border-collapsing/003.html
fast/repaint/table-cell-collapsed-border.html
fast/table/border-collapsing/003-vertical.html
tables/mozilla_expected_failures/marvin/x_colgroup_width_px.xml
tables/mozilla/bugs/bug14159-1.html

[Build Bot]

Created attachment 212125 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

Created attachment 212330 [details]
Patch

[Build Bot]

Comment on attachment 212330 [details]
Patch

Attachment 212330 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2083098

New failing tests:
css3/selectors3/xhtml/css3-modsel-74b.xml
css3/selectors3/xhtml/css3-modsel-28b.xml
fast/css/percentage-non-integer.html
css3/selectors3/xml/css3-modsel-28b.xml
fast/dynamic/create-renderer-for-whitespace-only-text.html
fast/block/positioning/053.html
accessibility/table-cells.html
css3/selectors3/xml/css3-modsel-28.xml
fast/css/css3-nth-child.html
css3/selectors3/html/css3-modsel-28b.html
css3/selectors3/xml/css3-modsel-74.xml
css3/selectors3/html/css3-modsel-18b.html
css3/selectors3/xml/css3-modsel-18b.xml
fast/css/acid2-pixel.html
css3/selectors3/xhtml/css3-modsel-18.xml
css3/selectors3/html/css3-modsel-28.html
css3/selectors3/xhtml/css3-modsel-18b.xml
css3/selectors3/html/css3-modsel-18.html
fast/css/acid2.html
css3/selectors3/html/css3-modsel-74.html
css3/selectors3/xml/css3-modsel-74b.xml
fast/dynamic/insert-before-table-part-in-continuation.html
fast/dom/HTMLTableElement/colSpan.html
css3/selectors3/xhtml/css3-modsel-74.xml
fast/forms/basic-buttons.html
editing/pasteboard/paste-table-001.html
css3/selectors3/html/css3-modsel-74b.html
css3/selectors3/xhtml/css3-modsel-28.xml
css3/selectors3/xml/css3-modsel-18.xml
css2.1/20110323/table-caption-002.htm

[Build Bot]

Created attachment 212333 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 212330 [details]
Patch

Attachment 212330 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2095062

New failing tests:
css3/selectors3/xhtml/css3-modsel-74b.xml
css3/selectors3/xhtml/css3-modsel-28b.xml
fast/css/percentage-non-integer.html
css3/selectors3/xml/css3-modsel-28b.xml
fast/dynamic/create-renderer-for-whitespace-only-text.html
fast/block/positioning/053.html
accessibility/table-cells.html
css3/selectors3/xml/css3-modsel-28.xml
fast/css/css3-nth-child.html
css3/selectors3/html/css3-modsel-28b.html
css3/selectors3/xml/css3-modsel-74.xml
css3/selectors3/html/css3-modsel-18b.html
css3/selectors3/xml/css3-modsel-18b.xml
fast/css/acid2-pixel.html
css3/selectors3/xhtml/css3-modsel-18.xml
css3/selectors3/html/css3-modsel-28.html
css3/selectors3/xhtml/css3-modsel-18b.xml
css3/selectors3/html/css3-modsel-18.html
fast/css/acid2.html
css3/selectors3/html/css3-modsel-74.html
css3/selectors3/xml/css3-modsel-74b.xml
fast/dynamic/insert-before-table-part-in-continuation.html
fast/dom/HTMLTableElement/colSpan.html
css3/selectors3/xhtml/css3-modsel-74.xml
fast/forms/basic-buttons.html
editing/pasteboard/paste-table-001.html
css3/selectors3/html/css3-modsel-74b.html
css3/selectors3/xhtml/css3-modsel-28.xml
css3/selectors3/xml/css3-modsel-18.xml
css2.1/20110323/table-caption-002.htm

[Build Bot]

Created attachment 212334 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

Hi Julien. I am working on this issue but had one doubt. 
As per the content initial width is set to 200px. When the width changes the table width is not set to the new values because

void AutoTableLayout::recalcColumn(unsigned effCol)

if (cell->maxPreferredLogicalWidth() > columnLayout.maxLogicalWidth) {
      columnLayout.maxLogicalWidth = cell->maxPreferredLogicalWidth();
      maxContributor = cell;
}
cell->maxPreferredLogicalWidth is still 200 and columnLayout.maxLogicalWidth is the new for example 180 (reduce by 20px) and since the condition doesnot satisfy the new width 180px is not set to columnLayout.maxLogicalWidth.

So is it better to do some workaround for this in void AutoTableLayout::recalcColumn(unsigned effCol) or calculates the preflogicalwidths again?

I tried the first way but some test cases are failing

Please suggest.

(In reply to comment #11)
> Hi Julien. I am working on this issue but had one doubt. 
> As per the content initial width is set to 200px. When the width changes the table width is not set to the new values because
> 
> void AutoTableLayout::recalcColumn(unsigned effCol)
> 
> if (cell->maxPreferredLogicalWidth() > columnLayout.maxLogicalWidth) {
>       columnLayout.maxLogicalWidth = cell->maxPreferredLogicalWidth();
>       maxContributor = cell;
> }
> cell->maxPreferredLogicalWidth is still 200 and columnLayout.maxLogicalWidth is the new for example 180 (reduce by 20px) and since the condition doesnot satisfy the new width 180px is not set to columnLayout.maxLogicalWidth.
> 
> So is it better to do some workaround for this in void AutoTableLayout::recalcColumn(unsigned effCol) or calculates the preflogicalwidths again?
> 
> I tried the first way but some test cases are failing
> 
> Please suggest.

Someone Please suggest.

Created attachment 212997 [details]
Patch

[Build Bot]

Comment on attachment 212997 [details]
Patch

Attachment 212997 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2695168

New failing tests:
tables/mozilla/marvin/body_col.html
fast/table/021.html
fast/text/international/bidi-layout-across-linebreak.html
tables/mozilla/core/one_row.html
tables/mozilla/other/body_col.html
fast/table/border-collapsing/cached-change-col-border-width.html
tables/mozilla_expected_failures/bugs/bug14007-2.html
fast/table/border-collapsing/cached-cell-remove.html
tables/mozilla_expected_failures/marvin/x_colgroup_width_px.xml
tables/mozilla_expected_failures/core/col_span2.html
fast/table/border-collapsing/cached-change-colgroup-border-width.html
fast/table/border-collapsing/cached-change-cell-border-width.html
fast/table/border-collapsing/cached-cell-append.html
fast/table/border-collapsing/cached-change-table-border-width.html

[Build Bot]

Created attachment 213011 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 212997 [details]
Patch

Attachment 212997 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2816101

New failing tests:
tables/mozilla/marvin/body_col.html
fast/table/021.html
fast/text/international/bidi-layout-across-linebreak.html
tables/mozilla/core/one_row.html
tables/mozilla/other/body_col.html
fast/table/border-collapsing/cached-change-col-border-width.html
tables/mozilla_expected_failures/bugs/bug14007-2.html
fast/table/border-collapsing/cached-cell-remove.html
tables/mozilla_expected_failures/marvin/x_colgroup_width_px.xml
tables/mozilla_expected_failures/core/col_span2.html
fast/table/border-collapsing/cached-change-colgroup-border-width.html
fast/table/border-collapsing/cached-change-cell-border-width.html
fast/table/border-collapsing/cached-cell-append.html
fast/table/border-collapsing/cached-change-table-border-width.html

[Build Bot]

Created attachment 213013 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 212997 [details]
Patch

Attachment 212997 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2720226

New failing tests:
tables/mozilla/marvin/body_col.html
fast/table/021.html
fast/text/international/bidi-layout-across-linebreak.html
tables/mozilla/core/one_row.html
tables/mozilla/other/body_col.html
fast/table/border-collapsing/cached-change-col-border-width.html
tables/mozilla_expected_failures/bugs/bug14007-2.html
fast/table/border-collapsing/cached-cell-remove.html
tables/mozilla_expected_failures/marvin/x_colgroup_width_px.xml
tables/mozilla_expected_failures/core/col_span2.html
fast/table/border-collapsing/cached-change-colgroup-border-width.html
fast/table/border-collapsing/cached-change-cell-border-width.html
fast/table/border-collapsing/cached-cell-append.html
fast/table/border-collapsing/cached-change-table-border-width.html

[Build Bot]

Created attachment 213017 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

Created attachment 213155 [details]
Patch

(In reply to comment #20)
> Created an attachment (id=213155) [details]
> Patch

Can someone please review this?

(In reply to comment #21)
> (In reply to comment #20)
> > Created an attachment (id=213155) [details] [details]
> > Patch
> 
> Can someone please review this?

Hi. Can someone please review this. Thanks

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > Created an attachment (id=213155) [details] [details] [details]
> > > Patch
> > 
> > Can someone please review this?
> 
> Hi. Can someone please review this. Thanks

Please review.Thanks

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > (In reply to comment #20)
> > > > Created an attachment (id=213155) [details] [details] [details] [details]
> > > > Patch
> > > 
> > > Can someone please review this?
> > 
> > Hi. Can someone please review this. Thanks
> 
> Please review.Thanks

Hi Simon. Can you please review this patch?

[Antti Koivisto]

Comment on attachment 213155 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213155&action=review

> Source/WebCore/rendering/AutoTableLayout.cpp:85
>                      columnLayout.minLogicalWidth = max<int>(cell->minPreferredLogicalWidth(), columnLayout.minLogicalWidth);
> +
> +                    Length cellLogicalWidth = cell->styleOrColLogicalWidth();
> +                    if (cellLogicalWidth.isFixed() && cell->style()->logicalWidth().isAuto()) {
> +                        int logicalWidth = cell->adjustBorderBoxLogicalWidthForBoxSizing(cellLogicalWidth.value());
> +                        if (cellLogicalWidth.value() > cell->minPreferredLogicalWidth() && cell->maxPreferredLogicalWidth() != logicalWidth)
> +                            cell->setPreferredLogicalWidthsDirty(true);
> +                    }

Actually this looks wrong. The code saves minPreferredLogicalWidth to columnLayout.minLogicalWidth and then marks logical width dirty. 

Also I suspect the invalidation should be done when the col width changes rather than during recalcColumn (which happens during layout).

Created attachment 221123 [details]
Patch

[Build Bot]

Comment on attachment 221123 [details]
Patch

Attachment 221123 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6538699684708352

New failing tests:
fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html

[Build Bot]

Created attachment 221135 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 221123 [details]
Patch

Attachment 221123 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5507948201639936

New failing tests:
fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html

[Build Bot]

Created attachment 221136 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 221123 [details]
Patch

Attachment 221123 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5231997626613760

New failing tests:
fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html

[Build Bot]

Created attachment 221138 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

Created attachment 221143 [details]
Patch

[Build Bot]

Comment on attachment 221143 [details]
Patch

Attachment 221143 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5729575493435392

New failing tests:
fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html

[Build Bot]

Created attachment 221148 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 221143 [details]
Patch

Attachment 221143 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5217174083862528

New failing tests:
fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html

[Build Bot]

Created attachment 221154 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 221143 [details]
Patch

Attachment 221143 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4810267405844480

New failing tests:
fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html

[Build Bot]

Created attachment 221157 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

Created attachment 221227 [details]
Patch

[Antti Koivisto]

Comment on attachment 221227 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221227&action=review

> Source/WebCore/rendering/RenderTableCol.cpp:58
> +            unsigned nEffCols = table->numEffCols();
> +            for (unsigned j = 0; j < nEffCols; j++) {

Ideally this would be optimized by figuring out which columns this RenderTableCol actually affects. This may not be critical to do now as animating <col> width is likely rare.

> Source/WebCore/rendering/RenderTableCol.cpp:62
> +                for (RenderObject* child = table->firstChild(); child; child = child->nextSibling()) {
> +                    if (child->isTableSection()) {
> +                        RenderTableSection* section = toRenderTableSection(child);
> +                        unsigned numRows = section->numRows();

You should use C++11 and child iterators here:

for (auto& section : childrenOfType<RenderTableSection>(*table)) { 
    unsigned rowCount = section.numRows()
    ....

> Source/WebCore/rendering/RenderTableCol.cpp:66
> +                            RenderTableSection::CellStruct current = section->cellAt(i, j);
> +                            RenderTableCell* cell = current.primaryCell();
> +                            cell->setPreferredLogicalWidthsDirty(true);

You can just use section.primaryCellAt(i, j)

Are you sure the cell can't be null? Looking around it is always null tested in similar cases.

Created attachment 221526 [details]
Patch

Created attachment 221608 [details]
Patch

[Build Bot]

Comment on attachment 221608 [details]
Patch

Attachment 221608 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5820269700579328

New failing tests:
fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html

[Build Bot]

Created attachment 221609 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 221608 [details]
Patch

Attachment 221608 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5459725986562048

New failing tests:
fast/dom/HTMLTableColElement/resize-table-width-using-col-width.html

[Build Bot]

Created attachment 221612 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

Created attachment 221613 [details]
Patch

[Antti Koivisto]

Comment on attachment 221613 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221613&action=review

> Source/WebCore/rendering/RenderTableCol.cpp:63
> +            unsigned nEffCols = table->numEffCols();
> +            for (unsigned j = 0; j < nEffCols; j++) {
> +                for (auto& section : childrenOfType<RenderTableSection>(*table)) {
> +                    unsigned rowCount = section.numRows();
> +                    for (unsigned i = 0; i < rowCount; i++) {
> +                        RenderTableCell* cell = section.primaryCellAt(i, j);

It is bit strange (and inefficient) to iterate over columns and then over sections. Could you make the section loop the outermost one?

(In reply to comment #49)
> (From update of attachment 221613 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221613&action=review
> 
> > Source/WebCore/rendering/RenderTableCol.cpp:63
> > +            unsigned nEffCols = table->numEffCols();
> > +            for (unsigned j = 0; j < nEffCols; j++) {
> > +                for (auto& section : childrenOfType<RenderTableSection>(*table)) {
> > +                    unsigned rowCount = section.numRows();
> > +                    for (unsigned i = 0; i < rowCount; i++) {
> > +                        RenderTableCell* cell = section.primaryCellAt(i, j);
> 
> It is bit strange (and inefficient) to iterate over columns and then over sections. Could you make the section loop the outermost one?

Just one query in void AutoTableLayout::fullRecalc() it recalcColumn which internally gets the cells from section. Something similiar I was trying to do.

Created attachment 221633 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 221633 [details]
Patch

Clearing flags on attachment: 221633

Committed r162334: <http://trac.webkit.org/changeset/162334>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

(In reply to comment #52)
> (From update of attachment 221633 [details])
> Clearing flags on attachment: 221633
> 
> Committed r162334: <http://trac.webkit.org/changeset/162334>

This caused a regression fixed in r165837:
<http://trac.webkit.org/changeset/165837>