Ensure logical scrolling does not happen if scroller snaps

The contract for `SnapCoordinator::PerformSnapping` is that it returns true
when scroll operations results in snapping. Returning true ensures we skip the
logical scroll.

In a previous patch [1] the logic was incorrectly changed to return true only
if we snap *and scroll as a result of the snap*. This meant that if we snap to
current position, that snap operation is ignored as it does not cause any
scrolling. This lead to failure of a few tests.

This patch reverts that change. It also removes a unnecessary check where we
check in snap logic where it tries to avoid scrolling if offsets are the same.
`ScrollableArea::SetScrollOffset` already has similar shortcuts so there is no
need to recreate them here which may have contributed to the error.

Note: At the time [1] was landing at least one of the two tests affected [2]
was marked as skipped (See http://crbug.com/922951), and the other test
started flaking one day after [1] was landed (See http://crbug.com/986018).


[1] https://chromium.googlesource.com/chromium/src/+/b65606034fd91c1aaf1d132a7601721e552d625f%5E%21/#F3
[2] fast/scroll-snap/snaps-after-keyboard-scrolling.html

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

third_party/blink/renderer/core/page/scrolling/snap_coordinator.cc

@@ -363,13 +363,10 @@ bool SnapCoordinator::PerformSnapping(
 
   scrollable_area->CancelScrollAnimation();
   scrollable_area->CancelProgrammaticScrollAnimation();
-  if (snap_point.value() != scrollable_area->ScrollPosition()) {
   scrollable_area->SetScrollOffset(
       scrollable_area->ScrollPositionToOffset(snap_point.value()),
       kProgrammaticScroll, kScrollBehaviorSmooth);
   return true;
-  }
-  return false;
 }
 
 base::Optional<cc::SnapContainerData> SnapCoordinator::GetSnapContainerData(

third_party/blink/renderer/core/page/scrolling/snap_coordinator.h

@@ -54,9 +54,11 @@ class CORE_EXPORT SnapCoordinator final
 
   // SnapAtCurrentPosition(), SnapForEndPosition(), SnapForDirection(), and
   // SnapForEndAndDirection() return true if snapping was performed, and false
-  // otherwise.
-  // SnapAtCurrentPosition() calls SnapForEndPosition() with the current scroll
-  // position.
+  // otherwise. Note that this does not necessarily mean that any scrolling was
+  // performed as a result e.g., if we are already at the snap point.
+  //
+  // SnapAtCurrentPosition() calls SnapForEndPosition() with the current
+  // scroll position.
   bool SnapAtCurrentPosition(const LayoutBox& snap_container,
                              bool scrolled_x,
                              bool scrolled_y) const;
@@ -81,6 +83,8 @@ class CORE_EXPORT SnapCoordinator final
 
  private:
   friend class SnapCoordinatorTest;
+
+  // Returns true if a snap point was found.
   bool PerformSnapping(const LayoutBox& snap_container,
                        const cc::SnapSelectionStrategy& strategy) const;
 

third_party/blink/web_tests/TestExpectations

@@ -5731,7 +5731,6 @@ crbug.com/922951 fast/dom/shadow/move-marquee-crossing-treescope-crash.html [ Sk
 crbug.com/922951 fast/forms/number/number-input-event-composed.html [ Skip ]
 crbug.com/922951 fast/loader/detach-while-printing.html [ Skip ]
 crbug.com/922951 fast/loader/document-open-iframe-then-detach.html [ Skip ]
-crbug.com/922951 fast/scroll-snap/snaps-after-keyboard-scrolling.html [ Skip ]
 crbug.com/922951 fast/text/international/inline-plaintext-relayout-with-leading-neutrals.html [ Skip ]
 crbug.com/922951 http/tests/cache/subresource-fragment-identifier.html [ Skip ]
 crbug.com/922951 http/tests/devtools/tracing/timeline-network-received-data.js [ Skip ]
@@ -6195,9 +6194,6 @@ crbug.com/985811 [ Win ] http/tests/devtools/elements/edit/edit-dom-actions-2.js
 crbug.com/985869 [ Linux Win7 ] http/tests/security/contentSecurityPolicy/object-src-no-url-allowed.html [ Pass Failure ]
 crbug.com/985869 [ Linux Win7 ] virtual/blink-cors/http/tests/security/contentSecurityPolicy/object-src-no-url-allowed.html [ Pass Failure ]
 
-# Sheriff 2019-07-20
-crbug.com/986018 fast/scroll-snap/snaps-after-keyboard-scrolling-rtl.html [ Pass Failure ]
-
 # ecosystem-infra sheriff 2019-07-22
 crbug.com/986477 [ Win ] external/wpt/cookies/path/match.html [ Pass Timeout ]
 crbug.com/986477 [ Win ] virtual/samesite-by-default-cookies/external/wpt/cookies/path/match.html [ Pass Timeout ]