Enable WebViewAccessiblityTest for OOPIF webview.

content/browser/web_contents/web_contents_impl.cc

 // Copyright (c) 2012 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/web_contents/web_contents_impl.h"
 
 #include <stddef.h>
 
 #include <cmath>
 #include <utility>
@@ skipping 362 matching lines @@
 // WebContentsImpl::WebContentsTreeNode ----------------------------------------
 WebContentsImpl::WebContentsTreeNode::WebContentsTreeNode()
     : outer_web_contents_(nullptr),
       outer_contents_frame_tree_node_id_(
           FrameTreeNode::kFrameTreeNodeInvalidId),
       focused_web_contents_(nullptr) {}
 
 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(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 child : inner_web_contents_tree_nodes_)
+    child.second->node_->outer_web_contents_ = nullptr;
 }
 
 void WebContentsImpl::WebContentsTreeNode::ConnectToOuterWebContents(
+    WebContentsImpl* web_contents,
     WebContentsImpl* outer_web_contents,
     RenderFrameHostImpl* outer_contents_frame) {
   DCHECK(!focused_web_contents_) << "Should not attach a root node.";
   outer_web_contents_ = outer_web_contents;
   outer_contents_frame_tree_node_id_ =
       outer_contents_frame->frame_tree_node()->frame_tree_node_id();
 
   if (!outer_web_contents_->node_) {
     // This will only be reached when creating a new WebContents tree.
     // Initialize the root of this tree and set it as focused.
     outer_web_contents_->node_.reset(new WebContentsTreeNode());
     outer_web_contents_->node_->SetFocusedWebContents(outer_web_contents_);
   }
 
-  outer_web_contents_->node_->inner_web_contents_tree_nodes_.insert(this);
+  outer_web_contents_->node_
+      ->inner_web_contents_tree_nodes_[outer_contents_frame_tree_node_id_] =
+      web_contents;
 }
 
 void WebContentsImpl::WebContentsTreeNode::SetFocusedWebContents(
     WebContentsImpl* web_contents) {
   DCHECK(!outer_web_contents())
       << "Only the outermost WebContents tracks focus.";
   focused_web_contents_ = web_contents;
 }
 
+WebContentsImpl* WebContentsImpl::WebContentsTreeNode::find_contents_at_node(

[alexmos, 2017/02/23 20:07:04]

Name this FindContentsAtNode, since it's not a simple getter/setter.  Or better
yet, FindInnerWebContentsAtNode to clarify exactly what it's getting.

[avallee, 2017/02/28 20:13:00]

Done.

+    int frame_tree_node_id) {
+  auto iter = inner_web_contents_tree_nodes_.find(frame_tree_node_id);
+  if (iter == inner_web_contents_tree_nodes_.end())
+    return nullptr;
+  return iter->second;
+}
+
 // WebContentsImpl -------------------------------------------------------------
 
 WebContentsImpl::WebContentsImpl(BrowserContext* browser_context)
     : delegate_(NULL),
       controller_(this, browser_context),
       render_view_host_delegate_view_(NULL),
       created_with_opener_(false),
       frame_tree_(new NavigatorImpl(&controller_, this),
                   this,
                   this,
@@ skipping 1000 matching lines @@
   // before attaching. If the browser side is already initialized, the calls
   // below will just early return.
   render_manager->InitRenderView(GetRenderViewHost(), nullptr);
   GetMainFrame()->Init();
   if (!render_manager->GetRenderWidgetHostView())
     CreateRenderWidgetHostViewForRenderManager(GetRenderViewHost());
 
   // Create a link to our outer WebContents.
   node_.reset(new WebContentsTreeNode());
   node_->ConnectToOuterWebContents(
-      static_cast<WebContentsImpl*>(outer_web_contents),
+      this, static_cast<WebContentsImpl*>(outer_web_contents),

[alexmos, 2017/02/23 20:07:04]

Maybe it would help if WebContentsTreeNode just knew about the WebContents it's
associated with?  Just something to think about for future cleanup.

[avallee, 2017/02/28 20:13:01]

My original iteration of the downward link did just this, but then nodes are 1
pointer larger each. Walking down focused frame tree nodes, we first look at the
WebContents anyways and then its node_.

       static_cast<RenderFrameHostImpl*>(outer_contents_frame));
 
   DCHECK(outer_contents_frame);
 
   // Create a proxy in top-level RenderFrameHostManager, pointing to the
   // SiteInstance of the outer WebContents. The proxy will be used to send
   // postMessage to the inner WebContents.
   render_manager->CreateOuterDelegateProxy(
       outer_contents_frame->GetSiteInstance(),
       static_cast<RenderFrameHostImpl*>(outer_contents_frame));
 
   render_manager->SetRWHViewForInnerContents(
       render_manager->GetRenderWidgetHostView());
 
   static_cast<RenderWidgetHostViewChildFrame*>(
       render_manager->GetRenderWidgetHostView())
       ->RegisterFrameSinkId();
 
+  // Set up the the guest's AX tree to point back at the embedder's AX tree.
+  auto* parent_frame = outer_contents_frame->GetParent();
+  GetMainFrame()->set_browser_plugin_embedder_ax_tree_id(
+      parent_frame->GetAXTreeID());
+  GetMainFrame()->UpdateAXTreeData();
+
   // At this point, we should destroy the TextInputManager which will notify all
   // the RWHV in this WebContents. The RWHV in this WebContents should use the
   // TextInputManager owned by the outer WebContents.
   // TODO(ekaramad): Is it possible to have TextInputState before attaching to
   // outer WebContents? In such a case, is this still the right way to hand off
   // state tracking from inner WebContents's TextInputManager to that of the
   // outer WebContent (crbug.com/609846)?
   text_input_manager_.reset(nullptr);
 }
 
