Fix keyboard focus for OOPIF-<webview>.

content/browser/frame_host/frame_tree.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/frame_tree.h"
 
 #include <stddef.h>
 
 #include <queue>
 #include <utility>
@@ skipping 233 matching lines @@
 }
 
 FrameTreeNode* FrameTree::GetFocusedFrame() {
   return FindByID(focused_frame_tree_node_id_);
 }
 
 void FrameTree::SetFocusedFrame(FrameTreeNode* node, SiteInstance* source) {
   if (node == GetFocusedFrame())
     return;
 
+  if (node == nullptr) {
+    if (GetFocusedFrame())
+      GetFocusedFrame()->current_frame_host()->UnsetFocusedFrame();

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

Do proxies need to be notified also?  E.g., suppose the guest has a main frame A
and a cross-process subframe B.  Suppose A is focused, and then the embedder
receives focus, so we get here to clear focus from A.  It seems the renderer for
B also needs to clear focus.

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

Agreed that they would need to be notified.

However, my current understanding is that all frames inside a webview are local.
I'm not sure if there are cases other than webview where the WebContents has a
child WebContents that loses focus like this.

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

I brought this up on site isolation chat, and indeed, today there's no support
for OOPIFs inside a <webview>.  However, that's going to be fixed in the future,
and it's better not to rely on this assumption when we can.  Ideally, it'd be
nice to make this future-proof, but I guess notifying proxies would require
adding all the ClearFocusedFrame plumbing for proxies?  Can we at least add a
detailed TODO to fix this once <webview> supports OOPIFs, along with a bug
reference?

+    focused_frame_tree_node_id_ = FrameTreeNode::kFrameTreeNodeInvalidId;
+
+    // TODO(avallee): https://crbug.com/610795 This line is not sufficient to

[dmazzoni, 2016/05/12 02:58:56]

I'm okay with landing this change as-is and fixing the accessibility issue in a
follow-up.
As I described in a separate email, I think you probably need to call
UpdateAXTreeData
on the root frame of the embedder WebContents.

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

Thanks for the follow up dmazzoni.

I have an idea on getting the root updated correctly. I'll follow up on that
thread with my additional questions.

+    // make the test pass. There seems to be no focus change events generated.
+    root()->current_frame_host()->UpdateAXTreeData();
+    return;
+  }
+
   std::set<SiteInstance*> frame_tree_site_instances =
       CollectSiteInstances(this);
 
   SiteInstance* current_instance =
       node->current_frame_host()->GetSiteInstance();
 
   // Update the focused frame in all other SiteInstances.  If focus changes to
   // a cross-process frame, this allows the old focused frame's renderer
   // process to clear focus from that frame and fire blur events.  It also
   // ensures that the latest focused frame is available in all renderers to
   // compute document.activeElement.
   //
   // We do not notify the |source| SiteInstance because it already knows the
   // new focused frame (since it initiated the focus change), and we notify the
   // new focused frame's SiteInstance (if it differs from |source|) separately
   // below.
   for (const auto& instance : frame_tree_site_instances) {
     if (instance != source && instance != current_instance) {
       DCHECK(SiteIsolationPolicy::AreCrossProcessFramesPossible());
       RenderFrameProxyHost* proxy =
           node->render_manager()->GetRenderFrameProxyHost(instance);
-      proxy->SetFocusedFrame();
+      if (proxy)

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

Why is adding this check necessary?

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

I don't think this is necessary anymore, but was initially getting an instance
that had no proxy and thus ended up being null. I'll check, but I think my code
no longer triggers this.

+        proxy->SetFocusedFrame();
     }
   }
 
   // If |node| was focused from a cross-process frame (i.e., via
   // window.focus()), tell its RenderFrame that it should focus.
-  if (current_instance != source)
+  if (current_instance && current_instance != source)

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

Similar question here.  I can't see how this can be null.

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

Removed.

     node->current_frame_host()->SetFocusedFrame();
 
   focused_frame_tree_node_id_ = node->frame_tree_node_id();
   node->DidFocus();
 
   // The accessibility tree data for the root of the frame tree keeps
   // track of the focused frame too, so update that every time the
   // focused frame changes.
   root()->current_frame_host()->UpdateAXTreeData();
 }
@@ skipping 134 matching lines @@
   // This is only used to set page-level focus in cross-process subframes, and
   // requests to set focus in main frame's SiteInstance are ignored.
   if (instance != root_manager->current_frame_host()->GetSiteInstance()) {
     RenderFrameProxyHost* proxy =
         root_manager->GetRenderFrameProxyHost(instance);
     proxy->Send(new InputMsg_SetFocus(proxy->GetRoutingID(), is_focused));
   }
 }
 
 }  // namespace content