Closed Bug 38370 Opened 25 years ago Closed 21 years ago

Can't change <HR> and <BR> color


(Core :: Layout, defect, P3)






(Reporter: supersamat, Assigned: caillon)


(Blocks 1 open bug)


(4 keywords, Whiteboard: [Hixie-P1] [fix almost in hand] BLOCKED)


(6 files, 7 obsolete files)

478 bytes, text/html
2.33 KB, text/html
31.70 KB, image/png
16.08 KB, image/png
69.39 KB, patch
Details | Diff | Splinter Review
82.70 KB, patch
: review+
: superreview+
Details | Diff | Splinter Review
In html.css, the HR is defined to be 1px wide and have a color of -moz-bg-inset. 
 Since color: or background-color: (on thinner HRs) does not work on an HR, 
border-color: should change the -moz-bg-inset color to another color.

My pre-M16 nightly does not seem to let background-color: change anything with 
the HR. Setting background-color: does change the shade of the border, but 
border-color: doesn't. Attached testcase demonstrates (with workaround).

Also, using border-color: to set the color of an HR is a bit illogical... Yes, 
HR is a box element (I think) and is rendered that way and that is the way it 
has to be done. Web developers unfamiliar with how Mozilla works may not know to 
use it though. With IE, it lets you simply use a color: to assign a color to the 
HR. This is much more logical; if I wanted to make a 2px high HR that is black, 
in IE I could write: "hr { height: 2px; color: black; }". Perhaps something with 
how Mozilla handles HRs may be useful?
Attached file Testcase
looks like a dup of bug 2590

Yeah, forgot to mention it may be related to 2590... HRs can be styled using a 
full border style declaration (ex border: solid 1px black) but this causes 
problems in Nav4. border-color: (which should work anyway) or color: would be 
nicer to use.
It's a rendering problem: reassigned to buster.
See bug 38370 for related comments.
Assignee: pierre → buster
Component: Style System → Layout
Ever confirmed: true
Summary: Can't change HR color → Can't change HR and BR color
The fix for this bug should be relatively simple. At the moment, we are treating
HR and BR as special cases -- in the case of BR, this is completely unnecessary,
and in the case of HR, it is only required for making sure that the element does
not overlap floats (ok, so that is not easy, but it is all that needs doing).

Here is a summary of possible ways of implementing these two elements in a
spec compliant way, while keeping full backwards compatibility:

  == BR ==
Simply define BR as...

   BR:before { display: inline; content: "\A"; }

...and then treat BRs the same way as empty DIVs. Kipp and I discussed how
BRs affect the lines they are in, and I drew some diagrams which I will insert
here too:


...renders as:

   | +-------+ +                           |
   | | a b c | |                           |
   | +-------+ +                           |
   | +                                     |
   | |                                     |
   | +                                     |
   | +                                     |
   | |                                     |
   | +                                     |
   | +-------+                             |
   | | d e f |                             |
   | +-------+                             |

(where the lines are construction lines, only designed to show where
the boxes are.)



...renders as

   | +------------+                        |
   | | IMG with   |                        |
   | | v-align    |                        |
   | | bottom     |                        |
   | | and inline | +                      |
   | | display    | |                      |
   | +------------+ +                      |
   | +-------+                             |
   | | d e f |                             |
   | +-------+                             |

Related pages:
 bug 4247 - Empty inline elements are not affecting the inline box model 
 bug 4248 - Trailing spaces on inline elements wrapping @ boundary badly treated
 bug 2590 - <HR> and <BR> are unstylable!!!
 bug 26996 - </font><br><font> not fixed by temporary bug-24186 fix
 bug 15428 - line-height values less than 1 are wrong  (see comments near end)

  == HR ==
