WebKit Bugzilla
Attachment 357239 Details for
Bug 192639
: REGRESSION (r230064): Focus rings on webpages are fainter than in native UI
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[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:
Patch
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.
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 192639
: 357239