Changeset 281189 in webkit

Timestamp:

Aug 18, 2021 8:59:48 AM (11 months ago)

Author:

Martin Robinson

Message:

[css-scroll-snap] Don't snap to offscreen snap areas in unidirectional scrolls
​https://bugs.webkit.org/show_bug.cgi?id=227949
<rdar://problem/80895783>

Reviewed by Frédéric Wang.

LayoutTests/imported/w3c:

• web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-both-expected.txt: This bidirectional

scrolling test no longer snaps because we don't have support for choosing between two candidates
properly yet.

• web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-x-axis-expected.txt: Updated to show newly passing test.
• web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-y-axis-expected.txt: Ditto.

Source/WebCore:

No new tests. This is covered by two existing WPT tests.

• page/scrolling/ScrollSnapOffsetsInfo.cpp:

(WebCore::componentForAxis): Added this helper.
(WebCore::hasCompatibleSnapArea): Added this helper that checks to see if any of the snap areas
at a given scroll snap position are compatible with the viewport.
(WebCore::adjustPreviousAndNextForOnscreenSnapAreas): Adjusts the selected previous and next snap
positions by looking backward and forward for the first compatible snap position.
(WebCore::closestSnapOffsetWithInfoAndAxis): Use the new helper.

Location:

trunk

Files:

