Changeset 233280 in webkit

Timestamp:

Jun 27, 2018 3:14:53 PM (6 years ago)

Author:

timothy@apple.com

Message:

Find on page selection color isn't adapted for dark mode.
​https://bugs.webkit.org/show_bug.cgi?id=187072
rdar://problem/40354841

Reviewed by Tim Horton.

Source/WebCore:

• page/mac/TextIndicatorWindow.mm:

(-[WebTextIndicatorView initWithFrame:textIndicator:margin:offset:]): Use [NSColor findHighlightColor].

• platform/mac/LocalDefaultSystemAppearance.h:

(WebCore::LocalDefaultSystemAppearance::usingDarkAppearance const): Added.

• platform/mac/LocalDefaultSystemAppearance.mm:

(WebCore::LocalDefaultSystemAppearance::LocalDefaultSystemAppearance): Set m_usingDarkAppearance.

• rendering/InlineTextBox.cpp:

(WebCore::InlineTextBox::paintPlatformDocumentMarkers): Use TextPaintPhase::Decoration since this
matches step three of InlineTextBox::paint ("Paint fancy decorations"). This allows TextMatch to
paint a forground and not end up painting during this "fancy decorations" phase.
(WebCore::InlineTextBox::resolveStyleForMarkedText): Set the fillColor for TextMarker to force a
dark text color which will draw over the yellow highlight.
(WebCore::InlineTextBox::collectMarkedTextsForDocumentMarkers): Added support for TextPaintPhase::Decoration.
Seperate DocumentMarker::TelephoneNumber and DocumentMarker::TextMatch. Have DocumentMarker::TextMatch
support Forground and Background phases.

• rendering/RenderTheme.cpp:

(WebCore::RenderTheme::platformColorsDidChange):
(WebCore::RenderTheme::activeTextSearchHighlightColor const): Added. Call the platfrom version.
(WebCore::RenderTheme::inactiveTextSearchHighlightColor const): Added. Ditto.
(WebCore::RenderTheme::platformActiveTextSearchHighlightColor const): Added StyleColor::Options.
(WebCore::RenderTheme::platformInactiveTextSearchHighlightColor const): Ditto.

• rendering/RenderTheme.h:
• rendering/RenderThemeMac.h:
• rendering/RenderThemeMac.mm:

(WebCore::RenderThemeMac::platformActiveTextSearchHighlightColor const): Added.
(WebCore::RenderThemeMac::platformInactiveTextSearchHighlightColor const): Added.
(WebCore::RenderThemeMac::platformColorsDidChange): Clear new color caches.
(WebCore::RenderThemeMac::systemColor const): Cache system colors by light and dark mode.

LayoutTests:

