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

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

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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.
   constexpr int kHeadlineSize = 20;
   constexpr int kTitleSize = 15;
   constexpr int kBodyTextLargeSize = 13;
   constexpr int kDefaultSize = 12;
 
   int size_delta = kDefaultSize - gfx::PlatformFont::kDefaultBaseFontSize;
   gfx::Font::Weight font_weight = gfx::Font::Weight::NORMAL;
@@ skipping 22 matching lines @@
   // Ignore |style| since it only affects color in the Harmony spec.
 
   return ui::ResourceBundle::GetSharedInstance().GetFontListWithDelta(
       size_delta, gfx::Font::NORMAL, font_weight);
 }
 
 SkColor HarmonyTypographyProvider::GetColor(
     int context,
     int style,
     const ui::NativeTheme& theme) const {
-  const SkColor foreground_color =
-      theme.GetSystemColor(ui::NativeTheme::kColorId_LabelEnabledColor);
+  if (ShouldIgnoreHarmonySpec(theme))
+    return GetHarmonyTextColorForNonStandardNativeTheme(context, style, theme);
 
-  // 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 (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) {
     case views::style::STYLE_DIALOG_BUTTON_DEFAULT:
       return SK_ColorWHITE;
     case views::style::STYLE_DISABLED:
       return SkColorSetRGB(0x9e, 0x9e, 0x9e);

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

While here: I read the spec as saying these are actually supposed to all be #000
with different alphas (87%, 54%, 38%) rather than opaque colors.  It does
provide opaque values, but those seem to be explanatory rather than normative.

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

I think it's the opposite - if we added an alpha component, the color would
change depending on the background, but I've seen no indication we should ever
do that from the spec. The spec would also need to be explicit about the
compositing order: dialog background, button background, hover shade, inkdrop,
and text are all layered but only the inkdrop is influenced by other colors.

transparent colors are more expensive to draw and raise questions around how
subpixel antialiasing should work. Unless we are actually drawing text over
something like a gradient AND want the font to have different color values
depending where it is on the gradient, then I think we should never have alpha
values in font colors.

Also methods like color_utils::GetReadableColor say "alpha values will be
ignored".

Basically alpha makes lots of things more complex and we don't have a use-case
that requires alpha for text.

     case views::style::STYLE_LINK:
       return gfx::kGoogleBlue700;
     case STYLE_SECONDARY:
     case STYLE_HINT:
       return SkColorSetRGB(0x75, 0x75, 0x75);
     case STYLE_RED:
       return gfx::kGoogleRed700;
     case STYLE_GREEN:
       return gfx::kGoogleGreen700;
   }
@@ skipping 60 matching lines @@
     case views::style::CONTEXT_DIALOG_TITLE:
       return title_height;
     case CONTEXT_BODY_TEXT_LARGE:
       return body_large_height;
     case CONTEXT_HEADLINE:
       return headline_height;
     default:
       return default_height;
   }
 }