Prevent browser-side hit testing when page popup is selected

Prevent browser-side hit testing when page popup is selected

When any form of cross-process frames are enabled, the browser process
locks mouse input to a RenderWidgetHostView while a mouse button is
being held down. This also applies to page popups, which causes a
problem on Linux when a user drag-selects a drop-down menu from a
<select> element, because the popup does not receive any input until
the button is released.

This changelist exempts popup RenderWidgetHostViewAuras from browser-
side hit testing, ignoring mouse capture.

BUG=703191
Review-Url: https://codereview.chromium.org/2770923002
Cr-Commit-Position: refs/heads/master@{#459476}
Committed: https://chromium.googlesource.com/chromium/src/+/11f213aad14e128ee471fe900a10a05936384f41

[kenrb, 2017-03-22 22:19:59]

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

[commit-bot: I haz the power, 2017-03-22 22:20:34]

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, 2017-03-22 22:55:59]

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

[commit-bot: I haz the power, 2017-03-22 22:56:00]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[kenrb, 2017-03-23 15:07:23]

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

[commit-bot: I haz the power, 2017-03-23 15:07:41]

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, 2017-03-23 16:03:30]

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

[commit-bot: I haz the power, 2017-03-23 16:03:31]

Dry run: This issue passed the CQ dry run.

[kenrb, 2017-03-23 16:46:19]

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

[kenrb, 2017-03-23 16:46:20]

alexmos: PTAL?

[alexmos, 2017-03-23 22:25:19]

Thanks for fixing this!  LGTM with a test suggestion.

https://codereview.chromium.org/2770923002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/2770923002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:225: virtual bool
IsWidgetForMainFrame(RenderWidgetHostImpl*);
nit: A comment here on what this is for might be useful.

https://codereview.chromium.org/2770923002/diff/20001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2770923002/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:6212: click_event.y = 90;
The 90 here seems weird to me, since it seems that the <select> sits at y=94
(from popup_rect.y() above), so to click it in root frame's coordinates,
wouldn't this need to be >94?  The test still works, so I'm probably just
confused by how the coordinates work out.

https://codereview.chromium.org/2770923002/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:6225: // Next send a mouse up
directly targeting the first option, simulating a
It'd actually be nice to target one of the other options, and then verify that
the selection was changed correctly, just like in the bug repro.  Might be worth
doing if it's easy.

[Charlie Reis, 2017-03-23 23:41:21]

Description was changed from

==========
Prevent browser-side hit testing when page popup is selected

When any form of cross-process frames are enabled, the browser process
locks mouse input to a RenderWidgetHostView while a mouse button is
being held down. This also applies to page popups, which causes a
problem on Linux when a user drag-selects a drop-down menu from a
<select> element, because the popup does not receive any input until
the button is released.

This changelist exempts popup RenderWidgetHostViewAuras from browser-
side hit testing, ignoring mouse capture.

BUG=703191
==========

to

==========
Prevent browser-side hit testing when page popup is selected

When any form of cross-process frames are enabled, the browser process
locks mouse input to a RenderWidgetHostView while a mouse button is
being held down. This also applies to page popups, which causes a
problem on Linux when a user drag-selects a drop-down menu from a
<select> element, because the popup does not receive any input until
the button is released.

This changelist exempts popup RenderWidgetHostViewAuras from browser-
side hit testing, ignoring mouse capture.

BUG=703191
==========

[kenrb, 2017-03-24 14:18:43]

Thanks for the review.

https://codereview.chromium.org/2770923002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/2770923002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:225: virtual bool
IsWidgetForMainFrame(RenderWidgetHostImpl*);
On 2017/03/23 22:25:18, alexmos wrote:
> nit: A comment here on what this is for might be useful.

Done. I had intended to do that but forgot, thanks for catching it.

https://codereview.chromium.org/2770923002/diff/20001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2770923002/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:6212: click_event.y = 90;
On 2017/03/23 22:25:18, alexmos wrote:
> The 90 here seems weird to me, since it seems that the <select> sits at y=94
> (from popup_rect.y() above), so to click it in root frame's coordinates,
> wouldn't this need to be >94?  The test still works, so I'm probably just
> confused by how the coordinates work out.

The y >= 94 is for the position of the drop-down menu, which appears below the
element.

