Update RootFrameViewport::userScroll to distribute scrolls between viewports.

third_party/WebKit/Source/core/frame/RootFrameViewport.cpp

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "config.h"
 #include "core/frame/RootFrameViewport.h"
 
 #include "core/frame/FrameView.h"
 #include "core/layout/ScrollAlignment.h"
 #include "platform/geometry/DoubleRect.h"
@@ skipping 228 matching lines @@
 {
     updateScrollAnimator();
 
     ScrollbarOrientation orientation;
 
     if (direction == ScrollUp || direction == ScrollDown)
         orientation = VerticalScrollbar;
     else
         orientation = HorizontalScrollbar;
 
-    if (layoutViewport().userInputScrollable(orientation) && visualViewport().userInputScrollable(orientation))
-        return ScrollableArea::userScroll(direction, granularity, delta);
+    if (layoutViewport().userInputScrollable(orientation) && visualViewport().userInputScrollable(orientation)) {
+        // Distribute the scroll between the visual and layout viewport.
+        ScrollbarOrientation orientation = scrollbarOrientationFromDirection(direction);
+        float step = scrollStep(granularity, orientation);
+
+        if (direction == ScrollUp || direction == ScrollLeft)
+            delta = -delta;
+
+        // This is the total amount we need to scroll. Instead of passing step
+        // to the scroll animator and letting it compute the total delta, we
+        // simply give it the total delta so that the unused delta it reports
+        // back can be applied to the secondary viewport.
+        delta *= step;

[bokan, 2015/12/04 19:39:02]

I don't think you need to do this. I believe the fact that the
ScrollAnimator::userScroll returns the unused delta in terms of steps rather
than pixels is a bug. I suspect the only reason we haven't seen it in real
behaviour is that overscrolling only happens on things with step 1 like touch
gestures and mac wheel events.

I'd remove this, fix ScrollAnimator to return the correct values, and use
primary/secondary.userScroll below. That should simplify this quite a bit.

[ymalik, 2015/12/04 20:11:01]

So the current flow is as follows
delta -> delta * step -> unused delta

Given that step >= 1, we are always actually scrolling more pixels than the
caller expects us to. It would be odd to return a higher unused delta than what
the function was called with. Ideally the userScroll method would get called
with the right pixels to scroll, but perhaps that's work for another CL. I just
want to unblock crbug.com/552556 and I can add a bug to clean this up on a whole
(along with your TODO on ScrollablaArea::userScrollHelper)? 

[bokan, 2015/12/04 20:42:52]

Ok, just had a look at ScrollAnimatorMac and it seems that does the same thing
so this is fine for now. Please add a TODO.

You still have a problem though :). It seems ScrollAnimator::userScroll always
returns 0 for unusedScrollDelta if its going to animate. You'll have to fix that
on the Animator side. 

[ymalik, 2015/12/07 17:53:14]

Hmm, so the ScrollAnimator::userScroll returns 0 for unsedScrollDelta so that
the scroll latches and does not cause the parent layer to scroll. So we can't
reply on the unused delta returned by the animators. That's fine because we
already know how much each viewport can scroll. Updated to give
visualviewport.userscroll the portion of delta that it can scroll for sure. 

+
+        cancelProgrammaticScrollAnimation();
+
+        ScrollableArea& primary = !m_invertScrollOrder ? layoutViewport() : visualViewport();

[bokan, 2015/12/04 20:42:52]

Replace primary with visual and secondary with layout. I could have sworn I
removed this weeks ago but I just checked and the CL is sitting uncommitted in
my queue with all the l-g-t-m :(

[ymalik, 2015/12/07 17:53:14]

Done.

+        ScrollableArea& secondary = !m_invertScrollOrder ? visualViewport() : layoutViewport();
+
+        ScrollResultOneDimensional pResult = primary.scrollAnimator()->userScroll(
+            orientation, granularity, /* step */ 1, delta);
+
+        // Scroll the secondary viewport if all of the scroll was not applied to the
+        // primary viewport.
+        if (pResult.unusedScrollDelta == 0)

[bokan, 2015/12/04 19:39:02]

Use ScrollResultOneDimensional::didScroll for this check instead

[ymalik, 2015/12/04 20:13:54]

didScroll can be true while we still want to distribute unsusedScrollDelta to
the secondary viewport. This check specifically checks whether we need to
delegate the remainder of the scroll delta to the secondary viewport.

[bokan, 2015/12/04 20:42:52]

Right, right, my bad.

+            return pResult;
+
+        ScrollResultOneDimensional sResult = secondary.scrollAnimator()->userScroll(
+            orientation, granularity, /* step */ 1, pResult.unusedScrollDelta);
+
+        if (!pResult.didScroll && !sResult.didScroll)
+            return ScrollResultOneDimensional(false, pResult.unusedScrollDelta / step);
+
+        return ScrollResultOneDimensional(true, sResult.unusedScrollDelta / step);

[bokan, 2015/12/04 19:39:02]

You can merge the last two returns together in the first parameter. Also, wrt
above comment, don't divide the step out of the delta.

[bokan, 2015/12/04 20:42:52]

You can still merge these two together (in fact, your second one should be a sum
of unused deltas, right?

[ymalik, 2015/12/07 17:53:14]

Merged. layoutResult.unusedScrollDelta is the amount out of delta that we
couldn't scroll.

+    }
 
     if (visualViewport().userInputScrollable(orientation))
         return visualViewport().userScroll(direction, granularity, delta);
 
     if (layoutViewport().userInputScrollable(orientation))
         return layoutViewport().userScroll(direction, granularity, delta);
 
     return ScrollResultOneDimensional(false, delta);
 }
 
@@ skipping 14 matching lines @@
     visualViewport().serviceScrollAnimations(monotonicTime);
 }
 
 void RootFrameViewport::updateCompositorScrollAnimations()
 {
     ScrollableArea::updateCompositorScrollAnimations();
     layoutViewport().updateCompositorScrollAnimations();
     visualViewport().updateCompositorScrollAnimations();
 }
 
+void RootFrameViewport::cancelProgrammaticScrollAnimation()
+{
+    ScrollableArea::cancelProgrammaticScrollAnimation();
+    layoutViewport().cancelProgrammaticScrollAnimation();
+    visualViewport().cancelProgrammaticScrollAnimation();
+}
+
 DEFINE_TRACE(RootFrameViewport)
 {
     visitor->trace(m_visualViewport);
     visitor->trace(m_layoutViewport);
     ScrollableArea::trace(visitor);
 }
 
 } // namespace blink