Bug 91638 – [css3-text] Add support for text-decoration-color

• Component: CSS
• Status: RESOLVED FIXED

[Bruno Abinader (history only)]

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

[Bruno Abinader (history only)]

Created attachment 155702 [details]
Patch

Proposed patch. Please note that patches from bug 92000, bug 90959 and bug 90958 (respectively) should land before this.

[Bruno Abinader (history only)]

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.

[Bruno Abinader (history only)]

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.

[Bruno Abinader (history only)]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Bruno Abinader (history only)]

Created attachment 157097 [details]
Patch

Fixed bug in RenderObject::getTextDecorationColors, added chromium-linux layout test expectations.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Bruno Abinader (history only)]

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.

[Bruno Abinader (history only)]

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.

[Build Bot]

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

[Build Bot]

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

[Bruno Abinader (history only)]

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

[Ojan Vafai]

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?

[Adam Barth]

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

[Bruno Abinader (history only)]

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.

[Bruno Abinader (history only)]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Bruno Abinader (history only)]

Created attachment 160013 [details]
Patch

Fixed implementation issues with failing tests, updated layout tests.

[Bruno Abinader (history only)]

Created attachment 160017 [details]
Patch (EWS run only)

Patch for EWS run only.

[Build Bot]

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

[Build Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Bruno Abinader (history only)]

Created attachment 161513 [details]
Patch

Fixed broken backwards behavior, updated layout tests.

[Bruno Abinader (history only)]

Created attachment 161514 [details]
Patch (EWS run only)

[Build Bot]

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

[Build Bot]

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

[WebKit Review Bot]

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

[Bruno Abinader (history only)]

Created attachment 161697 [details]
Patch (please ignore - should be in bug 92000).

Included required patches from bug 94094 and bug 91638.

[Julien Chaffraix]

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)

[Julien Chaffraix]

CC'ing Dave in case he has a comment about the visited side of this change.

[Bruno Abinader (history only)]

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

[Bruno Abinader (history only)]

Created attachment 162021 [details]
Patch

Updated ChangeLog, replaced 'red' color on layout tests.

[Bruno Abinader (history only)]

Created attachment 162023 [details]
Patch (EWS run only)

[Build Bot]

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

[Bruno Abinader (history only)]

Created attachment 162491 [details]
Patch

Rebased/updated colorIncludingFallback after changes from r127649 (bug 95863).

[Bruno Abinader (history only)]

Created attachment 162492 [details]
Patch (EWS run only)

[Build Bot]

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

[Build Bot]

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

[Julien Chaffraix]

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.

[Bruno Abinader (history only)]

Created attachment 163156 [details]
Patch

Updated text decoration color fallback logic to be back inside decorationColor().

[Bruno Abinader (history only)]

Created attachment 163158 [details]
Patch (EWS run only)

[Build Bot]

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

[Build Bot]

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

[Julien Chaffraix]

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.

[Bruno Abinader (history only)]

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

[Bruno Abinader (history only)]

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

[Bruno Abinader (history only)]

Created attachment 170669 [details]
Patch

Fixed a typo in repaint layout test (s/repaintTest/runRepaintTest/).

[Bruno Abinader (history only)]

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

[Build Bot]

Comment on attachment 170970 [details]
Patch (EWS only)

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

[Build Bot]

Comment on attachment 170970 [details]
Patch (EWS only)

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

[kov's GTK+ EWS bot]

Comment on attachment 170970 [details]
Patch (EWS only)

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

[WebKit Review Bot]

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

[Bruno Abinader (history only)]

Created attachment 171232 [details]
Patch

Fixed layout test internal paths after directory changes.

[Bruno Abinader (history only)]

Ping?

[Bruno Abinader (history only)]

Created attachment 179057 [details]
Patch

Rebased after text-align-last changes.

[Rajath Kamath]

(In reply to comment #65)
> Created an attachment (id=179057) [details]
> Patch
> 
> Rebased after text-align-last changes.

[Rajath Kamath]

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

[Bruno Abinader (history only)]

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

[Julien Chaffraix]

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.

[Bruno Abinader (history only)]

Created attachment 193053 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 193053 [details]
Patch for landing

Clearing flags on attachment: 193053

Committed r145785: <http://trac.webkit.org/changeset/145785>

[Bruno Abinader (history only)]

All reviewed patches have been landed. Closing bug.