Bug 198217 - iOS: REGRESSION(async scroll): Caret doesn't scroll when scrolling textarea
Summary: iOS: REGRESSION(async scroll): Caret doesn't scroll when scrolling textarea
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Local Build
Hardware: iPhone / iPad iOS 12
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-23 23:26 PDT by Daniel Bates
Modified: 2019-07-01 11:14 PDT (History)
16 users (show)

See Also:


Attachments
Restore iOS 12 behavior; for EWS only (5.33 KB, patch)
2019-06-30 14:56 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Approach #2 (11.54 KB, patch)
2019-07-01 07:36 PDT, Wenson Hsieh
simon.fraser: review-
Details | Formatted Diff | Diff
Approach #1 (12.76 KB, patch)
2019-07-01 08:02 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2019-05-23 23:26:29 PDT
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.
Comment 1 Radar WebKit Bug Importer 2019-05-23 23:26:43 PDT
<rdar://problem/51097296>
Comment 2 Ryosuke Niwa 2019-05-31 00:48:51 PDT
This is a regression from async overflow scroll work
Comment 3 Wenson Hsieh 2019-06-30 14:52:06 PDT
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).
Comment 4 Wenson Hsieh 2019-06-30 14:56:38 PDT
Created attachment 373198 [details]
Restore iOS 12 behavior; for EWS only
Comment 5 Wenson Hsieh 2019-06-30 15:11:42 PDT
> 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.
Comment 6 Wenson Hsieh 2019-07-01 07:36:26 PDT
Created attachment 373232 [details]
Approach #2
Comment 7 Wenson Hsieh 2019-07-01 08:02:07 PDT
Created attachment 373235 [details]
Approach #1
Comment 8 Simon Fraser (smfr) 2019-07-01 10:39:00 PDT
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 9 Wenson Hsieh 2019-07-01 10:40:51 PDT
Comment on attachment 373235 [details]
Approach #1

Sounds good — thanks for the review!
Comment 10 WebKit Commit Bot 2019-07-01 11:14:16 PDT
Comment on attachment 373235 [details]
Approach #1

Clearing flags on attachment: 373235

Committed r247013: <https://trac.webkit.org/changeset/247013>
Comment 11 WebKit Commit Bot 2019-07-01 11:14:18 PDT
All reviewed patches have been landed.  Closing bug.