[webkit-reviews] review denied: [Bug 20032] CSS 'content' attribute does not support 'none' or 'normal' values : [Attachment 35196] Add support for "content: none" and "content: normal".

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Yuta Kitamura <yutak at chromium.org>'s
request for review:
Bug 20032: CSS 'content' attribute does not support 'none' or 'normal' values
https://bugs.webkit.org/show_bug.cgi?id=20032

Attachment 35196: Add support for "content: none" and "content: normal".
https://bugs.webkit.org/attachment.cgi?id=35196&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	       if (equalIgnoringCase(val->string, "none") ||
equalIgnoringCase(val->string, "normal")) {
> +		   parsedValue = CSSPrimitiveValue::create("",
CSSPrimitiveValue::CSS_STRING);
> +	       }

There should not be braces here.

Changing "content: none" and "content: normal" into "content: ''" at the parser
level is wrong because it prevents us from doing the right thing with computed
style. To make computed style work properly we need to know the actual value of
the property. See bug 23668 for something tracking the need to implement
computed style for the content attribute.

And I do not think it is correct to treat "content: normal" exactly the same as
"content: ''". At the moment it mostly works only because the code in
RenderObject::createObject completely ignores the content property if it's not
an image.

But if you test this with the <br>, <frameset>, and <image> elements, I think
you'll see that it's not working correctly. All three of those classes do
something different if there is contentData than if there is not. I believe
that "content: normal" should not change things.