[css-scroll-snap-2] Resolve scroll-start-targets with reverse DOM order

Per CSSWG resolution[1], user agents should handle competing scroll-start-targets by scrolling to each target in reverse-DOM-order. This patch implements this and re-enables scroll-start-target tests. Note that, for now, we only need to store the scroll-start-target which is first-in-tree/DOM order and not the whole list. This is because when we scroll to the scroll-start-targets (in reverse DOM order) we're effectively only scrolling to the first-in-tree/DOM order scroll-start-target. Scrolling to each target will only have an effect when a "nearest" value is accepted by scroll-snap-align which will have the same effect as "nearest" in scrollIntoView. So for now we can keep track of just one element, the first in tree/DOM order. [1] w3c/csswg-drafts#10774 (comment) Bug: 40909052, 368038561 Change-Id: I029920bdb053ae258932e3e7579f28d8256baf70 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5881923 Commit-Queue: David Awogbemila <awogbemila@chromium.org> Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org> Cr-Commit-Position: refs/heads/main@{#1363980}
web-platform-tests · Oct 4, 2024 · e87b420 · e87b420
1 parent a18606c
commit e87b420
Show file tree

Hide file tree

Showing 14 changed files with 140 additions and 97 deletions.
diff --git a/...roll-snap-2/scroll-start-target/scroll-start-target-aligns-with-snap-align.tentative.html b/...roll-snap-2/scroll-start-target/scroll-start-target-aligns-with-snap-align.tentative.html
@@ -26,7 +26,7 @@
         width: 100px;
         height: 100px;
         background-color: pink;
-        scroll-start-target: auto auto;
+        scroll-start-target: auto;
         position: absolute;
         top: 400px;
         left: 400px;

diff --git a/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-display-toggled.tentative.html b/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-display-toggled.tentative.html
@@ -33,7 +33,7 @@
         width: 100px;
         height: 100px;
         background-color: pink;
-        scroll-start-target: auto auto;
+        scroll-start-target: auto;
       }
     </style>
     <div id="outer-container">

diff --git a/...css-scroll-snap-2/scroll-start-target/scroll-start-target-nested-container.tentative.html b/...css-scroll-snap-2/scroll-start-target/scroll-start-target-nested-container.tentative.html
@@ -14,6 +14,16 @@
   </head>
   <body>
     <style>
+      /**
+        large-space divs are added to simplify scrolling calculations
+        (i.e. expected offsets are just distances from scroller border top to
+        scrollee border top).
+      */
+      .large-space {
+        position: absolute;
+        height: 500vh;
+        width: 500vw;
+      }
       #space-filler {
         width: 500px;
         height: 500px;
@@ -23,13 +33,14 @@
         width: 400px;
         height: 400px;
         overflow: scroll;
+        position: relative;
         background-color: yellow;
       }
       #inner-container {
         top: 20px;
         left: 20px;
-        width: 300px;
-        height: 300px;
+        width: 400px;
+        height: 400px;
         overflow: visible;
         position: relative;
         background-color: blue;
@@ -38,12 +49,14 @@
         width: 100px;
         height: 100px;
         background-color: pink;
-        scroll-start-target: auto auto;
+        scroll-start-target: auto;
       }
     </style>
     <div id="outer-container">
+      <div class="large-space" id="large_space_outer"></div>
       <div id="space-filler"></div>
       <div id="inner-container">
+        <div class="large-space" id="large_space_inner"></div>
         <div id="space-filler"></div>
         <div id="target">
         </div>
@@ -61,110 +74,108 @@
       const inner_content_height = target_height + space_filler_height;
       const inner_container_height = inner_container.getBoundingClientRect().height;
 
+      let outer_to_target_scrolltop =
+      /*outer space-filler*/space_filler_height +
+      /* 20px top */  inner_scroller_top_offset +
+      /*inner space filler*/ space_filler_height;
+
+      let outer_to_inner_scrolltop =
+      /* outer space-filler*/ space_filler_height +
+      /* 20px top    */  inner_scroller_top_offset;
+
+      let inner_to_target_scrolltop =
+      /*inner space filler*/ space_filler_height;
+
       async function resetDisplay() {
-        return new Promise((resolve) => {
-          if (getComputedStyle(outer_container).display == "block" &&
-              getComputedStyle(inner_container).display == "block" &&
-              getComputedStyle(target).display == "block") {
-            resolve();
-          } else {
-            outer_container.style.display = "block";
-            inner_container.style.display = "block";
-            target.style.display = "block";
-            requestAnimationFrame(async () => {
-              await resetDisplay();
-              resolve();
-            });
-          }
-        });
+        outer_container.style.display = "block";
+        inner_container.style.display = "block";
+        target.style.display = "block";
+        return waitForAnimationFrames(2);
       }
 
