Closed Bug 8113 Opened 21 years ago Closed 20 years ago

Empty cell's bgcolor not rendered

Categories

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

x86
Windows NT
defect

Tracking

()

VERIFIED FIXED
mozilla0.9

People

(Reporter: michael, Assigned: karnaze)

References

()

Details

(Keywords: testcase)

Attachments

(6 files)

I'm not sure if this is a bug or the feature was changed recently. Older builds
displayed the background color of a cell even if it was empty. This doesn't
really require any example, but you can compare the above address to
http://www.dayah.com/periodic/. Several empty cells that have a bgcolor do not
display (color-coded blocks in the group key and dark blue borders around the
boundary between transition and actinide/lanthanide series). The cell
background colors were set with CSS property background-color.
I recently changed tables so that empty cells do not get their own backgrounds.
One of David's bugs indicated that this is the way it is supposed to work. Maybe
I should only do this in "Standard" mode and let empty cell's have their own
background's in "Nav Quirks" mode.
You might want to check Nav4 behavior before doing that.  I distinctly remember
putting   in a bunch of empty cells so their backgrounds showed up.
Requiring something inside table cells to render its background color is a
Navigator quirk and has always been an annoying obstacle that has no benefit.
W3C specs make no mention or hint of this strange behavior, and all the other
browsers do render empty table cells.
Watch what you say about W3 specs.  It's actually a requirement of CSS2.
In http://www.w3.org/TR/REC-CSS2/tables.html#table-layers , rule 6 says:

# The topmost layer contains the cells themselves. As the figure shows, although
# all rows contain the same number of cells, not every cell may have specified
# content. These "empty" cells are transparent, letting lower layers shine
# through.

Like it or not, that's the spec.  Since that's what Nav4 implemented, I think
it's a bad idea to deviate from it.  If you *want* the background, you can
always throw in the &nbsp.  If things work the other way around, and you don't
want it, it's much harder.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → INVALID
Marking invalid, per David's comments.
Status: RESOLVED → VERIFIED
Verifying bug invalid per dbaron@fas.harvard.edu 6/14 comments.
Status: VERIFIED → REOPENED
Peter is going to suggest to the CSS working group that the "empty-cells"
property should apply to backgrounds in addition to borders. This would satisfy
the requirements of this bug.
Status: REOPENED → RESOLVED
Closed: 21 years ago21 years ago
Resolution: INVALID → REMIND
Changing the resolution to remind.
Status: RESOLVED → VERIFIED
Verifying bug REMIND
*** Bug 22824 has been marked as a duplicate of this bug. ***
One important thing - if cells are forced to be transparent in Mozilla - there
is a problem with putting   in some cells. This causes the cell to be the
height of whatever the default font is, making it impossible (or at least very
hard) to display a table cell with color that has a height less than 10 pixels
or so in Mozilla. If this behaviour is not addressed in Mozilla, it should
absolutely be addressed with a CSS spec update.
Summary: Empty cell's bgcolor not rendered if empty → Empty cell's bgcolor not rendered
*** Bug 29946 has been marked as a duplicate of this bug. ***
Just had a ug of mine marked duplicate of this one and it was suggested to use 
the CSS code for empty-cells: show.

However, the CSS code for "{ empty-cells: show }" only applies to the empty 
cell's borders being shown. It does not apply to the cell's BGCOLOR value which 
was the problem with the webpage.

Now I do see that if you look up a few spots from this posting, at:
"Additional Comments From karnaze@netscape.com  1999-06-15 19:41"

It says something about getting it to actually apply that CSS code to BGCOLOR's 
as well. 

Now, that was about 9 months ago now, I'm just wondering what, if anything ever 
came of this, as like the post says, it would address the BGCOLOR issue ....
I'm not sure what has been done regarding "empty-cells" applying to backgrounds. 
Adding Troy (our CSS representative) to the CC list.
One thing that should be pointed out (when mailing the CSS WG?) is that the 
HTML4 spec says nothing about hiding the backgrounds or borders of empty cells. 
So if empty-cells dosn't apply to backgrounds then HTML tables are incompatible 
with CSS tables.