• fast/css/apple-system-control-colors-expected.txt: Updated.
• fast/text/mark-matches-broken-line-rendering-expected.html: Ditto.
• fast/text/mark-matches-rendering-expected.html: Ditto.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/css/apple-system-control-colors-expected.txt (modified) (1 diff)
• LayoutTests/fast/text/mark-matches-broken-line-rendering-expected.html (modified) (1 diff)
• LayoutTests/fast/text/mark-matches-rendering-expected.html (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/page/mac/TextIndicatorWindow.mm (modified) (2 diffs)
• Source/WebCore/platform/mac/LocalDefaultSystemAppearance.h (modified) (1 diff)
• Source/WebCore/platform/mac/LocalDefaultSystemAppearance.mm (modified) (1 diff)
• Source/WebCore/rendering/InlineTextBox.cpp (modified) (5 diffs)
• Source/WebCore/rendering/RenderTheme.cpp (modified) (2 diffs)
• Source/WebCore/rendering/RenderTheme.h (modified) (3 diffs)
• Source/WebCore/rendering/RenderThemeMac.h (modified) (2 diffs)
• Source/WebCore/rendering/RenderThemeMac.mm (modified) (6 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2018-06-27  Timothy Hatcher  <timothy@apple.com>
+
+        Find on page selection color isn't adapted for dark mode.
+        https://bugs.webkit.org/show_bug.cgi?id=187072
+        rdar://problem/40354841
+
+        Reviewed by Tim Horton.
+
+        * fast/css/apple-system-control-colors-expected.txt: Updated.
+        * fast/text/mark-matches-broken-line-rendering-expected.html: Ditto.
+        * fast/text/mark-matches-rendering-expected.html: Ditto.
+
 2018-06-27  Youenn Fablet  <youenn@apple.com>
 

trunk/LayoutTests/fast/css/apple-system-control-colors-expected.txt

@@ -13 +13 @@
 -apple-system-unemphasized-selected-text-background : rgb(212, 212, 212)
 -apple-system-placeholder-text : rgba(0, 0, 0, 0.247059)
--apple-system-find-highlight-background : rgb(255, 255, 0)
+-apple-system-find-highlight-background : rgb(255, 204, 0)
 -apple-system-label : rgba(0, 0, 0, 0.85098)
 -apple-system-secondary-label : rgba(0, 0, 0, 0.498039)

trunk/LayoutTests/fast/text/mark-matches-broken-line-rendering-expected.html

@@ -7 +7 @@
 
 span {
-    background-color: yellow;
+    background-color: rgb(255, 204, 0);
+    color: -apple-system-label;
 }
 </style>

trunk/LayoutTests/fast/text/mark-matches-rendering-expected.html

@@ -1 @@
-<p style="display:inline-block; background-color:yellow">
+<p style="display: inline-block; background-color: rgb(255, 204, 0); color: -apple-system-label">
 Quo usque tandem abutere, Catilina, patientia nostra?
 </p>

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2018-06-27  Timothy Hatcher  <timothy@apple.com>
+
+        Find on page selection color isn't adapted for dark mode.
+        https://bugs.webkit.org/show_bug.cgi?id=187072
+        rdar://problem/40354841
+
+        Reviewed by Tim Horton.
+
+        * page/mac/TextIndicatorWindow.mm:
+        (-[WebTextIndicatorView initWithFrame:textIndicator:margin:offset:]): Use [NSColor findHighlightColor].
+        * platform/mac/LocalDefaultSystemAppearance.h:
+        (WebCore::LocalDefaultSystemAppearance::usingDarkAppearance const): Added.
+        * platform/mac/LocalDefaultSystemAppearance.mm:
+        (WebCore::LocalDefaultSystemAppearance::LocalDefaultSystemAppearance): Set m_usingDarkAppearance.
+        * rendering/InlineTextBox.cpp:
+        (WebCore::InlineTextBox::paintPlatformDocumentMarkers): Use TextPaintPhase::Decoration since this
+        matches step three of InlineTextBox::paint ("Paint fancy decorations"). This allows TextMatch to
+        paint a forground and not end up painting during this "fancy decorations" phase.
+        (WebCore::InlineTextBox::resolveStyleForMarkedText): Set the fillColor for TextMarker to force a
+        dark text color which will draw over the yellow highlight.
+        (WebCore::InlineTextBox::collectMarkedTextsForDocumentMarkers): Added support for TextPaintPhase::Decoration.
+        Seperate DocumentMarker::TelephoneNumber and DocumentMarker::TextMatch. Have DocumentMarker::TextMatch
+        support Forground and Background phases.
+        * rendering/RenderTheme.cpp:
+        (WebCore::RenderTheme::platformColorsDidChange):
+        (WebCore::RenderTheme::activeTextSearchHighlightColor const): Added. Call the platfrom version.
+        (WebCore::RenderTheme::inactiveTextSearchHighlightColor const): Added. Ditto.
+        (WebCore::RenderTheme::platformActiveTextSearchHighlightColor const): Added StyleColor::Options.
+        (WebCore::RenderTheme::platformInactiveTextSearchHighlightColor const): Ditto.
+        * rendering/RenderTheme.h:
+        * rendering/RenderThemeMac.h:
+        * rendering/RenderThemeMac.mm:
+        (WebCore::RenderThemeMac::platformActiveTextSearchHighlightColor const): Added.
+        (WebCore::RenderThemeMac::platformInactiveTextSearchHighlightColor const): Added.
+        (WebCore::RenderThemeMac::platformColorsDidChange): Clear new color caches.
+        (WebCore::RenderThemeMac::systemColor const): Cache system colors by light and dark mode.
+
 2018-06-27  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/WebCore/page/mac/TextIndicatorWindow.mm

@@ -35 +35 @@
 #import "WebActionDisablingCALayerDelegate.h"
 #import <pal/spi/cg/CoreGraphicsSPI.h>
+#import <pal/spi/cocoa/NSColorSPI.h>
 #import <pal/spi/cocoa/QuartzCoreSPI.h>
 
@@ -169 +170 @@
     RetainPtr<NSMutableArray> bounceLayers = adoptNS([[NSMutableArray alloc] init]);
 
-    RetainPtr<CGColorRef> highlightColor = [NSColor colorWithDeviceRed:1 green:1 blue:0 alpha:1].CGColor;
+#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300
+    RetainPtr<CGColorRef> highlightColor = [NSColor findHighlightColor].CGColor;
+#else
+    RetainPtr<CGColorRef> highlightColor = [NSColor colorWithDeviceRed:1 green:0.8 blue:0 alpha:1].CGColor;
+#endif
     RetainPtr<CGColorRef> rimShadowColor = [NSColor colorWithDeviceWhite:0 alpha:0.35].CGColor;
     RetainPtr<CGColorRef> dropShadowColor = [NSColor colorWithDeviceWhite:0 alpha:0.2].CGColor;

trunk/Source/WebCore/platform/mac/LocalDefaultSystemAppearance.h

@@ -39 +39 @@
 class LocalDefaultSystemAppearance {
     WTF_MAKE_NONCOPYABLE(LocalDefaultSystemAppearance);
+
 public:
     WEBCORE_EXPORT LocalDefaultSystemAppearance(bool useSystemAppearance, bool useDefaultAppearance);
     WEBCORE_EXPORT ~LocalDefaultSystemAppearance();
+
+    bool usingDarkAppearance() const
+    {
+#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
+        return m_usingDarkAppearance;
+#else
+        return false;
+#endif
+    }
+
 private:
 #if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
     RetainPtr<NSAppearance> m_savedSystemAppearance;
+    bool m_usingDarkAppearance { false };
 #endif
 };

trunk/Source/WebCore/platform/mac/LocalDefaultSystemAppearance.mm

@@ -43 +43 @@
 
     m_savedSystemAppearance = [NSAppearance currentAppearance];
-    [NSAppearance setCurrentAppearance:[NSAppearance appearanceNamed:(!useSystemAppearance || useDefaultAppearance ? NSAppearanceNameAqua : NSAppearanceNameDarkAqua)]];
+    m_usingDarkAppearance = useSystemAppearance && !useDefaultAppearance;
+
+    [NSAppearance setCurrentAppearance:[NSAppearance appearanceNamed:m_usingDarkAppearance ? NSAppearanceNameDarkAqua : NSAppearanceNameAqua]];
 #else
     UNUSED_PARAM(useSystemAppearance);

trunk/Source/WebCore/rendering/InlineTextBox.cpp

@@ -654 +654 @@
 void InlineTextBox::paintPlatformDocumentMarkers(GraphicsContext& context, const FloatPoint& boxOrigin)
 {
-    for (auto& markedText : subdivide(collectMarkedTextsForDocumentMarkers(TextPaintPhase::Foreground), OverlapStrategy::Frontmost))
+    for (auto& markedText : subdivide(collectMarkedTextsForDocumentMarkers(TextPaintPhase::Decoration), OverlapStrategy::Frontmost))
         paintPlatformDocumentMarker(context, boxOrigin, markedText);
 }
@@ -764 +764 @@
         break;
     }
-    case MarkedText::TextMatch:
-        style.backgroundColor = markedText.marker->isActiveMatch() ? renderer().theme().platformActiveTextSearchHighlightColor() : renderer().theme().platformInactiveTextSearchHighlightColor();
+    case MarkedText::TextMatch: {
+        // Text matches always use the light system appearance.
+        OptionSet<StyleColor::Options> styleColorOptions = { StyleColor::Options::UseSystemAppearance, StyleColor::Options::UseDefaultAppearance };
+#if PLATFORM(MAC)
+        style.textStyles.fillColor = renderer().theme().systemColor(CSSValueAppleSystemLabel, styleColorOptions);
+#endif
+        style.backgroundColor = markedText.marker->isActiveMatch() ? renderer().theme().activeTextSearchHighlightColor(styleColorOptions) : renderer().theme().inactiveTextSearchHighlightColor(styleColorOptions);
         break;
+    }
     }
     StyledMarkedText styledMarkedText = markedText;
