<webview>: Add test to confirm the TAB key will escape the view.

third_party/WebKit/Source/core/page/FocusController.cpp

Index: third_party/WebKit/Source/core/page/FocusController.cpp
diff --git a/third_party/WebKit/Source/core/page/FocusController.cpp b/third_party/WebKit/Source/core/page/FocusController.cpp
index 110d8bf12a8de8c3fa4cf6a7c0088d3b2008b582..c386c9b495af9c8b7811ef9be7e0c5c449acd7af 100644
--- a/third_party/WebKit/Source/core/page/FocusController.cpp
+++ b/third_party/WebKit/Source/core/page/FocusController.cpp
@@ -783,8 +783,20 @@ void FocusController::focusDocumentView(Frame* frame, bool notifyEmbedder) {
   if (focusedFrame && focusedFrame->view()) {
     Document* document = focusedFrame->document();
     Element* focusedElement = document ? document->focusedElement() : nullptr;
-    if (focusedElement)
-      dispatchBlurEvent(*document, *focusedElement);
+    if (focusedElement) {
+      bool focusedFrameIsAncestor = false;

[alexmos, 2017/03/16 17:37:11]

I think this is really newFocusedFrameIsDescendant, but it doesn't matter -- see
below.

+      for (Frame* newFrame = frame; newFrame;
+           newFrame = newFrame->client()->parent()) {

[alexmos, 2017/03/10 07:08:33]

Why do we need to walk up the ancestor chain?  Is this for the case that
intermediate frames don't have any focusable elements?

[avallee, 2017/03/13 19:18:10]

I'm not sure how exiting a frame works. In the top > iframe > webview structure,
I'm not sure if there were no focusable elements in the iframe if the return to
top from webview is done in 1 or 2 steps. If it's done in 2 steps with focus
momentarily passing through the iframe, then this should not be needed. If
however, the focus passes straight through, it's possible that the top frame now
tries to advance and has an element focused.

[alexmos, 2017/03/16 17:37:11]

I was really confused here and what this has to do with exiting a frame, but I
think I understand things better now after following some code around and
patching in your CL.  I mostly thought about this with just OOPIFs and no
webviews, since that's easier and the fix is the same for both.

So, the intent of this code is really to clear the focused element when we learn
that a remote frame has become focused.  I.e., in the bug repro, we have:
  <iframe A>
    <iframe B>
      <input 2>
    </iframe>
    <input 1>  <-- focused initially
  </iframe>

The <input 1> is focused initially, so frame A is the focused frame. You then
click on <input 2>.  At that point, the B renderer makes frame B the focused
frame and forwards the focused frame change to renderer A, which will end up
calling focusDocumentView() on the RemoteFrame for B.  At this point, you get to
this code and want to clear focused element (<input>).

A few concerns about this approach:

1.  I'm not convinced this should be gated on the ancestor walk.  Consider this
example: 

  <iframe A>
    <input 1>  <-- click this second
    <iframe B>
      <input 2> <-- click this first
    </iframe>
  </iframe>

  Then press tab.  Wouldn't that have the same problem unless you clear the
focused element (<input 2>) in frame B?  The current code won't do it, since
it'll get here in renderer B, with A being the new focused frame, i.e., B will
be the descendant not an ancestor of the new focused frame.

  In practice, I tried this out on
http://csreis.github.io/tests/cross-site-iframe-simple.html with
--site-per-process:
    a. in devtools,
document.querySelector("h1").appendChild(document.createElement("input"))
    b. click first <input> in iframe
    c. click the main frame's <input> created in a)
    d. press tab
  Focus should've switch to the first <input> in the iframe, but instead it
shifts to the *second* input there.  I don't quite understand why it does that,
and whether that's due to not calling clearFocusedElement() in B when we clicked
on the <input 1> in A, or a different issue.  Thoughts?

(It'd be good to add a test for this case as well.)

2.  I was thinking if we should instead just call clearFocusedElement when
|frame| is a remote frame.  That should fix 1 and any similar cases (sibling
iframes?).  But I can't convince myself that it's safe.

Thinking more carefully about how this works without any OOPIFs today, it seems
that normally clearFocusedElement is called as part of
FocusController::setFocusedElement if the old focused element was in a different
frame, as part of mouse click dispatch (EventHandler::handleMousePressEvent ->
MouseEventManager::handleMouseFocus -> FocusController::setFocusedElement ->
Document::clearFocusedElement).  However, setFocusedElement is not called
unconditionally in handleMouseFocus; there are a few
WebInputEventResult::NotHandled early returns that I don't fully understand
(e.g., for clicking on scrollbar, some drag stuff).  

What I'm wondering is whether it's possible to click on a frame and focus that
frame, but follow one of those early returns and not focus any elements in that
frame (which would leave the focused element intact in the old focused frame). 
If not, maybe we can get away with clearFocusedElement when |frame| is a remote
frame.  If yes, we may need to send a separate IPC to clear the focused element
in the old focused frame only when needed.  Maybe you could try seeing if any of
those early returns matter in practice?  

FWIW, although usually FocusController::setFocusedFrame is called as part of
FocusController::setFocusedElement, it's also called again via
MouseEventManager::handleMousePressEvent -> MouseEventManager::focusDocumentView
-> FocusController::focusDocumentView -> setFocusedFrame (which is usually a
noop), but could matter in case we end up not calling setFocusedElement.

Regarding your question on how tab traversal exits a frame, I think it's a
one-step process.  If you clicked on a remote grandchild frame nested in a
middle frame with no focusable elements, and then pressing tab to get out to the
root frame,  advanceFocusInDocumentOrder and
findFocusableElementAcrossFocusScopes will look for elements to focus in all
possible local frames, including ancestor frames if they exist, in one step
before handing the search over to the remote parent of the local root.

[alexmos, 2017/03/16 17:59:47]

I'll also note that at some point, when replicating the new focused frame  I
tried using setFocusedElement(nullptr, newFocusedFrame) instead of
focusDocumentView, which would've avoided this bug since it clears the old
focused element.  I can't remember all the details, but I think I ran into
issues with that, possibly because of window.focus called on remote frames?  See
https://codereview.chromium.org/1423053002/#msg4 and PS1->PS2.  (Just mentioning
this as that's another alternative for a fix.)

+        if (newFrame == m_focusedFrame.get()) {
+          focusedFrameIsAncestor = true;
+          break;
+        }
+      }
+      if (focusedFrameIsAncestor)

[alexmos, 2017/03/10 07:08:33]

What about the reverse case, where an <input> that has focus is before the
OOPIF, then the OOPIF is clicked, and then you shift-tab upwards?  Does that
case also work?

[avallee, 2017/03/13 19:18:10]

This was doing the wrong thing when the browser notified you that the guest has
become focused. Tab/Shift-tab would be sent to a renderer who might decide to
initiate a focus change into a remote parent or child frame. When this renderer
initiate the focus change, it will be in a consistent state.

[alexmos, 2017/03/16 17:37:11]

I'm not sure I followed your explanation, but I think the case I asked about
works. :)  It doesn't matter where the previously focused element is, we'll
clear it regardless when we click on a descendant of a currently focused frame,
whether it's an OOPIF or webview.  So whether we get to the element via tab or
shift-tab later doesn't matter.

+        document->clearFocusedElement();

[alexmos, 2017/03/10 07:08:33]

Will |frame| always be a RemoteFrame when this is needed?  Also, this could use
a comment with an explanation.

[avallee, 2017/03/13 19:18:10]

I think the issue would only happen with remote frames. On a local frame, the
change happens within the renderer and things are set correctly at that point.

Here we were reacting to a remote renderer changing focus.

[alexmos, 2017/03/16 17:37:11]

See the big comment above regarding this.

+      else
+        dispatchBlurEvent(*document, *focusedElement);

[alexmos, 2017/03/10 07:08:33]

Just to check, does clearFocusedElement also dispatch this blur event?

[avallee, 2017/03/13 19:18:10]

Yes, it calls setFocusedElement(nullptr, /*some other stuff*/) and then
dispatched on the oldFocusedElement:
http://google3/third_party/chromium_headless/blink/Source/core/dom/Document.c...

[alexmos, 2017/03/16 17:37:11]

Acknowledged.

+    }
   }
 
   LocalFrame* newFocusedFrame =