Remove COLOR_STATUS_BAR_TEXT.

Remove COLOR_STATUS_BAR_TEXT.

This can be dynamically computed.

BUG=none
TEST=none

Committed: https://crrev.com/9355b054f37315e5825faf9068dee0d16ba32b74
Cr-Commit-Position: refs/heads/master@{#379241}

[Peter Kasting, 2016-03-04 02:27:56]

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

[Peter Kasting, 2016-03-04 02:27:56]

estade: Main review.  In particular, it looks to me like the effect of the GTK
change may be that the status bubble text is now displayed blended with the
toolbar color (like on other platforms) instead of not?  If so this is a
behavior change, but it's one to bring GTK into compliance with other platforms
and seems more correct.

msw: A couple of years ago, ego sent you a CL that moved the calculation of this
into theme_service.cc.  You asked then for us to just ditch it and do a simpler
calculation.  erg demurred, saying that themes expected this.  AFAICT, this
simpler calculation will not only produce better-looking results, there's no
specific "theme dependence" on this (it's only based on the tab and toolbar
colors, which themes will certainly be adjusting independently, not based on the
status bubble).  So I'm doing it.  Thought I'd CC you FYI :)

[Evan Stade, 2016-03-04 04:22:16]

https://codereview.chromium.org/1764003002/diff/1/chrome/browser/ui/views/sta...
File chrome/browser/ui/views/status_bubble_views.cc (right):

https://codereview.chromium.org/1764003002/diff/1/chrome/browser/ui/views/sta...
chrome/browser/ui/views/status_bubble_views.cc:438:
theme_provider_->GetColor(ThemeProperties::COLOR_TAB_TEXT);
I tried this out on GTK theme mode and yea, it looks fine, if you assume the
classic theme also looks fine. But honestly I don't know why we don't just use
tab text color full stop (100% alpha). Can we ask a designer if that's OK?

I'm somewhat worried that this calculation will leave us with unreadable text in
some cases (not enough contrast). At the very least we should use
color_utils::GetReadableColor() but it would be easier (and imo looks fine) to
just use normal tab text color.

[Peter Kasting, 2016-03-04 05:53:46]

https://codereview.chromium.org/1764003002/diff/1/chrome/browser/ui/views/sta...
File chrome/browser/ui/views/status_bubble_views.cc (right):

https://codereview.chromium.org/1764003002/diff/1/chrome/browser/ui/views/sta...
chrome/browser/ui/views/status_bubble_views.cc:438:
theme_provider_->GetColor(ThemeProperties::COLOR_TAB_TEXT);
On 2016/03/04 04:22:16, Evan Stade wrote:
> I tried this out on GTK theme mode and yea, it looks fine, if you assume the
> classic theme also looks fine. But honestly I don't know why we don't just use
> tab text color full stop (100% alpha). Can we ask a designer if that's OK?

We use this intentionally because we're trying to avoid drawing attention to the
status bar text.  We found early on that the status bar was super distracting
and we want to at least minimize that effect.  It's been this was since at least
2009 on all non-Linux platforms, so I think it's fine; Linux just went under the
radar.

[Evan Stade, 2016-03-04 06:50:39]

On 2016/03/04 05:53:46, Peter Kasting wrote:
>
https://codereview.chromium.org/1764003002/diff/1/chrome/browser/ui/views/sta...
> File chrome/browser/ui/views/status_bubble_views.cc (right):
> 
>
https://codereview.chromium.org/1764003002/diff/1/chrome/browser/ui/views/sta...
> chrome/browser/ui/views/status_bubble_views.cc:438:
> theme_provider_->GetColor(ThemeProperties::COLOR_TAB_TEXT);
> On 2016/03/04 04:22:16, Evan Stade wrote:
> > I tried this out on GTK theme mode and yea, it looks fine, if you assume the
> > classic theme also looks fine. But honestly I don't know why we don't just
use
> > tab text color full stop (100% alpha). Can we ask a designer if that's OK?
> 
> We use this intentionally because we're trying to avoid drawing attention to
the
> status bar text.  We found early on that the status bar was super distracting
> and we want to at least minimize that effect.  It's been this was since at
least
> 2009 on all non-Linux platforms, so I think it's fine; Linux just went under
the
> radar.

