From e87b420137180382c1bbaacabb2a9b768622775f Mon Sep 17 00:00:00 2001 From: David Awogbemila Date: Thu, 3 Oct 2024 18:16:27 -0700 Subject: [PATCH] [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] https://github.com/w3c/csswg-drafts/issues/10774#issuecomment-2358920264 Bug: 40909052, 368038561 Change-Id: I029920bdb053ae258932e3e7579f28d8256baf70 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5881923 Commit-Queue: David Awogbemila Reviewed-by: Ian Kilpatrick Cr-Commit-Position: refs/heads/main@{#1363980} --- ...rget-aligns-with-snap-align.tentative.html | 2 +- ...tart-target-display-toggled.tentative.html | 2 +- ...art-target-nested-container.tentative.html | 126 ++++++++++-------- .../scroll-start-target-root.tentative.html | 2 +- .../scroll-start-target-rtl.tentative.html | 2 +- .../scroll-start-target-span.tentative.html | 53 ++++++++ ...et-with-anchor-navigation-inner-frame.html | 2 +- ...-hash-fragment-navigation-inner-frame.html | 2 +- ...art-target-with-scroll-snap.tentative.html | 2 +- ...rget-with-scroll-start-root.tentative.html | 2 +- ...rt-target-with-scroll-start.tentative.html | 2 +- ...-with-text-fragment-navigation-target.html | 2 +- ...th-user-programmatic-scroll.tentative.html | 2 +- .../scroll-start-target.tentative.html | 36 +---- 14 files changed, 140 insertions(+), 97 deletions(-) create mode 100644 css/css-scroll-snap-2/scroll-start-target/scroll-start-target-span.tentative.html diff --git a/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-aligns-with-snap-align.tentative.html b/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-aligns-with-snap-align.tentative.html index 6b133dea7d722b..3203e1e1a61593 100644 --- a/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-aligns-with-snap-align.tentative.html +++ b/css/css-scroll-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 index 527d7502678bab..a0224a568e5658 100644 --- 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; }
diff --git a/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-nested-container.tentative.html b/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-nested-container.tentative.html index 4574421c8cd9e4..e3a1df6e6a1b6a 100644 --- a/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-nested-container.tentative.html +++ b/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-nested-container.tentative.html @@ -14,6 +14,16 @@
+
+
@@ -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"); 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 index f2af38bbabcc21..860a592bb106a5 100644 --- 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 index 5a2fa0a93cad6f..e7766cc07a2c86 100644 --- 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 new file mode 100644 index 00000000000000..0985dcc2ee91f0 --- /dev/null +++ b/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-span.tentative.html @@ -0,0 +1,53 @@ + + + + + CSS Scroll Snap 2 Test: scroll-start-target with a <span> element + + + + + + + +
+
+ +
+
+
+
+ + + diff --git a/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-with-anchor-navigation-inner-frame.html b/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-with-anchor-navigation-inner-frame.html index bea0525ecd4181..de02dcf00f68ee 100644 --- a/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-with-anchor-navigation-inner-frame.html +++ b/css/css-scroll-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/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-with-hash-fragment-navigation-inner-frame.html b/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-with-hash-fragment-navigation-inner-frame.html index 9bf77363d3e60d..90629c4990f675 100644 --- a/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-with-hash-fragment-navigation-inner-frame.html +++ b/css/css-scroll-snap-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/css-scroll-snap-2/scroll-start-target/scroll-start-target-with-scroll-snap.tentative.html b/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-with-scroll-snap.tentative.html index 9cb66c01fcefeb..28bea40d7a0448 100644 --- a/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-with-scroll-snap.tentative.html +++ b/css/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/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-with-scroll-start-root.tentative.html b/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-with-scroll-start-root.tentative.html index af99595f254138..daf664aa7fbdf7 100644 --- a/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-with-scroll-start-root.tentative.html +++ b/css/css-scroll-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/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-with-scroll-start.tentative.html b/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-with-scroll-start.tentative.html index a37c8312887125..a08daad76db6dd 100644 --- a/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-with-scroll-start.tentative.html +++ b/css/css-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/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-with-text-fragment-navigation-target.html b/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-with-text-fragment-navigation-target.html index da53e7a566d142..80cbb55fb2730d 100644 --- a/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-with-text-fragment-navigation-target.html +++ b/css/css-scroll-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/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-with-user-programmatic-scroll.tentative.html b/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-with-user-programmatic-scroll.tentative.html index 2d487e9b85cde8..6bcf103253d6c5 100644 --- a/css/css-scroll-snap-2/scroll-start-target/scroll-start-target-with-user-programmatic-scroll.tentative.html +++ b/css/css-scroll-snap-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 { diff --git a/css/css-scroll-snap-2/scroll-start-target/scroll-start-target.tentative.html b/css/css-scroll-snap-2/scroll-start-target/scroll-start-target.tentative.html index 2e679c37396646..1fcb3cbbe40ae9 100644 --- a/css/css-scroll-snap-2/scroll-start-target/scroll-start-target.tentative.html +++ b/css/css-scroll-snap-2/scroll-start-target/scroll-start-target.tentative.html @@ -37,12 +37,8 @@ background-color: red; } - .target_for_x_and_y { - scroll-start-target: auto auto; - } - - .target_for_x { - scroll-start-target: none auto; + .target { + scroll-start-target: auto; } .center { @@ -57,38 +53,20 @@ background-color: yellow; } -
+
-
-
-
-
-
-
-
-
+
+