views: support disabled color for CTA buttons

views: support disabled color for CTA buttons

Disabled buttons use their usual background at 0.38a, so this change
adds a new color ID for their faded background and uses it in
MdTextButton::UpdateColors().

BUG=644543
Committed: https://crrev.com/17e42fe9291fd349c074d84cc6ddf994dc9a98bb
Cr-Commit-Position: refs/heads/master@{#417263}

[Elly Fong-Jones, 2016-09-07 11:48:47]

Description was changed from

==========
views: support disabled color for CTA buttons

Disabled buttons use their usual background at 0.38a, so this change
adds a new color ID for their faded background and uses it in
MdTextButton::UpdateColors().

BUG=644543
==========

to

==========
views: support disabled color for CTA buttons

Disabled buttons use their usual background at 0.38a, so this change
adds a new color ID for their faded background and uses it in
MdTextButton::UpdateColors().

BUG=644543
==========

[Elly Fong-Jones, 2016-09-07 11:48:48]

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

[Elly Fong-Jones, 2016-09-07 11:48:54]

estade: ptal? :)

[Evan Stade, 2016-09-07 16:00:35]

https://codereview.chromium.org/2315233002/diff/1/ui/native_theme/native_them...
File ui/native_theme/native_theme_dark_aura.cc (right):

https://codereview.chromium.org/2315233002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_dark_aura.cc:46: return
SkColorSetA(gfx::kGoogleBlue300, 255.0 * 0.38);
I think it makes sense to keep this in MdTextButton unless we're going to use it
somewhere else. Since presumably all native themes want the disabled CTA color
to be enabled with 0.38a (normally we use hex, so 0x61), it's simpler to avoid
adding a new color id and copying the constant in a lot of places, and instead
just stick a hard-coded alpha in MdTextButton.

[Elly Fong-Jones, 2016-09-07 16:59:59]

done, ptal? :)

https://codereview.chromium.org/2315233002/diff/1/ui/native_theme/native_them...
File ui/native_theme/native_theme_dark_aura.cc (right):

https://codereview.chromium.org/2315233002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_dark_aura.cc:46: return
SkColorSetA(gfx::kGoogleBlue300, 255.0 * 0.38);
On 2016/09/07 16:00:35, Evan Stade wrote:
> I think it makes sense to keep this in MdTextButton unless we're going to use
it
> somewhere else. Since presumably all native themes want the disabled CTA color
> to be enabled with 0.38a (normally we use hex, so 0x61), it's simpler to avoid
> adding a new color id and copying the constant in a lot of places, and instead
> just stick a hard-coded alpha in MdTextButton.

Done.

[Evan Stade, 2016-09-07 17:14:33]

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

https://codereview.chromium.org/2315233002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/md_text_button.cc:304: bg_color = SkColorSetA(bg_color,
0x61);
I know it's expressed as an alpha value in the spec, but I'm thinking the blend
towards opposite luma (like below) is somewhat more correct here than setting an
alpha because we probably don't want the color to change based on the
background. For example, if we had a disabled CTA button on an infobar with a
yellow bg, would we want the button to be somewhat yellow? probably not.

https://codereview.chromium.org/2315233002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/md_text_button.cc:309: bg_color =
PlatformStyle::BackgroundColorForMdButton(bg_color, state());
can we make it a todo to get rid of this? Color logic is currently spread across
three places: this file, native theme, and platform style :<

perhaps:

if (pressed()) {
  SkColor shade_color = theme->GetSystemColor(kColorId_ButtonPressedShade);
  bg_color = color_utils::Blend(shade_color, bg_color);
}

(note that AlphaBlend doesn't do quite what we want here so we have to invent a
new utility function. I'm pretty sure I've wanted this function in the past but
worked around its non-existence so this won't be the only time we'd use it.)

[Elly Fong-Jones, 2016-09-07 17:43:50]

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

https://codereview.chromium.org/2315233002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/md_text_button.cc:309: bg_color =
PlatformStyle::BackgroundColorForMdButton(bg_color, state());
On 2016/09/07 17:14:33, Evan Stade wrote:
> can we make it a todo to get rid of this? Color logic is currently spread
across
> three places: this file, native theme, and platform style :<
> 
> perhaps:
> 
> if (pressed()) {
>   SkColor shade_color = theme->GetSystemColor(kColorId_ButtonPressedShade);
>   bg_color = color_utils::Blend(shade_color, bg_color);
> }
> 
> (note that AlphaBlend doesn't do quite what we want here so we have to invent
a
> new utility function. I'm pretty sure I've wanted this function in the past
but
> worked around its non-existence so this won't be the only time we'd use it.)

Hm - I like this idea, but what would color_utils::Blend() do? As I understand
it there are many different ways to blend two colors together.

