Fix focus navigation getting stuck

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 89be696b17f9347030c1c3633e9cb95b47477fbe..4f61e24a34a8ee4ff9e6321e23e0afc71f92f811 100644
--- a/third_party/WebKit/Source/core/page/FocusController.cpp
+++ b/third_party/WebKit/Source/core/page/FocusController.cpp
@@ -558,13 +558,14 @@ inline Element* findFocusableElementInternal(WebFocusType type,
   Element* found = (type == WebFocusTypeForward)
                        ? nextFocusableElement(scope)
                        : previousFocusableElement(scope);
+  scope.setCurrentElement(found);

[hayato, 2016/12/13 05:41:57]

It does not look a good idea to call setCurrentElement() outside of
ScopedFocusNavigation.

I think this is not the fault of this CL. It looks FocusController already calls
setCurrentElement() from the outside of ScopedFocusNavigator.

However, ideally, we should not make setCurrentElement a public member function.
m_currentElement should be set only via ScopedFocusNavigator's other traversal
functions.

Could you try to make ScopedFocusNavigator::setCurrentElement() a private member
function? I guess this requires a kind of refactoring, but it would make a
responsibility much clearer and would become less error-prone.

[kochi, 2016/12/13 07:20:37]

Agreed.

   return found;
 }
 
 Element* findFocusableElementRecursivelyForward(ScopedFocusNavigation& scope) {
   // Starting element is exclusive.
-  Element* found = findFocusableElementInternal(WebFocusTypeForward, scope);
-  while (found) {
+  while (Element* found =
+             findFocusableElementInternal(WebFocusTypeForward, scope)) {
     if (isShadowHostDelegatesFocus(*found)) {
       // If tabindex is positive, find focusable element inside its shadow tree.
       if (found->tabIndex() >= 0 &&
@@ -576,7 +577,6 @@ Element* findFocusableElementRecursivelyForward(ScopedFocusNavigation& scope) {
           return foundInInnerFocusScope;
       }
       // Skip to the next element in the same scope.
-      found = findFocusableElementInternal(WebFocusTypeForward, scope);
       continue;
     }
     if (!isNonFocusableFocusScopeOwner(*found))
@@ -590,18 +590,14 @@ Element* findFocusableElementRecursivelyForward(ScopedFocusNavigation& scope) {
     if (Element* foundInInnerFocusScope =
             findFocusableElementRecursivelyForward(innerScope))
       return foundInInnerFocusScope;
-
-    scope.setCurrentElement(found);
-    found = findFocusableElementInternal(WebFocusTypeForward, scope);
   }
   return nullptr;
 }
 
 Element* findFocusableElementRecursivelyBackward(ScopedFocusNavigation& scope) {
   // Starting element is exclusive.
-  Element* found = findFocusableElementInternal(WebFocusTypeBackward, scope);
-
-  while (found) {
+  while (Element* found =
+             findFocusableElementInternal(WebFocusTypeBackward, scope)) {
     // Now |found| is on a focusable shadow host.
     // Find inside shadow backwards. If any focusable element is found, return
     // it, otherwise return the host itself.
@@ -612,39 +608,29 @@ Element* findFocusableElementRecursivelyBackward(ScopedFocusNavigation& scope) {
           findFocusableElementRecursivelyBackward(innerScope);
       if (foundInInnerFocusScope)
         return foundInInnerFocusScope;
-      if (isShadowHostDelegatesFocus(*found)) {
-        found = findFocusableElementInternal(WebFocusTypeBackward, scope);
+      if (isShadowHostDelegatesFocus(*found))
         continue;
-      }
       return found;
     }
 
     // If delegatesFocus is true and tabindex is negative, skip the whole shadow
     // tree under the shadow host.
-    if (isShadowHostDelegatesFocus(*found) && found->tabIndex() < 0) {
-      found = findFocusableElementInternal(WebFocusTypeBackward, scope);
+    if (isShadowHostDelegatesFocus(*found) && found->tabIndex() < 0)
       continue;
-    }
 
-    // Now |found| is on a non focusable scope owner (either shadow host or
-    // <shadow> or slot).  Find focusable element in descendant scope. If not
-    // found, find next focusable element within the current scope.
+    // Now |found| is on a non focusable scope owner (a shadow host, a <shadow>
+    // or a slot).  Find focusable element in descendant scope. If not found,
+    // find the next focusable element within the current scope.
     if (isNonFocusableFocusScopeOwner(*found)) {
       ScopedFocusNavigation innerScope =
           ScopedFocusNavigation::ownedByNonFocusableFocusScopeOwner(*found);
-      Element* foundInInnerFocusScope =
-          findFocusableElementRecursivelyBackward(innerScope);
-
-      if (foundInInnerFocusScope)
+      if (Element* foundInInnerFocusScope =
+              findFocusableElementRecursivelyBackward(innerScope))
         return foundInInnerFocusScope;
-      found = findFocusableElementInternal(WebFocusTypeBackward, scope);
       continue;
     }
     if (!isShadowHostDelegatesFocus(*found))
       return found;
-
-    scope.setCurrentElement(found);
-    found = findFocusableElementInternal(WebFocusTypeBackward, scope);
   }
   return nullptr;
 }