Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[macOS] Scrolling with a physical mouse wheel should not always anima…
…te to the closest snap point

https://bugs.webkit.org/show_bug.cgi?id=255493
rdar://107885426

Reviewed by Simon Fraser.

When scrolling using a physical mouse wheel in a scroll snap container, WebKit's current scroll snap
implementation handles each wheel event in a stateless manner, kicking off a scroll snap animation
to the closest snap point if no other wheel event is observed after 750 ms. This can lead to some
unintuitive behaviors when distances between scroll snap points are large, since the user may scroll
for a single wheel tick expecting to advance to the next page, only for the scroll position to
animate back to where they started.

This patch improves this by treating a stream of discrete wheel events similarly to trackpad-based
momentum scrolling, and animates to the appropriate snap point in the direction of scrolling; this
also aligns our implementation more closely with both Gecko and Blink.

See below for more details.

Test: css3/scroll-snap/scroll-snap-discrete-wheel-event-in-mainframe.html

* LayoutTests/css3/scroll-snap/scroll-snap-discrete-wheel-event-in-mainframe-expected.txt: Added.
* LayoutTests/css3/scroll-snap/scroll-snap-discrete-wheel-event-in-mainframe.html: Added.

Add a new layout test to exercise the change, in a mainframe (root) scroll snapping context.

* LayoutTests/css3/scroll-snap/scroll-snap-wheel-event.html:

Adjust an existing stateless scroll snapping test to exercise the change by lowering the scrolling
tick count from 3 to 1. Without this change, this adjustment would've bumped us back to the original
scroll position; after this change, we'll now animate to the next snap point.

* LayoutTests/platform/glib/TestExpectations:
* LayoutTests/platform/ios-wk2/TestExpectations:
* LayoutTests/platform/mac-wk1/TestExpectations:

Discrete wheel events on the root don't seem to trigger scroll snapping at all in WebKit1, both
before and after this patch. I filed webkit.org/b/255498, to track that issue separately.

* Source/WebCore/platform/ScrollingEffectsController.h:

Maintain a LIFO queue of up to three discrete wheel event deltas, which we use to determine the
user's intended scrolling direction after finishing a stream of discrete wheel events.

* Source/WebCore/platform/mac/ScrollingEffectsController.mm:
(WebCore::ScrollingEffectsController::stopAllTimers):
(WebCore::toWheelEventStatus):
(WebCore::operator<<):
(WebCore::ScrollingEffectsController::scheduleDiscreteScrollSnap):
(WebCore::ScrollingEffectsController::discreteSnapTransitionTimerFired):

Rename "stateless" -> "discrete", to reflect the fact that the new implementation is now stateful
by way of maintaining a queue of recent discrete wheel event deltas. Additionally, use
`transitionToGlideAnimationState()` to kick off scroll snapping if the average wheel event delta is
nonzero.

(WebCore::ScrollingEffectsController::processWheelEventForScrollSnap):
(WebCore::ScrollingEffectsController::scheduleStatelessScrollSnap): Deleted.

Dramatically reduce the delay before firing the scroll snap timer for discrete wheel events, now
that the purpose is no longer to wait for the user to manually scroll to the next page before
snapping, but rather observe enough events to estimate the user's intended scrolling direction.

(WebCore::ScrollingEffectsController::statelessSnapTransitionTimerFired): Deleted.

Canonical link: https://commits.webkit.org/263071@main
  • Loading branch information
whsieh committed Apr 18, 2023
1 parent 616bcfa commit 939e8d4
Show file tree
Hide file tree
Showing 8 changed files with 135 additions and 25 deletions.
@@ -0,0 +1,14 @@
This test verifies that discrete wheel events trigger scroll snapping in the mainframe.
To manually run the test, scroll using a physical mouse wheel and verify that a single scroll
tick animates to the next snap point, and a second scroll tick should not trigger snapping (due
to the content being too tall)

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".

PASS pageYOffset became 600
PASS pageYOffset > 600 became true
PASS pageYOffset < 1500 is true
PASS successfullyParsed is true

TEST COMPLETE

@@ -0,0 +1,65 @@
<!DOCTYPE html>
<html>
<head>
<style>
:root {
scroll-snap-type: y mandatory;
overflow-x: hidden;
}

body, html {
width: 100%;
height: 100%;
margin: 0;
}

