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 [email protected] Review URL: https://codereview.chromium.org/1310183005 Cr-Commit-Position: refs/heads/master@{#345989} (cherry picked from commit 9c75e37) Review URL: https://codereview.chromium.org/1327203003 . Cr-Commit-Position: refs/branch-heads/2490@{#218} Cr-Branched-From: 7790a35-refs/heads/master@{#344925}
crosswalk-project · Sep 10, 2015 · 1f9f16b · 1f9f16b
1 parent 5659db2
commit 1f9f16b
Show file tree

Hide file tree

Showing 9 changed files with 71 additions and 33 deletions.
diff --git a/chrome/app/nibs/Toolbar.xib b/chrome/app/nibs/Toolbar.xib
@@ -24,51 +24,51 @@
             <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>
                     <connections>
                         <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>
                     <connections>
                         <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>
                     <connections>
                         <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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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