Bug 90068 – Setting table layout to fixed causes incorrect cell width calculations

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

Created attachment 149731 [details]
Html page and css file

If you have a table element that have colgroups defined, setting the table's 'table-layout' style attribute to 'fixed' causes the column values to be incorrectly calculate, if the columns where specifed with percentages. What happens is that the cells does not fill the table fully, causing a visible "space" to occur at the right end of the table.  The calculations seem to become more incorrect when more col tags are added.

E.g

<table style="table-layout:fixed">
  <colgroup>
     <col width="9.09%" />
     <col width="9.09%" />
     <col width="9.09%" />
     <col width="9.09%" />
     <col width="9.09%" />
     <col width="9.09%" />
     <col width="9.09%" />
     <col width="9.09%" />
     <col width="9.09%" />
     <col width="9.09%" />
  </colgroup>
  <tbody> 
     <tr>
       <td>
     ...
  </tbody>
</table>

This is crucial to the release of our product.

[Julien Chaffraix]

I don't think I understand your issue. If you meant that there is some empty space on the right side of the table (meaning that we ignore the width=100% on the table), I didn't see that on Chromium 22 Linux or on a recent WebKit-build on Mac.

It's unlikely that this bug is windows specific so could you confirm that you still see the issue on a nightly build?

(In reply to comment #2)
> I don't think I understand your issue. If you meant that there is some empty space on the right side of the table (meaning that we ignore the width=100% on the table), I didn't see that on Chromium 22 Linux or on a recent WebKit-build on Mac.
> 
> It's unlikely that this bug is windows specific so could you confirm that you still see the issue on a nightly build?

Hi,

That is exactly the problem, when columns count up to 100% when table-layout is not fixed, the columns renders correctly, but as soon as I change the precise same HTML/CSS to render with fixed-table layout, does not render the columns properly leaving the space.  This behaviour is not observed in Internet Explorer 9 and FireFox.

Thanks for looking into the issue.

Regards,
Jaco

[Pravin D]

I think there is a issue here.. checked on windows and Qt(r123273) ports and the issue is observed.

1st issue is that the cell sizes in auto table and fixed table layouts differ by 1px(auto layout > fixed layout), especially the initial cells of a row. 

2nd issue tht there is a a white space at the end of table(end of the row containing "ID") for fixed layout... the bg color does not make it easy to see thou :( .

Also if we resize the browser window, the white width varies as fixed layout seems to be jerky when sides of the browser window is dragged using mouse.

[Pravin D]

Created attachment 153654 [details]
Reduction

[Pravin D]

The issue seems to be related to the way we distribute width to the auto cells in Fixed Table rendering algo.
One way of looking at the issue is comparing it with AutoTable rendering algo. In Auto Table rendering algo, col containing only empty cells are not given any width. However in our Fixed Table rendering algo no such distinction is made(cannot be made as only cell in the 1st row are considered). 

The issue happens only when the colspan > number of cols in the table(see the second table in the reduction). So another possible cause can be that we create an empty last column if the colspan > number of cols in the table. Maybe we should no do so... Require clarification on this though.

[Pravin D]

Created attachment 155067 [details]
Patch

[WebKit Review Bot]

Comment on attachment 155067 [details]
Patch

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

New failing tests:
tables/mozilla/bugs/bug69382-2.html
fast/table/colspanMinWidth.html
tables/mozilla/core/misc.html
tables/mozilla/bugs/bug1188.html
tables/mozilla/bugs/bug42443.html
tables/mozilla_expected_failures/bugs/bug1647.html
fast/table/colspanMinWidth-vertical.html
tables/mozilla/bugs/bug69382-1.html

[WebKit Review Bot]

Created attachment 155108 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Julien Chaffraix]

Comment on attachment 155067 [details]
Patch

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

> Source/WebCore/rendering/RenderTableSection.cpp:272
> +        if (m_cCol == nCols) {
> +            if (cSpan > columns[m_cCol].span)
> +                columns[m_cCol].span = cSpan;
> +            break;
> +        }

I don't think it is the right fix: RenderTable::splitColumn updates all sections' representation. Here you override the columns[m_cCol].span *after* splitting which will cause some sections' representation to drift from the table's and will be the cause of super nasty bugs. You could probably see the bug if you had multiple sections (which is a good test to add). I would support adding some ASSERT after splitting / adding columns that all our representations are in sync.

If anything RenderTable::splitColumn should be the one handling that properly so that we propagate the right information to all sections. Also why does auto table layout works correctly whereas fixed table layout doesn't?

[Pravin D]

(In reply to comment #10)
> (From update of attachment 155067 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155067&action=review
> 
> > Source/WebCore/rendering/RenderTableSection.cpp:272
> > +        if (m_cCol == nCols) {
> > +            if (cSpan > columns[m_cCol].span)
> > +                columns[m_cCol].span = cSpan;
> > +            break;
> > +        }
> 
The issue is that we are creating empty column at the end of the table. 
The fix is to prevent the creation of such a column.

> I don't think it is the right fix: RenderTable::splitColumn updates all sections' representation. Here you override the columns[m_cCol].span *after* splitting which will cause some sections' representation to drift from the table's and will be the cause of super nasty bugs. You could probably see the bug if you had multiple sections (which is a good test to add). I would support adding some ASSERT after splitting / adding columns that all our representations are in sync.
> 
I'm not sure... need to check. 

> If anything RenderTable::splitColumn should be the one handling that properly so that we propagate the right information to all sections. Also why does auto table layout works correctly whereas fixed table layout doesn't?
> 
The fixed table layout algo inspects only cells in the first row to determine the columns width. Once the width of all the columns containing fixed and percent cells(first row) are calculated, the remaining width(if any ) is distributed to columns with auto cells . The above issue is that we are having an empty auto column at the end of the table and this column is given some width but not rendered as it does not have any content. Greater the number of percent columns greater the width for the auto columns, due to round off errors. 

Auto table algo does not distribute the extra space to empty columns as it can analysis every cell in a column. However this not the case with fixed table layout.
Maybe there is different approach to solve the issue, but tot it would a good opportunity to also prevent the creation of extra empty columns...

[Julien Chaffraix]

Comment on attachment 155067 [details]
Patch

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

>>> Source/WebCore/rendering/RenderTableSection.cpp:272
>>> +        }
>> 
>> I don't think it is the right fix: RenderTable::splitColumn updates all sections' representation. Here you override the columns[m_cCol].span *after* splitting which will cause some sections' representation to drift from the table's and will be the cause of super nasty bugs. You could probably see the bug if you had multiple sections (which is a good test to add). I would support adding some ASSERT after splitting / adding columns that all our representations are in sync.
>> 
>> If anything RenderTable::splitColumn should be the one handling that properly so that we propagate the right information to all sections. Also why does auto table layout works correctly whereas fixed table layout doesn't?
> 
> The issue is that we are creating empty column at the end of the table. 
> The fix is to prevent the creation of such a column.

Then the fix is even more wrong that I thought. The empty column is required per HTML 4 (see http://www.w3.org/TR/html4/struct/tables.html#h-11.2.4.3):

"The number of columns is equal to the number of columns required by the row with the most columns, including cells that span multiple columns. For any row that has fewer than this number of columns, the end of that row should be padded with empty cells"

To be frank, I am not 100% convinced this is a bug. The HTML is broken in the first place and we are trying to fix a broken page with more undefined behavior. You will have to check what other browsers are doing with the extra column(s) in a variety of situation involving fixed table layout before proceeding.

(In reply to comment #12)
> (From update of attachment 155067 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155067&action=review
> 
> >>> Source/WebCore/rendering/RenderTableSection.cpp:272
> >>> +        }
> >> 
> >> I don't think it is the right fix: RenderTable::splitColumn updates all sections' representation. Here you override the columns[m_cCol].span *after* splitting which will cause some sections' representation to drift from the table's and will be the cause of super nasty bugs. You could probably see the bug if you had multiple sections (which is a good test to add). I would support adding some ASSERT after splitting / adding columns that all our representations are in sync.
> >> 
> >> If anything RenderTable::splitColumn should be the one handling that properly so that we propagate the right information to all sections. Also why does auto table layout works correctly whereas fixed table layout doesn't?
> > 
> > The issue is that we are creating empty column at the end of the table. 
> > The fix is to prevent the creation of such a column.
> 
> Then the fix is even more wrong that I thought. The empty column is required per HTML 4 (see http://www.w3.org/TR/html4/struct/tables.html#h-11.2.4.3):
> 
> "The number of columns is equal to the number of columns required by the row with the most columns, including cells that span multiple columns. For any row that has fewer than this number of columns, the end of that row should be padded with empty cells"
> 
> To be frank, I am not 100% convinced this is a bug. The HTML is broken in the first place and we are trying to fix a broken page with more undefined behavior. You will have to check what other browsers are doing with the extra column(s) in a variety of situation involving fixed table layout before proceeding.

Hi,

I just would like to thank everyone so far looking into this issue.  I just want to notify that if you look at the sample I attached, that the amount of <col/> tags and <td/> tags are the same.  The issue is that when a table is set to table-layout:fixed, that the issue occurs, this can be observed in either Chrome and Safari.  IE 9 and Firefox does not have this issue.

Regards,
Jaco

Hi,

Can someone please tell me if this fix has been added to the release builds ?

Regards,
Jaco

The reason why I am asking as we still have this issue.

[Daniel Bates]

This bug was fixed sometime between Safari 9.0 (11601.1.56) and Safari 10.1.2 (12603.3.8).

[Daniel Bates]

Will add a regression test.

[Daniel Bates]

Created attachment 346661 [details]
Patch

[Daniel Bates]

Comment on attachment 346661 [details]
Patch

Clearing flags on attachment: 346661

Committed r234630: <https://trac.webkit.org/changeset/234630>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43011561>