One other thing that affects this is that empty-cells now defaults to 'hide' 
which is wrong according to the spec. This is filed in bug 33244
I'm going to attach a small testcase that derives from bug 33244.
*** Bug 37525 has been marked as a duplicate of this bug. ***
In reference to David's comments (And the reason this bug is in 'VERIFIED 
REMIND'):

You are correct on the W3C spec (data-empty cells are hidden), but only in 
relation to the example they provide.  In their example, the style defines a 
background for <TABLE> and <TD>.  In the spec, the TD is only transparent if no 
data is within it, and as such that TABLE color shows through. (see JUST ABOVE 
http://www.w3.org/TR/REC-CSS2/tables.html#propdef-table-layout for 
demonstration).

However, if a TABLE background is specified and a TD is NOT specified, the CSS 
rules of inheritability apply, and the TD tag would inherit it's parent 
object's properties.  In this example, the BACKGROUND is only specified in the 
TABLE tag, and is not specifically change or removed (set to transparent) in 
any of the cascading levels down to the TD tag.

In any of these cases, the TABLE tag has a style applied to it, and therefore 
to all of it's child objects.

In this case, displaying the background of the page itself is technically 
incorrect, and should be considered an open bug.
The way TABLE backrounds work in quirks mode is a separate issue, and I believe
it was just changed yesterday or today so that tables do draw their backgrounds
in quirks mode like they do in standard mode.  Check out a newer build (i.e, any
that hasn't been built yet) to see how it works.
*** Bug 33005 has been marked as a duplicate of this bug. ***
*** Bug 49465 has been marked as a duplicate of this bug. ***
*** Bug 60551 has been marked as a duplicate of this bug. ***
*** Bug 56258 has been marked as a duplicate of this bug. ***
*** Bug 59563 has been marked as a duplicate of this bug. ***
This has now been fixed in the css2 errata, empty cells should now be shown as 
normal except when 'empty-cells' are set to 'hidden'. When empty-cells are set 
to hidden empty cells should be completely hidden, i.e. no borders or 
backgrounds.

How outlines should be handled are a bit unclear but as far as I can read out 
they should behave the same as backgrounds and borders.
Status: VERIFIED → REOPENED
Keywords: mozilla1.0
Resolution: REMIND → ---
There was a recent addition to the CSS2 errata on this topic:

Section 17.5.1 Table layers and transparency

[2000-12-12] In point 6, change 'These "empty" cells are transparent' to:
These "empty" cells are transparent if the value of their 'empty-cells' property
is 'hide'
Borders around empty cells: the 'empty-cells' property

[2000-12-12] The 'empty-cells' property not only controls the borders, but also
the background.
also note that this dosn't require any special treatment in quirks mode since 
in quirks "empty-cells" defaults to "hide" which makes both background and 
border disappear.
Keywords: testcase
Milestone 0.8 has been released. We should either resolve this bug or update its
milestone.
With 2/22 build, bgcolor is not appearing correctly. Please fix this someone. 
Its annoying to have to add an &nbsp; Also - 1 pixel wide cells will get ruined 
by a space.
Target Milestone: M7 → ---
Moving to m1.0
Status: REOPENED → ASSIGNED
Target Milestone: --- → mozilla1.0
QA contact update
QA Contact: chrisd → amar
This seems like a bug that needs to be fixed. Lets get this thing fixed! 
Actually two bugs here:

Bug 1: {empty-cell: show} should allow background to be shown. That is shown in
the testcase. This means that empty-cell needs to be implemented for all
possible properties (hide, show, etc). Errata
Bug 2: {empty-cell: show} should be made default. Someone please make a second
testcase for this. The default is show.
http://www.w3.org/TR/REC-CSS2/tables.html#propdef-empty-cells

Bug 2 can 
be fixed in html.css
http://lxr.mozilla.org/mozilla/source/layout/html/document/src/html.css
after Bug 1 is fixed. (Simple)

/* tables */

table, :table {
  display: table;
  border-spacing: 2px; 
  border-collapse: separate;
  margin-top: 0;
  margin-bottom: 0;
  -moz-box-sizing: border-box;
  empty-cells: show;  /* Add this */
}

Bug 1 needs
http://lxr.mozilla.org/mozilla/source/layout/html/table/src/nsTableCellFrame.cpp#261
It needs to do a second check to see whether empty-cell hide is chosen.

Old code:

      // only non empty cells render their background
      if (PR_FALSE == GetContentEmpty()) {
        nsCSSRendering::PaintBackground(aPresContext, aRenderingContext, this,
                                        aDirtyRect, rect, *myColor, *myBorder,
0, 0);
      }
    
      // empty cells do not render their border
      PRBool renderBorder = PR_TRUE;
      if (PR_TRUE==GetContentEmpty())
      {
        if (NS_STYLE_TABLE_EMPTY_CELLS_HIDE==cellTableStyle->mEmptyCells)
          renderBorder=PR_FALSE;
      }
      if (PR_TRUE==renderBorder)
      {
        PRIntn skipSides = GetSkipSides();
        nsTableFrame* tableFrame=nsnull;  // I should be checking my own style
context, but border-collapse isn't inheriting correctly
        nsresult rv = nsTableFrame::GetTableFrame(this, tableFrame);
        if ((NS_SUCCEEDED(rv)) && (nsnull!=tableFrame))
        {
          const nsStyleTable* tableStyle;
          tableFrame->GetStyleData(eStyleStruct_Table, ((const nsStyleStruct
*&)tableStyle)); 
          if (NS_STYLE_BORDER_SEPARATE == tableFrame->GetBorderCollapseStyle())
          {
            nsCSSRendering::PaintBorder(aPresContext, aRenderingContext, this,
                                        aDirtyRect, rect, *myBorder,
mStyleContext, skipSides);
          }
          else
          {
            nsCSSRendering::PaintBorderEdges(aPresContext, aRenderingContext, this,
                                             aDirtyRect, rect, mBorderEdges,
mStyleContext, skipSides);
          }
        }
      }
    }
  }





