Bug 90959 - [css3-text] Add support for text-decoration-line
: [css3-text] Add support for text-decoration-line
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Bruno Abinader (history only)
http://www.w3.org/TR/css3-text/#text-...
: WebExposed
Depends on: 93863
Blocks: 58491 90958 91638 92000 94108
  Show dependency treegraph
 
Reported: 2012-07-11 02:43 PDT by Bruno Abinader (history only)
Modified: 2012-08-15 07:18 PDT (History)
20 users (show)

See Also:


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

Updated patch with style issues fixed.
Comment 4 WebKit Review Bot 2012-07-12 08:04:10 PDT
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 Bruno Abinader (history only) 2012-07-12 08:09:21 PDT
Created attachment 151956 [details]
Proposed patch (v3)

Shame on me... sorry for the noise, updated the patch again.
Comment 6 WebKit Review Bot 2012-07-12 09:28:47 PDT
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
Comment 7 WebKit Review Bot 2012-07-12 09:28:51 PDT
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
Comment 8 Bruno Abinader (history only) 2012-07-12 14:06:02 PDT
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.
Comment 9 WebKit Review Bot 2012-07-12 19:50:44 PDT
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
Comment 10 WebKit Review Bot 2012-07-12 19:50:48 PDT
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
Comment 11 Bruno Abinader (history only) 2012-07-12 23:02:44 PDT
Created attachment 152158 [details]
Proposed patch (v5)

Another missing result from svg/css/getComputedStyle-basic.html, now fixed.
Comment 12 Bruno Abinader (history only) 2012-07-16 11:08:22 PDT
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).
Comment 13 Alexis Menard (darktears) 2012-07-17 07:30:34 PDT
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
Comment 14 Bruno Abinader (history only) 2012-07-17 08:36:16 PDT
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
Comment 15 Bruno Abinader (history only) 2012-07-17 09:19:44 PDT
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.
Comment 16 Bruno Abinader (history only) 2012-07-18 06:35:50 PDT
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 Simon Fraser (smfr) 2012-07-18 10:17:43 PDT
The appropriate spec link is <http://www.w3.org/TR/css3-text/#text-decoration-line>
Comment 18 Bruno Abinader (history only) 2012-07-18 10:31:17 PDT
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 Bruno Abinader (history only) 2012-07-19 08:13:35 PDT
Created attachment 153268 [details]
Proposed patch (v8)

Added missing initializer & copy operator assignment for textDecorationLine @ Source/WebCore/rendering/style/StyleVisualData.cpp.
Comment 20 Bruno Abinader (history only) 2012-07-27 12:40:38 PDT
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).
Comment 21 Bruno Abinader (history only) 2012-07-30 11:41:08 PDT
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.
Comment 22 Bruno Abinader (history only) 2012-07-31 07:47:34 PDT
Created attachment 155534 [details]
Patch

Rebased. Please note that patch from bug 92000 should land before this.
Comment 23 Bruno Abinader (history only) 2012-07-31 07:56:53 PDT
Note: EWS is not able to apply patch because it depends on patch from bug 92000 to be landed first.
Comment 24 Allan Sandfeld Jensen 2012-07-31 09:58:01 PDT
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.
Comment 25 Bruno Abinader (history only) 2012-07-31 10:08:07 PDT
(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 :)
Comment 26 Bruno Abinader (history only) 2012-08-06 18:52:51 PDT
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.
Comment 27 Bruno Abinader (history only) 2012-08-06 19:11:18 PDT
Created attachment 156829 [details]
Patch

Fixed double ChangeLog entry.
Comment 28 Bruno Abinader (history only) 2012-08-07 05:34:58 PDT
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.
Comment 29 Kenneth Rohde Christiansen 2012-08-08 09:55:01 PDT
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
Comment 30 Bruno Abinader (history only) 2012-08-08 10:12:11 PDT
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).
Comment 31 Bruno Abinader (history only) 2012-08-08 11:23:02 PDT
Created attachment 157256 [details]
Patch

Fixed issues pointed out by Kenneth.
Comment 32 Kenneth Rohde Christiansen 2012-08-08 16:21:34 PDT
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)
Comment 33 Bruno Abinader (history only) 2012-08-08 20:35:23 PDT
(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.
Comment 34 Kenneth Rohde Christiansen 2012-08-08 22:49:03 PDT
(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.
Comment 35 Bruno Abinader (history only) 2012-08-09 05:07:02 PDT
(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 Bruno Abinader (history only) 2012-08-09 05:15:26 PDT
(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 Bruno Abinader (history only) 2012-08-09 09:09:01 PDT
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.
Comment 38 Kenneth Rohde Christiansen 2012-08-09 09:32:10 PDT
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?
Comment 39 Bruno Abinader (history only) 2012-08-09 10:21:14 PDT
> // 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 Bruno Abinader (history only) 2012-08-09 10:42:23 PDT
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.
Comment 41 Kenneth Rohde Christiansen 2012-08-09 10:48:24 PDT
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
Comment 42 Bruno Abinader (history only) 2012-08-09 10:53:52 PDT
(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.
Comment 43 Bruno Abinader (history only) 2012-08-09 11:08:32 PDT
Created attachment 157494 [details]
Patch

Using Kenneth's code comment for readibility.
Comment 44 WebKit Review Bot 2012-08-09 13:10:08 PDT
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
Comment 45 Eric Seidel 2012-08-09 13:44:06 PDT
Comment on attachment 157494 [details]
Patch

The checkout got corrupted on one of the instances.  Adam is fixing.
Comment 46 WebKit Review Bot 2012-08-09 14:49:53 PDT
Comment on attachment 157494 [details]
Patch

Clearing flags on attachment: 157494

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