Changeset 280527 in webkit

Timestamp:

Aug 2, 2021 1:54:16 AM (12 months ago)

Author:

Martin Robinson

Message:

[css-scroll-snap] Consider all snap areas at a given snap offset when snapping
​https://bugs.webkit.org/show_bug.cgi?id=228141

LayoutTests/imported/w3c:

Reviewed by Frédéric Wang.

• web-platform-tests/css/css-scroll-snap/overflowing-snap-areas-expected.txt:
• web-platform-tests/css/css-scroll-snap/overflowing-snap-areas.html:

Source/WebCore:

Reviewed by Frédéric Wang.

When deciding whether to snap to snap areas that overflow the snap port, don't
just consider the first snap area at a given snap offset. Instead, keep information
about all snap areas in the ScrolSnapOffsetInfo. In order to avoid iterating over
arrays of hundreds of rectangles, also record whether any of them are larger than
the viewport. This will avoid extra work on the most common usecase of large sets
of snap areas (gridded table layouts).

This change is tested by extending an existing WPT test.

• page/scrolling/ScrollSnapOffsetsInfo.cpp:

(WebCore::rangeForAxis): Added this helper.
(WebCore::searchForPotentialSnapPoints): When deciding if a particular snap area
overflows the snap port, look at all available snap areas instead of just the
first one.
(WebCore::updateSnapOffsetsForScrollableArea): Instead of only recording the first
snap area, record all of them in the list of snap area rectangles.
(WebCore::convertOffsetInfo): Convert the list of snap ares.

• page/scrolling/ScrollSnapOffsetsInfo.h:

(WebCore::operator==): Added this operator which is necessary now that snapAreaIndices
is a vector.

Source/WebKit:

Reviewed by Frédéric Wang.

• Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp:

(ArgumentCoder<SnapOffset<float>>::encode): Serialize new members.
(ArgumentCoder<SnapOffset<float>>::decode): Deserialize new members.

Location:

trunk

Files:

