Enable keyboard accelerators while a menu is open

ui/views/controls/menu/menu_controller_unittest.cc

Index: ui/views/controls/menu/menu_controller_unittest.cc
diff --git a/ui/views/controls/menu/menu_controller_unittest.cc b/ui/views/controls/menu/menu_controller_unittest.cc
index 3b29d7bc1b4ee2cf40a7da00199d083675f09e54..786b158d2c6127abca20f6460a127c9a0e323c56 100644
--- a/ui/views/controls/menu/menu_controller_unittest.cc
+++ b/ui/views/controls/menu/menu_controller_unittest.cc
@@ -4,28 +4,22 @@
 
 #include "ui/views/controls/menu/menu_controller.h"
 
-#include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "ui/aura/scoped_window_targeter.h"
 #include "ui/aura/window.h"
 #include "ui/events/event_handler.h"
 #include "ui/events/null_event_targeter.h"
-#include "ui/events/platform/platform_event_source.h"
+#include "ui/events/test/event_generator.h"
 #include "ui/views/controls/menu/menu_item_view.h"
 #include "ui/views/controls/menu/submenu_view.h"
 #include "ui/views/test/views_test_base.h"
-#include "ui/wm/public/dispatcher_client.h"
 
-#if defined(OS_WIN)
-#include "base/message_loop/message_pump_dispatcher.h"
-#elif defined(USE_X11)
+#if defined(USE_X11)
 #include <X11/Xlib.h>
 #undef Bool
 #undef None
 #include "ui/events/devices/x11/device_data_manager_x11.h"
 #include "ui/events/test/events_test_utils_x11.h"
