Mac: Don't allow history swiping after substantial vertical motion.

Previously, the logic for history swiping depended on the relative Y distance
of the gesture's current location from the gesture's start location. Gestures
which consisted of a large up motion, followed by a large down motion, followed
by a horizontal motion could still cause history swiping. The new logic depends
on the total Y distance of the gesture, which fixes this problem.

This CL also includes a change to allowed the gesture recognizer state machine
to enter the state kCancelled directly from the state kPending. Previously, it
was possible for the history overlay to show for a very brief period of time
even if the gesture was about to be cancelled.

This CL also includes a minor refactor:
 - Renamed several methods to better reflect their intended purpose.
 - Removed lastProcessedGestureId_ and currentGestureId_, which did not cause a
 functional change.

BUG=421629

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

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

chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.h

@@ -13,10 +13,10 @@ class WebMouseWheelEvent;
 
 @class HistorySwiper;
 @protocol HistorySwiperDelegate
-// Return NO from this method is the view/render_widget_host should not
+// Return NO from this method if the view/render_widget_host should not
 // allow history swiping.
 - (BOOL)shouldAllowHistorySwiping;
-// The history overlay is added to the view returning from this method.
+// The history overlay is added to the view returned from this method.
 - (NSView*)viewThatWantsHistoryOverlay;
 @end
 
@@ -25,6 +25,11 @@ enum NavigationDirection {
   kBackwards = 0,
   kForwards,
 };
