Move asserts in FocusNavigationController.

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

 /*
  * Copyright (C) 2006, 2007 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nuanti Ltd.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 346 matching lines @@
 inline bool hasCustomFocusLogic(const Element& element)
 {
     return element.isHTMLElement() && toHTMLElement(element).hasCustomFocusLogic();
 }
 
 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());
 }
 
 inline bool isKeyboardFocusableShadowHost(const Element& element)
 {
     return isShadowHostWithoutCustomFocusLogic(element) && element.isKeyboardFocusable();
 }
 
@@ skipping 255 matching lines @@
             break;
         ASSERT(element != foundElement);
         element = foundElement;
     }
     return element;
 }
 
 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);
         Element* foundInInnerFocusScope = findFocusableElementRecursivelyForward(innerScope);
         found = foundInInnerFocusScope ? foundInInnerFocusScope : findFocusableElementRecursivelyForward(scope);
     } else {
         found = findFocusableElementRecursivelyForward(scope);
     }
 
     // If there's no focusable element to advance to, move up the focus scopes until we find one.
     ScopedFocusNavigation currentScope = scope;
     while (!found) {
         Element* owner = currentScope.owner();
         if (!owner)
             break;
         currentScope = ScopedFocusNavigation::createFor(*owner);
         found = findFocusableElementRecursivelyForward(currentScope);
     }
     return findFocusableElementDescendingDownIntoFrameDocument(WebFocusTypeForward, found);
 }
 
 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.
     ScopedFocusNavigation currentScope = scope;
     while (!found) {
         Element* owner = currentScope.owner();
         if (!owner)
             break;
         currentScope = ScopedFocusNavigation::createFor(*owner);
         if (isKeyboardFocusableShadowHost(*owner) && !isShadowHostDelegatesFocus(*owner)) {
@@ skipping 622 matching lines @@
     return consumed;
 }
 
 DEFINE_TRACE(FocusController)
 {
     visitor->trace(m_page);
     visitor->trace(m_focusedFrame);
 }
 
 } // namespace blink