[Mac] Use CommonTheme for the new avatar menu button hover background

[Mac] Use CommonTheme for the new avatar menu button hover background

As of r286651, NativeThemeMac now provides user-configurable system
colours for better OS integration. However, these are not needed for
dialogs that aren't trying to look native.

This change gets the button hover background used for the new avatar
menu from CommonTheme instead, to get a cross-platform look.

BUG=399820
TEST=On Mac, run Chrome with --new-avatar-menu and open the avatar
drop-down; hovering over the "Not xxx?" item should switch to a gray
background.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287725

[tapted, 2014-08-04 03:11:48]

Hi Monica, see what you think. This feels like the right fix, but feel free to
ignore/roll your own CL, or suggest an alternative on the bug.

if you like it or want a second opinion, feel free to add a c/b/ui/cocoa OWNER -
I'm in a sucky timezone for doing that, so it might save a day of turnaround :)

- Thanks! (and sorry for the regression!).

https://codereview.chromium.org/440633002/diff/1/chrome/browser/ui/cocoa/prof...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/440633002/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:188:
ui::NativeTheme::kColorId_DialogBackground));
note, I might be looking to change this later too.. But I'll change them all at
once. Something like kColorId_MenuBackgroundColor might be suitable here.

https://codereview.chromium.org/440633002/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:652:
hoverColor_.reset([gfx::SkColorToSRGBNSColor(hoverColor) retain]);
Note it's using sRGB rather than "Calibrated" here. sRGB is the colorspace that
PNGs, CSS, iOS and Windows use by default [more info: http://crbug.com/254361 ].
This might be darker than previously -- sRGB values are encoded with a gamma of
2.2 which became the default in Snow Leopard [1], but older NSColor methods tend
to use a gamma of 1.8.

Seems NSColor documentation [2] now also recommends srgb: "Where possible, it is
preferable to specify the colorspace explicitly using the
colorWithSRGBRed:green:blue:alpha: or colorWithGenericGamma22White:alpha:
method."

[1] http://support.apple.com/kb/ht3712
[2]
https://developer.apple.com/library/mac/documentation/cocoa/Reference/Applica...:

[noms (inactive), 2014-08-04 18:25:17]

Should we remove kColorId_ButtonHoverBackgroundColor from the mac native theme?
I'm not sure what would stop me from using the wrong colour in the future :(

Otherwise, this lgtm, though you're probably still going to need a Cocoa owner
anyway (I have no cocoa powers)

[tapted, 2014-08-05 00:09:07]

+rsesek for OWNERS and ++wisdom

On 2014/08/04 18:25:17, Monica Dinculescu wrote:
> Should we remove kColorId_ButtonHoverBackgroundColor from the mac native
theme?
> I'm not sure what would stop me from using the wrong colour in the future :(

NativeTheme is mostly used by the UI elements in ui/views/controls. Although
this colour constant is currently unused outside of the profile selector (not
sure if it was always that way). However, kColorId_ButtonHoverColor *is* used
for the foreground text in controls/button/label_button.cc. It's probably odd
that it doesn't use ButtonHoverBackgroundColor as well, but if it did I think it
should be derived the same way as ButtonHoverColor. Which on mac is [NSColor
selectedControlTextColor].

Looking around.. Windows has a distinct constant, which just happens to be the
same as CommonTheme. GTK2 has its own constant, so someone with a custom colour
scheme in Linux could see something different here.

Mac doesn't offer that level of customisation, but making the hoverbackground
the same as the regular background is a signal that native buttons do not
typically change backgrounds when hovered.

> Otherwise, this lgtm, though you're probably still going to need a Cocoa owner
> anyway (I have no cocoa powers)

[tapted, 2014-08-05 23:34:18]

+groby for OWNERS - please take a look, and see if you agree with earlier
comments. Many thanks

(rsesek OOO - oops)

[groby-ooo-7-16, 2014-08-06 00:05:25]

LGTM

https://codereview.chromium.org/440633002/diff/1/chrome/browser/ui/cocoa/prof...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/440633002/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:188:
ui::NativeTheme::kColorId_DialogBackground));
On 2014/08/04 03:11:47, tapted wrote:
> note, I might be looking to change this later too.. But I'll change them all
at
> once. Something like kColorId_MenuBackgroundColor might be suitable here.

It'd be nice if the theme called out a color id for dialog backgrounds. (Not
that it matters to this CL)

https://codereview.chromium.org/440633002/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:650:
ui::NativeTheme::kColorId_ButtonHoverBackgroundColor, &hoverColor);
I wish CommonTheme followed the NativeTheme API and didn't return a bool :(

https://codereview.chromium.org/440633002/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:652:
hoverColor_.reset([gfx::SkColorToSRGBNSColor(hoverColor) retain]);
On 2014/08/04 03:11:47, tapted wrote:
> Note it's using sRGB rather than "Calibrated" here. sRGB is the colorspace
that
> PNGs, CSS, iOS and Windows use by default [more info: http://crbug.com/254361
].
> This might be darker than previously -- sRGB values are encoded with a gamma
of
> 2.2 which became the default in Snow Leopard [1], but older NSColor methods
tend
> to use a gamma of 1.8.
> 
> Seems NSColor documentation [2] now also recommends srgb: "Where possible, it
is
> preferable to specify the colorspace explicitly using the
> colorWithSRGBRed:green:blue:alpha: or colorWithGenericGamma22White:alpha:
> method."
> 
> [1] http://support.apple.com/kb/ht3712
> [2]
>
https://developer.apple.com/library/mac/documentation/cocoa/Reference/Applica...:

