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

ui/views/controls/menu/menu_controller_unittest.cc

 // Copyright 2014 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/menu_controller.h"
 
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "ui/events/event_handler.h"
 #include "ui/events/null_event_targeter.h"
@@ skipping 28 matching lines @@
 
 class TestMenuItemView : public MenuItemView {
  public:
   TestMenuItemView() : MenuItemView(nullptr) {}
   ~TestMenuItemView() override {}
 
  private:
   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() {
 #if defined(USE_X11)
     ui::DeviceDataManagerX11::CreateInstance();
 #endif
   }
   ~TestPlatformEventSource() override {}
 
   uint32_t Dispatch(const ui::PlatformEvent& event) {
@@ skipping 104 matching lines @@
 #endif
     return widget.Pass();
   }
 
   const MenuItemView* pending_state_item() const {
     return controller_->pending_state_.item;
   }
 
   void SetPendingStateItem(MenuItemView* item) {
     controller_->pending_state_.item = item;
+    controller_->pending_state_.submenu_open = true;
   }
 
   void ResetSelection() {
     controller_->SetSelection(nullptr,
                               MenuController::SELECTION_EXIT |
                               MenuController::SELECTION_UPDATE_IMMEDIATELY);
   }
 
   void IncrementSelection(int delta) {
     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,
                                            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);
@@ skipping 171 matching lines @@
                  base::Unretained(this), MenuController::EXIT_OUTERMOST));
 
   RunMenu(owner.get());
   EXPECT_EQ(0, test_event_handler.outstanding_touches());
 
   owner->GetNativeWindow()->GetRootWindow()->RemovePreTargetHandler(
       &test_event_handler);
 }
 #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));
 
   // 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);
@@ skipping 33 matching lines @@
   EXPECT_EQ(2, pending_state_item()->GetCommand());
 
   // Select previous item.
   IncrementSelection(-1);
   EXPECT_EQ(1, pending_state_item()->GetCommand());
 
   // Clear references in menu controller to the menu item that is going away.
   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