[scroll-snap] Treat wheel scroll as directional scrolling for snapping

Previously wheel scrolling was being treated similar to touchpad.
This lead to behavior where it was very hard to escape snap points
since at the end of every wheel tick which scrolls a small amount
Blink would snap back. The only escape was to wheel continuously
to get closer to the next snap point.

This is not ideal. It is much more natural for wheel ticks which
are discrete to be treated as "direction scroll" for snapping
purpose. So single wheel tick would take user to the next snap points.

See [1] for a before/after video.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=963609#c5


Bug: 963609
Change-Id: I72d1f23ab25e3d1ce29afa47d2d591497e8f1c6b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1758498
Commit-Queue: Majid Valipour <majidvp@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#692075}

third_party/blink/renderer/core/input/scroll_manager.cc

@@ -101,6 +101,7 @@ void ScrollManager::ClearGestureScrollState() {
   last_gesture_scroll_over_embedded_content_view_ = false;
   scroll_gesture_handling_node_ = nullptr;
   previous_gesture_scrolled_node_ = nullptr;
+  last_scroll_delta_for_scroll_gesture_ = FloatSize();
   delta_consumed_for_scroll_sequence_ = false;
   did_scroll_x_for_scroll_gesture_ = false;
   did_scroll_y_for_scroll_gesture_ = false;
@@ -630,6 +631,8 @@ WebInputEventResult ScrollManager::HandleGestureScrollUpdate(
   delta_consumed_for_scroll_sequence_ =
       scroll_state->DeltaConsumedForScrollSequence();
 
+  last_scroll_delta_for_scroll_gesture_ = delta;
+
   bool did_scroll_x = scroll_state->deltaX() != delta.Width();
   bool did_scroll_y = scroll_state->deltaY() != delta.Height();
 
@@ -706,7 +709,8 @@ WebInputEventResult ScrollManager::HandleGestureScrollEnd(
     ScrollState* scroll_state =
         ScrollState::Create(std::move(scroll_state_data));
     CustomizedScroll(*scroll_state);
-    snap_at_gesture_scroll_end = SnapAtGestureScrollEnd();
+
+    snap_at_gesture_scroll_end = SnapAtGestureScrollEnd(gesture_event);
     NotifyScrollPhaseEndForCustomizedScroll();
 
     if (RuntimeEnabledFeatures::OverscrollCustomizationEnabled() &&
@@ -737,7 +741,11 @@ LayoutBox* ScrollManager::LayoutBoxForSnapping() const {
   return previous_gesture_scrolled_node_->GetLayoutBox();
 }
 
-bool ScrollManager::SnapAtGestureScrollEnd() {
+ScrollOffset GetScrollDirection(FloatSize delta) {
+  return delta.ShrunkTo(FloatSize(1, 1)).ExpandedTo(FloatSize(-1, -1));
+}
+
+bool ScrollManager::SnapAtGestureScrollEnd(const WebGestureEvent& end_event) {
   if (!previous_gesture_scrolled_node_ ||
       (!did_scroll_x_for_scroll_gesture_ && !did_scroll_y_for_scroll_gesture_))
     return false;
@@ -747,6 +755,30 @@ bool ScrollManager::SnapAtGestureScrollEnd() {
   if (!snap_coordinator || !layout_box)
     return false;
 
+  bool is_mouse_wheel =
+      end_event.SourceDevice() == WebGestureDevice::kTouchpad &&
+      end_event.data.scroll_end.delta_units !=
+          ScrollGranularity::kScrollByPrecisePixel;
+
+  // Treat mouse wheel scrolls as direction only scroll with its last scroll
+  // delta amout. This means each wheel tick will prefer the next snap position
+  // in the given direction. This leads to a much better UX for wheels.
+  //
+  // Precise wheel and trackpads continue to be treated similar as end position
+  // scrolling.
+  if (is_mouse_wheel && !last_scroll_delta_for_scroll_gesture_.IsZero()) {
+    // TODO(majidvp): Currently DirectionStrategy uses current offset + delta as
+    // the intended offset and chooses snap offset closest to that intended
+    // offset. In this case, this is not correct because we are already at the
+    // intended position. DirectionStategy should be updated to use current
+    // offset as intended position. This requires changing how we snap in
+    // |ScrollManager::LogicalScroll()|. For now use a unit scroll offset to
+    // limit the miscalculation to 1px.
+    ScrollOffset scroll_direction =
+        GetScrollDirection(last_scroll_delta_for_scroll_gesture_);
+    return snap_coordinator->SnapForDirection(*layout_box, scroll_direction);
+  }
+
   return snap_coordinator->SnapAtCurrentPosition(
       *layout_box, did_scroll_x_for_scroll_gesture_,
       did_scroll_y_for_scroll_gesture_);

third_party/blink/renderer/core/input/scroll_manager.h

@@ -142,7 +142,7 @@ class CORE_EXPORT ScrollManager
   WebGestureEvent SynthesizeGestureScrollBegin(
       const WebGestureEvent& update_event);
 
-  bool SnapAtGestureScrollEnd();
+  bool SnapAtGestureScrollEnd(const WebGestureEvent& end_event);
 
   void NotifyScrollPhaseBeginForCustomizedScroll(const ScrollState&);
   void NotifyScrollPhaseEndForCustomizedScroll();
@@ -167,6 +167,8 @@ class CORE_EXPORT ScrollManager
   // customization.
   Member<Node> previous_gesture_scrolled_node_;
 
+  FloatSize last_scroll_delta_for_scroll_gesture_;
+
   // True iff some of the delta has been consumed for the current
   // scroll sequence in this frame, or any child frames. Only used
   // with ScrollCustomization. If some delta has been consumed, a

third_party/blink/web_tests/fast/scroll-snap/snaps-after-wheel-scrolling.html

+<!DOCTYPE html>
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+<script src="../../resources/gesture-util.js"></script>
+<style>
+div {
+  position: absolute;
+}
+#scroller {
+  width: 500px;
+  height: 500px;
+  overflow: scroll;
+  scroll-snap-type: x mandatory;
+  border: solid black 5px;
+}
+#space {
+  width: 2000px;
+  height: 2000px;
+}
+.target {
+  width: 100px;
+  height: 100px;
+  scroll-snap-align: start;
+  background-color: blue;
+}
+</style>
+
+<div id="scroller">
+  <div id="space"></div>
+  <div class="target" style="left: 0px;"></div>
+  <div class="target" style="left: 120px;"></div>
+  <div class="target" style="left: 240px;"></div>
+
+</div>
+
+<script>
+// Turn off smooth scrolling so wheel scrolls immediately.
+internals.settings.setScrollAnimatorEnabled(false);
+
+const scroller = document.getElementById("scroller");
+
+function scrollLeft() {
+  return scroller.scrollLeft;
+}
+
+function wheelScroll(delta, x, y, direction) {
+  // A mouse wheel scroll comes from mouse input with imprecise delta.
+  return smoothScroll(delta, x ,y , GestureSourceType.MOUSE_INPUT, direction,
+                      SPEED_INSTANT, false /* is precise delta */);
+}
+
+function cleanup() {
+  scroller.scrollTo(0, 0);
+  assert_approx_equals(scroller.scrollLeft, 0, 1);
+}
+
+promise_test (async t => {
+  t.add_cleanup(cleanup);
+  // scroll just 10px so the current snap point remains closest.
+  await wheelScroll(10 /* pixels to scroll */, 50, 50, 'right');
+  await waitForAnimationEnd(scrollLeft, 500, 5);
+  assert_approx_equals(scroller.scrollLeft, 120, 1);
+}, "Wheel scrolling (right) should prefer the next snap point in scroll"
+   + " direction not the closest.");
+
+promise_test (async t => {
+  t.add_cleanup(cleanup);
+
+  // Scroll just 1px so the current snap point remains closest.
+  await wheelScroll(10 /* pixels to scroll */, 50, 50, 'left');
+  await waitForAnimationEnd(scrollLeft, 500, 5);
+  assert_approx_equals(scroller.scrollLeft, 0, 1);
+}, "Wheel scrolling (left) should prefer the next snap point in scroll"
+   + " direction not the closest.");
+
+promise_test (async t => {
+  t.add_cleanup(cleanup);
+  // Scroll so we pass the second snap point.
+  await wheelScroll(140 /* pixels to scroll */, 50, 50, 'right');
+  await waitForAnimationEnd(scrollLeft, 500, 5);
+  assert_approx_equals(scroller.scrollLeft, 240, 1);
+}, "Wheel scrolling should prefer the next snap point in scroll"
+   + " direction from scroll end position and not the scroll start position.");
+
+promise_test (async t => {
+  t.add_cleanup(cleanup);
+  // Scroll passed the last snap point.
+  await wheelScroll(200 /* pixels to scroll */, 50, 50, 'right');
+  await waitForAnimationEnd(scrollLeft, 500, 5);
+  assert_approx_equals(scroller.scrollLeft, 240, 1);
+}, "Wheel scrolling should prefer the closest scroll offset in the opposite"
+   + " direction when there is no snap point in the scroll direction.");
+
+</script>