Bug 94093 – [css3-text] Add parsing support for -webkit-text-decoration-style

• Component: CSS
• Status: RESOLVED FIXED

[Bruno Abinader (history only)]

This patch implements the "text-decoration-style" property parsing as specified in CSS3 working draft, with "-webkit-" prefix. The specification can be found below:
http://dev.w3.org/csswg/css3-text/#text-decoration-style

Additionally, Mozilla implementation details can be found here:
https://developer-dev.allizom.org/en-US/docs/CSS/text-decoration-style

This is an individual task for bug 90958. Rendering support will be handled on a different bug.

[Bruno Abinader (history only)]

Created attachment 158567 [details]
Patch

Proposed patch.

[Bruno Abinader (history only)]

Please note EWS will fail as this patch depends on bug 93863 to be landed first, reason why I didn't requested review / commit-queue flags for now.

[Bruno Abinader (history only)]

Comment on attachment 158567 [details]
Patch

Requesting review and commit-queue flags now that bug 93863 has landed - r125716.

[Bruno Abinader (history only)]

Created attachment 159116 [details]
Patch (EWS run only)

This patch adds changes to make CSS3_TEXT_DECORATION feature flag enabled and remove skip of fast/css3-text-decoration layout test directory from chromium build. This is intentional to get EWS results, but not intended for landing (as suggested by Peter Beverloo in http://lists.webkit.org/pipermail/webkit-dev/2012-August/021925.html ). It is not intended for landing, but acts as a workaround to get proper EWS results for the previous patch.

[Julien Chaffraix]

Comment on attachment 158567 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158567&action=review

> Source/WebCore/ChangeLog:13
> +        https://developer-dev.allizom.org/en-US/docs/CSS/text-decoration-style

Interesting link, I though MDN was the reference for those but I may be wrong.

> Source/WebCore/ChangeLog:42
> +        * rendering/style/StyleRareNonInheritedData.h:

Don't forget the function level entries.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:-1953
> -        case CSSPropertyTextDecoration:

Is this necessary to move this property?

> Source/WebCore/rendering/style/RenderStyleConstants.h:350
> +    TextDecorationStyleSolid, TextDecorationStyleDouble, TextDecorationStyleDotted, TextDecorationStyleDashed, TextDecorationStyleWavy

Nit: I prefer one per line as it's easier to read but there is no rule in our guide and this file is not very consistent so it's up to you.

> LayoutTests/fast/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-style-expected.txt:12
> +PASS [dashed] computed style of -webkit-text-decoration-style should contain dashed value

The output is kinda confusing and you don't strictly test the computed value as taken back from JS.

Here is what is missing from your testing:
* setting the value from JS (style.webkitTextDecorationStyle)
* erroneous values
* testing that *all the possible values* are properly parsed, set and converted back when queried.
* inheritance

> LayoutTests/fast/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-decoration-style.js:12
> +        testPassed(desc+" EXPECTED:"+expected+" ACTUAL:"+actual);

We prefer tests to follow WebKit style (ie space before and after binary operators)

> LayoutTests/fast/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-decoration-style.js:18
> +expect('[initial] -webkit-text-decoration-style should be null', e.style.getPropertyCSSValue('-webkit-text-decoration-style'), null);

I think you should use shouldBe here instead of rolling out your own version of that. shouldBe does a lot more than just the comparison.

[Bruno Abinader (history only)]

Created attachment 159218 [details]
Patch

Fixed issues pointed out by Julien.

[Bruno Abinader (history only)]

Created attachment 159221 [details]
Patch (EWS run only) v2

"EWS run only" for the updated patch.

[Julien Chaffraix]

Comment on attachment 159218 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159218&action=review

r=me, with comments

> Source/WebCore/ChangeLog:43
> +        (StyleRareNonInheritedData):

Still missing those information from the ChangeLog, please don't forget it before landing.

> LayoutTests/fast/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-decoration-style.js:43
> +// Value 'double'.
> +e.style.webkitTextDecorationStyle = "double";
> +shouldBe("e.style.getPropertyCSSValue('-webkit-text-decoration-style').toString()", "'[object CSSPrimitiveValue]'");
> +shouldBe("e.style.webkitTextDecorationStyle", "'double'");
> +
> +computedStyle = window.getComputedStyle(e, null);
> +shouldBe("computedStyle.getPropertyCSSValue('-webkit-text-decoration-style').toString()", "'[object CSSPrimitiveValue]'");
> +shouldBe("computedStyle.webkitTextDecorationStyle", "e.style.webkitTextDecorationStyle");
> +shouldBe("computedStyle.getPropertyCSSValue('-webkit-text-decoration-style').cssText", "'double'");

This would probably be written as a function for all the valid values. That would remove the need for the leading "// Value XXX" comments as they would be obvious (and hopefully dumped into the output, see below as to how).

> LayoutTests/fast/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-decoration-style.js:86
> +// Values 'double' and 'dotted' (invalid, previous valid value 'initial' is assumed).

All in all, I think those should be dumped in the output using debug("..."), including some spaces for readability using debug(''); as needed.

That would make the output above more readable by adding some information about what you trying to test. Think of the port maintainers or developers who haven't followed this bug and will have to understand the output of this test if it ever starts failing.

[Bruno Abinader (history only)]

Created attachment 159254 [details]
Patch

Added missing info to ChangeLog and made layout test results more readible.

[Bruno Abinader (history only)]

Created attachment 159471 [details]
Patch (updated "Reviewed by" section)

Thanks to Igor, manually updated "Reviewed by" section from ChangeLogs.

[WebKit Review Bot]

Comment on attachment 159471 [details]
Patch (updated "Reviewed by" section)

Clearing flags on attachment: 159471

Committed r126054: <http://trac.webkit.org/changeset/126054>