Closed Bug 84307 Opened 23 years ago Closed 18 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: conorlennon, Assigned: fantasai.bugs)

References

(Blocks 3 open bugs)

Details

(Keywords: testcase)

Attachments

(4 files, 4 obsolete files)

testcast containing html
122 bytes, text/html
Details
testcase with CSS-applied border
143 bytes, text/html
Details
patch to remove -moz-bg-inset/outset/solid (use color, not border-color)
35.09 KB, patch
dbaron
: superreview+
Details | Diff | Splinter Review
followup patch to move gray table borders to quirks mode
956 bytes, patch
fantasai.bugs
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review 
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

    
    

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

    
    

    
  
I think this is quirks mode only

    
    

    
  
fantasai can you help triaging the bug? Thanks
Component: HTMLTables → ActiveX Wrapper

    
    

    
  


  

    
    

    
  
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

    
    

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

    
    

    
  
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)

    
    

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

    
    

    
  
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. :-) 



    
  
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0

    
    

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

    
  
Target Milestone: mozilla1.0 → mozilla1.0.1

    
  
Blocks: 55623

    
    

    
  
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"

    
    

    
  
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?

    
    

    
  
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 → ---

    
    

    
  
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....



    
  
Blocks: 252530

    
  
Assignee: dbaron → fantasai.bugs

    
    

    
  
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

    
    

    
  
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.

    
    

    
  


  
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.

    
  

        Attachment #246171 -
        Flags: review?(dbaron)

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

    
  
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+

    
    

    
  


  
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)

    
  
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....

    
    

    
  
> 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?

    
    

    
  


  
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)

    
    

    
  
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.

    
  
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+

    
    

    
  


  
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

    
    

    
  
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.

    
    

    
  


  
New patch, following dbaron's suggestion. Please review/checkin?

        Attachment #252096 -
        Attachment is obsolete: true
Whiteboard: [checkin needed]

    
    

    
  
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.)

    
    

    
  
That's fine with me. Do you want to just make the change, or should I send you a patch?

    
    

    
  


  
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)

    
  

        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
Closed: 18 years ago
Resolution: --- → FIXED

    
    

    
  
And marking fixed -- thanks for the patch, fantasai.

    
  
Whiteboard: [checkin needed]

    
    

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

    
  
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.

    
  
Blocks: 188715

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: