Add call to Label::OnNativeThemeChagned() from views::Link

Add call to Label::OnNativeThemeChagned() from views::Link

BUG=707087
R=pkasting@chromium.org

Review-Url: https://codereview.chromium.org/2785973004
Cr-Commit-Position: refs/heads/master@{#464960}
Committed: https://chromium.googlesource.com/chromium/src/+/0e4f997e76d4868fa3ccbe690c2ffd4a795a104b

[Tom (Use chromium acct), 2017-03-31 00:10:10]

Description was changed from

==========
Disable auto color readability on links by default

Auto color readability on labels relies on GetReadableColor(), which
only works well for greyscale colors.  Since links are usually very
saturated, auto color readability does not work well for them, so this
CL disables it for links.

BUG=707087
R=sky@chromium.org
==========

to

==========
Disable auto color readability on links by default

Auto color readability on labels relies on GetReadableColor(), which
only works well for greyscale colors.  Since links are usually very
saturated, auto color readability does not work well for them, so this
CL disables it for links.

Before:
https://bugs.chromium.org/p/chromium/issues/detail?id=707087
After:
https://bugs.chromium.org/p/chromium/issues/detail?id=707087#c1

BUG=707087
R=sky@chromium.org
==========

[Tom (Use chromium acct), 2017-03-31 00:11:06]

sky@ ptal

I tried a few approaches, and this one was just the least-bad of them :)

[Tom (Use chromium acct), 2017-03-31 00:24:46]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 00:25:55]

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-03-31 01:36:33]

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

[commit-bot: I haz the power, 2017-03-31 01:36:34]

Dry run: This issue passed the CQ dry run.

[sky, 2017-03-31 04:46:24]

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

[sky, 2017-03-31 04:46:24]

+pkasting, who I think knows more about what readability does than I do.

[Peter Kasting, 2017-03-31 20:06:20]

Why do you say auto color readability only works well for greyscale colors?  It
was designed for full color cases, not greyscale.

In the orange-on-grey case in the bug, the with-auto-coloring change is higher
contrast than without.  In the blue-on-charcoal case, I wonder if the link's
background color is not getting properly set, so the code is computing
readability against a light background instead of a dark one?

So I disagree with the premise of this fix and I'd want to understand better why
we're getting the results we are in cases where things don't look correct.

[Tom (Use chromium acct), 2017-03-31 21:05:50]

On 2017/03/31 20:06:20, Peter Kasting wrote:
> Why do you say auto color readability only works well for greyscale colors? 
It
> was designed for full color cases, not greyscale.
> 

The main variable that GetReadableColor() seems to use is the luminance as
returned by GetRelativeLuminance(), so it seems that if we had two colors that
had a different hue but the same luminance, then GetReadableColor() would think
the contrast ratio was very low even though the colors are readable?

> In the orange-on-grey case in the bug, the with-auto-coloring change is higher
> contrast than without.  

