Bug 91638

Summary: [css3-text] Add support for text-decoration-color
Product: WebKit Reporter: Bruno Abinader (history only) <bruno.abinader>
Component: CSSAssignee: Bruno Abinader (history only) <bruno.abinader>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, abecsi, allan.jensen, cmarcelo, dbates, dglazkov, donggwan.kim, eric, esprehn+autocc, gtk-ews, gustavo, gyuyoung.kim, hyatt, igor.oliveira, jchaffraix, koivisto, lamarque, macpherson, masayuki, menard, mitz, ojan.autocc, ojan, peter, rakuco, rv.kamath, senorblanco, simon.fraser, syoichi, tony, vestbo, webkit.review.bot, xan.lopez
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://dev.w3.org/csswg/css-text-decor-3/#text-decoration-color-property
Bug Depends on: 90959, 93829, 93863, 94093, 99804    
Bug Blocks: 58491, 92000    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from gce-cr-linux-06
none
Archive of layout-test-results from gce-cr-linux-08
none
Patch
none
Archive of layout-test-results from gce-cr-linux-06
none
Archive of layout-test-results from gce-cr-linux-03
none
Patch
none
Archive of layout-test-results from apple-mac-2
none
Patch
none
Patch (EWS run only)
none
Archive of layout-test-results from gce-cr-linux-06
none
Patch
none
Patch (EWS run only)
none
Archive of layout-test-results from gce-cr-linux-02
none
Archive of layout-test-results from gce-cr-linux-06
none
Patch
none
Patch (EWS run only)
none
Patch (please ignore - should be in bug 92000).
none
Patch
none
Patch (EWS run only)
none
Patch
none
Patch (EWS run only)
none
Patch
none
Patch (EWS run only)
none
Patch
none
Patch
none
Patch (EWS only)
none
Patch
none
Patch
jchaffraix: review+, jchaffraix: commit-queue-
Patch for landing none

Description Bruno Abinader (history only) 2012-07-18 10:30:48 PDT
WebKit currently only has support for CSS text-decoration property, but CSS3's text-decoration-color. This is an individual task for bug 58491.

More info about "text-decoration-color" property can be found here: http://www.w3.org/TR/css3-text/#text-decoration-color

Mozilla implementation (using -moz prefix) is described here: https://developer.mozilla.org/en/CSS/text-decoration-color
Comment 1 Bruno Abinader (history only) 2012-07-31 19:14:43 PDT
Created attachment 155702 [details]
Patch

Proposed patch. Please note that patches from bug 92000, bug 90959 and bug 90958 (respectively) should land before this.
Comment 2 Bruno Abinader (history only) 2012-08-01 05:18:53 PDT
Created attachment 155785 [details]
Patch

Reverted part of original getTextDecorations() code to avoid behavior change if no -webkit-text-decoration-color property is present. Please note that patches from bug 92000, bug 90959 and bug 90958 (respectively) should land before this.
Comment 3 Bruno Abinader (history only) 2012-08-02 10:07:27 PDT
Created attachment 156113 [details]
Patch

