New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 12
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-06-18
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug

Blocking:
issue 678370
issue 824521
issue 818214
issue 828422



Sign in to add a comment

DevTools Emulated Touch Events should not bypass RenderWidgetHostInputEventRouter

Project Member Reported by wjmaclean@chromium.org, Mar 22

Issue description

pfeldman@ - Can you look into this, and/or find someone on the DevTools team to own this? We can provide help w.r.t. to understanding scrolling for OOPIFs.

======================

At present, when DevTools wants to emulate touch events for devices (e.g. Pixel 2XL), it does so by intercepting mouse events in RenderWidgetHostImpl and converting them to corresponding touch events. Specifically,

MouseDown => TouchStart
MouseMove => TouchMove
MouseUp => TouchEnd

All of these converted events go directly to the renderer's InputRouter without the knowledge of RenderWidgetHostInputEventRouter, which controls scroll bubbling for OOPIFs.

Many of these touch events will never make it to the renderer, but in those cases they will cause GestureScroll events to be sent to the renderer. If not consumed, these events get acked and *do* go to the RWHIER, causing confusion for the scroll bubbling logic there.

The RWHI TouchEmulators cannot track state across different RWHIs, so if MouseDown occurs in one RWHI and the mouse moves out of that RWHI's frame, then no GestureScrollEnd is sent.

This is problematic, since RWHIER will have a 'dangling scroll bubbling target' so to speak. If a subsequent scroll sequence goes to the bubbling target, it may bubble unconsumed scroll, but those events get targetted back to itself, leading to issues like

https://bugs.chromium.org/p/chromium/issues/detail?id=818214

Or, if the mouse moves into a frame with a RWHI without a MouseDown, it may trigger one of the the DCHECKs in RenderWidgetHostImpl::ForwardGestureEventWithLatencyInfo(), e.g.

https://bugs.chromium.org/p/chromium/issues/detail?id=678370

and it may also be related to seeing gesture events without proper targets, as in

https://bugs.chromium.org/p/chromium/issues/detail?id=595422

I suspect the ultimate solution is to have a "TouchEmulatorController" that lives higher up, perhaps owned by RenderWidgetHostViewAura/Mac/??, and that can look after making sure the emulated events are tracked by RWHIER. Or perhaps even just have a single TouchEmulator in RenderWidgetHostViewEventHandler?

 
Blocking: 818214 678370
Blocking: 824521
Owner: einbinder@chromium.org
Labels: Proj-SiteIsolation-LaunchBlocking
Adding Proj-SiteIsolation-LaunchBlocking out of caution
Cc: abdulsyed@chromium.org
Labels: Target-67
Friendly ping-- any updates on the progress here?  We're hoping to get this fix into M67 if possible.

I'm not sure if this is a launch blocker (per comment 4), but it does sound like it means scroll bubbling won't work in mobile device emulation mode.  If it's helpful context at all, there's another mobile emulation OOPIF bug in  issue 826651 , where it appears that OOPIFs aren't aware of being in that mode.
Blocking: 828422
I took a look at https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_view_event_handler.cc?dr=C&l=481.

The issue is that routing events happens within the functions that generate web events from ui events and dev tools injects web events and that's why the event routing gets skipped. Maybe the proper approach here is to decouple routing web events from the functions that generate them.
Cc: dtapu...@chromium.org
Note that we already route most events to the right OOPIF using RenderWidgetHostInputEventRouter, just not the emulated touch. The problem is that touch sequences require heavy handling of ACKs, and wiring that up differently is cumbersome.

I suspect that having callbacks for ACKs might help. Dave, do we have plans for switching to callbacks instead of separate ACKs?
I don't have any immediate plans this quarter. We have to remove the Chrome IPC path first; we can do that once M66 ships with the mojo input messages on for webview. Then some of this code can become cleaner.
dtapuska@/wjmaclean@/dgozman@: Is there another way we can make the touch sequences and ACKs easier for DevTools to emulate in the short term, if callbacks aren't an option?  We're aiming for a fix in M67 to avoid regressing scroll bubbling in this case.
All touch events generated by devtools are marked with fromdebugger modiifer. Does that help any?

