Skip to content

Commit

Permalink
Fix snaps-after-keyboard-scrolling
Browse files Browse the repository at this point in the history
Two issues were addressed in fixing these snap tests. Since the scroll
timing is not accelerated, we need to be careful about how many tests
we include in a single file to avoid timeouts. The subtests were split
into two groups to reduce the risk of a timeout. Secondly, the key-
presses are resolved when queued and not when handled, which can lead to
incorrectly flagging the end of an animation before it has actually
started.

Bug: 878878, 1047164
Change-Id: I8cd2f1ceb52f6486ea04068f0920525869455682
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2727595
Reviewed-by: Xida Chen <xidachen@chromium.org>
Commit-Queue: Kevin Ellis <kevers@chromium.org>
Cr-Commit-Position: refs/heads/master@{#859055}
  • Loading branch information
Kevin Ellis authored and Chromium LUCI CQ committed Mar 2, 2021
1 parent fc692e1 commit f4db195
Show file tree
Hide file tree
Showing 4 changed files with 79 additions and 39 deletions.
2 changes: 0 additions & 2 deletions third_party/blink/web_tests/SlowTests
Expand Up @@ -205,8 +205,6 @@ crbug.com/826957 fast/peerconnection/RTCPeerConnection-manyCandidates.html [ Slo
crbug.com/853977 [ Linux ] http/tests/fetch/chromium/call-extra-crash-tee.html [ Slow ]
crbug.com/853977 [ Linux ] http/tests/fetch/chromium/release-handle-crash.html [ Slow ]

crbug.com/1047164 [ Mac ] fast/scroll-snap/snaps-after-keyboard-scrolling.html [ Slow ]

crbug.com/865262 [ Mac ] media/controls/text-track-menu-pointer-selection.html [ Slow ]

crbug.com/942951 media/controls/controls-layout-in-different-size.html [ Slow ]
Expand Down
2 changes: 0 additions & 2 deletions third_party/blink/web_tests/TestExpectations
Expand Up @@ -4859,7 +4859,6 @@ crbug.com/1041830 http/tests/devtools/tracing/timeline-js/timeline-js-line-level

# Enable scroll-snap tests on impl thread
# These are currently failing on Mac which needs more investigation, snap-scrolls-visual-viewport seems flaky
crbug.com/878878 [ Mac ] virtual/threaded/fast/scroll-snap/snaps-after-keyboard-scrolling.html [ Pass Timeout Failure ]

crbug.com/878878 virtual/threaded/fast/scroll-snap/animate-fling-to-snap-points.html [ Failure Pass Timeout ]
crbug.com/878878 virtual/threaded/fast/scroll-snap/snap-scrolls-visual-viewport.html [ Failure Pass ]
Expand Down Expand Up @@ -4951,7 +4950,6 @@ crbug.com/1046440 fast/loader/submit-form-while-parsing-2.html [ Pass Failure Ti
crbug.com/995663 [ Linux ] http/tests/media/autoplay/document-user-activation-cross-origin-feature-policy-header.html [ Pass Failure Timeout ]

# Sheriff 2020-01-30
crbug.com/1047164 [ Mac ] fast/scroll-snap/snaps-after-keyboard-scrolling.html [ Pass Failure ]
crbug.com/1047208 http/tests/serviceworker/update-served-from-cache.html [ Pass Failure ]
crbug.com/1047293 [ Mac ] editing/pasteboard/pasteboard_with_unfocused_selection.html [ Pass Failure ]

Expand Down
@@ -0,0 +1,68 @@
<!DOCTYPE html>
<link rel="help" href="https://drafts.csswg.org/css-scroll-snap-1" />
<link rel="stylesheet" href="resources/simple-snap.css">
<script src="../../resources/testharness.js"></script>
<script src="../../resources/testharnessreport.js"></script>
<script src="../../resources/gesture-util.js"></script>

<div id='scroller'>
<div id="space"></div>
<div class="snap left top" id="top-left"></div>
<div class="snap right top" id="top-right"></div>
<div class="snap left bottom" id="bottom-left"></div>
</div>

<script>
var scroller = document.getElementById("scroller");
var topLeft = document.getElementById("top-left");
var topRight = document.getElementById("top-right");

function scrollLeft() {
return scroller.scrollLeft;
}

function scrollTop() {
return scroller.scrollTop;
}

function keyPress(key) {
return new Promise((resolve, reject) => {
if (window.eventSender) {
eventSender.keyDown(key);
resolve();
}
else {
reject('This test requires window.eventSender');
}
})
}

promise_test (async () => {
await mouseClickOn(10, 10);
scroller.scrollTo(0, 0);
await keyPress("ArrowDown");
await waitForScrollEnd(scroller, scrollTop, 400);
}, "Snaps to bottom-left after pressing ArrowDown");

promise_test (async () => {
await mouseClickOn(10, 10);
scroller.scrollTo(0, 400);
await keyPress("ArrowUp");
await waitForScrollEnd(scroller, scrollTop, 0);
}, "Snaps to top-left after pressing ArrowUp");

promise_test (async () => {
await mouseClickOn(10, 10);
scroller.scrollTo(0, 0);
await keyPress("ArrowRight");
await waitForScrollEnd(scroller, scrollLeft, 400);
}, "Snaps to top-right after pressing ArrowRight");

promise_test (async () => {
await mouseClickOn(10, 10);
scroller.scrollTo(400, 0);
await keyPress("ArrowLeft");
await waitForScrollEnd(scroller, scrollLeft, 0);
}, "Snaps to top-left after pressing ArrowLeft");
</script>

Expand Up @@ -37,38 +37,6 @@
})
}

