REGRESSION (r230064): Focus rings on webpages are fainter than in native UI.
authortimothy@apple.com <timothy@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Dec 2018 19:56:44 +0000 (19:56 +0000)
committertimothy@apple.com <timothy@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Dec 2018 19:56:44 +0000 (19:56 +0000)
https://bugs.webkit.org/show_bug.cgi?id=192639
rdar://problem/42669297

Reviewed by Tim Horton.

The focus ring color passed to CoreGraphics is expected to be opaque, since they
will apply opacity when drawing (because opacity is normally animated).
We were getting this by accident before when the old `RenderThemeMac::systemColor()`
used the old `convertNSColorToColor()`, which ignored alpha on NSColor.
Existing tests use fixed test focus ring color.

* css/StyleResolver.cpp:
(WebCore::StyleResolver::colorFromPrimitiveValue const): Use RenderTheme singleton for `focusRingColor()`.
* html/canvas/CanvasRenderingContext2D.cpp:
(WebCore::CanvasRenderingContext2D::drawFocusIfNeededInternal): Ditto.
* platform/graphics/cocoa/GraphicsContextCocoa.mm:
(WebCore::drawFocusRingAtTime): Use `CGContextStateSaver`.
* platform/mac/ThemeMac.mm:
(WebCore::drawCellFocusRingWithFrameAtTime): Force alpha to 1 on the focus ring color. Use `CGContextStateSaver`.
* rendering/RenderElement.cpp:
(WebCore::RenderElement::paintFocusRing): Use RenderTheme singleton for `focusRingColor()`.
* rendering/RenderImage.cpp:
(WebCore::RenderImage::paintAreaElementFocusRing): Ditto.
* rendering/RenderTheme.cpp:
(WebCore::RenderTheme::focusRingColor const): Made const. Cache the result of `platformFocusRingColor()`.
* rendering/RenderTheme.h: Made `focusRingColor()` a member function instead of static.
* rendering/RenderThemeMac.mm:
(WebCore::RenderThemeMac::platformFocusRingColor const): Force alpha to 1 on the focus ring color.
(WebCore::RenderThemeMac::systemColor const): Use `focusRingColor()`, instead of caching color here.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@239173 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/css/StyleResolver.cpp
Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp
Source/WebCore/platform/graphics/cocoa/GraphicsContextCocoa.mm
Source/WebCore/platform/mac/ThemeMac.mm
Source/WebCore/rendering/RenderElement.cpp
Source/WebCore/rendering/RenderImage.cpp
Source/WebCore/rendering/RenderTheme.cpp
Source/WebCore/rendering/RenderTheme.h
Source/WebCore/rendering/RenderThemeMac.mm

index c7c333b..97464dc 100644 (file)
@@ -1,3 +1,36 @@
+2018-12-13  Timothy Hatcher  <timothy@apple.com>
+
+        REGRESSION (r230064): Focus rings on webpages are fainter than in native UI.
+        https://bugs.webkit.org/show_bug.cgi?id=192639
+        rdar://problem/42669297
+
+        Reviewed by Tim Horton.
+
+        The focus ring color passed to CoreGraphics is expected to be opaque, since they
+        will apply opacity when drawing (because opacity is normally animated).
+        We were getting this by accident before when the old `RenderThemeMac::systemColor()`
+        used the old `convertNSColorToColor()`, which ignored alpha on NSColor.
+        Existing tests use fixed test focus ring color.
+
+        * css/StyleResolver.cpp:
+        (WebCore::StyleResolver::colorFromPrimitiveValue const): Use RenderTheme singleton for `focusRingColor()`.
+        * html/canvas/CanvasRenderingContext2D.cpp:
+        (WebCore::CanvasRenderingContext2D::drawFocusIfNeededInternal): Ditto.
+        * platform/graphics/cocoa/GraphicsContextCocoa.mm:
+        (WebCore::drawFocusRingAtTime): Use `CGContextStateSaver`.
+        * platform/mac/ThemeMac.mm:
+        (WebCore::drawCellFocusRingWithFrameAtTime): Force alpha to 1 on the focus ring color. Use `CGContextStateSaver`.
+        * rendering/RenderElement.cpp:
+        (WebCore::RenderElement::paintFocusRing): Use RenderTheme singleton for `focusRingColor()`.
+        * rendering/RenderImage.cpp:
+        (WebCore::RenderImage::paintAreaElementFocusRing): Ditto.
+        * rendering/RenderTheme.cpp:
+        (WebCore::RenderTheme::focusRingColor const): Made const. Cache the result of `platformFocusRingColor()`.
+        * rendering/RenderTheme.h: Made `focusRingColor()` a member function instead of static.
+        * rendering/RenderThemeMac.mm:
+        (WebCore::RenderThemeMac::platformFocusRingColor const): Force alpha to 1 on the focus ring color.
+        (WebCore::RenderThemeMac::systemColor const): Use `focusRingColor()`, instead of caching color here.
+
 2018-12-13  Eric Carlson  <eric.carlson@apple.com>
 
         [MediaStream] Calculate width or height when constraints contain only the other
