Last Comment Bug 84307 - [quirks] can't set table border color in css
: [quirks] can't set table border color in css
Status: RESOLVED FIXED
: testcase
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal with 4 votes (vote)
: ---
Assigned To: fantasai
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks: 55623 190655 252530 370833 155318 188715
  Show dependency treegraph
 
Reported: 2001-06-06 08:20 PDT by Conor Lennon
Modified: 2014-04-25 15:15 PDT (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcast containing html (122 bytes, text/html)
2001-06-06 08:21 PDT, Conor Lennon
no flags Details
testcase with CSS-applied border (143 bytes, text/html)
2001-07-11 17:55 PDT, fantasai
no flags Details
patch to remove -moz-bg-inset/outset/solid (36.30 KB, patch)
2006-11-21 10:28 PST, fantasai
bzbarsky: review+
Details | Diff | Review
patch to remove -moz-bg-inset/outset/solid (+bz) (36.47 KB, patch)
2006-12-05 17:43 PST, fantasai
no flags Details | Diff | Review
patch to remove -moz-bg-inset/outset/solid (+bz) (35.04 KB, patch)
2006-12-06 14:08 PST, fantasai
dbaron: superreview+
Details | Diff | Review
patch to remove -moz-bg-inset/outset/solid (up-to-date, fix <HR color>) (36.52 KB, patch)
2007-01-19 13:48 PST, fantasai
no flags Details | Diff | Review
patch to remove -moz-bg-inset/outset/solid (use color, not border-color) (35.09 KB, patch)
2007-01-29 10:42 PST, fantasai
dbaron: superreview+
Details | Diff | Review
followup patch to move gray table borders to quirks mode (956 bytes, patch)
2007-02-01 12:20 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
fantasai.bugs: review+
bzbarsky: superreview+
Details | Diff | Review

Description Conor Lennon 2001-06-06 08:20:54 PDT
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:0.9.1+) Gecko/20010605
BuildID:    2001060520

The table in the following html does not have a red border

<html>
<body>
<TABLE border=3 style="border-color:red;">
<TR> <TD>Red Border</TD> </TR>
</TABLE>
</body>
</html>



Reproducible: Always
Steps to Reproduce:
1. Load the html


Actual Results:  table does not have red border

Expected Results:  table should have red border
Comment 1 Conor Lennon 2001-06-06 08:21:21 PDT
Created attachment 37354 [details]
testcast containing html
Comment 2 James Green 2001-06-06 15:19:08 PDT
Linux too, OS/All. Is this a regression or something?
Comment 3 Bernd 2001-07-10 13:15:41 PDT
I think this is quirks mode only
Comment 4 Bernd 2001-07-11 10:50:53 PDT
fantasai can you help triaging the bug? Thanks
Comment 5 fantasai 2001-07-11 17:55:26 PDT
Created attachment 42031 [details]
testcase with CSS-applied border
Comment 6 fantasai 2001-07-11 19:28:19 PDT
This is caused by a Quirks setting in Mozilla that places a background-color 
based outset border on the table when it is specified by the 'border' attribute 
of <table>. Setting the border by CSS overrides this behavior (see testcase).

http://lxr.mozilla.org/seamonkey/source/layout/base/public/nsStyleConsts.h

The problem is that the "background-color based outset border" is all one 
setting, a border-style value, instead of being split into two settings. It 
should probably be "border-style: outset", as in standard CSS, with a 
nonstandard NS_STYLE_COLOR_MOZ_USE_BG_COLOR value for the 'border-color'.

I noticed that we seem to be setting this twice--once in the table element, and 
once in the table frame (in MapHTMLBorderStyle). But the nsTableFrame:: 
MapHTMLBorderStyle function doesn't seem to be called from anywhere. (Am I 
reading this right?)


Bernd, this is either under HTMLTables or Style System, not ActiveX. Did you 
change the wrong bug or something?

colour->color, for search purposes
Comment 7 Bernd 2001-07-14 13:06:19 PDT
I am so (in)famous bug 90363 resetting category
Comment 8 Waheed Islam 2001-08-08 06:13:12 PDT
does anyone know what is happening about this?

cause the original css border-color bug still exists in build: 20010807 21
however, it seems that if you add the value, 'border-style: solid'
then the colour is displayed properly, (as with the above attachment from fantasai)
Comment 9 fantasai 2001-08-08 07:14:25 PDT
This bug is currently unassigned, so I doubt anyone's working on it right now.
Comment 10 karnaze (gone) 2001-08-08 08:14:22 PDT
reassigning to hyatt.
Comment 11 Andreas Franke (gone) 2001-08-08 10:14:07 PDT
I have encountered this bug using 2001080104 on Linux. 
Thanks to Waheed Islam for the 'border-style: solid'-workaround. :-) 

