Consider slots as a focus scope

third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp

+// Copyright 2015 The Chromium Authors. All rights reserved.

[kochi, 2016/03/01 12:55:40]

2016?

[yuzuchan, 2016/03/02 09:02:14]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "core/dom/AssignedElementTraversal.h"
+
+#include "core/dom/Element.h"
+#include "core/dom/ElementTraversal.h"
+#include "core/html/HTMLSlotElement.h"
+
+namespace blink {
+
+HTMLSlotElement* AssignedElementTraversal::slot(const Element& current)
+{
+    if (Element* assignedAncestor = AssignedElementTraversal::assignedAncestor(current)) {
+        return assignedAncestor->assignedSlot();
+    }
+    return nullptr;
+}
+
+Element* AssignedElementTraversal::assignedAncestor(const Element& current)
+{
+    // assignedAncestor returns an ancestor element of current which is directly assigned to a slot.

[kochi, 2016/03/01 12:55:40]

Maybe isChildOfV1ShadowHost() is enough for this purpose (instead of using
assignedSlot())?

[yuzuchan, 2016/03/02 09:02:14]

isChildOfV1ShadowHost() returns true even if the node is not assigned to any
slot.
Since this method returns an ancestor element which is assigned to a slot,
isChildOfV1ShadowHost() is not enough here.

[kochi, 2016/03/02 13:26:56]

Acknowledged.

+    Element* element = const_cast<Element*>(&current);

[kochi, 2016/03/01 12:55:40]

Is const_cast<> really necessary?

[yuzuchan, 2016/03/02 09:02:14]

Yes, because I update element in the for loop below.

[kochi, 2016/03/02 13:26:56]

That is not the reason.
You can write

const Element* element = &current;

then you can update element below, like element = element->parentElement();.
What you cannot update is the content of element.

that said, you end up in returning element on line 32,
and that line require const_cast<> anyway.

So it doesn't make much difference in the end :(

[yuzuchan, 2016/03/03 04:12:42]

Acknowledged.

+    while (!element->assignedSlot()) {
+        if (element->parentElement()) {
+            element = element->parentElement();
+        } else {
+            return nullptr;
+        }
+    }
+    return element;

[kochi, 2016/03/01 12:55:40]

This can be written as:

for (Element* element = const_cast<Element*>(&current); element; element =
element->parentElement()) {
    if (element->assignedSlot())
        break;
}
return element;

[yuzuchan, 2016/03/02 09:02:14]

I kept Element* element = const_cast<Element*>(&current); in the first line of
this method, since I would not be able to return element if I put it in for loop
initialization.

[kochi, 2016/03/02 13:26:56]

I see your point... but

Element* element = const_cast<Element*>(&current);
for (; element; element->parentElement()) {
    if (element->assignedSlot())
        break;
}
return element;

is shorter, isn't it?

or 

for (Element* element = const_cast<Element*>(&current); element; element =
element->parentElement()) {
    if (element->assignedSlot())
        return element;
}
return nullptr;

(you already done the former!)

[yuzuchan, 2016/03/03 04:12:42]

Acknowledged.

+}
+
+Element* AssignedElementTraversal::next(const Element& current)
+{
+    // current.assignedSlot returns a slot only when current is assigned explicitly
+    // if current is directly assigned to a slot, return a descendant of current, which is indirectly assigned to the same slot as current.
+    if (HTMLSlotElement* slot = current.assignedSlot()) {
+        if (Element* next = ElementTraversal::next(current, &current))
+            return next;
+        // if not, jump to another tree of assigned elements.
+        const WillBeHeapVector<RefPtrWillBeMember<Node>> assignedNodes = slot->getAssignedNodes();
+        for (size_t i = 0; i < assignedNodes.size()-1; ++i) {

[kochi, 2016/03/01 12:55:40]

Vector<> has find() method, though you should take care of the '-1' part
yourself.
nit: space around '-'.

[yuzuchan, 2016/03/02 09:02:14]

Done.

+            if (current == toElement(assignedNodes[i].get()))

[kochi, 2016/03/01 12:55:40]

nit: is toElement() necessary here?

(because this is comparing const Element& and Node* ?)

[yuzuchan, 2016/03/02 09:02:14]

Yes exactly.

+                return toElement(assignedNodes[i+1].get());

[kochi, 2016/03/01 12:55:40]

nit: space around '+'.

[yuzuchan, 2016/03/02 09:02:14]

Done.

+        }
+        return nullptr;
+    }
+    // if current is indirectly assigned to a slot, find a directly-assigned ancestor.
+    Element* assignedAncestor = AssignedElementTraversal::assignedAncestor(current);
+    if (Element* next = ElementTraversal::next(current, assignedAncestor))
+        return next;
+    const WillBeHeapVector<RefPtrWillBeMember<Node>> assignedNodes = AssignedElementTraversal::slot(*assignedAncestor)->getAssignedNodes();
+    for (size_t i = 0; i < assignedNodes.size()-1; ++i) {

[kochi, 2016/03/01 12:55:40]

This is almost same as line 44-.
Probably you can factor out this as a function (or class method) as
nextAssignedElement(slot, current)?

[yuzuchan, 2016/03/02 09:02:14]

I refactored this part of code so that find method is written only once. PTAL of
an updated version of this CL.

+        if (assignedAncestor == toElement(assignedNodes[i].get()))
+            return toElement(assignedNodes[i+1].get());
+    }
+    return nullptr;
+}
+
+Element* AssignedElementTraversal::previous(const Element& current)
+{
+    Element* assignedAncestor = AssignedElementTraversal::assignedAncestor(current);
+    // NodeTraversal within assignedAncestor
+    if (Element* previous = ElementTraversal::previous(current, assignedAncestor)) {
+        return previous;
+    }

[kochi, 2016/03/01 12:55:40]

nit: if the body is one-liner, omit {}.

[yuzuchan, 2016/03/02 09:02:14]

Done.

+    // if null, jump to previous assigned node's descendant
+    const WillBeHeapVector<RefPtrWillBeMember<Node>> assignedNodes = AssignedElementTraversal::slot(*assignedAncestor)->getAssignedNodes();
+    for (size_t i = assignedNodes.size()-1; i > 0; --i) {

[kochi, 2016/03/01 12:55:40]

Vector<> also has reverseFind()

[yuzuchan, 2016/03/02 09:02:14]

Done.

+        if (assignedAncestor == toElement(assignedNodes[i].get())) {
+            if (Node* lastChild = assignedNodes[i-1]->lastChild())
+                return toElement(lastChild);
+            // if the previous assigned node has no children, return the assigned node itself
+            return toElement(assignedNodes[i-1].get());
+        }
+    }
+    return nullptr;
+}
+
+bool AssignedElementTraversal::isInAssignedScope(const Element& current)
+{
+    if (AssignedElementTraversal::assignedAncestor(current)) {
+        return true;
+    }
+    return false;

[kochi, 2016/03/01 12:55:40]

You can write this as
return !!AssignedElementTraversal::assignedAncestor(current);

[yuzuchan, 2016/03/02 09:02:14]

Done.

+}
+
+} // namespace blink