Fix focus navigation for nested slot case

third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp

Index: third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp
diff --git a/third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp b/third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp
index bbffc1cef8b7212c90bc0a9a4f10efaf0e6417bf..ddf5e554389502a79ddaf7bf5842a188bb14e541 100644
--- a/third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp
+++ b/third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp
@@ -12,16 +12,14 @@ namespace blink {
 
 HTMLSlotElement* SlotScopedTraversal::findScopeOwnerSlot(
     const Element& current) {
-  if (Element* nearestAncestorAssignedToSlot =
-          SlotScopedTraversal::nearestAncestorAssignedToSlot(current))
-    return nearestAncestorAssignedToSlot->assignedSlot();
+  if (Element* slottedAncestor =
+          SlotScopedTraversal::nearestInclusiveAncestorAssignedToSlot(current))
+    return slottedAncestor->assignedSlot();
   return nullptr;
 }
 
-Element* SlotScopedTraversal::nearestAncestorAssignedToSlot(
+Element* SlotScopedTraversal::nearestInclusiveAncestorAssignedToSlot(
     const Element& current) {
-  // nearestAncestorAssignedToSlot returns an ancestor element of current which
-  // is directly assigned to a slot.
   Element* element = const_cast<Element*>(&current);
   for (; element; element = element->parentElement()) {
     if (element->assignedSlot())
@@ -31,27 +29,32 @@ Element* SlotScopedTraversal::nearestAncestorAssignedToSlot(
 }
 
 Element* SlotScopedTraversal::next(const Element& current) {
-  // current.assignedSlot returns a slot only when current is assigned
-  // explicitly
-  // If current is assigned to a slot, return a descendant of current, which is
-  // in the assigned scope of the same slot as current.
-  HTMLSlotElement* slot = current.assignedSlot();
-  Element* nearestAncestorAssignedToSlot =
-      SlotScopedTraversal::nearestAncestorAssignedToSlot(current);
-  if (slot) {
-    if (Element* next = ElementTraversal::next(current, &current))
-      return next;
+  Element* next = nullptr;
+  Element* slottedAncestor =

[hayato, 2016/10/26 04:24:12]

Could you avoid to use "Ancestor" here?
It is confusing because we are likely to think that this is exclusive of
|current|.

[kochi, 2016/10/26 10:20:03]

Does |slottedInclusiveAncestor| sound okay then?
I didn't come up with a better name - so reverted to
|nearestInclusiveAncestorAssignedToSlot|.

+      SlotScopedTraversal::nearestInclusiveAncestorAssignedToSlot(current);
+  // Search within children of an element which is assigned to a slot.
+  if (current.assignedSlot()) {

[hayato, 2016/10/26 04:24:12]

This "if .. else .." looks redundant.

We can use ElementTraversal::next(current, slottedAncestor) in either case.

Also, we can merge this into the bellow "while" loop as "for" loop.

e.g.
for (next = ElementTraversal::next(..., ...); next; next =
ElementTraversal::nextSkippingChildren(...)) {
  if (!next->assignedSlot())
    return next;
}

[kochi, 2016/10/26 10:20:03]

Done.
Really good catch!

+    next = ElementTraversal::next(current, &current);
   } else {
     // If current is in assigned scope, find an assigned ancestor.
-    DCHECK(nearestAncestorAssignedToSlot);
-    if (Element* next =
-            ElementTraversal::next(current, nearestAncestorAssignedToSlot))
-      return next;
-    slot = nearestAncestorAssignedToSlot->assignedSlot();
-    DCHECK(slot);
+    DCHECK(slottedAncestor);

[hayato, 2016/10/26 04:24:12]

We should DCHECK immediately after L34.

[kochi, 2016/10/26 10:20:03]

Done.

+    next = ElementTraversal::next(current, slottedAncestor);
   }
+
+  // Exclude elements that is assigned to another slot scope.
+  while (next && next->assignedSlot())
+    next = ElementTraversal::nextSkippingChildren(*next, &current);
+
+  if (next) {
+    DCHECK(!next->assignedSlot());

[hayato, 2016/10/26 04:24:12]

Nit: I would remove this DCHECK because it is just repeating the condition of
the above "while" loop.

[kochi, 2016/10/26 10:20:03]

Done.

+    return next;
+  }
+
+  // Seek to the next element assigned to the same slot.
+  HTMLSlotElement* slot = slottedAncestor->assignedSlot();
+  DCHECK(slot);
   HeapVector<Member<Node>> assignedNodes = slot->assignedNodes();
-  size_t currentIndex = assignedNodes.find(*nearestAncestorAssignedToSlot);
+  size_t currentIndex = assignedNodes.find(*slottedAncestor);
   DCHECK_NE(currentIndex, kNotFound);
   for (++currentIndex; currentIndex < assignedNodes.size(); ++currentIndex) {
     if (assignedNodes[currentIndex]->isElementNode())
@@ -60,34 +63,95 @@ Element* SlotScopedTraversal::next(const Element& current) {
   return nullptr;
 }
 
+namespace {
+
+Element* findPreviousSkippingShadowHost(Node* start,
+                                        const Element& scope,
+                                        const Element* expected) {
+  DCHECK(!isShadowHost(start));
+  Node* current = start;
+  while (current != expected) {
+    current = NodeTraversal::previousPostOrder(*current, &scope);
+    DCHECK(current);
+    if (!current->isElementNode())
+      continue;
+    if (isShadowHost(current))
+      break;
+  }
+  return toElement(current);
+}
+
+Element* previousInternal(const Element& current, const Element& scope) {
+  DCHECK(scope.assignedSlot());
+  DCHECK_NE(&current, &scope);
+  Element* previous = ElementTraversal::previous(current, &scope);
+  if (isShadowHost(current))
+    return previous;
+  return findPreviousSkippingShadowHost(
+      const_cast<Node*>(static_cast<const Node*>(&current)), scope, previous);
+}
+
+Element* lastWithinOrSelfInternal(const Element& scope) {
+  DCHECK(scope.assignedSlot());
+  // Shortcut for Shadow Host. Do not traverse in its light DOM.
+  if (isShadowHost(scope))
+    return const_cast<Element*>(&scope);
+  Element* last = ElementTraversal::lastWithin(scope);
+  if (!last || last == scope)
+    return const_cast<Element*>(&scope);
+  return findPreviousSkippingShadowHost(scope.lastChild(), scope, last);
+}
+
+}  // namespace
+
 Element* SlotScopedTraversal::previous(const Element& current) {
-  Element* nearestAncestorAssignedToSlot =
-      SlotScopedTraversal::nearestAncestorAssignedToSlot(current);
-  DCHECK(nearestAncestorAssignedToSlot);
-  // NodeTraversal within nearestAncestorAssignedToSlot
-  if (Element* previous =
-          ElementTraversal::previous(current, nearestAncestorAssignedToSlot))
+  Element* slottedAncestor =
+      SlotScopedTraversal::nearestInclusiveAncestorAssignedToSlot(current);
+  DCHECK(slottedAncestor);
+
+  if (current != slottedAncestor) {
+    // Search within children of an element which is assigned to a slot.
+    Element* previous = previousInternal(current, *slottedAncestor);
+    DCHECK(previous);
     return previous;
-  // If null, jump to previous assigned node's descendant
+  }
+
+  // Seek to the previous element assigned to the same slot.
   const HeapVector<Member<Node>> assignedNodes =
-      nearestAncestorAssignedToSlot->assignedSlot()->assignedNodes();
-  size_t currentIndex =
-      assignedNodes.reverseFind(*nearestAncestorAssignedToSlot);
+      slottedAncestor->assignedSlot()->assignedNodes();
+  size_t currentIndex = assignedNodes.reverseFind(*slottedAncestor);
   DCHECK_NE(currentIndex, kNotFound);
   for (; currentIndex > 0; --currentIndex) {
-    const Member<Node> assignedPrevious = assignedNodes[currentIndex - 1];
-    if (assignedPrevious->isElementNode()) {
-      if (Element* last =
-              ElementTraversal::lastWithin(*toElement(assignedPrevious)))
-        return last;
-      return toElement(assignedPrevious);
-    }
+    const Member<Node> assignedNode = assignedNodes[currentIndex - 1];
+    if (!assignedNode->isElementNode())
+      continue;
+    return lastWithinOrSelfInternal(*toElement(assignedNode));
+  }
+  return nullptr;
+}
+
+Element* SlotScopedTraversal::firstAssignedToSlot(HTMLSlotElement& slot) {
+  HeapVector<Member<Node>> assignedNodes = slot.assignedNodes();
+  for (auto assignedNode : assignedNodes) {
+    if (assignedNode->isElementNode())
+      return toElement(assignedNode);
+  }
+  return nullptr;
+}
+
+Element* SlotScopedTraversal::lastAssignedToSlot(HTMLSlotElement& slot) {
+  HeapVector<Member<Node>> assignedNodes = slot.assignedNodes();
+  for (auto assignedNode = assignedNodes.rbegin();
+       assignedNode != assignedNodes.rend(); ++assignedNode) {
+    if (!(*assignedNode)->isElementNode())
+      continue;
+    return lastWithinOrSelfInternal(*toElement(*assignedNode));
   }
   return nullptr;
 }
 
 bool SlotScopedTraversal::isSlotScoped(const Element& current) {
-  return SlotScopedTraversal::nearestAncestorAssignedToSlot(current);
+  return SlotScopedTraversal::nearestInclusiveAncestorAssignedToSlot(current);
 }
 
 }  // namespace blink