• LayoutTests/imported/w3c/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/overflowing-snap-areas-expected.txt (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/overflowing-snap-areas.html (modified) (4 diffs)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp (modified) (5 diffs)
• Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.h (modified) (2 diffs)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp (modified) (2 diffs)

trunk/LayoutTests/imported/w3c/ChangeLog

@@ +1 @@
+2021-08-02  Martin Robinson  <mrobinson@webkit.org>
+
+        [css-scroll-snap] Consider all snap areas at a given snap offset when snapping
+        https://bugs.webkit.org/show_bug.cgi?id=228141
+
+        Reviewed by Frédéric Wang.
+
+        * web-platform-tests/css/css-scroll-snap/overflowing-snap-areas-expected.txt:
+        * web-platform-tests/css/css-scroll-snap/overflowing-snap-areas.html:
+
 2021-07-31  Commit Queue  <commit-queue@webkit.org>
 

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

@@ -11 +11 @@
 PASS Don't snap back even if scrollTo tries to scroll to positions which are outside of the scroll range and if a snap target element covers the snaport
 PASS Snap to current scroll position on x as the area is covering x axis.However, we snap to the specified snap position on y as the area is not covering y axis.
+PASS snap to current scroll position on y as the area is covering y axis, even though that area is not the only scroll area at the same position.
+PASS snap to current scroll position on x as the area is covering x axis, even though that area is not the only scroll area at the same position.
 

trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/overflowing-snap-areas.html

@@ -25 +25 @@
 .target {
   scroll-snap-align: start;
+  height: 400px;
+  width: 400px;
 }
 .large-x {
   width: 3000px;
-  height: 400px;
   background-color: yellow;
 }
 .large-y {
-  width: 400px;
   height: 2000px;
-  background-color: green;
+  background-color: yellow;
 }
 .small {
@@ -73 +73 @@
 </div>
 
+<div class="scroller-x" id="overlapping-overflow" style="scroll-snap-type: both mandatory">
+  <div class="space"></div>
+  <div style="left: 200px; top: 200px;">
+    <div class="target small"></div>
+    <div class="target small"></div>
+    <div class="target small"></div>
+    <div class="target large-y large-x"></div>
+    <div class="target small"></div>
+    <div class="target small"></div>
+    <div class="target small"></div>
+  </div>
+</div>
+
 <script>
 var one_target_scroller = document.getElementById("one-target");
@@ -78 +91 @@
 var scroller_y = document.getElementById("y");
 var two_axes_scroller = document.getElementById("two-axes");
+var overlapping_scroller = document.getElementById("overlapping-overflow");
 
 test(() => {
@@ -166 +180 @@
    "However, we snap to the specified snap position on y as the area is not " +
    "covering y axis.");
+
+test(() => {
+  overlapping_scroller.scrollTo(200, 800);
+  assert_equals(overlapping_scroller.scrollLeft, 200);
+  assert_equals(overlapping_scroller.scrollTop, 800);
+}, "snap to current scroll position on y as the area is covering y axis, " +
+   "even though that area is not the only scroll area at the same position.");
+
+test(() => {
+  overlapping_scroller.scrollTo(800, 200);
+  assert_equals(overlapping_scroller.scrollLeft, 800);
+  assert_equals(overlapping_scroller.scrollTop, 200);
+}, "snap to current scroll position on x as the area is covering x axis, " +
+   "even though that area is not the only scroll area at the same position.");
 </script>
+</script>

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-08-02  Martin Robinson  <mrobinson@webkit.org>
+
+        [css-scroll-snap] Consider all snap areas at a given snap offset when snapping
+        https://bugs.webkit.org/show_bug.cgi?id=228141
+
+        Reviewed by Frédéric Wang.
+
+        When deciding whether to snap to snap areas that overflow the snap port, don't
+        just consider the first snap area at a given snap offset. Instead, keep information
+        about all snap areas in the ScrolSnapOffsetInfo. In order to avoid iterating over
+        arrays of hundreds of rectangles, also record whether any of them are larger than
+        the viewport. This will avoid extra work on the most common usecase of large sets
+        of snap areas (gridded table layouts).
+
+        This change is tested by extending an existing WPT test.
+
+        * page/scrolling/ScrollSnapOffsetsInfo.cpp:
+        (WebCore::rangeForAxis): Added this helper.
+        (WebCore::searchForPotentialSnapPoints): When deciding if a particular snap area
+        overflows the snap port, look at all available snap areas instead of just the
+        first one.
+        (WebCore::updateSnapOffsetsForScrollableArea): Instead of only recording the first
+        snap area, record all of them in the list of snap area rectangles.
+        (WebCore::convertOffsetInfo): Convert the list of snap ares.
+        * page/scrolling/ScrollSnapOffsetsInfo.h:
+        (WebCore::operator==): Added this operator which is necessary now that snapAreaIndices
+        is a vector.
+
 2021-08-01  Rob Buis  <rbuis@igalia.com>
 

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

@@ -41 +41 @@
 namespace WebCore {
 
+template <typename UnitType, typename RectType>
+static std::pair<UnitType, UnitType> rangeForAxis(RectType rect, ScrollEventAxis axis)
+{
+    return axis == ScrollEventAxis::Horizontal ? std::make_pair(rect.x(), rect.maxX()) : std::make_pair(rect.y(), rect.maxY());
+}
+
 template <typename UnitType>
 struct PotentialSnapPointSearchResult {
@@ -68 +74 @@
 
     for (unsigned i = 0; i < snapOffsets.size(); i++) {
+        if (!landedInsideSnapAreaThatConsumesViewport && snapOffsets[i].hasSnapAreaLargerThanViewport) {
+            for (auto snapAreaIndices : snapOffsets[i].snapAreaIndices) {
+                auto [snapAreaMin, snapAreaMax] = rangeForAxis<UnitType>(info.snapAreas[snapAreaIndices], axis);
+                if (snapAreaMin <= destinationOffset && snapAreaMax >= (destinationOffset + viewportLength)) {
+                    landedInsideSnapAreaThatConsumesViewport = true;
+                    break;
+                }
+            }
+        }
+
         UnitType potentialSnapOffset = snapOffsets[i].offset;
-
-        const auto& snapArea = info.snapAreas[snapOffsets[i].snapAreaIndex];
-        auto snapAreaMin = axis == ScrollEventAxis::Horizontal ? snapArea.x() : snapArea.y();
-        auto snapAreaMax = axis == ScrollEventAxis::Horizontal ? snapArea.maxX() : snapArea.maxY();
-        landedInsideSnapAreaThatConsumesViewport |= snapAreaMin <= destinationOffset && snapAreaMax >= (destinationOffset + viewportLength);
-
         if (potentialSnapOffset == destinationOffset)
             exact = std::make_pair(potentialSnapOffset, i);
@@ -220 +230 @@
     }
 
-    auto addOrUpdateStopForSnapOffset = [](HashMap<float, SnapOffset<LayoutUnit>>& offsets, SnapOffset<LayoutUnit> newOffset)
+    auto addOrUpdateStopForSnapOffset = [](HashMap<float, SnapOffset<LayoutUnit>>& offsets, LayoutUnit newOffset, ScrollSnapStop stop, bool hasSnapAreaLargerThanViewport, size_t snapAreaIndices)
     {
+        auto offset = offsets.ensure(newOffset, [&] {
+            return SnapOffset<LayoutUnit> { newOffset, stop, hasSnapAreaLargerThanViewport, { } };
+        });
+
         // If the offset already exists, we ensure that it has ScrollSnapStop::Always, when appropriate.
-        auto addResult = offsets.add(newOffset.offset, newOffset);
-        if (newOffset.stop == ScrollSnapStop::Always)
-            addResult.iterator->value.stop = ScrollSnapStop::Always;
+        if (stop == ScrollSnapStop::Always)
+            offset.iterator->value.stop = ScrollSnapStop::Always;
+
+        offset.iterator->value.hasSnapAreaLargerThanViewport |= hasSnapAreaLargerThanViewport;
+        offset.iterator->value.snapAreaIndices.append(snapAreaIndices);
     };
 
@@ -294 +310 @@
             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());
-            addOrUpdateStopForSnapOffset(horizontalSnapOffsetsMap, { absoluteScrollOffset, stop, snapAreas.size() - 1 });
+            addOrUpdateStopForSnapOffset(horizontalSnapOffsetsMap, absoluteScrollOffset, stop, scrollSnapAreaAsOffsets.width() > scrollSnapPort.width(), snapAreas.size() - 1);
         }
         if (snapsVertically) {
             auto absoluteScrollYPosition = computeScrollSnapAlignOffset(scrollSnapArea.y(), scrollSnapArea.maxY(), yAlign, areaYAxisFlipped) - computeScrollSnapAlignOffset(scrollSnapPort.y(), scrollSnapPort.maxY(), yAlign, areaYAxisFlipped);
             auto absoluteScrollOffset = clampTo<int>(scrollableArea.scrollOffsetFromPosition({ 0, roundToInt(absoluteScrollYPosition) }).y(), 0, maxScrollOffset.y());
-            addOrUpdateStopForSnapOffset(verticalSnapOffsetsMap, { absoluteScrollOffset, stop, snapAreas.size() - 1 });
+            addOrUpdateStopForSnapOffset(verticalSnapOffsetsMap, absoluteScrollOffset, stop, scrollSnapAreaAsOffsets.height() > scrollSnapPort.height(), snapAreas.size() - 1);
         }
 
@@ -349 +365 @@
         output.reserveInitialCapacity(input.size());
         for (auto& offset : input)
-            output.uncheckedAppend({ convertOffsetUnit(offset.offset, scaleFactor), offset.stop, offset.snapAreaIndex });
+            output.uncheckedAppend({ convertOffsetUnit(offset.offset, scaleFactor), offset.stop, offset.hasSnapAreaLargerThanViewport, offset.snapAreaIndices });
         return output;
     };

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

@@ -44 +44 @@
     T offset;
     ScrollSnapStop stop;
-    size_t snapAreaIndex;
+    bool hasSnapAreaLargerThanViewport;
+    Vector<size_t> snapAreaIndices;
 };
 
@@ -75 +76 @@
 };
 
