Closed Bug 444928 Opened 12 years ago Closed 12 years ago

fixed layout: cellspacing with colspans wrong

Categories

(Core :: Layout: Tables, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1b1

People

(Reporter: gurney_j, Assigned: dbaron)

References

Details

(Keywords: regression, testcase)

Attachments

(7 files, 5 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0

With a table that contains 181 columns, a single th in a tr w/ a colspan of 181 does not span the entire width of the table.  The first column is 10%, the rest of the columns have their widths not specified (or specified as 1, does not matter).  I will attach a test case.  The dark blue should extend to the end of of the page w/ a very minor light blue edge.

Using firebug, the 10% width appears to be properly cacluted, and the remaining 180 columns do cover the remaining of the table, but it appears that the first 10% column is counting for more than a single column.

Reproducible: Always

Steps to Reproduce:
1.  Load the attached html pge into FireFox 3.
Actual Results:  
The th's contents do not cover the entire width of the table, leaving a large block of light blue to the right.

Expected Results:  
The th's contents should cover the entire width of the table as there are only 181 columns in the document, not more than that.
FireFox 2 did not have this rendering problem.  It rendered as expected.  Also tested in IE7.0.5730.13 and it renders correctly also.
Version: unspecified → 3.0 Branch
Component: General → Layout: Tables
Product: Firefox → Core
QA Contact: general → layout.tables
Version: 3.0 Branch → 1.9.0 Branch
Ria, I assume you found out this is a regression from bug 300030, by finding out the regression range, right?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Keywords: regression, testcase
Version: 1.9.0 Branch → Trunk
Summary: Regression from FF2: TH w/ colspan does not span width of table → fixed layout: TH w/ colspan does not span width of table
Attached file more obvious testcase
this is cellspacing issue, probably related to the columns that do not have a cell originating at it.
Summary: fixed layout: TH w/ colspan does not span width of table → fixed layout: cellspacing with colspans wrong
the regression range would make sense as at this time also the fixed layout algorithm was completely rewritten.
Yeah, this basically needs an equivalent to attachment 249039 [details] [diff] [review] for the fixed layout strategy.
Will this really consistently work? I thought that only the first row defines what has to happen. As soon as we have this GetNumCellsOriginatingInCol we have the other rows defining what shall happen, this might require a relayout of the table as new rows arrive.
Assignee: nobody → dbaron
OS: Windows XP → All
Hardware: PC → All
Attachment #339068 - Attachment description: more obvius testcase → more obvious testcase
Ah, good point.  So we probably want to just assume for fixed layout that all columns have cells originating in them.  This means that we'd need to change code outside of the layout strategy to put the cellspacing between columns when the table is fixed layout, even if there's no cells originating in the column.

Does that seem reasonable?

(I wonder if that's what IE does...)
(In reply to comment #10)
> (I wonder if that's what IE does...)

In fixed-layout tables, IE7 doesn't seem to care whether columns have cells originating in them or not, as far as I can tell.

However, it does have a bug that if you specify a width on a column-spanning cell, it doesn't subtract the spanned spacing when distributing that width to the columns.  I don't think we need to emulate that, though.
it looks like IE adds a cellspacing as we do for every virtual column regardless of fixed or auto layout. However we do differently between auto and fixed.
Webkit does what we do for auto also for fixed layout
Attached file another example (obsolete) —
It depends what you're testing.  Attachment 339191 [details] is about intrinsic widths too, I think, and IE has some internal inconsistencies there (as we do, for fixed layout, hence this bug).

This attachment is a case where IE does behave differently between fixed and auto layout.
Comment on attachment 339299 [details]
another example

Er, actually, never mind; this just shows the part of IE's behavior that I *don't* want to emulate.
Attachment #339299 - Attachment is obsolete: true
Attached file another example (obsolete) —
This shows that IE treats auto and fixed differently.
Attached file another example
And here's a version of that with widths on the tables so it triggers fixed layout in Gecko, too.
Attachment #339302 - Attachment is obsolete: true
Attachment #339349 - Flags: review?(bernd_mozilla)
I noticed CalcAvailWidth could use a good bit of cleanup; both outputs were always the same, and it seems more complex than needed in other ways.
Attachment #339350 - Flags: review?(bernd_mozilla)
Oops, used mTableLayoutStrategy instead of LayoutStrategy(), resulting in null dereference running reftests.
Attachment #339349 - Attachment is obsolete: true
Attachment #339364 - Flags: review?(bernd_mozilla)
Attachment #339349 - Flags: review?(bernd_mozilla)
Attachment #339364 - Attachment is obsolete: true
Attachment #339365 - Flags: review?(bernd_mozilla)
Attachment #339364 - Flags: review?(bernd_mozilla)
(And reftests do pass this time.)
And another crash fix of the same form.
Attachment #339365 - Attachment is obsolete: true
Attachment #339608 - Flags: review?(bernd_mozilla)
Attachment #339365 - Flags: review?(bernd_mozilla)
Comment on attachment 339608 [details] [diff] [review]
patch to always consider cell spacing for fixed layout

looks good,
Attachment #339608 - Flags: review?(bernd_mozilla) → review+
Attachment #339608 - Flags: superreview?(roc)
Attachment #339608 - Flags: superreview?(roc) → superreview+
Flags: wanted1.9.1?
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Fixed: http://hg.mozilla.org/mozilla-central/rev/2192a846d7ac
... though I'd still like to land the above cleanup.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
Attachment #339350 - Flags: review?(bernd_mozilla) → review+
sorry for the late r+ on the cleanup but you should have pinged me.
You need to log in before you can comment on or make changes to this bug.