Bug 223181 – Fix interpolation of the caret-color CSS property

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

Fix interpolation of the caret-color CSS property

[Antoine Quint]

Created attachment 423172 [details]
Patch

[Antti Koivisto]

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!

[Antti Koivisto]

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.

[Antti Koivisto]

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.

[Antti Koivisto]

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

[Radar WebKit Bug Importer]

<rdar://problem/75687413>

[Antoine Quint]

Created attachment 424260 [details]
Patch

[Antti Koivisto]

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

[Antoine Quint]

Created attachment 424265 [details]
Patch

[Antoine Quint]

Created attachment 424274 [details]
Patch

[Antti Koivisto]

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.

[Antoine Quint]

Created attachment 424326 [details]
Patch

[Antoine Quint]

Created attachment 424328 [details]
Patch

[Antoine Quint]

Created attachment 424333 [details]
Patch

[Antoine Quint]

Committed r275092 (235800@main): <https://commits.webkit.org/235800@main>