Allow OOPIFs to capture mouse input while mouse button is held down

Allow OOPIFs to capture mouse input while mouse button is held down

Currently, if you perform a mouse dragging action on an OOPIF such as
moving a scroll bar or selecting text, and the mouse cursor moves out
of the OOPIF, input events get routed to the parent frame so the
dragging action stops working. This CL cause the browser process to
lock input to a single RenderWidgetHostView while a mouse button is
being held down, correcting that problem.

BUG=529377
Committed: https://crrev.com/30d21915acf01e3cb5746b1a3cb2e8786c2bd841
Cr-Commit-Position: refs/heads/master@{#418940}

[kenrb, 2016-09-13 15:10:22]

The CQ bit was checked by kenrb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 15:10:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 16:04:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 16:04:35]

Dry run: This issue passed the CQ dry run.

[kenrb, 2016-09-13 16:05:08]

kenrb@chromium.org changed reviewers:
+ dtapuska@chromium.org

[kenrb, 2016-09-13 16:05:09]

dtapuska: PTAL? This is a small change that locks mouse input for OOPIFs.

[dtapuska, 2016-09-13 16:12:38]

dtapuska@chromium.org changed reviewers:
+ mustaq@chromium.org, rbyers@chromium.org

[dtapuska, 2016-09-13 16:12:38]

Do we ever want to support drag and drop from an OOPIF?

[kenrb, 2016-09-13 16:16:28]

On 2016/09/13 16:12:38, dtapuska wrote:
> Do we ever want to support drag and drop from an OOPIF?

Yes, paulmeyer@ had been looking at that but nobody is actively working on it
right now. I am expecting that we will need to handle that as a special case in
RenderWidgetHostInputEventRouter.

[mustaq, 2016-09-13 16:57:15]

