[webkit-changes] [WebKit/WebKit] f4b233: [iOS] Mainframe scroll snap jumps to unpredictable...

[Wenson Hsieh]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: f4b2339d9e5187c28766b443ac6c29c27f59821c
      https://github.com/WebKit/WebKit/commit/f4b2339d9e5187c28766b443ac6c29c27f59821c
  Author: Wenson Hsieh <wenson_hsieh at apple.com>
  Date:   2023-04-24 (Mon, 24 Apr 2023)

  Changed paths:
    A LayoutTests/css3/scroll-snap/ios/scroll-snap-mainframe-pinch-out-expected.txt
    A LayoutTests/css3/scroll-snap/ios/scroll-snap-mainframe-pinch-out.html
    M LayoutTests/css3/scroll-snap/ios/scroll-snap-mainframe-scroll-deceleration-with-obscured-inset.html
    M LayoutTests/resources/ui-helper.js
    M Source/WebCore/page/ChromeClient.h
    M Source/WebCore/page/LocalFrameView.cpp
    M Source/WebCore/page/LocalFrameView.h
    M Source/WebCore/platform/ScrollableArea.cpp
    M Source/WebCore/platform/ScrollableArea.h
    M Source/WebKit/Platform/spi/ios/UIKitSPI.h
    M Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm
    M Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteScrollingCoordinatorProxyIOS.mm
    M Source/WebKit/UIProcess/ios/WKScrollView.mm
    M Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp
    M Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h
    M Source/WebKit/WebProcess/WebPage/WebPage.h

  Log Message:
  -----------
  [iOS] Mainframe scroll snap jumps to unpredictable offsets when pinch zooming
https://bugs.webkit.org/show_bug.cgi?id=255848
rdar://108008629

Reviewed by Brent Fulgham and Simon Fraser.

When pinch zooming out to less than the initial scale on book.stevejobsarchive.com (when scroll
snapping on the root element is enabled), the scroll position frequently jumps to (seemingly) random
offsets in the page, sometimes after attempting to scroll (with animation) to another wrong scroll
snap point. There are multiple root causes that trigger these symptoms, described below (along with
the changes in this patch that address each one):

(1) We occasionally use the wrong scale to compute the appropriate snap offset in
    `RemoteScrollingCoordinatorProxyIOS`. Currently, `WebPageProxy::displayedContentScale` only
    yields the latest zoom scale we sent via `VisibleContentRectUpdate`, but we scale (and
    counterscale) geometry directly on the `WKScrollView` using this scale. There's no guarantee
    that `-[UIScrollView zoomScale]` is in sync with this last sent scale, so we can end up
    computing the wrong nearest snap position in both `closestSnapOffsetForMainFrameScrolling` (when
    ending the pinch) and `nearestActiveContentInsetAdjustedSnapOffset` (when we receive the first
    stable-state commit after the bounce animation).

    To fix this, we simply use `UIScrollView`'s `-zoomScale` instead so that the scale is consistent
    with the current value of `-contentOffset`, as well as the retargeted content offset.

(2) Even ignoring (1), the scroll view sometimes fails to retarget to any offset at all after
    pinching out, and instead settles back down to the current offset (as if snapping were
    disabled), before instantly jumping to a nearby snap point. Whether or not this bug happens is
    dependent on the order in which `WKScrollView`'s pan gesture action fires in Ended state —
    relative to the pinch gesture — when ending the pinch. If the scroll view observes the pan
    gesture ending (`-handlePan:`) before the pinch gesture ends (`-handlePinch:`), we'll run our
    retargeting logic for scroll snapping, but then `UIScrollView` immediately stomps over the
    retargeted position due to the `-zoomScale` clamping back to `-minimumZoomScale` when ending the
    pinch. This ordering is currently non-deterministic, as it depends on hashing order of the
    scroll view's pinch and pan gesture recognizers, when sending actions to gestures after ending
    the touch.

    To work around this, we take advantage of the fact that `-handlePinch:` is idempotent when the
    pinch gesture has ended (since we'll clear the `zooming` flag and transition to `zoomBouncing`
    state only once), and directly invoke `-handlePinch:` from within `-handlePan:` here to ensure
    that we're able to use the retargeting delegate hook.

    There are other workarounds that I considered, such as detecting that this bug is about to
    happen and falling back to an animated scroll instead of retargeting; however, the above is the
    only approach I could find that results in a smooth "glide" animation to the correct snap
    destination (as opposed to zooming back to minimum scale and then scrolling to the final
    location).

