Merged FrameView and LayoutBox scrolling in EventHandler.

third_party/WebKit/Source/core/input/EventHandler.cpp

Index: third_party/WebKit/Source/core/input/EventHandler.cpp
diff --git a/third_party/WebKit/Source/core/input/EventHandler.cpp b/third_party/WebKit/Source/core/input/EventHandler.cpp
index 22323e0e19cc2ee1cd2e78d43becbc10fc7322be..9a946826a43bdb87c8729c433e9d8d16f7e10c10 100644
--- a/third_party/WebKit/Source/core/input/EventHandler.cpp
+++ b/third_party/WebKit/Source/core/input/EventHandler.cpp
@@ -600,8 +600,9 @@ void EventHandler::stopAutoscroll()
         controller->stopAutoscroll();
 }
 
-ScrollResult EventHandler::physicalScroll(ScrollGranularity granularity, const FloatSize& delta, Node* startNode, Node** stopNode)
+ScrollResult EventHandler::physicalScroll(ScrollGranularity granularity, const FloatSize& delta, Node* startNode, Node** stopNode, bool& consumed)

[tdresser, 2016/03/01 21:57:40]

Would it be better to include this as part of the ScrollResult?

(Question, not suggestion)

[bokan, 2016/03/01 22:09:39]

I tried that at first but it got ambiguous in a number of places. As far as I
can tell, this is the only place that makes the distinction between scrolling
and consumption so I think it makes sense to leave it separate for now. We can
reevaluate later if it makes sense in other places. At the least, it would take
a separate patch where we adjust all the userScroll call sites to check for
consumed instead.

[tdresser, 2016/03/02 14:27:20]

