[css-scroll-snap] Skip renderers with no element in updateSnapOffsets…

…ForScrollableArea https://bugs.webkit.org/show_bug.cgi?id=263093 rdar://111579277 Reviewed by Simon Fraser. We should skip renderers in updateSnapOffsetsForScrollableArea if they have no related element. For generated content we do generate an associated element as well, so the only time a renderer's element is null is for anonymous renderers (as specified in RenderElement.h as well), which we shouldn't snap to. * Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp: (WebCore::updateSnapOffsetsForScrollableArea): Canonical link: https://commits.webkit.org/275523@main
WebKit · Mar 1, 2024 · 2f88930 · 2f88930
1 parent e7c5034
commit 2f88930
Show file tree

Hide file tree

Showing 4 changed files with 100 additions and 4 deletions.
diff --git a/...w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-both-pseudo-expected.txt b/...w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-both-pseudo-expected.txt
@@ -0,0 +1,3 @@
+
+PASS Only snap to visible areas in the case where taking the closest snap point of   each axis does not snap to a visible area
+
diff --git a/...mported/w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-both-pseudo.html b/...mported/w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-both-pseudo.html
@@ -0,0 +1,93 @@
+<!DOCTYPE html>
+<title>
+  Snap to a visible area only even when there is a closer snap point for an area
+  that is closer but not visible (using both axes snap type), where the relevant 
+  snap areas are pseudo-elements
+</title>
+<link rel="help" href="https://drafts.csswg.org/css-scroll-snap-1/#snap-scope"/>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<style>
+
+body, html { height: 100%; }
+
+div {
+  position: absolute;
+  margin: 0px;
+}
+
+#scroller {
+  height: 600px;
+  width: 600px;
+  overflow: scroll;
+  scroll-snap-type: both mandatory;
+}
+
+#space {
+  width: 2000px;
+  height: 2000px;
+}
+
+.snap {
+  width: 200px;
+  height: 200px;
+  background-color: blue;
+  scroll-snap-align: start;
+}
+
+#left-top {
+  left: 0px;
+  top: 0px;
+}
+
+#left-top::before {
+  position: absolute;
+  margin: 0px;
+  content: "";
+
+  display:block;
+
+  left: 0px;
+  top: 800px;
+  width: 200px;
+  height: 200px;
+  background-color: yellow;
+  scroll-snap-align: start;
+}
+
+#left-top::after {
+  position: absolute;
+  margin: 0px;
+  content: "";
+
+  display:block;
+
+  left: 800px;
+  top: 0px;
+  width: 200px;
+  height: 200px;
+  background-color: yellow;
+  scroll-snap-align: start;
+
+}
+
+</style>
+<div id="scroller">
+  <div id="space"></div>
+  <div id="left-top" class="snap"></div>
+</div>
+<script>
+test(t => {
+  const scroller = document.getElementById("scroller");
+  scroller.scrollTo(0, 0);
+  assert_equals(scroller.scrollLeft, 0);
+  assert_equals(scroller.scrollTop, 0);
+  scroller.scrollTo(500, 600);
+  assert_equals(scroller.scrollLeft, 0);
+  assert_equals(scroller.scrollTop, 800);
+  scroller.scrollTo(600, 500);
+  assert_equals(scroller.scrollLeft, 800);
+  assert_equals(scroller.scrollTop, 0);
+}, 'Only snap to visible areas in the case where taking the closest snap point of \
+  each axis does not snap to a visible area');
+</script>
diff --git a/LayoutTests/platform/mac/TestExpectations b/LayoutTests/platform/mac/TestExpectations
@@ -2524,7 +2524,6 @@ webkit.org/b/264001 [ Debug ] imported/w3c/web-platform-tests/requestidlecallbac
 
 # webkit.org/b/264419 ([ Sonoma 14.1+ ] Multiple tests are constantly failing following update from 14.0.)
 webkit.org/b/264544 [ Sonoma+ ] css3/filters/effect-opacity-hw.html [ ImageOnlyFailure ]
-webkit.org/b/263783 [ Sonoma+ ] fast/scrolling/scroll-snap-crash.html [ Skip ]
 
 webkit.org/b/264177 requestidlecallback/requestidlecallback-deadline-shortened-by-rendering-update.html [ Pass Failure ]
 

diff --git a/Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp b/Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp
@@ -347,7 +347,7 @@ void updateSnapOffsetsForScrollableArea(ScrollableArea& scrollableArea, const Re
     auto scrollSnapPort = computeScrollSnapPortOrAreaRect(viewportRectInBorderBoxCoordinates, scrollingElementStyle.scrollPadding(), InsetOrOutset::Inset);
     LOG_WITH_STREAM(ScrollSnap, stream << "Computing scroll snap offsets for " << scrollableArea << " in snap port " << scrollSnapPort);
     for (auto& child : boxesWithScrollSnapPositions) {
-        if (child.enclosingScrollableContainerForSnapping() != &scrollingElementBox)
+        if (child.enclosingScrollableContainerForSnapping() != &scrollingElementBox || !child.element())
             continue;
 
         // The bounds of the child element's snap area, where the top left of the scrolling container's border box is the origin.
@@ -361,7 +361,7 @@ void updateSnapOffsetsForScrollableArea(ScrollableArea& scrollableArea, const Re
             scrollSnapArea.moveBy(scrollPosition);
 
         scrollSnapArea = computeScrollSnapPortOrAreaRect(scrollSnapArea, child.style().scrollMargin(), InsetOrOutset::Outset);
-        LOG_WITH_STREAM(ScrollSnap, stream << "    Considering scroll snap target area " << scrollSnapArea);
+        LOG_WITH_STREAM(ScrollSnap, stream << "    Considering scroll snap target area " << scrollSnapArea << " scroll snap id: " << child.element()->identifier() << " element: " << *child.element());
         auto alignment = child.style().scrollSnapAlign();
         auto stop = child.style().scrollSnapStop();
 
@@ -388,8 +388,9 @@ void updateSnapOffsetsForScrollableArea(ScrollableArea& scrollableArea, const Re
         snapAreas.append(scrollSnapAreaAsOffsets);
 
         auto isFocused = child.element() ? focusedElement == child.element() : false;
-        auto identifier = child.element() ? child.element()->identifier() : ObjectIdentifier<ElementIdentifierType>(0);
+        auto identifier = child.element()->identifier();
         snapAreasIDs.append(identifier);
+
         if (snapsHorizontally) {
             auto absoluteScrollXPosition = computeScrollSnapAlignOffset(scrollSnapArea.x(), scrollSnapArea.maxX(), xAlign, areaXAxisFlipped) - computeScrollSnapAlignOffset(scrollSnapPort.x(), scrollSnapPort.maxX(), xAlign, areaXAxisFlipped);
             auto absoluteScrollOffset = clampTo<int>(scrollableArea.scrollOffsetFromPosition({ roundToInt(absoluteScrollXPosition), 0 }).x(), 0, maxScrollOffset.x());