Closed Bug 444928 Opened 12 years ago Closed 12 years ago
fixed layout: cellspacing with colspans wrong
523 bytes, text/html
502 bytes, text/html
322 bytes, text/html
467 bytes, text/html
1.34 KB, text/html; charset=UTF-8
4.93 KB, patch
|Details | Diff | Splinter Review|
23.50 KB, patch
|Details | Diff | Splinter Review|
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?
Summary: Regression from FF2: TH w/ colspan does not span width of table → fixed layout: TH w/ colspan does not span width of table
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
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
This shows that IE treats auto and fixed differently.
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
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.
(And reftests do pass this time.)
And another crash fix of the same form.
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+
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
sorry for the late r+ on the cleanup but you should have pinged me.
Landed cleanup: http://hg.mozilla.org/mozilla-central/rev/5254e429712d
You need to log in before you can comment on or make changes to this bug.