Bug 223181 - Fix interpolation of the caret-color CSS property
Summary: Fix interpolation of the caret-color CSS property
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-15 06:00 PDT by Antoine Quint
Modified: 2021-03-26 04:17 PDT (History)
15 users (show)

See Also:


Attachments
Patch (41.65 KB, patch)
2021-03-15 06:39 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (42.61 KB, patch)
2021-03-25 10:55 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (42.80 KB, patch)
2021-03-25 11:22 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (38.57 KB, patch)
2021-03-25 13:19 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (38.78 KB, patch)
2021-03-26 01:30 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (38.89 KB, patch)
2021-03-26 01:40 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (39.40 KB, patch)
2021-03-26 02:11 PDT, Antoine Quint
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2021-03-15 06:00:21 PDT
Fix interpolation of the caret-color CSS property
Comment 1 Antoine Quint 2021-03-15 06:39:26 PDT
Created attachment 423172 [details]
Patch
Comment 2 Antti Koivisto 2021-03-15 06:44:13 PDT
Comment on attachment 423172 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2788
> +            if (!m_allowVisitedStyle && style.hasAutoCaretColor())
> +                return cssValuePool.createIdentifierValue(CSSValueAuto);
> +            if (m_allowVisitedStyle && style.hasVisitedLinkAutoCaretColor())
> +                return cssValuePool.createIdentifierValue(CSSValueAuto);

Please don't expand visited link support!
Comment 3 Antti Koivisto 2021-03-15 06:59:58 PDT
Comment on attachment 423172 [details]
Patch

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

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2788
>> +                return cssValuePool.createIdentifierValue(CSSValueAuto);
> 
> Please don't expand visited link support!

Sorry, I misread this. This just keeps the existing support.

> Source/WebCore/style/StyleBuilderCustom.h:830
> +inline void BuilderCustom::applyInitialCaretColor(BuilderState& builderState)
> +{
> +    if (builderState.applyPropertyToRegularStyle())
> +        builderState.style().setHasAutoCaretColor();
> +    if (builderState.applyPropertyToVisitedLinkStyle())
> +        builderState.style().setHasVisitedLinkAutoCaretColor();
> +}
> +
> +inline void BuilderCustom::applyInheritCaretColor(BuilderState& builderState)
> +{
> +    Color color = builderState.parentStyle().caretColor();
> +    if (builderState.parentStyle().hasAutoCaretColor())
> +        builderState.style().setHasAutoCaretColor();
> +    else if (builderState.applyPropertyToRegularStyle())
> +        builderState.style().setCaretColor(color);
> +    if (builderState.parentStyle().hasVisitedLinkAutoCaretColor())
> +        builderState.style().setHasVisitedLinkAutoCaretColor();
> +    else if (builderState.applyPropertyToVisitedLinkStyle())
> +        builderState.style().setVisitedLinkCaretColor(color);
> +}
> +
> +inline void BuilderCustom::applyValueCaretColor(BuilderState& builderState, CSSValue& value)
> +{
> +    auto& primitiveValue = downcast<CSSPrimitiveValue>(value);
> +    if (primitiveValue.valueID() == CSSValueAuto)
> +        builderState.style().setHasAutoCaretColor();
> +    else if (builderState.applyPropertyToRegularStyle())
> +        builderState.style().setCaretColor(builderState.colorFromPrimitiveValue(primitiveValue, /* forVisitedLink */ false));
> +    if (primitiveValue.valueID() == CSSValueAuto)
> +        builderState.style().setHasVisitedLinkAutoCaretColor();
> +    else if (builderState.applyPropertyToVisitedLinkStyle())
> +        builderState.style().setVisitedLinkCaretColor(builderState.colorFromPrimitiveValue(primitiveValue, /* forVisitedLink */ true));
> +}

We should really add a separate type ("StyleColor") that can support these sort of things without having to use custom wrappers or in-band signalling like currentColor.
Comment 4 Antti Koivisto 2021-03-15 07:47:59 PDT
Comment on attachment 423172 [details]
Patch

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

>>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2788
>>> +                return cssValuePool.createIdentifierValue(CSSValueAuto);
>> 
>> Please don't expand visited link support!
> 
> Sorry, I misread this. This just keeps the existing support.

The logic here doesn't look right though. m_allowVisitedStyle doesn't tell if the visited style should be returned, just that it can be returned.
Comment 5 Antti Koivisto 2021-03-15 07:54:57 PDT
Comment on attachment 423172 [details]
Patch

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

> Source/WebCore/css/CSSProperties.json:191
>                  "initial": "currentColor",

but the ChangeLog claims the initial value is 'auto'?
Comment 6 Radar WebKit Bug Importer 2021-03-22 06:01:12 PDT
<rdar://problem/75687413>
Comment 7 Antoine Quint 2021-03-25 10:55:33 PDT
Created attachment 424260 [details]
Patch
Comment 8 Antti Koivisto 2021-03-25 11:10:58 PDT
Comment on attachment 424260 [details]
Patch

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

> Source/WebCore/rendering/style/StyleRareInheritedData.cpp:139
> +    , hasAutoCaretColor(false)
> +    , hasVisitedLinkAutoCaretColor(false)

Isn't the initial value for these 'true'?
Comment 9 Antoine Quint 2021-03-25 11:22:37 PDT
Created attachment 424265 [details]
Patch
Comment 10 Antoine Quint 2021-03-25 13:19:28 PDT
Created attachment 424274 [details]
Patch
Comment 11 Antti Koivisto 2021-03-26 00:13:20 PDT
Comment on attachment 424274 [details]
Patch

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

> Source/WebCore/rendering/style/RenderStyle.h:1115
> +    void setHasAutoCaretColor(bool flag) { SET_VAR(m_rareInheritedData, hasAutoCaretColor, flag); SET_VAR(m_rareInheritedData, caretColor, Color()); }

You can use currentColor() here instead of Color(). It is the same thing except more informative.
Comment 12 Antoine Quint 2021-03-26 01:30:54 PDT
Created attachment 424326 [details]
Patch
Comment 13 Antoine Quint 2021-03-26 01:40:47 PDT
Created attachment 424328 [details]
Patch
Comment 14 Antoine Quint 2021-03-26 02:11:17 PDT
Created attachment 424333 [details]
Patch
Comment 15 Antoine Quint 2021-03-26 04:17:17 PDT
Committed r275092 (235800@main): <https://commits.webkit.org/235800@main>