[Reland#3] 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
https://crrev.com/c/2009842
https://crrev.com/c/2022356

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).

Second reland fixes a few more tests on Mac. See diff from original
patchset for changes.

Third fix addresses flakiness in some of the Mac tests. Original
patchset has reverted CL.

TBR=flackr@chromium.org,nasko@chromium.org

Bug: 944583
Change-Id: I6b1ccb719b6bbed2b6d7495efa4d7846424796d9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2024163Reviewed-by: David Bokan <bokan@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735931}

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);
@@ -215,7 +216,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());
@@ -223,7 +224,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());
@@ -248,7 +249,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());
@@ -256,7 +257,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());
@@ -300,7 +301,7 @@ TEST_F(RootFrameViewportTest, TestScrollAnimatorUpdatedBeforeScroll) {
       mojom::blink::ScrollIntoViewParams::Type::kProgrammatic);
   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());
@@ -317,7 +318,7 @@ TEST_F(RootFrameViewportTest, TestScrollAnimatorUpdatedBeforeScroll) {
       mojom::blink::ScrollIntoViewParams::Type::kProgrammatic);
   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

@@ -325,7 +325,8 @@ void ScrollableArea::ProgrammaticScrollHelper(
   }
 
   if (scroll_behavior ==
-      mojom::blink::ScrollIntoViewParams::Behavior::kSmooth) {
+          mojom::blink::ScrollIntoViewParams::Behavior::kSmooth &&
+      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());
@@ -282,6 +300,39 @@ 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),
+        mojom::blink::ScrollIntoViewParams::Type::kProgrammatic,
+        mojom::blink::ScrollIntoViewParams::Behavior::kSmooth);
+    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),
+        mojom::blink::ScrollIntoViewParams::Type::kProgrammatic,
+        mojom::blink::ScrollIntoViewParams::Behavior::kSmooth);
+    // 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

