Improve appearance of higlighting in profile chooser view.

Improve appearance of higlighting in profile chooser view.

1. Use NativeTheme menu colors for selected rows on all platforms. This
allows us to remove platform ifdefs (only Linux did this before). This
has almost no visible effect on a classic theme as the theme menu colors
were almost identical to the colors being used in the profile chooser.
One thing that does change is that the color of the hovered text is a
smidgeon darker, improving contrast.

2. Don't draw a dashed focus rect, instead reuse selection. This matches
how menus look. Also make focus actually follow the mouse, also matching
how menus work.

BUG=696003
Review-Url: https://codereview.chromium.org/2736613008
Cr-Commit-Position: refs/heads/master@{#457494}
Committed: https://chromium.googlesource.com/chromium/src/+/e5ec3568878feddb6978cb2a352e1226ccb74bd2

[Evan Stade, 2017-03-08 19:28:29]

Description was changed from

==========
Improve appearance of higlighting in profile chooser view.

1. Use NativeTheme menu colors for selected rows on all platforms. This
allows us to remove platform ifdefs (only Linux did this before). This
has almost no visible effect on a classic theme as the theme menu colors
were almost identical to the colors being used in the profile chooser.

2. Don't draw a dashed focus rect, instead reuse selection. This matches
how menus look. Also make focus actually follow the mouse, also matching
how menus work.

BUG=696003
==========

to

==========
Improve appearance of higlighting in profile chooser view.

1. Use NativeTheme menu colors for selected rows on all platforms. This
allows us to remove platform ifdefs (only Linux did this before). This
has almost no visible effect on a classic theme as the theme menu colors
were almost identical to the colors being used in the profile chooser.
One thing that does change is that the color of the hovered text is a
smidgeon darker, improving contrast.

2. Don't draw a dashed focus rect, instead reuse selection. This matches
how menus look. Also make focus actually follow the mouse, also matching
how menus work.

BUG=696003
==========

[Evan Stade, 2017-03-08 19:28:29]

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

[Evan Stade, 2017-03-08 19:29:07]

+tapted for review (will send to a c/b/u/v/ owner later)
+thomasanderson for fyi/optional review

[Tom (Use chromium acct), 2017-03-08 20:12:31]

thomasanderson@google.com changed reviewers:
+ thomasanderson@google.com

[Tom (Use chromium acct), 2017-03-08 20:12:31]

When I test this patch locally, and I press tab over the profile chooser, I
still see the dashed focus rect and no selection color.

[Evan Stade, 2017-03-08 20:38:04]

On 2017/03/08 20:12:31, Tom Anderson wrote:
> When I test this patch locally, and I press tab over the profile chooser, I
> still see the dashed focus rect and no selection color.

Can you post a screenshot? I'm not sure how this could be.

[Tom (Use chromium acct), 2017-03-08 20:59:06]

On 2017/03/08 20:38:04, Evan Stade wrote:
> On 2017/03/08 20:12:31, Tom Anderson wrote:
> > When I test this patch locally, and I press tab over the profile chooser, I
> > still see the dashed focus rect and no selection color.
> 
> Can you post a screenshot? I'm not sure how this could be.

Nvm, I think the 'git cl patch' did not apply the first time.

However, when I move my cursor off of a button, it remains selected.
https://bugs.chromium.org/p/chromium/issues/detail?id=696003#c3

[Evan Stade, 2017-03-08 21:06:19]

On 2017/03/08 20:59:06, Tom Anderson wrote:
> On 2017/03/08 20:38:04, Evan Stade wrote:
> > On 2017/03/08 20:12:31, Tom Anderson wrote:
> > > When I test this patch locally, and I press tab over the profile chooser,
I
> > > still see the dashed focus rect and no selection color.
> > 
> > Can you post a screenshot? I'm not sure how this could be.
> 
> Nvm, I think the 'git cl patch' did not apply the first time.
> 
> However, when I move my cursor off of a button, it remains selected.
> https://bugs.chromium.org/p/chromium/issues/detail?id=696003#c3

Good point. I should make that not happen. Will ping after I have a chance to
fix this next week (will be OOO till then).

[Evan Stade, 2017-03-15 23:16:12]

updated, PTAL. Unhovering blurs the view. This does align with menus, where if
you interleave mouse movements and keyboard arrow presses, you can get
focus/selection jumping back and forth.

[Evan Stade, 2017-03-15 23:16:18]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 23:17:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2017-03-15 23:18:05]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 23:19:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom (Use chromium acct), 2017-03-15 23:30:31]