Comment 12 Waheed Islam 2001-10-06 07:33:15 PDT
don't thank me, thanks to fantasai for that css-applied border
testcase. ;)
Comment 13 Micheal 2001-12-13 00:34:28 PST
I to ran into this problem, and solved it by useing CSS entireley for the border.
Instead of "border=3 style="border-color:red;"" i would use
"style="border-width: 3; border-color: red"
Comment 14 Madhur Bhatia 2002-04-23 17:55:15 PDT
still happening on win2000 build id : 2002-04-22-1.0.0 branch build
Comment 15 Boris Zbarsky [:bz] 2003-05-03 22:55:10 PDT
bernd, didn't I mention something like what comment 6 is proposing in some other
bug?  Is this one a duplicate?
Comment 16 Robin Harris 2004-07-19 18:13:23 PDT
With border width set as 1 and border colour white, Safari and Explorer show no
borders  on white background - but Camino show grey borders. Only with no border
width will Camino show nothing on a white background.(In reply to comment #0)
> From Bugzilla Helper:
> User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:0.9.1+) Gecko/20010605
> BuildID:    2001060520
> 
> The table in the following html does not have a red border
> 
> <html>
> <body>
> <TABLE border=3 style="border-color:red;">
> <TR> <TD>Red Border</TD> </TR>
> </TABLE>
> </body>
> </html>
> 
> 
> 
> Reproducible: Always
> Steps to Reproduce:
> 1. Load the html
> 
> 
> Actual Results:  table does not have red border
> 
> Expected Results:  table should have red border

Comment 17 Egon Knapen 2004-10-09 03:16:56 PDT
Okay I'm having the same problem, I can fix it ofcourse by adding border-style.
But what about the other websites, who are omitting this border-style. Shouldn't
the default border style be set to something like 'insert' or 'solid' then?

I didn't really find anything comparable to the standard used by IE. (Except
ofcourse not setting the color at all).

I'm not trying to imitate IE, but if a feature doesn't work because you first
need to set an other, than I think maybe something should be fixed...

A strange notice I made, is that the default, is just the one I want, it looks
better than the border-style:solid or the border-style:outset. If only we could
change its color now....

Comment 18 fantasai 2006-11-03 15:12:08 PST
So, what I'd like to do here is to separate out the "use bg color for border color" setting from the style setting. This means getting rid of -moz-bg-inset, -moz-bg-outset, and -moz-bg-solid on the border-style property and adding a -moz-use-bg-color value to the border-color property.

I'm looking at the nsStyleStruct interface for borders.. the call for getting the border color is
   void GetBorderColor(PRUint8 aSide, nscolor& aColor,
                       PRBool& aTransparent, PRBool& aForeground) const

The border can be a specific color, the foreground color, or transparent. I need to add an option for it to be the background color. I can do that either by adding another boolean, or by combining aTransparent, aForeground and the background-color option into a single enum argument.
   void GetBorderColor(PRUint8 aSide, nscolor& aColor,
                       SpecialColor& aSpecialColor) const

I'm currently leaning toward using an enum, but where should I be declaring it? Inside nsStyleBorder (and nsStyleOutline)'s namespace, or somewhere else inside nsStyleStruct.h?

Also, this is going to have repercussions on the interface to some protected functions of nsCSSRendering, because several of those functions take an nscolor as the border color, and that's not enough information to do background coloring. (In the current code that information is tied into and carried by the border style setting.)
Comment 19 Bernd 2006-11-03 21:39:16 PST
Can't we remove -moz-use-bg-color at all? Who uses this property? If the only consumer are tables then remove this without a trace. If the HR really needs this then lets limit it to it. But I remember an old jwz bug/rant that hr color can't be adapted to text color.
Comment 20 fantasai 2006-11-21 10:28:45 PST
Created attachment 246171 [details] [diff] [review]
patch to remove -moz-bg-inset/outset/solid

This patch removes support for -moz-bg-* border styles, as Bernd requested. It will, as a side effect, fix this bug and bug 190655. Quirks mode specifies a gray border to be compatible with IE and Opera, which always use gray.

The color contrast in quirks mode is significantly higher than in standards mode and in Opera. This may or may not be a problem.
Comment 21 Boris Zbarsky [:bz] 2006-12-03 12:36:31 PST
Comment on attachment 246171 [details] [diff] [review]
patch to remove -moz-bg-inset/outset/solid

>Index: layout/style/quirk.css

>+/* Quirk: table and hr borders are gray by default */

