Bug 226816 – [iOS 15 Regression]: scroll position resets when using scroll snap

• Component: Scrolling
• Status: RESOLVED FIXED

[Liam DeBeasi]

On iOS 15 beta 1, it appears that scroll snapping is broken and resets the scroll position whenever you try to scroll.

Steps to reproduce:

1. Open https://webkit.org/demos/scroll-snap/ on a device running iOS 15.
2. Go to the second demo (the one that says "This second container shows what basic scroll snapping can do for us")
3. Try to scroll.
4. Observe that the scroll position is reset to the beginning when you let go of the screen.

Expected Behavior:

I would expect that scrolling continues until the container snaps into place.

Actual Behavior:

Scrolling resets to the beginning. Alternatively, it could be that WebKit is snapping back to the first element, but I am not entirely sure.

[Radar WebKit Bug Importer]

<rdar://problem/79081301>

[Simon Fraser (smfr)]

This affects tesla.com

[Simon Fraser (smfr)]

Two possible issues here. One is that I'm not convinced that the logic towards the end of closestSnapOffsetWithInfoAndAxis() is correct.

Secondly, and more seriously, we're snapping in the WebContent process when receiving notifications about scrolls from the UI process, which is also snapping, and these are fighting with each other.

UI process:

  * frame #0: 0x0000000128b37e05 WebCore`std::__1::pair<float, unsigned int> WebCore::closestSnapOffsetWithInfoAndAxis<WebCore::ScrollSnapOffsetsInfo<float, WebCore::FloatRect>, WebCore::FloatSize, float>(info=0x0000000145eafe08, axis=Vertical, viewportSize={ width = 980.0, height = 1924.89063 }, scrollDestinationOffset=351.090332, velocity=2.98693037, originalOffsetForDirectionalSnapping= Has Value=false ) at ScrollSnapOffsetsInfo.cpp:146:56
    frame #1: 0x0000000128b3796b WebCore`std::__1::pair<float, unsigned int> WebCore::ScrollSnapOffsetsInfo<float, WebCore::FloatRect>::closestSnapOffset<WebCore::FloatSize>(this=0x0000000145eafe08, axis=Vertical, viewportSize={ width = 980.0, height = 1924.89063 }, scrollDestinationOffset=351.090332, velocity=2.98693037, originalPositionForDirectionalSnapping= Has Value=false ) const at ScrollSnapOffsetsInfo.cpp:387:12
    frame #2: 0x0000000114f65d52 WebKit`WebKit::RemoteScrollingCoordinatorProxy::closestSnapOffsetForMainFrameScrolling(this=0x0000000145ee62f8, axis=Vertical, scrollDestination=153.333328, velocity=2.98693037, currentIndex=0x0000000145ee636c) const at RemoteScrollingCoordinatorProxyIOS.mm:244:68
    frame #3: 0x0000000114f659a4 WebKit`WebKit::RemoteScrollingCoordinatorProxy::adjustTargetContentOffsetForSnapping(this=0x0000000145ee62f8, maxScrollOffsets=(width = 0, height = 139.33333333333326), velocity=(x = 0, y = 2.986930437998057), topInset=66, targetContentOffset=(x = 0, y = 153.33333333333334)) at RemoteScrollingCoordinatorProxyIOS.mm:209:39
    frame #4: 0x0000000114d044da WebKit`-[WKWebView(self=Failed to get the 'some' field from optional 'self', _cmd="scrollViewWillEndDragging:withVelocity:targetContentOffset:", scrollView=Failed to get the 'some' field from optional 'scrollView', velocity=(x = 0, y = 2.986930437998057), targetContentOffset=(x = 0, y = 153.33333333333334)) scrollViewWillEndDragging:withVelocity:targetContentOffset:] at WKWebViewIOS.mm:1589:22

