Unify MenuButton Pushed state logic

ui/views/controls/button/menu_button.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/button/menu_button.h"
 
 #include "base/strings/utf_string_conversions.h"
 #include "ui/accessibility/ax_view_state.h"
 #include "ui/base/dragdrop/drag_drop_types.h"
 #include "ui/base/l10n/l10n_util.h"
@@ skipping 153 matching lines @@
   }
   return prefsize;
 }
 
 const char* MenuButton::GetClassName() const {
   return kViewClassName;
 }
 
 bool MenuButton::OnMousePressed(const ui::MouseEvent& event) {
   RequestFocus();
-  if (state() != STATE_DISABLED) {
-    // If we're draggable (GetDragOperations returns a non-zero value), then
-    // don't pop on press, instead wait for release.
-    if (event.IsOnlyLeftMouseButton() &&
-        HitTestPoint(event.location()) &&
-        GetDragOperations(event.location()) == ui::DragDropTypes::DRAG_NONE) {
-      TimeDelta delta = TimeTicks::Now() - menu_closed_time_;
-      if (delta.InMilliseconds() > kMinimumMsBetweenButtonClicks)
-        return Activate();
-    }
+  if (ShouldEnterPushedState(event) && HitTestPoint(event.location())) {
+    TimeDelta delta = TimeTicks::Now() - menu_closed_time_;
+    if (delta.InMilliseconds() > kMinimumMsBetweenButtonClicks)
+      return Activate();
   }
   return true;
 }
 
 void MenuButton::OnMouseReleased(const ui::MouseEvent& event) {
-  // Explicitly test for left mouse button to show the menu. If we tested for
-  // !IsTriggerableEvent it could lead to a situation where we end up showing
-  // the menu and context menu (this would happen if the right button is not
-  // triggerable and there's a context menu).
-  if (GetDragOperations(event.location()) != ui::DragDropTypes::DRAG_NONE &&
-      state() != STATE_DISABLED && !InDrag() && event.IsOnlyLeftMouseButton() &&
-      HitTestPoint(event.location())) {
+  if (ShouldEnterPushedState(event) && HitTestPoint(event.location()))
     Activate();
-  } else {
+  else
     LabelButton::OnMouseReleased(event);
-  }
 }
 
 void MenuButton::OnMouseEntered(const ui::MouseEvent& event) {
   if (pressed_lock_count_ == 0)  // Ignore mouse movement if state is locked.
-    CustomButton::OnMouseEntered(event);
+    LabelButton::OnMouseEntered(event);
 }
 
 void MenuButton::OnMouseExited(const ui::MouseEvent& event) {
   if (pressed_lock_count_ == 0)  // Ignore mouse movement if state is locked.
-    CustomButton::OnMouseExited(event);
+    LabelButton::OnMouseExited(event);
 }
 
 void MenuButton::OnMouseMoved(const ui::MouseEvent& event) {
   if (pressed_lock_count_ == 0)  // Ignore mouse movement if state is locked.
-    CustomButton::OnMouseMoved(event);
+    LabelButton::OnMouseMoved(event);
 }
 
 void MenuButton::OnGestureEvent(ui::GestureEvent* event) {
-  if (state() != STATE_DISABLED && event->type() == ui::ET_GESTURE_TAP &&
-      !Activate()) {
+  if (ShouldEnterPushedState(*event) && !Activate()) {
     // When |Activate()| returns |false|, it means that a menu is shown and
     // has handled the gesture event. So, there is no need to further process
     // the gesture event here.
     return;
   }
   LabelButton::OnGestureEvent(event);
 }
 
 bool MenuButton::OnKeyPressed(const ui::KeyEvent& event) {
   switch (event.key_code()) {
@@ skipping 50 matching lines @@
   gfx::Size s = size();
 
   if (show_menu_marker_) {
     s.set_width(s.width() - menu_marker_->width() - kMenuMarkerPaddingLeft -
                 kMenuMarkerPaddingRight);
   }
 
   return gfx::Rect(s);
 }
 
+bool MenuButton::ShouldEnterPushedState(const ui::Event& event) {
+  if (state() == STATE_DISABLED)

[sky, 2014/10/14 20:05:06]

This function shouldn't have to check disabled (just as it doesn't hit test).
Call sites should do that (same as CustomButton does now).

[jonross, 2014/10/14 21:42:17]

Done.

+    return false;
+
+  if (event.IsMouseEvent()) {
+    const ui::MouseEvent& mouseev = static_cast<const ui::MouseEvent&>(event);
+    // Active on left mouse button only, to prevent a menu from being activated
+    // when a right-click would also activate a context menu.
+    if (!mouseev.IsOnlyLeftMouseButton())
+      return false;
+    // If dragging is supported activate on release, otherwise activate on
+    // pressed.
+    ui::EventType active_on =
+        GetDragOperations(mouseev.location()) == ui::DragDropTypes::DRAG_NONE ?
+            ui::ET_MOUSE_PRESSED : ui::ET_MOUSE_RELEASED;
+    if (event.type() == active_on && !InDrag())

[sky, 2014/10/14 20:05:06]

I also think InDrag should be checked in the call site too.

[jonross, 2014/10/14 21:42:17]

Done.

+      return true;
+  }
+
+  if (event.type() == ui::ET_GESTURE_TAP)

[sky, 2014/10/14 20:05:06]

nit: return event.type() == ET_GESTURE_TAP.

[jonross, 2014/10/14 21:42:17]

Done.

+    return true;
+
+  return false;
+}
+
 void MenuButton::IncrementPressedLocked() {
   ++pressed_lock_count_;
   SetState(STATE_PRESSED);
 }
 
 void MenuButton::DecrementPressedLocked() {
   --pressed_lock_count_;
   DCHECK_GE(pressed_lock_count_, 0);
 
   // If this was the last lock, manually reset state to "normal". We set
   // "normal" and not "hot" because the likelihood is that the mouse is now
   // somewhere else (user clicked elsewhere on screen to close the menu or
   // selected an item) and we will inevitably refresh the hot state in the event
   // the mouse _is_ over the view.
   if (pressed_lock_count_ == 0)
     SetState(STATE_NORMAL);
 }
 
 int MenuButton::GetMaximumScreenXCoordinate() {
   if (!GetWidget()) {
     NOTREACHED();
     return 0;
   }
 
   gfx::Rect monitor_bounds = GetWidget()->GetWorkAreaBoundsInScreen();
   return monitor_bounds.right() - 1;
 }
 
 }  // namespace views