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
Created attachment 155702 [details] Patch Proposed patch. Please note that patches from bug 92000, bug 90959 and bug 90958 (respectively) should land before this.
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.
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.
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 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
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 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
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
Created attachment 157097 [details] Patch Fixed bug in RenderObject::getTextDecorationColors, added chromium-linux layout test expectations.
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
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 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
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
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 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 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
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
(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).
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?
> 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.
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.
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 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
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
Created attachment 160013 [details] Patch Fixed implementation issues with failing tests, updated layout tests.
Created attachment 160017 [details] Patch (EWS run only) Patch for EWS run only.
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 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 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
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 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
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
Created attachment 161513 [details] Patch Fixed broken backwards behavior, updated layout tests.
Created attachment 161514 [details] Patch (EWS run only)
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 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 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
Created attachment 161697 [details] Patch (please ignore - should be in bug 92000). Included required patches from bug 94094 and bug 91638.
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)
CC'ing Dave in case he has a comment about the visited side of this change.
(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.
Created attachment 162021 [details] Patch Updated ChangeLog, replaced 'red' color on layout tests.
Created attachment 162023 [details] Patch (EWS run only)
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
Created attachment 162491 [details] Patch Rebased/updated colorIncludingFallback after changes from r127649 (bug 95863).
Created attachment 162492 [details] Patch (EWS run only)
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 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 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.
Created attachment 163156 [details] Patch Updated text decoration color fallback logic to be back inside decorationColor().
Created attachment 163158 [details] Patch (EWS run only)
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 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 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.
(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.
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).
Created attachment 170669 [details] Patch Fixed a typo in repaint layout test (s/repaintTest/runRepaintTest/).
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 on attachment 170970 [details] Patch (EWS only) Attachment 170970 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14608161
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 on attachment 170970 [details] Patch (EWS only) Attachment 170970 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14603433
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
Created attachment 171232 [details] Patch Fixed layout test internal paths after directory changes.
Ping?
Created attachment 179057 [details] Patch Rebased after text-align-last changes.
(In reply to comment #65) > Created an attachment (id=179057) [details] > Patch > > Rebased after text-align-last changes.
(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?
(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 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.
Created attachment 193053 [details] Patch for landing
Comment on attachment 193053 [details] Patch for landing Clearing flags on attachment: 193053 Committed r145785: <http://trac.webkit.org/changeset/145785>
All reviewed patches have been landed. Closing bug.