[quirks] can't set table border color in css

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
16 years ago
3 years ago

People

(Reporter: Conor Lennon, Assigned: fantasai)

Tracking

(Blocks: 3 bugs, {testcase})

Trunk
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

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

Comment 1

16 years ago
Created attachment 37354 [details]
testcast containing html

Comment 2

16 years ago
Linux too, OS/All. Is this a regression or something?
OS: Windows NT → All

Comment 3

16 years ago
I think this is quirks mode only

Comment 4

16 years ago
fantasai can you help triaging the bug? Thanks
Component: HTMLTables → ActiveX Wrapper
(Assignee)

Comment 5

16 years ago
Created attachment 42031 [details]
testcase with CSS-applied border
(Assignee)

Comment 6

16 years ago
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
Hardware: PC → All
Summary: table border colour not set correctly → table border color not set correctly

Comment 7

16 years ago
I am so (in)famous bug 90363 resetting category
Component: ActiveX Wrapper → HTMLTables

Comment 8

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

Comment 9

16 years ago
This bug is currently unassigned, so I doubt anyone's working on it right now.

Comment 10

16 years ago
reassigning to hyatt.
Assignee: karnaze → hyatt
Component: HTMLTables → Style System
I have encountered this bug using 2001080104 on Linux. 
Thanks to Waheed Islam for the 'border-style: solid'-workaround. :-) 

Updated

16 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0

Comment 12

16 years ago
don't thank me, thanks to fantasai for that css-applied border
testcase. ;)

Updated

16 years ago
Target Milestone: mozilla1.0 → mozilla1.0.1

Updated

16 years ago
Blocks: 55623

Comment 13

16 years ago
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

15 years ago
still happening on win2000 build id : 2002-04-22-1.0.0 branch build
Keywords: testcase
Priority: -- → P3
bernd, didn't I mention something like what comment 6 is proposing in some other
bug?  Is this one a duplicate?

Comment 16

13 years ago
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

Assignee: hyatt → dbaron
Status: ASSIGNED → NEW
Priority: P3 → --
QA Contact: amar → ian
Target Milestone: mozilla1.0.1 → ---

Comment 17

13 years ago
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....

(Assignee)

Updated

11 years ago
Blocks: 252530
(Assignee)

Updated

11 years ago
Assignee: dbaron → fantasai.bugs
(Assignee)

Comment 18

11 years ago
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.)
Summary: table border color not set correctly → separate bg-color from border style

Comment 19

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

Comment 20

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

Updated

11 years ago
Attachment #246171 - Flags: review?(dbaron)
(Assignee)

Updated

11 years ago
Assignee: fantasai.bugs → dbaron
Summary: separate bg-color from border style → [quirks] can't set table border color in css
(Assignee)

Updated

11 years ago
Assignee: dbaron → fantasai.bugs
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.
Attachment #246171 - Flags: superreview?(dbaron)
Attachment #246171 - Flags: review?(dbaron)
Attachment #246171 - Flags: review+
(Assignee)

Comment 22

11 years ago
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.
Attachment #246171 - Attachment is obsolete: true
Attachment #246171 - Flags: superreview?(dbaron)
Attachment #247609 - Flags: superreview?(dbaron)
(Assignee)

Updated

11 years ago
Blocks: 190655
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....
(Assignee)

Comment 24

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

Comment 26

11 years ago
Created attachment 247729 [details] [diff] [review]
patch to remove -moz-bg-inset/outset/solid (+bz)

Done.
Attachment #247609 - Attachment is obsolete: true
Attachment #247609 - Flags: superreview?(dbaron)
Attachment #247729 - Flags: superreview?(dbaron)
Could you summarize what the intended behavior changes of the current patch are?
(and what they make us more/less compatible with)
(Assignee)

Comment 29

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

Updated

11 years ago
Blocks: 155318
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.
Attachment #247729 - Flags: superreview?(dbaron) → superreview+
Whiteboard: [checkin needed]
(Assignee)

Comment 31

10 years ago
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?
Attachment #247729 - Attachment is obsolete: true

Comment 32

10 years ago
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.
Whiteboard: [checkin needed]
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
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?
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.
(Assignee)

Comment 36

10 years ago
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?
Attachment #252096 - Attachment is obsolete: true

Updated

10 years ago
Whiteboard: [checkin needed]
Attachment #253202 - Flags: superreview+
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.)
(Assignee)

Comment 38

10 years ago
That's fine with me. Do you want to just make the change, or should I send you a patch?
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...
Attachment #253650 - Flags: superreview?(bzbarsky)
Attachment #253650 - Flags: review?(fantasai.bugs)
(Assignee)

Updated

10 years ago
Attachment #253650 - Flags: review?(fantasai.bugs) → review+
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
Attachment #253650 - Flags: superreview?(bzbarsky) → superreview+
Above patch checked in to trunk, with that comment change.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
And marking fixed -- thanks for the patch, fantasai.

Updated

10 years ago
Whiteboard: [checkin needed]
(Assignee)

Comment 43

10 years ago
Thanks for the thorough review, David. I'd say half the quality in this fix is yours. :)

Updated

10 years ago
Blocks: 370833
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?
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.
But if you're fixing that, you should also make it const.

Updated

8 years ago
Blocks: 188715
You need to log in before you can comment on or make changes to this bug.