lgtm

[tapted, 2017-03-15 23:33:29]

lgtm - my only concern is around having RequestFocus() follow mouse hover. It's
not how menus do it, and it may have implications for things like screen readers
or other a11y use cases. But it's only a vague hunch. I can't point to anything
specific that will go wrong.

Since the profile selector is now basically "just" a menu maybe it will be
fine...

[commit-bot: I haz the power, 2017-03-16 00:04:09]

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

[commit-bot: I haz the power, 2017-03-16 00:04:10]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2017-03-16 00:20:29]

On 2017/03/15 23:33:29, tapted wrote:
> lgtm - my only concern is around having RequestFocus() follow mouse hover.
It's
> not how menus do it, and it may have implications for things like screen
readers
> or other a11y use cases. But it's only a vague hunch. I can't point to
anything
> specific that will go wrong.

I had the same thought wrt screen readers. But when I run chromeos with
chromevox and open a menu, the orange box actually does follow hover around and
announce the rows you're mousing over.

[Evan Stade, 2017-03-16 00:20:41]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2017-03-16 00:21:31]

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-16 00:34:46]

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

[commit-bot: I haz the power, 2017-03-16 00:34:47]

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...)

[Evan Stade, 2017-03-16 01:21:11]

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

[Evan Stade, 2017-03-16 01:21:12]

+msw for owners (for some reason I thought tapted was an owner)

[msw, 2017-03-16 01:41:45]

lgtm with nits

https://codereview.chromium.org/2736613008/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/2736613008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:239: if ((state() ==
STATE_HOVERED || state() == STATE_PRESSED))
nit: extra parens not needed

https://codereview.chromium.org/2736613008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:241: else if (state()
== STATE_NORMAL && HasFocus())
q: odd, when does this happen? add a comment?

https://codereview.chromium.org/2736613008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:247: void
UpdateColors() {
nit: should this be called from the constructor? (maybe so if we use menu
enabled/disabled colors instead of label colors). maybe add a function comment.

https://codereview.chromium.org/2736613008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:259: :
ui::NativeTheme::kColorId_LabelEnabledColor);
nit: kColorId_EnabledMenuItemForegroundColor for consistency?

https://codereview.chromium.org/2736613008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:269: :
ui::NativeTheme::kColorId_LabelDisabledColor));
nit: kColorId_DisabledMenuItemForegroundColor for consistency?

[Evan Stade, 2017-03-16 17:28:15]

https://codereview.chromium.org/2736613008/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/2736613008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:239: if ((state() ==
STATE_HOVERED || state() == STATE_PRESSED))
On 2017/03/16 01:41:45, msw wrote:
> nit: extra parens not needed

Done.

https://codereview.chromium.org/2736613008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:241: else if (state()
== STATE_NORMAL && HasFocus())
On 2017/03/16 01:41:45, msw wrote:
> q: odd, when does this happen? add a comment?

this is the blur-on-unhover part of "focus follows the mouse". I've elaborated
the comment above.

https://codereview.chromium.org/2736613008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:247: void
UpdateColors() {
On 2017/03/16 01:41:45, msw wrote:
> nit: should this be called from the constructor? (maybe so if we use menu
> enabled/disabled colors instead of label colors). maybe add a function
comment.

It relies on the native theme, and the correct native theme isn't set until this
view is added to a widget, at which point OnNativeThemeChanged will be called
and thus UpdateColors.

https://codereview.chromium.org/2736613008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:259: :
ui::NativeTheme::kColorId_LabelEnabledColor);
On 2017/03/16 01:41:45, msw wrote:
> nit: kColorId_EnabledMenuItemForegroundColor for consistency?