https://codereview.chromium.org/2339503002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/2339503002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_input_event_router.cc:153:
event->type != blink::WebInputEvent::MouseDown) {
Please also check that event modifier has at least one of
{Left,Middle,Right}ButtonDown bits set, to make sure it's a real drag case.
Otherwise, dragging out-of-window & releasing would mean a hovering mouse is
still appear captured.

https://codereview.chromium.org/2339503002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_input_event_router.cc:163:
mouse_capture_target_.target = nullptr;
Or perhaps add the check (mentioned above) here and move this |if| to before the
above |if|.

https://codereview.chromium.org/2339503002/diff/1/content/browser/site_per_pr...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2339503002/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:1375:
router->RouteMouseEvent(root_view, &mouse_event);
Nit: May be test for mouseup too that the event goes to child frame only.

https://codereview.chromium.org/2339503002/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:1388: }
Is the behavior symmetric for the "reciprocal" case? If yes, may be check that
case too?

[kenrb, 2016-09-13 20:01:40]

The CQ bit was checked by kenrb@chromium.org to run a CQ dry run

[kenrb, 2016-09-13 20:01:53]

https://codereview.chromium.org/2339503002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/2339503002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_input_event_router.cc:153:
event->type != blink::WebInputEvent::MouseDown) {
On 2016/09/13 16:57:15, mustaq wrote:
> Please also check that event modifier has at least one of
> {Left,Middle,Right}ButtonDown bits set, to make sure it's a real drag case.
> Otherwise, dragging out-of-window & releasing would mean a hovering mouse is
> still appear captured.

Done.

https://codereview.chromium.org/2339503002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_input_event_router.cc:163:
mouse_capture_target_.target = nullptr;
On 2016/09/13 16:57:15, mustaq wrote:
> Or perhaps add the check (mentioned above) here and move this |if| to before
the
> above |if|.

I have fixed this according to my current understanding of correctness. Mouse
capture now requires that either a ButtonDown modifier is set, or the event is a
MouseUp (I think we still want the capturing frame to get the MouseUp).

https://codereview.chromium.org/2339503002/diff/1/content/browser/site_per_pr...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2339503002/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:1375:
router->RouteMouseEvent(root_view, &mouse_event);
On 2016/09/13 16:57:15, mustaq wrote:
> Nit: May be test for mouseup too that the event goes to child frame only.

Done.

https://codereview.chromium.org/2339503002/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:1388: }
On 2016/09/13 16:57:15, mustaq wrote:
> Is the behavior symmetric for the "reciprocal" case? If yes, may be check that
> case too?

Done.

[commit-bot: I haz the power, 2016-09-13 20:02:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dtapuska, 2016-09-13 20:08:49]

On 2016/09/13 20:01:53, kenrb wrote:
>
https://codereview.chromium.org/2339503002/diff/1/content/browser/renderer_ho...
> File content/browser/renderer_host/render_widget_host_input_event_router.cc
> (right):
> 
>
https://codereview.chromium.org/2339503002/diff/1/content/browser/renderer_ho...
> content/browser/renderer_host/render_widget_host_input_event_router.cc:153:
> event->type != blink::WebInputEvent::MouseDown) {
> On 2016/09/13 16:57:15, mustaq wrote:
> > Please also check that event modifier has at least one of
> > {Left,Middle,Right}ButtonDown bits set, to make sure it's a real drag case.
> > Otherwise, dragging out-of-window & releasing would mean a hovering mouse is
> > still appear captured.
> 
> Done.
> 
>
https://codereview.chromium.org/2339503002/diff/1/content/browser/renderer_ho...
> content/browser/renderer_host/render_widget_host_input_event_router.cc:163:
> mouse_capture_target_.target = nullptr;
> On 2016/09/13 16:57:15, mustaq wrote:
> > Or perhaps add the check (mentioned above) here and move this |if| to before
> the
> > above |if|.
> 
> I have fixed this according to my current understanding of correctness. Mouse
> capture now requires that either a ButtonDown modifier is set, or the event is
a
> MouseUp (I think we still want the capturing frame to get the MouseUp).
> 
>
https://codereview.chromium.org/2339503002/diff/1/content/browser/site_per_pr...
> File content/browser/site_per_process_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2339503002/diff/1/content/browser/site_per_pr...
> content/browser/site_per_process_browsertest.cc:1375:
> router->RouteMouseEvent(root_view, &mouse_event);
> On 2016/09/13 16:57:15, mustaq wrote:
> > Nit: May be test for mouseup too that the event goes to child frame only.
> 
> Done.
> 
>
https://codereview.chromium.org/2339503002/diff/1/content/browser/site_per_pr...
> content/browser/site_per_process_browsertest.cc:1388: }
> On 2016/09/13 16:57:15, mustaq wrote:
> > Is the behavior symmetric for the "reciprocal" case? If yes, may be check
that
> > case too?
> 
> Done.

should mouse wheel events be targeted to this host as well if there is a
capture?

[kenrb, 2016-09-13 20:11:32]

On 2016/09/13 20:08:49, dtapuska wrote:
> On 2016/09/13 20:01:53, kenrb wrote:
> >
>
https://codereview.chromium.org/2339503002/diff/1/content/browser/renderer_ho...
> > File content/browser/renderer_host/render_widget_host_input_event_router.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2339503002/diff/1/content/browser/renderer_ho...
> > content/browser/renderer_host/render_widget_host_input_event_router.cc:153:
> > event->type != blink::WebInputEvent::MouseDown) {
> > On 2016/09/13 16:57:15, mustaq wrote:
> > > Please also check that event modifier has at least one of
> > > {Left,Middle,Right}ButtonDown bits set, to make sure it's a real drag
case.
> > > Otherwise, dragging out-of-window & releasing would mean a hovering mouse
is
> > > still appear captured.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2339503002/diff/1/content/browser/renderer_ho...
> > content/browser/renderer_host/render_widget_host_input_event_router.cc:163:
> > mouse_capture_target_.target = nullptr;
> > On 2016/09/13 16:57:15, mustaq wrote:
> > > Or perhaps add the check (mentioned above) here and move this |if| to
before
> > the
> > > above |if|.
> > 
> > I have fixed this according to my current understanding of correctness.
Mouse
> > capture now requires that either a ButtonDown modifier is set, or the event
is
> a
> > MouseUp (I think we still want the capturing frame to get the MouseUp).
> > 
> >
>
https://codereview.chromium.org/2339503002/diff/1/content/browser/site_per_pr...
> > File content/browser/site_per_process_browsertest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2339503002/diff/1/content/browser/site_per_pr...
> > content/browser/site_per_process_browsertest.cc:1375:
> > router->RouteMouseEvent(root_view, &mouse_event);
> > On 2016/09/13 16:57:15, mustaq wrote:
> > > Nit: May be test for mouseup too that the event goes to child frame only.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2339503002/diff/1/content/browser/site_per_pr...
> > content/browser/site_per_process_browsertest.cc:1388: }
> > On 2016/09/13 16:57:15, mustaq wrote:
> > > Is the behavior symmetric for the "reciprocal" case? If yes, may be check
> that
> > > case too?
> > 
> > Done.
> 
> should mouse wheel events be targeted to this host as well if there is a
> capture?

