Fix unwanted underline on three dot menu.

This is due to an incorrect heuristic for showing keyboard menu hints
from menu_runner_impl.cc which checks if the button is currently
focused.

Added a new run type based on whether or not the event that triggered
the app menu is a key event.

Bug: 928678
Change-Id: I6df9159e9a63dedcfa5115fa9ddbfcbd86759d46
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1516777
Commit-Queue: Charlene Yan <cyan@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#641712}

chrome/browser/ui/views/frame/hosted_app_menu_button.cc

@@ -76,7 +76,9 @@ void HostedAppMenuButton::OnMenuButtonClicked(views::MenuButton* source,
                                               const ui::Event* event) {
   Browser* browser = browser_view_->browser();
   RunMenu(std::make_unique<HostedAppMenuModel>(browser_view_, browser), browser,
-          AppMenu::NO_FLAGS, false);
+          event && event->IsKeyEvent() ? AppMenu::SHOW_MNEMONICS
+                                       : AppMenu::NO_FLAGS,
+          false);
 
   // Add UMA for how many times the hosted app menu button are clicked.
   base::RecordAction(

chrome/browser/ui/views/media_router/app_menu_test_api_views.cc

@@ -42,7 +42,7 @@ bool AppMenuTestApiViews::IsMenuShowing() {
 }
 
 void AppMenuTestApiViews::ShowMenu() {
-  GetAppMenuButton()->ShowMenu(false);
+  GetAppMenuButton()->ShowMenu(AppMenu::NO_FLAGS);
 }
 
 void AppMenuTestApiViews::ExecuteCommand(int command) {

chrome/browser/ui/views/toolbar/app_menu.cc

@@ -804,6 +804,9 @@ void AppMenu::Init(ui::MenuModel* model) {
     // BrowserActionsContainer view.
     types |= views::MenuRunner::FOR_DROP | views::MenuRunner::NESTED_DRAG;
   }
+  if (run_flags_ & AppMenu::SHOW_MNEMONICS)
+    types |= views::MenuRunner::SHOULD_SHOW_MNEMONICS;
+
   menu_runner_.reset(new views::MenuRunner(root_, types));
 }
 

chrome/browser/ui/views/toolbar/app_menu.h

@@ -35,10 +35,15 @@ class AppMenu : public views::MenuDelegate,
                 public content::NotificationObserver,
                 public base::SupportsWeakPtr<AppMenu> {
  public:
+  // TODO(cyan): Remove this enum and use MenuRunner::RunTypes instead.
   enum RunFlags {
     NO_FLAGS = 0,
+
     // Indicates that the menu was opened for a drag-and-drop operation.
     FOR_DROP = 1 << 0,
+
+    // Indicates that the menu should show mnemonics.
+    SHOW_MNEMONICS = 1 << 1,
   };
 
   AppMenu(Browser* browser, int run_flags, bool alert_reopen_tab_items);

chrome/browser/ui/views/toolbar/browser_app_menu_button.cc

@@ -218,7 +218,7 @@ void BrowserAppMenuButton::SetPromoFeature(
 }
 #endif
 
-void BrowserAppMenuButton::ShowMenu(bool for_drop) {
+void BrowserAppMenuButton::ShowMenu(int run_types) {
   if (IsMenuShowing())
     return;
 
@@ -234,13 +234,13 @@ void BrowserAppMenuButton::ShowMenu(bool for_drop) {
   alert_reopen_tab_items = promo_feature_ == InProductHelpFeature::kReopenTab;
 #endif
   base::TimeTicks menu_open_time = base::TimeTicks::Now();
+
   RunMenu(
       std::make_unique<AppMenuModel>(toolbar_view_, browser,
                                      toolbar_view_->app_menu_icon_controller()),
-      browser, for_drop ? AppMenu::FOR_DROP : AppMenu::NO_FLAGS,
-      alert_reopen_tab_items);
+      browser, run_types, alert_reopen_tab_items);
 
-  if (!for_drop) {
+  if (!(run_types & AppMenu::FOR_DROP)) {
     // Record the time-to-action for the menu. We don't record in the case of a
     // drag-and-drop command because menus opened for drag-and-drop don't block
     // the message loop.
@@ -370,14 +370,18 @@ bool BrowserAppMenuButton::CanDrop(const ui::OSExchangeData& data) {
 
 void BrowserAppMenuButton::OnDragEntered(const ui::DropTargetEvent& event) {
   DCHECK(!weak_factory_.HasWeakPtrs());
+  int run_types = AppMenu::FOR_DROP;
+  if (event.IsKeyEvent())
+    run_types |= AppMenu::SHOW_MNEMONICS;
+
   if (!g_open_app_immediately_for_testing) {
     base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
         FROM_HERE,
         base::BindOnce(&BrowserAppMenuButton::ShowMenu,
-                       weak_factory_.GetWeakPtr(), true),
+                       weak_factory_.GetWeakPtr(), run_types),
         base::TimeDelta::FromMilliseconds(views::GetMenuShowDelay()));
   } else {
-    ShowMenu(true);
+    ShowMenu(run_types);
   }
 }
 

chrome/browser/ui/views/toolbar/browser_app_menu_button.h

@@ -37,9 +37,9 @@ class BrowserAppMenuButton : public AppMenuButton,
     return type_and_severity_.severity;
   }
 
-  // Shows the app menu. |for_drop| indicates whether the menu is opened for a
-  // drag-and-drop operation.
-  void ShowMenu(bool for_drop);
+  // Shows the app menu. |run_types| denotes the MenuRunner::RunTypes associated
+  // with the menu.
+  void ShowMenu(int run_types);
 
 #if BUILDFLAG(ENABLE_DESKTOP_IN_PRODUCT_HELP)
   // Called to inform the button that it's being used as an anchor for a promo

chrome/browser/ui/views/toolbar/toolbar_view.cc

@@ -39,6 +39,7 @@
 #include "chrome/browser/ui/views/location_bar/star_view.h"
 #include "chrome/browser/ui/views/media_router/cast_toolbar_button.h"
 #include "chrome/browser/ui/views/tabs/tab_strip.h"
+#include "chrome/browser/ui/views/toolbar/app_menu.h"
 #include "chrome/browser/ui/views/toolbar/browser_actions_container.h"
 #include "chrome/browser/ui/views/toolbar/browser_app_menu_button.h"
 #include "chrome/browser/ui/views/toolbar/home_button.h"
@@ -403,7 +404,7 @@ void ToolbarView::OnMenuButtonClicked(views::MenuButton* source,
                                       const ui::Event* event) {
   TRACE_EVENT0("views", "ToolbarView::OnMenuButtonClicked");
   DCHECK_EQ(VIEW_ID_APP_MENU, source->id());
-  app_menu_button_->ShowMenu(false);  // Not for drop.
+  app_menu_button_->ShowMenu(AppMenu::NO_FLAGS);
 }
 
 ////////////////////////////////////////////////////////////////////////////////

ui/views/BUILD.gn

@@ -1023,6 +1023,8 @@ test("views_unittests") {
     "controls/menu/menu_runner_cocoa_unittest.mm",
     "controls/menu/menu_runner_unittest.cc",
     "controls/menu/submenu_view_unittest.cc",
+    "controls/menu/test_menu_item_view.cc",
+    "controls/menu/test_menu_item_view.h",
     "controls/native/native_view_host_mac_unittest.mm",
     "controls/native/native_view_host_test_base.cc",
     "controls/native/native_view_host_test_base.h",

ui/views/controls/menu/menu_item_view_unittest.cc

@@ -15,6 +15,7 @@
 #include "ui/gfx/geometry/insets.h"
 #include "ui/strings/grit/ui_strings.h"
 #include "ui/views/controls/menu/submenu_view.h"
+#include "ui/views/controls/menu/test_menu_item_view.h"
 #include "ui/views/test/menu_test_utils.h"
 #include "ui/views/test/views_test_base.h"
 #include "ui/views/vector_icons.h"
@@ -37,23 +38,8 @@ class SquareView : public views::View {
 
 }  // namespace
 
-// A MenuItemView implementation with a public destructor (so we can clean up
-// in tests).
-class TestMenuItemView : public MenuItemView {
- public:
-  TestMenuItemView() : MenuItemView(NULL) {}
-  ~TestMenuItemView() override {}
-
-  void AddEmptyMenus() { MenuItemView::AddEmptyMenus(); }
-
-  void SetHasMnemonics(bool has_mnemonics) { has_mnemonics_ = has_mnemonics; }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(TestMenuItemView);
-};
-
 TEST(MenuItemViewUnitTest, TestMenuItemViewWithFlexibleWidthChild) {
-  TestMenuItemView root_menu;
+  views::TestMenuItemView root_menu;
   root_menu.set_owned_by_client();
 
   // Append a normal MenuItemView.
@@ -92,7 +78,7 @@ TEST(MenuItemViewUnitTest, TestMenuItemViewWithFlexibleWidthChild) {
 // Tests that the top-level menu item with hidden children should contain the
 // "(empty)" menu item to display.
 TEST(MenuItemViewUnitTest, TestEmptyTopLevelWhenAllItemsAreHidden) {
-  TestMenuItemView root_menu;
+  views::TestMenuItemView root_menu;
   views::MenuItemView* item1 =
       root_menu.AppendMenuItemWithLabel(1, base::ASCIIToUTF16("item 1"));
   views::MenuItemView* item2 =
@@ -123,7 +109,7 @@ TEST(MenuItemViewUnitTest, TestEmptyTopLevelWhenAllItemsAreHidden) {
 // Tests that submenu with hidden children should contain the "(empty)" menu
 // item to display.
 TEST(MenuItemViewUnitTest, TestEmptySubmenuWhenAllChildItemsAreHidden) {
-  TestMenuItemView root_menu;
+  views::TestMenuItemView root_menu;
   MenuItemView* submenu_item =
       root_menu.AppendSubMenu(1, base::ASCIIToUTF16("My Submenu"));
   MenuItemView* child1 = submenu_item->AppendMenuItemWithLabel(
@@ -159,13 +145,13 @@ TEST(MenuItemViewUnitTest, TestEmptySubmenuWhenAllChildItemsAreHidden) {
 }
 
 TEST(MenuItemViewUnitTest, UseMnemonicOnPlatform) {
-  TestMenuItemView root_menu;
+  views::TestMenuItemView root_menu;
   views::MenuItemView* item1 =
       root_menu.AppendMenuItemWithLabel(1, base::ASCIIToUTF16("&Item 1"));
   views::MenuItemView* item2 =
       root_menu.AppendMenuItemWithLabel(2, base::ASCIIToUTF16("I&tem 2"));
 
-  root_menu.SetHasMnemonics(true);
+  root_menu.set_has_mnemonics(true);
 
   if (MenuConfig::instance().use_mnemonics) {
     EXPECT_EQ('i', item1->GetMnemonic());

ui/views/controls/menu/menu_runner.h

@@ -104,6 +104,9 @@ class VIEWS_EXPORT MenuRunner {
     // propagate back to the parent so the combobox content can be edited even
     // while the menu is open.
     EDITABLE_COMBOBOX = 1 << 9,
+
+    // Indicates that the menu should show mnemonics.
+    SHOULD_SHOW_MNEMONICS = 1 << 10,
   };
 
   // Creates a new MenuRunner, which may use a native menu if available.

ui/views/controls/menu/menu_runner_impl.cc

@@ -140,7 +140,7 @@ void MenuRunnerImpl::RunMenuAt(Widget* parent,
   controller_ = controller->AsWeakPtr();
   menu_->set_controller(controller_.get());
   menu_->PrepareForRun(owns_controller_, has_mnemonics,
-                       !for_drop_ && ShouldShowMnemonics(button));
+                       !for_drop_ && ShouldShowMnemonics(button, run_types));
 
   controller->Run(parent, button, menu_, bounds, anchor,
                   (run_types & MenuRunner::CONTEXT_MENU) != 0,
@@ -203,9 +203,10 @@ MenuRunnerImpl::~MenuRunnerImpl() {
     delete *i;
 }
 
-bool MenuRunnerImpl::ShouldShowMnemonics(MenuButton* button) {
+bool MenuRunnerImpl::ShouldShowMnemonics(MenuButton* button,
+                                         int32_t run_types) {
+  bool show_mnemonics = run_types & MenuRunner::SHOULD_SHOW_MNEMONICS;
   // Show mnemonics if the button has focus or alt is pressed.
-  bool show_mnemonics = button ? button->HasFocus() : false;
 #if defined(OS_WIN)
   show_mnemonics |= ui::win::IsAltPressed();
 #elif defined(USE_X11)

ui/views/controls/menu/menu_runner_impl.h

@@ -60,7 +60,7 @@ class VIEWS_EXPORT MenuRunnerImpl : public MenuRunnerImplInterface,
   ~MenuRunnerImpl() override;
 
   // Returns true if mnemonics should be shown in the menu.
-  bool ShouldShowMnemonics(MenuButton* button);
+  bool ShouldShowMnemonics(MenuButton* button, int32_t run_types);
 
   // The menu. We own this. We don't use scoped_ptr as the destructor is
   // protected and we're a friend.

ui/views/controls/menu/menu_runner_unittest.cc

@@ -21,6 +21,7 @@
 #include "ui/views/controls/menu/menu_runner_impl.h"
 #include "ui/views/controls/menu/menu_types.h"
 #include "ui/views/controls/menu/submenu_view.h"
+#include "ui/views/controls/menu/test_menu_item_view.h"
 #include "ui/views/test/menu_test_utils.h"
 #include "ui/views/test/test_views.h"
 #include "ui/views/test/test_views_delegate.h"
@@ -35,28 +36,26 @@ namespace {
 // occurring immediately during the release of ViewsDelegate.
 class DeletingTestViewsDelegate : public views::TestViewsDelegate {
  public:
-  DeletingTestViewsDelegate() : menu_runner_(nullptr) {}
-  ~DeletingTestViewsDelegate() override {}
+  DeletingTestViewsDelegate() = default;
+  ~DeletingTestViewsDelegate() override = default;
 
   void set_menu_runner(views::internal::MenuRunnerImpl* menu_runner) {
     menu_runner_ = menu_runner;
   }
 
   // views::ViewsDelegate:
-  void ReleaseRef() override;
+  void ReleaseRef() override {
+    if (menu_runner_)
+      menu_runner_->Release();
+  }
 
  private:
   // Not owned, deletes itself.
-  views::internal::MenuRunnerImpl* menu_runner_;
+  views::internal::MenuRunnerImpl* menu_runner_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(DeletingTestViewsDelegate);
 };
 
-void DeletingTestViewsDelegate::ReleaseRef() {
-  if (menu_runner_)
-    menu_runner_->Release();
-}
-
 }  // namespace
 
 namespace views {
@@ -64,33 +63,58 @@ namespace test {
 
 class MenuRunnerTest : public ViewsTestBase {
  public:
-  MenuRunnerTest();
-  ~MenuRunnerTest() override;
+  MenuRunnerTest() = default;
+  ~MenuRunnerTest() override = default;
 
   // Initializes the delegates and views needed for a menu. It does not create
   // the MenuRunner.
-  void InitMenuViews();
+  void InitMenuViews() {
+    menu_delegate_ = std::make_unique<TestMenuDelegate>();
+    menu_item_view_ = new views::TestMenuItemView(menu_delegate_.get());
+    menu_item_view_->AppendMenuItemWithLabel(1, base::ASCIIToUTF16("One"));
+    menu_item_view_->AppendMenuItemWithLabel(
+        2, base::WideToUTF16(L"\x062f\x0648"));
+
+    owner_ = std::make_unique<Widget>();
+    Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_POPUP);
+    params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+    owner_->Init(params);
+    owner_->Show();
+  }
 
   // Initializes all delegates and views needed for a menu. A MenuRunner is also
   // created with |run_types|, it takes ownership of |menu_item_view_|.
-  void InitMenuRunner(int32_t run_types);
+  void InitMenuRunner(int32_t run_types) {
+    InitMenuViews();
+    menu_runner_ = std::make_unique<MenuRunner>(menu_item_view_, run_types);
+  }
 
-  MenuItemView* menu_item_view() { return menu_item_view_; }
+  views::TestMenuItemView* menu_item_view() { return menu_item_view_; }
   TestMenuDelegate* menu_delegate() { return menu_delegate_.get(); }
   MenuRunner* menu_runner() { return menu_runner_.get(); }
   Widget* owner() { return owner_.get(); }
 
   // ViewsTestBase:
-  void TearDown() override;
+  void TearDown() override {
+    if (owner_)
+      owner_->CloseNow();
+    ViewsTestBase::TearDown();
+  }
 
   bool IsItemSelected(int command_id) {
     MenuItemView* item = menu_item_view()->GetMenuItemByID(command_id);
     return item ? item->IsSelected() : false;
   }
 
+  // Menus that use prefix selection don't support mnemonics - the input is
+  // always part of the prefix.
+  bool MenuSupportsMnemonics() {
+    return !MenuConfig::instance().all_menus_use_prefix_selection;
+  }
+
  private:
   // Owned by menu_runner_.
-  MenuItemView* menu_item_view_ = nullptr;
+  views::TestMenuItemView* menu_item_view_ = nullptr;
 
   std::unique_ptr<TestMenuDelegate> menu_delegate_;
   std::unique_ptr<MenuRunner> menu_runner_;
@@ -99,35 +123,6 @@ class MenuRunnerTest : public ViewsTestBase {
   DISALLOW_COPY_AND_ASSIGN(MenuRunnerTest);
 };
 
-MenuRunnerTest::MenuRunnerTest() {}
-
-MenuRunnerTest::~MenuRunnerTest() {}
-
-void MenuRunnerTest::InitMenuViews() {
-  menu_delegate_.reset(new TestMenuDelegate);
-  menu_item_view_ = new MenuItemView(menu_delegate_.get());
-  menu_item_view_->AppendMenuItemWithLabel(1, base::ASCIIToUTF16("One"));
-  menu_item_view_->AppendMenuItemWithLabel(2,
-                                           base::WideToUTF16(L"\x062f\x0648"));
-
-  owner_.reset(new Widget);
-  Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_POPUP);
-  params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
-  owner_->Init(params);
-  owner_->Show();
-}
-
-void MenuRunnerTest::InitMenuRunner(int32_t run_types) {
-  InitMenuViews();
-  menu_runner_.reset(new MenuRunner(menu_item_view_, run_types));
-}
-
-void MenuRunnerTest::TearDown() {
-  if (owner_)
-    owner_->CloseNow();
-  ViewsTestBase::TearDown();
-}
-
 // Tests that MenuRunner is still running after the call to RunMenuAt when
 // initialized with , and that MenuDelegate is notified upon
 // the closing of the menu.
@@ -165,9 +160,7 @@ TEST_F(MenuRunnerTest, AsynchronousKeyEventHandling) {
 // Tests that a key press on a US keyboard layout activates the correct menu
 // item.
 TEST_F(MenuRunnerTest, LatinMnemonic) {
-  // Menus that use prefix selection don't support mnemonics - the input is
-  // always part of the prefix.
-  if (MenuConfig::instance().all_menus_use_prefix_selection)
+  if (!MenuSupportsMnemonics())
     return;
 
   views::test::DisableMenuClosureAnimations();
@@ -191,9 +184,7 @@ TEST_F(MenuRunnerTest, LatinMnemonic) {
 // Tests that a key press on a non-US keyboard layout activates the correct menu
 // item. Disabled on Windows because a WM_CHAR event does not activate an item.
 TEST_F(MenuRunnerTest, NonLatinMnemonic) {
-  // Menus that use prefix selection don't support mnemonics - the input is
-  // always part of the prefix.
-  if (MenuConfig::instance().all_menus_use_prefix_selection)
+  if (!MenuSupportsMnemonics())
     return;
 
   views::test::DisableMenuClosureAnimations();
@@ -215,6 +206,30 @@ TEST_F(MenuRunnerTest, NonLatinMnemonic) {
 }
 #endif  // !defined(OS_WIN)
 
+TEST_F(MenuRunnerTest, MenuItemViewShowsMnemonics) {
+  if (!MenuSupportsMnemonics())
+    return;
+
+  InitMenuRunner(MenuRunner::HAS_MNEMONICS | MenuRunner::SHOULD_SHOW_MNEMONICS);
+
+  menu_runner()->RunMenuAt(owner(), nullptr, gfx::Rect(), MENU_ANCHOR_TOPLEFT,
+                           ui::MENU_SOURCE_NONE);
+
+  EXPECT_TRUE(menu_item_view()->show_mnemonics());
+}
+
+TEST_F(MenuRunnerTest, MenuItemViewDoesNotShowMnemonics) {
+  if (!MenuSupportsMnemonics())
+    return;
+
+  InitMenuRunner(MenuRunner::HAS_MNEMONICS);
+
+  menu_runner()->RunMenuAt(owner(), nullptr, gfx::Rect(), MENU_ANCHOR_TOPLEFT,
+                           ui::MENU_SOURCE_NONE);
+
+  EXPECT_FALSE(menu_item_view()->show_mnemonics());
+}
+
 TEST_F(MenuRunnerTest, PrefixSelect) {
   if (!MenuConfig::instance().all_menus_use_prefix_selection)
     return;

ui/views/controls/menu/test_menu_item_view.cc

+// Copyright 2019 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 "ui/views/controls/menu/test_menu_item_view.h"
+
+namespace views {
+
+TestMenuItemView::TestMenuItemView() : MenuItemView(nullptr) {}
+
+TestMenuItemView::TestMenuItemView(MenuDelegate* delegate)
+    : MenuItemView(delegate) {}
+
+TestMenuItemView::~TestMenuItemView() = default;
+
+}  // namespace views

ui/views/controls/menu/test_menu_item_view.h

+// Copyright 2019 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.
+
+#ifndef UI_VIEWS_CONTROLS_MENU_TEST_MENU_ITEM_VIEW_H_
+#define UI_VIEWS_CONTROLS_MENU_TEST_MENU_ITEM_VIEW_H_
+
+#include "ui/views/controls/menu/menu_item_view.h"
+
+namespace views {
+
+class MenuDelegate;
+
+// A MenuItemView implementation with a public destructor (so we can clean up
+// in tests).
+class TestMenuItemView : public MenuItemView {
+ public:
+  TestMenuItemView();
+  explicit TestMenuItemView(MenuDelegate* delegate);
+  ~TestMenuItemView() override;
+
+  using MenuItemView::AddEmptyMenus;
+
+  void set_has_mnemonics(bool has_mnemonics) { has_mnemonics_ = has_mnemonics; }
+
+  bool show_mnemonics() { return show_mnemonics_; }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestMenuItemView);
+};
+
+}  // namespace views
+
+#endif  // UI_VIEWS_CONTROLS_MENU_TEST_MENU_ITEM_VIEW_H_