What about the second half of my response? a11y is a valid concern regardless of
how long this has been around.

[Peter Kasting, 2016-03-04 06:55:31]

Description was changed from

==========
Fill COLOR_STATUS_BAR_TEXT.

This can be dynamically computed.

BUG=none
TEST=none
==========

to

==========
Remove COLOR_STATUS_BAR_TEXT.

This can be dynamically computed.

BUG=none
TEST=none
==========

[Peter Kasting, 2016-03-04 07:01:20]

On 2016/03/04 06:50:39, Evan Stade wrote:
> On 2016/03/04 05:53:46, Peter Kasting wrote:
> >
>
https://codereview.chromium.org/1764003002/diff/1/chrome/browser/ui/views/sta...
> > File chrome/browser/ui/views/status_bubble_views.cc (right):
> > 
> >
>
https://codereview.chromium.org/1764003002/diff/1/chrome/browser/ui/views/sta...
> > chrome/browser/ui/views/status_bubble_views.cc:438:
> > theme_provider_->GetColor(ThemeProperties::COLOR_TAB_TEXT);
> > On 2016/03/04 04:22:16, Evan Stade wrote:
> > > I tried this out on GTK theme mode and yea, it looks fine, if you assume
the
> > > classic theme also looks fine. But honestly I don't know why we don't just
> use
> > > tab text color full stop (100% alpha). Can we ask a designer if that's OK?
> > 
> > We use this intentionally because we're trying to avoid drawing attention to
> the
> > status bar text.  We found early on that the status bar was super
distracting
> > and we want to at least minimize that effect.  It's been this was since at
> least
> > 2009 on all non-Linux platforms, so I think it's fine; Linux just went under
> the
> > radar.
> 
> What about the second half of my response? a11y is a valid concern regardless
of
> how long this has been around.

We actually _want_ this to be relatively low contrast -- not unreadable, but
GetReadableColor() just aims to maximize contrast, it doesn't merely ensure
you're above the minimum.  So calling it is going to make this stand out way too
much sometimes.

We could try to write something fairly complex to compare this to the
accessibility min-contrast value and, if it's too low, try to boost it slightly.
 But we don't do this with the tab text itself, which is a thousand times more
important.  And such code isn't easy; I've been writing something like this for
COLOR_TOOLBAR_TOP_SEPARATOR and it's extremely complicated and costly to execute
(since contrast checks involve linearizing the RGB values, which is a bunch of
floating point math and exponentiation), so much so that I believe I'm going to
have to introduce some kind of caching system into the theme color system to
accommodate it (expect questions on that soon).

I really don't think we should block this change -- which simply aims to take
what we're already doing and do it more simply and correctly, and bring Linux
into line with all the rest of the platforms -- on these concerns.  Looking back
I think this kind of behavior predates 2009 and actually dates from before we
were public, so it's extremely hard to believe there are major accessibility
issues; I scanned the bug database and found nothing.  I think instead, if
you're worried about this, the right thing to do is land this and get our
behavior consistent across platforms, then file a separate bug with the a11y
folks regarding concerns about this.

[Evan Stade, 2016-03-04 07:29:22]