• LayoutTests/imported/w3c/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-both-expected.txt (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-x-axis-expected.txt (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-y-axis-expected.txt (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp (modified) (3 diffs)

trunk/LayoutTests/imported/w3c/ChangeLog

@@ +1 @@
+2021-08-18  Martin Robinson  <mrobinson@webkit.org>
+
+        [css-scroll-snap] Don't snap to offscreen snap areas in unidirectional scrolls
+        https://bugs.webkit.org/show_bug.cgi?id=227949
+        <rdar://problem/80895783>
+
+        Reviewed by Frédéric Wang.
+
+        * web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-both-expected.txt: This bidirectional
+        scrolling test no longer snaps because we don't have support for choosing between two candidates
+        properly yet.
+        * web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-x-axis-expected.txt: Updated to show newly passing test.
+        * web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-y-axis-expected.txt: Ditto.
+
 2021-08-17  Alex Christensen  <achristensen@webkit.org>
 

trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-both-expected.txt

@@ -1 +1 @@
 
-FAIL Only snap to visible areas in the case where taking the closest snap point of   each axis does not snap to a visible area assert_equals: expected 0 but got 800
+FAIL Only snap to visible areas in the case where taking the closest snap point of   each axis does not snap to a visible area assert_equals: expected 800 but got 0
 

trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-x-axis-expected.txt

@@ -1 +1 @@
 
-FAIL Only snap to visible area on X axis, even when the non-visible ones are closer assert_equals: expected 800 but got 700
+PASS Only snap to visible area on X axis, even when the non-visible ones are closer
 

trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-y-axis-expected.txt

@@ -1 +1 @@
 
-FAIL Only snap to visible area on Y axis, even when the non-visible ones are closer assert_equals: expected 800 but got 700
+PASS Only snap to visible area on Y axis, even when the non-visible ones are closer
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-08-18  Martin Robinson  <mrobinson@webkit.org>
+
+        [css-scroll-snap] Don't snap to offscreen snap areas in unidirectional scrolls
+        https://bugs.webkit.org/show_bug.cgi?id=227949
+        <rdar://problem/80895783>
+
+        Reviewed by Frédéric Wang.
+
+        No new tests. This is covered by two existing WPT tests.
+
+        * page/scrolling/ScrollSnapOffsetsInfo.cpp:
+        (WebCore::componentForAxis): Added this helper.
+        (WebCore::hasCompatibleSnapArea): Added this helper that checks to see if any of the snap areas
+        at a given scroll snap position are compatible with the viewport.
+        (WebCore::adjustPreviousAndNextForOnscreenSnapAreas): Adjusts the selected previous and next snap
+        positions by looking backward and forward for the first compatible snap position.
+        (WebCore::closestSnapOffsetWithInfoAndAxis): Use the new helper.
+
 2021-08-18  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp

@@ -101 +101 @@
 }
 
+template <typename UnitType, typename PointType>
+static UnitType componentForAxis(PointType point, ScrollEventAxis axis)
+{
+    return axis == ScrollEventAxis::Horizontal ? point.x() : point.y();
+}
+
+template <typename InfoType, typename UnitType, typename PointType, typename SizeType>
+static bool hasCompatibleSnapArea(const InfoType& info, const SnapOffset<UnitType>& snapOffset, ScrollEventAxis axis, const SizeType& viewportSize, PointType destinationOffsetPoint)
+{
+    auto otherAxis = axis == ScrollEventAxis::Horizontal ? ScrollEventAxis::Vertical : ScrollEventAxis::Horizontal;
+    auto scrollDestinationInOtherAxis = componentForAxis<UnitType, PointType>(destinationOffsetPoint, otherAxis);
+    auto viewportLengthInOtherAxis = axis == ScrollEventAxis::Horizontal ? viewportSize.height() : viewportSize.width();
+
+    return snapOffset.snapAreaIndices.findMatching([&] (auto index) {
+        const auto& snapArea = info.snapAreas[index];
+        auto [otherAxisMin, otherAxisMax] = rangeForAxis<UnitType>(snapArea, otherAxis);
+        return (scrollDestinationInOtherAxis + viewportLengthInOtherAxis) > otherAxisMin && scrollDestinationInOtherAxis < otherAxisMax;
+    }) != notFound;
+}
+
+template <typename InfoType, typename UnitType, typename PointType, typename SizeType>
+static void adjustPreviousAndNextForOnScreenSnapAreas(const InfoType& info, ScrollEventAxis axis, const SizeType& viewportSize, PointType destinationOffsetPoint, PotentialSnapPointSearchResult<UnitType>& searchResult)
+{
+    // hasCompatibleSnapArea needs to look at all compatible snap areas, which might be a large
+    // number for snap areas arranged in a grid. Since this might be expensive, this code tries
+    // to look at the mostly closest compatible snap areas first.
+    const auto& snapOffsets = info.offsetsForAxis(axis);
+    if (searchResult.previous) {
+        unsigned oldIndex = (*searchResult.previous).second;
+        searchResult.previous.reset();
+        for (unsigned offset = 0; offset <= oldIndex; offset++) {
+            unsigned index = oldIndex - offset;
+            const auto& snapOffset = snapOffsets[index];
+            if (hasCompatibleSnapArea(info, snapOffset, axis, viewportSize, destinationOffsetPoint)) {
+                searchResult.previous = { snapOffset.offset, index };
+                break;
+            }
+        }
+    }
+
+    if (searchResult.next) {
+        unsigned oldIndex = (*searchResult.next).second;
+        searchResult.next.reset();
+        for (unsigned index = oldIndex; index < snapOffsets.size(); index++) {
+            const auto& snapOffset = snapOffsets[index];
+            if (hasCompatibleSnapArea(info, snapOffset, axis, viewportSize, destinationOffsetPoint)) {
+                searchResult.next = { snapOffset.offset, index };
+                break;
+            }
+        }
+    }
+}
+
 template <typename InfoType, typename SizeType, typename LayoutType, typename PointType>
 static std::pair<LayoutType, std::optional<unsigned>> closestSnapOffsetWithInfoAndAxis(const InfoType& info, ScrollEventAxis axis, const SizeType& viewportSize, PointType scrollDestinationOffsetPoint, float velocity, std::optional<LayoutType> originalOffsetForDirectionalSnapping)
@@ -111 +164 @@
 
     auto viewportLength = axis == ScrollEventAxis::Horizontal ? viewportSize.width() : viewportSize.height();
-    auto [previous, next, snapStop, landedInsideSnapAreaThatConsumesViewport] = searchForPotentialSnapPoints(info, axis, viewportLength, scrollDestinationOffset, originalOffsetForDirectionalSnapping);
-    if (snapStop)
-        return *snapStop;
+    auto searchResult = searchForPotentialSnapPoints(info, axis, viewportLength, scrollDestinationOffset, originalOffsetForDirectionalSnapping);
+    if (searchResult.snapStop)
+        return *(searchResult.snapStop);
+
+    adjustPreviousAndNextForOnScreenSnapAreas<InfoType, LayoutType, PointType, SizeType>(info, axis, viewportSize, scrollDestinationOffsetPoint, searchResult);
+    auto& previous = searchResult.previous;
+    auto& next = searchResult.next;
 
     // From https://www.w3.org/TR/css-scroll-snap-1/#snap-overflow
@@ -121 +178 @@
     // that axis, is a valid snap position in that axis. The UA may use the specified alignment as a
     // more precise target for certain scroll operations (e.g. explicit paging)."
-    if (landedInsideSnapAreaThatConsumesViewport && (!previous || !next || ((*next).first - (*previous).first) >= viewportLength))
+    if (searchResult.landedInsideSnapAreaThatConsumesViewport && (!previous || !next || ((*next).first - (*previous).first) >= viewportLength))
         return pairForNoSnapping;