+template<typename UnitType> inline bool operator==(const SnapOffset<UnitType>& a, const SnapOffset<UnitType>& b)
+{
+    return a.offset == b.offset && a.stop == b.stop && a.snapAreaIndices == b.snapAreaIndices;
+}
+
 using LayoutScrollSnapOffsetsInfo = ScrollSnapOffsetsInfo<LayoutUnit, LayoutRect>;
 using FloatScrollSnapOffsetsInfo = ScrollSnapOffsetsInfo<float, FloatRect>;

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2021-08-02  Martin Robinson  <mrobinson@igalia.com>
+
+        [css-scroll-snap] Consider all snap areas at a given snap offset when snapping
+        https://bugs.webkit.org/show_bug.cgi?id=228141
+
+        Reviewed by Frédéric Wang.
+
+        * Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp:
+        (ArgumentCoder<SnapOffset<float>>::encode): Serialize new members.
+        (ArgumentCoder<SnapOffset<float>>::decode): Deserialize new members.
+
 2021-08-01  Youenn Fablet  <youenn@apple.com>
 

trunk/Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp

@@ -528 +528 @@
     encoder << offset.offset;
     encoder << offset.stop;
-    encoder << offset.snapAreaIndex;
+    encoder << offset.hasSnapAreaLargerThanViewport;
+    encoder << offset.snapAreaIndices;
 }
 
@@ -537 +538 @@
     if (!decoder.decode(offset.stop))
         return false;
-    if (!decoder.decode(offset.snapAreaIndex))
+    if (!decoder.decode(offset.hasSnapAreaLargerThanViewport))
+        return false;
+    if (!decoder.decode(offset.snapAreaIndices))
         return false;
     return true;