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

Source/core/html/HTMLFormControlElement.cpp

Index: Source/core/html/HTMLFormControlElement.cpp
diff --git a/Source/core/html/HTMLFormControlElement.cpp b/Source/core/html/HTMLFormControlElement.cpp
index 75f2402d2c782128eb57e2c5c92b7f9adfe90112..2018afe65268e7eceff9da7192b201489fabd6ca 100644
--- a/Source/core/html/HTMLFormControlElement.cpp
+++ b/Source/core/html/HTMLFormControlElement.cpp
@@ -268,23 +268,19 @@ void HTMLFormControlElement::removedFrom(ContainerNode* 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)
@@ -410,15 +406,25 @@ bool HTMLFormControlElement::recalcWillValidate() const
     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
@@ -431,13 +437,8 @@ bool HTMLFormControlElement::willValidate() const
 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();
 }
@@ -564,13 +565,14 @@ bool HTMLFormControlElement::isValidElement()
 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()) {