+1 on fixing the color space. (I believe I suggested "calibrated" here, so
that's my fault)

But N.B. that half of our colors still use SkColorToCalibratedNSColor. Not
directly relevant to the bug, but should we add a presubmit that prevents use of
calibrated?

[tapted, 2014-08-06 01:49:09]

https://codereview.chromium.org/440633002/diff/1/chrome/browser/ui/cocoa/prof...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/440633002/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:188:
ui::NativeTheme::kColorId_DialogBackground));
On 2014/08/06 00:05:25, groby wrote:
> On 2014/08/04 03:11:47, tapted wrote:
> > note, I might be looking to change this later too.. But I'll change them all
> at
> > once. Something like kColorId_MenuBackgroundColor might be suitable here.
> 
> It'd be nice if the theme called out a color id for dialog backgrounds. (Not
> that it matters to this CL)

Maybe a separate colors for NSAlert-style dialog backgrounds, and another for
bubble-like backgrounds is in order.

I'll probably end up getting some input from a UX person. The constant that
NativeThemeMac offers up is quite white. I think, e.g., the bookmark bubble
looks a bit out of place on Mac. On Linux, it's nice shade of gray that matches
the toolbar background.

Checking iTunes.. it has some bubbles that you get from its _two_ hotdog buttons
(one in the status display that only appears on hover). One is quite gray, and
one is quite white.  So, I guess that's not much help :/. 

One of my many yaks to shave...

https://codereview.chromium.org/440633002/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:650:
ui::NativeTheme::kColorId_ButtonHoverBackgroundColor, &hoverColor);
On 2014/08/06 00:05:25, groby wrote:
> I wish CommonTheme followed the NativeTheme API and didn't return a bool :(

Yeah :/ - I think it's to avoid something special to handle SkColor(0), although
100% transparent black is a pretty useless colour (and the SystemColor tests
I've added treat it as an error anyway).

https://codereview.chromium.org/440633002/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:652:
hoverColor_.reset([gfx::SkColorToSRGBNSColor(hoverColor) retain]);
On 2014/08/06 00:05:24, groby wrote:
> On 2014/08/04 03:11:47, tapted wrote:
> > Note it's using sRGB rather than "Calibrated" here. sRGB is the colorspace
> that
> > PNGs, CSS, iOS and Windows use by default [more info:
http://crbug.com/254361
> ].
> > This might be darker than previously -- sRGB values are encoded with a gamma
> of
> > 2.2 which became the default in Snow Leopard [1], but older NSColor methods
> tend
> > to use a gamma of 1.8.
> > 
> > Seems NSColor documentation [2] now also recommends srgb: "Where possible,
it
> is
> > preferable to specify the colorspace explicitly using the
> > colorWithSRGBRed:green:blue:alpha: or colorWithGenericGamma22White:alpha:
> > method."
> > 
> > [1] http://support.apple.com/kb/ht3712
> > [2]
> >
>
https://developer.apple.com/library/mac/documentation/cocoa/Reference/Applica...:
> 
> +1 on fixing the color space. (I believe I suggested "calibrated" here, so
> that's my fault)
> 
> But N.B. that half of our colors still use SkColorToCalibratedNSColor. Not
> directly relevant to the bug, but should we add a presubmit that prevents use
of
> calibrated?

Good idea - I think something like https://codereview.chromium.org/448483003
will do this, but I'll start some discussions first (and update the
cocoa-dos-and-don'ts page). There was some uncertainty when it came up before.
However, since the NSColor documentation has been updated to suggest srgb over
other methods, I think it's more clear.

[tapted, 2014-08-06 03:33:30]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-08-06 03:36:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tapted@chromium.org/440633002/1

[commit-bot: I haz the power, 2014-08-06 07:19:02]

Change committed as 287725

[groby-ooo-7-16, 2014-08-07 00:37:09]

https://codereview.chromium.org/440633002/diff/1/chrome/browser/ui/cocoa/prof...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/440633002/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:188:
ui::NativeTheme::kColorId_DialogBackground));
On 2014/08/06 01:49:08, tapted wrote:
> On 2014/08/06 00:05:25, groby wrote:
> > On 2014/08/04 03:11:47, tapted wrote:
> > > note, I might be looking to change this later too.. But I'll change them
all
> > at
> > > once. Something like kColorId_MenuBackgroundColor might be suitable here.
> > 
> > It'd be nice if the theme called out a color id for dialog backgrounds. (Not
> > that it matters to this CL)
> 
> Maybe a separate colors for NSAlert-style dialog backgrounds, and another for
> bubble-like backgrounds is in order.
> 
> I'll probably end up getting some input from a UX person. The constant that
> NativeThemeMac offers up is quite white. I think, e.g., the bookmark bubble
> looks a bit out of place on Mac. On Linux, it's nice shade of gray that
matches
> the toolbar background.
> 
> Checking iTunes.. it has some bubbles that you get from its _two_ hotdog
buttons
> (one in the status display that only appears on hover). One is quite gray, and
> one is quite white.  So, I guess that's not much help :/. 
> 
> One of my many yaks to shave...

I <3 yaks! ;)

In that vein - maybe there should be a CommonBubbleTheme, and a
CommonDialogTheme? It'd be nice to have those constants centralized.