Closed Bug 127040 Opened 20 years ago Closed 20 years ago

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

Categories

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

x86
Windows 2000
defect

Tracking

()

VERIFIED FIXED
mozilla0.9.9

People

(Reporter: tar, Assigned: karnaze)

References

Details

(Whiteboard: PATCH)

Attachments

(3 files, 2 obsolete files)

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>
Severity: normal → major
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P2
Target Milestone: --- → mozilla1.0
Attached file testcase from bug
Keywords: nsbeta1
Attached patch patch to fix the bug (obsolete) — Splinter Review
Comment on attachment 70863 [details] [diff] [review]
patch to fix the bug

r=bernd
Attachment #70863 - Flags: review+
*** Bug 127174 has been marked as a duplicate of this bug. ***
Marking nsbeta1+
Keywords: nsbeta1nsbeta1+
Comment on attachment 70863 [details] [diff] [review]
patch to fix the bug

r= alexsavulov
Is this really want you want for dotted/dashed/transparent borders?
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. 
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.
Attachment #70863 - Attachment is obsolete: true
Attached patch new patch (obsolete) — Splinter Review
Paints in the order outlined in comment #9.
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.)
Attachment #71104 - Flags: review+
Attachment #71104 - Attachment is obsolete: true
Attached patch revised patchSplinter Review
Attached file html diff of patch
Attachment #71242 - Flags: review+
Attachment #71242 - Flags: superreview+
Whiteboard: PATCH
Comment on attachment 71242 [details] [diff] [review]
revised patch

a=shaver for 0.9.9
Attachment #71242 - Flags: approval+
The patch is in.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.0 → mozilla0.9.9
verified fixed
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.