diff --git a/chrome/browser/ui/cocoa/extensions/browser_action_button.h b/chrome/browser/ui/cocoa/extensions/browser_action_button.h index fb1378854f6daf..cbf9a97d8e034f 100644 --- a/chrome/browser/ui/cocoa/extensions/browser_action_button.h +++ b/chrome/browser/ui/cocoa/extensions/browser_action_button.h @@ -33,8 +33,13 @@ extern NSString* const kBrowserActionButtonDragEndNotification; // The bridge between the view controller and this object. scoped_ptr viewControllerDelegate_; + // The context menu controller. base::scoped_nsobject contextMenuController_; + // A substitute context menu to use in testing. We need this because normally + // menu code is blocking, making it difficult to test. + NSMenu* testContextMenu_; + // The controller for the browser actions bar that owns this button. Weak. BrowserActionsController* browserActionsController_; @@ -82,6 +87,11 @@ extern NSString* const kBrowserActionButtonDragEndNotification; @end +@interface BrowserActionButton(TestingAPI) +// Sets a context menu to use for testing purposes. +- (void)setTestContextMenu:(NSMenu*)testContextMenu; +@end + @interface BrowserActionCell : ImageButtonCell { @private // The controller for the browser actions bar that owns the button. Weak. diff --git a/chrome/browser/ui/cocoa/extensions/browser_action_button.mm b/chrome/browser/ui/cocoa/extensions/browser_action_button.mm index d86f93949c430d..ccce1451d68d12 100644 --- a/chrome/browser/ui/cocoa/extensions/browser_action_button.mm +++ b/chrome/browser/ui/cocoa/extensions/browser_action_button.mm @@ -8,14 +8,18 @@ #include #include "base/logging.h" +#include "base/memory/weak_ptr.h" #include "base/strings/sys_string_conversions.h" #include "chrome/browser/extensions/extension_context_menu_model.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_window.h" +#import "chrome/browser/ui/cocoa/browser_window_controller.h" #import "chrome/browser/ui/cocoa/extensions/browser_actions_controller.h" #import "chrome/browser/ui/cocoa/themed_window.h" #import "chrome/browser/ui/cocoa/toolbar/toolbar_action_view_delegate_cocoa.h" +#import "chrome/browser/ui/cocoa/toolbar/toolbar_controller.h" +#import "chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller.h" #include "chrome/browser/ui/extensions/extension_action_view_controller.h" #include "chrome/browser/ui/toolbar/toolbar_action_view_controller.h" #include "grit/theme_resources.h" @@ -45,6 +49,9 @@ ToolbarActionViewController* viewController); ~ToolbarActionViewDelegateBridge() override; + // Shows the context menu for the owning action. + void ShowContextMenu(); + private: // ToolbarActionViewDelegateCocoa: ToolbarActionViewController* GetPreferredPopupViewController() override; @@ -52,6 +59,9 @@ void UpdateState() override; NSPoint GetPopupPoint() override; + // A helper method to implement showing the context menu. + void DoShowContextMenu(); + // The owning button. Weak. BrowserActionButton* owner_; @@ -61,6 +71,8 @@ // The ToolbarActionViewController for which this is the delegate. Weak. ToolbarActionViewController* viewController_; + base::WeakPtrFactory weakFactory_; + DISALLOW_COPY_AND_ASSIGN(ToolbarActionViewDelegateBridge); }; @@ -70,7 +82,8 @@ ToolbarActionViewController* viewController) : owner_(owner), controller_(controller), - viewController_(viewController) { + viewController_(viewController), + weakFactory_(this) { viewController_->SetDelegate(this); } @@ -78,9 +91,32 @@ viewController_->SetDelegate(nullptr); } +void ToolbarActionViewDelegateBridge::ShowContextMenu() { + // We should only be showing the context menu in this way if we're doing so + // for an overflowed action. + DCHECK(![owner_ superview]); + + WrenchMenuController* wrenchMenuController = + [[[BrowserWindowController browserWindowControllerForWindow: + [controller_ browser]->window()->GetNativeWindow()] + toolbarController] wrenchMenuController]; + // If the wrench menu is open, we have to first close it. Part of this happens + // asynchronously, so we have to use a posted task to open the next menu. + if ([wrenchMenuController isMenuOpen]) { + [wrenchMenuController cancel]; + base::MessageLoop::current()->PostTask( + FROM_HERE, + base::Bind(&ToolbarActionViewDelegateBridge::DoShowContextMenu, + weakFactory_.GetWeakPtr())); + } else { + DoShowContextMenu(); + } +} + ToolbarActionViewController* ToolbarActionViewDelegateBridge::GetPreferredPopupViewController() { - return viewController_; + return [[controller_ mainButtonForId:viewController_->GetId()] + viewController]; } content::WebContents* ToolbarActionViewDelegateBridge::GetCurrentWebContents() @@ -96,6 +132,27 @@ return [controller_ popupPointForId:[owner_ viewController]->GetId()]; } +void ToolbarActionViewDelegateBridge::DoShowContextMenu() { + NSButton* wrenchButton = + [[[BrowserWindowController browserWindowControllerForWindow: + [controller_ browser]->window()->GetNativeWindow()] + toolbarController] wrenchButton]; + // The point the menu shows matches that of the normal wrench menu - that is, + // the right-left most corner of the menu is left-aligned with the wrench + // button, and the menu is displayed "a little bit" lower. It would be nice to + // be able to avoid the magic '5' here, but since it's built into Cocoa, it's + // not too hopeful. + NSPoint menuPoint = NSMakePoint(0, NSHeight([wrenchButton bounds]) + 5); + + // We set the wrench to be highlighted so it remains "pressed" when the menu + // is running. + [[wrenchButton cell] setHighlighted:YES]; + [[owner_ menu] popUpMenuPositioningItem:nil + atLocation:menuPoint + inView:wrenchButton]; + [[wrenchButton cell] setHighlighted:NO]; +} + @interface BrowserActionCell (Internals) - (void)drawBadgeWithinFrame:(NSRect)frame forWebContents:(content::WebContents*)webContents; @@ -164,6 +221,20 @@ - (BOOL)acceptsFirstResponder { return YES; } +- (void)rightMouseDown:(NSEvent*)theEvent { + // Cocoa doesn't allow menus-running-in-menus, so in order to show the + // context menu for an overflowed action, we close the wrench menu and show + // the context menu over the wrench (similar to what we do for popups). + // Let the main bar's button handle showing the context menu, since the wrench + // menu will close.. + if ([browserActionsController_ isOverflow]) { + [browserActionsController_ mainButtonForId:viewController_->GetId()]-> + viewControllerDelegate_->ShowContextMenu(); + } else { + [super rightMouseDown:theEvent]; + } +} + - (void)mouseDown:(NSEvent*)theEvent { NSPoint location = [self convertPoint:[theEvent locationInWindow] fromView:nil]; @@ -353,6 +424,9 @@ - (NSImage*)compositedImage { } - (NSMenu*)menu { + if (testContextMenu_) + return testContextMenu_; + ui::MenuModel* contextMenu = viewController_->GetContextMenu(); if (!contextMenu) return nil; @@ -362,6 +436,13 @@ - (NSMenu*)menu { return [contextMenuController_ menu]; } +#pragma mark - +#pragma mark Testing Methods + +- (void)setTestContextMenu:(NSMenu*)testContextMenu { + testContextMenu_ = testContextMenu; +} + @end @implementation BrowserActionCell diff --git a/chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm b/chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm new file mode 100644 index 00000000000000..b2745dc6768696 --- /dev/null +++ b/chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm @@ -0,0 +1,219 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#import + +#include "base/memory/scoped_ptr.h" +#include "chrome/browser/extensions/extension_action_test_util.h" +#include "chrome/browser/extensions/extension_browsertest.h" +#include "chrome/browser/extensions/extension_service.h" +#include "chrome/browser/extensions/extension_toolbar_model.h" +#import "chrome/browser/ui/cocoa/browser_window_controller.h" +#import "chrome/browser/ui/cocoa/extensions/browser_action_button.h" +#import "chrome/browser/ui/cocoa/extensions/browser_actions_controller.h" +#import "chrome/browser/ui/cocoa/toolbar/toolbar_controller.h" +#import "chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller.h" +#include "chrome/browser/ui/browser_window.h" +#include "chrome/browser/ui/toolbar/toolbar_actions_bar.h" +#include "chrome/test/base/interactive_test_utils.h" +#include "extensions/common/feature_switch.h" +#import "ui/events/test/cocoa_test_event_utils.h" + +namespace { + +// Returns the center point for a particular |view|. +NSPoint GetCenterPoint(NSView* view) { + NSWindow* window = [view window]; + NSScreen* screen = [window screen]; + DCHECK(screen); + + // Converts the center position of the view into the coordinates accepted + // by ui_controls methods. + NSRect bounds = [view bounds]; + NSPoint center = NSMakePoint(NSMidX(bounds), NSMidY(bounds)); + center = [view convertPoint:center toView:nil]; + center = [window convertBaseToScreen:center]; + return NSMakePoint(center.x, [screen frame].size.height - center.y); +} + +// Moves the mouse (synchronously) to the center of the given |view|. +void MoveMouseToCenter(NSView* view) { + NSPoint centerPoint = GetCenterPoint(view); + base::RunLoop runLoop; + ui_controls::SendMouseMoveNotifyWhenDone( + centerPoint.x, centerPoint.y, runLoop.QuitClosure()); + runLoop.Run(); +} + +} // namespace + +// A simple helper menu delegate that will keep track of if a menu is opened, +// and closes them immediately (which is useful because message loops with +// menus open in cocoa don't play nicely with testing). +@interface MenuHelper : NSObject { + // Whether or not a menu has been opened. This can be reset so the helper can + // be used multiple times. + BOOL menuOpened_; +} + +// Bare-bones init. +- (id)init; + +@property(nonatomic, assign) BOOL menuOpened; + +@end + +@implementation MenuHelper + +- (void)menuWillOpen:(NSMenu*)menu { + menuOpened_ = YES; + [menu cancelTracking]; +} + +- (id)init { + self = [super init]; + return self; +} + +@synthesize menuOpened = menuOpened_; + +@end + +class BrowserActionButtonUiTest : public ExtensionBrowserTest { + protected: + BrowserActionButtonUiTest() {} + ~BrowserActionButtonUiTest() override {} + + void SetUpCommandLine(base::CommandLine* command_line) override { + enable_redesign_.reset(new extensions::FeatureSwitch::ScopedOverride( + extensions::FeatureSwitch::extension_action_redesign(), true)); + ToolbarActionsBar::disable_animations_for_testing_ = true; + } + + void TearDownOnMainThread() override { + enable_redesign_.reset(); + ToolbarActionsBar::disable_animations_for_testing_ = false; + } + + private: + scoped_ptr enable_redesign_; + + DISALLOW_COPY_AND_ASSIGN(BrowserActionButtonUiTest); +}; + +// Simulates a clicks on the action button in the overflow menu, and runs +// |closure| upon completion. +void ClickOnOverflowedAction(ToolbarController* toolbarController, + const base::Closure& closure) { + WrenchMenuController* wrenchMenuController = + [toolbarController wrenchMenuController]; + // The wrench menu should start as open (since that's where the overflowed + // actions are). + EXPECT_TRUE([wrenchMenuController isMenuOpen]); + BrowserActionsController* overflowController = + [wrenchMenuController browserActionsController]; + + ASSERT_TRUE(overflowController); + BrowserActionButton* actionButton = + [overflowController buttonWithIndex:0]; + // The action should be attached to a superview. + EXPECT_TRUE([actionButton superview]); + + // ui_controls:: methods don't play nice when there is an open menu (like the + // wrench menu). Instead, simulate a right click by feeding the event directly + // to the button. + NSPoint centerPoint = GetCenterPoint(actionButton); + NSEvent* mouseEvent = cocoa_test_event_utils::RightMouseDownAtPointInWindow( + centerPoint, [actionButton window]); + [actionButton rightMouseDown:mouseEvent]; + + // This should close the wrench menu. + EXPECT_FALSE([wrenchMenuController isMenuOpen]); + base::MessageLoop::current()->PostTask(FROM_HERE, closure); +} + +// Test that opening a context menu works for both actions on the main bar and +// actions in the overflow menu. +IN_PROC_BROWSER_TEST_F(BrowserActionButtonUiTest, + ContextMenusOnMainAndOverflow) { + // Add an extension with a browser action. + scoped_refptr extension = + extensions::extension_action_test_util::CreateActionExtension( + "browser_action", + extensions::extension_action_test_util::BROWSER_ACTION); + extension_service()->AddExtension(extension.get()); + extensions::ExtensionToolbarModel* model = + extensions::ExtensionToolbarModel::Get(profile()); + ASSERT_EQ(1u, model->toolbar_items().size()); + + ToolbarController* toolbarController = + [[BrowserWindowController + browserWindowControllerForWindow:browser()-> + window()->GetNativeWindow()] + toolbarController]; + ASSERT_TRUE(toolbarController); + + BrowserActionsController* actionsController = + [toolbarController browserActionsController]; + BrowserActionButton* actionButton = [actionsController buttonWithIndex:0]; + ASSERT_TRUE(actionButton); + + // Stub out the action button's normal context menu with a fake one so we + // can track when it opens. + base::scoped_nsobject testContextMenu( + [[NSMenu alloc] initWithTitle:@""]); + base::scoped_nsobject menuHelper([[MenuHelper alloc] init]); + [testContextMenu setDelegate:menuHelper.get()]; + [actionButton setTestContextMenu:testContextMenu.get()]; + + // Right now, the action button should be visible (attached to a superview). + EXPECT_TRUE([actionButton superview]); + + // Move the mouse to the center of the action button, preparing to click. + MoveMouseToCenter(actionButton); + + { + // No menu should yet be open. + EXPECT_FALSE([menuHelper menuOpened]); + base::RunLoop runLoop; + ui_controls::SendMouseEventsNotifyWhenDone( + ui_controls::RIGHT, + ui_controls::DOWN | ui_controls::UP, + runLoop.QuitClosure()); + runLoop.Run(); + // The menu should have opened from the click. + EXPECT_TRUE([menuHelper menuOpened]); + } + + // Reset the menu helper so we can use it again. + [menuHelper setMenuOpened:NO]; + + // Shrink the visible count to be 0. This should hide the action button. + model->SetVisibleIconCount(0); + EXPECT_EQ(nil, [actionButton superview]); + + // Move the mouse over the wrench button. + NSView* wrenchButton = [toolbarController wrenchButton]; + ASSERT_TRUE(wrenchButton); + MoveMouseToCenter(wrenchButton); + + { + // No menu yet (on the browser action). + EXPECT_FALSE([menuHelper menuOpened]); + base::RunLoop runLoop; + // Click on the wrench menu, and pass in a callback to continue the test + // in ClickOnOverflowedAction (Due to the blocking nature of cocoa menus, + // passing in runLoop.QuitClosure() is not sufficient here.) + ui_controls::SendMouseEventsNotifyWhenDone( + ui_controls::LEFT, ui_controls::DOWN | ui_controls::UP, + base::Bind(&ClickOnOverflowedAction, + base::Unretained(toolbarController), + runLoop.QuitClosure())); + runLoop.Run(); + // The menu should have opened. Note that the menu opened on the main bar's + // action button, not the overflow's. Since cocoa doesn't support running + // a menu-within-a-menu, this is what has to happen. + EXPECT_TRUE([menuHelper menuOpened]); + } +} diff --git a/chrome/browser/ui/cocoa/extensions/browser_actions_controller.h b/chrome/browser/ui/cocoa/extensions/browser_actions_controller.h index 4a2ea9a67d2d8c..af71cd3dce2dd8 100644 --- a/chrome/browser/ui/cocoa/extensions/browser_actions_controller.h +++ b/chrome/browser/ui/cocoa/extensions/browser_actions_controller.h @@ -101,6 +101,10 @@ extern NSString* const kBrowserActionVisibilityChangedNotification; // Returns the currently-active web contents. - (content::WebContents*)currentWebContents; +// Returns the BrowserActionButton in the main browser actions container (as +// opposed to the overflow) for the action of the given id. +- (BrowserActionButton*)mainButtonForId:(const std::string&)id; + @end // @interface BrowserActionsController @interface BrowserActionsController(TestingAPI) diff --git a/chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm b/chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm index 94f257689970fc..0ff8de8d16d284 100644 --- a/chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm +++ b/chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm @@ -139,6 +139,9 @@ - (void)chevronItemSelected:(id)menuItem; // Updates the container's grippy cursor based on the number of hidden buttons. - (void)updateGrippyCursors; +// Returns the associated ToolbarController. +- (ToolbarController*)toolbarController; + @end namespace { @@ -236,9 +239,8 @@ void OnOverflowedActionWantsToRunChanged(bool overflowed_action_wants_to_run) void ToolbarActionsBarBridge::OnOverflowedActionWantsToRunChanged( bool overflowed_action_wants_to_run) { - [[[BrowserWindowController browserWindowControllerForWindow: - [controller_ browser]->window()->GetNativeWindow()] toolbarController] - setOverflowedToolbarActionWantsToRun:overflowed_action_wants_to_run]; + [[controller_ toolbarController] + setOverflowedToolbarActionWantsToRun:overflowed_action_wants_to_run]; } } // namespace @@ -353,15 +355,8 @@ - (NSPoint)popupPointForId:(const std::string&)id { NSRect bounds; NSView* referenceButton = button; if ([button superview] != containerView_ || isOverflow_) { - if (toolbarActionsBar_->platform_settings().chevron_enabled) { - referenceButton = chevronMenuButton_.get(); - } else { - referenceButton = - [[[BrowserWindowController - browserWindowControllerForWindow:browser_-> - window()->GetNativeWindow()] - toolbarController] wrenchButton]; - } + referenceButton = toolbarActionsBar_->platform_settings().chevron_enabled ? + chevronMenuButton_.get() : [[self toolbarController] wrenchButton]; bounds = [referenceButton bounds]; } else { bounds = [button convertRect:[button frameAfterAnimation] @@ -408,6 +403,12 @@ - (BOOL)chevronIsHidden { return browser_->tab_strip_model()->GetActiveWebContents(); } +- (BrowserActionButton*)mainButtonForId:(const std::string&)id { + BrowserActionsController* mainController = isOverflow_ ? + [[self toolbarController] browserActionsController] : self; + return [mainController buttonForId:id]; +} + #pragma mark - #pragma mark NSMenuDelegate @@ -819,6 +820,12 @@ - (void)updateGrippyCursors { [[containerView_ window] invalidateCursorRectsForView:containerView_]; } +- (ToolbarController*)toolbarController { + return [[BrowserWindowController browserWindowControllerForWindow: + browser_->window()->GetNativeWindow()] toolbarController]; +} + + #pragma mark - #pragma mark Testing Methods diff --git a/chrome/browser/ui/cocoa/toolbar/toolbar_controller.h b/chrome/browser/ui/cocoa/toolbar/toolbar_controller.h index bda42c14f0ae37..48811abd627ba0 100644 --- a/chrome/browser/ui/cocoa/toolbar/toolbar_controller.h +++ b/chrome/browser/ui/cocoa/toolbar/toolbar_controller.h @@ -169,7 +169,7 @@ class NotificationBridge; - (BrowserActionsController*)browserActionsController; // Returns the wrench button. -- (NSView*)wrenchButton; +- (NSButton*)wrenchButton; // Returns the wrench menu controller. - (WrenchMenuController*)wrenchMenuController; diff --git a/chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller.h b/chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller.h index a5cb21b8960fec..b8b6d368daf2ee 100644 --- a/chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller.h +++ b/chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller.h @@ -87,6 +87,9 @@ class ZoomLevelObserver; // updating the recent tabs submenu. - (void)updateRecentTabsSubmenu; +// Retuns the weak reference to the BrowserActionsController. +- (BrowserActionsController*)browserActionsController; + @end //////////////////////////////////////////////////////////////////////////////// diff --git a/chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller.mm b/chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller.mm index 34d65723772e28..a5e048c96d68db 100644 --- a/chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller.mm +++ b/chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller.mm @@ -321,6 +321,10 @@ - (void)updateRecentTabsSubmenu { } } +- (BrowserActionsController*)browserActionsController { + return browserActionsController_.get(); +} + - (void)createModel { recentTabsMenuModelDelegate_.reset(); wrenchMenuModel_.reset( diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index 1572da0fae04a9..020c19c5f22904 100644 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -908,6 +908,7 @@ 'browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc', 'browser/ui/browser_focus_uitest.cc', 'browser/ui/cocoa/apps/quit_with_apps_controller_mac_interactive_uitest.mm', + 'browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm', 'browser/ui/cocoa/panels/panel_cocoa_browsertest.mm', 'browser/ui/exclusive_access/flash_fullscreen_interactive_browsertest.cc', 'browser/ui/exclusive_access/fullscreen_controller_interactive_browsertest.cc',