Implement WebContentsViewChildFrame::TakeFocus.

content/browser/web_contents/web_contents_view_child_frame.cc

Index: content/browser/web_contents/web_contents_view_child_frame.cc
diff --git a/content/browser/web_contents/web_contents_view_child_frame.cc b/content/browser/web_contents/web_contents_view_child_frame.cc
index 9b17012781b3864279b2473a25ca08ae97a6c877..a7923ac4c999fa7723cbf68907a2018a38786fae 100644
--- a/content/browser/web_contents/web_contents_view_child_frame.cc
+++ b/content/browser/web_contents/web_contents_view_child_frame.cc
@@ -5,6 +5,7 @@
 #include "content/browser/web_contents/web_contents_view_child_frame.h"
 
 #include "build/build_config.h"
+#include "content/browser/frame_host/render_frame_proxy_host.h"
 #include "content/browser/frame_host/render_widget_host_view_child_frame.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/web_contents_view_delegate.h"
@@ -152,8 +153,15 @@ void WebContentsViewChildFrame::GotFocus() {
 }
 
 void WebContentsViewChildFrame::TakeFocus(bool reverse) {

[alexmos, 2016/11/22 02:45:27]

Could we also update the documentation on ViewHostMsg_TakeFocus while we're at
it?  It currently says: 

// Tells the browser to move the focus to the next (previous if reverse is
// true) focusable element.

This seems outdated - I think it should say something about advancing focus to
Chrome itself if this is the top-level view, or, in the <webview> case, continue
focus traversal in an outer WebContents if it exists.

[alexmos, 2016/11/22 02:45:27]

How does the other part of tab traversal work, when you need to descend from
outer contents into inner contents?  Does that already work today, or something
you will be fixing in a followup patch?

(FWIW, I tried out this patch on the "Webview Transparency" sample, with two
<webviews> and each having some <input>'s, and the tab traversal wasn't going
from one webview to the next, when starting from one of the inputs in either
<webview>.  Interestingly, it worked fine when starting from the embedder.  You
may want to double-check this case.)

[avallee, 2016/11/25 19:39:54]

The problem here I think is that tabbing into the webview will have the embedder
renderer show the webview as the document.activeElement.

If clicking into the webview, the embedder does not know it lost focus to the
webview and doesn't update the active element. In this case it doesn't know
where to continue from (I think.) This seems strange since I though the source
frame should have handled this.

On further investigation it seems the failure is the early out here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu...

It seems that the next element after the webview (in the capture visible region
sample, the 'screenshot' button) is focused and the one that should have gotten
focus.

I think that we need to focus the webview's frame before attempting to move
focus to ensure that something else is not. The other option would be to clear
the focused element when moving into the webview.

[avallee, 2016/11/25 19:39:54]

Done.

[alexmos, 2016/11/28 23:52:23]

Yes, this is indeed strange, since the early out you mention below happens after
the call to findFocusableElementAcrossFocusScopes, which should've gotten the
right HTMLFrameOwnerElement as the |start| element (which is computed using the
source frame).  Can you verify that this is indeed what's happening, and |start|
is valid?
Yes, I can see how that is needed for document.activeElement.  I'm still not
100% sure I see how not having the <webview> frame focused in the embedder leads
to the bug above, given that we plumb in the source frame element.

Clearing the focused element when moving into/out of the <webview> would be
similar to what we did for OOPIFs, here:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu...

Is moving into a <webview> for tab traversal also going to go through this code?
 I'm just wondering if that's any easier to do.

[avallee, 2016/12/09 21:16:51]

On further investigation, this is what happens. Oddly enough, when clicking into
the webview, the webview will become the focused/active element in some cases.
For example, in the sample app, if the body is the active element, clicking the
body of the webview will focus the webview element in the embedder. If there was
a button focused, the button stays focused.

When tabbing into the webview, the embedder will go through the code path you
mention above and clear the focused element in the embedder document. Now if the
screenshot button happens to be focused and you click the webview, you'll now be
in the webview and tabbing through its elements. However, clicking the webview
leaves the button element focused in the embedder. Then, when we focus forward
past the end of the webview contents, that's when we ask the embedder to
continue focus and it starts from the right place and then decides that the
button is the next element correctly. However, it decides to early out since it
notices the button is already focused.

Yes, this is called fromn advanceFocusAcrossFrames with the start element set to
the HTMLFrameOwnerElement(fromFrame->Owner());

This also seems to be broken with oopifs.
Yes, it looks like tabbing into the webview goes through this code path.

[alexmos, 2016/12/12 22:41:15]

Wow, good find, thanks for the investigation.  Indeed, I've reproed this with
OOPIFs:

1. go to http://csreis.github.io/tests/cross-site-iframe-simple.html with
--site-per-process
2. from devtools, document.body.appendChild(document.createElement("input"))
3. click on the new <input>
4. click on an <input> in the OOPIF, then keep tabbing.  When reaching the end,
focus should shift to the new <input> in the main frame, but instead it wraps
back into the OOPIF.

Can you please file a bug with these steps, along with the webview repro steps? 
I checked that document.activeElement is also wrong -- it should be pointing at
the <iframe>/<webview> element after the click in step 4, but it keeps pointing
at the old <input>.  No need to fix this here, but I can take a look at the
OOPIF side of this in a followup.  I think we just need to update activeElement
to the right HTMLFrameOwnerElement in this case, when we receive a message in
the parent frame that a RemoteFrame is focused.

[avallee, 2017/01/11 00:48:49]

Done. Filed: https://bugs.chromium.org/p/chromium/issues/detail?id=674219

-  // TODO(avallee): http://crbug.com/610819 Advance focus to next element in
-  // outer WebContents.
+  RenderFrameProxyHost* rfp =
+      web_contents_->GetRenderManager()->GetProxyToOuterDelegate();

[alexmos, 2016/11/22 02:45:27]

It seems you can avoid making GetRenderManager public and get this via
web_contents_->GetMainFrame()->frame_tree_node()->GetRenderManager(), though
that's a long detour.  OTOH, if you make GetRenderManager public, then you
probably should also remove GetRenderManagerForTesting() (perhaps in a
followup).  I'd ask creis@ or nasko@ for opinions on whether it's ok to make
GetRenderManager() public in web_contents_impl.h.

[avallee, 2016/11/25 19:39:54]

I'll use the detour for now. There's already a comment there saying it should be
removed.

+  FrameTreeNode* outer_node = FrameTreeNode::GloballyFindByID(
+      web_contents_->GetOuterDelegateFrameTreeNodeId());
+  RenderFrameHostImpl* rfhi =
+      outer_node->parent()->render_manager()->current_frame_host();
+
+  rfhi->AdvanceFocus(
+      reverse ? blink::WebFocusTypeBackward : blink::WebFocusTypeForward, rfp);
 }
 
 void WebContentsViewChildFrame::ShowContextMenu(