Branch: refs/heads/main
Home: https://github.com/WebKit/WebKit
Commit: f4b2339d9e5187c28766b443ac6c29c27f59821c
https://github.com/WebKit/WebKit/commit/f4b2339d9e5187c28766b443ac6c29c27f59821c
Author: Wenson Hsieh <[email protected]>
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
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