-      let initial_expected_scroll_top = space_filler_height +
-          inner_scroller_top_offset + inner_content_height -
-          outer_container.clientHeight;
       promise_test(async (t) => {
         await resetDisplay();
-        assert_equals(outer_container.scrollTop, initial_expected_scroll_top,
+        assert_equals(outer_container.scrollTop, outer_to_target_scrolltop,
           "outer-container is scrolled to scroll-start-target");
 
+        // Remove large_space_outer so we can observe outer-container adjust to
+        // its new content height.
+        large_space_outer.style.display = "none";
         inner_container.style.display = "none";
         await waitForAnimationFrames(2);
 
         assert_equals(outer_container.scrollTop,
             space_filler_height - outer_container.clientHeight,
-            "outer-container has no content to scroll");
+            "outer-container has only the outer space filler to scroll");
 
+        large_space_outer.style.display = "initial";
         inner_container.style.display = "block";
         await waitForAnimationFrames(2);
 
-        assert_equals(outer_container.scrollTop, initial_expected_scroll_top,
+        assert_equals(outer_container.scrollTop, outer_to_target_scrolltop,
             "outer-scroller is updated as scroll-start-target reappears");
       }, "display:none scroll-start-target becomes display:block");
 
       promise_test(async (t) => {
         await waitForCompositorCommit();
         await resetDisplay();
-        assert_equals(outer_container.scrollTop, initial_expected_scroll_top,
+        assert_equals(outer_container.scrollTop, outer_to_target_scrolltop,
             "outer-container is scrolled to scroll-start-target");
 
+        // Remove large_space_outer so we can observe outer-container adjust to
+        // its new content height.
+        large_space_outer.style.display = "none";
         inner_container.style.overflow = "scroll";
         await waitForAnimationFrames(2);
 
         // inner-container has become a scroller and should be scrolled to
         // scroll-start-target.
         assert_equals(inner_container.scrollTop,
-            inner_content_height - inner_container.clientHeight,
-            "inner-container is fully scrolled to target");
+          inner_to_target_scrolltop,
+          "inner-container is fully scrolled to target");
         // outer-container should be adjusted to its new max scroll offset.
-        const scrollbar_width = outer_container.offsetHeight -
-            outer_container.clientHeight;
         assert_equals(outer_container.scrollTop,
             space_filler_height + inner_scroller_top_offset +
             inner_container_height - outer_container.clientHeight,
             "outer-container's overflowing content is only its direct " +
             "children");
 
+        large_space_outer.style.display = "initial";
         inner_container.style.overflow = "visible";
         await waitForAnimationFrames(2);
 
         assert_equals(inner_container.scrollTop, 0,
             "inner-container is no longer a scroll container");
-        assert_equals(outer_container.scrollTop, initial_expected_scroll_top,
+        assert_equals(outer_container.scrollTop, outer_to_target_scrolltop,
             "outer-scroller is the scroll container for target once again");
       }, "intermediate overflow:visible container becomes overflow:scroll");
 
       promise_test(async (t) => {
         // This test verifies that:
         // 1. when both the child and grandchild are scroll-start-targets, the
-        //    grandchild wins/is scrolled to.
+        //    first-in-tree-order (i.e. the child) wins/is scrolled to.
         // 2. if/when the grandchild stops being a scroll-start-target, the
         //    child (inner container) is scrolled to.
         await waitForCompositorCommit();
         await resetDisplay();
         t.add_cleanup(async () => {
-          target.style.scrollStartTarget = "auto auto";
+          inner_container.style.scrollStartTarget = "none";
           await waitForAnimationFrames(2);
         });
 
-        assert_equals(outer_container.scrollTop, initial_expected_scroll_top,
-            "outer-container is scrolled to scroll-start-target");
+        assert_equals(outer_container.scrollTop, outer_to_target_scrolltop,
+          "outer-container is scrolled to scroll-start-target");
         // Make the inner container a scroll-start-target.
-        inner_container.style.scrollStartTarget = "auto auto";
+        inner_container.style.scrollStartTarget = "auto";
         await waitForAnimationFrames(2);
 
         // The inner container has overflow: visible, so it's not the scroll
         // container of target.
-        assert_equals(outer_container.scrollTop, initial_expected_scroll_top,
-        "outer-container is still scrolled to inner scroll-start-target");
-
-        // Make target no longer a scroll-start-target. The outer container's
-        // scroll-start-target should now be the inner container.
-        target.style.scrollStartTarget = "none none";
-        await waitForAnimationFrames(2);
         assert_equals(outer_container.scrollTop,
-            space_filler_height + inner_scroller_top_offset,
-            "outer-container is scrolled to inner-container");
-      }, "inner scroll-start-target takes precedence over outer");
+          outer_to_inner_scrolltop,
+          "outer-container is scrolled to inner-container (which is before the" +
+          "inner scroll-start-target in tree order)");
+      }, "outer scroll-start-target takes precedence over inner");
 
       promise_test(async (t) => {
         // This test verifies that a child which is a scroller, is a
@@ -175,32 +186,33 @@
         await resetDisplay();
         t.add_cleanup(async () => {
           inner_container.style.overflow = "visible";
-          inner_container.style.scrollStartTarget = "none none";
+          inner_container.style.scrollStartTarget = "none";
           await waitForAnimationFrames(2);
         });
 
-        assert_equals(outer_container.scrollTop, initial_expected_scroll_top,
-            "outer-container is scrolled to scroll-start-target");
+        assert_equals(outer_container.scrollTop, outer_to_target_scrolltop,
+          "outer-container is scrolled to scroll-start-target");
 
         // Make the inner container a scroll-start-target.
-        inner_container.style.scrollStartTarget = "auto auto";
+        inner_container.style.scrollStartTarget = "auto";
         await waitForAnimationFrames(2);
 
-        assert_equals(outer_container.scrollTop, initial_expected_scroll_top,
-            "outer-container is still scrolled to inner scroll-start-target");
+        assert_equals(outer_container.scrollTop,
+          outer_to_inner_scrolltop,
+          "outer-container is still scrolled to inner scroll-container" +
+          "as it is a scroll-start-target and comes before #target in " +
+          "tree-order");
 
         // Make the inner container a scroller.
         inner_container.style.overflow = "scroll";
         await waitForAnimationFrames(2);
 
         assert_equals(outer_container.scrollTop,
-            space_filler_height + inner_scroller_top_offset +
-            inner_container.offsetHeight - outer_container.clientHeight,
-            "outer-container is scrolled to the inner container");
+          outer_to_inner_scrolltop,
+          "outer-container is scrolled to the inner container");
         assert_equals(inner_container.scrollTop,
-            space_filler_height + target.offsetHeight -
-            inner_container.clientHeight,
-            "inner-container is scrolled to target");
+          inner_to_target_scrolltop,
+          "inner-container is scrolled to target");
       }, "scroll containers can also be scroll-start-targets");
     </script>
   </body>

