Fix bug where form/fieldset :valid/:invalid won't be recalculated upon control's willValidate change

Source/core/html/HTMLFormControlElement.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007 Apple Inc. All rights reserved.
  *           (C) 2006 Alexey Proskuryakov (ap@nypop.com)
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
@@ skipping 250 matching lines @@
 {
     fieldSetAncestorsSetNeedsValidityCheck(insertionPoint);
     hideVisibleValidationMessage();
     m_hasValidationMessage = false;
     m_ancestorDisabledState = AncestorDisabledStateUnknown;
     m_dataListAncestorState = Unknown;
     HTMLElement::removedFrom(insertionPoint);
     FormAssociatedElement::removedFrom(insertionPoint);
 }
 
-void HTMLFormControlElement::willChangeForm()
+void HTMLFormControlElement::didChangeForm(HTMLFormElement* oldForm)
 {
-    formOwnerSetNeedsValidityCheck();
-    FormAssociatedElement::willChangeForm();
+    FormAssociatedElement::didChangeForm(oldForm);
+    if (oldForm)
+        oldForm->setNeedsValidityCheck(ElementRemoval, isValidElement());
+    formOwnerSetNeedsValidityCheck(ElementAddition, isValidElement());
 }
 
-void HTMLFormControlElement::didChangeForm()
-{
-    formOwnerSetNeedsValidityCheck();
-    FormAssociatedElement::didChangeForm();
-}
-
-void HTMLFormControlElement::formOwnerSetNeedsValidityCheck()
+void HTMLFormControlElement::formOwnerSetNeedsValidityCheck(ValidityChangeAction action, bool isValid)
 {
     HTMLFormElement* form = formOwner();
     if (form)
-        form->setNeedsValidityCheck();
+        form->setNeedsValidityCheck(action, isValid);
 }
 
 void HTMLFormControlElement::fieldSetAncestorsSetNeedsValidityCheck(Node* node)
 {
     if (!node)
         return;
     HTMLFieldSetElement* fieldSet = Traversal<HTMLFieldSetElement>::firstAncestorOrSelf(*node);
     HTMLFieldSetElement* lastFieldSet = 0;
     while (fieldSet) {
         lastFieldSet = fieldSet;
@@ skipping 105 matching lines @@
 {
     if (m_dataListAncestorState == Unknown) {
         if (Traversal<HTMLDataListElement>::firstAncestor(*this))
             m_dataListAncestorState = InsideDataList;
         else
             m_dataListAncestorState = NotInsideDataList;
     }
     return m_dataListAncestorState == NotInsideDataList && !isDisabledOrReadOnly();
 }
 
+bool HTMLFormControlElement::refreshWillValidate()
+{
+    bool newWillValidate = recalcWillValidate();
+    if (m_willValidateInitialized && m_willValidate == newWillValidate)
+        return false;
+    m_willValidateInitialized = true;
+    m_willValidate = newWillValidate;
+    // Use m_isValid, because isValidElement() calls valid(), which in turn can
+    // call willValidate() again.

[keishi, 2014/10/24 10:25:49]

Is this true? m_willValidateInitialized is set to true above and
recalcWillValidate will set m_dataListAncestorState so I don't think this
creates an infinite recursion.

[Bartek Nowierski, 2014/10/24 10:48:30]

It's a little more complex than that. Calling isValidElement() here will trigger
the m_isValid==valid() assert, because the above two line will change the
validity result.

So what I tried was calling isValidElement two lines above, but that one does
cause infinite recursion as you noted.

One way or the other, the statement that willValidate may be called is true.

[keishi, 2014/10/24 11:07:17]

I think using ElementAddition/Removal here when we should be using
ElementModified is bad.

The problem seems to be in HTMLFormControlElement::setNeedsValidityCheck.
formOwnerSetNeedsValidityCheck and fieldSetAncestorsSetNeedsValidityCheck should
be called from setNeedsValidityCheck() but it isn't. 
Inside HTMLFormControlElement::setNeedsValidityCheck,
formOwnerSetNeedsValidityCheck and fieldSetAncestorsSetNeedsValidityCheck should
be moved out of the if statement.

[Bartek Nowierski, 2014/10/24 14:47:16]

Done.

+    formOwnerSetNeedsValidityCheck(newWillValidate ? ElementAddition : ElementRemoval, m_isValid);
+    fieldSetAncestorsSetNeedsValidityCheck(parentNode());
+    setNeedsValidityCheck();
+    return true;
+}
+
 bool HTMLFormControlElement::willValidate() const
 {
     if (!m_willValidateInitialized || m_dataListAncestorState == Unknown) {
-        m_willValidateInitialized = true;
-        bool newWillValidate = recalcWillValidate();
-        if (m_willValidate != newWillValidate) {
-            m_willValidate = newWillValidate;
-            const_cast<HTMLFormControlElement*>(this)->setNeedsValidityCheck();
-        }
+        const_cast<HTMLFormControlElement*>(this)->refreshWillValidate();
     } else {
         // If the following assertion fails, setNeedsWillValidateCheck() is not
         // called correctly when something which changes recalcWillValidate() result
         // is updated.
         ASSERT(m_willValidate == recalcWillValidate());
     }
     return m_willValidate;
 }
 
 void HTMLFormControlElement::setNeedsWillValidateCheck()
 {
     // We need to recalculate willValidate immediately because willValidate change can causes style change.
-    bool newWillValidate = recalcWillValidate();
-    if (m_willValidateInitialized && m_willValidate == newWillValidate)
-        return;
-    m_willValidateInitialized = true;
-    m_willValidate = newWillValidate;
-    setNeedsValidityCheck();
-    setNeedsStyleRecalc(SubtreeStyleChange, StyleChangeReasonForTracing::create(StyleChangeReason::Validate));
+    if (refreshWillValidate())
+        setNeedsStyleRecalc(SubtreeStyleChange, StyleChangeReasonForTracing::create(StyleChangeReason::Validate));
     if (!m_willValidate)
         hideVisibleValidationMessage();
 }
 
 void HTMLFormControlElement::findCustomValidationMessageTextDirection(const String& message, TextDirection &messageDir, String& subMessage, TextDirection &subMessageDir)
 {
     bool hasStrongDirection;
     subMessage = fastGetAttribute(titleAttr);
     messageDir = determineDirectionality(message, hasStrongDirection);
     if (!subMessage.isEmpty())
@@ skipping 106 matching lines @@
 {
     // If the following assertion fails, setNeedsValidityCheck() is not called
     // correctly when something which changes validity is updated.
     ASSERT(m_isValid == valid());
     return m_isValid;
 }
 
 void HTMLFormControlElement::setNeedsValidityCheck()
 {
     bool newIsValid = valid();
-    if (willValidate() && newIsValid != m_isValid) {
-        formOwnerSetNeedsValidityCheck();
+    bool changed = newIsValid != m_isValid;
+    m_isValid = newIsValid;
+    if (willValidate() && changed) {
+        formOwnerSetNeedsValidityCheck(ElementModification, newIsValid);
         fieldSetAncestorsSetNeedsValidityCheck(parentNode());
         // Update style for pseudo classes such as :valid :invalid.
         setNeedsStyleRecalc(SubtreeStyleChange, StyleChangeReasonForTracing::createWithExtraData(StyleChangeReason::PseudoClass, StyleChangeExtraData::Invalid));
     }
-    m_isValid = newIsValid;
 
     // Updates only if this control already has a validation message.
     if (isValidationMessageVisible()) {
         // Calls updateVisibleValidationMessage() even if m_isValid is not
         // changed because a validation message can be changed.
         updateVisibleValidationMessage();
     }
 }
 
 void HTMLFormControlElement::setCustomValidity(const String& error)
@@ skipping 38 matching lines @@
 
 void HTMLFormControlElement::setFocus(bool flag)
 {
     LabelableElement::setFocus(flag);
 
     if (!flag && wasChangedSinceLastFormControlChangeEvent())
         dispatchFormControlChangeEvent();
 }
 
 } // namespace blink