Fix focus navigation for nested slot case

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/dom/shadow/SlotScopedTraversal.h"
 
 #include "core/dom/Element.h"
 #include "core/dom/ElementTraversal.h"
 #include "core/html/HTMLSlotElement.h"
 
 namespace blink {
 
-HTMLSlotElement* SlotScopedTraversal::findScopeOwnerSlot(
-    const Element& current) {
-  if (Element* nearestAncestorAssignedToSlot =
-          SlotScopedTraversal::nearestAncestorAssignedToSlot(current))
-    return nearestAncestorAssignedToSlot->assignedSlot();
+namespace {
+Element* nextSkippingShadowHost(const Element& start, const Element& scope) {

[hayato, 2016/10/31 05:27:12]

This name is confusing because it is inconsistent with the usage of
ElementTraversal::nextSkippingChildren.
It looks that this does not skip a shadow host. This could return a shadow host.

[kochi, 2016/10/31 07:30:25]

All renamed to *SkippingChildrenOfShadowHost().

+  DCHECK(scope.assignedSlot());
+  const Element* current = &start;

[hayato, 2016/10/31 05:27:12]

It does not need to declare |current| here. It is confusing.
Declare |current| and assign |start| to it before L22, using |for| loop.

e.g. 
for (const Element* current = &start; current != scope; current =
current->parentElement() ) {
  ...
}

[kochi, 2016/10/31 07:30:25]

Done.

+  if (!current->authorShadowRoot()) {
+    if (Element* first = ElementTraversal::firstChild(*current))
+      return first;
+  }
+
+  while (current != scope) {
+    if (Element* nextSibling = ElementTraversal::nextSibling(*current))
+      return nextSibling;
+    current = current->parentElement();
+  }
   return nullptr;
 }
 
-Element* SlotScopedTraversal::nearestAncestorAssignedToSlot(
+Element* lastWithinOrSelfSkippingShadowHost(const Element& scope) {
+  Element* current = const_cast<Element*>(&scope);
+  while (!current->authorShadowRoot()) {

[hayato, 2016/10/31 05:27:12]

You can rewrite this with for loop.

[kochi, 2016/10/31 07:30:25]

I'm not sure rewriting with for loop improves readability for this case:

The shortest one (duplicate ElementTraversal::lastChild() calls):

Element* lastWithinOrSelfSkippingChildrenOfShadowHost(const Element& scope) {
  Element* current = const_cast<Element*>(&scope);
  for (; !current->authorShadowRoot() && ElementTraversal::lastChild(*current);
       current = ElementTraversal::lastChild(*current));
  return current;
}

Or (still redundant nullptr initialization for lastChild):

Element* lastWithinOrSelfSkippingChildrenOfShadowHost(const Element& scope) {
  Element* current = const_cast<Element*>(&scope);
  for (Element* lastChild = nullptr; !current->authorShadowRoot();
       current = lastChild) {
    lastChild = ElementTraversal::lastChild(*current);                          
    if (!lastChild)
      break;
  }
  return current;
}

?
Do you have any particular idea?

[hayato, 2016/10/31 08:12:00]

I see. Thanks. It is okay to leave this as is because it wouldn't get better.

+    Element* lastChild = ElementTraversal::lastChild(*current);
+    if (!lastChild)
+      break;
+    current = lastChild;
+  }
+  return current;
+}
+
+Element* previousSkippingShadowHost(const Element& start,
+                                    const Element& scope) {
+  DCHECK(scope.assignedSlot());
+  DCHECK_NE(start, &scope);
+  if (Element* previousSibling = ElementTraversal::previousSibling(start))
+    return lastWithinOrSelfSkippingShadowHost(*previousSibling);
+  return start.parentElement();
+}
+}  // namespace
+
+HTMLSlotElement* SlotScopedTraversal::findScopeOwnerSlot(
     const Element& current) {
-  // nearestAncestorAssignedToSlot returns an ancestor element of current which
-  // is directly assigned to a slot.
+  if (Element* nearestInclusiveAncestorAssignedToSlot =
+          SlotScopedTraversal::nearestInclusiveAncestorAssignedToSlot(current))
+    return nearestInclusiveAncestorAssignedToSlot->assignedSlot();
+  return nullptr;
+}
+
+Element* SlotScopedTraversal::nearestInclusiveAncestorAssignedToSlot(
+    const Element& current) {
   Element* element = const_cast<Element*>(&current);
   for (; element; element = element->parentElement()) {
     if (element->assignedSlot())
       break;
   }
   return element;
 }
 
 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;
-  } 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);
-  }
+  Element* nearestInclusiveAncestorAssignedToSlot =
+      SlotScopedTraversal::nearestInclusiveAncestorAssignedToSlot(current);
+  DCHECK(nearestInclusiveAncestorAssignedToSlot);
+  // Search within children of an element which is assigned to a slot.
+  if (Element* next = nextSkippingShadowHost(
+          current, *nearestInclusiveAncestorAssignedToSlot))
+    return next;
+
+  // Seek to the next element assigned to the same slot.
+  HTMLSlotElement* slot =
+      nearestInclusiveAncestorAssignedToSlot->assignedSlot();
+  DCHECK(slot);
   HeapVector<Member<Node>> assignedNodes = slot->assignedNodes();
