hr color attribute not working.

hr color attribute not working.

The noshade attribute is a boolean attribute and when set on hr element
it shows a gray color. When there is color attribute the default gray
color should be replaced by the color mentioned by the color attribute.
Firefox and IE show the same behaviour but Blink is different. Making
the behaviour of Blink similiar to Firefox and IE's behaviour.

When the color attribute is present that value is applied and the
default gray color is ignored. Incase of no color attribute the default
gray color is applied.

I already landed this patch on Webkit. Just modified the layout test file.
http://trac.webkit.org/changeset/161334

R=esprehn@chromium.org,eseidel@chromium.org
BUG=175359
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168571

[rwlbuis, 2014-02-14 19:04:15]

See my nit.

https://codereview.chromium.org/162993002/diff/1/LayoutTests/fast/dom/HTMLHrE...
File LayoutTests/fast/dom/HTMLHrElement/hr-color-noshade-attribute.html (right):

https://codereview.chromium.org/162993002/diff/1/LayoutTests/fast/dom/HTMLHrE...
LayoutTests/fast/dom/HTMLHrElement/hr-color-noshade-attribute.html:7: <hr
id="hrElement1" color="red" noshade>
Patch looks good but it seems like you use a tab here? Please make the test
indenting consistent.

[Gurpreet, 2014-02-17 04:42:25]

On 2014/02/14 19:04:15, rwlbuis wrote:
> See my nit.
> 
>
https://codereview.chromium.org/162993002/diff/1/LayoutTests/fast/dom/HTMLHrE...
> File LayoutTests/fast/dom/HTMLHrElement/hr-color-noshade-attribute.html
(right):
> 
>
https://codereview.chromium.org/162993002/diff/1/LayoutTests/fast/dom/HTMLHrE...
> LayoutTests/fast/dom/HTMLHrElement/hr-color-noshade-attribute.html:7: <hr
> id="hrElement1" color="red" noshade>
> Patch looks good but it seems like you use a tab here? Please make the test
> indenting consistent.

Thanks for the review. Done. Please have a look.

[Gurpreet, 2014-02-17 07:45:35]

Changes done. Please have a look.

https://codereview.chromium.org/162993002/diff/1/LayoutTests/fast/dom/HTMLHrE...
File LayoutTests/fast/dom/HTMLHrElement/hr-color-noshade-attribute.html (right):

https://codereview.chromium.org/162993002/diff/1/LayoutTests/fast/dom/HTMLHrE...
LayoutTests/fast/dom/HTMLHrElement/hr-color-noshade-attribute.html:7: <hr
id="hrElement1" color="red" noshade>
On 2014/02/14 19:04:15, rwlbuis wrote:
> Patch looks good but it seems like you use a tab here? Please make the test
> indenting consistent.

Done.

[Gurpreet, 2014-02-19 09:31:08]

Please review. Thanks.

[Gurpreet, 2014-02-28 05:41:18]

The CQ bit was checked by k.gurpreet@samsung.com

[Gurpreet, 2014-02-28 05:41:19]

The CQ bit was unchecked by k.gurpreet@samsung.com

[Gurpreet, 2014-02-28 05:41:26]

The CQ bit was checked by k.gurpreet@samsung.com

[commit-bot: I haz the power, 2014-02-28 05:41:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-28 05:41:32]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Gurpreet, 2014-02-28 05:50:00]

The CQ bit was unchecked by k.gurpreet@samsung.com

[rwlbuis, 2014-03-04 19:11:34]

https://codereview.chromium.org/162993002/diff/90001/LayoutTests/fast/dom/HTM...
File LayoutTests/fast/dom/HTMLHrElement/script-tests/color-noshade-attribute.js
(right):

https://codereview.chromium.org/162993002/diff/90001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLHrElement/script-tests/color-noshade-attribute.js:12:
document.getElementById('hrElement5').setAttribute("noshade", "");
Nit: you should choose one indenting style and stick to it (' ' use in parameter
list).

[eseidel, 2014-03-04 19:14:47]

lgtm

