Skip to content

Commit f72d588

Browse files
rr-codesrr-codes
committed
[UI-side compositing] Keyboard scroll should not start if a wheel gesture event is in progress
https://bugs.webkit.org/show_bug.cgi?id=253726 rdar://106563114 Reviewed by Simon Fraser. Wheel event scrolls should always take precedence over keyboard scrolls. Without UI Side Compositing, this currently works by stopping any keyboard scrolls whenever any wheel event is received. For keyboard scrolling with UI Side Compositing enabled, this is not an ideal mechanism. Instead, a keyboard scroll should never begin in the first place if a wheel event gesture scroll is in progress. There already exists ways to know if a wheel event gesture scroll is in progress on iOS, so this PR just brings the same logic to macOS. Then, when a keyboard scroll is about to begin, it bails out if there is a wheel event gesture scroll in progress. This PR also improves and fixes the flakyness of the existing layout test which tests this functionality, and makes it work when UI-side compositing is enabled or disabled. * Source/WebCore/platform/KeyboardScrollingAnimator.cpp: (WebCore::KeyboardScrollingAnimator::beginKeyboardScrollGesture): * Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp: (WebKit::RemoteScrollingCoordinatorProxy::scrollingTreeNodeWillStartScroll): (WebKit::RemoteScrollingCoordinatorProxy::scrollingTreeNodeDidEndScroll): * Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.h: (WebKit::RemoteScrollingCoordinatorProxy::scrollingTreeNodeWillStartScroll): Deleted. (WebKit::RemoteScrollingCoordinatorProxy::scrollingTreeNodeDidEndScroll): Deleted. * Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingTree.cpp: (WebKit::RemoteScrollingTree::scrollingTreeNodeWillStartScroll): (WebKit::RemoteScrollingTree::scrollingTreeNodeDidEndScroll): * Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingTree.h: * Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteScrollingCoordinatorProxyIOS.h: * Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteScrollingCoordinatorProxyIOS.mm: (WebKit::RemoteScrollingCoordinatorProxyIOS::scrollingTreeNodeWillStartScroll): Deleted. (WebKit::RemoteScrollingCoordinatorProxyIOS::scrollingTreeNodeDidEndScroll): Deleted. * Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteScrollingTreeIOS.cpp: (WebKit::RemoteScrollingTreeIOS::scrollingTreeNodeWillStartScroll): Deleted. (WebKit::RemoteScrollingTreeIOS::scrollingTreeNodeDidEndScroll): Deleted. * Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteScrollingTreeIOS.h: * Source/WebKit/UIProcess/RemoteLayerTree/mac/RemoteScrollingTreeMac.mm: (WebKit::RemoteScrollingTreeMac::handleWheelEventPhase): Canonical link: https://commits.webkit.org/261581@main
1 parent cec8a64 commit f72d588

14 files changed

+130
-54
lines changed

‎LayoutTests/fast/scrolling/mac/keyboard-scrolling-with-mouse-scroll-expected.txt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1+
PASS endY >= startY is true
12
PASS successfullyParsed is true
23

34
TEST COMPLETE
4-
Successful.
5-
5+
fixed

‎LayoutTests/fast/scrolling/mac/keyboard-scrolling-with-mouse-scroll.html‎

Lines changed: 69 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,46 +1,81 @@
1-
<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true EventHandlerDrivenSmoothKeyboardScrollingEnabled=true ] -->
2-
1+
<!DOCTYPE html> <!-- webkit-test-runner [ ScrollAnimatorEnabled=true ] -->
32
<html>
3+
<head>
4+
<style>
5+
body {
6+
height: 10000px;
7+
background-image: repeating-linear-gradient(white, silver 200px);
8+
}
49

5-
<head>
10+
.fixed {
11+
position: fixed;
12+
top: 0;
13+
}
14+
</style>
15+
<script src="../../../resources/js-test-pre.js"></script>
616
<script src="../../../resources/ui-helper.js"></script>
7-
<script src="../../../resources/js-test.js"></script>
8-
<meta name="viewport" content="initial-scale=1.5, user-scalable=no">
917
<script>
10-
if (window.testRunner) {
11-
testRunner.dumpAsText();
12-
testRunner.waitUntilDone();
13-
}
14-
15-
async function runTest()
18+
var jsTestIsAsync = true;
19+
20+
async function scrollTest()
1621
{
22+
await UIHelper.renderingUpdate();
23+
1724
await UIHelper.startMonitoringWheelEvents();
18-
if (!window.testRunner || !testRunner.runUIScript)
19-
return;
25+
eventSender.mouseMoveTo(100, 100);
2026

21-
await UIHelper.rawKeyDown("downArrow");
22-
23-
setTimeout(async () => {
24-
await UIHelper.rawKeyUp("downArrow");
25-
const startingPosition = window.scrollY;
27+
// normal scroll
28+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -10, "began", "none");
29+
await UIHelper.renderingUpdate();
30+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -10, "changed", "none");
31+
await UIHelper.renderingUpdate();
32+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -10, "changed", "none");
33+
await UIHelper.renderingUpdate();
34+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -10, "changed", "none");
35+
await UIHelper.renderingUpdate();
36+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, "ended", "none");
37+
await UIHelper.renderingUpdate();
2638