So why is this a quirk?  Why not just do it in both modes, especially for tables?  People who want to have it use the current color can just set "border-color: currentColor" if they want.

Basically, I'd rather we didn't add quirks unless we absolutely have to....

>Index: layout/base/nsStyleConsts.h
> #define NS_STYLE_BORDER_STYLE_HIDDEN            9
>-#define NS_STYLE_BORDER_STYLE_BG_INSET          10
>-#define NS_STYLE_BORDER_STYLE_BG_OUTSET         11
>-#define NS_STYLE_BORDER_STYLE_BG_SOLID          12
> #define NS_STYLE_BORDER_STYLE_AUTO              13 // for outline-style only

Make NS_STYLE_BORDER_STYLE_AUTO be 10?

>Index: layout/base/nsCSSRendering.cpp
>+  if ((style == NS_STYLE_BORDER_STYLE_OUTSET) ||
>       (style == NS_STYLE_BORDER_STYLE_RIDGE)) {
>     // Flip colors for these three border styles

s/three/two/

>@@ -3687,29 +3571,20 @@ nsCSSRendering::RenderSide(nsFloatPoint 
>+          aRenderingContext.SetColor(MakeBevelColor(aSide, border_Style,
>+                                     bgColor->mBackgroundColor, sideColor));

That's a really confusing indent; please indent the second line two more spaces.  Yes, I know other places in this file use this crappy indent too...

r=bzbarsky with those nits.  I'd like dbaron to OK the html.css/quirk.css stuff, though.
Comment 22 fantasai 2006-12-05 17:43:05 PST
Created attachment 247609 [details] [diff] [review]
patch to remove -moz-bg-inset/outset/solid  (+bz)

Patch updated to current tree, also addresses your comments, bz, except the quirk one. The reason I have strict mode using currentColor is to fix bug 190655.
Comment 23 Boris Zbarsky [:bz] 2006-12-05 20:32:58 PST
Yes, I realize why you're doing it.  I question whether that fix is worth introducing a quirk, and I question expanding that quirk to tables....
Comment 24 fantasai 2006-12-06 00:02:08 PST
> question whether that fix is worth introducing a quirk,

I'm not going to make that call, because I don't have a good enough understanding
of the cost of a quirk. I'll just note that it's a CSS one-liner, affects a very
common property, and is therefore super-easy to override either way.

> and I question expanding that quirk to tables....

I think tables and <hr> should be consistent in their coloring. They both use 3D
borders, and unless we change it here, they have always had consistent coloring.
Comment 25 Boris Zbarsky [:bz] 2006-12-06 07:58:35 PST
The main cost of a quirk is confusion for authors.

> I think tables and <hr> should be consistent in their coloring.

I frankly don't see why.

> they have always had consistent coloring.

Which is largely a historical accident, as far as I can tell.  But they're used very differently, and while it makes some sense to make hr use the current text color, it's not nearly as important for tables.

Frankly, I think we should stick with the opera/IE behavior for now (gray by default, all modes), and worry about bug 190655 in bug 190655 (at which point maybe we should introduce a quirk, and maybe we should just change the behavior period).  One bug per problem, eh?
Comment 26 fantasai 2006-12-06 14:08:52 PST
Created attachment 247729 [details] [diff] [review]
patch to remove -moz-bg-inset/outset/solid (+bz)

Done.
Comment 27 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-12-11 17:50:10 PST
Could you summarize what the intended behavior changes of the current patch are?
Comment 28 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-12-11 17:50:32 PST
(and what they make us more/less compatible with)
Comment 29 fantasai 2006-12-14 23:12:50 PST
The patch removes support for -moz-bg-inset/outset/solid.
It instead uses 'gray' + inset/outset for tables and <hr>.
This makes us more compatible with Opera and Konqueror and IE6 (iirc, Bernd?).
It makes us less compatible with Netscape.
Comment 30 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-01-18 10:28:54 PST
Comment on attachment 247729 [details] [diff] [review]
patch to remove -moz-bg-inset/outset/solid (+bz)

sr=dbaron.  Sorry for the delay.

It looks like the code in nsHTMLHRElement.cpp that you're modifying might be removable.  If you want to investigate that as a followup rather than before landing this, that's fine.

It would also be nice to have some testcases for default behavior in the reftests -- particularly testcases that have been verified to match exactly on other browsers.
Comment 31 fantasai 2007-01-19 13:48:49 PST
Created attachment 252096 [details] [diff] [review]
patch to remove -moz-bg-inset/outset/solid (up-to-date, fix <HR color>)

Found a bug in my previous patch: I needed to switch 'color' handling to use border-color instead of color. This patch includes that fix; it's also up to date so it should (hopefully) patch without any conflicts. dbaron and/or bz, can you please review this?
Comment 32 Nickolay_Ponomarev 2007-01-24 08:56:47 PST
Removing [checkin needed] for now, due to previous comment.

Note: if you need re-reviews you should request them, a review request is less likely to get lost than a bug comment.
Comment 33 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-01-24 23:41:40 PST
It seems like the new change in nsHTMLHRElement should set all 4 border colors -- it won't hurt in the normal case, and it'll work better if the color attribute is mixed with, say, a border-style in CSS.  So I'd do:

if (colorIsSet) {
  nsCSSRect& borderColor = aData->mMarginData->mBorderColor;
  NS_FOR_CSS_SIDES(side) {
    nsCSSValue& side = borderColor.*(nsCSSRect::sides[side]);
    if (side.GetUnit() == eCSSUnit_Null) {
      side.SetColorValue(color);
    }
  }
}

r+sr=dbaron on the new addition with that change
Comment 34 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-01-24 23:44:07 PST
Though, actually, I think the new change is wrong.  Why do you need it?  You should make everything low in the cascade use 'color' rather than 'border-color' so that either property works.  Can you fix this issue another way?
Comment 35 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-01-24 23:45:48 PST
In particular, if you change this line in html.css:

+  border: 1px inset gray;

to:

+  border: 1px inset;
+  color: gray;

I'd think it would also fix what you were fixing.
Comment 36 fantasai 2007-01-29 10:42:32 PST
Created attachment 253202 [details] [diff] [review]
patch to remove -moz-bg-inset/outset/solid (use color, not border-color)

New patch, following dbaron's suggestion. Please review/checkin?
Comment 37 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-01-31 18:41:57 PST
So, I was about to check this in, but there's one thing I'm not so happy about:  changing the default border color for tables in standards mode.  We've been able to use foreground-color-based borders in standards mode up to now, and they have significant advantages -- they do the right thing in more cases.  Could we put the rule making table, td, and th borders gray in quirks.css instead of in html.css ?  I realize it means we're incompatible, but I think it's a better behavior and that the compatibility behavior should remain a quirk.

What do you think about that change?

(I have this in my tree, all ready to land.  I still might just land it.)
Comment 38 fantasai 2007-02-01 10:12:58 PST
That's fine with me. Do you want to just make the change, or should I send you a patch?
Comment 39 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-02-01 12:20:41 PST
Created attachment 253650 [details] [diff] [review]
followup patch to move gray table borders to quirks mode

I landed the previous patch last night.

This restores our standards mode behavior for table borders, since I think using the foreground color is more likely to work in more cases.  I have somewhat mixed feelings about undoing this change, since all other browsers seem to do it, but we haven't gotten in trouble for doing it better that I know of...
Comment 40 Boris Zbarsky [:bz] 2007-02-01 18:55:52 PST
Comment on attachment 253650 [details] [diff] [review]
followup patch to move gray table borders to quirks mode

>+ * Make table borders gray for compatibility with other browsers (in all
>+ * modes)

Maybe:

  Make table borders gray for compatibility with what
  other browsers do in all modes

?

sr=bzbarsky
Comment 41 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-02-01 22:16:31 PST
Above patch checked in to trunk, with that comment change.
Comment 42 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-02-01 22:17:42 PST
And marking fixed -- thanks for the patch, fantasai.
Comment 43 fantasai 2007-02-03 04:41:56 PST
Thanks for the thorough review, David. I'd say half the quality in this fix is yours. :)
Comment 44 Justin Wood (:Callek) 2007-06-11 20:28:48 PDT
Comment on attachment 253202 [details] [diff] [review]
patch to remove -moz-bg-inset/outset/solid (use color, not border-color)

>Index: layout/tables/nsTableFrame.cpp
>===================================================================

....

> // will return the priority of NS_STYLE_BORDER_STYLE_SOLID. 
> static PRUint8 styleToPriority[13] = { 0,  // NS_STYLE_BORDER_STYLE_NONE
>-                                       3,  // 

Would it be worth making this

static PRUint8 styleToPriotity[10] = ...

now that we don't need all 13 spots, or is the savings not worth the effort?
Comment 45 Justin Wood (:Callek) 2007-06-11 20:32:37 PDT
after having posted that comment, I now realize this bug is a few months old, so it might not even be valid now.

/me thinks he should stop posting places when he is half asleep.
Comment 46 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-06-11 20:56:10 PDT
But if you're fixing that, you should also make it const.

Note You need to log in before you can comment on or make changes to this bug.