[Extensions Toolbar] Fix issue with overflowed actions requesting focus

MenuButtons didn't respect the "request_focus_on_press_" bit set in
CustomButtons, which led to them stealing focus from the wrench menu.
Fix this, and add a test.

BUG=522520

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

Cr-Commit-Position: refs/heads/master@{#344510}

11 files changed, 169 insertions, 3 deletions

diff --git a/chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.h b/chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.h
index 0c05c85..0b86ffb 100644
--- a/chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.h
+++ b/chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.h
@@ -35,6 +35,10 @@ class ExtensionToolbarMenuView : public views::View,
   int GetHeightForWidth(int width) const override;
   void Layout() override;
 
+  BrowserActionsContainer* container_for_testing() {
+    return container_;
+  }
+
  private:
   // BrowserActionsContainerObserver:
   void OnBrowserActionDragDone() override;
diff --git a/chrome/browser/ui/views/toolbar/toolbar_action_view.cc b/chrome/browser/ui/views/toolbar/toolbar_action_view.cc
index 3c73ee1..cb45a6d 100644
--- a/chrome/browser/ui/views/toolbar/toolbar_action_view.cc
+++ b/chrome/browser/ui/views/toolbar/toolbar_action_view.cc
@@ -77,9 +77,12 @@ ToolbarActionView::ToolbarActionView(
           ThemeServiceFactory::GetForProfile(profile_)));
 
   // If the button is within a menu, we need to make it focusable in order to
-  // have it accessible via keyboard navigation.
-  if (delegate_->ShownInsideMenu())
+  // have it accessible via keyboard navigation, but it shouldn't request focus
+  // (because that would close the menu).
+  if (delegate_->ShownInsideMenu()) {
+    set_request_focus_on_press(false);
     SetFocusable(true);
+  }
 
   UpdateState();
 }
