127040 - Defining background-color for TD destroys border-collapse: collapse border

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect, P2)

Description

[Tarmo Järvalt]

IE 5.5 SP2 renders it correctly, Gecko/20020221 doesn't.

<TABLE style="background-color: green; border-collapse: collapse; border-color:
blue; border-style: solid; border-width: 2px; ">
<TR>
<TD style="background-color: red; border-color: blue; border-width: 2px;
border-style: solid; ">asdf</TD>
<TD style="border-color: blue; border-width: 2px; border-style: solid; ">qwer</TD>
</TR>
<TR>
<TD style="background-color: red; border-color: blue; border-width: 2px;
border-style: solid; ">fdsa</TD>
<TD style="background-color: red; border-color: blue; border-width: 2px;
border-style: solid; ">rewq</TD>
</TR>
<TR>
<TD style="border-color: blue; border-width: 2px; border-style: solid; ">zxcv</TD>
<TD style="border-color: blue; border-width: 2px; border-style: solid; ">uiop</TD>
</TR>
</TABLE>

Comment 1

[Bernd]

Attachment: testcase from bug

Comment 2

[karnaze (gone)]

Attachment: patch to fix the bug

Comment 3

[Bernd]

Comment on attachment 70863 [details] [diff] [review]
patch to fix the bug

r=bernd

Comment 4

[karnaze (gone)]

*** Bug 127174 has been marked as a duplicate of this bug. ***

Comment 5

[Kevin McCluskey (gone)]

Marking nsbeta1+

Comment 6

[Alexandru Savulov]

Comment on attachment 70863 [details] [diff] [review]
patch to fix the bug

r= alexsavulov

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is this really want you want for dotted/dashed/transparent borders?

Comment 8

[karnaze (gone)]

dbaron, good point. I did consider having the table paint the collapsed borders 
after the backgrounds of the cells and not change the recs of the cells. In that 
case if there was any background overflow outside the cells, then the borders 
would overwrite some of that. But I guess that isn't a very common case. So, 
I'll submit another patch later today where painting happens in this order - 
background on table, backgrounds on cells, borders on everything. 

Comment 9

[karnaze (gone)]

I guess all requirements can be satisfied by painting in this order - table 
background, backgrounds of the cells (excluding backgrounds/borders of elements 
inside the cells), collapsed borders, backgrounds/borders of elements inside the 
cells.

Comment 10

[karnaze (gone)]

Attachment: new patch

Paints in the order outlined in comment #9.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 71104 [details] [diff] [review]
new patch

>+      if ((tableFrame->IsBorderCollapse() && (aFlags == BORDER_COLLAPSE_BACKGROUNDS_PASS1)) ||
>+          !tableFrame->IsBorderCollapse()) {
>+        if (!GetContentEmpty() || 
>+            (NS_STYLE_TABLE_EMPTY_CELLS_SHOW            == cellTableStyle->mEmptyCells) || 
>+            (NS_STYLE_TABLE_EMPTY_CELLS_SHOW_BACKGROUND == cellTableStyle->mEmptyCells)) {

This could be simplified a bit (note the single call to
|tableFrame->IsBorderCollapse()|):

if ((!tableFrame->IsBorderCollapse() || (aFlags ==
BORDER_COLLAPSE_BACKGROUNDS_PASS1)) &&
    (!GetContentEmpty() ||
     NS_STYLE_TABLE_EMPTY_CELLS_SHOW		== cellTableStyle->mEmptyCells
|| 
     NS_STYLE_TABLE_EMPTY_CELLS_SHOW_BACKGROUND ==
cellTableStyle->mEmptyCells)) {


>+        const nsStyleTableBorder* tableStyle;
>+        tableFrame->GetStyleData(eStyleStruct_TableBorder, ((const nsStyleStruct *&)tableStyle)); 
>+        nsCSSRendering::PaintBorder(aPresContext, aRenderingContext, this,
>+                                    aDirtyRect, rect, *myBorder, mStyleContext, skipSides);

What's this GetStyleData doing here?  It looks unused.

>+  // paint the children unless its the background layer, there are collapsed border, and its pass1

A |diff -w| would have made this easier to read, but it looks like
it's mostly reindenting.

>+  // for collapsed borders paint the backgrounds of cells, but not their contents (that happens below)
>+  PRUint32 flags = ((NS_FRAME_PAINT_LAYER_BACKGROUND == aWhichLayer) && IsBorderCollapse()) 
>+                   ? BORDER_COLLAPSE_BACKGROUNDS_PASS1 : 0;
>+  PaintChildren(aPresContext, aRenderingContext, aDirtyRect, aWhichLayer, flags);
>+
>+  if ((NS_FRAME_PAINT_LAYER_BACKGROUND == aWhichLayer) && IsBorderCollapse()) {
>+    // for collapsed borders, paint the borders and then the backgrounds of cell
>+    // contents but not the backgrounds of the cells
>+    PaintBCBorders(aPresContext, aRenderingContext, aDirtyRect);
>+    PaintChildren(aPresContext, aRenderingContext, aDirtyRect, aWhichLayer, 
>+                  BORDER_COLLAPSE_BACKGROUNDS_PASS2);

What else is the flags parameter used for?  Do you lose anything
by not passing |aFlags| through?

Other than those minor issues, r=dbaron.  (Note: I'm not a super-reviewer.)

Comment 12

[karnaze (gone)]

Attachment: revised patch

Comment 13

[karnaze (gone)]

Attachment: html diff of patch

Comment 14

[kinmoz]

Comment on attachment 71242 [details] [diff] [review]
revised patch

sr=[e-mail]

Comment 15

[Mike Shaver (:shaver emeritus)]

Comment on attachment 71242 [details] [diff] [review]
revised patch

a=shaver for 0.9.9

Comment 16

[karnaze (gone)]

The patch is in.

Comment 17

[Amarendra Hanumanula]

verified fixed