Closed
Bug 494667
Opened 15 years ago
Closed 15 years ago
there is incorrect gap between cells if the table is border collapsed and a cell colspan > 1
Categories
(Core :: Layout: Tables, defect)
Core
Layout: Tables
Tracking
()
VERIFIED
FIXED
mozilla1.9.2a1
People
(Reporter: masayuki, Assigned: roc)
Details
(Keywords: regression, verified1.9.1)
Attachments
(3 files, 2 obsolete files)
533 bytes,
text/html
|
Details | |
653 bytes,
text/html
|
Details | |
24.22 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
See the attached testcase. The red line is background color of the table and the border of it is collapsed. I think that on border collapsed table, the background color of the table should not be displayed between cells, in other words, there should be no gaps between the cells. Gecko 1.9.0 and later didn't render the gap, so this is a regression of Gecko1.9.1 and later. # the original reporter of this bug finds this bug on 3.5b4 http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=6601
Reporter | ||
Updated•15 years ago
|
Summary: there is incorrect gap between cells if the table is border collapsed and a cell collspan > 1 → there is incorrect gap between cells if the table is border collapsed and a cell colspan > 1
Reporter | ||
Comment 1•15 years ago
|
||
Maybe, the behavior is not defined in CSS2.1 spec, but this bug makes an incompatibility issue between older Fx/other borwsers and Fx3.5 or later.
Comment 2•15 years ago
|
||
This build works: 20080812195635 And this build fails: 20080812212316 Range: http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-08-12+17%3A00%3A00&enddate=2008-08-12+22%3A00%3A00 Did only rough random checks for a red line.
Flags: blocking1.9.2?
Flags: blocking1.9.1?
looks like bug 449324 from the regression range
Assignee | ||
Comment 4•15 years ago
|
||
OK, this bug is happening because the table's top cell is being given a bottom-border of 2px in GetStyleBorder()->mBorder --- and actually using that for layout, effectively --- but nsStyleBorder::SetBorderWidth leavesGetStyleBorder()->mComputedBorder at 0 because the border style is still NS_STYLE_BORDER_STYLE_NONE.
Assignee | ||
Comment 5•15 years ago
|
||
This patch fixes the bug by ensuring that if a cell with a border style of NONE is actually assigned a nonzero border width, we change the style to SOLID (with a transparent color) for the purposes of background drawing, so border invariants aren't violated.
Assignee: nobody → roc
Attachment #379513 -
Flags: review?(dbaron)
Assignee | ||
Updated•15 years ago
|
Attachment #379513 -
Flags: review?(bernd_mozilla)
Assignee | ||
Comment 6•15 years ago
|
||
Comment on attachment 379513 [details] [diff] [review] fix Bernd could review this if he gets here first.
Assignee | ||
Comment 7•15 years ago
|
||
I don't think we should actually stop the release on this, but it would certainly be a good low-risk fix to take if we can.
Flags: blocking1.9.1? → wanted1.9.1+
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs review]
Reporter | ||
Comment 8•15 years ago
|
||
roc: you posted a video patch.
Assignee | ||
Comment 9•15 years ago
|
||
Oops, right patch.
Attachment #379513 -
Attachment is obsolete: true
Attachment #379514 -
Flags: review?(dbaron)
Attachment #379513 -
Flags: review?(dbaron)
Attachment #379513 -
Flags: review?(bernd_mozilla)
Assignee | ||
Updated•15 years ago
|
Attachment #379514 -
Flags: review?(bernd_mozilla)
Comment 10•15 years ago
|
||
Does that alter the style that we will retrieve when we next time do a border collapse computation? If so this will create more problems than it solves.
Comment 11•15 years ago
|
||
the more I think about the more I am convinced that this is wrong, the border specified on a given cell can only hint what what might be the border just imagine a colspan, two cells from below specify a border one 10 px tall solid green, the other 4px tall dashed and the colspan cell has border style:none on its bottom border, to do this right one would have look up all border styles that form the corresponding border. I think for bc tables the clipping is just wrong, the background should extend from one edge to the other and not be clipped by the border that might be drawn later over it (thats the pink dashed line at http://www.w3.org/TR/CSS2/tables.html#collapsing-borders) And fantasai should weight in, she is *the* one to ask if backgrounds are concerned.
Comment 12•15 years ago
|
||
this illustrates my dislike of this.
Comment 13•15 years ago
|
||
it shows red after the patch has been applied.
Assignee | ||
Comment 14•15 years ago
|
||
(In reply to comment #10) > Does that alter the style that we will retrieve when we next time do a border > collapse computation? If so this will create more problems than it solves. No, this is just temporary style data used only for when painting the cell background. But I think you're right. I'll do this differently.
Assignee | ||
Comment 15•15 years ago
|
||
I guess dbaron should review this version. This patch does two things: -- We pass a flag into PaintBackground/PaintBackgroundWithSC to indicate when the frame's borders are going to be painted normally (via nsDisplayBorder) right on top of the background. We only do the solid border optimization when that flag is passed in. This fixes the bug. I considered adding a new method to nsIFrame to indicate if the frame draws normal borders, but I think this approach is more elegant since we determine it automatically. Note that it's possible for nsDisplayBorder to be removed by the display list optimizer when the invalid area is entirely inside the borders. In that case, the isSolidBorder optimization will be disabled by this patch. But that's OK because the dirty area will be inside the border area and background painting will be optimized in other ways. -- I was going to make aUsePrintSettings be a separate flag, but I realized we don't actually need it, we can just call HonorPrintBackgroundSettings from PaintBackgroundWithSC and have that method return false for nsGroupBoxFrame and nsHTMLButtonFrame, the two places we pass false for aUsePrintSettings.
Attachment #379514 -
Attachment is obsolete: true
Attachment #379615 -
Flags: review?(dbaron)
Attachment #379514 -
Flags: review?(dbaron)
Attachment #379514 -
Flags: review?(bernd_mozilla)
Comment 16•15 years ago
|
||
+ enum { + /** + * When this flag is passed, the element's nsDisplayBorder will be + * painted immediately on top of this background. + */ + PAINT_WILL_PAINT_BORDER = 0x01 + }; It's not clear to me from this comment what exactly this flag means or why it exists. Nitpicky: *tableData.mBorder, - PR_TRUE); + 0); and aDirtyRect, rect, *backg, *border, - PR_TRUE); + 0); probably don't need a separate line for 0);
Assignee | ||
Comment 17•15 years ago
|
||
(In reply to comment #16) > + enum { > + /** > + * When this flag is passed, the element's nsDisplayBorder will be > + * painted immediately on top of this background. > + */ > + PAINT_WILL_PAINT_BORDER = 0x01 > + }; > > It's not clear to me from this comment what exactly this flag means or why it > exists. It means we promise to paint the borders on top of this background with nothing in between. Does that make sense?
Assignee | ||
Updated•15 years ago
|
Attachment #379615 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 18•15 years ago
|
||
Comment on attachment 379615 [details] [diff] [review] fix v2 Boris could review this.
Comment 19•15 years ago
|
||
Comment on attachment 379615 [details] [diff] [review] fix v2 r+sr=bzbarsky, but if that testcase doesn't test the issue bernd's testcase does, please add one that does?
Attachment #379615 -
Flags: superreview+
Attachment #379615 -
Flags: review?(dbaron)
Attachment #379615 -
Flags: review?(bzbarsky)
Attachment #379615 -
Flags: review+
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs review] → [needs 191 approval]
Assignee | ||
Updated•15 years ago
|
Attachment #379615 -
Flags: approval1.9.1?
Assignee | ||
Comment 20•15 years ago
|
||
Comment on attachment 379615 [details] [diff] [review] fix v2 Fixes a layout regression. Low risk well understood fix.
Updated•15 years ago
|
Attachment #379615 -
Flags: approval1.9.1? → approval1.9.1+
Comment 21•15 years ago
|
||
Comment on attachment 379615 [details] [diff] [review] fix v2 a191=beltzner
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs 191 approval] → [needs 191 landing]
Assignee | ||
Comment 22•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/7c4cf9d4af34
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: blocking1.9.2?
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Flags: in-testsuite+
Assignee | ||
Comment 23•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f06ccca1ccae The test 494667-2.html doesn't pass on 1.9.1, since it requires the fix for bug 155955, which is trunk-only, so I marked it failing on 1.9.1.
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
Comment 24•15 years ago
|
||
Verified fixed on trunk and 1.9.1 with builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090604 Minefield/3.6a1pre ID:20090604031228 Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090604 Shiretoko/3.5pre ID:20090604031153
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
OS: Windows Vista → All
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.2a1
You need to log in
before you can comment on or make changes to this bug.
Description
•