Selects last item in a menu when pressing 'up' initially

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 1fd47a55a8eaf19a4be82451077a4092641fd9d2..60872bf399af31079e99f7d9096d24571cab9fdc 100644
--- a/ui/views/controls/menu/menu_controller_unittest.cc
+++ b/ui/views/controls/menu/menu_controller_unittest.cc
@@ -34,6 +34,7 @@
 #endif
 
 namespace views {
+namespace test {
 
 namespace {
 
@@ -46,6 +47,16 @@ class TestMenuItemView : public MenuItemView {
   DISALLOW_COPY_AND_ASSIGN(TestMenuItemView);
 };
 
+class SubmenuViewShown : public SubmenuView {
+ public:
+  SubmenuViewShown(MenuItemView* parent) : SubmenuView(parent) {}
+  ~SubmenuViewShown() override {}
+  bool IsShowing() override { return true; }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SubmenuViewShown);
+};
+
 class TestPlatformEventSource : public ui::PlatformEventSource {
  public:
   TestPlatformEventSource() {
@@ -103,6 +114,17 @@ class TestDispatcherClient : public aura::client::DispatcherClient {
 
 }  // namespace
 
+class TestMenuItemViewShown : public MenuItemView {
+ public:
+  TestMenuItemViewShown() : MenuItemView(nullptr) {
+    submenu_ = new SubmenuViewShown(this);
+  }
+  ~TestMenuItemViewShown() override {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestMenuItemViewShown);
+};
+
 class MenuControllerTest : public ViewsTestBase {
  public:
   MenuControllerTest() : controller_(nullptr) {}
@@ -170,6 +192,7 @@ class MenuControllerTest : public ViewsTestBase {
 
   void SetPendingStateItem(MenuItemView* item) {
     controller_->pending_state_.item = item;
+    controller_->pending_state_.submenu_open = true;
   }
 
   void ResetSelection() {
@@ -178,19 +201,36 @@ class MenuControllerTest : public ViewsTestBase {
                               MenuController::SELECTION_UPDATE_IMMEDIATELY);
   }
 
-  void IncrementSelection(int delta) {
-    controller_->IncrementSelection(delta);
+  void IncrementSelection() {
+    controller_->IncrementSelection(MenuController::INCREMENT_SELECTION_DOWN);
   }
 
-  MenuItemView* FindFirstSelectableMenuItem(MenuItemView* parent) {
-    return controller_->FindFirstSelectableMenuItem(parent);
+  void DecrementSelection() {
+    controller_->IncrementSelection(MenuController::INCREMENT_SELECTION_UP);
+  }
+
+  MenuItemView* FindInitialSelectableMenuItemDown(MenuItemView* parent) {
+    return controller_->FindInitialSelectableMenuItem(
+        parent, MenuController::INCREMENT_SELECTION_DOWN);
+  }
+
+  MenuItemView* FindInitialSelectableMenuItemUp(MenuItemView* parent) {
+    return controller_->FindInitialSelectableMenuItem(
+        parent, MenuController::INCREMENT_SELECTION_UP);
   }
 
   MenuItemView* FindNextSelectableMenuItem(MenuItemView* parent,
-                                           int index,
-                                           int delta) {
-    return controller_->FindNextSelectableMenuItem(parent, index, delta);
+                                           int index) {
+    return controller_->FindNextSelectableMenuItem(
+        parent, index, MenuController::INCREMENT_SELECTION_DOWN);
+  }
+
+  MenuItemView* FindPreviousSelectableMenuItem(MenuItemView* parent,
+                                               int index) {
+    return controller_->FindNextSelectableMenuItem(
+        parent, index, MenuController::INCREMENT_SELECTION_UP);
   }
+
   void SetupMenu(views::Widget* owner, views::MenuItemView* item) {
     ResetMenuController();
     controller_ = new MenuController(nullptr, true, nullptr);
@@ -341,9 +381,8 @@ class TestEventHandler : public ui::EventHandler {
   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.
+// 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;
@@ -375,30 +414,91 @@ TEST_F(MenuControllerTest, TouchIdsReleasedCorrectly) {
 }
 #endif // defined(USE_X11)
 
-TEST_F(MenuControllerTest, FirstSelectedItem) {
+// Tests that initial selected menu items are correct when items are enabled or
+// disabled.
+TEST_F(MenuControllerTest, InitialSelectedItem) {
   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);
-
+  menu_item->AppendMenuItemWithLabel(3, base::ASCIIToUTF16("Three"));
   SetupMenu(owner.get(), menu_item.get());
 
-  // First selectable item should be item "Two".
-  MenuItemView* first_selectable = FindFirstSelectableMenuItem(menu_item.get());
+  // Leave items "Two" and "Three" enabled.
+  menu_item->GetSubmenu()->GetMenuItemAt(0)->SetEnabled(false);
+  // The first selectable item should be item "Two".
+  MenuItemView* first_selectable =
+      FindInitialSelectableMenuItemDown(menu_item.get());
+  ASSERT_NE(nullptr, first_selectable);
   EXPECT_EQ(2, first_selectable->GetCommand());
+  // The last selectable item should be item "Three".
+  MenuItemView* last_selectable =
+      FindInitialSelectableMenuItemUp(menu_item.get());
+  ASSERT_NE(nullptr, last_selectable);
+  EXPECT_EQ(3, last_selectable->GetCommand());
+
+  // Leave items "One" and "Two" enabled.
+  menu_item->GetSubmenu()->GetMenuItemAt(0)->SetEnabled(true);
+  menu_item->GetSubmenu()->GetMenuItemAt(1)->SetEnabled(true);
+  menu_item->GetSubmenu()->GetMenuItemAt(2)->SetEnabled(false);
+  // The first selectable item should be item "One".
+  first_selectable = FindInitialSelectableMenuItemDown(menu_item.get());
+  ASSERT_NE(nullptr, first_selectable);
+  EXPECT_EQ(1, first_selectable->GetCommand());
+  // The last selectable item should be item "Two".
+  last_selectable = FindInitialSelectableMenuItemUp(menu_item.get());
+  ASSERT_NE(nullptr, last_selectable);
+  EXPECT_EQ(2, last_selectable->GetCommand());
+
+  // Leave only a single item "One" enabled.
+  menu_item->GetSubmenu()->GetMenuItemAt(0)->SetEnabled(true);
+  menu_item->GetSubmenu()->GetMenuItemAt(1)->SetEnabled(false);
+  menu_item->GetSubmenu()->GetMenuItemAt(2)->SetEnabled(false);
+  // The first selectable item should be item "One".
+  first_selectable = FindInitialSelectableMenuItemDown(menu_item.get());
+  ASSERT_NE(nullptr, first_selectable);
+  EXPECT_EQ(1, first_selectable->GetCommand());
+  // The last selectable item should be item "One".
+  last_selectable = FindInitialSelectableMenuItemUp(menu_item.get());
+  ASSERT_NE(nullptr, last_selectable);
+  EXPECT_EQ(1, last_selectable->GetCommand());
+
+  // Leave only a single item "Three" enabled.
+  menu_item->GetSubmenu()->GetMenuItemAt(0)->SetEnabled(false);
+  menu_item->GetSubmenu()->GetMenuItemAt(1)->SetEnabled(false);
+  menu_item->GetSubmenu()->GetMenuItemAt(2)->SetEnabled(true);
+  // The first selectable item should be item "Three".
+  first_selectable = FindInitialSelectableMenuItemDown(menu_item.get());
+  ASSERT_NE(nullptr, first_selectable);
+  EXPECT_EQ(3, first_selectable->GetCommand());
+  // The last selectable item should be item "Three".
+  last_selectable = FindInitialSelectableMenuItemUp(menu_item.get());
+  ASSERT_NE(nullptr, last_selectable);
+  EXPECT_EQ(3, last_selectable->GetCommand());
+
+  // Leave only a single item ("Two") selected. It should be the first and the
+  // last selectable item.
+  menu_item->GetSubmenu()->GetMenuItemAt(0)->SetEnabled(false);
+  menu_item->GetSubmenu()->GetMenuItemAt(1)->SetEnabled(true);
+  menu_item->GetSubmenu()->GetMenuItemAt(2)->SetEnabled(false);
+  first_selectable = FindInitialSelectableMenuItemDown(menu_item.get());
+  ASSERT_NE(nullptr, first_selectable);
+  EXPECT_EQ(2, first_selectable->GetCommand());
+  last_selectable = FindInitialSelectableMenuItemUp(menu_item.get());
+  ASSERT_NE(nullptr, last_selectable);
+  EXPECT_EQ(2, last_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.get(), 1));
+  EXPECT_EQ(nullptr, FindPreviousSelectableMenuItem(menu_item.get(), 1));
 
   // Clear references in menu controller to the menu item that is going away.
   ResetSelection();
 }
 
+// Tests that opening menu and pressing 'Down' and 'Up' iterates over enabled
+// items.
 TEST_F(MenuControllerTest, NextSelectedItem) {
   scoped_ptr<Widget> owner(CreateOwnerWidget());
   scoped_ptr<TestMenuItemView> menu_item(new TestMenuItemView);
@@ -417,32 +517,58 @@ TEST_F(MenuControllerTest, NextSelectedItem) {
 
   // Move down in the menu.
   // Select next item.
-  IncrementSelection(1);
+  IncrementSelection();
   EXPECT_EQ(2, pending_state_item()->GetCommand());
 
   // Skip disabled item.
-  IncrementSelection(1);
+  IncrementSelection();
   EXPECT_EQ(4, pending_state_item()->GetCommand());
 
   // Wrap around.
-  IncrementSelection(1);
+  IncrementSelection();
   EXPECT_EQ(1, pending_state_item()->GetCommand());
 
   // Move up in the menu.
   // Wrap around.
-  IncrementSelection(-1);
+  DecrementSelection();
   EXPECT_EQ(4, pending_state_item()->GetCommand());
 
   // Skip disabled item.
-  IncrementSelection(-1);
+  DecrementSelection();
   EXPECT_EQ(2, pending_state_item()->GetCommand());
 
   // Select previous item.
-  IncrementSelection(-1);
+  DecrementSelection();
   EXPECT_EQ(1, pending_state_item()->GetCommand());
 
   // Clear references in menu controller to the menu item that is going away.
   ResetSelection();
 }
 
+// Tests that opening menu and pressing 'Up' selects the last enabled menu item.
+TEST_F(MenuControllerTest, PreviousSelectedItem) {
+  scoped_ptr<Widget> owner(CreateOwnerWidget());
+  scoped_ptr<TestMenuItemViewShown> menu_item(new TestMenuItemViewShown);
+  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 "Four" gets it skipped when using keyboard to navigate.
+  menu_item->GetSubmenu()->GetMenuItemAt(3)->SetEnabled(false);
+
+  SetupMenu(owner.get(), menu_item.get());
+
+  // Fake initial root item selection and submenu showing.
+  SetPendingStateItem(menu_item.get());
+  EXPECT_EQ(0, pending_state_item()->GetCommand());
+
+  // Move up and select a previous (in our case the last enabled) item.
+  DecrementSelection();
+  EXPECT_EQ(3, pending_state_item()->GetCommand());
+
+  // Clear references in menu controller to the menu item that is going away.
+  ResetSelection();
+}
+
+}  // namespace test
 }  // namespace views