Nit: It looks like the "expected" grey there is drawn as #D2D2D2, but I compute #000 0.2 as #CCCCCC.

Sending this to Allen; Trent might be a logical choice since he should be working on text colors soon and this is another color issue, but he's pretty stacked up, and Allen is working on button-related stuff right now anyway.

This is what I'm currently seeing WRT the hung renderer dialog. The "Kill" button certainly looks lighter than the above screenshot... although not quite as light (it's close) as the expected.

Deleted: HungRenderer.png 6.7 KB

	HungRenderer.png 6.7 KB View Download

Note r483954 should have resolved the text color since this bug was filed.

Screenshots at  http://crbug.com/691891#c28  e.g. https://bugs.chromium.org/p/chromium/issues/attachment?aid=291001&inline=1

That screenshot has:

Enabled button stroke #d5d5d5
Disabled button stroke #cecece

Spec ( https://bugs.chromium.org/p/chromium/issues/attachment?aid=290994&inline=1 ) wants

Enabled button stroke #cccccc
Disabled button stroke #e6e6e6

(I made a jsfiddle since I've been finding myself doing a lot of manual alpha compositing lately - https://jsfiddle.net/tapted/rk9cfqkj/ )


The MD button code tries to apply logic. IMO it should just use constants. To me, changing colours always feels harder that it should be with all these layers and logic.


  // Specified text color: 5a5a5a @ 1.0 alpha
  // Specified stroke color: 000000 @ 0.2 alpha
  // 000000 @ 0.2 is very close to 5a5a5a @ 0.308 (== 0x4e); both are cccccc @
  // 1.0, and this way if NativeTheme changes the button color, the button
  // stroke will also change colors to match.
  SkColor stroke_color =
      is_prominent_ ? SK_ColorTRANSPARENT : SkColorSetA(text_color, 0x4e);

  // Disabled, non-prominent buttons need their stroke lightened. Prominent
  // buttons need it left at SK_ColorTRANSPARENT from above.
  if (state() == STATE_DISABLED && !is_prominent_) {
    stroke_color = color_utils::BlendTowardOppositeLuma(
        stroke_color, gfx::kDisabledControlAlpha);
  }


If we scrap the alpha channel and just use #cccccc or #e6e6e6 it will probably look fine on both inverted and normal color schemes.

How should I proceed on this? From what was said, the code should just assign the text_color to stroke_color (unless it's a prominent button)? What about the case where NativeTheme changes the button color per the comment?

In HarmonyTypographyProvider::GetColor, it looks like it's returning 757575:

  if (context == views::style::CONTEXT_BUTTON_MD) {
    switch (style) {
      case views::style::STYLE_DIALOG_BUTTON_DEFAULT:
        return SK_ColorWHITE;
      case views::style::STYLE_DISABLED:
        return SkColorSetRGB(0x9e, 0x9e, 0x9e);
      default:
        return SkColorSetRGB(0x75, 0x75, 0x75);
    }
  }

Should this change to 5a5a5a? Where did the 757575 value come from?

In theory the text color should be visible over whatever the background button color is.  In practice I'm not sure that's true with the typography provider code today, but if not, it's a bug we have to fix there anyway.  So using the text color for the stroke color seems OK to me even when the NativeTheme changes the button color.

The #757575 value came from some spec.  I asked Trent about this when he checked it in and I guess the button spec he's working from is newer than the one I have or something.

So using Trent's jsfiddle code, it converts 757575 @ 0.308 (0x4e) to d6d6d6... that's aways away from cccccc and is a little darker. I wonder if the alpha should be changed when calculating the stroke color? Using 757575 @ 0.47 (0x78) would convert to bfbfbf.

Oh, I didn't see that the spec actually has different colors for these (well, different alphas).

Bah.  I suggested that the typography provider actually return transparent colors (e.g. #000 @0.54) as the spec suggests rather than opaque ones.  Trent didn't like that idea, but I'm wondering if it makes sense here, since we could basically say something like "the stroke is the text color with a different alpha) (either "0.2a" or "37% of the original a").

Yup - 757575 is from "2017-06-28-SPEC-secondary-UI-03-buttons.png" on  Issue 691891  . Relevant bit attached (also linked in #c4).

I'm super wary of transparency.. On an inverted/high-contrast color scheme, the dialog background will be black and transparency of 0.1 or 0.2 will be virtually invisible, causing Chrome to fail at the whole "high contrast" thing that these themes often get picked for.

It might be more reasonable to use a premultiplied color and just remove the alpha component. E.g. SkColorSetA(SkPreMultiplyColor(SkColorSetA(text_color, alpha)), 0xff)   (I think that works..)

But also if you consider that any divergence from the Harmony spec could be due to a high-contrast theme, we might just want to use those colors directly rather than reducing their contrast.

Deleted: Screen Shot 2017-07-13 at 10.26.52 am.png 16.6 KB

	Screen Shot 2017-07-13 at 10.26.52 am.png 16.6 KB View Download

Right, things like inverted color schemes are why I get very leery of hardcoded black/white values and instead try to convince people to do everything in terms of system/theme colors and blends between them.  Using "system text color" instead of #000, even if tweaking the alpha, is way safer.

If the correct base color is picked, colors with alpha should actually be easier to "get right" than fully-opaque colors, since you don't have to know what you're blending against to compute the blend.

If I turn off font smoothing, screen-shot the dialog, zoom-in and then eye-dropper the text... I get 117,117,117 which is 0x757575.

So the text in the red square is, in fact, the required 0x757575.

The stroke, however, around the button is 0xD5D5D5. That's as expected based on setting the alpha for 0x757575 to 0x4e (.308) and the jsfiddle code confirms it.

What am I missing here? Is there anything that we need to do? Should the alpha change for the stroke? What about the text color?

Deleted: TextColor.png 4.9 KB

	TextColor.png 4.9 KB View Download

The easiest thing to do would be to change that 0.308 to something like 0.37.

The next possible thing to try would be to have TypographyProvider return TEXT_COLOR @0.54, and simply change its alpha to 0.2.  This would require TEXT_COLOR to be something known to contrast against the background, i.e. not just hardcoded #000.  Long term this seems better to me, but it could be a long thread to pull.

This one changes the .308 to .37 (0x5f). It results in 0xcbcbcb, pretty close to 0xcccccc.

As for changing to only using the alpha, I think Trent has a strong point about high-contrast.

As I'm not a color/text theory specialist, I'm just spit-balling here... There are places where color_utils::BlendTowardOppositeLuma is used for the disabled state. Could that not be used if we can detect that the contrast is below a certain threshold? We should not explicitly look for a high-contrast mode since, I presume, a theme could also be high-contrast merely by the colors it uses.

Deleted: StrokeColor.png 7.4 KB

	StrokeColor.png 7.4 KB View Download

You are correct that you can't just look for a high-contrast bit being set.

The biggest challenge in picking colors is in places where you don't know what one is -- for example, if you're asked to do the text color for any arbitrary background color, it's impossible to pick a good one.

As noted before, just changing alpha requires that first we be consistent about doing things in terms of specific native or theme colors (which may then be blended), and not doing anything as random color values from a spec.  I'd like to go there some day, I don't want to go there for Harmony phase 1.

Feel free to tweak that 0.37 value, I didn't actually compute the right thing that would be equivalent to "#000 0.2".

Maybe the TypographyProvider should return a "stroke alpha" where it'd be 0x4e by default and 0x5f from the HarmonyTypographyProvider? Just changing the 0x4e to 0x5f would make the non-Harmony mode wrong; 0x5a5a5a @ 0x5f would be 0xc2c2c2... may it's not a very perceptible difference?

This brings me to another point; The HarmonyTypographyProvider is returning hard-coded color values, whereas the default TypographyProvider interrogates the theme. Why would these colors not come from the default theme? Could we not create a "Harmony" theme and switch that in for the default under Harmony mode? Of course my ignorance here is probably clearly on display.

We could create a stroke alpha if we really cared, but it's not really a "typography" issue, it's more a theming thing.  For now, if you want to have them differ, I'd do this via a direct check of Harmony mode in the button code.  If we get more of these, it will more clearly inform how we want to structure the abstractions to serve it.

As for themes -- you're kinda wading into an argument Trent and I were having :).  I think there's some potential to improve this, but I don't think we know what we want enough yet, and the existing theme system is (for better or worse) very complicated, so I'd prefer to do expedient things for the moment and try and refactor them once it's more clear what we want.

OK, I'll refrain from poking that bear for now :).

The "Kill" button in this shot is disabled under Harmony. As you can see, the text and stroke colors are, in fact, darker. This is with using GetResultingPaintColor() in BlendTowardOppositeLuma().

The stroke color ends up being 0x7d7d7d and the text color is 0x6d6d6d.

Deleted: DisabledTextStrokeColor.png 5.5 KB

	DisabledTextStrokeColor.png 5.5 KB View Download

I'm confused.  Why is the text #6d6d6d?  Shouldn't it be unchanged from comment 15?

Also I realized that what we'd really want for the stroke is BlendTowardSameLuma (which doesn't yet exist, but could be trivially added).

Since the button is disabled, it should be 0x9e9e9e... that's what is strange. I've been trying to track down why it's 0x6d6d6d. While debugging, I see that the color set on the button label is 0x9e9e9e. However it ends up as 0x6d6d6d when rendered.

The stroke color is always based off the enabled text color.

Could it be due to Label::SetAutoColorReadabilityEnabled(..) ?

(note I think we need to keep that for the general case. If we need it to be disabled for Harmony, it may suggest that the spec fails GAR, at least for disabled buttons - maybe there's an exception for that, but I am not really an a11y person).

Note I'm not vehemently opposed to BlendTowardFooLuma .. I think we need it for Chrome themes (e.g. downloads bar). But one consideration could be that we don't actually have any disabled buttons that are subject to Chrome themes, so we're adding complexity for a button that doesn't actually exist [note: maybe we do need this - I'm just guessing].

But if we want BlendTowardFooLuma to work for Harmony with its light text shades, we'd probably want to pick the direction based on whether the background IsDark() rather than the whether the text color IsDark() (and use FooLuma=SameLuma as peter suggests).


If we can ignore Chrome themes, I think any situation where the background is other than solid white or black should be super rare, and transparency will work fine.

That sounds like a plausible guess on why the color is off.  The contrast ratio of #9e9e9e against #ffffff is 2.7.  I'd think lightness-inverting would give something like #616161 rather than #6d6d6d, but either one is higher-contrast with the background.

The minimum contrast ratio for non-disabled text is 4.5.  Disabled text is not required to hit minimum contrast ratios (though the MD specs advise using an alternate indicator if it doesn't).  This suggests maybe two changes:

(1) Disable auto color readability on disabled buttons (and anything else with text when in a DISABLED state)
(2) As Trent notes, either blend the text color toward white/black based on the background color's darkness, or just shrug and use transparency here.  I'd personally do the former because it doesn't seem much harder than doing the latter (a few extra minutes) and it's more future-proof.

Plus added bonus that I noticed while thinking about this, but isn't necessary here:
(3) Adjust either GetReadableColor() or PickContrastingColor() (depending on which is used for what) to not muck with the input color when the contrast ratio is at least 4.5; that's "readable enough" already

Label::SetAutoColorReadabilityEnabled(..) is set to false, so that should not be the problem. It's explicitly set to false in LabelButton::ResetColorsFromNativeTheme().

I think I may have figured out where the 0x6d6d6d is coming from. It's getting that color from the theme.

There is a bug in the manner in which we're interrogating the TypographyProvider for text colors. See LabelButton::ResetColorsFromNativeTheme() and then LabelButton::SetTextColor();

If the LabelButtonLabel is actually disabled, LabelButtonLabel::SetDisabledColor() will overwrite the "enabled color" (the names of the methods here are really confusing... but that's a different discussion).

Ahh. hm. I'll poke at this a bit today.

Yes - Label::enabled_color is confusing -- it should just be renamed to text_color - that's on me since I hid disabled colors from views::Label so that UI implementers would stop making disabled labels just to get grey text :|

It's possible I inadvertently changed the stroke for disabled buttons in a patchset revision on r481374.

The screenshots I took for the button text color update (r483954) in  http://crbug.com/691891#c28  show a correct *text* color for disabled buttons (and that's after the suspect revision, r481374). It's 0x9f9f9f there for disabled text, which matches the spec (0xff at 0.38a). However, the button stroke does appear to be getting darker in those screengrabs, rather than lighter.


I agree that LabelButton::ResetColorsFromNativeTheme() looks wrong for MD buttons.  MdTextButton::UpdateColors() clobbers all the enabled-state colors set in LabelButton::ResetColorsFromNativeTheme() when it calls LabelButton::SetEnabledTextColors. However, it doesn't clobber the disabled color. I think we can just do that in MdTextButton::UpdateColors(), then at least things are consistent.

(none of that explains how I was able to get a button with the right color in the screenshots in  http://crbug.com/691891#c28  though ;).

screengrab for https://chromium-review.googlesource.com/c/571174/#message-3c8dd16d0da47ccd8c2aefdaa849583d3f4574bd

(Edit button is disabled - the eyedropper cursor is over the disabled button stroke circled)

Deleted: disabled_md.png 75.8 KB

	disabled_md.png 75.8 KB View Download

With the suggested changes, this is now what I get. The text is now 0x9e9e9e and the stroke is 0xe6e6e6.

Deleted: MaterialDisabledButton.png 5.4 KB

	MaterialDisabledButton.png 5.4 KB View Download

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c45d868e83ca45608d74929b203a4cc9b9ce9a91

commit c45d868e83ca45608d74929b203a4cc9b9ce9a91
Author: Allen Bauer <kylixrd@chromium.org>
Date: Fri Jul 21 18:48:35 2017

Change alpha for stroke to 0x5f.

Bug:  737271 
Change-Id: Ic87eed4db221b93d4aca8d7bead66a4208b4b53f
Reviewed-on: https://chromium-review.googlesource.com/571174
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488722}
[modify] https://crrev.com/c45d868e83ca45608d74929b203a4cc9b9ce9a91/ui/views/controls/button/md_text_button.cc

 Issue 658351  has been merged into this issue.

It looks as if maybe the stroke should actually be 0.3a for non-disabled buttons and the spec wasn't updated?  See  bug 660414 .

Let's call this P1 since it's almost done (I hope) and fixing it should improve the compliance of lots of dialogs.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/867e878f0a6a6aa940e3851f3df93cff54d4ea37

commit 867e878f0a6a6aa940e3851f3df93cff54d4ea37
Author: Allen Bauer <kylixrd@chromium.org>
Date: Wed Aug 02 18:47:58 2017

Fix disabled stroke color processing to ensure the non-Harmony disabled color is properly lightened.

Bug:  737271 
Change-Id: Ie6bb767b8138472037ffb685091cab8c8986b662
Reviewed-on: https://chromium-review.googlesource.com/581630
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491451}
[modify] https://crrev.com/867e878f0a6a6aa940e3851f3df93cff54d4ea37/ui/views/controls/button/md_text_button.cc