Changeset 235191 in webkit


Ignore:
Timestamp:
Aug 22, 2018 12:59:24 PM (6 years ago)
Author:
rniwa@webkit.org
Message:

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.

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:
Location:
trunk
Files:
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r235181 r235191  
     12018-08-21  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Focus navigation order in slot fallback contents is wrong
     4        https://bugs.webkit.org/show_bug.cgi?id=178001
     5        <rdar://problem/42842997>
     6
     7        Reviewed by Antti Koivisto.
     8
     9        Updated the sequential focus navigation test for shadow DOM and its expectation.
     10
     11        New test passes in Firefox & Chrome other than the fact both browsers fail to focus a slot elemennt.
     12
     13        * fast/shadow-dom/focus-navigation-across-slots-expected.txt:
     14        * fast/shadow-dom/focus-navigation-across-slots.html:
     15
    1162018-08-22  Per Arne Vollan  <pvollan@apple.com>
    217

  • trunk/LayoutTests/fast/shadow-dom/focus-navigation-across-slots-expected.txt

    r200964 r235191  
    151511. Content in slot 2 with tabindex=1
    161612. Content in slot 2 with tabindex=0
    17 13. Non-focusable slot fallback with tabindex=1
    18 14. Focusable slot element.
     1713. Focusable slot element.
     1814. Focusable slot fallback content with tabindex=0
    191915. Shadow content with tabindex=2
    20 16. Non-focusable slot fallback with tabindex=0
    21 17. Focusable slot fallback content with tabindex=0
    22 16. Non-focusable slot fallback with tabindex=0
     2016. Non-focusable slot fallback with tabindex=1
     2117. Non-focusable slot fallback with tabindex=0
     2216. Non-focusable slot fallback with tabindex=1
    232315. Shadow content with tabindex=2
    24 14. Focusable slot element.
    25 13. Non-focusable slot fallback with tabindex=1
     2414. Focusable slot fallback content with tabindex=0
     2513. Focusable slot element.
    262612. Content in slot 2 with tabindex=0
    272711. Content in slot 2 with tabindex=1

  • trunk/LayoutTests/fast/shadow-dom/focus-navigation-across-slots.html

    r200964 r235191  
    103103        <slot name="slot1" onfocus="log(this)">
    104104            Non-focusable slot should not be focused.
    105             <div tabindex="0">16. Non-focusable slot fallback with tabindex=0</div>
    106             <div tabindex="1">13. Non-focusable slot fallback with tabindex=1</div>
     105            <div tabindex="0">17. Non-focusable slot fallback with tabindex=0</div>
     106            <div tabindex="1">16. Non-focusable slot fallback with tabindex=1</div>
    107107        </slot>
    108108        <div tabindex="2" onfocus="log(this)">15. Shadow content with tabindex=2</div>
    109109        <slot name="slot2" tabindex="1" style="display:block;" onfocus="log(this)">
    110             14. Focusable slot element.
    111             <div tabindex="0">17. Focusable slot fallback content with tabindex=0</div>
     110            13. Focusable slot element.
     111            <div tabindex="0">14. Focusable slot fallback content with tabindex=0</div>
    112112        </slot>`;
    113113

  • trunk/Source/WebCore/ChangeLog

    r235190 r235191  
     12018-08-21  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Focus navigation order in slot fallback contents is wrong
     4        https://bugs.webkit.org/show_bug.cgi?id=178001
     5        <rdar://problem/42842997>
     6
     7        Reviewed by Antti Koivisto.
     8
     9        The bug here is that when a slot uses its fallback content, the fallback content's focus order doesn't get
     10        grouped by that of the slot. Consider the following DOM tree:
     11
     12        - ShadowRoot
     13            - div tabindex = 2
     14            - slot tabindex = 1
     15                - span tabindex = 3
     16
     17        In this example, the sequential focus navigation should be slot, span, then div. Even though span has tabindex
     18        order of 3, which is lower than that of div, the fallback content of the slot should be grouped together
     19        before the focus moves out of the slot content.
     20
     21        In WebKit, this concept of grouping elements for the sequential focus navigation ordering is implemeneted
     22        as FocusNavigationScope. Both ShadowRoot and HTMLSlotElement are treated as a focus scope owner but we had
     23        a bug that a slot element which uses its fallback content was not treated as a focus scope owner.
     24
     25        This patch addresses the bug by treating a slot wich uses its fallback content as a focus scope owner.
     26
     27        Test: fast/shadow-dom/focus-navigation-across-slots.html
     28
     29        * page/FocusController.cpp:
     30        (WebCore::isFocusScopeOwner): Treat a slot elment hs a focus scope owner regardless of whether it has assigned
     31        nodes or not.
     32        (WebCore::FocusNavigationScope::SlotKind): Added.
     33        (WebCore::FocusNavigationScope::m_slotKind): Added.
     34        (WebCore::FocusNavigationScope::parentInScope const): Return null if `node` is a child of the slot element for
     35        which this FocusNavigationScope is created (i.e. `node` is slot's fallback content).
     36        (WebCore::FocusNavigationScope::firstNodeInScope const): Return the first child node when this
     37        FocusNavigationScope is for a slot element using its fallback content.
     38        (WebCore::FocusNavigationScope::lastNodeInScope const): Ditto for the last child.
     39        (WebCore::FocusNavigationScope::FocusNavigationScope):
     40        (WebCore::FocusNavigationScope::scopeOf): The scope of a child of a slot element which uses its fallback content
     41        is its slot element (i.e. the current node is a fallback content). We can't simply check the current node is
     42        a slot element which uses a fallback content since the scope of a slot element is the parent scope. e.g. its
     43        tree scope like ShadowRoot or Document inside which this slot element appears.
     44        (WebCore::FocusNavigationScope::scopeOwnedByScopeOwner): Create the appropriate FocusNavigationScope based on
     45        whether the slot element has assigned or it uses its fallback content.
     46
    1472018-08-22  David Kilzer  <ddkilzer@apple.com>
    248

  • trunk/Source/WebCore/page/FocusController.cpp

    r234995 r235191  
    7676    if (element.shadowRoot() && !hasCustomFocusLogic(element))
    7777        return true;
    78     if (is<HTMLSlotElement>(element) && downcast<HTMLSlotElement>(element).assignedNodes()) {
     78    if (is<HTMLSlotElement>(element)) {
    7979        ShadowRoot* root = element.containingShadowRoot();
    8080        if (root && root->host() && !hasCustomFocusLogic(*root->host()))
    … …  
    9898
    9999private:
     100    enum class SlotKind : uint8_t { Assigned, Fallback };
     101
    100102    Node* firstChildInScope(const Node&) const;
    101103
    … …  
    106108
    107109    explicit FocusNavigationScope(TreeScope&);
    108 
    109     explicit FocusNavigationScope(HTMLSlotElement&);
     110    explicit FocusNavigationScope(HTMLSlotElement&, SlotKind);
    110111
    111112    TreeScope* m_rootTreeScope { nullptr };
    112113    HTMLSlotElement* m_slotElement { nullptr };
     114    SlotKind m_slotKind { SlotKind::Assigned };
    113115};
    114116
    … …  
    133135        return nullptr;
    134136
    135     if (UNLIKELY(m_slotElement && m_slotElement == node.assignedSlot()))
    136         return nullptr;
     137    if (UNLIKELY(m_slotElement)) {
     138        if (m_slotKind == SlotKind::Assigned) {
     139            if (m_slotElement == node.assignedSlot())
     140                return nullptr;
     141        } else {
     142            ASSERT(m_slotKind == SlotKind::Fallback);
     143            auto* parentNode = node.parentNode();
     144            if (parentNode == m_slotElement)
     145                return nullptr;
     146        }
     147    }
    137148
    138149    return node.parentNode();
    … …  
    167178    if (UNLIKELY(m_slotElement)) {
    168179        auto* assigneNodes = m_slotElement->assignedNodes();
    169         ASSERT(assigneNodes);
    170         return assigneNodes->first();
     180        if (m_slotKind == SlotKind::Assigned) {
     181            ASSERT(assigneNodes);
     182            return assigneNodes->first();
     183        }
     184        ASSERT(m_slotKind == SlotKind::Fallback);
     185        return m_slotElement->firstChild();
    171186    }
    172187    ASSERT(m_rootTreeScope);
    … …  
    178193    if (UNLIKELY(m_slotElement)) {
    179194        auto* assigneNodes = m_slotElement->assignedNodes();
    180         ASSERT(assigneNodes);
    181         return assigneNodes->last();
     195        if (m_slotKind == SlotKind::Assigned) {
     196            ASSERT(assigneNodes);
     197            return assigneNodes->last();
     198        }
     199        ASSERT(m_slotKind == SlotKind::Fallback);
     200        return m_slotElement->lastChild();
    182201    }
    183202    ASSERT(m_rootTreeScope);
    … …  
    214233}
    215234
    216 FocusNavigationScope::FocusNavigationScope(HTMLSlotElement& slotElement)
     235FocusNavigationScope::FocusNavigationScope(HTMLSlotElement& slotElement, SlotKind slotKind)
    217236    : m_slotElement(&slotElement)
     237    , m_slotKind(slotKind)
    218238{
    219239}
    … …  
    236256{
    237257    ASSERT(startingNode.isInTreeScope());
    238     Node* root = nullptr;
    239     for (Node* currentNode = &startingNode; currentNode; currentNode = currentNode->parentNode()) {
     258    RefPtr<Node> root;
     259    RefPtr<Node> parentNode;
     260    for (RefPtr<Node> currentNode = &startingNode; currentNode; currentNode = parentNode) {
    240261        root = currentNode;
    241262        if (HTMLSlotElement* slot = currentNode->assignedSlot()) {
    242263            if (isFocusScopeOwner(*slot))
    243                 return FocusNavigationScope(*slot);
     264                return FocusNavigationScope(*slot, SlotKind::Assigned);
    244265        }
    245266        if (is<ShadowRoot>(currentNode))
    246267            return FocusNavigationScope(downcast<ShadowRoot>(*currentNode));
     268        parentNode = currentNode->parentNode();
     269        // The scope of a fallback content of a HTMLSlotElement is the slot element
     270        // but the scope of a HTMLSlotElement is its parent scope.
     271        if (parentNode && is<HTMLSlotElement>(parentNode) && !downcast<HTMLSlotElement>(*parentNode).assignedNodes())
     272            return FocusNavigationScope(downcast<HTMLSlotElement>(*parentNode), SlotKind::Fallback);
    247273    }
    248274    ASSERT(root);
    … …  
    253279{
    254280    ASSERT(element.shadowRoot() || is<HTMLSlotElement>(element));
    255     if (is<HTMLSlotElement>(element))
    256         return FocusNavigationScope(downcast<HTMLSlotElement>(element));
     281    if (is<HTMLSlotElement>(element)) {
     282        auto& slot = downcast<HTMLSlotElement>(element);
     283        return FocusNavigationScope(slot, slot.assignedNodes() ? SlotKind::Assigned : SlotKind::Fallback);
     284    }
    257285    return FocusNavigationScope(*element.shadowRoot());
    258286}
Note: See TracChangeset for help on using the changeset viewer.