[scroll-snap] Fix viewport style propagation

Previously we propagated scroll snap properties [1] from viewport
defining element's style to the viewport. However this is not spec
compliant [1] and more importantly our behavior is different from
Gecko.

This patch fixes this issue by having these properties propagate
from document.documentElement instead.


TODO: better understand the backward compat impact.
Note that this can cause previously snapping viewport to no longer snap
if the web developer has declared the snap type on body as opposed to
document and body is viewport defining element

[1] scroll-snap-type, scroll-padding
[2] https://github.com/w3c/csswg-drafts/issues/3740

BUG= 952711
TEST= wpt/css/css-scroll-snap/scroll-snap-type-on-root-element.html


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

third_party/blink/renderer/core/dom/document.cc

@@ -2335,6 +2335,41 @@ void Document::SetupFontBuilder(ComputedStyle& document_style) {
   font_builder.CreateFontForDocument(selector, document_style);
 }
 
+#define PROPAGATE_FROM(source, getter, setter, initial) \
+  PROPAGATE_VALUE(source ? source->getter() : initial, getter, setter);
+
+#define PROPAGATE_VALUE(value, getter, setter)     \
+  if ((new_viewport_style->getter()) != (value)) { \
+    new_viewport_style->setter(value);             \
+    changed = true;                                \
+  }
+
+bool PropagateScrollSnapStyleToViewport(
+    Document& document,
+    const ComputedStyle* document_element_style,
+    scoped_refptr<ComputedStyle> new_viewport_style) {
+  bool changed = false;
+  // We only propagate the properties related to snap container since viewport
+  // defining element cannot be a snap area.
+  PROPAGATE_FROM(document_element_style, GetScrollSnapType, SetScrollSnapType,
+                 cc::ScrollSnapType());
+  PROPAGATE_FROM(document_element_style, ScrollPaddingTop, SetScrollPaddingTop,
+                 Length());
+  PROPAGATE_FROM(document_element_style, ScrollPaddingRight,
+                 SetScrollPaddingRight, Length());
+  PROPAGATE_FROM(document_element_style, ScrollPaddingBottom,
+                 SetScrollPaddingBottom, Length());
+  PROPAGATE_FROM(document_element_style, ScrollPaddingLeft,
+                 SetScrollPaddingLeft, Length());
+
+  if (changed) {
+    document.GetSnapCoordinator()->SnapContainerDidChange(
+        *document.GetLayoutView(), false /* is_removed */);
+  }
+
+  return changed;
+}
+
 void Document::PropagateStyleToViewport() {
   DCHECK(InStyleRecalc());
   HTMLElement* body = this->body();
@@ -2352,15 +2387,6 @@ void Document::PropagateStyleToViewport() {
       ComputedStyle::Clone(viewport_style);
   bool changed = false;
 
-#define PROPAGATE_FROM(source, getter, setter, initial) \
-  PROPAGATE_VALUE(source ? source->getter() : initial, getter, setter);
-
-#define PROPAGATE_VALUE(value, getter, setter)     \
-  if ((new_viewport_style->getter()) != (value)) { \
-    new_viewport_style->setter(value);             \
-    changed = true;                                \
-  }
-
   // Writing mode and direction
   {
     const ComputedStyle* direction_style =
@@ -2440,23 +2466,9 @@ void Document::PropagateStyleToViewport() {
       }
     }
 
-    // TODO(954423, 952711): scroll-snap-* and overscroll-behavior (and most
-    // likely overflow-anchor) should be propagated from the document element
-    // and not the viewport defining element.
-
-    // We only propagate the properties related to snap container since viewport
-    // defining element cannot be a snap area.
-    PROPAGATE_FROM(overflow_style, GetScrollSnapType, SetScrollSnapType,
-                   cc::ScrollSnapType());
-    PROPAGATE_FROM(overflow_style, ScrollPaddingTop, SetScrollPaddingTop,
-                   Length());
-    PROPAGATE_FROM(overflow_style, ScrollPaddingRight, SetScrollPaddingRight,
-                   Length());
-    PROPAGATE_FROM(overflow_style, ScrollPaddingBottom, SetScrollPaddingBottom,
-                   Length());
-    PROPAGATE_FROM(overflow_style, ScrollPaddingLeft, SetScrollPaddingLeft,
-                   Length());
-
+    // TODO(954423, 952711): overscroll-behavior (and most likely
+    // overflow-anchor) should be propagated from documet element and not the
+    // viewport defining element.
     PROPAGATE_FROM(overflow_style, OverscrollBehaviorX, SetOverscrollBehaviorX,
                    EOverscrollBehavior::kAuto);
     PROPAGATE_FROM(overflow_style, OverscrollBehaviorY, SetOverscrollBehaviorY,
@@ -2533,6 +2545,9 @@ void Document::PropagateStyleToViewport() {
                    false);
   }
 
+  changed |= PropagateScrollSnapStyleToViewport(*this, document_element_style,
+                                                new_viewport_style);
+
   if (changed) {
     new_viewport_style->UpdateFontOrientation();
     GetLayoutView()->SetStyle(new_viewport_style);
@@ -2548,10 +2563,9 @@ void Document::PropagateStyleToViewport() {
         scrollable_area->VerticalScrollbar()->StyleChanged();
     }
   }
-
+}
 #undef PROPAGATE_VALUE
 #undef PROPAGATE_FROM
-}
 
 #if DCHECK_IS_ON()
 static void AssertLayoutTreeUpdated(Node& root) {

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

@@ -211,9 +211,11 @@ TEST_F(ScrollSnapTest, SnapWhenBodyViewportDefining) {
   request.Complete(R"HTML(
     <!DOCTYPE html>
     <style>
+    html {
+      scroll-snap-type: both mandatory;
+    }
     body {
       overflow: scroll;
-      scroll-snap-type: both mandatory;
       height: 300px;
       width: 300px;
       margin: 0px;

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

@@ -50,18 +50,20 @@ TEST_F(ScrollIntoViewTest, InstantScroll) {
   ASSERT_EQ(Window().scrollY(), content->OffsetTop());
 }
 
-TEST_F(ScrollIntoViewTest, ScrollPaddingOnBodyViewportDefining) {
+TEST_F(ScrollIntoViewTest, ScrollPaddingOnDocumentElWhenBodyDefinesViewport) {
   v8::HandleScope HandleScope(v8::Isolate::GetCurrent());
   WebView().MainFrameWidget()->Resize(WebSize(300, 300));
   SimRequest request("https://example.com/test.html", "text/html");
   LoadURL("https://example.com/test.html");
   request.Complete(R"HTML(
       <style>
+      html {
+        scroll-padding: 10px;
+      }
       body {
         margin: 0px;
         height: 300px;
         overflow: scroll;
-        scroll-padding: 10px;
       }
       </style>
       <div id='space' style='height: 1000px'></div>
@@ -79,7 +81,8 @@ TEST_F(ScrollIntoViewTest, ScrollPaddingOnBodyViewportDefining) {
   ASSERT_EQ(Window().scrollY(), target->OffsetTop() - 10);
 }
 
-TEST_F(ScrollIntoViewTest, ScrollPaddingOnHtmlViewportDefining) {
+TEST_F(ScrollIntoViewTest,
+       ScrollPaddingOnDocumentElWhenDocumentElDefinesViewport) {
   v8::HandleScope HandleScope(v8::Isolate::GetCurrent());
   WebView().MainFrameWidget()->Resize(WebSize(300, 300));
   SimRequest request("https://example.com/test.html", "text/html");
@@ -108,7 +111,7 @@ TEST_F(ScrollIntoViewTest, ScrollPaddingOnHtmlViewportDefining) {
   ASSERT_EQ(Window().scrollY(), target->OffsetTop() - 10);
 }
 
-TEST_F(ScrollIntoViewTest, ScrollPaddingBodyOverflowHtmlViewportDefining) {
+TEST_F(ScrollIntoViewTest, ScrollPaddingOnBodyWhenDocumentElDefinesViewport) {
   v8::HandleScope HandleScope(v8::Isolate::GetCurrent());
   WebView().MainFrameWidget()->Resize(WebSize(300, 300));
   SimRequest request("https://example.com/test.html", "text/html");

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

@@ -31,15 +31,14 @@ static LayoutBox* FindSnapContainer(const LayoutBox& snap_area) {
   // 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* viewport_defining_element =
-      snap_area.GetDocument().ViewportDefiningElement();
+  Element* document_element = snap_area.GetDocument().documentElement();
   LayoutBox* box = snap_area.ContainingBlock();
   while (box && !box->HasOverflowClip() && !box->IsLayoutView() &&
-         box->GetNode() != viewport_defining_element)
+         box->GetNode() != document_element)
     box = box->ContainingBlock();
 
-  // If we reach to viewportDefiningElement then we dispatch to viewport
-  if (box && box->GetNode() == viewport_defining_element)
+  // If we reach to document element then we dispatch to viewport
+  if (box && box->GetNode() == document_element)
     return snap_area.GetDocument().GetLayoutView();
 
   return box;
@@ -52,11 +51,11 @@ void SnapCoordinator::SnapContainerDidChange(LayoutBox& snap_container,
     return;
   }
 
-  // Scroll snap properties have no effect on the viewport defining element
-  // instead they are propagated to (See Document::PropagateStyleToViewport) and
-  // handled by the LayoutView.
+  // Scroll snap properties have no effect on the document element instead they
+  // are propagated to (See Document::PropagateStyleToViewport) and handled by
+  // the LayoutView.
   if (snap_container.GetNode() ==
-      snap_container.GetDocument().ViewportDefiningElement())
+      snap_container.GetDocument().documentElement())
     return;
 
   bool is_scroll_container =

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

@@ -213,6 +213,38 @@ TEST_F(SnapCoordinatorTest, UpdateStyleForSnapElement) {
   EXPECT_EQ(1U, SizeOfSnapAreas(SnapContainer()));
 }
 
+TEST_F(SnapCoordinatorTest, ViewpoertScrollSnapStyleComesFromDocumentElement) {
+  SetHTML(R"HTML(
+    <style>
+    :root {
+      scroll-snap-type: both mandatory;
+    }
+    body {
+     scroll-snap-type: none;
+    }
+    </style>
+    <body>
+    </body>
+    )HTML");
+  UpdateAllLifecyclePhasesForTest();
+
+  Element* body = GetDocument().body();
+  EXPECT_EQ(body, GetDocument().ViewportDefiningElement());
+
+  SnapCoordinator* snap_coordinator = GetDocument().GetSnapCoordinator();
+  base::Optional<cc::SnapContainerData> viewport_data =
+      snap_coordinator->GetSnapContainerData(*GetDocument().GetLayoutView());
+  EXPECT_TRUE(viewport_data.has_value());
+  EXPECT_EQ(viewport_data.value().scroll_snap_type(),
+            cc::ScrollSnapType(false, cc::SnapAxis::kBoth,
+                               cc::SnapStrictness::kMandatory));
+
+  base::Optional<cc::SnapContainerData> body_data =
+      snap_coordinator->GetSnapContainerData(*body->GetLayoutBox());
+
+  EXPECT_FALSE(body_data.has_value());
+}
+
 TEST_F(SnapCoordinatorTest, LayoutViewCapturesWhenBodyElementViewportDefining) {
   SetHTML(R"HTML(
     <style>
@@ -327,6 +359,8 @@ TEST_F(SnapCoordinatorTest,
   // should capture snap points defined on it as opposed to layout view.
   Element& body = *GetDocument().body();
   EXPECT_EQ(2U, SizeOfSnapAreas(body));
+  EXPECT_EQ(0U, SizeOfSnapAreas(*GetDocument().documentElement()));
+  EXPECT_EQ(0U, SizeOfSnapAreas(GetDocument()));
 }
 
 #define EXPECT_EQ_CONTAINER(expected, actual)                          \
@@ -416,9 +450,11 @@ TEST_F(SnapCoordinatorTest, ScrolledSnapDataCalculation) {
 TEST_F(SnapCoordinatorTest, ScrolledSnapDataCalculationOnViewport) {
   SetHTML(R"HTML(
     <style>
+    :root {
+      scroll-snap-type: both mandatory;
+    }
     body {
       margin: 0px;
-      scroll-snap-type: both mandatory;
       overflow: scroll;
     }
     #container {

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

@@ -1303,9 +1303,11 @@ TEST_P(PaintPropertyTreeUpdateTest, EnsureSnapContainerData) {
   SetBodyInnerHTML(R"HTML(
     <!DOCTYPE html>
     <style>
+    html {
+      scroll-snap-type: both proximity;
+    }
     body {
       overflow: scroll;
-      scroll-snap-type: both proximity;
       height: 300px;
       width: 300px;
       margin: 0px;
@@ -1325,7 +1327,6 @@ TEST_P(PaintPropertyTreeUpdateTest, EnsureSnapContainerData) {
       height: 200px;
       scroll-snap-align: start;
     }
-
     </style>
 
     <div id="container">

third_party/blink/web_tests/external/wpt/css/css-scroll-snap/scroll-snap-type-on-root-element-expected.txt

-This is a testharness.js-based test.
-FAIL The scroll-snap-type on the root element is applied assert_equals: expected 515 but got 800
-FAIL The writing-mode (vertical-lr) on the body is used assert_equals: inline should snap expected 515 but got 800
-FAIL The writing-mode (horizontal-tb) on the body is used  assert_equals: inline should snap expected 100 but got 200
-Harness: the test ran to completion.
-

third_party/blink/web_tests/fast/scroll-snap/snap-scrolls-visual-viewport.html

@@ -3,9 +3,11 @@
 <script src="../../resources/testharness.js"></script>
 <script src="../../resources/testharnessreport.js"></script>
 <style>
+html {
+  scroll-snap-type: both proximity;
+}
 body {
   overflow: scroll;
-  scroll-snap-type: both proximity;
   height: 300px;
   width: 300px;
   margin: 0px;