Change to:





	  /* 
	  bug # 8113:

	  http://www.w3.org/TR/REC-CSS2/tables.html#table-layers

      There was a recent addition to the CSS2 errata on this topic:

      Section 17.5.1 Table layers and transparency

      [2000-12-12] In point 6, change 'These "empty" cells are transparent' to:
      These "empty" cells are transparent if the value of their 'empty-cells'
property
      is 'hide'
      Borders around empty cells: the 'empty-cells' property

      [2000-12-12] The 'empty-cells' property not only controls the borders, but
also
      the background.




	  */

    
      // empty cells do not render their border or background if {empty-cells:
hide} is given
      PRBool renderBgBorder = PR_TRUE;
      if (PR_TRUE==GetContentEmpty())
      {
        if (NS_STYLE_TABLE_EMPTY_CELLS_HIDE==cellTableStyle->mEmptyCells)
          renderBgBorder=PR_FALSE;
      }
      if (PR_TRUE==renderBgBorder)
      {
		//Render background
        nsCSSRendering::PaintBackground(aPresContext, aRenderingContext, this,
                                        aDirtyRect, rect, *myColor, *myBorder,
0, 0);
		//Render border
        PRIntn skipSides = GetSkipSides();
        nsTableFrame* tableFrame=nsnull;  // I should be checking my own style
context, but border-collapse isn't inheriting correctly
        nsresult rv = nsTableFrame::GetTableFrame(this, tableFrame);
        if ((NS_SUCCEEDED(rv)) && (nsnull!=tableFrame))
        {
          const nsStyleTable* tableStyle;
          tableFrame->GetStyleData(eStyleStruct_Table, ((const nsStyleStruct
*&)tableStyle)); 
          if (NS_STYLE_BORDER_SEPARATE == tableFrame->GetBorderCollapseStyle())
          {
            nsCSSRendering::PaintBorder(aPresContext, aRenderingContext, this,
                                        aDirtyRect, rect, *myBorder,
mStyleContext, skipSides);
          }
          else
          {
            nsCSSRendering::PaintBorderEdges(aPresContext, aRenderingContext, this,
                                             aDirtyRect, rect, mBorderEdges,
mStyleContext, skipSides);
          }
        }
      }
    }
  }


