OOPIF: Enable TabAndMouseFocusNavigation.

OOPIF: Enable TabAndMouseFocusNavigation.

When advancing focus into an iframe, allow the frame to focus itself
after deciding which element will be focused. This prevents sending a
focus event to the previous element in that frame only to blur it
immediately. Non-oopifs simply fire a focus event to the new element,
the previously focused element in that frame received a blur event when
another frame was focused.

The test ensures a consistent state when mixing tab and clicking
navigation. In specific cases, focus would early out when the element to
advance to was previously focused, and never cleared when the mouse is
clicked in another frame.

BUG=702330
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2796533002
Cr-Commit-Position: refs/heads/master@{#463006}
Committed: https://chromium.googlesource.com/chromium/src/+/6a539b43629daa2a49d5603fea5ade7d48d31dd2

[avallee, 2017-04-01 03:48:35]

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

[commit-bot: I haz the power, 2017-04-01 03:48: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, 2017-04-01 05:53:44]

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

[commit-bot: I haz the power, 2017-04-01 05:53:45]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[avallee, 2017-04-03 15:36:46]

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

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

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-04-03 18:07:08]

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

[commit-bot: I haz the power, 2017-04-03 18:07:08]

Dry run: This issue passed the CQ dry run.

[avallee, 2017-04-04 18:43:26]

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

[commit-bot: I haz the power, 2017-04-04 18:44:15]

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

[avallee, 2017-04-04 18:45:50]

Description was changed from

==========
OOPIF: Enable TabAndMouseFocusNavigation.

Variant where the focusDocumentView is not special-cased for remote
frames.

BUG=702330
==========

to

==========
OOPIF: Enable TabAndMouseFocusNavigation.

When advancing focus into an iframe, allow the frame to focus itself
after deciding which element will be focused. This prevents sending a
focus event to the previous element in that frame only to blur it
immediately. Non-oopifs simply fire a focus event to the new element,
the previously focused element in that frame received a blur event when
another frame was focused.

The test ensures a consistent state when mixing tab and clicking
navigation. In specific cases, focus would early out when the element to
advance to was previously focused, and never cleared when the mouse is
clicked in another frame.

BUG=702330
==========

[avallee, 2017-04-04 18:45:50]

avallee@chromium.org changed reviewers:
+ alexmos@chromium.org, dcheng@chromium.org

[avallee, 2017-04-04 19:44:35]

Reworked version of the CL at https://codereview.chromium.org/2780883003/

[alexmos, 2017-04-05 00:21:14]

Thanks, this approach looks ok to me, and I think it makes more sense that the
previous one.  Just a couple of minor followup questions.

https://codereview.chromium.org/2796533002/diff/60001/chrome/browser/site_per...
File chrome/browser/site_per_process_interactive_browsertest.cc (right):

https://codereview.chromium.org/2796533002/diff/60001/chrome/browser/site_per...
chrome/browser/site_per_process_interactive_browsertest.cc:305: #if
(defined(OS_LINUX) && !defined(USE_OZONE)) || defined(OS_WIN)
(Carrying over the discussion from previous CL)
On 2017/04/02 02:27:30, avallee wrote:
> On 2017/03/30 17:16:11, alexmos wrote:
> > What's wrong with the test on the excluded platforms?  Is it possible to get
> the
> > test working on Mac/ChromeOS?
> 
> For Mac, I wrote a sample that just prints mousemoves and there's nothing that
> shows up.
> 
> On OZONE, the mousemoves seem to show up but then clicks don't happen.

Hmm, that's weird. Are these mousemoves really essential for this test though? 
Would things work if, instead of ui_controls::SendMouseMove/Click, you used
something like ForwardMouseEvent, or SimulateMouseClickAt from
browser_test_utils?  I think I've used these to simulate mouse clicks in tests
and haven't run into them not working on Ozone/Mac.

What about ChromeOS?  That's disabled too as it's written now.

https://codereview.chromium.org/2796533002/diff/60001/chrome/browser/site_per...
chrome/browser/site_per_process_interactive_browsertest.cc:442: //
EXPECT_EQ(main_frame, web_contents->GetFocusedFrame());
nit: remove commented out line

