Bug 14274 - Right border missing from table with colspan and collapsing border
Summary: Right border missing from table with colspan and collapsing border
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
Keywords: HasReduction
Depends on:
Reported: 2007-06-21 00:21 PDT by Michael Wybrow
Modified: 2011-10-03 10:52 PDT (History)
9 users (show)

See Also:

Reduced test case (275 bytes, text/html)
2007-06-21 00:24 PDT, Michael Wybrow
no flags Details
Reduced test case (266 bytes, text/html)
2007-06-21 00:30 PDT, Michael Wybrow
no flags Details
Reduced test case for bugzilla.mozilla.org (278 bytes, text/html)
2008-04-28 09:56 PDT, David Kilzer (:ddkilzer)
no flags Details
Reduced test case for border conflict resolution (316 bytes, text/html)
2009-10-27 14:24 PDT, Oleg Oshmyan
no flags Details
Initial patch, test failures expected (3.59 KB, patch)
2011-09-16 08:27 PDT, Konstantin Shcheglov
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Probably will pass cr-linux tests (74.49 KB, patch)
2011-09-21 13:51 PDT, Konstantin Shcheglov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Wybrow 2007-06-21 00:21:43 PDT
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.
Comment 1 Michael Wybrow 2007-06-21 00:24:18 PDT
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.
Comment 2 Michael Wybrow 2007-06-21 00:30:01 PDT
Created attachment 15156 [details]
Reduced test case

Corrected the test case to match description in original report.
Comment 3 David Kilzer (:ddkilzer) 2008-04-28 09:56:23 PDT
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:

Comment 4 David Kilzer (:ddkilzer) 2008-04-29 08:12:15 PDT
Not a regression as this happens in Safari 2.0.4 on Mac OS X 10.4.11 as well.
Comment 5 mitz 2008-09-29 09:50:15 PDT
See also bug 5515.
Comment 6 David Kilzer (:ddkilzer) 2009-07-05 09:15:50 PDT
This looks pretty bad (on bugs with no attachments) now that bugs.webkit.org has upgraded to a recent Bugzilla.
Comment 7 Oleg Oshmyan 2009-10-27 14:24:48 PDT
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>.
Comment 8 Konstantin Shcheglov 2011-09-15 14:11:45 PDT
I'm working on this bug now.

It seems that there is just incorrect implementation for 

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?
Comment 9 Konstantin Shcheglov 2011-09-16 08:27:00 PDT
Created attachment 107657 [details]
Initial patch, test failures expected
Comment 10 WebKit Review Bot 2011-09-16 22:04:24 PDT
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:
Comment 11 Konstantin Shcheglov 2011-09-21 13:51:42 PDT
Created attachment 108228 [details]
Probably will pass cr-linux tests
Comment 12 Darin Adler 2011-09-21 15:17:19 PDT
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.)
Comment 13 Konstantin Shcheglov 2011-09-22 07:21:49 PDT
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?
Comment 14 Dave Hyatt 2011-09-22 09:40:56 PDT
Comment on attachment 108228 [details]
Probably will pass cr-linux tests

Comment 15 WebKit Review Bot 2011-10-03 09:18:16 PDT
Comment on attachment 108228 [details]
Probably will pass cr-linux tests

Clearing flags on attachment: 108228

Committed r96509: <http://trac.webkit.org/changeset/96509>
Comment 16 WebKit Review Bot 2011-10-03 09:18:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Adam Barth 2011-10-03 10:36:31 PDT
Additional baselines updates in http://trac.webkit.org/changeset/96516.  Please let me know if any of those are in error.
Comment 18 Konstantin Shcheglov 2011-10-03 10:47:02 PDT
Yes, these changes are expected, they reflect same changes in Chromium Linux version.
Comment 19 Adam Barth 2011-10-03 10:52:20 PDT
Thanks for checking!