Skip to content

Commit

Permalink
REGRESSION (269897@main): [ Sonoma wk2 ] 2 tests in imported/w3c/web-…
Browse files Browse the repository at this point in the history 
…platform-tests/css/cssom-view/ are a constant failure

https://bugs.webkit.org/show_bug.cgi?id=263925
rdar://117706782

Reviewed by Richard Robinson.

Fix a bug introduced in 269897@main, and an older bug that prevented scroll-behavior-main-frame* tests from passing.

In 269897@main the intermediate scrollTo() that we do when we have `requestedDataBeforeAnimatedScroll` data needs
to also contribute its destination position to the animated scroll that happens second, but both
`ScrollingTreeScrollingNode::handleScrollPositionRequest()` and `RemoteScrollingTreeMac::startPendingScrollAnimations()`
failed to do this.

Even wit this fix, two WPT tests were failing when they made the following sequence of programmatic scrolls:
    scrollBy(0, X, animated)
    scrollTo(0, 0)
    scrollBy(0, Y, animated)

We'd end up scrolling to X + Y because `ScrollView::setScrollPosition()` early returned if there was no position
change for the `scrollTo(0, 0)`. But we need to hit `requestScrollToPosition()` so that we have a chance to
cancel the enqueued animated scroll, so remove the `!delegatesScrollingToNativeView()` check.

* LayoutTests/platform/mac-ventura-wk2/TestExpectations:
* LayoutTests/platform/mac-wk2/TestExpectations:
* Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:
(WebCore::ScrollingTreeScrollingNode::handleScrollPositionRequest):
* Source/WebCore/platform/ScrollView.cpp:
(WebCore::ScrollView::setScrollPosition):
* Source/WebKit/UIProcess/RemoteLayerTree/mac/RemoteScrollingTreeMac.mm:
(WebKit::RemoteScrollingTreeMac::startPendingScrollAnimations):

Canonical link: https://commits.webkit.org/270243@main
  • Loading branch information
@smfr
smfr committed Nov 5, 2023
1 parent a2bec72 commit a53d9e0
Show file tree
Hide file tree
Showing 9 changed files with 22 additions and 19 deletions.
3 changes: 3 additions & 0 deletions LayoutTests/platform/mac-ventura-wk2/TestExpectations
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,6 @@ webkit.org/b/261444 [ Debug x86_64 ] http/tests/security/referrer-policy-header.

# Asserts on pre-Sonoma macOS: rdar://116291539
[ Debug ] http/tests/site-isolation/window-properties.html [ Skip ]

# webkit.org/b/263955 Twelve css WPTs are failing with async overflow scrolling enabled on macOS
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-smooth-positions.html [ Failure ]
5 changes: 0 additions & 5 deletions LayoutTests/platform/mac-wk2/TestExpectations
View file
Original file line number Diff line number Diff line change
Expand Up @@ -614,7 +614,6 @@ imported/w3c/web-platform-tests/css/css-ui/outline-negative-offset-composited-sc
imported/w3c/web-platform-tests/css/css-scroll-snap/snap-inline-block.html [ Failure ]
imported/w3c/web-platform-tests/css/css-transforms/transform3d-preserve3d-001.html [ ImageOnlyFailure ]
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-element.html [ Failure ]
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-smooth-positions.html [ Failure ]

### END OF (1) Classified failures with bug reports
########################################
Expand Down Expand Up @@ -1986,7 +1985,3 @@ http/tests/webgpu/webgpu/api/operation/render_pipeline/overrides.html [ Pass ]
http/tests/webgpu/webgpu/api/operation/render_pipeline/primitive_topology.html [ Pass ]

webkit.org/b/263920 svg/transforms/transformed-text-fill-gradient.html [ ImageOnlyFailure ]

