Bug 17674 – <hr> appears gray instead of green because of color attribute is defined followed by noshade attribute

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[jasneet]

I Steps:
Go to
http://niwango.jp/pc/niwanews/search.php?id=14875

II Issue:
Notice the border outside marquee text box is gray. In FF/IE it is green.

III Conclusion: Removing noshade="noshade" solves the issue.

IV Other browsers: 
FF2: ok
Opera: not ok
IE 7: ok

V Nightly tested: 30628

[jasneet]

Created attachment 19536 [details]
screenshot

[jasneet]

Created attachment 19537 [details]
reduction

[Robert Blaut]

I can confirm the issue as incompatibility bug. Gecko, Presto (Opera 9.5) and Trident color a hr element in the attached test case.

However this particular case rises a general problem – coloring of hr elements is a huge incompatiblity area between browsers. Check for example this test case:

http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cp%3EColoring%20of%20%20hr%20element%20without%20noshade%20attribute%0A%3Chr%20color%3D%22red%22%20style%3D%22height%3A%20.5em%3B%22%3E%0A%3Chr%20bgcolor%3D%22red%22%20style%3D%22height%3A%20.5em%3B%22%3E%0A%3Chr%20style%3D%22color%3A%20red%3B%20height%3A%20.5em%3B%22%20%3E%0A%3Chr%20style%3D%22background-color%3A%20red%3B%20height%3A%20.5em%3B%22%3E%0A%3Cp%3EColoring%20of%20hr%20element%20with%20noshade%20attribute%0A%3Chr%20color%3D%22red%22%20style%3D%22height%3A%20.5em%3B%22%20noshade%3E%0A%3Chr%20bgcolor%3D%22red%22%20style%3D%22height%3A%20.5em%3B%22%20noshade%3E%0A%3Chr%20style%3D%22color%3A%20red%3B%20height%3A%20.5em%3B%22%20noshade%3E%0A%3Chr%20style%3D%22background-color%3A%20red%3B%20height%3A%20.5em%3B%22%20noshade%3E

Every single tested engine (Gecko, Presto, Webkit, Trident) displays it completely differently. Moreover HTML5 removes noshade and other attributes of hr element. The open question is: how we should deal with all these issues?

[Robert Blaut]

After digging into HTMLHRElement.cpp 

} else if (attr->name() == colorAttr) {
        addCSSProperty(attr, CSS_PROP_BORDER_TOP_STYLE, CSS_VAL_SOLID);
        addCSSProperty(attr, CSS_PROP_BORDER_RIGHT_STYLE, CSS_VAL_SOLID);
        addCSSProperty(attr, CSS_PROP_BORDER_BOTTOM_STYLE, CSS_VAL_SOLID);
        addCSSProperty(attr, CSS_PROP_BORDER_LEFT_STYLE, CSS_VAL_SOLID);
        addCSSColor(attr, CSS_PROP_BORDER_COLOR, attr->value());
        addCSSColor(attr, CSS_PROP_BACKGROUND_COLOR, attr->value());
    } else if (attr->name() == noshadeAttr) {
        addCSSProperty(attr, CSS_PROP_BORDER_TOP_STYLE, CSS_VAL_SOLID);
        addCSSProperty(attr, CSS_PROP_BORDER_RIGHT_STYLE, CSS_VAL_SOLID);
        addCSSProperty(attr, CSS_PROP_BORDER_BOTTOM_STYLE, CSS_VAL_SOLID);
        addCSSProperty(attr, CSS_PROP_BORDER_LEFT_STYLE, CSS_VAL_SOLID);
        addCSSColor(attr, CSS_PROP_BORDER_COLOR, String("grey"));
        addCSSColor(attr, CSS_PROP_BACKGROUND_COLOR, String("grey"));


I found that the problem will disappear if you define: <hr noshade color="red">. The construction <hr color="red" noshade> is buggy

It seems the bug is easy to fix.

[Pruthviraj S P]

Hi

I was analysing this bug. Going through the comments already made, I feel the behaviour the test case in safari correct. Initially we are giving color attribute to HR element and later giving the attribute noshade which means we dont want color attribute. I think this behaviour is fine.
Moreover noshade is deprecated in HTML5.Let me know whether it is still bug and if so the expected behaviour.

Thanks

Created attachment 219966 [details]
Patch

Please review.Thanks

[Darin Adler]

Comment on attachment 219966 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=219966&action=review

This patch might be an OK start, but I need to see more test cases, because I don’t think these are handled correctly:

1) Cases where color and noshade attribute are added in different sequence.
2) Invalid color attribute; present but not something that we can parse.
3) Color set by CSS rather than with a color attribute.