I initially had done that, but removed it after I noticed that wheel events
don't get processed that way by Blink, so it matches current behavior to not
have them follow capture.

[dtapuska, 2016-09-13 20:12:18]

On 2016/09/13 20:11:32, kenrb wrote:
> On 2016/09/13 20:08:49, dtapuska wrote:
> > On 2016/09/13 20:01:53, kenrb wrote:
> > >
> >
>
https://codereview.chromium.org/2339503002/diff/1/content/browser/renderer_ho...
> > > File
content/browser/renderer_host/render_widget_host_input_event_router.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2339503002/diff/1/content/browser/renderer_ho...
> > >
content/browser/renderer_host/render_widget_host_input_event_router.cc:153:
> > > event->type != blink::WebInputEvent::MouseDown) {
> > > On 2016/09/13 16:57:15, mustaq wrote:
> > > > Please also check that event modifier has at least one of
> > > > {Left,Middle,Right}ButtonDown bits set, to make sure it's a real drag
> case.
> > > > Otherwise, dragging out-of-window & releasing would mean a hovering
mouse
> is
> > > > still appear captured.
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/2339503002/diff/1/content/browser/renderer_ho...
> > >
content/browser/renderer_host/render_widget_host_input_event_router.cc:163:
> > > mouse_capture_target_.target = nullptr;
> > > On 2016/09/13 16:57:15, mustaq wrote:
> > > > Or perhaps add the check (mentioned above) here and move this |if| to
> before
> > > the
> > > > above |if|.
> > > 
> > > I have fixed this according to my current understanding of correctness.
> Mouse
> > > capture now requires that either a ButtonDown modifier is set, or the
event
> is
> > a
> > > MouseUp (I think we still want the capturing frame to get the MouseUp).
> > > 
> > >
> >
>
https://codereview.chromium.org/2339503002/diff/1/content/browser/site_per_pr...
> > > File content/browser/site_per_process_browsertest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2339503002/diff/1/content/browser/site_per_pr...
> > > content/browser/site_per_process_browsertest.cc:1375:
> > > router->RouteMouseEvent(root_view, &mouse_event);
> > > On 2016/09/13 16:57:15, mustaq wrote:
> > > > Nit: May be test for mouseup too that the event goes to child frame
only.
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/2339503002/diff/1/content/browser/site_per_pr...
> > > content/browser/site_per_process_browsertest.cc:1388: }
> > > On 2016/09/13 16:57:15, mustaq wrote:
> > > > Is the behavior symmetric for the "reciprocal" case? If yes, may be
check
> > that
> > > > case too?
> > > 
> > > Done.
> > 
> > should mouse wheel events be targeted to this host as well if there is a
> > capture?
> 
> I initially had done that, but removed it after I noticed that wheel events
> don't get processed that way by Blink, so it matches current behavior to not
> have them follow capture.

ok; we have an outstanding issue to allow wheel events to be processed during a
drag. I think someone was looking at fixing.

[dtapuska, 2016-09-13 20:15:15]

