Fix scroll chaining for non-descendants of root scroller.

cc/trees/layer_tree_host_impl_unittest.cc

 // Copyright 2011 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 "cc/trees/layer_tree_host_impl.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <cmath>
@@ skipping 4137 matching lines @@
   EXPECT_EQ(0.f, outer_scroll->CurrentScrollOffset().y());
   EXPECT_EQ(gfx::ScrollOffset(), outer_scroll->MaxScrollOffset());
 
   host_impl_->ScrollEnd(EndState().get());
 
   EXPECT_EQ(InputHandler::SCROLL_ON_IMPL_THREAD,
             host_impl_
                 ->ScrollBegin(BeginState(gfx::Point()).get(),
                               InputHandler::TOUCHSCREEN)
                 .thread);
-  EXPECT_EQ(host_impl_->CurrentlyScrollingLayer(), inner_scroll);
+  EXPECT_EQ(host_impl_->CurrentlyScrollingLayer(), outer_scroll);
 
   host_impl_->ScrollBy(
       UpdateState(gfx::Point(), gfx::Vector2dF(0.f, -50.f)).get());
 
   // The entire scroll delta should have been used to show the top controls.
   // The outer viewport should be resized to accomodate and scrolled to the
   // bottom of the document to keep the viewport in place.
   EXPECT_EQ(1.f, host_impl_->active_tree()->CurrentTopControlsShownRatio());
   EXPECT_EQ(50.f, outer_container->BoundsForScrolling().height());
   EXPECT_EQ(50.f, inner_container->BoundsForScrolling().height());
@@ skipping 435 matching lines @@
   // Send a gesture scroll that will scroll the outer viewport, make sure the
   // top controls get scrolled.
   gfx::Vector2dF scroll_delta(0.f, 15.f);
   EXPECT_EQ(InputHandler::SCROLL_ON_IMPL_THREAD,
             host_impl_
                 ->ScrollBegin(BeginState(gfx::Point()).get(),
                               InputHandler::TOUCHSCREEN)
                 .thread);
   host_impl_->ScrollBy(UpdateState(gfx::Point(), scroll_delta).get());
 
-  EXPECT_EQ(host_impl_->InnerViewportScrollLayer(),
+  EXPECT_EQ(host_impl_->OuterViewportScrollLayer(),
             host_impl_->CurrentlyScrollingLayer());
   host_impl_->ScrollEnd(EndState().get());
 
   EXPECT_FLOAT_EQ(scroll_delta.y(),
                   top_controls_height_ -
                       host_impl_->top_controls_manager()->ContentTopOffset());
 
   scroll_delta = gfx::Vector2dF(0.f, 50.f);
   EXPECT_EQ(InputHandler::SCROLL_ON_IMPL_THREAD,
             host_impl_
                 ->ScrollBegin(BeginState(gfx::Point()).get(),
                               InputHandler::TOUCHSCREEN)
                 .thread);
   host_impl_->ScrollBy(UpdateState(gfx::Point(), scroll_delta).get());
 
   EXPECT_EQ(0, host_impl_->top_controls_manager()->ContentTopOffset());
-  EXPECT_EQ(host_impl_->InnerViewportScrollLayer(),
+  EXPECT_EQ(host_impl_->OuterViewportScrollLayer(),
             host_impl_->CurrentlyScrollingLayer());
 
   host_impl_->ScrollEnd(EndState().get());
 
   // Position the viewports such that the inner viewport will be scrolled.
   gfx::Vector2dF inner_viewport_offset(0.f, 25.f);
   SetScrollOffsetDelta(host_impl_->OuterViewportScrollLayer(),
                        gfx::Vector2dF());
   SetScrollOffsetDelta(host_impl_->InnerViewportScrollLayer(),
                        inner_viewport_offset);
@@ skipping 1827 matching lines @@
     host_impl_->ScrollBy(
         UpdateState(gfx::Point(0, 0), gfx::Vector2dF(100.f, 100.f)).get());
     host_impl_->ScrollEnd(EndState().get());
 
     EXPECT_VECTOR_EQ(gfx::Vector2dF(0.f, 0.f),
                      outer_scroll_layer->CurrentScrollOffset());
     EXPECT_VECTOR_EQ(gfx::Vector2dF(50.f, 50.f),
                      scroll_layer->CurrentScrollOffset());
   }
 }
