This patch implements the "text-decoration-style" property rendering as specified in CSS3 working draft, with "-webkit-" prefix. The specification can be found below: http://dev.w3.org/csswg/css3-text/#text-decoration-style Additionally, Mozilla implementation details can be found here: https://developer-dev.allizom.org/en-US/docs/CSS/text-decoration-style This is an individual task for bug 90958. Parsing support is handled in bug 94093 and should be landed first.
Created attachment 158580 [details] Patch Proposed patch. Please note EWS will fail as this patch depends on bug 93863 and bug 94093 to be landed first, thus not requesting review / commit-queue flags for now.
Created attachment 158606 [details] Patch Removed comment in Source/WebCore/rendering/RenderObject, as it affects changes made by bug 94131.
Comment on attachment 158606 [details] Patch Requesting review and commit-queue flags now that bug 93863 has landed - r125716.
Created attachment 159128 [details] Patch (EWS run only) This patch adds changes to make CSS3_TEXT_DECORATION feature flag enabled and remove skip of fast/css3-text-decoration layout test directory from chromium build (expected results included). This is intentional to get EWS results, but not intended for landing (as suggested by Peter Beverloo in http://lists.webkit.org/pipermail/webkit-dev/2012-August/021925.html ). This is a workaround to get proper EWS results for the previous patch.
Created attachment 159466 [details] Patch Updated patch: Added missing Qt platform-specific hooks for StrokeStyle enum handling, updated layout tests to test corner cases and improve readibility.
Created attachment 159732 [details] Patch Updated layout tests.
Created attachment 159733 [details] Patch (EWS run only) Patch for EWS run only. Skipping fast/css3-text-decoration/repaint/repaint-text-decoration-style.html as it requires platform support for missing text decoration styles.
Comment on attachment 159733 [details] Patch (EWS run only) Attachment 159733 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13543916
(In reply to comment #8) > (From update of attachment 159733 [details]) > Attachment 159733 [details] did not pass mac-ews (mac): > Output: http://queues.webkit.org/results/13543916 I wonder why these platforms are ignoring the defines set in both Source/WebCore/css/CSSValueKeywords.in and Source/WebCore/css/CSSPropertyNames.in...
Comment on attachment 159733 [details] Patch (EWS run only) Attachment 159733 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13554347
The problem seems to be that the .in files are not being touched, so they end up not being reprocessed.
(In reply to comment #11) > The problem seems to be that the .in files are not being touched, so they end up not being reprocessed. Indeed, like discussed in webkit-dev (see http://lists.webkit.org/pipermail/webkit-dev/2012-August/022025.html), we need to fill a bug with a scoped test case to identify the root of this behavior.
Comment on attachment 159732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159732&action=review r- for the 'double' decoration which I think is badly implemented. I think you will need a bit more testing of this case as it's more complex. > Source/WebCore/ChangeLog:16 > + 94093 and should be landed first. If 94093 wasn't landed, this change wouldn't compile when the flag is enabled. I hope you are ensuring this ordering to happen so it's not super useful to mention in the ChangeLog. > Source/WebCore/rendering/InlineTextBox.cpp:988 > +#endif // CSS3_TEXT_DECORATION Changing the function signature to always have |decorationStyle| will likely break the build due to the unused variable if you disabled the compile time flag. You can use UNUSED_PARAM in an #else branch here to solve the issue. > Source/WebCore/rendering/InlineTextBox.cpp:1008 > +#if ENABLE(CSS3_TEXT_DECORATION) > + context->setStrokeStyle(strokeStyle); > +#else > + context->setStrokeStyle(SolidStroke); > +#endif // CSS3_TEXT_DECORATION If you move the definition of |strokeStyle| out of your #if-def above, you can replace that with context->setStrokeStyle(strokeStyle); > Source/WebCore/rendering/InlineTextBox.cpp:1031 > + context->drawLineForText(FloatPoint(localOrigin.x(), localOrigin.y() + 1.5 * baseline / 3), width, isPrinting); > + context->drawLineForText(FloatPoint(localOrigin.x(), localOrigin.y() + 2.5 * baseline / 3), width, isPrinting); I don't think this is right. 'double' should match 'border-style' and here is what CSS 2.1 says about 'double': Two parallel solid lines with some space between them. (The thickness of the lines is not specified, but the sum of the lines and the space must equal ‘border-width’.) You are basically drawing a border that will be too thick. Also what is preventing those 2 lines to overlap? Probably a crazy idea but shouldn't there be a way to unify some of this code with our border painting code (see RenderObject::drawLineForBoxSide)? We already support 'border-style: double' (and other values) and I would rather avoid re-implementing that logic with more bugs in it. > Source/WebCore/rendering/InlineTextBox.h:187 > +#if ENABLE(CSS3_TEXT_DECORATION) > + void paintDecoration(GraphicsContext*, const FloatPoint& boxOrigin, ETextDecoration textDecorations, TextDecorationStyle, const ShadowData*); > +#else > void paintDecoration(GraphicsContext*, const FloatPoint& boxOrigin, int decoration, const ShadowData*); > +#endif // CSS3_TEXT_DECORATION We shouldn't have 2 different declarations / definitions of this function. If you need to pass TextDecorationStyle then let's do it. > LayoutTests/ChangeLog:11 > + * fast/css3-text-decoration/repaint/repaint-text-decoration-style-expected.png: Added. It's sad that we land a bad baseline but we don't have much choice here, unless you implement Mac or Chromium. It's worth mentioning that the baseline is known to over-repaint. > LayoutTests/fast/css3-text-decoration/repaint/repaint-text-decoration-style.html:12 > + <script src="../../repaint/resources/repaint.js" type="text/javascript"></script> We don't really need a repaint test to test the basic functionality. I would rather have a regular paint test for that and a small repaint test to see that invalidation are done properly. > LayoutTests/fast/css3-text-decoration/repaint/repaint-text-decoration-style.html:18 > + document.getElementById("test-underline-dashed").style.webkitTextDecorationStyle = 'dashed'; We don't really see the dashed one due to the small length of your text below. I definitely think it would be better to have one long line per invalidation as it would make the results more obvious.
https://bugs.webkit.org/show_bug.cgi?id=94927 should fix this build issue for gtk
Created attachment 161206 [details] Text decoration styles on Firefox I've generated this screenshot of text decoration styles on Firefox as basis for my implementation. The specification says the meaning is the same as values from 'border-style', however you can see that for 'double' implementation both line thickness and space between lines are visually not the same as 'border-width'. This makes sense as 'border-width' value shouldn't affect text decoration, in a sense that currently we can't directly set a value for text decoration line thickness (there's a FIXME on the code - defaults to 1.f - shall we propose a 'text-decoration-width' property?. I am, though, modifying the 'double' implementation to use the same 'third of thickness' approach as 'border-width' does to calculate the space between lines. A patch with the modified changes will arrive soon :)
Created attachment 161257 [details] Patch
The fast/css3-text-decoration/text-decoration-style.html layout test also comes with matching border styles, so you can expect same style meaning (as explained earlier, Firefox also does not use the same border thickness on text decoration lines). As currently text decoration thickness is fixed to '1.f', the 'third of thickness' code from drawLineForBoxSide is not needed, as it only works from thickness greater than 3 (when less than 3, double 'acts' like 'solid' on 'border-style').
Created attachment 161263 [details] Patch (EWS run only)
Comment on attachment 161263 [details] Patch (EWS run only) Attachment 161263 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13691297
Comment on attachment 161263 [details] Patch (EWS run only) Attachment 161263 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13683363
Comment on attachment 161263 [details] Patch (EWS run only) Attachment 161263 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13684528 New failing tests: fast/css3-text-decoration/text-decoration-style.html animations/suspend-resume-animation-events.html fast/css3-text-decoration/repaint/repaint-text-decoration-style.html
Comment on attachment 161263 [details] Patch (EWS run only) Attachment 161263 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13695519 New failing tests: fast/css3-text-decoration/text-decoration-style.html fast/css3-text-decoration/repaint/repaint-text-decoration-style.html
Created attachment 162523 [details] Patch Added Chromium-specific expectations
Created attachment 162524 [details] Patch (EWS run only)
Comment on attachment 162524 [details] Patch (EWS run only) Attachment 162524 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13766472
Comment on attachment 162523 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162523&action=review The code change looks fine. One more question. > The specification says the meaning is the same as values from 'border-style', however you can see that for 'double' implementation both line thickness and space between lines are visually not the same as 'border-width'. You misunderstood what I said: 'border-style' explicitly mentions that for 'double', the lines including the spaces should be 'border-width'. Here it would make sense to have something similar defined. As you pointed out, it would make sense for 'text-decoration-width' to fill this role. It's orthogonal to this bug though. > Source/WebCore/rendering/InlineTextBox.cpp:1028 > + context->drawLineForText(FloatPoint(localOrigin.x(), localOrigin.y() + doubleOffset + 2 * baseline / 3), width, isPrinting); Where does this formula come from? Is this matching what Firefox or IE are doing? > LayoutTests/ChangeLog:14 > + * fast/css3-text-decoration/repaint/repaint-text-decoration-style-expected.png: Added. This baseline is wrong and was generated on a platform that doesn't support repaint tracking. > LayoutTests/ChangeLog:26 > + Chromium-specific expectations (chromium does not render 'dashed', > + 'dotted' and 'wavy' styles - handled in bug 93509). Why do you land the wrong results for Chromium? It's a bad idea as those would be considered expected (read good) results for Chromium.
(In reply to comment #26) > (From update of attachment 162523 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162523&action=review > > Source/WebCore/rendering/InlineTextBox.cpp:1028 > > + context->drawLineForText(FloatPoint(localOrigin.x(), localOrigin.y() + doubleOffset + 2 * baseline / 3), width, isPrinting); > > Where does this formula come from? Is this matching what Firefox or IE are doing? This formula mimics the behavior from single line-through (see line 1030), with the addition of doubleOffset (which is never zero, so it avoids lines colliding with each other). As far as I've checked it matches what Firefox is doing. > > LayoutTests/ChangeLog:14 > > + * fast/css3-text-decoration/repaint/repaint-text-decoration-style-expected.png: Added. > > This baseline is wrong and was generated on a platform that doesn't support repaint tracking. > > > LayoutTests/ChangeLog:26 > > + Chromium-specific expectations (chromium does not render 'dashed', > > + 'dotted' and 'wavy' styles - handled in bug 93509). > > Why do you land the wrong results for Chromium? It's a bad idea as those would be considered expected (read good) results for Chromium. Shall I completely remove pixel results from both Qt and Chromium, then? I could add a line to TestExpectations to ignore pixel results from this test until Chromium fixes the line style rendering issues.
@Bruno, Julien: do you have any estimates about when this feature can be completed? Web Inspector suggests "blink" as a valid "text-decoration" value, and we've been asked to remove this value, since it is not actually rendered by WebCore now. My guess is that it's not wise to remove something that can become valid soon enough.
(In reply to comment #28) > @Bruno, Julien: do you have any estimates about when this feature can be completed? Web Inspector suggests "blink" as a valid "text-decoration" value, and we've been asked to remove this value, since it is not actually rendered by WebCore now. My guess is that it's not wise to remove something that can become valid soon enough. This patch is waiting for review AFAIK. There's a layout test results issue that should be fixed when the platform support for "wavy" style comes around (already implemented on Qt and Cairo platforms, thanks to Lamarque and KyungTae, respectively). The blink value continues to be accepted as valid value, it just gets disconsidered as WebKit does not have a rendering implementation for that.
(In reply to comment #28) > @Bruno, Julien: do you have any estimates about when this feature can be completed? Web Inspector suggests "blink" as a valid "text-decoration" value, and we've been asked to remove this value, since it is not actually rendered by WebCore now. My guess is that it's not wise to remove something that can become valid soon enough. The new feature is guarded by a feature flag which is disabled on Chromium. If the inspector is suggesting a value that is not recognized by our parser, I don't think finishing the feature will change anything. Also "blink" is valid for "-webkit-text-decoration" not "text-decoration".
(In reply to comment #30) > (In reply to comment #28) > The new feature is guarded by a feature flag which is disabled on Chromium. If the inspector is suggesting a value that is not recognized by our parser, I don't think finishing the feature will change anything. Also "blink" is valid for "-webkit-text-decoration" not "text-decoration". That's what I wanted to clarify. I believe the flag is there exactly for the reason of the CSS3 Text module support not being ready for the prime time. I don't see any reasons to have this flag disabled on Chromium once the feature is implemented in full (in the scope deemed sufficient for the enablement). Also, the value IS recognized by the parser (add "text-decoration: blink" to any style in the DevTools and notice that the property is not marked with an exclamation mark in a yellow triangle).
> Also, the value IS recognized by the parser (add "text-decoration: blink" to any style in the DevTools and notice that the property is not marked with an exclamation mark in a yellow triangle). You are right, I should have checked the code before answering from memory as I got the answer backwards. "text-decoration: blink" predates implementing CSS 3 text (the parsing bits have been around since 2006 at least). As -webkit-text-decoration explicitly ignores "blink" (see CSSParser.cpp:8033), landing this bug won't help the webinspector. Not sure when it was broken - or if it ever worked - but the only reference I have found on the issue is a closed Chromium bug (http://crbug.com/1235) which suggests the rendering has been broken for years.
Comment on attachment 162523 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162523&action=review Sorry for the delay. Some comments but the general direction is good. If we can get another round with passing EWS, it should be good to go. > Source/WebCore/rendering/InlineTextBox.cpp:763 > +#if ENABLE(CSS3_TEXT_DECORATION) > + paintDecoration(context, boxOrigin, textDecorations, styleToUse->textDecorationStyle(), textShadow); > +#else > + paintDecoration(context, boxOrigin, textDecorations, TextDecorationStyleSolid, textShadow); > +#endif // CSS3_TEXT_DECORATION If textDecorationStyle was not #if'defed out you could just write this down as: paintDecoration(context, boxOrigin, textDecorations, styleToUse->textDecorationStyle(), textShadow); which looks better to me. > Source/WebCore/rendering/InlineTextBox.cpp:985 > + StrokeStyle strokeStyle = SolidStroke; > +#if ENABLE(CSS3_TEXT_DECORATION) > + switch (decorationStyle) { > + case TextDecorationStyleSolid: > + break; > + case TextDecorationStyleDouble: > + strokeStyle = DoubleStroke; > + break; > + case TextDecorationStyleDotted: > + strokeStyle = DottedStroke; > + break; > + case TextDecorationStyleDashed: > + strokeStyle = DashedStroke; > + break; > + case TextDecorationStyleWavy: > + // FIXME: https://bugs.webkit.org/show_bug.cgi?id=92868 - Needs platform support. > + strokeStyle = WavyStroke; > + break; > + } This should go in a helper function so that you just write: StrokeStyle strokeStyle = textDecorationStyleToStrokeStyle(decorationStyle); It also would remove the weird TextDecorationStyleSolid case and the UNUSED_PARAM which are artificial. >>> Source/WebCore/rendering/InlineTextBox.cpp:1028 >>> + context->drawLineForText(FloatPoint(localOrigin.x(), localOrigin.y() + doubleOffset + 2 * baseline / 3), width, isPrinting); >> >> Where does this formula come from? Is this matching what Firefox or IE are doing? > > This formula mimics the behavior from single line-through (see line 1030), with the addition of doubleOffset (which is never zero, so it avoids lines colliding with each other). As far as I've checked it matches what Firefox is doing. Please add some comment about that in the ChangeLog. > Source/WebCore/rendering/style/RenderStyleConstants.h:-351 > -#endif // CSS3_TEXT_DECORATION You should still hide the non-reachable value for TextDecorationStyle if CSS3_TEXT_DECORATION is disabled. >>> LayoutTests/ChangeLog:14 >>> + * fast/css3-text-decoration/repaint/repaint-text-decoration-style-expected.png: Added. >> >> This baseline is wrong and was generated on a platform that doesn't support repaint tracking. > > Shall I completely remove pixel results from both Qt and Chromium, then? I could add a line to TestExpectations to ignore pixel results from this test until Chromium fixes the line style rendering issues. Fine by me.
Created attachment 168745 [details] Patch Updated changes based on Julien's review - got rid of non-necessary #ifdef's, moved text decoration to stroke style code to a static function, commented code on ChangeLog.
Created attachment 168747 [details] Patch (EWS run only)
Comment on attachment 168745 [details] Patch Attachment 168745 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14293757
Comment on attachment 168747 [details] Patch (EWS run only) Attachment 168747 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14294725
Comment on attachment 168745 [details] Patch Attachment 168745 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14298784
Comment on attachment 168747 [details] Patch (EWS run only) Attachment 168747 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14300753
Comment on attachment 168745 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168745&action=review > Source/WebCore/platform/graphics/GraphicsContext.h:147 > +#if ENABLE(CSS3_TEXT_DECORATION) > + DoubleStroke, > + DottedStroke, > + DashedStroke, > + WavyStroke > +#else > DottedStroke, > DashedStroke > +#endif // CSS3_TEXT_DECORATION You may as well just put the DoubleStroke and WavyStroke at the end. It would be more readable to me even if not alphabetically ordered. > Source/WebCore/rendering/style/RenderStyle.h:-622 > -#if ENABLE(CSS3_TEXT_DECORATION) > TextDecorationStyle textDecorationStyle() const { return static_cast<TextDecorationStyle>(rareNonInheritedData->m_textDecorationStyle); } > -#endif // CSS3_TEXT_DECORATION This could return the only available value (TextDecorationStyleSolid) when CSS3_TEXT_DECORATION is disabled instead of having to remove the #if everywhere. > Source/WebCore/rendering/style/RenderStyleConstants.h:349 > }; Exposing all the values is what breaks all platforms as they need to be handled in 'switch' statements. I would just define TextDecorationStyleSolid if CSS3_TEXT_DECORATION is off and return this value in textDecorationStyle above. Open to better suggestions though. > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:-183 > -#if ENABLE(CSS3_TEXT_DECORATION) > unsigned m_textDecorationStyle : 3; // TextDecorationStyle > -#endif // CSS3_TEXT_DECORATION You are basically adding the cost of the new feature even if we don't enable it which is wrong. This should still be behind #if guard.
Comment on attachment 168745 [details] Patch Attachment 168745 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14288853
Comment on attachment 168747 [details] Patch (EWS run only) Attachment 168747 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14292875
Comment on attachment 168745 [details] Patch Attachment 168745 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14296814
Comment on attachment 168747 [details] Patch (EWS run only) Attachment 168747 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14291855 New failing tests: fast/css3-text-decoration/text-decoration-style.html fast/css3-text-decoration/repaint/repaint-text-decoration-style.html
Comment on attachment 168747 [details] Patch (EWS run only) Attachment 168747 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14297814 New failing tests: fast/css3-text-decoration/text-decoration-style.html fast/css3-text-decoration/repaint/repaint-text-decoration-style.html
Created attachment 169034 [details] Patch Revert a bunch of #ifdef's removal by letting textDecorationStyle() return a single value when CSS3_TEXT_DECORATION feature flag is disabled.
Comment on attachment 169034 [details] Patch Attachment 169034 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14386307
Created attachment 169181 [details] Patch (EWS run only)
Comment on attachment 169181 [details] Patch (EWS run only) Attachment 169181 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14390574
Comment on attachment 169181 [details] Patch (EWS run only) Attachment 169181 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14400455
Comment on attachment 169181 [details] Patch (EWS run only) Attachment 169181 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14391611
Created attachment 169202 [details] Patch Added a initial value to strokeStyle to avoid warnings on chromium build.
Did you see this: http://lists.w3.org/Archives/Public/www-style/2012Aug/0379.html
(In reply to comment #53) > Did you see this: > > http://lists.w3.org/Archives/Public/www-style/2012Aug/0379.html Very interesting read. WebKit currently draws 'line-through' according to the font width, so it varies accordingly on the first test-case. For the second and third test cases, the subscript and superscript 'bar' gets a varying height for underline, which disagrees with Aryeh's proposal, but makes perfect sense. IMO, I agree with current WebKit's implementation: if one type of text decoration varies in height according to the font style, so does all the others. What do you guys think?
Created attachment 169420 [details] Patch (EWS run only)
Comment on attachment 169420 [details] Patch (EWS run only) Attachment 169420 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14466009
Comment on attachment 169420 [details] Patch (EWS run only) Attachment 169420 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14456101 New failing tests: fast/css3-text-decoration/text-decoration-style.html fast/css3-text-decoration/repaint/repaint-text-decoration-style.html
(In reply to comment #57) > (From update of attachment 169420 [details]) > Attachment 169420 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/14456101 > > New failing tests: > fast/css3-text-decoration/text-decoration-style.html > fast/css3-text-decoration/repaint/repaint-text-decoration-style.html As previously said, the tests above are failing because I've removed the expected results for now, at least until all the text decoration styles are implemented on platform-side (this is OK for now as the layout tests are disabled on TestExpectations).
Comment on attachment 169202 [details] Patch There will be a clash with the CSS3_TEXT_DECORATION -> CSS3_TEXT rename that will land before this change. I don't need to see the updated patch as it's a mechanical rewrite. Thanks for fixing the nits. Concerning the inconsistency with other browsers, it's an orthogonal bug as you are relying on our existing behavior. Please file a follow-up bug about it as it's definitely something we will want to investigate and fix depending on what the WG decides.
Created attachment 169887 [details] Patch Reviewed patch with s/CSS3_TEXT_DECORATION/CSS3_TEXT/
(In reply to comment #59) > (From update of attachment 169202 [details]) > There will be a clash with the CSS3_TEXT_DECORATION -> CSS3_TEXT rename that will land before this change. I don't need to see the updated patch as it's a mechanical rewrite. Thanks for fixing the nits. Thank you Julien! Attachment 169887 [details] has been uploaded with the namespace renaming and your review applied to ChangeLog. > > Concerning the inconsistency with other browsers, it's an orthogonal bug as you are relying on our existing behavior. Please file a follow-up bug about it as it's definitely something we will want to investigate and fix depending on what the WG decides. I've created a new bug as you suggested (bug 99986) to track down this issue, whenever WebKit needs to modify its rendering behavior on this topic.
Comment on attachment 169887 [details] Patch Just noticed the layout tests directory has been moved recently, I'm going to update the layout tests file paths accordingly and resend the patch.
Created attachment 169912 [details] Patch Moved layout tests to fast/css3-text/css3-text-decoration accordingly.
Comment on attachment 169912 [details] Patch Clearing flags on attachment: 169912 Committed r132076: <http://trac.webkit.org/changeset/132076>
Comment on attachment 169202 [details] Patch Cleaning review flags as patch has landed already.