Merged FrameView and LayoutBox scrolling in EventHandler.

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

 /*
  * Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2006 Alexey Proskuryakov (ap@webkit.org)
  * Copyright (C) 2012 Digia Plc. and/or its subsidiary(-ies)
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
@@ skipping 582 matching lines @@
 
     return result;
 }
 
 void EventHandler::stopAutoscroll()
 {
     if (AutoscrollController* controller = autoscrollController())
         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();
 
     Node* node = startNode;
     ASSERT(node && node->layoutObject());
 
     m_frame->document()->updateLayoutIgnorePendingStylesheets();
 
     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 =
             stopNode && *stopNode && curBox->node() == *stopNode;
         result = curBox->scroll(granularity, delta);
 
         if (result.didScroll() && stopNode)
             *stopNode = curBox->node();
 
         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;
 }
 
 bool EventHandler::logicalScroll(ScrollDirection direction, ScrollGranularity granularity, Node* startNode)
 {
     Node* node = startNode;
 
     if (!node)
         node = m_frame->document()->focusedElement();
 
     if (!node)
         node = m_mousePressNode.get();
 
     if (!node || !node->layoutObject())
         return false;
 
     m_frame->document()->updateLayoutIgnorePendingStylesheets();
 
     LayoutBox* curBox = node->layoutObject()->enclosingBox();
-    while (curBox && !curBox->isLayoutView()) {
+    while (curBox) {
         ScrollDirectionPhysical physicalDirection = toPhysicalDirection(
             direction, curBox->isHorizontalWritingMode(), curBox->style()->isFlippedBlocksWritingMode());
 
         ScrollResult result = curBox->scroll(granularity, toScrollDelta(physicalDirection, 1));
 
         if (result.didScroll()) {
             setFrameWasScrolledByUser();
             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;
 }
 
 void EventHandler::customizedScroll(const Node& startNode, ScrollState& scrollState)
 {
     if (scrollState.fullyConsumed())
         return;
 
@@ skipping 10 matching lines @@
 // TODO(bokan): This should be merged with logicalScroll assuming
 // defaultSpaceEventHandler's chaining scroll can be done crossing frames.
 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. 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.
     return toLocalFrame(parentFrame)->eventHandler().bubblingScroll(direction, granularity, m_frame->deprecatedLocalOwner());
 }
 
 IntPoint EventHandler::lastKnownMousePosition() const
 {
     return m_lastKnownMousePosition;
 }
@@ skipping 1033 matching lines @@
         Element* found = page->focusController().findFocusableElementInShadowHost(element);
         if (found && element.containsIncludingShadowDOM(found)) {
             // Use WebFocusTypeForward instead of WebFocusTypeMouse here to mean the focus has slided.
             found->focus(FocusParams(SelectionBehaviorOnFocus::Reset, WebFocusTypeForward, nullptr));
             return true;
         }
     }
     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();
 
     if (!doc->layoutView())
         return WebInputEventResult::NotHandled;
 
     RefPtrWillBeRawPtr<FrameView> protector(m_frame->view());
 
     FrameView* view = m_frame->view();
@@ skipping 29 matching lines @@
 
     if (node && sendDOMEvent) {
         RefPtrWillBeRawPtr<Event> domEvent = WheelEvent::create(event, node->document().domWindow());
         DispatchEventResult domEventResult = node->dispatchEvent(domEvent);
         if (domEventResult != DispatchEventResult::NotCanceled) {
             setFrameWasScrolledByUser();
             return toWebInputEventResult(domEventResult);
         }
     }
 
-    // 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;
 }
 
 void EventHandler::defaultWheelEventHandler(Node* startNode, WheelEvent* wheelEvent)
 {
     if (!startNode || !wheelEvent)
         return;
 
     // When the wheelEvent do not scroll, we trigger zoom in/out instead.
     if (!wheelEvent->canScroll())
         return;
 
     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;
 
     if (wheelEvent->getRailsMode() != Event::RailsModeVertical)
         delta.setWidth(wheelEvent->deltaX());
 
     if (wheelEvent->getRailsMode() != Event::RailsModeHorizontal)
         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()
 {
     m_lastShowPressTimestamp = WTF::monotonicallyIncreasingTime();
 
     FrameView* view = m_frame->view();
     if (!view)
         return WebInputEventResult::NotHandled;
     if (ScrollAnimatorBase* scrollAnimator = view->existingScrollAnimator())
@@ skipping 507 matching lines @@
                 // This is an optimization which doesn't apply with
                 // scroll customization enabled.
                 m_previousGestureScrolledNode = m_scrollGestureHandlingNode;
             }
             // FIXME: we should allow simultaneous scrolling of nested
             // iframes along perpendicular axes. See crbug.com/466991.
             m_deltaConsumedForScrollSequence = true;
             return result;
         }
 
-        bool scrolled = false;
         if (handleScrollCustomization) {
             OwnPtr<ScrollStateData> scrollStateData = adoptPtr(new ScrollStateData());
             scrollStateData->delta_x = delta.width();
             scrollStateData->delta_y = delta.height();
             scrollStateData->velocity_x = velocity.width();
             scrollStateData->velocity_y = velocity.height();
             scrollStateData->should_propagate = !gestureEvent.preventPropagation();
             scrollStateData->is_in_inertial_phase = gestureEvent.inertial();
             scrollStateData->from_user_input = true;
             scrollStateData->delta_consumed_for_scroll_sequence = m_deltaConsumedForScrollSequence;
             RefPtrWillBeRawPtr<ScrollState> scrollState = ScrollState::create(scrollStateData.release());
             if (m_previousGestureScrolledNode) {
                 // The ScrollState needs to know what the current
                 // native scrolling element is, so that for an
                 // inertial scroll that shouldn't propagate, only the
                 // currently scrolling element responds.
                 ASSERT(m_previousGestureScrolledNode->isElementNode());
                 scrollState->setCurrentNativeScrollingElement(toElement(m_previousGestureScrolledNode.get()));
             }
             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 (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);
+            }
+
+            if (consumed)
+                return WebInputEventResult::HandledSystem;
         }
     }
 
-    if (handleScrollCustomization)
-        return WebInputEventResult::NotHandled;
-
-    // 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;
-    }
-
     return WebInputEventResult::NotHandled;
 }
 
 void EventHandler::clearGestureScrollState()
 {
     m_scrollGestureHandlingNode = nullptr;
     m_previousGestureScrolledNode = nullptr;
     m_deltaConsumedForScrollSequence = false;
     m_currentScrollChain.clear();
     m_accumulatedRootOverscroll = FloatSize();
@@ skipping 939 matching lines @@
     if (event->ctrlKey() || event->metaKey() || event->altKey())
         return;
 
     ScrollDirection direction = event->shiftKey() ? ScrollBlockDirectionBackward : ScrollBlockDirectionForward;
 
     // FIXME: enable scroll customization in this case. See crbug.com/410974.
     if (logicalScroll(direction, ScrollByPage)) {
         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)
 {
     ASSERT(event->type() == EventTypeNames::keydown);
 
     if (event->ctrlKey() || event->metaKey() || event->altKey())
         return;
 
     if (!m_frame->editor().behavior().shouldNavigateBackOnBackspace())
@@ skipping 537 matching lines @@
 PlatformEvent::Modifiers EventHandler::accessKeyModifiers()
 {
 #if OS(MACOSX)
     return static_cast<PlatformEvent::Modifiers>(PlatformEvent::CtrlKey | PlatformEvent::AltKey);
 #else
     return PlatformEvent::AltKey;
 #endif
 }
 
 } // namespace blink