Bug 94094 - [css3-text] Add rendering support for -webkit-text-decoration-style
Summary: [css3-text] Add rendering support for -webkit-text-decoration-style
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Bruno Abinader (history only)
URL: http://dev.w3.org/csswg/css3-text/#te...
Keywords:
Depends on: 93863 94093 94927 99804
Blocks: 58491 99986 90958 92000 93507 93509 96408 100064
  Show dependency treegraph
 
Reported: 2012-08-15 05:24 PDT by Bruno Abinader (history only)
Modified: 2012-10-22 19:42 PDT (History)
22 users (show)

See Also:


Attachments
Patch (22.16 KB, patch)
2012-08-15 09:32 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (21.37 KB, patch)
2012-08-15 11:36 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (EWS run only) (103.40 KB, patch)
2012-08-17 08:20 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (24.53 KB, patch)
2012-08-20 10:13 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (23.41 KB, patch)
2012-08-21 11:40 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (EWS run only) (32.19 KB, patch)
2012-08-21 11:48 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Text decoration styles on Firefox (15.40 KB, image/png)
2012-08-29 05:58 PDT, Bruno Abinader (history only)
no flags Details
Patch (28.57 KB, patch)
2012-08-29 10:03 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (EWS run only) (37.20 KB, patch)
2012-08-29 10:26 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (37.26 KB, patch)
2012-09-06 09:35 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (EWS run only) (45.89 KB, patch)
2012-09-06 09:36 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (28.17 KB, patch)
2012-10-15 11:25 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (EWS run only) (37.00 KB, patch)
2012-10-15 11:29 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (24.51 KB, patch)
2012-10-16 14:50 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (EWS run only) (33.37 KB, patch)
2012-10-17 07:25 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (24.55 KB, patch)
2012-10-17 09:59 PDT, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (EWS run only) (33.29 KB, patch)
2012-10-18 08:36 PDT, Bruno Abinader (history only)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (24.13 KB, patch)
2012-10-22 05:46 PDT, Bruno Abinader (history only)
bruno.abinader: commit-queue-
Details | Formatted Diff | Diff
Patch (24.33 KB, patch)
2012-10-22 08:01 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-08-15 05:24:24 PDT
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.
Comment 1 Bruno Abinader (history only) 2012-08-15 09:32:59 PDT
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.
Comment 2 Bruno Abinader (history only) 2012-08-15 11:36:20 PDT
Created attachment 158606 [details]
Patch

Removed comment in Source/WebCore/rendering/RenderObject, as it affects changes made by bug 94131.
Comment 3 Bruno Abinader (history only) 2012-08-15 15:51:03 PDT
Comment on attachment 158606 [details]
Patch

Requesting review and commit-queue flags now that bug 93863 has landed - r125716.
Comment 4 Bruno Abinader (history only) 2012-08-17 08:20:48 PDT
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.
Comment 5 Bruno Abinader (history only) 2012-08-20 10:13:36 PDT
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.
Comment 6 Bruno Abinader (history only) 2012-08-21 11:40:05 PDT
Created attachment 159732 [details]
Patch

