Cherry-pick 2f88930. rdar://111579277

[css-scroll-snap] Skip renderers with no element in updateSnapOffsetsForScrollableArea 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 Identifier: 272448.677@safari-7618-branch
WebKit · Mar 4, 2024 · ba17353 · ba17353
1 parent 3546fe5
commit ba17353
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
@@ -2554,7 +2554,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());