Attachment 357239 Details for Bug 192639 – Patch

[patch] Patch

bug-192639-20181213105941.patch (text/plain), 13.21 KB, created by Timothy Hatcher on 2018-12-13 10:59:41 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Timothy Hatcher

Created: 2018-12-13 10:59:41 PST

Size: 13.21 KB

patch

obsolete

>Subversion Revision: 239074
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 6bc33384e05e35cfba4b62827b51ac87e0f8d606..1c105fdb92fd01e51a7872677cba17d2c3f68182 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        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-10  Brent Fulgham  <bfulgham@apple.com>
> 
>         SVGViewSpec objects should mark relevant SVG elements
>diff --git a/Source/WebCore/css/StyleResolver.cpp b/Source/WebCore/css/StyleResolver.cpp
>index ecfa4c560fac3cc0d2e04b39f75e2163e7943123..74fa3f5398fe6fbe4246ce6c10d38d59f8e2b3a6 100644
>--- a/Source/WebCore/css/StyleResolver.cpp
>+++ b/Source/WebCore/css/StyleResolver.cpp
>@@ -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?
>diff --git a/Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp b/Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp
>index da1c80fd2fd8110d9f58c73213bdc83bebd02f7f..28638c6c83a5ddca1c3afd0bfc606d14ba018853 100644
>--- a/Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp
>+++ b/Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp
>@@ -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
>diff --git a/Source/WebCore/platform/graphics/cocoa/GraphicsContextCocoa.mm b/Source/WebCore/platform/graphics/cocoa/GraphicsContextCocoa.mm
>index bf5e8b1d8a2799d7a30a58eb5eff8e60123335d5..068aefa0ab5ad98301a0c1ec4cd14b13571e179e 100644
>--- a/Source/WebCore/platform/graphics/cocoa/GraphicsContextCocoa.mm
>+++ b/Source/WebCore/platform/graphics/cocoa/GraphicsContextCocoa.mm
>@@ -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;
> }
>diff --git a/Source/WebCore/platform/mac/ThemeMac.mm b/Source/WebCore/platform/mac/ThemeMac.mm
>index 955d693e923525689ccca3fb298f686746e2ddcf..5e3eae0bde3f07c3b0ab2c4700117b83050a494d 100644
>--- a/Source/WebCore/platform/mac/ThemeMac.mm
>+++ b/Source/WebCore/platform/mac/ThemeMac.mm
>@@ -29,8 +29,10 @@
> #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;
> }
>diff --git a/Source/WebCore/rendering/RenderElement.cpp b/Source/WebCore/rendering/RenderElement.cpp
>index 1db56c17ed6e28ba6a44084de43ec97a58eebb7c..c9b95d82389907d6913bd3c53a5f6f0c70ed55da 100644
>--- a/Source/WebCore/rendering/RenderElement.cpp
>+++ b/Source/WebCore/rendering/RenderElement.cpp
>@@ -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
>diff --git a/Source/WebCore/rendering/RenderImage.cpp b/Source/WebCore/rendering/RenderImage.cpp
>index 44a6e0ef6064b91db90a323e897af91d555c4a1e..85a101dc86c563b51c5e3150d3868561914baa91 100644
>--- a/Source/WebCore/rendering/RenderImage.cpp
>+++ b/Source/WebCore/rendering/RenderImage.cpp
>@@ -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
>diff --git a/Source/WebCore/rendering/RenderTheme.cpp b/Source/WebCore/rendering/RenderTheme.cpp
>index d65a58daa96948f2b8fc045d529e38766a82c987..76c9c5c78d56d3e8d97d421f4dfaa4ab1bd9ecc0 100644
>--- a/Source/WebCore/rendering/RenderTheme.cpp
>+++ b/Source/WebCore/rendering/RenderTheme.cpp
>@@ -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
>diff --git a/Source/WebCore/rendering/RenderTheme.h b/Source/WebCore/rendering/RenderTheme.h
>index 132a6e49f0966d7d5037cb5c6a0364f8398999b9..a6292a437c9f9352dde0b6b25bca8175896a909f 100644
>--- a/Source/WebCore/rendering/RenderTheme.h
>+++ b/Source/WebCore/rendering/RenderTheme.h
>@@ -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; }
>diff --git a/Source/WebCore/rendering/RenderThemeMac.mm b/Source/WebCore/rendering/RenderThemeMac.mm
>index 423e2930e6ea483f5755e0121a94ebd74f1ca331..aa8b68b52dc49c76ee9b7de942f0e239a0f62d99 100644
>--- a/Source/WebCore/rendering/RenderThemeMac.mm
>+++ b/Source/WebCore/rendering/RenderThemeMac.mm
>@@ -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.

Actions: View | Formatted Diff | Diff

Attachments on bug 192639: 357239