diff --git a/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-root.tentative.html b/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-root.tentative.html
@@ -32,7 +32,7 @@
       top: 60vh;
       left: 60vw;
       background-color: purple;
-      scroll-start-target: auto auto;
+      scroll-start-target: auto;
     }
 
     .bottom_right {

diff --git a/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-rtl.tentative.html b/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-rtl.tentative.html
@@ -34,7 +34,7 @@
     .center {
       top: 60%;
       background-color: purple;
-      scroll-start-target: auto auto;
+      scroll-start-target: auto;
     }
 
     .bottom_left {

diff --git a/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-span.tentative.html b/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-span.tentative.html
@@ -0,0 +1,53 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8">
+    <title> CSS Scroll Snap 2 Test: scroll-start-target with a &lt;span> element</title>
+    <link rel="help" href="https://drafts.csswg.org/css-scroll-snap-2/#scroll-start-target">
+    <script src="/resources/testharness.js"></script>
+    <script src="/resources/testharnessreport.js"></script>
+    <script src="/css/css-animations/support/testcommon.js"></script>
+  </head>
+  <body>
+    <style>
+      * {
+        margin: 0px;
+      }
+      .space {
+        width: 150%;
+        height: 150%;
+      }
+      .box {
+        height: 50px;
+        width: 50px;
+        border: solid 1px black;
+        background-color: turquoise;
+      }
+      .target {
+        scroll-start-target: auto;
+      }
+      .scroller {
+        overflow: scroll;
+        height: 100px;
+        width: 100px;
+      }
+    </style>
+    <div id="scroller" class="scroller">
+      <div class="space"></div>
+      <span id="target" class="target">
+        <div class="box"></div>
+      </span>
+      <div class="space"></div>
+    </div>
+    <script>
+      promise_test(async (t) => {
+        await waitForAnimationFrames(2);
+        const scroller = document.getElementById("scroller");
+        const target = document.getElementById("target");
+
+        assert_equals(scroller.scrollTop, target.offsetTop,
+          "scroller is initially scrolled to it <span> scroll-start-target");
+      }, "<span> scroll-start-target is honored");
+    </script>
+  </body>
+</html>
diff --git a/...ll-snap-2/scroll-start-target/scroll-start-target-with-anchor-navigation-inner-frame.html b/...ll-snap-2/scroll-start-target/scroll-start-target-with-anchor-navigation-inner-frame.html
@@ -23,7 +23,7 @@
     #middle_box {
       width: 100px;
       height: 60vh;
-      scroll-start-target: auto auto;
+      scroll-start-target: auto;
       background-color: purple;
     }
     #bottom_box {

diff --git a/...-2/scroll-start-target/scroll-start-target-with-hash-fragment-navigation-inner-frame.html b/...-2/scroll-start-target/scroll-start-target-with-hash-fragment-navigation-inner-frame.html
@@ -23,7 +23,7 @@
     #middle_box {
       width: 100px;
       height: 60vh;
-      scroll-start-target: auto auto;
+      scroll-start-target: auto;
       background-color: purple;
     }
     #bottom_box {

