Summary: | Fix interpolation of the caret-color CSS property | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||||||||||||
Component: | Animations | Assignee: | Antoine Quint <graouts> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | changseok, dino, esprehn+autocc, ews-watchlist, glenn, graouts, gyuyoung.kim, joepeck, koivisto, kondapallykalyan, macpherson, menard, pdr, simon.fraser, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Antoine Quint
2021-03-15 06:00:21 PDT
Created attachment 423172 [details]
Patch
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 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 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 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'? Created attachment 424260 [details]
Patch
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'? Created attachment 424265 [details]
Patch
Created attachment 424274 [details]
Patch
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. Created attachment 424326 [details]
Patch
Created attachment 424328 [details]
Patch
Created attachment 424333 [details]
Patch
Committed r275092 (235800@main): <https://commits.webkit.org/235800@main> |