Ok, I agree (but I still think the bright orange color should be used, because
that's how links in system apps are colored)

> In the blue-on-charcoal case, I wonder if the link's
> background color is not getting properly set, so the code is computing
> readability against a light background instead of a dark one?
> 

It appears OnNativeThemeChanged() is never getting called on these links, which
means this code never runs
https://cs.chromium.org/chromium/src/ui/views/controls/label.cc?rcl=f262313c8...

Label::Init() initially sets these colors with the default theme
https://cs.chromium.org/chromium/src/ui/views/controls/label.cc?rcl=d25c21ff3...
This theme apparently has 0xffffffff as the dialog bg color, so this would
explain why the links are always darkened.

> So I disagree with the premise of this fix and I'd want to understand better
why
> we're getting the results we are in cases where things don't look correct.

[Peter Kasting, 2017-03-31 21:52:05]

On 2017/03/31 21:05:50, Tom Anderson wrote:
> On 2017/03/31 20:06:20, Peter Kasting wrote:
> > Why do you say auto color readability only works well for greyscale colors? 
> It
> > was designed for full color cases, not greyscale.
> 
> The main variable that GetReadableColor() seems to use is the luminance as
> returned by GetRelativeLuminance(), so it seems that if we had two colors that
> had a different hue but the same luminance, then GetReadableColor() would
think
> the contrast ratio was very low even though the colors are readable?

If you have two colors with different hues but the same luminance atop each
other, then their contrast _is_ very low, and they're not going to be very
readable.  This is part of why blue-on-dark-grey is, indeed, not very readable. 
Similar examples also exist (e.g. middle red on somewhat-darker green).  

The source on all this stuff is the WCAG; basically, we're trying to comply with
standard metrics for content accessibility.

> > In the orange-on-grey case in the bug, the with-auto-coloring change is
higher
> > contrast than without.  
> 
> Ok, I agree (but I still think the bright orange color should be used, because
> that's how links in system apps are colored)

It might be possible to do something like, in places where we set auto color
readability, disable it if the source of both foreground and background colors
is a system theme, since we assume the user knows what they're doing or will
change things there.  Auto color readability is primarily meant for cases where
we control one of the colors internally.

I don't know how hard it would be to turn that handwavey suggestion into
something practical, but it would suggest a fix in a slightly different place.

> > In the blue-on-charcoal case, I wonder if the link's
> > background color is not getting properly set, so the code is computing
> > readability against a light background instead of a dark one?
> 
> It appears OnNativeThemeChanged() is never getting called on these links,
which
> means this code never runs
>
https://cs.chromium.org/chromium/src/ui/views/controls/label.cc?rcl=f262313c8...
> 
> Label::Init() initially sets these colors with the default theme
>
https://cs.chromium.org/chromium/src/ui/views/controls/label.cc?rcl=d25c21ff3...
> This theme apparently has 0xffffffff as the dialog bg color, so this would
> explain why the links are always darkened.

I would imagine fixing that will make things better then :)

[Tom (Use chromium acct), 2017-04-14 17:47:35]

On 2017/03/31 21:52:05, Peter Kasting wrote:
> On 2017/03/31 21:05:50, Tom Anderson wrote:
> > Ok, I agree (but I still think the bright orange color should be used,
because
> > that's how links in system apps are colored)
> 
> It might be possible to do something like, in places where we set auto color
> readability, disable it if the source of both foreground and background colors
> is a system theme, since we assume the user knows what they're doing or will
> change things there.  Auto color readability is primarily meant for cases
where
> we control one of the colors internally.
> 
> I don't know how hard it would be to turn that handwavey suggestion into
> something practical, but it would suggest a fix in a slightly different place.
> 

Label() knows when it sets the bg or fg colors with native theme colors, so we
can track that.  We could add new variables similar to how we already have
|enabled_color_set_| etc.

The only thing is, clients can call SetEnabledColor() with a native theme color,
so we'd have to add SetEnabledColorFromNativeThemeColor(ColorId) and clients
would have to call this instead.

Not sure if this is the right approach, or if it's even worth the effort.  wdyt?

> > It appears OnNativeThemeChanged() is never getting called on these links,
> which
> > means this code never runs
> >
>
https://cs.chromium.org/chromium/src/ui/views/controls/label.cc?rcl=f262313c8...
> > 
> > Label::Init() initially sets these colors with the default theme
> >
>
https://cs.chromium.org/chromium/src/ui/views/controls/label.cc?rcl=d25c21ff3...
> > This theme apparently has 0xffffffff as the dialog bg color, so this would
> > explain why the links are always darkened.
> 
> I would imagine fixing that will make things better then :)

Turns out it's because Link::OnNativeThemeChanged() doesn't call
Label::OnNativeThemeChanged().  Making this change fixes the readability issue
in the blue-on-grey case.

[Peter Kasting, 2017-04-14 18:31:51]

On 2017/04/14 17:47:35, Tom Anderson wrote:
> On 2017/03/31 21:52:05, Peter Kasting wrote:
> > On 2017/03/31 21:05:50, Tom Anderson wrote:
> > > Ok, I agree (but I still think the bright orange color should be used,
> because
> > > that's how links in system apps are colored)
> > 
> > It might be possible to do something like, in places where we set auto color
> > readability, disable it if the source of both foreground and background
colors
> > is a system theme, since we assume the user knows what they're doing or will
> > change things there.  Auto color readability is primarily meant for cases
> where
> > we control one of the colors internally.
> > 
> > I don't know how hard it would be to turn that handwavey suggestion into
> > something practical, but it would suggest a fix in a slightly different
place.
> > 
> 
> Label() knows when it sets the bg or fg colors with native theme colors, so we
> can track that.  We could add new variables similar to how we already have
> |enabled_color_set_| etc.
> 
> The only thing is, clients can call SetEnabledColor() with a native theme
color,
> so we'd have to add SetEnabledColorFromNativeThemeColor(ColorId) and clients
> would have to call this instead.
> 
> Not sure if this is the right approach, or if it's even worth the effort. 
wdyt?

I think it makes sense, especially if we rename the other version
...FromNonNativeColor or something to try and discourage people from using it
for themed colors.

> > > It appears OnNativeThemeChanged() is never getting called on these links,
> > which
> > > means this code never runs
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/views/controls/label.cc?rcl=f262313c8...
> > > 
> > > Label::Init() initially sets these colors with the default theme
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/views/controls/label.cc?rcl=d25c21ff3...
> > > This theme apparently has 0xffffffff as the dialog bg color, so this would
> > > explain why the links are always darkened.
> > 
> > I would imagine fixing that will make things better then :)
> 
> Turns out it's because Link::OnNativeThemeChanged() doesn't call
> Label::OnNativeThemeChanged().  Making this change fixes the readability issue
> in the blue-on-grey case.

Ah, cool.  Is that a CL elsewhere?

[Tom (Use chromium acct), 2017-04-14 18:31:52]

Description was changed from

==========
Disable auto color readability on links by default

Auto color readability on labels relies on GetReadableColor(), which
only works well for greyscale colors.  Since links are usually very
saturated, auto color readability does not work well for them, so this
CL disables it for links.

Before:
https://bugs.chromium.org/p/chromium/issues/detail?id=707087
After:
https://bugs.chromium.org/p/chromium/issues/detail?id=707087#c1

BUG=707087
R=sky@chromium.org
==========

to

==========
Add call to Label::OnNativeThemeChagned() from views::Link

BUG=707087
R=pkasting@chromium.org
==========

[Tom (Use chromium acct), 2017-04-14 18:31:52]

thomasanderson@google.com changed reviewers:
- sky@chromium.org

[Tom (Use chromium acct), 2017-04-14 18:34:23]

On 2017/04/14 18:31:51, Peter Kasting wrote:
> On 2017/04/14 17:47:35, Tom Anderson wrote:
> > On 2017/03/31 21:52:05, Peter Kasting wrote:
> > > On 2017/03/31 21:05:50, Tom Anderson wrote:
> > > > Ok, I agree (but I still think the bright orange color should be used,
> > because
> > > > that's how links in system apps are colored)
> > > 
> > > It might be possible to do something like, in places where we set auto
color
> > > readability, disable it if the source of both foreground and background
> colors
> > > is a system theme, since we assume the user knows what they're doing or
will
> > > change things there.  Auto color readability is primarily meant for cases
> > where
> > > we control one of the colors internally.
> > > 
> > > I don't know how hard it would be to turn that handwavey suggestion into
> > > something practical, but it would suggest a fix in a slightly different
> place.
> > > 
> > 
> > Label() knows when it sets the bg or fg colors with native theme colors, so
we
> > can track that.  We could add new variables similar to how we already have
> > |enabled_color_set_| etc.
> > 
> > The only thing is, clients can call SetEnabledColor() with a native theme
> color,
> > so we'd have to add SetEnabledColorFromNativeThemeColor(ColorId) and clients
> > would have to call this instead.
> > 
> > Not sure if this is the right approach, or if it's even worth the effort. 
> wdyt?
> 
> I think it makes sense, especially if we rename the other version
> ...FromNonNativeColor or something to try and discourage people from using it
> for themed colors.
> 

Ok, I'll try creating a separate CL for that

> > > > It appears OnNativeThemeChanged() is never getting called on these
links,
> > > which
> > > > means this code never runs
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/views/controls/label.cc?rcl=f262313c8...
> > > > 
> > > > Label::Init() initially sets these colors with the default theme
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/views/controls/label.cc?rcl=d25c21ff3...
> > > > This theme apparently has 0xffffffff as the dialog bg color, so this
would
> > > > explain why the links are always darkened.
> > > 
> > > I would imagine fixing that will make things better then :)
> > 
> > Turns out it's because Link::OnNativeThemeChanged() doesn't call
> > Label::OnNativeThemeChanged().  Making this change fixes the readability
issue
> > in the blue-on-grey case.
> 
> Ah, cool.  Is that a CL elsewhere?

Updated this cl just as you mailed this

[Peter Kasting, 2017-04-14 18:47:30]

LGTM

[Tom (Use chromium acct), 2017-04-14 18:51:09]

The CQ bit was checked by thomasanderson@google.com

[commit-bot: I haz the power, 2017-04-14 18:51:27]

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-04-14 19:00:00]

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