Indeed this is sort of weird, but the background of the avatar bubble menu
thingy is not the same as a menu background, it's a bubble background. The
button/label text should be readable on a normal background, not a menu
background. This actually matters as there are GTK themes (of the ones I have
installed, Numix is one) where a menu is white text on black bg (red bg for
highlight) but black text on a light grey bg for windows/bubbles/dialogs.

[Evan Stade, 2017-03-16 17:28:22]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 17:29:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msw, 2017-03-16 17:40:17]

Nice, lgtm

https://codereview.chromium.org/2736613008/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/2736613008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:241: else if (state()
== STATE_NORMAL && HasFocus())
On 2017/03/16 17:28:14, Evan Stade wrote:
> On 2017/03/16 01:41:45, msw wrote:
> > q: odd, when does this happen? add a comment?
> 
> this is the blur-on-unhover part of "focus follows the mouse". I've elaborated
> the comment above.

Acknowledged.

https://codereview.chromium.org/2736613008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:247: void
UpdateColors() {
On 2017/03/16 17:28:14, Evan Stade wrote:
> On 2017/03/16 01:41:45, msw wrote:
> > nit: should this be called from the constructor? (maybe so if we use menu
> > enabled/disabled colors instead of label colors). maybe add a function
> comment.
> 
> It relies on the native theme, and the correct native theme isn't set until
this
> view is added to a widget, at which point OnNativeThemeChanged will be called
> and thus UpdateColors.

Acknowledged.

https://codereview.chromium.org/2736613008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:259: :
ui::NativeTheme::kColorId_LabelEnabledColor);
On 2017/03/16 17:28:15, Evan Stade wrote:
> On 2017/03/16 01:41:45, msw wrote:
> > nit: kColorId_EnabledMenuItemForegroundColor for consistency?
> 
> Indeed this is sort of weird, but the background of the avatar bubble menu
> thingy is not the same as a menu background, it's a bubble background. The
> button/label text should be readable on a normal background, not a menu
> background. This actually matters as there are GTK themes (of the ones I have
> installed, Numix is one) where a menu is white text on black bg (red bg for
> highlight) but black text on a light grey bg for windows/bubbles/dialogs.

Acknowledged.

[Evan Stade, 2017-03-16 17:41:21]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2017-03-16 17:41:46]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-03-16 17:41:46]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
thomasanderson@google.com
Link to the patchset: https://codereview.chromium.org/2736613008/#ps60001
(title: "msw review")

[commit-bot: I haz the power, 2017-03-16 17:42:33]

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-16 18:14:39]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489686106113760,
"parent_rev": "c3cb7d84db9a7d99ae917f32c1acca0f44ca3b22", "commit_rev":
"e5ec3568878feddb6978cb2a352e1226ccb74bd2"}

[commit-bot: I haz the power, 2017-03-16 18:15:28]

Description was changed from

==========
Improve appearance of higlighting in profile chooser view.

1. Use NativeTheme menu colors for selected rows on all platforms. This
allows us to remove platform ifdefs (only Linux did this before). This
has almost no visible effect on a classic theme as the theme menu colors
were almost identical to the colors being used in the profile chooser.
One thing that does change is that the color of the hovered text is a
smidgeon darker, improving contrast.

2. Don't draw a dashed focus rect, instead reuse selection. This matches
how menus look. Also make focus actually follow the mouse, also matching
how menus work.

BUG=696003
==========

to

==========
Improve appearance of higlighting in profile chooser view.

1. Use NativeTheme menu colors for selected rows on all platforms. This
allows us to remove platform ifdefs (only Linux did this before). This
has almost no visible effect on a classic theme as the theme menu colors
were almost identical to the colors being used in the profile chooser.
One thing that does change is that the color of the hovered text is a
smidgeon darker, improving contrast.

2. Don't draw a dashed focus rect, instead reuse selection. This matches
how menus look. Also make focus actually follow the mouse, also matching
how menus work.

BUG=696003

Review-Url: https://codereview.chromium.org/2736613008
Cr-Commit-Position: refs/heads/master@{#457494}
Committed:
https://chromium.googlesource.com/chromium/src/+/e5ec3568878feddb6978cb2a352e...
==========

[commit-bot: I haz the power, 2017-03-16 18:15:29]

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