Also, I think for this specific use, kColorId_ButtonPressedShade would be
#FFFFFF with an alpha of #08, and we need a function that would:

1) Extract the alpha, and set the shade's alpha to #FF
2) Set the bg color's alpha to #FF
3) Alpha blend the two with the extracted alpha from step 1

Is that the kind of thing you're thinking of? I don't understand color that
well.

[Elly Fong-Jones, 2016-09-07 18:01:45]

estade: ptal? :)

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

https://codereview.chromium.org/2315233002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/md_text_button.cc:304: bg_color = SkColorSetA(bg_color,
0x61);
On 2016/09/07 17:14:33, Evan Stade wrote:
> I know it's expressed as an alpha value in the spec, but I'm thinking the
blend
> towards opposite luma (like below) is somewhat more correct here than setting
an
> alpha because we probably don't want the color to change based on the
> background. For example, if we had a disabled CTA button on an infobar with a
> yellow bg, would we want the button to be somewhat yellow? probably not.

Done.

https://codereview.chromium.org/2315233002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/md_text_button.cc:309: bg_color =
PlatformStyle::BackgroundColorForMdButton(bg_color, state());
On 2016/09/07 17:43:50, Elly Jones wrote:
> On 2016/09/07 17:14:33, Evan Stade wrote:
> > can we make it a todo to get rid of this? Color logic is currently spread
> across
> > three places: this file, native theme, and platform style :<
> > 
> > perhaps:
> > 
> > if (pressed()) {
> >   SkColor shade_color = theme->GetSystemColor(kColorId_ButtonPressedShade);
> >   bg_color = color_utils::Blend(shade_color, bg_color);
> > }
> > 
> > (note that AlphaBlend doesn't do quite what we want here so we have to
invent
> a
> > new utility function. I'm pretty sure I've wanted this function in the past
> but
> > worked around its non-existence so this won't be the only time we'd use it.)
> 
> Hm - I like this idea, but what would color_utils::Blend() do? As I understand
> it there are many different ways to blend two colors together.
> 
> Also, I think for this specific use, kColorId_ButtonPressedShade would be
> #FFFFFF with an alpha of #08, and we need a function that would:
> 
> 1) Extract the alpha, and set the shade's alpha to #FF
> 2) Set the bg color's alpha to #FF
> 3) Alpha blend the two with the extracted alpha from step 1
> 
> Is that the kind of thing you're thinking of? I don't understand color that
> well.

I ended up adding a TODO for this.

[Evan Stade, 2016-09-07 18:07:01]

lgtm

[Elly Fong-Jones, 2016-09-07 18:27:05]

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

[Elly Fong-Jones, 2016-09-07 18:27:10]

sky: ptal? :)

[sky, 2016-09-07 21:24:21]

LGTM

[Elly Fong-Jones, 2016-09-08 10:30:48]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2016-09-08 10:31:23]

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, 2016-09-08 10:32:46]

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

[commit-bot: I haz the power, 2016-09-08 10:32:47]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Elly Fong-Jones, 2016-09-08 10:43:50]

The CQ bit was checked by ellyjones@chromium.org

[Elly Fong-Jones, 2016-09-08 10:43:50]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2315233002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-09-08 10:44:03]

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, 2016-09-08 12:16:55]

Description was changed from

==========
views: support disabled color for CTA buttons

Disabled buttons use their usual background at 0.38a, so this change
adds a new color ID for their faded background and uses it in
MdTextButton::UpdateColors().

BUG=644543
==========

to

==========
views: support disabled color for CTA buttons

Disabled buttons use their usual background at 0.38a, so this change
adds a new color ID for their faded background and uses it in
MdTextButton::UpdateColors().

BUG=644543
==========

[commit-bot: I haz the power, 2016-09-08 12:16:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-08 12:18:35]

Description was changed from

==========
views: support disabled color for CTA buttons

Disabled buttons use their usual background at 0.38a, so this change
adds a new color ID for their faded background and uses it in
MdTextButton::UpdateColors().

BUG=644543
==========

to

==========
views: support disabled color for CTA buttons

Disabled buttons use their usual background at 0.38a, so this change
adds a new color ID for their faded background and uses it in
MdTextButton::UpdateColors().

BUG=644543

Committed: https://crrev.com/17e42fe9291fd349c074d84cc6ddf994dc9a98bb
Cr-Commit-Position: refs/heads/master@{#417263}
==========

[commit-bot: I haz the power, 2016-09-08 12:18:36]

Patchset 4 (id:??) landed as
https://crrev.com/17e42fe9291fd349c074d84cc6ddf994dc9a98bb
Cr-Commit-Position: refs/heads/master@{#417263}