@@ -94,7 +94,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 {}
   IntPoint ConvertFromRootFrame(const IntPoint& point_in_root_frame) const {
@@ -130,11 +130,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

@@ -5189,6 +5189,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/dom/vertical-scrollbar-in-rtl-expected.txt

@@ -4,8 +4,8 @@ vertical scroll: : Success
 continuously call window.scrollY : Success
 zoom in and out preserve scroll position: Success
 resize preserves scroll position: Success
-PASS window.scrollY == -3425 became true
-PASS window.scrollY == 0 became true
+Home key scrolls to top: Success
+End key scrolls to bottom: Success
 selectAll selects all document: Success
 PASS successfullyParsed is true
 

third_party/blink/web_tests/fast/dom/vertical-scrollbar-in-rtl.html

 <html dir=rtl style="-webkit-writing-mode:vertical-lr">
     <head>
         <script src="../../resources/js-test.js"></script>
+        <script src="../../resources/gesture-util.js"></script>
         <script language="javascript">
             window.jsTestIsAsync = true;
 
@@ -32,7 +33,7 @@
             var expectedScrollX = 500;
             var expectedScrollY = -1000;
 
-            onload = function()
+            onload = async function()
             {
                 if (window.testRunner)
                     testRunner.dumpAsText();
@@ -63,20 +64,34 @@
 
                 // Test HOME/END operation.
                 if (window.eventSender) {
-                    // Not using assert equal here since the behavior is different in each port.
-                    // For example, in Mac, HOME/END key reset both x and y scroll position.
-                    // In Chromium, HOME/END key only reset y scroll position, and x scroll position is preserved.
-                    function testEnd() {
+                    // Note: in Mac, HOME/END key resets the scroll position in
+                    // the x direction first, then y on a second call. This
+                    // differs from other platforms where HOME/END key only
+                    // reset y scroll position, and x scroll position is
+                    // preserved.
+                    async function testEnd() {
                         window.scrollTo(expectedScrollX, expectedScrollY);
                         eventSender.keyDown("End");
-                        shouldBecomeEqual('window.scrollY == 0', 'true', testSelectAll);
+                        await waitForCompositorCommit();
+                        await waitForAnimationEndTimeBased(() => { return window.scrollX; });
+                        eventSender.keyDown("End");
+                        await waitForCompositorCommit();
+                        await waitForAnimationEndTimeBased(() => { return window.scrollY; });
+                        assertEqual("End key scrolls to bottom", window.scrollY, 0);
                     }
-                    function testHome() {
+                    async function testHome() {
                         window.scrollTo(expectedScrollX, expectedScrollY);
                         eventSender.keyDown("Home");
-                        shouldBecomeEqual('window.scrollY == -3425', 'true', testEnd);
+                        await waitForCompositorCommit();
+                        await waitForAnimationEndTimeBased(() => { return window.scrollX; });
+                        eventSender.keyDown("Home");
+                        await waitForCompositorCommit();
+                        await waitForAnimationEndTimeBased(() => { return window.scrollY; });
+                        assertEqual("Home key scrolls to top", window.scrollY, -3425);
                     }
-                    testHome();
+                    await testHome();
+                    await waitForCompositorCommit();
+                    await testEnd();
                 }
 
                 function testSelectAll() {
@@ -90,6 +105,8 @@
 
                     finishJSTest();
                 }
+
+                testSelectAll();
             }
         </script>
     </head>

third_party/blink/web_tests/fast/events/keyboard-scroll-by-page.html

@@ -2,6 +2,7 @@
 <html>
 <head>
 <script src="../../resources/js-test.js"></script>
+<script src="../../resources/gesture-util.js"></script>
 <style type="text/css">
 ::-webkit-scrollbar {
     width: 0px;
@@ -30,7 +31,7 @@
 <div id="console"></div>
 <script type="text/javascript">
 
-function keyboardScrollPageDown()
+async function keyboardScrollPageDown()
 {
     if (navigator.platform.indexOf('Mac') == 0) {
         eventSender.keyDown('ArrowDown', ["altKey"]);
@@ -38,11 +39,14 @@ function keyboardScrollPageDown()
         eventSender.keyDown('PageDown');
     }
 
+    await waitForCompositorCommit();
+    await waitForAnimationEndTimeBased(() => { return document.scrollingElement.scrollTop; });
+
     // see kMinFractionToStepWhenPaging = 0.875f in ScrollableArea.cpp
     shouldBecomeEqual("document.scrollingElement.scrollTop >= window.innerHeight * 0.875", "true", keyboardScrollPageUp);
 }
 
-function keyboardScrollPageUp()
+async function keyboardScrollPageUp()
 {
     if (navigator.platform.indexOf('Mac') == 0) {
         eventSender.keyDown('ArrowUp', ["altKey"]);
@@ -50,13 +54,16 @@ function keyboardScrollPageUp()
         eventSender.keyDown('PageUp');
     }
 
+    await waitForCompositorCommit();
+    await waitForAnimationEndTimeBased(() => { return document.scrollingElement.scrollTop; });
+
     // PageDown + PageUp goes back to 0
     shouldBecomeEqual("document.scrollingElement.scrollTop", "0", finishJSTest);
 }
 
 jsTestIsAsync = true;
 
-function runTest()
+async function runTest()
 {
     if (window.eventSender) {
         description('This tests keyboard scrolling by pages.');

third_party/blink/web_tests/fast/events/platform-wheelevent-paging-xy-in-scrolling-page.html

 <html>
     <head>
         <script src="../../resources/js-test.js"></script>
+        <script src="../../resources/gesture-util.js"></script>
         <script>
             window.jsTestIsAsync = true;
             var isMacOSX = navigator.userAgent.indexOf("Mac OS X") >= 0;
@@ -23,13 +24,16 @@
             var event;
             var div;
 
-            function runTest()
+            async function runTest()
             {
                 document.body.addEventListener("mousewheel", mousewheelHandler, false);
 
                 if (window.eventSender) {
                     eventSender.mouseMoveTo(100, 110);
                     eventSender.continuousMouseScrollBy(-window.givenScrollLeft, -window.givenScrollTop, true);
+                    await waitForCompositorCommit();
+                    await waitForAnimationEndTimeBased(() => {return document.scrollingElement.scrollTop;});
+                    await waitForAnimationEndTimeBased(() => {return document.scrollingElement.scrollLeft;});
                 }
 
                 shouldBecomeEqual("document.scrollingElement.scrollTop == window.expectedScrollTop && " +

third_party/blink/web_tests/fast/events/space-scroll-textinput-canceled.html

@@ -2,6 +2,7 @@
 <html>
 <head>
 <script src="../../resources/js-test.js"></script>
+<script src="../../resources/gesture-util.js"></script>
 <style type="text/css">
 ::-webkit-scrollbar {
     width: 0px;
@@ -27,7 +28,7 @@ txt.addEventListener('textInput', function(e) {
     }
 }, false);
 
-function focusAndPressSpace()
+async function focusAndPressSpace()
 {
     document.getElementById('txt').focus();
     // Leave some space for PageUp
@@ -37,13 +38,16 @@ function focusAndPressSpace()
     // Space key shouldn't cancel PageUp
     eventSender.keyDown(" ", []);
 
+    await waitForCompositorCommit();
+    await waitForAnimationEndTimeBased(() => { return document.scrollingElement.scrollTop; });
+
     // PageUp should finish
     shouldBecomeEqual("document.scrollingElement.scrollTop", "0", finishJSTest);
 }
 
 jsTestIsAsync = true;
 
-function runTest()
+async function runTest()
 {
     if (window.eventSender) {
         focusAndPressSpace();

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>
+</script>
\ No newline at end of file

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 = 200;
+
+  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() {