+// Test that scrolls chain correctly when a child scroller on the page (e.g. a
+// scrolling div) is set as the outer viewport but scrolls start from a layer
+// that's not a descendant of the outer viewport. This happens in the
+// rootScroller proposal.
+TEST_F(LayerTreeHostImplTest, RootScrollerScrollNonDescendant) {
+  const gfx::Size content_size(300, 300);
+  const gfx::Size viewport_size(300, 300);
+
+  LayerTreeImpl* layer_tree_impl = host_impl_->active_tree();
+
+  LayerImpl* content_layer =
+      CreateBasicVirtualViewportLayers(viewport_size, content_size);
+  LayerImpl* inner_scroll_layer = host_impl_->InnerViewportScrollLayer();
+
+  LayerImpl* outer_scroll_layer = nullptr;
+  LayerImpl* sibling_scroll_layer = nullptr;
+
+  // Initialization: Add a scrolling layer, simulating an ordinary DIV, to be
+  // set as the outer viewport. Add a sibling scrolling layer that isn't a child
+  // of the outer viewport scroll layer.
+  {
+    std::unique_ptr<LayerImpl> clip = LayerImpl::Create(layer_tree_impl, 10);
+    clip->SetBounds(content_size);
+    clip->SetPosition(gfx::PointF(100, 100));
+
+    std::unique_ptr<LayerImpl> scroll = LayerImpl::Create(layer_tree_impl, 11);
+    scroll->SetBounds(gfx::Size(1200, 1200));
+    scroll->SetScrollClipLayer(clip->id());
+    scroll->SetDrawsContent(true);
+
+    outer_scroll_layer = scroll.get();

[tdresser, 2016/09/23 15:59:58]

Having a raw pointer point to something owned by a unique_ptr is a bit scary.
I'm not sure why this doesn't crash.

[bokan, 2016/09/23 19:57:01]

It's because we std::move them into the layer tree below, the LayerTreeImpl
maintains a unique_ptr to each layer so I think using a raw ptr here should be
ok since the LayerTreeImpl is long lived. 

[tdresser, 2016/09/23 20:40:30]

D'oh, sure, that's fine.

+
+    clip->test_properties()->AddChild(std::move(scroll));
+    content_layer->test_properties()->AddChild(std::move(clip));
+
+    // Create the non-descendant.
+    std::unique_ptr<LayerImpl> clip2 = LayerImpl::Create(layer_tree_impl, 14);
+    clip2->SetBounds(gfx::Size(600, 600));
+    clip2->SetPosition(gfx::PointF());
+
+    std::unique_ptr<LayerImpl> scroll2 = LayerImpl::Create(layer_tree_impl, 15);
+    scroll2->SetBounds(gfx::Size(1200, 1200));
+    scroll2->SetScrollClipLayer(clip2->id());
+    scroll2->SetDrawsContent(true);
+
+    sibling_scroll_layer = scroll2.get();
+
+    clip2->test_properties()->AddChild(std::move(scroll2));
+    content_layer->test_properties()->AddChild(std::move(clip2));
+
+    layer_tree_impl->SetViewportLayersFromIds(
+        Layer::INVALID_ID, layer_tree_impl->PageScaleLayer()->id(),
+        inner_scroll_layer->id(), outer_scroll_layer->id());
+    layer_tree_impl->BuildPropertyTreesForTesting();
+
+    ASSERT_EQ(outer_scroll_layer, layer_tree_impl->OuterViewportScrollLayer());
+  }
+
+  // Scrolls should target the non-descendant scroller. Chaining should not
+  // propagate to the outer viewport scroll layer.
+  {
+    // This should fully scroll the layer.
+    host_impl_->ScrollBegin(BeginState(gfx::Point(0, 0)).get(),
+                            InputHandler::TOUCHSCREEN);
+    host_impl_->ScrollBy(
+        UpdateState(gfx::Point(0, 0), gfx::Vector2dF(1000.f, 1000.f)).get());
+    host_impl_->ScrollEnd(EndState().get());
+
+    EXPECT_VECTOR_EQ(gfx::Vector2dF(600.f, 600.f),
+                     sibling_scroll_layer->CurrentScrollOffset());
+    EXPECT_VECTOR_EQ(gfx::Vector2dF(),
+                     outer_scroll_layer->CurrentScrollOffset());

[tdresser, 2016/09/23 15:59:58]

Shouldn't outer_scroll_layer be deleted by the time you get here?

[bokan, 2016/09/23 19:57:01]

The unique_ptr's are std::moved into the layer tree so it's kept around.

+
+    // Scrolling now should chain up but, since the outer viewport is a sibling
+    // rather than an ancestor, we shouldn't chain to it.
+    host_impl_->ScrollBegin(BeginState(gfx::Point(0, 0)).get(),
+                            InputHandler::TOUCHSCREEN);
+    host_impl_->ScrollBy(
+        UpdateState(gfx::Point(0, 0), gfx::Vector2dF(1000.f, 1000.f)).get());
+    host_impl_->ScrollEnd(EndState().get());
+
+    EXPECT_VECTOR_EQ(gfx::Vector2dF(600.f, 600.f),
+                     sibling_scroll_layer->CurrentScrollOffset());
+    EXPECT_VECTOR_EQ(gfx::Vector2dF(),
+                     outer_scroll_layer->CurrentScrollOffset());
+  }
+
+  float min_page_scale = 1.f, max_page_scale = 4.f;
+  float page_scale_factor = 1.f;
+  host_impl_->active_tree()->PushPageScaleFromMainThread(
+      page_scale_factor, min_page_scale, max_page_scale);
+
+  gfx::Vector2dF viewport_size_vec(viewport_size.width(),
+                                   viewport_size.height());
+
+  // Reset the scroll offset.
+  sibling_scroll_layer->SetCurrentScrollOffset(gfx::ScrollOffset());
+
+  // Now pinch-zoom in. Anchoring should cause scrolling only on the inner
+  // viewport layer.
+  {
+    // Pinch in to the middle of the screen. The inner viewport should scroll
+    // to keep the gesture anchored but not the outer or the sibling scroller.
+    page_scale_factor = 2.f;
+    gfx::Point anchor(viewport_size.width() / 2, viewport_size.height() / 2);
+    host_impl_->ScrollBegin(BeginState(anchor).get(),
+                            InputHandler::TOUCHSCREEN);
+    host_impl_->PinchGestureBegin();
+    host_impl_->PinchGestureUpdate(page_scale_factor, anchor);
+    host_impl_->PinchGestureEnd();
+
+    EXPECT_VECTOR_EQ(gfx::Vector2dF(anchor.x() / 2, anchor.y() / 2),
+                     inner_scroll_layer->CurrentScrollOffset());
+
+    host_impl_->ScrollBy(UpdateState(anchor, viewport_size_vec).get());
+
+    EXPECT_VECTOR_EQ(ScaleVector2d(viewport_size_vec, 1.f / page_scale_factor),
+                     inner_scroll_layer->CurrentScrollOffset());
+    // TODO(bokan): This doesn't yet work but we'll probably want to fix this
+    // at some point.
+    // EXPECT_VECTOR_EQ(
+    //     gfx::Vector2dF(),
+    //     outer_scroll_layer->CurrentScrollOffset());
+    EXPECT_VECTOR_EQ(gfx::Vector2dF(),
+                     sibling_scroll_layer->CurrentScrollOffset());
+
+    host_impl_->ScrollEnd(EndState().get());
+  }
+
+  // Reset the scroll offsets
+  sibling_scroll_layer->SetCurrentScrollOffset(gfx::ScrollOffset());
+  inner_scroll_layer->SetCurrentScrollOffset(gfx::ScrollOffset());
+  outer_scroll_layer->SetCurrentScrollOffset(gfx::ScrollOffset());
+
+  // Scrolls over the sibling while pinched in should scroll the sibling first,
+  // but then chain up to the inner viewport so that the user can still pan
+  // around. The outer viewport should be unaffected.
+  {
+    // This should fully scroll the sibling but, because we latch to the
+    // scroller, it shouldn't chain up to the inner viewport yet.
+    host_impl_->ScrollBegin(BeginState(gfx::Point(0, 0)).get(),
+                            InputHandler::TOUCHSCREEN);
+    host_impl_->ScrollBy(
+        UpdateState(gfx::Point(0, 0), gfx::Vector2dF(2000.f, 2000.f)).get());
+    host_impl_->ScrollEnd(EndState().get());
+
+    EXPECT_VECTOR_EQ(gfx::Vector2dF(600.f, 600.f),
+                     sibling_scroll_layer->CurrentScrollOffset());
+    EXPECT_VECTOR_EQ(gfx::Vector2dF(),
+                     inner_scroll_layer->CurrentScrollOffset());
+
+    // Scrolling now should chain up to the inner viewport.
+    host_impl_->ScrollBegin(BeginState(gfx::Point(0, 0)).get(),
+                            InputHandler::TOUCHSCREEN);
+    host_impl_->ScrollBy(
+        UpdateState(gfx::Point(0, 0), gfx::Vector2dF(2000.f, 2000.f)).get());
+    host_impl_->ScrollEnd(EndState().get());
+
+    EXPECT_VECTOR_EQ(ScaleVector2d(viewport_size_vec, 1 / page_scale_factor),
+                     inner_scroll_layer->CurrentScrollOffset());
+    EXPECT_VECTOR_EQ(gfx::Vector2dF(),
+                     outer_scroll_layer->CurrentScrollOffset());
+
+    // No more scrolling should be possible.
+    host_impl_->ScrollBegin(BeginState(gfx::Point(0, 0)).get(),
+                            InputHandler::TOUCHSCREEN);
+    host_impl_->ScrollBy(
+        UpdateState(gfx::Point(0, 0), gfx::Vector2dF(2000.f, 2000.f)).get());
+    host_impl_->ScrollEnd(EndState().get());
+
+    EXPECT_VECTOR_EQ(gfx::Vector2dF(),
+                     outer_scroll_layer->CurrentScrollOffset());
+  }
+}
 
 TEST_F(LayerTreeHostImplTest, OverscrollOnImplThread) {
   InputHandlerScrollResult scroll_result;
   LayerTreeSettings settings = DefaultSettings();
   CreateHostImpl(settings, CreateCompositorFrameSink());
 
   const gfx::Size content_size(50, 50);
   const gfx::Size viewport_size(50, 50);
   CreateBasicVirtualViewportLayers(viewport_size, content_size);
 
@@ skipping 1881 matching lines @@
   scroll_layer->test_properties()->AddChild(std::move(child_scroll));
   host_impl_->active_tree()->BuildPropertyTreesForTesting();
 
   DrawFrame();
 
   // We should not have scrolled |child_scroll| even though we technically "hit"
   // it. The reason for this is that if the scrolling the scroll would not move
   // any layer that is a drawn RSLL member, then we can ignore the hit.
   //
   // Why SCROLL_STARTED? In this case, it's because we've bubbled out and
-  // started scrolling the inner viewport.
+  // started scrolling the viewport.
   EXPECT_EQ(InputHandler::SCROLL_ON_IMPL_THREAD,
             host_impl_->ScrollBegin(BeginState(gfx::Point()).get(),
                                     InputHandler::WHEEL)
                 .thread);
 
-  EXPECT_EQ(2, host_impl_->CurrentlyScrollingLayer()->id());
+  EXPECT_EQ(117, host_impl_->CurrentlyScrollingLayer()->id());
 }
 
 TEST_F(LayerTreeHostImplTest, ScrollInvisibleScrollerWithVisibleDescendent) {
   gfx::Size content_size(100, 100);
   SetupScrollAndContentsLayers(content_size);
 
   LayerImpl* root = host_impl_->active_tree()->LayerById(1);
   LayerImpl* root_scroll_layer = host_impl_->active_tree()->LayerById(2);
 
   std::unique_ptr<LayerImpl> invisible_scroll_layer =
@@ skipping 959 matching lines @@
     EXPECT_VECTOR_EQ(inner_expected, inner_scroll->CurrentScrollOffset());
     EXPECT_VECTOR_EQ(outer_expected, outer_scroll->CurrentScrollOffset());
 
     // Scrolling the viewport always sets the outer scroll layer as the
     // currently scrolling layer.
     EXPECT_EQ(InputHandler::SCROLL_ON_IMPL_THREAD,
               host_impl_
                   ->ScrollBegin(BeginState(gfx::Point()).get(),
                                 InputHandler::TOUCHSCREEN)
                   .thread);
-    EXPECT_EQ(inner_scroll, host_impl_->CurrentlyScrollingLayer());
+    EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
     EXPECT_EQ(InputHandler::SCROLL_ON_IMPL_THREAD,
               host_impl_->FlingScrollBegin().thread);
-    EXPECT_EQ(inner_scroll, host_impl_->CurrentlyScrollingLayer());
+    EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
 
     gfx::Vector2d scroll_delta(inner_viewport.width() / 2.f,
                                inner_viewport.height() / 2.f);
     host_impl_->ScrollBy(UpdateState(gfx::Point(), scroll_delta).get());
     inner_expected += gfx::Vector2dF(scroll_delta.x(), scroll_delta.y());
-    EXPECT_EQ(inner_scroll, host_impl_->CurrentlyScrollingLayer());
+    EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
 
     host_impl_->ScrollEnd(EndState().get());
     host_impl_->ClearCurrentlyScrollingLayerForTesting();
     EXPECT_EQ(nullptr, host_impl_->CurrentlyScrollingLayer());
 
     EXPECT_VECTOR_EQ(inner_expected, inner_scroll->CurrentScrollOffset());
     EXPECT_VECTOR_EQ(outer_expected, outer_scroll->CurrentScrollOffset());
 
     // Fling past the inner viewport boundry, make sure outer viewport scrolls.
     EXPECT_EQ(InputHandler::SCROLL_ON_IMPL_THREAD,
               host_impl_
                   ->ScrollBegin(BeginState(gfx::Point()).get(),
                                 InputHandler::TOUCHSCREEN)
                   .thread);
-    EXPECT_EQ(inner_scroll, host_impl_->CurrentlyScrollingLayer());
+    EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
     EXPECT_EQ(InputHandler::SCROLL_ON_IMPL_THREAD,
               host_impl_->FlingScrollBegin().thread);
-    EXPECT_EQ(inner_scroll, host_impl_->CurrentlyScrollingLayer());
+    EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
 
     host_impl_->ScrollBy(UpdateState(gfx::Point(), scroll_delta).get());
     inner_expected += gfx::Vector2dF(scroll_delta.x(), scroll_delta.y());
-    EXPECT_EQ(inner_scroll, host_impl_->CurrentlyScrollingLayer());
+    EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
 
     host_impl_->ScrollBy(UpdateState(gfx::Point(), scroll_delta).get());
     outer_expected += gfx::Vector2dF(scroll_delta.x(), scroll_delta.y());
-    EXPECT_EQ(inner_scroll, host_impl_->CurrentlyScrollingLayer());
+    EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
 
     host_impl_->ScrollEnd(EndState().get());
     host_impl_->ClearCurrentlyScrollingLayerForTesting();
     EXPECT_EQ(nullptr, host_impl_->CurrentlyScrollingLayer());
 
     EXPECT_VECTOR_EQ(inner_expected, inner_scroll->CurrentScrollOffset());
     EXPECT_VECTOR_EQ(outer_expected, outer_scroll->CurrentScrollOffset());
   }
 }
 
