Log 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.
Modified Paths
- trunk/LayoutTests/imported/w3c/ChangeLog
- trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-both-expected.txt
- trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-x-axis-expected.txt
- trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-y-axis-expected.txt
- trunk/Source/WebCore/ChangeLog
- trunk/Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp
Diff
Modified: trunk/LayoutTests/imported/w3c/ChangeLog (281188 => 281189)
--- trunk/LayoutTests/imported/w3c/ChangeLog 2021-08-18 15:25:04 UTC (rev 281188)
+++ trunk/LayoutTests/imported/w3c/ChangeLog 2021-08-18 15:59:48 UTC (rev 281189)
@@ -1,3 +1,17 @@
+2021-08-18 Martin Robinson <mrobin...@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 <achristen...@webkit.org>
Document load events should update timing in PerformanceNavigationTiming even after its instantiation
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-both-expected.txt (281188 => 281189)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-both-expected.txt 2021-08-18 15:25:04 UTC (rev 281188)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-both-expected.txt 2021-08-18 15:59:48 UTC (rev 281189)
@@ -1,3 +1,3 @@
-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
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-x-axis-expected.txt (281188 => 281189)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-x-axis-expected.txt 2021-08-18 15:25:04 UTC (rev 281188)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-x-axis-expected.txt 2021-08-18 15:59:48 UTC (rev 281189)
@@ -1,3 +1,3 @@
-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
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-y-axis-expected.txt (281188 => 281189)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-y-axis-expected.txt 2021-08-18 15:25:04 UTC (rev 281188)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/snap-to-visible-areas-y-axis-expected.txt 2021-08-18 15:59:48 UTC (rev 281189)
@@ -1,3 +1,3 @@
-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
Modified: trunk/Source/WebCore/ChangeLog (281188 => 281189)
--- trunk/Source/WebCore/ChangeLog 2021-08-18 15:25:04 UTC (rev 281188)
+++ trunk/Source/WebCore/ChangeLog 2021-08-18 15:59:48 UTC (rev 281189)
@@ -1,3 +1,21 @@
+2021-08-18 Martin Robinson <mrobin...@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 <cdu...@apple.com>
Crash under JSIntersectionObserverCallback::handleEvent()
Modified: trunk/Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp (281188 => 281189)
--- trunk/Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp 2021-08-18 15:25:04 UTC (rev 281188)
+++ trunk/Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp 2021-08-18 15:59:48 UTC (rev 281189)
@@ -100,6 +100,59 @@
return { previous, next, snapStop, landedInsideSnapAreaThatConsumesViewport };
}
+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)
{
@@ -110,10 +163,14 @@
return pairForNoSnapping;
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
// "If the snap area is larger than the snapport in a particular axis, then any scroll position
// in which the snap area covers the snapport, and the distance between the geometrically
@@ -120,7 +177,7 @@
// previous and subsequent snap positions in that axis is larger than size of the snapport in
// 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;
auto isNearEnoughToOffsetForProximity = [&](LayoutType candidateSnapOffset) {
_______________________________________________ webkit-changes mailing list webkit-changes@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-changes
