User Agent: Mozilla/5.0 (Windows NT 6.1; rv:11.0a2) Gecko/20120109 Firefox/11.0a2 Build ID: 20120109042006 Steps to reproduce: Fork from the bug 596515 There should be a possibility of styling the fieldset which is invalid, now we can style only :invalid form elements or :-moz-submit-invalid input. Styling fieldset:invalid would be much more flexible and more intuitive than the -moz-submit-invalid

This has been added to the HTML specification: https://www.w3.org/Bugs/Public/show_bug.cgi?id=20585

Hi I'm willing to work on this, but I'm somewhat new around here. Mounir (or anyone willing) can you break down for me what is required here and where to start with that? If that's okay can I get assigned too?

First of all, you should be familiar with the state handling in Gecko. You can look at how the :invalid / :valid pseudo-classes are applied to HTMLFormElement: https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLFormElement.cpp#2336 The ::IntrinsicState() method will be called very often so it should be quick. We can't go through the entire sub-tree. Luckily, we already have a system in place to keep a cache of elements below the fieldset. When we toggle the disabled state of the fieldset, we have to notify those elements because there disabled state depends on it. A consequence of this is that each form elements has a pointer to its fieldset parent, see: https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsGenericHTMLElement.h#1228 If we look closely, the way we update the form element validity state is mostly via: https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLFormElement.cpp#2011 which is called from nsIConstraintValidation.cpp: https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsIConstraintValidation.cpp This class is inherited by every HTML elements that supports constraint validation and as soon as one of those elements get a validity update, we notify its form parent. We could probably as well notify its fieldset parent if any. You should also consider that testing this feature is going to be the hardest part of this bug. Feel free to get some inspiration from bug 596515. Feel free to ask if you need more information.

Here is my work so far. The patch builds and works. I think what's left is (the big work) writing the tests, if you're fine with the patch. I'm not sure you'll be fine with the patch though: I found that I had to implement UpdateValidity, GetValidity, and IntrinsicState for HTMLFieldSetElement, and couldn't use the implementations in HTMLFormElement (because HTMLFieldSet doesn't inherit those.) Is this fine, or is there a cleaner way to do it? What tests do you want? Or should i follow the list in the bug you posted?