Fixed failing layout tests plus optimization changes on RenderObject::getTextDecorationColors. Please note that patches from bug 92000, bug 90959 and bug 90958 (respectively) should land before this.
Comment 4 Bruno Abinader (history only) 2012-08-06 19:28:25 PDT
Created attachment 156834 [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 5 WebKit Review Bot 2012-08-06 21:14:43 PDT
Comment on attachment 156834 [details]
Patch

Attachment 156834 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13444666

New failing tests:
fast/css/text-decoration-color.html
fast/selectors/visited-descendant.html
fast/css/child-style-can-override-visited-style.html
Comment 6 WebKit Review Bot 2012-08-06 21:14:49 PDT
Created attachment 156855 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 7 WebKit Review Bot 2012-08-06 22:18:56 PDT
Comment on attachment 156834 [details]
Patch

Attachment 156834 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13446586

New failing tests:
fast/css/text-decoration-color.html
fast/selectors/visited-descendant.html
fast/css/child-style-can-override-visited-style.html
Comment 8 WebKit Review Bot 2012-08-06 22:19:01 PDT
Created attachment 156863 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 9 Bruno Abinader (history only) 2012-08-07 20:33:08 PDT
Created attachment 157097 [details]
Patch

Fixed bug in RenderObject::getTextDecorationColors, added chromium-linux layout test expectations.
Comment 10 WebKit Review Bot 2012-08-07 21:44:56 PDT
Comment on attachment 157097 [details]
Patch

Attachment 157097 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13455356

New failing tests:
fast/css/text-decoration-color.html
Comment 11 WebKit Review Bot 2012-08-07 21:45:01 PDT
Created attachment 157107 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 12 WebKit Review Bot 2012-08-07 22:47:49 PDT
Comment on attachment 157097 [details]
Patch

Attachment 157097 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13451670

New failing tests:
fast/css/text-decoration-color.html
Comment 13 WebKit Review Bot 2012-08-07 22:47:57 PDT
Created attachment 157121 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 14 Bruno Abinader (history only) 2012-08-08 06:47:00 PDT
Created attachment 157206 [details]
Patch

As similarly happened to patch from bug 90958, my local chromium build has font aliasing issues, now using expected image generated from build bot.
Comment 15 Bruno Abinader (history only) 2012-08-08 07:26:58 PDT
Comment on attachment 157206 [details]
Patch

This patch, as part of the new CSS3 text decoration properties work, is now ready for review. The implementation has changed significantly since first patch proposal. Please note this is rebased against master, so for commit purposes, it is recommended that patches from bug 90959 and bug 90958 gets landed first, then we rebase it against it before landing.
Comment 16 Build Bot 2012-08-08 18:15:36 PDT
Comment on attachment 157206 [details]
Patch

Attachment 157206 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13459432

New failing tests:
fast/css/text-decoration-color.html
Comment 17 Build Bot 2012-08-08 18:15:41 PDT
Created attachment 157357 [details]
Archive of layout-test-results from apple-mac-2

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: apple-mac-2  Port: <class 'webkitpy.common.config.ports.MacPort'>  Platform: Mac OS X 10.7.4
Comment 18 Bruno Abinader (history only) 2012-08-08 20:52:05 PDT
(In reply to comment #16)
> (From update of attachment 157206 [details])
> Attachment 157206 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/13459432
> 
> New failing tests:
> fast/css/text-decoration-color.html

Inside http://queues.webkit.org/results/13459432 we can find:

Unexpected flakiness: text failures (7)
  editing/spelling/grammar.html = TEXT PASS
  editing/spelling/markers.html = TEXT PASS
  fast/loader/javascript-url-in-object.html = TEXT PASS
  fullscreen/video-controls-timeline.html = TEXT PASS
  http/tests/security/sandboxed-iframe-origin-add.html = TEXT PASS
  inspector/timeline/timeline-network-received-data.html = TEXT PASS
  media/video-pause-immediately.html = TEXT PASS

Regressions: Unexpected text failures : (11)
  inspector/device-orientation-success.html = TEXT
  mathml/presentation/attributes.xhtml = TEXT
  mathml/presentation/fenced-mi.xhtml = TEXT
  mathml/presentation/fractions.xhtml = TEXT
  mathml/presentation/mo.xhtml = TEXT
  mathml/presentation/over.xhtml = TEXT
  mathml/presentation/roots.xhtml = TEXT
  mathml/presentation/row.xhtml = TEXT
  mathml/presentation/sub.xhtml = TEXT
  mathml/presentation/tokenElements.xhtml = TEXT
  mathml/presentation/underover.xhtml = TEXT

Regressions: Unexpected crashes : (1)
  fast/forms/search-delete-while-cancel-button-clicked.html = CRASH

None of these regressions are related with the code implementation,  likely to be a build bot platform issue (same happened with patch from bug 90958).
Comment 19 Ojan Vafai 2012-08-08 23:09:54 PDT
I wouldn't trust the link there. I think a lot of times the output that those links point to is incorrect. Adam, Eric, every time I've every looked at the data at one of those links, it never matches the list of tests that are shown in the bug comment. It seems maybe we're linking to the wrong run?
Comment 20 Adam Barth 2012-08-09 10:43:02 PDT
> It seems maybe we're linking to the wrong run?

Yes.  We always link to the wrong run.  It's on my list of things to fix, it just hasn't gotten to the top yet.
Comment 21 Bruno Abinader (history only) 2012-08-16 06:33:27 PDT
Created attachment 158799 [details]
Patch

Updated patch with additional layout tests and code now is guarded by feature flag. Please note this patch might conflict with bug 93829 and bug 94131, as they touch the same function.
Comment 22 Bruno Abinader (history only) 2012-08-17 08:44:12 PDT
Created attachment 159131 [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 23 WebKit Review Bot 2012-08-17 12:25:22 PDT
Comment on attachment 159131 [details]
Patch (EWS run only)

Attachment 159131 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13521487

New failing tests:
fast/forms/range/slider-delete-while-dragging-thumb.html
fast/selectors/017.html
fast/loader/loadInProgress.html
fast/loader/unload-form-post-about-blank.html
css3/selectors3/html/css3-modsel-61.html
fast/css/child-style-can-override-visited-style.html
css2.1/t051103-c21-hover-ln-00-e-i.html
fast/canvas/webgl/shader-precision-format.html
css2.1/t060403-c21-pseu-cls-00-e-i.html
http/tests/xmlhttprequest/zero-length-response.html
css2.1/t0511-c21-pseud-link-03-e.html
css2.1/t051103-c21-activ-ln-00-e-i.html
css2.1/t051103-c21-focus-ln-00-e-i.html
css3/selectors3/xml/css3-modsel-61.xml
css3/selectors3/xml/css3-modsel-17.xml
css2.1/t060403-c21-pseu-id-00-e-i.html
platform/chromium/virtual/gpu/fast/canvas/webgl/shader-precision-format.html
fast/forms/range/slider-mouse-events.html
fast/selectors/visited-descendant.html
css3/selectors3/html/css3-modsel-17.html
fast/events/reveal-link-when-focused.html
fast/frames/cached-frame-counter.html
fast/selectors/061.html
css3/selectors3/xhtml/css3-modsel-17.xml
css2.1/t0511-c21-pseud-link-02-e.html
fast/forms/range/slider-onchange-event.html
css3/selectors3/xhtml/css3-modsel-61.xml
Comment 24 WebKit Review Bot 2012-08-17 12:26:47 PDT
Created attachment 159178 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 25 Bruno Abinader (history only) 2012-08-22 14:43:01 PDT
Created attachment 160013 [details]
Patch

Fixed implementation issues with failing tests, updated layout tests.
Comment 26 Bruno Abinader (history only) 2012-08-22 15:06:35 PDT
Created attachment 160017 [details]
Patch (EWS run only)

Patch for EWS run only.
Comment 27 Build Bot 2012-08-22 15:46:49 PDT
Comment on attachment 160017 [details]
Patch (EWS run only)

Attachment 160017 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13558463
Comment 28 Build Bot 2012-08-22 16:02:30 PDT
Comment on attachment 160017 [details]
Patch (EWS run only)

Attachment 160017 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13562417
Comment 29 WebKit Review Bot 2012-08-22 16:34:43 PDT
Comment on attachment 160017 [details]
Patch (EWS run only)

Attachment 160017 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13570430

New failing tests:
fast/text/stroking-decorations.html
Comment 30 WebKit Review Bot 2012-08-22 16:34:51 PDT
Created attachment 160030 [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 31 WebKit Review Bot 2012-08-22 17:35:43 PDT
Comment on attachment 160017 [details]
Patch (EWS run only)

Attachment 160017 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13565496

New failing tests:
fast/text/stroking-decorations.html
Comment 32 WebKit Review Bot 2012-08-22 17:35:52 PDT
Created attachment 160045 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 33 Bruno Abinader (history only) 2012-08-30 10:38:05 PDT
Created attachment 161513 [details]
Patch

Fixed broken backwards behavior, updated layout tests.
Comment 34 Bruno Abinader (history only) 2012-08-30 10:42:23 PDT
Created attachment 161514 [details]
Patch (EWS run only)
Comment 35 Build Bot 2012-08-30 11:11:03 PDT
Comment on attachment 161514 [details]
Patch (EWS run only)

Attachment 161514 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13695753
Comment 36 Build Bot 2012-08-30 11:33:10 PDT
Comment on attachment 161514 [details]
Patch (EWS run only)

Attachment 161514 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13685840
Comment 37 WebKit Review Bot 2012-08-30 13:51:18 PDT
Comment on attachment 161514 [details]
Patch (EWS run only)

Attachment 161514 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13693853

New failing tests:
http/tests/cache/subresource-expiration-1.html
http/tests/cache/stopped-revalidation.html
Comment 38 Bruno Abinader (history only) 2012-08-31 07:54:52 PDT
Created attachment 161697 [details]
Patch (please ignore - should be in bug 92000).

Included required patches from bug 94094 and bug 91638.
Comment 39 Julien Chaffraix 2012-08-31 10:54:08 PDT
Comment on attachment 161513 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161513&action=review

All in all the change looks fine and should be good to go once the comments have been answered.

> Source/WebCore/ChangeLog:45
> +        (StyleRareNonInheritedData):

Don't forget the entries here.

> Source/WebCore/rendering/RenderObject.cpp:2695
> +    return style->visitedDependentColor(CSSPropertyWebkitTextDecorationColor);

If we don't have any text-decoration-color but a text-stroke-color, wouldn't this return the wrong value? Same question for a text-fill-color?

All in all, this fallback should be tested as it's non-trivial. Also I would favor a single place for this fallback if that's feasible.

> LayoutTests/fast/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-color-expected.txt:93
> +Empty value with different 'currentColor' initial value (red):

Glad that you test the initial 'currentColor' case. I would use 'green' instead of 'red' to indicate success.

> LayoutTests/fast/css3-text-decoration/repaint/repaint-text-decoration-color.html:33
> +        <!-- Red text with green underline on repaint -->

As a rule, red should be reserved for failures (and green for successes). It's better to use any other color in this test.

> LayoutTests/fast/css3-text-decoration/text-decoration-color.html:20
> +        <!-- Valid values for underline, overline and line-through text decoration lines -->

You can dump this description on the ref-test (not repeated)
Comment 40 Julien Chaffraix 2012-08-31 10:54:49 PDT
CC'ing Dave in case he has a comment about the visited side of this change.
Comment 41 Bruno Abinader (history only) 2012-09-03 06:35:54 PDT
(In reply to comment #39)
> (From update of attachment 161513 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161513&action=review
> > Source/WebCore/rendering/RenderObject.cpp:2695
> > +    return style->visitedDependentColor(CSSPropertyWebkitTextDecorationColor);
> 
> If we don't have any text-decoration-color but a text-stroke-color, wouldn't this return the wrong value? Same question for a text-fill-color?

The fallback options for text-decoration-color are now properly handled in RenderStyle::colorIncludingFallback9), which is IMO the best place to handle color fallbacks.

> 
> All in all, this fallback should be tested as it's non-trivial. Also I would favor a single place for this fallback if that's feasible.

This fallback is tested in a sense that previous text decoration layout tests (see fast/text/stroking-decorations.html) passes when the feature flag is set, thus no need for redundant tests.

I agree about a single place for this handling, that's why I believe the decorationColor() static function inside RenderObject.cpp should be removed in favor of a call to style->visitedDependentColor(CSSPropertyWebkitTextDecorationColor) inside RenderObject::getTextDecorationColors(), which properly handles the color fallbacks.

I am updating the ChangeLog as well as replacing red with green on layout tests, the updated patch will be uploaded soon.
Comment 42 Bruno Abinader (history only) 2012-09-04 05:47:30 PDT
Created attachment 162021 [details]
Patch

Updated ChangeLog, replaced 'red' color on layout tests.
Comment 43 Bruno Abinader (history only) 2012-09-04 05:49:54 PDT
Created attachment 162023 [details]
Patch (EWS run only)
Comment 44 Build Bot 2012-09-04 06:16:38 PDT
Comment on attachment 162023 [details]
Patch (EWS run only)

Attachment 162023 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13746454
Comment 45 Bruno Abinader (history only) 2012-09-06 06:28:29 PDT
Created attachment 162491 [details]
Patch

Rebased/updated colorIncludingFallback after changes from r127649 (bug 95863).
Comment 46 Bruno Abinader (history only) 2012-09-06 06:31:13 PDT
Created attachment 162492 [details]
Patch (EWS run only)
Comment 47 Build Bot 2012-09-06 08:01:44 PDT
Comment on attachment 162492 [details]
Patch (EWS run only)

Attachment 162492 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13764709
Comment 48 Build Bot 2012-09-06 08:54:56 PDT
Comment on attachment 162492 [details]
Patch (EWS run only)

Attachment 162492 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13776297
Comment 49 Julien Chaffraix 2012-09-06 10:33:47 PDT
Comment on attachment 162491 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162491&action=review

r- for the code duplication but the patch is fine apart from that.

> Source/WebCore/rendering/RenderObject.cpp:2693
> +    return style->visitedDependentColor(CSSPropertyWebkitTextDecorationColor);

This line made me think a long time about whether it was correct. It turns out that it is but it's not the proper way of adding what you want.

> Source/WebCore/rendering/style/RenderStyle.cpp:1397
> +    // Text decoration color has a four-way fallback (text decoration color, then text stroke color
> +    // if stroke width is non-zero or text fill color, which falls back to color if invalid.
> +    if (colorProperty == CSSPropertyWebkitTextDecorationColor) {
> +        if (result.alpha() && result.isValid())
> +            return result;
> +        if (textStrokeWidth() > 0) {
> +            // Prefer stroke color if possible but not if it's fully transparent.
> +            result = colorIncludingFallback(CSSPropertyWebkitTextStrokeColor, visitedLink);
> +            if (result.alpha())
> +                return result;
> +        }
> +        return colorIncludingFallback(CSSPropertyWebkitTextFillColor, visitedLink);
> +    }

This should go into decorationColor. colorIncludingFallback shouldn't include paint fallbacks as it is used outside painting (f.e. collapsed borders resolution). Look at what we do for text-stroke-color or text-fill-color.

On top of that, it would avoid duplicating some of the existing fallback logic.
Comment 50 Bruno Abinader (history only) 2012-09-10 10:03:57 PDT
Created attachment 163156 [details]
Patch

Updated text decoration color fallback logic to be back inside decorationColor().
Comment 51 Bruno Abinader (history only) 2012-09-10 10:11:09 PDT
Created attachment 163158 [details]
Patch (EWS run only)
Comment 52 Build Bot 2012-09-10 10:48:34 PDT
Comment on attachment 163158 [details]
Patch (EWS run only)

Attachment 163158 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13800891
Comment 53 Build Bot 2012-09-10 11:28:25 PDT
Comment on attachment 163158 [details]
Patch (EWS run only)

Attachment 163158 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13795948
Comment 54 Julien Chaffraix 2012-09-19 17:27:44 PDT
Comment on attachment 163156 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163156&action=review

> Source/WebCore/ChangeLog:43
> +        Text decoration color does not fallback in colorIncludingFallback and
> +        might return an invalid color in visitedDependentColor (checked in
> +        decorationColor).

Explaining the 'why' here would help. I can't read minds, only what you have written, and they don't give my any indication as to why this is a valid assumption to make.

AFAICT there is no reason why 'text-decoration' shouldn't fallback to using 'color' like other text colors (see http://www.w3.org/TR/2011/REC-css3-color-20110607/#foreground). Consistency between the different text color properties is important (unless obvious the spec says otherwise).

> Source/WebCore/rendering/RenderObject.cpp:2698
> +#if ENABLE(CSS3_TEXT_DECORATION)
> +    // Check for text decoration color first.
> +    Color result = style->visitedDependentColor(CSSPropertyWebkitTextDecorationColor);
> +    if (result.isValid())
> +        return result;
> +#else
>      Color result;
> +#endif // CSS3_TEXT_DECORATION

How about:

Color result;
#if ENABLE(CSS3_TEXT_DECORATION)
result = ...;
[snip]
#endif
(less lines and less #else)

Also what happened to the alpha() check?

The comment would be better if you explained 'why' you need to check text decoration first. As it is written, it adds little value.
Comment 55 Bruno Abinader (history only) 2012-10-02 11:02:06 PDT
(In reply to comment #54)
> (From update of attachment 163156 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163156&action=review
> 
> > Source/WebCore/ChangeLog:43
> > +        Text decoration color does not fallback in colorIncludingFallback and
> > +        might return an invalid color in visitedDependentColor (checked in
> > +        decorationColor).
> 
> Explaining the 'why' here would help. I can't read minds, only what you have written, and they don't give my any indication as to why this is a valid assumption to make.
> 
> AFAICT there is no reason why 'text-decoration' shouldn't fallback to using 'color' like other text colors (see http://www.w3.org/TR/2011/REC-css3-color-20110607/#foreground). Consistency between the different text color properties is important (unless obvious the spec says otherwise).

The reason I'm not fallbacking 'text-decoration-color' property to 'color' if not set is because we need a way to know when the 'text-decoration' property is set or not at getTextDecoration() static function. Fallbacking to 'color' causes regression on layout tests like fast/text/stroking-decorations.html beceause it falls back to 'color' before testing for 'text-stroke-color' neither 'text-fill-color' properties. This has been tested already on previous patches. This is same reason why we check also on visitedDependentColor, because otherwise it would never return an invalid Color.

Until there is no other proper way to check if a property is set, this is a hack that might be needed.

> 
> > Source/WebCore/rendering/RenderObject.cpp:2698
> > +#if ENABLE(CSS3_TEXT_DECORATION)
> > +    // Check for text decoration color first.
> > +    Color result = style->visitedDependentColor(CSSPropertyWebkitTextDecorationColor);
> > +    if (result.isValid())
> > +        return result;
> > +#else
> >      Color result;
> > +#endif // CSS3_TEXT_DECORATION
> 
> How about:
> 
> Color result;
> #if ENABLE(CSS3_TEXT_DECORATION)
> result = ...;
> [snip]
> #endif
> (less lines and less #else)
> 
> Also what happened to the alpha() check?
> 
> The comment would be better if you explained 'why' you need to check text decoration first. As it is written, it adds little value.

As I said on IRC, the alpha() check could be suppressed as this might not be the proper way to tell us if the property was set or not. I'm going to provide a better explanation about this code change on the next patch cycle.
Comment 56 Bruno Abinader (history only) 2012-10-23 07:34:03 PDT
Created attachment 170162 [details]
Patch

Updates on namespace (now CSS3_TEXT), layout test directories and better explanation of property fallback mechanism (also in comment 55).
Comment 57 Bruno Abinader (history only) 2012-10-25 09:18:03 PDT
Created attachment 170669 [details]
Patch

Fixed a typo in repaint layout test (s/repaintTest/runRepaintTest/).
Comment 58 Bruno Abinader (history only) 2012-10-26 11:34:27 PDT
Created attachment 170970 [details]
Patch (EWS only)

More info on failing layout tests in bug 100546 "[css3-text] Provide pixel results for text decoration style layout tests."
Comment 59 Build Bot 2012-10-26 12:01:55 PDT
Comment on attachment 170970 [details]
Patch (EWS only)

Attachment 170970 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14608161
Comment 60 Build Bot 2012-10-26 12:04:40 PDT
Comment on attachment 170970 [details]
Patch (EWS only)

Attachment 170970 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14609152
Comment 61 kov's GTK+ EWS bot 2012-10-26 12:40:32 PDT
Comment on attachment 170970 [details]
Patch (EWS only)

Attachment 170970 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14603433
Comment 62 WebKit Review Bot 2012-10-26 15:13:08 PDT
Comment on attachment 170970 [details]
Patch (EWS only)

Attachment 170970 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14617132

New failing tests:
fast/css3-text/css3-text-decoration/text-decoration-style.html
fast/css3-text/css3-text-decoration/repaint/repaint-text-decoration-style.html
fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-color.html
Comment 63 Bruno Abinader (history only) 2012-10-29 06:44:02 PDT
Created attachment 171232 [details]
Patch

Fixed layout test internal paths after directory changes.
Comment 64 Bruno Abinader (history only) 2012-12-12 07:32:28 PST
Ping?
Comment 65 Bruno Abinader (history only) 2012-12-12 08:53:01 PST
Created attachment 179057 [details]
Patch

Rebased after text-align-last changes.
Comment 66 Rajath Kamath 2013-03-13 05:19:25 PDT
(In reply to comment #65)
> Created an attachment (id=179057) [details]
> Patch
> 
> Rebased after text-align-last changes.
Comment 67 Rajath Kamath 2013-03-13 05:23:13 PDT
(In reply to comment #65)
> Created an attachment (id=179057) [details]
> Patch
> 
> Rebased after text-align-last changes.

Could you please update the current review status of this patch?
Comment 68 Bruno Abinader (history only) 2013-03-13 05:26:21 PDT
(In reply to comment #67)
> (In reply to comment #65)
> > Created an attachment (id=179057) [details] [details]
> > Patch
> > 
> > Rebased after text-align-last changes.
> 
> Could you please update the current review status of this patch?

It's currently pending review. @Julien, can you please have another look?
Comment 69 Julien Chaffraix 2013-03-13 15:40:44 PDT
Comment on attachment 179057 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179057&action=review

r=me, the change will very likely not apply on ToT and should be updated.

> LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-color-expected.txt:12
> +Parent should cointain 'green':

Typo: cointain

> LayoutTests/fast/css3-text/css3-text-decoration/repaint/repaint-text-decoration-color.html:7
> +        <!-- Bugzilla link: http://webkit.org/b/91638 -->
> +        <title>CSS Test: CSS3 text-decoration-color repaint</title>
> +        <link rel="help" href="http://http://dev.w3.org/csswg/css3-text/#text-decoration-color"/>
> +        <meta name="flags" content="ahem"/>

My take on those meta information is that if they are important enough, they should be displayed. If not, they should just be removed as they will probably not be read.

Here you would need:
* A passing condition (probably dumped, though I would be fine as a comment (it's an exception from the previous rule as it is important for debugging the test to have the intent somewhere)).
* What you are testing (a 'description').

This applies to the ref-test too.

> LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-color.html:14
> +            #blue-underline { text-decoration: underline; -webkit-text-decoration-color: blue; }
> +            #gray-overline { text-decoration: overline; -webkit-text-decoration-color: gray; }
> +            #green-line-through { text-decoration: line-through; -webkit-text-decoration-color: green; }
> +            #transparent-fill { -webkit-text-fill-color: transparent; -webkit-text-stroke-width: 1px; -webkit-text-stroke-color: black; }

I prefer tests to follow WebKit's style and have one statement per line.
Comment 70 Bruno Abinader (history only) 2013-03-13 20:23:26 PDT
Created attachment 193053 [details]
Patch for landing
Comment 71 WebKit Review Bot 2013-03-13 21:08:40 PDT
Comment on attachment 193053 [details]
Patch for landing

Clearing flags on attachment: 193053

Committed r145785: <http://trac.webkit.org/changeset/145785>
Comment 72 Bruno Abinader (history only) 2013-03-13 21:10:10 PDT
All reviewed patches have been landed. Closing bug.