Use style::CONTEXT_TEXTFIELD for Combobox and Textfield

Use style::CONTEXT_TEXTFIELD for Combobox and Textfield

Combobox currently uses all of the folowing text-color constants:

ui::NativeTheme::kColorId_TextfieldDefaultColor
ui::NativeTheme::kColorId_TextfieldReadOnlyColor
ui::NativeTheme::kColorId_LabelEnabledColor
ui::NativeTheme::kColorId_LabelDisabledColor
ui::NativeTheme::kColorId_ButtonEnabledColor

That's too many, and kColorId_ButtonEnabledColor wants to go lighter
under Harmony. Stop using it for Combobox.

Also Textfields under Harmony are currently using Black only, but they
should use the "primary" text color, which is a tiny bit lighter than
black.

Consolidate them both under views::style::CONTEXT_TEXTFIELD and a
corresponding disabled style.

Some tests didn't have a ViewsDelegate. Fix that.

BUG=691891
Review-Url: https://codereview.chromium.org/2964453002
Cr-Commit-Position: refs/heads/master@{#483968}
Committed: https://chromium.googlesource.com/chromium/src/+/1397566bcda02ab4f38737a2bd46a2e013341e0b

[tapted, 2017-06-29 04:23:26]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-29 04:23:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 04:59:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-29 04:59:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-06-29 06:24:26]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-29 06:24:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 07:08:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-29 07:08:27]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tapted, 2017-06-29 07:50:01]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-29 07:50:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 09:14:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-29 09:14:46]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[tapted, 2017-06-29 10:21:50]

Description was changed from

==========
Use style::CONTEXT_TEXTFIELD for Combobox and Textfield

Combobox currently uses all of the folowing text-color constants:

ui::NativeTheme::kColorId_TextfieldDefaultColor
ui::NativeTheme::kColorId_TextfieldReadOnlyColor
ui::NativeTheme::kColorId_LabelEnabledColor
ui::NativeTheme::kColorId_LabelDisabledColor
ui::NativeTheme::kColorId_ButtonEnabledColor

That's too many, and kColorId_ButtonEnabledColor wants to go lighter
under Harmony. Stop using it for Combobox.

Also Textfields under Harmony are currently using Black only, but they
should use the "primary" text color, which is a tiny bit lighter than
black.

Consolidate them both under views::style::CONTEXT_TEXTFIELD and a
corresponding disabled style.

BUG=691891
==========

to

==========
Use style::CONTEXT_TEXTFIELD for Combobox and Textfield

Combobox currently uses all of the folowing text-color constants:

ui::NativeTheme::kColorId_TextfieldDefaultColor
ui::NativeTheme::kColorId_TextfieldReadOnlyColor
ui::NativeTheme::kColorId_LabelEnabledColor
ui::NativeTheme::kColorId_LabelDisabledColor
ui::NativeTheme::kColorId_ButtonEnabledColor

That's too many, and kColorId_ButtonEnabledColor wants to go lighter
under Harmony. Stop using it for Combobox.

Also Textfields under Harmony are currently using Black only, but they
should use the "primary" text color, which is a tiny bit lighter than
black.

Consolidate them both under views::style::CONTEXT_TEXTFIELD and a
corresponding disabled style.

Some tests didn't have a ViewsDelegate. Fix that.

BUG=691891
==========

[tapted, 2017-06-29 10:21:50]

tapted@chromium.org changed reviewers:
+ pkasting@chromium.org

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

[2/4] PTAL. (This one is orthogonal). Thanks!

https://codereview.chromium.org/2964453002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/harmony/harmony_typography_provider.cc (right):

https://codereview.chromium.org/2964453002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/harmony_typography_provider.cc:108: return
SkColorSetRGB(0x21, 0x21, 0x21);  // Primary for everything else.
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.

https://codereview.chromium.org/2964453002/diff/40001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/2964453002/diff/40001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:99: return
style::GetColor(style::CONTEXT_TEXTFIELD,
This consolidates the two colors, which I think is fine. Linux,Mac,ChromeOS it
should be black. Windows 10 only distinguishes button text colours from other
text in its high contrast themes.

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

LGTM

https://codereview.chromium.org/2964453002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/harmony/harmony_typography_provider.cc (right):

https://codereview.chromium.org/2964453002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/harmony_typography_provider.cc:70: return
theme.GetSystemColor(
Can we just return TypographyProvider::GetColor(...) here?  This looks like a
duplication of that code, and that seems like less of a maintenance risk.

https://codereview.chromium.org/2964453002/diff/40001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2964453002/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:389: read_only() || !enabled() ?
style::STYLE_DISABLED : style::STYLE_PRIMARY;
Nit: Please put parens around the condition before the ?: here, precedence on
things like ? isn't something I know offhand.

[tapted, 2017-06-30 05:42:25]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-30 05:42:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-06-30 05:45:39]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-30 05:45:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-06-30 05:49:33]

