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 765 matching lines @@
   DCHECK(!frame || frame->page() == m_page);
   if (m_focusedFrame == frame)
     return;
 
   LocalFrame* focusedFrame = (m_focusedFrame && m_focusedFrame->isLocalFrame())
                                  ? toLocalFrame(m_focusedFrame.get())
                                  : nullptr;
   if (focusedFrame && focusedFrame->view()) {
     Document* document = focusedFrame->document();
     Element* focusedElement = document ? document->focusedElement() : nullptr;
-    if (focusedElement)
-      dispatchBlurEvent(*document, *focusedElement);
+    if (focusedElement) {

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

I think this needs a comment to explain what's going on.  

For Daniel's reference, we've discussed this one on
https://codereview.chromium.org/2732913005/.  I think I also had some questions
there about whether this is safe, e.g. I'm still curious whether it's possible
to somehow focus a frame but not focus any elements in that frame.  That would
leave the focused element intact in the old focused frame and break the
assumption that this makes.

[avallee, 2017/04/02 02:27:30]

I don't believe that focusing a frame requires a focused element. OTOH: Focusing
a widget will focus its main frame if none is focused.

Also there is a chain of WebContent -> RenderFrameHost -> RenderFrame ->
Document that all provide clearFocusedElement.

+      if (frame && frame->isRemoteFrame())

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

Were you referring to this in the description, saying that if you did this
always and not just for remote frames, that would break
fast/frames/frame-focus-send-blur.html and 
fast/events/frame-click-clear-focus.html?  These two tests currently use data
URLs in frames, which stay in-process, but does this mean that if the frames
were OOPIFs instead, that this change would be breaking them?

[avallee, 2017/04/02 02:27:30]

I am not sure. They are secondary assertions that have a comment that the result
is not important (probably only there for synchronization). The test itself is
strange, calling focus() in another iframe does not clear the focused element
while clicking the other iframe does. If the test is changed, this code could be
simplified and the test would seem more sane.

The issue in my test was that in oopif, we were seeing the previously focused
element in an iframe have a focus event dispatched to it as part of focusing the
frame when tabbing into that frame, whether that element was the correct one to
select. Then the appropriate element would get focused.

I've tried an alternate implementation but some different tests fail:
https://codereview.chromium.org/2796533002/. 

+        document->clearFocusedElement();
+      else
+        dispatchBlurEvent(*document, *focusedElement);
+    }
   }
 
   LocalFrame* newFocusedFrame =
       (frame && frame->isLocalFrame()) ? toLocalFrame(frame) : nullptr;
   if (newFocusedFrame && newFocusedFrame->view()) {
     Document* document = newFocusedFrame->document();
     Element* focusedElement = document ? document->focusedElement() : nullptr;
     if (focusedElement)
       dispatchFocusEvent(*document, *focusedElement);
   }
@@ skipping 129 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 or sibling frame, we need to restart from the
+  // first or last focusable element.

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

Why did the old code fail to do this?  It seems in this case, |start| would be
null, so in advanceFocusInDocumentOrder, |current| would start out as null, and
this will not execute anymore due to |initialFocus| being true:

  if (!current && !initialFocus)
    current = document->sequentialFocusNavigationStartingPoint(type);

Is the issue that sequentialFocusNavigationStartingPoint will return
m_focusedElement if it's not null?  How will this determine the right starting
point now, via ScopedFocusNavigation::createForDocument(*document)?

[avallee, 2017/04/02 02:27:30]

|start| is null but sequentialFocusNavigationStartingPoint starts from the
currently focused element, otherwise the m_sequentialFocusStartingPoint [1].

We don't want that, instjead we want the starting point to be the initial focus
point, so we want to skip this check and then
ScopedFocusNavigation::createForDocument(Document*) instead of the
ScopedFocusNavigation::createFor(Node*).

The other problem is the extra event dispatching. In non-oopif, we will get a
single focus event on the new focused element. With the current oopif
implementation, the renderer that processes the advance focus will (below around
line 1024) setFocus on the remote frame and then advance focus. This has the
effect of refocusing the previous element

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum...

+  bool initialFocus = to->tree().parent() == from ||
+                      to->tree().parent() == from->tree().parent();

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

Hmm, can we actually get here for a sibling frame?  We call RF::advanceFocus in
two places, one parent-to-child [1] and the other child-to-parent [2].  It
doesn't seem like we could ever go directly from one sibling to another, unless
there's now another path to get here other than through advanceFocus.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu...

[2]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu...

+
+  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 446 matching lines @@
     it->focusedFrameChanged();
 }
 
 DEFINE_TRACE(FocusController) {
   visitor->trace(m_page);
   visitor->trace(m_focusedFrame);
   visitor->trace(m_focusChangedObservers);
 }
 
 }  // namespace blink