Mac: Don't trigger mouse down events on bookmark buttons if command key is down

Currently, all clicks on a bookmark folder button trigger their action on
mouse down, so that the folder menu can open on mouse down as expected.

This also includes command click, which, rather than opening the folder menu,
opens all the bookmarks in the folder as new tabs. This isn't desirable since:
- The expected behavior is that button clicks trigger on mouse up.
- Triggering this on mouse down interacts poorly with the nested run loop
created by the confirmation dialog we display when the folder has many links.

This change causes the action to no longer fire on mouse down if the user
is command-clicking the folder button.

Bug: 840387
Change-Id: Ic1364160cf26f2f3ea6e67d7dd0d6cf5079082eb
Reviewed-on: https://chromium-review.googlesource.com/1053947Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Leonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557676}

chrome/browser/ui/cocoa/draggable_button_mixin.mm

@@ -7,6 +7,7 @@
 #include <cmath>
 
 #include "base/logging.h"
+#include "ui/base/cocoa/cocoa_base_utils.h"
 
 // Code taken from <http://codereview.chromium.org/180036/diff/3001/3004>.
 // TODO(viettrungluu): Do we want common, standard code for drag hysteresis?
@@ -86,6 +87,15 @@ const CGFloat kDragExpirationTimeout = 0.45;
   whenMouseDown_ = [theEvent timestamp];
   actionHasFired_ = NO;
 
+  // Even if the regular click should trigger on mouse down (for example,
+  // opening the bookmark menu), command click should behave like a
+  // button click.
+  WindowOpenDisposition disposition =
+      ui::WindowOpenDispositionFromNSEvent(theEvent);
+  BOOL shouldActOnMouseDown =
+      actsOnMouseDown_ &&
+      disposition != WindowOpenDisposition::NEW_BACKGROUND_TAB;
+
   if (draggable_) {
     NSDate* date = [NSDate dateWithTimeIntervalSinceNow:kDragExpirationTimeout];
     if ([self dragShouldBeginFromMouseDown:theEvent
@@ -93,7 +103,7 @@ const CGFloat kDragExpirationTimeout = 0.45;
       [button_ beginDrag:theEvent];
       [self endDrag];
     } else {
-      if (actsOnMouseDown_) {
+      if (shouldActOnMouseDown) {
         [self performMouseDownAction:theEvent];
         return kDraggableButtonImplDidWork;
       } else {
@@ -101,7 +111,7 @@ const CGFloat kDragExpirationTimeout = 0.45;
       }
     }
   } else {
-    if (actsOnMouseDown_) {
+    if (shouldActOnMouseDown) {
       [self performMouseDownAction:theEvent];
       return kDraggableButtonImplDidWork;
     } else {

chrome/browser/ui/cocoa/draggable_button_unittest.mm

@@ -11,7 +11,7 @@
 
 @interface TestableDraggableButton : DraggableButton {
   NSUInteger dragCount_;
-  BOOL wasTriggered_;
+  NSUInteger triggerCount_;
 }
 - (void)trigger:(id)sender;
 - (BOOL)wasTriggered;
@@ -22,7 +22,7 @@
 - (id)initWithFrame:(NSRect)frame {
   if ((self = [super initWithFrame:frame])) {
     dragCount_ = 0;
-    wasTriggered_ = NO;
+    triggerCount_ = 0;
   }
   return self;
 }
@@ -31,16 +31,24 @@
 }
 
 - (void)trigger:(id)sender {
-  wasTriggered_ = YES;
+  triggerCount_++;
 }
 
 - (BOOL)wasTriggered {
-  return wasTriggered_;
+  return triggerCount_ > 0;
+}
+
+- (NSUInteger)triggerCount {
+  return triggerCount_;
 }
 
 - (NSUInteger)dragCount {
   return dragCount_;
 }
+
+- (void)performBlock:(void (^)(void))block {
+  block();
+}
 @end
 
 class DraggableButtonTest : public CocoaTest {};
@@ -124,7 +132,7 @@ TEST_F(DraggableButtonTest, ResetState) {
       cocoa_test_event_utils::MouseEventAtPoint(NSMakePoint(100,100),
                                                 NSLeftMouseUp,
                                                 0);
-  // If the mouse moves > 5 pixels in either direciton it should cause a drag.
+  // If the mouse moves > 5 pixels in either direction it should cause a drag.
   [NSApp postEvent:upEvent atStart:YES];
   [NSApp postEvent:moveEvent atStart:YES];
   [button mouseDown:downEvent];
@@ -140,3 +148,42 @@ TEST_F(DraggableButtonTest, ResetState) {
   EXPECT_EQ(2U, [button dragCount]);
   EXPECT_FALSE([[button cell] isHighlighted]);
 }
+
+TEST_F(DraggableButtonTest, ActsOnMouseDown) {
+  base::scoped_nsobject<TestableDraggableButton> button(
+      [[TestableDraggableButton alloc]
+          initWithFrame:NSMakeRect(0, 0, 500, 500)]);
+  [button setTarget:button];
+  [button setAction:@selector(trigger:)];
+  [[button draggableButton] setActsOnMouseDown:YES];
+
+  [[test_window() contentView] addSubview:button.get()];
+  [button performSelector:@selector(performBlock:)
+               withObject:^{
+                 EXPECT_EQ([button triggerCount], 1U);
+               }
+               afterDelay:.5
+                  inModes:@[ NSEventTrackingRunLoopMode ]];
+  NSEvent* downEvent = cocoa_test_event_utils::MouseEventAtPoint(
+      NSMakePoint(10, 10), NSLeftMouseDown, 0);
+  [button mouseDown:downEvent];
+}
+
+TEST_F(DraggableButtonTest, ActsOnMouseDownIgnoredForCommandClick) {
+  base::scoped_nsobject<TestableDraggableButton> button(
+      [[TestableDraggableButton alloc]
+          initWithFrame:NSMakeRect(0, 0, 500, 500)]);
+  [button setTarget:button];
+  [button setAction:@selector(trigger:)];
+  [[button draggableButton] setActsOnMouseDown:YES];
+
+  [[test_window() contentView] addSubview:button.get()];
+  NSEvent* downEvent = cocoa_test_event_utils::MouseEventAtPoint(
+      NSMakePoint(10, 10), NSLeftMouseDown, NSEventModifierFlagCommand);
+  [button mouseDown:downEvent];
+  EXPECT_FALSE([button wasTriggered]);
+  NSEvent* upEvent = cocoa_test_event_utils::MouseEventAtPoint(
+      NSMakePoint(100, 100), NSLeftMouseUp, 0);
+  [button mouseUp:upEvent];
+  EXPECT_EQ([button triggerCount], 1U);
+}