<webview>: Correctly shift focus between WebContents.

content/browser/renderer_host/render_widget_host_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/renderer_host/render_widget_host_impl.h"
 
 #include <math.h>
 
 #include <set>
 #include <tuple>
@@ skipping 674 matching lines @@
 
 void RenderWidgetHostImpl::GotFocus() {
   Focus();
   if (owner_delegate_)
     owner_delegate_->RenderWidgetGotFocus();
   if (delegate_)
     delegate_->RenderWidgetGotFocus(this);
 }
 
 void RenderWidgetHostImpl::Focus() {
+  RenderWidgetHostImpl* focused_widget =
+      delegate_ ? delegate_->GetFocusedRenderWidgetHost(this) : nullptr;

[alexmos, 2016/10/28 06:36:39]

I think in the case where GetFocusedRenderWidgetHost returns null, we're
supposed to drop the event to avoid giving it to the wrong frame (see comments
inside that function).  Can you check how this works if the guest crashes while
having page focus, and then you get into here?

[avallee, 2016/10/28 19:19:56]

The main place where Focus() is called is from the RenderWidgetHostView which
only knows about the top level RenderWidget. If an internal RenderWidget dies, I
think it's fine to refocus the outermost RenderWidget when we refocus the
window.

[alexmos, 2016/10/31 18:57:57]

It might not always be safe: a malicious embedder could load some  sensitive
site in a <webview>, then crash it just as you start typing sensitive data
inside the <webview>.  The embedder might then start getting the rest of the
data if we focus it, until user realizes that the keystrokes are going to the
wrong place?  See
https://codereview.chromium.org/1505723005/diff/1/content/browser/web_content...
for a discussion when this came up for OOPIFs.

[avallee, 2016/11/16 05:38:09]

Would crashing the renderer lead a focus change? Currently, the embedder can
just call element.focus() and steal from the webview anyways.

[alexmos, 2016/11/16 23:29:02]

I'm not sure -- that's why I was curious if you could try it. :)  But given your
latter point, I think the current code is ok.

+
+  if (focused_widget)
+    focused_widget->FocusDirect();

[alexmos, 2016/10/28 06:36:39]

I'm trying to understand the implication of this change for regular OOPIF cases.
 Say we have a page with frame A embedding frame B, and B is an OOPIF.  If we
focus frame A and then alt-tab back and forth, this will behave as before: send
InputMsg_SetFocus to the main frame RWH as well as to main frame's proxy in
process B (thanks to ReplicatePageFocus).  So both renderers get the page focus
messages.

If we now focus frame B and then alt-tab back and forth, this seems to behave
differently: GetFocusedRenderWidgetHost will return B's RWH, hence we will send
the InputMsg_SetFocus to B's RWH, and because RenderViewHost::From(this) is
false for that RWH, we don't replicate the page focus message to A.  

That will also cause the is_focused_ bit to become inconsistent for both main
frame and subframe's RWHs, though I'm not sure what that entails.

That doesn't seem consistent/desirable.  Is the different behavior intentional? 
Was this meant to only apply to the RWH for embedders/guests, or to subframes as
well?

[avallee, 2016/10/28 19:19:56]

I believe that GetFocusedWebContents()->GetMainFrame() is the right thing to do
here.

[alexmos, 2016/10/31 18:57:57]

Yes, that looks better.  However, are we sure that this can't ever be called on
a non-main-frame widget?  At first glance in codesearch, most callsites get here
via smth like GetRenderViewHost()->GetWidget()->Focus(), which implies main
frame widget, but I didn't check all the paths.  I'm especially wondering if
this is ever called for fullscreen RWHs (especially Flash fullscreen) and/or
popup menu widgets (i.e., <select> dropdowns).  Do you mind double-checking
those cases?  See https://crbug.com/593522 for some examples of Flash
fullscreen.  I'm curious if we need to keep the old behavior of just sending
InputMsg_SetFocus to the RWH itself for those cases.

[alexmos, 2016/11/09 02:47:54]

Still curious about this.  Might be worth trying putting a DCHECK here and in
Blur(), to see if we ever have a focused_widget != this, yet they point to the
same delegates.  I'm curious if this breaks any tests.

[avallee, 2016/11/16 05:38:09]