I’m not sure what the right concept is here. Is the color attribute special, or is any color specified in either an attribute or in CSS special?

> Source/WebCore/html/HTMLHRElement.cpp:85
> +        if (nullAtom == getAttribute(colorAttr)) {

The efficient way to write this is:

    if (!hasAttribute(colorAttr))

> Source/WebCore/html/HTMLHRElement.cpp:90
> +            style.setProperty(CSSPropertyBorderColor, darkGrayValue);
> +            style.setProperty(CSSPropertyBackgroundColor, darkGrayValue);

This seems OK if the noshade attribute is handled after the color attribute. But what will remove these properties if someone later adds a color attribute to the <hr> element?

(In reply to comment #8)
> (From update of attachment 219966 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219966&action=review
> 
> This patch might be an OK start, but I need to see more test cases, because I don’t think these are handled correctly:
> 
> 1) Cases where color and noshade attribute are added in different sequence.
> 2) Invalid color attribute; present but not something that we can parse.
> 3) Color set by CSS rather than with a color attribute.
> 
> I’m not sure what the right concept is here. Is the color attribute special, or is any color specified in either an attribute or in CSS special?
> 
> > Source/WebCore/html/HTMLHRElement.cpp:85
> > +        if (nullAtom == getAttribute(colorAttr)) {
> 
> The efficient way to write this is:
> 
>     if (!hasAttribute(colorAttr))
> 
> > Source/WebCore/html/HTMLHRElement.cpp:90
> > +            style.setProperty(CSSPropertyBorderColor, darkGrayValue);
> > +            style.setProperty(CSSPropertyBackgroundColor, darkGrayValue);
> 
> This seems OK if the noshade attribute is handled after the color attribute. But what will remove these properties if someone later adds a color attribute to the <hr> element?

Hi Darin.Thanks for the review.
Cases where color and noshade attribute are added in different sequence : I have added test cases for noshade and color attribute in different sequence.
header-color-noshade-attribute.html : in this color attribute is mentioned before noshade
header-noshade-color-attribute.html : in this color attribute is mentioned after noshade .

Invalid color attribute; present but not something that we can parse : Incase of invalid color attribute default black color is applied.

Color set by CSS rather than with a color attribute : color set via css is not handled right now.

But what will remove these properties if someone later adds a color attribute to the <hr> element? : If hr is with no attribute and we add it through javascript then if color attribute is added it will be applied else the noshade(gray color) will be applied.

The color and noshade attribute both have been deprecated and it should be used via css.
http://www.w3.org/TR/html-markup/hr.html.

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 219966 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=219966&action=review
> > 
> > This patch might be an OK start, but I need to see more test cases, because I don’t think these are handled correctly:
> > 
> > 1) Cases where color and noshade attribute are added in different sequence.
> > 2) Invalid color attribute; present but not something that we can parse.
> > 3) Color set by CSS rather than with a color attribute.
> > 
> > I’m not sure what the right concept is here. Is the color attribute special, or is any color specified in either an attribute or in CSS special?
> > 
> > > Source/WebCore/html/HTMLHRElement.cpp:85
> > > +        if (nullAtom == getAttribute(colorAttr)) {
> > 
> > The efficient way to write this is:
> > 
> >     if (!hasAttribute(colorAttr))
> > 
> > > Source/WebCore/html/HTMLHRElement.cpp:90
> > > +            style.setProperty(CSSPropertyBorderColor, darkGrayValue);
> > > +            style.setProperty(CSSPropertyBackgroundColor, darkGrayValue);
> > 
> > This seems OK if the noshade attribute is handled after the color attribute. But what will remove these properties if someone later adds a color attribute to the <hr> element?
> 
> Hi Darin.Thanks for the review.
> Cases where color and noshade attribute are added in different sequence : I have added test cases for noshade and color attribute in different sequence.
> header-color-noshade-attribute.html : in this color attribute is mentioned before noshade
> header-noshade-color-attribute.html : in this color attribute is mentioned after noshade .
> 
> Invalid color attribute; present but not something that we can parse : Incase of invalid color attribute default black color is applied.
> 
> Color set by CSS rather than with a color attribute : color set via css is not handled right now.
> 
> But what will remove these properties if someone later adds a color attribute to the <hr> element? : If hr is with no attribute and we add it through javascript then if color attribute is added it will be applied else the noshade(gray color) will be applied.
> 
> The color and noshade attribute both have been deprecated and it should be used via css.
> http://www.w3.org/TR/html-markup/hr.html.

When color attribute is specified we set the background-color and border-color and in RenderBox::paintBoxDecorations it gets painted.
As what I understand when color is specified through CSS though the property is set but no paint function which makes use of this value set via CSS.
So instead of 
hr{
color:red
}
if
hr{
border-color:red;
background-color:red
}
is specified it works.

Created attachment 220102 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 220102 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220102&action=review

Code seems OK but please reduce the number of separate tests.

> Source/WebCore/ChangeLog:9
> +        it shows a gray color. Incase there is color attribute the default gray

Incase there is -> When there is

> Source/WebCore/ChangeLog:12
> +        Firefox and IE show the same behaviour and webkit is different.Making
> +        the behaviour similiar to Firefox and IE.

Space after period. "Making the behaviour similiar to Firefox and IE" is not a full sentence.

> Source/WebCore/ChangeLog:23
> +        Incase color attribute is present that value is applied and the default

Incase -> when the

> LayoutTests/ChangeLog:20
> +        * fast/dom/HTMLHrElement: Added.
> +        * fast/dom/HTMLHrElement/header-color-noshade-attribute-expected.txt: Added.
> +        * fast/dom/HTMLHrElement/header-color-noshade-attribute-javascript-expected.txt: Added.
> +        * fast/dom/HTMLHrElement/header-color-noshade-attribute-javascript.html: Added.
> +        * fast/dom/HTMLHrElement/header-color-noshade-attribute.html: Added.
> +        * fast/dom/HTMLHrElement/header-noshade-attribute-expected.txt: Added.
> +        * fast/dom/HTMLHrElement/header-noshade-attribute.html: Added.
> +        * fast/dom/HTMLHrElement/header-noshade-color-attribute-expected.txt: Added.
> +        * fast/dom/HTMLHrElement/header-noshade-color-attribute-javascript-expected.txt: Added.
> +        * fast/dom/HTMLHrElement/header-noshade-color-attribute-javascript.html: Added.
> +        * fast/dom/HTMLHrElement/header-noshade-color-attribute.html: Added.
> +        * fast/dom/HTMLHrElement/header-noshade-invalidcolor-attribute-expected.txt: Added.
> +        * fast/dom/HTMLHrElement/header-noshade-invalidcolor-attribute.html: Added.

That's a lot of tests for such a simple change. I think should fold all these into a single test (it can test multiple <hr> elements).

Created attachment 220279 [details]
Patch

(In reply to comment #12)
> (From update of attachment 220102 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220102&action=review
> 
> Code seems OK but please reduce the number of separate tests.
> 
> > Source/WebCore/ChangeLog:9
> > +        it shows a gray color. Incase there is color attribute the default gray
> 
> Incase there is -> When there is
> 
> > Source/WebCore/ChangeLog:12
> > +        Firefox and IE show the same behaviour and webkit is different.Making
> > +        the behaviour similiar to Firefox and IE.
> 
> Space after period. "Making the behaviour similiar to Firefox and IE" is not a full sentence.
> 
> > Source/WebCore/ChangeLog:23
> > +        Incase color attribute is present that value is applied and the default
> 
> Incase -> when the
> 
> > LayoutTests/ChangeLog:20
> > +        * fast/dom/HTMLHrElement: Added.
> > +        * fast/dom/HTMLHrElement/header-color-noshade-attribute-expected.txt: Added.
> > +        * fast/dom/HTMLHrElement/header-color-noshade-attribute-javascript-expected.txt: Added.
> > +        * fast/dom/HTMLHrElement/header-color-noshade-attribute-javascript.html: Added.
> > +        * fast/dom/HTMLHrElement/header-color-noshade-attribute.html: Added.
> > +        * fast/dom/HTMLHrElement/header-noshade-attribute-expected.txt: Added.
> > +        * fast/dom/HTMLHrElement/header-noshade-attribute.html: Added.
> > +        * fast/dom/HTMLHrElement/header-noshade-color-attribute-expected.txt: Added.
> > +        * fast/dom/HTMLHrElement/header-noshade-color-attribute-javascript-expected.txt: Added.
> > +        * fast/dom/HTMLHrElement/header-noshade-color-attribute-javascript.html: Added.
> > +        * fast/dom/HTMLHrElement/header-noshade-color-attribute.html: Added.
> > +        * fast/dom/HTMLHrElement/header-noshade-invalidcolor-attribute-expected.txt: Added.
> > +        * fast/dom/HTMLHrElement/header-noshade-invalidcolor-attribute.html: Added.
> 
> That's a lot of tests for such a simple change. I think should fold all these into a single test (it can test multiple <hr> elements).

Thanks for the review Simon. Uploaded new patch.

[WebKit Commit Bot]

Comment on attachment 220279 [details]
Patch

Clearing flags on attachment: 220279

Committed r161334: <http://trac.webkit.org/changeset/161334>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.