gadjo Reporter
	Description • 12 years ago

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

	Mounir Lamouri (:mounir)
	Comment 1 • 11 years ago

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

	Mina Almasry Assignee
	Comment 2 • 11 years ago

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?

	Mounir Lamouri (:mounir)
	Comment 3 • 11 years ago

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.

	Mina Almasry Assignee
	Comment 4 • 11 years ago

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?

	Mounir Lamouri (:mounir)
	Comment 5 • 11 years ago

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?

	Mina Almasry Assignee
	Comment 6 • 11 years ago

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?

	Mina Almasry Assignee
	Comment 7 • 11 years ago

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.

	Mounir Lamouri (:mounir)
	Comment 8 • 11 years ago

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.

	Mounir Lamouri (:mounir)
	Comment 9 • 11 years ago

Comment on attachment 774869 [details] [diff] [review]
patch with all passing tests

Sorry, I meant to cancel the review.

	Mina Almasry Assignee
	Comment 10 • 11 years ago

> 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!

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 12 • 11 years ago

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

	Mina Almasry Assignee
	Comment 13 • 11 years ago

(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

	Mina Almasry Assignee
	Comment 14 • 11 years ago

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

	Mounir Lamouri (:mounir)
	Comment 15 • 11 years ago

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.

	Mina Almasry Assignee
	Comment 16 • 11 years ago

Changes made, and try push:

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

	Mina Almasry Assignee
	Comment 17 • 11 years ago

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

	Mina Almasry Assignee
	Comment 18 • 11 years ago

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.

	Mina Almasry Assignee
	Comment 19 • 11 years ago

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

	Mounir Lamouri (:mounir)
	Comment 20 • 11 years ago

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>?

	Mounir Lamouri (:mounir)
	Comment 21 • 11 years ago

BTW, good catch for the initial validity state ;)

	Mina Almasry Assignee
	Comment 22 • 11 years ago

> 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.

	Mina Almasry Assignee
	Comment 23 • 11 years ago

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

And thanks! :-D

	Mounir Lamouri (:mounir)
	Comment 24 • 11 years ago

(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.

	Mounir Lamouri (:mounir)
	Comment 25 • 11 years ago

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.

	Mina Almasry Assignee
	Comment 26 • 11 years ago

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.

	Mounir Lamouri (:mounir)
	Comment 27 • 11 years ago

(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?

	Mina Almasry Assignee
	Comment 28 • 11 years ago

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

	Mounir Lamouri (:mounir)
	Comment 29 • 11 years ago

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>

	Mina Almasry Assignee
	Comment 30 • 11 years ago

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.

	Mounir Lamouri (:mounir)
	Comment 31 • 11 years ago

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 ;)

	Mounir Lamouri (:mounir)
	Comment 32 • 11 years ago

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.)

	Mina Almasry Assignee
	Comment 33 • 11 years ago

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

	Mounir Lamouri (:mounir)
	Comment 34 • 11 years ago

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.

	Mounir Lamouri (:mounir)
	Comment 35 • 11 years ago

(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

	Ryan VanderMeulen [:RyanVM]
	Comment 36 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/5b5a0de73c74

	Mina Almasry Assignee
	Comment 37 • 11 years ago

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

	Mounir Lamouri (:mounir)
	Comment 38 • 11 years ago

(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 :)

	Mina Almasry Assignee
	Comment 39 • 11 years ago

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: