[Reland] Make programmatic scrolls respect smooth scroll flag

Programmatic smooth scrolls should also be instant if the smooth scroll
flag is explicitly disabled.

Previously landed in https://crrev.com/c/1536759

To reland, this CL force enables the scroll animator in Blink web
tests. Without this, the usual flow for determining if the animator is
enabled is followed, which leads to inconsistencies. On Mac, the
animator is enabled in Chrome but not in content shell. This leads to
inconsistencies with other platforms (Mac tests ran without smooth
scroll while Linux/Windows ran with) and ensuring we test what we ship.

This change required some minor fixes to tests that assumed instant
scroll on Mac (and uncovered one real bug).

Bug: 944583
Change-Id: I114d8b882d9855fbe0c43645575a1cdd7c3b6e61
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2009842Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735006}

content/shell/renderer/web_test/blink_test_helpers.cc

@@ -117,6 +117,8 @@ void ApplyWebTestDefaultPreferences(WebPreferences* prefs) {
   prefs->strict_mixed_content_checking = false;
   prefs->strict_powerful_feature_restrictions = false;
   prefs->webgl_errors_to_console_enabled = false;
+  prefs->enable_scroll_animator =
+      !command_line.HasSwitch(switches::kDisableSmoothScrolling);
   base::string16 serif;
 #if defined(OS_MACOSX)
   prefs->cursive_font_family_map[kCommonScript] =

third_party/blink/renderer/core/frame/root_frame_viewport_test.cc

@@ -83,6 +83,7 @@ class ScrollableAreaStub : public GarbageCollected<ScrollableAreaStub>,
     DEFINE_STATIC_LOCAL(ScrollbarThemeOverlayMock, theme, ());
     return theme;
   }
