Is it possible to do a bisect on this?

From downloading and installing different version off of https://commondatastorage.googleapis.com/chromium-browser-snapshots/
I found that the regression has happened somewhere in (523606, 523625].

wjmaclean@: One CL of yours seems relevant. I can try reverting this locally and see if the bug is fixed.
https://chromium.googlesource.com/chromium/src/+/a7f4ab11cf9b65467aab860edc38e613d604b37d

Just confirming that reverting the CL in comment #2 fixes the problem. Tested locally on Android 5.1 Nexus 6.

Assigning to James for further investigation.

I have investigated this with fsamuel@, and it appears that we're hitting a surface sync issue, in that when we attempt to do the (surface-based) coordinate conversion, the updated surface from the OOPIF's renderer isn't correctly registered with the surface hierarchy yet, and so the hit-testing fails to find it.

It will apparently be fixed by surface sync becoming available for Android, though we may wish to do a short-term fix similar to what was in place prior to https://chromium.googlesource.com/chromium/src/+/a7f4ab11cf9b65467aab860edc38e613d604b37d

fsamuel@ ... any timeline for surface-sync on Android?

Surface sync finch trial will begin soon on M68.

So I manually tested today with viz::SurfaceSynchronization turned on for Android, and the selection handles appear to work correctly.

Just checking on the status of surface synchronization on Android, as I'm still seeing this issue on Android canary 71.0.3564.0.  This seems like a pretty visible issue, so we'll likely want to fix this before we can launch any form of site isolation for Android.

Also, I tried running a recent local build on Android with --enable-features=SurfaceSynchronization, and that doesn't seem to fix the issue.  James, how did you get this working in #8 - is there something else I need to enable?

alexmos@ - I have a CL in the works for this at https://chromium-review.googlesource.com/c/chromium/src/+/1251023, I'm just trying to figure out why the android debug build is failing on undefined symbols in the TouchSelectionController (when the android regular build works).

I hope to land it at the beginning of next week.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fdef91879a15a0a4407d5a41ab3181bd05ae30ea

commit fdef91879a15a0a4407d5a41ab3181bd05ae30ea
Author: W. James MacLean <wjmaclean@chromium.org>
Date: Sat Sep 29 19:17:03 2018

Fix touch-handle positions for OOPIF on Android.

This CL removes a hack from Android's
TouchSelectionControllerClientManager that attempts to add the current
page scale factor in to the handles' coordinates. Somewhere along the
way the underlying lack of page scale factor in the coordinate
transforms went away. meaning the hack was causing the coordinates to
be incorrect.

This CL also makes a small fix to the function
RenderWidgetHostViewAndroid::TransformPointToCoordSpaceForView that
generalizes how it checks for the FrameSinkId necessary to do the
transformations.

Bug:  809122 
Change-Id: I74e7ddc8d448ae28061d34a42c295e5ae4f7f624
Reviewed-on: https://chromium-review.googlesource.com/1251023
Commit-Queue: James MacLean <wjmaclean@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595321}
[modify] https://crrev.com/fdef91879a15a0a4407d5a41ab3181bd05ae30ea/content/browser/renderer_host/input/touch_selection_controller_client_manager_android.cc
[modify] https://crrev.com/fdef91879a15a0a4407d5a41ab3181bd05ae30ea/content/browser/renderer_host/input/touch_selection_controller_client_manager_android.h
[modify] https://crrev.com/fdef91879a15a0a4407d5a41ab3181bd05ae30ea/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/fdef91879a15a0a4407d5a41ab3181bd05ae30ea/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/fdef91879a15a0a4407d5a41ab3181bd05ae30ea/content/test/BUILD.gn

James, thanks for landing r595321, which made 71.0.3566.0.  I tried it out on a Pixel C with Android Canary 71.0.3567.0, but I'm still seeing touch handles at wrong positions.  I got this on http://csreis.github.io/tests/cross-site-iframe-simple.html in tablet mode, typing some text into the first <input>, and then trying to select it.  Same thing with OOP-D enabled (chrome://flags/#enable-viz-display-compositor).  Is there more work needed here?

What happens when you just select some non-editable text?