+
+// The states of a state machine that recognizes the history swipe gesture.
+// When a gesture first begins, the state is reset to kPending. The state
+// machine only applies to trackpad gestures. Magic Mouse gestures use a
+// different mechanism.
 enum RecognitionState {
   // Waiting to see whether the renderer will handle the event with phase
   // NSEventPhaseBegan. The state machine will also stay in this state if
@@ -97,13 +102,13 @@ enum RecognitionState {
 //   then this class will transition to the state kTracking. Events are
 //   consumed, and not passed to the renderer.
 //
-// There are multiple APIs that provide information about gestures on the
-// trackpad. This class uses two different set of APIs.
-//   1. The -[NSView touches*WithEvent:] APIs provide detailed information
+// There are multiple callbacks that provide information about gestures on the
+// trackpad. This class uses two different sets of callbacks.
+//   1. The -[NSView touches*WithEvent:] callbacks provide detailed information
 //   about the touches within a gesture. The callbacks happen with more
-//   frequency, and have higher accuracy. These APIs are used to transition
-//   between all state, exception for kPending -> kPotential.
-//   2. The -[NSView scrollWheel:] API provides less information, but the
+//   frequency, and have higher accuracy. These callbacks are used to
+//   transition between all states except for kPending -> kPotential.
+//   2. The -[NSView scrollWheel:] callback provides less information, but the
 //   events are passed to the renderer. This class must process these events so
 //   that it can decide whether to consume the events and prevent them from
 //   being passed to the renderer. This API is used to transition from kPending
@@ -113,13 +118,12 @@ enum RecognitionState {
  @private
   // This controller will exist if and only if the UI is in history swipe mode.
   HistoryOverlayController* historyOverlay_;
-  // Each gesture received by the window is given a unique id.
-  // The id is monotonically increasing.
-  int currentGestureId_;
   // The location of the fingers when the gesture started.
   NSPoint gestureStartPoint_;
   // The current location of the fingers in the gesture.
   NSPoint gestureCurrentPoint_;
+  // The total Y distance moved since the beginning of the gesture.
+  CGFloat gestureTotalY_;
   // A flag that indicates that there is an ongoing gesture. Only used to
   // determine whether swipe events are coming from a Magic Mouse.
   BOOL inGesture_;
@@ -128,11 +132,13 @@ enum RecognitionState {
   // Each time a new gesture begins, we must get a new start point.
   // This ivar determines whether the start point is valid.
   int gestureStartPointValid_;
-  // The id of the last gesture that we processed as a history swipe.
-  int lastProcessedGestureId_;
-  // A flag that indicates the user's intended direction with the history swipe.
+
+  // The user's intended direction with the history swipe. Set during the
+  // transition from kPending -> kPotential.
   history_swiper::NavigationDirection historySwipeDirection_;
-  // A flag that indicates whether the gesture has its direction inverted.
+
+  // Whether the history swipe gesture has its direction inverted. Set during
+  // the transition from kPending -> kPotential.
   BOOL historySwipeDirectionInverted_;
 
   // Whether the event with phase NSEventPhaseBegan was not consumed by the

chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm

@@ -49,6 +49,46 @@ BOOL forceMagicMouse = NO;
 // Returns whether the wheel event should be consumed, and not passed to the
 // renderer.
 - (BOOL)shouldConsumeWheelEvent:(NSEvent*)event;
+
+// Shows the history swiper overlay.
+- (void)showHistoryOverlay:(history_swiper::NavigationDirection)direction;
+
+// Removes the history swiper overlay.
+- (void)removeHistoryOverlay;
+
+// Returns YES if the event was consumed or NO if it should be passed on to the
+// renderer. If |event| was generated by a Magic Mouse, this method forwards to
+// handleMagicMouseWheelEvent. Otherwise, this method attempts to transition
+// the state machine from kPending -> kPotential. If it performs the
+// transition, it also shows the history overlay. In order for a history swipe
+// gesture to be recognized, the transition must occur.
+//
+// There are 4 types of scroll wheel events:
+// 1. Magic mouse swipe events.
+//      These are identical to magic trackpad events, except that there are no
+//      -[NSView touches*WithEvent:] callbacks.  The only way to accurately
+//      track these events is with the  `trackSwipeEventWithOptions:` API.
+//      scrollingDelta{X,Y} is not accurate over long distances (it is computed
+//      using the speed of the swipe, rather than just the distance moved by
+//      the fingers).
+// 2. Magic trackpad swipe events.
+//      These are the most common history swipe events. The logic of this
+//      method is predominantly designed to handle this use case.
+// 3. Traditional mouse scrollwheel events.
+//      These should not initiate scrolling. They can be distinguished by the
+//      fact that `phase` and `momentumPhase` both return NSEventPhaseNone.
+// 4. Momentum swipe events.
+//      After a user finishes a swipe, the system continues to generate
+//      artificial callbacks. `phase` returns NSEventPhaseNone, but
+//      `momentumPhase` does not. Unfortunately, the callbacks don't work
+//      properly (OSX 10.9). Sometimes, the system start sending momentum swipe
+//      events instead of trackpad swipe events while the user is still
+//      2-finger swiping.
+- (BOOL)handleScrollWheelEvent:(NSEvent*)event;
+
+// Returns YES if the event was consumed or NO if it should be passed on to the
+// renderer. Attempts to initiate history swiping for Magic Mouse events.
+- (BOOL)handleMagicMouseWheelEvent:(NSEvent*)theEvent;
 @end
 
 @implementation HistorySwiper
@@ -57,25 +97,21 @@ BOOL forceMagicMouse = NO;
 - (id)initWithDelegate:(id<HistorySwiperDelegate>)delegate {
   self = [super init];
   if (self) {
-    // Gesture ids start at 0.
-    currentGestureId_ = 0;
-    // No gestures have been processed
-    lastProcessedGestureId_ = -1;
     delegate_ = delegate;
   }
   return self;
 }
 
 - (void)dealloc {
-  [self endHistorySwipe];
+  [self removeHistoryOverlay];
   [super dealloc];
 }
 
 - (BOOL)handleEvent:(NSEvent*)event {
-  if ([event type] == NSScrollWheel)
-    return [self maybeHandleHistorySwiping:event];
+  if ([event type] != NSScrollWheel)
+    return NO;
 
-  return NO;
+  return [self handleScrollWheelEvent:event];
 }
 
 - (void)rendererHandledWheelEvent:(const blink::WebMouseWheelEvent&)event
@@ -155,8 +191,14 @@ BOOL forceMagicMouse = NO;
 }
 
 - (void)updateGestureCurrentPointFromEvent:(NSEvent*)event {
+  NSPoint averagePosition = [self averagePositionInEvent:event];
+
+  // If the start point is valid, then so is the current point.
+  if (gestureStartPointValid_)
+    gestureTotalY_ += fabs(averagePosition.y - gestureCurrentPoint_.y);
+
   // Update the current point of the gesture.
-  gestureCurrentPoint_ = [self averagePositionInEvent:event];
+  gestureCurrentPoint_ = averagePosition;
 
   // If the gesture doesn't have a start point, set one.
   if (!gestureStartPointValid_) {
@@ -170,10 +212,10 @@ BOOL forceMagicMouse = NO;
 // available after the gesture begins.
 - (void)touchesBeganWithEvent:(NSEvent*)event {
   receivingTouches_ = YES;
-  ++currentGestureId_;
 
   // Reset state pertaining to previous gestures.
   gestureStartPointValid_ = NO;
+  gestureTotalY_ = 0;
   mouseScrollDelta_ = NSZeroSize;
   beganEventUnconsumed_ = NO;
   recognitionState_ = history_swiper::kPending;
@@ -194,7 +236,6 @@ BOOL forceMagicMouse = NO;
 
 - (void)touchesEndedWithEvent:(NSEvent*)event {
   receivingTouches_ = NO;
-
   if (![self processTouchEventForHistorySwiping:event])
     return;
 
@@ -205,8 +246,8 @@ BOOL forceMagicMouse = NO;
     if (finished)
       [self navigateBrowserInDirection:historySwipeDirection_];
 
-    // Remove the history overlay.
-    [self endHistorySwipe];
+    [self removeHistoryOverlay];
+
     // The gesture was completed.
     recognitionState_ = history_swiper::kCompleted;
   }
@@ -224,8 +265,6 @@ BOOL forceMagicMouse = NO;
     case history_swiper::kCompleted:
       return NO;
     case history_swiper::kPending:
-      [self updateGestureCurrentPointFromEvent:event];
-      return NO;
     case history_swiper::kPotential:
     case history_swiper::kTracking:
       break;
@@ -240,6 +279,10 @@ BOOL forceMagicMouse = NO;
     return NO;
   }
 
+  // Don't do any more processing if the state machine is in the pending state.
+  if (recognitionState_ == history_swiper::kPending)
+    return NO;
+
   if (recognitionState_ == history_swiper::kPotential) {
     // The user is in the process of doing history swiping.  If the history
     // swipe has progressed sufficiently far, stop sending events to the
@@ -259,7 +302,7 @@ BOOL forceMagicMouse = NO;
 // vertical swipe.
 - (BOOL)shouldCancelHorizontalSwipeWithCurrentPoint:(NSPoint)currentPoint
     startPoint:(NSPoint)startPoint {
-  CGFloat yDelta = fabs(currentPoint.y - startPoint.y);
+  CGFloat yDelta = gestureTotalY_;
   CGFloat xDelta = fabs(currentPoint.x - startPoint.x);
 
   // The gesture is pretty clearly more vertical than horizontal.
@@ -278,11 +321,11 @@ BOOL forceMagicMouse = NO;
 }
 
 - (void)cancelHistorySwipe {
-  [self endHistorySwipe];
+  [self removeHistoryOverlay];
   recognitionState_ = history_swiper::kCancelled;
 }
 
-- (void)endHistorySwipe {
+- (void)removeHistoryOverlay {
   [historyOverlay_ dismiss];
   [historyOverlay_ release];
   historyOverlay_ = nil;
@@ -323,19 +366,11 @@ BOOL forceMagicMouse = NO;
   return NO;
 }
 
-// goForward indicates whether the user is starting a forward or backward
-// history swipe.
-// Creates and displays a history overlay controller.
-// Responsible for cleaning up after itself when the gesture is finished.
-// Responsible for starting a browser navigation if necessary.
-// Does not prevent swipe events from propagating to other handlers.
-- (void)beginHistorySwipeInDirection:
-        (history_swiper::NavigationDirection)direction
-                               event:(NSEvent*)event {
+- (void)showHistoryOverlay:(history_swiper::NavigationDirection)direction {
   // We cannot make any assumptions about the current state of the
   // historyOverlay_, since users may attempt to use multiple gesture input
   // devices simultaneously, which confuses Cocoa.
-  [self endHistorySwipe];
+  [self removeHistoryOverlay];
 
   HistoryOverlayController* historyOverlay = [[HistoryOverlayController alloc]
       initForMode:(direction == history_swiper::kForwards)
@@ -343,11 +378,6 @@ BOOL forceMagicMouse = NO;
                      : kHistoryOverlayModeBack];
   [historyOverlay showPanelForView:[delegate_ viewThatWantsHistoryOverlay]];
   historyOverlay_ = historyOverlay;
-
-  // Record whether the user was swiping forwards or backwards.
-  historySwipeDirection_ = direction;
-  // Record the user's settings.
-  historySwipeDirectionInverted_ = [self isEventDirectionInverted:event];
 }
 
 - (BOOL)systemSettingsAllowHistorySwiping:(NSEvent*)event {
@@ -383,9 +413,7 @@ BOOL forceMagicMouse = NO;
   }
 }
 
-// We use an entirely different set of logic for magic mouse swipe events,
-// since we do not get NSTouch callbacks.
-- (BOOL)maybeHandleMagicMouseHistorySwiping:(NSEvent*)theEvent {
+- (BOOL)handleMagicMouseWheelEvent:(NSEvent*)theEvent {
   // The 'trackSwipeEventWithOptions:' api doesn't handle momentum events.
   if ([theEvent phase] == NSEventPhaseNone)
     return NO;
@@ -483,31 +511,7 @@ BOOL forceMagicMouse = NO;
       }];
 }
 
-// Checks if |theEvent| should trigger history swiping, and if so, does
-// history swiping. Returns YES if the event was consumed or NO if it should
-// be passed on to the renderer.
-//
-// There are 4 types of scroll wheel events:
-// 1. Magic mouse swipe events.
-//      These are identical to magic trackpad events, except that there are no
-//      NSTouch callbacks.  The only way to accurately track these events is
-//      with the  `trackSwipeEventWithOptions:` API. scrollingDelta{X,Y} is not
-//      accurate over long distances (it is computed using the speed of the
-//      swipe, rather than just the distance moved by the fingers).
-// 2. Magic trackpad swipe events.
-//      These are the most common history swipe events. Our logic is
-//      predominantly designed to handle this use case.
-// 3. Traditional mouse scrollwheel events.
-//      These should not initiate scrolling. They can be distinguished by the
-//      fact that `phase` and `momentumPhase` both return NSEventPhaseNone.
-// 4. Momentum swipe events.
-//      After a user finishes a swipe, the system continues to generate
-//      artificial callbacks. `phase` returns NSEventPhaseNone, but
-//      `momentumPhase` does not. Unfortunately, the callbacks don't work
-//      properly (OSX 10.9). Sometimes, the system start sending momentum swipe
-//      events instead of trackpad swipe events while the user is still
-//      2-finger swiping.
-- (BOOL)maybeHandleHistorySwiping:(NSEvent*)theEvent {
+- (BOOL)handleScrollWheelEvent:(NSEvent*)theEvent {
   if (![theEvent respondsToSelector:@selector(phase)])
     return NO;
 
@@ -520,8 +524,7 @@ BOOL forceMagicMouse = NO;
   }
 
   // We've already processed this gesture.
-  if (lastProcessedGestureId_ == currentGestureId_ &&
-      recognitionState_ != history_swiper::kPending) {
+  if (recognitionState_ != history_swiper::kPending) {
     return [self shouldConsumeWheelEvent:theEvent];
   }
 
@@ -546,7 +549,7 @@ BOOL forceMagicMouse = NO;
   // callbacks to perform history swiping, magic mouse swipe events use an
   // entirely different set of logic.
   if ((inGesture_ && !receivingTouches_) || forceMagicMouse)
-    return [self maybeHandleMagicMouseHistorySwiping:theEvent];
+    return [self handleMagicMouseWheelEvent:theEvent];
 
   // The scrollWheel: callback is only relevant if it happens while the user is
   // still actively using the touchpad.
@@ -573,9 +576,10 @@ BOOL forceMagicMouse = NO;
   if (!browserCanMove)
     return NO;
 
-  lastProcessedGestureId_ = currentGestureId_;
-  [self beginHistorySwipeInDirection:direction event:theEvent];
+  historySwipeDirection_ = direction;
+  historySwipeDirectionInverted_ = [self isEventDirectionInverted:theEvent];
   recognitionState_ = history_swiper::kPotential;
+  [self showHistoryOverlay:direction];
   return [self shouldConsumeWheelEvent:theEvent];
 }
 
@@ -594,6 +598,7 @@ BOOL forceMagicMouse = NO;
       return event.scrollingDeltaY == 0;
   }
 }
+
 @end
 
 @implementation HistorySwiper (PrivateExposedForTesting)

chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_unit_test.mm

@@ -17,10 +17,8 @@
 - (BOOL)browserCanNavigateInDirection:
         (history_swiper::NavigationDirection)forward
                                 event:(NSEvent*)event;
-- (void)endHistorySwipe;
-- (void)beginHistorySwipeInDirection:
-        (history_swiper::NavigationDirection)goForward
-                               event:(NSEvent*)event;
+- (void)removeHistoryOverlay;
+- (void)showHistoryOverlay:(history_swiper::NavigationDirection)direction;
 - (void)navigateBrowserInDirection:(history_swiper::NavigationDirection)forward;
 - (void)initiateMagicMouseHistorySwipe:(BOOL)isRightScroll
                                  event:(NSEvent*)event;
@@ -51,22 +49,18 @@ class MacHistorySwiperTest : public CocoaTest {
         browserCanNavigateInDirection:history_swiper::kBackwards
                                 event:[OCMArg any]];
     [[[[mockHistorySwiper stub] andDo:^(NSInvocation* invocation) {
-      ++begin_count_;
-      // beginHistorySwipeInDirection: calls endHistorySwipe internally.
-      --end_count_;
-    }] andForwardToRealObject]
-        beginHistorySwipeInDirection:history_swiper::kForwards
-                               event:[OCMArg any]];
+        ++begin_count_;
+        // showHistoryOverlay: calls removeHistoryOverlay internally.
+        --end_count_;
+    }] andForwardToRealObject] showHistoryOverlay:history_swiper::kForwards];
     [[[[mockHistorySwiper stub] andDo:^(NSInvocation* invocation) {
-      ++begin_count_;
-      // beginHistorySwipeInDirection: calls endHistorySwipe internally.
-      --end_count_;
-    }] andForwardToRealObject]
-        beginHistorySwipeInDirection:history_swiper::kBackwards
-                               event:[OCMArg any]];
+        ++begin_count_;
+        // showHistoryOverlay: calls removeHistoryOverlay internally.
+        --end_count_;
+    }] andForwardToRealObject] showHistoryOverlay:history_swiper::kBackwards];
     [[[[mockHistorySwiper stub] andDo:^(NSInvocation* invocation) {
-      ++end_count_;
-    }] andForwardToRealObject] endHistorySwipe];
+        ++end_count_;
+    }] andForwardToRealObject] removeHistoryOverlay];
     [[[mockHistorySwiper stub] andDo:^(NSInvocation* invocation) {
         navigated_right_ = true;
     }] navigateBrowserInDirection:history_swiper::kForwards];
