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?

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.

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: abc<br><br><br>def ...renders as: +---------------------------------------+ | +-------+ + | | | a b c | | | | +-------+ + | | + | | | | | + | | + | | | | | + | | +-------+ | | | d e f | | | +-------+ | +---------------------------------------+ (where the lines are construction lines, only designed to show where the boxes are.) Also: <img><br>def ...renders as +---------------------------------------+ | +------------+ | | | IMG with | | | | v-align | | | | bottom | | | | and inline | + | | | display | | | | +------------+ + | | +-------+ | | | d e f | | | +-------+ | +---------------------------------------+ Related pages: http://www.bath.ac.uk/%7Epy8ieh/internet/projects/mozilla/br.html http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/hrbrstyles.html 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: http://www.bath.ac.uk/%7Epy8ieh/internet/projects/mozilla/hr.html http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/hrbrstyles.html 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.

*** Bug 2590 has been marked as a duplicate of this bug. ***

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

We'll do this for the next release. It's a minor issue compared to the other things we need to fix for rtm.

Ian: 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 implementation. Oops. Anyway: http://lists.w3.org/Archives/Public/www-style/2000Mar/0006.html <BR clear="left"> Clear doesn't apply to inline-level elements. [So BR can't be inline-level) [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.

adding some keyword nominations

Asa: Why do you consider this bug to be important for 0.9?

removing nomination, misread of the report. please excuse the spam.

Buster: Are you actually doing some work on this right now?

nada, as indicated by the "future" milestone

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?

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

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

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.

Please test the performance consequences of doing this, especially on long pages.

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

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

*** Bug 103546 has been marked as a duplicate of this bug. ***

*** Bug 105413 has been marked as a duplicate of this bug. ***

Bug 119606 has an example which uses colors.

*** 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 better. * This patch doesn't remove the hacky centering code in nsLineLayout, but it should. * |#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 obsolete.

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

-moz-bg-inset is treated as a special case at http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSRendering.cpp#489 which invokes at http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSRendering.cpp#199 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

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

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

*** Bug 193200 has been marked as a duplicate of this bug. ***

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.

Yeah, it compiles fine. I'll run with it for a while to make sure there's no unexpected regressions.

Notes: 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 fix microsoft.com. 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.

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.

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.

(That screenshot was taken after changing font zoom to work around bug 209066.)

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.

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 www.microsoft.com 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.

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 nsIDOMNSHTMLHRElement >+ else if (aAttribute == nsHTMLAtoms::color) { >+ if (aResult.ParseColor(aValue, mDocument)) { >+ return NS_CONTENT_ATTR_HAS_VALUE; >+ } >+ } 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; PRBool. >+ } 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 ?

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

*** Bug 213078 has been marked as a duplicate of this bug. ***

Latest, greatest. This exposes another layout bug, though. I think bz is going to file that shortly and post the link here.

Comment on attachment 128831 [details] [diff] [review] Patch >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 <vidur@netscape.com> (original author) >+ * Johnny Stenback <jst@netscape.com> 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

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.