-#elif defined(USE_OZONE)
-#include "ui/events/event.h"
 #endif
 
 namespace views {
@@ -41,357 +35,241 @@ class TestMenuItemView : public MenuItemView {
   DISALLOW_COPY_AND_ASSIGN(TestMenuItemView);
 };
 
-class TestPlatformEventSource : public ui::PlatformEventSource {
- public:
-  TestPlatformEventSource() {
-#if defined(USE_X11)
-    ui::DeviceDataManagerX11::CreateInstance();
-#endif
-  }
-  ~TestPlatformEventSource() override {}
-
-  uint32_t Dispatch(const ui::PlatformEvent& event) {
-    return DispatchEvent(event);
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(TestPlatformEventSource);
-};
-
-class TestDispatcherClient : public aura::client::DispatcherClient {
+class TestEventHandler : public ui::EventHandler {
  public:
-  TestDispatcherClient() : dispatcher_(nullptr) {}
-  ~TestDispatcherClient() override {}
+  TestEventHandler() : outstanding_touches_(0) {}
 
-  base::MessagePumpDispatcher* dispatcher() {
-    return dispatcher_;
+  void OnTouchEvent(ui::TouchEvent* event) override {
+    switch(event->type()) {
+      case ui::ET_TOUCH_PRESSED:
+        outstanding_touches_++;
+        break;
+      case ui::ET_TOUCH_RELEASED:
+      case ui::ET_TOUCH_CANCELLED:
+        outstanding_touches_--;
+        break;
+      default:
+        break;
+    }
   }
 
-  // aura::client::DispatcherClient:
-  void PrepareNestedLoopClosures(base::MessagePumpDispatcher* dispatcher,
-                                 base::Closure* run_closure,
-                                 base::Closure* quit_closure) override {
-    scoped_ptr<base::RunLoop> run_loop(new base::RunLoop());
-    *quit_closure = run_loop->QuitClosure();
-    *run_closure = base::Bind(&TestDispatcherClient::RunNestedDispatcher,
-                              base::Unretained(this),
-                              base::Passed(&run_loop),
-                              dispatcher);
-  }
+  int outstanding_touches() const { return outstanding_touches_; }
 
  private:
-  void RunNestedDispatcher(scoped_ptr<base::RunLoop> run_loop,
-                           base::MessagePumpDispatcher* dispatcher) {
-    base::AutoReset<base::MessagePumpDispatcher*> reset_dispatcher(&dispatcher_,
-                                                                   dispatcher);
-    base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
-    base::MessageLoop::ScopedNestableTaskAllower allow(loop);
-    run_loop->Run();
-  }
-
-  base::MessagePumpDispatcher* dispatcher_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestDispatcherClient);
+  int outstanding_touches_;
 };
 
 }  // namespace
 
 class MenuControllerTest : public ViewsTestBase {
  public:
-  MenuControllerTest() : controller_(nullptr) {}
-  ~MenuControllerTest() override { ResetMenuController(); }
-
-  // Dispatches |count| number of items, each in a separate iteration of the
-  // message-loop, by posting a task.
-  void Step3_DispatchEvents(int count) {
-    base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
-    base::MessageLoop::ScopedNestableTaskAllower allow(loop);
-    controller_->exit_type_ = MenuController::EXIT_ALL;
-
-    DispatchEvent();
-    if (count) {
-      base::MessageLoop::current()->PostTask(
-          FROM_HERE,
-          base::Bind(&MenuControllerTest::Step3_DispatchEvents,
-                     base::Unretained(this),
-                     count - 1));
-    } else {
-      EXPECT_TRUE(run_loop_->running());
-      run_loop_->Quit();
-    }
+  MenuControllerTest()
+      : owner_(),
+        event_generator_(),
+        menu_item_(),

[oshima, 2015/06/12 19:59:18]

you can skip these

[afakhry, 2015/06/12 23:50:46]

I know but I always like to be explicit .. however I removed them in this test.

+        menu_controller_(nullptr) {
   }
 
-  // Runs a nested message-loop that does not involve the menu itself.
-  void Step2_RunNestedLoop() {
-    base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
-    base::MessageLoop::ScopedNestableTaskAllower allow(loop);
-    base::MessageLoop::current()->PostTask(
-        FROM_HERE,
-        base::Bind(&MenuControllerTest::Step3_DispatchEvents,
-                   base::Unretained(this),
-                   3));
-    run_loop_.reset(new base::RunLoop());
-    run_loop_->Run();
+  ~MenuControllerTest() override {}
+
+  void SetUp() override {
+    ViewsTestBase::SetUp();
+    Init();
   }
 
-  void Step1_RunMenu() {
-    base::MessageLoop::current()->PostTask(
-        FROM_HERE,
-        base::Bind(&MenuControllerTest::Step2_RunNestedLoop,
-                   base::Unretained(this)));
-    scoped_ptr<Widget> owner(CreateOwnerWidget());
-    RunMenu(owner.get());
+  void TearDown() override {
+    if (menu_controller_) {
+      menu_controller_->owner_ = nullptr;
+      menu_controller_->showing_ = false;

[oshima, 2015/06/12 19:59:18]

do you need these?

[afakhry, 2015/06/12 23:50:46]

Yes the destructor will crash on a DCHECK() if |showing| is not set to false
previously.

+      delete menu_controller_;
+      menu_controller_ = nullptr;

[oshima, 2015/06/12 19:59:18]

any reason why we shouldn't use scoped_ptr for this as well?

[afakhry, 2015/06/12 23:50:46]

Done. Using a scoped_ptr with a specialized Deleter as discussed offline.

+    }
+
+    ViewsTestBase::TearDown();
   }
 
-  scoped_ptr<Widget> CreateOwnerWidget() {
-    scoped_ptr<Widget> widget(new Widget);
-    Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_POPUP);
-    params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
-    widget->Init(params);
-    widget->Show();
+  void ReleaseTouchId(int id) {
+    event_generator_->ReleaseTouchId(id);
+  }
 
-    aura::client::SetDispatcherClient(
-        widget->GetNativeWindow()->GetRootWindow(), &dispatcher_client_);
-    return widget.Pass();
+  void PressKey(ui::KeyboardCode key_code) {
+    event_generator_->PressKey(key_code, 0);
   }
 
-  const MenuItemView* pending_state_item() const {
-    return controller_->pending_state_.item;
+  void TestEventTargeter() {
+    // With the NULL targeter instantiated and assigned we expect the menu to

[oshima, 2015/06/12 19:59:18]

nullptr

[afakhry, 2015/06/12 23:50:46]

Actually NULL here refers to the ui::NullEventTargeter.
I updated the comment.

+    // not handle the key event.
+    scoped_ptr<aura::ScopedWindowTargeter> scoped_targeter(
+        new aura::ScopedWindowTargeter(
+            owner()->GetNativeWindow()->GetRootWindow(),
+            scoped_ptr<ui::EventTargeter>(new ui::NullEventTargeter)));
+    event_generator_->PressKey(ui::VKEY_ESCAPE, 0);
+    EXPECT_EQ(MenuController::EXIT_NONE, menu_exit_type());
+
+    // Now remove the targeter an expect to exit the menu normally.
+    scoped_targeter.reset();
+    event_generator_->PressKey(ui::VKEY_ESCAPE, 0);
+    EXPECT_EQ(MenuController::EXIT_OUTERMOST, menu_exit_type());
   }
 
+ protected:
   void SetPendingStateItem(MenuItemView* item) {
-    controller_->pending_state_.item = item;
+    menu_controller_->pending_state_.item = item;
   }
 
   void ResetSelection() {
-    controller_->SetSelection(nullptr,
-                              MenuController::SELECTION_EXIT |
-                              MenuController::SELECTION_UPDATE_IMMEDIATELY);
+    menu_controller_->SetSelection(
+        nullptr,
+        MenuController::SELECTION_EXIT |
+        MenuController::SELECTION_UPDATE_IMMEDIATELY);
   }
 
   void IncrementSelection(int delta) {
-    controller_->IncrementSelection(delta);
+    menu_controller_->IncrementSelection(delta);
   }
 
   MenuItemView* FindFirstSelectableMenuItem(MenuItemView* parent) {
-    return controller_->FindFirstSelectableMenuItem(parent);
+    return menu_controller_->FindFirstSelectableMenuItem(parent);
   }
 
   MenuItemView* FindNextSelectableMenuItem(MenuItemView* parent,
                                            int index,
                                            int delta) {
-    return controller_->FindNextSelectableMenuItem(parent, index, delta);
-  }
-  void SetupMenu(views::Widget* owner, views::MenuItemView* item) {
-    ResetMenuController();
-    controller_ = new MenuController(nullptr, true, nullptr);
-    controller_->owner_ = owner;
-    controller_->showing_ = true;
-    controller_->SetSelection(item,
-                              MenuController::SELECTION_UPDATE_IMMEDIATELY);
+
+    return menu_controller_->FindNextSelectableMenuItem(parent, index, delta);
   }
 
-  void RunMenu(views::Widget* owner) {
-    scoped_ptr<TestMenuItemView> menu_item(new TestMenuItemView);
-    SetupMenu(owner, menu_item.get());
-    controller_->RunMessageLoop(false);
+  void RunMenu() {
+    menu_controller_->RunMessageLoop(false);
   }
 
-#if defined(USE_X11)
-  void DispatchEscapeAndExpect(MenuController::ExitType exit_type) {
-    ui::ScopedXI2Event key_event;
-    key_event.InitKeyEvent(ui::ET_KEY_PRESSED, ui::VKEY_ESCAPE, 0);
-    event_source_.Dispatch(key_event);
-    EXPECT_EQ(exit_type, controller_->exit_type());
-    controller_->exit_type_ = MenuController::EXIT_ALL;
-    DispatchEvent();
+  Widget* owner() { return owner_.get(); }
+  ui::test::EventGenerator* event_generator() { return event_generator_.get(); }
+  TestMenuItemView* menu_item() { return menu_item_.get(); }
+  MenuController* menu_controller() { return menu_controller_; }
+  const MenuItemView* pending_state_item() const {
+      return menu_controller_->pending_state_.item;
   }
+  const MenuController::ExitType menu_exit_type() const {
+    return menu_controller_->exit_type_;
+  }
+
+ private:
+  void Init() {
+    owner_.reset(new Widget);
+    Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_POPUP);
+    params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+    owner_->Init(params);
+    event_generator_.reset(new ui::test::EventGenerator(
+        owner_->GetNativeWindow()->GetRootWindow(), owner_->GetNativeWindow()));
+    owner_->Show();
 
-  void DispatchTouch(int evtype, int id) {
-    ui::ScopedXI2Event touch_event;
-    std::vector<ui::Valuator> valuators;
-    touch_event.InitTouchEvent(1, evtype, id, gfx::Point(10, 10), valuators);
-    event_source_.Dispatch(touch_event);
-    DispatchEvent();
+    SetupMenuItem();
+
+    SetupMenuController();
   }
-#endif
 
-  void DispatchEvent() {
-#if defined(USE_X11)
-    XEvent xevent;
-    memset(&xevent, 0, sizeof(xevent));
-    event_source_.Dispatch(&xevent);
-#elif defined(OS_WIN)
-    MSG msg;
-    memset(&msg, 0, sizeof(MSG));
-    dispatcher_client_.dispatcher()->Dispatch(msg);
-#elif defined(USE_OZONE)
-    ui::KeyEvent event(' ', ui::VKEY_SPACE, ui::EF_NONE);
-    event_source_.Dispatch(&event);
-#else
-#error Unsupported platform
-#endif
+  void SetupMenuItem() {
+    menu_item_.reset(new TestMenuItemView);
+    menu_item_->AppendMenuItemWithLabel(1, base::ASCIIToUTF16("One"));
+    menu_item_->AppendMenuItemWithLabel(2, base::ASCIIToUTF16("Two"));
+    menu_item_->AppendMenuItemWithLabel(3, base::ASCIIToUTF16("Three"));
+    menu_item_->AppendMenuItemWithLabel(4, base::ASCIIToUTF16("Four"));
   }
 
- private:
-  void ResetMenuController() {
-    if (controller_) {
-      // These properties are faked by RunMenu for the purposes of testing and
-      // need to be undone before we call the destructor.
-      controller_->owner_ = nullptr;
-      controller_->showing_ = false;
-      delete controller_;
-      controller_ = nullptr;
-    }
+  void SetupMenuController() {
+    menu_controller_ = new MenuController(nullptr, true, nullptr);
+    menu_controller_->owner_ = owner_.get();
+    menu_controller_->showing_ = true;
+    menu_controller_->SetSelection(
+        menu_item_.get(), MenuController::SELECTION_UPDATE_IMMEDIATELY);
   }
 
-  // A weak pointer to the MenuController owned by this class.
-  MenuController* controller_;
-  scoped_ptr<base::RunLoop> run_loop_;
-  TestPlatformEventSource event_source_;
-  TestDispatcherClient dispatcher_client_;
+  scoped_ptr<Widget> owner_;
+  scoped_ptr<ui::test::EventGenerator> event_generator_;
+  scoped_ptr<TestMenuItemView> menu_item_;
+  MenuController* menu_controller_;
 
   DISALLOW_COPY_AND_ASSIGN(MenuControllerTest);
 };
 
-TEST_F(MenuControllerTest, Basic) {
-  base::MessageLoop::ScopedNestableTaskAllower allow_nested(
-      base::MessageLoop::current());
-  message_loop()->PostTask(
-      FROM_HERE,
-      base::Bind(&MenuControllerTest::Step1_RunMenu, base::Unretained(this)));
-}
-
-#if defined(OS_LINUX) && defined(USE_X11)
 // Tests that an event targeter which blocks events will be honored by the menu
 // event dispatcher.
 TEST_F(MenuControllerTest, EventTargeter) {
-  {
-    // Verify that the menu handles the escape key under normal circumstances.
-    scoped_ptr<Widget> owner(CreateOwnerWidget());
-    message_loop()->PostTask(
-        FROM_HERE,
-        base::Bind(&MenuControllerTest::DispatchEscapeAndExpect,
-                   base::Unretained(this),
-                   MenuController::EXIT_OUTERMOST));
-    RunMenu(owner.get());
-  }
-
-  {
-    // With the NULL targeter instantiated and assigned we expect the menu to
-    // not handle the key event.
-    scoped_ptr<Widget> owner(CreateOwnerWidget());
-    aura::ScopedWindowTargeter scoped_targeter(
-        owner->GetNativeWindow()->GetRootWindow(),
-        scoped_ptr<ui::EventTargeter>(new ui::NullEventTargeter));
-    message_loop()->PostTask(
-        FROM_HERE,
-        base::Bind(&MenuControllerTest::DispatchEscapeAndExpect,
-                   base::Unretained(this),
-                   MenuController::EXIT_NONE));
-    RunMenu(owner.get());
-  }
+  base::MessageLoopForUI::current()->PostTask(
+  FROM_HERE,
+  base::Bind(&MenuControllerTest::TestEventTargeter,
+      base::Unretained(this)));
+  RunMenu();
 }
-#endif
 
 #if defined(USE_X11)
 
-class TestEventHandler : public ui::EventHandler {
- public:
-  TestEventHandler() : outstanding_touches_(0) {}
-
-  void OnTouchEvent(ui::TouchEvent* event) override {
-    switch(event->type()) {
-      case ui::ET_TOUCH_PRESSED:
-        outstanding_touches_++;
-        break;
-      case ui::ET_TOUCH_RELEASED:
-      case ui::ET_TOUCH_CANCELLED:
-        outstanding_touches_--;
-        break;
-      default:
-        break;
-    }
-  }
-
-  int outstanding_touches() const { return outstanding_touches_; }
-
- private:
-  int outstanding_touches_;
-};
-
 // Tests that touch event ids are released correctly. See
 // crbug.com/439051 for details. When the ids aren't managed
 // correctly, we get stuck down touches.
 TEST_F(MenuControllerTest, TouchIdsReleasedCorrectly) {
-  scoped_ptr<Widget> owner(CreateOwnerWidget());
   TestEventHandler test_event_handler;
-  owner->GetNativeWindow()->GetRootWindow()->AddPreTargetHandler(
+  owner()->GetNativeWindow()->GetRootWindow()->AddPreTargetHandler(
       &test_event_handler);
 
   std::vector<int> devices;
   devices.push_back(1);
   ui::SetUpTouchDevicesForTest(devices);
 
-  DispatchTouch(XI_TouchBegin, 0);
-  DispatchTouch(XI_TouchBegin, 1);
-  DispatchTouch(XI_TouchEnd, 0);
+  event_generator()->PressTouchId(0);
+  event_generator()->PressTouchId(1);
+  event_generator()->ReleaseTouchId(0);
 
-  message_loop()->PostTask(FROM_HERE,
-                           base::Bind(&MenuControllerTest::DispatchTouch,
-                                      base::Unretained(this), XI_TouchEnd, 1));
+  base::MessageLoopForUI::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&MenuControllerTest::ReleaseTouchId,
+                 base::Unretained(this),
+                 1));
 
-  message_loop()->PostTask(
+  base::MessageLoopForUI::current()->PostTask(
       FROM_HERE,
-      base::Bind(&MenuControllerTest::DispatchEscapeAndExpect,
-                 base::Unretained(this), MenuController::EXIT_OUTERMOST));
+      base::Bind(&MenuControllerTest::PressKey,
+                 base::Unretained(this),
+                 ui::VKEY_ESCAPE));
+
+  RunMenu();
 
-  RunMenu(owner.get());
+  EXPECT_EQ(MenuController::EXIT_OUTERMOST, menu_exit_type());
   EXPECT_EQ(0, test_event_handler.outstanding_touches());
 
-  owner->GetNativeWindow()->GetRootWindow()->RemovePreTargetHandler(
+  owner()->GetNativeWindow()->GetRootWindow()->RemovePreTargetHandler(
       &test_event_handler);
 }
-#endif // defined(USE_X11)
 
-TEST_F(MenuControllerTest, FirstSelectedItem) {
-  scoped_ptr<Widget> owner(CreateOwnerWidget());
-  scoped_ptr<TestMenuItemView> menu_item(new TestMenuItemView);
-  menu_item->AppendMenuItemWithLabel(1, base::ASCIIToUTF16("One"));
-  menu_item->AppendMenuItemWithLabel(2, base::ASCIIToUTF16("Two"));
-  // Disabling the item "One" gets it skipped when a menu is first opened.
-  menu_item->GetSubmenu()->GetMenuItemAt(0)->SetEnabled(false);
+#endif  // defined(USE_X11)
 
-  SetupMenu(owner.get(), menu_item.get());
+TEST_F(MenuControllerTest, FirstSelectedItem) {
+  // disabling all items but "Two".
+  menu_item()->GetSubmenu()->GetMenuItemAt(0)->SetEnabled(false);
+  menu_item()->GetSubmenu()->GetMenuItemAt(2)->SetEnabled(false);
+  menu_item()->GetSubmenu()->GetMenuItemAt(3)->SetEnabled(false);
 
-  // First selectable item should be item "Two".
-  MenuItemView* first_selectable = FindFirstSelectableMenuItem(menu_item.get());
+  // "Two" should be the first selectable item.
+  MenuItemView* first_selectable = FindFirstSelectableMenuItem(menu_item());
   EXPECT_EQ(2, first_selectable->GetCommand());
 
   // There should be no next or previous selectable item since there is only a
   // single enabled item in the menu.
   SetPendingStateItem(first_selectable);
-  EXPECT_EQ(nullptr, FindNextSelectableMenuItem(menu_item.get(), 1, 1));
-  EXPECT_EQ(nullptr, FindNextSelectableMenuItem(menu_item.get(), 1, -1));
+  EXPECT_EQ(nullptr, FindNextSelectableMenuItem(menu_item(), 1, 1));
+  EXPECT_EQ(nullptr, FindNextSelectableMenuItem(menu_item(), 1, -1));
 
   // Clear references in menu controller to the menu item that is going away.
   ResetSelection();
 }
 
 TEST_F(MenuControllerTest, NextSelectedItem) {
-  scoped_ptr<Widget> owner(CreateOwnerWidget());
-  scoped_ptr<TestMenuItemView> menu_item(new TestMenuItemView);
-  menu_item->AppendMenuItemWithLabel(1, base::ASCIIToUTF16("One"));
-  menu_item->AppendMenuItemWithLabel(2, base::ASCIIToUTF16("Two"));
-  menu_item->AppendMenuItemWithLabel(3, base::ASCIIToUTF16("Three"));
-  menu_item->AppendMenuItemWithLabel(4, base::ASCIIToUTF16("Four"));
   // Disabling the item "Three" gets it skipped when using keyboard to navigate.
-  menu_item->GetSubmenu()->GetMenuItemAt(2)->SetEnabled(false);
-
-  SetupMenu(owner.get(), menu_item.get());
+  menu_item()->GetSubmenu()->GetMenuItemAt(2)->SetEnabled(false);
 
   // Fake initial hot selection.
-  SetPendingStateItem(menu_item->GetSubmenu()->GetMenuItemAt(0));
+  SetPendingStateItem(menu_item()->GetSubmenu()->GetMenuItemAt(0));
   EXPECT_EQ(1, pending_state_item()->GetCommand());
 
   // Move down in the menu.