Use scroll customization primitives for touch scrolling (behind REF).

Source/core/page/EventHandler.cpp

Index: Source/core/page/EventHandler.cpp
diff --git a/Source/core/page/EventHandler.cpp b/Source/core/page/EventHandler.cpp
index 7f2bf62203808476afff59d4924d8ac3c554a3d7..107c871e6bbb3c3f9093983f0d8e40d5b95c83c8 100644
--- a/Source/core/page/EventHandler.cpp
+++ b/Source/core/page/EventHandler.cpp
@@ -80,6 +80,7 @@
 #include "core/page/Page.h"
 #include "core/page/SpatialNavigation.h"
 #include "core/page/TouchAdjustment.h"
+#include "core/page/scrolling/ScrollState.h"
 #include "core/svg/SVGDocumentExtensions.h"
 #include "platform/PlatformGestureEvent.h"
 #include "platform/PlatformKeyboardEvent.h"
@@ -195,6 +196,31 @@ static inline bool shouldRefetchEventTarget(const MouseEventWithHitTestResults&
     return targetNode->isShadowRoot() && isHTMLInputElement(*toShadowRoot(targetNode)->host());
 }
 
+void recomputeScrollChain(const LocalFrame& frame, const Node& startNode,
+    WillBeHeapDeque<RefPtrWillBeMember<Element>>& scrollChain)
+{
+    scrollChain.clear();
+    bool rootLayerScrolls = frame.settings() && frame.settings()->rootLayerScrolls();
+
+    ASSERT(startNode.layoutObject());
+    LayoutBox* curBox = startNode.layoutObject()->enclosingBox();
+    bool addedDocumentElement = false;
+
+    while (curBox && (rootLayerScrolls || !curBox->isLayoutView())) {

[Rick Byers, 2015/03/11 02:22:18]

I think this deserves a comment describing why we're doing this.  Maybe just
"scrolling propagates along the containing block chain" or some such.  

Also I think this code is Shadow-DOM correct, but you should probably add at
least one test (eg. a scroller as a template argument to a custom element,
instantiated inside some other scroller inside the custom element - the chain
should reflect the instantiation).

[tdresser, 2015/03/20 18:00:37]

Done.
I'll add a test for scroll customization with shadow dom in the patch which
enables javascript scroll customization.

+        Node* curNode = curBox->node();
+        curBox = curBox->containingBlock();

[Rick Byers, 2015/03/11 02:22:18]

What if the first curBox is scrollable itself?  It won't get added to the scroll
chain since we walk up first before doing the below logic.

[tdresser, 2015/03/20 18:00:37]

We don't use curBox until the next iteration of the loop, so this is logically
correct. I've cleaned it up so it's a bit clearer.

[Rick Byers, 2015/03/26 21:22:49]

Acknowledged.

+        // FIXME: this should reject more elements.
+        if (!curNode || !curNode->isElementNode())
+            continue;
+        scrollChain.prepend(toElement(curNode));
+        if (curNode == frame.document()->documentElement())
+            addedDocumentElement = true;
+    }
+
+    if (!addedDocumentElement)

[Rick Byers, 2015/03/11 02:22:19]

Do we want to do this even when rootLayerScrolls is false?

Can't we simplify the loop by always stopping once we hit the documentElement or
RenderView, and then always (or at least whenver rootLayerScrolls is true)
adding the document element?  It's really special in that many properties /
operations on it are redirected to the RenderView.

[tdresser, 2015/03/20 18:00:36]

I've simplified the loop a bit. Actually determining if adding the
documentElement is necessary won't be possible until JS scroll customization is
implemented, so I'm going to mark this with a FIXME for now.

[Rick Byers, 2015/03/26 21:22:49]

Acknowledged.

+        scrollChain.prepend(frame.document()->documentElement());
+}
+
 EventHandler::EventHandler(LocalFrame* frame)
     : m_frame(frame)
     , m_mousePressed(false)
@@ -222,6 +248,7 @@ EventHandler::EventHandler(LocalFrame* frame)
     , m_longTapShouldInvokeContextMenu(false)
     , m_activeIntervalTimer(this, &EventHandler::activeIntervalTimerFired)
     , m_lastShowPressTimestamp(0)
+    , m_deltaConsumedForScrollSequence(false)
 {
 }
 
@@ -250,6 +277,7 @@ DEFINE_TRACE(EventHandler)
     visitor->trace(m_scrollGestureHandlingNode);
     visitor->trace(m_previousGestureScrolledNode);
     visitor->trace(m_lastDeferredTapElement);
