Bug 3297 – height property is not honored on table rows

• Component: Tables
• Status: RESOLVED FIXED

[Omar Kilani]

The CSS height property is ignored on table rows.

Test cases:

1. height applied to tr elements.

http://aurore.net/safari/tr.html

2. height applied to td elements.

http://aurore.net/safari/td.html

Results:

Safari misrenders the table if height is applied to tr elements. It generally
ignores the height property. Sometimes the first row gets the correct height,
and all other rows get auto height.

Works fine if height is applied to td elements.

[Matias Larsson]

Created attachment 2147 [details]
Another testcase

[Matias Larsson]

Same behaviour on Mac OS X 10.3.9 / Safari 1.3 (v312)

[Alexey Proskuryakov]

Uploaded yet another test case to <http://nypop.com/~ap/webkit/rowheight.html>. Safari, Firefox and 
WinIE all render it differently:
- Firefox always honors the height (Opera 8.50 does the same);
- WinIE sets the height to 1 if the row is empty, honors it otherwise.

The test case includes a rather weird usage from www.fenzin.ru - <tr height=9><td><img src="images/
spacer.gif" border=0 hspace=0 vspace=0 width=1 height=6 align=left></td></tr> (why specify a 
different height for the spacer image itself?).

[Eric Seidel (no email)]

Still a bug in TOT.

[Nicholas Shanks]

(In reply to comment #3)
> why specify a different height for the spacer image itself?

Perhaps the other three pixels are intercell spacing?
Perhaps the web developer can't tell the difference between 6 and 9?

The main culprit here is in RenderTableSection::calcRowHeight() at RenderTableSection.cpp:293:

        int pos = rowPos[r + 1] + ch + spacing;

should really be

        int pos = rowPos[r] + ch + spacing;

That change alone fixes a few of the table layout tests, but breaks a few others, so there's further tweaking that needs to be done beyond that.

Created attachment 8437 [details]
Patch

Created attachment 8516 [details]
Patch, including change log and updated layout test results

Note that the patch doesn't include updated pixel results (or checksums).

[Dave Hyatt]

Comment on attachment 8516 [details]
Patch, including change log and updated layout test results

r=me... did you go over the tests to make sure the new renderings are all correct?

(In reply to comment #9)
> (From update of attachment 8516 [details] [edit])
> r=me... did you go over the tests to make sure the new renderings are all
> correct?
> 

Yes :-)
There is one test that the patch will break if applied before the patch for bug 9012. (I didn't include the broken results in the patch).