Attachment 347765 Details for Bug 178001 – Fixes the bug

[patch] Fixes the bug

bug-178001-20180821213727.patch (text/plain), 12.06 KB, created by Ryosuke Niwa on 2018-08-21 21:37:28 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2018-08-21 21:37:28 PDT

Size: 12.06 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 235157)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -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 NOBODY (OOPS!).
>+
>+        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-21  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         [Attachment Support] Remove _WKAttachments and notify the UI client upon mainframe navigation
>Index: Source/WebCore/page/FocusController.cpp
>===================================================================
>--- Source/WebCore/page/FocusController.cpp	(revision 235153)
>+++ Source/WebCore/page/FocusController.cpp	(working copy)
>@@ -75,7 +75,7 @@ static inline bool isFocusScopeOwner(con
> {
>     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::parentInScop
>     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::firstNodeInS
> {
>     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::lastNodeInSc
> {
>     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::FocusNavigationSco
> {
> }
> 
>-FocusNavigationScope::FocusNavigationScope(HTMLSlotElement& slotElement)
>+FocusNavigationScope::FocusNavigationScope(HTMLSlotElement& slotElement, SlotKind slotKind)
>     : m_slotElement(&slotElement)
>+    , m_slotKind(slotKind)
> {
> }
> 
>@@ -235,15 +255,21 @@ Element* FocusNavigationScope::owner() c
> 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 FocusNavigationScop
> 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 = makeRef(downcast<HTMLSlotElement>(element));
>+        return FocusNavigationScope(slot, slot->assignedNodes() ? SlotKind::Assigned : SlotKind::Fallback);
>+    }
>     return FocusNavigationScope(*element.shadowRoot());
> }
> 
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 235153)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -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 NOBODY (OOPS!).
>+
>+        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-21  Megan Gardner  <megan_gardner@apple.com>
> 
>         Change Selection modification to not snap the grabber when selecting above or below the selection anchor
>Index: LayoutTests/fast/shadow-dom/focus-navigation-across-slots-expected.txt
>===================================================================
>--- LayoutTests/fast/shadow-dom/focus-navigation-across-slots-expected.txt	(revision 235153)
>+++ LayoutTests/fast/shadow-dom/focus-navigation-across-slots-expected.txt	(working copy)
>@@ -14,15 +14,15 @@ It should traverse focusable elements in
> 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: LayoutTests/fast/shadow-dom/focus-navigation-across-slots.html
>===================================================================
>--- LayoutTests/fast/shadow-dom/focus-navigation-across-slots.html	(revision 235153)
>+++ LayoutTests/fast/shadow-dom/focus-navigation-across-slots.html	(working copy)
>@@ -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();

Flags:

koivisto: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 178001: 347762 | 347765