WebContent process:

  * frame #0: 0x00000002128a0834 WebCore`std::__1::pair<WebCore::LayoutUnit, unsigned int> WebCore::closestSnapOffsetWithInfoAndAxis<WebCore::ScrollSnapOffsetsInfo<WebCore::LayoutUnit, WebCore::LayoutRect>, WebCore::LayoutSize, WebCore::LayoutUnit>(info=0x000000022a4edc10, axis=Vertical, viewportSize={ width = 980px (62720), height = 1708px (109312) }, scrollDestinationOffset={ 57px (3648) }, velocity=0, originalOffsetForDirectionalSnapping= Has Value=false ) at ScrollSnapOffsetsInfo.cpp:146:56
    frame #1: 0x00000002128a033d WebCore`std::__1::pair<WebCore::LayoutUnit, unsigned int> WebCore::ScrollSnapOffsetsInfo<WebCore::LayoutUnit, WebCore::LayoutRect>::closestSnapOffset<WebCore::LayoutSize>(this=0x000000022a4edc10, axis=Vertical, viewportSize={ width = 980px (62720), height = 1708px (109312) }, scrollDestinationOffset={ 57px (3648) }, velocity=0, originalPositionForDirectionalSnapping= Has Value=false ) const at ScrollSnapOffsetsInfo.cpp:381:12
    frame #2: 0x00000002129159f2 WebCore`WebCore::ScrollController::setNearestScrollSnapIndexForAxisAndOffset(this=0x0000000230af6528, axis=Vertical, offset=57) at ScrollController.cpp:157:55
    frame #3: 0x000000021291462c WebCore`WebCore::ScrollController::setActiveScrollSnapIndicesForOffset(this=0x0000000230af6528, offset={ x = 0, y = 57 }) at ScrollController.cpp:190:5
    frame #4: 0x0000000212913c7f WebCore`WebCore::ScrollAnimator::updateActiveScrollSnapIndexForOffset(this=0x0000000230af6510) at ScrollAnimator.cpp:277:24
    frame #5: 0x00000002129145c7 WebCore`WebCore::ScrollAnimator::setCurrentPosition(this=0x0000000230af6510, position={ x = 0.0, y = 57.0 }) at ScrollAnimator.cpp:270:5
    frame #6: 0x000000021292b20b WebCore`WebCore::ScrollableArea::notifyScrollPositionChanged(this=0x000000022efc8010, position={ x = 0, y = 57 }) at ScrollableArea.cpp:171:22
    frame #7: 0x0000000212887a06 WebCore`WebCore::AsyncScrollingCoordinator::reconcileScrollingState(this=0x000000022a4cb5e8, frameView=0x000000022efc8010, scrollPosition={ x = 0.0, y = 57.0 }, layoutViewportOriginOrOverrideRect=0x00007ffeef4f1cb8, scrollType=User, viewportRectStability=ChangingObscuredInsetsInteractively, scrollingLayerPositionAction=SetApproximate) at AsyncScrollingCoordinator.cpp:417:15
    frame #8: 0x00000001f1a82219 WebKit`WebKit::WebPage::updateVisibleContentRects(this=0x00007f9100008208, visibleContentRectUpdateInfo=0x000000022a441000, oldestTimestamp=(m_value = 2837791.1456069001)) at WebPageIOS.mm:4151:31
    frame #9: 0x00000001f1fb6375 WebKit`WebKit::ViewUpdateDispatcher::dispatchVisibleContentRectUpdate(this=0x000000022a4ec050) at ViewUpdateDispatcher.cpp:88:22
    frame #10: 0x00000001f1fc8a7d WebKit`WebKit::ViewUpdateDispatcher::visibleContentRectUpdate(this=0x000000022a4110b8)::$_1::operator()() at ViewUpdateDispatcher.cpp:73:28
    frame #11: 0x00000001f1fc88ee WebKit`WTF::Detail::CallableWrapper<WebKit::ViewUpdateDispatcher::visibleContentRectUpdate(WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::VisibleContentRectUpdateInfo const&)::$_1, void>::call(this=0x000000022a4110b0) at Function.h:53:39

[Martin Robinson]

Here's what I'm seeing:

Expected behavior:
When either dragging the scroll view and releasing or momentum scrolling (without enough momentum to move to the next scroll position), the view should animate while snapping back to the previous snap point.

Actual behavior:
When dragging the scroll view and releasing, the view animates back as expected. When using momentum scrolling (but not enough to move to the next snap position), the view snaps back without animation.

So there are two issues here, I think:

1. The lack of animation when snapping back after a momentum scrolling. I'm investigating this now. 
2. Whether momentum scrolls *should* snap back at all. It's arguable that any momentum scroll at all, no matter how small should always snap to the next snap position. This is a very trivial change.

Regardless of what happens for 2, there are situations where we must snap back and in those cases it should animate back to the original snap position.

[Martin Robinson]

Created attachment 431966 [details]
Patch

[Martin Robinson]

(In reply to Simon Fraser (smfr) from comment #3)
> Two possible issues here. One is that I'm not convinced that the logic
> towards the end of closestSnapOffsetWithInfoAndAxis() is correct.
> 
> Secondly, and more seriously, we're snapping in the WebContent process when
> receiving notifications about scrolls from the UI process, which is also
> snapping, and these are fighting with each other.
> 

I also noticed this the other day. I think the call to closestSnapOffset in the WebContent process does not actually snap, but updates the value of m_activeSnapIndexX/m_activesnapIndexY in ScrollSnapAnimatorState. You can see that ScrollController::updateActiveScrollSnapIndexForClientOffset only ultimately calls m_scrollSnapState->setActiveSnapIndexForAxis(axis, index). Still, this seems completely unnecessary since the same value should also be in the serialized ScrollingTree update. I don't totally understand why the code was designed to constantly recalculate the "active snap point" like this. I'm going to write a patch to try to remove this, but I don't think it will ultimately change behavior or fix this issue.

[Martin Robinson]

I have uploaded a patch which I think fixes the issue with tesla.com and the webkit.org demo. What it does is to always move to the next or previous scroll point for momentum scrolling. This should also ensure that "strong" iOS momentum scrolls work properly with `scroll-snap-stop`.

[Martin Robinson]

Created attachment 431968 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 431968 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431968&action=review

r+ but please add a test.

> Source/WebKit/ChangeLog:14
> +        No new tests because iOS doesn't currently run scroll snap tests relying on EventSender.

iOS can run touch-based tests with UIScriptController. There are lots in fast/scrolling/ios

> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteScrollingCoordinatorProxyIOS.mm:235
> +std::pair<float, std::optional<unsigned>> RemoteScrollingCoordinatorProxy::closestSnapOffsetForMainFrameScrolling(ScrollEventAxis axis, float scrollOrigin, float scrollDestination, float velocity) const

scrollOrigin is a confusing name. Call it currentScrollOffset?

[Martin Robinson]

Created attachment 432037 [details]
Patch

[Martin Robinson]

Created attachment 432062 [details]
Patch

[EWS]

Committed r279195 (239085@main): <https://commits.webkit.org/239085@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 432062 [details].

[Liam DeBeasi]

Thank you! I will give it a test whenever it comes out on iOS.

[Simon Fraser (smfr)]

*** Bug 227580 has been marked as a duplicate of this bug. ***