REGRESSION: source code viewer on Github renders two overlapping carets

https://bugs.webkit.org/show_bug.cgi?id=259166 rdar://112056259 Reviewed by Aditya Keerthi. The color of the caret should respect both the `caret-color` and the `color` properties. Because every element implicitly has a `color`, the color of the caret should only reflect this property if set by the author. * LayoutTests/fast/css/caret-color-with-color-property-expected.html: Added. * LayoutTests/fast/css/caret-color-with-color-property.html: Added. * LayoutTests/fast/css/caret-color-with-inherited-color-property-expected.html: Added. * LayoutTests/fast/css/caret-color-with-inherited-color-property.html: Added. * Source/WebCore/css/CSSProperties.json: * Source/WebCore/editing/FrameSelection.cpp: (WebCore::CaretBase::computeCaretColor): * Source/WebCore/rendering/style/RenderStyle.h: * Source/WebCore/rendering/style/RenderStyleInlines.h: (WebCore::RenderStyle::hasExplicitlySetColor const): (WebCore::RenderStyle::InheritedFlags::operator== const): * Source/WebCore/rendering/style/RenderStyleSetters.h: (WebCore::RenderStyle::setHasExplicitlySetColor): * Source/WebCore/style/StyleBuilderCustom.h: (WebCore::Style::BuilderCustom::applyValueColor): (WebCore::Style::BuilderCustom::applyInitialColor): * Source/WebCore/style/StyleBuilderState.h: (WebCore::Style::BuilderState::isAuthorOrigin const): Canonical link: https://commits.webkit.org/266070@main
WebKit · Jul 14, 2023 · 4cc63df · 4cc63df
1 parent e89c9b4
commit 4cc63df
Show file tree

Hide file tree

Showing 18 changed files with 177 additions and 35 deletions.
diff --git a/LayoutTests/fast/css/caret-color-auto-expected.html b/LayoutTests/fast/css/caret-color-auto-expected.html
@@ -23,7 +23,7 @@
 </style>
 </head>
 <body>
-<p>Tests that "caret-color: auto" behaves identical to omitting property caret-color.</p>
+<p>Tests that "caret-color: auto" causes the caret to take the "color" property of itself, if applicable.</p>
 <div id="test-container">
     <div id="test" contenteditable="true">
         <span>&nbsp;<!-- Needed for the caret to render in Firefox. --></span>

diff --git a/LayoutTests/fast/css/caret-color-auto.html b/LayoutTests/fast/css/caret-color-auto.html
@@ -24,7 +24,7 @@
 </style>
 </head>
 <body>
-<p>Tests that "caret-color: auto" behaves identical to omitting property caret-color.</p>
+<p>Tests that "caret-color: auto" causes the caret to take the "color" property of itself, if applicable.</p>
 <div id="test-container">
     <div id="test" contenteditable="true">
         <span>&nbsp;<!-- Needed for the caret to render in Firefox. --></span>