Comment on attachment 771171 [details] [diff] [review] WIP patch To start with, your patch has a weird format that makes bugzilla's review system unable to parse it. I filed bug 890732 to fix that but could you, on your side try to fix this by following this document: https://developer.mozilla.org/en/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F Also, is your editor using 80 lines wrapping? Your code seems to have shorter wrapping. Regarding the patch, the idea is there. There are a few things to fix but mostly details. Regarding the tests, you should test whatever sound needed. I think that this feature was extensively tested for the form element so you should probably look into what has been done to get some inspiration. And thanks for working on this Mina :) >diff -r 0a10eca0c521 -r 1ca3ed3372ed content/html/content/public/nsIFormControl.h >--- a/content/html/content/public/nsIFormControl.h Sat Mar 23 18:45:56 2013 -0400 >+++ b/content/html/content/public/nsIFormControl.h Thu Jul 04 01:16:07 2013 -0400 >@@ -87,6 +87,12 @@ > NS_DECLARE_STATIC_IID_ACCESSOR(NS_IFORMCONTROL_IID) > > /** >+ * Get the fieldset for this form control. >+ * @return the fieldset >+ */ >+ virtual mozilla::dom::Element *GetFieldSetElement() = 0; I think it is okay to have: virtual mozilla::dom::HTMLFieldSetElement* GetFieldSet() = 0; You might need to forward declare HTMLFieldSetElement after the forward declaration of Element. >diff -r 0a10eca0c521 -r 1ca3ed3372ed content/html/content/src/HTMLFieldSetElement.cpp >--- a/content/html/content/src/HTMLFieldSetElement.cpp Sat Mar 23 18:45:56 2013 -0400 >+++ b/content/html/content/src/HTMLFieldSetElement.cpp Thu Jul 04 01:16:07 2013 -0400 >@@ -244,6 +244,45 @@ > } > } > >+void >+HTMLFieldSetElement::UpdateValidity(bool aElementValidity) >+{ >+ if (aElementValidity) { >+ --mInvalidElementsCount; >+ } else { >+ ++mInvalidElementsCount; >+ } >+ >+ NS_ASSERTION(mInvalidElementsCount >= 0, "Something went seriously wrong!"); I would use MOZ_ASSERT() here with no comment. NS_ASSERTION() was enforcing the presence of a comment which leads to useless comments like that. Also, MOZ_ASSERT will stop the execution in debug builds which is going to trigger test failures and will forces developers to look into the issues if it happens. >+ // The form validity has just changed if: >+ // - there are no more invalid elements ; >+ // - or there is one invalid elmement and an element just became >+ // invalid. >+ // If we have invalid elements and we used to before as well, >+ // do nothing. >+ if (mInvalidElementsCount && >+ (mInvalidElementsCount != 1 || aElementValidity)) { >+ return; >+ } >+ >+ UpdateState(true); You copy-pasted the comment without changing it. Also, the code in the fieldset element isn't the same as the form element so we should actually use those conditions to know if we call UpdateState() instead of using them to know if we do an early return. >diff -r 0a10eca0c521 -r 1ca3ed3372ed content/html/content/src/HTMLFieldSetElement.h >--- a/content/html/content/src/HTMLFieldSetElement.h Sat Mar 23 18:45:56 2013 -0400 >+++ b/content/html/content/src/HTMLFieldSetElement.h Thu Jul 04 01:16:07 2013 -0400 >@@ -106,6 +106,30 @@ > > // XPCOM SetCustomValidity is OK for us > >+ /** >+ * Returns the fieldSet's validity based on the last UpdateValidity() call. >+ * @return Whether the form was valid the last time UpdateValidity() was >+ * called. >+ * @note This method may not return the *current* validity state! >+ */ >+ bool GetValidity() const { return !mInvalidElementsCount; } I doubt you need GetValidity(). >+ nsEventStates IntrinsicState() const; I should mark this as virtual as long as HTMLFieldSetElement isn't marked as MOZ_FINAL. >+ /* >+ * This method will update the field's validity so the submit controls states will >+ * be updated (for -moz-submit-invalid pseudo-class). This method has to be >+ * called by form elements whenever their validity state or status regarding >+ * constraint validation changes. >+ * >+ * @note If an element becomes barred from constraint validation, it has to >+ * be considered as valid. >+ * >+ * @param aElementValidityState the new validity state of the element >+ */ >+ void UpdateValidity(bool aElementValidityState); Please, re-read the comment you copy-pasted. >+ /** >+ * Number of invalid and candidate for constraint validation >+ * elements in the fieldSet the last time UpdateValidity has been called. >+ * @note Should only be used by UpdateValidity() and GetValidity()! >+ * >+ */ >+ int32_t mInvalidElementsCount; The comments isn't wrapped correctly. >diff -r 0a10eca0c521 -r 1ca3ed3372ed content/html/content/src/nsIConstraintValidation.cpp >--- a/content/html/content/src/nsIConstraintValidation.cpp Sat Mar 23 18:45:56 2013 -0400 >+++ b/content/html/content/src/nsIConstraintValidation.cpp Thu Jul 04 01:16:07 2013 -0400 >@@ -8,6 +8,7 @@ > #include "nsAString.h" > #include "nsGenericHTMLElement.h" > #include "nsHTMLFormElement.h" >+#include "mozilla/dom/HTMLFieldSetElement.h" > #include "mozilla/dom/ValidityState.h" > #include "nsIFormControl.h" > #include "nsContentUtils.h" >@@ -141,9 +142,14 @@ > > nsHTMLFormElement* form = > static_cast<nsHTMLFormElement*>(formCtrl->GetFormElement()); >+ mozilla::dom::HTMLFieldSetElement* fieldSet = >+ static_cast<mozilla::dom::HTMLFieldSetElement*>(formCtrl->GetFieldSetElement()); You should add at the top of the file: using namespace mozilla; using namespace mozilla::dom; So you don't have to add those "mozilla::dom::". By the way, your mozilla-central is old, nsHTMLFormElement no longer exists. > if (form) { > // If the element is going to be barred from constraint validation, > // we can inform the form that we are now valid. > // Otherwise, we are now invalid. > form->UpdateValidity(aBarred); > } >+ if (fieldSet) { >+ fieldSet->UpdateValidity(aBarred); >+ } Could you move the comment before the if (form) and make it generic for both cases?

