Bug 193171 - Implement the css-color-4 behavior for inheritance of currentColor
Summary: Implement the css-color-4 behavior for inheritance of currentColor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joonghun Park
URL:
Keywords: BrowserCompat, InRadar
: 158782 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-01-05 11:50 PST by Simon Fraser (smfr)
Modified: 2020-04-04 22:14 PDT (History)
23 users (show)

See Also:


Attachments
Testcase (537 bytes, text/html)
2019-01-05 11:56 PST, Simon Fraser (smfr)
no flags Details
wip (11.11 KB, patch)
2020-04-03 05:54 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
wip (11.09 KB, patch)
2020-04-03 06:04 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
wip (30.84 KB, patch)
2020-04-03 09:58 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
wip (60.83 KB, patch)
2020-04-03 10:27 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (71.13 KB, patch)
2020-04-04 00:52 PDT, Antti Koivisto
simon.fraser: review+
Details | Formatted Diff | Diff
patch (72.67 KB, patch)
2020-04-04 09:49 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (72.68 KB, patch)
2020-04-04 10:04 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (72.68 KB, patch)
2020-04-04 10:05 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2019-01-05 11:50:16 PST
According to https://drafts.csswg.org/css-color/#resolving-color-values, currentColor should inherit as the keyword, not the resolved color.
Comment 1 Simon Fraser (smfr) 2019-01-05 11:56:37 PST
Created attachment 358440 [details]
Testcase
Comment 2 Simon Fraser (smfr) 2019-01-05 12:07:23 PST
It looks like StyleColor was added for this, but it's not instantiated anywhere.
Comment 3 Radar WebKit Bug Importer 2019-01-15 10:18:28 PST
<rdar://problem/47287516>
Comment 4 Miriam Suzanne 2019-12-20 16:03:39 PST
This is a pretty frustrating bug in a number of common `currentColor` use-cases, not limited to:
- Using `fill: currentColor` to get SVG fills in-sync with text colors
- Changing link/button colors in different states, `currentColor` is a much more useful "reset" than something like `initial`
- This is especially true with changing `text-decoration-color`
Comment 5 Antti Koivisto 2020-04-03 05:54:51 PDT
Created attachment 395368 [details]
wip
Comment 6 Antti Koivisto 2020-04-03 06:04:50 PDT
Created attachment 395369 [details]
wip
Comment 7 Antti Koivisto 2020-04-03 09:58:24 PDT
Created attachment 395385 [details]
wip
Comment 8 Antti Koivisto 2020-04-03 10:27:50 PDT
Created attachment 395391 [details]
wip
Comment 9 Antti Koivisto 2020-04-04 00:21:45 PDT
*** Bug 158782 has been marked as a duplicate of this bug. ***
Comment 10 Antti Koivisto 2020-04-04 00:52:01 PDT
Created attachment 395441 [details]
patch
Comment 11 Emilio Cobos Álvarez (:emilio) 2020-04-04 06:38:36 PDT
Comment on attachment 395441 [details]
patch

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

> Source/WebCore/style/StyleBuilderState.cpp:-317
> -        return m_style.color();

Setting setHasExplicitlyInheritedProperties shouldn't be needed here anymore, right? currentColor() doesn't depend on m_style.
Comment 12 Emilio Cobos Álvarez (:emilio) 2020-04-04 06:39:23 PDT
Not that it matters that much though, better safe than sorry I guess...
Comment 13 Antti Koivisto 2020-04-04 07:09:37 PDT
Need to do some more checking to ensure there is nothing left that needs it. I'll try to remove it in a separate patch.
Comment 14 Simon Fraser (smfr) 2020-04-04 08:44:14 PDT
Comment on attachment 395441 [details]
patch

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

> LayoutTests/fast/borders/border-color-inherit-expected.html:1
> +<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN">

Maybe just <!DOCTYPE html> (and in the test too).

> Source/WebCore/rendering/style/RenderStyle.h:1735
> +    static Color currentColor() { return { }; }

Maybe change initialTextEmphasisColor() to return curentColor().

> Source/WebCore/style/StyleBuilderState.cpp:324
> +    // FIXME: 'currentcolor' should be resolved at use time to make it inherit correctly.

You should file a bug on the FIXME and reference it here. That seems fairly important to fix.

> Source/WebCore/svg/SVGStopElement.cpp:97
> +    if (!stopColor.isValid())
> +        stopColor = style.color();

A little helper function like RenderStyle::resolveCurrentColor or something would make this much more understandable.
Comment 15 Antti Koivisto 2020-04-04 09:49:36 PDT
Created attachment 395451 [details]
patch
Comment 16 Antti Koivisto 2020-04-04 10:04:11 PDT
Created attachment 395453 [details]
patch
Comment 17 Antti Koivisto 2020-04-04 10:05:19 PDT
Created attachment 395454 [details]
patch
Comment 18 EWS 2020-04-04 11:16:30 PDT
Committed r259532: <https://trac.webkit.org/changeset/259532>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395454 [details].