27-
await UIHelper.mouseWheelScrollAt(0, 0, 0, 0, 0, 0);
39+
// momentum scroll
40+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -10, "none", "begin");
41+
await UIHelper.renderingUpdate();
42+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -10, "none", "continue");
43+
await UIHelper.renderingUpdate();
44+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -10, "none", "continue");
45+
await UIHelper.renderingUpdate();
46+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -10, "none", "continue");
47+
await UIHelper.renderingUpdate();
2848

29-
const position = window.scrollY;
30-
if (Math.abs(startingPosition - position) < 20)
31-
debug("Successful.");
32-
else
33-
debug("Unsuccessful. window.scrollY after wheel event == " + position + "; window.scrollY before wheel event == " + startingPosition);
49+
startY = window.scrollY;
3450

35-
testRunner.notifyDone();
36-
}, 100);
37-
}
38-
</script>
39-
</head>
51+
await UIHelper.rawKeyDown("upArrow");
52+
await UIHelper.renderingUpdate();
53+
54+
await UIHelper.delayFor(100);
55+
56+
await UIHelper.rawKeyUp("upArrow");
57+
await UIHelper.renderingUpdate();
58+
59+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -10, "none", "continue");
60+
await UIHelper.renderingUpdate();
61+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -10, "none", "continue");
62+
await UIHelper.renderingUpdate();
63+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -10, "none", "continue");
64+
await UIHelper.renderingUpdate();
65+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, "none", "end");
66+
await UIHelper.renderingUpdate();
4067

41-
<body onload="runTest()">
42-
<div style="height: 5000px;">
43-
</div>
44-
</body>
68+
endY = window.scrollY;
69+
shouldBeTrue('endY >= startY');
4570

46-
</html>
71+
finishJSTest();
72+
}
73+
74+
window.addEventListener('load', scrollTest, false);
75+
</script>
76+
</head>
77+
<body>
78+
<div class="fixed">fixed</div>
79+
<script src="../../../resources/js-test-post.js"></script>
80+
</body>
81+
</html>

‎LayoutTests/platform/mac-wk2/TestExpectations‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1773,7 +1773,6 @@ webkit.org/b/252866 [ Debug ] fast/text/glyph-display-lists/glyph-display-list-c
17731773
[ BigSur+ ] streams/readable-stream-default-controller-error.html [ Pass Crash ]
17741774
[ BigSur+ ] fast/mediastream/mediastreamtrack-video-clone.html [ Pass Failure ]
17751775
[ BigSur+ ] fast/repaint/fixed-move-after-keyboard-scroll.html [ Pass Failure ]
1776-
[ BigSur+ ] fast/scrolling/mac/keyboard-scrolling-with-mouse-scroll.html [ Pass Failure ]
17771776
[ BigSur+ x86_64 ] http/tests/media/hls/track-in-band-hls-metadata-cue-duration.html [ Pass Failure ]
17781777
[ Monterey+ x86_64 ] http/tests/websocket/tests/hybi/handshake-ok-with-legacy-websocket-response-headers.html [ Pass Failure ]
17791778
[ Bigsur+ x86_64 ] http/wpt/mediarecorder/pause-recording.html [ Pass Failure ]

