Fix viewport scrolling with overflow:hidden

This CL fixes an issue introduced in https://crrev.com/c/1999288.
Scrollers can set |user_scrollable_horizontal| (or vertical) to false
to indicate that the user shouldn't be able to scroll them  This
corresponds to |overflow: hidden| in CSS (which can still be
programmatically scrolled). In several places, compositor code checks
these bits and, if false, clears the requested scrolling delta in the
disabled axis.

The bug in the CL above was that, now that the scrolling paths were
consolidated,  we moved this "user_scrollable" adjustment from the
update animation curve to happen as part of the common path in
ScrollUpdate so that it would happen for all kinds of scroll modalities
in one place.

However, this adjustment is too simplistic for viewport scrolling.
Scrolling the outer viewport must use the cc::Viewport class which will
distribute the scroll between both inner and outer scroll nodes. The
outer viewport may have user_scrollable=false, corresponding to the
documentElement having |overflow: hidden| but we should still be able
to scroll the inner viewport if the user zooms in. The above adjustment
prevents that since it clears the delta. (I suspect this issue existed
prior to this in the "update existing animation" case but is hard to hit
since pinch-zoom and animated wheel scrolling aren't a common
combination).

The solution here is to move these adjustments to the point of node
scrolling. This means that we'll adjust the delta right before applying
a scroll to an individual ScrollNode. This ensures the entire delta is
sent to the viewport which has special cases for dealing with the double
nodes.

Bug: 1055790
Change-Id: I2e9849b15c512003352299fc82e58817a98ef778
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2072407
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745229}

cc/layers/viewport.cc