diff --git a/chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc b/chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc
new file mode 100644
index 0000000..2aa0c99
--- /dev/null
+++ b/chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc
@@ -0,0 +1,123 @@
+// 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.
+
+#include "base/run_loop.h"
+#include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/ui/toolbar/toolbar_actions_bar.h"
+#include "chrome/browser/ui/views/frame/browser_view.h"
+#include "chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.h"
+#include "chrome/browser/ui/views/toolbar/browser_actions_container.h"
+#include "chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.h"
+#include "chrome/browser/ui/views/toolbar/toolbar_view.h"
+#include "chrome/browser/ui/views/toolbar/wrench_menu.h"
+#include "chrome/browser/ui/views/toolbar/wrench_toolbar_button.h"
+#include "chrome/test/base/interactive_test_utils.h"
+#include "extensions/common/feature_switch.h"
+#include "extensions/test/extension_test_message_listener.h"
+
+namespace {
+
+// Tests clicking on an overflowed toolbar action. This is called when the
+// wrench menu is open, and handles actually clicking on the action.
+void TestOverflowedToolbarAction(Browser* browser,
+                                 const base::Closure& quit_closure) {
+  // A bunch of plumbing to safely get at the overflowed toolbar action.
+  ToolbarView* toolbar =
+      BrowserView::GetBrowserViewForBrowser(browser)->toolbar();
+  EXPECT_TRUE(toolbar->IsWrenchMenuShowing());
+  WrenchMenu* wrench_menu = toolbar->wrench_menu_for_testing();
+  ASSERT_TRUE(wrench_menu);
+  ExtensionToolbarMenuView* menu_view =
+      wrench_menu->extension_toolbar_for_testing();
+  ASSERT_TRUE(menu_view);
+  BrowserActionsContainer* overflow_container =
+      menu_view->container_for_testing();
+  ASSERT_TRUE(overflow_container);
+  ToolbarActionView* action_view =
+      overflow_container->GetToolbarActionViewAt(0);
+  EXPECT_TRUE(action_view->visible());
+
+  // Left click on the toolbar action to activate it.
+  gfx::Point action_view_loc =
+      test::GetCenterInScreenCoordinates(action_view);
+  ui_controls::SendMouseMove(action_view_loc.x(), action_view_loc.y());
+  ui_controls::SendMouseEventsNotifyWhenDone(
+      ui_controls::LEFT, ui_controls::DOWN | ui_controls::UP, quit_closure);
+}
+
+}  // namespace
+
+class ToolbarActionViewInteractiveUITest : public ExtensionBrowserTest {
+ protected:
+  ToolbarActionViewInteractiveUITest();
+  ~ToolbarActionViewInteractiveUITest() override;
+
+  // ExtensionBrowserTest:
+  void SetUpCommandLine(base::CommandLine* command_line) override;
+  void TearDownOnMainThread() override;
+
+ private:
+  // Override the extensions-action-redesign switch.
+  scoped_ptr<extensions::FeatureSwitch::ScopedOverride> feature_override_;
+
+  DISALLOW_COPY_AND_ASSIGN(ToolbarActionViewInteractiveUITest);
+};
+
+ToolbarActionViewInteractiveUITest::ToolbarActionViewInteractiveUITest() {}
+ToolbarActionViewInteractiveUITest::~ToolbarActionViewInteractiveUITest() {}
+
+void ToolbarActionViewInteractiveUITest::SetUpCommandLine(
+    base::CommandLine* command_line) {
+  ExtensionBrowserTest::SetUpCommandLine(command_line);
+  // We do this before the rest of the setup because it can affect how the views
+  // are constructed.
+  feature_override_.reset(new extensions::FeatureSwitch::ScopedOverride(
+      extensions::FeatureSwitch::extension_action_redesign(), true));
+  ToolbarActionsBar::disable_animations_for_testing_ = true;
+}
+
+void ToolbarActionViewInteractiveUITest::TearDownOnMainThread() {
+  ToolbarActionsBar::disable_animations_for_testing_ = false;
+}
+
+#if defined(USE_OZONE)
+// ozone bringup - http://crbug.com/401304
+#define MAYBE_TestClickingOnOverflowedAction DISABLED_TestClickingOnOverflowedAction
+#else
+#define MAYBE_TestClickingOnOverflowedAction TestClickingOnOverflowedAction
+#endif
+// Tests clicking on an overflowed extension action.
+IN_PROC_BROWSER_TEST_F(ToolbarActionViewInteractiveUITest,
+                       MAYBE_TestClickingOnOverflowedAction) {
+  // Load an extension.
+  ASSERT_TRUE(LoadExtension(
+      test_data_dir_.AppendASCII("ui").AppendASCII("browser_action_popup")));
+  base::RunLoop().RunUntilIdle();  // Ensure the extension is fully loaded.
+
+  // Reduce visible count to 0 so that the action is overflowed.
+  ToolbarActionsModel::Get(profile())->SetVisibleIconCount(0);
+
+  // When the extension is activated, it will send a message that its popup
+  // opened. Listen for the message.
+  ExtensionTestMessageListener listener("Popup opened", false);
+
+  ToolbarView* toolbar_view =
+      BrowserView::GetBrowserViewForBrowser(browser())->toolbar();
+
+  // Click on the wrench.
+  gfx::Point wrench_button_loc =
+      test::GetCenterInScreenCoordinates(toolbar_view->app_menu());
+  base::RunLoop loop;
+  ui_controls::SendMouseMove(wrench_button_loc.x(), wrench_button_loc.y());
+  ui_controls::SendMouseEventsNotifyWhenDone(
+      ui_controls::LEFT,
+      ui_controls::DOWN | ui_controls::UP,
+      base::Bind(&TestOverflowedToolbarAction, browser(), loop.QuitClosure()));
+  loop.Run();
+  // The wrench menu should no longer me showing.
+  EXPECT_FALSE(toolbar_view->IsWrenchMenuShowing());
+
+  // And the extension should have been activated.
+  listener.WaitUntilSatisfied();
+}
diff --git a/chrome/browser/ui/views/toolbar/toolbar_view.h b/chrome/browser/ui/views/toolbar/toolbar_view.h
index 32eebde..931892c 100644
--- a/chrome/browser/ui/views/toolbar/toolbar_view.h
+++ b/chrome/browser/ui/views/toolbar/toolbar_view.h
@@ -178,6 +178,8 @@ class ToolbarView : public views::AccessiblePaneView,
     kVertSpacing = 5,
   };
 
+  WrenchMenu* wrench_menu_for_testing() { return wrench_menu_.get(); }
+
  protected:
   // AccessiblePaneView:
   bool SetPaneFocusAndFocusDefault() override;
diff --git a/chrome/browser/ui/views/toolbar/wrench_menu.cc b/chrome/browser/ui/views/toolbar/wrench_menu.cc
index 44533b1..7a77e93 100644
--- a/chrome/browser/ui/views/toolbar/wrench_menu.cc
+++ b/chrome/browser/ui/views/toolbar/wrench_menu.cc
@@ -807,6 +807,7 @@ WrenchMenu::WrenchMenu(Browser* browser, int run_flags)
       bookmark_menu_(nullptr),
       feedback_menu_item_(nullptr),
       screenshot_menu_item_(nullptr),
