Closed Bug 345624 Opened 18 years ago Closed 14 years ago

Implement HTML 5's Constraint Validation API

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b5

People

(Reporter: WeirdAl, Assigned: mounir)

References

()

Details

(Keywords: dev-doc-complete, html5)

Attachments

(2 files, 23 obsolete files)

75.76 KB, image/png
Details
78.13 KB, patch
Details | Diff | Splinter Review
We need to lay down the groundwork for validation checking at the form level before we can lay down specific form control validation routines.
Assignee: general → ajvincent
Attached patch patch (work in progress) (obsolete) — Splinter Review
This implements a basic checkValidity routine for form elements.  Because there is no implementation of the proposed nsIDOMWF2FormControl yet, this patch is so far untestable and I am therefore not asking for reviews at this time.
Attached patch patch (almost ready for reviews) (obsolete) — Splinter Review
Attachment #230385 - Attachment is obsolete: true
Comment on attachment 230400 [details] [diff] [review]
patch (almost ready for reviews)

missed a file
Attachment #230400 - Attachment is obsolete: true
Attached patch patch (almost ready for reviews) (obsolete) — Splinter Review
There are a few problems that I'm aware of with this patch.  Help wanted to fix these.

(1) ###!!! ASSERTION: Class name and proto chain interface name mismatch!: 'nsCRT::strcmp(CutPrefix(name), mData->mName) == 0', file m:/webforms2/mozilla/dom/src/base/nsDOMClassInfo.cpp, line 3378
name == "nsIDOMWF2ValidityState", mData->mName == "DOMWF2ValidityState"

(2) nsHTMLInputElement::CheckValidity is supposed to dispatch invalid events in the XML Events namespace for any validation problems.  I've not sifted through to determine how to do that.

Test steps through Venkman's console on http://www.mozilla.org/projects/seamonkey/start/ :

0001: document.forms[0]
[object HTMLFormElement @ 0x4fa6048 (native @ 0x3beb220)]
0002: document.forms[0].elements[3]
[object HTMLInputElement @ 0x4fdd780 (native @ 0x3be5650)]
0003: document.forms[0].elements[3].type
text
0004: document.forms[0].elements[3].value
(I entered "foo" into the search box.)
0005: document.forms[0].elements[3].value
foo
0006: document.forms[0].elements[3].validity
[object DOMWF2ValidityState @ 0x521c688 (native @ 0x521bc20)]
0007: document.forms[0].elements[3].validity
[object DOMWF2ValidityState @ 0x521c688 (native @ 0x521bc20)]
0008: document.forms[0].elements[3].validity.valid
true
0009: document.forms[0].elements[3].checkValidity()
true
00010: document.forms[0].checkValidity()
true
00011: document.forms[0].elements[3].willValidate
true
00012: document.forms[0].elements[3].disabled=true
true
00013: document.forms[0].elements[3].willValidate
false
00014: document.forms[0].elements[3].disabled=false
false
00015: document.forms[0].elements[3].willValidate
true
00016: document.forms[0].elements[3].removeAttribute("name")
undefined
00017: document.forms[0].elements[3].willValidate
false
00018: document.forms[0].elements[3].name="foo"
foo
00019: document.forms[0].elements[3].willValidate
true
Blocks: 345822
The actual patch for this bug will go hand-in-hand with the patch for bug 345822, which implements code using form.checkValidity().
Component: DOM: HTML → DOM: Core & HTML
QA Contact: ian → general
If the validity of a form control isn't given, nsIAccessibleStates::STATE_INVALID must be set on such a WebForms control in the accessible module. This is to communicate this fact to screen readers. Obviously, once this is no longer invalid, the state flag must be cleared again.
I've been thinking... it'd almost be trivial to add support for HTML 5's setCustomValidity, and that would give us a real use case for ValidityState right away.  Arguably, the most flexible one of all - setCustomValidity is supposed to be for script-driven validation.
I decided the best way to go forward on this patch was to add a Mochitest and see what it would actually do.

I also decided, per comment 7, to roll in the setCustomValidity code, so it would actually have something to do.  I'm currently thinking of rolling in the pattern support code from bug 345512, but I haven't decided on that yet.

As it so happens, there's a serious bug that I haven't been able to fix yet - which is why the test exits so early.  I tried dispatching a DOM "invalid" event using the EventDispatcher code, but it didn't seem to work - it dispatched the GUI event without the corresponding DOM event.  I could use some help figuring that out - or if I'm supposed to use the standard DOM methods for the event.

I spent most of Saturday and part of Sunday on this, and so far it's done fairly well.
Attachment #230408 - Attachment is obsolete: true
What kind of event did you try to dispatch?
If it is just a simple event which implements nsIDOMEvent, the easiest
way to do that is to use nsContentUtils::DispatchTrustedEvent.
And you need to add something to nsContentUtils::InitializeEventTable()
Comment on attachment 396216 [details] [diff] [review]
patch with mochitest and custom validity, work in progress