@@ -52,13 +52,13 @@ Viewport::ScrollResult Viewport::ScrollBy(const gfx::Vector2dF& physical_delta,
 
   // Attempt to scroll inner viewport first.
   pending_scroll_node_delta -= host_impl_->ScrollSingleNode(
-      InnerScrollNode(), pending_scroll_node_delta, viewport_point,
+      *InnerScrollNode(), pending_scroll_node_delta, viewport_point,
       is_direct_manipulation, &scroll_tree());
 
   // Now attempt to scroll the outer viewport.
   if (scroll_outer_viewport) {
     pending_scroll_node_delta -= host_impl_->ScrollSingleNode(
-        OuterScrollNode(), pending_scroll_node_delta, viewport_point,
+        *OuterScrollNode(), pending_scroll_node_delta, viewport_point,
         is_direct_manipulation, &scroll_tree());
   }
 

cc/trees/layer_tree_host_impl.h

@@ -736,7 +736,8 @@ class CC_EXPORT LayerTreeHostImpl : public InputHandler,
 
   bool prepare_tiles_needed() const { return tile_priorities_dirty_; }
 
-  gfx::Vector2dF ScrollSingleNode(ScrollNode* scroll_node,
+  // TODO(bokan): Make |scroll_node| a const ref.
+  gfx::Vector2dF ScrollSingleNode(ScrollNode& scroll_node,
                                   const gfx::Vector2dF& delta,
                                   const gfx::Point& viewport_point,
                                   bool is_direct_manipulation,
@@ -877,7 +878,7 @@ class CC_EXPORT LayerTreeHostImpl : public InputHandler,
       gfx::Vector2dF* out_local_scroll_delta,
       gfx::PointF* out_local_start_point = nullptr);
   gfx::Vector2dF ScrollNodeWithViewportSpaceDelta(
-      ScrollNode* scroll_node,
+      ScrollNode& scroll_node,
       const gfx::PointF& viewport_point,
       const gfx::Vector2dF& viewport_delta,
       ScrollTree* scroll_tree);
@@ -995,7 +996,7 @@ class CC_EXPORT LayerTreeHostImpl : public InputHandler,
 
   void UpdateRootLayerStateForSynchronousInputHandler();
 
-  bool ScrollAnimationUpdateTarget(ScrollNode* scroll_node,
+  bool ScrollAnimationUpdateTarget(const ScrollNode& scroll_node,
                                    const gfx::Vector2dF& scroll_delta,
                                    base::TimeDelta delayed_by);
 

cc/trees/layer_tree_host_impl_unittest.cc

@@ -8007,6 +8007,170 @@ TEST_F(LayerTreeHostImplTest,
                         gfx::ScrollOffsetToVector2dF(scroll_offset));
 }
 
+// Ensure the viewport correctly handles the user_scrollable bits. That is, if
+// the outer viewport disables user scrolling, we should still be able to
+// scroll the inner viewport.
+TEST_F(LayerTreeHostImplTest, ViewportUserScrollable) {
+  gfx::Size viewport_size(100, 100);
+  gfx::Size content_size(200, 200);
+  SetupViewportLayersOuterScrolls(viewport_size, content_size);
+
+  auto* outer_scroll = OuterViewportScrollLayer();
+  auto* inner_scroll = InnerViewportScrollLayer();
+
+  ScrollTree& scroll_tree =
+      host_impl_->active_tree()->property_trees()->scroll_tree;
+  ElementId inner_element_id = inner_scroll->element_id();
+  ElementId outer_element_id = outer_scroll->element_id();
+
+  DrawFrame();
+
+  // "Zoom in" so that the inner viewport is scrollable.
+  float page_scale_factor = 2;
+  host_impl_->active_tree()->PushPageScaleFromMainThread(
+      page_scale_factor, page_scale_factor, page_scale_factor);
+
+  // Disable scrolling the outer viewport horizontally. The inner viewport
+  // should still be allowed to scroll.
+  GetScrollNode(outer_scroll)->user_scrollable_vertical = true;
+  GetScrollNode(outer_scroll)->user_scrollable_horizontal = false;
+
+  gfx::Vector2dF scroll_delta(30 * page_scale_factor, 0);
+  {
+    auto begin_state =
+        BeginState(gfx::Point(), scroll_delta, InputHandler::TOUCHSCREEN);
+    EXPECT_EQ(
+        InputHandler::SCROLL_ON_IMPL_THREAD,
+        host_impl_->ScrollBegin(begin_state.get(), InputHandler::TOUCHSCREEN)
+            .thread);
+
+    // Try scrolling right, the inner viewport should be allowed to scroll.
+    auto update_state =
+        UpdateState(gfx::Point(), scroll_delta, InputHandler::TOUCHSCREEN);
+    host_impl_->ScrollUpdate(update_state.get());
+
+    EXPECT_VECTOR_EQ(gfx::ScrollOffset(30, 0),
+                     scroll_tree.current_scroll_offset(inner_element_id));
+    EXPECT_VECTOR_EQ(gfx::ScrollOffset(0, 0),
+                     scroll_tree.current_scroll_offset(outer_element_id));
+
+    // Continue scrolling. The inner viewport should scroll until its extent,
+    // however, the outer viewport should not accept any scroll.
+    update_state =
+        UpdateState(gfx::Point(), scroll_delta, InputHandler::TOUCHSCREEN);
+    host_impl_->ScrollUpdate(update_state.get());
+    update_state =
+        UpdateState(gfx::Point(), scroll_delta, InputHandler::TOUCHSCREEN);
+    host_impl_->ScrollUpdate(update_state.get());
+    update_state =
+        UpdateState(gfx::Point(), scroll_delta, InputHandler::TOUCHSCREEN);
+    host_impl_->ScrollUpdate(update_state.get());
+
+    EXPECT_VECTOR_EQ(gfx::ScrollOffset(50, 0),
+                     scroll_tree.current_scroll_offset(inner_element_id));
+    EXPECT_VECTOR_EQ(gfx::ScrollOffset(0, 0),
+                     scroll_tree.current_scroll_offset(outer_element_id));
+
+    host_impl_->ScrollEnd();
+  }
+
+  // Reset. Try the same test above but using animated scrolls.
+  SetScrollOffset(outer_scroll, gfx::ScrollOffset(0, 0));
+  SetScrollOffset(inner_scroll, gfx::ScrollOffset(0, 0));
+
+  {
+    auto begin_state =
+        BeginState(gfx::Point(), scroll_delta, InputHandler::WHEEL);
+    EXPECT_EQ(
+        InputHandler::SCROLL_ON_IMPL_THREAD,
+        host_impl_->ScrollBegin(begin_state.get(), InputHandler::WHEEL).thread);
+
+    // Try scrolling right, the inner viewport should be allowed to scroll.
+    auto update_state = AnimatedUpdateState(gfx::Point(), scroll_delta);
+    host_impl_->ScrollUpdate(update_state.get());
+
+    base::TimeTicks cur_time =
+        base::TimeTicks() + base::TimeDelta::FromMilliseconds(100);
+    viz::BeginFrameArgs begin_frame_args =
+        viz::CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0, 1);
+
+#define ANIMATE(time_ms)                                  \
+  cur_time += base::TimeDelta::FromMilliseconds(time_ms); \
+  begin_frame_args.frame_time = (cur_time);               \
+  begin_frame_args.frame_id.sequence_number++;            \
+  host_impl_->WillBeginImplFrame(begin_frame_args);       \
+  host_impl_->Animate();                                  \
+  host_impl_->UpdateAnimationState(true);                 \
+  host_impl_->DidFinishImplFrame(begin_frame_args);
+
+    // The animation is setup in the first frame so tick twice to actually
+    // animate it.
+    ANIMATE(0);
+    ANIMATE(200);
+
+    EXPECT_VECTOR_EQ(gfx::ScrollOffset(30, 0),
+                     scroll_tree.current_scroll_offset(inner_element_id));
+    EXPECT_VECTOR_EQ(gfx::ScrollOffset(0, 0),
+                     scroll_tree.current_scroll_offset(outer_element_id));
+
+    // Continue scrolling. The inner viewport should scroll until its extent,
+    // however, the outer viewport should not accept any scroll.
+    update_state = AnimatedUpdateState(gfx::Point(), scroll_delta);
+    host_impl_->ScrollUpdate(update_state.get());
+    ANIMATE(10);
+    ANIMATE(200);
+
+    EXPECT_VECTOR_EQ(gfx::ScrollOffset(50, 0),
+                     scroll_tree.current_scroll_offset(inner_element_id));
+    EXPECT_VECTOR_EQ(gfx::ScrollOffset(0, 0),
+                     scroll_tree.current_scroll_offset(outer_element_id));
+
+    // Continue scrolling. the outer viewport should still not scroll.
+    update_state = AnimatedUpdateState(gfx::Point(), scroll_delta);
+    host_impl_->ScrollUpdate(update_state.get());
+    ANIMATE(10);
+    ANIMATE(200);
+
+    EXPECT_VECTOR_EQ(gfx::ScrollOffset(50, 0),
+                     scroll_tree.current_scroll_offset(inner_element_id));
+    EXPECT_VECTOR_EQ(gfx::ScrollOffset(0, 0),
+                     scroll_tree.current_scroll_offset(outer_element_id));
+
+    // Fully scroll the inner viewport. We'll now try to start an animation on
+    // the outer viewport in the vertical direction, which is scrollable. We'll
+    // then try to update the curve to scroll horizontally. Ensure that doesn't
+    // allow any horizontal scroll.
+    SetScrollOffset(inner_scroll, gfx::ScrollOffset(50, 50));
+    update_state = AnimatedUpdateState(gfx::Point(), gfx::Vector2dF(0, 100));
+    host_impl_->ScrollUpdate(update_state.get());
+    ANIMATE(16);
+    ANIMATE(64);
+
+    // We don't care about the exact offset, we just want to make sure the
+    // scroll is in progress but not finished.
+    ASSERT_LT(0, scroll_tree.current_scroll_offset(outer_element_id).y());
+    ASSERT_GT(50, scroll_tree.current_scroll_offset(outer_element_id).y());
+    ASSERT_EQ(0, scroll_tree.current_scroll_offset(outer_element_id).x());
+    EXPECT_VECTOR_EQ(gfx::ScrollOffset(50, 50),
+                     scroll_tree.current_scroll_offset(inner_element_id));
+
+    // Now when we scroll we should do so by updating the ongoing animation
+    // curve. Ensure this doesn't allow any horizontal scrolling.
+    update_state = AnimatedUpdateState(gfx::Point(), gfx::Vector2dF(100, 100));
+    host_impl_->ScrollUpdate(update_state.get());
+    ANIMATE(200);
+
+    EXPECT_VECTOR_EQ(gfx::ScrollOffset(0, 100),
+                     scroll_tree.current_scroll_offset(outer_element_id));
+    EXPECT_VECTOR_EQ(gfx::ScrollOffset(50, 50),
+                     scroll_tree.current_scroll_offset(inner_element_id));
+
+#undef ANIMATE
+
+    host_impl_->ScrollEnd();
+  }
+}
+
 // Ensure that the SetSynchronousInputHandlerRootScrollOffset method used by
 // the WebView API correctly respects the user_scrollable bits on both of the
 // inner and outer viewport scroll nodes.

third_party/blink/web_tests/fast/scrolling/overflow-hidden-viewport-scrolling.html

+<!DOCTYPE html>
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+<script src="../../resources/gesture-util.js"></script>
+<style>
+  #viewport {
+    position: absolute;
+    left: 0;
+    top: 0;
+    width: 100%;
+    height: 100%;
+    z-index: 1;
+    background-color: green;
+  }
+
+  #spacer {
+    width: 200vw;
+    height: 200vh;
+    position: absolute;
+    left: 0;
+    top: 0;
+    z-index: -1;
+    background-color: red;
+  }
+
+  html, body {
+    padding: 0px;
+    margin: 0px;
+    width: 100%;
+    height: 100%;
+  }
+
+  html {
+    overflow-x: hidden;
+    overflow-y: hidden;
+  }
+</style>
+<div id="viewport">
+  To test manually, pinch zoom into the page and scroll around. You should be
+  able to pan around within the green area but you should never scroll the
+  document (you should never see red).
+</div>
+<div id="spacer"></div>
+<script>
+  const viewport = document.getElementById('viewport');
+
+  window.onload = () => {
+    if (!window.internals)
+      return;
+
+    internals.setPageScaleFactor(2);
+
+    promise_test (async (test) => {
+      await waitForCompositorCommit();
+
+      const delta = 2000;
+      const location = elementCenter(viewport);
+      await smoothScroll(delta,
+                         location.x,
+                         location.y,
+                         GestureSourceType.TOUCH_INPUT,
+                         'down',
+                         SPEED_INSTANT);
+
+      assert_equals(window.scrollY, 0, "Document doesn't scroll vertically.");
+      assert_equals(window.visualViewport.offsetTop, 300, "VisualViewport pans vertically.");
+    }, "Test viewport scrolling for overflow-y: hidden.");
+
+    promise_test (async (test) => {
+      await waitForCompositorCommit();
+
+      const delta = 2000;
+      const location = elementCenter(viewport);
+      await smoothScroll(delta,
+                         location.x,
+                         location.y,
+                         GestureSourceType.TOUCH_INPUT,
+                         'right',
+                         SPEED_INSTANT);
+
+      assert_equals(window.scrollX, 0, "Document doesn't scroll horizontally.");
+      assert_equals(window.visualViewport.offsetLeft, 400, "VisualViewport pans horizontally.");
+    }, "Test viewport scrolling for overflow-x: hidden.");
+  }
+</script>