Bug 94093 - [css3-text] Add parsing support for -webkit-text-decoration-style
Summary: [css3-text] Add parsing support for -webkit-text-decoration-style
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Bruno Abinader (history only)
URL: http://dev.w3.org/csswg/css3-text/#te...
Depends on: 93863
Blocks: 58491 90958 91638 92000 94094 94108 94480
  Show dependency treegraph
Reported: 2012-08-15 05:21 PDT by Bruno Abinader (history only)
Modified: 2012-08-20 14:04 PDT (History)
10 users (show)

See Also:

Patch (23.09 KB, patch)
2012-08-15 07:28 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (EWS run only) (32.69 KB, patch)
2012-08-17 06:59 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (30.66 KB, patch)
2012-08-17 16:04 PDT, Bruno Abinader (history only)
jchaffraix: review+
jchaffraix: commit-queue-
Details | Formatted Diff | Diff
Patch (EWS run only) v2 (40.33 KB, patch)
2012-08-17 16:09 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (30.95 KB, patch)
2012-08-17 18:33 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (updated "Reviewed by" section) (30.98 KB, patch)
2012-08-20 10:32 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bruno Abinader (history only) 2012-08-15 05:21:52 PDT
This patch implements the "text-decoration-style" property parsing as specified in CSS3 working draft, with "-webkit-" prefix. The specification can be found below:

Additionally, Mozilla implementation details can be found here:

This is an individual task for bug 90958. Rendering support will be handled on a different bug.
Comment 1 Bruno Abinader (history only) 2012-08-15 07:28:13 PDT
Created attachment 158567 [details]

Proposed patch.
Comment 2 Bruno Abinader (history only) 2012-08-15 07:29:08 PDT
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.
Comment 3 Bruno Abinader (history only) 2012-08-15 15:37:39 PDT
Comment on attachment 158567 [details]

Requesting review and commit-queue flags now that bug 93863 has landed - r125716.
Comment 4 Bruno Abinader (history only) 2012-08-17 06:59:37 PDT
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.
Comment 5 Julien Chaffraix 2012-08-17 11:03:18 PDT
Comment on attachment 158567 [details]

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.
Comment 6 Bruno Abinader (history only) 2012-08-17 16:04:54 PDT
Created attachment 159218 [details]

Fixed issues pointed out by Julien.
Comment 7 Bruno Abinader (history only) 2012-08-17 16:09:32 PDT
Created attachment 159221 [details]
Patch (EWS run only) v2

"EWS run only" for the updated patch.
Comment 8 Julien Chaffraix 2012-08-17 16:40:53 PDT
Comment on attachment 159218 [details]

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.
Comment 9 Bruno Abinader (history only) 2012-08-17 18:33:20 PDT
Created attachment 159254 [details]

Added missing info to ChangeLog and made layout test results more readible.
Comment 10 Bruno Abinader (history only) 2012-08-20 10:32:27 PDT
Created attachment 159471 [details]
Patch (updated "Reviewed by" section)

Thanks to Igor, manually updated "Reviewed by" section from ChangeLogs.
Comment 11 WebKit Review Bot 2012-08-20 12:36:19 PDT
Comment on attachment 159471 [details]
Patch (updated "Reviewed by" section)

Clearing flags on attachment: 159471

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