Reassign snap areas on snap container removal

Removing a snap container (i.e. making it unscrollable) should reassign
its snap areas to the next scrollable ancestor.

Bug: 1007456
Change-Id: Ic3ebea990a25a36020d278a33fb3c67dea708db6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1899189Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Reviewed-by: Yi Gu <yigu@chromium.org>
Commit-Queue: Kaan Alsan <alsan@google.com>
Cr-Commit-Position: refs/heads/master@{#715889}

third_party/blink/renderer/core/layout/layout_box.cc

@@ -6292,6 +6292,17 @@ void LayoutBox::RemoveSnapArea(const LayoutBox& snap_area) {
   }
 }
 
+void LayoutBox::ReassignSnapAreas(LayoutBox& new_container) {
+  SnapAreaSet* areas = SnapAreas();
+  if (!areas)
+    return;
+  for (auto* const snap_area : *areas) {
+    snap_area->rare_data_->snap_container_ = &new_container;
+    new_container.AddSnapArea(*snap_area);
+  }
+  areas->clear();
+}
+
 bool LayoutBox::AllowedToPropagateRecursiveScrollToParentFrame(
     const WebScrollIntoViewParams& params) {
   if (!GetFrameView()->SafeToPropagateScrollToParent())

third_party/blink/renderer/core/layout/layout_box.h

@@ -1477,6 +1477,8 @@ class CORE_EXPORT LayoutBox : public LayoutBoxModelObject {
   // For snap containers, returns all associated snap areas.
   SnapAreaSet* SnapAreas() const;
   void ClearSnapAreas();
+  // Moves all snap areas to the new container.
+  void ReassignSnapAreas(LayoutBox& new_container);
 
   // CustomLayoutChild only exists if this LayoutBox is a IsCustomItem (aka. a
   // child of a LayoutCustom). This is created/destroyed when this LayoutBox is

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

@@ -26,28 +26,29 @@ SnapCoordinator::SnapCoordinator() : snap_containers_() {}
 
 SnapCoordinator::~SnapCoordinator() = default;
 
-// Returns the scroll container that can be affected by this snap area.
-static LayoutBox* FindSnapContainer(const LayoutBox& snap_area) {
+// Returns the layout box's next ancestor that can be a snap container.
+// The origin may be either a snap area or a snap container.
+LayoutBox* FindSnapContainer(const LayoutBox& origin_box) {
   // According to the new spec
   // https://drafts.csswg.org/css-scroll-snap/#snap-model
   // "Snap positions must only affect the nearest ancestor (on the element’s
   // containing block chain) scroll container".
-  Element* document_element = snap_area.GetDocument().documentElement();
-  LayoutBox* box = snap_area.ContainingBlock();
+  Element* document_element = origin_box.GetDocument().documentElement();
+  LayoutBox* box = origin_box.ContainingBlock();
   while (box && !box->HasOverflowClip() && !box->IsLayoutView() &&
          box->GetNode() != document_element)
     box = box->ContainingBlock();
 
-  // If we reach to document element then we dispatch to viewport
+  // If we reach to document element then we dispatch to layout view.
   if (box && box->GetNode() == document_element)
-    return snap_area.GetDocument().GetLayoutView();
+    return origin_box.GetDocument().GetLayoutView();
 
   return box;
 }
 
 // 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) {
+cc::ScrollSnapType GetPhysicalSnapType(const LayoutBox& snap_container) {
   cc::ScrollSnapType scroll_snap_type =
       snap_container.Style()->GetScrollSnapType();
   if (scroll_snap_type.axis == cc::SnapAxis::kInline) {
@@ -67,13 +68,31 @@ static cc::ScrollSnapType GetPhysicalSnapType(const LayoutBox& snap_container) {
 }
 
 void SnapCoordinator::AddSnapContainer(LayoutBox& snap_container) {
+  // TODO(http://crbug.com/1007456): Reassign snap areas that should be assigned
+  // to this new snap container.
   snap_containers_.insert(&snap_container);
 }
 
 void SnapCoordinator::RemoveSnapContainer(LayoutBox& snap_container) {
-  // TODO(majidvp): The snap areas assigned to this container may need to be
-  // re-assigned. http://crbug.com/1007456
-  snap_container.ClearSnapAreas();
+  LayoutBox* ancestor_snap_container = FindSnapContainer(snap_container);
+
+  // We remove the snap container if it is no longer scrollable, or if the
+  // element is detached.
+  // - If it is no longer scrollable, then we reassign its snap areas to the
+  // next ancestor snap container.
+  // - If it is detached, then we simply clear its snap areas since they will be
+  // detached as well.
+  if (ancestor_snap_container) {
+    // No need to update the ancestor's snap container data because it will be
+    // updated at the end of the layout.
+    snap_container.ReassignSnapAreas(*ancestor_snap_container);
+  } else {
+    DCHECK(!snap_container.Parent());
+    snap_container.ClearSnapAreas();
+  }
+  // We don't need to update the old snap container's data since the
+  // corresponding ScrollableArea is being removed, and thus the snap container
+  // data is removed too.
   snap_container.SetNeedsPaintPropertyUpdate();
   snap_containers_.erase(&snap_container);
 }
@@ -105,13 +124,6 @@ void SnapCoordinator::SnapContainerDidChange(LayoutBox& snap_container) {
 
   // TODO(sunyunjia): Only update when the localframe doesn't need layout.
   UpdateSnapContainerData(snap_container);
-
-  // TODO(majidvp): Add logic to correctly handle orphaned snap areas here.
-  // 1. Removing container: find a new snap container for its orphan snap
-  // areas (most likely nearest ancestor of current container) otherwise add
-  // them to an orphan list.
-  // 2. Adding container: may take over snap areas from nearest ancestor snap
-  // container or from existing areas in orphan pool.
 }
 
 void SnapCoordinator::SnapAreaDidChange(LayoutBox& snap_area,
@@ -133,10 +145,6 @@ void SnapCoordinator::SnapAreaDidChange(LayoutBox& snap_area,
     UpdateSnapContainerData(*new_container);
     if (old_container && old_container != new_container)
       UpdateSnapContainerData(*old_container);
-  } else {
-    // TODO(majidvp): keep track of snap areas that do not have any
-    // container so that we check them again when a new container is
-    // added to the page.
   }
 }
 

third_party/blink/web_tests/external/wpt/css/css-scroll-snap/snap-area-capturing-remove-scroll-container.html

+<!DOCTYPE html>
+<title>
+  When an element no longer captures snap positions (e.g., no longer
+  scrollable), then its currently captured snap areas must be reassigned.
+</title>
+<link rel="help" href="https://drafts.csswg.org/css-scroll-snap/#captures-snap-positions"/>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<style>
+div {
+  position: absolute;
+  margin: 0px;
+}
+
+html {
+  scroll-snap-type: y mandatory;
+}
+
+body {
+  margin: 0px;
+}
+
+#middle-scroller {
+  top: 100px;
+  height: 500px;
+  width: 500px;
+  overflow: scroll;
+  background-color: rgb(12, 61, 2);
+  scroll-snap-type: none;
+}
+
+#inner-scroller {
+  top: 200px;
+  height: 400px;
+  width: 400px;
+  overflow: scroll;
+  background-color: rgb(65, 139, 50);
+  scroll-snap-type: y mandatory;
+}
+
+.space {
+  width: 2000px;
+  height: 2000px;
+}
+
+#inner-snap-area {
+  top: 300px;
+  width: 200px;
+  height: 200px;
+  background-color: blue;
+  scroll-snap-align: start;
+}
+
+#document-snap-area {
+  top: 500px;
+  width: 200px;
+  height: 200px;
+  background-color: lightblue;
+  scroll-snap-align: start;
+}
+
+</style>
+<div class="space"></div>
+  <div id="middle-scroller">
+    <div class="space"></div>
+    <div id="inner-scroller">
+        <div class="space"></div>
+        <div id="inner-snap-area"></div>
+    </div>
+  </div>
+</div>
+<div id="document-snap-area"></div>
+<script>
+
+// This tests that making a snap container no longer scrollable will reassign
+// its snap areas to the next scrollable ancestor, per spec [1].
+// [1] https://drafts.csswg.org/css-scroll-snap/#captures-snap-positions
+test(() => {
+  const innerscroller = document.getElementById("inner-scroller");
+  const middlescroller = document.getElementById("middle-scroller");
+  const documentscroller = document.scrollingElement;
+
+  // Middle scroller doesn't snap.
+  // Document scroller should snap to its only captured area.
+  documentscroller.scrollBy(0, 100);
+  middlescroller.scrollBy(0, 10);
+  assert_equals(innerscroller.scrollTop, 0);
+  assert_equals(middlescroller.scrollTop, 10);
+  assert_equals(documentscroller.scrollTop, 500);
+
+  // Inner scroller snaps.
+  innerscroller.scrollBy(0, 10);
+  assert_equals(innerscroller.scrollTop, 300);
+  assert_equals(middlescroller.scrollTop, 10);
+  assert_equals(documentscroller.scrollTop, 500);
+
+  // Inner scroller is no longer a scroll container.
+  innerscroller.style.setProperty("overflow", "visible");
+  assert_equals(innerscroller.scrollTop, 0);
+  assert_equals(middlescroller.scrollTop, 10);
+  assert_equals(documentscroller.scrollTop, 500);
+
+  // The new snap container is the middle scroller, which has snap-type 'none'.
+  // Per spec, the scroll container should capture snap positions even if it has
+  // snap-type 'none'.
+  // The middle scroller should not snap.
+  // The document scroller should still only snap to its captured snap area.
+  documentscroller.scrollBy(0, 100);
+  middlescroller.scrollBy(0, 10);
+  assert_equals(innerscroller.scrollTop, 0);
+  assert_equals(middlescroller.scrollTop, 20);
+  assert_equals(documentscroller.scrollTop, 500);
+
+  // The scroll container should now be at the document level.
+  middlescroller.style.setProperty("overflow", "visible");
+  documentscroller.scrollBy(0, -10);
+  assert_equals(innerscroller.scrollTop, 0);
+  assert_equals(middlescroller.scrollTop, 0);
+
+  // Check that the existing snap area did not get removed when reassigning
+  // the inner snap area.
+  assert_equals(documentscroller.scrollTop, 500);
+
+  // Check that the inner snap area got reassigned to the document.
+  documentscroller.scrollBy(0, 150);
+  assert_equals(documentscroller.scrollTop, 600);
+}, 'Making a snap container not scrollable should promote the next scrollable\
+ ancestor to become a snap container.');
+</script>