Blur immediately if an attribute change made an element unfocasable.

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

Index: third_party/WebKit/Source/core/dom/Element.cpp
diff --git a/third_party/WebKit/Source/core/dom/Element.cpp b/third_party/WebKit/Source/core/dom/Element.cpp
index 76f4f86ce67437d2eb5495dd860eca1c412c24a3..dd5fdd84bff768d6b2418e3592f0c3e68ff48727 100644
--- a/third_party/WebKit/Source/core/dom/Element.cpp
+++ b/third_party/WebKit/Source/core/dom/Element.cpp
@@ -1310,6 +1310,15 @@ void Element::attributeChanged(const QualifiedName& name,
     if (AXObjectCache* cache = document().existingAXObjectCache())
       cache->handleAttributeChanged(name, this);
   }
+
+  if (reason == AttributeModificationReason::kDirectly &&
+      name == tabindexAttr && adjustedFocusedElementInTreeScope() == this) {
+    // supportsFocus() behavior depends on element classes. It's hard to avoid
+    // to call it, and it needs up-to-date style.

[kochi, 2016/12/21 05:58:09]

nit: s/avoid to call/avoid calling/

Let me understand the comment correctly.
"depends on element classes" = supportsFocus() is virtual for
element C++ classes?  I'm not sure why "It's hard to avoid" from
the comment.

Does the below sound correct?
// The attribute change may cause supportsFocus() to return false
// for the element which had focus, because some elements use
// attributes and up-to-date style to determine focusability.

[tkent, 2016/12/21 06:51:15]

We don't call supportsFocus() and updateStyleAndLayoutTreeForNode() for |href|
and |disabled| attribute change because their behavior are fixed.
tabindex change won't affect style, so I'm unwilling to call
updateStyleAndLayoutTreeForNode().
This is also correct though it doesn't include the unwillingness of
updateStyleAndLayoutTreeForNode().
Your comment and a TODO() would be better?

// The attribute change may cause supportsFocus() to return false
// for the element which had focus, because some elements use
// attributes and up-to-date style to determine focusability.
// TODO(tkent): tabindex doesn't affect style. We should avoid to
// update style if possible.

[kochi, 2016/12/21 08:05:19]

Looks good.

+    document().updateStyleAndLayoutTreeForNode(this);
+    if (!supportsFocus())
+      blur();
+  }
 }
 
 bool Element::hasLegalLinkAttribute(const QualifiedName&) const {
@@ -1488,9 +1497,11 @@ void Element::parserSetAttributes(const Vector<Attribute>& attributeVector) {
 
   // Use attributeVector instead of m_elementData because attributeChanged might
   // modify m_elementData.
-  for (const auto& attribute : attributeVector)
-    attributeChangedFromParserOrByCloning(attribute.name(), attribute.value(),
-                                          ModifiedDirectly);
+  for (const auto& attribute : attributeVector) {
+    attributeChangedFromParserOrByCloning(
+        attribute.name(), attribute.value(),
+        AttributeModificationReason::kByParser);
+  }
 }
 
 bool Element::hasEquivalentAttributes(const Element* other) const {
@@ -2444,11 +2455,6 @@ void Element::parseAttribute(const QualifiedName& name,
     int tabindex = 0;
     if (value.isEmpty() || !parseHTMLInteger(value, tabindex)) {
       clearTabIndexExplicitlyIfNeeded();
-      if (adjustedFocusedElementInTreeScope() == this) {
-        // We might want to call blur(), but it's dangerous to dispatch
-        // events here.
-        document().setNeedsFocusedElementCheck();
-      }
     } else {
       // We only set when value is in integer range.
       setTabIndexExplicitly();
@@ -3527,7 +3533,8 @@ void Element::didAddAttribute(const QualifiedName& name,
                               const AtomicString& value) {
   if (name == HTMLNames::idAttr)
     updateId(nullAtom, value);
-  attributeChanged(name, nullAtom, value);
+  attributeChanged(name, nullAtom, value,
+                   AttributeModificationReason::kDirectly);
   InspectorInstrumentation::didModifyDOMAttr(this, name, value);
   dispatchSubtreeModifiedEvent();
 }
@@ -3537,7 +3544,8 @@ void Element::didModifyAttribute(const QualifiedName& name,
                                  const AtomicString& newValue) {
   if (name == HTMLNames::idAttr)
     updateId(oldValue, newValue);
-  attributeChanged(name, oldValue, newValue);
+  attributeChanged(name, oldValue, newValue,
+                   AttributeModificationReason::kDirectly);
   InspectorInstrumentation::didModifyDOMAttr(this, name, newValue);
   // Do not dispatch a DOMSubtreeModified event here; see bug 81141.
 }
@@ -3546,7 +3554,8 @@ void Element::didRemoveAttribute(const QualifiedName& name,
                                  const AtomicString& oldValue) {
   if (name == HTMLNames::idAttr)
     updateId(oldValue, nullAtom);
-  attributeChanged(name, oldValue, nullAtom);
+  attributeChanged(name, oldValue, nullAtom,
+                   AttributeModificationReason::kDirectly);
   InspectorInstrumentation::didRemoveDOMAttr(this, name);
   dispatchSubtreeModifiedEvent();
 }
@@ -3743,9 +3752,10 @@ void Element::cloneAttributesFromElement(const Element& other) {
     m_elementData = other.m_elementData->makeUniqueCopy();
 
   AttributeCollection attributes = m_elementData->attributes();
-  for (const Attribute& attr : attributes)
-    attributeChangedFromParserOrByCloning(attr.name(), attr.value(),
-                                          ModifiedByCloning);
+  for (const Attribute& attr : attributes) {
+    attributeChangedFromParserOrByCloning(
+        attr.name(), attr.value(), AttributeModificationReason::kByCloning);
+  }
 }
 
 void Element::cloneDataFromElement(const Element& other) {
@@ -3840,7 +3850,7 @@ void Element::styleAttributeChanged(
 
   if (newStyleString.isNull()) {
     ensureUniqueElementData().m_inlineStyle.clear();
-  } else if (modificationReason == ModifiedByCloning ||
+  } else if (modificationReason == AttributeModificationReason::kByCloning ||
              ContentSecurityPolicy::shouldBypassMainWorld(&document()) ||
              (containingShadowRoot() &&
               containingShadowRoot()->type() == ShadowRootType::UserAgent) ||