[webkit-changes] [200576] trunk/Source/WebCore

Mon, 09 May 2016 09:20:46 -0700

Title: [200576] trunk/Source/WebCore
Revision
200576
Author
rn...@webkit.org
Date
2016-05-09 09:20:20 -0700 (Mon, 09 May 2016)

Log Message

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:

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (200575 => 200576)


--- trunk/Source/WebCore/ChangeLog	2016-05-09 16:09:17 UTC (rev 200575)
+++ trunk/Source/WebCore/ChangeLog	2016-05-09 16:20:20 UTC (rev 200576)
@@ -1,3 +1,39 @@
+2016-05-09  Ryosuke Niwa  <rn...@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  <r...@igalia.com>
 
         [css-grid] Fix static position for positioned grid items

Modified: trunk/Source/WebCore/page/FocusController.cpp (200575 => 200576)


--- trunk/Source/WebCore/page/FocusController.cpp	2016-05-09 16:09:17 UTC (rev 200575)
+++ trunk/Source/WebCore/page/FocusController.cpp	2016-05-09 16:20:20 UTC (rev 200576)
@@ -300,7 +300,8 @@
         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 @@
         }
 
         // 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 @@
 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 @@
 
 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 @@
     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))

Modified: trunk/Source/WebCore/page/FocusController.h (200575 => 200576)


--- trunk/Source/WebCore/page/FocusController.h	2016-05-09 16:09:17 UTC (rev 200575)
+++ trunk/Source/WebCore/page/FocusController.h	2016-05-09 16:20:20 UTC (rev 200576)
@@ -89,7 +89,11 @@
     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 @@
     // @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);

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to