Updated layout tests.
Comment 7 Bruno Abinader (history only) 2012-08-21 11:48:57 PDT
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 8 Build Bot 2012-08-21 12:31:22 PDT
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
Comment 9 Bruno Abinader (history only) 2012-08-21 12:58:14 PDT
(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 10 Build Bot 2012-08-21 14:08:45 PDT
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
Comment 11 Gustavo Noronha (kov) 2012-08-23 11:01:52 PDT
The problem seems to be that the .in files are not being touched, so they end up not being reprocessed.
Comment 12 Bruno Abinader (history only) 2012-08-23 11:27:41 PDT
(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 13 Julien Chaffraix 2012-08-23 15:22:30 PDT
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.
Comment 14 Gustavo Noronha (kov) 2012-08-24 05:49:15 PDT
https://bugs.webkit.org/show_bug.cgi?id=94927 should fix this build issue for gtk
Comment 15 Bruno Abinader (history only) 2012-08-29 05:58:24 PDT
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 :)
Comment 16 Bruno Abinader (history only) 2012-08-29 10:03:52 PDT
Created attachment 161257 [details]
Patch
Comment 17 Bruno Abinader (history only) 2012-08-29 10:04:48 PDT
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').
Comment 18 Bruno Abinader (history only) 2012-08-29 10:26:00 PDT
Created attachment 161263 [details]
Patch (EWS run only)
Comment 19 Build Bot 2012-08-29 11:25:51 PDT
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 20 Build Bot 2012-08-29 12:08:55 PDT
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 21 WebKit Review Bot 2012-08-29 20:55:44 PDT
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 22 WebKit Review Bot 2012-08-29 21:58:32 PDT
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
Comment 23 Bruno Abinader (history only) 2012-09-06 09:35:02 PDT
Created attachment 162523 [details]
Patch

Added Chromium-specific expectations
Comment 24 Bruno Abinader (history only) 2012-09-06 09:36:39 PDT
Created attachment 162524 [details]
Patch (EWS run only)
Comment 25 Build Bot 2012-09-06 09:59:33 PDT
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 26 Julien Chaffraix 2012-09-06 17:41:54 PDT
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.
Comment 27 Bruno Abinader (history only) 2012-09-11 05:59:51 PDT
(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.
Comment 28 Alexander Pavlov (apavlov) 2012-10-11 07:01:28 PDT
@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.
Comment 29 Bruno Abinader (history only) 2012-10-11 07:14:37 PDT
(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.
Comment 30 Julien Chaffraix 2012-10-11 10:08:33 PDT
(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".
Comment 31 Alexander Pavlov (apavlov) 2012-10-11 12:22:08 PDT
(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).
Comment 32 Julien Chaffraix 2012-10-12 10:29:26 PDT
> 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 33 Julien Chaffraix 2012-10-12 10:41:41 PDT
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.
Comment 34 Bruno Abinader (history only) 2012-10-15 11:25:33 PDT
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.
Comment 35 Bruno Abinader (history only) 2012-10-15 11:29:44 PDT
Created attachment 168747 [details]
Patch (EWS run only)
Comment 36 Build Bot 2012-10-15 12:10:41 PDT
Comment on attachment 168745 [details]
Patch

Attachment 168745 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14293757
Comment 37 Build Bot 2012-10-15 12:12:49 PDT
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 38 Peter Beverloo (cr-android ews) 2012-10-15 13:23:05 PDT
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 39 Peter Beverloo (cr-android ews) 2012-10-15 13:59:16 PDT
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 40 Julien Chaffraix 2012-10-15 14:22:56 PDT
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 41 Build Bot 2012-10-15 14:50:46 PDT
Comment on attachment 168745 [details]
Patch

Attachment 168745 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14288853
Comment 42 Build Bot 2012-10-15 15:06:45 PDT
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 43 WebKit Review Bot 2012-10-15 15:48:00 PDT
Comment on attachment 168745 [details]
Patch

Attachment 168745 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14296814
Comment 44 WebKit Review Bot 2012-10-15 17:11:36 PDT
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 45 WebKit Review Bot 2012-10-15 18:14:15 PDT
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
Comment 46 Bruno Abinader (history only) 2012-10-16 14:50:32 PDT
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 47 Build Bot 2012-10-16 15:27:02 PDT
Comment on attachment 169034 [details]
Patch

Attachment 169034 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14386307
Comment 48 Bruno Abinader (history only) 2012-10-17 07:25:49 PDT
Created attachment 169181 [details]
Patch (EWS run only)
Comment 49 Build Bot 2012-10-17 08:16:19 PDT
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 50 Peter Beverloo (cr-android ews) 2012-10-17 08:47:41 PDT
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 51 Build Bot 2012-10-17 08:59:18 PDT
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
Comment 52 Bruno Abinader (history only) 2012-10-17 09:59:04 PDT
Created attachment 169202 [details]
Patch

Added a initial value to strokeStyle to avoid warnings on chromium build.
Comment 53 Kenneth Rohde Christiansen 2012-10-17 10:00:44 PDT
Did you see this:

http://lists.w3.org/Archives/Public/www-style/2012Aug/0379.html
Comment 54 Bruno Abinader (history only) 2012-10-17 11:00:31 PDT
(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?
Comment 55 Bruno Abinader (history only) 2012-10-18 08:36:46 PDT
Created attachment 169420 [details]
Patch (EWS run only)
Comment 56 Build Bot 2012-10-18 09:01:28 PDT
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 57 WebKit Review Bot 2012-10-18 10:12:17 PDT
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
Comment 58 Bruno Abinader (history only) 2012-10-18 12:37:05 PDT
(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 59 Julien Chaffraix 2012-10-19 11:43:30 PDT
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.
Comment 60 Bruno Abinader (history only) 2012-10-22 05:46:31 PDT
Created attachment 169887 [details]
Patch

Reviewed patch with s/CSS3_TEXT_DECORATION/CSS3_TEXT/
Comment 61 Bruno Abinader (history only) 2012-10-22 05:53:11 PDT
(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 62 Bruno Abinader (history only) 2012-10-22 07:56:12 PDT
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.
Comment 63 Bruno Abinader (history only) 2012-10-22 08:01:40 PDT
Created attachment 169912 [details]
Patch

Moved layout tests to fast/css3-text/css3-text-decoration accordingly.
Comment 64 WebKit Review Bot 2012-10-22 08:52:33 PDT
Comment on attachment 169912 [details]
Patch

Clearing flags on attachment: 169912

Committed r132076: <http://trac.webkit.org/changeset/132076>
Comment 65 Bruno Abinader (history only) 2012-10-22 10:34:01 PDT
Comment on attachment 169202 [details]
Patch

Cleaning review flags as patch has landed already.