(In reply to comment #9)
> What kind of event did you try to dispatch?
> If it is just a simple event which implements nsIDOMEvent, the easiest
> way to do that is to use nsContentUtils::DispatchTrustedEvent.

This is what I tried to do, based on other code I saw in the tree.

>+  {
>+    // Dispatch an invalid event at the control.
>+    nsFormEvent event(PR_TRUE, NS_FORM_INVALID);
>+    nsEventDispatcher::Dispatch(mContent, nsnull, &event);
>+  }

I'll try your suggestions tonight, thanks!
Summary: Implement Web Forms 2 form.checkValidity() → Implement HTML 5's HTMLFormElement.checkValidity()
In this patch, the setCustomValidity code works, as does dispatching events.

I'm trying for something more ambitious, though:  resolving support for pattern, required and maxlength as well.  Those parts haven't been tested yet (as you can see, the test hasn't been completed for those yet), or fully implemented (I need to come up with some .properties file for validation messages for the non-custom errors).
Attachment #396216 - Attachment is obsolete: true
OK!  I've taken this as far as I can.  customError works, form validation works, tooLong works, valueMissing works... and nsRegularExpression triggers a JSAssert very, very quickly.

DOM peers:  would you like to take a patch without patternMismatch support?

JSENG:  Help wanted resolving the crash (and others which are bound to be lurking in here!)  I'm currently dying in JS_NewObject with CHECK_REQUEST(cx).
Attachment #398394 - Attachment is obsolete: true
Attached file stack trace for comment 13 (obsolete) —
Attached patch patch, work in progress (obsolete) — Splinter Review
Thanks to timeless for helping out on comment 13 - JSAPI issues have been resolved.  He also spotted a few mistakes and suggested fixes, which I've implemented.  So now the patch supports patternMismatch as well.

Two things left that I'm aware of:
(1) I asked bz his opinion as a DOM peer what I hadn't covered.  His answer was CSS pseudo-classes (:valid, :invalid).  Bug 506554 also mentions :required and :optional, and I might as well do them at the same time.

Spec:  http://www.whatwg.org/specs/web-apps/current-work/multipage/interactive-elements.html#matching-html-elements-using-selectors

(2) This patch leaks when the mochitest runs.  A lot.  I have no idea why though.  I'd welcome some assistance on that.
Attachment #398617 - Attachment is obsolete: true
Attachment #398618 - Attachment is obsolete: true
Attached patch patch, v1 (obsolete) — Splinter Review
This covers everything I can think of.

Talking it over with smaug, he sounded willing to review the non-JSAPI parts.  The JSAPI parts are at the top of the patch, in nsRegularExpression.

We now have pseudo-classes, and the tests for those are reftests.  Unfortunately, the validity state part of the code grew more complex as a result.  Plus, I had to put in some scaffolding to prevent stack overflows.  I'm not really pleased with that.

Also, I fixed the leaks.  The node leaks were from calling nsIFormControl::GetForm() - I didn't know there was an addref.  The URL leaks I fixed by moving the RemoveMutationObserver into a new NodeWillBeDestroyed method.  (Maybe a NS_WARNING for nsStubMutationListener::NodeWillBeDestroyed, as a note for these URL leaks?)
Attachment #398920 - Attachment is obsolete: true
Attachment #399045 - Flags: review?(Olli.Pettay)
Attachment #399045 - Flags: review?(jorendorff)
Comment on attachment 399045 [details] [diff] [review]
patch, v1

Requesting JSAPI review on nsRegularExpression code.
The validation constraints for the button element have changed since I posted this patch.
Sorry for the delay!

- Could you explain why you need nsIDOMHTMLValidityTearoff.
  You could just add the properties to the existing interfaces and
  make some generic helper class to implement them.

- nsHTMLValidityOwner, nsHTMLValidityTearoff and nsHTMLValidityObserver
  should be probably merged together. Currently there are quite a few
  different classes.

- nsHtml5Atoms isn't used in content/ currently. Could you just add the
  atom to nsGkAtoms. At least for now.

- In nsHTMLFormElement::CheckValidity you dispatch events in a loop, but nothing
  ensures that owner stays alive. (Currently elements don't have strong
  reference to their ownerDocument)
Attachment #399045 - Flags: review?(Olli.Pettay) → review-
(In reply to comment #19)
> Sorry for the delay!

What delay?  That's one of the fastest reviews I've had!  :-)  Especially for such a big patch.

> - Could you explain why you need nsIDOMHTMLValidityTearoff.
>   You could just add the properties to the existing interfaces and
>   make some generic helper class to implement them.

Perhaps we don't need it.  My original intent was less code duplication. (There's five different HTML elements that need it, but not every form control needs it.)

> - nsHTMLValidityOwner, nsHTMLValidityTearoff and nsHTMLValidityObserver
>   should be probably merged together. Currently there are quite a few
>   different classes.

I originally wanted to keep the mutation observer separate because I believed either all HTML elements or all elements implemented nsIMutationObserver.  Apparently that's not the case, so maybe we can roll all of them up into the base elements.

> - In nsHTMLFormElement::CheckValidity you dispatch events in a loop, but
> nothing
>   ensures that owner stays alive. (Currently elements don't have strong
>   reference to their ownerDocument)

Could you write up a testcase for this, please?  I do not know what exactly you mean by this, but I think you mean the document might get destroyed by an invalid event listener... I don't see that happening.

Also, I forgot about the a11y request in comment 6.
(In reply to comment #20)
> Could you write up a testcase for this, please?  I do not know what exactly you
> mean by this, but I think you mean the document might get destroyed by an
> invalid event listener... I don't see that happening.
There is nothing "invalid" in the listener. 
I don't have a testcase, but event listeners can destroy "the world".
window, document, document tree, all.
Common case is having iframe and calling window.frameElement.parentNode.removeChild(window.frameElement) in the iframe window.
(In reply to comment #21)
I think you mean the document might get destroyed by an
> > invalid event listener... I don't see that happening.
> There is nothing "invalid" in the listener. 

Poor phrasing:  I meant an event listener listening for the "invalid" event type - which this code introduces.

> Common case is having iframe and calling
> window.frameElement.parentNode.removeChild(window.frameElement) in the iframe
> window.

Okay, that's what I thought you meant.
Comment on attachment 399045 [details] [diff] [review]
patch, v1

>diff --git a/dom/base/nsRegularExpression.h b/dom/base/nsRegularExpression.h
>+class nsRegularExpression {
...
>+    JSContext* mContext;

A JSContext for every form field is a bit much. Every page already has its own JSContext; you should be able to just pass it in to the methods that need it.

Creating and populating a new global is unnecessarily heavyweight, too. You should just call JS_NewUCRegExpObject and store the result as a JSObject* field of the nsRegularExpression. However, this creates two new problems:

1. GC reachability. If this nsRegularExpression class is only ever used on the C stack, it is appropriate to make the JSObject* field a JSAutoTempValueRooter. If it is to be a member of a COM object, it needs to follow tracing rules which I don't fully understand--David Baron and Graydon Hoare know them quite well, and I think Johnny Stenback does too.

2. In TestValue(), you can't just JS_CallFunctionName(cx, regexpobj, "test" ...) because content can tinker with the test method (RegExp.prototype.test). The right way to do it is to add a new function JS_TestRegExp() in js/src/jsapi.h.  I'm in favor of doing that. If you look, you'll actually see a TODO comment in both jsapi.h and jsapi.cpp for this.

>+PRBool
>+nsRegularExpression::TestValue(nsString & value)

The implementation of this method is more work for the JS engine that it should be. We really want JS_TestRegExp here.
Attachment #399045 - Flags: review?(jorendorff) → review-
Jason, thanks for your review comments - it might take me some time to understand them, though.

One concern I have is possibly polluting the web page's global RegExp properties:  .$1 through .$9, .lastMatch, .lastParen, .leftContext, .rightContext, .multiline.  Could that happen with a JS_TestRegExp (or a future JS_MatchRegExp) method?
Yes, it can:

js> const re = /(abc)\w+/;
js> typeof RegExp.lastParen
string
js> RegExp.lastParen

js> re.test("abcde")
true
js> RegExp.lastParen
abc

Now, if a web page is looking at any of these properties, and we try to validate with the pattern attribute, the web page's RegExp.lastParen property could mutate unpredictably.  That is not good.

I may have to add an argument to js_ExecuteRegExp to preserve cx->regExpStatics's current values.
(In reply to comment #25)
> I may have to add an argument to js_ExecuteRegExp to preserve
> cx->regExpStatics's current values.

No, instead use

jsregexp.h:js_SaveRegExpStatics(JSContext *cx, JSRegExpStatics *statics,
jsregexp.h:js_RestoreRegExpStatics(JSContext *cx, JSRegExpStatics *statics,

/be
(In reply to comment #23)
> Every page already has its own
> JSContext; you should be able to just pass it in to the methods that need it.

How do I get that JSContext?
OS: Windows XP → All
Hardware: x86 → All
Something like
node->GetOwnerDoc()->GetScriptGlobalObject()->GetContext()->GetNativeContext()
(null checks needed)
Blocks: 273046
Maybe we should focus on implementing the constraint validation API without the validations in a first time. Some validation implementation need elements not in Gecko (input type="number" for example) or worth an entire bug because they are complex (pattern).
So, I think the best strategy is having the API telling everything is valid then, implements the checks for each ones.

For more information about this API, look at $URL
Mounir: could you please link to recent drafts, rather than the outdated /TR/ version?
Attached patch Patch v0.1 (obsolete) — Splinter Review
I've contacted Alex and it looks like I will have more time than him to dedicate to this bug so I'm taking it.

As I said to him, I choose to write the patch without re-using the patch attached. That is not a matter of quality but only to have two versions to compare and a way for me to understand what I am doing because a lot of HTML5 Forms stuff is going to be based on that.

This patch is only implementing the constraint validation API for the listed form elements with two working checks : tooLong and customError. Actually, those are the only one available as the other ones need new attributes. I am not implementing CSS rules too nor the invalid event and the validity checks from the form element.
However, this patch is complete enough to compare both implementation 'spirit' so I would appreciate if you (Alex, Olli and anyone interested) can provide me some feedback. Also, if you can found a clean way to access to the content from nsConstraintValidation, it would be appreciated because at the moment, I have to do all checks even common ones with re-defined virtual functions.
Assignee: ajvincent → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #432162 - Flags: feedback?(Olli.Pettay)
Attachment #432162 - Flags: feedback?(ajvincent)
Attached patch Test v0.1 (obsolete) — Splinter Review
Just for information, the tests cases. It will help to understand what is implemented in the attached patch.
Summary: Implement HTML 5's HTMLFormElement.checkValidity() → Implement HTML 5's Constraint Validation API
Comment on attachment 432162 [details] [diff] [review]
Patch v0.1

>diff -r ada84eb56c2f content/html/content/src/nsConstraintValidation.cpp
...
>+nsresult
>+nsConstraintValidation::IsValueMissing(PRBool* aValueMissing)
>+{
>+  *aValueMissing = PR_FALSE;
>+  return NS_OK;
>+}

Stuff like this begs for deCOMtamination.  These methods can't fail, so there's no reason to have them return nsresult.  Better to have:

PRBool
nsConstraintValidation::IsValueMissing()
{
  return PR_FALSE;
}


>+nsresult
>+nsConstraintValidation::IsValid(PRBool* aValid)
>+{
>+  nsresult rv;
>+  PRBool conditionFailed = PR_FALSE;
>+  *aValid = PR_FALSE;
>+
>+  rv = IsValueMissing(&conditionFailed);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (conditionFailed)
>+    return rv;
...

Then this becomes:

nsresult
nsConstraintValidation::IsValid(PRBool* aValid)
{
  NS_ENSURE_ARG_POINTER(aValid); // NS_ENSURE_ARG?  I can't keep these straight...

  PRBool conditionFailed = PR_FALSE;
  *aValid = PR_FALSE;

  if (IsValueMissing() || IsTypeMismatch() /* ... || */)
    return NS_OK;

  *aValid = PR_TRUE;
  return NS_OK;
}

>diff -r ada84eb56c2f dom/interfaces/html/nsIDOMNSHTMLButtonElement.idl
> [scriptable, uuid(c914d7a4-63b3-4d40-943f-91a3c7ab0d4d)]
> interface nsIDOMNSHTMLButtonElement : nsISupports
> {
>   void                      blur();
>   void                      focus();
>   void                      click();
>   attribute DOMString       type;
>+
>+  readonly attribute boolean          willValidate;
>+  readonly attribute nsIDOMValidityState validity;
>+  readonly attribute DOMString        validationMessage;
>+  boolean checkValidity();
>+  void setCustomValidity(in DOMString error);
> };

If you change the interface, you must change the UUID on the interface.

>diff -r ada84eb56c2f dom/interfaces/html/nsIDOMValidityState.idl
>+[scriptable, uuid(5e62197a-9b74-4812-b5a2-ca102e886f7a)]
>+interface nsIDOMValidityState : nsISupports
>+{
>+  readonly attribute boolean valueMissing;
>+  readonly attribute boolean typeMismatch;
>+  readonly attribute boolean patternMismatch;
>+  readonly attribute boolean tooLong;
>+  readonly attribute boolean rangeUnderflow;
>+  readonly attribute boolean rangeOverflow;
>+  readonly attribute boolean stepMismatch;
>+  readonly attribute boolean customError;
>+  readonly attribute boolean valid;
>+};

JavaDoc these interfaces, please!

I'll take a deeper look later - leaving the feedback flag as-is for now.
Comment on attachment 432162 [details] [diff] [review]
Patch v0.1

Here are some random drive-by comments.

>+NS_IMETHODIMP
>+nsConstraintValidation::GetValidity(nsIDOMValidityState** aValidity)
>+{
>+  if (!mValidity)
>+  {
>+    mValidity = new nsDOMValidityState(this);
>+    if (!mValidity)
>+    {
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    }

You don't need to worry about operator new returning null any more (because we now have infallible allocators.)

>+NS_IMETHODIMP
>+nsConstraintValidation::CheckValidity(PRBool* aValidity)
>+{
>+  nsresult rv;

Please use NS_ENSURE_ARG_POINTER here, and for other implementations of XPCOM methods which accept a pointer.

>+/**
>+ * This interface is used for form elements implementing the
>+ * validity constraint API.
>+ */
>+class nsConstraintValidation {
>+public:
> ...
>+  virtual nsresult IsValueMissing    (PRBool* aValueMissing);

Is there any reason for making all these methods return nsresult?  Most of them seem to be just about returning bools, and returning an extra NS_OK.  Those can be changed to return PRBool directly I guess.

>diff -r ada84eb56c2f dom/interfaces/html/nsIDOMNSHTMLButtonElement.idl
>--- a/dom/interfaces/html/nsIDOMNSHTMLButtonElement.idl	Fri Mar 12 17:46:39 2010 +0100
>+++ b/dom/interfaces/html/nsIDOMNSHTMLButtonElement.idl	Fri Mar 12 17:47:26 2010 +0100
>@@ -34,16 +34,24 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "domstubs.idl"
> 
>+interface nsIDOMValidityState;
>+
> [scriptable, uuid(c914d7a4-63b3-4d40-943f-91a3c7ab0d4d)]
> interface nsIDOMNSHTMLButtonElement : nsISupports

Please rev the uuid for  any interfaces that you change.
> Please use NS_ENSURE_ARG_POINTER here, and for other implementations of XPCOM
> methods which accept a pointer.

Except that's an out param, not an in param, right?  Shouldn't need to null-check those.
(In reply to comment #35)
> > Please use NS_ENSURE_ARG_POINTER here, and for other implementations of XPCOM
> > methods which accept a pointer.
> 
> Except that's an out param, not an in param, right?  Shouldn't need to
> null-check those.

Oh, I didn't know that.  I'm fairly certain that I had seen it being used for out-params before...  Are we sure that binary components honor XPCOM rules in this regard?
> I'm fairly certain that I had seen it being used for out-params before...

Yes, some code is over-defensive.

> Are we sure that binary components honor XPCOM rules in this regard?

No, but binary components that really want to cause crashes have simpler ways of doing it.
I think I should explain a bit the patch.
I've created the XPCOM component nsDOMValidityState which implements ValidityState. In addition, there is nsConstraintValidation which is a class that needs to be inherited by class which implement the constraint validity api. nsConstraintValidation manages ValidityState (actually, ValidityState is calling a nsConstraintValidation instance for each calls). nsConstraintValidation lets elements redefine some functions that's why some functions look weird like:
nsresult
nsConstraintValidation::IsTooLong(PRBool* aTooLong)
{
  *aTooLong = PR_FALSE;
  return NS_OK;
}

Actually, this function is redefined by nsHTMLInputElement. That's why nsresult is needed: i can't be sure no redefinition will need nsresult.
(In reply to comment #38)
> nsConstraintValidation lets elements redefine some functions that's why some
> functions look weird like:
> nsresult
> nsConstraintValidation::IsTooLong(PRBool* aTooLong)
> {
>   *aTooLong = PR_FALSE;
>   return NS_OK;
> }
> 
> Actually, this function is redefined by nsHTMLInputElement. That's why nsresult
> is needed: i can't be sure no redefinition will need nsresult.

I can't imagine under any conceivable definition why IsTooLong wouldn't be a true/false condition.  nsHTMLInputElement::IsTooLong can also be defined to return PRBool, since it's really, really simple.

If you look closely at the macro which defines MaxLength(), you'll find it eventually leads to nsGenericHTMLElement::GetIntAttr(), which unconditionally returns NS_OK.

nsHTMLInputElement::GetTextLength() calls on GetValue(), which also unconditionally returns NS_OK.  (Though I'd love to see a NS_ASSERTION or NS_POSTCONDITION on that!)

Based on that, I think you're safe in assuming IsTooLong can be defined as true or false.
Attachment #432162 - Flags: feedback?(Olli.Pettay) → feedback+
Comment on attachment 432162 [details] [diff] [review]
Patch v0.1


>+NS_IMETHODIMP
>+nsConstraintValidation::GetValidity(nsIDOMValidityState** aValidity)
>+{
>+  if (!mValidity)
>+  {

'{' should be in the same line as 'if'
if (!mValidity) {

Same also elsewhere.

>+    mValidity = new nsDOMValidityState(this);
>+    if (!mValidity)
>+    {
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    }
NS_ENSURE_TRUE(mValidity, NS_ERROR_OUT_OF_MEMORY);

>+NS_IMETHODIMP
>+nsConstraintValidation::GetValidationMessage(nsAString & aValidationMessage)
>+{
>+  nsresult rv;
>+  PRBool candidate;
>+  PRBool valid;
>+
>+  rv = IsCandidateForConstraintValidation(&candidate);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = IsValid(&valid);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (candidate && !valid)
>+  {
>+    if (!mCustomValidity.IsEmpty())
>+    {
>+      aValidationMessage.Assign(mCustomValidity);
>+    }
>+    else
>+    {
>+      aValidationMessage.AssignLiteral("should not happen atm");
This is temporary, I expect.
And I don't understand the "should not happen atm" anyway.

>+nsresult
>+nsConstraintValidation::IsValid(PRBool* aValid)
>+{
>+  nsresult rv;
>+  PRBool conditionFailed = PR_FALSE;
>+  *aValid = PR_FALSE;
>+
>+  rv = IsValueMissing(&conditionFailed);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (conditionFailed)
>+    return rv;

if (expr) {
  statement;
}
Also elsewhere
>+/**
>+ * This interface is used for form elements implementing the
>+ * validity constraint API.
>+ */
>+class nsConstraintValidation {
>+public:
>+
>+  nsresult GetValidity(nsIDOMValidityState** aValidity);
>+  nsresult GetWillValidate(PRBool* aWillValidate);
>+  nsresult GetValidationMessage(nsAString & aValidationMessage);
>+  nsresult CheckValidity(PRBool* aValidity);
>+  nsresult SetCustomValidity(const nsAString & aError);
Do these actually compile on Windows?
nsConstraintValidation version of the methods return nsresult, 
the methods from .idl return NS_IMETHODIMP
(which doesn't expand exactly to nsresult).
I recall seeing some compilation problems on Windows related to
cases which are pretty similar to this.


>+class nsDOMValidityState : public nsIDOMValidityState
>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIDOMVALIDITYSTATE
>+
>+  friend class nsConstraintValidation;
>+
>+protected:
>+  nsDOMValidityState(nsConstraintValidation* aConstraintValidation);
>+
>+  nsConstraintValidation*  mConstraintValidation;
Is it guaranteed that mConstraintValidation stays alive longer than
nsDOMValidityState object?

>+nsresult
>+nsHTMLInputElement::IsTooLong(PRBool* aTooLong)
>+{
>+  nsresult rv = NS_OK;
*aTooLong = PR_FALSE

>+  PRInt32 maxLength = -1;
>+  PRInt32 textLength = -1;
>+
>+  rv = GetMaxLength(&maxLength);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = GetTextLength(&textLength);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (maxLength == -1)
>+    *aTooLong = PR_FALSE;
>+  else
>+    *aTooLong = textLength > maxLength;

Maybe something like
*aTooLong = (maxLength >= 0) && (textLength > maxLength);


>+nsresult
>+nsHTMLInputElement::IsCandidateForConstraintValidation(PRBool* aCandidate)
>+{
>+  nsAutoString name;
>+
>+  // A disabled or un-named form element is never candidate for constraint validation.
>+  if (HasAttr(kNameSpaceID_None, nsGkAtoms::disabled)
>+      || !GetNameIfExists(name) || !mForm)
'||' shouldn't start a new line, but be in the previous line.


I wonder if you could make all the nsIDOMNS* interfaces to inherit some
interface which defines the new methods and properties.


(I have no idea how to handle the new 'feedback' flag. Marking +)
(In reply to comment #40)
> (From update of attachment 432162 [details] [diff] [review])
> >+    mValidity = new nsDOMValidityState(this);
> >+    if (!mValidity)
> >+    {
> >+      return NS_ERROR_OUT_OF_MEMORY;
> >+    }
> NS_ENSURE_TRUE(mValidity, NS_ERROR_OUT_OF_MEMORY);

Actually, it not even needed according to ehsan comment.

> >+NS_IMETHODIMP
> >+nsConstraintValidation::GetValidationMessage(nsAString & aValidationMessage)
> >+{
> >+  nsresult rv;
> >+  PRBool candidate;
> >+  PRBool valid;
> >+
> >+  rv = IsCandidateForConstraintValidation(&candidate);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+  rv = IsValid(&valid);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+  if (candidate && !valid)
> >+  {
> >+    if (!mCustomValidity.IsEmpty())
> >+    {
> >+      aValidationMessage.Assign(mCustomValidity);
> >+    }
> >+    else
> >+    {
> >+      aValidationMessage.AssignLiteral("should not happen atm");
> This is temporary, I expect.
> And I don't understand the "should not happen atm" anyway.

Yes it is. I have to write the messages but except tooLong, no constraint are usable at the moment.

> >+/**
> >+ * This interface is used for form elements implementing the
> >+ * validity constraint API.
> >+ */
> >+class nsConstraintValidation {
> >+public:
> >+
> >+  nsresult GetValidity(nsIDOMValidityState** aValidity);
> >+  nsresult GetWillValidate(PRBool* aWillValidate);
> >+  nsresult GetValidationMessage(nsAString & aValidationMessage);
> >+  nsresult CheckValidity(PRBool* aValidity);
> >+  nsresult SetCustomValidity(const nsAString & aError);
> Do these actually compile on Windows?
> nsConstraintValidation version of the methods return nsresult, 
> the methods from .idl return NS_IMETHODIMP
> (which doesn't expand exactly to nsresult).
> I recall seeing some compilation problems on Windows related to
> cases which are pretty similar to this.

Maybe I could use NS_IMETHOD and NS_IMETHODIMP to prevent this ?

> >+class nsDOMValidityState : public nsIDOMValidityState
> >+{
> >+public:
> >+  NS_DECL_ISUPPORTS
> >+  NS_DECL_NSIDOMVALIDITYSTATE
> >+
> >+  friend class nsConstraintValidation;
> >+
> >+protected:
> >+  nsDOMValidityState(nsConstraintValidation* aConstraintValidation);
> >+
> >+  nsConstraintValidation*  mConstraintValidation;
> Is it guaranteed that mConstraintValidation stays alive longer than
> nsDOMValidityState object?

mConstraintValidation is the object which have created the ValidityState object so except if mConstraintValidation intentionaly destroy the ValidityState object, this should be ok, isn't it ?

> I wonder if you could make all the nsIDOMNS* interfaces to inherit some
> interface which defines the new methods and properties.

You mean having a nsIConstraintValidation instead which would be used in IDL files ? I thought about that solution but I found it unnecessary because it will make the IDL less clear and it is only preventing adding three lines per IDL file.

> (I have no idea how to handle the new 'feedback' flag. Marking +)

As I see it, '+' means the general idea/quality/implementation is good and '-' means it isn't.
Comment on attachment 432162 [details] [diff] [review]
Patch v0.1

>diff -r ada84eb56c2f content/html/content/src/nsConstraintValidation.cpp
>+  else
>+  {
>+    aValidationMessage.AssignLiteral("");
>+  }

aValidationMessage.Truncate() ?  SetDOMStringToNull (from nsDOMString.h)?

>+NS_IMETHODIMP
>+nsConstraintValidation::CheckValidity(PRBool* aValidity)
>+{
>+  nsresult rv;
>+
>+  rv = IsCandidateForConstraintValidation(aValidity);
>+  if (*aValidity == PR_FALSE)
>+  {
>+    *aValidity = PR_TRUE;
>+    return rv;
>+  }

Something about this just feels wrong.  Reusing the variable for two different purposes here... I don't know about this.  Sure, it saves a couple bytes of memory, but potentially at the expense of readability and subtle bugs later.

>diff -r ada84eb56c2f content/html/content/src/nsDOMValidityState.h
>+//#include "nsAutoPtr.h"
>+//#include "nsCycleCollectionParticipant.h"
>+//#include "nsConstraintValidation.h"

If it's commented out, it shouldn't be in a new file patch.

>+++ b/dom/interfaces/html/nsIDOMNSHTMLInputElement.idl	Fri Mar 12 17:47:26 2010 +0100
> #include "domstubs.idl"
> 
> interface nsIControllers;
> interface nsIDOMFileList;
>+interface nsIDOMValidityState;

Shouldn't we just forward declare nsIDOMValidityState in domstubs.idl?

>+  readonly attribute boolean          willValidate;
>+  readonly attribute nsIDOMValidityState validity;
>+  readonly attribute DOMString        validationMessage;
>+  boolean checkValidity();
>+  void setCustomValidity(in DOMString error);

Even though you and I both know these came from HTML 5's specification, a reader who has never seen it before wouldn't know what these mean, or where they came from.  These need JavaDoc, at the very least, and a reference to the HTML 5 specification would be wise.

I'm marking this feedback- because I'm really serious about "internal stuff that can't fail shouldn't return nsresult".  Find me a code path where it CAN fail, and I'll back off - and recommend a bug be filed to make it infallible.
Attachment #432162 - Flags: feedback?(ajvincent) → feedback-
(In reply to comment #42)
> (From update of attachment 432162 [details] [diff] [review])
> >diff -r ada84eb56c2f content/html/content/src/nsConstraintValidation.cpp
> >+  else
> >+  {
> >+    aValidationMessage.AssignLiteral("");
> >+  }
> 
> aValidationMessage.Truncate() ?  SetDOMStringToNull (from nsDOMString.h)?

"The validationMessage attribute must return the empty string..." [1]
So Truncate rather than SetDOMStringToNull. (Or possibly Truncate at the start of the method?)

[1] <http://www.whatwg.org/html/#dom-cva-validationmessage>
Attached patch Test v0.2 (obsolete) — Splinter Review
I did not found a way to check the usage of Disconnect() function. The element has to be destroyed and the ValidityState abject has to be used.
If someone has an idea...
Attachment #432163 - Attachment is obsolete: true
Attachment #433737 - Flags: feedback?(Olli.Pettay)
Attached patch Patch v0.2 (obsolete) — Splinter Review
I tried to fix every issues mentioned before except the java doc. I do not think it is needed to add "tooLong is true if the element suffers from being too long". It sounds too trivial and idl files do not have so much comments (for what i've seen).

I still have two issues:
- sometimes nsConstraintValidation destructor throws a segfault. That's why it is commented in the patch. I did not take the time to look at this bug but if one of you has an idea...
- i don't know if taking in parameter nsIFormControl is the best idea. In addition, I have to found a clean way to take maxLength and value length for the 'tooLong' error message.

By the way, I would like this patch to be landed with this amount of functionality. That means some constraint validation will not be implemented (but we need new attributes for them), CSS rules will not be implemented and constraint validation in the form will not be implemented. I think this patch is big enough. What is your opinion ?
Attachment #432162 - Attachment is obsolete: true
Attachment #433738 - Flags: feedback?(Olli.Pettay)
Attachment #433738 - Flags: feedback?(ajvincent)
Attachment #433737 - Flags: feedback?(ajvincent)
(In reply to comment #44)
> Created an attachment (id=433737) [details]
> Test v0.2
> 
> I did not found a way to check the usage of Disconnect() function. The element
> has to be destroyed and the ValidityState abject has to be used.
> If someone has an idea...
Take reference to the ValidityState object in JS and remove the element from DOM
(and make sure it isn't kept alive in some JS variable) and call garbage collector in DOMWindowUtils.
I took a first glance at both patches, and I really like what I see.  I'm currently doing feedback reviews on another set of patches, so I'll get back to you later tonight.
(In reply to comment #45)
> - sometimes nsConstraintValidation destructor throws a segfault. That's why it
> is commented in the patch. I did not take the time to look at this bug but if
> one of you has an idea...
You should not NS_RELEASE mValidity.
It is nsRefPtr and will be released automatically when
nsConstraintValidation object is deleted.


> By the way, I would like this patch to be landed with this amount of
> functionality. That means some constraint validation will not be implemented
> (but we need new attributes for them), CSS rules will not be implemented and
> constraint validation in the form will not be implemented. I think this patch
> is big enough. What is your opinion ?
Makes sense, though this doesn't have to land before you've implement
other things. You could create new patches for this bug or new bugs and make
them to depend on the initial patch. And once everything, or at least most of
the things work reasonable well, patches could land.
Attached patch Tests v0.3 (obsolete) — Splinter Review
I think these tests are ready for a real review.
Attachment #433737 - Attachment is obsolete: true
Attachment #434066 - Flags: review?(Olli.Pettay)
Attachment #433737 - Flags: feedback?(ajvincent)
Attachment #433737 - Flags: feedback?(Olli.Pettay)
Attached patch Patch v0.3 (obsolete) — Splinter Review
I fixed every issue I had so I think the patch is ready to face a review and a feedback from Alex.

I think it would be better to land this patch or to choose what would be needed to land it because a lot of features depend on it (required, pattern, min, max and step attributes, :invalid, :valid, :required and :optional CSS pseudo-class, etc.).
Attachment #433738 - Attachment is obsolete: true
Attachment #434073 - Flags: review?(Olli.Pettay)
Attachment #434073 - Flags: feedback?(ajvincent)
Attachment #433738 - Flags: feedback?(ajvincent)
Attachment #433738 - Flags: feedback?(Olli.Pettay)
Warning: I'm not likely to get to this before Wednesday.  Most likely Saturday.  I still owe feedback comments on another set of patches...
Blocks: 555559
Comment on attachment 434073 [details] [diff] [review]
Patch v0.3

>+PRBool
>+nsConstraintValidation::IsCandidateForConstraintValidation(nsGenericHTMLFormElement* aElement)
>+{
>+  /**
>+   * An element is not candidate for constraint validation if:

An element is not a candidate...

>+   * - or it is un-named ;

Partly unclear:
* - or it has no name attribute ;
* - or its name attribute is empty ;

>+  if (aElement->HasAttr(kNameSpaceID_None, nsGkAtoms::disabled)) {
>+    return PR_FALSE;
>+  }
>+
>+  nsAutoString name;
>+  aElement->GetAttr(kNameSpaceID_None, nsGkAtoms::name, name);
>+  if (name.IsEmpty()) {
>+    return PR_FALSE;
>+  }

At first, I was going to ask "Why do these checks differently?"  Section 4.10.19.1 and 4.10.19.2 of WhatWG's HTML5 spec answer that.  I'd still like to see a comment explaining such:  "The disabled attribute merely has to be present.  The name attribute can be present, but if it's empty, that doesn't make the element a constraint."

Please add a XXX comment (and mention a bug number, preferably with comment) stating you need to check to see if you have an ancestor fieldset which is disabled (unless you find an ancestor legend element first), per 4.10.19.2.  I'm not going to require that check for now, but don't be surprised if smaug, or the super-reviewer for this, do.

>diff -r fb62f90bfd00 content/html/content/src/nsConstraintValidation.h

>+/**
>+ * This interface is used for form elements implementing the
>+ * validity constraint API.
>+ */

Let's add a reference to the HTML 5 specification for that validity constraint API in this comment.  Just to say we didn't make it up. :)

>+  virtual PRBool   IsValueMissing   ();
>+  virtual PRBool   IsTooLong        ();
>+  virtual PRBool   IsValid          ();

Nit:  I like these... (order rearranged for this comment)

>+  virtual PRBool   IsTypeMismatch   ();
>+  virtual PRBool   IsPatternMismatch();
>+  virtual PRBool   IsRangeUnderflow ();
>+  virtual PRBool   IsRangeOverflow  ();
>+  virtual PRBool   IsStepMismatch   ();
>+          PRBool   IsCustomError    () const;

I'm not sure I like the naming of these.  "HasRangeOverflow", "HasPatternMismatch". "HasCustomError" especially.  Or, you could just drop the verb from all of these methods, including the ones I like:  "ValueMissing()", "TooLong()", "Valid()".

I won't require you change the naming.  Just think about it.

>diff -r fb62f90bfd00 content/html/content/src/nsHTMLButtonElement.cpp
>+// nsConstraintValidation
>+
>+PRBool
>+nsHTMLButtonElement::IsBarredFromConstraintValidation()
>+{
>+  PRBool rValue = PR_FALSE;
>+
>+  switch (mType)
>+  {
>+    case NS_FORM_BUTTON_BUTTON:
>+    case NS_FORM_BUTTON_RESET:
>+      rValue = PR_TRUE;
>+      break;
>+    default:
>+      rValue = PR_FALSE;
>+  }

You don't really need the default case here, do you?  Particularly since you define rValue up front.

>diff -r fb62f90bfd00 content/html/content/src/nsHTMLFieldSetElement.cpp
>+

Nit:  Why the extra line at the end of the file?

>diff -r fb62f90bfd00 content/html/content/src/nsHTMLInputElement.cpp
>+PRBool
>+nsHTMLInputElement::IsTooLong()

I see you took my advice. :)

>+  rv = GetMaxLength(&maxLength);
>+  NS_ENSURE_SUCCESS(rv, PR_FALSE);

I want to see a NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv), "How did this fail?") between these last two lines.  (In opt code, it'll compile to a null-op.)  If it DOES happen, we'll want to file bugs on it immediately.

(In my previous comment, I'd forgotten that NS_ABORT_IF_FALSE replaces NS_ASSERTION.)

However, if DOM peers disagree and specify NS_ASSERTION, that's fine.  In any case, if the assertion fails here, I want to know about it (particularly since I claimed "That can't happen".)

>+  rv = GetTextLength(&textLength);
>+  NS_ENSURE_SUCCESS(rv, PR_FALSE);

Same here.

Do you get compile-time warnings for signed/unsigned mismatches here?  Looking at nsIDOMNSHTMLInputElement, I'm betting you don't... but I'd also like to see an NS_ABORT_IF_FALSE that textLength >= 0 here.  (It's conceivable that a text input could have 2GB+ of data in it... but unlikely.)

(Even better would be in the GetTextLength() method, but that's not your fault.  Best would be to explicitly define textLength as unsigned in the IDL, and fix it all the way through.  That's not your responsibility either.)

>+nsresult
>+nsHTMLInputElement::GetValidationMessage(nsAString& aValidationMessage,
>+                                         ValidationMessageType aType)
>+{
>+  nsresult rv = NS_OK;
>+
>+  switch (aType)
>+  {
>+    case VALIDATION_MESSAGE_TOO_LONG:
>+      {
>+        nsXPIDLString message;
>+        PRInt32 maxLength = -1;
>+        PRInt32 textLength = -1;
>+
>+        rv = GetMaxLength(&maxLength);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+
>+        rv = GetTextLength(&textLength);
>+        NS_ENSURE_SUCCESS(rv, rv);

We need similar assertions here.

>+        nsString strMaxLength;
>+        strMaxLength.AppendInt(maxLength);
>+        nsString strTextLength;
>+        strTextLength.AppendInt(textLength);
>+
>+        const PRUnichar* params[] = { ToNewUnicode(strTextLength),
>+                                      ToNewUnicode(strMaxLength) };
>+        rv = nsContentUtils::FormatLocalizedString(nsContentUtils::eDOM_PROPERTIES,
>+                                                   "TooLong", params, 2, message);

Something feels wrong about this block.  I don't know what.  I thought nsString objects were already Unicode (as opposed to nsCString).

It might be okay.  Smaug?

>diff -r fb62f90bfd00 content/html/content/src/nsHTMLTextAreaElement.cpp
>+PRBool
>+nsHTMLTextAreaElement::IsTooLong()
>+{
>+  nsresult rv = NS_OK;
>+  PRInt32 maxLength = -1;
>+  PRInt32 textLength = -1;
>+
>+  rv = GetMaxLength(&maxLength);
>+  NS_ENSURE_SUCCESS(rv, PR_FALSE);
>+
>+  rv = GetTextLength(&textLength);
>+  NS_ENSURE_SUCCESS(rv, PR_FALSE);

Similar assertions to the ones mentioned above, please.

>+nsresult
>+nsHTMLTextAreaElement::GetValidationMessage(nsAString& aValidationMessage,
>+                                            ValidationMessageType aType)
>+        rv = GetMaxLength(&maxLength);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+
>+        rv = GetTextLength(&textLength);
>+        NS_ENSURE_SUCCESS(rv, rv);

Similar assertions to the ones mentioned above, please.

>+        nsString strMaxLength;
>+        strMaxLength.AppendInt(maxLength);
>+        nsString strTextLength;
>+        strTextLength.AppendInt(textLength);
>+
>+        const PRUnichar* params[] = { ToNewUnicode(strTextLength),
>+                                      ToNewUnicode(strMaxLength) };
>+        rv = nsContentUtils::FormatLocalizedString(nsContentUtils::eDOM_PROPERTIES,
>+                                                   "TooLong", params, 2, message);

Same Unicode concern as above.

>diff -r fb62f90bfd00 dom/interfaces/html/nsIDOMNSHTMLButtonElement.idl
>+interface nsIDOMValidityState;

Repeating from comment 42:  "Shouldn't we just forward declare nsIDOMValidityState in domstubs.idl?"

>+  readonly attribute boolean          willValidate;
>+  readonly attribute nsIDOMValidityState validity;
>+  readonly attribute DOMString        validationMessage;
>+  boolean checkValidity();
>+  void setCustomValidity(in DOMString error);

Repeating from comment 42:  "Even though you and I both know these came from HTML 5's specification, a reader who has never seen it before wouldn't know what these mean, or where they came from.  These need JavaDoc, at the very least, and a reference to the HTML 5 specification would be wise."

Forget JavaDoc'ing it.  Just paste a link to where these properties and methods are specified.

>diff -r fb62f90bfd00 dom/interfaces/html/nsIDOMNSHTMLFieldSetElement.idl
>+interface nsIDOMValidityState;

...

>+  readonly attribute boolean          willValidate;
>+  readonly attribute nsIDOMValidityState   validity;
>+  readonly attribute DOMString        validationMessage;
>+  boolean checkValidity();
>+  void setCustomValidity(in DOMString error);

Ditto.

>diff -r fb62f90bfd00 dom/interfaces/html/nsIDOMNSHTMLInputElement.idl

>+interface nsIDOMValidityState;

...

>+  readonly attribute boolean          willValidate;
>+  readonly attribute nsIDOMValidityState validity;
>+  readonly attribute DOMString        validationMessage;
>+  boolean checkValidity();
>+  void setCustomValidity(in DOMString error);

Ditto.

>diff -r fb62f90bfd00 dom/interfaces/html/nsIDOMNSHTMLSelectElement.idl
>+interface nsIDOMValidityState;

...

>+  readonly attribute boolean          willValidate;
>+  readonly attribute nsIDOMValidityState validity;
>+  readonly attribute DOMString        validationMessage;
>+  boolean checkValidity();
>+  void setCustomValidity(in DOMString error);

Ditto.

>diff -r fb62f90bfd00 dom/interfaces/html/nsIDOMNSHTMLTextAreaElement.idl
>+interface nsIDOMValidityState;

...

>+  readonly attribute boolean          willValidate;
>+  readonly attribute nsIDOMValidityState validity;
>+  readonly attribute DOMString        validationMessage;
>+  boolean checkValidity();
>+  void setCustomValidity(in DOMString error);

Ditto.

>diff -r fb62f90bfd00 dom/interfaces/html/nsIDOMValidityState.idl
>+[scriptable, uuid(5e62197a-9b74-4812-b5a2-ca102e886f7a)]
>+interface nsIDOMValidityState : nsISupports
>+{
>+  readonly attribute boolean valueMissing;
>+  readonly attribute boolean typeMismatch;
>+  readonly attribute boolean patternMismatch;
>+  readonly attribute boolean tooLong;
>+  readonly attribute boolean rangeUnderflow;
>+  readonly attribute boolean rangeOverflow;
>+  readonly attribute boolean stepMismatch;
>+  readonly attribute boolean customError;
>+  readonly attribute boolean valid;
>+};

This needs a comment pointing to the ValidityState interface definition in HTML 5.  Unless you want to JavaDoc every single one of these.
Attachment #434073 - Flags: feedback?(ajvincent) → feedback+
You may notice I recommend putting in links to specifications as comments.  There's a couple reasons for that:

(1) It shows we are following a standard for the methods in question. ("We didn't just make this stuff up.")
(2) If someone wonders what we're doing, and why, they can directly compare it to the spec, without having to search the web for the spec, and the specific sections we're implementing.
(3) Having the spec reduces the need for JavaDoc.  Personally, without the spec I'd have no idea what "rangeOverflow" means at first glance.
Do this patch contain support for the :required and :optional psuedo-classes, as mentioned in bug 506554?
I might as well have checked the patch. It doesn't seem to be the case. Good work so far, though. Will a patch for the psuedo-classes follow?
Thank you very much for your feedback Alex. I will update my patch tomorrow. Olli, do not hesitate to do the review in the meantime.

Magne, :required and :optional need bug 25551 to be solved (ie. they need the required attribute to be implemented). The required attribute can be added quite easily but I prefer to have this patch as small as possible and add stuff after.
Comment on attachment 434066 [details] [diff] [review]
Tests v0.3


>+function invalidEventHandler(aEvent)
>+{
>+  function checkInvalidEvent(aEvent)
>+  {
>+    is(aEvent.type, "invalid", "Invalid event type should be invalid");
>+    ok(!aEvent.bubbles, "Invalid event should not bubbling");
should not be bubbling
or
should not bubble 


does some other browser implement all the things this is testing?
Did you check that you get the same results with that?
Attachment #434066 - Flags: review?(Olli.Pettay) → review+
(In reply to comment #56)
> Magne, :required and :optional need bug 25551 to be solved [...]

Sorry, i mean bug 345822, not bug 25551 (this is the one from WebKit ;)).
As asked by Olli, there are the test results for Opera 10.10 (Presto) and Chromium 5.0.307.11 (WebKit).
I think it shows our tests are correct.
Keywords: dev-doc-needed
Attached patch Tests v0.3.1 (obsolete) — Splinter Review
r=smaug
Attachment #434066 - Attachment is obsolete: true
Adding David in CC as he may want to follow this bug regarding a10y.
(In reply to comment #52)
> Please add a XXX comment (and mention a bug number, preferably with comment)
> stating you need to check to see if you have an ancestor fieldset which is
> disabled (unless you find an ancestor legend element first), per 4.10.19.2. 
> I'm not going to require that check for now, but don't be surprised if smaug,
> or the super-reviewer for this, do.

I don't get it. 4.10.19.2 is about disabled elements. Were you talking about datalist ancestor ? I've added the bug about datalist implementation.

> >diff -r fb62f90bfd00 content/html/content/src/nsHTMLFieldSetElement.cpp
> >+
> 
> Nit:  Why the extra line at the end of the file?

A habit a got with gcc. I don't remember why and how it warns you when there is no empty line at the end of the file. Other files have it too.

> >+  rv = GetMaxLength(&maxLength);
> >+  NS_ENSURE_SUCCESS(rv, PR_FALSE);
> 
> I want to see a NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv), "How did this fail?")
> between these last two lines.  (In opt code, it'll compile to a null-op.)  If
> it DOES happen, we'll want to file bugs on it immediately.
> 
> (In my previous comment, I'd forgotten that NS_ABORT_IF_FALSE replaces
> NS_ASSERTION.)
> 
> However, if DOM peers disagree and specify NS_ASSERTION, that's fine.  In any
> case, if the assertion fails here, I want to know about it (particularly since
> I claimed "That can't happen".)
> 
> >+  rv = GetTextLength(&textLength);
> >+  NS_ENSURE_SUCCESS(rv, PR_FALSE);
> 
> Same here.
> 
> Do you get compile-time warnings for signed/unsigned mismatches here?  Looking
> at nsIDOMNSHTMLInputElement, I'm betting you don't... but I'd also like to see
> an NS_ABORT_IF_FALSE that textLength >= 0 here.  (It's conceivable that a text
> input could have 2GB+ of data in it... but unlikely.)
>
> (Even better would be in the GetTextLength() method, but that's not your fault.
>  Best would be to explicitly define textLength as unsigned in the IDL, and fix
> it all the way through.  That's not your responsibility either.)

I would be for adding assertions in GetTextLength instead of having too many checks in the functions calling it.
For GetMaxLength I should check that in callers... What should I use assertions or NS_ABORT_IF_FALSE ?
Olli, what is your opinion ?

> >+        nsString strMaxLength;
> >+        strMaxLength.AppendInt(maxLength);
> >+        nsString strTextLength;
> >+        strTextLength.AppendInt(textLength);
> >+
> >+        const PRUnichar* params[] = { ToNewUnicode(strTextLength),
> >+                                      ToNewUnicode(strMaxLength) };
> >+        rv = nsContentUtils::FormatLocalizedString(nsContentUtils::eDOM_PROPERTIES,
> >+                                                   "TooLong", params, 2, message);
> 
> Something feels wrong about this block.  I don't know what.  I thought nsString
> objects were already Unicode (as opposed to nsCString).
> 
> It might be okay.  Smaug?

Without ToNewUnicode() I got "error: cannot convert ‘nsString’ to ‘const PRUnichar*’". Maybe there is another way but I don't know it.

> >diff -r fb62f90bfd00 dom/interfaces/html/nsIDOMNSHTMLButtonElement.idl
> >+interface nsIDOMValidityState;
> 
> Repeating from comment 42:  "Shouldn't we just forward declare
> nsIDOMValidityState in domstubs.idl?"

I do not think that would make sense but I may be wrong.

Non-answered comments will be fixed in the next patch.
Attached patch Patch v0.3.1 (obsolete) — Splinter Review
Attachment #434073 - Attachment is obsolete: true
Attachment #435742 - Flags: review?(Olli.Pettay)
Attachment #434073 - Flags: review?(Olli.Pettay)
Attachment #435742 - Flags: review?(Olli.Pettay) → review-
Comment on attachment 435742 [details] [diff] [review]
Patch v0.3.1

>+NS_IMETHODIMP
>+nsConstraintValidation::GetWillValidate(PRBool* aWillValidate,
>+                                        nsGenericHTMLFormElement* aElement)
>+{
>+  *aWillValidate = IsCandidateForConstraintValidation(aElement);
>+  return NS_OK;
>+}
Why is this returning NS_IMETHODIMP? It is defined as nsresult.
Same with other nsConstraintValidation methods.


>+
>+NS_IMETHODIMP
>+nsConstraintValidation::GetValidationMessage(nsAString & aValidationMessage,
>+                                             nsGenericHTMLFormElement* aElement)
>+{
>+  nsresult rv = NS_OK;
>+  PRBool candidate;
>+  PRBool valid;
>+
>+  candidate = IsCandidateForConstraintValidation(aElement);
>+  valid     = IsValid();
PRBool candidate = IsCandidateForConstraintValidation(aElement);
PRBool valid     = IsValid();


>+NS_IMETHODIMP
>+nsConstraintValidation::CheckValidity(PRBool* aValidity, nsGenericHTMLFormElement* aElement)
>+{
>+  nsresult rv = NS_OK;
>+
>+  if (!IsCandidateForConstraintValidation(aElement)) {
>+    *aValidity = PR_TRUE;
>+    return rv;
>+  }
>+
>+  *aValidity = IsValid();
>+
>+  if (!*aValidity) {
>+    rv = nsContentUtils::DispatchTrustedEvent(aElement->GetOwnerDoc(),
>+                                              (nsIFormControl*)aElement,
>+                                              NS_LITERAL_STRING("invalid"),
>+                                              PR_FALSE, PR_TRUE);
>+  }
>+
>+  return rv;
>+}
This could be

nsresult
nsConstraintValidation::CheckValidity(PRBool* aValidity, nsGenericHTMLFormElement* aElement)
{
  if (!IsCandidateForConstraintValidation(aElement) || IsValid()) {
    *aValidity = PR_TRUE;
     return NS_OK;
  }

  *aValidity = PR_FALSE;
  return nsContentUtils::DispatchTrustedEvent(aElement->GetOwnerDoc(),
                                              static_cast<nsIContent*>(aElement),
                                              NS_LITERAL_STRING("invalid"),
                                              PR_FALSE, PR_TRUE);
}


>+PRBool
>+nsConstraintValidation::IsValid()
>+{
>+  return !(IsValueMissing() || HasTypeMismatch() || HasPatternMismatch() ||
>+      IsTooLong() || HasRangeUnderflow() || HasRangeOverflow() ||
>+      HasStepMismatch() || HasCustomError());
Strange indentation. Indentation is 2 spaces, not 4. Though, in this case put
IsTooLong() under IsValueMissing()

>+class nsDOMValidityState : public nsIDOMValidityState
>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIDOMVALIDITYSTATE
>+
>+  friend class nsConstraintValidation;
>+
>+protected:
>+  // This function should be called by nsConstraintValidation
>+  // to set mConstraintValidation to null to be sure it will not be called.
>+  void Disconnect();
>+
>+  nsDOMValidityState(nsConstraintValidation* aConstraintValidation);
>+
>+  nsConstraintValidation*  mConstraintValidation;
Extra space after *


>+};
>+
>+inline void nsDOMValidityState::Disconnect()
>+{
>+  mConstraintValidation = nsnull;
>+}
I prefer this inside the class

>+++ b/dom/locales/en-US/chrome/dom/dom.properties	Tue Mar 30 01:04:35 2010 +0200
> WrongEventPropertyAccessWarning=The '%S' property of a %S event should not be used. The value is meaningless.
>+TooLong=The text is too long. It is %S characters long and the limit is %S.
TooLong is a bit vague.

I could re-review this.
Depends on: 558788
Attached patch Patch v3.2 (obsolete) — Splinter Review
This is fixing the issues mentioned in previous comment and updating regarding the specification changes (name and form parent are no longer a reason to be barred from constraint validation).
Attachment #435742 - Attachment is obsolete: true
Attachment #438497 - Flags: review?(Olli.Pettay)
Blocks: 344615
Attached patch Tests v3.2 (obsolete) — Splinter Review
r=smaug

Updating the tests to reflect the changes in the specifications.
Attachment #435638 - Attachment is obsolete: true
Comment on attachment 438497 [details] [diff] [review]
Patch v3.2


>+
>+NS_IMETHODIMP
>+nsConstraintValidation::CheckValidity(PRBool* aValidity,
>+                                      nsGenericHTMLFormElement* aElement)
>+{
>+  nsresult rv = NS_OK;
>+
>+  if (!IsCandidateForConstraintValidation(aElement) || IsValid()) {
>+    *aValidity = PR_TRUE;
>+    return rv;
>+  }
>+
>+  *aValidity = PR_FALSE;
>+
>+  rv = nsContentUtils::DispatchTrustedEvent(aElement->GetOwnerDoc(),
>+                                            static_cast<nsIContent*>(aElement),
>+                                            NS_LITERAL_STRING("invalid"),
>+                                            PR_FALSE, PR_TRUE);
>+
No need for nsresult rv.
Just return NS_OK in the if
and then return nsContentUtils::DispatchTrustedEvent


>+PRBool
>+nsHTMLButtonElement::IsBarredFromConstraintValidation()
>+{
>+  PRBool rValue = PR_FALSE;
>+
>+  switch (mType)
>+  {
>+    case NS_FORM_BUTTON_BUTTON:
>+    case NS_FORM_BUTTON_RESET:
>+      rValue = PR_TRUE;
>+      break;
>+  }
>+
>+  return rValue;
>+}

do just 
PRBool
nsHTMLButtonElement::IsBarredFromConstraintValidation()
{
  return (mType == NS_FORM_BUTTON_BUTTON ||
          mType == NS_FORM_BUTTON_RESET);
}
>+nsresult
>+nsHTMLInputElement::GetValidationMessage(nsAString& aValidationMessage,
>+                                         ValidationMessageType aType)
>+{
>+  nsresult rv = NS_OK;
>+
>+  switch (aType)
>+  {
>+    case VALIDATION_MESSAGE_TOO_LONG:
>+      {
Extra indentation
{ should be under case


I'd really like to get the pattern validation done before landing this.

Also, is there a bug open to get some UI for validation error messages?
Attachment #438497 - Flags: review?(Olli.Pettay) → review+
Attached patch Patch v3.3 (obsolete) — Splinter Review
r=smaug
Attachment #438497 - Attachment is obsolete: true
Attachment #440055 - Flags: superreview?(jonas)
I'm going to work on the pattern attribute this week.
For the UI aspect of the constraint validation, I'm wondering which product/component would be appropriate to fill the bug in.
Comment on attachment 440055 [details] [diff] [review]
Patch v3.3

First off, as I've said elsewhere, I don't think we should ship with this amount of support. I think we need to implement more of the constraints or nothing at all. Ideally I don't even think we should land this amount of support, we can always leave the patch as it is in a reviewed state and then write further patches on top of it and only land once the other patches are finished.

That said, if you are prepared to back this out if we get too close to shipping date without having more of the constraints done, then I'd be ok with landing.

What I think we need before shipping is support for the various constraints attributes (required, pattern, etc) on the form control types that we already support.


>+nsConstraintValidation::GetValidationMessage(nsAString & aValidationMessage,
>+                                             nsGenericHTMLFormElement* aElement)
>+{
>+  nsresult rv = NS_OK;
>+  PRBool candidate = IsCandidateForConstraintValidation(aElement);
>+  PRBool valid = IsValid();
>+
>+  if (candidate && !valid) {

Is there a need for the temporaries here?

>+    if (!mCustomValidity.IsEmpty()) {
>+      aValidationMessage.Assign(mCustomValidity);
>+    } else if (IsTooLong()) {
>+      GetValidationMessage(aValidationMessage, VALIDATION_MESSAGE_TOO_LONG);
>+    } else {
>+      // TODO: The other messages have not been written
>+      // because related constraint validation are not implemented yet.
>+      // We should not be here.
>+      aValidationMessage.Truncate();
>+      rv = NS_ERROR_UNEXPECTED;
>+    }
>+  } else {
>+    aValidationMessage.Truncate();
>+  }
>+
>+  return rv;

In general I really dislike the coding pattern of retaining values in rv and ending functions with |return rv|. Someone is bound to insert code that modifies rv somewhere without realizing the consequences.

Please change the rv assignment above to simply |return NS_ERROR_UNEXPECTED;| and end the function with |return NS_OK|

Also, it'd be clearer to truncate the return value at the beginning of the function and avoid having to worry about clearing it at every error path. There really is no need to squeeze a few cycles at the cost of duplicated code.



>+nsConstraintValidation::IsCandidateForConstraintValidation(nsGenericHTMLFormElement* aElement)
>+{
>+  // Only for optimization.
>+  if (IsAlwaysBarredFromConstraintValidation()) {
>+    return PR_FALSE;
>+  }

Is this optimization really needed? Fieldsets aren't really used enough to warrant optimizing their performance. We can always modify this if it starts showing up in profiles.

>+
>+  /**
>+   * An element is not candidate for constraint validation if:
>+   * - it is disabled ;
>+   * - TODO: or it's ancestor is a datalist element (bug 555840).
>+   */
>+
>+  if (aElement->CanBeDisabled() &&
>+      aElement->HasAttr(kNameSpaceID_None, nsGkAtoms::disabled)) {
>+    return PR_FALSE;
>+  }

Is the CanBeDisabled check really useful? It seems like none of the elements which can't be disabled will ever be invalid. So removing the call should make no detectable difference.

Though given that we have the function anyhow, I guess we might as well call it. Though please add a comment saying that the call is an optimization.


>diff -r 8324e1d86455 content/html/content/src/nsConstraintValidation.h
>+class nsConstraintValidation
>+{
>+public:
>+
>+  virtual ~nsConstraintValidation();
>+
>+  NS_IMETHOD GetValidity(nsIDOMValidityState** aValidity);
>+  NS_IMETHOD GetWillValidate(PRBool* aWillValidate,
>+                             nsGenericHTMLFormElement* aElement);
>+  NS_IMETHOD GetValidationMessage(nsAString & aValidationMessage,
>+                                  nsGenericHTMLFormElement* aElement);
>+  NS_IMETHOD CheckValidity(PRBool* aValidity,
>+                           nsGenericHTMLFormElement* aElement);
>+  NS_IMETHOD SetCustomValidity(const nsAString & aError);

Is there a reason these aren't non-virtual and simply returning nsresult?

NS_IMETHOD comes with extra overhead, not just by being virtual, but also by forcing a supoptimal calling convention on some platforms (windows IIRC). So use it as little as possible, ideally only when implementing idl interfaces.

>+  virtual PRBool   IsValueMissing    ();
>+  virtual PRBool   HasTypeMismatch   ();
>+  virtual PRBool   HasPatternMismatch();
>+  virtual PRBool   IsTooLong         ();
>+  virtual PRBool   HasRangeUnderflow ();
>+  virtual PRBool   HasRangeOverflow  ();
>+  virtual PRBool   HasStepMismatch   ();
>+          PRBool   HasCustomError    () const;

Might as well inline HasCustomError



@@ -128,8 +141,9 @@ nsHTMLFieldSetElement::Reset()
> }
> 
> NS_IMETHODIMP
> nsHTMLFieldSetElement::SubmitNamesValues(nsFormSubmission* aFormSubmission,
>                                          nsIContent* aSubmitElement)
> {
>   return NS_OK;
> }
>+

Nit, this change doesn't seem needed :)

>diff -r 8324e1d86455 content/html/content/src/nsHTMLInputElement.cpp

>+nsHTMLInputElement::IsTooLong()
>+{
>+  nsresult rv = NS_OK;
>+  PRInt32 maxLength = -1;
>+  PRInt32 textLength = -1;
>+
>+  rv = GetMaxLength(&maxLength);
>+  NS_ENSURE_SUCCESS(rv, PR_FALSE);
>+
>+  rv = GetTextLength(&textLength);
>+  NS_ENSURE_SUCCESS(rv, PR_FALSE);
>+
>+  return (maxLength >= 0) && (textLength > maxLength);
>+}

Move declaring rv to where you're first using it. Though really, looking at the code we know that none of those functions will ever return anything but NS_OK. So I say simply ignore the return values and get rid of rv completely. This is one of the sadnesses of XPCOM :(

Don't you need to check the dirty flag here? I.e. if the dirty flag is false, then this function should always return false.

>+PRBool
>+nsHTMLInputElement::IsBarredFromConstraintValidation()
>+{
>+  PRBool rValue = PR_FALSE;
>+
>+  switch (mType)
>+  {
>+    case NS_FORM_INPUT_HIDDEN:
>+    case NS_FORM_INPUT_BUTTON:
>+    case NS_FORM_INPUT_RESET:
>+      rValue = PR_TRUE;
>+      break;
>+    default:
>+      rValue = HasAttr(kNameSpaceID_None, nsGkAtoms::readonly);
>+  }
>+
>+  return rValue;
>+}

You could simply do:

return mType == NS_FORM_INPUT_HIDDEN ||
       mType == NS_FORM_INPUT_BUTTON ||
       mType == NS_FORM_INPUT_RESET ||
       HasAttr(...);

>+nsresult
>+nsHTMLInputElement::GetValidationMessage(nsAString& aValidationMessage,
>+                                         ValidationMessageType aType)
>+{
>+  nsresult rv = NS_OK;
>+
>+  switch (aType)
>+  {
>+    case VALIDATION_MESSAGE_TOO_LONG:
>+    {
>+      nsXPIDLString message;
>+      PRInt32 maxLength = -1;
>+      PRInt32 textLength = -1;
>+
>+      rv = GetMaxLength(&maxLength);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      rv = GetTextLength(&textLength);
>+      NS_ENSURE_SUCCESS(rv, rv);

Same thing here regarding return values.

>+      nsString strMaxLength;
>+      strMaxLength.AppendInt(maxLength);
>+      nsString strTextLength;
>+      strTextLength.AppendInt(textLength);
>+
>+      const PRUnichar* params[] = { ToNewUnicode(strTextLength),
>+                                    ToNewUnicode(strMaxLength) };

Aren't you leaking these strings?


>diff -r 8324e1d86455 content/html/content/src/nsHTMLTextAreaElement.cpp

Most of the comments for nsHTMLInputElement also apply in nsHTMLTextAreaElement

sr=me with this fixed.
Attachment #440055 - Flags: superreview?(jonas) → superreview+
Blocks: 558788
No longer depends on: 558788
Attached patch Patch v3.4 (obsolete) — Splinter Review
r=smaug sr=sicking

Do we agree this patch could be landed when <input type='email'>, <input type='url'>, @required and @pattern patches will be super-reviewed ?
Attachment #440055 - Attachment is obsolete: true
Blocks: 561634
Blocks: 561635
Blocks: 561636
Blocks: 561640
Attached patch Patch v3.5 (obsolete) — Splinter Review
r=smaug sr=sicking

The previous patch wasn't building on Windows because of a NS_IMPLMETHOD which I didn't changed to nsresult. This should now pass the try server now. I will re-send the patch when we will think about landing it.
Attachment #441353 - Attachment is obsolete: true
Blocks: 566348
Blocks: 344616
Depends on: 580575
Attached patch Patch refreshed to tip (obsolete) — Splinter Review
Had to be refreshed due to XPCClassInfo stuff
Blocks: 581947
Blocks: 582248
Attached patch Patch v3.6Splinter Review
Updated patch to apply on current tip.
Merging test and patch patches.
Attachment #399045 - Attachment is obsolete: true
Attachment #438992 - Attachment is obsolete: true
Attachment #442159 - Attachment is obsolete: true
Attachment #460410 - Attachment is obsolete: true
This is now in the tree.
rev: http://hg.mozilla.org/mozilla-central/rev/8f80a2d92cae
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
Blocks: 595282
Blocks: 605365
You need to log in before you can comment on or make changes to this bug.