Bug 92000 – [css3-text] Implement CSS3 text-decoration shorthand

• Component: CSS
• Status: RESOLVED FIXED

[Bruno Abinader (history only)]

Current "text-decoration" implementation on WebKit does not support new CSS3 variations, such as -style, -color and -line, for instance. This bug intends to create a "-webkit-text-decoration" property with such feature support, as specified in the link below:

http://www.w3.org/TR/css3-text/#text-decoration

[Bruno Abinader (history only)]

Created attachment 153811 [details]
Proposed patch

This patch acts as a base for -line, -style and -color implementations. Currently, "-webkit-text-decoration*" does nothing, so no layout tests required for now.

[Masayuki Nakano]

I'm the implementer of CSS3 text-decoration on Gecko. I'd like to explain what we do for this property.

Gecko handles the text-decoration property as both shorthand property and longhand property.

I mean that CSSStyleDeclaration object returned from getComputedStyle() returns both null or used value for its textDecoration attribute.

Here is actual code:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsComputedDOMStyle.cpp#2427

If the style or color is changed, it means the new CSS3 properties are used. Then, we assume that the author knows the text-decoration property is changed to a shorthand property. In this case, Gecko returns null for the attribute.

Otherwise, we could break web pages written with CSS2.* if we returned null for it. Therefore, Gecko returns used value for that.

[Bruno Abinader (history only)]

Thanks for the info! I'll have a look at how it's implemented in Gecko :) I am considering this on the shorthand implementations, like "-webkit-text-emphasis" implementation does. But currently I am more interested in implementing the shorthand ones, then when these have landed I'll put effort in making "-webkit-text-decoration" work as expected in its full awesomeness!

