Refactor FocusController::findFocusableElementRecursively
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 9 May 2016 16:20:20 +0000 (16:20 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 9 May 2016 16:20:20 +0000 (16:20 +0000)
https://bugs.webkit.org/show_bug.cgi?id=157415

Reviewed by Darin Adler.

Refactor FocusController::findFocusableElementRecursively and related functions. Extracted two functions:
nextFocusableElementWithinScope and previousFocusableElementWithinScope out of it since they didn't really share
any code other than calling findFocusableElement at the beginning.

Also renamed internal variant of nextFocusableElement and previousFocusableElement to nextFocusableElementOrScopeOwner
and previousFocusableElementOrScopeOwner. It was confusing to have these internal functions in addition to public
member functions that are used in Objective-C DOM API.

No new tests are added since there should be no behavioral change.

* page/FocusController.cpp:
(WebCore::FocusController::findFocusableElementDescendingDownIntoFrameDocument): Added a FIXME.
(WebCore::FocusController::advanceFocusInDocumentOrder): Use findFocusableElementAcrossFocusScope instead of manually
calling findFocusableElementRecursively and findFocusableElementDescendingDownIntoFrameDocument separately.
(WebCore::FocusController::findFocusableElementAcrossFocusScope): Now that findFocusableElementWithinScope calls
findFocusableElementDescendingDownIntoFrameDocument internally, there is no need to keep around "found" local variable.
Introduce a few early exists for a better clarity.
(WebCore::FocusController::findFocusableElementWithinScope): Renamed from findFocusableElementRecursively. Also call
findFocusableElementDescendingDownIntoFrameDocument here instead of findFocusableElementAcrossFocusScope.
(WebCore::FocusController::nextFocusableElementWithinScope): Extracted from findFocusableElementRecursively.
(WebCore::FocusController::previousFocusableElementWithinScope): Ditto.
(WebCore::FocusController::findFocusableElement):
(WebCore::FocusController::nextFocusableElement): Added a FIXME.
(WebCore::FocusController::previousFocusableElement): Ditto.
(WebCore::FocusController::findFocusableElementOrScopeOwner): Renamed from findFocusableElement for clarity.
(WebCore::FocusController::nextFocusableElementOrScopeOwner): Ditto from nextFocusableElement.
(WebCore::FocusController::previousFocusableElementOrScopeOwner): Ditto from nextFocusableElement.
* page/FocusController.h:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@200576 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/page/FocusController.cpp
Source/WebCore/page/FocusController.h

index 4c2e272..f8e7f90 100644 (file)
@@ -1,3 +1,39 @@
+2016-05-09  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Refactor FocusController::findFocusableElementRecursively
+        https://bugs.webkit.org/show_bug.cgi?id=157415
+
+        Reviewed by Darin Adler.
+
+        Refactor FocusController::findFocusableElementRecursively and related functions. Extracted two functions:
+        nextFocusableElementWithinScope and previousFocusableElementWithinScope out of it since they didn't really share
+        any code other than calling findFocusableElement at the beginning.
+
+        Also renamed internal variant of nextFocusableElement and previousFocusableElement to nextFocusableElementOrScopeOwner
+        and previousFocusableElementOrScopeOwner. It was confusing to have these internal functions in addition to public
+        member functions that are used in Objective-C DOM API.
+
+        No new tests are added since there should be no behavioral change.
+
+        * page/FocusController.cpp:
+        (WebCore::FocusController::findFocusableElementDescendingDownIntoFrameDocument): Added a FIXME.
+        (WebCore::FocusController::advanceFocusInDocumentOrder): Use findFocusableElementAcrossFocusScope instead of manually
+        calling findFocusableElementRecursively and findFocusableElementDescendingDownIntoFrameDocument separately.
+        (WebCore::FocusController::findFocusableElementAcrossFocusScope): Now that findFocusableElementWithinScope calls
+        findFocusableElementDescendingDownIntoFrameDocument internally, there is no need to keep around "found" local variable.
+        Introduce a few early exists for a better clarity.
+        (WebCore::FocusController::findFocusableElementWithinScope): Renamed from findFocusableElementRecursively. Also call
+        findFocusableElementDescendingDownIntoFrameDocument here instead of findFocusableElementAcrossFocusScope.
+        (WebCore::FocusController::nextFocusableElementWithinScope): Extracted from findFocusableElementRecursively.
+        (WebCore::FocusController::previousFocusableElementWithinScope): Ditto.
+        (WebCore::FocusController::findFocusableElement):
+        (WebCore::FocusController::nextFocusableElement): Added a FIXME.
+        (WebCore::FocusController::previousFocusableElement): Ditto.
+        (WebCore::FocusController::findFocusableElementOrScopeOwner): Renamed from findFocusableElement for clarity.
+        (WebCore::FocusController::nextFocusableElementOrScopeOwner): Ditto from nextFocusableElement.
+        (WebCore::FocusController::previousFocusableElementOrScopeOwner): Ditto from nextFocusableElement.
+        * page/FocusController.h:
+
 2016-05-09  Manuel Rego Casasnovas  <rego@igalia.com>
 
         [css-grid] Fix static position for positioned grid items
index dead3dd..270d4f9 100644 (file)
@@ -300,7 +300,8 @@ Element* FocusController::findFocusableElementDescendingDownIntoFrameDocument(Fo
         HTMLFrameOwnerElement& owner = downcast<HTMLFrameOwnerElement>(*element);
         if (!owner.contentFrame())
             break;
-        Element* foundElement = findFocusableElement(direction, FocusNavigationScope::scopeOwnedByIFrame(owner), 0, event);
+        // FIXME: This can return a non-focusable shadow root.
+        Element* foundElement = findFocusableElementOrScopeOwner(direction, FocusNavigationScope::scopeOwnedByIFrame(owner), 0, event);
         if (!foundElement)
             break;
         ASSERT(element != foundElement);
@@ -366,8 +367,7 @@ bool FocusController::advanceFocusInDocumentOrder(FocusDirection direction, Keyb
         }
 
         // Chrome doesn't want focus, so we should wrap focus.
-        element = findFocusableElementRecursively(direction, FocusNavigationScope::scopeOf(*m_page.mainFrame().document()), 0, event);
-        element = findFocusableElementDescendingDownIntoFrameDocument(direction, element.get(), event);
+        element = findFocusableElementAcrossFocusScope(direction, FocusNavigationScope::scopeOf(*m_page.mainFrame().document()), nullptr, event);
 
         if (!element)
             return false;
@@ -421,57 +421,74 @@ bool FocusController::advanceFocusInDocumentOrder(FocusDirection direction, Keyb
 Element* FocusController::findFocusableElementAcrossFocusScope(FocusDirection direction, const FocusNavigationScope& scope, Node* currentNode, KeyboardEvent* event)
 {
     ASSERT(!is<Element>(currentNode) || !isNonFocusableShadowHost(*downcast<Element>(currentNode), *event));
-    Element* found;
+
     if (currentNode && direction == FocusDirectionForward && isFocusableShadowHost(*currentNode, *event)) {
-        Element* foundInInnerFocusScope = findFocusableElementRecursively(direction, FocusNavigationScope::scopeOwnedByShadowHost(downcast<Element>(*currentNode)), 0, event);
-        found = foundInInnerFocusScope ? foundInInnerFocusScope : findFocusableElementRecursively(direction, scope, currentNode, event);
-    } else
-        found = findFocusableElementRecursively(direction, scope, currentNode, event);
+        if (Element* candidateInInnerScope = findFocusableElementWithinScope(direction, FocusNavigationScope::scopeOwnedByShadowHost(downcast<Element>(*currentNode)), 0, event))
+            return candidateInInnerScope;
+    }
+
+    if (Element* candidateInCurrentScope = findFocusableElementWithinScope(direction, scope, currentNode, event))
+        return candidateInCurrentScope;
 
     // If there's no focusable node to advance to, move up the focus scopes until we find one.
     Element* owner = scope.owner();
-    while (!found && owner) {
-        FocusNavigationScope currentScope = FocusNavigationScope::scopeOf(*owner);
-        if (direction == FocusDirectionBackward && isFocusableShadowHost(*owner, *event)) {
-            found = owner;
-            break;
-        }
-        found = findFocusableElementRecursively(direction, currentScope, owner, event);
-        owner = currentScope.owner();
+    while (owner) {
+        FocusNavigationScope outerScope = FocusNavigationScope::scopeOf(*owner);
+        if (direction == FocusDirectionBackward && isFocusableShadowHost(*owner, *event))
+            return findFocusableElementDescendingDownIntoFrameDocument(direction, owner, event);
+        if (Element* candidateInOuterScope = findFocusableElementWithinScope(direction, outerScope, owner, event))
+            return candidateInOuterScope;
+        owner = outerScope.owner();
     }
-    found = findFocusableElementDescendingDownIntoFrameDocument(direction, found, event);
-    return found;
+    return nullptr;
 }
 
-Element* FocusController::findFocusableElementRecursively(FocusDirection direction, const FocusNavigationScope& scope, Node* start, KeyboardEvent* event)
+Element* FocusController::findFocusableElementWithinScope(FocusDirection direction, const FocusNavigationScope& scope, Node* start, KeyboardEvent* event)
 {
     // Starting node is exclusive.
-    Element* found = findFocusableElement(direction, scope, start, event);
+    Element* candidate = direction == FocusDirectionForward
+        ? nextFocusableElementWithinScope(scope, start, event)
+        : previousFocusableElementWithinScope(scope, start, event);
+    return findFocusableElementDescendingDownIntoFrameDocument(direction, candidate, event);
+}
+
+Element* FocusController::nextFocusableElementWithinScope(const FocusNavigationScope& scope, Node* start, KeyboardEvent* event)
+{
+    Element* found = nextFocusableElementOrScopeOwner(scope, start, event);
     if (!found)
         return nullptr;
-    if (direction == FocusDirectionForward) {
-        if (!isNonFocusableShadowHost(*found, *event))
-            return found;
-        Element* foundInInnerFocusScope = findFocusableElementRecursively(direction, FocusNavigationScope::scopeOwnedByShadowHost(*found), 0, event);
-        return foundInInnerFocusScope ? foundInInnerFocusScope : findFocusableElementRecursively(direction, scope, found, event);
+    if (isNonFocusableShadowHost(*found, *event)) {
+        if (Element* foundInInnerFocusScope = nextFocusableElementWithinScope(FocusNavigationScope::scopeOwnedByShadowHost(*found), 0, event))
+            return foundInInnerFocusScope;
+        return nextFocusableElementWithinScope(scope, found, event);
     }
-    ASSERT(direction == FocusDirectionBackward);
+    return found;
+}
+
+Element* FocusController::previousFocusableElementWithinScope(const FocusNavigationScope& scope, Node* start, KeyboardEvent* event)
+{
+    Element* found = previousFocusableElementOrScopeOwner(scope, start, event);
+    if (!found)
+        return nullptr;
     if (isFocusableShadowHost(*found, *event)) {
-        Element* foundInInnerFocusScope = findFocusableElementRecursively(direction, FocusNavigationScope::scopeOwnedByShadowHost(*found), 0, event);
-        return foundInInnerFocusScope ? foundInInnerFocusScope : found;
+        // Search an inner focusable element in the shadow tree from the end.
+        if (Element* foundInInnerFocusScope = previousFocusableElementWithinScope(FocusNavigationScope::scopeOwnedByShadowHost(*found), 0, event))
+            return foundInInnerFocusScope;
+        return found;
     }
     if (isNonFocusableShadowHost(*found, *event)) {
-        Element* foundInInnerFocusScope = findFocusableElementRecursively(direction, FocusNavigationScope::scopeOwnedByShadowHost(*found), 0, event);
-        return foundInInnerFocusScope ? foundInInnerFocusScope :findFocusableElementRecursively(direction, scope, found, event);
+        if (Element* foundInInnerFocusScope = previousFocusableElementWithinScope(FocusNavigationScope::scopeOwnedByShadowHost(*found), 0, event))
+            return foundInInnerFocusScope;
+        return previousFocusableElementWithinScope(scope, found, event);
     }
     return found;
 }
 
-Element* FocusController::findFocusableElement(FocusDirection direction, const FocusNavigationScope& scope, Node* node, KeyboardEvent* event)
+Element* FocusController::findFocusableElementOrScopeOwner(FocusDirection direction, const FocusNavigationScope& scope, Node* node, KeyboardEvent* event)
 {
     return (direction == FocusDirectionForward)
-        ? nextFocusableElement(scope, node, event)
-        : previousFocusableElement(scope, node, event);
+        ? nextFocusableElementOrScopeOwner(scope, node, event)
+        : previousFocusableElementOrScopeOwner(scope, node, event);
 }
 
 Element* FocusController::findElementWithExactTabIndex(const FocusNavigationScope& scope, Node* start, int tabIndex, KeyboardEvent* event, FocusDirection direction)
@@ -528,17 +545,19 @@ static Element* previousElementWithLowerTabIndex(const FocusNavigationScope& sco
 
 Element* FocusController::nextFocusableElement(Node& start)
 {
+    // FIXME: This can return a non-focusable shadow host.
     Ref<KeyboardEvent> keyEvent = KeyboardEvent::createForDummy();
-    return nextFocusableElement(FocusNavigationScope::scopeOf(start), &start, keyEvent.ptr());
+    return nextFocusableElementOrScopeOwner(FocusNavigationScope::scopeOf(start), &start, keyEvent.ptr());
 }
 
 Element* FocusController::previousFocusableElement(Node& start)
 {
+    // FIXME: This can return a non-focusable shadow host.
     Ref<KeyboardEvent> keyEvent = KeyboardEvent::createForDummy();
-    return previousFocusableElement(FocusNavigationScope::scopeOf(start), &start, keyEvent.ptr());
+    return previousFocusableElementOrScopeOwner(FocusNavigationScope::scopeOf(start), &start, keyEvent.ptr());
 }
 
-Element* FocusController::nextFocusableElement(const FocusNavigationScope& scope, Node* start, KeyboardEvent* event)
+Element* FocusController::nextFocusableElementOrScopeOwner(const FocusNavigationScope& scope, Node* start, KeyboardEvent* event)
 {
     int startTabIndex = 0;
     if (start && is<Element>(*start))
@@ -575,7 +594,7 @@ Element* FocusController::nextFocusableElement(const FocusNavigationScope& scope
     return findElementWithExactTabIndex(scope, &scope.rootNode(), 0, event, FocusDirectionForward);
 }
 
-Element* FocusController::previousFocusableElement(const FocusNavigationScope& scope, Node* start, KeyboardEvent* event)
+Element* FocusController::previousFocusableElementOrScopeOwner(const FocusNavigationScope& scope, Node* start, KeyboardEvent* event)
 {
     Node* last = nullptr;
     for (Node* node = &scope.rootNode(); node; node = scope.lastChildInScope(node))
index 4f3c5c2..b156534 100644 (file)
@@ -89,7 +89,11 @@ private:
     bool advanceFocusInDocumentOrder(FocusDirection, KeyboardEvent*, bool initialFocus);
 
     Element* findFocusableElementAcrossFocusScope(FocusDirection, const FocusNavigationScope& startScope, Node* start, KeyboardEvent*);
-    Element* findFocusableElementRecursively(FocusDirection, const FocusNavigationScope&, Node* start, KeyboardEvent*);
+
+    Element* findFocusableElementWithinScope(FocusDirection, const FocusNavigationScope&, Node* start, KeyboardEvent*);
+    Element* nextFocusableElementWithinScope(const FocusNavigationScope&, Node* start, KeyboardEvent*);
+    Element* previousFocusableElementWithinScope(const FocusNavigationScope&, Node* start, KeyboardEvent*);
+
     Element* findFocusableElementDescendingDownIntoFrameDocument(FocusDirection, Element*, KeyboardEvent*);
 
     // Searches through the given tree scope, starting from start node, for the next/previous selectable element that comes after/before start node.
@@ -101,12 +105,12 @@ private:
     // @return The focus node that comes after/before start node.
     //
     // See http://www.w3.org/TR/html4/interact/forms.html#h-17.11.1
-    Element* findFocusableElement(FocusDirection, const FocusNavigationScope&, Node* start, KeyboardEvent*);
+    Element* findFocusableElementOrScopeOwner(FocusDirection, const FocusNavigationScope&, Node* start, KeyboardEvent*);
 
     Element* findElementWithExactTabIndex(const FocusNavigationScope&, Node* start, int tabIndex, KeyboardEvent*, FocusDirection);
     
-    Element* nextFocusableElement(const FocusNavigationScope&, Node* start, KeyboardEvent*);
-    Element* previousFocusableElement(const FocusNavigationScope&, Node* start, KeyboardEvent*);
+    Element* nextFocusableElementOrScopeOwner(const FocusNavigationScope&, Node* start, KeyboardEvent*);
+    Element* previousFocusableElementOrScopeOwner(const FocusNavigationScope&, Node* start, KeyboardEvent*);
 
     bool advanceFocusDirectionallyInContainer(Node* container, const LayoutRect& startingRect, FocusDirection, KeyboardEvent*);
     void findFocusCandidateInContainer(Node& container, const LayoutRect& startingRect, FocusDirection, KeyboardEvent*, FocusCandidate& closest);