https://codereview.chromium.org/2339503002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/2339503002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:155: if
(mouse_capture_target_.target &&
Should you not be clearing the target when the view gets destroyed. Perhaps like
this code?
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...

[kenrb, 2016-09-13 20:18:50]

https://codereview.chromium.org/2339503002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/2339503002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:155: if
(mouse_capture_target_.target &&
On 2016/09/13 20:15:14, dtapuska wrote:
> Should you not be clearing the target when the view gets destroyed. Perhaps
like
> this code?
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...

I definitely should. Thanks for catching that.

Fixed.

[kenrb, 2016-09-13 20:22:11]

On 2016/09/13 20:12:18, dtapuska wrote:
> 
> ok; we have an outstanding issue to allow wheel events to be processed during
a
> drag. I think someone was looking at fixing.

Sorry, I missed this previously. Should I change this to do that?

[dtapuska, 2016-09-13 20:22:26]

On 2016/09/13 20:18:50, kenrb wrote:
>
https://codereview.chromium.org/2339503002/diff/20001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_input_event_router.cc
> (right):
> 
>
https://codereview.chromium.org/2339503002/diff/20001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_input_event_router.cc:155: if
> (mouse_capture_target_.target &&
> On 2016/09/13 20:15:14, dtapuska wrote:
> > Should you not be clearing the target when the view gets destroyed. Perhaps
> like
> > this code?
> >
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...
> 
> I definitely should. Thanks for catching that.
> 
> Fixed.

lgtm

[mustaq, 2016-09-13 20:28:03]

lgtm

[kenrb, 2016-09-14 14:43:35]

kenrb@chromium.org changed reviewers:
+ nasko@chromium.org

[kenrb, 2016-09-14 14:43:35]

Nasko, can you give this content owner review?

[Rick Byers, 2016-09-14 15:11:17]

It sounds like this will make OOPIF mouse capturing behavior different than
iframe capturing behavior, right?

We have a bug today where mouse events are captured to an iframe when they
shouldn't be (https://bugs.chromium.org/p/chromium/issues/detail?id=269917). 
Our plan was to fix that so that mouse is never captured to a frame by default,
but can be explicitly captured via the setPointerCapture API.

Making OOPIF align with our existing case (captured to the frame only when
mousedown isn't canceled) would make sense.  But I'm a little worried about the
impact of capturing OOPIF cases always while non-OOPIF wont - makes it harder
for web developers to reason about.

Thoughts?

[kenrb, 2016-09-14 15:45:04]

On 2016/09/14 15:11:17, Rick Byers wrote:
> It sounds like this will make OOPIF mouse capturing behavior different than
> iframe capturing behavior, right?
> 
> We have a bug today where mouse events are captured to an iframe when they
> shouldn't be (https://bugs.chromium.org/p/chromium/issues/detail?id=269917). 
> Our plan was to fix that so that mouse is never captured to a frame by
default,
> but can be explicitly captured via the setPointerCapture API.
> 
> Making OOPIF align with our existing case (captured to the frame only when
> mousedown isn't canceled) would make sense.  But I'm a little worried about
the
> impact of capturing OOPIF cases always while non-OOPIF wont - makes it harder
> for web developers to reason about.
> 
> Thoughts?

Interesting, thanks for the feedback.

On 269917, won't things get weird with the two cases I am trying to address with
this CL? It is especially awkward to have a scroll bar stop scrolling when the
mouse cursor accidentally exits the frame. That seems to be a consequence of
defaulting to not capture, though.

I don't quite understand part of your comment. How does this make OOPIF
capturing behavior differ from current iframe capturing behavior?

[Rick Byers, 2016-09-14 16:26:46]

On 2016/09/14 15:45:04, kenrb wrote:
> On 2016/09/14 15:11:17, Rick Byers wrote:
> > It sounds like this will make OOPIF mouse capturing behavior different than
> > iframe capturing behavior, right?
> > 
> > We have a bug today where mouse events are captured to an iframe when they
> > shouldn't be (https://bugs.chromium.org/p/chromium/issues/detail?id=269917).

> > Our plan was to fix that so that mouse is never captured to a frame by
> default,
> > but can be explicitly captured via the setPointerCapture API.
> > 
> > Making OOPIF align with our existing case (captured to the frame only when
> > mousedown isn't canceled) would make sense.  But I'm a little worried about
> the
> > impact of capturing OOPIF cases always while non-OOPIF wont - makes it
harder
> > for web developers to reason about.
> > 
> > Thoughts?
> 
> Interesting, thanks for the feedback.
> 
> On 269917, won't things get weird with the two cases I am trying to address
with
> this CL? It is especially awkward to have a scroll bar stop scrolling when the
> mouse cursor accidentally exits the frame. That seems to be a consequence of
> defaulting to not capture, though.

Oh yeah that can't happen for sure.  IIRC blink has code to explicitly capture
cases like scrollbars.

The way this would ideally work IMHO is that mouse would not be captured by
default, but blink can request capture (eg. by default for widgets like
scrollbars, and explicitly via the setPointerCapture JavaScript API).

> I don't quite understand part of your comment. How does this make OOPIF
> capturing behavior differ from current iframe capturing behavior?

If the user mouses down inside an iframe, the page calls preventDefault on the
mousedown event and the user drags out then today the iframe gets a mouseleave
and the additional movement events go to the outer frame.  It sounds like this
isn't the behavior the page will see when the frame is OOPIF.  I don't know
specific sites offhand that would be broken by that but I'd guess that they
exist somewhere :-)

[nasko, 2016-09-14 16:36:48]

content/ LGTM with couple of nits.

https://codereview.chromium.org/2339503002/diff/2/content/browser/site_per_pr...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2339503002/diff/2/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:1303: // cursor crosses over
inter-process frame boundaries.
nit: s/inter-process/cross-process/

https://codereview.chromium.org/2339503002/diff/2/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:1315: NavigateToURL(shell(),
main_url);
EXPECT_TRUE

https://codereview.chromium.org/2339503002/diff/2/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:1388: mouse_event.y = 1;
nit: Should we actually move the mouse event to a coordinate different than the
previous one? The previous MouseMove was also 1,1, so I wonder if there would be
some optimization code (now or future) that might not send events if the coords
didn't change. Maybe move to 1,2?

[kenrb, 2016-09-14 17:31:56]

On 2016/09/14 16:26:46, Rick Byers wrote:
> Oh yeah that can't happen for sure.  IIRC blink has code to explicitly capture
> cases like scrollbars.
>
> The way this would ideally work IMHO is that mouse would not be captured by
> default, but blink can request capture (eg. by default for widgets like
> scrollbars, and explicitly via the setPointerCapture JavaScript API).
>

That makes sense.
 
> > I don't quite understand part of your comment. How does this make OOPIF
> > capturing behavior differ from current iframe capturing behavior?
> 
> If the user mouses down inside an iframe, the page calls preventDefault on the
> mousedown event and the user drags out then today the iframe gets a mouseleave
> and the additional movement events go to the outer frame.  It sounds like this
> isn't the behavior the page will see when the frame is OOPIF.  I don't know
> specific sites offhand that would be broken by that but I'd guess that they
> exist somewhere :-)

I could make this match that behavior (modulo events received during IPC round
trip latency), but it would require a Blink change. It might be mitigated
somewhat by the fact that this only happens across origins, so the frames can't
be aware of the events targeted to each other.

Is it reasonable to land this as is and file a bug to add a mechanism for Blink
to notify the browser process about preventDefault on a mousedown to cancel the
capture?

[kenrb, 2016-09-14 20:33:32]

https://codereview.chromium.org/2339503002/diff/2/content/browser/site_per_pr...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2339503002/diff/2/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:1303: // cursor crosses over
inter-process frame boundaries.
On 2016/09/14 16:36:47, nasko (slow) wrote:
> nit: s/inter-process/cross-process/

Done.

https://codereview.chromium.org/2339503002/diff/2/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:1315: NavigateToURL(shell(),
main_url);
On 2016/09/14 16:36:48, nasko (slow) wrote:
> EXPECT_TRUE

Done.

https://codereview.chromium.org/2339503002/diff/2/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:1388: mouse_event.y = 1;
On 2016/09/14 16:36:48, nasko (slow) wrote:
> nit: Should we actually move the mouse event to a coordinate different than
the
> previous one? The previous MouseMove was also 1,1, so I wonder if there would
be
> some optimization code (now or future) that might not send events if the
coords
> didn't change. Maybe move to 1,2?

Done.

[kenrb, 2016-09-14 20:40:49]

The CQ bit was checked by kenrb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 20:41:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 21:03:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 21:03:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kenrb, 2016-09-15 17:46:16]

The CQ bit was checked by kenrb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 17:46:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 19:20:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 19:20:15]