This is more complicated, because the attributes do not map directly onto
the CSS model. Again, the HR can be treated as an empty primitive element,
except that some attribute mapping must be performed, and the width must be 
based on available width minus the width of floats. For each attribute, here
is the mapping I would recommend (applied in this order):

   /* default ua.css */
   hr { border-style: 1px -moz-bg-inset; margin: auto; display: block; }

   /* noshade */
   #thishr { -moz-border-radius: 100%; border: none;
             background: -moz-shade-bg; }

   /* align=left */
   #thishr { margin-left: 0; margin-right: auto; }

   /* align=right */
   #thishr { margin-left: auto; margin-right: 0; }

   /* align=center */
   #thishr { margin-left: auto; margin-right: auto; }

   /* size */
   #thishr { height: <value of size attribute>; }

   /* width */
   #thishr { width: <value of width attribute>; }

   /* color */
   #thishr { color: <value of color attribute>;
             background: <value of color attribute>; }

The following mozilla specific keywords apply:

-moz-bg-inset (<border-style> keyword)
   Already implemented. Emulates nav4's 3D border style for HRs (whatever
   that is).

-moz-border-radius (property)
   Already implemented. Causes background and borders to be rounded.

-moz-shade-bg (suggested <color> value for 'background' property)
   Should emulate the nav4 colour selection code for noshade style HRs,
   whatever that is.

Note that since HR is a block-level element, the font-size will not have
any effect on rendering.

Related pages:
 bug 2590
Changing BRs is not just that simple.  One must also be careful of the
quirks-mode stuff for dealing with BR.  Do we support ' content: "\n"; '? 
Should we?
Oops, minor error on my part. BR should be
   BR:before { display: inline; content: "\A"; white-space: pre; }
...note the white-space declaration.

We support content: "\A", is that what you meant?

There are some issues with <br> and quirks mode, yes (as you know, since you
wrote most of that code...). The above only applies to strict mode, it almost
certainly requires tweaking to "work" in quirks mode.
Summary: Can't change HR and BR color → Can't change <HR> and <BR> color
*** Bug 2590 has been marked as a duplicate of this bug. ***
Blocks: 22181
Depends on: 20582
QA Contact: chrisd → py8ieh=bugzilla
Refering back to the orriginal post...."background-color:" sets the fill color 
of a HR and not "color:" 

Is this the way it should be?
Taking a stab at prioritizing buster's nsbeta3 bugs...
Whiteboard: [nsbeta3-]
We'll do this for the next release. It's a minor issue compared to the other
things we need to fix for rtm.
Target Milestone: --- → Future
Blocks: 18150
Keywords: ns6.01

You write:
'Simply define BR as...

   BR:before { display: inline; content: "\A"; }'

but you seem to have missed several explanations as to why this doesn't work, 
and, in fact, breaks either (a) countless existing pages or (b) Mozilla's clear 


<BR clear="left">

Clear doesn't apply to inline-level elements. [So BR can't be

[And there are others that I can't find at the moment.]

At the W3C face to face meeting this week, it was mentioned that we could even
do the floats thing, by using -moz-float-edge. So fixing this bug may actually
cover _all_ the bases for HR and BR.
Keywords: ns601highrisk, mozilla0.9
adding some keyword nominations
Keywords: nsbeta3mozilla0.9, nsbeta1
Asa: Why do you consider this bug to be important for 0.9?
removing nomination, misread of the report. please excuse the spam.
Keywords: mozilla0.9, nsbeta1
Blocks: 63434
Blocks: 62845
Keywords: mozilla1.0
Whiteboard: [nsbeta3-] → (py8ieh: ho hum)
Buster: Are you actually doing some work on this right now?
nada, as indicated by the "future" milestone
Depends on: 69491
Depends on: 69518
Taking bug. I have a patch which fixes this, modulo dependencies on bug 69491 
and bug 69518. I shall attach it presently, but checking it in without fixing
the other two bugs would break backwards compatability.

Nevertheless, I would appreciate it if people could look over the patch and 
give me any comments suggesting possible improvements. David? Marc?
Assignee: buster → ian
Keywords: patch
OS: Windows 98 → All
QA Contact: ian → dbaron
Hardware: PC → All
Whiteboard: (py8ieh: ho hum)
Target Milestone: Future → mozilla1.0
No longer depends on: 20582
Blocks: 20582
I'll review the changes. It will take a while as they are pretty extensive. BTW:
since when does Hixie write code? We are all in trouble now ;)
Depends on: 69398
Hmm, just found another bug that is blocking the checkin of this fix. So far,
the following bugs are causing problems that block this fix from going in:
 bug 69398: label:after {content: ":"} repeats creation of : with mouse movement
 bug 69491: -moz-float-edge sucks with auto margins and left floaters
 bug 69518: br:before stiffles dynamic changes of the clear attribute
