Bug 69425 – display: table-cell always uses content-box sizing to calculate height, even when box-sizing: border-box

• Component: Tables
• Status: RESOLVED FIXED

[Nicholas Shanks]

Created attachment 109791 [details]
reduced test case

The height of a table cell with both a height and vertical padding is calculated incorrectly, adding the two together instead of just using the height (cells are always border-box). Changing the box-sizing property or setting a max-height has no effect.

This is unrelated to bug #13339, bug #18565 or bug #37685 all of which relate to widths of cells in fixed layout tables.

In Aurora 9 and Opera Next 12, both tables in the attached test case are 100px high. In WebKit Nightlies and Chrome Canary it is 152px high.

[Dominic Cooney]

Created attachment 109927 [details]
Patch

[Dominic Cooney]

Comment on attachment 109927 [details]
Patch

Bouncing this off bots.

[Nicholas Shanks]

Dominic, thanks for the quick patch, but the problem is that Firefox and Opera, in strict mode, still calculate cell sizes using the border-box model, even if box-sizing is set to content-box.

[Dominic Cooney]

(All of my comments below relate to strict mode. I didn’t investigate quirks mode.)

Your comment about Firefox and Opera doesn’t jive with what I see.

I noticed Firefox (7.0.1 and Minefield 10.0a1 2011-10-05) seems to ignore -moz-box-sizing/box-sizing for display: table-cell elements. FF uses something like the content-box calculation for cell width, and border-box calculation for cell height.

Opera 11.51’s default and box-sizing: content-box behavior matches Firefox’s (invariant) behavior. box-sizing: border-box matches the patch I have posted here.

It is possible there are multiple WebKit bugs here:

1. WebKit applies box-sizing: border-box of display: table-cell elements to the width, but not the height, of the element.

2. WebKit’s box-sizing: content-box sizing of display: table-cell elements might be wrong, because it doesn’t match that of Firefox or Opera, that is, WebKit uses something like the border-box calculation for cell width, and content-box calculation for cell height.

[Dominic Cooney]

Two more data points:

IE9 display: table-cell with box-sizing: content-box matches WebKit ToT.

IE9 display: table-cell with box-sizing: border-box matches Opera and the patch I have posted here.

[Nicholas Shanks]

(In reply to comment #4)
> FF uses something like the content-box calculation for cell width, and border-box calculation for cell height.
> 2. WebKit’s box-sizing: content-box sizing of display: table-cell elements might be wrong, because it doesn’t match that of Firefox or Opera, that is, WebKit uses something like the border-box calculation for cell width, and content-box calculation for cell height.

I did not test how width was handled (there are many other bugs for that, as cited earlier). All of my previous comments were related to height only, i.e. when I said:

> the problem is that Firefox and Opera, in strict mode, still calculate cell sizes using the border-box model, even if box-sizing is set to content-box.

I meant to say:

> the problem is that Firefox and Opera, in strict mode, still calculate cell *heights* using the border-box model, even if box-sizing is set to content-box.

From your testing, it looks like Firefox is the odd one out with border-box cell width. IE, Opera & WKToT with your patch all now agree on heights. Width differences can be covered in a separate bug.


Do you think this patch needs some sort of website impact review before landing? It's pretty fundamental to be changing how table cell sizes are calculated, even if our current state is so clearly wrong.

[Dominic Cooney]

(In reply to comment #6)
> I did not test how width was handled (there are many other bugs for that, as cited earlier). All of my previous comments were related to height only

Yes! I see now.

> From your testing, it looks like Firefox is the odd one out with border-box cell width. IE, Opera & WKToT with your patch all now agree on heights. Width differences can be covered in a separate bug.

The fact that Firefox doesn’t change the size of the box between content-box and border-box looks like a bug on their part.

> Do you think this patch needs some sort of website impact review before landing? It's pretty fundamental to be changing how table cell sizes are calculated, even if our current state is so clearly wrong.

Actually I think that this is relatively safe from a site compatibility perspective, first because it brings WebKit into line with Firefox, Opera and IE9; second because it only changes the height of display: table-cell box-sizing: border-box elements. Most table cells are box-sizing: content-box.

[Nicholas Shanks]

(In reply to comment #7)

> The fact that Firefox doesn’t change the size of the box between content-box and border-box looks like a bug on their part.

I was about to file it, but it looks like they have several serious bugs with border-box (open for 6 years). Seems like Tantek is at least aware of one of them:

"table-cell height calculation uses border-box sizing; should use content-box"
https://bugzilla.mozilla.org/show_bug.cgi?id=295315

"min-height/max-height does not work for box-sizing:border-box"
https://bugzilla.mozilla.org/show_bug.cgi?id=308801

> second because it only changes the height of display: table-cell box-sizing: border-box elements.

... with non-zero vertical padding and a fixed height. Okay, maybe you're right :-)

[Nicholas Shanks]

Oh, I meant to add: are you intending to address the max-height issue with a second patch on this bug, or would you like me to file another bug?

[Dominic Cooney]

(In reply to comment #9)
> Oh, I meant to add: are you intending to address the max-height issue with a second patch on this bug, or would you like me to file another bug?

Usual practice in WebKit is to file separate bugs. Please CC me on that one, although I don’t know table layout well, so I will probably wait and see what a reviewer says about this one first.

Comment on attachment 109927 [details]
Patch

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

> Source/WebCore/rendering/RenderTableSection.cpp:373
> +                // In strict mode, box-sizing: content-box do the
> +                // right thing and actually add in the border and
> +                // padding.

This comment can fit on two lines (or maybe even one line).

> Source/WebCore/rendering/RenderTableSection.cpp:376
> +                ch += (adjustedPaddingBefore + adjustedPaddingAfter + cell->borderBefore() + cell->borderAfter());

The parentheses seem unnecessary.

[Dominic Cooney]

Committed r98997: <http://trac.webkit.org/changeset/98997>