+    visitor->trace(m_currentScrollChain);
 #endif
 }
 
@@ -944,11 +972,23 @@ bool EventHandler::scroll(ScrollDirection direction, ScrollGranularity granulari
     return false;
 }
 
+void EventHandler::customizedScroll(const Node& startNode, ScrollState& scrollState)
+{
+    if (scrollState.fullyConsumed())
+        return;
+
+    if (m_currentScrollChain.isEmpty())
+        recomputeScrollChain(*m_frame, startNode, m_currentScrollChain);
+    scrollState.setScrollChain(m_currentScrollChain);
+    scrollState.distributeToScrollChainDescendant();
+}
+
 bool EventHandler::bubblingScroll(ScrollDirection direction, ScrollGranularity granularity, Node* startingNode)
 {
     // The layout needs to be up to date to determine if we can scroll. We may be
     // here because of an onLoad event, in which case the final layout hasn't been performed yet.
     m_frame->document()->updateLayoutIgnorePendingStylesheets();
+    // FIXME: enable scroll customization in this case.

[Rick Byers, 2015/03/11 02:22:18]

you might want to reference your scroll customization bug number - then we can
do a search for that URL in the code before calling your bug fixed.  I assume
this will be the first of many such FIXMEs ;-)

[tdresser, 2015/03/20 18:00:37]

Done.

     if (scroll(direction, granularity, startingNode))
         return true;
     LocalFrame* frame = m_frame;