Secondly all touch events have a unique ID on the ack so it is possible to keep track of the ones you've seen and not seen. 

I'm not entirely certain how call backs would help devtools solve this but i've proposed solutions currently used in the ack situations. 
I looked at the TouchEmulator a little more. It seems relatively complicated in that it generates Gestures itself as well. I think the ideal model is that it actually generates a touch sequence that injected back into the top of the stack and it causes gesture recognizer to handle them and then the flow would be correct. 

What was the motivation for doing it the current way as opposed to the way gpu benchmarking/chrome driver works?
+1 to re-injecting the appropriately created touch events back at the top-level RenderWidgetHostViewX (or maybe even higher?). The less we special case this pathway the better.
It's good that you ask! The motivation was:
- we convert shift+drag into pinch instead of scroll;
- at least mac platform entirely lacks gesture recognition support.

A while ago I tried to address the  issue #2  and move our pipeline higher, but my first preparation patch has never landed [1]. I'm glad that there is interest in fixing this now!

[1] https://chromium-review.googlesource.com/c/chromium/src/+/706184
Re C#14: Is there anything we can do to assist with this work? If there are things we can discuss over VC I'd be happy to arrange a time.
I was not planning to work on this in the nearest future. The feature is minor, and I want to get rid of existing implementation anyway. That said, the underlying issue with gestures on mac is not going anywhere.
Comment 16: We were treating this as a launch blocker for M67.  Are you saying it's ok for scroll bubbling to not work in DevTools mobile device emulation?  There will be a lot of OOPIFs out there starting in M67.
re #c17: oh, I misunderstood the issue. If this affects device emulation, not emulateTouchFromMouse which is used for remote debugging screencast, then we should definitely address it soon.
dgozman@: Can you follow up on comment 18?  Will you or einbinder@ be able to address this soon, possibly with a merge to M67?  In theory we could launch Site Isolation without this, but it would be unfortunate to regress scroll bubbling in mobile device emulation mode.
Owner: ----
Status: Available (was: Assigned)
I don't think we have bandwidth to address this soon. I feel like some help from SiteIsolation and/or input team is required, due to issues outlined in #c0 and #c14. Someone needs to come up with a design which satisfies stakeholders, and my last quick attempt was rejected.
Ah, sorry-- I didn't realize there were still questions about the approach to take here.  dtapuska@ and wjmaclean@, can we try to come up with a design for something that can work here, ideally in the short term?
Cc: -mcnee@chromium.org nzolghadr@chromium.org
+nzolghadr@ could you or wjmaclean@ come up with a plan here?
Owner: wjmaclean@chromium.org
Status: Assigned (was: Available)
Sounds like wjmaclean@ and nzolghadr@ are working on a plan for this.  Thanks!

It sounds like it may not be viable to fix this in M67, meaning scroll bubbling would regress for cross-origin iframes in DevTools mobile device emulation mode when Site Isolation is turned on.  Maybe that's ok for one milestone, if we can get the fix into M68.  (And if the fix is possible to get into M67, so much the better.)

I'm leaving the milestone labels for now, until we have a more concrete sense of what the fix involves.  We can update as needed from there.
Status: Started (was: Assigned)
Labels: -Proj-SiteIsolation-LaunchBlocking
Labels: -M-67 -Target-67 M-68 Target-68
Project Member

Comment 28 by bugdroid1@chromium.org, Jun 11

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

commit a1403ff3efbd8f412e2e0c9b0257da3933f8692b
Author: W. James MacLean <wjmaclean@chromium.org>
Date: Mon Jun 11 19:28:08 2018

Route Emulated Touch Events for OOPIFs

This CL makes TouchEmulator work for OOPIFs by

1) Instantiating a single TouchEmulator for all views,

2) Moving ownership of the single TouchEmulator to
   RenderWidgetHostInputEventRouter (RWHIER), and

3) Routing the subsequent touch/gesture events through
   the input event router.

