Skips disabled menu items when selection is changed

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 73461e07ede6f408764c77e1af494b8637197817..48ac5709d1fbb789ccb5924f4c956d57c576bed3 100644
--- a/ui/views/controls/menu/menu_controller_unittest.cc
+++ b/ui/views/controls/menu/menu_controller_unittest.cc
@@ -5,12 +5,14 @@
 #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/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"
 
@@ -153,14 +155,30 @@ class MenuControllerTest : public ViewsTestBase {
     return widget.Pass();
   }
 
-  void RunMenu(views::Widget* owner) {
-    scoped_ptr<TestMenuItemView> menu_item(new TestMenuItemView);
+  const MenuItemView* pending_state_item() const {
+    return controller_->pending_state_.item;
+  }
+
+  void SetPendingStateItem(MenuItemView* item) {
+    controller_->pending_state_.item = item;
+  }
+
+  void IncrementSelection(int delta) {
+    controller_->IncrementSelection(delta);
+  }
+
+  void SetupMenu(views::Widget* owner, views::MenuItemView* item) {
     ResetMenuController();
     controller_ = new MenuController(NULL, true, NULL);
     controller_->owner_ = owner;
     controller_->showing_ = true;
-    controller_->SetSelection(menu_item.get(),
+    controller_->SetSelection(item,
                               MenuController::SELECTION_UPDATE_IMMEDIATELY);
+  }
+
+  void RunMenu(views::Widget* owner) {
+    scoped_ptr<TestMenuItemView> menu_item(new TestMenuItemView);
+    SetupMenu(owner, menu_item.get());
     controller_->RunMessageLoop(false);
   }
 
@@ -321,4 +339,47 @@ TEST_F(MenuControllerTest, TouchIdsReleasedCorrectly) {
 }
 #endif // defined(USE_X11)
 
+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 "Two" gets it skipped when using keyboard to navigate.

[sadrul, 2015/05/21 13:44:28]

Item "Three" is actually being disabled, right?

[varkha, 2015/05/21 16:33:12]

Done. Yes, good eye! Forgot to fix this after copy and paste.

+  menu_item->GetSubmenu()->GetMenuItemAt(2)->SetEnabled(false);
+
+  SetupMenu(owner.get(), menu_item.get());
+
+  // Fake initial hot selection.
+  SetPendingStateItem(menu_item->GetSubmenu()->GetMenuItemAt(0));
+  EXPECT_EQ(1, pending_state_item()->GetCommand());
+
+  // Move down in the menu.
+  // Select next item.
+  IncrementSelection(1);
+  EXPECT_EQ(2, pending_state_item()->GetCommand());
+
+  // Skip disabled item.
+  IncrementSelection(1);
+  EXPECT_EQ(4, pending_state_item()->GetCommand());
+
+  // Wrap around.
+  IncrementSelection(1);
+  EXPECT_EQ(1, pending_state_item()->GetCommand());
+
+  // Move up in the menu.
+  // Wrap around.
+  IncrementSelection(-1);
+  EXPECT_EQ(4, pending_state_item()->GetCommand());
+
+  // Skip disabled item.
+  IncrementSelection(-1);
+  EXPECT_EQ(2, pending_state_item()->GetCommand());
+
+  // Select previous item.
+  IncrementSelection(-1);
+  EXPECT_EQ(1, pending_state_item()->GetCommand());
+}

[sadrul, 2015/05/21 13:44:27]

Would it be possible to add another test case where the first item of a
menu/submenu is disabled, and the test verifies that the right item is selected
when the [sub]menu opens?

[varkha, 2015/05/21 16:33:12]

Done.

+
 }  // namespace views