Whiteboard: BLOCKED
BTW, I'm keeping this live in my tree through all the massive changes, so if
the patch attached to this bug stops working at any point just ask me to create
a new one.
Whiteboard: BLOCKED → [fix in hand] BLOCKED
Whiteboard: [fix in hand] BLOCKED → [Hixie-P4] [fix in hand] BLOCKED
No longer blocks: 20582
Depends on: 20582
Depends on: 53610
Blocks: 55475
Blocks: 76244
Please test the performance consequences of doing this, especially on long 
waterson: this is the HR bug you asked about. The patch no longer applies 
because hyatt rewrote the attribute mapping code, but in theory it should be
possible to update it. The only problem then is the four blocking bugs, all of
which are serious in their own right anyway.
Well, this bug covers _both_ <hr> and <br>; do they need to be conflated? (Seems
like we could fix one without the other...)
Blocks: 71979
They can be fixed separately, yes. The fixes are quite similar (as my now 
defunct patch demonstrated).
*** Bug 91885 has been marked as a duplicate of this bug. ***
Reassigning to waterson.

waterson: In theory, the only thing that needs doing is to take my patch and
make it work with hyatt's rule tree change. That should only involve the first
file affected by the patch (nsHTMLHRElement.cpp). Beyond that the "only" changes
required are those to fix the bugs blocking this one.

Needless to say, the hard bit is the fixing of the blocking bugs...
Assignee: ian → waterson
QA Contact: dbaron → ian
Whiteboard: [Hixie-P4] [fix in hand] BLOCKED → [Hixie-P1] [fix almost in hand] BLOCKED
*** Bug 103546 has been marked as a duplicate of this bug. ***
*** Bug 105413 has been marked as a duplicate of this bug. ***
Blocks: 78992
Bug 119606 has an example which uses colors.
Target Milestone: mozilla1.0 → Future
*** Bug 142338 has been marked as a duplicate of this bug. ***
Is there any workaround for being able to set the color of an HR? If I use
background-color (the only thing that seems to work in mozilla), it breaks the
HR in Explorer (at least on a Mac). Specifically, Explorer seems to treat the
whole line as the background, not just the horizontal rule, so even if your HR
is less that 100% wide, the color goes all the way to the edges of the page. It
looks like there's no way to get it to work on both browsers since their
behavoirs are mutually exclusive. Can anyone verify or disprove this?
border-color should work... does it?
Comment on attachment 25714 [details] [diff] [review]
Patch: removes nsHRFrame and (in standard mode) nsBRFrame, and replaces them with CSS

A few comments on this:

 * GetMutableStyleData is evil.  The attribute mapping functions would be much

 * This patch doesn't remove the hacky centering code in nsLineLayout, but it

 * |#if 0| and "// XXX not part of build" are unnecessary.  Just delete and
remove from build. :-)

