From 982d085c7b6a26f3d8f5c94ed0cdf90873be0764 Mon Sep 17 00:00:00 2001 From: tapted Date: Thu, 2 Jul 2015 16:05:53 -0700 Subject: Revert of Mac: Fix missing focus rings on toolbar, profile, and tab buttons. (patchset #4 id:100001 of https://codereview.chromium.org/1160143010/) Reason for revert: Tickles an AppKit Bug on OSX 10.8, causing a double free: [Cocoa Zombie] -[NSFocusStack fixInvalidatedFocusForFocusView] Original issue's description: > Mac: Fix missing focus rings on toolbar, profile, and tab buttons. > > In 10.7, AppKit introduced new API for drawing focus rings. > NSControl/NSCell subclasses can opt-in by overriding -focusRingMaskBounds and > -drawFocusRingMask. Implement these methods on the toolbar, profile, new tab and > tab close buttons so that they get focus rings when Full Keyboard Access is > enabled. > > But it turns out that this new focus ring does not work on buttons on 10.7, only on > editable text fields. They do work starting with 10.8. > > 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 > > Committed: https://crrev.com/c72aa7b3cf651695deb3d781e99a9b122806949d > Cr-Commit-Position: refs/heads/master@{#334045} R=rsesek@chromium.org TBR=groby@chromium.org,andresantoso@chromium.org BUG=459860, 504808 Review URL: https://codereview.chromium.org/1220683005 Cr-Commit-Position: refs/heads/master@{#337262} --- chrome/app/nibs/Toolbar.xib | 10 ++++----- .../cocoa/autofill/down_arrow_popup_menu_cell.mm | 5 ++++- chrome/browser/ui/cocoa/hover_close_button.mm | 19 ++++++++--------- chrome/browser/ui/cocoa/image_button_cell.h | 3 --- chrome/browser/ui/cocoa/image_button_cell.mm | 24 ++++++++-------------- chrome/browser/ui/cocoa/new_tab_button.mm | 13 ------------ .../ui/cocoa/profiles/avatar_button_controller.mm | 12 ----------- .../ui/cocoa/toolbar/toolbar_button_cocoa.mm | 12 ----------- 8 files changed, 25 insertions(+), 73 deletions(-) diff --git a/chrome/app/nibs/Toolbar.xib b/chrome/app/nibs/Toolbar.xib index b5e7b9a8..cde4f53 100644 --- a/chrome/app/nibs/Toolbar.xib +++ b/chrome/app/nibs/Toolbar.xib @@ -105,7 +105,7 @@ YES 67108864 - 134217728 + 134250496 LucidaGrande @@ -133,7 +133,7 @@ YES 67108864 - 134217728 + 134250496 @@ -157,7 +157,7 @@ YES -2080374784 - 134217728 + 134250496 @@ -181,7 +181,7 @@ YES -2080374784 - 134217728 + 134250496 @@ -204,7 +204,7 @@ YES 67108864 - 134217728 + 134250496 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 43de6e9..0c8b46b 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,7 +41,10 @@ const int kSidePadding = 2.0; if ([title length]) [self drawTitle:title withFrame:titleRect inView:controlView]; - [self drawFocusRingWithFrame: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:)]) + [super drawFocusRingWithFrame:cellFrame inView:controlView]; } @end diff --git a/chrome/browser/ui/cocoa/hover_close_button.mm b/chrome/browser/ui/cocoa/hover_close_button.mm index fbcb2ec..c2fed30 100644 --- a/chrome/browser/ui/cocoa/hover_close_button.mm +++ b/chrome/browser/ui/cocoa/hover_close_button.mm @@ -93,6 +93,14 @@ NSString* const kFadeOutValueKeyPath = @"fadeOutValue"; 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 @@ -129,17 +137,6 @@ NSString* const kFadeOutValueKeyPath = @"fadeOutValue"; } } -- (NSRect)focusRingMaskBounds { - // This override won't be needed once we link with 10.8+ SDK. - return [self bounds]; -} - -- (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.h b/chrome/browser/ui/cocoa/image_button_cell.h index 7d93d43..e539284 100644 --- a/chrome/browser/ui/cocoa/image_button_cell.h +++ b/chrome/browser/ui/cocoa/image_button_cell.h @@ -75,9 +75,6 @@ enum ButtonState { // Draws the cell's image within |cellFrame|. - (void)drawImageWithFrame:(NSRect)cellFrame inView:(NSView*)controlView; -// Draws |image| centered within |dstRect|. -+ (void)drawImage:(NSImage*)image inRect:(NSRect)dstRect alpha:(CGFloat)alpha; - // If |controlView| is a first responder then draws a blue focus ring. - (void)drawFocusRingWithFrame:(NSRect)cellFrame inView:(NSView*)controlView; diff --git a/chrome/browser/ui/cocoa/image_button_cell.mm b/chrome/browser/ui/cocoa/image_button_cell.mm index 1b43e70..ac15515 100644 --- a/chrome/browser/ui/cocoa/image_button_cell.mm +++ b/chrome/browser/ui/cocoa/image_button_cell.mm @@ -5,7 +5,6 @@ #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" @@ -84,16 +83,12 @@ const CGFloat kImageNoFocusAlpha = 0.65; } } - [ImageButtonCell drawImage:image inRect:cellFrame alpha:alpha]; -} - -+ (void)drawImage:(NSImage*)image inRect:(NSRect)dstRect alpha:(CGFloat)alpha { NSRect imageRect; imageRect.size = [image size]; - imageRect.origin.x = - dstRect.origin.x + roundf((NSWidth(dstRect) - NSWidth(imageRect)) / 2.0); - imageRect.origin.y = dstRect.origin.y + - roundf((NSHeight(dstRect) - NSHeight(imageRect)) / 2.0); + imageRect.origin.x = cellFrame.origin.x + + roundf((NSWidth(cellFrame) - NSWidth(imageRect)) / 2.0); + imageRect.origin.y = cellFrame.origin.y + + roundf((NSHeight(cellFrame) - NSHeight(imageRect)) / 2.0); [image drawInRect:imageRect fromRect:NSZeroRect @@ -105,7 +100,10 @@ const CGFloat kImageNoFocusAlpha = 0.65; - (void)drawWithFrame:(NSRect)cellFrame inView:(NSView*)controlView { [self drawImageWithFrame:cellFrame inView:controlView]; - [self drawFocusRingWithFrame: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]; } - (void)setImageID:(NSInteger)imageID @@ -139,12 +137,6 @@ const CGFloat kImageNoFocusAlpha = 0.65; } - (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 02fd53d..176c80c 100644 --- a/chrome/browser/ui/cocoa/new_tab_button.mm +++ b/chrome/browser/ui/cocoa/new_tab_button.mm @@ -56,19 +56,6 @@ return nil; } -- (NSRect)focusRingMaskBounds { - // This override won't be needed once we link with 10.8+ SDK. - return [self bounds]; -} - -- (void)drawFocusRingMask { - // Match the button's shape. - ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance(); - NSImage* image = - bundle.GetNativeImageNamed(IDR_NEWTAB_BUTTON_MASK).ToNSImage(); - [ImageButtonCell drawImage:image inRect:[self bounds] alpha:1.0]; -} - // ThemedWindowDrawing implementation. - (void)windowDidChangeTheme { diff --git a/chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm b/chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm index 50dae68..ba88a32 100644 --- a/chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm +++ b/chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm @@ -114,18 +114,6 @@ NSImage* GetImageFromResourceID(int resourceId) { ui::DrawNinePartImage(frame, imageIds, NSCompositeSourceOver, 1.0, true); } -- (NSRect)focusRingMaskBoundsForFrame:(NSRect)cellFrame inView:(NSView*)view { - // This override won't be needed once we link with 10.8+ SDK. - return cellFrame; -} - -- (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 320e008..7e971cd 100644 --- a/chrome/browser/ui/cocoa/toolbar/toolbar_button_cocoa.mm +++ b/chrome/browser/ui/cocoa/toolbar/toolbar_button_cocoa.mm @@ -48,16 +48,4 @@ return handleMiddleClick_ && [theEvent buttonNumber] == 2; } -- (NSRect)focusRingMaskBounds { - // This override won't be needed once we link with 10.8+ SDK. - return [self bounds]; -} - -- (void)drawFocusRingMask { - // Match the hover image's bezel. - [[NSBezierPath bezierPathWithRoundedRect:NSInsetRect([self bounds], 2, 2) - xRadius:2 - yRadius:2] fill]; -} - @end -- cgit v1.1