@@ -833 +839 @@
 Vector<MarkedText> InlineTextBox::collectMarkedTextsForDocumentMarkers(TextPaintPhase phase)
 {
-    ASSERT(phase == TextPaintPhase::Background || phase == TextPaintPhase::Foreground);
+    ASSERT_ARG(phase, phase == TextPaintPhase::Background || phase == TextPaintPhase::Foreground || phase == TextPaintPhase::Decoration);
+
     if (!renderer().textNode())
         return { };
@@ -877 +884 @@
         case DocumentMarker::DictationPhraseWithAlternatives:
 #endif
-            if (phase == TextPaintPhase::Background)
+            if (phase != TextPaintPhase::Decoration)
                 continue;
             break;
@@ -883 +890 @@
             if (!renderer().frame().editor().markedTextMatchesAreHighlighted())
                 continue;
-#if ENABLE(TELEPHONE_NUMBER_DETECTION)
-            FALLTHROUGH;
-        case DocumentMarker::TelephoneNumber:
-#endif
-            if (phase == TextPaintPhase::Foreground)
+            if (phase == TextPaintPhase::Decoration)
                 continue;
             break;
+#if ENABLE(TELEPHONE_NUMBER_DETECTION)
+        case DocumentMarker::TelephoneNumber:
+            if (!renderer().frame().editor().markedTextMatchesAreHighlighted())
+                continue;
+            if (phase != TextPaintPhase::Background)
+                continue;
+            break;
+#endif
         default:
             continue;

trunk/Source/WebCore/rendering/RenderTheme.cpp

@@ -1159 +1159 @@
     m_inactiveListBoxSelectionForegroundColor = Color();
 
+    m_activeTextSearchHighlightColor = Color();
+    m_inactiveTextSearchHighlightColor = Color();
+
     Page::updateStyleForAllPagesAfterGlobalChangeInEnvironment();
 }
