Bug 17674 - <hr> appears gray instead of green because of color attribute is defined followed by noshade attribute
Summary: <hr> appears gray instead of green because of color attribute is defined foll...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: gur.trio
URL: http://niwango.jp/pc/niwanews/search....
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2008-03-04 16:57 PST by jasneet
Modified: 2014-01-06 01:01 PST (History)
10 users (show)

See Also:


Attachments
screenshot (566.94 KB, image/jpeg)
2008-03-04 16:57 PST, jasneet
no flags Details
reduction (188 bytes, text/html)
2008-03-04 16:58 PST, jasneet
no flags Details
Patch (11.84 KB, patch)
2013-12-24 05:07 PST, gur.trio
no flags Details | Formatted Diff | Diff
Patch (21.06 KB, patch)
2013-12-30 02:08 PST, gur.trio
no flags Details | Formatted Diff | Diff
Patch (9.54 KB, patch)
2014-01-02 21:40 PST, gur.trio
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jasneet 2008-03-04 16:57:06 PST
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
Comment 1 jasneet 2008-03-04 16:57:40 PST
Created attachment 19536 [details]
screenshot
Comment 2 jasneet 2008-03-04 16:58:28 PST
Created attachment 19537 [details]
reduction
Comment 3 Robert Blaut 2008-03-05 01:25:44 PST
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 &#8211; 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?
Comment 4 Robert Blaut 2008-03-05 05:58:23 PST
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.

Comment 5 Pruthviraj S P 2009-06-09 21:02:23 PDT
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
Comment 6 gur.trio 2013-12-24 05:07:24 PST
Created attachment 219966 [details]
Patch
Comment 7 gur.trio 2013-12-24 07:09:58 PST
Please review.Thanks
Comment 8 Darin Adler 2013-12-28 13:52:09 PST
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?
Comment 9 gur.trio 2013-12-29 22:01:18 PST
(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.
Comment 10 gur.trio 2013-12-29 23:36:36 PST
(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.
Comment 11 gur.trio 2013-12-30 02:08:37 PST
Created attachment 220102 [details]
Patch
Comment 12 Simon Fraser (smfr) 2014-01-02 12:16:30 PST
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).
Comment 13 gur.trio 2014-01-02 21:40:17 PST
Created attachment 220279 [details]
Patch
Comment 14 gur.trio 2014-01-02 21:41:18 PST
(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.
Comment 15 WebKit Commit Bot 2014-01-06 01:00:57 PST
Comment on attachment 220279 [details]
Patch

Clearing flags on attachment: 220279

Committed r161334: <http://trac.webkit.org/changeset/161334>
Comment 16 WebKit Commit Bot 2014-01-06 01:01:01 PST
All reviewed patches have been landed.  Closing bug.