diff --git a/LayoutTests/fast/css/caret-color-with-inherited-color-property-expected.html b/LayoutTests/fast/css/caret-color-with-inherited-color-property-expected.html
@@ -0,0 +1,26 @@
+<!DOCTYPE html>
+<html>
+    <head>
+        <style>
+        #test-container {
+            height: 50px;
+            width: 50px;
+        }
+
+        #mock-caret {
+            width: 50px;
+            height: 100px;
+            position: absolute;
+            top: 100px;
+            left: 8px;
+            background-color: green;
+        }
+        </style>
+    </head>
+    <body>
+        <p>The caret should show as a green flashing square below.</p>
+        <div id="test-container">
+            <div id="mock-caret"></div>
+        </div>
+    </body>
+</html>
diff --git a/LayoutTests/fast/css/caret-color-with-inherited-color-property.html b/LayoutTests/fast/css/caret-color-with-inherited-color-property.html
@@ -0,0 +1,34 @@
+<!DOCTYPE html>
+<html>
+    <head>
+        <style>
+        #test-container {
+            height: 150px;
+            width: 100px;
+            overflow: hidden;
+            color: green;
+        }
+
+        #test {
+            transform-origin: left top;
+            transform: scale(50, 50);
+            clip-path: inset(1px 99px 0px 0px);
+            font-size: 10px; /* Needed for the caret to render in Firefox. */
+            color: inherit;
+        }
+
+        </style>
+    </head>
+    <body>
+        <p>The caret should show as a green flashing square below.</p>
+        <div id="test-container">
+            <div id="test" contenteditable="true">
+                <span>&nbsp;<!-- Needed for the caret to render in Firefox. --></span>
+            </div>
+        </div>
+        <script>
+            document.getElementById("test").focus();
+            window.getSelection().modify("move", "left", "character"); // Place the caret at the start of the <span>.
+        </script>
+    </body>
+</html>
diff --git a/LayoutTests/fast/css/caret-color-with-initial-color-property-expected.html b/LayoutTests/fast/css/caret-color-with-initial-color-property-expected.html
@@ -0,0 +1,26 @@
+<!DOCTYPE html>
+<html>
+    <head>
+        <style>
+        #test-container {
+            height: 50px;
+            width: 50px;
+        }
+
+        #mock-caret {
+            width: 50px;
+            height: 100px;
+            position: absolute;
+            top: 100px;
+            left: 8px;
+            background-color: black;
+        }
+        </style>
+    </head>
+    <body>
+        <p>The caret should show as a black flashing square below.</p>
+        <div id="test-container">
+            <div id="mock-caret"></div>
+        </div>
+    </body>
+</html>
diff --git a/LayoutTests/fast/css/caret-color-with-initial-color-property.html b/LayoutTests/fast/css/caret-color-with-initial-color-property.html
@@ -0,0 +1,33 @@
+<!DOCTYPE html>
+<html>
+    <head>
+        <style>
+        #test-container {
+            height: 150px;
+            width: 100px;
+            overflow: hidden;
+        }
+
+        #test {
+            transform-origin: left top;
+            transform: scale(50, 50);
+            clip-path: inset(1px 99px 0px 0px);
+            font-size: 10px; /* Needed for the caret to render in Firefox. */
+            color: initial;
+        }
+
+        </style>
+    </head>
+    <body>
+        <p>The caret should show as a black flashing square below.</p>
+        <div id="test-container">
+            <div id="test" contenteditable="true">
+                <span>&nbsp;<!-- Needed for the caret to render in Firefox. --></span>
+            </div>
+        </div>
+        <script>
+            document.getElementById("test").focus();
+            window.getSelection().modify("move", "left", "character"); // Place the caret at the start of the <span>.
+        </script>
+    </body>
+</html>
diff --git a/LayoutTests/platform/ios/TestExpectations b/LayoutTests/platform/ios/TestExpectations
@@ -1170,6 +1170,8 @@ fast/css/caret-color-fallback-to-color.html [ Skip ]
 fast/css/caret-color-inherit.html [ Skip ]
 fast/css/caret-color-span-inside-editable-parent.html [ Skip ]
 fast/css/caret-color.html [ Skip ]
+fast/css/caret-color-with-initial-color-property.html [ Skip ]
+fast/css/caret-color-with-inherited-color-property.html [ Skip ]
 fast/history/visited-link-caret-color.html [ Skip ]
 css3/color-filters/color-filter-caret-color.html [ Skip ]
 

