Do not swap X & Y for for shift-scrolling on Mac

MacOSX scrollWheel event already provides the correct behavior. If the
event (physical) source has precise deltas and comes from a trackpad
or apple mouse then it has bidirectional deltas already and pressing
shift key should not swap the axis. If the source of the event is an
ordinary single-axis mouse, then the NSEvent already packs |delta_x|
instead of |delta_y| (for shift-scroll).

Also, it turns out that rails mode should only be used for trackpad. The
logic in MosueWheelRailsFilter uses an auto-regressive filter to smooth
out rails latching. This state is reset for a kPhaseScrollBegan. However,
apple mice would always have a phase of kPhaseNone. Since it never resets
it might lead to a large value in one axis (i.e., in y-axis after a
vertical scroll phase) and when scrolling in the other direction occurs,
the rails mode is set incorrectly. The side effects of this are zero
scroll deltas which also fires a DCHECK in mouse_wheel_event_queue.cc:
DCHECK(needs_update). To repro use an apple mouse and scroll vertically
and then horizontally using the bidirectional wheel.

Bug: 862661

Change-Id: I4e5210454fe7352ab0230b312f44e1a374f22f0c
Reviewed-on: https://chromium-review.googlesource.com/1134434Reviewed-by: ccameron <ccameron@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Ehsan Karamad <ekaramad@chromium.org>
Commit-Queue: Ehsan Karamad <ekaramad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577296}

content/browser/renderer_host/input/mouse_wheel_event_queue.cc

@@ -6,6 +6,7 @@
 
 #include "base/metrics/histogram_macros.h"
 #include "base/trace_event/trace_event.h"
+#include "build/build_config.h"
 #include "content/common/input/input_event_dispatch_type.h"
 #include "content/public/common/content_features.h"
 #include "ui/events/base_event_utils.h"
@@ -126,6 +127,7 @@ void MouseWheelEventQueue::ProcessMouseWheelAck(
         event_sent_for_gesture_ack_->event.PositionInScreen());
     scroll_update.resending_plugin_id = -1;
 
+#if !defined(OS_MACOSX)
     // Swap X & Y if Shift is down and when there is no horizontal movement.
     if ((event_sent_for_gesture_ack_->event.GetModifiers() &
          WebInputEvent::kShiftKey) != 0 &&
@@ -134,7 +136,9 @@ void MouseWheelEventQueue::ProcessMouseWheelAck(
           event_sent_for_gesture_ack_->event.delta_y;
       scroll_update.data.scroll_update.delta_y =
           event_sent_for_gesture_ack_->event.delta_x;
-    } else {
+    } else
+#endif  // OS_MACOSX
+    {
       scroll_update.data.scroll_update.delta_x =
           event_sent_for_gesture_ack_->event.delta_x;
       scroll_update.data.scroll_update.delta_y =

content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc

@@ -15,6 +15,7 @@
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_task_environment.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "build/build_config.h"
 #include "content/browser/renderer_host/input/timeout_monitor.h"
 #include "content/common/input/synthetic_web_input_event_builders.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -621,4 +622,29 @@ TEST_F(MouseWheelEventQueueTest, WheelScrollingWasLatchedHistogramCheck) {
     histogram_tester.ExpectBucketCount(latching_histogram_name, 1, 1);
 }
 
+#if defined(OS_MACOSX)
+TEST_F(MouseWheelEventQueueTest, DoNotSwapXYForShiftScroll) {
+  // Send an event with shift modifier, zero value for delta X, and no direction
+  // for |rails_mode|. Do not swap the scroll direction.
+  SendMouseWheel(kWheelScrollX, kWheelScrollY, kWheelScrollGlobalX,
+                 kWheelScrollGlobalY, 0.0, 1.0, WebInputEvent::kShiftKey, false,
+                 WebMouseWheelEvent::kPhaseBegan,
+                 WebMouseWheelEvent::kPhaseNone, WebInputEvent::kRailsModeFree);
+  EXPECT_EQ(0U, queued_event_count());
+  EXPECT_TRUE(event_in_flight());
+  EXPECT_EQ(1U, GetAndResetSentEventCount());
+
+  // Receive an ACK for the mouse wheel event.
+  SendMouseWheelEventAck(INPUT_EVENT_ACK_STATE_NOT_CONSUMED);
+  EXPECT_EQ(0U, queued_event_count());
+  EXPECT_FALSE(event_in_flight());
+  EXPECT_EQ(WebInputEvent::kMouseWheel, acked_event().GetType());
+  EXPECT_EQ(1U, GetAndResetAckedEventCount());
+
+  EXPECT_EQ(2U, all_sent_events().size());
+  EXPECT_EQ(0U, sent_gesture_event(1)->data.scroll_update.delta_x);
+  EXPECT_EQ(1U, sent_gesture_event(1)->data.scroll_update.delta_y);
+  EXPECT_EQ(2U, GetAndResetSentEventCount());
+}
+#endif
 }  // namespace content

content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.cc

@@ -17,6 +17,15 @@ MouseWheelRailsFilterMac::~MouseWheelRailsFilterMac() {
 
 WebInputEvent::RailsMode MouseWheelRailsFilterMac::UpdateRailsMode(
     const WebMouseWheelEvent& event) {
+  if (event.phase == WebMouseWheelEvent::kPhaseNone &&
+      event.momentum_phase == WebMouseWheelEvent::kPhaseNone) {
+    // We should only set the rails mode for trackpad wheel events. The AppKit
+    // documentation state that legacy mouse events (legacy mouse) do not have
+    // |phase| and |momentum_phase|.
+    // https://developer.apple.com/documentation/appkit/nsevent/1533550-phase.
+    return WebInputEvent::kRailsModeFree;
+  }
+
   // A somewhat-arbitrary decay constant for hysteresis.
   const float kDecayConstant = 0.8f;