Fix scroll chaining for non-descendants of root scroller.

This CL fixes the scroll chaining behavior for elements that aren't descendants
of the root scroller element. This can only happen in the non-document root
scroller proposal.

On the main thread scrolling path, we were previously assuming that all scrolls
would chain through the effective root scroller. This CL fixes the terminating
condition as well as removes an invalid DCHECK.

On the compositor side, the chaining mechanisms would previously use the inner
viewport scroll layer to designate viewport scrolling. Further on, we would
explicitly check for the inner viewport layer and scroll using cc::Viewport.
When scrolling a non-descendant of the root scroller, we chain up to the inner
viewport scroll layer without going through the outer viewport. Scrolling
cc::Viewport causes scrolling in the outer viewport though so we would scroll
it inadvertaintly.  I've made changes so that we use the outer viewport to
designate cc::Viewport scrolling. Scrolls will terminate at the outer viewport
scroll layer, but they don't have to chain through it.

Unrelated, but I also removed LayerImpl::ApplyScroll as it's unused.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2365793002
Cr-Commit-Position: refs/heads/master@{#420763}

cc/layers/layer_impl.cc

@@ -125,14 +125,7 @@ void LayerImpl::SetDebugInfo(
 void LayerImpl::DistributeScroll(ScrollState* scroll_state) {
   ScrollTree& scroll_tree = layer_tree_impl()->property_trees()->scroll_tree;
   ScrollNode* scroll_node = scroll_tree.Node(scroll_tree_index());
-  return scroll_tree.DistributeScroll(scroll_node, scroll_state);
-}
-
-void LayerImpl::ApplyScroll(ScrollState* scroll_state) {
-  DCHECK(scroll_state);
-  ScrollNode* node = layer_tree_impl()->property_trees()->scroll_tree.Node(
-      scroll_tree_index());
-  layer_tree_impl()->ApplyScroll(node, scroll_state);
+  scroll_tree.DistributeScroll(scroll_node, scroll_state);
 }
 
 void LayerImpl::SetTransformTreeIndex(int index) {

cc/layers/layer_impl.h

@@ -102,7 +102,6 @@ class CC_EXPORT LayerImpl {
   bool IsActive() const;
 
   void DistributeScroll(ScrollState* scroll_state);
-  void ApplyScroll(ScrollState* scroll_state);
 
   void set_property_tree_sequence_number(int sequence_number) {}
 

cc/trees/layer_tree_host_impl.cc

@@ -2574,15 +2574,14 @@ LayerImpl* LayerTreeHostImpl::FindScrollLayerForDeviceViewportPoint(
     }
   }
 
+  // The outer viewport layer represents the viewport.
+  if (potentially_scrolling_layer_impl == InnerViewportScrollLayer())
+    potentially_scrolling_layer_impl = OuterViewportScrollLayer();
+
   // Falling back to the root scroll layer ensures generation of root overscroll
-  // notifications. The inner viewport layer represents the viewport during
-  // scrolling.
+  // notifications.
   if (!potentially_scrolling_layer_impl)
-    potentially_scrolling_layer_impl = InnerViewportScrollLayer();
-
-  // The inner viewport layer represents the viewport.
-  if (potentially_scrolling_layer_impl == OuterViewportScrollLayer())
-    potentially_scrolling_layer_impl = InnerViewportScrollLayer();
+    potentially_scrolling_layer_impl = OuterViewportScrollLayer();
 
   if (potentially_scrolling_layer_impl) {
     // Ensure that final layer scrolls on impl thread (crbug.com/625100)
@@ -2855,19 +2854,18 @@ InputHandler::ScrollStatus LayerTreeHostImpl::ScrollAnimated(
     if (scroll_node) {
       for (; scroll_tree.parent(scroll_node);
            scroll_node = scroll_tree.parent(scroll_node)) {
-        if (!scroll_node->scrollable ||
-            scroll_node->is_outer_viewport_scroll_layer)
+        if (!scroll_node->scrollable)
           continue;
 
-        if (scroll_node->is_inner_viewport_scroll_layer) {
+        if (scroll_node->is_outer_viewport_scroll_layer ||
+            scroll_node->is_inner_viewport_scroll_layer) {
           gfx::Vector2dF scrolled =
               viewport()->ScrollAnimated(pending_delta, delayed_by);
           // Viewport::ScrollAnimated returns pending_delta as long as it
           // starts an animation.
           if (scrolled == pending_delta)
             return scroll_status;
-          pending_delta -= scrolled;
-          continue;
+          break;
         }
 
         gfx::Vector2dF scroll_delta =
@@ -3010,7 +3008,7 @@ void LayerTreeHostImpl::ApplyScroll(ScrollNode* scroll_node,
   // details.
   const float kEpsilon = 0.1f;
 
-  if (scroll_node->is_inner_viewport_scroll_layer) {
+  if (scroll_node->is_outer_viewport_scroll_layer) {
     bool affect_top_controls = !wheel_scrolling_;
     Viewport::ScrollResult result = viewport()->ScrollBy(
         delta, viewport_point, scroll_state->is_direct_manipulation(),
@@ -3031,7 +3029,7 @@ void LayerTreeHostImpl::ApplyScroll(ScrollNode* scroll_node,
   bool scrolled = std::abs(applied_delta.x()) > kEpsilon;
   scrolled = scrolled || std::abs(applied_delta.y()) > kEpsilon;
 
-  if (scrolled && !scroll_node->is_inner_viewport_scroll_layer) {
+  if (scrolled && !scroll_node->is_outer_viewport_scroll_layer) {
     // If the applied delta is within 45 degrees of the input
     // delta, bail out to make it easier to scroll just one layer
     // in one direction without affecting any of its parents.
@@ -3068,11 +3066,9 @@ void LayerTreeHostImpl::DistributeScrollDelta(ScrollState* scroll_state) {
          scroll_node = scroll_tree.parent(scroll_node)) {
       if (scroll_node->is_outer_viewport_scroll_layer) {
         // Don't chain scrolls past the outer viewport scroll layer. Once we
-        // reach that, we should scroll the viewport, which is represented by
-        // the inner viewport scroll layer.
-        ScrollNode* inner_viewport_scroll_node =
-            scroll_tree.Node(InnerViewportScrollLayer()->scroll_tree_index());
-        current_scroll_chain.push_front(inner_viewport_scroll_node);
+        // reach that, we're viewport scrolling which is special and handled by
+        // cc's Viewport class.
+        current_scroll_chain.push_front(scroll_node);
         break;
       }
 
@@ -3246,8 +3242,6 @@ void LayerTreeHostImpl::MouseMoveAt(const gfx::Point& viewport_point) {
   LayerImpl* scroll_layer_impl = FindScrollLayerForDeviceViewportPoint(
       device_viewport_point, InputHandler::TOUCHSCREEN, layer_impl,
       &scroll_on_main_thread, &main_thread_scrolling_reasons);
-  if (scroll_layer_impl == InnerViewportScrollLayer())
-    scroll_layer_impl = OuterViewportScrollLayer();
   if (scroll_on_main_thread || !scroll_layer_impl)
     return;
 
@@ -3295,7 +3289,7 @@ void LayerTreeHostImpl::PinchGestureBegin() {
   client_->RenewTreePriority();
   pinch_gesture_end_should_clear_scrolling_layer_ = !CurrentlyScrollingLayer();
   active_tree_->SetCurrentlyScrollingLayer(
-      active_tree_->InnerViewportScrollLayer());
+      active_tree_->OuterViewportScrollLayer());
   top_controls_manager_->PinchBegin();
 }
 

third_party/WebKit/LayoutTests/fast/scrolling/scroll-non-descendant-of-root-scroller.html

+<!DOCTYPE html>
+<style>
+  ::-webkit-scrollbar {
+    width: 0px;
+    height: 0px;
+  }
+
+  body, html {
+    width: 100%;
+    height: 100%;
+    margin: 0px;
+  }
+
+  #rootscroller {
+    width: 100%;
+    height: 100%;
+    overflow: auto;
+  }
+
+  #dialog {
+      position: fixed;
+      left: 50px;
+      right: 50px;
+      top: 50px;
+      bottom: 50px;
+      overflow: auto;
+  }
+
+  .bigspace {
+      width: 2000px;
+      height: 2000px;
+      background-image: repeating-linear-gradient(
+          45deg,
+          yellow,
+          yellow 80px,
+          orange 80px,
+          orange 160px);
+  }
+
+  .bigspace2 {
+      width: 2000px;
+      height: 2000px;
+      background-image: repeating-linear-gradient(
+          -45deg,
+          blue,
+          blue 80px,
+          red 80px,
+          red 160px);
+  }
+
+</style>
+
+<div id="rootscroller">
+  To test manually, try scrolling the red/orange box past the end. Scrolling
+  shouldn't chain up to this blue/red box.
+  <div class="bigspace2"></div>
+</div>
+<div id="dialog">
+  <div class="bigspace"></div>
+</div>
+
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+
+<script>
+  var rootScroller = document.querySelector('#rootscroller');
+  var dialog = document.querySelector('#dialog');
+
+  document.rootScroller = rootScroller;
+
+  var dialogRect = dialog.getBoundingClientRect();
+
+  var x = dialogRect.left + dialogRect.width / 2;
+  var y = dialogRect.top + dialogRect.height / 2;
+
+  test(function() {
+      if (!window.eventSender)
+          return;
+
+          // Sanity check - there should be no initial scroll.
+          assert_equals(dialog.scrollLeft, 0);
+          assert_equals(dialog.scrollTop, 0);
+          assert_equals(rootscroller.scrollLeft, 0);
+          assert_equals(rootscroller.scrollTop, 0);
+
+          // This scroll should fully scroll the dialog
+          eventSender.gestureScrollBegin(x, y);
+          eventSender.gestureScrollUpdate(-3000, 0);
+          eventSender.gestureScrollUpdate(0, -3000);
+          eventSender.gestureScrollEnd(0, 0);
+
+          assert_equals(dialog.scrollLeft, dialog.scrollWidth - dialog.clientWidth);
+          assert_equals(dialog.scrollTop, dialog.scrollHeight - dialog.clientHeight);
+          assert_equals(rootscroller.scrollLeft, 0);
+          assert_equals(rootscroller.scrollTop, 0);
+
+          // This scroll would normally chain up to the viewport but the "real"
+          // viewport has no scrolling. Make sure we don't scroll the
+          // document.rootScroller.
+          eventSender.gestureScrollBegin(x, y);
+          eventSender.gestureScrollUpdate(-3000, 0);
+          eventSender.gestureScrollUpdate(0, -3000);
+          eventSender.gestureScrollEnd(0, 0);
+
+          assert_equals(dialog.scrollLeft, dialog.scrollWidth - dialog.clientWidth);
+          assert_equals(dialog.scrollTop, dialog.scrollHeight - dialog.clientHeight);
+          assert_equals(rootscroller.scrollLeft, 0);
+          assert_equals(rootscroller.scrollTop, 0);
+
+  }, 'Scrolls on the dialog should not chain to the sibling root scroller.');
+</script>

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

@@ -83,6 +83,7 @@ void ScrollManager::recomputeScrollChain(const Node& startNode,
 
     DCHECK(startNode.layoutObject());
     LayoutBox* curBox = startNode.layoutObject()->enclosingBox();
+    Element* documentElement = m_frame->document()->documentElement();
 
     // Scrolling propagates along the containing block chain and ends at the
     // RootScroller element. The RootScroller element will have a custom
@@ -98,13 +99,13 @@ void ScrollManager::recomputeScrollChain(const Node& startNode,
             // In normal circumastances, the documentElement will be the root
             // scroller but the documentElement itself isn't a containing block,
             // that'll be the document node rather than the element.
-            curElement = m_frame->document()->documentElement();
-            DCHECK(!curElement || isEffectiveRootScroller(*curElement));
+            curElement = documentElement;
         }
 
         if (curElement) {
             scrollChain.push_front(DOMNodeIds::idForNode(curElement));
-            if (isEffectiveRootScroller(*curElement))
+            if (isEffectiveRootScroller(*curElement)
+                || curElement->isSameNode(documentElement))
                 break;
         }