https://codereview.chromium.org/2796533002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2796533002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:1007:
dispatchFocusEvent(*document, *element);
Sanity check: could this have any side effects (like an extra focus event) in
non-OOPIF modes?  Doesn't seem likely, as ChromeClientImpl::canTakeFocus always
returns true, unless we're in layout test mode, so realistically I don't see how
it's possible to press tab from a focused element and wrap around to that same
focused element (since we'd return early after the canTakeFocus check above.)

https://codereview.chromium.org/2796533002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:1023: // elements in
that frame's process.
Might be worth also mentioning that contentFrame will be set as the focused
frame from its own process.  

(is this right?  I think the call that would do this is here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu...)

[avallee, 2017-04-05 15:11:24]

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

[commit-bot: I haz the power, 2017-04-05 15:12:00]

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

[avallee, 2017-04-05 15:13:01]

https://codereview.chromium.org/2796533002/diff/60001/chrome/browser/site_per...
File chrome/browser/site_per_process_interactive_browsertest.cc (right):

https://codereview.chromium.org/2796533002/diff/60001/chrome/browser/site_per...
chrome/browser/site_per_process_interactive_browsertest.cc:305: #if
(defined(OS_LINUX) && !defined(USE_OZONE)) || defined(OS_WIN)
On 2017/04/05 00:21:13, alexmos wrote:
> (Carrying over the discussion from previous CL)
> On 2017/04/02 02:27:30, avallee wrote:
> > On 2017/03/30 17:16:11, alexmos wrote:
> > > What's wrong with the test on the excluded platforms?  Is it possible to
get
> > the
> > > test working on Mac/ChromeOS?
> > 
> > For Mac, I wrote a sample that just prints mousemoves and there's nothing
that
> > shows up.
> > 
> > On OZONE, the mousemoves seem to show up but then clicks don't happen.
> 
> Hmm, that's weird. Are these mousemoves really essential for this test though?

> Would things work if, instead of ui_controls::SendMouseMove/Click, you used
> something like ForwardMouseEvent, or SimulateMouseClickAt from
> browser_test_utils?  I think I've used these to simulate mouse clicks in tests
> and haven't run into them not working on Ozone/Mac.
> 
> What about ChromeOS?  That's disabled too as it's written now.  

I've fixed the test, but requires a relatively major change to
SimulateMouseClickAt. I may need to split that out into another cl.

https://codereview.chromium.org/2796533002/diff/60001/chrome/browser/site_per...
chrome/browser/site_per_process_interactive_browsertest.cc:442: //
EXPECT_EQ(main_frame, web_contents->GetFocusedFrame());
On 2017/04/05 00:21:13, alexmos wrote:
> nit: remove commented out line

Done.

https://codereview.chromium.org/2796533002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2796533002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:1007:
dispatchFocusEvent(*document, *element);
On 2017/04/05 00:21:13, alexmos wrote:
> Sanity check: could this have any side effects (like an extra focus event) in
> non-OOPIF modes?  Doesn't seem likely, as ChromeClientImpl::canTakeFocus
always
> returns true, unless we're in layout test mode, so realistically I don't see
how
> it's possible to press tab from a focused element and wrap around to that same
> focused element (since we'd return early after the canTakeFocus check above.)

It could mean an extra focus event, but I don't know how this would be
constructed. If there's a single element and we tab through, we'd ask the
embedder to TakeFocus(). This wrapping around doesn't seem like a problem. 
Maybe there's a way to listen for a tab event that would focus the next element
and then have the default handlers advance focus onto the same element?

This early out is exactly how we are getting trapped with the change in focused
frame in the case where we steal focus and the document doesn't clear the
focused element,

Also, there seems to be no test to verify that this case would not result in a
focus event. The cl passes dry run so I'm feeling decently confident that we're
fine or we'll break everything and end up with a test case.

https://codereview.chromium.org/2796533002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:1023: // elements in
that frame's process.
On 2017/04/05 00:21:13, alexmos wrote:
> Might be worth also mentioning that contentFrame will be set as the focused
> frame from its own process.  
> 
> (is this right?  I think the call that would do this is here:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu...)

Yes, it would either be set up above where your other comment is, or down below
near the final return.

Done.

[commit-bot: I haz the power, 2017-04-05 16:14:19]

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

[commit-bot: I haz the power, 2017-04-05 16:14:19]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[alexmos, 2017-04-05 18:08:33]

Thanks, LGTM once the SimulateMouseClickAt comment is addressed.

https://codereview.chromium.org/2796533002/diff/60001/chrome/browser/site_per...
File chrome/browser/site_per_process_interactive_browsertest.cc (right):

https://codereview.chromium.org/2796533002/diff/60001/chrome/browser/site_per...
chrome/browser/site_per_process_interactive_browsertest.cc:305: #if
(defined(OS_LINUX) && !defined(USE_OZONE)) || defined(OS_WIN)
On 2017/04/05 15:13:00, avallee wrote:
> On 2017/04/05 00:21:13, alexmos wrote:
> > (Carrying over the discussion from previous CL)
> > On 2017/04/02 02:27:30, avallee wrote:
> > > On 2017/03/30 17:16:11, alexmos wrote:
> > > > What's wrong with the test on the excluded platforms?  Is it possible to
> get
> > > the
> > > > test working on Mac/ChromeOS?
> > > 
> > > For Mac, I wrote a sample that just prints mousemoves and there's nothing
> that
> > > shows up.
> > > 
> > > On OZONE, the mousemoves seem to show up but then clicks don't happen.
> > 
> > Hmm, that's weird. Are these mousemoves really essential for this test
though?
> 
> > Would things work if, instead of ui_controls::SendMouseMove/Click, you used
> > something like ForwardMouseEvent, or SimulateMouseClickAt from
> > browser_test_utils?  I think I've used these to simulate mouse clicks in
tests
> > and haven't run into them not working on Ozone/Mac.
> > 
> > What about ChromeOS?  That's disabled too as it's written now.  
> 
> I've fixed the test, but requires a relatively major change to
> SimulateMouseClickAt. I may need to split that out into another cl.

I'm ok with that change - I think it'd be great for SimulateMouseClickAt to
route events into OOPIFs if they are present, especially now that we've removed
the input forwarding path.  But yes, you might want to split it into another cl
and have kenrb@ review it.  It also seems like there's one test failure due to
it (WebViewFocusInteractiveTest.Focus_FocusRestored).

An alternative to that refactor could be to use the ForwardMouseEvent directly
on the RWH of each frame, but that probably requires too many changes to your
test.

https://codereview.chromium.org/2796533002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2796533002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:1007:
dispatchFocusEvent(*document, *element);
On 2017/04/05 15:13:00, avallee wrote:
> On 2017/04/05 00:21:13, alexmos wrote:
> > Sanity check: could this have any side effects (like an extra focus event)
in
> > non-OOPIF modes?  Doesn't seem likely, as ChromeClientImpl::canTakeFocus
> always
> > returns true, unless we're in layout test mode, so realistically I don't see
> how
> > it's possible to press tab from a focused element and wrap around to that
same
> > focused element (since we'd return early after the canTakeFocus check
above.)
> 
> It could mean an extra focus event, but I don't know how this would be
> constructed. If there's a single element and we tab through, we'd ask the
> embedder to TakeFocus(). This wrapping around doesn't seem like a problem. 
> Maybe there's a way to listen for a tab event that would focus the next
element
> and then have the default handlers advance focus onto the same element?
> 
> This early out is exactly how we are getting trapped with the change in
focused
> frame in the case where we steal focus and the document doesn't clear the
> focused element,
> 
> Also, there seems to be no test to verify that this case would not result in a
> focus event. The cl passes dry run so I'm feeling decently confident that
we're
> fine or we'll break everything and end up with a test case.

Acknowledged.

[avallee, 2017-04-05 20:43:48]

Description was changed from

==========
OOPIF: Enable TabAndMouseFocusNavigation.

When advancing focus into an iframe, allow the frame to focus itself
after deciding which element will be focused. This prevents sending a
focus event to the previous element in that frame only to blur it
immediately. Non-oopifs simply fire a focus event to the new element,
the previously focused element in that frame received a blur event when
another frame was focused.

The test ensures a consistent state when mixing tab and clicking
navigation. In specific cases, focus would early out when the element to
advance to was previously focused, and never cleared when the mouse is
clicked in another frame.

BUG=702330
==========

to

==========
OOPIF: Enable TabAndMouseFocusNavigation.

When advancing focus into an iframe, allow the frame to focus itself
after deciding which element will be focused. This prevents sending a
focus event to the previous element in that frame only to blur it
immediately. Non-oopifs simply fire a focus event to the new element,
the previously focused element in that frame received a blur event when
another frame was focused.

The test ensures a consistent state when mixing tab and clicking
navigation. In specific cases, focus would early out when the element to
advance to was previously focused, and never cleared when the mouse is
clicked in another frame.

BUG=702330
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[avallee, 2017-04-05 20:43:50]

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

[commit-bot: I haz the power, 2017-04-05 20:44:56]

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-04-05 22:36:42]

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

[commit-bot: I haz the power, 2017-04-05 22:36:43]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dcheng, 2017-04-06 20:24:33]