I've made all the changes you mentioned and added all the tests in the other bug mentioned. I should be honest, the way I got this so quickly is I took the tests in the other bug and replaced "form" with "fieldset" everywhere. I proof read and tested though. 1/23 test fails. It's the fieldset-remove-invalid-element.html. I follow the code in gdb and it works exactly as expected, and the webpage the build shows looks exactly like the expected. I'm not sure why it fails then... any clues?

I figured out what was wrong with my test and corrected it. Now the patch builds and passes all 23 tests written for it. I think you'll be happy with this patch.

Comment on attachment 774869 [details] [diff] [review] patch with all passing tests Review of attachment 774869 [details] [diff] [review]: ----------------------------------------------------------------- Can't you replace fieldset-invalid-ref.html in reftest.list by about:blank? If yes, I guess that would allow you to remove the file, right? This looks generally good but there are a few cleanup to do. The next iteration should very likely get a r+ if all the comments are applied. ::: content/html/content/public/nsIFormControl.h @@ +90,5 @@ > /** > + * Get the fieldset for this form control. > + * @return the fieldset > + */ > + virtual mozilla::dom::Element *GetFieldSet() = 0; Make this an HTMLFieldSetElement*. ::: content/html/content/src/HTMLFieldSetElement.h @@ +68,5 @@ > void AddElement(nsGenericHTMLFormElement* aElement) { > mDependentElements.AppendElement(aElement); > + > + // We need to update the validity of the fieldset. > + nsCOMPtr<nsIConstraintValidation> cvElmt = do_QueryObject(aElement); You should use "do_QueryInterface", not "do_QueryObject". @@ +72,5 @@ > + nsCOMPtr<nsIConstraintValidation> cvElmt = do_QueryObject(aElement); > + if (cvElmt && > + cvElmt->IsCandidateForConstraintValidation() && !cvElmt->IsValid()) { > + UpdateValidity(false); > + } Please, move this code to the cpp file. @@ +79,5 @@ > void RemoveElement(nsGenericHTMLFormElement* aElement) { > mDependentElements.RemoveElement(aElement); > + > + // We need to update the validity of the fieldset. > + nsCOMPtr<nsIConstraintValidation> cvElmt = do_QueryObject(aElement); do_QueryInterface() again. @@ +83,5 @@ > + nsCOMPtr<nsIConstraintValidation> cvElmt = do_QueryObject(aElement); > + if (cvElmt && > + cvElmt->IsCandidateForConstraintValidation() && !cvElmt->IsValid()) { > + UpdateValidity(true); > + } This code should move to the cpp file. ::: content/html/content/src/nsGenericHTMLElement.h @@ +1095,5 @@ > virtual bool IsNodeOfType(uint32_t aFlags) const MOZ_OVERRIDE; > virtual void SaveSubtreeState() MOZ_OVERRIDE; > > // nsIFormControl > + virtual mozilla::dom::Element* GetFieldSet(); mozilla::dom::HTMLFielSetElement* You might need to forward declare the class. ::: content/html/content/src/nsIConstraintValidation.cpp @@ +142,5 @@ > > + HTMLFormElement* form = > + static_cast<HTMLFormElement*>(formCtrl->GetFormElement()); > + HTMLFieldSetElement* fieldSet = > + static_cast<HTMLFieldSetElement*>(formCtrl->GetFieldSet()); Hopefully, you should be able to return a HTMLFieldSetElement from GetFieldSet() and remove this static_cast<>. @@ +166,5 @@ > bool previousBarred = mBarredFromConstraintValidation; > > mBarredFromConstraintValidation = aBarred; > > + // Inform the form and fieldset elements if our status regarding constraint You are adding a trailing whitespace. @@ +178,5 @@ > + HTMLFieldSetElement* fieldSet = > + static_cast<HTMLFieldSetElement*>(formCtrl->GetFieldSet()); > + > + // If the element is going to be barred from constraint validation, we can > + // inform the form and fieldset that we are now valid. Otherwise, we are now You are adding a trailing whitespace in the two previous lines. ::: layout/reftests/css-invalid/fieldset/fieldset-add-valid-element.html @@ +2,5 @@ > +<!--fieldset with one valid element and another valid one is added dynamically --> > +<html class='reftest-wait'> > + <head> > + <style> > + fieldset:invalid { display: none; } Can't you do: fieldset:valid { display: none; } and then simply compare this file with about:blank? ::: layout/reftests/css-invalid/fieldset/fieldset-add-valid-with-invalid-element.html @@ +1,2 @@ > +<!DOCTYPE html> > +<!--fieldset with one invalid element and another valid one is added dynamically --> Isn't that the same as fieldset-add-control.html ? ::: layout/reftests/css-invalid/fieldset/fieldset-add-valid-with-no-element.html @@ +2,5 @@ > +<!--fieldset with no valid element and another valid one is added dynamically --> > +<html class='reftest-wait'> > + <head> > + <style> > + fieldset:invalid { display: none; } ditto, can't you do: fieldset:valid { display: none; } ? ::: layout/reftests/css-invalid/fieldset/fieldset-dynamic-invalid-barred.html @@ +3,5 @@ > +barred constraints --> > +<html class='reftest-wait'> > + <head> > + <style> > + fieldset:invalid { display: none; } fieldset:valid { display: none; } ::: layout/reftests/css-invalid/fieldset/fieldset-dynamic-invalid.html @@ +3,5 @@ > +<html class='reftest-wait'> > + <head> > + <style> > + fieldset:invalid { display: none; } > + :-moz-ui-invalid { box-shadow: none; } Why is that needed? ::: layout/reftests/css-invalid/fieldset/fieldset-dynamic-valid.html @@ +2,5 @@ > +<!-- fieldset with one valid element which is made valid dynamically --> > +<html> > + <head> > + <style> > + fieldset:invalid {display: none;} fieldset:valid { display: none; } ::: layout/reftests/css-invalid/fieldset/fieldset-invalid-barred.html @@ +2,5 @@ > +<!-- fieldset with invalid barred for constraint validation element --> > +<html> > + <head> > + <style> > + fieldset:invalid { display: none ;} fieldset:valid { display: none; } ::: layout/reftests/css-invalid/fieldset/fieldset-invalid.html @@ -1,4 @@ > -<!DOCTYPE html> > -<html class="reftest-wait"> > - <!-- Test: even if it has a custom error, a fieldset should not be affected by > - :invalid pseudo-class. --> Why not keeping this test? ::: layout/reftests/css-invalid/fieldset/fieldset-remove-invalid-element.html @@ +2,5 @@ > +<!-- fieldset with one invalid element and the element is dynamically removed --> > +<html class='reftest-wait'> > + <head> > + <style> > + fieldset:invalid { display: none; } fieldset:valid { ... } ::: layout/reftests/css-invalid/fieldset/fieldset-static-invalid-barred.html @@ +1,5 @@ > +<!DOCTYPE html> > +<html> > + <head> > + <style> > + fieldset:invalid { display: none; } ...:valid { ... } Also, isn't that test the same as fieldset-invalid-barred.html? You might want to make the other dynamic in the sense of you would add readonly at load time. ::: layout/reftests/css-invalid/fieldset/fieldset-valid-and-barred-remove-barred.html @@ +3,5 @@ > +validation element then you remove the second element --> > +<html class='reftest-wait'> > + <head> > + <style> > + fieldset:invalid { display: none; } ditto ::: layout/reftests/css-invalid/fieldset/fieldset-valid-and-barred.html @@ +2,5 @@ > +<!-- fieldset with one invalid element and a barred for constraint validation element --> > +<html> > + <head> > + <style> > + fieldset:invalid { display: none; } ditto ::: layout/reftests/css-invalid/fieldset/fieldset-valid.html @@ -1,4 @@ > -<!DOCTYPE html> > -<html> > - <!-- Test: fieldset is always barred from constraint validation. > - It should not be affected by :invalid pseudo-class. --> I guess you could keep this test. ::: layout/reftests/css-invalid/fieldset/fieldset-with-valid-and-invalid.html @@ +9,5 @@ > + <body> > + <fieldset id="fieldset"> > + <input id='i' value='foo'> > + <input required> > + <fieldset id="fieldset2"> Why is there two fieldsets here? And you do not close both of them. Is that on purpose? ::: layout/reftests/css-invalid/fieldset/fieldset-with-valid-element-add-barred-dynamic.html @@ +3,5 @@ > +validation element --> > +<html> > + <head> > + <style> > + fieldset:invalid { display:none } ditto ::: layout/reftests/css-invalid/fieldset/reftest.list @@ +19,5 @@ > +== fieldset-invalid-and-barred-remove-barred.html fieldset-invalid-ref.html > +== fieldset-valid-and-barred.html fieldset-valid-and-barred-ref.html > +== fieldset-valid-and-barred-remove-barred.html fieldset-valid-ref.html > +== fieldset-with-invalid-element-add-barred-dynamic.html fieldset-invalid-ref.html > +== fieldset-with-valid-element-add-barred-dynamic.html fieldset-valid-and-barred-ref.html I think you can try to compare all files with about:blank.

