Support slots' fallback contents in focus navigation

third_party/WebKit/Source/core/page/FocusController.cpp

Index: third_party/WebKit/Source/core/page/FocusController.cpp
diff --git a/third_party/WebKit/Source/core/page/FocusController.cpp b/third_party/WebKit/Source/core/page/FocusController.cpp
index 096bdb946d1ec80162dd6c2c1b9d2858ce6b8144..c75d051f98d34e411319a8dad847364804fcf015 100644
--- a/third_party/WebKit/Source/core/page/FocusController.cpp
+++ b/third_party/WebKit/Source/core/page/FocusController.cpp
@@ -95,6 +95,7 @@ private:
     RawPtrWillBeMember<ContainerNode> m_rootNode;
     RawPtrWillBeMember<HTMLSlotElement> m_rootSlot;
     RawPtrWillBeMember<Element> m_current;
+    bool m_slotFallbackTraversal;
 };
 
 ScopedFocusNavigation::ScopedFocusNavigation(TreeScope& treeScope, const Element* current)
@@ -108,6 +109,7 @@ ScopedFocusNavigation::ScopedFocusNavigation(HTMLSlotElement& slot, const Elemen
     : m_rootNode(nullptr)
     , m_rootSlot(&slot)
     , m_current(const_cast<Element*>(current))
+    , m_slotFallbackTraversal(slot.getAssignedNodes().isEmpty())
 {
 }
 
@@ -125,10 +127,16 @@ void ScopedFocusNavigation::moveToNext()
 {
     ASSERT(m_current);
     if (m_rootSlot) {
-        m_current = SlotScopedTraversal::next(*m_current);
+        if (m_slotFallbackTraversal) {

[hayato, 2016/03/31 09:45:02]

I am afraid that the responsibilities of SlotScopedTraversal and
ScopedFocusNavigation are getting unclear.


- Now SlotScopedTraversal has findFallbackScopeOwnerSlot().
- But ScopedFocusNavagion::moveToNext is being updated here, instead of updating
ScopedFocusTraversal::next().

If SlotScopedTraversal is only used for traversing *assigned nodes* (and its
descendants),
it would be better to avoid to define findFallbackScopeOwnerSlot() in
SlotScopedTraversal.

Instead, findFallbackScopeOwnerSlot() should be defined in ScopedFocusNavigation
to make the responsibility clear.

[yuzuchan, 2016/04/01 04:05:13]

Thanks for the advice, actually I was not sure where to put the functions as
well.
I moved them to ScopedFocusNavigation.

+            m_current = ElementTraversal::next(*m_current, m_rootSlot);
+            while (m_current && (SlotScopedTraversal::isSlotScoped(*m_current) || SlotScopedTraversal::findFallbackScopeOwnerSlot(*m_current) != m_rootSlot))

[hayato, 2016/03/31 09:45:02]

It looks we have several similar patterns here.
Could you extract this pattern as an utility function?

e.g.
bool ScopedFocusNavigation::isFallbackContent(HTMLSlotElement& slot, Element&
current) {
  // Implement this.
}

Then, the loop would become more readable:

while (m_current && !ScopedFocusNavigation::isFallbackContent(*m_rootSlot,
*m_current)) {
    m_current = ElementTraversal::next(*m_current, m_rootSlot);
}

[yuzuchan, 2016/04/01 04:05:13]

I created a utility function as you suggested.
However, isSlotScoped || isFallbackContentForAnotherSlot does not necessarily
mean that it is not "isFallbackContent",
so I added several lines to make sure that the input element is a fallback
content.

+                m_current = ElementTraversal::next(*m_current, m_rootSlot);
+        } else {
+            m_current = SlotScopedTraversal::next(*m_current);
+        }
     } else {
         m_current = ElementTraversal::next(*m_current);
-        while (m_current && SlotScopedTraversal::isSlotScoped(*m_current))
+        while (m_current && (SlotScopedTraversal::isSlotScoped(*m_current) || SlotScopedTraversal::isSlotFallbackScoped(*m_current)))
             m_current = ElementTraversal::next(*m_current);
     }
 }
@@ -137,10 +145,18 @@ void ScopedFocusNavigation::moveToPrevious()
 {
     ASSERT(m_current);
     if (m_rootSlot) {
-        m_current = SlotScopedTraversal::previous(*m_current);
+        if (m_slotFallbackTraversal) {
+            m_current = ElementTraversal::previous(*m_current, m_rootSlot);
+            if (m_current == m_rootSlot)
+                m_current = nullptr;
+            while (m_current && (SlotScopedTraversal::isSlotScoped(*m_current) || SlotScopedTraversal::findFallbackScopeOwnerSlot(*m_current) != m_rootSlot))
+                m_current = ElementTraversal::previous(*m_current);
+        } else {
+            m_current = SlotScopedTraversal::previous(*m_current);
+        }
     } else {
         m_current = ElementTraversal::previous(*m_current);
-        while (m_current && SlotScopedTraversal::isSlotScoped(*m_current))
+        while (m_current && (SlotScopedTraversal::isSlotScoped(*m_current) || SlotScopedTraversal::isSlotFallbackScoped(*m_current)))
             m_current = ElementTraversal::previous(*m_current);
     }
 }
@@ -148,7 +164,7 @@ void ScopedFocusNavigation::moveToPrevious()
 void ScopedFocusNavigation::moveToFirst()
 {
     if (m_rootSlot) {
-        if (!m_rootSlot->getAssignedNodes().isEmpty()) {
+        if (!m_slotFallbackTraversal) {
             WillBeHeapVector<RefPtrWillBeMember<Node>> assignedNodes = m_rootSlot->getAssignedNodes();
             for (auto assignedNode : assignedNodes) {
                 if (assignedNode->isElementNode()) {
@@ -157,20 +173,24 @@ void ScopedFocusNavigation::moveToFirst()
                 }
             }
         } else {
-            m_current = nullptr;
+            Element* first = ElementTraversal::firstChild(*m_rootSlot);
+            while (first && (SlotScopedTraversal::isSlotScoped(*first) || SlotScopedTraversal::findFallbackScopeOwnerSlot(*first) != m_rootSlot))
+                first = ElementTraversal::next(*first, m_rootSlot);
+            m_current = first;
         }
     } else {
         Element* first = m_rootNode->isElementNode() ? &toElement(*m_rootNode) : ElementTraversal::next(*m_rootNode);
-        while (first && SlotScopedTraversal::isSlotScoped(*first))
+        while (first && (SlotScopedTraversal::isSlotScoped(*first) || SlotScopedTraversal::isSlotFallbackScoped(*first)))
             first = ElementTraversal::next(*first, m_rootNode);
         m_current = first;
     }
+
 }
 
 void ScopedFocusNavigation::moveToLast()
 {
     if (m_rootSlot) {
-        if (!m_rootSlot->getAssignedNodes().isEmpty()) {
+        if (!m_slotFallbackTraversal) {
             WillBeHeapVector<RefPtrWillBeMember<Node>> assignedNodes = m_rootSlot->getAssignedNodes();
             for (auto assignedNode = assignedNodes.rbegin(); assignedNode != assignedNodes.rend(); ++assignedNode) {
                 if ((*assignedNode)->isElementNode()) {
@@ -179,11 +199,14 @@ void ScopedFocusNavigation::moveToLast()
                 }
             }
         } else {
-            m_current = nullptr;
+            Element* last = ElementTraversal::lastWithin(*m_rootSlot);
+            while (last && (SlotScopedTraversal::isSlotScoped(*last) || SlotScopedTraversal::findFallbackScopeOwnerSlot(*last) != m_rootSlot))
+                last = ElementTraversal::previous(*last, m_rootSlot);
+            m_current = last;
         }
     } else {
         Element* last = ElementTraversal::lastWithin(*m_rootNode);
-        while (last && SlotScopedTraversal::isSlotScoped(*last))
+        while (last && (SlotScopedTraversal::isSlotScoped(*last) || SlotScopedTraversal::isSlotFallbackScoped(*last)))
             last = ElementTraversal::previous(*last, m_rootNode);
         m_current = last;
     }
@@ -193,6 +216,7 @@ Element* ScopedFocusNavigation::owner() const
 {
     if (m_rootSlot)
         return m_rootSlot;
+    ASSERT(m_rootNode);
     if (m_rootNode->isShadowRoot()) {
         ShadowRoot& shadowRoot = toShadowRoot(*m_rootNode);
         return shadowRoot.isYoungest() ? shadowRoot.host() : shadowRoot.shadowInsertionPointOfYoungerShadowRoot();
@@ -207,6 +231,8 @@ ScopedFocusNavigation ScopedFocusNavigation::createFor(const Element& current)
 {
     if (SlotScopedTraversal::isSlotScoped(current))
         return ScopedFocusNavigation(*SlotScopedTraversal::findScopeOwnerSlot(current), &current);
+    if (SlotScopedTraversal::isSlotFallbackScoped(current))

[hayato, 2016/03/31 09:45:02]

It looks a little wasteful to do the same check twice here.

This can be:

if (HTMLSlotElement* slot =
SlotScopedTraversal::findFallbackScopeOwnerSlot(current))
  return ScopedFocusNavigation(*slot, &current);

Can you do the similar thing to line 232-234?

[yuzuchan, 2016/04/01 04:05:13]

Done.

+        return ScopedFocusNavigation(*SlotScopedTraversal::findFallbackScopeOwnerSlot(current), &current);
     return ScopedFocusNavigation(current.treeScope(), &current);
 }