Skips disabled menu items when selection is changed

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/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"
 
 #if defined(OS_WIN)
 #include "base/message_loop/message_pump_dispatcher.h"
 #elif defined(USE_X11)
 #include <X11/Xlib.h>
 #undef Bool
 #undef None
 #include "ui/events/devices/x11/device_data_manager_x11.h"
@@ skipping 122 matching lines @@
     Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_POPUP);
     params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
     widget->Init(params);
     widget->Show();
 
     aura::client::SetDispatcherClient(
         widget->GetNativeWindow()->GetRootWindow(), &dispatcher_client_);
     return widget.Pass();
   }
 
+  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(item,
+                              MenuController::SELECTION_UPDATE_IMMEDIATELY);
+  }
+
   void RunMenu(views::Widget* owner) {
     scoped_ptr<TestMenuItemView> menu_item(new TestMenuItemView);
-    ResetMenuController();
-    controller_ = new MenuController(NULL, true, NULL);
-    controller_->owner_ = owner;
-    controller_->showing_ = true;
-    controller_->SetSelection(menu_item.get(),
-                              MenuController::SELECTION_UPDATE_IMMEDIATELY);
+    SetupMenu(owner, menu_item.get());
     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;
@@ skipping 140 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, 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