The last item is important so that scroll bubbling from
OOPIFs to parent views will work properly. It's important
to note that emulated touch events are *always* routed to
the same view that originally received the mouse event
that is being converted into a touch event. But GestureEvent
targetting will now follow the usual rules enforced by
RWHIER.

A core component of this CL involves having RWHIER implement
TouchEmulatorClient, so that it can mediate actions among
multiple views without the TouchEmulator needing to be aware
of their existence.

Ideally we would like the single TouchEmulator to use
the same GestureProvider as the root view, but since touch
emulation requires custom configuration of the GestureProvider,
and since (at present) GestureProvider does not allow most
configuration parameters to change after instantiation,
having a single GestureProvider will have to wait.

Bug:  824772 
Change-Id: I1c007631e08fd7f4ece8accb896cad9bc4982f97
Reviewed-on: https://chromium-review.googlesource.com/1069661
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Commit-Queue: James MacLean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566103}
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/devtools/protocol/emulation_handler.cc
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/devtools/protocol/input_handler.cc
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/input/main_thread_event_queue_browsertest.cc
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/input/touch_emulator.cc
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/input/touch_emulator.h
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/input/touch_emulator_client.h
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/input/touch_emulator_unittest.cc
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/render_widget_host_browsertest.cc
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/render_widget_host_impl.h
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/render_widget_host_input_event_router.cc
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/render_widget_host_input_event_router.h
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/render_widget_host_unittest.cc
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/site_per_process_hit_test_browsertest.cc
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/testing/buildbot/filters/viz.content_browsertests.filter

Status: Fixed (was: Started)
Modulo any follow-on refactors, which should have little-to-no behavioural effects, this is fixed.

Comment 30 by creis@chromium.org, Jun 15 (6 days ago)

Thanks James!  I can verify the fix in Windows Canary 69.0.3460.0, using the steps below (with the scroll bubbling URL from issue 852932):

1) Start Chrome with Site Isolation.
2) Visit https://thewirecutter.com/reviews/best-patio-furniture/
3) Open DevTools and enable Mobile device emulation mode (2 box icon in upper left).
4) Emulate touch scrolls down to the comment section.
5) Try to scroll in the comment section.

In 67.0.3396.87 (Stable) and 69.0.3452.0 (Dev), the scrolling is very janky and doesn't really make progress.  Works as expected in 69.0.3460.0.  (The r566103 fix landed in 69.0.3456.0.)

James, is this mergeable to M68?  Would be good for scrolling in this mode to be functional with Site Isolation enabled, depending on how risky you think the change is.  (Maybe try it out in a local checkout of M68?)

Comment 31 by wjmaclean@chromium.org, Jun 15 (5 days ago)

Yes, I think it's potentially mergeable (the CL lived throughout it's development cycle without any merge conflicts I think).

I'll try merging it to a local M68 checkout as you suggest and give it a spin.

Comment 32 by creis@chromium.org, Jun 15 (5 days ago)

Cc: gov...@chromium.org
NextAction: 2018-06-18
Thanks!  If it works locally, please request merge to M68 on Monday in time for the next beta cut.

Comment 33 by creis@chromium.org, Jun 15 (5 days ago)

For reference, I was able to merge this cleanly to my local M68 branch and verify that it fixed the bug and passed the newly added content_browsertests.  If you think it's unlikely to cause regressions, let's request merge on Monday.  Thanks!

Comment 34 by monor...@bugs.chromium.org, Jun 18 (2 days ago)

The NextAction date has arrived: 2018-06-18

Comment 35 by wjmaclean@chromium.org, Jun 18 (2 days ago)

Labels: Merge-Request-68
Thanks for doing the testing Charlie! Yes, I think it's unlikely to cause problems, and the net win is big. It's certainly worth putting it in the beta and giving it some bake time.
Project Member

Comment 36 by sheriffbot@chromium.org, Jun 18 (2 days ago)

Labels: -Merge-Request-68 Hotlist-Merge-Review Merge-Review-68
This bug requires manual review: M68 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 37 by abdulsyed@google.com, Jun 18 (2 days ago)

