Skip to content

Commit e869783

Browse files
smfrsmfr
committed
[Async scrolling] Programmatic scrolls can cause missing tiles and offset hit-testing
https://bugs.webkit.org/show_bug.cgi?id=262287 rdar://116205365 Reviewed by Alan Baradlay and Abrar Rahman Protyasha. This change fixes some long-standing issues with asynchronous scrolling, where UI and web processes could be left with different scroll positions when both user and programmatic scrolling was happening at the same time. This could result in bad tile coverage (missing content), and offset hit-testing. The fundamental problem was that IPC message for programmatic scrolling, from web to UI process, could overlap with messages in the other direction for user scrolling; the messages would pass like ships in the night, with no final reconciliation step. 307867@main introduced the concept of `ScrollRequestIdentifier`. This change makes use of those identifiers to ensure that the web process is updated with the correct scroll position via a message from the UI process back to the web process. Every programmatic scroll IPC now triggers a response message, which returns the originating identifier. This allows `RemoteScrollingCoordinator` in the web process to know when there's programmatic scroll in flight. When there is, we have to ignore user scrolls, because they will contain a scroll position that is stale relative to the current web process scroll position. With multiple programmatic scrolls in a row, we also have to ignore the response for the same reason. We need to take care to fire `scrollend` when necessary. ScrollUpdate now has a data member with two variants; one for responses to programmatic scrolls (`ScrollRequestResponseData`) and one for other messages from UI to web process. We actually apply the scroll position in `RemoteScrollingCoordinator::scrollUpdateForNode()`, to handle the case where the web process sent a "delta" scroll (e.g. for scroll anchoring or `scrollBy`); the response will contain the most recent UI-side scroll position with the delta applied. There was already a `ProgrammaticScrollDidEnd` sent back to the web process; now that we always send `ScrollRequestResponse` updates back, use that to trigger `scrollend` via a `ShouldFireScrollEnd` member on ScrollUpdate. `scrollingTreeNodeDidStopProgrammaticScroll` is no longer needed. `scrollingTreeNodeRequestsScroll()` has to distinguish between "scrolling tree handled it for animated scroll", and "scrolling tree delayed handling for delegated scrolling" so add `RequestsScrollHandling` to indicate that. The changes in `ScrollingTreeScrollingNode::handleScrollPositionRequest()` and `RemoteScrollingCoordinatorProxy::adjustMainFrameDelegatedScrollPosition()` are needed to handle the iOS-specific behavior, seen in `RemoteLayerTreeDrawingAreaProxy::commitLayerTreeTransaction()`, where the application of the `requestedScroll` is delayed until after layer tree commit; `didHandleScrollRequestForNode()` needs to be called once the final scroll position has been computed (this is tested by existing tests). Tests: fast/scrolling/mac/momentum-then-programmatic-hit-test.html fast/scrolling/mac/momentum-then-programmatic-missing-tiles.html * LayoutTests/fast/scrolling/mac/momentum-then-programmatic-hit-test-expected.txt: Added. * LayoutTests/fast/scrolling/mac/momentum-then-programmatic-hit-test.html: Added. * LayoutTests/fast/scrolling/mac/momentum-then-programmatic-missing-tiles-expected.html: Added. * LayoutTests/fast/scrolling/mac/momentum-then-programmatic-missing-tiles.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-anchoring/dirty-contents-reselect-anchor.tentative-expected.txt: * LayoutTests/platform/ios/TestExpectations: * Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp: (WebCore::AsyncScrollingCoordinator::requestScrollToPosition): (WebCore::AsyncScrollingCoordinator::synchronizeStateFromScrollingTree): (WebCore::AsyncScrollingCoordinator::applyScrollPositionUpdate): (WebCore::AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll): * Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h: * Source/WebCore/page/scrolling/ScrollingCoordinatorTypes.cpp: (WebCore::ScrollUpdate::canMerge const): (WebCore::ScrollUpdate::merge): (WebCore::operator<<): * Source/WebCore/page/scrolling/ScrollingCoordinatorTypes.h: (WebCore::ScrollUpdate::canMerge const): Deleted. (WebCore::ScrollUpdate::merge): Deleted. * Source/WebCore/page/scrolling/ScrollingTree.h: (WebCore::ScrollingTree::scrollingTreeNodeDidStopWheelEventScroll): (WebCore::ScrollingTree::scrollingTreeNodeRequestsScroll): (WebCore::ScrollingTree::didHandleScrollRequestForNode): (WebCore::ScrollingTree::scrollingTreeNodeDidStopProgrammaticScroll): Deleted. * Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp: (WebCore::ScrollingTreeScrollingNode::handleScrollPositionRequest): (WebCore::ScrollingTreeScrollingNode::didStopProgrammaticScroll): Deleted. * Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp: (WebCore::ThreadedScrollingTree::scrollingTreeNodeRequestsScroll): (WebCore::ThreadedScrollingTree::scrollingTreeNodeDidScroll): (WebCore::ThreadedScrollingTree::didHandleScrollRequestForNode): (WebCore::ThreadedScrollingTree::scrollingTreeNodeScrollUpdated): * Source/WebCore/page/scrolling/ThreadedScrollingTree.h: * Source/WebKit/Shared/WebCoreArgumentCoders.serialization.in: * Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp: (WebKit::RemoteScrollingCoordinatorProxy::adjustMainFrameDelegatedScrollPosition): (WebKit::RemoteScrollingCoordinatorProxy::sendScrollingTreeNodeUpdate): (WebKit::RemoteScrollingCoordinatorProxy::scrollingTreeNodeRequestsScroll): * Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.h: * Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingTree.cpp: (WebKit::RemoteScrollingTree::scrollingTreeNodeDidScroll): (WebKit::RemoteScrollingTree::scrollingTreeNodeDidStopAnimatedScroll): (WebKit::RemoteScrollingTree::scrollingTreeNodeDidStopWheelEventScroll): (WebKit::RemoteScrollingTree::scrollingTreeNodeRequestsScroll): (WebKit::RemoteScrollingTree::didHandleScrollRequestForNode): (WebKit::RemoteScrollingTree::scrollingTreeNodeDidStopProgrammaticScroll): Deleted. * Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingTree.h: * Source/WebKit/UIProcess/RemoteLayerTree/mac/RemoteScrollingCoordinatorProxyMac.h: * Source/WebKit/UIProcess/RemoteLayerTree/mac/RemoteScrollingCoordinatorProxyMac.mm: (WebKit::RemoteScrollingCoordinatorProxyMac::scrollingTreeNodeRequestsScroll): * Source/WebKit/UIProcess/RemoteLayerTree/mac/RemoteScrollingTreeMac.h: * Source/WebKit/UIProcess/RemoteLayerTree/mac/RemoteScrollingTreeMac.mm: (WebKit::RemoteScrollingTreeMac::scrollingTreeNodeDidScroll): (WebKit::RemoteScrollingTreeMac::scrollingTreeNodeDidStopAnimatedScroll): (WebKit::RemoteScrollingTreeMac::scrollingTreeNodeDidStopWheelEventScroll): (WebKit::RemoteScrollingTreeMac::scrollingTreeNodeRequestsScroll): (WebKit::RemoteScrollingTreeMac::scrollingTreeNodeDidStopProgrammaticScroll): Deleted. * Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.h: * Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.mm: (WebKit::RemoteScrollingCoordinator::willSendScrollPositionRequest): (WebKit::RemoteScrollingCoordinator::scrollUpdateForNode): * Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm: (WebKit::WebPage::updateVisibleContentRects): Canonical link: https://commits.webkit.org/308215@main
1 parent 9817f5b commit e869783