@@ -2059,6 +2099,8 @@ void EventHandler::defaultWheelEventHandler(Node* startNode, WheelEvent* wheelEv
 
     // Break up into two scrolls if we need to.  Diagonal movement on
     // a MacBook pro is an example of a 2-dimensional mouse wheel event (where both deltaX and deltaY can be set).
+
+    // FIXME: enable scroll customization in this case.
     if (scroll(ScrollRight, granularity, startNode, &stopNode, wheelEvent->deltaX(), roundedIntPoint(wheelEvent->absoluteLocation())))
         wheelEvent->setDefaultHandled();
 
@@ -2195,7 +2237,6 @@ bool EventHandler::handleGestureScrollEvent(const PlatformGestureEvent& gestureE
 
         m_lastGestureScrollOverWidget = result.isOverWidget();
         m_scrollGestureHandlingNode = eventTarget;
-        m_previousGestureScrolledNode = nullptr;

[Rick Byers, 2015/03/11 02:22:19]

why remove this?

[tdresser, 2015/03/20 18:00:37]

Looks wrong to me. Fixed.

 
         if (!scrollbar)
             scrollbar = result.scrollbar();
@@ -2448,11 +2489,19 @@ bool EventHandler::passScrollGestureEventToWidget(const PlatformGestureEvent& ge
 
 bool EventHandler::handleGestureScrollEnd(const PlatformGestureEvent& gestureEvent) {
     RefPtrWillBeRawPtr<Node> node = m_scrollGestureHandlingNode;
-    clearGestureScrollNodes();
 
-    if (node)
-        passScrollGestureEventToWidget(gestureEvent, node->layoutObject());
+    if (node) {
+        if (passScrollGestureEventToWidget(gestureEvent, node->layoutObject()))
+            return false;

[Rick Byers, 2015/03/11 02:22:18]

this doesn't make sense to me.  If the subframe handles the GSE then we report
NOT handled and terminate?  The original code made sure to clear any scrolling
state across the entire frame chain involved.  Your code doesn't, does it?

Do you need some more iframe tests?

[tdresser, 2015/03/20 18:00:37]

This was correct before, but _extremely_ confusing.

Note that the return value here is the same as it was before - we only ever
return false (I believe this is because we need to ensure that all frames
receive the gesture end). The return value of passScrollGestureEventToWidget is
ignored.

The early out will never trigger, since we always return false in
handleGestureScrollEnd... I've gotten rid of it.


clearGestureScrollNodes() will always be called across all frames, and it clears
up all of the scrolling state.

There's a reasonable set of existing iframe tests - I'm planning to add some
more when I align the behavior of main thread iframe scroll bubbling with impl
thread iframe scroll bubbling, but until then I think the coverage is
acceptable. Are there specific tests you think are missing?

[Rick Byers, 2015/03/26 21:22:49]

Thanks, makes sense.
No, that sounds fine.  Thanks.

+        if (RuntimeEnabledFeatures::scrollCustomizationEnabled()) {
+            RefPtrWillBeRawPtr<ScrollState> scrollState = ScrollState::create(
+                0, 0, 0, 0, 0, gestureEvent.inertial(), false, true, true);

[Rick Byers, 2015/03/11 02:22:18]

Blink coding style says: "Prefer enums to bools on function parameters if
callers are likely to be passing constants"
(http://www.chromium.org/blink/coding-style#TOC-Names) precisely because this
"false, true, true" is hard to understand.  I'd probably also accept comments in
front of each of these 3 args giving its name.

[tdresser, 2015/03/20 18:00:37]

Done.

+            customizedScroll(*node.get(), *scrollState);
+        }
+    }
 
+    clearGestureScrollNodes();
+    m_currentScrollChain.clear();
     return false;
 }
 
@@ -2471,24 +2520,30 @@ bool EventHandler::handleGestureScrollBegin(const PlatformGestureEvent& gestureE
     while (m_scrollGestureHandlingNode && !m_scrollGestureHandlingNode->layoutObject())
         m_scrollGestureHandlingNode = m_scrollGestureHandlingNode->parentOrShadowHostNode();
 
-    if (!m_scrollGestureHandlingNode)
-        return false;
+    if (!m_scrollGestureHandlingNode) {
+        if (RuntimeEnabledFeatures::scrollCustomizationEnabled())
+            m_scrollGestureHandlingNode = m_frame->document()->documentElement();
+        else
+            return false;
+    }
+    ASSERT(m_scrollGestureHandlingNode);
 
     passScrollGestureEventToWidget(gestureEvent, m_scrollGestureHandlingNode->layoutObject());
+    if (!RuntimeEnabledFeatures::scrollCustomizationEnabled()) {

[Rick Byers, 2015/03/11 02:22:18]

this would be slightly clearer to me as a if(enabled)/else block (with a single
return true at the end.  WDYT?

[tdresser, 2015/03/20 18:00:37]

Done.

[Rick Byers, 2015/03/26 21:22:49]

nit: I meants put the true case first to avoid a double-negative

[tdresser, 2015/03/27 15:28:52]

Done.

+        if (m_frame->isMainFrame())
+            m_frame->host()->topControls().scrollBegin();
+        return true;
+    }
 
-    if (m_frame->isMainFrame())
-        m_frame->host()->topControls().scrollBegin();
+    m_deltaConsumedForScrollSequence = false;
+    m_currentScrollChain.clear();
+    RefPtrWillBeRawPtr<ScrollState> scrollState = ScrollState::create(
+        0, 0, 0, 0, 0, false, true, false, true);

[Rick Byers, 2015/03/11 02:22:18]

ditto

[tdresser, 2015/03/20 18:00:37]

Done.

+    customizedScroll(*m_scrollGestureHandlingNode.get(), *scrollState);
 
     return true;
 }
 
-static bool scrollAreaOnBothAxes(const FloatSize& delta, ScrollableArea& view)
-{
-    bool scrolledHorizontal = view.scroll(ScrollLeft, ScrollByPrecisePixel, delta.width());
-    bool scrolledVertical = view.scroll(ScrollUp, ScrollByPrecisePixel, delta.height());
-    return scrolledHorizontal || scrolledVertical;
-}
-
 bool EventHandler::handleGestureScrollUpdate(const PlatformGestureEvent& gestureEvent)
 {
     ASSERT(gestureEvent.type() == PlatformEvent::GestureScrollUpdate);
@@ -2511,60 +2566,55 @@ bool EventHandler::handleGestureScrollUpdate(const PlatformGestureEvent& gesture
         if (passScrollGestureEventToWidget(gestureEvent, renderer)) {
             if (gestureEvent.preventPropagation())
                 m_previousGestureScrolledNode = m_scrollGestureHandlingNode;
-
+            m_deltaConsumedForScrollSequence = true;
             return true;
         }
 
+        bool scrolled = false;
+        if (RuntimeEnabledFeatures::scrollCustomizationEnabled()) {
+            RefPtrWillBeRawPtr<ScrollState> scrollState = ScrollState::create(
+                gestureEvent.deltaX(), gestureEvent.deltaY(),
+                0, gestureEvent.velocityX(), gestureEvent.velocityY(),
+                gestureEvent.inertial(), false, false, true,
+                !gestureEvent.preventPropagation(), m_deltaConsumedForScrollSequence);
+            if (m_previousGestureScrolledNode) {

[Rick Byers, 2015/03/11 02:22:18]

So does this field now represent just native scrolls (doesn't include custom
scrolled nodes)?  Perhaps update it's comment or maybe even it's name.  I'm not
fully following (although I haven't finished looking at all code yet) why you
need this, and 'currentNativeScrollingElement', perhaps some more comments would
help.

[tdresser, 2015/03/20 18:00:37]

Done.

+                ASSERT(m_previousGestureScrolledNode->isElementNode());
+                scrollState->setCurrentNativeScrollingElement(toElement(m_previousGestureScrolledNode.get()));
+            }
+            customizedScroll(*node, *scrollState);
+            m_previousGestureScrolledNode = scrollState->currentNativeScrollingElement();
+            m_deltaConsumedForScrollSequence = scrollState->deltaConsumedForScrollSequence();
 
-        if (gestureEvent.preventPropagation())
-            stopNode = m_previousGestureScrolledNode.get();
-
-        // First try to scroll the closest scrollable LayoutBox ancestor of |node|.
-        ScrollGranularity granularity = ScrollByPixel;
-        bool horizontalScroll = scroll(ScrollLeft, granularity, node, &stopNode, delta.width());
-        bool verticalScroll = scroll(ScrollUp, granularity, node, &stopNode, delta.height());
+            scrolled = scrollState->deltaX() != gestureEvent.deltaX()
+                || scrollState->deltaY() != gestureEvent.deltaY();
+        } else {
+            if (gestureEvent.preventPropagation())
+                stopNode = m_previousGestureScrolledNode.get();
 
-        if (gestureEvent.preventPropagation())
-            m_previousGestureScrolledNode = stopNode;
+            // First try to scroll the closest scrollable LayoutBox ancestor of |node|.
+            ScrollGranularity granularity = ScrollByPixel;
+            scrolled = scroll(ScrollLeft, granularity, node, &stopNode, delta.width())
+                | scroll(ScrollUp, granularity, node, &stopNode, delta.height());

[Rick Byers, 2015/03/11 02:22:19]

This is a little subtle (I could imagine someone replacing | with || and
breaking things).  Maybe a comment or just return to the more explicit form
(scrolled = horizontalScroll || verticalScroll).

[tdresser, 2015/03/20 18:00:37]

Done.

 
-        if (horizontalScroll || verticalScroll) {
+            if (gestureEvent.preventPropagation())
+                m_previousGestureScrolledNode = stopNode;
+        }
+        if (scrolled) {
             setFrameWasScrolledByUser();
             return true;
         }
     }
 
-    // If this is main frame, allow top controls to scroll first and update
-    // delta accordingly
-    bool consumed = false;
-    if (m_frame->isMainFrame() && shouldTopControlsConsumeScroll(delta)) {
-        FloatSize excessDelta = m_frame->host()->topControls().scrollBy(delta);
-        consumed = excessDelta != delta;
-        delta = excessDelta;
-
-        if (delta.isZero())
-            return consumed;
-    }
+    if (RuntimeEnabledFeatures::scrollCustomizationEnabled())
+        return false;
 
     // Try to scroll the frame view.
-    FrameView* view = m_frame->view();
-    if (!view)
-        return consumed;
-
-    if (scrollAreaOnBothAxes(delta, *view)) {
+    if (m_frame->applyScrollDelta(delta, false)) {
         setFrameWasScrolledByUser();
         return true;
     }
 
-    // If this is the main frame and it didn't scroll, propagate up to the pinch viewport.
-    if (!m_frame->settings()->pinchVirtualViewportEnabled() || !m_frame->isMainFrame())
-        return consumed;
-
-    if (scrollAreaOnBothAxes(delta, m_frame->host()->pinchViewport())) {
-        setFrameWasScrolledByUser();
-        return true;
-    }
-
-    return consumed;
+    return false;
 }
 
 void EventHandler::clearGestureScrollNodes()
@@ -3394,6 +3444,7 @@ void EventHandler::defaultSpaceEventHandler(KeyboardEvent* event)
         return;
 
     ScrollDirection direction = event->shiftKey() ? ScrollBlockDirectionBackward : ScrollBlockDirectionForward;
+    // FIXME: enable scroll customization in this case.
     if (scroll(direction, ScrollByPage)) {
         event->setDefaultHandled();
         return;
@@ -3911,18 +3962,4 @@ unsigned EventHandler::accessKeyModifiers()
 #endif
 }
 
-bool EventHandler::shouldTopControlsConsumeScroll(FloatSize scrollDelta) const
-{
-    // Always consume if it's in the direction to show the top controls.
-    if (scrollDelta.height() > 0)
-        return true;
-
-    // If it's in the direction to hide the top controls, only consume when the frame can also scroll.
-    if (m_frame->view()->scrollPosition().y() < m_frame->view()->maximumScrollPosition().y())
-        return true;
-
-    return false;
-}
-
-
 } // namespace blink