Fix focus navigation getting stuck

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 540 matching lines @@
 // @return The focus element that comes after/before start element.
 //
 // [1]
 // https://html.spec.whatwg.org/multipage/interaction.html#sequential-focus-navigation
 // [2] https://w3c.github.io/webcomponents/spec/shadow/#focus-navigation
 inline Element* findFocusableElementInternal(WebFocusType type,
                                              ScopedFocusNavigation& scope) {
   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 &&
           isShadowHostWithoutCustomFocusLogic(*found)) {
         ScopedFocusNavigation innerScope =
             ScopedFocusNavigation::ownedByShadowHost(*found);
         if (Element* foundInInnerFocusScope =
                 findFocusableElementRecursivelyForward(innerScope))
           return foundInInnerFocusScope;
       }
       // Skip to the next element in the same scope.
-      found = findFocusableElementInternal(WebFocusTypeForward, scope);
       continue;
     }
     if (!isNonFocusableFocusScopeOwner(*found))
       return found;
 
     // Now |found| is on a non focusable scope owner (either shadow host or
     // <shadow> or slot) Find inside the inward scope and return it if found.
     // Otherwise continue searching in the same scope.
     ScopedFocusNavigation innerScope =
         ScopedFocusNavigation::ownedByNonFocusableFocusScopeOwner(*found);
     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.
     if (isKeyboardFocusableShadowHost(*found)) {
       ScopedFocusNavigation innerScope =
           ScopedFocusNavigation::ownedByShadowHost(*found);
       Element* foundInInnerFocusScope =
           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;
 }
 
 Element* findFocusableElementRecursively(WebFocusType type,
                                          ScopedFocusNavigation& scope) {
   return (type == WebFocusTypeForward)
              ? findFocusableElementRecursivelyForward(scope)
              : findFocusableElementRecursivelyBackward(scope);
 }
@@ skipping 753 matching lines @@
 
   return consumed;
 }
 
 DEFINE_TRACE(FocusController) {
   visitor->trace(m_page);
   visitor->trace(m_focusedFrame);
 }
 
 }  // namespace blink