corysanin/tubestation
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Bug 1922904 - Fix bug 1780701 in a different approach. r=botond

Browse Source 
In bug 1780701, we changed ScrollSnapUtils::GetSnapPointForDestination not to
return a valid SnapDestination if the snap position is equal to the original
scroll destination. But it has a bad side-effect on wheel scrolling.

While wheel scrolling, there are multiple wheel inputs comming from OS, in every
wheel input we do use the destination of the animation running at that moment
[1].  So if in the first wheel input we created a smooth animation for snapping
to the SnapDestination, and then in the second wheel input
GetSnapPointForDestination doesn't return any valid SnapDestination so that the
smooth animation is clobbered [2].

To avoid this bad side-effect, we do skip scroll snapping in the call site,
ScrollContainerFrame::ScrollSnap, if the snap position equals to the original
scroll destination.

[1] https://searchfox.org/mozilla-central/rev/53e8dfd81c32f1ab275516406ec06a68136aaef0/gfx/layers/apz/src/AsyncPanZoomController.cpp#2585-2586
[2] https://searchfox.org/mozilla-central/rev/53e8dfd81c32f1ab275516406ec06a68136aaef0/gfx/layers/apz/src/AsyncPanZoomController.cpp#2603

Differential Revision: https://phabricator.services.mozilla.com/D227409
This commit is contained in:
Hiroyuki Ikezoe
2024-11-02 06:37:14 +00:00
parent 9ca48497fc
commit e38ad9d39d
6 changed files with 143 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files

132
gfx/layers/apz/test/mochitest/helper_bug1922904.html Normal file
View File

@@ -0,0 +1,132 @@
<!DOCTYPE HTML>
<html>
<head>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1">
<script src="apz_test_utils.js"></script>
<script src="apz_test_native_event_utils.js"></script>
<script src="/tests/SimpleTest/paint_listener.js"></script>
<style>
body {
position: fixed;
width: 100%;
height: 100%;
}
.container {
display: flex;
gap: 40px;
height: 100%;
overflow-y: hidden;
overflow-x: auto;
scroll-snap-type: x mandatory;
}
section {
display: flex;
flex-direction: column;
min-width: 92vw;
padding: 1rem;
gap: 30px;
background-color: grey;
height: 100%;
scroll-snap-align: none center;
scroll-snap-stop: always;
overflow-x: hidden;
overflow-y: auto;
}
section div {
width: 100%;
}
article:nth-child(n) {
background-color: turquoise;
}
article:nth-child(2n) {
background-color: tomato;
}
article:nth-child(3n) {
background-color: purple;
}
article {
width: 100%;
height: 75vh;
}
</style>
</head>
<body>
<div class="container">
<section>
<div>
<article>
Some stuff 1
</article>
<article>
Some stuff 2
</article>
<article>
Some stuff 3
</article>
</div>
</section>
<section>
<div>
<article>
Some stuff 3
</article>
<article>
Some stuff 4
</article>
<article>
Some stuff 5
</article>
</div>
</section>
</div>
</body>
<script type="application/javascript">
async function test() {
const container = document.querySelector(".container");
let scrollendPromise = promiseOneEvent(container, "scrollend");
// Send two horizontal wheel events quickly.
synthesizeNativeWheel(container, 50, 50, NativePanHandler.delta, 0);
synthesizeNativeWheel(container, 50, 50, NativePanHandler.delta, 0);
// Wait for the end of the smooth scrolling triggered by above wheel events.
await scrollendPromise;
// Store the destination of the smooth scrolling temporarily.
const endOfScrollPosition = container.scrollLeft;
// Try to do an instant scroll to the left edge of the scroll container,
// there's a scroll snap point.
container.scrollTo(container.clientWidth, 0);
is(endOfScrollPosition, container.scrollLeft);
}
if (getPlatform() == "android") {
ok(true, "Skipping test because native wheel events are not supported on Android");
subtestDone();
} else if (getPlatform() == "linux") {
ok(true, "Skipping test on Linux because of bug 1889039. ");
subtestDone();
} else {
waitUntilApzStable()
.then(test)
.then(subtestDone, subtestFailed);
}
</script>
</html>

