Chromium Code Reviews Chromium Code Reviews
Issue 2451143003:
<webview>: Correctly shift focus between WebContents. (Closed)
| 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 ae0a6ecea2d81617526e2ec85bd4aa8e25c63d73..3542223ebd1cdb1ee778033e474b7196b1e1f394 100644 |
| --- a/content/browser/web_contents/web_contents_impl.cc |
| +++ b/content/browser/web_contents/web_contents_impl.cc |
| @@ -4626,29 +4626,62 @@ void WebContentsImpl::EnsureOpenerProxiesExist(RenderFrameHost* source_rfh) { |
| } |
| } |
| +void WebContentsImpl::ChangeFocus(WebContentsImpl* old_contents) { |
| + // Focus is moving between frame trees. |
| + // |
| + // Ensure that the embedding frame in each outer WebContent's frame tree is |
| + // focused, if present. |
|
alexmos
2016/10/28 06:36:39
Can you please help me understand why we need the
avallee
2016/10/28 19:19:57
We want to let the embedding renderer(s) know that
alexmos
2016/10/31 18:57:57
Sorry, I'm still a bit fuzzy on this. So looking
avallee
2016/11/07 19:18:44
Now that I look at it, it seems to work. I don't r
|
| + // |
| + // Send a page level blur to the old contents so that it displays inactive UI |
| + // and focus this contents to activate it. |
|
alexmos
2016/10/28 06:36:39
These two lines of the comment seem more appropria
avallee
2016/10/28 19:19:57
Done.
|
| + WebContentsImpl* web_contents = this; |
| + while (web_contents->GetOuterWebContents()) { |
| + FrameTreeNode* tree_node = FrameTreeNode::GloballyFindByID( |
| + web_contents->GetOuterDelegateFrameTreeNodeId()); |
| + RenderFrameProxyHost* outer_proxy = web_contents->frame_tree_.root() |
| + ->render_manager() |
| + ->GetProxyToOuterDelegate(); |
| + if (!tree_node) { |
|
lfg
2016/10/27 20:00:35
It took me some time to understand that this can o
avallee
2016/10/28 19:19:57
Done.
My main worry: <body><webview></webview><if
|
| + tree_node = web_contents->GetOuterWebContents() |
| + ->GetMainFrame() |
|
lfg
2016/10/27 20:00:35
+ekaramad@ FYI. This will probably need to be fixe
avallee
2016/11/07 19:18:44
+lfg, +ekaramad@ deleted...
EhsanK
2016/11/08 16:05:47
Thanks! It probably should. I think webcontents->G
|
| + ->frame_tree_node(); |
| + outer_proxy = web_contents->frame_tree_.root() |
|
lfg
2016/10/27 20:00:35
Why do we want to call Focus on this proxy? (assum
avallee
2016/10/28 19:19:56
Doesn't make sense. Deleted.
|
| + ->render_manager() |
| + ->GetRenderFrameProxyHost( |
| + tree_node->current_frame_host()->GetSiteInstance()); |
| + } |
| + |
| + web_contents = web_contents->GetOuterWebContents(); |
| + if (tree_node == web_contents->frame_tree_.GetFocusedFrame()) |
| + break; |
| + if (outer_proxy) { |
| + web_contents->frame_tree_.SetFocusedFrame(tree_node, |
| + outer_proxy->GetSiteInstance()); |
| + outer_proxy->SetFocusedFrame(); |
| + } |
| + } |
| + |
| + // Since we are changing focus, we don't want to call Focus()/Blur() which |
| + // would indirect back into WebContentsImpl(::GetFocusedRenderWidgetHost). |
| + if (old_contents) |
| + old_contents->GetMainFrame()->GetRenderWidgetHost()->BlurDirect(); |
| + GetMainFrame()->GetRenderWidgetHost()->FocusDirect(); |
| + GetOutermostWebContents()->node_->SetFocusedWebContents(this); |
| +} |
| + |
| void WebContentsImpl::SetFocusedFrame(FrameTreeNode* node, |
| SiteInstance* source) { |
| if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) { |
| frame_tree_.SetFocusedFrame(node, source); |
| return; |
| } |
| - |
| - // 1. Find old focused frame and unfocus it. |
| - // 2. Focus the new frame in the current FrameTree. |
| - // 3. Set current WebContents as focused. |
| WebContentsImpl* old_focused_contents = GetFocusedWebContents(); |
| if (old_focused_contents != this) { |
| // Focus is moving between frame trees, unfocus the frame in the old tree. |
|
alexmos
2016/10/28 06:36:39
Comment looks stale (ChangeFocus tinkers with page
avallee
2016/10/28 19:19:57
Done.
|
| - old_focused_contents->frame_tree_.SetFocusedFrame(nullptr, source); |
|
alexmos
2016/10/28 06:36:39
To sanity check my understanding, previously, old_
avallee
2016/10/28 19:19:56
Yes, we used to clear focus in order to have the r
|
| - GetOutermostWebContents()->node_->SetFocusedWebContents(this); |
| + ChangeFocus(old_focused_contents); |
| } |
| frame_tree_.SetFocusedFrame(node, source); |
| - |
| - // TODO(avallee): Remove this once page focus is fixed. |
| - RenderWidgetHostImpl* rwh = node->current_frame_host()->GetRenderWidgetHost(); |
| - if (rwh && old_focused_contents != this) |
| - rwh->Focus(); |
| } |
| bool WebContentsImpl::AddMessageToConsole(int32_t level, |
| @@ -4678,6 +4711,20 @@ void WebContentsImpl::OnUserInteraction( |
| rdh->OnUserGesture(); |
| } |
| +void WebContentsImpl::EnsureOwningContentsIsFocused( |
| + RenderWidgetHostImpl* render_widget_host) { |
| + if (!GuestMode::IsCrossProcessFrameGuest(this) && browser_plugin_guest_) |
| + return; |
| + |
| + RenderWidgetHostImpl* focused_widget = |
| + GetFocusedRenderWidgetHost(render_widget_host); |
| + |
| + if (focused_widget != render_widget_host && |
| + focused_widget->delegate() != render_widget_host->delegate()) { |
| + ChangeFocus(static_cast<WebContentsImpl*>(focused_widget->delegate())); |
|
lfg
2016/10/27 20:00:35
I'm not sure this cast is safe, a quick search tel
avallee
2016/10/28 19:19:56
Moved the check into ChangeFocusIfNeeded();
The c
|
| + } |
| +} |
| + |
| void WebContentsImpl::OnIgnoredUIEvent() { |
| // Notify observers. |
| for (auto& observer : observers_) |