Patchset #5 (id:80001) has been deleted

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

https://codereview.chromium.org/2964453002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/harmony/harmony_typography_provider.cc (right):

https://codereview.chromium.org/2964453002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/harmony_typography_provider.cc:70: return
theme.GetSystemColor(
On 2017/06/29 20:24:57, Peter Kasting wrote:
> Can we just return TypographyProvider::GetColor(...) here?  This looks like a
> duplication of that code, and that seems like less of a maintenance risk.


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.

https://codereview.chromium.org/2964453002/diff/40001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2964453002/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:389: read_only() || !enabled() ?
style::STYLE_DISABLED : style::STYLE_PRIMARY;
On 2017/06/29 20:24:57, Peter Kasting wrote:
> Nit: Please put parens around the condition before the ?: here, precedence on
> things like ? isn't something I know offhand.

Done.

[commit-bot: I haz the power, 2017-06-30 06:52:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-30 06:52:23]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2017-06-30 08:36:24]

LGTM

[tapted, 2017-07-03 04:11:18]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-03 04:11:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-07-03 05:16:32]

Patchset #6 (id:120001) has been deleted

[tapted, 2017-07-03 08:08:54]

The CQ bit was unchecked by tapted@chromium.org

[tapted, 2017-07-03 08:09:01]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2017-07-03 08:09:10]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-03 09:19:34]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1499069341342960,
"parent_rev": "2f0bd13b3cf5ac9b2794dc4b30f3d3bb5271c8f6", "commit_rev":
"1397566bcda02ab4f38737a2bd46a2e013341e0b"}

[commit-bot: I haz the power, 2017-07-03 09:19:49]

Description was changed from

==========
Use style::CONTEXT_TEXTFIELD for Combobox and Textfield

Combobox currently uses all of the folowing text-color constants:

ui::NativeTheme::kColorId_TextfieldDefaultColor
ui::NativeTheme::kColorId_TextfieldReadOnlyColor
ui::NativeTheme::kColorId_LabelEnabledColor
ui::NativeTheme::kColorId_LabelDisabledColor
ui::NativeTheme::kColorId_ButtonEnabledColor

That's too many, and kColorId_ButtonEnabledColor wants to go lighter
under Harmony. Stop using it for Combobox.

Also Textfields under Harmony are currently using Black only, but they
should use the "primary" text color, which is a tiny bit lighter than
black.

Consolidate them both under views::style::CONTEXT_TEXTFIELD and a
corresponding disabled style.

Some tests didn't have a ViewsDelegate. Fix that.

BUG=691891
==========

to

==========
Use style::CONTEXT_TEXTFIELD for Combobox and Textfield

Combobox currently uses all of the folowing text-color constants:

ui::NativeTheme::kColorId_TextfieldDefaultColor
ui::NativeTheme::kColorId_TextfieldReadOnlyColor
ui::NativeTheme::kColorId_LabelEnabledColor
ui::NativeTheme::kColorId_LabelDisabledColor
ui::NativeTheme::kColorId_ButtonEnabledColor

That's too many, and kColorId_ButtonEnabledColor wants to go lighter
under Harmony. Stop using it for Combobox.

Also Textfields under Harmony are currently using Black only, but they
should use the "primary" text color, which is a tiny bit lighter than
black.

Consolidate them both under views::style::CONTEXT_TEXTFIELD and a
corresponding disabled style.

Some tests didn't have a ViewsDelegate. Fix that.

BUG=691891

Review-Url: https://codereview.chromium.org/2964453002
Cr-Commit-Position: refs/heads/master@{#483968}
Committed:
https://chromium.googlesource.com/chromium/src/+/1397566bcda02ab4f38737a2bd46...
==========

[commit-bot: I haz the power, 2017-07-03 09:19:50]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/1397566bcda02ab4f38737a2bd46...