Make notification buttons draw a background so text gets subpixel AA.

Make notification buttons draw a background so text gets subpixel AA.

Adds a new NSButton subclass that draws its own background before
rendering itself in the usual way.

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

[dewittj, 2014-02-04 00:33:51]

PTAL

Interestingly drawing the background in -drawRect wasn't enough, the cell needed
to also say it was opaque (even though it didn't draw its own background unless
the mouse is hovering over the button).  LMK if you can see a better way to do
this.

[Robert Sesek, 2014-02-04 14:46:08]

Is just returning YES from -[MCNotificationButtonCell isOpaque] not enough?

https://codereview.chromium.org/131453005/diff/1/ui/message_center/cocoa/noti...
File ui/message_center/cocoa/notification_controller.mm (right):

https://codereview.chromium.org/131453005/diff/1/ui/message_center/cocoa/noti...
ui/message_center/cocoa/notification_controller.mm:59: // drawRect needs to fill
the button with a background, otherwise we don't get
nit: colon after drawRect:

https://codereview.chromium.org/131453005/diff/1/ui/message_center/cocoa/noti...
ui/message_center/cocoa/notification_controller.mm:63:
message_center::kNotificationBackgroundColor) set];
This is a bit awkward looking. I'd pull the NSColor out into a local.

[dewittj, 2014-02-04 17:12:05]

Just returning isOpaque = YES is not enough.  In that case, the NSButton only
draws its background if the mouse is hovering, since the border is only drawn
while the mouse is inside (this is necessary to track the mouse enter and exit
events, judging from the existing comments).  So drawRect needs to draw a
background at least when the mouse isn't hovering.  It seemed simpler to just
always draw the background and then let the border overwrite it if necessary.

[cell setBackground] doesn't work either, since the background color is only
used if the button doesn't have a border.

I suppose the other way to do this is to convert the button into a completely
transparent tracking-only area and painting a text/image view beneath it.  But
that seemed like a lot more work than just drawing the background in the
NSButton subclass.

https://codereview.chromium.org/131453005/diff/1/ui/message_center/cocoa/noti...
File ui/message_center/cocoa/notification_controller.mm (right):

https://codereview.chromium.org/131453005/diff/1/ui/message_center/cocoa/noti...
ui/message_center/cocoa/notification_controller.mm:59: // drawRect needs to fill
the button with a background, otherwise we don't get
On 2014/02/04 14:46:09, rsesek wrote:
> nit: colon after drawRect:

Done.

https://codereview.chromium.org/131453005/diff/1/ui/message_center/cocoa/noti...
ui/message_center/cocoa/notification_controller.mm:63:
message_center::kNotificationBackgroundColor) set];
On 2014/02/04 14:46:09, rsesek wrote:
> This is a bit awkward looking. I'd pull the NSColor out into a local.

Done.

[Robert Sesek, 2014-02-04 19:28:19]

LGTM

On 2014/02/04 17:12:05, dewittj wrote:
> Just returning isOpaque = YES is not enough.  In that case, the NSButton only
> draws its background if the mouse is hovering, since the border is only drawn
> while the mouse is inside (this is necessary to track the mouse enter and exit
> events, judging from the existing comments).  So drawRect needs to draw a
> background at least when the mouse isn't hovering.  It seemed simpler to just
> always draw the background and then let the border overwrite it if necessary.
> 
> [cell setBackground] doesn't work either, since the background color is only
> used if the button doesn't have a border.

Yeah, that's what I was wondering, if using the background color and -isOpaque
would work.

[dewittj, 2014-02-04 19:31:27]

The CQ bit was checked by dewittj@chromium.org

[commit-bot: I haz the power, 2014-02-04 19:41:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/131453005/80001

[commit-bot: I haz the power, 2014-02-05 00:41:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/131453005/80001

[Paweł Hajdan Jr., 2014-02-05 18:51:21]

The CQ bit was unchecked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2014-02-05 19:16:54]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-05 19:24:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/131453005/80001

[commit-bot: I haz the power, 2014-02-05 20:45:08]

Change committed as 249104