Bug 90959 – [css3-text] Add support for text-decoration-line

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Bruno Abinader (history only)]

WebKit currently only has support for CSS text-decoration property, but CSS3's text-decoration-line. This is an individual task for bug 58491.

[Bruno Abinader (history only)]

Created attachment 151950 [details]
Proposed patch

This is the first patch proposal, implementing the "text-decoration-line" CSS3 property. Currently, WebKit implements "text-decoration" with the following properties:

none | [ underline || overline || line-through || blink ] | inherit

Whereas "text-decoration-line" defines:

none | [ underline || overline || line-through ] | inherit

For now I've used the same enum "ETextDecoration", however "blink" value is not valid to "text-decoration-line". It might be the case that text-decoration-line has its own structure, and make "text-decoration" inherit from it somehow. I am not familiar with other CSS properties that can use the same set of values, like "font-size" values being valid also for "font", for instance. Comments are well appreciated :)

[WebKit Review Bot]

Attachment 151950 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/css/CSSParser.cpp:2093:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/rendering/style/StyleVisualData.h:46:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Bruno Abinader (history only)]

Created attachment 151954 [details]
Proposed patch (v2)

Updated patch with style issues fixed.

[WebKit Review Bot]

Attachment 151954 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/rendering/style/StyleVisualData.h:47:  Extra space before )  [whitespace/parens] [2]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Bruno Abinader (history only)]

Created attachment 151956 [details]
Proposed patch (v3)

Shame on me... sorry for the noise, updated the patch again.

[WebKit Review Bot]

Comment on attachment 151956 [details]
Proposed patch (v3)

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

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style-without-renderer.html

[WebKit Review Bot]

Created attachment 151972 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  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)]

Created attachment 152052 [details]
Proposed patch (v4) + layout test

Updated patch with inclusion of expected results for chromium build layout tests and a newly created fast/css/text-decoration-line.html layout test.

[WebKit Review Bot]

Comment on attachment 152052 [details]
Proposed patch (v4) + layout test

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

New failing tests:
svg/css/getComputedStyle-basic.xhtml

[WebKit Review Bot]

Created attachment 152136 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  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)]

Created attachment 152158 [details]
Proposed patch (v5)

Another missing result from svg/css/getComputedStyle-basic.html, now fixed.

[Bruno Abinader (history only)]

Created attachment 152566 [details]
Proposed patch (v6) + pixel test results

Added missing expected pixel test results as well as a check if "blink", a value only recognized by text-decoration property, would work in text-decoration-line (which properly didn't).

[Alexis Menard (darktears)]

Comment on attachment 152566 [details]
Proposed patch (v6) + pixel test results

I think we should prefix it with -webkit as far I understand with https://bugs.webkit.org/show_bug.cgi?id=87678#c16

[Bruno Abinader (history only)]

Agreed, Alexis! Thanks for the info :) I am now finishing a layout test run with the new -webkit-text-decoration-line to see if everything's as ok as last patch atm.