@@ skipping 2850 matching lines @@
   return false;
 }
 
 WebContentsImpl* WebContentsImpl::GetOutermostWebContents() {
   WebContentsImpl* root = this;
   while (root->GetOuterWebContents())
     root = root->GetOuterWebContents();
   return root;
 }
 
+void WebContentsImpl::FocusOuterAttachmentFrameChain() {
+  WebContentsImpl* outer_contents = GetOuterWebContents();
+  if (!outer_contents)
+    return;
+
+  FrameTreeNode* outer_node =
+      FrameTreeNode::GloballyFindByID(GetOuterDelegateFrameTreeNodeId());
+  outer_contents->frame_tree_.SetFocusedFrame(outer_node, nullptr);

[alexmos, 2017/02/23 20:07:04]

Can you please explain why this is now necessary?  I don't see how it's related
to the other changes.

Another issue is that this can be called from WCI::SetFocusedFrame, and I'm
concerned that we'd be losing the source SiteInstance that's passed there if we
just use nullptr here.

[avallee, 2017/02/28 20:13:01]

Maybe the site instance still needs to be passed along, I'm not quite sure. Note
that this is FrameTree::SetFocusedFrame, not WebContentsImpl.

For accessibility, each parent needs to correctly show the container frame tree
node of the inner contents as focused so that the
BrowserAccessibility/BrowserAccessibilityManager can walk down and determine
which one is focused.

[alexmos, 2017/03/03 20:07:57]

Acknowledged.  

Thinking about the source SiteInstance some more, it should matter only within
the same FrameTree, as it's used to prevent notifying a process that already
knows about the focus change.  If we're moving to an outer WebContents, that
implies a process change, so passing |source| shouldn't be necessary (as it
shouldn't match any of the SiteInstances in the outer WC's FrameTree).

+  if (GetRenderManager()->GetProxyToOuterDelegate())
+    GetRenderManager()->GetProxyToOuterDelegate()->SetFocusedFrame();
+
+  outer_contents->FocusOuterAttachmentFrameChain();
+}
+
 void WebContentsImpl::RenderViewCreated(RenderViewHost* render_view_host) {
   // Don't send notifications if we are just creating a swapped-out RVH for
   // the opener chain.  These won't be used for view-source or WebUI, so it's
   // ok to return early.
   if (!static_cast<RenderViewHostImpl*>(render_view_host)->is_active())
     return;
 
   if (delegate_)
     view_->SetOverscrollControllerEnabled(CanOverscrollContent());
 
@@ skipping 384 matching lines @@
     return;
 
   // Send a page level blur to the old contents so that it displays inactive UI
   // and focus this contents to activate it.
   if (old_contents)
     old_contents->GetMainFrame()->GetRenderWidgetHost()->SetPageFocus(false);
 
   // Make sure the outer web contents knows our frame is focused. Otherwise, the
   // outer renderer could have the element before or after the frame element
   // focused which would return early without actually advancing focus.
-  if (GetRenderManager()->GetProxyToOuterDelegate())
-    GetRenderManager()->GetProxyToOuterDelegate()->SetFocusedFrame();
+  FocusOuterAttachmentFrameChain();
 
   GetMainFrame()->GetRenderWidgetHost()->SetPageFocus(true);
   GetOutermostWebContents()->node_->SetFocusedWebContents(this);
 }
 
 void WebContentsImpl::SetFocusedFrame(FrameTreeNode* node,
                                       SiteInstance* source) {
   // The PDF plugin still runs as a BrowserPlugin and must go through the
   // input redirection mechanism. It must not become focused direcly.
   if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) {
     frame_tree_.SetFocusedFrame(node, source);
     return;
   }
 
-  SetAsFocusedWebContentsIfNecessary();
+  frame_tree_.SetFocusedFrame(node, source);
 
-  frame_tree_.SetFocusedFrame(node, source);
+  WebContentsImpl* inner_contents = nullptr;
+  if (node_)
+    inner_contents = node_->find_contents_at_node(node->frame_tree_node_id());
+
+  WebContentsImpl* contents_to_focus = inner_contents ? inner_contents : this;

