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

VERIFIED FIXED in mozilla0.9.9

Status

()

Core
Layout: Tables
P2
major
VERIFIED FIXED
16 years ago
4 years ago

People

(Reporter: Tarmo Järvalt, Assigned: karnaze (gone))

Tracking

Trunk
mozilla0.9.9
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PATCH)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

16 years ago
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>
(Assignee)

Updated

16 years ago
Severity: normal → major
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P2
Target Milestone: --- → mozilla1.0

Comment 1

16 years ago
Created attachment 70755 [details]
testcase from bug
(Assignee)

Updated

16 years ago
Keywords: nsbeta1
(Assignee)

Comment 2

16 years ago
Created attachment 70863 [details] [diff] [review]
patch to fix the bug

Comment 3

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

r=bernd
Attachment #70863 - Flags: review+
(Assignee)

Comment 4

16 years ago
*** Bug 127174 has been marked as a duplicate of this bug. ***
Marking nsbeta1+
Keywords: nsbeta1 → nsbeta1+

Comment 6

16 years ago
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?
(Assignee)

Comment 8

16 years ago
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. 
(Assignee)

Comment 9

16 years ago
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.
(Assignee)

Updated

16 years ago
Attachment #70863 - Attachment is obsolete: true
(Assignee)

Comment 10

16 years ago
Created attachment 71104 [details] [diff] [review]
new patch

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+
(Assignee)

Updated

16 years ago
Attachment #71104 - Attachment is obsolete: true
(Assignee)

Comment 12

16 years ago
Created attachment 71242 [details] [diff] [review]
revised patch
(Assignee)

Comment 13

16 years ago
Created attachment 71243 [details]
html diff of patch
Attachment #71242 - Flags: review+

Updated

16 years ago
Attachment #71242 - Flags: superreview+

Comment 14

16 years ago
Comment on attachment 71242 [details] [diff] [review]
revised patch

sr=kin@netscape.com
(Assignee)

Updated

16 years ago
Whiteboard: PATCH
Comment on attachment 71242 [details] [diff] [review]
revised patch

a=shaver for 0.9.9
Attachment #71242 - Flags: approval+
(Assignee)

Comment 16

16 years ago
The patch is in.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Assignee)

Updated

16 years ago
Target Milestone: mozilla1.0 → mozilla0.9.9

Comment 17

16 years ago
verified fixed
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.