Snap with the RootFrameViewport of root scrollers

When performing snapping on a root scroller that is not the main
frame's layout viewport we now perform snapping using the
RootFrameViewport instead of only using the layout viewport (the
PaintLayerScrollableArea).

Also added a test to check this behaviour.

Bug: 1015451
Change-Id: I2276b904adef1bb970bd290b49d44ac5e7153982
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1865048
Commit-Queue: Kaan Alsan <alsan@google.com>
Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Yi Gu <yigu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#709190}

third_party/blink/renderer/core/input/scroll_manager.cc

@@ -62,16 +62,6 @@ cc::SnapFlingController::GestureScrollUpdateInfo GetGestureScrollUpdateInfo(
                               WebGestureEvent::InertialPhaseState::kMomentum,
       .event_time = event.TimeStamp()};
 }
-
-ScrollableArea* ScrollableAreaForSnapping(LayoutBox* layout_box) {
-  if (!layout_box)
-    return nullptr;
-
-  return layout_box->IsLayoutView()
-             ? layout_box->GetFrameView()->GetScrollableArea()
-             : layout_box->GetScrollableArea();
-}
-
 }  // namespace
 
 ScrollManager::ScrollManager(LocalFrame& frame) : frame_(frame) {
@@ -284,27 +274,9 @@ bool ScrollManager::LogicalScroll(ScrollDirection direction,
         ToPhysicalDirection(direction, box->IsHorizontalWritingMode(),
                             box->Style()->IsFlippedBlocksWritingMode());
 
-    ScrollableArea* scrollable_area = nullptr;
-
-    // The global root scroller must be scrolled by the RootFrameViewport.
-    if (box->IsGlobalRootScroller()) {
-      LocalFrame& main_frame = frame_->LocalFrameRoot();
-      // The local root must be the main frame if we have the global root
-      // scroller since it can't yet be set on OOPIFs.
-      DCHECK(main_frame.IsMainFrame());
-
-      scrollable_area = main_frame.View()->GetScrollableArea();
-    } else {
-      scrollable_area = box->GetScrollableArea();
-    }
-
+    ScrollableArea* scrollable_area = ScrollableArea::GetForScrolling(box);
     DCHECK(scrollable_area);
 
-    // TODO(crbug.com/1015451): Remove ScrollableAreaForSnapping and use the
-    // RootFrameViewport if the scroller is a global root scroller.
-    ScrollableArea* scrollable_area_for_snapping =
-        ScrollableAreaForSnapping(box);
-
     ScrollOffset delta = ToScrollDelta(physical_direction, 1);
     delta.Scale(scrollable_area->ScrollStep(granularity, kHorizontalScrollbar),
                 scrollable_area->ScrollStep(granularity, kVerticalScrollbar));
@@ -314,12 +286,12 @@ bool ScrollManager::LogicalScroll(ScrollDirection direction,
     // scroll with intended end position only.
     switch (granularity) {
       case ScrollGranularity::kScrollByLine: {
-        if (scrollable_area_for_snapping->SnapForDirection(delta))
+        if (scrollable_area->SnapForDirection(delta))
           return true;
         break;
       }
       case ScrollGranularity::kScrollByPage: {
-        if (scrollable_area_for_snapping->SnapForEndAndDirection(delta))
+        if (scrollable_area->SnapForEndAndDirection(delta))
           return true;
         break;
       }
@@ -329,8 +301,8 @@ bool ScrollManager::LogicalScroll(ScrollDirection direction,
                           physical_direction == kScrollRight;
         bool scrolled_y = physical_direction == kScrollUp ||
                           physical_direction == kScrollDown;
-        if (scrollable_area_for_snapping->SnapForEndPosition(
-                end_position, scrolled_x, scrolled_y))
+        if (scrollable_area->SnapForEndPosition(end_position, scrolled_x,
+                                                scrolled_y))
           return true;
         break;
       }
@@ -767,9 +739,9 @@ bool ScrollManager::SnapAtGestureScrollEnd(
   if (!previous_gesture_scrolled_node_ ||
       (!did_scroll_x_for_scroll_gesture_ && !did_scroll_y_for_scroll_gesture_))
     return false;
-  LayoutBox* layout_box = LayoutBoxForSnapping();
-  ScrollableArea* scrollable_area = ScrollableAreaForSnapping(layout_box);
-  if (!layout_box || !scrollable_area)
+  ScrollableArea* scrollable_area =
+      ScrollableArea::GetForScrolling(LayoutBoxForSnapping());
+  if (!scrollable_area)
     return false;
 
   bool is_mouse_wheel =
@@ -806,9 +778,9 @@ bool ScrollManager::GetSnapFlingInfo(
     const gfx::Vector2dF& natural_displacement,
     gfx::Vector2dF* out_initial_position,
     gfx::Vector2dF* out_target_position) const {
-  LayoutBox* layout_box = LayoutBoxForSnapping();
-  ScrollableArea* scrollable_area = ScrollableAreaForSnapping(layout_box);
-  if (!layout_box || !scrollable_area)
+  ScrollableArea* scrollable_area =
+      ScrollableArea::GetForScrolling(LayoutBoxForSnapping());
+  if (!scrollable_area)
     return false;
 
   FloatPoint current_position = scrollable_area->ScrollPosition();
@@ -828,7 +800,7 @@ bool ScrollManager::GetSnapFlingInfo(
 gfx::Vector2dF ScrollManager::ScrollByForSnapFling(
     const gfx::Vector2dF& delta) {
   ScrollableArea* scrollable_area =
-      ScrollableAreaForSnapping(LayoutBoxForSnapping());
+      ScrollableArea::GetForScrolling(LayoutBoxForSnapping());
   if (!scrollable_area)
     return gfx::Vector2dF();
 

third_party/blink/renderer/core/page/scrolling/snap_coordinator.cc

@@ -44,12 +44,6 @@ static LayoutBox* FindSnapContainer(const LayoutBox& snap_area) {
   return box;
 }
 
-static ScrollableArea* ScrollableAreaForSnapping(const LayoutBox& layout_box) {
-  return layout_box.IsLayoutView()
-             ? layout_box.GetFrameView()->GetScrollableArea()
-             : layout_box.GetScrollableArea();
-}
-
 // Snap types are categorized according to the spec
 // https://drafts.csswg.org/css-scroll-snap-1/#snap-axis
 static cc::ScrollSnapType GetPhysicalSnapType(const LayoutBox& snap_container) {
@@ -91,10 +85,12 @@ void SnapCoordinator::SnapContainerDidChange(LayoutBox& snap_container) {
       snap_container.GetDocument().documentElement())
     return;
 
-  bool is_scroll_container =
-      snap_container.IsLayoutView() || snap_container.HasOverflowClip();
-  if (!is_scroll_container) {
-    DCHECK(!ScrollableAreaForSnapping(snap_container));
+  // Per specification snap positions only affect *scroll containers* [1]. So if
+  // the layout box is not a scroll container we ignore it here even if it has
+  // non-none scroll-snap-type. Note that in blink, existence of scrollable area
+  // directly maps to being a scroll container in the specification. [1]
+  // https://drafts.csswg.org/css-scroll-snap/#overview
+  if (!snap_container.GetScrollableArea()) {
     DCHECK(!snap_containers_.Contains(&snap_container));
     return;
   }
@@ -149,8 +145,8 @@ void SnapCoordinator::UpdateAllSnapContainerData() {
 }
 
 void SnapCoordinator::UpdateSnapContainerData(LayoutBox& snap_container) {
-
-  ScrollableArea* scrollable_area = ScrollableAreaForSnapping(snap_container);
+  ScrollableArea* scrollable_area =
+      ScrollableArea::GetForScrolling(&snap_container);
   const auto* old_snap_container_data = scrollable_area->GetSnapContainerData();
   auto snap_type = GetPhysicalSnapType(snap_container);
 
@@ -270,7 +266,9 @@ cc::SnapAreaData SnapCoordinator::CalculateSnapAreaData(
   PhysicalRect area_rect = snap_area.PhysicalBorderBoxRect();
   area_rect = snap_area.LocalToAncestorRect(area_rect, &snap_container,
                                             kTraverseDocumentBoundaries);
-  ScrollableArea* scrollable_area = ScrollableAreaForSnapping(snap_container);
+  ScrollableArea* scrollable_area =
+      ScrollableArea::GetForScrolling(&snap_container);
+
   if (scrollable_area) {
     if (snap_container.IsLayoutView()) {
       area_rect = snap_container.GetFrameView()->FrameToDocument(area_rect);
@@ -313,10 +311,12 @@ void SnapCoordinator::ShowSnapAreasFor(const LayoutBox* container) {
 }
 
 void SnapCoordinator::ShowSnapDataFor(const LayoutBox* snap_container) {
+  if (!snap_container)
+    return;
+  ScrollableArea* scrollable_area =
+      ScrollableArea::GetForScrolling(snap_container);
   const auto* optional_data =
-      ScrollableAreaForSnapping(*snap_container)
-          ? ScrollableAreaForSnapping(*snap_container)->GetSnapContainerData()
-          : nullptr;
+      scrollable_area ? scrollable_area->GetSnapContainerData() : nullptr;
   if (optional_data)
     LOG(INFO) << *optional_data;
 }

third_party/blink/renderer/core/page/scrolling/snap_coordinator.h

@@ -53,6 +53,7 @@ class CORE_EXPORT SnapCoordinator final
   // Called by LocalFrameView::PerformPostLayoutTasks(), so that the snap data
   // are updated whenever a layout happens.
   void UpdateAllSnapContainerData();
+  void UpdateSnapContainerData(LayoutBox&);
 
 #ifndef NDEBUG
   void ShowSnapAreaMap();
@@ -63,8 +64,6 @@ class CORE_EXPORT SnapCoordinator final
  private:
   friend class SnapCoordinatorTest;
 
-  void UpdateSnapContainerData(LayoutBox&);
-
   HashSet<LayoutBox*> snap_containers_;
   DISALLOW_COPY_AND_ASSIGN(SnapCoordinator);
 };

third_party/blink/renderer/core/paint/paint_layer_scrollable_area.cc

@@ -1072,6 +1072,12 @@ void PaintLayerScrollableArea::DidChangeGlobalRootScroller() {
     SetHasHorizontalScrollbar(needs_horizontal_scrollbar);
     SetHasVerticalScrollbar(needs_vertical_scrollbar);
   }
+
+  // Recalculate the snap container data since the scrolling behaviour for this
+  // layout box changed (i.e. it either became the layout viewport or it
+  // is no longer the layout viewport).
+  GetLayoutBox()->GetDocument().GetSnapCoordinator().UpdateSnapContainerData(
+      *GetLayoutBox());
 }
 
 bool PaintLayerScrollableArea::ShouldPerformScrollAnchoring() const {

third_party/blink/renderer/core/scroll/scrollable_area.cc

@@ -43,6 +43,7 @@
 #include "third_party/blink/renderer/core/layout/layout_shift_tracker.h"
 #include "third_party/blink/renderer/core/page/chrome_client.h"
 #include "third_party/blink/renderer/core/page/page.h"
+#include "third_party/blink/renderer/core/paint/paint_layer_scrollable_area.h"
 #include "third_party/blink/renderer/core/paint/paint_timing_detector.h"
 #include "third_party/blink/renderer/core/scroll/programmatic_scroll_animator.h"
 #include "third_party/blink/renderer/core/scroll/scroll_animator_base.h"
@@ -868,6 +869,7 @@ bool ScrollableArea::SnapAtCurrentPosition(
     bool scrolled_x,
     bool scrolled_y,
     base::ScopedClosureRunner on_finish) {
+  DCHECK(IsRootFrameViewport() || !GetLayoutBox()->IsGlobalRootScroller());
   FloatPoint current_position = ScrollPosition();
   return SnapForEndPosition(current_position, scrolled_x, scrolled_y,
                             std::move(on_finish));
@@ -877,6 +879,7 @@ bool ScrollableArea::SnapForEndPosition(const FloatPoint& end_position,
                                         bool scrolled_x,
                                         bool scrolled_y,
                                         base::ScopedClosureRunner on_finish) {
+  DCHECK(IsRootFrameViewport() || !GetLayoutBox()->IsGlobalRootScroller());
   std::unique_ptr<cc::SnapSelectionStrategy> strategy =
       cc::SnapSelectionStrategy::CreateForEndPosition(
           gfx::ScrollOffset(end_position), scrolled_x, scrolled_y);
@@ -885,6 +888,7 @@ bool ScrollableArea::SnapForEndPosition(const FloatPoint& end_position,
 
 bool ScrollableArea::SnapForDirection(const ScrollOffset& delta,
                                       base::ScopedClosureRunner on_finish) {
+  DCHECK(IsRootFrameViewport() || !GetLayoutBox()->IsGlobalRootScroller());
   FloatPoint current_position = ScrollPosition();
   std::unique_ptr<cc::SnapSelectionStrategy> strategy =
       cc::SnapSelectionStrategy::CreateForDirection(
@@ -894,6 +898,7 @@ bool ScrollableArea::SnapForDirection(const ScrollOffset& delta,
 }
 
 bool ScrollableArea::SnapForEndAndDirection(const ScrollOffset& delta) {
+  DCHECK(IsRootFrameViewport() || !GetLayoutBox()->IsGlobalRootScroller());
   FloatPoint current_position = ScrollPosition();
   std::unique_ptr<cc::SnapSelectionStrategy> strategy =
       cc::SnapSelectionStrategy::CreateForEndAndDirection(
@@ -907,7 +912,6 @@ bool ScrollableArea::PerformSnapping(const cc::SnapSelectionStrategy& strategy,
   base::Optional<FloatPoint> snap_point = GetSnapPosition(strategy);
   if (!snap_point)
     return false;
-
   CancelScrollAnimation();
   CancelProgrammaticScrollAnimation();
   SetScrollOffset(ScrollPositionToOffset(snap_point.value()),
@@ -935,4 +939,17 @@ void ScrollableArea::InjectGestureScrollEvent(
       GetCompositorElementId(), gesture_type);
 }
 
+ScrollableArea* ScrollableArea::GetForScrolling(const LayoutBox* layout_box) {
+  if (!layout_box)
+    return nullptr;
+
+  if (!layout_box->IsGlobalRootScroller())
+    return layout_box->GetScrollableArea();
+
+  // The global root scroller should be scrolled by the root frame view's
+  // ScrollableArea.
+  LocalFrame& root_frame = layout_box->GetFrame()->LocalFrameRoot();
+  return root_frame.View()->GetScrollableArea();
+}
+
 }  // namespace blink

third_party/blink/renderer/core/scroll/scrollable_area.h

@@ -462,6 +462,11 @@ class CORE_EXPORT ScrollableArea : public GarbageCollectedMixin {
                                 ScrollGranularity granularity,
                                 WebInputEvent::Type gesture_type) const;
 
+  // If the layout box is a global root scroller then the root frame view's
+  // ScrollableArea is returned. Otherwise, the layout box's
+  // PaintLayerScrollableArea (which can be null) is returned.
+  static ScrollableArea* GetForScrolling(const LayoutBox* layout_box);
+
  protected:
   // Deduces the ScrollBehavior based on the element style and the parameter set
   // by programmatic scroll into either instant or smooth scroll.

third_party/blink/renderer/core/scroll/scrollbar.cc

@@ -512,13 +512,9 @@ void Scrollbar::MouseUp(const WebMouseEvent& mouse_event) {
     if (is_captured)
       scrollable_area_->MouseReleasedScrollbar();
 
-    ScrollableArea* scrollable_area_for_snapping =
-        scrollable_area_->GetLayoutBox()->IsLayoutView()
-            ? scrollable_area_->GetLayoutBox()
-                  ->GetFrameView()
-                  ->GetScrollableArea()
-            : scrollable_area_.Get();
-    scrollable_area_for_snapping->SnapAfterScrollbarScrolling(orientation_);
+    ScrollableArea* scrollable_area_for_scrolling =
+        ScrollableArea::GetForScrolling(scrollable_area_->GetLayoutBox());
+    scrollable_area_for_scrolling->SnapAfterScrollbarScrolling(orientation_);
 
     ScrollbarPart part = GetTheme().HitTest(
         *this, FlooredIntPoint(mouse_event.PositionInRootFrame()));

third_party/blink/web_tests/fast/scroll-snap/root-scroller-snap-behaviour/arrow-key-scroll-snaps-visual-viewport.html

+<!DOCTYPE html>
+<style>
+body, html {
+  width: 100%;
+  height: 100%;
+  margin: 0px;
+}
+
+#root-scroller::-webkit-scrollbar {
+  width: 0px;
+  height: 0px;
+}
+
+#root-scroller {
+  width: 100%;
+  height: 100%;
+  overflow: scroll;
+  position: absolute;
+  left: 0;
+  top: 0;
+  background-color: red;
+  scroll-snap-type: y mandatory;
+}
+
+.spacer {
+  width: 100%;
+  height: 100%;
+}
+
+#snap-area {
+  width: 200px;
+  height: 50%;
+  background-color: blue;
+  scroll-snap-align: start;
+}
+</style>
+
+<script src="../../../resources/gesture-util.js"></script>
+<script src="../../../resources/testharness.js"></script>
+<script src="../../../resources/testharnessreport.js"></script>
+
+<div id="root-scroller">
+  <div class="spacer" style="background-color: PaleGreen"></div>
+  <div class="spacer" style="background-color: PaleGreen"></div>
+  <div id="snap-area"></div>
+</div>
+
+<script>
+if (window.internals) {
+  internals.runtimeFlags.implicitRootScrollerEnabled = true;
+}
+
+const rootscroller = document.getElementById("root-scroller");
+const snaparea = document.getElementById("snap-area");
+
+async function arrowDown() {
+  // Click on the middle of the viewport.
+  const initial_scroll_position = {
+    x: visualViewport.width / 2,
+    y: visualViewport.height / 2
+  }
+  await mouseClickOn(initial_scroll_position.x, initial_scroll_position.y);
+  await new Promise((resolve, reject) => {
+    if (window.eventSender) {
+      eventSender.keyDown("ArrowDown");
+      resolve();
+    }
+    else {
+      reject('This test requires window.eventSender');
+    }
+  });
+}
+
+// Tests that the visual viewport is affected when scrolling the global root
+// scroller with an arrow key. The snap area is located at the bottom of the layout
+// viewport, so the layout viewport cannot align with the snap area.
+// However, when it becomes the global root scroller we should be scrolling the
+// visual viewport too to align with the snap area.
+promise_test(async function () {
+  const scale_factor = 2;
+  internals.setPageScaleFactor(scale_factor);
+  internals.setVisualViewportOffset(0, 0);
+
+  assert_equals(visualViewport.scale, 2);
+  assert_equals(visualViewport.offsetTop, 0);
+
+  await waitForCompositorCommit();
+
+  assert_equals(window.internals.effectiveRootScroller(document),
+    rootscroller,
+    "#root-scroller must be the effective root scroller");
+
+  await arrowDown()
+
+  const max_frames = 500;
+  const max_unchanged_frames = 15;
+  await waitForAnimationEnd(() => {
+    return rootscroller.scrollTop;
+  }, max_frames, max_unchanged_frames);
+
+  // The offset of the visual viewport and the layout viewport combined should
+  // be at the snap point.
+  assert_equals(visualViewport.offsetTop + rootscroller.scrollTop,
+    snaparea.offsetTop, "Visual viewport offset combined with the scroller's\
+    scroll offset should add to the snap area's position.");
+}, "Snapping the root scroller after arrow key scrolling should affect the\
+visual viewport offset.");
+</script>

third_party/blink/web_tests/fast/scroll-snap/root-scroller-snap-behaviour/smooth-scroll-snaps-visual-viewport.html

+<!DOCTYPE html>
+<style>
+body, html {
+  width: 100%;
+  height: 100%;
+  margin: 0px;
+}
+
+#root-scroller::-webkit-scrollbar {
+  width: 0px;
+  height: 0px;
+}
+
+#root-scroller {
+  width: 100%;
+  height: 100%;
+  overflow: scroll;
+  position: absolute;
+  left: 0;
+  top: 0;
+  background-color: red;
+  scroll-snap-type: y mandatory;
+}
+
+.spacer {
+  width: 100%;
+  height: 100%;
+}
+
+#snap-area {
+  width: 200px;
+  height: 50%;
+  background-color: blue;
+  scroll-snap-align: start;
+}
+</style>
+
+<script src="../../../resources/gesture-util.js"></script>
+<script src="../../../resources/testharness.js"></script>
+<script src="../../../resources/testharnessreport.js"></script>
+
+<div id="root-scroller">
+  <div class="spacer" style="background-color: PaleGreen"></div>
+  <div class="spacer" style="background-color: PaleGreen"></div>
+  <div id="snap-area"></div>
+</div>
+
+<script>
+if (window.internals) {
+  internals.runtimeFlags.implicitRootScrollerEnabled = true;
+}
+
+const rootscroller = document.getElementById("root-scroller");
+const snaparea = document.getElementById("snap-area");
+
+// Tests that the visual viewport is affected when scrolling the global root
+// scroller by gesture. The snap area is located at the bottom of the layout
+// viewport, so the layout viewport cannot align with the snap area.
+// However, when it becomes the global root scroller we should be scrolling the
+// visual viewport too to align with the snap area.
+//
+// TODO: There is no test for programmatic scrolling because the expected
+// behaviour of the visual viewport while scrolling the root scroller is not
+// clear [1]. The current behaviour is that the visual viewport offset does not
+// change, and only the layout viewport scrolls to the target.
+// [1] https://github.com/w3c/csswg-drafts/issues/4393
+promise_test(async function () {
+  const scale_factor = 2;
+  internals.setPageScaleFactor(scale_factor);
+  internals.setVisualViewportOffset(0, 0);
+
+  assert_equals(visualViewport.scale, 2);
+  assert_equals(visualViewport.offsetTop, 0);
+
+  await waitForCompositorCommit();
+
+  assert_equals(window.internals.effectiveRootScroller(document),
+    rootscroller,
+    "#root-scroller must be the effective root scroller");
+
+  // Scroll halfway to the snap area. This will trigger the snapping.
+  const delta = snaparea.offsetTop / 2;
+
+  // Start scroll from the middle of the viewport.
+  const initial_scroll_position = {
+    x: visualViewport.width / 2,
+    y: visualViewport.height / 2
+  }
+  await smoothScroll(
+    delta,
+    initial_scroll_position.x,
+    initial_scroll_position.y,
+    GestureSourceType.TOUCH_INPUT,
+    'down',
+    SPEED_INSTANT);
+
+  const max_frames = 500;
+  const max_unchanged_frames = 15;
+  await waitForAnimationEnd(() => {
+    return rootscroller.scrollTop;
+  }, max_frames, max_unchanged_frames);
+
+  // The offset of the visual viewport and the layout viewport combined should
+  // be at the snap point.
+  assert_equals(visualViewport.offsetTop + rootscroller.scrollTop,
+    snaparea.offsetTop, "Visual viewport offset combined with the scroller's\
+    scroll offset should add to the snap area's position.");
+
+}, "Snapping the root scroller after smooth scrolling should affect the\
+visual viewport offset.");
+</script>

third_party/blink/web_tests/resources/gesture-util.js

@@ -63,6 +63,8 @@ function conditionHolds(condition, error_message = 'Condition is not true anymor
   });
 }
 
+// TODO: Frames are animated every 1ms for testing. It may be better to have the
+// timeout based on time rather than frame count.
 function waitForAnimationEnd(getValue, max_frame, max_unchanged_frame) {
   const MAX_FRAME = max_frame;
   const MAX_UNCHANGED_FRAME = max_unchanged_frame;