On 2016/03/04 07:01:20, Peter Kasting wrote:
> On 2016/03/04 06:50:39, Evan Stade wrote:
> > On 2016/03/04 05:53:46, Peter Kasting wrote:
> > >
> >
>
https://codereview.chromium.org/1764003002/diff/1/chrome/browser/ui/views/sta...
> > > File chrome/browser/ui/views/status_bubble_views.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1764003002/diff/1/chrome/browser/ui/views/sta...
> > > chrome/browser/ui/views/status_bubble_views.cc:438:
> > > theme_provider_->GetColor(ThemeProperties::COLOR_TAB_TEXT);
> > > On 2016/03/04 04:22:16, Evan Stade wrote:
> > > > I tried this out on GTK theme mode and yea, it looks fine, if you assume
> the
> > > > classic theme also looks fine. But honestly I don't know why we don't
just
> > use
> > > > tab text color full stop (100% alpha). Can we ask a designer if that's
OK?
> > > 
> > > We use this intentionally because we're trying to avoid drawing attention
to
> > the
> > > status bar text.  We found early on that the status bar was super
> distracting
> > > and we want to at least minimize that effect.  It's been this was since at
> > least
> > > 2009 on all non-Linux platforms, so I think it's fine; Linux just went
under
> > the
> > > radar.
> > 
> > What about the second half of my response? a11y is a valid concern
regardless
> of
> > how long this has been around.
> 
> We actually _want_ this to be relatively low contrast -- not unreadable, but
> GetReadableColor() just aims to maximize contrast, it doesn't merely ensure
> you're above the minimum.  So calling it is going to make this stand out way
too
> much sometimes.
> 
> We could try to write something fairly complex to compare this to the
> accessibility min-contrast value and, if it's too low, try to boost it
slightly.
>  But we don't do this with the tab text itself, which is a thousand times more
> important.  And such code isn't easy; I've been writing something like this
for
> COLOR_TOOLBAR_TOP_SEPARATOR and it's extremely complicated and costly to
execute
> (since contrast checks involve linearizing the RGB values, which is a bunch of
> floating point math and exponentiation), so much so that I believe I'm going
to
> have to introduce some kind of caching system into the theme color system to
> accommodate it (expect questions on that soon).
> 
> I really don't think we should block this change -- which simply aims to take
> what we're already doing and do it more simply and correctly, and bring Linux
> into line with all the rest of the platforms -- on these concerns.  Looking
back
> I think this kind of behavior predates 2009 and actually dates from before we
> were public, so it's extremely hard to believe there are major accessibility
> issues; I scanned the bug database and found nothing.  I think instead, if
> you're worried about this, the right thing to do is land this and get our
> behavior consistent across platforms, then file a separate bug with the a11y
> folks regarding concerns about this.

The accessibility issues would apply only under certain themes. I find it very
easy to believe that some themes will be unreadable in the status bubble, even
if the theme author was careful to choose contrasting enough colors in the more
noticeable parts of Chrome. Because of the cross section of secondary ui,
obscure theme, and awareness of a11y, I can see this going unreported for a long
period of time. "Bringing linux into line" doesn't seem like that big of a
priority to me --- I could use the same logic and say that because it's not
reported in the bug tracker it must not be a problem.

anyway, lgtm, assuming you tested this on the top N most popular themes.

[Peter Kasting, 2016-03-04 07:32:31]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2016-03-04 07:32:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1764003002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1764003002/1

[commit-bot: I haz the power, 2016-03-04 07:46:35]

Description was changed from

==========
Remove COLOR_STATUS_BAR_TEXT.

This can be dynamically computed.

BUG=none
TEST=none
==========

to

==========
Remove COLOR_STATUS_BAR_TEXT.

This can be dynamically computed.

BUG=none
TEST=none
==========

[commit-bot: I haz the power, 2016-03-04 07:46:36]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-03-04 07:48:13]

Description was changed from

==========
Remove COLOR_STATUS_BAR_TEXT.

This can be dynamically computed.

BUG=none
TEST=none
==========

to

==========
Remove COLOR_STATUS_BAR_TEXT.

This can be dynamically computed.

BUG=none
TEST=none

Committed: https://crrev.com/9355b054f37315e5825faf9068dee0d16ba32b74
Cr-Commit-Position: refs/heads/master@{#379241}
==========

[commit-bot: I haz the power, 2016-03-04 07:48:13]

Patchset 1 (id:??) landed as
https://crrev.com/9355b054f37315e5825faf9068dee0d16ba32b74
Cr-Commit-Position: refs/heads/master@{#379241}

[msw, 2016-03-04 17:56:01]

msw@chromium.org changed reviewers:
+ msw@chromium.org

[msw, 2016-03-04 17:56:01]

nice!