Bug 6838 – Incomplete repaint of collapsed table borders

• Component: Layout and Rendering
• Status: RESOLVED FIXED

See the attached testcase. All tables in the testcase have border-collapse: collapse and border: 8px solid. The Test buttons make changes that require some or all of the table's border to be repainted, yet only the inner 4 pixels paint. To force repaint, switch to another tab and go back.

Created attachment 5987 [details]
Testcase

Created attachment 5990 [details]
Basis for a possible fix

I'm not sure this is the right approach. If it is, then the margin overflow dimensions should probably be cached by the table and the FIXMEs should be addressed.

[Darin Adler]

Comment on attachment 5990 [details]
Basis for a possible fix

I'm disappointed we have to add so much new code, but it looks pretty good to me. Is there any way to share more of this? Should we invent a structure that holds the four separate widths so it can be a single function instead of 4?

I believe you forgot to mark the functions virtual.

+    const BorderValue sb = style()->borderRight();

We normally don't mark local variables const.

+    return int(borderWidth / 2.0 + 0.5);

Can be done more efficiently by doing (borderWidth + 1) / 2. This idiom comes up multiple times.

I've run into another problem: if you Select All in the testcase, the selection highlight eats into the last table's border. (I wonder why that table's contents aren't selected btw).

[Dave Hyatt]

Note that our implementation of collapsed table borders no longer matches the spec.  The spec actually changed.

"UAs must compute an initial left and right border width for the table by examining the first and last cells in the first row of the table. The left border width of the table is half of the first cell's collapsed left border, and the right border width of the table is half of the last cell's collapsed right border. If subsequent rows have larger collapsed left and right borders, then any excess spills into the margin area of the table.

The top border width of the table is computed by examining all cells who collapse their top borders with the top border of the table. The top border width of the table is equal to half of the maximum collapsed top border. The bottom border width is computed by examining all cells whose bottom borders collapse with the bottom of the table. The bottom border width is equal to half of the maximum collapsed bottom border."

So top/bottom collapsed border overflow is no longer possible, and left/right overflow becomes extremely unlikely with these changes.

Going to try again.

Created attachment 7701 [details]
Patch

This patch implements the updated spec. Some notes:
1. It might be better to cache outerBorderLeft/Right/Top/Bottom in RenderTable as well.
2. The spec says "CSS 2.1 does not define where the edge of a background on a table element lies" but I tried to maintain the current behavior. It works with solid backgrounds but image backgrounds are positioned differently, and I am not sure how to fix it.
3. The patch causes 43 layout tests to fail. With the exception of pixel failures due to (2) above, the new layouts look correct to me.
4. Some failures are due to the dump showing "[border: none]" due to the fact that a table has a non-zero border but its own border-width property is zero.

[Dave Hyatt]

Comment on attachment 7701 [details]
Patch

This looks good to me.

[Dave Hyatt]

Can you provide a testcase that illustrates an example where the image background changes? (Maybe include screenshots).  We should probably understand that different before we land anything.

[Dave Hyatt]

difference. :)

Created attachment 7797 [details]
Table background test case

Created attachment 7798 [details]
Current background positioning

Created attachment 7799 [details]
Background positioning with the patch

(In reply to comment #9)
> Can you provide a testcase that illustrates an example where the image
> background changes? (Maybe include screenshots).  We should probably understand
> that different before we land anything.

The difference is clear. I just don't know if paintBackground or paintBackgroundExtended can do what I want them to do.

[Dave Hyatt]

Comment on attachment 7701 [details]
Patch

Discussing the image positioning issue some more on IRC right now. :)

[Dave Hyatt]

Check WinIE to study what they do.  We may need a quirk.  Painting in the table's border box in strict mode seems like the right thing to do.  It won't match anyone else probably because nobody else has implemented this new spec.

Created attachment 7855 [details]
Another background test case

Created attachment 7856 [details]
WinIE rendering

Created attachment 7857 [details]
Firefox rendering

Created attachment 7858 [details]
TOT WebKit rendering

Created attachment 7859 [details]
Patched WebKit rendering - painting in the table's border box

This is with the patch (attachment 7701 [details]) minus these lines:
+    if (collapseBorders()) {
+        _tx += borderLeft();
+        w -= borderLeft() + borderRight();
+        _ty += borderTop();
+        h -= borderTop() + borderBottom();
+    }

Neither browser rendered the table differently w/o the DOCTYPE.

Created attachment 8094 [details]
Patch including change log and repaint test

Changed background positioning (by removing the 6 lines mentioned in the previous comment). I reviewed layout/pixel test failures from this patch and they all look like the new results should be accepted.

[Dave Hyatt]

Comment on attachment 8094 [details]
Patch including change log and repaint test

r=me

[Darin Adler]

Committed revision 14372.

The fix for this bug caused bug 9009.