Bug 198217

<rdar://problem/51097296>

This is a regression from async overflow scroll work

I looked into this and made some observations: 1. How did it work in iOS 12? Selection would not update during async scrolling; after scrolling ends, the selection rect would then appear in the right place. In iOS 12, we get a bunch of calls to AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll with the scrollingLayerPositionAction being Sync. Then, at the end, we get one with Set instead. This call with ScrollingLayerPositionAction::Set at the end allows us to update the selection to the final state. What is responsible for scheduling the final scroll position update using ScrollingLayerPositionAction::Set? ↪ AsyncScrollingCoordinator::scheduleUpdateScrollPositionAfterAsyncScroll … ↪ RemoteScrollingCoordinatorProxy::scrollingTreeNodeDidScroll ↪ ScrollingTree::scrollPositionChangedViaDelegatedScrolling ↪ -[WKScrollingNodeScrollViewDelegate scrollViewDidEndDecelerating:] ...in the case where the scrolling gesture ends with momentum; otherwise, the last update is triggered via -[WKScrollingNodeScrollViewDelegate scrollViewDidEndDragging:willDecelerate:]. In both cases, the important bit is that the _inUserInteraction flag is set to NO, which causes us to use ScrollingLayerPositionAction::Set instead of ScrollingLayerPositionAction::Sync. 2. What happens in iOS 13? During async scrolling, we only get ScrollingLayerPositionAction::Sync. There is no Set action at the end, so we never try to send the final editor state update after async scrolling. This is due to two reasons: a. When scrolling ends, we no longer push a scrolling update through RemoteScrollingCoordinatorProxy, since ScrollingTreeScrollingNode::wasScrolledByDelegatedScrolling now bails if the scroll position was unchanged (that is, the check for scrollPositionChanged). b. Even if we did push a scrolling update after scrolling ends, we no longer use ScrollingLayerPositionAction::Set if we're not in user interaction (that is, if userInteraction is false in ScrollingTreeScrollingNodeDelegateIOS::scrollViewDidScroll), so we would not end up scheduling an editor state update in the web process anyways. Tweaking the code to avoid these two conditions seems to "fix" this bug (at least, by making it behave like it does in iOS 12 and prior).

Created attachment 373198 [details] Restore iOS 12 behavior; for EWS only

> Tweaking the code to avoid these two conditions seems to "fix" this bug (at > least, by making it behave like it does in iOS 12 and prior). That said, our shipping behavior isn’t…super great. The selection view appears in the wrong place until the scrolling stops, after which the selection jumps to the expected rects. I think we should consider scheduling editor state updates during async scrolling (not just at the end). This was probably not feasible when we were computing and sending editor states immediately during scrolling; but after fairly recent work in this area, editor state updates are only scheduled and computed during the next remote layer tree flush, so it doesn’t sound crazy to have updateScrollPositionAfterAsyncScroll always schedule editor state updates, not just when scrollingLayerPositionAction == ScrollingLayerPositionAction::Set.

Created attachment 373232 [details] Approach #2

Created attachment 373235 [details] Approach #1

Comment on attachment 373235 [details] Approach #1 The intent is always to have a "Set" as the last thing, so this approach seems good.

Comment on attachment 373235 [details] Approach #1 Sounds good — thanks for the review!

Comment on attachment 373235 [details] Approach #1 Clearing flags on attachment: 373235 Committed r247013: <https://trac.webkit.org/changeset/247013>

All reviewed patches have been landed. Closing bug.