https://codereview.chromium.org/2796533002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2796533002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FocusController.cpp:1005: // Focus wrapped
around to the same element.
Can we update this comment? Per our chat, it seems somewhat inaccurate (and thus
a bit mystifying why we do this)

Another thought is: we could try to make this clearer... could we check that
focused frame != document->frame() and only do the frame change + event dispatch
in that case?

[avallee, 2017-04-07 18:49:19]

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

[commit-bot: I haz the power, 2017-04-07 18:49:45]

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

[avallee, 2017-04-07 18:57:36]

https://codereview.chromium.org/2796533002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2796533002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FocusController.cpp:1005: // Focus wrapped
around to the same element.
On 2017/04/06 20:24:33, dcheng wrote:
> Can we update this comment? Per our chat, it seems somewhat inaccurate (and
thus
> a bit mystifying why we do this)
> 
> Another thought is: we could try to make this clearer... could we check that
> focused frame != document->frame() and only do the frame change + event
dispatch
> in that case?

Done.

[dcheng, 2017-04-07 19:44:29]

LGTM

[commit-bot: I haz the power, 2017-04-07 20:41:50]

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

[commit-bot: I haz the power, 2017-04-07 20:41:51]

Dry run: This issue passed the CQ dry run.

[avallee, 2017-04-07 21:06:21]

