Material Design: Fix MdLabelButton not updating text colors on theme changes

Material Design: Fix MdLabelButton not updating text colors on theme changes

MdTextButton previously had this code:

if (!explicitly_set_normal_color())
    LabelButton::SetEnabledTextColors(theme->GetSystemColor(fg_color_id));

The call to SetEnabledTextColors() caused subsequent calls to
explicitly_set_normal_color() to be true, even though the colors were
inferred from the native theme.  When using the Gtk3 theme, when
Chrome first starts, GetNativeTheme() doesn't return an instance of
NativeThemeGtk3, so when the initial text color is set, it's using an
old theme.  This was causing the "Set Chrome as default" button to
have incorrect colors (but for some reason the background had the
right color).

The fix in this CL is to reset explicitly_set_text_colors_ after
SetEnabledTextColors().

R=estade@chromium.org
CC=erg@chromium.org

Review-Url: https://codereview.chromium.org/2620453005
Cr-Commit-Position: refs/heads/master@{#442463}
Committed: https://chromium.googlesource.com/chromium/src/+/532dbca20c3c49d6f543e6fe137f8b7e21832bb1

[Tom (Use chromium acct), 2017-01-06 21:45:43]

estade@ PTAL.  This is a bit of a hack

erg@ FYI

[Evan Stade, 2017-01-06 22:09:44]

I think you should copy this[1], i.e. unset the explicitly set flag right after
setting the color.

[1]
https://cs.chromium.org/chromium/src/ui/views/controls/button/label_button.cc...

[Tom (Use chromium acct), 2017-01-07 01:12:44]

On 2017/01/06 22:09:44, Evan Stade wrote:
> I think you should copy this[1], i.e. unset the explicitly set flag right
after
> setting the color.
> 
> [1]
>
https://cs.chromium.org/chromium/src/ui/views/controls/button/label_button.cc...

Done

[Evan Stade, 2017-01-07 01:31:40]

https://codereview.chromium.org/2620453005/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/2620453005/diff/20001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:381: for (ButtonState state :
{STATE_NORMAL, STATE_HOVERED, STATE_PRESSED})
hmm, this is not quite as I imagined, because STATE_DISABLED is missing. that is
what you want that for the MdTextButton case but perhaps not for the general
case. It would be nice to reuse this at least in two places (MdTextButton and
below in ResetColorsFromNativeTheme).

Options that come to mind:
a) create a SetTextColorInternal that does everything except flip the explicit
bit, or
b) create a |setting_text_colors_from_theme_| boolean which, when true, disables
flipping the explicit bit
c) expose explicitly_set_colors() and set_explicitly_set_colors(), then do:

auto old_explicitly_set_colors = explicitly_set_colors();
SetEnabledTextColors(...);
set_explicitly_set_colors(old_explicitly_set_colors);

[Tom (Use chromium acct), 2017-01-07 02:34:33]

Description was changed from

==========
Material Design: Fix MdLabelButton not updating text colors on theme changes

MdTextButton previously had this code:

if (!explicitly_set_normal_color())
    LabelButton::SetEnabledTextColors(theme->GetSystemColor(fg_color_id));

The call to SetEnabledTextColors() caused subsequent calls to
explicitly_set_normal_color() to be true, even though the colors were
inferred from the native theme.  When using the Gtk3 theme, when
Chrome first starts, GetNativeTheme() doesn't return an instance of
NativeThemeGtk3, so when the initial text color is set, it's using an
old theme.  This was causing the "Set Chrome as default" button to
have incorrect colors (but for some reason the background had the
right color).

The fix in this CL is to add |explicitly_set_normal_color_| for
MdTextButton to track if the text color was overridden.

R=estade@chromium.org
CC=erg@chromium.org
==========

to

==========
Material Design: Fix MdLabelButton not updating text colors on theme changes

MdTextButton previously had this code:

if (!explicitly_set_normal_color())
    LabelButton::SetEnabledTextColors(theme->GetSystemColor(fg_color_id));

The call to SetEnabledTextColors() caused subsequent calls to
explicitly_set_normal_color() to be true, even though the colors were
inferred from the native theme.  When using the Gtk3 theme, when
Chrome first starts, GetNativeTheme() doesn't return an instance of
NativeThemeGtk3, so when the initial text color is set, it's using an
old theme.  This was causing the "Set Chrome as default" button to
have incorrect colors (but for some reason the background had the
right color).

The fix in this CL is to reset explicitly_set_text_colors_ after
SetEnabledTextColors().

R=estade@chromium.org
CC=erg@chromium.org
==========

[Tom (Use chromium acct), 2017-01-07 02:35:11]