diff --git a/...css-scroll-snap-2/scroll-start-target/scroll-start-target-with-scroll-snap.tentative.html b/...css-scroll-snap-2/scroll-start-target/scroll-start-target-with-scroll-snap.tentative.html
@@ -42,7 +42,7 @@
       top: 200px;
       left: 200px;
       background-color: purple;
-      scroll-start-target: auto auto;
+      scroll-start-target: auto;
     }
 
     .bottom_right {

diff --git a/...roll-snap-2/scroll-start-target/scroll-start-target-with-scroll-start-root.tentative.html b/...roll-snap-2/scroll-start-target/scroll-start-target-with-scroll-start-root.tentative.html
@@ -36,7 +36,7 @@
       top: 60vh;
       left: 60vw;
       background-color: purple;
-      scroll-start-target: auto auto;
+      scroll-start-target: auto;
     }
 
     .bottom_right {

diff --git a/...ss-scroll-snap-2/scroll-start-target/scroll-start-target-with-scroll-start.tentative.html b/...ss-scroll-snap-2/scroll-start-target/scroll-start-target-with-scroll-start.tentative.html
@@ -43,7 +43,7 @@
       top: 200px;
       left: 200px;
       background-color: purple;
-      scroll-start-target: auto auto;
+      scroll-start-target: auto;
     }
 
     .bottom_right {

diff --git a/...-snap-2/scroll-start-target/scroll-start-target-with-text-fragment-navigation-target.html b/...-snap-2/scroll-start-target/scroll-start-target-with-text-fragment-navigation-target.html
@@ -20,7 +20,7 @@
     #middle_box {
       width: 100px;
       height: 60vh;
-      scroll-start-target: auto auto;
+      scroll-start-target: auto;
       background-color: purple;
     }
     #bottom_box {

diff --git a/...ap-2/scroll-start-target/scroll-start-target-with-user-programmatic-scroll.tentative.html b/...ap-2/scroll-start-target/scroll-start-target-with-user-programmatic-scroll.tentative.html
@@ -45,7 +45,7 @@
       top: 200px;
       left: 200px;
       background-color: purple;
-      scroll-start-target: auto auto;
+      scroll-start-target: auto;
     }
 
     .bottom_right {