(In reply to comment #13)
> (From update of attachment 152566 [details])
> I think we should prefix it with -webkit as far I understand with https://bugs.webkit.org/show_bug.cgi?id=87678#c16

[Bruno Abinader (history only)]

Created attachment 152771 [details]
Proposed patch (v7) w/ -webkit-text-decoration-line change

Thanks to Alexis, proposed patch w/ s/text-decoration-line/-webkit-text-decoration-line/ changes.

[Bruno Abinader (history only)]

As suggested by Andreas Kling, this property has landed already in Mozilla, as described here:

https://developer.mozilla.org/en/CSS/text-decoration-line

As the link says, they're using "-moz" prefix as well, just like Alexis suggested in comment 13.

[Simon Fraser (smfr)]

The appropriate spec link is <http://www.w3.org/TR/css3-text/#text-decoration-line>

[Bruno Abinader (history only)]

Hi Simon, thanks for the info! Though I already had put it into URL field. Also adding same info for text-decoration-style (bug 90958) and text-decoration-color (bug 91638).

(In reply to comment #17)
> The appropriate spec link is <http://www.w3.org/TR/css3-text/#text-decoration-line>

[Bruno Abinader (history only)]

Created attachment 153268 [details]
Proposed patch (v8)

Added missing initializer & copy operator assignment for textDecorationLine @ Source/WebCore/rendering/style/StyleVisualData.cpp.

[Bruno Abinader (history only)]

Comment on attachment 153268 [details]
Proposed patch (v8)

I'm currently working on a refactory for this patch, after changes from parent patch (handled in bug 92000).

[Bruno Abinader (history only)]

Created attachment 155328 [details]
Patch

Lightweight implementation with shared code between 'text-decoration' and '-webkit-text-decorations-in-effect' CSS properties. Please note that patch from bug 92000 should land before this.

[Bruno Abinader (history only)]

Created attachment 155534 [details]
Patch

Rebased. Please note that patch from bug 92000 should land before this.

[Bruno Abinader (history only)]

Note: EWS is not able to apply patch because it depends on patch from bug 92000 to be landed first.

[Allan Sandfeld Jensen]

Comment on attachment 155534 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        (WebCore):

I think you can discard useless lines like this.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1200
> +    RefPtr<CSSValue> value = renderTextDecorationLineFlagsToCSSValue(textDecoration).get();
> +    if (textDecoration & BLINK) {
> +        RefPtr<CSSValueList> list = value->isValueList() ? static_cast<CSSValueList*>(value.get()) : CSSValueList::createSpaceSeparated();
> +        list->append(cssValuePool().createIdentifierValue(CSSValueBlink));
> +        return list;
> +    }

What does this actually do? I am a bit surprised we are even parsing blink, I would have assumed we just discarded it or made parsing error.

[Bruno Abinader (history only)]

(In reply to comment #24)
> (From update of attachment 155534 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155534&action=review
> 
> > Source/WebCore/ChangeLog:18
> > +        (WebCore):
> 
> I think you can discard useless lines like this.

Right :) This is all generated by prepare-ChangeLog, but I'll remove this.

> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1200
> > +    RefPtr<CSSValue> value = renderTextDecorationLineFlagsToCSSValue(textDecoration).get();
> > +    if (textDecoration & BLINK) {
> > +        RefPtr<CSSValueList> list = value->isValueList() ? static_cast<CSSValueList*>(value.get()) : CSSValueList::createSpaceSeparated();
> > +        list->append(cssValuePool().createIdentifierValue(CSSValueBlink));
> > +        return list;
> > +    }
> 
> What does this actually do? I am a bit surprised we are even parsing blink, I would have assumed we just discarded it or made parsing error.

The previous code parses "blink", though later it is not used anywhere, indeed. This code basically avoids code duplication by taking advantage of the work done in renderTextDecorationLineFlagsToCSSValue. It is separated like this because "text-decoration-line" property does not support "blink" value.

My intention is to not break previous code, so the original "text-decoration" CSS2.1 property still works the same like before, so no harms done :)

[Bruno Abinader (history only)]

Created attachment 156824 [details]
Patch

Various fixes, including: simpler design, fixed failing layout tests, does not interfere with editing feature, fixed relayout/repaint when property differs, does not depend on bug 92000 anymore.

[Bruno Abinader (history only)]

Created attachment 156829 [details]
Patch

Fixed double ChangeLog entry.

[Bruno Abinader (history only)]

Comment on attachment 156829 [details]
Patch

As much discussed in webkit-dev and www-style, this patch is now ready for review. The implementation has changed significantly since first patch proposal.

[Kenneth Rohde Christiansen]

Comment on attachment 156829 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        working draft, with "-webkit-" prefix. The specification can be found below:

as agreed upon?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1194
> +    // Blink value is ignored

commends ends with a punctuation mark of some kind. (just add a dot :-)) It would be better if you said why.

> Source/WebCore/css/CSSParser.cpp:2099
>          // none | [ underline || overline || line-through || blink ] | inherit

he it says blink!

> Source/WebCore/css/CSSParser.cpp:2100
> +        // Note: blink value is not accepted by -webkit-text-decoration-line

Here you note it is not accepted?

> Source/WebCore/css/CSSParser.cpp:7911
> +        // Skip if -webkit-text-decoration-line was previously set

+ .

Was set previously. (I would say)

> Source/WebCore/css/CSSParser.cpp:7914
> +        for (unsigned i = 0; i < m_parsedProperties.size(); ++i)
> +            if (m_parsedProperties[i].id() == CSSPropertyWebkitTextDecorationLine)
> +                return;

That for loop has 2 actual lines as the content (not logical lines) so it must have braces.

Is this how it is done elsewhere? It seems like a common thing to check for

> Source/WebCore/css/CSSParser.cpp:7932
> +        case CSSValueBlink:
> +            isValid = propId != CSSPropertyWebkitTextDecorationLine;

Maybe the comment is more valuable here

[Bruno Abinader (history only)]

Thank you for the review, Kenneth! I'm going to fix the suggestions you commented. Just answering the questions you made below:

(In reply to comment #29)
> (From update of attachment 156829 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156829&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        working draft, with "-webkit-" prefix. The specification can be found below:
> 
> as agreed upon?

Alexis suggested on comment 13 that we should use "-webkit-" prefix whenever a new CSS property that is not yet "stable" is implemented.

> > Source/WebCore/css/CSSParser.cpp:2099
> >          // none | [ underline || overline || line-through || blink ] | inherit
> 
> he it says blink!
> 
> > Source/WebCore/css/CSSParser.cpp:2100
> > +        // Note: blink value is not accepted by -webkit-text-decoration-line
> 
> Here you note it is not accepted?

Yes, it is one of the things I've discussed in www-style (see http://lists.w3.org/Archives/Public/www-style/2012Aug/0145.html ). The CSS3 spec says that "blink" value is accepted by "text-decoration", but not by "text-decoration-line". The parsing implementation of "text-decoration-line" is pretty much the same as the previous "text-decoration" one, thus they share the same parseTextDecoration() usage. I'm going to change this to be more clear (and also move this comment to the right place as you suggested).

[Bruno Abinader (history only)]

Created attachment 157256 [details]
Patch

Fixed issues pointed out by Kenneth.

[Kenneth Rohde Christiansen]

Comment on attachment 157256 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:7917
> +    // Skip if -webkit-text-decoration-line was set previously.

I wonder why this is special, why isnt this done for other properties? what if the new one has important set? Shouldnt you also not test this with a test? I think it deserves a better comment

> LayoutTests/fast/css/text-decoration-line-expected.html:5
> +            .none { text-decoration: none; }
> +            .underline { text-decoration: underline; }

werent you implementing it with prefix? like -webkit-text-decoration?

Why do you have an expected file with the .html extension? (Maybe I am a bit behind on how the test system currently works)

[Bruno Abinader (history only)]

(In reply to comment #32)
> (From update of attachment 157256 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157256&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:7917
> > +    // Skip if -webkit-text-decoration-line was set previously.
> 
> I wonder why this is special, why isnt this done for other properties? what if the new one has important set? Shouldnt you also not test this with a test? I think it deserves a better comment

Both "text-decoration" and "-webkit-text-decoration-line" share the same style builder property handler (property data is stored on the same place). That said, if we have the following:

<span style="-webkit-text-decoration-line: underline; text-decoration: none;">Some text</span>

Without this code, the value from "text-decoration" would override the value from"-webkit-text-decoration-line". As far as I understood from the specification, "text-decoration-line" should be handled with higher priority over "text-decoration" usage. This is heavily tested on fast/css/text-decoration-line.html - except usage of !important value - where the <div> objects inherit style classes with "text-decoration" definitions, but contains custom styles with "-webkit-text-decoration-line" ones.

But I might be wrong about it (priorities between -webkit-text-decoration-line and text-decoration), so this part of the code could be totally unnecessary. Said this, I'm going to remove it.

> 
> > LayoutTests/fast/css/text-decoration-line-expected.html:5
> > +            .none { text-decoration: none; }
> > +            .underline { text-decoration: underline; }
> 
> werent you implementing it with prefix? like -webkit-text-decoration?
> 
> Why do you have an expected file with the .html extension? (Maybe I am a bit behind on how the test system currently works)

The fast/css/text-decoration-line.html is a reference layout test, where two different versions of a webpage should present the same graphical results, using different implementations. That said, the test page contains usage of new "-webkit-text-decoration-line" and the expected uses the old "text-decoration" property, both rendering the same graphical expectations.

[Kenneth Rohde Christiansen]

(In reply to comment #33)
> (In reply to comment #32)
> > (From update of attachment 157256 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=157256&action=review
> > 
> > > Source/WebCore/css/CSSParser.cpp:7917
> > > +    // Skip if -webkit-text-decoration-line was set previously.
> > 
> > I wonder why this is special, why isnt this done for other properties? what if the new one has important set? Shouldnt you also not test this with a test? I think it deserves a better comment
> 
> Both "text-decoration" and "-webkit-text-decoration-line" share the same style builder property handler (property data is stored on the same place). That said, if we have the following:
> 
> <span style="-webkit-text-decoration-line: underline; text-decoration: none;">Some text</span>
> 
> Without this code, the value from "text-decoration" would override the value from"-webkit-text-decoration-line". As far as I understood from the specification, "text-decoration-line" should be handled with higher priority over "text-decoration" usage. This is heavily tested on fast/css/text-decoration-line.html - except usage of !important value - where the <div> objects inherit style classes with "text-decoration" definitions, but contains custom styles with "-webkit-text-decoration-line" ones.

Ah the "-line" difference is easy to miss and could me explained better in the changelog. Anyway, if I then remove say the decoration-line property using the inspector, the text-decoration one kicks in?

> But I might be wrong about it (priorities between -webkit-text-decoration-line and text-decoration), so this part of the code could be totally unnecessary. Said this, I'm going to remove it.
> 
> > 
> > > LayoutTests/fast/css/text-decoration-line-expected.html:5
> > > +            .none { text-decoration: none; }
> > > +            .underline { text-decoration: underline; }
> > 
> > werent you implementing it with prefix? like -webkit-text-decoration?
> > 
> > Why do you have an expected file with the .html extension? (Maybe I am a bit behind on how the test system currently works)
> 
> The fast/css/text-decoration-line.html is a reference layout test, where two different versions of a webpage should present the same graphical results, using different implementations. That said, the test page contains usage of new "-webkit-text-decoration-line" and the expected uses the old "text-decoration" property, both rendering the same graphical expectations.

Ah yes ref tests :-) I didn't make one of those myself yet.

[Bruno Abinader (history only)]

(In reply to comment #34)
> (In reply to comment #33)
> > Both "text-decoration" and "-webkit-text-decoration-line" share the same style builder property handler (property data is stored on the same place). That said, if we have the following:
> > 
> > <span style="-webkit-text-decoration-line: underline; text-decoration: none;">Some text</span>
> > 
> > Without this code, the value from "text-decoration" would override the value from"-webkit-text-decoration-line". As far as I understood from the specification, "text-decoration-line" should be handled with higher priority over "text-decoration" usage. This is heavily tested on fast/css/text-decoration-line.html - except usage of !important value - where the <div> objects inherit style classes with "text-decoration" definitions, but contains custom styles with "-webkit-text-decoration-line" ones.
> 
> Ah the "-line" difference is easy to miss and could me explained better in the changelog. Anyway, if I then remove say the decoration-line property using the inspector, the text-decoration one kicks in?

Yes, that is the intention :) If the UA says only CSS1 and CSS2.1 are supported, for instance, only "text-decoration" value is taken into consideration, otherwise "-webkit-text-decoration-line" gets prioritized.

I have tested the layout test I've made for this patch with Firefox (changing -webkit- with -moz- prefix) and obtained the same behavior ("-moz-text-decoration-line" has higher priority over "text-decoration", even with !important value set). Said this, I'd ask for this part of the code to be sustained still :) I can do, however, write a better explanation on the ChangeLog, indeed.

[Bruno Abinader (history only)]

(In reply to comment #35)
> Yes, that is the intention :) If the UA says only CSS1 and CSS2.1 are supported, for instance, only "text-decoration" value is taken into consideration, otherwise "-webkit-text-decoration-line" gets prioritized.
> 
> I have tested the layout test I've made for this patch with Firefox (changing -webkit- with -moz- prefix) and obtained the same behavior ("-moz-text-decoration-line" has higher priority over "text-decoration", even with !important value set). Said this, I'd ask for this part of the code to be sustained still :) I can do, however, write a better explanation on the ChangeLog, indeed.

Actually, sorry, my mistake. The "!important" value is only ignored from "text-decoration" if value is initial - "none". If any other valid primitive is used together with !important, then "text-decoration" value is used instead of "-moz-text-decoration-line". You were right on that :) I am going to modify that code to verify if the !important value is set.

[Bruno Abinader (history only)]

Created attachment 157467 [details]
Patch

Fixed text-decoration reset when important is set (updated layout tests accordingly) and added explanation about it on ChangeLog, as pointed out by Kenneth.

[Kenneth Rohde Christiansen]

Comment on attachment 157467 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        former resets the latter if value from latter is not important (same behavior as

doesn't have important priority

> Source/WebCore/css/CSSParser.cpp:7917
> +    // Skip if -webkit-text-decoration-line was set previously and is not important.

// The text-decoration-line property takes priority over text-decoration, unless the latter has important priority set.

Question, what if both have important set?

> LayoutTests/fast/css/text-decoration-line.html:25
> +    <div style="-webkit-text-decoration-line: underline; text-decoration: none !important;">This text contains no decorations.</div><br/>
> +    <div style="-webkit-text-decoration-line: underline; text-decoration: overline !important;">This text is overlined.</div><br/>

what about testing -webkit-text-decoration-line with important priority?

[Bruno Abinader (history only)]

> // The text-decoration-line property takes priority over text-decoration, unless the latter has important priority set.
> 
> Question, what if both have important set?
> 
> > LayoutTests/fast/css/text-decoration-line.html:25
> > +    <div style="-webkit-text-decoration-line: underline; text-decoration: none !important;">This text contains no decorations.</div><br/>
> > +    <div style="-webkit-text-decoration-line: underline; text-decoration: overline !important;">This text is overlined.</div><br/>
> 
> what about testing -webkit-text-decoration-line with important priority?

If both sets the "important" value, then the text-decoration value gets value from the last parsed property, as expected. I'm going to add this line into the layout test for clarity (same behavior as Firefox).

[Bruno Abinader (history only)]

Created attachment 157484 [details]
Patch

Fixed comments, added case where both text-decoration and -webkit-text-decoration properties sets the "important" value to layout test.

[Kenneth Rohde Christiansen]

Comment on attachment 157484 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:7917
> +    // Skip if value doesn't have "important" value and -webkit-text-decoration-line was set previously.

Why not use my suggestion? for a better comment

[Bruno Abinader (history only)]

(In reply to comment #41)
> (From update of attachment 157484 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157484&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:7917
> > +    // Skip if value doesn't have "important" value and -webkit-text-decoration-line was set previously.
> 
> Why not use my suggestion? for a better comment

Oops, I guess I read that as part of the question :S Changing in a sec.

[Bruno Abinader (history only)]

Created attachment 157494 [details]
Patch

Using Kenneth's code comment for readibility.

[WebKit Review Bot]

Comment on attachment 157494 [details]
Patch

Rejecting attachment 157494 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/PerformanceTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13458786

[Eric Seidel (no email)]

Comment on attachment 157494 [details]
Patch

The checkout got corrupted on one of the instances.  Adam is fixing.

[WebKit Review Bot]

Comment on attachment 157494 [details]
Patch

Clearing flags on attachment: 157494

Committed r125205: <http://trac.webkit.org/changeset/125205>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.