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..604fcf18a4b1aa19998f3aa3aa2c924d12c101b6 100644
--- a/ui/views/controls/menu/menu_controller_unittest.cc
+++ b/ui/views/controls/menu/menu_controller_unittest.cc
@@ -46,6 +46,26 @@ 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 TestMenuItemViewShown : public MenuItemView {
+ public:
+  TestMenuItemViewShown() : MenuItemView(nullptr) {}

[sadrul, 2015/07/15 19:21:52]

Can it set |submenu_| here? Would that allow not making the changes (which looks
odd) to CreateSubmenu()?

[varkha, 2015/07/15 19:51:34]

It is probably possible but the way CreateSubmenu is written now doesn't really
allow meaningful overrides, does it (since it sets a private member that a
derived class would not see)? This was my motivation for changing it. Do we want
to preserve ability of a MenuItemView decendants to plug their own
implementations of a submenu?

[varkha, 2015/07/16 16:32:33]

I did that in Patch Set #4. Please see if you like PS3 or PS4 better. I think
submenu factory being virtual would only make sense if |submenu_| is truly
hidden but maybe I am missing something. I could also remove virtual attribute
from the Create/Has/GetSubmenu methods. They are never overridden in current
code.

+  ~TestMenuItemViewShown() override {}
+  SubmenuView* CreateSubmenu() override { return new SubmenuViewShown(this); }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestMenuItemViewShown);
+};
+
 class TestPlatformEventSource : public ui::PlatformEventSource {
  public:
   TestPlatformEventSource() {
@@ -170,6 +190,7 @@ class MenuControllerTest : public ViewsTestBase {
 
   void SetPendingStateItem(MenuItemView* item) {
     controller_->pending_state_.item = item;
+    controller_->pending_state_.submenu_open = true;
   }
 
   void ResetSelection() {
@@ -182,8 +203,8 @@ class MenuControllerTest : public ViewsTestBase {
     controller_->IncrementSelection(delta);
   }
 
-  MenuItemView* FindFirstSelectableMenuItem(MenuItemView* parent) {
-    return controller_->FindFirstSelectableMenuItem(parent);
+  MenuItemView* FindInitialSelectableMenuItem(MenuItemView* parent, int delta) {
+    return controller_->FindInitialSelectableMenuItem(parent, delta);
   }
 
   MenuItemView* FindNextSelectableMenuItem(MenuItemView* parent,
@@ -375,23 +396,80 @@ TEST_F(MenuControllerTest, TouchIdsReleasedCorrectly) {
 }
 #endif // defined(USE_X11)
 
-TEST_F(MenuControllerTest, FirstSelectedItem) {
+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 =
+      FindInitialSelectableMenuItem(menu_item.get(), 1);
+  ASSERT_NE(nullptr, first_selectable);
+  EXPECT_EQ(2, first_selectable->GetCommand());
+  // The last selectable item should be item "Three".
+  MenuItemView* last_selectable =
+      FindInitialSelectableMenuItem(menu_item.get(), -1);
+  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 = FindInitialSelectableMenuItem(menu_item.get(), 1);
+  ASSERT_NE(nullptr, first_selectable);
+  EXPECT_EQ(1, first_selectable->GetCommand());
+  // The last selectable item should be item "Two".
+  last_selectable = FindInitialSelectableMenuItem(menu_item.get(), -1);
+  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 = FindInitialSelectableMenuItem(menu_item.get(), 1);
+  ASSERT_NE(nullptr, first_selectable);
+  EXPECT_EQ(1, first_selectable->GetCommand());
+  // The last selectable item should be item "One".
+  last_selectable = FindInitialSelectableMenuItem(menu_item.get(), -1);
+  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 = FindInitialSelectableMenuItem(menu_item.get(), 1);
+  ASSERT_NE(nullptr, first_selectable);
+  EXPECT_EQ(3, first_selectable->GetCommand());
+  // The last selectable item should be item "Three".
+  last_selectable = FindInitialSelectableMenuItem(menu_item.get(), -1);
+  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 = FindInitialSelectableMenuItem(menu_item.get(), 1);
+  ASSERT_NE(nullptr, first_selectable);
   EXPECT_EQ(2, first_selectable->GetCommand());
+  last_selectable = FindInitialSelectableMenuItem(menu_item.get(), -1);
+  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));
 
@@ -445,4 +523,25 @@ TEST_F(MenuControllerTest, NextSelectedItem) {
   ResetSelection();
 }
 
+TEST_F(MenuControllerTest, NextSelectedItemUp) {
+  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.
+  IncrementSelection(-1);
+  EXPECT_EQ(3, pending_state_item()->GetCommand());
+}
+
 }  // namespace views