Implement constraint validation API for form element

RESOLVED FIXED in mozilla2.0b5

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

({dev-doc-complete, html5})

Trunk
mozilla2.0b5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

Assignee

Description

9 years ago
When a form validity is checked, it has to check all elements in the form.
bug 345624 is adding the needed API for the elements so the form will be able to check them.
Assignee

Updated

9 years ago
Blocks: 561636
Assignee

Updated

9 years ago
Blocks: 566348
Assignee

Updated

9 years ago
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Keywords: dev-doc-needed
Assignee

Updated

9 years ago
Depends on: 561635
Assignee

Comment 1

9 years ago
I'm going to divide this patch in two:
 - checkValidity() implementation (which is implementing static validation of constraints) and nsConstraintValidation becomes an interface ;
 - interactively validate the constraints when trying to submit the form.

Actually, this second part could be for another bug.
Attachment #458699 - Flags: review?(Olli.Pettay)
Assignee

Comment 2

9 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
Actually, I can't write most of the second part of the patch because I need a UI to block the form submission and without the ability to block form submission, I can't test anything. So, I've merged what can be merged in the current patch and most of the patch will come with the UI.

Jonas, I asked for a sr for nsIConstraintValidation change but I'm not sure a sr is needed. Feel free to cancel it otherwise.
Attachment #458699 - Attachment is obsolete: true
Attachment #458789 - Flags: superreview?(jonas)
Attachment #458789 - Flags: review?(Olli.Pettay)
Attachment #458699 - Flags: review?(Olli.Pettay)
It's unfortunate that you have to change to using an interface, we're generally trying to go the other direction and remove interfaces.

Could you put IsCandidateForConstraintValidation and IsValid on nsGenericHTMLElement instead? It's a bit ugly, but so is having an interface.cpp file...
Assignee

Comment 4

9 years ago
Ugly solution vs ugly solution, I would prefer to merge the cpp into the header.

Having these functions in nsGenericHTMLFormElement will force me to do have a virtual methods and redefined them in the elements supporting constraint validation so they can call nsConstraintValidation::Method().
In term of design, I found it really ugly.

This said, I'm just trying to defend my idea. If the interfaces have to be avoided at any cost, I will do that.
Assignee

Comment 5

9 years ago
Posted patch Patch v1.1 (obsolete) — Splinter Review
Looks like I did a bad manipulation just before attaching the patch: a method declaration was wrong.
Attachment #458789 - Attachment is obsolete: true
Attachment #459024 - Flags: superreview?(jonas)
Attachment #459024 - Flags: review?(Olli.Pettay)
Attachment #458789 - Flags: superreview?(jonas)
Attachment #458789 - Flags: review?(Olli.Pettay)
There are cases when concrete classes have IID. Looks like it could be used in
this case because nsIConstraintValidation isn't really an interface.
It is a real implementation.
Assignee

Updated

9 years ago
Depends on: 580575
So I'd prefer adding IID for nsConstraintValidation.
Attachment #459024 - Flags: review?(Olli.Pettay)
Assignee

Updated

9 years ago
Blocks: 581947
Assignee

Comment 8

9 years ago
Posted patch Patch v1.2 (obsolete) — Splinter Review
Attachment #459024 - Attachment is obsolete: true
Attachment #460591 - Flags: superreview?(jonas)
Attachment #460591 - Flags: review?(Olli.Pettay)
Attachment #459024 - Flags: superreview?(jonas)
Assignee

Comment 9

9 years ago
Posted patch Pathc v1.3 (obsolete) — Splinter Review
Updated patch to apply against current tip.
Attachment #460591 - Attachment is obsolete: true
Attachment #461188 - Flags: superreview?(jonas)
Attachment #461188 - Flags: review?(Olli.Pettay)
Attachment #460591 - Flags: superreview?(jonas)
Attachment #460591 - Flags: review?(Olli.Pettay)
Comment on attachment 461188 [details] [diff] [review]
Pathc v1.3

>diff --git a/content/html/content/public/nsIFormControl.h b/content/html/content/public/nsIFormControl.h
>--- a/content/html/content/public/nsIFormControl.h
>+++ b/content/html/content/public/nsIFormControl.h
You should update the IID of nsIFormControl