At present, the handles will be in the wrong position after a pinch event; when I tried what you described the keyboard immediately swooped in, causing a zoom event that screwed up the handles (and on my lowly Nexus 7, the keyboard took up the whole screen space, so the selected text didn't even display once the keyboard was present).

There's a 'scroll-ended' event that's supposed to trigger re-calculation of the handle positions, and it works on desktop for scrolls, but not pinches (which is weird, since all pinches are supposed to be wrapped by scrolls).

I'll look into this some more.

In order to get text selection handles in the right position all the time, I think we'll probably want to propagate their positions all the way up the hierarchy, one renderer at a time. 

E.g. The selection rect should go from child to parent, and parent to grand parent and so on. At each stage, the selection rect should propagate to the parent renderer, go from LayerTreeHost to LayerTreeHost all the way to RenderFrameMetadata and so on.

I think the problem here is just having the zoom effect somehow trigger re-calc of the handle position. As long as zoom stays out of the way, the handles *do* end up in the right position.

Just a further update ... I remember now why pinch doesn't invoke the DidScroll() mechanism ... that's because pinching will trigger a fresh UpdateSelectionBoundsIfNeeded() call, which (should) automatically update the handle positions. After poking at it for a bit, it seems the updates *are* happening, but somehow the positions are wrong ... I'm looking into it.

alexmos@ - I've uploaded a draft fix for the Android touch-selection handles at https://chromium-review.googlesource.com/c/chromium/src/+/1259945 if you want to give it a spin.
It seems the problem was that I had been testing with IsUseZoomForDSFEnable turned off ... it seems we no longer need to manually compensate for DSF, so I've removed that code. Also, this has an initial stab at making sure the handles update properly for pinch on both Android and desktop.
Let me know if you see any further issues with it, and in the meantime I'll work on improving the CL.

Thanks James.  I tried your CL on a Nexus 5, and the touch handles are better than before but still off - see screenshot.  Also, pinch-zooming the page when touch handles are visible makes the handles move in really funky ways.  Let me know if I can help debug this further in any way.

Deleted: Screenshot_20181004-074655.png 205 KB

	Screenshot_20181004-074655.png 205 KB View Download

Also tried on a Pixel C, and the handles work properly there, but only if the page is not zoomed in.  As soon as you zoom in, they move away from the text.  Also, I noticed if you scroll the main frame while the OOPIF has touch handles, the touch handle position is not always updated.

And you have viz surface sync and display compositor on?

Hmm, I just tested this on my Nexus 5, and while it's not perfect (handles lag a bit on pinch in an oopif, and sometimes get behind on main-frame scroll), it is much better than before.

I don't have a Pixel C to try it with.

I should add that we probably need to propagate selection handles up to the parent via RenderFrameMetadata in order to get perfect synchronization. Until then lag is inevitable. I’m guessing that part is not a launch blocker though?

+gklassen@, piman@, jonross@ for visibility 

Re C#22 ... I'm not sure I understand what you mean by "propagate selection handles up to the parent via RenderFrameMetadata". Do you mean SelectionBounds?

The handles only exist on the browser side.

My impression of why the handles lag during pinch is that, since the child frame isn't producing new frames (cf. the conversation on the raster-scale/page-scale bug), we have to rely on the updates going to the main frame (UpdateTouchSelectionController in RWHVAndroid), and using those to trigger recalculation (via the TouchSelectionClientManager) of the selection handle positions using the child's selection bound coordinates and the transform functions. I assumed that the transforms maybe weren't getting updated in sync? It works, it just gets behind. Some of the issues around the main-frame scroll may be due to the updates not happening in the parent frame?

Let me know if it's worth sitting down and looking at the current plumbing around the touch-selection handles if that makes things any clearer.

I've done a bit more investigation on Nexus 5, and it seems that RWHVAndroidUpdateTouchSelectionController() doesn't (always) get called for viewport scrolls. So we need to find another 'signal' to alert us to when the selection handles in a child need updating. This is possibly also to blame for the pinch lagginess.

I'm continuing to try and see how the main-frame handles get updated on Android; there is almost certainly a special pathway, since on desktop the handles just get hidden through all the scroll events.

I’m heading to the office now. Will drop by to chat. 

Writing things down here before I head over:

1. Text selection information is propagated to the parent via 
   RenderFrameMetadata. In order to synchronize, a new LocalSurfaceId must be 
   allocated.

2. In order to have full synchronization across the OOPIF hierarchy, we need to
   allocate a LocalSurfaceId across every frame up the hierarchy.

E.g. suppose A embeds B which embeds C. Suppose the text selection handle is in
C. 

Different things need to happen depending on the level in the hierarchy that is being scrolled (although one architecture can handle all these cases):

Suppose C is being scrolled. Then C allocates a LocalSurfaceId on every scroll, sends its text selection position (and LocalSurfaceId) up to its parent B, which translates the text selection position to its coordinate space and allocates a new LocalSurfaceId which sends its LocalSurfaceId and selection rect to its parent A which allocates a new LocalSurfaceId and so on.

Suppose just A scrolls, then only A needs to allocate a new LocalSurfaceId and translate the text selection rect in B's coordinate space.

Most of this plumbing is already in place, but just translating the text selection rect in the parent's coordinate space is the missing piece.

I've uploaded a new draft CL, with the primary purpose of improving notifications to a child frame about pinch and mainframe scroll via a HitTestRegionObserver. The handles will be a little bit jittery/laggy, but should settle in the right place and be functional. Based on discussions with fsamuel@ and jonross@ a more-polished fix is a ways off, as it will require more viz-related work to achieve.

There are other issues to figure out, but this CL should be a large step forward.

#c29: Thanks!  I tried out this latest draft, and it works great!  Touch handles now in correct spots, and zoom works fine, as does scrolling for both the subframe and main frame. The scroll lag is really small and not really a problem, I think.  This is on Nexus 5, and it worked both with and without Viz display compositor.

Cool! I definitely don't want to block anything on achieving perfect synchronization. If it's not blocking nor urgent, I might pass along that work to an intern in the winter. Seems like a nice visible win for an intern project. Any objections?

Maybe what I can also do is write a short doc explaining the remaining work for someone to pick up if we want it done sooner. The actual work left to do isn't too much but we don't have too many spare cycles at the moment.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/53a6e84ad2db5dcf1aa2bb1771698c0b2f29ce81

commit 53a6e84ad2db5dcf1aa2bb1771698c0b2f29ce81
Author: W. James MacLean <wjmaclean@chromium.org>
Date: Wed Oct 17 16:19:01 2018

Fix TouchSelectionHandle updates on Android.

This CL addresses the remaining issues with Android touch selection
handles. First, it removes the manual scaling done by the child frame
client for the device scale factor; apparently this is now handled by
the point-transform functions. Second, it makes sure the child frame's
handles update during pinch just like the main frame's handles do.

Because a child frame's handles are not locked to a surface like the
main frame's handles, there is a small amount of jitter/lag when
pinching or scrolling the main-frame, but not enough to impair
functionality.

Note this CL also resolves the issues for desktop handles not updating
after pinch as well.

Bug:  809122 
Change-Id: I120386c3d7a0d87dacc655e83b07e0cfeaf4920a
Reviewed-on: https://chromium-review.googlesource.com/c/1259945
Commit-Queue: James MacLean <wjmaclean@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600412}
[modify] https://crrev.com/53a6e84ad2db5dcf1aa2bb1771698c0b2f29ce81/content/browser/renderer_host/input/touch_selection_controller_client_child_frame.cc
[modify] https://crrev.com/53a6e84ad2db5dcf1aa2bb1771698c0b2f29ce81/content/browser/renderer_host/input/touch_selection_controller_client_child_frame.h
[modify] https://crrev.com/53a6e84ad2db5dcf1aa2bb1771698c0b2f29ce81/content/browser/renderer_host/input/touch_selection_controller_client_manager_android.cc
[modify] https://crrev.com/53a6e84ad2db5dcf1aa2bb1771698c0b2f29ce81/content/browser/renderer_host/input/touch_selection_controller_client_manager_android.h
[modify] https://crrev.com/53a6e84ad2db5dcf1aa2bb1771698c0b2f29ce81/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/53a6e84ad2db5dcf1aa2bb1771698c0b2f29ce81/content/browser/site_per_process_browsertest.cc

I verified that touch selection handles are now working properly on OOPIFs in Android Canary 72.0.3584.0, which includes r600412.  This is on Pixel C and Nexus 5 on http://csreis.github.io/tests/cross-site-iframe-simple.html with --site-per-process.  Thanks James!

Marking this as closed.

Adding label just for tracking this fixed bug as a past blocker for site isolation on Android.