Comment on attachment 774869 [details] [diff] [review] patch with all passing tests Sorry, I meant to cancel the review.

> You should use "do_QueryInterface", not "do_QueryObject". When I do that, I get this error: /Users/mina/Repos/mozilla-central/content/html/content/src/HTMLFieldSetElement.cpp:222:64: error: ambiguous conversion from derived class 'nsGenericHTMLFormElement' to base class 'nsISupports': class nsGenericHTMLFormElement -> class nsGenericHTMLElement -> nsGenericHTMLElementBase -> nsMappedAttributeElementBase -> class nsStyledElementNotElementCSSInlineStyle -> nsStyledElementBase -> class mozilla::dom::FragmentOrElement -> class nsIContent -> class nsINode -> mozilla::dom::EventTarget -> class nsIDOMEventTarget -> class nsISupports class nsGenericHTMLFormElement -> class nsIFormControl -> class nsISupports nsCOMPtr<nsIConstraintValidation> cvElmt = do_QueryInterface(aElement); Any idea how to get around that? But I have made all the other changes, I'm attaching a patch anyway. Thanks!

Mounir is wrong, you should use do_QueryObject there ;-)

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #12) > Mounir is wrong, you should use do_QueryObject there ;-) Are there docs on QueryObject anywhere? I'd like to learn how it and QueryInterface work :3

