Bug 74955 – WebKit adds vertical paddings and borders to the fixed width of CSS tables

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

[Max Vujovic]

Created attachment 120078 [details]
Reproduction

In the attached reproduction, the CSS table has a width style of 100px and a padding-top style of 400px. In WebKit, the table's computed width is 500px (100px+400px), but it should be 100px. Firefox and Opera produce the correct results.

[Max Vujovic]

Created attachment 120087 [details]
Patch

I uploaded patch that fixes the issue and makes WebKit's behavior match Firefox's and Opera's.

The problem was that the width calculation for CSS tables was using borderBefore(), paddingBefore(), borderAfter(), and paddingAfter(), which actually correspond to the vertical borders and paddings. I changed it to use borderStart(), paddingStart(), borderEnd(), and paddingEnd(), which correspond to the horizontal borders and paddings.

[Eric Seidel (no email)]

This seems reasonable to me, but really Beth/hyatt are more familiar with this code.

[Max Vujovic]

Thanks for looking at it, Eric. I was planning to ping Julien for a review when he comes on IRC since he changed some code around there recently. Of course, Beth or Hyatt are welcome to review if they have time.

[Julien Chaffraix]

Comment on attachment 120087 [details]
Patch

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

Good catch! Some comments but the change is very fine.

> LayoutTests/fast/table/script-tests/css-table-width.js:3
> +description(
> +"This test checks that the width style is applied correctly to CSS tables with respect to table paddings and borders."
> +);

You can put that on one line.

> LayoutTests/fast/table/script-tests/css-table-width.js:40
> +shouldEvaluateTo("computeCSSTableWidth('width: 200px; border-style: solid; border-width: 10px 2px 30px 4px; padding: 50px 6px 70px 8px;')", 200+2+4+6+8);
> +shouldEvaluateTo("computeCSSTableWidth('width: 200px; border-style: solid; border-width: 10px 2px 30px 4px; padding: 50px 6px 70px 8px; border-collapse: collapse;')", 200 + (2+4)/2);

I would love to see some examples with a vertical writing mode (like 'writing-mode: vertical-rl') and some values of 'text-orientation' / 'direction' as this test feels not sufficient. Also a vertical writing mode would have shown the issue pretty easily (just put some padding in the inline base direction but not in the block flow direction).

> Source/WebCore/rendering/RenderTable.cpp:233
> +        LayoutUnit bordersAndPadding = 0;

I am not too fond of the naming. You don't include the paddings if you are on a collapsing border CSS table.

I couldn't find anything better (my take was bordersAndPaddingsOnNonCollapsingBordersTable which is too long). The variable name should at least be bordersAndPaddings for consistency.

[Max Vujovic]

Thanks for the review, Julien!

> > LayoutTests/fast/table/script-tests/css-table-width.js:3
> > +description(
> > +"This test checks that the width style is applied correctly to CSS tables with respect to table paddings and borders."
> > +);
> 
> You can put that on one line.

Sounds good.

> I would love to see some examples with a vertical writing mode (like 'writing-mode: vertical-rl') and some values of 'text-orientation' / 'direction' as this test feels not sufficient. Also a vertical writing mode would have shown the issue pretty easily (just put some padding in the inline base direction but not in the block flow direction).

Very good idea- I'll craft some more tests and reupload a patch.

> > Source/WebCore/rendering/RenderTable.cpp:233
> > +        LayoutUnit bordersAndPadding = 0;
> 
> I am not too fond of the naming. You don't include the paddings if you are on a collapsing border CSS table.
> 
> I couldn't find anything better (my take was bordersAndPaddingsOnNonCollapsingBordersTable which is too long). The variable name should at least be bordersAndPaddings for consistency.

Good point. Both "borders" and "bordersAndPaddings" aren't entirely right, but "borders" sounds better. I'll change it back to "borders" :)

[Max Vujovic]

Created attachment 121122 [details]
Patch

I've updated the patch with the changes from the previous comment. I've added more test cases with different combinations of writing-mode, direction, and text-orientation.

[Julien Chaffraix]

Comment on attachment 121122 [details]
Patch

The cr-linux EWS seems to be cycling and failing on some other issue. The previous change properly passed the bot so it should be fine to land this one. r = me

[WebKit Review Bot]

Comment on attachment 121122 [details]
Patch

Clearing flags on attachment: 121122

Committed r104140: <http://trac.webkit.org/changeset/104140>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.