26 files changed

+645
-190
lines changed

‎LayoutTests/fast/scrolling/mac/momentum-then-programmatic-hit-test-expected.txt‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
PASS clickCount is 1
2+
PASS successfullyParsed is true
3+
4+
TEST COMPLETE
5+

‎LayoutTests/fast/scrolling/mac/momentum-then-programmatic-hit-test.html‎

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<style>
5+
.spacer {
6+
height: 4000px;
7+
width: 10px;
8+
background-color: silver;
9+
}
10+
11+
.target {
12+
margin-top: 120px;
13+
height: 50px;
14+
width: 300px;
15+
background-color: green;
16+
}
17+
18+
.target:active {
19+
background-color: orange;
20+
}
21+
</style>
22+
<script src="../../../resources/ui-helper.js"></script>
23+
<script src="../../../resources/js-test.js"></script>
24+
<script>
25+
jsTestIsAsync = true;
26+
27+
let clickCount = 0;
28+
async function scrollTest()
29+
{
30+
const scroller = document.getElementById("scroller");
31+
32+
await UIHelper.startMonitoringWheelEvents();
33+
eventSender.mouseMoveTo(100, 100);
34+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -2, "began", "none");
35+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -15, "changed", "none");
36+
await UIHelper.renderingUpdate();
37+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, "ended", "none");
38+
39+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, "none", "begin");
40+
await UIHelper.renderingUpdate();
41+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -20, "none", "continue");
42+
await UIHelper.renderingUpdate();
43+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -20, "none", "continue");
44+
await UIHelper.renderingUpdate();
45+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -20, "none", "continue");
46+
await UIHelper.renderingUpdate();
47+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -10, "none", "continue");
48+
49+
window.scrollTo(0, 100);
50+
51+
await UIHelper.renderingUpdate();
52+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, "none", "end");
53+
54+
await UIHelper.waitForScrollCompletion();
55+
await doClick();
56+
57+
shouldBe('clickCount', '1');
58+
59+
finishJSTest();
60+
}
61+
62+
async function doClick()
63+
{
64+
if (!window.eventSender)
65+
return;
66+
67+
eventSender.mouseMoveTo(100, 25);
68+
eventSender.mouseDown();
69+
await UIHelper.delayFor(20);
70+
eventSender.mouseUp();
71+
}
72+
73+
function targetClicked()
74+
{
75+
++clickCount;
76+
}
77+
78+
window.addEventListener('load', () => {
79+
const scroller = document.getElementById("scroller");
80+
81+
if (!window.eventSender) {
82+
setTimeout(scrollTest, 0);
83+
return;
84+
}
85+
86+
testRunner.waitUntilDone();
87+
88+
setTimeout(scrollTest, 0);
89+
}, false);
90+
</script>
91+
</head>
92+
<body>
93+
<div class="target" onclick="targetClicked()"></div>
94+
<div id="console"></div>
95+
<div class="spacer"></div>
96+
</body>
97+
</html>