(In reply to comment #2)
> I'm the implementer of CSS3 text-decoration on Gecko. I'd like to explain what we do for this property.
> 
> Gecko handles the text-decoration property as both shorthand property and longhand property.
> 
> I mean that CSSStyleDeclaration object returned from getComputedStyle() returns both null or used value for its textDecoration attribute.
> 
> Here is actual code:
> http://mxr.mozilla.org/mozilla-central/source/layout/style/nsComputedDOMStyle.cpp#2427
> 
> If the style or color is changed, it means the new CSS3 properties are used. Then, we assume that the author knows the text-decoration property is changed to a shorthand property. In this case, Gecko returns null for the attribute.
> 
> Otherwise, we could break web pages written with CSS2.* if we returned null for it. Therefore, Gecko returns used value for that.

[Masayuki Nakano]

I forgot to mention a fact. Note that Gecko doesn't allow web authors to specify the CSS3 properties to text-decoration (and also -moz-text-decoration). If the spec will be stable, we will support it.

[Bruno Abinader (history only)]

Created attachment 155021 [details]
Patch

Fixed inheritance for -line, -style and -color shorthands, as well as updated patch comments to better illustrate the purpose of this patch.

[Bruno Abinader (history only)]

Created attachment 155146 [details]
Patch

Actually updated -line to be inherited, as prescribed in previous WebKit text-decoration implementation and CSS 2.1 prose.

[Allan Sandfeld Jensen]

If this is a draft specification, shouldn't this be disablable on compile-time? Or did the discussion on webkit-dev end up with something else?

[Alexis Menard (darktears)]

(In reply to comment #7)
> If this is a draft specification, shouldn't this be disablable on compile-time? Or did the discussion on webkit-dev end up with something else?

It clearly needs to go behind a feature flag.

[Bruno Abinader (history only)]

I'm looking for a css-text-specific ENABLE() but am not successful finding one so far (ie. CSS3 text-emphasis implementation also does not use a flag). Shall I create a new one, then?

(In reply to comment #8)
> (In reply to comment #7)
> > If this is a draft specification, shouldn't this be disablable on compile-time? Or did the discussion on webkit-dev end up with something else?
> 
> It clearly needs to go behind a feature flag.

[Bruno Abinader (history only)]

Created attachment 157234 [details]
Patch

Various fixes, including: simpler design, fixed failing layout tests, full backwards compatible with CSS2.1 text-decoration spec.

[Bruno Abinader (history only)]

Comment on attachment 157234 [details]
Patch

Requesting review. EWS analysis (if submitted) will fail because this patch requires patches from bug 90959, bug 90958 and bug 91638 to be applied first.

[Bruno Abinader (history only)]

Created attachment 158833 [details]
Patch

Updated patch with additional layout tests and code now is guarded by feature flag.

[Bruno Abinader (history only)]

Created attachment 159137 [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 (expected results included). 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 ). This is a workaround to get proper EWS results for the previous patch.

[WebKit Review Bot]

Comment on attachment 159137 [details]
Patch (EWS run only)

Attachment 159137 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13530109

New failing tests:
fast/forms/range/slider-delete-while-dragging-thumb.html
fast/selectors/017.html
fast/loader/loadInProgress.html
fast/loader/unload-form-post-about-blank.html
css3/selectors3/html/css3-modsel-61.html
fast/css/child-style-can-override-visited-style.html
css2.1/t051103-c21-hover-ln-00-e-i.html
fast/canvas/webgl/shader-precision-format.html
css2.1/t060403-c21-pseu-cls-00-e-i.html
http/tests/xmlhttprequest/zero-length-response.html
css2.1/t0511-c21-pseud-link-03-e.html
css2.1/t051103-c21-activ-ln-00-e-i.html
css2.1/t051103-c21-focus-ln-00-e-i.html
css3/selectors3/xml/css3-modsel-61.xml
css3/selectors3/xml/css3-modsel-17.xml
css2.1/t060403-c21-pseu-id-00-e-i.html
fast/forms/range/slider-mouse-events.html
fast/frames/cached-frame-counter.html
css3/selectors3/html/css3-modsel-17.html
fast/events/reveal-link-when-focused.html
css1/text_properties/text_decoration.html
fast/selectors/061.html
css3/selectors3/xhtml/css3-modsel-17.xml
css2.1/t0511-c21-pseud-link-02-e.html
fast/forms/range/slider-onchange-event.html
css2.1/20110323/c543-txt-decor-000.html
css3/selectors3/xhtml/css3-modsel-61.xml

[WebKit Review Bot]

Created attachment 159170 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Bruno Abinader (history only)]

(In reply to comment #14)
> (From update of attachment 159137 [details])
> Attachment 159137 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/13530109
> 
> New failing tests:
> fast/forms/range/slider-delete-while-dragging-thumb.html
> fast/selectors/017.html
> fast/loader/loadInProgress.html
> fast/loader/unload-form-post-about-blank.html
> css3/selectors3/html/css3-modsel-61.html
> fast/css/child-style-can-override-visited-style.html
> css2.1/t051103-c21-hover-ln-00-e-i.html
> fast/canvas/webgl/shader-precision-format.html
> css2.1/t060403-c21-pseu-cls-00-e-i.html
> http/tests/xmlhttprequest/zero-length-response.html
> css2.1/t0511-c21-pseud-link-03-e.html
> css2.1/t051103-c21-activ-ln-00-e-i.html
> css2.1/t051103-c21-focus-ln-00-e-i.html
> css3/selectors3/xml/css3-modsel-61.xml
> css3/selectors3/xml/css3-modsel-17.xml
> css2.1/t060403-c21-pseu-id-00-e-i.html
> fast/forms/range/slider-mouse-events.html
> fast/frames/cached-frame-counter.html
> css3/selectors3/html/css3-modsel-17.html
> fast/events/reveal-link-when-focused.html
> css1/text_properties/text_decoration.html
> fast/selectors/061.html
> css3/selectors3/xhtml/css3-modsel-17.xml
> css2.1/t0511-c21-pseud-link-02-e.html
> fast/forms/range/slider-onchange-event.html
> css2.1/20110323/c543-txt-decor-000.html
> css3/selectors3/xhtml/css3-modsel-61.xml

This is not what the output link says (http://queues.webkit.org/results/13530109). Am I wrong or is it the failure output from some other build?

[Bruno Abinader (history only)]

Created attachment 161695 [details]
Patch

Simplified layout tests for readibility.

[Gyuyoung Kim]

Comment on attachment 161695 [details]
Patch

Attachment 161695 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13721182

[Bruno Abinader (history only)]

(In reply to comment #18)
> (From update of attachment 161695 [details])
> Attachment 161695 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/13721182

This patch requires patches from bug 94094 and bug 91638 to be applied first. As EFL has the feature flag enabled by default, it crashed by missing "CSSPropertyWebKitTextDecorationColor" enum.

[Bruno Abinader (history only)]

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

Included required patches from bug 94094 and bug 91638.

[Build Bot]

Comment on attachment 161702 [details]
Patch (EWS run only)

Attachment 161702 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13718207

[WebKit Review Bot]

Comment on attachment 161702 [details]
Patch (EWS run only)

Attachment 161702 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13713274

New failing tests:
fast/css3-text-decoration/text-decoration-style.html
css1/text_properties/text_decoration.html
css2.1/20110323/c543-txt-decor-000.html
fast/css3-text-decoration/repaint/repaint-text-decoration-style.html

[Bruno Abinader (history only)]

Created attachment 170667 [details]
Patch

Updates on namespace (now CSS3_TEXT), layout test directories and fixed segmentation fault when accessing an invalid pointer.

[EFL EWS Bot]

Comment on attachment 170667 [details]
Patch

Attachment 170667 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14563659

[Bruno Abinader (history only)]

Created attachment 170972 [details]
Patch (EWS only)

More info on failing layout tests in bug 100546 "[css3-text] Provide pixel results for text decoration style layout tests". This patch also contains patch from dependency bug 91638.

[Build Bot]

Comment on attachment 170972 [details]
Patch (EWS only)

Attachment 170972 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14610139

[WebKit Review Bot]

Comment on attachment 170972 [details]
Patch (EWS only)

Attachment 170972 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14549025

New failing tests:
fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-shorthand.html
fast/css3-text/css3-text-decoration/text-decoration-style.html
fast/css3-text/css3-text-decoration/repaint/repaint-text-decoration-style.html
fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-color.html

[Bruno Abinader (history only)]

Created attachment 171241 [details]
Patch

Fixed layout test internal paths after directory changes.

[Bruno Abinader (history only)]

Created attachment 179064 [details]
Patch

Rebased after text-align-last changes and bug fix made in r134156.

[EFL EWS Bot]

Comment on attachment 179064 [details]
Patch

Attachment 179064 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15280603

[Bruno Abinader (history only)]

Created attachment 193065 [details]
Patch

Rebased after r145785.

[Bruno Abinader (history only)]

@Julien, do you mind reviewing this patch as well? :-)

[Julien Chaffraix]

Comment on attachment 193065 [details]
Patch

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/CSSComputedStyleDeclaration.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/getComputedStyle-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/getComputedStyle-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'.

[Bruno Abinader (history only)]

Created attachment 204163 [details]
Proposed patch

[Bruno Abinader (history only)]

Created attachment 204164 [details]
Proposed patch

Fixed typo

[Bruno Abinader (history only)]

Created attachment 204165 [details]
Proposed patch

Fixed typo #2 (updated CSSPropertyWebkitTextDecoration comment on accepted values)

[Bruno Abinader (history only)]

Created attachment 204166 [details]
Proposed patch

Added missing test expectation result

[Alexis Menard (darktears)]

Comment on attachment 204166 [details]
Proposed patch

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

> LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-decoration-shorthand.js:33
> +shouldBe("computedStyle.getPropertyCSSValue('-webkit-text-decoration-color').cssText", "'rgb(0, 0, 0)'");

You should check that the value returned by computedStyle is valid, meaning we can parse it again. Look getComputedStyle-background-shorthand checkComputedStyleValue();

[Alexis Menard (darktears)]

Comment on attachment 204166 [details]
Proposed patch

You should implement the computed style for the shorthand too.

[Bruno Abinader (history only)]

Created attachment 204338 [details]
Proposed patch

Added missing computed style calculation for shorthand property - thanks Alexis\!

[Bruno Abinader]

Created attachment 206613 [details]
Patch

Rebased patch

[Build Bot]

Comment on attachment 206613 [details]
Patch

Attachment 206613 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1068121

New failing tests:
fullscreen/full-screen-iframe-with-max-width-height.html

[Build Bot]

Created attachment 206619 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Bruno Abinader]

(In reply to comment #43)
> Created an attachment (id=206619) [details]
> Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
> 
> The attached test failures were seen while running run-webkit-tests on the mac-ews.
> Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

This failure is not related to this patch, it has been failing since r152433:
http://build.webkit.org/TestFailures/#/Apple MountainLion Debug WK2 (Tests)

[Bruno Abinader]

Created attachment 209114 [details]
Patch

[Bruno Abinader]

Updated patch from Blink's backport:
- While in Blink it replaces previous text-decoration implementation, in WebKit it is added as a new, prefixed property -webkit-text-decoration;
- No need to modify editing code - unprefixed text-decoration remains the same;
- No need to modify layout test values - unprefixed text-decoration remains the same;

[Darin Adler]

Comment on attachment 209114 [details]
Patch

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

> Source/WebCore/css/StylePropertyShorthand.h:116
>  const StylePropertyShorthand& webkitMaskRepeatShorthand();
> +#if ENABLE(CSS3_TEXT)
> +const StylePropertyShorthand& webkitTextDecorationShorthand();
> +#endif // CSS3_TEXT
>  const StylePropertyShorthand& webkitTextEmphasisShorthand();

Awkward to have a conditional in the middle of a sorted list. Makes it hard to re-sort, for example.

Instead would be better to put this in a separate sorted list/paragraph. The same way we do with includes.

Also, the #endif comment really rubs me the wrong way when the #if is less than, say, 10 lines long. I understand the value of the comment when it might be scrolled off screen, but it seems like noise to me in these short ifs. Would be nice to remove some of these and maybe even make this official in our style guide. Unless others don’t agree with me on this.

[Bruno Abinader]

Comment on attachment 209114 [details]
Patch

Aborting commit to fix header inclusion as reviewed by Darin.

[Bruno Abinader]

Comment on attachment 209114 [details]
Patch

Committed r154338: <http://trac.webkit.org/changeset/154338>

[yisibl]

@mmaxfield

Safari nightly does not support text-decoration shorthand.