Bug 198217 – iOS: REGRESSION(async scroll): Caret doesn't scroll when scrolling textarea

• Component: HTML Editing
• Status: RESOLVED FIXED

[Daniel Bates]

Seen on iPad in landscape orientation (<—probably doesn’t matter) using a hardware keyboard.

Steps to reproduce:

1. Visit <https://bugs.webkit.org/show_bug.cgi?id=198181>.
2. Reply to comment #4: click Reply to the right of that comment.
3. Place the cursor somewhere that would be scrolled of screen, say line 4.
4. Scroll the text area using your finger.

Then the care is drawn at the wrong location.

[Radar WebKit Bug Importer]

<rdar://problem/51097296>

[Ryosuke Niwa]

This is a regression from async overflow scroll work

[Wenson Hsieh]

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).

[Wenson Hsieh]

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

[Wenson Hsieh]

> 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.

[Wenson Hsieh]

Created attachment 373232 [details]
Approach #2

[Wenson Hsieh]

Created attachment 373235 [details]
Approach #1

[Simon Fraser (smfr)]

Comment on attachment 373235 [details]
Approach #1

The intent is always to have a "Set" as the last thing, so this approach seems good.

[Wenson Hsieh]

Comment on attachment 373235 [details]
Approach #1

Sounds good — thanks for the review!

[WebKit Commit Bot]

Comment on attachment 373235 [details]
Approach #1

Clearing flags on attachment: 373235

Committed r247013: <https://trac.webkit.org/changeset/247013>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.