@@ skipping 1747 matching lines @@
                                    ->test_properties()
                                    ->children[0];
     // The scroll done on the active tree is undone on the pending tree.
     gfx::Transform translate;
     translate.Translate(0, accumulated_scroll);
     content_layer->test_properties()->transform = translate;
 
     LayerTreeImpl* pending_tree = host_impl_->pending_tree();
     pending_tree->PushPageScaleFromMainThread(1.f, 1.f, 1.f);
     LayerImpl* last_scrolled_layer = pending_tree->LayerById(
-        host_impl_->active_tree()->LastScrolledLayerId());
+        host_impl_->active_tree()->InnerViewportScrollLayer()->id());

[bokan, 2016/09/23 01:53:55]

The LastScrolledLayerId is set using SetCurrentlyScrollingLayer which isn't
completely precise w.r.t the viewport. For viewport scrolling, we always set the
currently scrolling layer as the one layer that represents the viewport.
Previously this was inner but this patch switched it to outer.

In this test, its the inner viewport that gets scrolled and so the last scrolled
was previously correct but breaks now since the below "hacking" is applied to
the wrong layer.

I'm not sure if this is a problem in practice, however, it's already broken
today for the opposite case. So if the user scrolls the outer viewport, the last
scrolled layer id will be set to the inner. +jaydasika@ as FYI. 