Mounir, just a friendly reminder this is still waiting for review.

Comment on attachment 776526 [details] [diff] [review] patch Review of attachment 776526 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the changes applied. ::: content/html/content/src/HTMLFieldSetElement.h @@ +145,5 @@ > + /** > + * Number of invalid and candidate for constraint validation > + * elements in the fieldSet the last time UpdateValidity has been called. > + * > + * @note Should only be used by UpdateValidity()! nit: ... and IntrinsicState(). ::: content/html/content/src/nsIConstraintValidation.cpp @@ +165,5 @@ > bool previousBarred = mBarredFromConstraintValidation; > > mBarredFromConstraintValidation = aBarred; > > + // Inform the form and fieldset elements if our status regarding constraint nit: trailing whitespace @@ +175,5 @@ > + HTMLFormElement* form = > + static_cast<HTMLFormElement*>(formCtrl->GetFormElement()); > + HTMLFieldSetElement* fieldSet = formCtrl->GetFieldSet(); > + > + // If the element is going to be barred from constraint validation, we can nit: trailing whitespace @@ +176,5 @@ > + static_cast<HTMLFormElement*>(formCtrl->GetFormElement()); > + HTMLFieldSetElement* fieldSet = formCtrl->GetFieldSet(); > + > + // If the element is going to be barred from constraint validation, we can > + // inform the form and fieldset that we are now valid. Otherwise, we are now nit: trailing whitespace ::: layout/reftests/css-invalid/fieldset/fieldset-dynamic-invalid-barred.html @@ +9,5 @@ > + </head> > + <script> > + function onloadHandler() > + { > + document.getElementById('i').readOnly = 'ro'; nit: .readOnly = true; ::: layout/reftests/css-invalid/fieldset/fieldset-dynamic-invalid-not-barred.html @@ +7,5 @@ > + </head> > + <script> > + function onloadHandler() > + { > + document.getElementById('i').removeAttribute('readonly'); nit: .readOnly = false; would work as well. ::: layout/reftests/css-invalid/fieldset/fieldset-invalid-and-barred-remove-barred.html @@ +1,3 @@ > +<!DOCTYPE html> > +<!-- fieldset with one invalid element and a barred for constraint > +validation element then you remove the second element --> Do you have a similar test that removes the invalid element? If not, could you add one? ::: layout/reftests/css-invalid/fieldset/fieldset-invalid.html @@ +1,2 @@ > <!DOCTYPE html> > +<!-- Invalid fieldset. --> Could you keep this test as it is, which means keep the .setCustomValidity() call. ::: layout/reftests/css-invalid/fieldset/fieldset-valid-and-barred-remove-barred.html @@ +15,5 @@ > + } > + </script> > + <body onload='onloadHandler();'> > + <fieldset id="fieldset"> > + <input id='i' value='foo'required> nit: you are missing a space between "value='foo'" and "required". ::: layout/reftests/css-invalid/fieldset/fieldset-valid-and-barred.html @@ +7,5 @@ > + </style> > + </head> > + <body> > + <fieldset id="fieldset"> > + <input id='i' value='bar'required> ditto ::: layout/reftests/css-invalid/fieldset/fieldset-valid.html @@ +1,2 @@ > <!DOCTYPE html> > +<!-- Valid fieldset. --> I was expecting this test to stay as it was, in the idea, which means just testing that an empty <fieldset> is valid.

Changes made, and try push: https://tbpl.mozilla.org/?tree=Try&rev=a755fd2062d1

Mounir, From a previous try push (not the one above) I found some test failures in other parts, and I fixed those in the patch I just posted. But in the process I found some behaviour that may or may not be what we want: - fieldset validity can't be set with setCustomValidity. To set the validity of the fieldset you have to set the validity of an element inside the fieldset using setCustomValidity - fieldset:valid does not apply if the fieldset doesn't have an id. In the patch I just posted, you will find lines with |<fieldset id="anything">|. The |id="anything"| part is necessary for the test to pass. Do you want these behaviours changed? If so can you point me to where I can start with that? Otherwise I think barring the output of the try push above, I can mark it checkin-needed

Okay nevermind. I figured what was wrong. We just need to set the proper state flags in the constructor of a fieldset. The reason adding an ID did the same thing is that, I think, adding an ID calls SetAttr, which calls some helpers but eventually calls UpdateState. Adding the state flag in the constructor does the same thing. I'm asking for a review because of all the changes I had to make to the tests under layout/reftest/css-valid/fieldset, and the bit in the constructor.

Try: https://tbpl.mozilla.org/?tree=Try&rev=84775ac5265f

Comment on attachment 781386 [details] [diff] [review] Patch, changes applied, ID's not req Review of attachment 781386 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following changes applied. ::: content/html/content/test/forms/test_validation.html @@ -319,5 @@ > checkCustomError(document.getElementById('s'), false); > checkCustomError(document.getElementById('t'), false); > checkCustomError(document.getElementById('o'), false); > checkCustomError(document.getElementById('b'), true); > -checkCustomError(document.getElementById('f'), true); Please, keep that test. I understand that it is failing because :valid applies to <fieldset> but I would be happy if you simply write a special case in checkCustomError() for fieldset. Like: is(window.getComputedStyle(element, null).getPropertyValue('background-color'), isBarred && element.tagName != "FIELDSET" ? "rgb(0, 0, 0)" : "rgb(0, 255, 0)", ":valid pseudo-classs should apply"); ::: layout/reftests/css-invalid/fieldset/fieldset-add-invalid-barred.html @@ +15,5 @@ > + </head> > + <body onload="onloadHandler();"> > + <input required readonly id="i"> > + <fieldset id="fieldset"> > + </fieldset> nit: indentation is wrong. ::: layout/reftests/css-invalid/fieldset/fieldset-invalid.html @@ +1,5 @@ > <!DOCTYPE html> > <html class="reftest-wait"> > +<!-- Invalid fieldset --> > +<style> > + fieldset:invalid { display: none; } Shouldn't that be: fieldset: valid { display: none; } Because, <fieldset> is barred from constraint validation so calling .setCustomValidity('foo') should have no effect. @@ +5,5 @@ > + fieldset:invalid { display: none; } > +</style> > +<body onload="document.getElementById('input').setCustomValidity('foo'); document.documentElement.className='';"> > +<fieldset> > + <input id="input"> Could you remove that <input>? ::: layout/reftests/css-valid/fieldset/fieldset-invalid.html @@ +5,2 @@ > <style> > + fieldset:invalid { display: none; } Shouldn't that be: fieldset: valid { display: none; } Because, <fieldset> is barred from constraint validation so calling .setCustomValidity('foo') should have no effect. @@ +7,4 @@ > </style> > <body onload="document.getElementById('f').setCustomValidity('foo'); document.documentElement.className='';"> > + <fieldset> > + <input id='f'> Could you remove that <input>?

BTW, good catch for the initial validity state ;)

> Shouldn't that be: > fieldset: valid { display: none; } > Because, <fieldset> is barred from constraint validation so calling > .setCustomValidity('foo') should have no effect. > Could you remove that <input>? setCustomValidity is called on the input inside the fieldset, not the fieldset, so it applies; as in the input becomes invalid and therefore the fieldset becomes invalid, which is what these tests are testing. That's why that call to setCustomValidity, and the <input> are necessary for the test. And I shouldn't remove them. Is that okay? The rest of the changes I can make.

(In reply to Mounir Lamouri (:mounir) from comment #21) > BTW, good catch for the initial validity state ;) And thanks! :-D

(In reply to Mina Almasry from comment #22) > > Shouldn't that be: > > fieldset: valid { display: none; } > > Because, <fieldset> is barred from constraint validation so calling > > .setCustomValidity('foo') should have no effect. > > > Could you remove that <input>? > > setCustomValidity is called on the input inside the fieldset, not the > fieldset, so it applies; as in the input becomes invalid and therefore the > fieldset becomes invalid, which is what these tests are testing. That's why > that call to setCustomValidity, and the <input> are necessary for the test. > And I shouldn't remove them. Is that okay? The rest of the changes I can > make. Could you change the test to call .setCustomValidity() on the <fieldset> instead? So, we make sure that the <fieldset> does not become invalid when .setCustomValidity() is called. Testing that the fieldset becomes invalid when one of its children is invalid is something we have extensively tested already.

Oh, actually, that makes me think of a few test cases that I would us to test: <fieldset> <fieldset> <input required> </fieldset> </fieldset> -> both fieldsets should be invalid, could you have one test like that and one when we change the input from invalid to valid and another when the input changes from invalid to valid. Bonus point if you do insertion/removal tests too ;) Also: <fieldset> <div> <input required> </div> </fieldset> -> fieldset should be invalid. Same kind of tests as above.

So an input inside of a <div> works fine, but this case doesn't: > <fieldset> > <fieldset> > <input required> > </fieldset> > </fieldset> the outer fieldset doesn't become invalid. I think that has to do with fieldsets being barred from validation, and so the property doesn't bubble up. The rest of the tests I can make. Do you mind me filing a separate bug for this, or does it have to be done here? It'll take me a while to get to that.

(In reply to Mina Almasry from comment #26) > So an input inside of a <div> works fine, but this case doesn't: > > > <fieldset> > > <fieldset> > > <input required> > > </fieldset> > > </fieldset> > > the outer fieldset doesn't become invalid. I think that has to do with > fieldsets being barred from validation, and so the property doesn't bubble > up. > > The rest of the tests I can make. Do you mind me filing a separate bug for > this, or does it have to be done here? It'll take me a while to get to that. Sorry for the delay. I think fixing that might not be so hard. When you update the validity state, you get the element's fieldset. You should actually go trough all fieldset chain until you get nullptr. Something like (this is pseudo-code): for (HTMLFieldSetElement fieldset = element->GetFieldSet(); fieldset; fieldset = fieldset->GetFieldSet()) { fieldset->UpdateValidity(...); } Can you try that?

That did fix it actually. Here is a patch with the changes.

Comment on attachment 790605 [details] [diff] [review] Patch Review of attachment 790605 [details] [diff] [review]: ----------------------------------------------------------------- Unfortunately, I think that you forgot to modify one call point of UpdateValidity :( Actually, to reduce the changes of that to happen, you could as well change HTMLFieldSetElement::UpdateValidity() to have it call its parent. Something like: HTMLFieldSetElement::UpdateValidity(bool aValidity) { // We should propagate the change to the fieldset parent chain. if (mFieldSet) { mFieldSet->UpdateValidity(aValidity); } [...] } ::: content/html/content/src/nsIConstraintValidation.cpp @@ +147,5 @@ > } > + for (HTMLFieldSetElement* fieldSet = formCtrl->GetFieldSet(); > + fieldSet; > + fieldSet = fieldSet->GetFieldSet()) > + { nit: the "{" needs to be at the end of the previous style per coding style. And add a comment explaining why we go trough the parent chain. @@ +187,5 @@ > form->UpdateValidity(aBarred); > } > + if (fieldSet) { > + fieldSet->UpdateValidity(aBarred); > + } You should probably go trough the parent chain here too. For example if you have: <fieldset> <fieldset> <input required> </fieldset> </fieldset> ... and then you mark the <input> as readonly, it becomes barred from constraint validation and the two <fieldset> should be valid. Could you fix that and write a test for it? Also, as long as you are there, could you test that in this case, the first inner fieldset is valid, and the second inner and the outer are both invalid: <fieldset> <fieldset> <input> </fieldset> <fieldset> <input required> </fieldset> </fieldset>

Okay here is a patch with the changes and the required test. I decided not to change HTMLFieldSetElement::UpdateValidity() and to change the calls in nsIConstraintValidation.cpp because the form and the first fieldset are updated in nsIConstraintValidation.cpp. If the rest of the fieldsets were updated in HTMLFieldSetElement::UpdateValidity() then we'd be updating things in two different places and that doesn't seem ideal.

Comment on attachment 790908 [details] [diff] [review] Patch with changes Review of attachment 790908 [details] [diff] [review]: ----------------------------------------------------------------- r=me ... and sent to try: https://tbpl.mozilla.org/?tree=Try&rev=f55d97015931 ;)

Unfortunately, it seems that you are raising an assert: https://tbpl.mozilla.org/php/getParsedLog.php?id=26641300&tree=Try https://tbpl.mozilla.org/php/getParsedLog.php?id=26639851&tree=Try https://tbpl.mozilla.org/php/getParsedLog.php?id=26639828&tree=Try (Ignore the M-1 failures on the push, they are related to another patch in the queue.)

Okay to solve the problem I went with update the parent fieldset inside of HTMLFieldSetElement::UpdateValidity. The cause of the problem is that when we add an invalid element, we call HTMLFieldSetElement::AddElement, which calls HTMLFieldSetElement::UpdateValidity and not something in nsIConstraintValidiation. Therefore transversing up the fieldset tree should be in AddElement or UpdateValidity. Try push: https://tbpl.mozilla.org/?tree=Try&rev=35615ac4bb9a

Comment on attachment 791573 [details] [diff] [review] Patch Review of attachment 791573 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thanks for looking into this. I will push this to try again.

(In reply to Mina Almasry from comment #33) > Try push: https://tbpl.mozilla.org/?tree=Try&rev=35615ac4bb9a Seems like pushing to try is actually not needed ;) I landed it: https://hg.mozilla.org/integration/mozilla-inbound/rev/5b5a0de73c74

OHHHH YEAHHHHHHHH! Mounir if we were in the same city I'd buy you a beer.

(In reply to Mina Almasry from comment #37) > OHHHH YEAHHHHHHHH! Mounir if we were in the same city I'd buy you a beer. No, you deserve the beer Mina, that's your patch ;) Thanks for working on this and for your patience with my reviews :)

Comment on attachment 791573 [details] [diff] [review] Patch [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): String or IDL/UUID changes made by this patch: