From 799e733383ef33c9f3f8bf121622dc708b4119a3 Mon Sep 17 00:00:00 2001 From: David Awogbemila Date: Tue, 12 Sep 2023 01:20:21 +0000 Subject: [PATCH] Choose closest covering position for large snap areas This patch modifies the logic to find snap positions so that when a scroller is scrolled to a position which only partially covers a snap area larger than itself, if it snaps to this large snap area, it snaps to the nearest boundary of that snap area where the snap area covers the snap port, rather than doing a jump to honor the specified scroll-snap-align. Below are a few notes about changes made to tests: snap-area-overflow-boundary.html is corrected so that both test cases correctly verify the scroll offset when snapped to the lower element. The scroll amount in the second test case is also adjusted to ensure it is enough to snap to the lower div. The expectations at the end of scroll-on-large-element-not-covering-snapport.tentative.html are updated to account for choosing a snap position different from the scroll-snap-align-specified position for snap areas larger than their snapports. scroll-snap-type-on-root-element.html is updated to account for scrollbar width since snapping to the right edge of the target is valid. This patch picks new programmatic scroll offsets for snap-to-visible-areas-margin-both.html so that the top-left of intersection of the right-top and left-bottom targets' scroll-margins is closer than the bottom-right of the intersection. scrollend-with-snap-on-fractional-offset.html and scroll-start-with-scroll-snap-tentative.html are adjusted to use snap area sizes that don't cover the snap port. (cherry picked from commit 0295a2c1847b35d139d22db0804ea23aa4975db2) Bug: 1420762 Change-Id: I72d18b8ca44ea5aef015e59b67e4de34a66d8a1e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4806971 Reviewed-by: Robert Flack Commit-Queue: David Awogbemila Cr-Original-Commit-Position: refs/heads/main@{#1193634} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4853949 Bot-Commit: Rubber Stamper Auto-Submit: David Awogbemila Reviewed-by: Steve Kobes Commit-Queue: Steve Kobes Cr-Commit-Position: refs/branch-heads/5993@{#188} Cr-Branched-From: 511350718e646be62331ae9d7213d10ec320d514-refs/heads/main@{#1192594} --- cc/base/features.cc | 4 + cc/base/features.h | 6 ++ cc/input/scroll_snap_data.cc | 16 +++- cc/input/scroll_snap_data_unittest.cc | 4 +- ...roll-start-with-scroll-snap.tentative.html | 22 ++++-- .../snap-area-overflow-boundary-expected.txt | 5 -- .../input/snap-area-overflow-boundary.html | 15 ++-- .../overflowing-snap-areas-expected.txt | 16 ---- ...ement-not-covering-snapport.tentative.html | 11 +-- .../scroll-snap-type-on-root-element.html | 13 +++- .../scroll-snap-writing-mode-000-ref.html | 45 ++++++++--- .../scroll-snap-writing-mode-000.html | 3 +- .../snap-into-covering-area.html | 77 +++++++++++++++++++ .../snap-to-visible-areas-margin-both.html | 8 +- ...ollend-with-snap-on-fractional-offset.html | 2 +- 15 files changed, 181 insertions(+), 66 deletions(-) delete mode 100644 third_party/blink/web_tests/external/wpt/css/css-scroll-snap/input/snap-area-overflow-boundary-expected.txt delete mode 100644 third_party/blink/web_tests/external/wpt/css/css-scroll-snap/overflowing-snap-areas-expected.txt create mode 100644 third_party/blink/web_tests/external/wpt/css/css-scroll-snap/snap-into-covering-area.html diff --git a/cc/base/features.cc b/cc/base/features.cc index 6a959dfee3ad19..1e658803a90b28 100644 --- a/cc/base/features.cc +++ b/cc/base/features.cc @@ -53,6 +53,10 @@ BASE_FEATURE(kScrollSnapCoveringUseNativeFling, "ScrollSnapCoveringUseNativeFling", base::FEATURE_ENABLED_BY_DEFAULT); +BASE_FEATURE(kScrollSnapPreferCloserCovering, + "ScrollSnapPreferCloserCovering", + base::FEATURE_ENABLED_BY_DEFAULT); + BASE_FEATURE(kHudDisplayForPerformanceMetrics, "HudDisplayForPerformanceMetrics", base::FEATURE_DISABLED_BY_DEFAULT); diff --git a/cc/base/features.h b/cc/base/features.h index 376b46fe66de15..41d105e8dd7f8f 100644 --- a/cc/base/features.h +++ b/cc/base/features.h @@ -47,6 +47,12 @@ CC_BASE_EXPORT BASE_DECLARE_FEATURE(kScrollSnapCoveringAvoidNestedSnapAreas); // allowing much more natural scrolling within these areas. CC_BASE_EXPORT BASE_DECLARE_FEATURE(kScrollSnapCoveringUseNativeFling); +// When enabled, if a snap container is snapping to a large snap area, it will +// snap to the closest covering position if it is closer than the aligned +// position. This avoid unnecessary jumps that attempt to honor the +// scroll-snap-align value. +CC_BASE_EXPORT BASE_DECLARE_FEATURE(kScrollSnapPreferCloserCovering); + // When enabled, cc will show blink's Web-Vital metrics inside its heads up // display. CC_BASE_EXPORT BASE_DECLARE_FEATURE(kHudDisplayForPerformanceMetrics); diff --git a/cc/input/scroll_snap_data.cc b/cc/input/scroll_snap_data.cc index aa9a952efaea01..3e07083b5cc85d 100644 --- a/cc/input/scroll_snap_data.cc +++ b/cc/input/scroll_snap_data.cc @@ -133,6 +133,15 @@ absl::optional SearchResultForDodgingRange( return result; } +bool CanCoverSnapportOnAxis(SearchAxis axis, + const gfx::RectF& container_rect, + const gfx::RectF& area_rect) { + return (axis == SearchAxis::kY && + area_rect.height() >= container_rect.height()) || + (axis == SearchAxis::kX && + area_rect.width() >= container_rect.width()); +} + } // namespace SnapSearchResult::SnapSearchResult(float offset, const gfx::RangeF& range) @@ -504,11 +513,10 @@ SnapContainerData::FindClosestValidAreaInternal( SnapSearchResult candidate = GetSnapSearchResult(axis, area); evaluate(candidate); - if (should_consider_covering && - IsSnapportCoveredOnAxis(axis, intended_position, area.rect)) { - // Since snap area will cover the snapport, we consider the intended - // position as a valid snap position. + (base::FeatureList::IsEnabled(features::kScrollSnapPreferCloserCovering) + ? CanCoverSnapportOnAxis(axis, rect_, area.rect) + : IsSnapportCoveredOnAxis(axis, intended_position, area.rect))) { if (absl::optional covering = FindCoveringCandidate(area, axis, candidate, intended_position)) { if (covering->snap_offset() == intended_position) { diff --git a/cc/input/scroll_snap_data_unittest.cc b/cc/input/scroll_snap_data_unittest.cc index dbb7d5cf74630e..66ed1692d9e721 100644 --- a/cc/input/scroll_snap_data_unittest.cc +++ b/cc/input/scroll_snap_data_unittest.cc @@ -798,11 +798,11 @@ TEST_F(ScrollSnapDataTest, CoveringWithOverlap1) { // Snap up, out of small_1. TestSnapPositionY(container, 2000, -150, Type::kCovered, 1800, 50, 1800); // Snap to small_2. - TestSnapPositionY(container, 1600, 600, Type::kAligned, 2300); + TestSnapPositionY(container, 1600, 601, Type::kAligned, 2300); // Scroll inside small_2. TestSnapPositionY(container, 2300, 50, Type::kCovered, 2350, 2300, 2400); // Snap out of small_2. - TestSnapPositionY(container, 2300, 150, Type::kCovered, 2600, 2600, 4750); + TestSnapPositionY(container, 2300, 200, Type::kCovered, 2600, 2600, 4750); // Snap up into small_2. TestSnapPositionY(container, 2700, -300, Type::kCovered, 2400, 2300, 2400); } diff --git a/third_party/blink/web_tests/external/wpt/css/css-scroll-snap-2/scroll-start/scroll-start-with-scroll-snap.tentative.html b/third_party/blink/web_tests/external/wpt/css/css-scroll-snap-2/scroll-start/scroll-start-with-scroll-snap.tentative.html index b813fcc750ef5b..8bdac300ba332c 100644 --- a/third_party/blink/web_tests/external/wpt/css/css-scroll-snap-2/scroll-start/scroll-start-with-scroll-snap.tentative.html +++ b/third_party/blink/web_tests/external/wpt/css/css-scroll-snap-2/scroll-start/scroll-start-with-scroll-snap.tentative.html @@ -17,21 +17,27 @@ } .spacer { - height: 100px; + height: 200px; width: 100px; } .scroller { - height: 100px; + height: 220px; width: 100px; overflow: scroll; - scroll-start: 200px; scroll-snap-type: both mandatory; } + #single_snap_scroller { + scroll-start: 100%; + } + #multi_snap_scroller { + scroll-start: 350px; + } + .snap_point { width: 100px; - height: 100px; + height: 200px; scroll-snap-align: start; } @@ -53,11 +59,11 @@ }, "snap overrides scroll-start position"); test((t) => { - // scroll-start sets the initial scroll offset to the top of the third - // snap_point, so the scroller snaps to the third snap_point. + // scroll-start sets the initial scroll offset to 350px which is closer to + // the third snap point than the second, so the scroller should snap to + // the third snap_point. assert_equals(multi_snap_scroller.scrollTop, - snap_point_1.getBoundingClientRect().height + - snap_point_2.getBoundingClientRect().height, + multi_snap_scroller.scrollHeight - multi_snap_scroller.clientHeight, "scroller snaps to snap point closer to start position."); }, "scroller snaps based on scroll-start position"); diff --git a/third_party/blink/web_tests/external/wpt/css/css-scroll-snap/input/snap-area-overflow-boundary-expected.txt b/third_party/blink/web_tests/external/wpt/css/css-scroll-snap/input/snap-area-overflow-boundary-expected.txt deleted file mode 100644 index 59ec6bf723c0a5..00000000000000 --- a/third_party/blink/web_tests/external/wpt/css/css-scroll-snap/input/snap-area-overflow-boundary-expected.txt +++ /dev/null @@ -1,5 +0,0 @@ -This is a testharness.js-based test. -FAIL Keyboard scrolling with vertical snap-area overflow assert_equals: End boundary of snap-area is valid snap target expected 39 but got 449 -FAIL Mouse-wheel scrolling with vertical snap-area overflow assert_equals: End boundary of snap-area is valid snap target expected 100 but got 0 -Harness: the test ran to completion. - diff --git a/third_party/blink/web_tests/external/wpt/css/css-scroll-snap/input/snap-area-overflow-boundary.html b/third_party/blink/web_tests/external/wpt/css/css-scroll-snap/input/snap-area-overflow-boundary.html index c497845263f544..1f00353e893045 100644 --- a/third_party/blink/web_tests/external/wpt/css/css-scroll-snap/input/snap-area-overflow-boundary.html +++ b/third_party/blink/web_tests/external/wpt/css/css-scroll-snap/input/snap-area-overflow-boundary.html @@ -3,6 +3,9 @@

Test passes if there is an orange square tucked into each blue corner without gaps, -and there is no red. +and there is no red, except for the large inverted cases which should have red +in the silver corner and smaller orange boxes in the blue corner.

@@ -87,6 +106,7 @@ +
@@ -165,3 +185,4 @@
+ diff --git a/third_party/blink/web_tests/external/wpt/css/css-scroll-snap/snap-after-initial-layout/scroll-snap-writing-mode-000.html b/third_party/blink/web_tests/external/wpt/css/css-scroll-snap/snap-after-initial-layout/scroll-snap-writing-mode-000.html index e5d3dd9358bdab..f44317c8585e87 100644 --- a/third_party/blink/web_tests/external/wpt/css/css-scroll-snap/snap-after-initial-layout/scroll-snap-writing-mode-000.html +++ b/third_party/blink/web_tests/external/wpt/css/css-scroll-snap/snap-after-initial-layout/scroll-snap-writing-mode-000.html @@ -78,7 +78,8 @@

Test passes if there is an orange square tucked into each blue corner without gaps, -and there is no red. + and there is no red, except for the large inverted cases which should have red + in the silver corner and smaller orange boxes in the blue corner.

diff --git a/third_party/blink/web_tests/external/wpt/css/css-scroll-snap/snap-into-covering-area.html b/third_party/blink/web_tests/external/wpt/css/css-scroll-snap/snap-into-covering-area.html new file mode 100644 index 00000000000000..2cc8741f5ef2e0 --- /dev/null +++ b/third_party/blink/web_tests/external/wpt/css/css-scroll-snap/snap-into-covering-area.html @@ -0,0 +1,77 @@ + + + + + + + + + + + + + + + +
+
+
+ +
+
+ + + + \ No newline at end of file diff --git a/third_party/blink/web_tests/external/wpt/css/css-scroll-snap/snap-to-visible-areas-margin-both.html b/third_party/blink/web_tests/external/wpt/css/css-scroll-snap/snap-to-visible-areas-margin-both.html index f148036a1d5d71..4c3c3c11a642b7 100644 --- a/third_party/blink/web_tests/external/wpt/css/css-scroll-snap/snap-to-visible-areas-margin-both.html +++ b/third_party/blink/web_tests/external/wpt/css/css-scroll-snap/snap-to-visible-areas-margin-both.html @@ -65,10 +65,14 @@ scroller.scrollTo(0, 0); assert_equals(scroller.scrollLeft, 0); assert_equals(scroller.scrollTop, 0); - scroller.scrollTo(500, 600); + // 750 and 650 are picked as those are closer to top left of the intersection + // (800, 800) of the snap areas where the browser should snap. This makes the + // intersection a closer snap option than a covering option that the browser + // might choose where the snapport is aligned on the bottom and right. + scroller.scrollTo(650, 750); assert_equals(scroller.scrollLeft, 800); assert_equals(scroller.scrollTop, 800); - scroller.scrollTo(600, 500); + scroller.scrollTo(750, 650); assert_equals(scroller.scrollLeft, 800); assert_equals(scroller.scrollTop, 800); }, 'Snap to area such that only the scroll margin from both axes\' areas are \ diff --git a/third_party/blink/web_tests/external/wpt/dom/events/scrolling/scrollend-with-snap-on-fractional-offset.html b/third_party/blink/web_tests/external/wpt/dom/events/scrolling/scrollend-with-snap-on-fractional-offset.html index 74dcb0f0bd068f..d1f50304add2ac 100644 --- a/third_party/blink/web_tests/external/wpt/dom/events/scrolling/scrollend-with-snap-on-fractional-offset.html +++ b/third_party/blink/web_tests/external/wpt/dom/events/scrolling/scrollend-with-snap-on-fractional-offset.html @@ -16,7 +16,7 @@ .box { scroll-snap-align: start; - width: 600px; + width: 400px; position: absolute; top: 200px; }