This DCHECK would fail if a webview is focused. Once the webview is supposed to
get keyboard input, if we blur or focus the embedder's window we would come
here, this == embedder, focused == webview.

As for flash and select, I messed around in a webview and they seem to work.
Losing window focus closes the full screen flash and select widget. Coming back
leave the flash or select element as the focused element on the page.

[alexmos, 2016/11/16 23:29:02]

Sorry, the DCHECK I had in mind was assuming a single WebContents.  I.e.,
something like DCHECK(focused_widget == this || focused_widget->delegate() !=
delegate_).  This would check that if we can't get here for a non-main-frame
widget in a single WebContents (because otherwise, we'd change behavior and send
the IPC to a different widget).

+  else
+    FocusDirect();
+}
+
+void RenderWidgetHostImpl::Blur() {
+  RenderWidgetHostImpl* focused_widget =
+      delegate_ ? delegate_->GetFocusedRenderWidgetHost(this) : nullptr;
+
+  if (focused_widget)
+    focused_widget->BlurDirect();
+  else
+    BlurDirect();
+}
+
+void RenderWidgetHostImpl::FocusDirect() {

[alexmos, 2016/10/28 06:36:39]

I find these names a bit hard to interpret.  Since this is just setting page
focus, wdyt about calling it SetPageFocus()/ClearPageFocus()?  (You could even
go one step further and merge the two nearly identical functions and take a bool
instead)

[avallee, 2016/10/28 19:19:56]

Done.

   is_focused_ = true;
 
   Send(new InputMsg_SetFocus(routing_id_, true));
 
   // Also send page-level focus state to other SiteInstances involved in
   // rendering the current FrameTree.
   if (RenderViewHost::From(this) && delegate_)
     delegate_->ReplicatePageFocus(true);
 }
 
-void RenderWidgetHostImpl::Blur() {
+void RenderWidgetHostImpl::BlurDirect() {
   is_focused_ = false;
 
   // If there is a pending mouse lock request, we don't want to reject it at
   // this point. The user can switch focus back to this view and approve the
   // request later.
   if (IsMouseLocked())
     view_->UnlockMouse();
 
   if (touch_emulator_)
     touch_emulator_->CancelTouch();
@@ skipping 1163 matching lines @@
     const blink::WebInputEvent& event, const ui::LatencyInfo& latency_info) {
   // Don't ignore touch cancel events, since they may be sent while input
   // events are being ignored in order to keep the renderer from getting
   // confused about how many touches are active.
   if (ShouldDropInputEvents() && event.type != WebInputEvent::TouchCancel)
     return INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS;
 
   if (!process_->HasConnection())
     return INPUT_EVENT_ACK_STATE_UNKNOWN;
 
-  if (delegate_ && (event.type == WebInputEvent::MouseDown ||
-                    event.type == WebInputEvent::GestureScrollBegin ||
-                    event.type == WebInputEvent::TouchStart ||
-                    event.type == WebInputEvent::RawKeyDown)) {
-    delegate_->OnUserInteraction(this, event.type);
+  if (delegate_) {
+    if (event.type == WebInputEvent::MouseDown ||
+        event.type == WebInputEvent::TouchStart) {
+      delegate_->EnsureOwningContentsIsFocused(this);
+    }
+    if (event.type == WebInputEvent::MouseDown ||
+        event.type == WebInputEvent::GestureScrollBegin ||
+        event.type == WebInputEvent::TouchStart ||
+        event.type == WebInputEvent::RawKeyDown) {
+      delegate_->OnUserInteraction(this, event.type);
+    }
   }
 
   return view_ ? view_->FilterInputEvent(event)
                : INPUT_EVENT_ACK_STATE_NOT_CONSUMED;
 }
 
 void RenderWidgetHostImpl::IncrementInFlightEventCount(
     blink::WebInputEvent::Type event_type) {
   increment_in_flight_event_count();
   if (!is_hidden_) {
@@ skipping 318 matching lines @@
   return delegate_ ? delegate_->GetRootBrowserAccessibilityManager() : NULL;
 }
 
 BrowserAccessibilityManager*
     RenderWidgetHostImpl::GetOrCreateRootBrowserAccessibilityManager() {
   return delegate_ ?
       delegate_->GetOrCreateRootBrowserAccessibilityManager() : NULL;
 }
 
 }  // namespace content