Labels: -Merge-Review-68 Merge-Approved-68
Approving merge to M68. Branch:3440
Project Member

Comment 38 by bugdroid1@chromium.org, Jun 18 (2 days ago)

Labels: -merge-approved-68 merge-merged-3440
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/40eecf0fe920314d1ffda724b9225ae6f977b7b5

commit 40eecf0fe920314d1ffda724b9225ae6f977b7b5
Author: W. James MacLean <wjmaclean@chromium.org>
Date: Mon Jun 18 20:18:24 2018

Route Emulated Touch Events for OOPIFs

This CL makes TouchEmulator work for OOPIFs by

1) Instantiating a single TouchEmulator for all views,

2) Moving ownership of the single TouchEmulator to
   RenderWidgetHostInputEventRouter (RWHIER), and

3) Routing the subsequent touch/gesture events through
   the input event router.

The last item is important so that scroll bubbling from
OOPIFs to parent views will work properly. It's important
to note that emulated touch events are *always* routed to
the same view that originally received the mouse event
that is being converted into a touch event. But GestureEvent
targetting will now follow the usual rules enforced by
RWHIER.

A core component of this CL involves having RWHIER implement
TouchEmulatorClient, so that it can mediate actions among
multiple views without the TouchEmulator needing to be aware
of their existence.

Ideally we would like the single TouchEmulator to use
the same GestureProvider as the root view, but since touch
emulation requires custom configuration of the GestureProvider,
and since (at present) GestureProvider does not allow most
configuration parameters to change after instantiation,
having a single GestureProvider will have to wait.

TBR=wjmaclean@chromium.org

(cherry picked from commit a1403ff3efbd8f412e2e0c9b0257da3933f8692b)

Bug:  824772 
Change-Id: I1c007631e08fd7f4ece8accb896cad9bc4982f97
Reviewed-on: https://chromium-review.googlesource.com/1069661
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Commit-Queue: James MacLean <wjmaclean@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#566103}
Reviewed-on: https://chromium-review.googlesource.com/1105030
Reviewed-by: James MacLean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#411}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/devtools/protocol/emulation_handler.cc
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/devtools/protocol/input_handler.cc
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/input/main_thread_event_queue_browsertest.cc
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/input/touch_emulator.cc
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/input/touch_emulator.h
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/input/touch_emulator_client.h
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/input/touch_emulator_unittest.cc
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/render_widget_host_browsertest.cc
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/render_widget_host_impl.h
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/render_widget_host_input_event_router.cc
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/render_widget_host_input_event_router.h
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/render_widget_host_unittest.cc
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/site_per_process_hit_test_browsertest.cc
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/testing/buildbot/filters/viz.content_browsertests.filter

Comment 39 by krajshree@chromium.org, Today (13 hours ago)

Cc: krajshree@chromium.org
Labels: Needs-Feedback
Unable to reproduce the issue on Win-10 using chrome reported version #67.0.3396.87 as per comment #30. Observed that the scrolling is very smooth. Hence, unable to proceed with verifying the issue on chrome version #68.0.3440.33.

Attached a screen cast for reference on chrome version #68.0.3440.33.

wjmaclean@ - Could you please check the attached screen cast and please help us in verifying the fix.

Thanks...!!
824772.mp4
13.5 MB View Download

Comment 40 by creis@chromium.org, Today (10 hours ago)

Comment 39: Are you scrolling by clicking and dragging the page, to simulate a touch scroll on a mobile device?  It looks like you might be scrolling using the mousewheel or trackpad instead, which doesn't repro the bug.

Sorry for leaving that detail out of comment 30!

Comment 41 by manoranj...@chromium.org, Today (6 hours ago)

Labels: -Needs-Feedback TE-Verified-68.0.3440.33
creis@, thank you for the clarification. I'm able to reproduce this issue and verified the above fix on Win, Mac & Linux platforms for today's M68 Beta RC# 68.0.3440.33 - Working as intended.

Sign in to add a comment