[tdresser, 2016/09/23 15:59:58]

This seems a bit scary – it should be a pretty simple fix, shouldn't it?

[bokan, 2016/09/23 19:57:01]

It's not as straightforward as it seems. This currently gets set during
SetCurrentlyScrollingLayer but, to correctly scroll the viewport, currently
scrolling layer needs to be set to the "representative" viewport layer rather
than the actually scrolled layer. So we'd have to split this out from the
currently scrolling layer. I suspect that it might be best to set this from the
ScrollTree itself whenever a layer's scroll offset changes but I'm not up to
speed on all the details there. This is only ever used to calculate the "jitter"
metric so I'm not sure what the impact is.

I could make cc::Viewport keep track of its last_scrolled_layer and then query
that from SetCurrentlyScrollingLayer but that seems like extra machinery and
it's not clear what that should do when we scroll both layers in one scroll
update. jaydasika@ added this so he might have some insight in the right way
forward here.

 
     // When building property trees from impl side, the builder uses the scroll
     // offset of layer_impl to initialize the scroll offset in scroll tree:
     //  scroll_tree.synced_scroll_offset.PushFromMainThread(
     //                                   layer->CurrentScrollOffset()).
     // However, layer_impl does not store scroll_offset, so it is using scroll
     // tree's scroll offset to initialize itself. Usually this approach works
     // because this is a simple assignment. However if scroll_offset's pending
     // delta is not zero, the delta would be counted twice.
     // This hacking here is to restore the damaged scroll offset.
@@ skipping 43 matching lines @@
   EXPECT_TRUE(host_impl_->use_gpu_rasterization());
 
   // Re-initialize with a software output surface.
   compositor_frame_sink_ = FakeCompositorFrameSink::CreateSoftware();
   host_impl_->InitializeRenderer(compositor_frame_sink_.get());
   EXPECT_FALSE(host_impl_->use_gpu_rasterization());
 }
 
 }  // namespace
 }  // namespace cc