Thanks to basic <_basic@yahoo.com> for his help.



Errata: http://www.w3.org/Style/css2-updates/REC-CSS2-19980512-errata.html

I need to test this code still. If someone else wants to test it, feel free. 
Sorry about all the line breaks in long lines (Bugzilla).
Bug 2 isn't a bug. empty-cells default is currently "show" in strict mode and
"hide" in quirks mode. This is the way it should be for backwards compability
(see also bug 33244 )
bug 33244 is "bug 2". Please see the posting I made to this respect in the bug.
Brian: Can you make diff's out of this and attach it as a patch to this bug?
Whiteboard: patch, review
Keywords: patch, review
Whiteboard: patch, review
Since boberb doesn't get any Bugzilla-Mail...

CCing attinasi and waterson:
Can you give r= and sr= ?
please attach a patch, cvs diff -u. thanks.
Attached patch New PatchSplinter Review
Ok, I created a new patch which should IMO do the right thing: The only way a
cell's background and border isn't draw is when the cell is empty and
'empty-cell: hide' is set.

This is my first patch so I hope I didn't screw up.
Thanks for the patch! I'm cc'ing karnaze, who owns the table code. Chris,
whatdya think?
The idea seems good -- it should bring us into conformance with the CSS2 errata
item on section 17.5.1 from 2000-12-12.  However, why don't you just get rid of
the variable |renderBorder| and combine the two if-statements (since they're now
testing for the same thing)?
Please refer to Additional Comments From Brian "NeTDeMoN" Bober 2001-03-23 
00:50.

If you look at the code I included earlier  - I did two things that aren't in 
this patch.

I created a variable to hold the flag state. That way you don't get a giant if 
statement.

