From 1f9f16be3d521b1aad7ba82af84c473e16936e0a Mon Sep 17 00:00:00 2001
From: Andre Santoso <andresantoso@chromium.org>
Date: Wed, 9 Sep 2015 20:27:11 -0700
Subject: [PATCH] Mac: Fix missing focus rings on toolbar, profile, and tab
 buttons

This is a reland of http://crrev.com/1160143010, with some additional tweaks.
It was reverted because it tickled an AppKit crash (crbug.com/504808).
The hope is that the crash won't reappear now that we link with 10.10 SDK.

In 10.7, AppKit introduced new API for drawing focus rings.
This automatic focus ring drawing is automatically enabled for applications linking
with 10.8 sdk or later.
After we link with 10.10 SDK the new style focus ring gets enabled but we get
focus rings with double lines. This is because AppKit thinks the buttons are
bordered (-isBordered is YES) and draws lines inside and outside the borders
that are actually not there due to drawing overrides.

10.6 before: manually drawn, tab close and avatar buttons missing focus rings.
10.6 after: no change.

10.7 before: broken, no focus rings on toolbar, avatar, tab close, new tab buttons.
10.7 after: manually drawn, same as 10.6.

10.8+ before: broken, no focus rings on toolbar, avatar, tab close, new tab buttons.
10.8+ after: drawn by AppKit.

Toolbar.xib changes:
Change the toolbar buttons (back, forward, reload, home, wrench) Focus Ring from
None to Default, to allow the new style focus ring to draw.

BUG=459860, 520471
TBR=rsesek@chromium.org

Review URL: https://codereview.chromium.org/1310183005