I didn't read the whole thing, though...
HR {border-style: solid; border-color: #000000; background-color:Black;}
works fine
BUT if you do a <hr noshade> it turns to a light to dark grey depending if you
have  background or not
Comment on attachment 25714 [details] [diff] [review]
Patch: removes nsHRFrame and (in standard mode) nsBRFrame, and replaces them with CSS

dbaron: That was the patch I wrote before hyatt changed the style system. It is
Attachment #25714 - Attachment is obsolete: true
*** Bug 174423 has been marked as a duplicate of this bug. ***
*** Bug 177174 has been marked as a duplicate of this bug. ***
*** Bug 182278 has been marked as a duplicate of this bug. ***
*** Bug 186716 has been marked as a duplicate of this bug. ***
*** Bug 187670 has been marked as a duplicate of this bug. ***
Keywords: mozilla1.0
Attached patch patch to allow coloring of <hr> (obsolete) — Splinter Review
-moz-bg-inset is treated as a special case at

which invokes at

a rendering based only on the background color. The only place where this
should be needed are tables in quirks mode, because IE and NN render them so
Changing the default border style to inset invokes the usual code that tables
use in standards mode and all other element in any mode. Please note also that
with this change the hr will really look like inset, they look currently like
outset due to bug 14090
Attachment #111282 - Flags: review?(ian)
removal of quirks is always fine by me -- r=hixie
However, this shouldn't be considered to fix the bug. For that we need to port
attachment 25714 [details] [diff] [review].
Attachment #111282 - Flags: superreview?(bzbarsky)
Comment on attachment 111282 [details] [diff] [review]
patch to allow coloring of <hr>

Please post screenshots of rendering <hr> on the following backgrounds: black,
white, red, green, cyan with this patch and without (and if possible in IE and
Attached file consequences of the previous patch (obsolete) —
Attachment #111282 - Flags: superreview?(bzbarsky) → superreview+
Attachment #111282 - Flags: review?(ian)
*** Bug 193200 has been marked as a duplicate of this bug. ***
Blocks: 60992
No longer depends on: 69518
This patch doesn't yet remove the hacky centering code in nsLineLayout, but it
updates the previous patch to the tip and addresses the other issues dbaron
mentioned. It also adds a new test case (for <br> in standard mode). I'll look
at the nsLineLayout stuff now, I just wanted to back this up first.

This patch accidentally fixes bug 69518 in quirks mode. Ironically, in standard
mode it doesn't (even though in standard mode BRs are done using normal CSS
frames). Also, the patch introduces some tabs. I'll clean those out before
asking for a review.

If anybody knows of any testcases for HR or BR, let me know. Cheers.
How does this affect HR alignment in the presence of floats?
In theory, it works as before, due to the use of -moz-float-edge: margin-box.
Untested; I'm updating to tip and building to check these latest changes work.

Anyway, in theory, this removes the remaining mentions of HR frames, e.g. in
nsLineLayout. It also removes a few tabs I came across.
Attachment #125340 - Attachment is obsolete: true
Yeah, it compiles fine. I'll run with it for a while to make sure there's no
unexpected regressions.
Attachment #125341 - Flags: superreview?(dbaron)
Attachment #125341 - Flags: review?(bz-bugspam)

  For size<2, for IE compatability I could nuke the side and bottom borders and 
  just have the top one. (Trunk doesn't support size<3 at the moment.) This would

  For IE compatability, we could default to size=2 instead of size=3.

  Could support color attribute, in IE it just sets the color instead of using
  what I've called -moz-bg-solid.
Comment on attachment 125341 [details] [diff] [review]
Patch: removes all mention of HR frames and (in standard mode) replaced nsBRFrame with CSS

I'm going to do the first and last of those. New patch ccming up.
Attachment #125341 - Attachment is obsolete: true
Attachment #125341 - Flags: superreview?(dbaron)
Attachment #125341 - Flags: review?(bz-bugspam)
Does that patch fix bug 116909?  If so, there might be some lines of
nsBRFrame.cpp that can be removed (see large comment block).  If not, is there
an easy way to fix that issue along with this one?
No, it doesn't solve that bug, and I don't want to roll a fix for that bug into
this one (I'm not sure how to fix it). Incidentally, I just noticed that this
removes the almost standards mode check. I'll make BR frames be used in that
mode too instead of only quirks mode.
This fixes the issues I mentioned above.
Attachment #125417 - Flags: superreview?(dbaron)
Attachment #125417 - Flags: review?(bz-bugspam)
From left to right:
IE6/win32  Moz/win32  Moz/linux  Opera7/linux  NN4.78/linux

The red lines were added by me to help counting the pixels - it shows that we
are consistently drawing 1 pixel more than SIZE is set to, which is different
from other browsers.  I guess this explains why it currently looks as if the
default in Mozilla is SIZE=3.  Could you attach a screenshot of Testcase #3
with your patch?

Also note the difference to other browsers for SIZE=0 and SIZE=1.
Ok, more regressions: I need to add hr to the list of things that lose their
bottom margin in cells, and this patch causes newlines to act really oddly in
standard mode textareas. (Editor can't handle elements that introduce generated
content? Why am I not surprised.)
Actually no, they shouldn't lose their bottom margin. Something else odd is
going on here.
Depends on: 209066
Attachment #125417 - Flags: superreview?(dbaron)
Attachment #125417 - Flags: review?(bz-bugspam)
Attachment #111282 - Attachment is obsolete: true
Attachment #111302 - Attachment is obsolete: true
Attachment #125417 - Attachment is obsolete: true
(That screenshot was taken after changing font zoom to work around bug 209066.)
Depends on: 209073
Ok, this patch breaks multiple newlines in <textarea>s and multiple <br>s (due
to bug 209073), so it's not ready for review after all.
Blocks: 84887
This is just the previous patch, updated to the tip.

I'm going to extract the BR changes and just post a patch with the HR ones,
since they are more mature.
Comment on attachment 126235 [details] [diff] [review]
Just the HR part of the patch

Ok, this really should be ready for review. I removed the problematic part (the
<br> stuff). This patch removes the <hr> frame, using just CSS and attribute
mapping for its rendering. It also improves our compatability with IE, allowing
<hr>s to be smaller than before, and adding support for a "color" attribute.
This makes look much closer to its rendering in IE.

There are several parts I'm really not sure of, especially the bit where I
implement a new interface. I couldn't find any firm documentation on how to do
that, so this is mainly guesswork based on hints from bbaetz.
Attachment #126235 - Flags: review?(bzbarsky)
Blocks: 119606
Comment on attachment 126235 [details] [diff] [review]
Just the HR part of the patch

>Index: content/html/content/src/nsHTMLHRElement.cpp

>+#include "nsIDOMHTMLHRElement2.h"

For consistency with other such compat interfaces, please call that

>+  else if (aAttribute == nsHTMLAtoms::color) {
>+    if (aResult.ParseColor(aValue, mDocument)) {
>+    }
>+  }

That will work incorrectly when setting innerHTML, eg, since mDocument is null
then.  You need to get the document from mNodeInfo if mDocument is null (the
other callers of ParseColor() were also doing that incorrectly until recently).

>+  nsHTMLValue value;
>+  bool noshade;

PRBool, please.  And PR_TRUE/PR_FALSE later on.

>+      bool allSides = true;


>+      } else
>+        sizePerSide = 1; // default to a 2px high line

Put curly braces around the else body if the if body had them, please.	And
1.0f instead of 1 here.

>+      // if a color is set, set the border-style to 'solid' so that the
>+      // 'color' property takes effect, otherwise, use '-moz-bg-solid'.
>+      // (we got the color attribute earlier)
>+      int style;
>+      if ((color.GetUnit() == eHTMLUnit_Color) ||
>+          (color.GetUnit() == eHTMLUnit_ColorName))

We keep doing that check.. could we save that boolean up front somewhere?  Also
those tests are overparenthesized.

>+    if (((color.GetUnit() == eHTMLUnit_Color) ||
>+         (color.GetUnit() == eHTMLUnit_ColorName)) &&
>+        (aData->mColorData->mColor.GetUnit() == eCSSUnit_Null)) {

Again, overparenthesized conditionals.

>Index: layout/base/public/nsStyleConsts.h
> #define NS_STYLE_BORDER_STYLE_BG_INSET          11
> #define NS_STYLE_BORDER_STYLE_BG_OUTSET         12
>+#define NS_STYLE_BORDER_STYLE_BG_SOLID          13

It would be good to document what those three do....

>Index: layout/html/base/src/nsLineLayout.cpp
>-    // here is where we do special adjustments for HR's 
>-    // see bug 18754

I assume that you have tested that you are not regressing that?

>Index: layout/html/style/src/nsCSSRendering.cpp

>+    np = MakeSide (theSide, aContext, whichSide, borderOutside, borderInside,aSkipSides,

I know you just copied this, but add a space before aSkipSides.

>+      //aContext.DrawLine (theSide[0].x, theSide[0].y, theSide[1].x, theSide[1].y);
>+      DrawLine (aContext, theSide[0].x, theSide[0].y, theSide[1].x, theSide[1].y, aGap);

Why copy this commented thing?

> Index: dom/public/idl/html/nsIDOMHTMLHRElement.idl

>+        // attribute DOMString        color; (see nsIDOMHTMLHRElement2.idl)

Just remove this; don't comment it.

> Index: dom/public/idl/html/nsIDOMHTMLHRElement2.idl

>+        // attribute DOMString        align;
>+        // attribute boolean          noShade;
>+        // attribute DOMString        size;
>+        // attribute DOMString        width;

No need for those.

It looks like this patch fixes bug 84887, right?  And perhaps bug 119606 and
bug 165261 ?
Attachment #126235 - Flags: review?(bzbarsky) → review-
>>Index: layout/base/public/nsStyleConsts.h
>> #define NS_STYLE_BORDER_STYLE_BG_INSET          11
>> #define NS_STYLE_BORDER_STYLE_BG_OUTSET         12
>>+#define NS_STYLE_BORDER_STYLE_BG_SOLID          13
> It would be good to document what those three do....

The NS_STYLE_BORDER_STYLE_BG_* constants (-moz-bg-* keywords) are hacks that use
the background colour instead of the foreground colour when they paint. They
should probably be implemented as normal styles with a special colour keyword,
but for historical reasons, they are not.

>>Index: layout/html/base/src/nsLineLayout.cpp
>>-    // here is where we do special adjustments for HR's 
>>-    // see bug 18754
> I assume that you have tested that you are not regressing that?

I did test that things worked ok around floats, yes. (This patch uses
-moz-float-edge, which recently started working more reliably.)

> It looks like this patch fixes bug 84887, right?  And perhaps bug 119606 and
> bug 165261 ?

This should fix most of the dependencies, I think.

Anyway, for obvious reasons, I won't be able to fix this and check it in. Could
I ask you to do the fixups and checkin? They all seem pretty simple.
caillon: this is the bug i mentioned
Assignee: waterson → caillon
Blocks: 145531
*** Bug 213078 has been marked as a duplicate of this bug. ***
Attached patch PatchSplinter Review
Latest, greatest.  This exposes another layout bug, though.  I think bz is
going to file that shortly and post the link here.
Attachment #126235 - Attachment is obsolete: true
Blocks: 107122
Blocks: 142197
Blocks: 165261
Comment on attachment 128831 [details] [diff] [review]

>Index: content/html/content/src/nsHTMLHRElement.cpp

>+  PRBool noshade;

PRBool noshade = PR_FALSE;

please, to shut up the idiotic compiler and the people who will spam this bug
with "added a warning" crap.

>Index: layout/html/style/src/nsCSSRendering.cpp

>+            aRenderingContext.SetColor(MakeBevelColor(aSide, border_Style, bgColor->mBackgroundColor, sideColor, 
>+                                       MOZ_BG_BORDER(border_Style) ? PR_FALSE : PR_TRUE));

Ummm... how about making that last arg be "!MOZ_BG_BORDER(border_Style)" ?

>Index: dom/public/idl/html/nsIDOMNSHTMLHRElement.idl

>+ * The Initial Developer of the Original Code is 
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2003
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Vidur Apparao <> (original author)
>+ *   Johnny Stenback <>

All of that is a blatant lie.  Please fix to reflect reality (original
developer is hixie, there are no contributors except maybe caillon).

With those nits, r+sr=me.  The bug I was seeing was bug 209066
Attachment #128831 - Flags: superreview+
Attachment #128831 - Flags: review+
Blocks: 82333
Blocks: 184104
Since nobody else mentioned it on the bug, this was checked in to the trunk
2003-07-30 01:12/13 -0700.
From comment #32, the attached example (attachment id 64622) in bug 119606 still
shows issues with using the style attribute/css to color HR's. (See screenshots
for difference.)
bug 119606 is showing correct behavior, in fact.  If you want to style <hr> via
CSS, you need to style the borders, which are what you see.  Changing the
"color" property in CSS will not help there.

I'm marking this bug fixed, since it started out as an <hr> bug and the <hr>
part is fixed.  Please file follow-ups on the <br> problem, if any, as needed.
Closed: 21 years ago
Resolution: --- → FIXED
No longer depends on: 209073
You need to log in before you can comment on or make changes to this bug.