REGRESSION(r130774): Width of table having table-layout: fixed and max-width is set is not calculated properly

• Component: Tables
• Status: RESOLVED DUPLICATE

[Julien Chaffraix]

Following r130774, it is possible for a table to not fit its content anymore. CSS 2.1 is clear on that:

"The width of the table is then the greater of the value of the 'width' property for the table element and the sum of the column widths (plus cell spacing or borders). If the table is wider than the columns, the extra space should be distributed over the columns."

See the URL for a reproduction.

[Pravin D]

(In reply to comment #0)
> Following r130774, it is possible for a table to not fit its content anymore. CSS 2.1 is clear on that:
> 
> "The width of the table is then the greater of the value of the 'width' property for the table element and the sum of the column widths (plus cell spacing or borders). If the table is wider than the columns, the extra space should be distributed over the columns."
> 
> See the URL for a reproduction.
> 

About the issue: max-width property should be ignored only in case of fixed table layout. Currently in FixedTableLayout class does not handle the same.

PS: In JSfiddle  max-width:100% is for all tables due to the css dumped by twitter(http://twitter.github.com/bootstrap/assets/css/bootstrap.css). 
Updated the Jsfiddle to reflect all the contents to repro the issue locally. 
http://jsfiddle.net/TgdpN/3/

[Pravin D]

Created attachment 188096 [details]
Patch

[Pravin D]

Created attachment 188114 [details]
Patch

[Pravin D]

About the bug... 
As pointed out by Julien, width of table using Fixed-table-layout is max(logicalWidth, Sum of Col widths).
The logical width for tables is calculated as follows (only relevent point):
computedWidth = max(styleLogicalWidth, minPreferredWidth)
logical width = min(computedWidth, max-width);

The issues arises as we restrict the logical width to max-width, which is suppose to ignored in case of Fixed-table-layout.
This should be handled in layout() of Fixed-table-layout class.

Currently in FixedTableLayout::layout(), when the calculated table width is greater than the above calculated logical width, columns with percent width are shrunk to accommodate adjust the calculated width. If this does not help, no further action is taken. The fix should be the the logical width be modified.

Patch 1 
https://bugs.webkit.org/attachment.cgi?id=188096
In this patch the percent columns are not allowed to shrink. As the css2.1 spec does not say anything in this regards. Only the logical width is updated. So we tend to get a much bigger table when it contains a mix of percent and fixed col width. However this is different from FF behavior.

Patch 2
https://bugs.webkit.org/attachment.cgi?id=188114
In this patch the percent colums are allowed to shrink as much as possible. If still the new calculated width is greated than the logical width, then the logical width is updated to the new calculated width. Also the patch contains a check to make sure that while shrinking the percent cols, their widths donot become negative.
Same behavior as FF.

[Build Bot]

Comment on attachment 188096 [details]
Patch

Attachment 188096 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16559011

New failing tests:
tables/mozilla_expected_failures/bugs/bug7243.html
fast/table/css-table-max-width.html

[Build Bot]

Comment on attachment 188114 [details]
Patch

Attachment 188114 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16538218

New failing tests:
http/tests/inspector/resource-tree/resource-request-content-while-loading.html

[Julien Chaffraix]

Comment on attachment 188114 [details]
Patch

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

I don't think you are fixing the right place. At line 256, there is an 'if (!numAuto || totalWidth > tableLogicalWidth) {'. This 'if' looks suspicious to me as I don't see how both clauses are related.

I would be better with the following change:
* make sure tableLogicalWidth can accomodate totalWidth (probably renamed totalColumnContentWidths).
* after the if (...) else (...) (or at the end) always write tableLogicalWidth back to the table.

> Source/WebCore/ChangeLog:3
> +        REGRESSION(r130774): table isn't sized after content

Please rename the bug to something more explicit. I filed it without really understanding the issue.

> Source/WebCore/rendering/FixedTableLayout.cpp:274
> +                        calcWidth[i] = m_width[i].percent() * max(0, (tableLogicalWidth - totalFixedWidth)) / totalPercent;

One test for that seems too low. Could you have more testing to this to ensure we don't regress other cases. I would like at least:
* several percentage.
* mix of fixed, percentage + sometimes auto.

> LayoutTests/fast/table/css-table-max-width-expected.txt:17
> +PASS maxWidthZeroFixedLayout.getBoundingClientRect().width is 2

Would be nice to have an explanation for the 2 (basically the default padding).

[Pravin D]

Created attachment 188357 [details]
Patch

[WebKit Review Bot]

Comment on attachment 188357 [details]
Patch

Attachment 188357 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16546699

New failing tests:
tables/mozilla/bugs/bug43039.html

[Build Bot]

Comment on attachment 188357 [details]
Patch

Attachment 188357 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16537891

New failing tests:
tables/mozilla/bugs/bug43039.html

[Pravin D]

Created attachment 188507 [details]
Patch

[Julien Chaffraix]

Comment on attachment 188507 [details]
Patch

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

> Source/WebCore/rendering/FixedTableLayout.cpp:255
> +    int totalColumnContentWidths = totalFixedWidth + totalPercentWidth;

This is where I would just do:

tableLogicalWidth = std::max(tableLogicalWidth, totalColumnContentWidths);

This is really what we need to do as we want to ensure that the table width is at least the column's content width. That would remove the need for the 'if' check + the max(0, ...) you added in the loop.

> Source/WebCore/rendering/FixedTableLayout.cpp:256
> +    if (!numAuto || totalColumnContentWidths > tableLogicalWidth) {

This is the 'if' I mentioned as wrong (or at least I don't fully understand why we mix !numAuto (about the intrinsic table format) and totalColumnContentWidths > tableLogicalWidth (layout information).

[Pravin D]

(In reply to comment #12)
> (From update of attachment 188507 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188507&action=review
> 
> > Source/WebCore/rendering/FixedTableLayout.cpp:255
> > +    int totalColumnContentWidths = totalFixedWidth + totalPercentWidth;
> 
> This is where I would just do:
> 
> tableLogicalWidth = std::max(tableLogicalWidth, totalColumnContentWidths);
> 
Updating the tableLogicalWidth here would mean that we would need another temporary variable to store the diff b/w tableLogicalWidth(orig) and totalColumnContentWidths so that the fixed and percent colms be adjusted accordingly.
Also IMHO tableLogicalWidth should not be modified until all width distributions are complete. It should be modified only when the calculated width is greater than the current logical width and the width distribution algo has not other way to accommodate the excess width.

> This is really what we need to do as we want to ensure that the table width is at least the column's content width. That would remove the need for the 'if' check + the max(0, ...) you added in the loop.
> 
max(0, ...) would be needed as comment ^^

> > Source/WebCore/rendering/FixedTableLayout.cpp:256
> > +    if (!numAuto || totalColumnContentWidths > tableLogicalWidth) {
> 
> This is the 'if' I mentioned as wrong (or at least I don't fully understand why we mix !numAuto (about the intrinsic table format) and totalColumnContentWidths > tableLogicalWidth (layout information).
> 

Have created bug to refactor the code for readability
see https://bugs.webkit.org/show_bug.cgi?id=110180

[Darin Adler]

Comment on attachment 188507 [details]
Patch

Patch is old enough that we need a new debased one if this problem still exists. Review- for now.

[Darin Adler]

rebased

[Noam Rosenthal]

I think that this is a duplicate of https://bugs.webkit.org/show_bug.cgi?id=115156, and has nothing to do with table-layout: fixed.

[Noam Rosenthal]

The patch in https://bugs.webkit.org/show_bug.cgi?id=115156 is verified to fix the test-case URL (http://jsfiddle.net/TgdpN/6/), and in a more succinct way that doesn't have anything to do with table-layout: fixed (this issue exists for all tables with max-width/width). 

Marking as duplicate, feel free to reopen if I overlooked something.

*** This bug has been marked as a duplicate of bug 115156 ***