Ash: Double-clicking on a shelf item now opens the item

Previously it would open the item then immediately minimize it. This caused
user confusion because they tried a double-click and didn't see anything
open on the screen.

BUG=404060
TEST=ash_unittests ShelfViewTest.ClickingTwiceActivatesOnce

Review URL: https://codereview.chromium.org/492963008

Cr-Commit-Position: refs/heads/master@{#291695}

ash/shelf/shelf_view.cc

@@ -1669,6 +1669,13 @@ void ShelfView::ButtonPressed(views::Button* sender, const ui::Event& event) {
   if (!IsUsableEvent(event))
     return;
 
+  // Don't activate the item twice on double-click. Otherwise the window starts
+  // animating open due to the first click, then immediately minimizes due to
+  // the second click. The user most likely intended to open or minimize the
+  // item once, not do both.
+  if (event.flags() & ui::EF_IS_DOUBLE_CLICK)
+    return;
+
   {
     ScopedTargetRootWindow scoped_target(
         sender->GetWidget()->GetNativeView()->GetRootWindow());

ash/shelf/shelf_view_unittest.cc

@@ -129,6 +129,9 @@ class ShelfItemSelectionTracker : public TestShelfItemDelegate {
   virtual ~ShelfItemSelectionTracker() {
   }
 
+  // Resets to the initial state.
+  void Reset() { selected_ = false; }
+
   // Returns true if the delegate was selected.
   bool WasSelected() {
     return selected_;
@@ -285,7 +288,11 @@ class TestShelfDelegateForShelfView : public ShelfDelegate {
 
 class ShelfViewTest : public AshTestBase {
  public:
-  ShelfViewTest() : model_(NULL), shelf_view_(NULL), browser_index_(1) {}
+  ShelfViewTest()
+      : model_(NULL),
+        shelf_view_(NULL),
+        browser_index_(1),
+        item_manager_(NULL) {}
   virtual ~ShelfViewTest() {}
 
   virtual void SetUp() OVERRIDE {
@@ -407,7 +414,7 @@ class ShelfViewTest : public AshTestBase {
   }
 
   void VerifyShelfItemBoundsAreValid() {
-    for (int i=0;i <= test_api_->GetLastVisibleIndex(); ++i) {
+    for (int i = 0; i <= test_api_->GetLastVisibleIndex(); ++i) {
       if (test_api_->GetButton(i)) {
         gfx::Rect shelf_view_bounds = shelf_view_->GetLocalBounds();
         gfx::Rect item_bounds = test_api_->GetBoundsByIndex(i);
@@ -419,10 +426,10 @@ class ShelfViewTest : public AshTestBase {
     }
   }
 
-  views::View* SimulateButtonPressed(ShelfButtonHost::Pointer pointer,
+  ShelfButton* SimulateButtonPressed(ShelfButtonHost::Pointer pointer,
                                      int button_index) {
     ShelfButtonHost* button_host = shelf_view_;
-    views::View* button = test_api_->GetButton(button_index);
+    ShelfButton* button = test_api_->GetButton(button_index);
     ui::MouseEvent click_event(ui::ET_MOUSE_PRESSED,
                                gfx::Point(),
                                button->GetBoundsInScreen().origin(), 0, 0);
@@ -430,12 +437,32 @@ class ShelfViewTest : public AshTestBase {
     return button;
   }
 
-  views::View* SimulateClick(ShelfButtonHost::Pointer pointer,
-                             int button_index) {
+  // Simulates a single mouse click.
+  void SimulateClick(int button_index) {
     ShelfButtonHost* button_host = shelf_view_;
-    views::View* button = SimulateButtonPressed(pointer, button_index);
+    ShelfButton* button =
+        SimulateButtonPressed(ShelfButtonHost::MOUSE, button_index);
+    ui::MouseEvent release_event(ui::ET_MOUSE_RELEASED,
+                                 gfx::Point(),
+                                 button->GetBoundsInScreen().origin(),
+                                 0,
+                                 0);
+    test_api_->ButtonPressed(button, release_event);
+    button_host->PointerReleasedOnButton(button, ShelfButtonHost::MOUSE, false);
+  }
+
+  // Simulates the second click of a double click.
+  void SimulateDoubleClick(int button_index) {
+    ShelfButtonHost* button_host = shelf_view_;
+    ShelfButton* button =
+        SimulateButtonPressed(ShelfButtonHost::MOUSE, button_index);
+    ui::MouseEvent release_event(ui::ET_MOUSE_RELEASED,
+                                 gfx::Point(),
+                                 button->GetBoundsInScreen().origin(),
+                                 ui::EF_IS_DOUBLE_CLICK,
+                                 0);
+    test_api_->ButtonPressed(button, release_event);
     button_host->PointerReleasedOnButton(button, ShelfButtonHost::MOUSE, false);
-    return button;
   }
 
   views::View* SimulateDrag(ShelfButtonHost::Pointer pointer,
@@ -606,8 +633,7 @@ class ShelfViewTest : public AshTestBase {
 
 class ScopedTextDirectionChange {
  public:
-  ScopedTextDirectionChange(bool is_rtl)
-      : is_rtl_(is_rtl) {
+  explicit ScopedTextDirectionChange(bool is_rtl) : is_rtl_(is_rtl) {
     original_locale_ = l10n_util::GetApplicationLocale(std::string());
     if (is_rtl_)
       base::i18n::SetICUDefaultLocale("he");
@@ -1043,7 +1069,7 @@ TEST_F(ShelfViewTest, ClickOneDragAnother) {
   SetupForDragTest(&id_map);
 
   // A click on item 1 is simulated.
-  SimulateClick(ShelfButtonHost::MOUSE, 1);
+  SimulateClick(1);
 
   // Dragging browser index at 0 should change the model order correctly.
   EXPECT_TRUE(model_->items()[1].type == TYPE_BROWSER_SHORTCUT);
@@ -1057,6 +1083,25 @@ TEST_F(ShelfViewTest, ClickOneDragAnother) {
   EXPECT_TRUE(model_->items()[3].type == TYPE_BROWSER_SHORTCUT);
 }
 
+// Tests that double-clicking an item does not activate it twice.
+TEST_F(ShelfViewTest, ClickingTwiceActivatesOnce) {
+  // Watch for selection of the browser shortcut.
+  ShelfID browser_shelf_id = model_->items()[browser_index_].id;
+  ShelfItemSelectionTracker* selection_tracker = new ShelfItemSelectionTracker;
+  item_manager_->SetShelfItemDelegate(
+      browser_shelf_id,
+      scoped_ptr<ShelfItemDelegate>(selection_tracker).Pass());
+
+  // A single click selects the item.
+  SimulateClick(browser_index_);
+  EXPECT_TRUE(selection_tracker->WasSelected());
+
+  // A double-click does not select the item.
+  selection_tracker->Reset();
+  SimulateDoubleClick(browser_index_);
+  EXPECT_FALSE(selection_tracker->WasSelected());
+}
+
 // Check that clicking an item and jittering the mouse a bit still selects the
 // item.
 TEST_F(ShelfViewTest, ClickAndMoveSlightly) {

ash/test/shelf_view_test_api.cc

@@ -114,6 +114,11 @@ int ShelfViewTestAPI::GetButtonSpacing() {
   return kShelfButtonSpacing;
 }
 
+void ShelfViewTestAPI::ButtonPressed(views::Button* sender,
+                                     const ui::Event& event) {
+  return shelf_view_->ButtonPressed(sender, event);
+}
+
 bool ShelfViewTestAPI::SameDragType(ShelfItemType typea,
                                     ShelfItemType typeb) const {
   return shelf_view_->SameDragType(typea, typeb);

ash/test/shelf_view_test_api.h

@@ -13,6 +13,14 @@ class Rect;
 class Size;
 }
 
+namespace ui {
+class Event;
+}
+
+namespace views {
+class Button;
+}
+
 namespace ash {
 class OverflowBubble;
 class ShelfButton;
@@ -70,6 +78,9 @@ class ShelfViewTestAPI {
   // Returns the button space size.
   int GetButtonSpacing();
 
+  // Wrapper for ShelfView::ButtonPressed.
+  void ButtonPressed(views::Button* sender, const ui::Event& event);
+
   // Wrapper for ShelfView::SameDragType.
   bool SameDragType(ShelfItemType typea, ShelfItemType typeb) const;