Closed Bug 392047 Opened 15 years ago Closed 13 years ago

CSS2.1 empty-cells: hide declaration should be ignored if border-collapse: collapse

Categories

(Core :: Layout: Tables, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bugzilla, Assigned: WCWedin)

Details

Attachments

(2 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6

The CSS2.1 CR seems to indicate that the empty-cells: hide declaration be ignored when border-collapse: collapse. Opera and Safari do just that while Firefox only ignores it (correctly) with regard to the borders; Firefox (incorrectly) hides the background anyway.

Reproducible: Always

Steps to Reproduce:
1. Apply border-collapse: collapse and empty-cells: hide to a table with one or more empty cells.
Actual Results:  
The backgrounds (but not borders) are hidden for those cells that are empty.

Expected Results:  
The backgrounds should not be hidden for those cells that are empty.
Attached file testcase
This demonstrates the bug and compared other browser renderings.
http://www.w3.org/TR/CSS21/tables.html#empty-cells
The section of the CSS 2.1 spec relevant to the bug. empty-cells only applies to tables using the separated borders model.
http://www.w3.org/Style/CSS/current-work#tables indicates that the table module won't be changing in CSS3.
indeed
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → bernd_mozilla
If you compare to the text in CSS 2.0, I'm not sure if this was intentional.  The change to make 'empty-cells' apply to backgrounds as well is new in CSS2.1.  It's not clear whether the restriction of applying only to the separated borders model was intended to apply to that as well.

What do other browsers do (both by default, and for the various values of 'empty-cells'?).
Er, never mind -- I read comment 0 now.  I guess given that Opera and Safari do what the spec says we should follow them, even if it was an accident.
It does seem kind of weird that you can't make the background fall through on a cell just because you're collapsing boarders. I would say it was a mistake, too, but we're kind of stuck if Safari and Opera are both doing it.

That said, the fix looks pretty simple, on one condition:

// Called by nsTablePainter
void
nsTableCellFrame::PaintCellBackground(nsIRenderingContext& aRenderingContext,
                                      const nsRect& aDirtyRect, nsPoint aPt)
{
  if (!GetStyleVisibility()->IsVisible())
    return;
  if (GetContentEmpty() &&
      NS_STYLE_TABLE_EMPTY_CELLS_HIDE == GetStyleTableBorder()->mEmptyCells)
    return;

  PaintBackground(aRenderingContext, aDirtyRect, aPt);
}

As long as this function is actually only called from nsTablePainter, we can just cut out the test for mEmptyCells, since nsTablePainter is already doing what is, as far as I can tell, an equivalent test. The only other change is that we have to make is to move the test against mIsBorderCollapse in nsTableCellFrame::PaintBackground so that the function doesn't return prematurely.
I tested this, and it seems to work, but I'm not sure if there would be adverse side-effects (i.e., if something else calls PaintCellBackground and depends on the check being there). If that's the case, the fix is only slightly more involved.
I know Bernd's got this one, but I figured I'd upload what I have anyway, maybe save a little trouble.
Comment on attachment 309385 [details] [diff] [review]
Possible patch, with potential problems?

Index: layout/tables/nsTableCellFrame.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/tables/nsTableCellFrame.cpp,v
retrieving revision 3.409
diff -u -8 -p -r3.409 nsTableCellFrame.cpp
--- layout/tables/nsTableCellFrame.cpp	8 Feb 2008 09:36:32 -0000	3.409
+++ layout/tables/nsTableCellFrame.cpp	14 Mar 2008 10:45:15 -0000
@@ -350,19 +350,19 @@ nsTableCellFrame::PaintBackground(nsIRen
 
 // Called by nsTablePainter
 void
 nsTableCellFrame::PaintCellBackground(nsIRenderingContext& aRenderingContext,
                                       const nsRect& aDirtyRect, nsPoint aPt)
 {
   if (!GetStyleVisibility()->IsVisible())
     return;
-  if (GetContentEmpty() &&
+  /*if (GetContentEmpty() &&
       NS_STYLE_TABLE_EMPTY_CELLS_HIDE == GetStyleTableBorder()->mEmptyCells)
-    return;
+    return;*/
 
   PaintBackground(aRenderingContext, aDirtyRect, aPt);
 }
 
 class nsDisplayTableCellBackground : public nsDisplayItem {
 public:
   nsDisplayTableCellBackground(nsTableCellFrame* aFrame) : nsDisplayItem(aFrame) {
     MOZ_COUNT_CTOR(nsDisplayTableCellBackground);
Index: layout/tables/nsTablePainter.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/tables/nsTablePainter.cpp,v
retrieving revision 3.21
diff -u -8 -p -r3.21 nsTablePainter.cpp
--- layout/tables/nsTablePainter.cpp	8 Jul 2007 07:08:33 -0000	3.21
+++ layout/tables/nsTablePainter.cpp	14 Mar 2008 10:45:15 -0000
@@ -595,24 +595,25 @@ TableBackgroundPainter::PaintRow(nsTable
   mRow.Clear();
   return NS_OK;
 }
 
 nsresult
 TableBackgroundPainter::PaintCell(nsTableCellFrame* aCell,
                                   PRBool aPassSelf)
 {
+  printf("\n\nStarting PaintCell.\n");
   NS_PRECONDITION(aCell, "null frame");
 
   const nsStyleTableBorder* cellTableStyle;
   cellTableStyle = aCell->GetStyleTableBorder();
   if (!(NS_STYLE_TABLE_EMPTY_CELLS_SHOW == cellTableStyle->mEmptyCells ||
         NS_STYLE_TABLE_EMPTY_CELLS_SHOW_BACKGROUND == cellTableStyle->mEmptyCells)
-      && aCell->GetContentEmpty()) {
-    return NS_OK;
+      && aCell->GetContentEmpty() && !mIsBorderCollapse) {
+	return NS_OK;
   }
 
   PRInt32 colIndex;
   aCell->GetColIndex(colIndex);
 
   //Paint column group background
   if (mCols && mCols[colIndex].mColGroup && mCols[colIndex].mColGroup->IsVisible()) {
     nsCSSRendering::PaintBackgroundWithSC(mPresContext, mRenderingContext,
@@ -620,39 +621,39 @@ TableBackgroundPainter::PaintCell(nsTabl
                                           mCols[colIndex].mColGroup->mRect,
                                           *mCols[colIndex].mColGroup->mBackground,
                                           *mCols[colIndex].mColGroup->mBorder,
                                           mZeroPadding, PR_TRUE, &mCellRect);
   }
 
   //Paint column background
   if (mCols && mCols[colIndex].mCol.IsVisible()) {
-    nsCSSRendering::PaintBackgroundWithSC(mPresContext, mRenderingContext,
+	nsCSSRendering::PaintBackgroundWithSC(mPresContext, mRenderingContext,
                                           mCols[colIndex].mCol.mFrame, mDirtyRect,
                                           mCols[colIndex].mCol.mRect,
                                           *mCols[colIndex].mCol.mBackground,
                                           *mCols[colIndex].mCol.mBorder,
                                           mZeroPadding, PR_TRUE, &mCellRect);
   }
 
   //Paint row group background
   if (mRowGroup.IsVisible()) {
-    nsCSSRendering::PaintBackgroundWithSC(mPresContext, mRenderingContext,
+	nsCSSRendering::PaintBackgroundWithSC(mPresContext, mRenderingContext,
                                           mRowGroup.mFrame, mDirtyRect, mRowGroup.mRect,
                                           *mRowGroup.mBackground, *mRowGroup.mBorder,
                                           mZeroPadding, PR_TRUE, &mCellRect);
   }
 
   //Paint row background
   if (mRow.IsVisible()) {
-    nsCSSRendering::PaintBackgroundWithSC(mPresContext, mRenderingContext,
+	nsCSSRendering::PaintBackgroundWithSC(mPresContext, mRenderingContext,
                                           mRow.mFrame, mDirtyRect, mRow.mRect,
                                           *mRow.mBackground, *mRow.mBorder,
                                           mZeroPadding, PR_TRUE, &mCellRect);
   }
 
   //Paint cell background in border-collapse unless we're just passing
-  if (mIsBorderCollapse && !aPassSelf) {
-    aCell->PaintCellBackground(mRenderingContext, mDirtyRect, mCellRect.TopLeft());
+  if (!aPassSelf) {
+	aCell->PaintCellBackground(mRenderingContext, mDirtyRect, mCellRect.TopLeft());
   }
 
   return NS_OK;
 }
Sorry about the spam, guys. Not really a Bugzilla vet.
Attachment #309385 - Attachment is obsolete: true
>I know Bernd's got this one, but I figured I'd upload what I have anyway, maybe
> save a little trouble.
please take it and get your feet wet
Assignee: bernd_mozilla → WCWedin
Status: NEW → ASSIGNED
One change of note from the earlier version: The test against empty-cells in nsTablePainter was changed to use NS_STYLE_TABLE_EMPTY_CELLS_HIDE. It's clearer this way, and all other comparisons against empty-cells were done this way. Previously, the test here was !(SHOW or SHOW_BACKGROUND). SHOW_BACKGROUND is only used elsewhere for assignment for Quirks Mode, so its use in nsTablePainter was kind of inconsistent.
Attachment #309388 - Attachment is obsolete: true
Attachment #309646 - Flags: review+
Attachment #309646 - Flags: review+ → review?(mkanat)
Comment on attachment 309646 [details] [diff] [review]
Polished version of the earlier patch

Wow, I have no idea how I got that review request--I'm not a Mozilla reviewer.
Attachment #309646 - Flags: review?(mkanat)
Comment on attachment 309646 [details] [diff] [review]
Polished version of the earlier patch

somebody has to review this
Attachment #309646 - Flags: review?(bernd_mozilla)
Attachment #335292 - Flags: superreview?(roc)
Attachment #335292 - Flags: review?(fantasai.bugs)
Attachment #309646 - Attachment is obsolete: true
Attachment #309646 - Flags: review?(bernd_mozilla)
Comment on attachment 335292 [details]
revised patch + reftest

+      && aCell->GetContentEmpty()&& !mIsBorderCollapse) {

Space before &&

I feel comfortable r+ing this
Attachment #335292 - Flags: superreview?(roc)
Attachment #335292 - Flags: superreview+
Attachment #335292 - Flags: review?(fantasai.bugs)
Attachment #335292 - Flags: review+
fix checked in. Thank you William and sorry for the long time, this went pretty badly with the review request. If you have another table patch please ask me for review I will make sure that it gets handled properly.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.