>+nsConstraintValidation::CheckValidity(PRBool* aValidity)
> {
>-  if (!IsCandidateForConstraintValidation(aElement) || IsValid()) {
>+  if (!IsCandidateForConstraintValidation() || IsValid()) {
>     *aValidity = PR_TRUE;
>     return NS_OK;
>   }
> 
>   *aValidity = PR_FALSE;
> 
>-  return nsContentUtils::DispatchTrustedEvent(aElement->GetOwnerDoc(),
>-                                              static_cast<nsIContent*>(aElement),
>+  nsCOMPtr<nsIContent> content = do_QueryInterface(this);
This is a big ugly, but I don't see any better way to do this.

>-nsConstraintValidation::IsCandidateForConstraintValidation(const nsGenericHTMLFormElement* const aElement) const
>+nsConstraintValidation::IsCandidateForConstraintValidation() const
> {
>   /**
>    * An element is never candidate for constraint validation if:
>    * - it is disabled ;
>    * - TODO: or it's ancestor is a datalist element (bug 555840).
>    * We are doing these checks here to prevent writing them in every
>    * |IsBarredFromConstraintValidation| function.
>    */
> 
>-  // At the moment, every elements which can be candidate for constraint
>-  // validation can be disabled. However, using |CanBeDisabled| is future-proof.
>-  if (aElement->CanBeDisabled() &&
>-      aElement->HasAttr(kNameSpaceID_None, nsGkAtoms::disabled)) {
>-    return PR_FALSE;
>-  }
>+  nsCOMPtr<nsIContent> content =
>+    do_QueryInterface(const_cast<nsConstraintValidation*>(this));
Same here. Is the change really needed? I know both ways are ugly, but
adding extra QI is just slow. (although this shouldn't be performance critical code)

>- * This class has to be inherited by all elements implementing the API.
>+ * This interface has to be used by all elements implementing the API.
Well, it is still a class, not really an interface.


>@@ -689,16 +698,24 @@ nsHTMLFormElement::DoSubmit(nsEvent* aEv
>   NS_ASSERTION(GetCurrentDoc(), "Should never get here without a current doc");
> 
>   if (mIsSubmitting) {
>     NS_WARNING("Preventing double form submission");
>     // XXX Should this return an error?
>     return NS_OK;
>   }
> 
>+  if (!CheckFormValidity()) {
>+    printf("= The form is not valid!\n");
Don't leave this here, or make it at least
#ifdef DEBUG



>+PRBool
>+nsHTMLFormElement::CheckFormValidity() const
>+{
>+  PRBool ret = PR_TRUE;
>+
>+  nsTArray<nsGenericHTMLFormElement*> sortedControls;
>+  if (NS_FAILED(mControls->GetSortedControls(sortedControls))) {
>+    return PR_FALSE;
>+  }
>+
>+  PRUint32 len = sortedControls.Length();
>+  for (PRUint32 i = 0; i < len; ++i) {
>+    if (!sortedControls[i]->IsSubmittableControl()) {
>+      continue;
>+    }
>+
>+    nsCOMPtr<nsConstraintValidation> cvElmt =
>+      do_QueryInterface((nsGenericHTMLElement*)sortedControls[i]);
>+    if (cvElmt && cvElmt->IsCandidateForConstraintValidation() &&
>+        !cvElmt->IsValid()) {
>+      ret = PR_FALSE;
>+      nsContentUtils::DispatchTrustedEvent(sortedControls[i]->GetOwnerDoc(),
>+                                           static_cast<nsIContent*>(sortedControls[i]),
>+                                           NS_LITERAL_STRING("invalid"),
>+                                           PR_FALSE, PR_TRUE);
>+    }
>+  }
>+
>+  return ret;
>+}
This looks rather scary. What keeps all the elements in sortedControls alive while dispatching events?
What if the first element is invalid, and the event is dispatched, and some event handler removes all the form controls from the
page?
 
> [scriptable, uuid(a5735b98-7a5f-4242-8c9a-3805f3f61b76)]
Update the uuid
Attachment #461188 - Flags: review?(Olli.Pettay) → review-
Assignee

Comment 11

9 years ago
(In reply to comment #10)
> >+nsConstraintValidation::CheckValidity(PRBool* aValidity)
> > {
> >-  if (!IsCandidateForConstraintValidation(aElement) || IsValid()) {
> >+  if (!IsCandidateForConstraintValidation() || IsValid()) {
> >     *aValidity = PR_TRUE;
> >     return NS_OK;
> >   }
> > 
> >   *aValidity = PR_FALSE;
> > 
> >-  return nsContentUtils::DispatchTrustedEvent(aElement->GetOwnerDoc(),
> >-                                              static_cast<nsIContent*>(aElement),
> >+  nsCOMPtr<nsIContent> content = do_QueryInterface(this);
> This is a big ugly, but I don't see any better way to do this.

Unfortunately, me neither.

> >-nsConstraintValidation::IsCandidateForConstraintValidation(const nsGenericHTMLFormElement* const aElement) const
> >+nsConstraintValidation::IsCandidateForConstraintValidation() const
> > {
> >   /**
> >    * An element is never candidate for constraint validation if:
> >    * - it is disabled ;
> >    * - TODO: or it's ancestor is a datalist element (bug 555840).
> >    * We are doing these checks here to prevent writing them in every
> >    * |IsBarredFromConstraintValidation| function.
> >    */
> > 
> >-  // At the moment, every elements which can be candidate for constraint
> >-  // validation can be disabled. However, using |CanBeDisabled| is future-proof.
> >-  if (aElement->CanBeDisabled() &&
> >-      aElement->HasAttr(kNameSpaceID_None, nsGkAtoms::disabled)) {
> >-    return PR_FALSE;
> >-  }
> >+  nsCOMPtr<nsIContent> content =
> >+    do_QueryInterface(const_cast<nsConstraintValidation*>(this));
> Same here. Is the change really needed? I know both ways are ugly, but
> adding extra QI is just slow. (although this shouldn't be performance critical
> code)

I really prefer this solution than using a pointer which is nothing else than 'this' casted differently. However, bug 580575 is completely removing that code so I guess we shouldn't worry too much.

> >@@ -689,16 +698,24 @@ nsHTMLFormElement::DoSubmit(nsEvent* aEv
> >   NS_ASSERTION(GetCurrentDoc(), "Should never get here without a current doc");
> > 
> >   if (mIsSubmitting) {
> >     NS_WARNING("Preventing double form submission");
> >     // XXX Should this return an error?
> >     return NS_OK;
> >   }
> > 
> >+  if (!CheckFormValidity()) {
> >+    printf("= The form is not valid!\n");
> Don't leave this here, or make it at least
> #ifdef DEBUG

Ok for #ifdef DEBUG.

> >+PRBool
> >+nsHTMLFormElement::CheckFormValidity() const
> >+{
> >+  PRBool ret = PR_TRUE;
> >+
> >+  nsTArray<nsGenericHTMLFormElement*> sortedControls;
> >+  if (NS_FAILED(mControls->GetSortedControls(sortedControls))) {
> >+    return PR_FALSE;
> >+  }
> >+
> >+  PRUint32 len = sortedControls.Length();
> >+  for (PRUint32 i = 0; i < len; ++i) {
> >+    if (!sortedControls[i]->IsSubmittableControl()) {
> >+      continue;
> >+    }
> >+
> >+    nsCOMPtr<nsConstraintValidation> cvElmt =
> >+      do_QueryInterface((nsGenericHTMLElement*)sortedControls[i]);
> >+    if (cvElmt && cvElmt->IsCandidateForConstraintValidation() &&
> >+        !cvElmt->IsValid()) {
> >+      ret = PR_FALSE;
> >+      nsContentUtils::DispatchTrustedEvent(sortedControls[i]->GetOwnerDoc(),
> >+                                           static_cast<nsIContent*>(sortedControls[i]),
> >+                                           NS_LITERAL_STRING("invalid"),
> >+                                           PR_FALSE, PR_TRUE);
> >+    }
> >+  }
> >+
> >+  return ret;
> >+}
> This looks rather scary. What keeps all the elements in sortedControls alive
> while dispatching events?
> What if the first element is invalid, and the event is dispatched, and some
> event handler removes all the form controls from the
> page?

I'm not sure I get it correctly. Do you want me to use a refcounted array or to be sure that the invalid event isn't sent dispatched if the element has just been removed from the form by another element invalid event handler? I agree with the former but the later doesn't seem to be what we want.
refcounted array sounds the right way.
Assignee

Comment 13

9 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
I didn't create a nsCOMArray which would have been a bit too much just for refcounting so I'm holding a reference of the objects that is released as soon as invalid events have been called.
Attachment #461188 - Attachment is obsolete: true
Attachment #462416 - Flags: superreview?(jonas)
Attachment #462416 - Flags: review?(Olli.Pettay)
Attachment #461188 - Flags: superreview?(jonas)
Comment on attachment 462416 [details] [diff] [review]
Patch v2

>diff --git a/content/html/content/public/nsIFormControl.h b/content/html/content/public/nsIFormControl.h
>--- a/content/html/content/public/nsIFormControl.h
>+++ b/content/html/content/public/nsIFormControl.h
Update IID

>+  // For the moment, all elements that are not barred from constraint validation
>+  // accept the disabled attribute and elements that are almays barred from
always


>   NS_IMETHOD GetWillValidate(PRBool* aWillValidate) {                         \
>-    return nsConstraintValidation::GetWillValidate(aWillValidate, this);      \
>+    *aWillValidate = IsCandidateForConstraintValidation();                    \
>+    return PR_TRUE;                                                           \
>   }                                                                           \
The method return nsresult, not PRBool. You want to return NS_OK;


>   NS_IMETHODIMP _from::GetWillValidate(PRBool* aWillValidate) {               \
>-    return nsConstraintValidation::GetWillValidate(aWillValidate, this);      \
>+    *aWillValidate = IsCandidateForConstraintValidation();                    \
>+    return PR_TRUE;                                                           \
Same here



>+PRBool
>+nsGenericHTMLFormElement::IsSubmittableControl() const
>+{
>+  // TODO: keygen should be in that list.
File a followup and add the bug number here.

>+PRBool
>+nsHTMLFormElement::CheckFormValidity() const
>+{
>+  PRBool ret = PR_TRUE;
>+
>+  nsTArray<nsGenericHTMLFormElement*> sortedControls;
>+  if (NS_FAILED(mControls->GetSortedControls(sortedControls))) {
I wonder if GetSortedControls should have nsTArray<nsRefPtr<nsGenericHTMLFormElement> > as the out param.
Depends on other use cases. But for now manual addref/release is ok, IMO.
Attachment #462416 - Flags: review?(Olli.Pettay) → review+
Assignee

Comment 15

9 years ago
Posted patch Patch v2.1Splinter Review
Thank you for your review and sorry, I realized I forgot to apply the requested changes except refcounting the elements. It's done now.
Attachment #462416 - Attachment is obsolete: true
Attachment #462811 - Flags: superreview?(jonas)
Attachment #462416 - Flags: superreview?(jonas)
Comment on attachment 462811 [details] [diff] [review]
Patch v2.1

The whole thing of giving nsConstraintValidation an interface ID and QIing to it is pretty ugly. I think I would prefer to simply rename nsConstraintValidation to nsIConstraintValidation everywhere, but otherwise keep the implementation exactly the same. I.e. make it a "real" interface, but keep non-virtual functions on it.

Other than that I think we should start landing this stuff. I'm nervous about having so many patches sitting in bugs rather than in the tree. We should just land them and commit to fixing the missing functionality before shipping.
Attachment #462811 - Flags: superreview?(jonas) → superreview+
Assignee

Comment 18

9 years ago
(In reply to comment #16)
> Comment on attachment 462811 [details] [diff] [review]
> Patch v2.1
> 
> The whole thing of giving nsConstraintValidation an interface ID and QIing to
> it is pretty ugly. I think I would prefer to simply rename
> nsConstraintValidation to nsIConstraintValidation everywhere, but otherwise
> keep the implementation exactly the same. I.e. make it a "real" interface, but
> keep non-virtual functions on it.

The new patch is doing the renaming.

> Other than that I think we should start landing this stuff. I'm nervous about
> having so many patches sitting in bugs rather than in the tree. We should just
> land them and commit to fixing the missing functionality before shipping.

I guess we can begin to land everything as soon as beta4 code freeze will be done?
By the way, my entire queue has been tested on the try server so, at least, there shouldn't be issues regarding tests.
Comment on attachment 465628 [details] [diff] [review]
Rename nsConstraintValidation to nsIConstraintValidation

Could have sworn I had r+'ed this one already.
Attachment #465628 - Flags: review?(jonas) → review+
Assignee

Updated

9 years ago
No longer depends on: 580575
Assignee

Comment 20

9 years ago
Landed:
http://hg.mozilla.org/mozilla-central/rev/607bb86d4dc3
http://hg.mozilla.org/mozilla-central/rev/81b52091ecd7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
You need to log in before you can comment on or make changes to this bug.