The CQ bit was checked by avallee@chromium.org

[avallee, 2017-04-07 21:06:22]

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/2796533002/#ps140001
(title: "Address dcheng comments")

[commit-bot: I haz the power, 2017-04-07 21:06:56]

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-04-07 21:16:11]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1491599181602070,
"parent_rev": "c1dcea6b4c401aae65df087fe5b9b32145305a46", "commit_rev":
"6a539b43629daa2a49d5603fea5ade7d48d31dd2"}

[commit-bot: I haz the power, 2017-04-07 21:17:03]

Description was changed from

==========
OOPIF: Enable TabAndMouseFocusNavigation.

When advancing focus into an iframe, allow the frame to focus itself
after deciding which element will be focused. This prevents sending a
focus event to the previous element in that frame only to blur it
immediately. Non-oopifs simply fire a focus event to the new element,
the previously focused element in that frame received a blur event when
another frame was focused.

The test ensures a consistent state when mixing tab and clicking
navigation. In specific cases, focus would early out when the element to
advance to was previously focused, and never cleared when the mouse is
clicked in another frame.

BUG=702330
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
OOPIF: Enable TabAndMouseFocusNavigation.

When advancing focus into an iframe, allow the frame to focus itself
after deciding which element will be focused. This prevents sending a
focus event to the previous element in that frame only to blur it
immediately. Non-oopifs simply fire a focus event to the new element,
the previously focused element in that frame received a blur event when
another frame was focused.

The test ensures a consistent state when mixing tab and clicking
navigation. In specific cases, focus would early out when the element to
advance to was previously focused, and never cleared when the mouse is
clicked in another frame.

BUG=702330
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2796533002
Cr-Commit-Position: refs/heads/master@{#463006}
Committed:
https://chromium.googlesource.com/chromium/src/+/6a539b43629daa2a49d5603fea5a...
==========

[commit-bot: I haz the power, 2017-04-07 21:17:05]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/6a539b43629daa2a49d5603fea5a...

[Charlie Reis, 2017-04-08 00:20:57]

On 2017/04/07 21:17:05, commit-bot: I haz the power wrote:
> Committed patchset #8 (id:140001) as
>
https://chromium.googlesource.com/chromium/src/+/6a539b43629daa2a49d5603fea5a...

Looks like this test is flaky.  It failed a few builds after the CL landed:
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/54519

I'll revert so that it doesn't keep flaking over the weekend.  Can you take a
look on Monday to see what it would take to reland?

[Charlie Reis, 2017-04-08 00:21:33]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/2804303004/ by creis@chromium.org.

The reason for reverting is: Test is flaky on Linux Tests bot:
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/54519.