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

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: 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

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

[Ria Klaassen (not reading all bugmail)]

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.

Comment 3

[Bernd]

looks like bug 449324 from the regression range

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 379513 [details] [diff] [review]
fix

Bernd could review this if he gets here first.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

roc: you posted a video patch.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Oops, right patch.

Comment 10

[Bernd]

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

[Bernd]

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

[Bernd]

Attachment: testcase that circumentvents the patch

this illustrates my dislike of this.

Comment 13

[Bernd]

it shows red after the patch has been applied.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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.

Comment 16

[fantasai]

+  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);

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 379615 [details] [diff] [review]
fix v2

Boris could review this.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 379615 [details] [diff] [review]
fix v2

Fixes a layout regression. Low risk well understood fix.

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 379615 [details] [diff] [review]
fix v2

a191=beltzner

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/7c4cf9d4af34

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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