+      if ( !(GetContentEmpty() && NS_STYLE_TABLE_EMPTY_CELLS_HIDE == 
cellTableStyle->mEmptyCells) ) {

I think should be changed to:

      PRBool renderBgBorder = PR_TRUE;
      if (PR_TRUE!=GetContentEmpty()) renderBgBorder=PR_FALSE;
      if (NS_STYLE_TABLE_EMPTY_CELLS_HIDE==cellTableStyle->mEmptyCells) 
renderBgBorder=PR_FALSE;
      if (PR_TRUE==renderBgBorder)
      {
or something equivalent. This breaks the if statement up and makes it more 
readable. I don't think the If statements should be combined into one big if 
statement because that makes it less readable.

Also, I think that the bug # and a little info about the changes should be 
added before the code.

/* 
	  bug # 8113:

	  http://www.w3.org/TR/REC-CSS2/tables.html#table-layers

      There was a recent addition to the CSS2 errata on this topic:

      Section 17.5.1 Table layers and transparency

      [2000-12-12] In point 6, change 'These "empty" cells are transparent' to:
      These "empty" cells are transparent if the value of their 'empty-cells'
property
      is 'hide'
      Borders around empty cells: the 'empty-cells' property

      [2000-12-12] The 'empty-cells' property not only controls the borders, but
also
      the background.

	  */


Besides that - looks good.

A question: Have you tried changing the doctype and having no doctype and does 
it still work?
The last question isn't really important. I was just curious because of all 
that quirks nonsense.
> or something equivalent. This breaks the if statement up and makes it more 
> readable. I don't think the If statements should be combined into one big if 
> statement because that makes it less readable.

I think it's far more readable as a single if statement because it shows clearly
that there is really only one complex condition that can lead to two results
(background and border vs. no background and no border), rather than many
possible results.

Adding a comment wouldn't be a bad idea... but it should probably be much more
consise so it doesn't interrupt the rest of the code.
The latest patch has some additional comments.
*** Bug 74682 has been marked as a duplicate of this bug. ***
I runned succesful the regression tests with the patch r = bernd
How would a change to a paint method affect the regression tests?
r=karnaze, it looks good. Changing to m0.9.

Christian, if you do not have cvs checkin rights, please let me.
Target Milestone: mozilla1.0 → mozilla0.9
Please karnaze, go ahead..
The patch is in. Thanks Christian.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago20 years ago
Resolution: --- → FIXED
Cool. I just hope that in future patches the number of patch revisions don't 
increase linearly with the lines I change... :-)
------- Additional Comments From David Baron 2001-04-16 09:41 -------

How would a change to a paint method affect the regression tests?
look at 

layout/html/tests/table/bugs/bug17138.html

before and after the patch, 

for me it produced a

file:///c|/moz_sour/mozilla/mozilla/layout/html/tests/table/bugs/bug17138.html: 
done loading (220 msec)
frame bbox mismatch: 0,0,6750,428 vs. 0,0,6750,415
Node 1:
  TableRow(tr)(0) 0x10004 0,0,6750,428, |null attr|0 0 3 3 16777215 0 0  1  
1.000000|left: Null top: Null right: Null bottom: Null  left: Null top: Null 
right: Null bottom: Null  left: 1[0x1]enum top: 1[0x1]enum right: 1[0x1]enum 
bottom: 1[0x1]enum  left: Null top: Null right: Null bottom: Null  left: Null 
top: Null right: Null bottom: Null  1[0x1]enum 0|100 100 |0 left: Auto top: Auto 
right: Auto bottom: Auto  Auto  0[0x0]tw  Null  Auto  0[0x0]tw  Null  0 Auto  |0 
0 0 0 Normal  Normal  0[0x0]tw  Normal  15[0xf]enum  |0 15 0 0 0 0 1 0 0 0 0 0 
0|0 0 4 1 0[0x0]tw  0[0x0]tw  Null  0 0 -1 1 Null  |0 0 0 0 Null  |3 0 7 0 0 4 
|0 0 0  2 2 0 Auto  
Node 2:
  TableRow(tr)(0) 0x10004 0,0,6750,415, |null attr|0 0 3 3 16777215 0 0  1  
1.000000|left: Null top: Null right: Null bottom: Null  left: Null top: Null 
right: Null bottom: Null  left: 1[0x1]enum top: 1[0x1]enum right: 1[0x1]enum 
bottom: 1[0x1]enum  left: Null top: Null right: Null bottom: Null  left: Null 
top: Null right: Null bottom: Null  1[0x1]enum 0|100 100 |0 left: Auto top: Auto 
right: Auto bottom: Auto  Auto  0[0x0]tw  Null  Auto  0[0x0]tw  Null  0 Auto  |0 
0 0 0 Normal  Normal  0[0x0]tw  Normal  15[0xf]enum  |0 15 0 0 0 0 1 0 0 0 0 0 
0|0 0 4 1 0[0x0]tw  0[0x0]tw  Null  0 0 -1 1 Null  |0 0 0 0 Null  |3 0 7 0 0 4 
|0 0 0  2 2 0 Auto  
frame bbox mismatch: 0,0,4875,428 vs. 0,0,4875,415
Node 1:
  TableCell(td)(1) 0x4 0,0,4875,428, |null attr|0 0 3 3 16777215 0 0  1  
1.000000|left: Null top: Null right: Null bottom: Null  left: 0[0x0]tw top: 
0[0x0]tw right: 0[0x0]tw bottom: 0[0x0]tw  left: 1[0x1]enum top: 1[0x1]enum 
right: 1[0x1]enum bottom: 1[0x1]enum  left: Null top: Null right: Null bottom: 
Null  left: Null top: Null right: Null bottom: Null  1[0x1]enum 0|100 100 |0 
left: Auto top: Auto right: Auto bottom: Auto  Auto  0[0x0]tw  Null  Auto  
0[0x0]tw  Null  0 Auto  |0 0 0 0 Normal  Normal  0[0x0]tw  Normal  13[0xd]enum  
|0 16 0 0 0 0 1 0 0 0 0 0 0|0 0 4 1 0[0x0]tw  0[0x0]tw  Null  0 0 -1 1 Null  |0 
0 0 0 Null  |3 0 7 0 0 4 |0 0 0  2 2 0 Auto  
Node 2:
  TableCell(td)(1) 0x4 0,0,4875,415, |null attr|0 0 3 3 16777215 0 0  1  
1.000000|left: Null top: Null right: Null bottom: Null  left: 0[0x0]tw top: 
0[0x0]tw right: 0[0x0]tw bottom: 0[0x0]tw  left: 1[0x1]enum top: 1[0x1]enum 
right: 1[0x1]enum bottom: 1[0x1]enum  left: Null top: Null right: Null bottom: 
Null  left: Null top: Null right: Null bottom: Null  1[0x1]enum 0|100 100 |0 
left: Auto top: Auto right: Auto bottom: Auto  Auto  0[0x0]tw  Null  Auto  
0[0x0]tw  Null  0 Auto  |0 0 0 0 Normal  Normal  0[0x0]tw  Normal  13[0xd]enum  
|0 16 0 0 0 0 1 0 0 0 0 0 0|0 0 4 1 0[0x0]tw  0[0x0]tw  Null  0 0 -1 1 Null  |0 
0 0 0 Null  |3 0 7 0 0 4 |0 0 0  2 2 0 Auto  
Regression data 1:
Regression data 2:
regression test 
c:\moz_sour\mozilla\mozilla\layout\html\tests\table\bugs\verify\bug17138.rgd 
failed

the short lesson is: Always run the regression tests!!! I did not mention that 
the picture for this testcase improved a lot ( we render it now like NN and 
IE6).
Blocks: 75664
Oops, I think we forgot to set the default of empty-cells to 'show', at least in 
standards mode.

Reopening, this should be a trivial fix.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I added the show-empty rules with the appropiate setting to the standard and 
quirk stylesheets.

Need r and sr.
We'd be better off setting default values for properties in the style system
rather than in html.css / quirks.css.  The need for your html.css patch shows
that there's a bug somewhere else in the style system (since CSS2 says the
default value of 'empty-cells' should be 'show'.  We shouldn't cover up bugs
with an html.css patch (which will only fix HTML and not XML).
Sorry, I screwed up. I had a typo in my DTD so that quirks-mode was triggered.
Standards mode actually has empty-cells:show set by default.

Marking as fixed.
Sorry about the spam.
Status: REOPENED → RESOLVED
Closed: 20 years ago20 years ago
Resolution: --- → FIXED
The default is SHOW in standard mode and HIDE for QuirksMode. The code says it
was done for bug 33244

     mEmptyCells = (compatMode == eCompatibility_NavQuirks
                    ? NS_STYLE_TABLE_EMPTY_CELLS_HIDE     // bug 33244
                    : NS_STYLE_TABLE_EMPTY_CELLS_SHOW);
Yes, that was the part of the code where I was looking for a bug before I saw my 
invalid DTD.
Should the testcase display like it says (both rows being equal)?

I'm using 2001052404 and the rows are displayed differently.
No, since a recent set of errata to CSS2 made 'empty-cells' apply to backgrounds
as well:

http://www.w3.org/Style/css2-updates/REC-CSS2-19980512-errata.html
In that case...  ;)

build id: 2001052404 on Windows 2000
Status: RESOLVED → VERIFIED
*** Bug 90247 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.