Cr-Commit-Position: refs/heads/master@{#345989}
(cherry picked from commit 9c75e3773754cfb242c2819cbfb45bd8b5ddf95f)

Review URL: https://codereview.chromium.org/1327203003 .

Cr-Commit-Position: refs/branch-heads/2490@{#218}
Cr-Branched-From: 7790a3535f2a81a03685eca31a32cf69ae0c114f-refs/heads/master@{#344925}
---
 chrome/app/nibs/Toolbar.xib                   | 20 +++++++++----------
 .../autofill/down_arrow_popup_menu_cell.mm    |  5 +----
 .../cocoa/extensions/browser_action_button.mm |  7 +++++++
 .../browser/ui/cocoa/gradient_button_cell.mm  | 14 +++++++++----
 chrome/browser/ui/cocoa/hover_close_button.mm | 14 ++++++-------
 chrome/browser/ui/cocoa/image_button_cell.mm  | 12 +++++++----
 chrome/browser/ui/cocoa/new_tab_button.mm     | 18 ++++++++++++++---
 .../profiles/avatar_button_controller.mm      |  7 +++++++
 .../ui/cocoa/toolbar/toolbar_button_cocoa.mm  |  7 +++++++
 9 files changed, 71 insertions(+), 33 deletions(-)

diff --git a/chrome/app/nibs/Toolbar.xib b/chrome/app/nibs/Toolbar.xib
index 2f8da15657081..72c3b58a6ab7c 100644
--- a/chrome/app/nibs/Toolbar.xib
+++ b/chrome/app/nibs/Toolbar.xib
@@ -24,18 +24,18 @@
             <rect key="frame" x="0.0" y="0.0" width="604" height="35"/>
             <autoresizingMask key="autoresizingMask" widthSizable="YES" flexibleMinY="YES"/>
             <subviews>
-                <button toolTip="^IDS_APPMENU_TOOLTIP" focusRingType="none" id="38" customClass="MenuButton">
+                <button toolTip="^IDS_APPMENU_TOOLTIP" id="38" customClass="MenuButton">
                     <rect key="frame" x="572" y="4" width="29" height="29"/>
                     <autoresizingMask key="autoresizingMask" flexibleMinX="YES" flexibleMinY="YES"/>
-                    <buttonCell key="cell" type="square" bezelStyle="shadowlessSquare" imagePosition="only" alignment="center" focusRingType="none" inset="2" id="39" customClass="WrenchToolbarButtonCell">
+                    <buttonCell key="cell" type="square" bezelStyle="shadowlessSquare" imagePosition="only" alignment="center" inset="2" id="39" customClass="WrenchToolbarButtonCell">
                         <behavior key="behavior" lightByContents="YES"/>
                         <font key="font" metaFont="system"/>
                     </buttonCell>
                 </button>
-                <button toolTip="^IDS_TOOLTIP_BACK" focusRingType="none" tag="33000" id="2" customClass="MenuButton">
+                <button toolTip="^IDS_TOOLTIP_BACK" tag="33000" id="2" customClass="MenuButton">
                     <rect key="frame" x="3" y="4" width="29" height="29"/>
                     <autoresizingMask key="autoresizingMask" flexibleMaxX="YES" flexibleMinY="YES"/>
-                    <buttonCell key="cell" type="square" bezelStyle="shadowlessSquare" imagePosition="only" alignment="center" state="on" focusRingType="none" inset="2" id="15" customClass="ClickHoldButtonCell">
+                    <buttonCell key="cell" type="square" bezelStyle="shadowlessSquare" imagePosition="only" alignment="center" state="on" inset="2" id="15" customClass="ClickHoldButtonCell">
                         <behavior key="behavior" pushIn="YES" lightByBackground="YES" lightByGray="YES"/>
                         <font key="font" metaFont="system"/>
                     </buttonCell>
@@ -43,10 +43,10 @@
                         <action selector="commandDispatchUsingKeyModifiers:" target="-1" id="138"/>
                     </connections>
                 </button>
-                <button toolTip="^IDS_TOOLTIP_FORWARD" focusRingType="none" tag="33001" id="7" customClass="MenuButton">
+                <button toolTip="^IDS_TOOLTIP_FORWARD" tag="33001" id="7" customClass="MenuButton">
                     <rect key="frame" x="31" y="4" width="29" height="29"/>
                     <autoresizingMask key="autoresizingMask" flexibleMaxX="YES" flexibleMinY="YES"/>
-                    <buttonCell key="cell" type="square" bezelStyle="shadowlessSquare" imagePosition="only" alignment="center" state="on" focusRingType="none" inset="2" id="10" customClass="ClickHoldButtonCell">
+                    <buttonCell key="cell" type="square" bezelStyle="shadowlessSquare" imagePosition="only" alignment="center" state="on" inset="2" id="10" customClass="ClickHoldButtonCell">
                         <behavior key="behavior" pushIn="YES" lightByBackground="YES" lightByGray="YES"/>
                         <font key="font" metaFont="system"/>
                     </buttonCell>
@@ -54,10 +54,10 @@
                         <action selector="commandDispatchUsingKeyModifiers:" target="-1" id="139"/>
                     </connections>
                 </button>
-                <button toolTip="^IDS_TOOLTIP_RELOAD" focusRingType="none" tag="33002" id="3" customClass="ReloadButton">
+                <button toolTip="^IDS_TOOLTIP_RELOAD" tag="33002" id="3" customClass="ReloadButton">
                     <rect key="frame" x="59" y="4" width="29" height="29"/>
                     <autoresizingMask key="autoresizingMask" flexibleMaxX="YES" flexibleMinY="YES"/>
-                    <buttonCell key="cell" type="square" bezelStyle="shadowlessSquare" imagePosition="only" alignment="center" focusRingType="none" inset="2" id="14" customClass="ClickHoldButtonCell">
+                    <buttonCell key="cell" type="square" bezelStyle="shadowlessSquare" imagePosition="only" alignment="center" inset="2" id="14" customClass="ClickHoldButtonCell">
                         <behavior key="behavior" pushIn="YES" lightByBackground="YES" lightByGray="YES"/>
                         <font key="font" metaFont="system"/>
                     </buttonCell>
@@ -65,10 +65,10 @@
                         <action selector="commandDispatchUsingKeyModifiers:" target="-1" id="26"/>
                     </connections>
                 </button>
-                <button toolTip="^IDS_TOOLTIP_HOME" focusRingType="none" tag="33003" id="8" customClass="ToolbarButton">
+                <button toolTip="^IDS_TOOLTIP_HOME" tag="33003" id="8" customClass="ToolbarButton">
                     <rect key="frame" x="87" y="4" width="29" height="29"/>
                     <autoresizingMask key="autoresizingMask" flexibleMaxX="YES" flexibleMinY="YES"/>
-                    <buttonCell key="cell" type="square" bezelStyle="shadowlessSquare" imagePosition="only" alignment="center" focusRingType="none" inset="2" id="9" customClass="ClickHoldButtonCell">
+                    <buttonCell key="cell" type="square" bezelStyle="shadowlessSquare" imagePosition="only" alignment="center" inset="2" id="9" customClass="ClickHoldButtonCell">
                         <behavior key="behavior" pushIn="YES" lightByBackground="YES" lightByGray="YES"/>
                         <font key="font" metaFont="system"/>
                     </buttonCell>
diff --git a/chrome/browser/ui/cocoa/autofill/down_arrow_popup_menu_cell.mm b/chrome/browser/ui/cocoa/autofill/down_arrow_popup_menu_cell.mm
index 0c8b46b2af141..43de6e9d03502 100644
--- a/chrome/browser/ui/cocoa/autofill/down_arrow_popup_menu_cell.mm
+++ b/chrome/browser/ui/cocoa/autofill/down_arrow_popup_menu_cell.mm
@@ -41,10 +41,7 @@ - (void)drawWithFrame:(NSRect)cellFrame inView:(NSView*)controlView {
   if ([title length])
     [self drawTitle:title withFrame:titleRect inView:controlView];
 
-  // Only draw custom focus ring if the 10.7 focus ring APIs are not available.
-  // TODO(groby): Remove once we build against the 10.7 SDK.
-  if (![self respondsToSelector:@selector(drawFocusRingMaskWithFrame:inView:)])
-    [super drawFocusRingWithFrame:cellFrame inView:controlView];
+  [self drawFocusRingWithFrame:cellFrame inView:controlView];
 }
 
 @end
diff --git a/chrome/browser/ui/cocoa/extensions/browser_action_button.mm b/chrome/browser/ui/cocoa/extensions/browser_action_button.mm
index dfaf575ea5a49..893584615417c 100644
--- a/chrome/browser/ui/cocoa/extensions/browser_action_button.mm
+++ b/chrome/browser/ui/cocoa/extensions/browser_action_button.mm
@@ -549,6 +549,13 @@ - (void)drawWithFrame:(NSRect)cellFrame inView:(NSView*)controlView {
                    hints:nil];
 }
 
+- (void)drawFocusRingMaskWithFrame:(NSRect)cellFrame inView:(NSView*)view {
+  // Match the hover image's bezel.
+  [[NSBezierPath bezierPathWithRoundedRect:NSInsetRect(cellFrame, 2, 2)
+                                   xRadius:2
+                                   yRadius:2] fill];
+}
+
 - (ui::ThemeProvider*)themeProviderForWindow:(NSWindow*)window {
   ui::ThemeProvider* themeProvider = [window themeProvider];
   if (!themeProvider)
diff --git a/chrome/browser/ui/cocoa/gradient_button_cell.mm b/chrome/browser/ui/cocoa/gradient_button_cell.mm
index 88b3e4b9215ae..74cb77800081d 100644
--- a/chrome/browser/ui/cocoa/gradient_button_cell.mm
+++ b/chrome/browser/ui/cocoa/gradient_button_cell.mm
@@ -7,6 +7,7 @@
 #include <cmath>
 
 #include "base/logging.h"
+#include "base/mac/mac_util.h"
 #import "base/mac/scoped_nsobject.h"
 #import "chrome/browser/themes/theme_properties.h"
 #import "chrome/browser/themes/theme_service.h"
@@ -528,14 +529,20 @@ - (void)drawWithFrame:(NSRect)cellFrame inView:(NSView*)controlView {
   ui::ThemeProvider* themeProvider = [window themeProvider];
   BOOL active = [window isKeyWindow] || [window isMainWindow];
 
+  // Draw custom focus ring only if AppKit won't draw one automatically.
+  // The new focus ring APIs became available with 10.7, but did not get
+  // applied to buttons (only editable text fields) until 10.8.
+  BOOL shouldDrawFocusRing = base::mac::IsOSLionOrEarlier() &&
+                             [self showsFirstResponder];
+
   // Stroke the borders and appropriate fill gradient. If we're borderless, the
   // only time we want to draw the inner gradient is if we're highlighted or if
-  // we're the first responder (when "Full Keyboard Access" is turned on).
+  // we're drawing the focus ring manually.
   if (([self isBordered] && ![self showsBorderOnlyWhileMouseInside]) ||
       pressed ||
       [self isMouseInside] ||
       [self isContinuousPulsing] ||
-      [self showsFirstResponder]) {
+      shouldDrawFocusRing) {
 
     // When pulsing we want the bookmark to stand out a little more.
     BOOL showClickedGradient = pressed ||
@@ -569,8 +576,7 @@ - (void)drawWithFrame:(NSRect)cellFrame inView:(NSView*)controlView {
   }
   [self drawInteriorWithFrame:innerFrame inView:controlView];
 
-  // Draws the blue focus ring.
-  if ([self showsFirstResponder]) {
+  if (shouldDrawFocusRing) {
     gfx::ScopedNSGraphicsContextSaveGState scoped_state;
     const CGFloat lineWidth = [controlView cr_lineWidth];
     // insetX = 1.0 is used for the drawing of blue highlight so that this
diff --git a/chrome/browser/ui/cocoa/hover_close_button.mm b/chrome/browser/ui/cocoa/hover_close_button.mm
index cd767a84bde77..be82dc52a8a71 100644
--- a/chrome/browser/ui/cocoa/hover_close_button.mm
+++ b/chrome/browser/ui/cocoa/hover_close_button.mm
@@ -96,14 +96,6 @@ - (void)drawRect:(NSRect)dirtyRect {
 
   switch(self.hoverState) {
     case kHoverStateMouseOver:
-      [image drawInRect:destRect
-               fromRect:imageRect
-              operation:NSCompositeSourceOver
-               fraction:1.0
-         respectFlipped:YES
-                  hints:nil];
-      break;
-
     case kHoverStateMouseDown:
       [image drawInRect:destRect
                fromRect:imageRect
@@ -140,6 +132,12 @@ - (void)drawRect:(NSRect)dirtyRect {
   }
 }
 
+- (void)drawFocusRingMask {
+  // Match the hover image's shape.
+  NSRect circleRect = NSInsetRect([self bounds], 2, 2);
+  [[NSBezierPath bezierPathWithOvalInRect:circleRect] fill];
+}
+
 - (void)setFadeOutValue:(CGFloat)value {
   [self setNeedsDisplay];
 }
diff --git a/chrome/browser/ui/cocoa/image_button_cell.mm b/chrome/browser/ui/cocoa/image_button_cell.mm
index ac15515105812..222ba36c7a53b 100644
--- a/chrome/browser/ui/cocoa/image_button_cell.mm
+++ b/chrome/browser/ui/cocoa/image_button_cell.mm
@@ -5,6 +5,7 @@
 #import "chrome/browser/ui/cocoa/image_button_cell.h"
 
 #include "base/logging.h"
+#include "base/mac/mac_util.h"
 #import "chrome/browser/themes/theme_service.h"
 #import "chrome/browser/ui/cocoa/rect_path_utils.h"
 #import "chrome/browser/ui/cocoa/themed_window.h"
@@ -100,10 +101,7 @@ - (void)drawImageWithFrame:(NSRect)cellFrame inView:(NSView*)controlView {
 
 - (void)drawWithFrame:(NSRect)cellFrame inView:(NSView*)controlView {
   [self drawImageWithFrame:cellFrame inView:controlView];
-  // Only draw custom focus ring if the 10.7 focus ring APIs are not available.
-  // TODO(groby): Remove once we build against the 10.7 SDK.
-  if (![self respondsToSelector:@selector(drawFocusRingMaskWithFrame:inView:)])
-    [self drawFocusRingWithFrame:cellFrame inView:controlView];
+  [self drawFocusRingWithFrame:cellFrame inView:controlView];
 }
 
 - (void)setImageID:(NSInteger)imageID
@@ -137,6 +135,12 @@ - (CGFloat)imageAlphaForWindowState:(NSWindow*)window {
 }
 
 - (void)drawFocusRingWithFrame:(NSRect)cellFrame inView:(NSView*)controlView {
+  // Draw custom focus ring only if AppKit won't draw one automatically.
+  // The new focus ring APIs became available with 10.7, but did not get
+  // applied to buttons (only editable text fields) until 10.8.
+  if (base::mac::IsOSMountainLionOrLater())
+    return;
+
   if (![self showsFirstResponder])
     return;
   gfx::ScopedNSGraphicsContextSaveGState scoped_state;
diff --git a/chrome/browser/ui/cocoa/new_tab_button.mm b/chrome/browser/ui/cocoa/new_tab_button.mm
index 176c80c063885..dbdd56ef6328f 100644
--- a/chrome/browser/ui/cocoa/new_tab_button.mm
+++ b/chrome/browser/ui/cocoa/new_tab_button.mm
@@ -8,6 +8,15 @@
 #include "grit/theme_resources.h"
 #include "ui/base/resource/resource_bundle.h"
 
+namespace {
+
+NSImage* GetMaskImage() {
+  ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance();
+  return bundle.GetNativeImageNamed(IDR_NEWTAB_BUTTON_MASK).ToNSImage();
+}
+
+}
+
 // A simple override of the ImageButtonCell to disable handling of
 // -mouseEntered.
 @interface NewTabButtonCell : ImageButtonCell
@@ -22,6 +31,11 @@ - (void)mouseEntered:(NSEvent*)theEvent {
   // Ignore this since the NTB enter is handled by the TabStripController.
 }
 
+- (void)drawFocusRingMaskWithFrame:(NSRect)cellFrame inView:(NSView*)view {
+  // Match the button's shape.
+  [self drawImage:GetMaskImage() withFrame:cellFrame inView:view];
+}
+
 @end
 
 
@@ -34,9 +48,7 @@ + (Class)cellClass {
 - (BOOL)pointIsOverButton:(NSPoint)point {
   NSPoint localPoint = [self convertPoint:point fromView:[self superview]];
   NSRect pointRect = NSMakeRect(localPoint.x, localPoint.y, 1, 1);
-  ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance();
-  NSImage* buttonMask =
-      bundle.GetNativeImageNamed(IDR_NEWTAB_BUTTON_MASK).ToNSImage();
+  NSImage* buttonMask = GetMaskImage();
   NSRect destinationRect = NSMakeRect(
       (NSWidth(self.bounds) - [buttonMask size].width) / 2,
       (NSHeight(self.bounds) - [buttonMask size].height) / 2,
diff --git a/chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm b/chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm
index ba88a32f2b258..6fc1051399325 100644
--- a/chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm
+++ b/chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm
@@ -114,6 +114,13 @@ - (void)drawBezelWithFrame:(NSRect)frame
   ui::DrawNinePartImage(frame, imageIds, NSCompositeSourceOver, 1.0, true);
 }
 
+- (void)drawFocusRingMaskWithFrame:(NSRect)frame inView:(NSView*)view {
+  // Match the bezel's shape.
+  [[NSBezierPath bezierPathWithRoundedRect:NSInsetRect(frame, 2, 2)
+                                   xRadius:2
+                                   yRadius:2] fill];
+}
+
 - (void)setIsThemedWindow:(BOOL)isThemedWindow {
   isThemedWindow_ = isThemedWindow;
 }
diff --git a/chrome/browser/ui/cocoa/toolbar/toolbar_button_cocoa.mm b/chrome/browser/ui/cocoa/toolbar/toolbar_button_cocoa.mm
index 7e971cdf2bb5f..d8c3acc5b34c3 100644
--- a/chrome/browser/ui/cocoa/toolbar/toolbar_button_cocoa.mm
+++ b/chrome/browser/ui/cocoa/toolbar/toolbar_button_cocoa.mm
@@ -48,4 +48,11 @@ - (BOOL)shouldHandleEvent:(NSEvent*)theEvent {
   return handleMiddleClick_ && [theEvent buttonNumber] == 2;
 }
 
+- (void)drawFocusRingMask {
+  // Match the hover image's bezel.
+  [[NSBezierPath bezierPathWithRoundedRect:NSInsetRect([self bounds], 2, 2)
+                                   xRadius:2
+                                   yRadius:2] fill];
+}
+
 @end