Closed Bug 860242 Opened 7 years ago Closed 7 years ago

Incorrect width for table cells with padding, box-sizing: border-box, and table-layout: fixed

Categories

(Core :: Layout: Tables, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: d.romanet, Assigned: dholbert)

Details

Attachments

(3 files, 2 obsolete files)

Attached file Test case
User Agent: Mozilla/5.0 (Windows NT 6.0; rv:20.0) Gecko/20100101 Firefox/20.0
Build ID: 20130326150557

Steps to reproduce:

I have a div with a display table, a width (500px) and the layout fixed. This div has some children, children are div with display table-cell and the box model is border-box with a padding at 10px.




Actual results:

If the width of children is specified in pixel, the final width of children is good, but if width is specified in percent, the final width of children are different than the width specified in css.


Expected results:

Whenever the width is in pixel or in percent, the width need to be the width specified in css like any other browsers (tested in IE8, IE9, chrome, opera).
Attachment #735707 - Attachment mime type: text/plain → text/html
Mats or Daniel, do you want to take a look?  At first glance, the border-box sizing is ignored for the percentage-width case for some reason...  I wonder whether it's because we end up doing intrinsic sizing or something.
Flags: needinfo?(matspal)
Flags: needinfo?(dholbert)
Attached file testcase 2 (reduced)
Here's a reduced testcase, with just two table cells.

This renders with the two tables looking the same on current versions of IE, Chrome, and Opera, but incorrectly in Firefox nightly (w/ lower light-gray cell being too wide).
Flags: needinfo?(dholbert)
OS: Windows Vista → All
Hardware: x86 → All
Version: 20 Branch → Trunk
It looks like this is at least partly from this chunk of code in FixedTableLayoutStrategy::ComputeColumnWidths:
> 249                 } else if (styleWidth->GetUnit() == eStyleUnit_Percent) {
[...]
> 253                     float pct = styleWidth->GetPercentValue();
> 254                     colWidth = NSToCoordFloor(pct * float(tableWidth)) +
> 255                                offsets.hPadding + offsets.hBorder;
https://mxr.mozilla.org/mozilla-central/source/layout/tables/FixedTableLayoutStrategy.cpp?mark=254-255#249

which doesn't take box-sizing into consideration, when computing the column-width from the cell's "width" value.

(In particular: this quoted code adds border+padding to the cell's resolved "width" value, when determining the width of the column.  It shouldn't add border + padding, though, because the cell's width is already supposed to already include those, given our "box-sizing" value.)
We might just want to avoid adding offsets.hPadding + offsets.hBorder entirely, in that chunk of code...  We don't seem add them in the corresponding chunk of BasicTableLayoutStrategy.cpp:
https://mxr.mozilla.org/mozilla-central/source/layout/tables/BasicTableLayoutStrategy.cpp#749

I tried this in a Try push (mochitests + crashtests + reftests, linux64 & mac64, debug + opt):
 https://tbpl.mozilla.org/?tree=Try&rev=799adb613cf9
and it was all green.

(The relevant patch I pushed to try was: https://hg.mozilla.org/try/rev/665d69260279 )
Attached patch strawman patch v1 (obsolete) — Splinter Review
Here's the patch discussed in comment 4 (which was green on Try).

I intend to write up some testcases to include with it, to sanity-check that it matches other browsers' rendering & matches analogous auto-layout behavior.

I think this matches what the spec calls for, too. The spec says this about fixed table layout:
 #  The width of each column is determined as follows:
 #  1. A column element[...snip, doesn't apply here...]
 #  2. Otherwise, a cell in the first row with a value other
 #     than 'auto' for the 'width' property determines the
 #     width for that column
http://www.w3.org/TR/CSS21/tables.html#fixed-table-layout

That doesn't mention box-sizing, nor does it mention padding. I think it's sensible that we just directly use "width" to figure out the column width, as directed by the spec, rather than adding padding.
Assignee: nobody → dholbert
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(matspal)
Summary: Incorrect width for table cells with padding and box-sizing(border-box) → Incorrect width for table cells with padding, box-sizing(border-box), and table-layout: fixed
Summary: Incorrect width for table cells with padding, box-sizing(border-box), and table-layout: fixed → Incorrect width for table cells with padding, box-sizing: border-box, and table-layout: fixed
This patch just changes the unconditional "offsets.hPadding + offsets.hBorder" addition, quoted in comment 3, such that we only add those things if it's appropriate, per box-sizing.
Attachment #737048 - Attachment is obsolete: true
Attachment #774253 - Flags: review?(dbaron)
(just tweaking a comment in this updated version)

Also -- brief description of the reftest: the key difference between testcase and reference case is percent-valued cell-widths vs. px-valued cell widths.

I also dropped all padding from the reference case, to simplify it, because (if we're doing things right) the padding has no visual effect. (since it's less than the 'width' value.)
Attachment #774253 - Attachment is obsolete: true
Attachment #774253 - Flags: review?(dbaron)
Attachment #774262 - Flags: review?
Attachment #774262 - Flags: review? → review?(dbaron)
Comment on attachment 774262 [details] [diff] [review]
fix v2a: only add border/padding if box-sizing doesn't already include them

>+                    // Even if we didn't explicitly add border + padding to
>+                    // colWidth (e.g. in the NS_STYLE_BOX_SIZING_BORDER case),
>+                    // we need to be sure we're requesting at least enough
>+                    // space to fit them:
>+                    colWidth = std::max(colWidth,
>+                                        offsets.hBorder + offsets.hPadding);

I'm inclined to say you should drop this bit -- I don't see a reason to ensure that a cell in the first row never goes below width:0 but not do the same for cells in later rows.  Though maybe it does make sense to have it, since it's somehow based off of the model of the cell having a computed width that can't be less than zero?  I guess I don't have a strong opinion, but somewhat inclined to drop this chunk.

r=dbaron either way
Attachment #774262 - Flags: review?(dbaron) → review+
Fair enough. I don't feel strongly about that chunk, so I'll drop it. Thanks for the quick review turnaround!
https://hg.mozilla.org/mozilla-central/rev/4c563176b066
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.