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
Created attachment 19536 [details] screenshot
Created attachment 19537 [details] reduction
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?
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.
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
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
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.
Comment on attachment 220279 [details] Patch Clearing flags on attachment: 220279 Committed r161334: <http://trac.webkit.org/changeset/161334>
All reviewed patches have been landed. Closing bug.