@@ -287,7 +281,7 @@ TEST_F(MacHistorySwiperTest, SwipeDiagonal) {
   moveGestureAtPoint(makePoint(0.6, 0.59));
   endGestureAtPoint(makePoint(0.6, 0.59));
 
-  EXPECT_EQ(begin_count_, 1);
+  EXPECT_EQ(begin_count_, 0);
   EXPECT_EQ(end_count_, 1);
   EXPECT_FALSE(navigated_right_);
   EXPECT_FALSE(navigated_left_);
@@ -380,7 +374,7 @@ TEST_F(MacHistorySwiperTest, NoSwipe) {
   // No movement.
   endGestureAtPoint(makePoint(0.44, 0.44));
 
-  EXPECT_EQ(begin_count_, 1);
+  EXPECT_EQ(begin_count_, 0);
   EXPECT_EQ(end_count_, 1);
   EXPECT_FALSE(navigated_right_);
   EXPECT_FALSE(navigated_left_);
@@ -451,3 +445,30 @@ TEST_F(MacHistorySwiperTest, SwipeRightEventOrdering) {
   EXPECT_TRUE(navigated_right_);
   EXPECT_FALSE(navigated_left_);
 }
+
+// Substantial vertical scrolling followed by horizontal scrolling should not
+// result in navigation.
+TEST_F(MacHistorySwiperTest, SubstantialVerticalThenHorizontal) {
+  // These tests require 10.7+ APIs.
+  if (![NSEvent
+          respondsToSelector:@selector(isSwipeTrackingFromScrollEventsEnabled)])
+    return;
+
+  startGestureInMiddle();
+  moveGestureInMiddle();
+
+  // Move up, then move down.
+  for (CGFloat y = 0.51; y < 0.6; y += 0.01)
+    moveGestureAtPoint(makePoint(0.5, y));
+  for (CGFloat y = 0.59; y > 0.5; y -= 0.01)
+    moveGestureAtPoint(makePoint(0.5, y));
+
+  // Large movement to the right.
+  moveGestureAtPoint(makePoint(0.6, 0.51));
+  endGestureAtPoint(makePoint(0.6, 0.51));
+
+  EXPECT_EQ(begin_count_, 0);
+  EXPECT_EQ(end_count_, 1);
+  EXPECT_FALSE(navigated_right_);
+  EXPECT_FALSE(navigated_left_);
+}