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

ui/views/controls/menu/menu_controller.cc

Index: ui/views/controls/menu/menu_controller.cc
diff --git a/ui/views/controls/menu/menu_controller.cc b/ui/views/controls/menu/menu_controller.cc
index 54d3410bdd8508a9c8e34d20036bacfa330a401f..c55d23a6bafe5c618583119d320cb6ceeb587abc 100644
--- a/ui/views/controls/menu/menu_controller.cc
+++ b/ui/views/controls/menu/menu_controller.cc
@@ -1904,7 +1904,7 @@ void MenuController::IncrementSelection(int delta) {
     // A menu is selected and open, but none of its children are selected,
     // select the first menu item that is visible and enabled.
     if (item->GetSubmenu()->GetMenuItemCount()) {
-      MenuItemView* to_select = FindFirstSelectableMenuItem(item);
+      MenuItemView* to_select = FindInitialSelectableMenuItem(item, delta);
       if (to_select)
         SetSelection(to_select, SELECTION_DEFAULT);
       return;
@@ -1953,29 +1953,28 @@ void MenuController::IncrementSelection(int delta) {
   }
 }
 
-MenuItemView* MenuController::FindFirstSelectableMenuItem(
-    MenuItemView* parent) {
-  MenuItemView* child = parent->GetSubmenu()->GetMenuItemAt(0);
-  if (!child->visible() || !child->enabled())
-    child = FindNextSelectableMenuItem(parent, 0, 1);
-  return child;
+MenuItemView* MenuController::FindInitialSelectableMenuItem(
+    MenuItemView* parent,
+    int delta) {
+  return FindNextSelectableMenuItem(parent, delta > 0 ? -1 : 0, delta);
 }
 
 MenuItemView* MenuController::FindNextSelectableMenuItem(MenuItemView* parent,
                                                          int index,
                                                          int delta) {
-  int start_index = index;
   int parent_count = parent->GetSubmenu()->GetMenuItemCount();
+  int stop_index = (index + parent_count) % parent_count;
+  bool include_all_items = index + delta < 0 || index < 0;

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

Hm, what's the use of |include_all_items|?

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

Just below. Normally we iterate on all but one current |index|. When
include_all_items is set (this happens for FindInitial calls) we need to
enumerate all items in hope of finding the first enabled (or the last for
reverse iteration).

   // Loop through the menu items skipping any invisible menus. The loop stops
   // when we wrap or find a visible and enabled child.
   do {
     index = (index + delta + parent_count) % parent_count;
-    if (index == start_index)
+    if (index == stop_index && !include_all_items)
       return NULL;
     MenuItemView* child = parent->GetSubmenu()->GetMenuItemAt(index);
     if (child->visible() && child->enabled())
       return child;
-  } while (index != start_index);
+  } while (index != stop_index);
   return NULL;
 }
 
@@ -1985,7 +1984,7 @@ void MenuController::OpenSubmenuChangeSelectionIfCan() {
     return;
   MenuItemView* to_select = NULL;
   if (item->GetSubmenu()->GetMenuItemCount() > 0)
-    to_select = FindFirstSelectableMenuItem(item);
+    to_select = FindInitialSelectableMenuItem(item, 1);
   if (to_select) {
     SetSelection(to_select, SELECTION_UPDATE_IMMEDIATELY);
     return;