Bug 90959 - [css3-text] Add support for text-decoration-line
: [css3-text] Add support for text-decoration-line
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
: http://www.w3.org/TR/css3-text/#text-...
: WebExposed
: 93863
: 58491 90958 91638 92000 94108
  Show dependency treegraph
 
Reported: 2012-07-11 02:43 PST by
Modified: 2012-08-15 07:18 PST (History)


Attachments
Proposed patch (25.60 KB, patch)
2012-07-12 07:49 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch (v2) (25.80 KB, patch)
2012-07-12 07:58 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch (v3) (25.80 KB, patch)
2012-07-12 08:09 PST, Bruno Abinader (history only)
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 (584.18 KB, application/zip)
2012-07-12 09:28 PST, WebKit Review Bot
no flags Details
Proposed patch (v4) + layout test (31.11 KB, patch)
2012-07-12 14:06 PST, Bruno Abinader (history only)
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-07 (366.04 KB, application/zip)
2012-07-12 19:50 PST, WebKit Review Bot
no flags Details
Proposed patch (v5) (31.67 KB, patch)
2012-07-12 23:02 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch (v6) + pixel test results (43.21 KB, patch)
2012-07-16 11:08 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch (v7) w/ -webkit-text-decoration-line change (43.54 KB, patch)
2012-07-17 09:19 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch (v8) (44.49 KB, patch)
2012-07-19 08:13 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (30.48 KB, patch)
2012-07-30 11:41 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (30.42 KB, patch)
2012-07-31 07:47 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (22.24 KB, patch)
2012-08-06 18:52 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (20.12 KB, patch)
2012-08-06 19:11 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (20.39 KB, patch)
2012-08-08 11:23 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (20.77 KB, patch)
2012-08-09 09:09 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (21.50 KB, patch)
2012-08-09 10:42 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (21.52 KB, patch)
2012-08-09 11:08 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-07-11 02:43:49 PST
WebKit currently only has support for CSS text-decoration property, but CSS3's text-decoration-line. This is an individual task for bug 58491.
------- Comment #1 From 2012-07-12 07:49:56 PST -------
Created an attachment (id=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 :)
------- Comment #2 From 2012-07-12 07:51:46 PST -------
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.
------- Comment #3 From 2012-07-12 07:58:15 PST -------
Created an attachment (id=151954) [details]
Proposed patch (v2)

Updated patch with style issues fixed.
------- Comment #4 From 2012-07-12 08:04:10 PST -------
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.
------- Comment #5 From 2012-07-12 08:09:21 PST -------
Created an attachment (id=151956) [details]
Proposed patch (v3)

