734569 - The width of table cells are incorrect when colspan specified

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Gerry Roberts]

User Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; WOW64; Trident/4.0; GTB7.3; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; eSobiSubscriber 2.0.4.16; InfoPath.2; .NET4.0C; .NET4.0E)

Steps to reproduce:

Created the following HTML table:

<table border="1" width="734" cellspacing="0" cellpadding="0"><tr>
<td width="265">a</td>
<td width="469" colspan="2">b</td>
</tr><tr>
<td width="367" colspan="2">c</td>
<td width="367">d</td>
</tr></table>



Actual results:

The table cells were rendered with incorrect widths.  The width of the cells in row 1 are the same as those in row 2.


Expected results:

The table is valid HTML 4.01, so the width of the cells in row 1 should be different than those in row two.  However, Firefox is unable to determine the correct widths even though there is enough information given for it to do so.  In this case, IE 8 was able to get it right, but not Firefox v10.0.2 (and earlier versions too).

Note, if you change all the widths to percent, Firefox is able to render the table correctly.

Comment 1

[Gerry Roberts]

The table has three columns with widths of 265, 102, and 367.  The 102 is calculated by the browser using the other widths.  The second cell in the first row spans across columns 102 and 367 resulting in a width of 469.  The first cell in the second row spans across columns 265 and 102 resulting in a width of 367.  The table cells should look something like this:
------------------------
|        |              |
|-----------------------
|             |         |
------------------------

Comment 2

[Boris Zbarsky [:bzbarsky]]

Opera is rendering this the same way as we are.  WebKit is rendering more like the diagram in comment 1.  I don't have IE to test with right this second.  David, thoughts?

Comment 3

[Alice0775 White]

IE10 Customer Preview10.0.8250.0 (Win8CPx64) is  like the diagram in comment 1.
IE9 9.08112.16421 (Win7x64)is the same as Firefox.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yeah, when we distribute the width of a cell with a colspan, we could probably make allocating to a column with currently 0 width but no specified width a little easier.  This would probably require BasicTableLayoutStrategy::DistributeWidthToColumns to distinguish between the BTLS_FINAL_WIDTH case (where its behavior should stay the same) and the other cases (where we should change the behavior).  We'd need to work out what the correct distribution priorities for those cases should be, though.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Given that all non-WebKit browsers agree on this there's also the site compat question....

Comment 6

[Gerry Roberts]

Site compat may not be a major issue here since browsers seem to be rendering it both ways.  On the other hand, I don't see how an interpretation that produces 4 cells of equal widths is HTML 4.01 compliant.  Since using percentages renders the table as expected, there's a case to be made for maintaining consistency as well.  One would not expect that changing from percentages to absolutes would render a different looking table.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: reporter's testcase

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I think the fix here is to just make the fix to bug 413286 apply to colspan distribution as well.  I'm not sure why it didn't when it was written; reading bug 413286 up through bug 413286 comment 11 doesn't tell me.

The DistributeWidthToColumns is used for two different things:
  (1) given a final table width, compute the widths of the columns
  (2) within computation of a preferred or minimum intrinsic width, how should the preferred or minimum width of a cell be distributed to the columns it spans (based on the widths already assigned to those columns as a result of cells with smaller colspans)

The fix for bug 413286 was conditioned on the call being of type (1); I'm not sure why.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

work in progress: https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/d18dc250d3d7/distribute-intrinsic-to-empty-columns

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Distribute the width of column-spanning cells to columns with nothing in them, like we do for final table widths. ()

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] from comment #8)
> So I think the fix here is to just make the fix to bug 413286 apply to
> colspan distribution as well.  I'm not sure why it didn't when it was
> written; reading bug 413286 up through bug 413286 comment 11 doesn't tell me.

I'm trying to remember why we made it final-only.  Your note in bug 413286 comment #26 hints at one possible reason -- it might've been for compatibility -- quoting that comment:
> Might want to comment that this is for final width only.  (It's sort of too
> bad that it has to be, actually, but nobody else does it for span
> distribution.)

Per comment 2 & 3 here, both Opera & IE < 10 match the current Firefox behavior (and who knows, maybe webkit used to as well), so that might be what you meant by "nobody else does it for span distribution."

Comment 12

[Daniel Holbert [:dholbert]]

(continuing trying to figure out why bug 413286 was BTLS_FINAL_WIDTH-specific)

It looks like my first patch in bug 413286 (in bug 413286 comment 11) was BTLS_FINAL_WIDTH-specific only because I was building up some state related to the spans ("SpanContainsPrefWidth"), and then distributing the width based on that state -- and it only made sense to use that state in the FINAL step, after we'd processed the colspans.  I think.

Anyway, seeing as the "SpanContainsPrefWidth" bit didn't survive to the final patch, I don't see any reason to keep this restricted to the FINAL step anymore.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 656483 [details] [diff] [review]
Distribute the width of column-spanning cells to columns with nothing in them, like we do for final table widths.  ()

Looks great! Just one thought on the tests:

>+++ b/layout/reftests/table-width/reftest.list
>+== colspan-distribute-to-empty-1.html colspan-distribute-to-empty-1-ref.html
>+== colspan-distribute-to-empty-2.html colspan-distribute-to-empty-1-ref.html

It seems a little strange to have foo-2.html use foo-1-ref.html as its reference case (different number).  For someone listing the files in the directory, it looks at first like we're missing foo-2-ref.html, and it's not clear why until you read the manifest file.

Personally, I prefer the naming-scheme "foo-1a.html", "foo-1b.html", etc. (all of which use "foo-1-ref.html" as their reference) -- that makes it a bit clearer, IMHO.  But the way you've got it is good, too.

r=me

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ead7ec99cea

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ead7ec99cea

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Added one more test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f240265df77

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8f240265df77

Comment 18

[Scoobidiver (away)]

Backed out from Beta and Aurora to fix bug 810586:
http://hg.mozilla.org/releases/mozilla-beta/rev/931590387ec9
http://hg.mozilla.org/releases/mozilla-aurora/rev/1313ee568d60