there is incorrect gap between cells if the table is border collapsed and a cell colspan > 1

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
Layout: Tables
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: masayuki, Assigned: roc)

Tracking

({regression, verified1.9.1})

Trunk
mozilla1.9.2a1
regression, verified1.9.1
Points:
---
Bug Flags:
wanted1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Created attachment 379422 [details]
testcase

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
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
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.
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?

Comment 3

9 years ago
looks like bug 449324 from the regression range
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.
Created attachment 379513 [details] [diff] [review]
fix

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)
Attachment #379513 - Flags: review?(bernd_mozilla)
Comment on attachment 379513 [details] [diff] [review]
fix

Bernd could review this if he gets here first.
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+
Whiteboard: [needs review]
roc: you posted a video patch.
Created attachment 379514 [details] [diff] [review]
fix

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)
Attachment #379514 - Flags: review?(bernd_mozilla)

Comment 10

9 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

9 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

9 years ago
Created attachment 379578 [details]
testcase that circumentvents the patch

this illustrates my dislike of this.

Comment 13

9 years ago
it shows red after the patch has been applied.
(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.
Created attachment 379615 [details] [diff] [review]
fix v2

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

9 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);
(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?
Comment on attachment 379615 [details] [diff] [review]
fix v2

Boris could review this.
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+
Whiteboard: [needs review] → [needs 191 approval]
Comment on attachment 379615 [details] [diff] [review]
fix v2

Fixes a layout regression. Low risk well understood fix.
Attachment #379615 - Flags: approval1.9.1? → approval1.9.1+
Whiteboard: [needs 191 approval] → [needs 191 landing]
http://hg.mozilla.org/mozilla-central/rev/7c4cf9d4af34
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: blocking1.9.2?
Resolution: --- → FIXED
Flags: in-testsuite+
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]
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.