diff --git a/Source/WebCore/css/CSSProperties.json b/Source/WebCore/css/CSSProperties.json
@@ -369,7 +369,7 @@
             "codegen-properties": {
                 "visited-link-color-support": true,
                 "color-property": true,
-                "custom": "Value",
+                "custom": "All",
                 "high-priority": true,
                 "fast-path-inherited": true,
                 "parser-grammar": "<color accept-quirky-colors-in-quirks-mode>"

diff --git a/Source/WebCore/editing/FrameSelection.cpp b/Source/WebCore/editing/FrameSelection.cpp
@@ -1895,44 +1895,31 @@ void CaretBase::invalidateCaretRect(Node* node, bool caretRectChanged, CaretAnim
 void FrameSelection::paintCaret(GraphicsContext& context, const LayoutPoint& paintOffset)
 {
     if (m_selection.isCaret() && m_selection.start().deprecatedNode())
-        CaretBase::paintCaret(*m_selection.start().deprecatedNode(), context, paintOffset, m_caretAnimator.ptr(), this->selection());
+        CaretBase::paintCaret(*m_selection.start().deprecatedNode(), context, paintOffset, m_caretAnimator.ptr());
 }
 
-Color CaretBase::computeCaretColor(const RenderStyle& elementStyle, const Node* node, const std::optional<VisibleSelection>& selection)
+Color CaretBase::computeCaretColor(const RenderStyle& elementStyle, const Node* node)
 {
     // On iOS, we want to fall back to the tintColor, and only override if CSS has explicitly specified a custom color.
 #if PLATFORM(IOS_FAMILY) && !PLATFORM(MACCATALYST)
     UNUSED_PARAM(node);
-    UNUSED_PARAM(selection);
-    if (elementStyle.hasAutoCaretColor())
+    if (elementStyle.hasAutoCaretColor() && !elementStyle.hasExplicitlySetColor())
         return { };
     return elementStyle.colorResolvingCurrentColor(elementStyle.caretColor());
 #elif HAVE(REDESIGNED_TEXT_CURSOR)
-    const RenderStyle* elementStyleToUse = [&] {
-        if (!selection)
-            return &elementStyle;
-
-        RefPtr editableRoot = selection->rootEditableElement();
-        if (!editableRoot || !editableRoot->renderer())
-            return &elementStyle;
-
-        return &editableRoot->renderer()->style();
-    }();
-
-    if (elementStyleToUse->hasAutoCaretColor()) {
+    if (elementStyle.hasAutoCaretColor() && !elementStyle.hasExplicitlySetColor()) {
 #if PLATFORM(MAC)
         auto cssColorValue = CSSValueAppleSystemControlAccent;
 #else
         auto cssColorValue = CSSValueAppleSystemBlue;
 #endif
-        auto styleColorOptions = node->document().styleColorOptions(elementStyleToUse);
+        auto styleColorOptions = node->document().styleColorOptions(&elementStyle);
         auto systemAccentColor = RenderTheme::singleton().systemColor(cssColorValue, styleColorOptions | StyleColorOptions::UseSystemAppearance);
-        return elementStyleToUse->colorByApplyingColorFilter(systemAccentColor);
+        return elementStyle.colorByApplyingColorFilter(systemAccentColor);
     }
 
-    return elementStyleToUse->colorByApplyingColorFilter(elementStyleToUse->colorResolvingCurrentColor(elementStyleToUse->caretColor()));
+    return elementStyle.colorByApplyingColorFilter(elementStyle.colorResolvingCurrentColor(elementStyle.caretColor()));
 #else
-    UNUSED_PARAM(selection);
     RefPtr parentElement = node ? node->parentElement() : nullptr;
     auto* parentStyle = parentElement && parentElement->renderer() ? &parentElement->renderer()->style() : nullptr;
     // CSS value "auto" is treated as an invalid color.
@@ -1947,7 +1934,7 @@ Color CaretBase::computeCaretColor(const RenderStyle& elementStyle, const Node*
 #endif
 }
 
-void CaretBase::paintCaret(const Node& node, GraphicsContext& context, const LayoutPoint& paintOffset, CaretAnimator* caretAnimator, const std::optional<VisibleSelection>& selection) const
+void CaretBase::paintCaret(const Node& node, GraphicsContext& context, const LayoutPoint& paintOffset, CaretAnimator* caretAnimator) const
 {
 #if ENABLE(TEXT_CARET)
     auto caretPresentationProperties = caretAnimator ? caretAnimator->presentationProperties() : CaretAnimator::PresentationProperties();
@@ -1964,7 +1951,7 @@ void CaretBase::paintCaret(const Node& node, GraphicsContext& context, const Lay
     Color caretColor = Color::black;
     auto* element = is<Element>(node) ? downcast<Element>(&node) : node.parentElement();
     if (element && element->renderer())
-        caretColor = CaretBase::computeCaretColor(element->renderer()->style(), &node, selection);
+        caretColor = CaretBase::computeCaretColor(element->renderer()->style(), &node);
 
     auto pixelSnappedCaretRect = snapRectToDevicePixels(caret, node.document().deviceScaleFactor());
     if (caretAnimator)
@@ -1976,7 +1963,6 @@ void CaretBase::paintCaret(const Node& node, GraphicsContext& context, const Lay
     UNUSED_PARAM(context);
     UNUSED_PARAM(paintOffset);
     UNUSED_PARAM(caretAnimator);
-    UNUSED_PARAM(selection);
 #endif
 }
 
@@ -2413,7 +2399,7 @@ void DragCaretController::paintDragCaret(LocalFrame* frame, GraphicsContext& p,
 {
 #if ENABLE(TEXT_CARET)
     if (m_position.deepEquivalent().deprecatedNode() && m_position.deepEquivalent().deprecatedNode()->document().frame() == frame)
-        paintCaret(*m_position.deepEquivalent().deprecatedNode(), p, paintOffset, nullptr, std::nullopt);
+        paintCaret(*m_position.deepEquivalent().deprecatedNode(), p, paintOffset, nullptr);
 #else
     UNUSED_PARAM(frame);
     UNUSED_PARAM(p);

diff --git a/Source/WebCore/editing/FrameSelection.h b/Source/WebCore/editing/FrameSelection.h
@@ -58,7 +58,7 @@ class CaretBase {
     WTF_MAKE_NONCOPYABLE(CaretBase);
     WTF_MAKE_FAST_ALLOCATED;
 public:
-    WEBCORE_EXPORT static Color computeCaretColor(const RenderStyle& elementStyle, const Node*, const std::optional<VisibleSelection>&);
+    WEBCORE_EXPORT static Color computeCaretColor(const RenderStyle& elementStyle, const Node*);
 protected:
     enum class CaretVisibility : bool { Visible, Hidden };
     explicit CaretBase(CaretVisibility = CaretVisibility::Hidden);
@@ -67,7 +67,7 @@ class CaretBase {
     void clearCaretRect();
     bool updateCaretRect(Document&, const VisiblePosition& caretPosition);
     bool shouldRepaintCaret(const RenderView*, bool isContentEditable) const;
-    void paintCaret(const Node&, GraphicsContext&, const LayoutPoint&, CaretAnimator*, const std::optional<VisibleSelection>&) const;
+    void paintCaret(const Node&, GraphicsContext&, const LayoutPoint&, CaretAnimator*) const;
 
     const LayoutRect& localCaretRectWithoutUpdate() const { return m_caretLocalRect; }
 

diff --git a/Source/WebCore/rendering/RenderThemeIOS.mm b/Source/WebCore/rendering/RenderThemeIOS.mm
@@ -1373,7 +1373,7 @@ static bool shouldUseConvexGradient(const Color& backgroundColor)
 
 Color RenderThemeIOS::autocorrectionReplacementMarkerColor(const RenderText& renderer) const
 {
-    auto caretColor = CaretBase::computeCaretColor(renderer.style(), renderer.textNode(), std::nullopt);
+    auto caretColor = CaretBase::computeCaretColor(renderer.style(), renderer.textNode());
     if (!caretColor.isValid())
         caretColor = insertionPointColor();
 

diff --git a/Source/WebCore/rendering/style/RenderStyle.h b/Source/WebCore/rendering/style/RenderStyle.h
@@ -1590,6 +1590,8 @@ class RenderStyle {
     inline SVGPaintType fillPaintType() const;
     inline StyleColor fillPaintColor() const;
     inline void setFillPaintColor(const StyleColor&);
+    inline void setHasExplicitlySetColor(bool);
+    inline bool hasExplicitlySetColor() const;
     inline float fillOpacity() const;
     inline void setFillOpacity(float);
 
@@ -2197,6 +2199,8 @@ class RenderStyle {
         unsigned autosizeStatus : 5;
 #endif
         // 51 bits
+        unsigned hasExplicitlySetColor : 1;
+        // 52 bits
     };
 
     // This constructor is used to implement the replace operation.

diff --git a/Source/WebCore/rendering/style/RenderStyleInlines.h b/Source/WebCore/rendering/style/RenderStyleInlines.h
@@ -464,6 +464,7 @@ constexpr TextEmphasisFill RenderStyle::initialTextEmphasisFill() { return TextE
 constexpr TextEmphasisMark RenderStyle::initialTextEmphasisMark() { return TextEmphasisMark::None; }
 constexpr OptionSet<TextEmphasisPosition> RenderStyle::initialTextEmphasisPosition() { return { TextEmphasisPosition::Over, TextEmphasisPosition::Right }; }
 inline StyleColor RenderStyle::initialTextFillColor() { return StyleColor::currentColor(); }
+inline bool RenderStyle::hasExplicitlySetColor() const { return m_inheritedFlags.hasExplicitlySetColor; }
 constexpr TextGroupAlign RenderStyle::initialTextGroupAlign() { return TextGroupAlign::None; }
 inline Length RenderStyle::initialTextIndent() { return zeroLength(); }
 constexpr TextIndentLine RenderStyle::initialTextIndentLine() { return TextIndentLine::FirstLine; }
@@ -803,7 +804,8 @@ inline bool RenderStyle::InheritedFlags::operator==(const InheritedFlags& other)
         && pointerEvents == other.pointerEvents
         && insideLink == other.insideLink
         && insideDefaultButton == other.insideDefaultButton
-        && writingMode == other.writingMode;
+        && writingMode == other.writingMode
+        && hasExplicitlySetColor == other.hasExplicitlySetColor;
 }
 
 inline bool RenderStyle::NonInheritedFlags::operator==(const NonInheritedFlags& other) const

diff --git a/Source/WebCore/rendering/style/RenderStyleSetters.h b/Source/WebCore/rendering/style/RenderStyleSetters.h
@@ -299,6 +299,7 @@ inline void RenderStyle::setTextEmphasisFill(TextEmphasisFill fill) { SET(m_rare
 inline void RenderStyle::setTextEmphasisMark(TextEmphasisMark mark) { SET(m_rareInheritedData, textEmphasisMark, static_cast<unsigned>(mark)); }
 inline void RenderStyle::setTextEmphasisPosition(OptionSet<TextEmphasisPosition> position) { SET(m_rareInheritedData, textEmphasisPosition, static_cast<unsigned>(position.toRaw())); }
 inline void RenderStyle::setTextFillColor(const StyleColor& color) { SET(m_rareInheritedData, textFillColor, color); }
+inline void RenderStyle::setHasExplicitlySetColor(bool value) { m_inheritedFlags.hasExplicitlySetColor = value; }
 inline void RenderStyle::setTextGroupAlign(TextGroupAlign value) { SET_NESTED(m_nonInheritedData, rareData, textGroupAlign, static_cast<unsigned>(value)); }
 inline void RenderStyle::setTextIndent(Length&& length) { SET(m_rareInheritedData, indent, WTFMove(length)); }
 inline void RenderStyle::setTextIndentLine(TextIndentLine value) { SET(m_rareInheritedData, textIndentLine, static_cast<unsigned>(value)); }

diff --git a/Source/WebCore/style/StyleBuilderCustom.h b/Source/WebCore/style/StyleBuilderCustom.h
@@ -89,6 +89,7 @@ class BuilderCustom {
     DECLARE_PROPERTY_CUSTOM_HANDLERS(BoxShadow);
     DECLARE_PROPERTY_CUSTOM_HANDLERS(CaretColor);
     DECLARE_PROPERTY_CUSTOM_HANDLERS(Clip);
+    DECLARE_PROPERTY_CUSTOM_HANDLERS(Color);
     DECLARE_PROPERTY_CUSTOM_HANDLERS(Contain);
     DECLARE_PROPERTY_CUSTOM_HANDLERS(ContainIntrinsicWidth);
     DECLARE_PROPERTY_CUSTOM_HANDLERS(ContainIntrinsicHeight);
@@ -164,8 +165,6 @@ class BuilderCustom {
     static void applyInheritCustomProperty(BuilderState&, const CSSRegisteredCustomProperty*, const AtomString& name);
     static void applyValueCustomProperty(BuilderState&, const CSSRegisteredCustomProperty*, const CSSCustomPropertyValue&);
 
-    static void applyValueColor(BuilderState&, CSSValue&);
-
 private:
     static void resetEffectiveZoom(BuilderState&);
 
@@ -2034,7 +2033,31 @@ inline void BuilderCustom::applyValueColor(BuilderState& builderState, CSSValue&
         auto color = builderState.colorFromPrimitiveValue(primitiveValue, ForVisitedLink::Yes);
         builderState.style().setVisitedLinkColor(resolveColor(color));
     }
+
+    builderState.style().setDisallowsFastPathInheritance();
+    builderState.style().setHasExplicitlySetColor(builderState.isAuthorOrigin());
+}
+
+inline void BuilderCustom::applyInitialColor(BuilderState& builderState)
+{
+    if (builderState.applyPropertyToRegularStyle())
+        builderState.style().setColor(RenderStyle::initialColor());
+    if (builderState.applyPropertyToVisitedLinkStyle())
+        builderState.style().setVisitedLinkColor(RenderStyle::initialColor());
+
+    builderState.style().setDisallowsFastPathInheritance();
+    builderState.style().setHasExplicitlySetColor(builderState.isAuthorOrigin());
+}
+
+inline void BuilderCustom::applyInheritColor(BuilderState& builderState)
+{
+    if (builderState.applyPropertyToRegularStyle())
+        builderState.style().setColor(builderState.parentStyle().color());
+    if (builderState.applyPropertyToVisitedLinkStyle())
+        builderState.style().setVisitedLinkColor(builderState.parentStyle().color());
+
     builderState.style().setDisallowsFastPathInheritance();
+    builderState.style().setHasExplicitlySetColor(builderState.isAuthorOrigin());
 }
 
 inline void BuilderCustom::applyInitialCustomProperty(BuilderState& builderState, const CSSRegisteredCustomProperty* registered, const AtomString& name)

diff --git a/Source/WebCore/style/StyleBuilderState.h b/Source/WebCore/style/StyleBuilderState.h
@@ -106,6 +106,11 @@ class BuilderState {
 
     void setIsBuildingKeyframeStyle() { m_isBuildingKeyframeStyle = true; }
 
+    bool isAuthorOrigin() const
+    {
+        return m_currentProperty && m_currentProperty->cascadeLevel == CascadeLevel::Author;
+    }
+
 private:
     // See the comment in maybeUpdateFontForLetterSpacing() about why this needs to be a friend.
     friend void maybeUpdateFontForLetterSpacing(BuilderState&, CSSValue&);

diff --git a/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm b/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
@@ -387,7 +387,7 @@ static void convertContentToRootView(const LocalFrameView& view, Vector<Selectio
             // rather than the focused element. This causes caret colors in editable children to be
             // ignored in favor of the editing host's caret color. See: <https://webkit.org/b/229809>.
             if (RefPtr editableRoot = selection.rootEditableElement(); editableRoot && editableRoot->renderer()) {
-                postLayoutData.caretColor = CaretBase::computeCaretColor(editableRoot->renderer()->style(), editableRoot.get(), std::nullopt);
+                postLayoutData.caretColor = CaretBase::computeCaretColor(editableRoot->renderer()->style(), editableRoot.get());
                 postLayoutData.hasGrammarDocumentMarkers = editableRoot->document().markers().hasMarkers(makeRangeSelectingNodeContents(*editableRoot), DocumentMarker::Grammar);
             }
         }

diff --git a/Source/WebKitLegacy/mac/WebView/WebFrame.mm b/Source/WebKitLegacy/mac/WebView/WebFrame.mm
@@ -1388,7 +1388,7 @@ - (CGColorRef)caretColor
     auto* renderer = focusedElement->renderer();
     if (!renderer)
         return nil;
-    auto color = WebCore::CaretBase::computeCaretColor(renderer->style(), renderer->element(), std::nullopt);
+    auto color = WebCore::CaretBase::computeCaretColor(renderer->style(), renderer->element());
     return color.isValid() ? cachedCGColor(color).autorelease() : nil;
 }