https://codereview.chromium.org/2620453005/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/2620453005/diff/20001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:381: for (ButtonState state :
{STATE_NORMAL, STATE_HOVERED, STATE_PRESSED})
On 2017/01/07 01:31:40, Evan Stade wrote:
> hmm, this is not quite as I imagined, because STATE_DISABLED is missing. that
is
> what you want that for the MdTextButton case but perhaps not for the general
> case. It would be nice to reuse this at least in two places (MdTextButton and
> below in ResetColorsFromNativeTheme).
> 
> Options that come to mind:
> a) create a SetTextColorInternal that does everything except flip the explicit
> bit, or
> b) create a |setting_text_colors_from_theme_| boolean which, when true,
disables
> flipping the explicit bit
> c) expose explicitly_set_colors() and set_explicitly_set_colors(), then do:
> 
> auto old_explicitly_set_colors = explicitly_set_colors();
> SetEnabledTextColors(...);
> set_explicitly_set_colors(old_explicitly_set_colors);

Done.  I liked (c) the most so I went with that

[Tom (Use chromium acct), 2017-01-09 19:20:44]

pinging estade@

[Evan Stade, 2017-01-09 20:02:29]

estade@chromium.org changed reviewers:
+ sky@chromium.org

[Evan Stade, 2017-01-09 20:02:30]

+sky

https://codereview.chromium.org/2620453005/diff/40001/ui/views/controls/butto...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/2620453005/diff/40001/ui/views/controls/butto...
ui/views/controls/button/label_button.h:154: std::array<bool, STATE_COUNT>
explicitly_set_colors() const {
should this return a const ref

https://codereview.chromium.org/2620453005/diff/40001/ui/views/controls/butto...
ui/views/controls/button/label_button.h:157: void
set_explicitly_set_colors(std::array<bool, STATE_COUNT> colors) {
const ref

[Tom (Use chromium acct), 2017-01-09 20:31:50]

https://codereview.chromium.org/2620453005/diff/40001/ui/views/controls/butto...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/2620453005/diff/40001/ui/views/controls/butto...
ui/views/controls/button/label_button.h:154: std::array<bool, STATE_COUNT>
explicitly_set_colors() const {
On 2017/01/09 20:02:30, Evan Stade wrote:
> should this return a const ref

Done.

https://codereview.chromium.org/2620453005/diff/40001/ui/views/controls/butto...
ui/views/controls/button/label_button.h:157: void
set_explicitly_set_colors(std::array<bool, STATE_COUNT> colors) {
On 2017/01/09 20:02:30, Evan Stade wrote:
> const ref

Done.

[sky, 2017-01-09 22:23:53]

LGTM

[Evan Stade, 2017-01-09 22:47:52]

lgtm

[Tom (Use chromium acct), 2017-01-10 00:22:21]

The CQ bit was checked by thomasanderson@google.com

[commit-bot: I haz the power, 2017-01-10 00:23:24]

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-01-10 03:13:38]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484007741064350,
"parent_rev": "e1a86a9954e04e149616c31c201644c52021e6df", "commit_rev":
"532dbca20c3c49d6f543e6fe137f8b7e21832bb1"}

[commit-bot: I haz the power, 2017-01-10 03:14:15]

Description was changed from

==========
Material Design: Fix MdLabelButton not updating text colors on theme changes

MdTextButton previously had this code:

if (!explicitly_set_normal_color())
    LabelButton::SetEnabledTextColors(theme->GetSystemColor(fg_color_id));

The call to SetEnabledTextColors() caused subsequent calls to
explicitly_set_normal_color() to be true, even though the colors were
inferred from the native theme.  When using the Gtk3 theme, when
Chrome first starts, GetNativeTheme() doesn't return an instance of
NativeThemeGtk3, so when the initial text color is set, it's using an
old theme.  This was causing the "Set Chrome as default" button to
have incorrect colors (but for some reason the background had the
right color).

The fix in this CL is to reset explicitly_set_text_colors_ after
SetEnabledTextColors().

R=estade@chromium.org
CC=erg@chromium.org
==========

to

==========
Material Design: Fix MdLabelButton not updating text colors on theme changes

MdTextButton previously had this code:

if (!explicitly_set_normal_color())
    LabelButton::SetEnabledTextColors(theme->GetSystemColor(fg_color_id));

The call to SetEnabledTextColors() caused subsequent calls to
explicitly_set_normal_color() to be true, even though the colors were
inferred from the native theme.  When using the Gtk3 theme, when
Chrome first starts, GetNativeTheme() doesn't return an instance of
NativeThemeGtk3, so when the initial text color is set, it's using an
old theme.  This was causing the "Set Chrome as default" button to
have incorrect colors (but for some reason the background had the
right color).

The fix in this CL is to reset explicitly_set_text_colors_ after
SetEnabledTextColors().

R=estade@chromium.org
CC=erg@chromium.org

Review-Url: https://codereview.chromium.org/2620453005
Cr-Commit-Position: refs/heads/master@{#442463}
Committed:
https://chromium.googlesource.com/chromium/src/+/532dbca20c3c49d6f543e6fe137f...
==========

[commit-bot: I haz the power, 2017-01-10 03:14:16]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/532dbca20c3c49d6f543e6fe137f...