Enables hot-tracking for overflow extension buttons in the app menu

ui/views/controls/menu/menu_controller.cc

 // Copyright (c) 2012 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/i18n/case_conversion.h"
 #include "base/i18n/rtl.h"
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 35 matching lines @@
 #endif
 
 #if defined(USE_AURA)
 #include "ui/views/controls/menu/menu_key_event_handler.h"
 #endif
 
 using base::Time;
 using base::TimeDelta;
 using ui::OSExchangeData;
 
-// Period of the scroll timer (in milliseconds).
-static const int kScrollTimerMS = 30;
-
-// Amount of time from when the drop exits the menu and the menu is hidden.
-static const int kCloseOnExitTime = 1200;
-
-// If a context menu is invoked by touch, we shift the menu by this offset so
-// that the finger does not obscure the menu.
-static const int kCenteredContextMenuYOffset = -15;
-
 namespace views {
 
 namespace {
 
 // When showing context menu on mouse down, the user might accidentally select
 // the menu item on the subsequent mouse up. To prevent this, we add the
 // following delay before the user is able to select an item.
-static int menu_selection_hold_time_ms = kMinimumMsPressedToActivate;
+int menu_selection_hold_time_ms = kMinimumMsPressedToActivate;
+
+// Period of the scroll timer (in milliseconds).
+const int kScrollTimerMS = 30;
+
+// Amount of time from when the drop exits the menu and the menu is hidden.
+const int kCloseOnExitTime = 1200;
+
+// If a context menu is invoked by touch, we shift the menu by this offset so
+// that the finger does not obscure the menu.
+const int kCenteredContextMenuYOffset = -15;
 
 // The spacing offset for the bubble tip.
 const int kBubbleTipSizeLeftRight = 12;
 const int kBubbleTipSizeTopBottom = 11;
 
 // The maximum distance (in DIPS) that the mouse can be moved before it should
 // trigger a mouse menu item activation (regardless of how long the menu has
 // been showing).
 const float kMaximumLengthMovedToActivate = 4.0f;
 
 // Returns true if the mnemonic of |menu| matches key.
 bool MatchesMnemonic(MenuItemView* menu, base::char16 key) {
   return key != 0 && menu->GetMnemonic() == key;
 }
 
 // Returns true if |menu| doesn't have a mnemonic and first character of the its
 // title is |key|.
 bool TitleMatchesMnemonic(MenuItemView* menu, base::char16 key) {
   if (menu->GetMnemonic())
     return false;
 
   base::string16 lower_title = base::i18n::ToLower(menu->title());
   return !lower_title.empty() && lower_title[0] == key;
 }
 
 // Returns the first descendant of |view| that is hot tracked.
-static CustomButton* GetFirstHotTrackedView(View* view) {
+CustomButton* GetFirstHotTrackedView(View* view) {
   if (!view)
     return NULL;
   CustomButton* button = CustomButton::AsCustomButton(view);
   if (button) {
     if (button->IsHotTracked())
       return button;
   }
 
   for (int i = 0; i < view->child_count(); ++i) {
     CustomButton* hot_view = GetFirstHotTrackedView(view->child_at(i));
     if (hot_view)
       return hot_view;
   }
   return NULL;
 }
 
+// Returns a CustomButton descendant of |view| that is (a) hot-tracked and
+// (b) not equal to |hot_button|. Returns null if such descendant is not found.
+// This allows a caller to detect when a button is set to be hot-tracked by a
+// mouse event and its |hot_button_| indicator becomes stale.
+CustomButton* GetFirstHotTrackedViewDifferingFrom(View* view,
+                                                  CustomButton* hot_button) {
+  if (!view)
+    return nullptr;
+  CustomButton* button = CustomButton::AsCustomButton(view);
+  if (button && button != hot_button && button->IsHotTracked())
+    return button;
+
+  for (int i = 0; i < view->child_count(); ++i) {
+    button = GetFirstHotTrackedViewDifferingFrom(view->child_at(i), hot_button);
+    if (button)
+      return button;
+  }
+  return nullptr;
+}
+
 // Recurses through the child views of |view| returning the first view starting
 // at |start| that is focusable. A value of -1 for |start| indicates to start at
 // the first view (if |forward| is false, iterating starts at the last view). If
 // |forward| is true the children are considered first to last, otherwise last
 // to first.
-static View* GetFirstFocusableView(View* view, int start, bool forward) {
+View* GetFirstFocusableView(View* view, int start, bool forward) {
   if (forward) {
     for (int i = start == -1 ? 0 : start; i < view->child_count(); ++i) {
       View* deepest = GetFirstFocusableView(view->child_at(i), -1, forward);
       if (deepest)
         return deepest;
     }
   } else {
     for (int i = start == -1 ? view->child_count() - 1 : start; i >= 0; --i) {
       View* deepest = GetFirstFocusableView(view->child_at(i), -1, forward);
       if (deepest)
         return deepest;
     }
   }
   return view->IsFocusable() ? view : NULL;
 }
 
 // Returns the first child of |start| that is focusable.
-static View* GetInitialFocusableView(View* start, bool forward) {
+View* GetInitialFocusableView(View* start, bool forward) {
   return GetFirstFocusableView(start, -1, forward);
 }
 
 // Returns the next view after |start_at| that is focusable. Returns NULL if
 // there are no focusable children of |ancestor| after |start_at|.
-static View* GetNextFocusableView(View* ancestor,
-                                  View* start_at,
-                                  bool forward) {
+View* GetNextFocusableView(View* ancestor, View* start_at, bool forward) {
   DCHECK(ancestor->Contains(start_at));
   View* parent = start_at;
   do {
     View* new_parent = parent->parent();
     int index = new_parent->GetIndexOf(parent);
     index += forward ? 1 : -1;
     if (forward || index != -1) {
       View* next = GetFirstFocusableView(new_parent, index, forward);
       if (next)
         return next;
@@ skipping 634 matching lines @@
 View* MenuController::GetTooltipHandlerForPoint(SubmenuView* source,
                                                 const gfx::Point& point) {
   MenuHostRootView* root_view = GetRootView(source, point);
   return root_view ? root_view->ProcessGetTooltipHandlerForPoint(point)
                    : nullptr;
 }
 
 void MenuController::ViewHierarchyChanged(
     SubmenuView* source,
     const View::ViewHierarchyChangedDetails& details) {
-  // If the current mouse handler is removed, remove it as the handler.
-  if (!details.is_add && details.child == current_mouse_event_target_) {
-    current_mouse_event_target_ = nullptr;
-    current_mouse_pressed_state_ = 0;
+  if (!details.is_add) {
+    // If the current mouse handler is removed, remove it as the handler.
+    if (details.child == current_mouse_event_target_) {
+      current_mouse_event_target_ = nullptr;
+      current_mouse_pressed_state_ = 0;
+    }
+    // Update |hot_button_| if it gets deleted while a menu is up.
+    if (details.child == hot_button_)
+      hot_button_ = nullptr;
   }
 }
 
 bool MenuController::GetDropFormats(
       SubmenuView* source,
       int* formats,
       std::set<ui::Clipboard::FormatType>* format_types) {
   return source->GetMenuItem()->GetDelegate()->GetDropFormats(
       source->GetMenuItem(), formats, format_types);
 }
@@ skipping 210 matching lines @@
   BuildPathsAndCalculateDiff(pending_state_.item, menu_item, &current_path,
                              &new_path, &paths_differ_at);
 
   size_t current_size = current_path.size();
   size_t new_size = new_path.size();
 
   bool pending_item_changed = pending_state_.item != menu_item;
   if (pending_item_changed && pending_state_.item) {
     CustomButton* button = GetFirstHotTrackedView(pending_state_.item);
     if (button)
-      button->SetHotTracked(false);
+      SetHotTrackedButton(button, false);
+  }
+  // A button could become hot-tracked when a mouse is moved without changing

[sky, 2016/02/23 20:11:13]

I still don't get what case you are hitting that is requiring the change. What
specific case are you fixing?

[varkha, 2016/02/23 21:01:39]

This scenario:
1. Open app menu with extension buttons overflow
2. Place mouse pointer over one of them (triggers hot-tracking)
Call that "button1"
3. Move visible focus with a keyboard (down) to the next extension button (call
that button2)
4. Move a mouse just enough to re-trigger a hot-tracked state on "button1".
5. Press [arrow down] key again.

This should move to "button2" but moves instead to "button3" (since step 4 did
not update the hot-tracked state in menu controller and the last updated state
in MenuController refers to step 3).

This is not affecting just the extension overflow but any menu items with
sub-views such as zoom panel or copy-cut-paste. This may result in multiple
hot-tracked visually active views as well.

+  // which MenuItem is selected. When a menu item selection is updated, presence
+  // of a hot-tracked button that is not |hot_button_| can mean only that a new
+  // hot-tracked button was set. In this case update |hot_button_| to match.
+  // This allows correct hot-tracked button to be used when alternating between
+  // keyboard and mouse navigation in the menu.
+  CustomButton* new_hot_button =
+      GetFirstHotTrackedViewDifferingFrom(menu_item, hot_button_);
+  if (new_hot_button) {
+    if (hot_button_)
+      SetHotTrackedButton(hot_button_, false);
+    SetHotTrackedButton(new_hot_button, true);
   }
 
   // Notify the old path it isn't selected.
   MenuDelegate* current_delegate =
       current_path.empty() ? NULL : current_path.front()->GetDelegate();
   for (size_t i = paths_differ_at; i < current_size; ++i) {
     if (current_delegate &&
         current_path[i]->GetType() == MenuItemView::SUBMENU) {
       current_delegate->WillHideMenu(current_path[i]);
     }
@@ skipping 231 matching lines @@
       drop_target_(NULL),
       drop_position_(MenuDelegate::DROP_UNKNOWN),
       owner_(NULL),
       possible_drag_(false),
       drag_in_progress_(false),
       did_initiate_drag_(false),
       valid_drop_coordinates_(false),
       last_drop_operation_(MenuDelegate::DROP_UNKNOWN),
       showing_submenu_(false),
       active_mouse_view_id_(ViewStorage::GetInstance()->CreateStorageID()),
+      hot_button_(nullptr),
       delegate_(delegate),
       message_loop_depth_(0),
       closing_event_time_(base::TimeDelta()),
       menu_start_time_(base::TimeTicks()),
       async_run_(false),
       is_combobox_(false),
       item_selected_by_touch_(false),
       current_mouse_event_target_(nullptr),
       current_mouse_pressed_state_(0),
       message_loop_(MenuMessageLoop::Create()) {
@@ skipping 15 matching lines @@
   message_loop_->Run(this, owner_, nested_menu);
 }
 
 bool MenuController::SendAcceleratorToHotTrackedView() {
   CustomButton* hot_view = GetFirstHotTrackedView(pending_state_.item);
   if (!hot_view)
     return false;
 
   ui::Accelerator accelerator(ui::VKEY_RETURN, ui::EF_NONE);
   hot_view->AcceleratorPressed(accelerator);
-  CustomButton* button = static_cast<CustomButton*>(hot_view);
-  button->SetHotTracked(true);
+  SetHotTrackedButton(hot_view, true);
   return true;
 }
 
 void MenuController::UpdateInitialLocation(const gfx::Rect& bounds,
                                            MenuAnchorPosition position,
                                            bool context_menu) {
   pending_state_.context_menu = context_menu;
   pending_state_.initial_bounds = bounds;
   if (bounds.height() > 1) {
     // Inset the bounds slightly, otherwise drag coordinates don't line up
@@ skipping 741 matching lines @@
 void MenuController::IncrementSelection(
     SelectionIncrementDirectionType direction) {
   MenuItemView* item = pending_state_.item;
   DCHECK(item);
   if (pending_state_.submenu_open && item->HasSubmenu() &&
       item->GetSubmenu()->IsShowing()) {
     // 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 = FindInitialSelectableMenuItem(item, direction);
-      if (to_select)
-        SetSelection(to_select, SELECTION_DEFAULT);
+      SetInitialHotTrackedView(to_select, direction);
       return;
     }
   }
 
   if (item->has_children()) {
     CustomButton* button = GetFirstHotTrackedView(item);
     if (button) {
-      button->SetHotTracked(false);
-      View* to_make_hot = GetNextFocusableView(
+      SetHotTrackedButton(button, false);
+      View* hot_view = GetNextFocusableView(
           item, button, direction == INCREMENT_SELECTION_DOWN);
-      CustomButton* button_hot = CustomButton::AsCustomButton(to_make_hot);
-      if (button_hot) {
-        button_hot->SetHotTracked(true);
+      CustomButton* hot_button = CustomButton::AsCustomButton(hot_view);
+      if (hot_button) {
+        SetHotTrackedButton(hot_button, true);
         return;
       }
     } else {
-      View* to_make_hot =
+      View* hot_view =
           GetInitialFocusableView(item, direction == INCREMENT_SELECTION_DOWN);
-      CustomButton* button_hot = CustomButton::AsCustomButton(to_make_hot);
-      if (button_hot) {
-        button_hot->SetHotTracked(true);
+      CustomButton* hot_button = CustomButton::AsCustomButton(hot_view);
+      if (hot_button) {
+        SetHotTrackedButton(hot_button, true);
         return;
       }
     }
   }
 
   MenuItemView* parent = item->GetParentMenuItem();
   if (parent) {
     int parent_count = parent->GetSubmenu()->GetMenuItemCount();
     if (parent_count > 1) {
       for (int i = 0; i < parent_count; ++i) {
         if (parent->GetSubmenu()->GetMenuItemAt(i) == item) {
           MenuItemView* to_select =
               FindNextSelectableMenuItem(parent, i, direction);
-          if (!to_select)
-            break;
-          SetSelection(to_select, SELECTION_DEFAULT);
-          View* to_make_hot = GetInitialFocusableView(
-              to_select, direction == INCREMENT_SELECTION_DOWN);
-          CustomButton* button_hot = CustomButton::AsCustomButton(to_make_hot);
-          if (button_hot)
-            button_hot->SetHotTracked(true);
+          SetInitialHotTrackedView(to_select, direction);
           break;
         }
       }
     }
   }
 }
 
 MenuItemView* MenuController::FindInitialSelectableMenuItem(
     MenuItemView* parent,
     SelectionIncrementDirectionType direction) {
@@ skipping 461 matching lines @@
              pending_state_.item->GetParentMenuItem() &&
              (!pending_state_.item->HasSubmenu() ||
               !pending_state_.item->GetSubmenu()->IsShowing())) {
     // On exit if the user hasn't selected an item with a submenu, move the
     // selection back to the parent menu item.
     SetSelection(pending_state_.item->GetParentMenuItem(),
                  SELECTION_OPEN_SUBMENU);
   }
 }
 
+void MenuController::SetHotTrackedButton(CustomButton* hot_button,
+                                         bool is_hot_tracked) {
+  DCHECK(hot_button);

[sky, 2016/02/23 20:11:13]

The constraints on calling this function are not obvious and confusing. Can't
this function take care of updating existing state so the call site doesn't
need:
if (hot_button_)
  SetHotTrackedButton(hot_button_, false);
SetHotTrackedButton(new_hot_button, true);
and instead can do:
SetHotTrackedButton(new_hot_button, true) and it takes the necessary steps?

[varkha, 2016/02/23 21:01:39]

Yes, will try that.

[varkha, 2016/02/24 01:42:21]

Done.

+  hot_button->SetHotTracked(is_hot_tracked);
+  DCHECK(is_hot_tracked || (hot_button_ == hot_button));
+  hot_button_ = is_hot_tracked ? hot_button : nullptr;
+}
+
+void MenuController::SetInitialHotTrackedView(
+    MenuItemView* item,
+    SelectionIncrementDirectionType direction) {
+  if (!item)
+    return;
+  SetSelection(item, SELECTION_DEFAULT);
+  View* hot_view =
+      GetInitialFocusableView(item, direction == INCREMENT_SELECTION_DOWN);
+  CustomButton* hot_button = CustomButton::AsCustomButton(hot_view);
+  if (hot_button)
+    SetHotTrackedButton(hot_button, true);
+}
+
 }  // namespace views