Dry run: This issue passed the CQ dry run.

[kenrb, 2016-09-15 19:52:18]

Bug filed for the preventDefault problem,
https://bugs.chromium.org/p/chromium/issues/detail?id=647378.

[kenrb, 2016-09-15 19:52:28]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2016-09-15 19:52:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtapuska@chromium.org, mustaq@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2339503002/#ps90001
(title: "Fix test breakage caused by rebase")

[commit-bot: I haz the power, 2016-09-15 19:53:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Rick Byers, 2016-09-15 20:00:03]

On 2016/09/14 17:31:56, kenrb wrote:
> On 2016/09/14 16:26:46, Rick Byers wrote:
> > Oh yeah that can't happen for sure.  IIRC blink has code to explicitly
capture
> > cases like scrollbars.
> >
> > The way this would ideally work IMHO is that mouse would not be captured by
> > default, but blink can request capture (eg. by default for widgets like
> > scrollbars, and explicitly via the setPointerCapture JavaScript API).
> >
> 
> That makes sense.
>  
> > > I don't quite understand part of your comment. How does this make OOPIF
> > > capturing behavior differ from current iframe capturing behavior?
> > 
> > If the user mouses down inside an iframe, the page calls preventDefault on
the
> > mousedown event and the user drags out then today the iframe gets a
mouseleave
> > and the additional movement events go to the outer frame.  It sounds like
this
> > isn't the behavior the page will see when the frame is OOPIF.  I don't know
> > specific sites offhand that would be broken by that but I'd guess that they
> > exist somewhere :-)
> 
> I could make this match that behavior (modulo events received during IPC round
> trip latency), but it would require a Blink change. It might be mitigated
> somewhat by the fact that this only happens across origins, so the frames
can't
> be aware of the events targeted to each other.
> 
> Is it reasonable to land this as is and file a bug to add a mechanism for
Blink
> to notify the browser process about preventDefault on a mousedown to cancel
the
> capture?

