Bug 90959

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 :)

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.

Created attachment 151954 [details] Proposed patch (v2) Updated patch with style issues fixed.

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.

Created attachment 151956 [details] Proposed patch (v3) Shame on me... sorry for the noise, updated the patch again.

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

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

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 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

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

Created attachment 152158 [details] Proposed patch (v5) Another missing result from svg/css/getComputedStyle-basic.html, now fixed.

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 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

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

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.

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.

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

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>

Created attachment 153268 [details] Proposed patch (v8) Added missing initializer & copy operator assignment for textDecorationLine @ Source/WebCore/rendering/style/StyleVisualData.cpp.

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).

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.

Created attachment 155534 [details] Patch Rebased. Please note that patch from bug 92000 should land before this.

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

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.

(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 :)

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.

Created attachment 156829 [details] Patch Fixed double ChangeLog entry.

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 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

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).

Created attachment 157256 [details] Patch Fixed issues pointed out by Kenneth.

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)

(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.

(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.

(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.

(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.

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 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?

> // 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).

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 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

(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.

Created attachment 157494 [details] Patch Using Kenneth's code comment for readibility.

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 on attachment 157494 [details] Patch The checkout got corrupted on one of the instances. Adam is fixing.

Comment on attachment 157494 [details] Patch Clearing flags on attachment: 157494 Committed r125205: <http://trac.webkit.org/changeset/125205>

All reviewed patches have been landed. Closing bug.