2
gfx/layers/apz/test/mochitest/test_group_scroll_snap.html
View File

@@ -50,6 +50,8 @@ const subtests = [
// Use the pixel mode to make the test predictable easily.
["apz.gtk.kinetic_scroll.delta_mode", 2],
["apz.gtk.kinetic_scroll.pixel_delta_mode_multiplier", 1]]},
{"file": "helper_bug1922904.html",
"prefs": [["apz.test.mac.synth_wheel_input", true]]},
];
if (isApzEnabled()) {

7
layout/generic/ScrollContainerFrame.cpp
View File

@@ -5024,6 +5024,13 @@ void ScrollContainerFrame::ScrollSnap(const nsPoint& aDestination,
// site using `GetScrollPosition()` as |aStartPos|.
if (auto snapDestination = GetSnapPointForDestination(
ScrollUnit::DEVICE_PIXELS, snapFlags, pos, destination)) {
// Bail out if there's no scroll position change to do a workaround for bug
// 1665932 (even if the __layout__ scroll position is unchanged, the
// corresponding scroll offset update will change the __visual__ scroll
// offset in APZ).
if (snapDestination->mPosition == destination) {
return;
}
destination = snapDestination->mPosition;
ScrollToWithOrigin(
destination, nullptr /* range */,

6
layout/generic/ScrollSnap.cpp
View File

@@ -527,8 +527,7 @@ Maybe<SnapDestination> ScrollSnapUtils::GetSnapPointForDestination(
aSnapInfo.mSnapportSize.height * proximityRatio) {
finalPos.mPosition.y = aDestination.y;
} else if (aSnapInfo.mScrollSnapStrictnessY !=
StyleScrollSnapStrictness::None &&
aDestination.y != finalPos.mPosition.y) {
StyleScrollSnapStrictness::None) {
snapped = true;
}
if (aSnapInfo.mScrollSnapStrictnessX ==
@@ -537,8 +536,7 @@ Maybe<SnapDestination> ScrollSnapUtils::GetSnapPointForDestination(
aSnapInfo.mSnapportSize.width * proximityRatio) {
finalPos.mPosition.x = aDestination.x;
} else if (aSnapInfo.mScrollSnapStrictnessX !=
StyleScrollSnapStrictness::None &&
aDestination.x != finalPos.mPosition.x) {
StyleScrollSnapStrictness::None) {
snapped = true;
}
return snapped ? Some(finalPos) : Nothing();

3
testing/web-platform/meta/css/css-scroll-snap/snap-after-relayout/multiple-aligned-targets/prefer-targeted-element-positioned.html.ini
View File

@@ -1,8 +1,5 @@
[prefer-targeted-element-positioned.html]
expected: [ERROR, OK]
[prefer-targeted-element-positioned]
expected: FAIL
[prefer-targeted-element-positioned 1]
expected: FAIL

9
testing/web-platform/meta/css/css-scroll-snap/snap-after-relayout/multiple-aligned-targets/prefer-targeted-element.html.ini
View File

@@ -1,25 +1,16 @@
[prefer-targeted-element.html]
[scroller selects targeted area box1 among multiple aligned areas.]
expected: FAIL
[scroller selects targeted area box2 among multiple aligned areas.]
expected: FAIL
[scroller selects targeted area box3 among multiple aligned areas.]
expected: FAIL
[scroller selects targeted area box4 among multiple aligned areas.]
expected: FAIL
[scroller selects targeted area box5 among multiple aligned areas.]
expected: FAIL
[scroller selects targeted area box6 among multiple aligned areas.]
expected: FAIL
[scroller selects targeted area box7 among multiple aligned areas.]
expected: FAIL
[scroller selects targeted area box8 among multiple aligned areas.]
expected: FAIL