Fix keyboard focus for OOPIF-<webview>.

content/browser/web_contents/web_contents_impl.cc

Index: content/browser/web_contents/web_contents_impl.cc
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index bb4128c8f0fda709aef73e59c5b2819c0d1bd7d6..ad213f1d8e464ca02aa254d3597e64637338f47f 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -328,28 +328,30 @@ WebContentsImpl::ColorChooserInfo::~ColorChooserInfo() {
 }
 
 // WebContentsImpl::WebContentsTreeNode ----------------------------------------
-WebContentsImpl::WebContentsTreeNode::WebContentsTreeNode()
-    : outer_web_contents_(nullptr),
+WebContentsImpl::WebContentsTreeNode::WebContentsTreeNode(
+    WebContentsImpl* inner)
+    : inner_web_contents_(inner),
+      outer_web_contents_(nullptr),
       outer_contents_frame_tree_node_id_(
-          FrameTreeNode::kFrameTreeNodeInvalidId) {
-}
+          FrameTreeNode::kFrameTreeNodeInvalidId) {}
 
 WebContentsImpl::WebContentsTreeNode::~WebContentsTreeNode() {
   // Remove child pointer from our parent.
   if (outer_web_contents_) {
-    ChildrenSet& child_ptrs_in_parent =
+    ChildrenMap& child_ptrs_in_parent =
         outer_web_contents_->node_->inner_web_contents_tree_nodes_;
-    ChildrenSet::iterator iter = child_ptrs_in_parent.find(this);
+    ChildrenMap::iterator iter =
+        child_ptrs_in_parent.find(outer_contents_frame_tree_node_id_);
     DCHECK(iter != child_ptrs_in_parent.end());
-    child_ptrs_in_parent.erase(this);
+    child_ptrs_in_parent.erase(iter);
   }
 
   // Remove parent pointers from our children.
   // TODO(lazyboy): We should destroy the children WebContentses too. If the
   // children do not manage their own lifetime, then we would leak their
   // WebContentses.
-  for (WebContentsTreeNode* child : inner_web_contents_tree_nodes_)
-    child->outer_web_contents_ = nullptr;
+  for (auto& children : inner_web_contents_tree_nodes_)

[alexmos, 2016/05/12 20:28:41]

nit: s/children/child/  (it's still referring to just one WebContentsTreeNode*,
right?)

[avallee, 2016/05/16 20:26:43]

Done.

+    children.second->outer_web_contents_ = nullptr;
 }
 
 void WebContentsImpl::WebContentsTreeNode::ConnectToOuterWebContents(
@@ -360,9 +362,21 @@ void WebContentsImpl::WebContentsTreeNode::ConnectToOuterWebContents(
       outer_contents_frame->frame_tree_node()->frame_tree_node_id();
 
   if (!outer_web_contents_->node_)
-    outer_web_contents_->node_.reset(new WebContentsTreeNode());
+    outer_web_contents_->node_.reset(
+        new WebContentsTreeNode(outer_web_contents_));
+
+  outer_web_contents_->node_
+      ->inner_web_contents_tree_nodes_[outer_contents_frame_tree_node_id_] =
+      this;
+}
 
-  outer_web_contents_->node_->inner_web_contents_tree_nodes_.insert(this);
+const WebContentsImpl*
+WebContentsImpl::WebContentsTreeNode::FindContentsForFrame(int frame_id) const {
+  auto iter = inner_web_contents_tree_nodes_.find(frame_id);
+  if (iter != inner_web_contents_tree_nodes_.end()) {
+    return iter->second->inner_web_contents_;
+  }
+  return nullptr;
 }
 
 // WebContentsImpl -------------------------------------------------------------
@@ -1393,7 +1407,7 @@ void WebContentsImpl::AttachToOuterWebContentsFrame(
     CreateRenderWidgetHostViewForRenderManager(GetRenderViewHost());
 
   // Create a link to our outer WebContents.
-  node_.reset(new WebContentsTreeNode());
+  node_.reset(new WebContentsTreeNode(this));
   node_->ConnectToOuterWebContents(
       static_cast<WebContentsImpl*>(outer_web_contents),
       static_cast<RenderFrameHostImpl*>(outer_contents_frame));
@@ -1793,6 +1807,16 @@ RenderWidgetHostImpl* WebContentsImpl::GetFocusedRenderWidgetHost(
   if (!focused_frame)
     return receiving_widget;
 
+  // If there is an inner web contents (<webview>, <guestview>, ...) with focus,
+  // return its RenderWidgetHost.

[alexmos, 2016/05/12 20:28:40]

This will recurse down if there are multiple levels of WebContents nesting,
right?  Might be worth mentioning that explicitly.

[avallee, 2016/05/16 20:26:43]

Done.

+  WebContentsImpl* inner_contents =
+      (node_.get())

[alexmos, 2016/05/12 20:28:42]

nit: .get() shouldn't be necessary

[avallee, 2016/05/16 20:26:44]

Done.

+          ? node_->FindContentsForFrame(focused_frame->frame_tree_node_id())
+          : nullptr;
+  if (inner_contents) {
+    return inner_contents->GetFocusedRenderWidgetHost(
+        inner_contents->GetMainFrame()->GetRenderWidgetHost());
+  }
   // The view may be null if a subframe's renderer process has crashed while
   // the subframe has focus.  Drop the event in that case.  Do not give
   // it to the main frame, so that the user doesn't unexpectedly type into the
@@ -4485,6 +4509,66 @@ void WebContentsImpl::EnsureOpenerProxiesExist(RenderFrameHost* source_rfh) {
   }
 }
 
+void WebContentsImpl::SetFocusedFrame(FrameTreeNode* node,
+                                      SiteInstance* source) {
+  SetFocusedFrameInternal(node, source, true /* can visit outer contents */);
+
+  auto rwh = node->current_frame_host()->GetRenderWidgetHost();

[alexmos, 2016/05/12 20:28:40]

nit: RenderWidgetHostImpl* instead of auto might be better for readability
(otherwise it's not clear whether it's an Impl or not, for instance), and is
more consistent with existing style.  Same for autos in function below.

[avallee, 2016/05/16 20:26:43]

Done.

+  if (rwh)
+    rwh->Focus();

[alexmos, 2016/05/12 20:28:40]

Why is this necessary?  Page focus and frame focus are usually disjoint -- you
can have frame focus but no page focus.  It doesn't seem right that for example
doing a window.focus() on a subframe would automatically give page focus as
well.  This also might result in an incorrect isFocused/isActive state in
blink::FocusController if you alt-tab out.

Is this due to WebContentsImpl::ReplicatePageFocus not working with inner/outer
WebContents?  Perhaps that should be fixed first?

[avallee, 2016/05/16 20:26:43]

Without this call, the renderer for the webview does not believe it is focused.
There is no highlight if I select an input box and no blinking caret. The caret
and focus border do disappear from the embedder though.

[alexmos, 2016/05/19 00:08:09]

Yup, that very likely indicates a problem with page focus.  I ran into the same
issue when fixing keyboard input in OOPIFs (see https://crbug.com/530663,
r355833 and https://goo.gl/B9S5ou).

I don't think having this call here is correct -- it's a bandaid over the real
problem, and it might break things (see my earlier comment).  FWIW, I've tried
out the current CL with 
https://chrome.google.com/webstore/detail/browser-sample/edggnmnajhcbhlnpjnog...,
and noticed that if you focus the Google search <input> field, and then click on
a different window (or just alt-tab out), that keeps the Google keyboard focus
blinking in the inactive webview window, which is wrong since the webview window
lost page focus.

I think we should just fix WebContentsImpl::ReplicatePageFocus for inner/outer
contents and whatever else is necessary for page focus instead of this call. 
(Fine to do it in a separate CL before/after.)

[avallee, 2016/05/24 20:07:07]

Added note in code. Will fix in follow-up cl. My first attempt to fix
ReplicatePageFocus did not help here.

+}
+
+void WebContentsImpl::SetFocusedFrameInternal(FrameTreeNode* node,
+                                              SiteInstance* source,
+                                              bool can_visit_outer_contents) {
+  // Approach: Recursively unfocus any inner web contents. Then set
+  // representation of this web contents in the outer contents to be focused,
+  // finally focus the node in current frame tree.
+  //
+  // Usually only one of unfocusing children and focusing self in parent would
+  // happen. If unfocusing a child, then we're already focused in our parent, if
+  // we have one.

[alexmos, 2016/05/12 20:28:40]

I had trouble following this comment when I first read it.  It makes sense after
looking at the code, but maybe try to rephrase to make it more clear?  The
second paragraph is especially hard to follow.

I.e., this is really doing three distinct steps:
1) Recursively clear focus in all inner WebContents.
2) Set |node| as the focused FTN in the current WebContents.
3) If there's an outer WebContents, update frame focus in its FrameTree to the
FTN representing the current WebContents.  Recursively do this for all
WebContents ancestors.

Let me make sure I understand this with an example (which may be useful in the
comment as well). If an embedder page A contains a <webview> B, which embeds a
subframe C, and focus shifts from A to C, then the outer FrameTree will update
its focused FTN from A to the FTN corresponding to the inner FrameTree, and the
inner FrameTree will update its focused FTN from null to the FTN for subframe C.
 Later, if we focus the embedder again, we will clear focused frame in the inner
FrameTree, and set the focused frame to FTN A in the outer FrameTree.  Is my
example correct?

[avallee, 2016/05/16 20:26:43]

Here's an enhanced version of your example
https://goo.gl/photos/GJLpaqP8DY7GJ5pD8

Initially, 3 is focused (1 implicitly by being the root web contents). If you
were you click on if2, we want to focus 5 and then focus 7 (1 is focused
implicitly as the root web contents, 2 is focused since frame 5 is focused).

When moving back to the embedder, we would clear 7 and then focus 3.

Here are two situations that I expect to happen eventually and requires the
slightly more complex implementation.

Document 1
<p>Content</p>
<webview name="page1"><!-- focus is here --></webview>
<webview name="page2"></webview>

In this document, if we clicked on the <p>, the web contents that originates the
event is the outer one. It would unfocus its inner contents and then focus
itself.

If we clicked on the sibling <webview>, its webcontents would trigger the focus
change. It has no inner web contents, but needs to unfocus its sibling first.
There is no recursion into inner contents, then we set focus in the outer
context. The outer context would unfocus the currently focused inner context,
possibly recur into an outer webcontents (not in the case of this example) and
then set focus in its own frame tree. Finally, it would return and the inner
contents would set focus in its own frame tree.

In this chain, there is never more than one focused element at a time.


Document 2
<div id=embedder>
<webview></webview>
<guestview name=a><guestview name=b><!--focus starts here
--></guestview></guestview>
</div>

Same idea here. If we want to focus guestview a, we need to unfocus b, then go
up to the embedder, nothing needs to be done there, then finally update the free
for the middle guestview.

I'm not sure if that was super clear, but that's the main idea.

The simpler approach might be to start at the root web contents (or focused
common ancestor to both the old and new focus web contents) and do an unfocus
traversal, then from the focused contents, traverse parents setting focus until
finally focusing itself.

[alexmos, 2016/05/19 00:08:09]

Thanks for the detailed examples, this makes sense.  I didn't consider the
sibling cases, so it helps to walk through that example.  Is it important that
the walk through outer contents happens before setting focus in the current
FrameTree?  It doesn't seem to matter, but wanted to verify.

Here's a different idea: instead of maintaining focused frames across the whole
WebContents ancestor chain, what if we only maintained one focused FrameTreeNode
across all inner/outer WebContents for a given page?  I.e., in your enhanced
example, when you click on if2, just focus 7, and clear focus from 3 (leaving
the FrameTree at the root without a focused frame).  Would anything break if you
didn't actually have 5 focused?  This would make things very simple: just
unfocus stuff in the old WebContentsTreeNode, and focus stuff in the new one
(though it probably would need to keep track of the currently focused
WebContentsTreeNode somewhere).

[avallee, 2016/05/24 20:07:07]

Your approach I think would work nicely. It's simple and I think all we need to
keep track of is which web contents has the currently focused tree.

+
+  auto old_focus_node = frame_tree_.GetFocusedFrame();

[alexmos, 2016/05/12 20:28:40]

nit: s/old_focus_node/old_focused_node/

[avallee, 2016/05/16 20:26:43]

Done.

+
+  if (old_focus_node != nullptr && node_.get()) {

[alexmos, 2016/05/12 20:28:42]

nit: .get() not necessary

[avallee, 2016/05/16 20:26:44]

Done.

+    if (!node || old_focus_node != node) {
+      WebContentsImpl* inner_contents =
+          node_->FindContentsForFrame(old_focus_node->frame_tree_node_id());
+      if (inner_contents)
+        inner_contents->SetFocusedFrameInternal(nullptr, source, false);

[alexmos, 2016/05/12 20:28:41]

I think this whole function could be simplified to make it easier to follow. 
Perhaps, rather than overloading it with being to recurse for two different
things (recursively clearing focus in inner WC, and recursively updating focus
in outer WC), maybe just make two different helpers for these two things?  Then
the main WC::SetFocusedFrame can be a really short and easy-to-follow function,
corresponding to my three steps in the comment above.  And/or, it seems you
could avoid recursion and passing a bool here by just iterating through the
focused inner_contents down, and through outer_contents_ up.  The only thing you
really need to do on each inner WC is
inner_contents->frame_tree_->SetFocusedFrame(nullptr).  

Also, I think there's no need to check if old_focused_node is not equal to
|node| when clearing inner WC focus -- FrameTree::SetFocusedFrame will already
return early if the focused frame doesn't change.

[avallee, 2016/05/16 20:26:43]

I agree that the split could work.
1. Find the common outer web contents with focus between the old and new frames.
2. unfocus the inner webcontents that are focused. (Stop recursion if the
currently focused frame has no inner webcontents). Set focused frame to nullptr
on each of those webcontents' frame trees.
3. For each outer webcontents to the one to be focused, set focused to its frame
in the outer free.
4. Set focus on the proper node in the target webcontents' frame tree.

[alexmos, 2016/05/19 00:08:09]

This makes sense, though I'm not sure if the code will be less complex.  Might
be worth trying, though I'm curious about your take on my proposal above first
(about not involving ancestors at all).  If that doesn't work, it might be ok to
just break apart the recursive inner focus clearing from the current function
into a helper?  I think this will let you remove the can_visit_outer_contents
and make the rest easier to follow.

+    }
+  }
+
+  // focus in parent.
+  auto outer_contents = GetOuterWebContents();
+  // If we're coming from parent, we don't want to recurse back into the parent.
+  if (outer_contents && can_visit_outer_contents) {
+    auto outer_node =
+        outer_contents->frame_tree_.FindByID(GetOuterDelegateFrameTreeNodeId());
+    if (outer_node == nullptr) {
+      // With browser plugin, we will have an outer contents but no node in the
+      // outer contents' frame tree.
+      // HACK: We are assuming that the guest contents can only be embedded
+      // within the embedder's main frame.
+      outer_contents->SetFocusedFrameInternal(

[alexmos, 2016/05/12 20:28:40]

Please follow up on this hack with BrowserPlugin experts - I don't really know
how BrowserPlugin focus works and whether this makes sense.

[avallee, 2016/05/16 20:26:44]

We tested it and it seems we can only embed in the main frame.

[alexmos, 2016/05/19 00:08:09]

Acknowledged.  Can you remove the "HACK" from the comment then?

[avallee, 2016/05/24 20:07:07]

Deleted this whole function.

+          outer_contents->frame_tree_.root(), source, true);
+    } else {
+      // Notify outer about our focus change.
+      auto outer_node_site_instance =
+          outer_node->parent()->current_frame_host()->GetSiteInstance();
+      auto outer_proxy =
+          frame_tree_.root()->render_manager()->GetRenderFrameProxyHost(
+              outer_node_site_instance);
+      if (outer_proxy)

[alexmos, 2016/05/12 20:28:40]

Why would outer_proxy be null?  After talking through <webview> proxy setup with
lfg@, it seems to me that this proxy should always exit.

[avallee, 2016/05/16 20:26:44]

Agreed, it should exist.

[alexmos, 2016/05/19 00:08:09]

So then, can you remove this if?

[avallee, 2016/05/24 20:07:07]

Deleted

+        outer_proxy->SetFocusedFrame();

[alexmos, 2016/05/12 20:28:42]

Won't you notify the embedder's SiteInstance as part of
FrameTree::SetFocusedFrame when it's called as part of
outer_contents->SetFocusedFrameInternal?  Why is this separate call necessary?

[avallee, 2016/05/16 20:26:43]

For the webview specific case, the rfp that we want to update is owned by the
root of the inner frame tree for the chrome-guest://... site instance no not
findable in the parent frame tree. Also, I'm not 100% on this, but it also seems
like CollectSiteInstances in the inner frame tree does not find that instance
and so never notifies it.

[alexmos, 2016/05/19 00:08:09]

Sorry, I'm confused about this.  Consider the sample webview app in the crbug. 
The embedder's FrameTree will have two FTNs, with the following SiteInstances
for current_frame_host():
   FTN_1 [chrome-extension://extid]
     |
   FTN_2 [chrome-extension://extid]

FTN_1 is for the main page, FTN_2 is for the <webview> (it's sitting at
about:blank, its RFH is unused (its RenderFrame isn't live), and its
SiteInstance matches its parent).

The inner contents will have a FTN_3, with SiteInstance [chrome-guest://extid]. 
 That FTN will also have a proxy RFP_1 for [chrome-extension://extid] (e.g.,
this allows the embedder to postMessage the webview).  IIUC, there are no
proxies created initially in the outer FrameTree FTNs, but one may be created
on-demand (RFP_2) for [chrome-guest://extid] in FTN_1 if embedder postMessages
the guest, so that the guest can reply to the message.

Now, the code as written will find the proxy RFP_1 and set the corresponding
remote frame as focused in the chrome-extension:// renderer.  But you said "for
the chrome-guest://... site instance", and there's no such proxy in FTN_3.  Did
you mean the chrome-extension:// RFP_1?

Now, in my original question, I was surprised by this, since I thought the
chrome-extension:// renderer should update focus when processing its own
FrameTree.  But I think I know why: when you recurse into outer WebContents and
set focus to FTN_2, it tries to send a message to its current_frame_host(), but
that isn't live, so the SetFocusedFrame message isn't sent and
chrome-extensions:// renderer isn't notified.  This is why you need to go
through RFP_1.  Correct?

Additionally, I agree with you that RFP_1 won't be found by CollectSiteInstances
in inner FrameTree::SetFocusedFrame, since chrome-extension:// won't correspond
to any of the RFHs in the inner tree.  

OK, the outer_proxy makes sense then.  But it's really subtle and needs a
thorough comment for why this isn't handled in any of the
FrameTree::SetFocusedFrame calls. :)

[avallee, 2016/05/24 20:07:07]

I think that this is no longer necessary with the much simplified version of
focus with no recursion. I must check that the CL for page focus makes this
correct.

+      outer_contents->SetFocusedFrameInternal(outer_node, source, true);
+    }
+  }
+  // Set focus for myself.
+  frame_tree_.SetFocusedFrame(node, source);
+}
+
 bool WebContentsImpl::AddMessageToConsole(int32_t level,
                                           const base::string16& message,
                                           int32_t line_no,