‎Source/WebCore/page/scrolling/ScrollingTree.cpp‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -635,10 +635,13 @@ void ScrollingTree::setUserScrollInProgressForNode(ScrollingNodeID nodeID, bool
635635
{
636636
ASSERT(nodeID);
637637
Locker locker { m_treeStateLock };
638-
if (isScrolling)
638+
if (isScrolling) {
639639
m_treeState.nodesWithActiveUserScrolls.add(nodeID);
640-
else
640+
scrollingTreeNodeWillStartScroll(nodeID);
641+
} else {
641642
m_treeState.nodesWithActiveUserScrolls.remove(nodeID);
643+
scrollingTreeNodeDidEndScroll(nodeID);
644+
}
642645
}
643646

644647
void ScrollingTree::clearNodesWithUserScrollInProgress()

‎Source/WebCore/page/scrolling/ScrollingTree.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,9 +138,9 @@ class ScrollingTree : public ThreadSafeRefCounted<ScrollingTree> {
138138

139139
#if PLATFORM(IOS_FAMILY)
140140
virtual void scrollingTreeNodeWillStartPanGesture(ScrollingNodeID) { }
141+
#endif
141142
virtual void scrollingTreeNodeWillStartScroll(ScrollingNodeID) { }
142143
virtual void scrollingTreeNodeDidEndScroll(ScrollingNodeID) { }
143-
#endif
144144

145145
WEBCORE_EXPORT TrackingType eventTrackingTypeForPoint(EventTrackingRegions::EventType, IntPoint);
146146

‎Source/WebCore/platform/KeyboardScrollingAnimator.cpp‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,12 @@ bool KeyboardScrollingAnimator::beginKeyboardScrollGesture(ScrollDirection direc
202202
if (!scroll)
203203
return false;
204204

205+
if (m_scrollableArea.isUserScrollInProgress()) {
206+
m_scrollTriggeringKeyIsPressed = false;
207+
m_scrollableArea.endKeyboardScroll(true);
208+
return true;
209+
}
210+
205211
if (m_scrollTriggeringKeyIsPressed)
206212
return false;
207213

‎Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingTree.cpp‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,18 @@ bool RemoteScrollingTree::scrollingTreeNodeRequestsKeyboardScroll(ScrollingNodeI
106106
return m_scrollingCoordinatorProxy->scrollingTreeNodeRequestsKeyboardScroll(nodeID, request);
107107
}
108108

109+
void RemoteScrollingTree::scrollingTreeNodeWillStartScroll(ScrollingNodeID nodeID)
110+
{
111+
if (m_scrollingCoordinatorProxy)
112+
m_scrollingCoordinatorProxy->scrollingTreeNodeWillStartScroll(nodeID);
113+
}
114+
115+
void RemoteScrollingTree::scrollingTreeNodeDidEndScroll(ScrollingNodeID nodeID)
116+
{
117+
if (m_scrollingCoordinatorProxy)
118+
m_scrollingCoordinatorProxy->scrollingTreeNodeDidEndScroll(nodeID);
119+
}
120+
109121
Ref<ScrollingTreeNode> RemoteScrollingTree::createScrollingTreeNode(ScrollingNodeType nodeType, ScrollingNodeID nodeID)
110122
{
111123
switch (nodeType) {

‎Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingTree.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,9 @@ class RemoteScrollingTree : public WebCore::ScrollingTree {
5858
bool scrollingTreeNodeRequestsScroll(WebCore::ScrollingNodeID, const WebCore::RequestedScrollData&) override;
5959
bool scrollingTreeNodeRequestsKeyboardScroll(WebCore::ScrollingNodeID, const WebCore::RequestedKeyboardScrollData&) override;
6060

61+
void scrollingTreeNodeWillStartScroll(WebCore::ScrollingNodeID) override;
62+
void scrollingTreeNodeDidEndScroll(WebCore::ScrollingNodeID) override;
63+
6164
void currentSnapPointIndicesDidChange(WebCore::ScrollingNodeID, std::optional<unsigned> horizontal, std::optional<unsigned> vertical) override;
6265
void reportExposedUnfilledArea(MonotonicTime, unsigned unfilledArea) override;
6366
void reportSynchronousScrollingReasonsChanged(MonotonicTime, OptionSet<WebCore::SynchronousScrollingReason>) override;

‎Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteScrollingTreeIOS.cpp‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -52,18 +52,6 @@ void RemoteScrollingTreeIOS::scrollingTreeNodeWillStartPanGesture(ScrollingNodeI
5252
m_scrollingCoordinatorProxy->scrollingTreeNodeWillStartPanGesture(nodeID);
5353
}
5454

55-
void RemoteScrollingTreeIOS::scrollingTreeNodeWillStartScroll(ScrollingNodeID nodeID)
56-
{
57-
if (m_scrollingCoordinatorProxy)
58-
m_scrollingCoordinatorProxy->scrollingTreeNodeWillStartScroll(nodeID);
59-
}
60-
61-
void RemoteScrollingTreeIOS::scrollingTreeNodeDidEndScroll(ScrollingNodeID nodeID)
62-
{
63-
if (m_scrollingCoordinatorProxy)
64-
m_scrollingCoordinatorProxy->scrollingTreeNodeDidEndScroll(nodeID);
65-
}
66-
6755
Ref<ScrollingTreeNode> RemoteScrollingTreeIOS::createScrollingTreeNode(ScrollingNodeType nodeType, ScrollingNodeID nodeID)
6856
{
6957
switch (nodeType) {

‎Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteScrollingTreeIOS.h‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,6 @@ class RemoteScrollingTreeIOS final : public RemoteScrollingTree {
4242
Ref<ScrollingTreeNode> createScrollingTreeNode(ScrollingNodeType, ScrollingNodeID) final;
4343

4444
void scrollingTreeNodeWillStartPanGesture(WebCore::ScrollingNodeID) final;
45-
void scrollingTreeNodeWillStartScroll(WebCore::ScrollingNodeID) final;
46-
void scrollingTreeNodeDidEndScroll(WebCore::ScrollingNodeID) final;
4745
};
4846

4947
} // namespace WebKit

0 commit comments

Comments
 (0)