Move asserts in FocusNavigationController.

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 42e6f3d80c8630ca756b7d8216f3c31352b41323..4cf75560954ed0a8b0d6aaaa1ac6d94cfc4dcab7 100644
--- a/third_party/WebKit/Source/core/page/FocusController.cpp
+++ b/third_party/WebKit/Source/core/page/FocusController.cpp
@@ -364,13 +364,6 @@ inline bool isShadowHostWithoutCustomFocusLogic(const Element& element)
     return isShadowHost(element) && !hasCustomFocusLogic(element);
 }
 
-#if ENABLE(ASSERT)
-inline bool isNonFocusableShadowHost(const Element& element)
-{
-    return isShadowHostWithoutCustomFocusLogic(element) && !element.isFocusable();
-}
-#endif
-
 inline bool isNonKeyboardFocusableShadowHost(const Element& element)
 {
     return isShadowHostWithoutCustomFocusLogic(element) && !(element.shadowRootIfV1() ? element.isFocusable() : element.isKeyboardFocusable());
@@ -646,7 +639,6 @@ Element* findFocusableElementDescendingDownIntoFrameDocument(WebFocusType type,
 Element* findFocusableElementAcrossFocusScopesForward(ScopedFocusNavigation& scope)
 {
     Element* current = scope.currentElement();
-    ASSERT(!current || !isNonFocusableShadowHost(*current));

[hayato, 2016/05/10 07:58:15]

Could you help me to understand how this ASSERT hits in the test?

It is not good to delete this ASSERT due to the feature which was introduced
recently.
Is there any way to preserve this ASSERT? Can we distinguish a special case and
preserve this ASSERT if we are not in a special case?

[kochi, 2016/05/10 09:49:51]

This assertion is, if de morgan's law is applied:
ASSERT(!(current && isNonFocusableShadowHost(*current)))

and the condition can be expanded to:
!(current && isShadowHost(current) && !hasCustomFocusLogic(current) &&
!current.isFocusable())

thus, this was intended to catch "current is not null, is a shadow host,
doesn't have custom focus logic, and is not focusable" condition.

All the 4 cases have to be met, but what was significant here was
"if shadow host is passed and it is not focusable".

A shadow host usually forms "scope" (inside its shadow root), and
shadow host should not be directly passed down to this function,
instead a scope with its shadow root as its root should be passed.
The case when a shadow host itself is passed down to this function
is when it had focus.

After sequential focus navigation starting point was introduced,
any node can be the starting point.  But about the same time,
a refactoring to restrict any starting point to Element (not Node)
was done.  Thus sequential focus navigation starting point introduced
a hack to return neighboring element to the exact node, depending on
the direction, e.g. if direction is forward, previous element is returned
as the starting point.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Therefore any Element which did not have focus can be passed as
a starting point, and it can be an element that meets all the
4 conditions above.  On material-design chrome:// pages it
is easy to hit the condition as it uses Polymer with native
Shadow DOM (thus it was reported from internal, not from the wild web).

I think the right solution is to allow any Node (not Element) to
be a starting point and remove the neighboring hack of sequential
focus navigation starting point.

     Element* found;
     if (current && isShadowHostWithoutCustomFocusLogic(*current)) {
         ScopedFocusNavigation innerScope = ScopedFocusNavigation::ownedByShadowHost(*current);
@@ -670,7 +662,6 @@ Element* findFocusableElementAcrossFocusScopesForward(ScopedFocusNavigation& sco
 
 Element* findFocusableElementAcrossFocusScopesBackward(ScopedFocusNavigation& scope)
 {
-    ASSERT(!scope.currentElement() || !isNonFocusableShadowHost(*scope.currentElement()));
     Element* found = findFocusableElementRecursivelyBackward(scope);
 
     // If there's no focusable element to advance to, move up the focus scopes until we find one.