For the record (Ken and I talked offline) yes this sounds fine to me.  At some
point before shipping OOPIF generally we should take a good look at the web
compat / interop impact - try to verify that OOPIFs are really transparent to
JavaScript (or at least catalog the known issues and convince ourselves they're
OK - probably as part of a blink intent to ship).  But it's not urgent.

[commit-bot: I haz the power, 2016-09-15 20:00:13]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2016-09-15 20:01:56]

Description was changed from

==========
Allow OOPIFs to capture mouse input while mouse button is held down

Currently, if you perform a mouse dragging action on an OOPIF such as
moving a scroll bar or selecting text, and the mouse cursor moves out
of the OOPIF, input events get routed to the parent frame so the
dragging action stops working. This CL cause the browser process to
lock input to a single RenderWidgetHostView while a mouse button is
being held down, correcting that problem.

BUG=529377
==========

to

==========
Allow OOPIFs to capture mouse input while mouse button is held down

Currently, if you perform a mouse dragging action on an OOPIF such as
moving a scroll bar or selecting text, and the mouse cursor moves out
of the OOPIF, input events get routed to the parent frame so the
dragging action stops working. This CL cause the browser process to
lock input to a single RenderWidgetHostView while a mouse button is
being held down, correcting that problem.

BUG=529377

Committed: https://crrev.com/30d21915acf01e3cb5746b1a3cb2e8786c2bd841
Cr-Commit-Position: refs/heads/master@{#418940}
==========

[commit-bot: I haz the power, 2016-09-15 20:01:57]

Patchset 6 (id:??) landed as
https://crrev.com/30d21915acf01e3cb5746b1a3cb2e8786c2bd841
Cr-Commit-Position: refs/heads/master@{#418940}

[hshi1, 2016-09-21 05:48:13]

hshi@chromium.org changed reviewers:
+ hshi@chromium.org

[hshi1, 2016-09-21 05:48:14]

Dear authors and reviewers: this CL seems to break the ARC++ opt-in flow (see
b/31591440). Can you please take a look, thanks.