@@ -1284 +1287 @@
 }
 
-Color RenderTheme::platformActiveTextSearchHighlightColor() const
+Color RenderTheme::activeTextSearchHighlightColor(OptionSet<StyleColor::Options> options) const
+{
+    if (!m_activeTextSearchHighlightColor.isValid())
+        m_activeTextSearchHighlightColor = platformActiveTextSearchHighlightColor(options);
+    return m_activeTextSearchHighlightColor;
+}
+
+Color RenderTheme::inactiveTextSearchHighlightColor(OptionSet<StyleColor::Options> options) const
+{
+    if (!m_inactiveTextSearchHighlightColor.isValid())
+        m_inactiveTextSearchHighlightColor = platformInactiveTextSearchHighlightColor(options);
+    return m_inactiveTextSearchHighlightColor;
+}
+
+Color RenderTheme::platformActiveTextSearchHighlightColor(OptionSet<StyleColor::Options>) const
 {
     return Color(255, 150, 50); // Orange.
 }
 
-Color RenderTheme::platformInactiveTextSearchHighlightColor() const
+Color RenderTheme::platformInactiveTextSearchHighlightColor(OptionSet<StyleColor::Options>) const
 {
     return Color(255, 255, 0); // Yellow.

trunk/Source/WebCore/rendering/RenderTheme.h

@@ -145 +145 @@
     Color inactiveListBoxSelectionForegroundColor(OptionSet<StyleColor::Options>) const;
 
-    // Highlighting colors for TextMatches.
-    virtual Color platformActiveTextSearchHighlightColor() const;
-    virtual Color platformInactiveTextSearchHighlightColor() const;
+    // Highlighting colors for search matches.
+    Color activeTextSearchHighlightColor(OptionSet<StyleColor::Options>) const;
+    Color inactiveTextSearchHighlightColor(OptionSet<StyleColor::Options>) const;
 
     virtual Color disabledTextColor(const Color& textColor, const Color& backgroundColor) const;
@@ -265 +265 @@
     virtual Color platformActiveListBoxSelectionForegroundColor(OptionSet<StyleColor::Options>) const;
     virtual Color platformInactiveListBoxSelectionForegroundColor(OptionSet<StyleColor::Options>) const;
+
+    // The platform highlighting colors for search matches.
+    virtual Color platformActiveTextSearchHighlightColor(OptionSet<StyleColor::Options>) const;
+    virtual Color platformInactiveTextSearchHighlightColor(OptionSet<StyleColor::Options>) const;
 
     virtual bool supportsSelectionForegroundColors() const { return true; }
@@ -405 +409 @@
     mutable Color m_activeListBoxSelectionForegroundColor;
     mutable Color m_inactiveListBoxSelectionForegroundColor;
+
+    mutable Color m_activeTextSearchHighlightColor;
+    mutable Color m_inactiveTextSearchHighlightColor;
 };
 

trunk/Source/WebCore/rendering/RenderThemeMac.h

@@ -63 +63 @@
     Color platformInactiveListBoxSelectionForegroundColor(OptionSet<StyleColor::Options>) const final;
     Color platformFocusRingColor(OptionSet<StyleColor::Options>) const final;
