Focus navigation order in slot fallback contents is wrong
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 22 Aug 2018 19:59:24 +0000 (19:59 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 22 Aug 2018 19:59:24 +0000 (19:59 +0000)
https://bugs.webkit.org/show_bug.cgi?id=178001
<rdar://problem/42842997>

Reviewed by Antti Koivisto.

Source/WebCore:

The bug here is that when a slot uses its fallback content, the fallback content's focus order doesn't get
grouped by that of the slot. Consider the following DOM tree:

- ShadowRoot
    - div tabindex = 2
    - slot tabindex = 1
        - span tabindex = 3

In this example, the sequential focus navigation should be slot, span, then div. Even though span has tabindex
order of 3, which is lower than that of div, the fallback content of the slot should be grouped together
before the focus moves out of the slot content.

In WebKit, this concept of grouping elements for the sequential focus navigation ordering is implemeneted
as FocusNavigationScope. Both ShadowRoot and HTMLSlotElement are treated as a focus scope owner but we had
a bug that a slot element which uses its fallback content was not treated as a focus scope owner.

This patch addresses the bug by treating a slot wich uses its fallback content as a focus scope owner.

Test: fast/shadow-dom/focus-navigation-across-slots.html

* page/FocusController.cpp:
(WebCore::isFocusScopeOwner): Treat a slot elment hs a focus scope owner regardless of whether it has assigned
nodes or not.
(WebCore::FocusNavigationScope::SlotKind): Added.
(WebCore::FocusNavigationScope::m_slotKind): Added.
(WebCore::FocusNavigationScope::parentInScope const): Return null if `node` is a child of the slot element for
which this FocusNavigationScope is created (i.e. `node` is slot's fallback content).
(WebCore::FocusNavigationScope::firstNodeInScope const): Return the first child node when this
FocusNavigationScope is for a slot element using its fallback content.
(WebCore::FocusNavigationScope::lastNodeInScope const): Ditto for the last child.
(WebCore::FocusNavigationScope::FocusNavigationScope):
(WebCore::FocusNavigationScope::scopeOf): The scope of a child of a slot element which uses its fallback content
is its slot element (i.e. the current node is a fallback content). We can't simply check the current node is
a slot element which uses a fallback content since the scope of a slot element is the parent scope. e.g. its
tree scope like ShadowRoot or Document inside which this slot element appears.
(WebCore::FocusNavigationScope::scopeOwnedByScopeOwner): Create the appropriate FocusNavigationScope based on
whether the slot element has assigned or it uses its fallback content.

LayoutTests:

Updated the sequential focus navigation test for shadow DOM and its expectation.

New test passes in Firefox & Chrome other than the fact both browsers fail to focus a slot elemennt.

* fast/shadow-dom/focus-navigation-across-slots-expected.txt:
* fast/shadow-dom/focus-navigation-across-slots.html:

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

LayoutTests/ChangeLog
LayoutTests/fast/shadow-dom/focus-navigation-across-slots-expected.txt
LayoutTests/fast/shadow-dom/focus-navigation-across-slots.html
Source/WebCore/ChangeLog
Source/WebCore/page/FocusController.cpp

index e2f7272..446acf9 100644 (file)
@@ -1,3 +1,18 @@
+2018-08-21  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Focus navigation order in slot fallback contents is wrong
+        https://bugs.webkit.org/show_bug.cgi?id=178001
+        <rdar://problem/42842997>
+
+        Reviewed by Antti Koivisto.
+
+        Updated the sequential focus navigation test for shadow DOM and its expectation.
+
+        New test passes in Firefox & Chrome other than the fact both browsers fail to focus a slot elemennt.
+
+        * fast/shadow-dom/focus-navigation-across-slots-expected.txt:
+        * fast/shadow-dom/focus-navigation-across-slots.html:
+
 2018-08-22  Per Arne Vollan  <pvollan@apple.com>
 
         [Win] Some video tests under http/tests/security are crashing on EWS.
index 0349f78..f274ba1 100644 (file)
@@ -14,15 +14,15 @@ It should traverse focusable elements in the increasing numerical order and then
 10. Content in slot 2 with tabindex=1
 11. Content in slot 2 with tabindex=1
 12. Content in slot 2 with tabindex=0
-13. Non-focusable slot fallback with tabindex=1
-14. Focusable slot element.
+13. Focusable slot element.
+14. Focusable slot fallback content with tabindex=0
 15. Shadow content with tabindex=2
-16. Non-focusable slot fallback with tabindex=0
-17. Focusable slot fallback content with tabindex=0
-16. Non-focusable slot fallback with tabindex=0
+16. Non-focusable slot fallback with tabindex=1
+17. Non-focusable slot fallback with tabindex=0
+16. Non-focusable slot fallback with tabindex=1
 15. Shadow content with tabindex=2
-14. Focusable slot element.
-13. Non-focusable slot fallback with tabindex=1
+14. Focusable slot fallback content with tabindex=0
+13. Focusable slot element.
 12. Content in slot 2 with tabindex=0
 11. Content in slot 2 with tabindex=1
 10. Content in slot 2 with tabindex=1
index c4056d2..d9915d4 100644 (file)
@@ -102,13 +102,13 @@ window.onload = function () {
     shadowWithSlotFallback.closedShadowRoot.innerHTML = `
         <slot name="slot1" onfocus="log(this)">
             Non-focusable slot should not be focused.
-            <div tabindex="0">16. Non-focusable slot fallback with tabindex=0</div>
-            <div tabindex="1">13. Non-focusable slot fallback with tabindex=1</div>
+            <div tabindex="0">17. Non-focusable slot fallback with tabindex=0</div>
+            <div tabindex="1">16. Non-focusable slot fallback with tabindex=1</div>
         </slot>
         <div tabindex="2" onfocus="log(this)">15. Shadow content with tabindex=2</div>
         <slot name="slot2" tabindex="1" style="display:block;" onfocus="log(this)">
-            14. Focusable slot element.
-            <div tabindex="0">17. Focusable slot fallback content with tabindex=0</div>
+            13. Focusable slot element.
+            <div tabindex="0">14. Focusable slot fallback content with tabindex=0</div>
         </slot>`;
 
     document.getElementById('first').focus();
index 57f68d7..084c864 100644 (file)
@@ -1,3 +1,49 @@
+2018-08-21  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Focus navigation order in slot fallback contents is wrong
+        https://bugs.webkit.org/show_bug.cgi?id=178001
+        <rdar://problem/42842997>
+
+        Reviewed by Antti Koivisto.
+
+        The bug here is that when a slot uses its fallback content, the fallback content's focus order doesn't get
+        grouped by that of the slot. Consider the following DOM tree:
+
+        - ShadowRoot
+            - div tabindex = 2
+            - slot tabindex = 1
+                - span tabindex = 3
+
+        In this example, the sequential focus navigation should be slot, span, then div. Even though span has tabindex
+        order of 3, which is lower than that of div, the fallback content of the slot should be grouped together
+        before the focus moves out of the slot content.
+
+        In WebKit, this concept of grouping elements for the sequential focus navigation ordering is implemeneted
+        as FocusNavigationScope. Both ShadowRoot and HTMLSlotElement are treated as a focus scope owner but we had
+        a bug that a slot element which uses its fallback content was not treated as a focus scope owner.
+
+        This patch addresses the bug by treating a slot wich uses its fallback content as a focus scope owner.
+
+        Test: fast/shadow-dom/focus-navigation-across-slots.html
+
+        * page/FocusController.cpp:
+        (WebCore::isFocusScopeOwner): Treat a slot elment hs a focus scope owner regardless of whether it has assigned
+        nodes or not.
+        (WebCore::FocusNavigationScope::SlotKind): Added.
+        (WebCore::FocusNavigationScope::m_slotKind): Added.
+        (WebCore::FocusNavigationScope::parentInScope const): Return null if `node` is a child of the slot element for
+        which this FocusNavigationScope is created (i.e. `node` is slot's fallback content).
+        (WebCore::FocusNavigationScope::firstNodeInScope const): Return the first child node when this
+        FocusNavigationScope is for a slot element using its fallback content.
+        (WebCore::FocusNavigationScope::lastNodeInScope const): Ditto for the last child.
+        (WebCore::FocusNavigationScope::FocusNavigationScope):
+        (WebCore::FocusNavigationScope::scopeOf): The scope of a child of a slot element which uses its fallback content
+        is its slot element (i.e. the current node is a fallback content). We can't simply check the current node is
+        a slot element which uses a fallback content since the scope of a slot element is the parent scope. e.g. its
+        tree scope like ShadowRoot or Document inside which this slot element appears.
+        (WebCore::FocusNavigationScope::scopeOwnedByScopeOwner): Create the appropriate FocusNavigationScope based on
+        whether the slot element has assigned or it uses its fallback content.
+
 2018-08-22  David Kilzer  <ddkilzer@apple.com>
 
         Move files in WebCore project to match Xcode folder structure
index d7fd42e..a8458a2 100644 (file)
@@ -75,7 +75,7 @@ static inline bool isFocusScopeOwner(const Element& element)
 {
     if (element.shadowRoot() && !hasCustomFocusLogic(element))
         return true;
-    if (is<HTMLSlotElement>(element) && downcast<HTMLSlotElement>(element).assignedNodes()) {
+    if (is<HTMLSlotElement>(element)) {
         ShadowRoot* root = element.containingShadowRoot();
         if (root && root->host() && !hasCustomFocusLogic(*root->host()))
             return true;
@@ -97,6 +97,8 @@ public:
     Node* lastChildInScope(const Node&) const;
 
 private:
+    enum class SlotKind : uint8_t { Assigned, Fallback };
+
     Node* firstChildInScope(const Node&) const;
 
     Node* parentInScope(const Node&) const;
@@ -105,11 +107,11 @@ private:
     Node* previousSiblingInScope(const Node&) const;
 
     explicit FocusNavigationScope(TreeScope&);
-
-    explicit FocusNavigationScope(HTMLSlotElement&);
+    explicit FocusNavigationScope(HTMLSlotElement&, SlotKind);
 
     TreeScope* m_rootTreeScope { nullptr };
     HTMLSlotElement* m_slotElement { nullptr };
+    SlotKind m_slotKind { SlotKind::Assigned };
 };
 
 // FIXME: Focus navigation should work with shadow trees that have slots.
@@ -132,8 +134,17 @@ Node* FocusNavigationScope::parentInScope(const Node& node) const
     if (m_rootTreeScope && &m_rootTreeScope->rootNode() == &node)
         return nullptr;
 
-    if (UNLIKELY(m_slotElement && m_slotElement == node.assignedSlot()))
-        return nullptr;
+    if (UNLIKELY(m_slotElement)) {
+        if (m_slotKind == SlotKind::Assigned) {
+            if (m_slotElement == node.assignedSlot())
+                return nullptr;
+        } else {
+            ASSERT(m_slotKind == SlotKind::Fallback);
+            auto* parentNode = node.parentNode();
+            if (parentNode == m_slotElement)
+                return nullptr;
+        }
+    }
 
     return node.parentNode();
 }
@@ -166,8 +177,12 @@ Node* FocusNavigationScope::firstNodeInScope() const
 {
     if (UNLIKELY(m_slotElement)) {
         auto* assigneNodes = m_slotElement->assignedNodes();
-        ASSERT(assigneNodes);
-        return assigneNodes->first();
+        if (m_slotKind == SlotKind::Assigned) {
+            ASSERT(assigneNodes);
+            return assigneNodes->first();
+        }
+        ASSERT(m_slotKind == SlotKind::Fallback);
+        return m_slotElement->firstChild();
     }
     ASSERT(m_rootTreeScope);
     return &m_rootTreeScope->rootNode();
@@ -177,8 +192,12 @@ Node* FocusNavigationScope::lastNodeInScope() const
 {
     if (UNLIKELY(m_slotElement)) {
         auto* assigneNodes = m_slotElement->assignedNodes();
-        ASSERT(assigneNodes);
-        return assigneNodes->last();
+        if (m_slotKind == SlotKind::Assigned) {
+            ASSERT(assigneNodes);
+            return assigneNodes->last();
+        }
+        ASSERT(m_slotKind == SlotKind::Fallback);
+        return m_slotElement->lastChild();
     }
     ASSERT(m_rootTreeScope);
     return &m_rootTreeScope->rootNode();
@@ -213,8 +232,9 @@ FocusNavigationScope::FocusNavigationScope(TreeScope& treeScope)
 {
 }
 
-FocusNavigationScope::FocusNavigationScope(HTMLSlotElement& slotElement)
+FocusNavigationScope::FocusNavigationScope(HTMLSlotElement& slotElement, SlotKind slotKind)
     : m_slotElement(&slotElement)
+    , m_slotKind(slotKind)
 {
 }
 
@@ -235,15 +255,21 @@ Element* FocusNavigationScope::owner() const
 FocusNavigationScope FocusNavigationScope::scopeOf(Node& startingNode)
 {
     ASSERT(startingNode.isInTreeScope());
-    Node* root = nullptr;
-    for (Node* currentNode = &startingNode; currentNode; currentNode = currentNode->parentNode()) {
+    RefPtr<Node> root;
+    RefPtr<Node> parentNode;
+    for (RefPtr<Node> currentNode = &startingNode; currentNode; currentNode = parentNode) {
         root = currentNode;
         if (HTMLSlotElement* slot = currentNode->assignedSlot()) {
             if (isFocusScopeOwner(*slot))
-                return FocusNavigationScope(*slot);
+                return FocusNavigationScope(*slot, SlotKind::Assigned);
         }
         if (is<ShadowRoot>(currentNode))
             return FocusNavigationScope(downcast<ShadowRoot>(*currentNode));
+        parentNode = currentNode->parentNode();
+        // The scope of a fallback content of a HTMLSlotElement is the slot element
+        // but the scope of a HTMLSlotElement is its parent scope.
+        if (parentNode && is<HTMLSlotElement>(parentNode) && !downcast<HTMLSlotElement>(*parentNode).assignedNodes())
+            return FocusNavigationScope(downcast<HTMLSlotElement>(*parentNode), SlotKind::Fallback);
     }
     ASSERT(root);
     return FocusNavigationScope(root->treeScope());
@@ -252,8 +278,10 @@ FocusNavigationScope FocusNavigationScope::scopeOf(Node& startingNode)
 FocusNavigationScope FocusNavigationScope::scopeOwnedByScopeOwner(Element& element)
 {
     ASSERT(element.shadowRoot() || is<HTMLSlotElement>(element));
-    if (is<HTMLSlotElement>(element))
-        return FocusNavigationScope(downcast<HTMLSlotElement>(element));
+    if (is<HTMLSlotElement>(element)) {
+        auto& slot = downcast<HTMLSlotElement>(element);
+        return FocusNavigationScope(slot, slot.assignedNodes() ? SlotKind::Assigned : SlotKind::Fallback);
+    }
     return FocusNavigationScope(*element.shadowRoot());
 }