index ecfa4c5..74fa3f5 100644 (file)
@@ -1865,7 +1865,7 @@ Color StyleResolver::colorFromPrimitiveValue(const CSSPrimitiveValue& value, boo
     case CSSValueWebkitActivelink:
         return document().activeLinkColor();
     case CSSValueWebkitFocusRingColor:
-        return RenderTheme::focusRingColor(document().styleColorOptions(m_state.style()));
+        return RenderTheme::singleton().focusRingColor(document().styleColorOptions(m_state.style()));
     case CSSValueCurrentcolor:
         // Color is an inherited property so depending on it effectively makes the property inherited.
         // FIXME: Setting the flag as a side effect of calling this function is a bit oblique. Can we do better?
index da1c80f..28638c6 100644 (file)
@@ -86,7 +86,7 @@ void CanvasRenderingContext2D::drawFocusIfNeededInternal(const Path& path, Eleme
     auto* context = drawingContext();
     if (!element.focused() || !state().hasInvertibleTransform || path.isEmpty() || !element.isDescendantOf(canvas()) || !context)
         return;
-    context->drawFocusRing(path, 1, 1, RenderTheme::focusRingColor(element.document().styleColorOptions(canvas().computedStyle())));
+    context->drawFocusRing(path, 1, 1, RenderTheme::singleton().focusRingColor(element.document().styleColorOptions(canvas().computedStyle())));
 }
 
 String CanvasRenderingContext2D::font() const
index bf5e8b1..068aefa 100644 (file)
@@ -73,10 +73,10 @@ static bool drawFocusRingAtTime(CGContextRef context, NSTimeInterval timeOffset,
     focusRingStyle.accumulate = -1;
     auto style = adoptCF(CGStyleCreateFocusRingWithColor(&focusRingStyle, cachedCGColor(color)));
 
-    CGContextSaveGState(context);
+    CGContextStateSaver stateSaver(context);
+
     CGContextSetStyle(context, style.get());
     CGContextFillPath(context);
-    CGContextRestoreGState(context);
 
     return needsRepaint;
 }
index 955d693..5e3eae0 100644 (file)
 #if PLATFORM(MAC)
 
 #import "AXObjectCache.h"
+#import "ColorMac.h"
 #import "ControlStates.h"
 #import "GraphicsContext.h"
+#import "GraphicsContextCG.h"
 #import "ImageBuffer.h"
 #import "LengthSize.h"
 #import "LocalCurrentGraphicsContext.h"
@@ -361,7 +363,8 @@ static bool drawCellFocusRingWithFrameAtTime(NSCell *cell, NSRect cellFrame, NSV
     ALLOW_DEPRECATED_DECLARATIONS_BEGIN
     CGContextRef cgContext = (CGContextRef)[[NSGraphicsContext currentContext] graphicsPort];
     ALLOW_DEPRECATED_DECLARATIONS_END
-    CGContextSaveGState(cgContext);
+
+    CGContextStateSaver stateSaver(cgContext);
 
     CGFocusRingStyle focusRingStyle;
     bool needsRepaint = NSInitializeCGFocusRingStyleForTime(NSFocusRingOnly, &focusRingStyle, timeOffset);
@@ -373,13 +376,14 @@ static bool drawCellFocusRingWithFrameAtTime(NSCell *cell, NSRect cellFrame, NSV
     focusRingStyle.accumulate = -1;
 
     // FIXME: This color should be shared with RenderThemeMac. For now just use the same NSColor color.
-    auto style = adoptCF(CGStyleCreateFocusRingWithColor(&focusRingStyle, [NSColor keyboardFocusIndicatorColor].CGColor));
+    // The color is expected to be opaque, since CoreGraphics will apply opacity when drawing (because opacity is normally animated).
+    auto color = colorWithOverrideAlpha(colorFromNSColor([NSColor keyboardFocusIndicatorColor]).rgb(), 1);
+    auto style = adoptCF(CGStyleCreateFocusRingWithColor(&focusRingStyle, cachedCGColor(color)));
     CGContextSetStyle(cgContext, style.get());
 
     CGContextBeginTransparencyLayerWithRect(cgContext, NSRectToCGRect(cellFrame), nullptr);
     [cell drawFocusRingMaskWithFrame:cellFrame inView:controlView];
     CGContextEndTransparencyLayer(cgContext);
-    CGContextRestoreGState(cgContext);
 
     return needsRepaint;
 }
index 1db56c1..c9b95d8 100644 (file)
@@ -1832,9 +1832,9 @@ void RenderElement::paintFocusRing(PaintInfo& paintInfo, const RenderStyle& styl
             for (auto rect : pixelSnappedFocusRingRects)
                 path.addRect(rect);
         }
-        paintInfo.context().drawFocusRing(path, page().focusController().timeSinceFocusWasSet().seconds(), needsRepaint, RenderTheme::focusRingColor(styleColorOptions()));
+        paintInfo.context().drawFocusRing(path, page().focusController().timeSinceFocusWasSet().seconds(), needsRepaint, RenderTheme::singleton().focusRingColor(styleColorOptions()));
     } else
-        paintInfo.context().drawFocusRing(pixelSnappedFocusRingRects, page().focusController().timeSinceFocusWasSet().seconds(), needsRepaint, RenderTheme::focusRingColor(styleColorOptions()));
+        paintInfo.context().drawFocusRing(pixelSnappedFocusRingRects, page().focusController().timeSinceFocusWasSet().seconds(), needsRepaint, RenderTheme::singleton().focusRingColor(styleColorOptions()));
     if (needsRepaint)
         page().focusController().setFocusedElementNeedsRepaint();
 #else
index 44a6e0e..85a101d 100644 (file)
@@ -592,7 +592,7 @@ void RenderImage::paintAreaElementFocusRing(PaintInfo& paintInfo, const LayoutPo
 
 #if PLATFORM(MAC)
     bool needsRepaint;
-    paintInfo.context().drawFocusRing(path, page().focusController().timeSinceFocusWasSet().seconds(), needsRepaint, RenderTheme::focusRingColor(styleColorOptions()));
+    paintInfo.context().drawFocusRing(path, page().focusController().timeSinceFocusWasSet().seconds(), needsRepaint, RenderTheme::singleton().focusRingColor(styleColorOptions()));
     if (needsRepaint)
         page().focusController().setFocusedElementNeedsRepaint();
 #else
index d65a58d..76c9c5c 100644 (file)
@@ -1411,9 +1411,15 @@ void RenderTheme::setCustomFocusRingColor(const Color& color)
     customFocusRingColor() = color;
 }
 
-Color RenderTheme::focusRingColor(OptionSet<StyleColor::Options> options)
+Color RenderTheme::focusRingColor(OptionSet<StyleColor::Options> options) const
 {
-    return customFocusRingColor().isValid() ? customFocusRingColor() : RenderTheme::singleton().platformFocusRingColor(options);
+    if (customFocusRingColor().isValid())
+        return customFocusRingColor();
+
+    auto& cache = colorCache(options);
+    if (!cache.systemFocusRingColor.isValid())
+        cache.systemFocusRingColor = platformFocusRingColor(options);
+    return cache.systemFocusRingColor;
 }
 
 String RenderTheme::fileListDefaultLabel(bool multipleFilesAllowed) const
index 132a6e4..a6292a4 100644 (file)
@@ -161,7 +161,7 @@ public:
 
     virtual Color disabledTextColor(const Color& textColor, const Color& backgroundColor) const;
 
-    static Color focusRingColor(OptionSet<StyleColor::Options>);
+    Color focusRingColor(OptionSet<StyleColor::Options>) const;
     virtual Color platformFocusRingColor(OptionSet<StyleColor::Options>) const { return Color(0, 0, 0); }
     static void setCustomFocusRingColor(const Color&);
     static float platformFocusRingWidth() { return 3; }
index 423e293..aa8b68b 100644 (file)
@@ -489,7 +489,9 @@ Color RenderThemeMac::platformFocusRingColor(OptionSet<StyleColor::Options> opti
 {
     if (usesTestModeFocusRingColor())
         return oldAquaFocusRingColor();
-    return systemColor(CSSValueWebkitFocusRingColor, options);
+    LocalDefaultSystemAppearance localAppearance(options.contains(StyleColor::Options::UseDarkAppearance));
+    // The color is expected to be opaque, since CoreGraphics will apply opacity when drawing (because opacity is normally animated).
+    return colorWithOverrideAlpha(colorFromNSColor([NSColor keyboardFocusIndicatorColor]).rgb(), 1);
 }
 
 Color RenderThemeMac::platformActiveTextSearchHighlightColor(OptionSet<StyleColor::Options> options) const
@@ -654,7 +656,7 @@ Color RenderThemeMac::systemColor(CSSValueID cssValueID, OptionSet<StyleColor::O
         // These should only be available when the web view is wanting the system appearance.
         case CSSValueWebkitFocusRingColor:
         case CSSValueActiveborder:
-            return systemAppearanceColor(cache.systemFocusRingColor, @selector(keyboardFocusIndicatorColor));
+            return focusRingColor(options);
 
         case CSSValueAppleSystemControlAccent:
 #if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
@@ -841,8 +843,8 @@ Color RenderThemeMac::systemColor(CSSValueID cssValueID, OptionSet<StyleColor::O
         case CSSValueActiveborder:
             // Hardcoded to avoid exposing a user appearance preference to the web for fingerprinting.
             if (localAppearance.usingDarkAppearance())
-                return Color(0x4C1AA9FF, Color::Semantic);
-            return Color(0x3F0067F4, Color::Semantic);
+                return Color(0xFF1AA9FF, Color::Semantic);
+            return Color(0xFF0067F4, Color::Semantic);
 
         case CSSValueAppleSystemControlAccent:
             // Hardcoded to avoid exposing a user appearance preference to the web for fingerprinting.