+      extension_toolbar_(nullptr),
       run_flags_(run_flags) {
   registrar_.Add(this, chrome::NOTIFICATION_GLOBAL_ERRORS_CHANGED,
                  content::Source<Profile>(browser_->profile()));
@@ -1136,6 +1137,7 @@ void WrenchMenu::PopulateMenu(MenuItemView* parent,
       case IDC_EXTENSIONS_OVERFLOW_MENU: {
         scoped_ptr<ExtensionToolbarMenuView> extension_toolbar(
             new ExtensionToolbarMenuView(browser_, this));
+        extension_toolbar_ = extension_toolbar.get();
         if (extension_toolbar->ShouldShow())
           item->AddChildView(extension_toolbar.release());
         else
diff --git a/chrome/browser/ui/views/toolbar/wrench_menu.h b/chrome/browser/ui/views/toolbar/wrench_menu.h
index 2cb987c..c4671d4 100644
--- a/chrome/browser/ui/views/toolbar/wrench_menu.h
+++ b/chrome/browser/ui/views/toolbar/wrench_menu.h
@@ -18,6 +18,7 @@
 #include "ui/base/models/menu_model.h"
 #include "ui/views/controls/menu/menu_delegate.h"
 
+class ExtensionToolbarMenuView;
 class BookmarkMenuDelegate;
 class Browser;
 class WrenchMenuObserver;
@@ -110,6 +111,10 @@ class WrenchMenu : public views::MenuDelegate,
                const content::NotificationSource& source,
                const content::NotificationDetails& details) override;
 
+  ExtensionToolbarMenuView* extension_toolbar_for_testing() {
+    return extension_toolbar_;
+  }
+
  private:
   class CutCopyPasteView;
   class RecentTabsMenuModelDelegate;
@@ -179,6 +184,10 @@ class WrenchMenu : public views::MenuDelegate,
   // Menu corresponding to IDC_TAKE_SCREENSHOT.
   views::MenuItemView* screenshot_menu_item_;
 
+  // The view within the IDC_EXTENSIONS_OVERFLOW_MENU item (only present with
+  // the toolbar action redesign enabled).
+  ExtensionToolbarMenuView* extension_toolbar_;
+
   // Used for managing "Recent tabs" menu items.
   scoped_ptr<RecentTabsMenuModelDelegate> recent_tabs_menu_model_delegate_;
 
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 7ed4c16..9831847 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1112,6 +1112,7 @@
       'browser/ui/views/ssl_client_certificate_selector_browsertest.cc',
       'browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc',
       'browser/ui/views/tabs/tab_drag_controller_interactive_uitest.h',
+      'browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc',
       'browser/ui/views/toolbar/toolbar_button_test.cc',
       'browser/ui/views/toolbar/toolbar_view_interactive_uitest.cc',
     ],
diff --git a/chrome/test/data/extensions/ui/browser_action_popup/manifest.json b/chrome/test/data/extensions/ui/browser_action_popup/manifest.json
new file mode 100644
index 0000000..9669f94
--- /dev/null
+++ b/chrome/test/data/extensions/ui/browser_action_popup/manifest.json
@@ -0,0 +1,9 @@
+{
+  "name": "Browser Action Popup",
+  "description": "An extension with a popup that sends a message on opening",
+  "manifest_version": 2,
+  "version": "0.1",
+  "browser_action": {
+    "default_popup": "popup.html"
+  }
+}
diff --git a/chrome/test/data/extensions/ui/browser_action_popup/popup.html b/chrome/test/data/extensions/ui/browser_action_popup/popup.html
new file mode 100644
index 0000000..4c6566e
--- /dev/null
+++ b/chrome/test/data/extensions/ui/browser_action_popup/popup.html
@@ -0,0 +1,7 @@
+<!doctype html>
+<head>
+<script src="popup.js"></script>
+</head>
+<html>
+This is a popup
+</html>
diff --git a/chrome/test/data/extensions/ui/browser_action_popup/popup.js b/chrome/test/data/extensions/ui/browser_action_popup/popup.js
new file mode 100644
index 0000000..d483fac
--- /dev/null
+++ b/chrome/test/data/extensions/ui/browser_action_popup/popup.js
@@ -0,0 +1,5 @@
+// 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.
+
+chrome.test.sendMessage('Popup opened');
diff --git a/ui/views/controls/button/menu_button.cc b/ui/views/controls/button/menu_button.cc
index 614fd5e..dd7e489 100644
--- a/ui/views/controls/button/menu_button.cc
+++ b/ui/views/controls/button/menu_button.cc
@@ -173,7 +173,8 @@ const char* MenuButton::GetClassName() const {
 }
 
 bool MenuButton::OnMousePressed(const ui::MouseEvent& event) {
-  RequestFocus();
+  if (request_focus_on_press())
+    RequestFocus();
   if (state() != STATE_DISABLED && ShouldEnterPushedState(event) &&
       HitTestPoint(event.location())) {
     TimeDelta delta = TimeTicks::Now() - menu_closed_time_;