[commit-bot: I haz the power, 2017-04-14 19:00:02]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Tom (Use chromium acct), 2017-04-14 19:13:54]

thomasanderson@google.com changed reviewers:
+ sadrul@chromium.org

[Tom (Use chromium acct), 2017-04-14 19:13:54]

+sadrul for views OWNERS approval

[sky, 2017-04-17 17:55:15]

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

[sky, 2017-04-17 17:55:15]

LGTM

[sky, 2017-04-17 17:55:20]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2017-04-17 17:55:44]

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-04-17 18:54:49]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492451720150170,
"parent_rev": "4ee9c80b89c4798b1db1df851fb361ba0a383019", "commit_rev":
"0e4f997e76d4868fa3ccbe690c2ffd4a795a104b"}

[commit-bot: I haz the power, 2017-04-17 18:55:05]

Description was changed from

==========
Add call to Label::OnNativeThemeChagned() from views::Link

BUG=707087
R=pkasting@chromium.org
==========

to

==========
Add call to Label::OnNativeThemeChagned() from views::Link

BUG=707087
R=pkasting@chromium.org

Review-Url: https://codereview.chromium.org/2785973004
Cr-Commit-Position: refs/heads/master@{#464960}
Committed:
https://chromium.googlesource.com/chromium/src/+/0e4f997e76d4868fa3ccbe690c2f...
==========

[commit-bot: I haz the power, 2017-04-17 18:55:10]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/0e4f997e76d4868fa3ccbe690c2f...