https://codereview.chromium.org/2770923002/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:6225: // Next send a mouse up
directly targeting the first option, simulating a
On 2017/03/23 22:25:18, alexmos wrote:
> It'd actually be nice to target one of the other options, and then verify that
> the selection was changed correctly, just like in the bug repro.  Might be
worth
> doing if it's easy.

It turns out not to be easy -- if you look at the first patch set, I was trying
to do exactly that. Unfortunately to select an option in the menu you have to
send a MouseMove, which causes flakiness in content_browsertests, and would
necessitate creating a new test in interactive_ui_tests instead. Since we are
just trying to catch a specific case (i.e. verifying that popups are exempt from
mouse capture), this simpler test condition seemed adequate.

[kenrb, 2017-03-24 14:19:08]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2017-03-24 14:19:08]

The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2770923002/#ps40001
(title: "Added a comment")

[commit-bot: I haz the power, 2017-03-24 14:19:31]

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, 2017-03-24 15:51:01]

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

[commit-bot: I haz the power, 2017-03-24 15:51:03]

Failed to apply patch for content/browser/web_contents/web_contents_impl.h:
While running git apply --index -p1;
  error: patch failed: content/browser/web_contents/web_contents_impl.h:671
  error: content/browser/web_contents/web_contents_impl.h: patch does not apply

Patch:       content/browser/web_contents/web_contents_impl.h
Index: content/browser/web_contents/web_contents_impl.h
diff --git a/content/browser/web_contents/web_contents_impl.h
b/content/browser/web_contents/web_contents_impl.h
index
d8c4d1fdb017ecd05be2c11d11887abcd09347da..726269bac7c9c88f90afb70faa8b87b9fdf430c0
100644
--- a/content/browser/web_contents/web_contents_impl.h
+++ b/content/browser/web_contents/web_contents_impl.h
@@ -671,6 +671,7 @@ class CONTENT_EXPORT WebContentsImpl
   void OnFirstPaintAfterLoad(RenderWidgetHostImpl* render_widget_host)
override;
   TextInputManager* GetTextInputManager() override;
   bool OnUpdateDragCursor() override;
+  bool IsWidgetForMainFrame(RenderWidgetHostImpl* render_widget_host) override;
   bool AddDomainInfoToRapporSample(rappor::Sample* sample) override;
   void FocusedNodeTouched(bool editable) override;

[kenrb, 2017-03-24 16:57:59]

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

[commit-bot: I haz the power, 2017-03-24 16:58:39]

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, 2017-03-24 17:56:14]

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

[commit-bot: I haz the power, 2017-03-24 17:56:15]

Dry run: This issue passed the CQ dry run.

[kenrb, 2017-03-24 18:03:19]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2017-03-24 18:03:19]

The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2770923002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-24 18:04:42]

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, 2017-03-24 18:12:10]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490378599097230,
"parent_rev": "1a5be4fc4a86617e3c0700eb3be847989a7d175a", "commit_rev":
"11f213aad14e128ee471fe900a10a05936384f41"}

[commit-bot: I haz the power, 2017-03-24 18:13:37]

Description was changed from

==========
Prevent browser-side hit testing when page popup is selected

When any form of cross-process frames are enabled, the browser process
locks mouse input to a RenderWidgetHostView while a mouse button is
being held down. This also applies to page popups, which causes a
problem on Linux when a user drag-selects a drop-down menu from a
<select> element, because the popup does not receive any input until
the button is released.

This changelist exempts popup RenderWidgetHostViewAuras from browser-
side hit testing, ignoring mouse capture.

BUG=703191
==========

to

==========
Prevent browser-side hit testing when page popup is selected

When any form of cross-process frames are enabled, the browser process
locks mouse input to a RenderWidgetHostView while a mouse button is
being held down. This also applies to page popups, which causes a
problem on Linux when a user drag-selects a drop-down menu from a
<select> element, because the popup does not receive any input until
the button is released.

This changelist exempts popup RenderWidgetHostViewAuras from browser-
side hit testing, ignoring mouse capture.

BUG=703191

Review-Url: https://codereview.chromium.org/2770923002
Cr-Commit-Position: refs/heads/master@{#459476}
Committed:
https://chromium.googlesource.com/chromium/src/+/11f213aad14e128ee471fe900a10...
==========

[commit-bot: I haz the power, 2017-03-24 18:13:38]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/11f213aad14e128ee471fe900a10...