[webkit-reviews] review denied: [Bug 92000] [css3-text] Add support for "text-decoration" using CSS3 spec : [Attachment 193065] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Bruno Abinader
<bruno.abinader at basyskom.com>'s request for review:
Bug 92000: [css3-text] Add support for "text-decoration" using CSS3 spec
https://bugs.webkit.org/show_bug.cgi?id=92000

Attachment 193065: Patch
https://bugs.webkit.org/attachment.cgi?id=193065&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=193065&action=review


> Source/WebCore/ChangeLog:23
> +

This change is missing several important bits:
* CSSComputedStyleDeclarations.cpp has a list of properties *excluding*
shorthands
(http://trac.webkit.org/browser/trunk/Source/WebCore/css/CSSComputedStyleDeclar
ation.cpp#L94) which wasn't updated.
* shorthands *shouldn't* be handled in StyleSelector / StyleBuilder and I don't
see any change here. Btw, this now triggers an ASSERT after bug 111505 as it is
wrong *not* to expand a shorthand into its longhands properties.

> Source/WebCore/css/CSSParser.cpp:2297
> +	   // <text-decoration-line> || <text-decoration-style> ||
<text-decoration-color> || blink

It's unclear to me why 'inherit' is not accepted anymore. It should be at least
tested (in combination to the other comments on testing).

This is probably a specification bug as they don't seem to accept that either.

> Source/WebCore/css/CSSParser.cpp:9168
> +    if (list->length() && (isValid || m_currentShorthand !=
CSSPropertyInvalid)) {

I think this is better written: || isShorthand().

Also if you don't allow the extended parsing code if CSS3_TEXT is off, it seems
like this should be common to both implementations.

> Source/WebCore/css/CSSParser.cpp:9201
> +    bool isShorthand = false;

That is just wrong. The specification is fairly explicit that it is *always* a
shorthand, not conditionally a shorthand.

>
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/g
etComputedStyle-text-decoration-shorthand.js:11
> +shouldBeNull("e.style.getPropertyCSSValue('text-decoration')");

'none' is different from null AFAICT, that doesn't seem correct.

>
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/g
etComputedStyle-text-decoration-shorthand.js:48
> +

This testing is lacking: you should be able to omit 'text-decoration-color' and
'text-decoration-style' and they would revert to their initial value ('none').

Also you should be able to change 'text-decoration-color' /
'text-decoration-style' and 'text-decoration-line' and see the result reflected
when querying 'text-decoration'.