(3) There's logic in `-_updateVisibleContentRects` that's responsible for scrolling to the active
    snap position when we're about to send a stable visible content rect update; however, this fires
    way too early in some cases (i.e. right after the pinch ends, in the middle of zooming), causing
    us to jump to the final offset. This is because we send a visible content rect update after the
    `-isZooming` flag is unset (i.e. after the pinch gesture recognizer transitions to Ended state),
    but before the zooming actually begins. Correct this gap in view stability state by additionally
    consulting the `-isZoomBouncing` flag, which is set right after zooming ends when pinching out
    and unset after the zoom animation completes.

(4) After fixing all the above, the behavior is considerably improved, but scrolling still
    occasionally jumps to the final offset in the middle of the zoom bounce animation. This is
    because `resnapAfterLayout()` triggers before the zoom animation is complete, causing a stutter
    in the scroll position as we attempt an instant scroll to the final offset while still zooming.

    Address this by avoiding `resnapAfterLayout()` in the case where the viewport is not in stable
    state; to achieve this, we add some client plumbing through `LocalFrameView` ->
    `WebChromeClient` -> `WebPage` to return whether or not we're in stable state. Note that we also
    rely on (3) here, otherwise the web process will enter stable state too early.

With the 4 issues above fixed, pinch zooming on book.stevejobsarchive.com no longer causes the
scroll position to visually stutter or jump, and we instead transition in one smooth animation from
the offset in the pinched-out view to the final scroll snap destination, in all cases.

Test: css3/scroll-snap/ios/scroll-snap-mainframe-pinch-out.html

* LayoutTests/css3/scroll-snap/ios/scroll-snap-mainframe-pinch-out-expected.txt: Added.
* LayoutTests/css3/scroll-snap/ios/scroll-snap-mainframe-pinch-out.html: Added.

Add a new test case to exercise this scenario; this test uses scroll snapping on the root element,
and additionally continuously triggers layout on the page by changing the widths of each of the
scroll snap children every frame. The test then programmatically scrolls to a snap point in the
middle of the page, zooms out by pinching, and verifies that after the bounce has ended (i.e. we get
a stable state presentation update), the final scroll offset matches what we started with.

* LayoutTests/css3/scroll-snap/ios/scroll-snap-mainframe-scroll-deceleration-with-obscured-inset.html:

Drive-by fix: clean up this test page by moving the `script` elements in this test under the `head`.

* LayoutTests/resources/ui-helper.js:
(window.UIHelper.async pinch):

Add a `UIHelper` method to make it easier to synthesize a pinch gesture; this helper method takes
4 points (8 x/y values total) that represent the two starting touch locations, and two ending touch
locations. It then creates and synthesizes a 2-finger gesture stream using these points.

* Source/WebCore/page/ChromeClient.h:
(WebCore::ChromeClient::isInStableState const):
* Source/WebCore/page/LocalFrameView.cpp:
(WebCore::LocalFrameView::isInStableState const):
* Source/WebCore/page/LocalFrameView.h:
* Source/WebCore/platform/ScrollableArea.cpp:
(WebCore::ScrollableArea::resnapAfterLayout):

See (4) in the above diagnosis.

* Source/WebCore/platform/ScrollableArea.h:
(WebCore::ScrollableArea::isInStableState const):
* Source/WebKit/Platform/spi/ios/UIKitSPI.h:

Add more forward declarations on `UIScrollView`.

* Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:
(-[WKWebView _viewStabilityState:]):

See (3) in the above diagnosis.

* Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteScrollingCoordinatorProxyIOS.mm:
(WebKit::RemoteScrollingCoordinatorProxyIOS::closestSnapOffsetForMainFrameScrolling const):
(WebKit::RemoteScrollingCoordinatorProxyIOS::nearestActiveContentInsetAdjustedSnapOffset const):

See (1) in the above diagnosis.

* Source/WebKit/UIProcess/ios/WKScrollView.mm:
(-[WKScrollView _sendPinchGestureActionEarlyIfNeeded]):
(-[WKScrollView handlePan:]):

See (2) in the above diagnosis.

* Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:
(WebKit::WebChromeClient::isInStableState const):
* Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:
* Source/WebKit/WebProcess/WebPage/WebPage.h:
(WebKit::WebPage::isInStableState const):

Canonical link: https://commits.webkit.org/263336@main