[alexmos, 2017/02/23 20:07:04]

I'm a bit confused about this change also.  See my earlier question about the
"SetFocusedFrame(outer_node, nullptr)" above, which I'm guessing is how you
would get here for the case with a valid inner_contents?  Why does this become
necessary?  

Suppose I click on a <webview>.  As part of WCI::SetFocusedFrame in the inner
contents, we call SetAsFocusedWebContentsIfNecessary on the inner contents. 
That now calls the new FocusOuterAttachmentFrameChain, which calls
SetFocusedFrame(outer_node, nullptr), which ends up here and calls
SetAsFocusedWebContentsIfNecessary on inner_contents again, which seems
unnecessary?

[avallee, 2017/02/28 20:13:00]

If you clicked the webview before routing the input there would be a call to
WebContentsImpl::FocusOwningWebContents which would focus the inner contents and
then the frame in the outer contents, but that wouldn't call back down here. If
the outer WebContents decided to focus the frame with an inner contents, then we
need to tell the inner contents that it is now active which is why we move
downwards like this.

[alexmos, 2017/03/03 20:07:57]

Ack.  I was confused that FocusOuterAttachmentFrameChain would call
WebContentsImpl::SetFocusedFrame on the outer contents, but it actually calls
the FrameTree version, so it doesn't call back down here.

+  contents_to_focus->SetAsFocusedWebContentsIfNecessary();
+}
+
+RenderFrameHost* WebContentsImpl::GetFocusedFrameIncludingInnerWebContents() {
+  if (!node_)
+    return GetFocusedFrame();
+
+  auto* contents = this;
+  auto* focused_node = contents->frame_tree_.GetFocusedFrame();

[alexmos, 2017/02/23 20:07:04]

nit: at least for me, an explicit type here instead of auto would be more
readable, so it's obvious that this is a FrameTreeNode* (and different from
RFHI* that the other GetFocusedFrame() returns above).

[avallee, 2017/02/28 20:13:00]

Done.

+
+  // If there is no focused frame in the outer WebContents, we need to return
+  // null.
+  if (!focused_node)
+    return nullptr;
+
+  // We cannot return the frame where an inner WebContents is attached, if the
+  // inner WebContents does not have a focused frame, return its main frame.
+  while (true) {
+    contents = contents->node_->find_contents_at_node(
+        focused_node->frame_tree_node_id());

[alexmos, 2017/02/23 20:07:04]

Why not just pass in the FTN* and retrieve the FTN ID internally in that helper?

[avallee, 2017/02/28 20:13:01]

Done.

+    if (!contents)
+      return focused_node->current_frame_host();
+
+    focused_node = contents->frame_tree_.GetFocusedFrame();
+    if (!focused_node)
+      return contents->GetMainFrame();

[alexmos, 2017/02/23 20:07:04]

What makes this necessary?  How can we get here in practice?

[avallee, 2017/02/28 20:13:00]

On attach or after reaching the end of content, there is no focused frame.

Should I log a warning here if you think this is unexpected?

[alexmos, 2017/03/03 20:07:57]

Not quite sure what you meant by "after reaching the end of content".  Does AX
call GetFocusedFrameIncludingInnerWebContents on attach when there's no focused
frame in the inner contents yet, but the inner contents is already focused in
the outer contents?  It'd be nice to add rationale for returning the main frame
in the comment above the while().

This is probably fine, and I'm not that surprised about returning the main frame
(knowing how widely Blink's focusedOrMainFrame() is used), but just want to
understand why it's needed here.  It seems a bit weird that if there's no
focused frame, we return null for outer WC and the main frame for inner WC -
would AX break if you returned null here instead?

[avallee, 2017/06/21 02:25:35]

I think this will get called on attach, but as a result of
WebContentsImpl::SetFocusedFrame so there should be one focused already.
However, when you tab (shift-tab) with the last (first) focusable element in the
document, the FocusController will clear the focused frame.
As I understand it, it seems that it wants some frame to be focused overall but
in a subtree it's fine that there's no focused frame. (See
RenderFrameHostImpl::AXContentTreeDataToAXTreeData)

+  }
 }
 
 void WebContentsImpl::OnFocusedElementChangedInFrame(
     RenderFrameHostImpl* frame,
     const gfx::Rect& bounds_in_root_view) {
   RenderWidgetHostViewBase* root_view =
       static_cast<RenderWidgetHostViewBase*>(GetRenderWidgetHostView());
   if (!root_view || !frame->GetView())
     return;
 
@@ skipping 664 matching lines @@
     RenderViewHost* render_view_host = RenderViewHost::From(render_widget_host);
     if (!render_view_host)
       continue;
     render_view_host_set.insert(render_view_host);
   }
   for (RenderViewHost* render_view_host : render_view_host_set)
     render_view_host->OnWebkitPreferencesChanged();
 }
 
 }  // namespace content