‎LayoutTests/fast/scrolling/mac/momentum-then-programmatic-missing-tiles-expected.html‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
<!DOCTYPE html> <!-- webkit-test-runner [ AsyncOverflowScrollingEnabled=true ] -->
2+
<html>
3+
<head>
4+
<style>
5+
.list {
6+
width: 400px;
7+
height: 400px;
8+
display: flex;
9+
flex-flow: row nowrap;
10+
overflow-x: scroll;
11+
overflow-y: hidden;
12+
border: 1px solid black;
13+
padding: 0;
14+
margin: 0;
15+
}
16+
17+
.list li {
18+
flex: none;
19+
width: 100%;
20+
height: 100%;
21+
padding: 0;
22+
margin: 0;
23+
list-style-type: none;
24+
}
25+
</style>
26+
</head>
27+
<body>
28+
<ul id="scroller" class="list">
29+
<li style="background: #ff0;">test</li>
30+
<li style="background: #0f0;">test</li>
31+
<li style="background: #0ff;">test</li>
32+
<li style="background: #00f;">test</li>
33+
<li style="background: #f0f;">test</li>
34+
<li style="background: #f00;">test</li>
35+
<li style="background: #ff0;">test</li>
36+
<li style="background: #0f0;">test</li>
37+
<li style="background: #0ff;">test</li>
38+
<li style="background: #00f;">test</li>
39+
<li style="background: #f0f;">test</li>
40+
<li style="background: #f00;">test</li>
41+
</ul>
42+
<div id="result"></div>
43+
</body>
44+
</html>