Shame on me... sorry for the noise, updated the patch again.
------- Comment #6 From 2012-07-12 09:28:47 PST -------
(From update of attachment 151956 [details])
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
------- Comment #7 From 2012-07-12 09:28:51 PST -------
Created an attachment (id=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
------- Comment #8 From 2012-07-12 14:06:02 PST -------
Created an attachment (id=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.
------- Comment #9 From 2012-07-12 19:50:44 PST -------
(From update of attachment 152052 [details])
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
------- Comment #10 From 2012-07-12 19:50:48 PST -------
Created an attachment (id=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
------- Comment #11 From 2012-07-12 23:02:44 PST -------
Created an attachment (id=152158) [details]
Proposed patch (v5)

Another missing result from svg/css/getComputedStyle-basic.html, now fixed.
------- Comment #12 From 2012-07-16 11:08:22 PST -------
Created an attachment (id=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).
------- Comment #13 From 2012-07-17 07:30:34 PST -------
(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
------- Comment #14 From 2012-07-17 08:36:16 PST -------
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] [details])
> I think we should prefix it with -webkit as far I understand with https://bugs.webkit.org/show_bug.cgi?id=87678#c16
------- Comment #15 From 2012-07-17 09:19:44 PST -------
Created an attachment (id=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.
------- Comment #16 From 2012-07-18 06:35:50 PST -------
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.
------- Comment #17 From 2012-07-18 10:17:43 PST -------
The appropriate spec link is <http://www.w3.org/TR/css3-text/#text-decoration-line>
------- Comment #18 From 2012-07-18 10:31:17 PST -------
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>
------- Comment #19 From 2012-07-19 08:13:35 PST -------
Created an attachment (id=153268) [details]
Proposed patch (v8)

Added missing initializer & copy operator assignment for textDecorationLine @ Source/WebCore/rendering/style/StyleVisualData.cpp.
------- Comment #20 From 2012-07-27 12:40:38 PST -------
(From update of attachment 153268 [details])
I'm currently working on a refactory for this patch, after changes from parent patch (handled in bug 92000).
------- Comment #21 From 2012-07-30 11:41:08 PST -------
Created an attachment (id=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.
------- Comment #22 From 2012-07-31 07:47:34 PST -------
Created an attachment (id=155534) [details]
Patch

Rebased. Please note that patch from bug 92000 should land before this.
------- Comment #23 From 2012-07-31 07:56:53 PST -------
Note: EWS is not able to apply patch because it depends on patch from bug 92000 to be landed first.
------- Comment #24 From 2012-07-31 09:58:01 PST -------
(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.

> 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.
------- Comment #25 From 2012-07-31 10:08:07 PST -------
(In reply to comment #24)
> (From update of attachment 155534 [details] [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 :)
------- Comment #26 From 2012-08-06 18:52:51 PST -------
Created an attachment (id=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.
------- Comment #27 From 2012-08-06 19:11:18 PST -------
Created an attachment (id=156829) [details]
Patch

Fixed double ChangeLog entry.
------- Comment #28 From 2012-08-07 05:34:58 PST -------
(From update of attachment 156829 [details])
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.
------- Comment #29 From 2012-08-08 09:55:01 PST -------
(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?

> 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
------- Comment #30 From 2012-08-08 10:12:11 PST -------
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] [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).
------- Comment #31 From 2012-08-08 11:23:02 PST -------
Created an attachment (id=157256) [details]
Patch

Fixed issues pointed out by Kenneth.
------- Comment #32 From 2012-08-08 16:21:34 PST -------
(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

> 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)
------- Comment #33 From 2012-08-08 20:35:23 PST -------
(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.

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.
------- Comment #34 From 2012-08-08 22:49:03 PST -------
(In reply to comment #33)
> (In reply to comment #32)
> > (From update of attachment 157256 [details] [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.
------- Comment #35 From 2012-08-09 05:07:02 PST -------
(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.
------- Comment #36 From 2012-08-09 05:15:26 PST -------
(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.
------- Comment #37 From 2012-08-09 09:09:01 PST -------
Created an attachment (id=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.
------- Comment #38 From 2012-08-09 09:32:10 PST -------
(From update of attachment 157467 [details])
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?
------- Comment #39 From 2012-08-09 10:21:14 PST -------
> // 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).
------- Comment #40 From 2012-08-09 10:42:23 PST -------
Created an attachment (id=157484) [details]
Patch

Fixed comments, added case where both text-decoration and -webkit-text-decoration properties sets the "important" value to layout test.
------- Comment #41 From 2012-08-09 10:48:24 PST -------
(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
------- Comment #42 From 2012-08-09 10:53:52 PST -------
(In reply to comment #41)
> (From update of attachment 157484 [details] [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.
------- Comment #43 From 2012-08-09 11:08:32 PST -------
Created an attachment (id=157494) [details]
Patch

Using Kenneth's code comment for readibility.
------- Comment #44 From 2012-08-09 13:10:08 PST -------
(From update of attachment 157494 [details])
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
------- Comment #45 From 2012-08-09 13:44:06 PST -------
(From update of attachment 157494 [details])
The checkout got corrupted on one of the instances.  Adam is fixing.
------- Comment #46 From 2012-08-09 14:49:53 PST -------
(From update of attachment 157494 [details])
Clearing flags on attachment: 157494

Committed r125205: <http://trac.webkit.org/changeset/125205>
------- Comment #47 From 2012-08-09 14:50:03 PST -------
All reviewed patches have been landed.  Closing bug.