Update dialog button text color for Harmony per the latest button spec

chrome/browser/ui/views/harmony/harmony_typography_provider.cc

Index: chrome/browser/ui/views/harmony/harmony_typography_provider.cc
diff --git a/chrome/browser/ui/views/harmony/harmony_typography_provider.cc b/chrome/browser/ui/views/harmony/harmony_typography_provider.cc
index 939f5a5844dd0adedd803aac227a3fefd92ce7df..abbf650f772e2733be9daf01537661a8d4f3397f 100644
--- a/chrome/browser/ui/views/harmony/harmony_typography_provider.cc
+++ b/chrome/browser/ui/views/harmony/harmony_typography_provider.cc
@@ -4,16 +4,89 @@
 
 #include "chrome/browser/ui/views/harmony/harmony_typography_provider.h"
 
+#include "build/build_config.h"
 #include "chrome/browser/ui/views/harmony/chrome_typography.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/platform_font.h"
 #include "ui/native_theme/native_theme.h"
 
+#if defined(OS_WIN)
+#include "ui/native_theme/native_theme_win.h"
+#endif
+
 #if defined(USE_ASH)
 #include "ash/public/cpp/ash_typography.h"  // nogncheck
 #endif
 
+namespace {
+
+// If the default foreground color from the native theme isn't black, the rest
+// of the Harmony spec isn't going to work. Also skip Harmony if a Windows
+// High Contrast theme is enabled. One of the four standard High Contrast themes
+// in Windows 10 still has black text, but (since the user wants high contrast)
+// the grey text shades in Harmony should not be used.
+bool ShouldIgnoreHarmonySpec(const ui::NativeTheme& theme) {
+#if defined(OS_WIN)
+  if (ui::NativeThemeWin::IsUsingHighContrastTheme())
+    return true;
+#endif
+  constexpr auto kTestColorId = ui::NativeTheme::kColorId_LabelEnabledColor;
+  return theme.GetSystemColor(kTestColorId) != SK_ColorBLACK;
+}
+
+// Returns a color for a possibly inverted or high-contrast OS color theme.
+SkColor GetHarmonyTextColorForNonStandardNativeTheme(
+    int context,
+    int style,
+    const ui::NativeTheme& theme) {
+  // At the time of writing, very few UI surfaces need typography for a Chrome-
+  // provided theme. Typically just incognito browser windows (when the native
+  // theme is NativeThemeDarkAura). Instead, this method is consulted when the
+  // actual OS theme is configured in a special way. So pick from a small number
+  // of NativeTheme constants that are known to adapt properly to distinct
+  // colors when configuring the OS to use a high-contrast theme. For example,
+  // ::GetSysColor() on Windows has 8 text colors: BTNTEXT, CAPTIONTEXT,
+  // GRAYTEXT, HIGHLIGHTTEXT, INACTIVECAPTIONTEXT, INFOTEXT (tool tips),
+  // MENUTEXT, and WINDOWTEXT. There's also hyperlinks: COLOR_HOTLIGHT.
+  // Diverging from these risks using a color that doesn't match user
+  // expectations.
+
+  // Heuristically determine whether the color scheme is inverted. The three
+  // standard inverted high contrast themes in Windows 10 use a white button
+  // text, so that's a good indicator.
+  const bool inverted_scheme =

[Peter Kasting, 2017/06/30 08:56:12]

Ideally, avoid looking for an inverted scheme and then toggling below; instead,
use GetReadableColor() with something you think is a representative color for
the background where you'd expect the proposed foreground to appear.  (I realize
you don't know what this might be, but doing something like picking the window
or button background can't be any less accurate than checking for the button
enabled color being white.)

If for some reason this absolutely wouldn't work, I'd use utilities like
IsDark() to check for "darker vs. lighter" instead of looking at "== white"
directly.  (A similar comment may apply in line 35, where it's not obvious to me
that "label color is black" is the right test.  In an ideal world, we'd probably
adjust our colors color-by-color, or else punt to default/theme colors when any
custom theme is enabled.  But even in a "test one color" sort of world, maybe we
should be looking for "default color is dark enough" rather than "black"?)

[tapted, 2017/07/03 05:20:50]

I've discovered color_utils::IsInvertedColorScheme()  -  I think it captures
what you are after from the second paragraph.


I don't think GetReadableColor is right here - per the comment about matching
user expectations - I think we'd actually want to match a color from the theme.
High contrast themes simplify the UI - generating new colors will work against
that. And views::Label already applies GetReadableColor (and permits code to
override that).

Also GetReadableColor says "This won't do anything but waste time if the
supplied foreground color has a luma value close to the midpoint (0.5 in the HSL
representation)." Disabled colors are exactly that - e.g. 0.54a.

Technically.. "solid yellow" is also that (i.e. L in HSL is 0.5) but
color_util's `GetLuma/GetRelativeLuminance()` is not derived from HSL :/

+      theme.GetSystemColor(ui::NativeTheme::kColorId_ButtonEnabledColor) ==
+      SK_ColorWHITE;
+
+  // Start with BTNTEXT for buttons, otherwise WINDOWTEXT. Note RED and GREEN
+  // will skip the NativeTheme entirely.

[Peter Kasting, 2017/06/30 08:56:12]

Nit: Avoid Windows-specific IDs in comments here and below (and honestly maybe
above), as it doesn't really give the reader needed info but it does confuse the
issue on non-Win.  I think it's clearer to always simply use the nativetheme IDs
(abbreviated) rather than referring to the underlying Windows native names.

[tapted, 2017/07/03 05:20:50]

Done. However, I think it does give the reader needed info.. The point of the
comments is to make it clear where colors come from if a future reader ever gets
here and needs to debug an issue. 99% of the cases where a Chrome process will
get here `in the wild` will be because of a high-contrast theme on Windows, and
will be directly influenced by those constants. Burying it under layers makes
that harder, IMO.

For code, sure. Referring to Windows constants in code here would be unnecessary
and undesirable coupling. But I don't think that applies to comments. The
constants are very strongly related.

+  ui::NativeTheme::ColorId color_id =
+      (context == views::style::CONTEXT_BUTTON ||
+       context == views::style::CONTEXT_BUTTON_MD)
+          ? ui::NativeTheme::kColorId_ButtonEnabledColor
+          : ui::NativeTheme::kColorId_TextfieldDefaultColor;
+  switch (style) {
+    case views::style::STYLE_DIALOG_BUTTON_DEFAULT:
+      // This is just white in Harmony and, even in inverted themes, prominent
+      // buttons have a dark background, so white will maximize contrast.
+      return SK_ColorWHITE;
+    case views::style::STYLE_DISABLED:
+      color_id = ui::NativeTheme::kColorId_LabelDisabledColor;  // GRAYTEXT.
+      break;
+    case views::style::STYLE_LINK:
+      color_id = ui::NativeTheme::kColorId_LinkEnabled;  // HOTLIGHT.
+      break;
+    case STYLE_RED:
+      return inverted_scheme ? gfx::kGoogleRed300 : gfx::kGoogleRed700;
+    case STYLE_GREEN:
+      return inverted_scheme ? gfx::kGoogleGreen300 : gfx::kGoogleGreen700;
+  }
+  return theme.GetSystemColor(color_id);
+}
+
+}  // namespace
+
 const gfx::FontList& HarmonyTypographyProvider::GetFont(int context,
                                                         int style) const {
   // "Target" font size constants from the Harmony spec.
@@ -56,32 +129,18 @@ SkColor HarmonyTypographyProvider::GetColor(
     int context,
     int style,
     const ui::NativeTheme& theme) const {
-  const SkColor foreground_color =
-      theme.GetSystemColor(ui::NativeTheme::kColorId_LabelEnabledColor);
-
-  // If the default foreground color from the native theme isn't black, the rest
-  // of the Harmony spec isn't going to work. TODO(tapted): Something more
-  // generic would be nice here, but that requires knowing the background color
-  // for the text. At the time of writing, very few UI surfaces need native-
-  // themed typography with a custom native theme. Typically just incognito
-  // browser windows, when the native theme is NativeThemeDarkAura.
-  if (foreground_color != SK_ColorBLACK) {
+  if (ShouldIgnoreHarmonySpec(theme))
+    return GetHarmonyTextColorForNonStandardNativeTheme(context, style, theme);
+
+  if (context == views::style::CONTEXT_BUTTON_MD) {
     switch (style) {
+      case views::style::STYLE_DIALOG_BUTTON_DEFAULT:
+        return SK_ColorWHITE;
       case views::style::STYLE_DISABLED:
-      case STYLE_SECONDARY:
-      case STYLE_HINT:
-        return theme.GetSystemColor(
-            ui::NativeTheme::kColorId_LabelDisabledColor);
-      case views::style::STYLE_LINK:
-        return theme.GetSystemColor(ui::NativeTheme::kColorId_LinkEnabled);
-      case STYLE_RED:
-        return foreground_color == SK_ColorWHITE ? gfx::kGoogleRed300
-                                                 : gfx::kGoogleRed700;
-      case STYLE_GREEN:
-        return foreground_color == SK_ColorWHITE ? gfx::kGoogleGreen300
-                                                 : gfx::kGoogleGreen700;
+        return SkColorSetRGB(0x9e, 0x9e, 0x9e);

[Peter Kasting, 2017/06/30 08:56:12]

The spec doesn't actually seem to give button-specific colors for disabled
buttons, so I think my comment about #000 w/alpha below applies here.

[tapted, 2017/07/03 05:20:50]

There's a "disabled" column in the 2017-06-28 spec attached to
http://crbug.com/691891 --
 https://bugs.chromium.org/p/chromium/issues/attachment?aid=290994&inline=1

It says "Font color: #000 0.38a (#757575)"  -- #000 @ 0.38a is actually #9e9e9e 
(the 757575 is just a typo I think, since that's what the old color was, and in
the new spec it wouldn't be any different to non-disabled).

+      default:
+        return SkColorSetRGB(0x75, 0x75, 0x75);

[Peter Kasting, 2017/06/30 08:56:12]

Shouldn't this be #5a5a5a?  Or am I misreading the spec?

[tapted, 2017/07/03 05:20:50]

That's the old color :). This change is to make it lighter per the latest,
button specific spec. The original sticker sheet (which covers more than just
buttons) is obsolete for buttons.

     }
-    return foreground_color;
   }
 
   switch (style) {