Use style::CONTEXT_TEXTFIELD for Combobox and Textfield

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 "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"
@@ skipping 48 matching lines @@
   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 (context == views::style::CONTEXT_TEXTFIELD) {
+      return theme.GetSystemColor(

[Peter Kasting, 2017/06/29 20:24:57]

Can we just return TypographyProvider::GetColor(...) here?  This looks like a
duplication of that code, and that seems like less of a maintenance risk.

[tapted, 2017/06/30 05:53:43]

I ended up tearing out this "what to do when we can't use Harmony" code into
some explicit functions (patchset 4), but those changes don't make as much sense
for this CL. Moved it to the CL that started all this
(https://codereview.chromium.org/2957263002/) and rebased on top. (I'll wait to
land this).


Longer explanation:

HarmonyTypographyProvider doesn't currently inherit from
DefaultTypographyProvider, so not without other changes. I've been hesitant to
add more layers of indirection. It's already super hard to answer questions like
"where did this color come from". We have explicitly set colors, implied
disabled/state colors, spec colors, native theme colors, common/aura theme
colors which all might get a say in what a color should be. (that's ignoring
Chrome themes for now).

I think "de-duplicating" things that happen to give the same result now, but
aren't really the same concept, is leading to the current set of problems in
NativeTheme -- everything is tangled and needs to be unwound to make simple
changes like "the text on buttons should be lighter".

The code here is effectively "when the user has an inverted color scheme how do
we draw text in textfields on a Harmony dialog". But the code in
DefaultTypographyProvider is just ~"what's a good default". And actually, the
decisions that Legacy/DefaultTypographyProvider makes for text are currently
wrong, at least for Windows 10's high contrast themes (but changes in the
"legacy" code I've been trying to do without side-effects).

For example, what usually ends up as the default text color is
"kColorId_LabelEnabledColor" and that uses COLOR_BTNTEXT on Windows. But the
::GetSysColor() documentation and that window that lets you preview the
high-contrast theme in Windows 10 suggest that COLOR_BTNTEXT should *only* be
used for buttons. In high contrast themes COLOR_BTNTEXT is usually white, but
all the other window text can be yellow or green.

I've fed this data into a function here, so we can get started on doing the
right things for high contrast themes. See what you think.

+          style == views::style::STYLE_DISABLED
+              ? ui::NativeTheme::kColorId_TextfieldReadOnlyColor
+              : ui::NativeTheme::kColorId_TextfieldDefaultColor);
+    }
     switch (style) {
       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
@@ skipping 13 matching lines @@
     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;
   }
   return SkColorSetRGB(0x21, 0x21, 0x21);  // Primary for everything else.

[tapted, 2017/06/29 10:29:33]

This is what the text will be otherwise.. I haven't seen a color in any spec for
the text *in* a textfield. But there is an image saying the text in combobox
should be primary, and we do already (in code) say that the color in a combobox
should match the color in a textfield.

 }
 
 int HarmonyTypographyProvider::GetLineHeight(int context, int style) const {
   // "Target" line height constants from the Harmony spec. A default OS
   // configuration should use these heights. However, if the user overrides OS
   // defaults, then GetLineHeight() should return the height that would add the
   // same extra space between lines as the default configuration would have.
   constexpr int kHeadlineHeight = 32;
   constexpr int kTitleHeight = 22;
   constexpr int kBodyHeight = 20;  // For both large and small.
@@ skipping 49 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;
   }
 }