.snap {
width: 100%;
height: 100%;
scroll-snap-align: start;
}

#extra-tall {
height: 150%;
}

.output {
color: white;
position: fixed;
top: 0;
}
</style>
<script src="../../resources/js-test.js"></script>
<script src="../../resources/ui-helper.js"></script>
<script>
jsTestIsAsync = true;

addEventListener("load", async () => {
description(`This test verifies that discrete wheel events trigger scroll snapping in the mainframe.
To manually run the test, scroll using a physical mouse wheel and verify that a single scroll
tick animates to the next snap point, and a second scroll tick should not trigger snapping (due
to the content being too tall)`);

if (!window.testRunner)
return;

await UIHelper.statelessMouseWheelScrollAt(100, 100, 0, -1);
await shouldBecomeEqual("pageYOffset", "600");
await UIHelper.statelessMouseWheelScrollAt(200, 200, 0, -1);
await shouldBecomeEqual("pageYOffset > 600", "true");
shouldBeTrue("pageYOffset < 1500");

finishJSTest();
});
</script>
</head>
<body>
<div class="snap" id="first-container" style="background: #80475E"></div>
<div class="snap" id="extra-tall" style="background: #CC5A71"></div>
<div class="snap" style="background: #32228B"></div>
<div class="output">
<pre id="description"></pre>
<pre id="console"></pre>
</div>
</body>
</html>
6 changes: 3 additions & 3 deletions LayoutTests/css3/scroll-snap/scroll-snap-wheel-event.html
@@ -1,7 +1,7 @@
<!DOCTYPE html>
<html>
<head>
<title>Simple wheel events should trigger scroll snapping</title>
<title>Discrete wheel events should trigger scroll snapping</title>
<style type="text/css">
.container {
height: 200px;
Expand Down Expand Up @@ -38,7 +38,7 @@
eventSender.mouseMoveTo(80, 190);
eventSender.mouseUp();

await UIHelper.statelessMouseWheelScrollAt(80, 190, -3, 0);
await UIHelper.statelessMouseWheelScrollAt(80, 190, -1, 0);

let horizontalContainer = document.getElementById("horizontal-container");
expectTrue(horizontalContainer.scrollLeft == 200, "horizontal mouse wheel event snapped");
Expand All @@ -48,7 +48,7 @@
eventSender.mouseMoveTo(190, 270);
eventSender.mouseUp();

await UIHelper.statelessMouseWheelScrollAt(190, 270, 0, -3);
await UIHelper.statelessMouseWheelScrollAt(190, 270, 0, -1);

let verticalContainer = document.getElementById("vertical-container");
expectTrue(verticalContainer.scrollTop == 185, "vertical mouse wheel event snapped");
Expand Down
2 changes: 2 additions & 0 deletions LayoutTests/platform/glib/TestExpectations
Expand Up @@ -1356,6 +1356,8 @@ webkit.org/b/212202 fast/scrolling/overflow-scrollable-after-back.html [ Skip ]
# Previous: webkit.org/b/223729 [ Debug ] css3/scroll-snap/scroll-snap-wheel-event.html [ Pass ]
webkit.org/b/212202 css3/scroll-snap/scroll-snap-wheel-event.html [ Skip ]

css3/scroll-snap/scroll-snap-discrete-wheel-event-in-mainframe.html [ Skip ]

webkit.org/b/213921 fast/visual-viewport/scroll-event-fired-during-scroll-alone.html [ Crash ]

#////////////////////////////////////////////////////////////////////////////////////////
Expand Down
1 change: 1 addition & 0 deletions LayoutTests/platform/ios-wk2/TestExpectations
Expand Up @@ -966,6 +966,7 @@ editing/selection/select-out-of-floated-non-editable-11.html [ Skip ]
editing/selection/select-out-of-floated-non-editable-12.html [ Skip ]
css3/scroll-snap/scroll-snap-click-scrollbar-gutter.html [ Skip ]
css3/scroll-snap/scroll-snap-wheel-event.html [ Skip ]
css3/scroll-snap/scroll-snap-discrete-wheel-event-in-mainframe.html [ Skip ]

webkit.org/b/192088 media/no-fullscreen-when-hidden.html [ Failure Pass ]

Expand Down
3 changes: 3 additions & 0 deletions LayoutTests/platform/mac-wk1/TestExpectations
Expand Up @@ -2494,3 +2494,6 @@ webkit.org/b/255228 [ Monterey+ ] imported/w3c/web-platform-tests/html/semantics
webkit.org/b/255228 [ Monterey+ ] imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/sizes/parse-a-sizes-attribute-quirks-mode.html [ Failure ]
webkit.org/b/255228 [ Monterey+ ] imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/sizes/parse-a-sizes-attribute-standards-mode.html [ Failure ]
webkit.org/b/255228 [ Monterey+ ] imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/sizes/parse-a-sizes-attribute-width-1000px.html [ Failure ]

# Discrete wheel events don't trigger scroll snapping on the root in WK1
webkit.org/b/255498 css3/scroll-snap/scroll-snap-discrete-wheel-event-in-mainframe.html [ Skip ]
7 changes: 4 additions & 3 deletions Source/WebCore/platform/ScrollingEffectsController.h
Expand Up @@ -204,8 +204,8 @@ class ScrollingEffectsController : public ScrollAnimationClient {

#if PLATFORM(MAC)
bool shouldOverrideMomentumScrolling() const;
void statelessSnapTransitionTimerFired();
void scheduleStatelessScrollSnap();
void discreteSnapTransitionTimerFired();
void scheduleDiscreteScrollSnap(const FloatSize& delta);

bool modifyScrollDeltaForStretching(const PlatformWheelEvent&, FloatSize&, bool isHorizontallyStretched, bool isVerticallyStretched);
bool applyScrollDeltaWithStretching(const PlatformWheelEvent&, FloatSize, bool isHorizontallyStretched, bool isVerticallyStretched);
Expand Down Expand Up @@ -277,7 +277,8 @@ class ScrollingEffectsController : public ScrollAnimationClient {
bool m_ignoreMomentumScrolls { false };
bool m_isRubberBanding { false };

std::unique_ptr<ScrollingEffectsControllerTimer> m_statelessSnapTransitionTimer;
Deque<FloatSize> m_recentDiscreteWheelDeltas;
std::unique_ptr<ScrollingEffectsControllerTimer> m_discreteSnapTransitionTimer;

#if HAVE(RUBBER_BANDING)
RectEdges<bool> m_rubberBandingEdges;
Expand Down
62 changes: 43 additions & 19 deletions Source/WebCore/platform/mac/ScrollingEffectsController.mm
Expand Up @@ -73,8 +73,8 @@ static float scrollWheelMultiplier()

void ScrollingEffectsController::stopAllTimers()
{
if (m_statelessSnapTransitionTimer)
m_statelessSnapTransitionTimer->stop();
if (m_discreteSnapTransitionTimer)
m_discreteSnapTransitionTimer->stop();

#if ASSERT_ENABLED
m_timersWereStopped = true;
Expand Down Expand Up @@ -519,7 +519,7 @@ static FloatSize deltaAlignedToDominantAxis(FloatSize delta)
MomentumScrollBegin,
MomentumScrolling,
MomentumScrollEnd,
StatelessScrollEvent,
DiscreteScrollEvent,
Unknown
};

Expand All @@ -537,7 +537,7 @@ static inline WheelEventStatus toWheelEventStatus(PlatformWheelEventPhase phase,
return WheelEventStatus::MomentumScrollEnd;

case PlatformWheelEventPhase::None:
return WheelEventStatus::StatelessScrollEvent;
return WheelEventStatus::DiscreteScrollEvent;

default:
return WheelEventStatus::Unknown;
Expand Down Expand Up @@ -573,7 +573,7 @@ static inline WheelEventStatus toWheelEventStatus(PlatformWheelEventPhase phase,
case WheelEventStatus::MomentumScrollBegin: ts << "MomentumScrollBegin"; break;
case WheelEventStatus::MomentumScrolling: ts << "MomentumScrolling"; break;
case WheelEventStatus::MomentumScrollEnd: ts << "MomentumScrollEnd"; break;
case WheelEventStatus::StatelessScrollEvent: ts << "StatelessScrollEvent"; break;
case WheelEventStatus::DiscreteScrollEvent: ts << "DiscreteScrollEvent"; break;
case WheelEventStatus::Unknown: ts << "Unknown"; break;
}
return ts;
Expand All @@ -589,32 +589,56 @@ static inline WheelEventStatus toWheelEventStatus(PlatformWheelEventPhase phase,
return scrollSnapState == ScrollSnapState::Gliding || scrollSnapState == ScrollSnapState::DestinationReached;
}

void ScrollingEffectsController::scheduleStatelessScrollSnap()
void ScrollingEffectsController::scheduleDiscreteScrollSnap(const FloatSize& delta)
{
stopScrollSnapAnimation();
if (m_statelessSnapTransitionTimer) {
m_statelessSnapTransitionTimer->stop();
m_statelessSnapTransitionTimer = nullptr;
}
if (m_discreteSnapTransitionTimer) {
m_discreteSnapTransitionTimer->stop();
m_discreteSnapTransitionTimer = nullptr;
m_recentDiscreteWheelDeltas.append(delta);
static constexpr auto numberOfDeltasToStoreForDiscreteScrollSnap = 3;
if (m_recentDiscreteWheelDeltas.size() > numberOfDeltasToStoreForDiscreteScrollSnap)
m_recentDiscreteWheelDeltas.removeFirst();
} else
m_recentDiscreteWheelDeltas = { delta };

if (!usesScrollSnap())
return;

static const Seconds statelessScrollSnapDelay = 750_ms;
m_statelessSnapTransitionTimer = m_client.createTimer([this] {
statelessSnapTransitionTimerFired();
static const Seconds discreteScrollSnapDelay = 100_ms;
m_discreteSnapTransitionTimer = m_client.createTimer([this] {
discreteSnapTransitionTimerFired();
});
m_statelessSnapTransitionTimer->startOneShot(statelessScrollSnapDelay);
m_discreteSnapTransitionTimer->startOneShot(discreteScrollSnapDelay);
startDeferringWheelEventTestCompletion(WheelEventTestMonitor::ScrollSnapInProgress);
}

void ScrollingEffectsController::statelessSnapTransitionTimerFired()
void ScrollingEffectsController::discreteSnapTransitionTimerFired()
{
m_statelessSnapTransitionTimer = nullptr;
auto recentDiscreteWheelDeltas = std::exchange(m_recentDiscreteWheelDeltas, { });
m_discreteSnapTransitionTimer = nullptr;

if (!usesScrollSnap())
return;

if (m_scrollSnapState->transitionToSnapAnimationState(m_client.scrollExtents(), m_client.pageScaleFactor(), m_client.scrollOffset()))
FloatSize wheelDeltaForGlideAnimation;
if (!recentDiscreteWheelDeltas.isEmpty()) {
for (auto delta : recentDiscreteWheelDeltas)
wheelDeltaForGlideAnimation += delta;
auto signOf = [](float value) {
return value > 0 ? 1 : (value < 0 ? -1 : 0);
};
wheelDeltaForGlideAnimation.setWidth(signOf(wheelDeltaForGlideAnimation.width()));
wheelDeltaForGlideAnimation.setHeight(signOf(wheelDeltaForGlideAnimation.height()));
}

bool shouldStartScrollSnapAnimation = [&] {
if (wheelDeltaForGlideAnimation.isZero())
return m_scrollSnapState->transitionToSnapAnimationState(m_client.scrollExtents(), m_client.pageScaleFactor(), m_client.scrollOffset());
return m_scrollSnapState->transitionToGlideAnimationState(m_client.scrollExtents(), m_client.pageScaleFactor(), m_client.scrollOffset(), -wheelDeltaForGlideAnimation, -wheelDeltaForGlideAnimation);
}();

if (shouldStartScrollSnapAnimation)
startScrollSnapAnimation();
else
stopDeferringWheelEventTestCompletion(WheelEventTestMonitor::ScrollSnapInProgress);
Expand Down Expand Up @@ -655,9 +679,9 @@ static inline WheelEventStatus toWheelEventStatus(PlatformWheelEventPhase phase,
case WheelEventStatus::MomentumScrollEnd:
isMomentumScrolling = true;
break;
case WheelEventStatus::StatelessScrollEvent:
case WheelEventStatus::DiscreteScrollEvent:
m_scrollSnapState->transitionToUserInteractionState();
scheduleStatelessScrollSnap();
scheduleDiscreteScrollSnap(wheelEvent.delta());
break;
case WheelEventStatus::Unknown:
ASSERT_NOT_REACHED();
Expand Down

0 comments on commit 939e8d4

Please sign in to comment.