‎LayoutTests/fast/scrolling/mac/momentum-then-programmatic-missing-tiles.html‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
<!DOCTYPE html> <!-- webkit-test-runner [ AsyncOverflowScrollingEnabled=true ] -->
2+
<html>
3+
<head>
4+
<style>
5+
.list {
6+
width: 400px;
7+
height: 400px;
8+
display: flex;
9+
flex-flow: row nowrap;
10+
overflow-x: scroll;
11+
overflow-y: hidden;
12+
border: 1px solid black;
13+
padding: 0;
14+
margin: 0;
15+
}
16+
17+
.list li {
18+
flex: none;
19+
width: 100%;
20+
height: 100%;
21+
padding: 0;
22+
margin: 0;
23+
list-style-type: none;
24+
}
25+
</style>
26+
<script src="../../../resources/ui-helper.js"></script>
27+
<script>
28+
async function scrollTest()
29+
{
30+
const scroller = document.getElementById("scroller");
31+
32+
await UIHelper.startMonitoringWheelEvents();
33+
eventSender.mouseMoveTo(100, 100);
34+
eventSender.mouseScrollByWithWheelAndMomentumPhases(-2, 0, "began", "none");
35+
eventSender.mouseScrollByWithWheelAndMomentumPhases(-15, 0, "changed", "none");
36+
await UIHelper.renderingUpdate();
37+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, "ended", "none");
38+
39+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, "none", "begin");
40+
await UIHelper.renderingUpdate();
41+
eventSender.mouseScrollByWithWheelAndMomentumPhases(-20, 0, "none", "continue");
42+
await UIHelper.renderingUpdate();
43+
eventSender.mouseScrollByWithWheelAndMomentumPhases(-20, 0, "none", "continue");
44+
await UIHelper.renderingUpdate();
45+
eventSender.mouseScrollByWithWheelAndMomentumPhases(-20, 0, "none", "continue");
46+
await UIHelper.renderingUpdate();
47+
eventSender.mouseScrollByWithWheelAndMomentumPhases(-10, 0, "none", "continue");
48+
await UIHelper.renderingUpdate();
49+
eventSender.mouseScrollByWithWheelAndMomentumPhases(-20, 0, "none", "continue");
50+
await UIHelper.renderingUpdate();
51+
52+
eventSender.mouseScrollByWithWheelAndMomentumPhases(-20, 0, "none", "continue");
53+
await UIHelper.renderingUpdate();
54+
eventSender.mouseScrollByWithWheelAndMomentumPhases(-10, 0, "none", "continue");
55+
56+
scroller.scrollTo(0, 0);
57+
58+
await UIHelper.renderingUpdate();
59+
eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, "none", "end");
60+
61+
await UIHelper.waitForScrollCompletion();
62+
63+
await UIHelper.renderingUpdate();
64+
testRunner.notifyDone();
65+
}
66+
67+
window.addEventListener('load', () => {
68+
const scroller = document.getElementById("scroller");
69+
70+
if (!window.eventSender) {
71+
setTimeout(scrollTest, 0);
72+
return;
73+
}
74+
75+
testRunner.waitUntilDone();
76+
77+
setTimeout(scrollTest, 0);
78+
}, false);
79+
</script>
80+
</head>
81+
<body>
82+
<ul id="scroller" class="list">
83+
<li style="background: #ff0;">test</li>
84+
<li style="background: #0f0;">test</li>
85+
<li style="background: #0ff;">test</li>
86+
<li style="background: #00f;">test</li>
87+
<li style="background: #f0f;">test</li>
88+
<li style="background: #f00;">test</li>
89+
<li style="background: #ff0;">test</li>
90+
<li style="background: #0f0;">test</li>
91+
<li style="background: #0ff;">test</li>
92+
<li style="background: #00f;">test</li>
93+
<li style="background: #f0f;">test</li>
94+
<li style="background: #f00;">test</li>
95+
</ul>
96+
<div id="result"></div>
97+
</body>
98+
</html>

