Title: [235191] trunk
- Revision
- 235191
- Author
- [email protected]
- Date
- 2018-08-22 12:59:24 -0700 (Wed, 22 Aug 2018)
Log 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:
Modified Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (235190 => 235191)
--- trunk/LayoutTests/ChangeLog 2018-08-22 19:33:46 UTC (rev 235190)
+++ trunk/LayoutTests/ChangeLog 2018-08-22 19:59:24 UTC (rev 235191)
@@ -1,3 +1,18 @@
+2018-08-21 Ryosuke Niwa <[email protected]>
+
+ 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 <[email protected]>
[Win] Some video tests under http/tests/security are crashing on EWS.
Modified: trunk/LayoutTests/fast/shadow-dom/focus-navigation-across-slots-expected.txt (235190 => 235191)
--- trunk/LayoutTests/fast/shadow-dom/focus-navigation-across-slots-expected.txt 2018-08-22 19:33:46 UTC (rev 235190)
+++ trunk/LayoutTests/fast/shadow-dom/focus-navigation-across-slots-expected.txt 2018-08-22 19:59:24 UTC (rev 235191)
@@ -14,15 +14,15 @@
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
Modified: trunk/LayoutTests/fast/shadow-dom/focus-navigation-across-slots.html (235190 => 235191)
--- trunk/LayoutTests/fast/shadow-dom/focus-navigation-across-slots.html 2018-08-22 19:33:46 UTC (rev 235190)
+++ trunk/LayoutTests/fast/shadow-dom/focus-navigation-across-slots.html 2018-08-22 19:59:24 UTC (rev 235191)
@@ -102,13 +102,13 @@
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();
Modified: trunk/Source/WebCore/ChangeLog (235190 => 235191)
--- trunk/Source/WebCore/ChangeLog 2018-08-22 19:33:46 UTC (rev 235190)
+++ trunk/Source/WebCore/ChangeLog 2018-08-22 19:59:24 UTC (rev 235191)
@@ -1,3 +1,49 @@
+2018-08-21 Ryosuke Niwa <[email protected]>
+
+ 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 <[email protected]>
Move files in WebCore project to match Xcode folder structure
Modified: trunk/Source/WebCore/page/FocusController.cpp (235190 => 235191)
--- trunk/Source/WebCore/page/FocusController.cpp 2018-08-22 19:33:46 UTC (rev 235190)
+++ trunk/Source/WebCore/page/FocusController.cpp 2018-08-22 19:59:24 UTC (rev 235191)
@@ -75,7 +75,7 @@
{
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 @@
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 @@
Node* previousSiblingInScope(const Node&) const;
explicit FocusNavigationScope(TreeScope&);
+ explicit FocusNavigationScope(HTMLSlotElement&, SlotKind);
- explicit FocusNavigationScope(HTMLSlotElement&);
-
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 @@
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 @@
{
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 @@
{
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(HTMLSlotElement& slotElement)
+FocusNavigationScope::FocusNavigationScope(HTMLSlotElement& slotElement, SlotKind slotKind)
: m_slotElement(&slotElement)
+ , m_slotKind(slotKind)
{
}
@@ -235,15 +255,21 @@
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::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());
}
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