Acknowledged.

 {
+    consumed = false;
     if (delta.isZero())
         return ScrollResult();
 
@@ -613,7 +614,7 @@ ScrollResult EventHandler::physicalScroll(ScrollGranularity granularity, const F
     ScrollResult result;
 
     LayoutBox* curBox = node->layoutObject()->enclosingBox();
-    while (curBox && !curBox->isLayoutView()) {
+    while (curBox) {
         // If we're at the stopNode, we should try to scroll it but we shouldn't
         // chain past it.
         bool shouldStopChaining =
@@ -625,16 +626,11 @@ ScrollResult EventHandler::physicalScroll(ScrollGranularity granularity, const F
 
         if (result.didScroll() || shouldStopChaining) {
             setFrameWasScrolledByUser();
-            if (!result.didScroll()) {
-                // TODO(bokan): We should probably add a "shouldPropagate" bit
-                // on the result rather than lying to the caller.
-                result.didScrollX = true;
-                result.didScrollY = true;
-            }
+            consumed = true;
             return result;
         }
 
-        curBox = curBox->containingBlock();
+        curBox = curBox->isLayoutView() ? nullptr : curBox->containingBlock();

[szager1, 2016/03/02 19:14:34]

Is is necessary to special-case LayoutView here?

[bokan, 2016/03/02 20:37:13]

Ah, I thought it was since I assumed containingBlock on an iframe's LayoutView
would return the owner LayoutObject in the parent document but I just checked
this and that's false. Thanks!

     }
 
     return result;
@@ -656,7 +652,7 @@ bool EventHandler::logicalScroll(ScrollDirection direction, ScrollGranularity gr
     m_frame->document()->updateLayoutIgnorePendingStylesheets();
 
     LayoutBox* curBox = node->layoutObject()->enclosingBox();
-    while (curBox && !curBox->isLayoutView()) {
+    while (curBox) {
         ScrollDirectionPhysical physicalDirection = toPhysicalDirection(
             direction, curBox->isHorizontalWritingMode(), curBox->style()->isFlippedBlocksWritingMode());
 
@@ -667,7 +663,7 @@ bool EventHandler::logicalScroll(ScrollDirection direction, ScrollGranularity gr
             return true;
         }
 
-        curBox = curBox->containingBlock();
+        curBox = curBox->isLayoutView() ? nullptr : curBox->containingBlock();

[szager1, 2016/03/02 19:14:34]

ditto

[bokan, 2016/03/02 20:37:13]

Acknowledged.

     }
 
     return false;
@@ -698,18 +694,8 @@ bool EventHandler::bubblingScroll(ScrollDirection direction, ScrollGranularity g
     // FIXME: enable scroll customization in this case. See crbug.com/410974.
     if (logicalScroll(direction, granularity, startingNode))
         return true;
-    LocalFrame* frame = m_frame;
-    FrameView* view = frame->view();
-    if (view) {
-        ScrollDirectionPhysical physicalDirection =
-            toPhysicalDirection(direction, view->isVerticalDocument(), view->isFlippedDocument());
-        if (view->scrollableArea()->userScroll(granularity, toScrollDelta(physicalDirection, 1)).didScroll()) {
-            setFrameWasScrolledByUser();
-            return true;
-        }
-    }
 
-    Frame* parentFrame = frame->tree().parent();
+    Frame* parentFrame = m_frame->tree().parent();
     if (!parentFrame || !parentFrame->isLocalFrame())
         return false;
     // FIXME: Broken for OOPI.
@@ -1763,35 +1749,6 @@ bool EventHandler::slideFocusOnShadowHostIfNecessary(const Element& element)
     return false;
 }
 
-namespace {
-
-ScrollResult scrollAreaWithWheelEvent(const PlatformWheelEvent& event, ScrollableArea& scrollableArea)
-{
-    float deltaX = event.getRailsMode() != PlatformEvent::RailsModeVertical ? event.deltaX() : 0;
-    float deltaY = event.getRailsMode() != PlatformEvent::RailsModeHorizontal ? event.deltaY() : 0;
-
-    ScrollGranularity granularity =
-        event.granularity() == ScrollByPixelWheelEvent ? ScrollByPixel : ScrollByPage;
-
-    if (event.hasPreciseScrollingDeltas() && granularity == ScrollByPixel)
-        granularity = ScrollByPrecisePixel;
-
-    // If the event is a "PageWheelEvent" we should disregard the delta and
-    // scroll by *one* page length per event.
-    if (event.granularity() == ScrollByPageWheelEvent) {
-        if (deltaX)
-            deltaX = deltaX > 0 ? 1 : -1;
-        if (deltaY)
-            deltaY = deltaY > 0 ? 1 : -1;

[tdresser, 2016/03/01 21:57:40]

I think this logic is so that we never coalesce page wheel scrolls into
scrolling multiple pages. I think this may make sense, though it only applies
when you have page based scrolling on in Windows.

[bokan, 2016/03/01 22:09:39]

Ah, could we ensure this is enforced at coalescing instead or do we honour a
non-(1|0) delta in other places? 

[tdresser, 2016/03/02 14:27:20]

Moving this to
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/inp...
makes sense to me.

-    }
-
-    // On a wheel event, positive delta is meant to scroll up and left, which
-    // is the opposite of deltas in the scrolling system.
-    return scrollableArea.userScroll(granularity, FloatSize(-deltaX, -deltaY));
-}
-
-} // namespace
-
 WebInputEventResult EventHandler::handleWheelEvent(const PlatformWheelEvent& event)
 {
     Document* doc = m_frame->document();
@@ -1841,24 +1798,6 @@ WebInputEventResult EventHandler::handleWheelEvent(const PlatformWheelEvent& eve
         }
     }
 
-    // We do another check on the frame view because the event handler can run
-    // JS which results in the frame getting destroyed.
-    view = m_frame->view();
-    if (!view)
-        return WebInputEventResult::NotHandled;
-
-    // Wheel events which do not scroll are used to trigger zooming.
-    if (!event.canScroll())
-        return WebInputEventResult::NotHandled;
-
-    ScrollResult scrollResult = scrollAreaWithWheelEvent(event, *view->scrollableArea());
-    if (m_frame->isMainFrame() && m_frame->settings() && m_frame->settings()->reportWheelOverscroll())
-        handleOverscroll(scrollResult);
-    if (scrollResult.didScroll()) {
-        setFrameWasScrolledByUser();
-        return WebInputEventResult::HandledSystem;
-    }
-
     return WebInputEventResult::NotHandled;
 }
 
@@ -1874,6 +1813,15 @@ void EventHandler::defaultWheelEventHandler(Node* startNode, WheelEvent* wheelEv
     ScrollGranularity granularity = wheelGranularityToScrollGranularity(wheelEvent);
     Node* node = nullptr;
 
+    // TODO(bokan): Previously, the code for scrolling a view from
+    // handleWheelEvent would clamp deltas to -1/1 for non-pixel scrolls, which
+    // is inconsistent with how the rest of the layout tree is scrolled (non-1
+    // deltas are allowed on non-pixel scrolls). We can probably just remove
+    // this ASSERT if its not firing. Otherwise add back in the clamping.
+    ASSERT((granularity == ScrollByPixel || granularity == ScrollByPrecisePixel)

[bokan, 2016/03/01 19:49:25]

Alternatively, we can just remove this ASSERT and wait for any bugs that might
be lurking.

[tdresser, 2016/03/01 21:57:40]

This should check if the granularity is page based, I think.

[tdresser, 2016/03/01 21:57:40]

ASSERT seems reasonable to me.

[bokan, 2016/03/01 22:09:39]

Right now, this check is making sure that only Pixel events have a delta that's
not 1 or 0. Do we want ScrollByLine to behave the same way, and if so, why treat
ScrollByLine and ScrollByPage differently?

My personal preference would be to honour whatever delta/granularity combo makes
it into EventHandler and ensure any kind of rules like this are enforced when we
generate the event. WDYT?

[bokan, 2016/03/01 22:09:39]

Acknowledged.

[tdresser, 2016/03/02 14:27:20]

Oops, the assert should fire if we coalesce page scrolls.

I think we've just treated scrollByPage differently historically.

You moved this from ScrollAnimator here:
https://codereview.chromium.org/1298973004/diff/100001/Source/core/input/Even...

Before that it was moved by apple folks in 2011, so I think the original reasons
for this are lost now...

I can imagine that a user rapidly wheel scrolling by line wants to scroll main
lines, but a user rapidly wheel scrolling by page would just want to advance to
the next page?

Hard to say...

I'd be fine switching this to scroll one page per unit of delta, but if we're
going to do that, let's land it in a separate patch.

[bokan, 2016/03/02 20:37:13]

Ok, I've removed the ASSERT here and reinstated the clamp to 1/-1/0. I'll try
moving it up to happen during coalescing in a future patch.

+        || (std::abs(wheelEvent->deltaX()) == 1 || !wheelEvent->deltaX()
+            && std::abs(wheelEvent->deltaY()) == 1 || !wheelEvent->deltaY()));
+
     // Diagonal movement on a MacBook pro is an example of a 2-dimensional
     // mouse wheel event (where both deltaX and deltaY can be set).
     FloatSize delta;
@@ -1885,10 +1833,14 @@ void EventHandler::defaultWheelEventHandler(Node* startNode, WheelEvent* wheelEv
         delta.setHeight(wheelEvent->deltaY());
 
     // FIXME: enable scroll customization in this case. See crbug.com/410974.
-    ScrollResult result = physicalScroll(granularity, delta, startNode, &node);
+    bool consumed = false;
+    ScrollResult result = physicalScroll(granularity, delta, startNode, &node, consumed);
 
-    if (result.didScroll())
+    if (consumed)
         wheelEvent->setDefaultHandled();
+
+    if (m_frame->isMainFrame() && m_frame->settings() && m_frame->settings()->reportWheelOverscroll())
+        handleOverscroll(result);
 }
 
 WebInputEventResult EventHandler::handleGestureShowPress()
@@ -2416,7 +2368,6 @@ WebInputEventResult EventHandler::handleGestureScrollUpdate(const PlatformGestur
             return result;
         }
 
-        bool scrolled = false;
         if (handleScrollCustomization) {
             OwnPtr<ScrollStateData> scrollStateData = adoptPtr(new ScrollStateData());
             scrollStateData->delta_x = delta.width();
@@ -2439,40 +2390,32 @@ WebInputEventResult EventHandler::handleGestureScrollUpdate(const PlatformGestur
             customizedScroll(*node, *scrollState);
             m_previousGestureScrolledNode = scrollState->currentNativeScrollingElement();
             m_deltaConsumedForScrollSequence = scrollState->deltaConsumedForScrollSequence();
-            scrolled = scrollState->deltaX() != delta.width()
-                || scrollState->deltaY() != delta.height();
+            if (scrollState->deltaX() != delta.width()
+                || scrollState->deltaY() != delta.width()) {
+                setFrameWasScrolledByUser();
+                return WebInputEventResult::HandledSystem;
+            }
         } else {
             Node* stopNode = nullptr;
             if (gestureEvent.preventPropagation())
                 stopNode = m_previousGestureScrolledNode.get();
 
-            ScrollResult result = physicalScroll(granularity, delta, node, &stopNode);
-
-            scrolled = result.didScroll();
+            bool consumed = false;
+            ScrollResult result = physicalScroll(granularity, delta, node, &stopNode, consumed);
 
             if (gestureEvent.preventPropagation())
                 m_previousGestureScrolledNode = stopNode;
 
-            resetOverscroll(result.didScrollX, result.didScrollY);
-        }
-        if (scrolled) {
-            setFrameWasScrolledByUser();
-            return WebInputEventResult::HandledSystem;
-        }
-    }
-
-    if (handleScrollCustomization)
-        return WebInputEventResult::NotHandled;
+            if (m_frame->isMainFrame() && (!stopNode || stopNode->layoutObject() == m_frame->view()->layoutView())) {
+                FloatPoint position = FloatPoint(gestureEvent.position().x(), gestureEvent.position().y());
+                handleOverscroll(result, position, velocity);
+            } else {
+                resetOverscroll(result.didScrollX, result.didScrollY);
+            }
 
-    // Try to scroll the frame view.
-    ScrollResult scrollResult = m_frame->applyScrollDelta(granularity, delta, false);
-    if (m_frame->isMainFrame()) {
-        FloatPoint position = FloatPoint(gestureEvent.position().x(), gestureEvent.position().y());
-        handleOverscroll(scrollResult, position, velocity);
-    }
-    if (scrollResult.didScroll()) {
-        setFrameWasScrolledByUser();
-        return WebInputEventResult::HandledSystem;
+            if (consumed)
+                return WebInputEventResult::HandledSystem;
+        }
     }
 
     return WebInputEventResult::NotHandled;
@@ -3434,16 +3377,6 @@ void EventHandler::defaultSpaceEventHandler(KeyboardEvent* event)
         event->setDefaultHandled();
         return;
     }
-
-    FrameView* view = m_frame->view();
-    if (!view)
-        return;
-
-    ScrollDirectionPhysical physicalDirection =
-        toPhysicalDirection(direction, view->isVerticalDocument(), view->isFlippedDocument());
-
-    if (view->scrollableArea()->userScroll(ScrollByPage, toScrollDelta(physicalDirection, 1)).didScroll())
-        event->setDefaultHandled();
 }
 
 void EventHandler::defaultBackspaceEventHandler(KeyboardEvent* event)