Bug 14274 – Right border missing from table with colspan and collapsing border

• Component: Tables
• Status: RESOLVED FIXED

[Michael Wybrow]

This bug was noticed in Safari 3 beta and is also present in WebKit nightly r23682.

The problem occurs for a table element with a border that is using the collapsing border model.
If the number of columns in the table is determined by a single td element in each row, each with the same colspan attribute of a value greater than 3, then the right border on the table does not show up.  For example if the table contents were:
<tr><td colspan="3">Cell one</td></tr>
<tr><td colspan="3">Cell two</td></tr>

Incidently, if colspan is 2 on both those lines instead, then it displays correctly.

[Michael Wybrow]

Created attachment 15155 [details]
Reduced test case

This is a reduced test case showing the problem.  The table should show a border on four sides instead of just three.

[Michael Wybrow]

Created attachment 15156 [details]
Reduced test case

Corrected the test case to match description in original report.

[David Kilzer (:ddkilzer)]

Created attachment 20868 [details]
Reduced test case for bugzilla.mozilla.org

This bug also affects default Bugzilla styling on <https://bugzilla.mozilla.org/> when rendering the "Attachments" table with no attachments:

https://bugzilla.mozilla.org/show_bug.cgi?id=431052

[David Kilzer (:ddkilzer)]

Not a regression as this happens in Safari 2.0.4 on Mac OS X 10.4.11 as well.

See also bug 5515.

[David Kilzer (:ddkilzer)]

This looks pretty bad (on bugs with no attachments) now that bugs.webkit.org has upgraded to a recent Bugzilla.

[Oleg Oshmyan]

Created attachment 41987 [details]
Reduced test case for border conflict resolution

This bug also manifests itself when border conflicts need to be resolved. As shown in the attached reduced test case, the thin border set on <td colspan=3> wins over the thick border set on the enclosing <tr>.

[Konstantin Shcheglov]

I'm working on this bug now.

It seems that there is just incorrect implementation for 
RenderTable::colToEffCol().

Second problem is that that I don't understand why there is
  m_columns.fill(ColumnStruct(), 1);
in constructor of RenderTable. It causes for example that in this case we have 2 effective columns instead of one. This is BTW reason why this bug is reproducable with colSpan=3, but not with colSpan=2.
So, I'm going to remove this extra column from constructor.

Unfortunately this breaks several tests, some of which as platform specific.
However I don't see any good reason to have these tests actually platform specific and going to rewrite them using Ahem font.

Can anyone comment these ideas?

[Konstantin Shcheglov]

Created attachment 107657 [details]
Initial patch, test failures expected

[WebKit Review Bot]

Comment on attachment 107657 [details]
Initial patch, test failures expected

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

New failing tests:
platform/chromium/fast/text/text-stroke-with-border.html
fast/table/inline-form-assert.html
scrollbars/disabled-scrollbar.html
fast/invalid/018.html
scrollbars/scrollbar-buttons.html
fast/invalid/table-residual-style-crash.html
fast/invalid/017.html
scrollbars/listbox-scrollbar-combinations.html
platform/mac-snowleopard/platform/mac/fast/text/international/Geeza-Pro-vertical-metrics-adjustment.html
fast/invalid/residual-style.html
scrollbars/overflow-scrollbar-combinations.html
scrollbars/basic-scrollbar.html
fast/invalid/020.xml
fast/forms/file-input-disabled.html
fast/invalid/table-inside-stray-table-content.html

[Konstantin Shcheglov]

Created attachment 108228 [details]
Probably will pass cr-linux tests

[Darin Adler]

Comment on attachment 108228 [details]
Probably will pass cr-linux tests

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

> Source/WebCore/rendering/RenderTable.h:164
> +        for (unsigned c = 0; effCol < numCols && c + m_columns[effCol].span - 1 < col; ++effCol)

What prevents overflow in the "c + span - 1" expression?

> Source/WebCore/rendering/RenderTable.h:165
> +            c += m_columns[effCol].span;

What prevents overflow in this addition? (I know it’s not new.)

[Konstantin Shcheglov]

Nothing prevents overflow here and in other places.
Actually once you set span a little greater than 1 billion (actually >= 2^30) we get overflow somewhere else (may be in appendColumn()) and span value in m_columns becomes invalid.


So yes, there is problem.
I'd say that all cells locations arithmetic should be done using unsigned just because this seems logical. But even in this case there still place for overflow.

I'm not sure how to solve this, except of may be adding some limitation on "span" value and number of columns, like maxColSpan=10000 and maxColCount=100000. Alternative approach is using longer number, or even number with unlimited precision. But I don't think that we should go so far.

In any case, this sounds as separate enhancement, unrelated with this bug.
Do you want me open new one?

[Dave Hyatt]

Comment on attachment 108228 [details]
Probably will pass cr-linux tests

r=me

[WebKit Review Bot]

Comment on attachment 108228 [details]
Probably will pass cr-linux tests

Clearing flags on attachment: 108228

Committed r96509: <http://trac.webkit.org/changeset/96509>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

Additional baselines updates in http://trac.webkit.org/changeset/96516.  Please let me know if any of those are in error.

[Konstantin Shcheglov]

Yes, these changes are expected, they reflect same changes in Chromium Linux version.

[Adam Barth]

Thanks for checking!