OOPIF: Enable TabAndMouseFocusNavigation.

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

 /*
  * Copyright (C) 2006, 2007 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nuanti Ltd.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 915 matching lines @@
   // If we are shifting focus from a child frame to its parent, the
   // child frame has no more focusable elements, and we should continue
   // looking for focusable elements in the parent, starting from the <iframe>
   // element of the child frame.
   Element* start = nullptr;
   if (from->tree().parent() == to) {
     DCHECK(from->owner()->isLocal());
     start = toHTMLFrameOwnerElement(from->owner());
   }
 
-  return advanceFocusInDocumentOrder(to, start, type, false,
+  // If we're coming from a parent frame, we need to restart from the first or
+  // last focusable element.
+  bool initialFocus = to->tree().parent() == from;
+
+  return advanceFocusInDocumentOrder(to, start, type, initialFocus,
                                      sourceCapabilities);
 }
 
 #if DCHECK_IS_ON()
 inline bool isNonFocusableShadowHost(const Element& element) {
   return isShadowHostWithoutCustomFocusLogic(element) && !element.isFocusable();
 }
 #endif
 
 bool FocusController::advanceFocusInDocumentOrder(
@@ skipping 45 matching lines @@
     element = findFocusableElementRecursively(type, scope);
     element =
         findFocusableElementDescendingDownIntoFrameDocument(type, element);
 
     if (!element)
       return false;
   }
 
   if (element == document->focusedElement()) {
     // Focus wrapped around to the same element.
+    setFocusedFrame(document->frame());
+    dispatchFocusEvent(*document, *element);

[alexmos, 2017/04/05 00:21:13]

Sanity check: could this have any side effects (like an extra focus event) in
non-OOPIF modes?  Doesn't seem likely, as ChromeClientImpl::canTakeFocus always
returns true, unless we're in layout test mode, so realistically I don't see how
it's possible to press tab from a focused element and wrap around to that same
focused element (since we'd return early after the canTakeFocus check above.)

[avallee, 2017/04/05 15:13:00]

It could mean an extra focus event, but I don't know how this would be
constructed. If there's a single element and we tab through, we'd ask the
embedder to TakeFocus(). This wrapping around doesn't seem like a problem. 
Maybe there's a way to listen for a tab event that would focus the next element
and then have the default handlers advance focus onto the same element?

This early out is exactly how we are getting trapped with the change in focused
frame in the case where we steal focus and the document doesn't clear the
focused element,

Also, there seems to be no test to verify that this case would not result in a
focus event. The cl passes dry run so I'm feeling decently confident that we're
fine or we'll break everything and end up with a test case.

[alexmos, 2017/04/05 18:08:33]

Acknowledged.

     return true;
   }
 
   if (element->isFrameOwnerElement() &&
       (!isHTMLPlugInElement(*element) || !element->isKeyboardFocusable())) {
     // We focus frames rather than frame owners.
     // FIXME: We should not focus frames that have no scrollbars, as focusing
     // them isn't useful to the user.
     HTMLFrameOwnerElement* owner = toHTMLFrameOwnerElement(element);
     if (!owner->contentFrame())
       return false;
 
     document->clearFocusedElement();
-    setFocusedFrame(owner->contentFrame());
 
     // If contentFrame is remote, continue the search for focusable
     // elements in that frame's process.

[alexmos, 2017/04/05 00:21:13]

Might be worth also mentioning that contentFrame will be set as the focused
frame from its own process.  

(is this right?  I think the call that would do this is here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu...)

[avallee, 2017/04/05 15:13:00]

Yes, it would either be set up above where your other comment is, or down below
near the final return.

Done.

     // clearFocusedElement() fires events that might detach the
     // contentFrame, hence the need to null-check it again.
     if (owner->contentFrame() && owner->contentFrame()->isRemoteFrame())
       toRemoteFrame(owner->contentFrame())->advanceFocus(type, frame);
+    else
+      setFocusedFrame(owner->contentFrame());
 
     return true;
   }
 
   DCHECK(element->isFocusable());
 
   // FIXME: It would be nice to just be able to call setFocusedElement(element)
   // here, but we can't do that because some elements (e.g. HTMLInputElement
   // and HTMLTextAreaElement) do extra work in their focus() methods.
   Document& newDocument = element->document();
@@ skipping 385 matching lines @@
     it->focusedFrameChanged();
 }
 
 DEFINE_TRACE(FocusController) {
   visitor->trace(m_page);
   visitor->trace(m_focusedFrame);
   visitor->trace(m_focusChangedObservers);
 }
 
 }  // namespace blink