-  size_t currentIndex = assignedNodes.find(*nearestAncestorAssignedToSlot);
+  size_t currentIndex =
+      assignedNodes.find(*nearestInclusiveAncestorAssignedToSlot);
   DCHECK_NE(currentIndex, kNotFound);
   for (++currentIndex; currentIndex < assignedNodes.size(); ++currentIndex) {
     if (assignedNodes[currentIndex]->isElementNode())
       return toElement(assignedNodes[currentIndex]);
   }
   return nullptr;
 }
 
 Element* SlotScopedTraversal::previous(const Element& current) {
-  Element* nearestAncestorAssignedToSlot =
-      SlotScopedTraversal::nearestAncestorAssignedToSlot(current);
-  DCHECK(nearestAncestorAssignedToSlot);
-  // NodeTraversal within nearestAncestorAssignedToSlot
-  if (Element* previous =
-          ElementTraversal::previous(current, nearestAncestorAssignedToSlot))
+  Element* nearestInclusiveAncestorAssignedToSlot =
+      SlotScopedTraversal::nearestInclusiveAncestorAssignedToSlot(current);
+  DCHECK(nearestInclusiveAncestorAssignedToSlot);
+
+  if (current != nearestInclusiveAncestorAssignedToSlot) {
+    // Search within children of an element which is assigned to a slot.
+    Element* previous = previousSkippingShadowHost(
+        current, *nearestInclusiveAncestorAssignedToSlot);
+    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();
+      nearestInclusiveAncestorAssignedToSlot->assignedSlot()->assignedNodes();
   size_t currentIndex =
-      assignedNodes.reverseFind(*nearestAncestorAssignedToSlot);
+      assignedNodes.reverseFind(*nearestInclusiveAncestorAssignedToSlot);
   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 lastWithinOrSelfSkippingShadowHost(*toElement(assignedNode));
+  }
+  return nullptr;
+}
+
+Element* SlotScopedTraversal::firstAssignedToSlot(HTMLSlotElement& slot) {
+  HeapVector<Member<Node>> assignedNodes = slot.assignedNodes();

[hayato, 2016/10/31 05:27:13]

Use a reference. Unless that, vector is copied, I'm afraid.

[kochi, 2016/10/31 07:30:25]

Done.

+  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 lastWithinOrSelfSkippingShadowHost(*toElement(*assignedNode));

[hayato, 2016/10/31 05:27:13]

Hmm. This function name is confusing because it return an element whose assigned
slot is null.

   }
   return nullptr;
 }
 
 bool SlotScopedTraversal::isSlotScoped(const Element& current) {
-  return SlotScopedTraversal::nearestAncestorAssignedToSlot(current);
+  return SlotScopedTraversal::nearestInclusiveAncestorAssignedToSlot(current);
 }
 
 }  // namespace blink