+    Color platformActiveTextSearchHighlightColor(OptionSet<StyleColor::Options>) const final;
+    Color platformInactiveTextSearchHighlightColor(OptionSet<StyleColor::Options>) const final;
 
     ScrollbarControlSize scrollbarControlSizeForPart(ControlPart) final { return SmallScrollbar; }
@@ -243 +245 @@
     bool m_isSliderThumbVerticalPressed { false };
 
-    mutable HashMap<int, Color> m_systemColorCache;
-    mutable Color m_systemVisitedLinkColor;
+    mutable HashMap<int, Color> m_lightSystemColorCache;
+    mutable HashMap<int, Color> m_darkSystemColorCache;
+    mutable Color m_lightSystemVisitedLinkColor;
+    mutable Color m_darkSystemVisitedLinkColor;
 
     RetainPtr<WebCoreRenderThemeNotificationObserver> m_notificationObserver;

trunk/Source/WebCore/rendering/RenderThemeMac.mm

@@ -392 +392 @@
 }
 
+Color RenderThemeMac::platformActiveTextSearchHighlightColor(OptionSet<StyleColor::Options> options) const
+{
+#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300
+    LocalDefaultSystemAppearance localAppearance(options.contains(StyleColor::Options::UseSystemAppearance), options.contains(StyleColor::Options::UseDefaultAppearance));
+    return colorFromNSColor([NSColor findHighlightColor]);
+#else
+    UNUSED_PARAM(options);
+    return Color(255, 204, 0); // Yellow.
+#endif
+}
+
+Color RenderThemeMac::platformInactiveTextSearchHighlightColor(OptionSet<StyleColor::Options> options) const
+{
+    // The inactive color is normally used, since no legacy WebKit client marks a text match as active.
+    // So just return the same color for both states.
+    return platformActiveTextSearchHighlightColor(options);
+}
+
 static FontSelectionValue toFontWeight(NSInteger appKitFontWeight)
 {
@@ -487 +505 @@
 void RenderThemeMac::platformColorsDidChange()
 {
-    m_systemColorCache.clear();
-    m_systemVisitedLinkColor = Color();
+    m_lightSystemColorCache.clear();
+    m_darkSystemColorCache.clear();
+
+    m_lightSystemVisitedLinkColor = Color();
+    m_darkSystemVisitedLinkColor = Color();
+
     RenderTheme::platformColorsDidChange();
 }
@@ -505 +527 @@
         // Only use NSColor when the system appearance is desired, otherwise use RenderTheme's default.
         if (useSystemAppearance) {
-            if (!m_systemVisitedLinkColor.isValid())
-                m_systemVisitedLinkColor = semanticColorFromNSColor([NSColor systemPurpleColor]);
-            return m_systemVisitedLinkColor;
+            if (localAppearance.usingDarkAppearance()) {
+                if (!m_darkSystemVisitedLinkColor.isValid())
+                    m_darkSystemVisitedLinkColor = semanticColorFromNSColor([NSColor systemPurpleColor]);
+                return m_darkSystemVisitedLinkColor;
+            }
+
+            if (!m_lightSystemVisitedLinkColor.isValid())
+                m_lightSystemVisitedLinkColor = semanticColorFromNSColor([NSColor systemPurpleColor]);
+            return m_lightSystemVisitedLinkColor;
         }
 
@@ -515 +543 @@
     ASSERT(!forVisitedLink);
 
-    return m_systemColorCache.ensure(cssValueID, [this, cssValueID, useSystemAppearance, options] () -> Color {
+    auto& systemColorCache = localAppearance.usingDarkAppearance() ? m_darkSystemColorCache : m_lightSystemColorCache;
+    return systemColorCache.ensure(cssValueID, [this, cssValueID, useSystemAppearance, options] () -> Color {
         auto selectCocoaColor = [cssValueID, useSystemAppearance] () -> SEL {
             switch (cssValueID) {
@@ -623 +652 @@
                 return @selector(findHighlightColor);
 #else
-                return @selector(systemYellowColor);
+                // Handled below.
+                return nullptr;
 #endif
             case CSSValueAppleSystemLabel:
@@ -689 +719 @@
             return menuBackgroundColor();
 
+#if __MAC_OS_X_VERSION_MIN_REQUIRED < 101300
+        case CSSValueAppleSystemFindHighlightBackground:
+            return platformActiveTextSearchHighlightColor(options);
+#endif
+
         case CSSValueAppleSystemEvenAlternatingContentBackground: {
 #if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400