promise_test (async () => {
await mouseClickOn(10, 10);
scroller.scrollTo(0, 0);
await keyPress("ArrowDown");
await waitForAnimationEndTimeBased(scrollTop);
assert_equals(scroller.scrollTop, 400);
}, "Snaps to bottom-left after pressing ArrowDown");

promise_test (async () => {
await mouseClickOn(10, 10);
scroller.scrollTo(0, 400);
await keyPress("ArrowUp");
await waitForAnimationEndTimeBased(scrollTop);
assert_equals(scroller.scrollTop, 0);
}, "Snaps to top-left after pressing ArrowUp");

promise_test (async () => {
await mouseClickOn(10, 10);
scroller.scrollTo(0, 0);
await keyPress("ArrowRight");
await waitForAnimationEndTimeBased(scrollLeft);
assert_equals(scroller.scrollLeft, 400);
}, "Snaps to top-right after pressing ArrowRight");

promise_test (async () => {
await mouseClickOn(10, 10);
scroller.scrollTo(400, 0);
await keyPress("ArrowLeft");
await waitForAnimationEndTimeBased(scrollLeft);
assert_equals(scroller.scrollLeft, 0);
}, "Snaps to top-left after pressing ArrowLeft");

promise_test (async () => {
// Make the snap area covers the snapport.
topLeft.style.width = "800px";
Expand All @@ -77,7 +45,10 @@
topRight.style.left = "500px";
await mouseClickOn(10, 10);
scroller.scrollTo(0, 0);
const scrollEventPromise = waitForScrollEvent(scroller);
await keyPress("ArrowRight");
await scrollEventPromise;
// Use time based wait since the scroll does not end at a fixed value.
await waitForAnimationEndTimeBased(scrollLeft);
assert_between_exclusive(scroller.scrollLeft, 0, 500);
topLeft.style.width = "";
Expand All @@ -94,8 +65,7 @@
await mouseClickOn(10, 10);
scroller.scrollTo(0, 0);
await keyPress("ArrowRight");
await waitForAnimationEndTimeBased(scrollLeft);
assert_equals(scroller.scrollLeft, 20);
await waitForScrollEnd(scroller, scrollLeft, 20);
topLeft.style.width = "";
topRight.style.left = "400px";
}, "If the original intended offset is valid as making a snap area cover the "
Expand All @@ -105,7 +75,9 @@
promise_test (async () => {
await mouseClickOn(10, 10);
scroller.scrollTo(400, 0);
const scrollEventPromise = waitForScrollEvent(scroller);
await keyPress("ArrowRight");
// Use time based wait since returning to the start position.
await waitForAnimationEndTimeBased(scrollLeft);
assert_equals(scroller.scrollLeft, 400);
}, "If there is no valid snap offset on the arrow key's direction other than "
Expand All @@ -116,11 +88,15 @@
scroller.style.scrollSnapType = "both proximity";
await mouseClickOn(10, 10);
scroller.scrollTo(400, 0);
const scrollEventPromise = waitForScrollEvent(scroller);
await keyPress("ArrowRight");
await scrollEventPromise;
// Use time based wait since the scroll does not end at a fixed value.
await waitForAnimationEndTimeBased(scrollLeft);
assert_greater_than(scroller.scrollLeft, 400);
scroller.style.scrollSnapType = "both mandatory";
}, "If there is no valid snap offset on the arrow key's direction other than "
+ "the current offset, and the scroll-snap-type is proximity, go to the "
+ "original intended offset");
</script>
</script>

0 comments on commit f4db195

Please sign in to comment.