# rdar://117706782 (REGRESSION (269897@main): [ Sonoma wk2 ] 2 tests in imported/w3c/web-platform-t ests/css/cssom-view/ are a constant failure
[ Sonoma+ ] imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-window.html [ Failure ]
[ Sonoma+ ] imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-root.html [ Failure ]
3 changes: 3 additions & 0 deletions LayoutTests/tiled-drawing/scrolling/clamp-out-of-bounds-scrolls-expected.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ Attempted scroll to -5000, 0
(Frame scrolling node
(scrollable area size 785 585)
(contents size 5008 5021)
(requested scroll position represents programmatic scroll 1)
(scrollable area parameters
(horizontal scroll elasticity 2)
(vertical scroll elasticity 2)
Expand All @@ -21,6 +22,7 @@ Attempted scroll to 0, -5000
(Frame scrolling node
(scrollable area size 785 585)
(contents size 5008 5021)
(requested scroll position represents programmatic scroll 1)
(scrollable area parameters
(horizontal scroll elasticity 2)
(vertical scroll elasticity 2)
Expand All @@ -39,6 +41,7 @@ Attempted scroll to -5000, -5000
(Frame scrolling node
(scrollable area size 785 585)
(contents size 5008 5021)
(requested scroll position represents programmatic scroll 1)
(scrollable area parameters
(horizontal scroll elasticity 2)
(vertical scroll elasticity 2)
Expand Down
1 change: 1 addition & 0 deletions LayoutTests/tiled-drawing/scrolling/fixed/negative-scroll-offset-expected.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
(Frame scrolling node
(scrollable area size 785 600)
(contents size 785 2221)
(requested scroll position represents programmatic scroll 1)
(scrollable area parameters
(horizontal scroll elasticity 2)
(vertical scroll elasticity 2)
Expand Down
1 change: 1 addition & 0 deletions LayoutTests/tiled-drawing/scrolling/fixed/negative-scroll-offset-in-view-expected.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
(Frame scrolling node
(scrollable area size 785 600)
(contents size 785 2221)
(requested scroll position represents programmatic scroll 1)
(scrollable area parameters
(horizontal scroll elasticity 2)
(vertical scroll elasticity 2)
Expand Down
1 change: 1 addition & 0 deletions LayoutTests/tiled-drawing/scrolling/sticky/negative-scroll-offset-expected.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
(Frame scrolling node
(scrollable area size 785 600)
(contents size 785 2216)
(requested scroll position represents programmatic scroll 1)
(scrollable area parameters
(horizontal scroll elasticity 2)
(vertical scroll elasticity 2)
Expand Down
11 changes: 5 additions & 6 deletions Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,27 +301,27 @@ void ScrollingTreeScrollingNode::requestKeyboardScroll(const RequestedKeyboardSc

void ScrollingTreeScrollingNode::handleScrollPositionRequest(const RequestedScrollData& requestedScrollData)
{
LOG_WITH_STREAM(Scrolling, stream << "ScrollingTreeScrollingNode " << scrollingNodeID() << " handleScrollPositionRequest()" << " animated " << (requestedScrollData.animated == ScrollIsAnimated::Yes) << " requestedScrollData: " << requestedScrollData);

stopAnimatedScroll();

if (requestedScrollData.requestType == ScrollRequestType::CancelAnimatedScroll) {
ASSERT(!requestedScrollData.requestedDataBeforeAnimatedScroll);
LOG_WITH_STREAM(Scrolling, stream << "ScrollingTreeScrollingNode " << scrollingNodeID() << " handleScrollPositionRequest() - cancel animated scroll");
scrollingTree().removePendingScrollAnimationForNode(scrollingNodeID());
return;
}

if (scrollingTree().scrollingTreeNodeRequestsScroll(scrollingNodeID(), requestedScrollData))
return;

auto currentScrollPosition = this->currentScrollPosition();
LOG_WITH_STREAM(Scrolling, stream << "ScrollingTreeScrollingNode " << scrollingNodeID() << " handleScrollPositionRequest() with data " << requestedScrollData);

if (requestedScrollData.requestedDataBeforeAnimatedScroll) {
auto& [requestType, positionOrDeltaBeforeAnimatedScroll, scrollType, clamping] = *requestedScrollData.requestedDataBeforeAnimatedScroll;

switch (requestType) {
case ScrollRequestType::PositionUpdate:
case ScrollRequestType::DeltaUpdate: {
auto intermediatePosition = RequestedScrollData::computeDestinationPosition(currentScrollPosition, requestType, positionOrDeltaBeforeAnimatedScroll);
auto intermediatePosition = RequestedScrollData::computeDestinationPosition(currentScrollPosition(), requestType, positionOrDeltaBeforeAnimatedScroll);
scrollTo(intermediatePosition, scrollType, clamping);
break;
}
Expand All @@ -331,8 +331,7 @@ void ScrollingTreeScrollingNode::handleScrollPositionRequest(const RequestedScro
}
}

auto destinationPosition = requestedScrollData.destinationPosition(currentScrollPosition);

auto destinationPosition = requestedScrollData.destinationPosition(currentScrollPosition());
if (requestedScrollData.animated == ScrollIsAnimated::Yes) {
startAnimatedScrollToPosition(destinationPosition);
return;
Expand Down
6 changes: 4 additions & 2 deletions Source/WebCore/platform/ScrollView.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -552,8 +552,10 @@ void ScrollView::setScrollPosition(const ScrollPosition& scrollPosition, const S
return;
}

ScrollPosition newScrollPosition = (!delegatesScrollingToNativeView() && options.clamping == ScrollClamping::Clamped) ? adjustScrollPositionWithinRange(scrollPosition) : scrollPosition;
if ((!delegatesScrollingToNativeView() || currentScrollType() == ScrollType::User) && newScrollPosition == this->scrollPosition()) {
auto newScrollPosition = (!delegatesScrollingToNativeView() && options.clamping == ScrollClamping::Clamped) ? adjustScrollPositionWithinRange(scrollPosition) : scrollPosition;
bool scrollPositionChanged = newScrollPosition != this->scrollPosition();

if (currentScrollType() == ScrollType::User && !scrollPositionChanged) {
LOG_WITH_STREAM(Scrolling, stream << "ScrollView::setScrollPosition " << scrollPosition << " return for no change");
return;
}
Expand Down
10 changes: 4 additions & 6 deletions Source/WebKit/UIProcess/RemoteLayerTree/mac/RemoteScrollingTreeMac.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,23 +153,20 @@
ASSERT(m_treeLock.isLocked());

auto nodesWithPendingScrollAnimations = std::exchange(m_nodesWithPendingScrollAnimations, { });

LOG_WITH_STREAM(Scrolling, stream << "RemoteScrollingTreeMac::startPendingScrollAnimations() - " << nodesWithPendingScrollAnimations.size() << " nodes with pending animations");

for (auto& [nodeID, data] : nodesWithPendingScrollAnimations) {
RefPtr targetNode = dynamicDowncast<ScrollingTreeScrollingNode>(nodeForID(nodeID));
if (!targetNode)
continue;

auto currentScrollPosition = targetNode->currentScrollPosition();
LOG_WITH_STREAM(Scrolling, stream << "RemoteScrollingTreeMac::startPendingScrollAnimations() for node " << nodeID << " with data " << data);

if (auto previousData = std::exchange(data.requestedDataBeforeAnimatedScroll, std::nullopt)) {
auto& [requestType, positionOrDeltaBeforeAnimatedScroll, scrollType, clamping] = *previousData;

switch (requestType) {
case ScrollRequestType::PositionUpdate:
case ScrollRequestType::DeltaUpdate: {
auto intermediatePosition = RequestedScrollData::computeDestinationPosition(currentScrollPosition, requestType, positionOrDeltaBeforeAnimatedScroll);
auto intermediatePosition = RequestedScrollData::computeDestinationPosition(targetNode->currentScrollPosition(), requestType, positionOrDeltaBeforeAnimatedScroll);
targetNode->scrollTo(intermediatePosition, scrollType, clamping);
break;
}
Expand All @@ -179,7 +176,8 @@
}
}

targetNode->startAnimatedScrollToPosition(data.destinationPosition(currentScrollPosition));
auto finalPosition = data.destinationPosition(targetNode->currentScrollPosition());
targetNode->startAnimatedScrollToPosition(finalPosition);
}

auto nodesWithPendingKeyboardScrollAnimations = std::exchange(m_nodesWithPendingKeyboardScrollAnimations, { });
Expand Down

0 comments on commit a53d9e0

Please sign in to comment.