+  bool ScrollAnimatorEnabled() const override { return true; }
 
   void Trace(blink::Visitor* visitor) override {
     ScrollableArea::Trace(visitor);
@@ -214,7 +215,7 @@ TEST_F(RootFrameViewportTest, UserInputScrollable) {
 
   // Layout viewport shouldn't scroll since it's not horizontally scrollable,
   // but visual viewport should.
-  root_frame_viewport->UserScroll(ScrollGranularity::kScrollByPixel,
+  root_frame_viewport->UserScroll(ScrollGranularity::kScrollByPrecisePixel,
                                   FloatSize(300, 0),
                                   ScrollableArea::ScrollCallback());
   EXPECT_EQ(ScrollOffset(0, 0), layout_viewport->GetScrollOffset());
@@ -222,7 +223,7 @@ TEST_F(RootFrameViewportTest, UserInputScrollable) {
   EXPECT_EQ(ScrollOffset(50, 0), root_frame_viewport->GetScrollOffset());
 
   // Vertical scrolling should be unaffected.
-  root_frame_viewport->UserScroll(ScrollGranularity::kScrollByPixel,
+  root_frame_viewport->UserScroll(ScrollGranularity::kScrollByPrecisePixel,
                                   FloatSize(0, 300),
                                   ScrollableArea::ScrollCallback());
   EXPECT_EQ(ScrollOffset(0, 150), layout_viewport->GetScrollOffset());
@@ -246,7 +247,7 @@ TEST_F(RootFrameViewportTest, UserInputScrollable) {
 
   // Layout viewport shouldn't scroll since it's not vertically scrollable,
   // but visual viewport should.
-  root_frame_viewport->UserScroll(ScrollGranularity::kScrollByPixel,
+  root_frame_viewport->UserScroll(ScrollGranularity::kScrollByPrecisePixel,
                                   FloatSize(0, 300),
                                   ScrollableArea::ScrollCallback());
   EXPECT_EQ(ScrollOffset(0, 0), layout_viewport->GetScrollOffset());
@@ -254,7 +255,7 @@ TEST_F(RootFrameViewportTest, UserInputScrollable) {
   EXPECT_EQ(ScrollOffset(0, 75), root_frame_viewport->GetScrollOffset());
 
   // Horizontal scrolling should be unaffected.
-  root_frame_viewport->UserScroll(ScrollGranularity::kScrollByPixel,
+  root_frame_viewport->UserScroll(ScrollGranularity::kScrollByPrecisePixel,
                                   FloatSize(300, 0),
                                   ScrollableArea::ScrollCallback());
   EXPECT_EQ(ScrollOffset(100, 0), layout_viewport->GetScrollOffset());
@@ -292,7 +293,7 @@ TEST_F(RootFrameViewportTest, TestScrollAnimatorUpdatedBeforeScroll) {
   visual_viewport->SetScrollOffset(ScrollOffset(50, 75), kProgrammaticScroll);
   EXPECT_EQ(ScrollOffset(50, 75), root_frame_viewport->GetScrollOffset());
 
-  root_frame_viewport->UserScroll(ScrollGranularity::kScrollByPixel,
+  root_frame_viewport->UserScroll(ScrollGranularity::kScrollByPrecisePixel,
                                   FloatSize(-50, 0),
                                   ScrollableArea::ScrollCallback());
   EXPECT_EQ(ScrollOffset(0, 75), root_frame_viewport->GetScrollOffset());
@@ -305,7 +306,7 @@ TEST_F(RootFrameViewportTest, TestScrollAnimatorUpdatedBeforeScroll) {
   layout_viewport->SetScrollOffset(ScrollOffset(100, 150), kProgrammaticScroll);
   EXPECT_EQ(ScrollOffset(100, 150), root_frame_viewport->GetScrollOffset());
 
-  root_frame_viewport->UserScroll(ScrollGranularity::kScrollByPixel,
+  root_frame_viewport->UserScroll(ScrollGranularity::kScrollByPrecisePixel,
                                   FloatSize(-100, 0),
                                   ScrollableArea::ScrollCallback());
   EXPECT_EQ(ScrollOffset(0, 150), root_frame_viewport->GetScrollOffset());

third_party/blink/renderer/core/scroll/scrollable_area.cc

@@ -315,7 +315,7 @@ void ScrollableArea::ProgrammaticScrollHelper(const ScrollOffset& offset,
         std::move(callback), WrapWeakPersistent(this)));
   }
 
-  if (scroll_behavior == kScrollBehaviorSmooth) {
+  if (scroll_behavior == kScrollBehaviorSmooth && ScrollAnimatorEnabled()) {
     GetProgrammaticScrollAnimator().AnimateToOffset(offset, is_sequenced_scroll,
                                                     std::move(callback));
   } else {

third_party/blink/renderer/core/scroll/scrollable_area_test.cc

@@ -24,8 +24,26 @@ namespace blink {
 namespace {
 
 using testing::_;
+using testing::Mock;
 using testing::Return;
 
+class MockAnimatingScrollableArea : public MockScrollableArea {
+ public:
+  static MockAnimatingScrollableArea* Create() {
+    return MakeGarbageCollected<MockAnimatingScrollableArea>();
+  }
+  static MockAnimatingScrollableArea* Create(
+      const ScrollOffset& maximum_scroll_offset) {
+    MockAnimatingScrollableArea* mock = Create();
+    mock->SetMaximumScrollOffset(maximum_scroll_offset);
+    return mock;
+  }
+  Scrollbar* HorizontalScrollbar() const override { return nullptr; }
+  Scrollbar* VerticalScrollbar() const override { return nullptr; }
+  MOCK_CONST_METHOD0(ScrollAnimatorEnabled, bool());
+  MOCK_METHOD0(ScheduleAnimation, bool());
+};
+
 class ScrollbarThemeWithMockInvalidation : public ScrollbarThemeOverlayMock {
  public:
   MOCK_CONST_METHOD0(ShouldRepaintAllPartsOnInvalidation, bool());
@@ -270,6 +288,35 @@ TEST_F(ScrollableAreaTest, ScrollableAreaDidScroll) {
   EXPECT_EQ(51, scrollable_area->ScrollOffsetInt().Height());
 }
 
+TEST_F(ScrollableAreaTest, ProgrammaticScrollRespectAnimatorEnabled) {
+  ScopedTestingPlatformSupport<TestingPlatformSupportWithMockScheduler>
+      platform;
+  MockAnimatingScrollableArea* scrollable_area =
+      MockAnimatingScrollableArea::Create(ScrollOffset(0, 100));
+  // Disable animations. Make sure an explicitly smooth programmatic scroll is
+  // instantly scrolled.
+  {
+    EXPECT_CALL(*scrollable_area, ScrollAnimatorEnabled())
+        .WillRepeatedly(Return(false));
+    EXPECT_CALL(*scrollable_area, ScheduleAnimation()).Times(0);
+    scrollable_area->SetScrollOffset(ScrollOffset(0, 100), kProgrammaticScroll,
+                                     kScrollBehaviorSmooth);
+    EXPECT_EQ(100, scrollable_area->GetScrollOffset().Height());
+  }
+  Mock::VerifyAndClearExpectations(scrollable_area);
+  // Enable animations. A smooth programmatic scroll should now schedule an
+  // animation rather than immediately mutating the offset.
+  {
+    EXPECT_CALL(*scrollable_area, ScrollAnimatorEnabled())
+        .WillRepeatedly(Return(true));
+    EXPECT_CALL(*scrollable_area, ScheduleAnimation()).WillOnce(Return(true));
+    scrollable_area->SetScrollOffset(ScrollOffset(0, 50), kProgrammaticScroll,
+                                     kScrollBehaviorSmooth);
+    // Offset is unchanged.
+    EXPECT_EQ(100, scrollable_area->GetScrollOffset().Height());
+  }
+}
+
 // Scrollbars in popups shouldn't fade out since they aren't composited and thus
 // they don't appear on hover so users without a wheel can't scroll if they fade
 // out.

third_party/blink/renderer/core/scroll/scrollbar_test_suite.h

@@ -93,7 +93,7 @@ class MockScrollableArea : public GarbageCollected<MockScrollableArea>,
   CompositorElementId GetScrollElementId() const override {
     return CompositorElementId();
   }
-  bool ScrollAnimatorEnabled() const override { return false; }
+  bool ScrollAnimatorEnabled() const override { return true; }
   int PageStep(ScrollbarOrientation) const override { return 0; }
   void ScrollControlWasSetNeedsPaintInvalidation() override {}
 
@@ -121,11 +121,12 @@ class MockScrollableArea : public GarbageCollected<MockScrollableArea>,
     ScrollableArea::Trace(visitor);
   }
 
- private:
+ protected:
   void SetMaximumScrollOffset(const ScrollOffset& maximum_scroll_offset) {
     maximum_scroll_offset_ = maximum_scroll_offset;
   }
 
+ private:
   ScrollOffset scroll_offset_;
   ScrollOffset maximum_scroll_offset_;
   Member<MockPlatformChromeClient> chrome_client_;

third_party/blink/web_tests/TestExpectations

@@ -5231,6 +5231,11 @@ crbug.com/875884 [ Linux ] lifecycle/background-change-lifecycle-count.html [ Pa
 crbug.com/875884 [ Win ] lifecycle/background-change-lifecycle-count.html [ Pass Failure ]
 crbug.com/875884 [ Win ] virtual/threaded/lifecycle/background-change-lifecycle-count.html [ Pass Failure ]
 
+# Broken when smooth scrolling was enabled in Mac web tests (real failure)
+crbug.com/1044137 [ Mac ] fast/scrolling/no-hover-during-smooth-keyboard-scroll.html [ Failure ]
+crbug.com/1044137 [ Mac ] virtual/scroll_customization/fast/scrolling/no-hover-during-smooth-keyboard-scroll.html [ Failure ]
+crbug.com/1044137 [ Mac ] virtual/threaded-prefer-compositing/fast/scrolling/no-hover-during-smooth-keyboard-scroll.html [ Failure ]
+
 # Test frequently times out on Mac CQ bots.
 crbug.com/874703 [ Mac ] http/tests/devtools/extensions/extensions-panel.js [ Timeout Pass ]
 

third_party/blink/web_tests/fast/scrolling/no-hover-during-smooth-keyboard-scroll.html

@@ -59,21 +59,27 @@
       assert_false(array[1].matches(":hover"));
       assert_equals(document.scrollingElement.scrollTop, 0);
 
-      // Keyboard scroll end up at 4th element. Hover state does not update during scrolling,
-      // only the 4th element has a hover effect.
+      // Keyboard scroll end up at 4th element. Hover state does not update
+      // during scrolling, only the 4th element has a hover effect. We wait for
+      // the end of each scroll animation for consistency; Mac differs from
+      // other platforms in how ongoing scroll animations are updated.
       eventSender.keyDown('ArrowDown');
+      await waitForAnimationEndTimeBased(() => { return document.scrollingElement.scrollTop; });
       eventSender.keyDown('ArrowDown');
+      await waitForAnimationEndTimeBased(() => { return document.scrollingElement.scrollTop; });
       eventSender.keyDown('ArrowDown');
+      await waitForAnimationEndTimeBased(() => { return document.scrollingElement.scrollTop; });
       eventSender.keyDown('ArrowDown');
-      // Wait enough time to see if we fire a fake mouse move event to update the hover state.
-      await waitForAnimationEnd(() => { return document.scrollingElement.scrollTop; }, 300, 15);
+      await waitForAnimationEndTimeBased(() => { return document.scrollingElement.scrollTop; });
+
+      // Make sure we update the  hover state when the scroll animation is finished.
       assert_approx_equals(document.scrollingElement.scrollTop, 3 * elementHeight, 25);
-      assert_false(array[0].matches(":hover"));
-      assert_false(array[1].matches(":hover"));
-      assert_false(array[2].matches(":hover"));
-      assert_true(array[3].matches(":hover"));
-      assert_false(array[4].matches(":hover"));
+      assert_false(array[0].matches(":hover"), "first element unexpectedly hovered");
+      assert_false(array[1].matches(":hover"), "second element unexpectedly hovered");
+      assert_false(array[2].matches(":hover"), "third element unexpectedly hovered");
+      assert_true(array[3].matches(":hover"), "expected element not hovered");
+      assert_false(array[4].matches(":hover"), "fifth element unexpectedly hovered");
     }, 'Keyboard smooth scroll on the page, no hover update during scrolling, but updating hover at the end of scroll.');
   }
 
-</script>
\ No newline at end of file
+</script>

third_party/blink/web_tests/fast/spatial-navigation/snav-unit-overflow-and-scroll-in-direction.html

@@ -43,7 +43,6 @@
       testRunner.dumpAsText();
       testRunner.overridePreference("WebKitTabToLinksPreferenceKey", 1);
       testRunner.overridePreference("WebKitSpatialNavigationEnabled", 1);
-      internals.settings.setScrollAnimatorEnabled(false);
       testRunner.waitUntilDone();
     }
 
@@ -68,8 +67,10 @@
       shouldBeTrue(String(document.scrollingElement.scrollTop != 0));
 
       // Then it scrolls down to the end of the page ...
-      if (window.eventSender)
+      if (window.eventSender) {
+        internals.settings.setScrollAnimatorEnabled(false);
         eventSender.keyDown("End");
+      }
 
       // And 'resultMap2' re-tries to move focus down.
       initTest(resultMap2, step2Completed);

third_party/blink/web_tests/platform/mac/fast/events/scrollbar-double-click-expected.txt

-Scroll offset is 720
+Scroll offset is 360

third_party/blink/web_tests/resources/gesture-util.js

@@ -89,6 +89,45 @@ function waitForAnimationEnd(getValue, max_frame, max_unchanged_frame) {
   })
 }
 
+// TODO(bokan): Replace uses of the above with this one.
+function waitForAnimationEndTimeBased(getValue) {
+  // Give up if the animation still isn't done after this many milliseconds.
+  const TIMEOUT_MS = 1000;
+
+  // If the value is unchanged for this many milliseconds, we consider the
+  // animation ended and return.
+  const END_THRESHOLD_MS = 50;
+
+  const START_TIME = performance.now();
+
+  let last_changed_time = START_TIME;
+  let last_value = getValue();
+  return new Promise((resolve, reject) => {
+    function tick() {
+      let cur_time = performance.now();
+
+      if (cur_time - last_changed_time > END_THRESHOLD_MS) {
+        resolve();
+        return;
+      }
+
+      if (cur_time - START_TIME > TIMEOUT_MS) {
+        reject();
+        return;
+      }
+
+      let current_value = getValue();
+      if (last_value != current_value) {
+        last_changed_time = cur_time;
+        last_value = current_value;
+      }
+
+      requestAnimationFrame(tick);
+    }
+    tick();
+  })
+}
+
 // Enums for gesture_source_type parameters in gpuBenchmarking synthetic
 // gesture methods. Must match C++ side enums in synthetic_gesture_params.h
 const GestureSourceType = (function() {