‎LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-anchoring/dirty-contents-reselect-anchor.tentative-expected.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
Scroll target
22

3-
FAIL Scroll anchor is re-selected after adjustment if there are dirty descendants at selection time assert_equals: Scroll position should've been preserved expected 1207.34375 but got 0.34375
3+
PASS Scroll anchor is re-selected after adjustment if there are dirty descendants at selection time
44

‎LayoutTests/platform/ios/TestExpectations‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3674,7 +3674,6 @@ webkit.org/b/296195 imported/w3c/web-platform-tests/dom/events/scrolling/scrolle
36743674
webkit.org/b/296195 imported/w3c/web-platform-tests/dom/events/scrolling/scrollend-event-fired-after-snap.html [ Skip ]
36753675
webkit.org/b/296195 imported/w3c/web-platform-tests/dom/events/scrolling/scrollend-event-fired-for-scrollIntoView.html?include=subframe-inline-end-block-end-behavior-auto [ Skip ]
36763676
webkit.org/b/296195 imported/w3c/web-platform-tests/dom/events/scrolling/scrollend-event-fired-for-scrollIntoView.html?include=subframe-inline-start-block-start-behavior-smooth [ Skip ]
3677-
webkit.org/b/307555 imported/w3c/web-platform-tests/dom/events/scrolling/scrollend-event-fired-for-programmatic-scroll.html?include=root-scrollBy-smooth [ Failure ]
36783677

36793678
webkit.org/b/231337 imported/w3c/web-platform-tests/xhr/send-timeout-events.htm [ Pass Failure ]
36803679

@@ -4640,9 +4639,6 @@ fast/forms/ios/show-file-upload-context-menu-above-keyboard.html [ Pass ]
46404639
# rdar://110034620 (REGRESSION ( iOS17 ): [ iOS17 ] compositing/clipping/nested-overflow-with-border-radius.html is a consistent image failure)
46414640
compositing/clipping/nested-overflow-with-border-radius.html [ ImageOnlyFailure ]
46424641

4643-
# rdar://104451028 (REGRESSION (iOS17 ): [ iOS17 ] fast/scrolling/keyboard-scrolling-distance-pageDown.html is a consistent timeout)
4644-
fast/scrolling/keyboard-scrolling-distance-pageDown.html [ Timeout ]
4645-
46464642
# rdar://104460194 (REGRESSION (iOS17 Sonoma23A177 ): [ iOS17 Sonoma ] fast/box-shadow/inset-box-shadow-fractional-radius.html is a consistent image failure)
46474643
fast/box-shadow/inset-box-shadow-fractional-radius.html [ ImageOnlyFailure ]
46484644

@@ -4699,7 +4695,6 @@ fast/borders/hidpi-border-painting-groove.html [ ImageOnlyFailure ]
46994695
fast/borders/hidpi-border-painting-ridge.html [ ImageOnlyFailure ]
47004696
imported/w3c/web-platform-tests/css/css-text/white-space/textarea-pre-wrap-013.html [ ImageOnlyFailure ]
47014697
editing/spelling/editing-word-with-marker-1.html [ Failure ]
4702-
fast/scrolling/ios/body-overflow-hidden-height-100-percent-keyboard.html [ Failure ]
47034698
http/wpt/webcodecs/videoFrame-colorSpace.html [ Failure ]
47044699
http/wpt/webcodecs/webcodecs-gc.html [ Failure ]
47054700
http/wpt/webrtc/video-script-transform-keyframe-only.html [ Failure ]

0 commit comments

Comments
 (0)