[commit-bot: I haz the power, 2014-03-04 19:15:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/k.gurpreet@samsung.com/162993002/90001

[commit-bot: I haz the power, 2014-03-04 19:15:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-04 19:15:25]

Failed to apply patch for Source/core/html/HTMLHRElement.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/html/HTMLHRElement.cpp
  Hunk #1 FAILED at 76.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/html/HTMLHRElement.cpp.rej

Patch:       Source/core/html/HTMLHRElement.cpp
Index: Source/core/html/HTMLHRElement.cpp
diff --git a/Source/core/html/HTMLHRElement.cpp
b/Source/core/html/HTMLHRElement.cpp
index
7a6edf88eb51c5aeadaf83839ea9482c77a9624c..00f0cbde606c26f51df191bdb941bbcf8d0c6d75
100644
--- a/Source/core/html/HTMLHRElement.cpp
+++ b/Source/core/html/HTMLHRElement.cpp
@@ -76,11 +76,13 @@ void
HTMLHRElement::collectStyleForPresentationAttribute(const QualifiedName& na
         addHTMLColorToStyle(style, CSSPropertyBorderColor, value);
         addHTMLColorToStyle(style, CSSPropertyBackgroundColor, value);
     } else if (name == noshadeAttr) {
-        addPropertyToPresentationAttributeStyle(style, CSSPropertyBorderStyle,
CSSValueSolid);
+        if (!hasAttribute(colorAttr)) {
+            addPropertyToPresentationAttributeStyle(style,
CSSPropertyBorderStyle, CSSValueSolid);
 
-        RefPtr<CSSPrimitiveValue> darkGrayValue =
cssValuePool().createColorValue(Color::darkGray);
-        style->setProperty(CSSPropertyBorderColor, darkGrayValue);
-        style->setProperty(CSSPropertyBackgroundColor, darkGrayValue);
+            RefPtr<CSSPrimitiveValue> darkGrayValue =
cssValuePool().createColorValue(Color::darkGray);
+            style->setProperty(CSSPropertyBorderColor, darkGrayValue);
+            style->setProperty(CSSPropertyBackgroundColor, darkGrayValue);
+        }
     } else if (name == sizeAttr) {
         StringImpl* si = value.impl();
         int size = si->toInt();

[Gurpreet, 2014-03-05 07:02:02]

The CQ bit was checked by k.gurpreet@samsung.com

[commit-bot: I haz the power, 2014-03-05 07:03:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/k.gurpreet@samsung.com/162993002/160001

[commit-bot: I haz the power, 2014-03-05 07:03:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-05 07:03:04]

Failed to apply patch for Source/core/html/HTMLHRElement.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/html/HTMLHRElement.cpp
  Hunk #1 FAILED at 76.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/html/HTMLHRElement.cpp.rej

Patch:       Source/core/html/HTMLHRElement.cpp
Index: Source/core/html/HTMLHRElement.cpp
diff --git a/Source/core/html/HTMLHRElement.cpp
b/Source/core/html/HTMLHRElement.cpp
index
7a6edf88eb51c5aeadaf83839ea9482c77a9624c..912c589e927827e4183666eef859a4be03da5150
100644
--- a/Source/core/html/HTMLHRElement.cpp
+++ b/Source/core/html/HTMLHRElement.cpp
@@ -76,11 +76,13 @@ void
HTMLHRElement::collectStyleForPresentationAttribute(const QualifiedName& na
         addHTMLColorToStyle(style, CSSPropertyBorderColor, value);
         addHTMLColorToStyle(style, CSSPropertyBackgroundColor, value);
     } else if (name == noshadeAttr) {
-        addPropertyToPresentationAttributeStyle(style, CSSPropertyBorderStyle,
CSSValueSolid);
+        if (!hasAttribute(colorAttr)) {
+            addPropertyToPresentationAttributeStyle(style,
CSSPropertyBorderStyle, CSSValueSolid);
 
-        RefPtr<CSSPrimitiveValue> darkGrayValue =
cssValuePool().createColorValue(Color::darkGray);
-        style->setProperty(CSSPropertyBorderColor, darkGrayValue);
-        style->setProperty(CSSPropertyBackgroundColor, darkGrayValue);
+            RefPtrWillBeRawPtr<CSSPrimitiveValue> darkGrayValue =
cssValuePool().createColorValue(Color::darkGray);
+            style->setProperty(CSSPropertyBorderColor, darkGrayValue);
+            style->setProperty(CSSPropertyBackgroundColor, darkGrayValue);
+        }
     } else if (name == sizeAttr) {
         StringImpl* si = value.impl();
         int size = si->toInt();

[Gurpreet, 2014-03-05 08:28:02]

The CQ bit was checked by k.gurpreet@samsung.com

[commit-bot: I haz the power, 2014-03-05 08:30:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/k.gurpreet@samsung.com/162993002/180001

[Gurpreet, 2014-03-05 13:05:09]

The CQ bit was unchecked by k.gurpreet@samsung.com

[Gurpreet, 2014-03-05 13:06:11]

The CQ bit was checked by k.gurpreet@samsung.com

[commit-bot: I haz the power, 2014-03-05 13:06:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/k.gurpreet@samsung.com/162993002/180001

[Gurpreet, 2014-03-05 14:15:50]

On 2014/03/04 19:14:47, eseidel wrote:
> lgtm

Thanks Eric for the review.

[rwlbuis, 2014-03-05 15:55:35]

On 2014/03/05 14:15:50, Gurpreet wrote:
> On 2014/03/04 19:14:47, eseidel wrote:
> > lgtm
> 
> Thanks Eric for the review.

BTW one thing to take into account for future patches, it is better to give a
clear description for each patch set that you upload. This makes it easier for
the reviewer to see what changed.

[commit-bot: I haz the power, 2014-03-05 20:58:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/k.gurpreet@samsung.com/162993002/180001

[commit-bot: I haz the power, 2014-03-06 04:19:43]

Change committed as 168571

[Gurpreet, 2014-03-07 07:47:42]

On 2014/03/05 15:55:35, rwlbuis wrote:
> On 2014/03/05 14:15:50, Gurpreet wrote:
> > On 2014/03/04 19:14:47, eseidel wrote:
> > > lgtm
> > 
> > Thanks Eric for the review.
> 
> BTW one thing to take into account for future patches, it is better to give a
> clear description for each patch set that you upload. This makes it easier for
> the reviewer to see what changed.

Sure Rob will take care for future patches. Thanks.