Closed Bug 8113 Opened 25 years ago Closed 24 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: 25 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: 25 years ago25 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 Patch (deleted) — Splinter 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)?
Attached patch Slightly improved patch (deleted) — Splinter Review
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: 25 years ago24 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 → ---
Attached patch patch to html.css and quirk.css (deleted) — Splinter Review
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: 24 years ago24 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.

Attachment

General

Created:
Updated:
Size: