1088761 - Add support for reportValidity() for form controls

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Šime Vidas]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141011015303

Steps to reproduce:

1. Open this demo: http://jsbin.com/yixamu/edit?html,output
2. Leave the text field empty and click on the “Report validity” button


Actual results:

Nothing.


Expected results:

The browser’s validation error bubble should have been displayed.

Comment 1

[Andrew Overholt [:overholt]]

https://html.spec.whatwg.org/multipage/forms.html#dom-cva-reportvalidity

Chrome ships in 40:  https://www.chromestatus.com/feature/5433505009893376

Comment 2

[Alexander]

Why is this still not fixed? I wanted to add a visual feedback to HTML 5 form validation in my Firefox extension, but there seems to be no way to do that aside from ridiculous hacks or including an additional JS library to replace HTML 5 validation.

I am trying to let user optionally fill in some of fields, based on contents of others, and would like to highlight *only* relevant fields, when their validation fails. This means, that submitting entire thing by calling submitBtn.click() does not suffice.

I can hack around this somehow, but it is hard to imagine a professional web developers using that kind of workarounds in their websites.

Comment 3

[rr-]

I'm a bit disappointed with this as well...

Comment 4

[kthornbloom]

Although a very simple form may not need this, the moment you attempt to build anything more advanced it becomes apparent that you need a way to validate forms without submitting them first. Please add this feature! It seems that it shouldn't be too difficult since you already have the error bubbles in place now.

Comment 5

[John Dai[:jdai]]

I would like to take this bug.

Comment 6

[John Dai[:jdai]]

Attachment: Part 1: Bug 1088761 - Remove redundant trailing spaces.

Comment 7

[John Dai[:jdai]]

Attachment: Part 2: Bug 1088761 - Support reportValidity() for form controls.

Comment 8

[John Dai[:jdai]]

Attachment: Part 3: Bug 1088761 - Update web-platform tests for reportValidity().

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8740388 [details] [diff] [review]
Part 2: Bug 1088761 - Support reportValidity() for form controls.


>+nsIConstraintValidation::ReportValidity()
>+{
>+  if (!nsIConstraintValidation::CheckValidity()) {
>+    nsCOMPtr<nsIFormControl> formCtrl = do_QueryInterface(this);
>+    NS_ASSERTION(formCtrl, "This interface should be used by form elements!");
MOZ_ASSERT

>+
>+    HTMLFormElement* form =
>+      static_cast<HTMLFormElement*>(formCtrl->GetFormElement());
Please make that 
RefPtr<HTMLFormElement>. Calling CheckValidFormSubmission may execute scripts, so nothing guarantees form stays alive unless
we have a strong pointer to it.

>+    if (form) {
>+      form->CheckValidFormSubmission();
>+    }
So I don't understand this call. What if some form has several invalid fields, but .reportValidity() isn't called on the first one.
Won't CheckValidFormSubmission() find all the invalid fields, and start from the first invalid one and show the popup near it?
And doesn't calling CheckValidFormSubmission() here end up dispatching possibly some "invalid" events. The one we wanted was already dispatched by 
nsIConstraintValidation::CheckValidity() call.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8740389 [details] [diff] [review]
Part 3: Bug 1088761 - Update web-platform tests for reportValidity().

Btw, sounds wpt could use some more tests for the method, at least for the invalid event dispatching and so.
But r+ for this.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Also, need to make sure "invalid" event is handled per spec
"the user agent must: fire a simple event named invalid that is cancelable at the element, and if that event is not canceled, report the problems with the constraints of that element to the user"
https://html.spec.whatwg.org/multipage/forms.html#dom-cva-reportvalidity

So, if preventDefault() is called, no validity popup should be shown, if I interpret the spec right.

Comment 12

[John Dai[:jdai]]

Attachment: Part 2: Bug 1088761 - Support reportValidity() for form controls. v2

Comment 13

[John Dai[:jdai]]

Attachment: Part 3: Bug 1088761 - Update web-platform tests for reportValidity(). v2

Comment 14

[John Dai[:jdai]]

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=011aa2857a5e7b86b0d08bb665713f94b0720014

Comment 15

[John Dai[:jdai]]

In this patch, I got a TIMEOUT when I running /html/semantics/forms/constraints/form-validation-reportValidity.html in e10s mode. The error came from this testcase[1], and root cause came from my code.

nsIConstraintValidation::ReportValidity()
if (observer) {
  observer->NotifyInvalidSubmit(nullptr,
  				static_cast<nsIArray*>(invalidElements));
}

Therefore, I cancel the review first. Need to spend time investigate it.

[1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/html/semantics/forms/constraints/form-validation-reportValidity.html?from=form-validation-reportValidity.html#63

Comment 16

[John Dai[:jdai]]

(In reply to John Dai[:johnz][:jdai] from comment #15)
> In this patch, I got a TIMEOUT when I running
> /html/semantics/forms/constraints/form-validation-reportValidity.html in
> e10s mode. The error came from this testcase[1], and root cause came from my
> code.
> 
> nsIConstraintValidation::ReportValidity()
> if (observer) {
>   observer->NotifyInvalidSubmit(nullptr,
>   				static_cast<nsIArray*>(invalidElements));
> }
> 
> Therefore, I cancel the review first. Need to spend time investigate it.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/
> html/semantics/forms/constraints/form-validation-reportValidity.
> html?from=form-validation-reportValidity.html#63

Finally, the problem comes from using EuclidLCM() handle big Decimal value[1] cause TIMEOUT. When we call GetValidationMessage(), if the validation message type is VALIDITY_STATE_STEP_MISMATCH, it calls EuclidLCM() to round step as an integer. If the step is a big Decimal value, in this testcase step is 2 * 1 * 86400000, it takes a lot of time to handle EuclidLCM(). Put stacktrace for reference:


#0  0x000000000040ff6a in blink::Decimal::alignOperands(blink::Decimal const&, blink::Decimal const&) (lhs=..., rhs=...) at /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/mfbt/deci
mal/Decimal.cpp:590
#1  0x000000000040f49c in blink::Decimal::operator-(blink::Decimal const&) const (this=0x7fffffff7420, rhs=...) at /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/mfbt/decimal/Decim
al.cpp:399
#2  0x00007fffe8567fc0 in mozilla::EuclidGCD<blink::Decimal>(blink::Decimal, blink::Decimal) (aA=..., aB=...) at /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-
gnu/dist/include/mozilla/MathAlgorithms.h:33
#3  0x00007fffe8560c4b in mozilla::EuclidLCM<blink::Decimal>(blink::Decimal, blink::Decimal) (aA=..., aB=...) at /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-
gnu/dist/include/mozilla/MathAlgorithms.h:48
#4  0x00007fffe853ab26 in mozilla::dom::HTMLInputElement::GetValidationMessage(nsAString_internal&, nsIConstraintValidation::ValidityStateType) (this=0x7fffc12c6520, aValidationMessage=..., aType=nsIConstr
aintValidation::VALIDITY_STATE_STEP_MISMATCH) at /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/dom/html/HTMLInputElement.cpp:7010
#5  0x00007fffe85eb6f0 in nsIConstraintValidation::GetValidationMessage(nsAString_internal&) (this=0x7fffc12c6650, aValidationMessage=...) at /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace
/firefox/dom/html/nsIConstraintValidation.cpp:93
#6  0x00007fffe85265a4 in mozilla::dom::HTMLInputElement::GetValidationMessage(nsAString_internal&) (this=0x7fffc12c6520, aValidationMessage=...) at /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/wo
rkspace/firefox/dom/html/HTMLInputElement.cpp:1068
#7  0x00007fffe8539ef3 in mozilla::dom::HTMLInputElement::GetValidationMessage(nsAString_internal&, mozilla::ErrorResult&) (this=0x7fffc12c6520, aValidationMessage=..., aRv=...) at /media/john/c6200233-c3c
3-4f24-8744-a964b0584bfa/john/workspace/firefox/dom/html/HTMLInputElement.cpp:6827


[1]https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.cpp?from=HTMLINPUTelement.cpp#7009-7012

Comment 17

[John Dai[:jdai]]

Attachment: Part 3: Bug 1088761 - Update web-platform tests for reportValidity(). v3

Update form-validation-validate.html testcase.

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b7b130f2b3997ab04222b0781a322e59dadca21

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8742321 [details] [diff] [review]
Part 3: Bug 1088761 - Update web-platform tests for reportValidity(). v3

>@@ -1,311 +1,48 @@
> [form-validation-reportValidity.html]
>   type: testharness
>-  [[INPUT in TEXT status\] no constraint]
>-    expected: FAIL
>-
>-  [[INPUT in TEXT status\] no constraint (in a form)]
>-    expected: FAIL
>-
>+  expected: TIMEOUT
Hmm, so the timeout happens on some debug builds only. I guess that is ok-ish then.
Is the test run fast in opt-builds? 

>-  [[INPUT in DATE status\] suffering from being missing (in a form)]
>-    expected: FAIL
>+      expected: FAIL
something odd here? Accidentally adding two spaces before 'expected'?

>+++ b/testing/web-platform/tests/html/semantics/forms/constraints/form-validation-validate.html
>@@ -94,16 +94,20 @@
>     assert_true(fm3.checkValidity(), "The checkValidity method should be true.");
> 
>     document.getElementById("inp1").value = "aaa";
>     document.getElementById("inp1").type = "url";
>     assert_false(fm3.checkValidity(), "The checkValidity method should be false.");
>   }, "Check the checkValidity method of the form element when it has a fieldset child");
> 
>   test(function(){
>+    document.getElementById("fs").disabled = false;
>+    document.getElementById("inp1").value = "";
>+    document.getElementById("inp1").type = "text";
>+
Took a bit time to understand why this change, but yes, looks like a bug in the test. This just
puts the state back to the same where it was before the previous test.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Note to myself, form-validation-validate.html has tests for invalid event.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8742214 [details] [diff] [review]
Part 2: Bug 1088761 - Support reportValidity() for form controls. v2

># HG changeset patch
># User John Dai <[e-mail]>
>Bug 1088761 - Support reportValidity() for form controls.
>
>
>diff --git a/browser/modules/FormSubmitObserver.jsm b/browser/modules/FormSubmitObserver.jsm
>index cf2b098..b2656d5 100644
>--- a/browser/modules/FormSubmitObserver.jsm
>+++ b/browser/modules/FormSubmitObserver.jsm
>@@ -101,17 +101,17 @@ FormSubmitObserver.prototype =
>     // first invalid element and show the corresponding validation message in a
>     // panel attached to the element.
>     if (!aInvalidElements.length) {
>       return;
>     }
> 
>     // Insure that this is the FormSubmitObserver associated with the form
>     // element / window this notification is about.
>-    if (this._content != aFormElement.ownerDocument.defaultView.top.document.defaultView) {
>+    if (aFormElement && (this._content != aFormElement.ownerDocument.defaultView.top.document.defaultView)) {
>       return;
>     }
You need to update also the nsIFormSubmitObserver for Android.
And we don't want to lose that check. So could you change it so that you ...

> 
>     let element = aInvalidElements.queryElementAt(0, Ci.nsISupports);
...move the check to test whether this._content != element.ownerDocument.defaultView.top.document.defaultView,
assuming element isn't null.
Applies to http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#5259
too.

aFormElement becomes unneeded parameter, but for addon compatibility we can still keep it.


>+nsIConstraintValidation::ReportValidity()
>+{
>+  if (!IsCandidateForConstraintValidation() || IsValid()) {
>+    return true;
>+  }
>+
>+  nsCOMPtr<nsIContent> content = do_QueryInterface(this);
>+  NS_ASSERTION(content, "This class should be inherited by HTML elements only!");
could be MOZ_ASSERT (which is stronger, crashes in debug builds by default)

>+
>+  nsresult rv = NS_OK;
>+  rv = nsContentUtils::DispatchTrustedEvent(content->OwnerDoc(), content,
>+                                            NS_LITERAL_STRING("invalid"),
>+                                            false, true);
You need to use the optional last param of DispatchTrustedEvent to see whether the event was canceled. rv doesn't tell that.
"fire a simple event named invalid that is cancelable at the element, and if that event is not canceled, report the problems with the constraints of that element to the user; then, return false."
And only if the event is not cancelled, report error to user.
https://html.spec.whatwg.org/multipage/forms.html#dom-cva-reportvalidity

Comment 21

[John Dai[:jdai]]

(In reply to Olli Pettay [:smaug] from comment #18)
> Comment on attachment 8742321 [details] [diff] [review]
> Part 3: Bug 1088761 - Update web-platform tests for reportValidity(). v3
> 
> >@@ -1,311 +1,48 @@
> > [form-validation-reportValidity.html]
> >   type: testharness
> >-  [[INPUT in TEXT status\] no constraint]
> >-    expected: FAIL
> >-
> >-  [[INPUT in TEXT status\] no constraint (in a form)]
> >-    expected: FAIL
> >-
> >+  expected: TIMEOUT
> Hmm, so the timeout happens on some debug builds only. I guess that is
> ok-ish then.
> Is the test run fast in opt-builds? 
> 
> >-  [[INPUT in DATE status\] suffering from being missing (in a form)]
> >-    expected: FAIL
> >+      expected: FAIL
> something odd here? Accidentally adding two spaces before 'expected'?

Didn't notice that, thanks for pointing it out!
> 
> >+++ b/testing/web-platform/tests/html/semantics/forms/constraints/form-validation-validate.html
> >@@ -94,16 +94,20 @@
> >     assert_true(fm3.checkValidity(), "The checkValidity method should be true.");
> > 
> >     document.getElementById("inp1").value = "aaa";
> >     document.getElementById("inp1").type = "url";
> >     assert_false(fm3.checkValidity(), "The checkValidity method should be false.");
> >   }, "Check the checkValidity method of the form element when it has a fieldset child");
> > 
> >   test(function(){
> >+    document.getElementById("fs").disabled = false;
> >+    document.getElementById("inp1").value = "";
> >+    document.getElementById("inp1").type = "text";
> >+
> Took a bit time to understand why this change, but yes, looks like a bug in
> the test. This just
> puts the state back to the same where it was before the previous test.
Sorry about that. I should add comments in the testcase.

Comment 22

[John Dai[:jdai]]

Attachment: Part 2: Bug 1088761 - Support reportValidity() for form controls. v3

Address comment 20 which are:
 - Update the nsIFormSubmitObserver for Android.
 - Use MOZ_ASSERT instead of NS_ASSERTION.
 - Use optional last param of DispatchTrustedEvent to see whether the event was canceled.

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d85da3c7f4456070df3f8d97edb308eaf15f24e

Comment 23

[John Dai[:jdai]]

Attachment: Part 3: Bug 1088761 - Update web-platform tests for reportValidity(). (Final) r=smaug

Address comment 18 and carry on r+.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8742675 [details] [diff] [review]
Part 2: Bug 1088761 - Support reportValidity() for form controls. v3

># HG changeset patch
># User John Dai <[e-mail]>
>Bug 1088761 - Support reportValidity() for form controls.
>
>
>diff --git a/browser/modules/FormSubmitObserver.jsm b/browser/modules/FormSubmitObserver.jsm
>index cf2b098..b2656d5 100644
>--- a/browser/modules/FormSubmitObserver.jsm
>+++ b/browser/modules/FormSubmitObserver.jsm
>@@ -101,17 +101,17 @@ FormSubmitObserver.prototype =
>     // first invalid element and show the corresponding validation message in a
>     // panel attached to the element.
>     if (!aInvalidElements.length) {
>       return;
>     }
> 
>     // Insure that this is the FormSubmitObserver associated with the form
>     // element / window this notification is about.
>-    if (this._content != aFormElement.ownerDocument.defaultView.top.document.defaultView) {
>+    if (aFormElement && (this._content != aFormElement.ownerDocument.defaultView.top.document.defaultView)) {
>       return;
>     }
> 
>     let element = aInvalidElements.queryElementAt(0, Ci.nsISupports);

Oh, I was hoping that we don't use aFormElement for the check, but the element here. 
So could we do this after let element = ...:
   // Insure that this is the FormSubmitObserver associated with the
   // element / window this notification is about.
   if (this._content != element.ownerDocument.defaultView.top.document.defaultView) {
     return;
   }
and not use aFormElement at all

>+nsIConstraintValidation::ReportValidity()
>+{
>+  if (!IsCandidateForConstraintValidation() || IsValid()) {
>+    return true;
>+  }
>+
>+  nsCOMPtr<nsIContent> content = do_QueryInterface(this);
>+  MOZ_ASSERT(content, "This class should be inherited by HTML elements only!");
>+
>+  bool defaultAction = true;
>+  nsContentUtils::DispatchTrustedEvent(content->OwnerDoc(), content,
>+                                       NS_LITERAL_STRING("invalid"),
>+                                       false, true, &defaultAction);
>+  if (defaultAction) {
>+    nsCOMPtr<nsIObserverService> service =
>+      mozilla::services::GetObserverService();
>+    if (!service) {
>+      NS_WARNING("No observer service available!");
>+      return true;
>+    }
>+
>+    nsCOMPtr<nsISimpleEnumerator> theEnum;
>+    nsresult rv = service->EnumerateObservers(NS_INVALIDFORMSUBMIT_SUBJECT,
>+                                              getter_AddRefs(theEnum));
>+
>+    // Return true on error here because that's what we always did
>+    NS_ENSURE_SUCCESS(rv, true);
>+
>+    bool hasObserver = false;
>+    rv = theEnum->HasMoreElements(&hasObserver);
>+
>+    nsCOMPtr<nsIMutableArray> invalidElements =
>+      do_CreateInstance(NS_ARRAY_CONTRACTID, &rv);
>+    invalidElements->AppendElement(content, false);
>+
>+    NS_ENSURE_SUCCESS(rv, true);
>+    nsCOMPtr<nsISupports> inst;
>+    nsCOMPtr<nsIFormSubmitObserver> observer;
>+    bool more = true;
>+    while (NS_SUCCEEDED(theEnum->HasMoreElements(&more)) && more) {
>+      theEnum->GetNext(getter_AddRefs(inst));
>+      observer = do_QueryInterface(inst);
>+
>+      if (observer) {
>+        observer->NotifyInvalidSubmit(nullptr,
>+                                      static_cast<nsIArray*>(invalidElements));
nsIMutableArray inherits nsIArray, so this case shouldn't be needed.
(looks like we have other code doing this kind of extra cast, but no need to copy-paste that :) )

>+      }
>+    }
>+
>+    if (content->IsHTMLElement(nsGkAtoms::input) &&
>+        nsContentUtils::IsFocusedContent(content)) {
>+      HTMLInputElement* inputElement =
>+      HTMLInputElement::FromContentOrNull(content);
>+
>+      inputElement->UpdateValidityUIBits(true);
>+    }
>+
>+    dom::Element* element = content->AsElement();
>+    element->UpdateState(true);
>+    return false;
>+  }
>+  return true;
This is not what the spec says, right? We should return false even if the the event was cancelled.
I know the spec language is weird, so asked native English speaker's interpretation too :)
http://logs.glob.uno/?c=freenode%23whatwg&s=20+Apr+2016&e=20+Apr+2016#c992782
Could you add a test which checks that the return value doesn't change even if invalid event is .preventDefault() 'ed.

>+++ b/mobile/android/chrome/content/browser.js
>@@ -5250,18 +5250,18 @@ var FormAssistant = {
>     }
>   },
> 
>   notifyInvalidSubmit: function notifyInvalidSubmit(aFormElement, aInvalidElements) {
>     if (!aInvalidElements.length)
>       return;
> 
>     // Ignore this notificaiton if the current tab doesn't contain the invalid form
>-    if (BrowserApp.selectedBrowser.contentDocument !=
>-        aFormElement.ownerDocument.defaultView.top.document)
>+    if (aFormElement && (BrowserApp.selectedBrowser.contentDocument !=
>+        aFormElement.ownerDocument.defaultView.top.document))
>       return;
I think also this should use first element in aInvalidElements to check the ownerDocument and such

Those fixed, r+
Feel free to ask review again, if you think it is needed.

Comment 25

[John Dai[:jdai]]

(In reply to Olli Pettay [:smaug] from comment #24)
> Comment on attachment 8742675 [details] [diff] [review]
> Part 2: Bug 1088761 - Support reportValidity() for form controls. v3
> 
> ># HG changeset patch
> ># User John Dai <[e-mail]>
> >Bug 1088761 - Support reportValidity() for form controls.
> >
> >
> >diff --git a/browser/modules/FormSubmitObserver.jsm b/browser/modules/FormSubmitObserver.jsm
> >index cf2b098..b2656d5 100644
> >--- a/browser/modules/FormSubmitObserver.jsm
> >+++ b/browser/modules/FormSubmitObserver.jsm
> >@@ -101,17 +101,17 @@ FormSubmitObserver.prototype =
> >     // first invalid element and show the corresponding validation message in a
> >     // panel attached to the element.
> >     if (!aInvalidElements.length) {
> >       return;
> >     }
> > 
> >     // Insure that this is the FormSubmitObserver associated with the form
> >     // element / window this notification is about.
> >-    if (this._content != aFormElement.ownerDocument.defaultView.top.document.defaultView) {
> >+    if (aFormElement && (this._content != aFormElement.ownerDocument.defaultView.top.document.defaultView)) {
> >       return;
> >     }
> > 
> >     let element = aInvalidElements.queryElementAt(0, Ci.nsISupports);
> 
> Oh, I was hoping that we don't use aFormElement for the check, but the
> element here. 
> So could we do this after let element = ...:
>    // Insure that this is the FormSubmitObserver associated with the
>    // element / window this notification is about.
>    if (this._content !=
> element.ownerDocument.defaultView.top.document.defaultView) {
>      return;
>    }
> and not use aFormElement at all
> 
After I looked again comment 20, I did miss this change. Thanks for pointing this out.
> >+nsIConstraintValidation::ReportValidity()
> >+{
> >+  if (!IsCandidateForConstraintValidation() || IsValid()) {
> >+    return true;
> >+  }
> >+
> >+  nsCOMPtr<nsIContent> content = do_QueryInterface(this);
> >+  MOZ_ASSERT(content, "This class should be inherited by HTML elements only!");
> >+
> >+  bool defaultAction = true;
> >+  nsContentUtils::DispatchTrustedEvent(content->OwnerDoc(), content,
> >+                                       NS_LITERAL_STRING("invalid"),
> >+                                       false, true, &defaultAction);
> >+  if (defaultAction) {
> >+    nsCOMPtr<nsIObserverService> service =
> >+      mozilla::services::GetObserverService();
> >+    if (!service) {
> >+      NS_WARNING("No observer service available!");
> >+      return true;
> >+    }
> >+
> >+    nsCOMPtr<nsISimpleEnumerator> theEnum;
> >+    nsresult rv = service->EnumerateObservers(NS_INVALIDFORMSUBMIT_SUBJECT,
> >+                                              getter_AddRefs(theEnum));
> >+
> >+    // Return true on error here because that's what we always did
> >+    NS_ENSURE_SUCCESS(rv, true);
> >+
> >+    bool hasObserver = false;
> >+    rv = theEnum->HasMoreElements(&hasObserver);
> >+
> >+    nsCOMPtr<nsIMutableArray> invalidElements =
> >+      do_CreateInstance(NS_ARRAY_CONTRACTID, &rv);
> >+    invalidElements->AppendElement(content, false);
> >+
> >+    NS_ENSURE_SUCCESS(rv, true);
> >+    nsCOMPtr<nsISupports> inst;
> >+    nsCOMPtr<nsIFormSubmitObserver> observer;
> >+    bool more = true;
> >+    while (NS_SUCCEEDED(theEnum->HasMoreElements(&more)) && more) {
> >+      theEnum->GetNext(getter_AddRefs(inst));
> >+      observer = do_QueryInterface(inst);
> >+
> >+      if (observer) {
> >+        observer->NotifyInvalidSubmit(nullptr,
> >+                                      static_cast<nsIArray*>(invalidElements));
> nsIMutableArray inherits nsIArray, so this case shouldn't be needed.
> (looks like we have other code doing this kind of extra cast, but no need to
> copy-paste that :) )
> 
Will do.
> >+      }
> >+    }
> >+
> >+    if (content->IsHTMLElement(nsGkAtoms::input) &&
> >+        nsContentUtils::IsFocusedContent(content)) {
> >+      HTMLInputElement* inputElement =
> >+      HTMLInputElement::FromContentOrNull(content);
> >+
> >+      inputElement->UpdateValidityUIBits(true);
> >+    }
> >+
> >+    dom::Element* element = content->AsElement();
> >+    element->UpdateState(true);
> >+    return false;
> >+  }
> >+  return true;
> This is not what the spec says, right? We should return false even if the
> the event was cancelled.
> I know the spec language is weird, so asked native English speaker's
> interpretation too :)
> http://logs.glob.uno/?c=freenode%23whatwg&s=20+Apr+2016&e=20+Apr+2016#c992782
> Could you add a test which checks that the return value doesn't change even
> if invalid event is .preventDefault() 'ed.
> 
Yes. you are right. Sorry for spec interpret error.
I will add a test for return value doesn't change when invalid event is .preventDefault() 'ed in next patch. 
> >+++ b/mobile/android/chrome/content/browser.js
> >@@ -5250,18 +5250,18 @@ var FormAssistant = {
> >     }
> >   },
> > 
> >   notifyInvalidSubmit: function notifyInvalidSubmit(aFormElement, aInvalidElements) {
> >     if (!aInvalidElements.length)
> >       return;
> > 
> >     // Ignore this notificaiton if the current tab doesn't contain the invalid form
> >-    if (BrowserApp.selectedBrowser.contentDocument !=
> >-        aFormElement.ownerDocument.defaultView.top.document)
> >+    if (aFormElement && (BrowserApp.selectedBrowser.contentDocument !=
> >+        aFormElement.ownerDocument.defaultView.top.document))
> >       return;
> I think also this should use first element in aInvalidElements to check the
> ownerDocument and such
Will do.
> 
> Those fixed, r+
> Feel free to ask review again, if you think it is needed.

Comment 26

[John Dai[:jdai]]

Attachment: Part 2: Bug 1088761 - Support reportValidity() for form controls. v4

Address comment 24, which are:
 - Don't use aFormElement in browser and android.
 - Remove static_cast code.
 - Update if the the event was cancelled, it should return false.

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&selectedJob=19728479

Comment 27

[John Dai[:jdai]]

Attachment: Part 3: Bug 1088761 - Update web-platform tests for reportValidity(). v4

Address comment 24, add a test_reportValidation_preventDefault.html testcase to check that the return value doesn't change even if invalid event is .preventDefault() 'ed.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8743271 [details] [diff] [review]
Part 2: Bug 1088761 - Support reportValidity() for form controls. v4


>+  if (content->IsHTMLElement(nsGkAtoms::input) &&
>+      nsContentUtils::IsFocusedContent(content)) {
>+    HTMLInputElement* inputElement =
>+    HTMLInputElement::FromContentOrNull(content);
Nit, missing indentation here. Add 2 spaces before HTMLInputElement::FromContentOrNull(content);


>-    // Ignore this notificaiton if the current tab doesn't contain the invalid form
>+    // Ignore this notificaiton if the current tab doesn't contain the invalid element
You could fix the existing spelling mistake here s/notificaiton/notification/

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8743273 [details] [diff] [review]
Part 3: Bug 1088761 - Update web-platform tests for reportValidity(). v4

>+function invalidEventHandler(aEvent, isPreventDefault)
>+{
>+  function checkInvalidEvent(aEvent, isPreventDefault)
>+  {
>+    if (isPreventDefault) {
>+        aEvent.preventDefault();
>+    }
>+
>+    is(aEvent.type, "invalid", "Invalid event type should be invalid");
>+    ok(!aEvent.bubbles, "Invalid event should not bubble");
>+    ok(aEvent.cancelable, "Invalid event should be cancelable");
>+  }
>+
>+  checkInvalidEvent(aEvent, isPreventDefault);
>+
>+  gInvalid = true;

Don't you want to set gInvalid to false in check checkReportValidity before reportValidity() call, otherwise it is true all the time.

>+// Not checking button, fieldset, object and keygen
>+// because they are always barred from constraint validation.
Well, you could test those too and just then have different checkReportValidity for them.
Perhaps the existing method could be called checkReportValidityForInvalid and add a new function for the cases where
element is always valid
function checkReportValidityForValid(element) {
  gInvalid = false;
  ok(element.reportValidity(), "reportValidity() should return true when the element is valid");
  ok(!gInvalid, "Invalid event shouldn't have been handled");
}

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

FYI, the spec language is about to be improved a bit here https://github.com/whatwg/html/issues/1075

Comment 31

[John Dai[:jdai]]

Attachment: Part 3: Bug 1088761 - Update web-platform tests for reportValidity(). v5

Address comment #29.

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2aa0573a5fe965750ba12940d62865a1256b0bd5

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8750705 [details] [diff] [review]
Part 3: Bug 1088761 - Update web-platform tests for reportValidity(). v5

>+function invalidEventHandler(aEvent, isPreventDefault)
>+{
>+  function checkInvalidEvent(aEvent, isPreventDefault)
>+  {
>+    if (isPreventDefault) {
>+        aEvent.preventDefault();
>+    }
>+
>+    is(aEvent.type, "invalid", "Invalid event type should be invalid");
>+    ok(!aEvent.bubbles, "Invalid event should not bubble");
>+    ok(aEvent.cancelable, "Invalid event should be cancelable");
>+  }
>+
>+  checkInvalidEvent(aEvent, isPreventDefault);
Actually, why we need checkInvalidEvent? Couldn't you just inline all that code where the function is called.
With that, r+

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

(so, conditional r+. No need to ask review again if you tweak the patch as requested.)

Comment 34

[John Dai[:jdai]]

Attachment: Part 1: Bug 1088761 - Remove redundant trailing spaces. (Final) r=smaug

Comment 35

[John Dai[:jdai]]

Attachment: Part 2: Bug 1088761 - Support reportValidity() for form controls. (Final) r=smaug

Comment 36

[John Dai[:jdai]]

Attachment: Part 3: Bug 1088761 - Update web-platform tests for reportValidity(). (Final) r=smaug

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1499f94e5d98761cd0fb35dc937c00df1bc117e0

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ff28843a8f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/403d9c085544
https://hg.mozilla.org/integration/mozilla-inbound/rev/f757f585e618

Comment 38

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing (or better: unexpected passing) form-validation-reportValidity.html on OSX 10.10 debug.

Backouts:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f5f4e6372c5b1f73ad0910cda7ba0dea4e0f52e
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c41600ba212e95a893659e7ec0324d4a99de79b
https://hg.mozilla.org/integration/mozilla-inbound/rev/05366ca62a430c90e3d2f99bde140674b2172102

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f94661bb15af8d6e91176e4e26ffa4aad0e7b927
"Failure" log: https://treeherder.mozilla.org/logviewer.html#?job_id=27737564&repo=mozilla-inbound
TEST-UNEXPECTED-OK | /html/semantics/forms/constraints/form-validation-reportValidity.html | expected TIMEOUT

Comment 39

[John Dai[:jdai]]

Sorry for the inconvenience, I will fix that and re-land my patch.

Comment 40

[John Dai[:jdai]]

Filed Bug 1273105 for comment 16.

Comment 41

[John Dai[:jdai]]

Attachment: Part 3: Bug 1088761 - Update web-platform tests for reportValidity(). (Final, v2) r=smaug

Add skip-if in wpt form-validation-reportValidity.html.

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03b8248d4af4c9a53385dcec56ef220b50fce5dc

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/525496d155a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6ed04dee175
https://hg.mozilla.org/integration/mozilla-inbound/rev/88c37d09e1b7

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/525496d155a8
https://hg.mozilla.org/mozilla-central/rev/f6ed04dee175
https://hg.mozilla.org/mozilla-central/rev/88c37d09e1b7

Comment 44

[sliploj]

reportValidity() seems to be permanently affecting the state of a form (making it impossible to clear validation status, even if the form is reset) -- please see bug 1427542

Comment 45

[erick.tucto]

Firefox on android is not showing validation

Test 1 (default message)

• click on button test

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <title>Document</title>
</head>
<body>
    <form action="">
    <input type="text" required/>
    <button type="submit">test</button>
    </form>
</body>
</html>

Test 2 (custom message)

• type on input

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <title>Document</title>
</head>
<body>
    <input type="text" required/>
    <script>
        (() => {
            const $input = document.querySelector("input");
            $input.addEventListener('input', () => {
                $input.setCustomValidity("Hola Erick");
                $input.reportValidity();
            })
        })();
    </script>
</body>
</html>