[webkit-reviews] review denied: [Bug 19681] borderColor on table elements should be ignored : [Attachment 121789] patch proposed for "border color on table should be ignored"

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied Vamshi Krishna N
<vnampally at innominds.com>'s request for review:
Bug 19681: borderColor on table elements should be ignored
https://bugs.webkit.org/show_bug.cgi?id=19681

Attachment 121789: patch proposed for "border color on table should be ignored"
https://bugs.webkit.org/attachment.cgi?id=121789&action=review

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=121789&action=review


Does this new behavior match what other browsers are doing? The bug description
seems to say that we should ignore bordercolorAttr altogether.

> Source/WebCore/html/HTMLTableElement.cpp:340
> -	   m_borderColorAttr = attr->decl();
> -	   if (!attr->decl() && !attr->isEmpty()) {
> -	       addCSSColor(attr, CSSPropertyBorderColor, attr->value());
> -	       m_borderColorAttr = true;
> -	   }
> +	  if (m_borderAttr) {	     
> +	       m_borderColorAttr = attr->decl();
> +	       if (!attr->decl() && !attr->isEmpty()) {
> +		   addCSSColor(attr, CSSPropertyBorderColor, attr->value());
> +		   m_borderColorAttr = true;
> +	       }
> +	  }

parseMappedAttribute() shouldn't depend on the presence/absensce of attributes
other than the one being parsed. This will break if borderAttr is parsed before
bordercolorAttr.
HTMLTableElement::additionalAttributeStyleDecls() exists for this purpose; to
apply additional style that depends on multiple attributes.

> LayoutTests/fast/table/border-table-ignore.html:12
> +<table class="brdrcls" cellpadding="10">

This table doesn't have a bordercolor attribute, which means that this test
will not hit the code path you're changing.
Furthermore, it should be possible to do this as a dumpAsText() test.