Last Comment Bug 345822 - Implement required attribute
: Implement required attribute
Status: RESOLVED FIXED
: access, dev-doc-complete, testcase
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 7 votes (vote)
: mozilla2.0b5
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
http://www.whatwg.org/specs/web-apps/...
Depends on: 345624 456229 557620
Blocks: html5forms 344615 506554 555559 558788 559275 561635 566348 1015305
  Show dependency treegraph
 
Reported: 2006-07-25 01:27 PDT by Alex Vincent [:WeirdAl]
Modified: 2014-05-23 10:38 PDT (History)
34 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch for <html:input/> (almost ready for reviews) (43.82 KB, patch)
2006-07-25 02:09 PDT, Alex Vincent [:WeirdAl]
no flags Details | Diff | Splinter Review
patch, v1 (52.33 KB, patch)
2006-07-25 20:50 PDT, Alex Vincent [:WeirdAl]
jonas: review-
Details | Diff | Splinter Review
Supplementary layout module patch (1.89 KB, patch)
2006-07-29 22:09 PDT, Alex Vincent [:WeirdAl]
jonas: review-
Details | Diff | Splinter Review
patch, v2 (45.55 KB, patch)
2006-09-10 00:57 PDT, Alex Vincent [:WeirdAl]
no flags Details | Diff | Splinter Review
perf testcase (2.31 KB, text/html)
2006-09-20 00:24 PDT, Alex Vincent [:WeirdAl]
no flags Details
patch, v2.1 (44.87 KB, patch)
2006-09-20 00:35 PDT, Alex Vincent [:WeirdAl]
no flags Details | Diff | Splinter Review
perf testcase, checkboxes (2.90 KB, text/html)
2006-09-26 13:01 PDT, Alex Vincent [:WeirdAl]
no flags Details
patch, v2.2 (56.11 KB, patch)
2006-09-26 14:38 PDT, Alex Vincent [:WeirdAl]
no flags Details | Diff | Splinter Review
patch, v3 (54.55 KB, patch)
2006-09-30 10:37 PDT, Alex Vincent [:WeirdAl]
jonas: review-
Details | Diff | Splinter Review
perf testcase results (9.41 KB, text/html)
2006-09-30 10:50 PDT, Alex Vincent [:WeirdAl]
no flags Details
Tests v1 (11.60 KB, patch)
2010-04-13 10:44 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1 (22.13 KB, patch)
2010-04-13 10:45 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.1 (22.93 KB, patch)
2010-04-13 13:58 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Tests v1.1 (10.06 KB, patch)
2010-04-14 10:45 PDT, Mounir Lamouri (:mounir)
bugs: review+
Details | Diff | Splinter Review
Patch v1.2 (22.81 KB, patch)
2010-04-14 10:46 PDT, Mounir Lamouri (:mounir)
ajvincent: feedback+
Details | Diff | Splinter Review
Patch v1.3 (22.81 KB, patch)
2010-04-19 17:27 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Tests v1.2 (10.17 KB, patch)
2010-04-19 17:42 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.4 (24.13 KB, patch)
2010-04-19 17:44 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.5 (24.13 KB, patch)
2010-04-19 17:54 PDT, Mounir Lamouri (:mounir)
bugs: review+
jonas: superreview+
Details | Diff | Splinter Review
Tests v1.3 (10.16 KB, patch)
2010-04-20 05:01 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.6 (35.02 KB, patch)
2010-07-28 06:10 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review

Description Alex Vincent [:WeirdAl] 2006-07-25 01:27:09 PDT
The required attribute is simple to implement.  Patch coming right up.
Comment 1 Alex Vincent [:WeirdAl] 2006-07-25 02:09:51 PDT
Created attachment 230556 [details] [diff] [review]
patch for <html:input/> (almost ready for reviews)
Comment 2 Alex Vincent [:WeirdAl] 2006-07-25 02:15:47 PDT
I haven't done a patch for textareas yet; I wanted to get this up for a first look.

Known problems (which keep this from being reviewable):
(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 3380

name == "nsIDOMWF2ValidityState", mData->mName == "DOMWF2ValidityState".

(2) http://webforms2.testsuite.org/controls/common-attributes/required/001.htm
DOM Inspector shows for inputs 2 and 3 that (input).validity.valid is indeed true (which is expected), but the CSS pseudoclass doesn't apply, apparently.

I show a similar result for 
http://webforms2.testsuite.org/controls/common-attributes/required/003.xht .

Other notes:

* http://webforms2.testsuite.org/controls/common-attributes/required/002.htm
The required attribute uses NS_IMPL_BOOL_ATTR, which has never before checked the actual value of the attribute.

I believe that by the current draft of the spec, the test is actually invalid.  HTML does not require the attribute value be "required", and technically speaking, neither does the current draft (in a normative way).  

XHTML does require it, but we've never gone out of our way to check that (and we should start - it should be easy to adjust the macro itself to check for a XHTML document type).  There's a bug on file somewhere about this.

That said, I'd have no objection going forward to writing a NS_IMPL_BOOL_ATTR_MATCH attribute for required and other new WF2 attributes, and forcing the value check.
Comment 3 Alex Vincent [:WeirdAl] 2006-07-25 02:21:48 PDT
Hixie's corrected me by e-mail:  the value of the attribute doesn't matter to the implementation.  We fail 002.htm only because of an issue with the CSS pseudoclasses.
Comment 4 Alex Vincent [:WeirdAl] 2006-07-25 02:54:37 PDT
Comment on attachment 230556 [details] [diff] [review]
patch for <html:input/> (almost ready for reviews)

>+  rv = mValidity->GetValid(_retval);
>+  if (!_retval) {

Whoops - (!(*_retval))
Comment 5 Alex Vincent [:WeirdAl] 2006-07-25 03:06:56 PDT
Events also appear to be problematic.  :(
Comment 6 Alex Vincent [:WeirdAl] 2006-07-25 18:53:38 PDT
Comment on attachment 230556 [details] [diff] [review]
patch for <html:input/> (almost ready for reviews)

>Index: dom/src/base/nsDOMClassInfo.cpp
>+  // Web Forms 2.0
>+  DOM_CLASSINFO_MAP_BEGIN(DOMWF2ValidityState, nsIDOMWF2ValidityState)

Thanks to bz and mrbkap, DOM_CLASSINFO_MAP_BEGIN_NO_CLASS_IF is the right macro for here.  This fixes the assertion for issue 1.
Comment 7 Alex Vincent [:WeirdAl] 2006-07-25 20:50:44 PDT
Created attachment 230689 [details] [diff] [review]
patch, v1

sicking says the pseudoclass selectors issue need not block this patch.  The other two problems I knew about - the assertion and events not being dispatched - I have fixed.  I've also implemented support for textareas.
Comment 8 Peter Van der Beken [:peterv] 2006-07-26 01:39:01 PDT
(In reply to comment #6)
> (From update of attachment 230556 [details] [diff] [review] [edit])
> >Index: dom/src/base/nsDOMClassInfo.cpp
> >+  // Web Forms 2.0
> >+  DOM_CLASSINFO_MAP_BEGIN(DOMWF2ValidityState, nsIDOMWF2ValidityState)
> 
> Thanks to bz and mrbkap, DOM_CLASSINFO_MAP_BEGIN_NO_CLASS_IF is the right macro
> for here.  This fixes the assertion for issue 1.

No, the problem is that it should either be:

DOM_CLASSINFO_MAP_BEGIN(DOMWF2ValidityState, nsIDOMDOMWF2ValidityState)

or

DOM_CLASSINFO_MAP_BEGIN(WF2ValidityState, nsIDOMWF2ValidityState)
Comment 9 Alex Vincent [:WeirdAl] 2006-07-29 16:57:28 PDT
Note: I forgot to add an entry to allmakefiles.sh for dom/public/idl/web-forms/Makefile.  (Doh!)
Comment 10 Alex Vincent [:WeirdAl] 2006-07-29 22:09:58 PDT
Created attachment 231275 [details] [diff] [review]
Supplementary layout module patch

I added the following lines to nsDOMWF2ValidityState.h:

#define NS_VALIDITYSTATE_CID                        \
 { /* 215793da-b1b2-4784-9f2d-f6368a90302c */       \
   0x215793da, 0xb1b2, 0x4784,                      \
  {0x9f, 0x2d, 0xf6, 0x36, 0x8a, 0x90, 0x30, 0x2c} }
#define NS_VALIDITYSTATE_CONTRACTID \
"@mozilla.org/webforms2-validity-state;1"

This patch does corresponding work for nsLayoutModule.cpp.  It turns out I need this for exposing the validity state code to non-C++ code (in particular, XBL-based WF2 controls, such as <html:output/>).
Comment 11 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-09-06 16:43:37 PDT
Comment on attachment 230689 [details] [diff] [review]
patch, v1

In general there are too many new interfaces. I would really like to get rid of some if at all possible. It seems like you should be able to add properties to existing 'NS' interfaces (like nsIDOMNSHTMLInputElement) rather than adding new interfaces.

Validation may still need its own interface though. Or you could maybe add methods to nsIFormControl.

If we really do need to keep the nsIDOMWF2InputElement it needs to have a different name since it's implemented by textareas.

>Index: content/html/content/src/nsHTMLFormElement.cpp
>+nsHTMLFormElement::CheckValidity(PRBool *_retval)
...
>+    if (*_retval && !validity) {
>+      *_retval = PR_FALSE;

No need to check if _retval has already been set. That checks costs you about as many cycles as setting too often will.

Also, prefix the _retval argument with 'a' (like aResult).

>Index: content/html/content/src/nsHTMLInputElement.cpp
>+nsHTMLInputElement::GetWillValidate(PRBool *aWillValidate)
>+{
>+  *aWillValidate = PR_FALSE;
>+
>+  if (!mForm)
>+    return NS_OK;
>+
>+  /* XXX ajvincent No ancestor of this node may be a repetition element or 
>+     a datalist element, but those haven't been implemented yet.
>+   */
>+
>+  if (!HasAttr(kNameSpaceID_None, nsHTMLAtoms::name))
>+    return NS_OK;
>+
>+  if (HasAttr(kNameSpaceID_None, nsHTMLAtoms::disabled))
>+    return NS_OK;
>+
>+  if (mType == NS_FORM_INPUT_RESET || mType == NS_FORM_INPUT_BUTTON)
>+    return NS_OK;
>+

Combine these to a single if-statement. You could even do

*aWillValidate = mForm && HasAttr(name) && !HasAttr(disabled) &&
                 !(mType == NS_FORM_INPUT_RESET || mType == NS_FORM_INPUT_BUTTON)

There's no readonly check. You should test for HIDDEN along with the other types.

>+nsHTMLInputElement::GetValidity(nsIDOMWF2ValidityState * *aValidity)
>+{
>+  NS_ENSURE_ARG_POINTER(aValidity);
>+
>+  nsresult rv = NS_OK;
>+  PRBool valid;
>+
>+  if (!mValidity)
>+    rv = CheckValidity(&valid);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  /* XXX ajvincent Yes, I know there's a build warning from not using valid.
>+     By calling on CheckValidity, we initialize the validity state object,
>+     but also dispatch the invalid event if we're invalid.
>+
>+     Suggestions would be welcome.
>+   */

I don't think you want to call CheckValidity since that might fire an event, and I can't find anything in the spec saying that getting .validity should do that. If however that is the correct behaviour, just declare |valid| inside the if-statement to get rid of the warning.

The solution here is probably to let CheckValidity call GetValidity rather than the other way around.

And only test rv when you're actually setting it, i.e. inside the if-statement.

>+NS_IMETHODIMP
>+nsHTMLInputElement::CheckValidity(PRBool *_retval)
>+{
>+  if (!mValidity)
>+    mValidity = new nsDOMWF2ValidityState();
>+  if (!mValidity)
>+    return NS_ERROR_OUT_OF_MEMORY;

Put the second if-statement inside the first. Use NS_ENSURE_TRUE.

>+  nsresult rv;
>+
>+  if ((mType != NS_FORM_INPUT_HIDDEN) &&
>+      (mType != NS_FORM_INPUT_IMAGE) &&
>+      (mType != NS_FORM_INPUT_BUTTON)) {
>+    // Check required attribute.
>+
>+/* XXX ajvincent "For disabled controls, the (required) attribute has no 
>+   effect."
>+
>+   What does this mean?  The missingValue bit of validityState is either on or
>+   off.  Do I need to change the required bit if we're disabled?  Or should I
>+   leave it alone?
>+
>+   Until I get clarification, I will assume disabled attribute has no bearing
>+   on updating the validity state's missingValue bit.
>+ */

Fix indentation. Please don't leave comments like this in the code. You're writing the code so you should find out. Ask Hixie.

>+    PRBool required;
>+    rv = GetRequired(&required);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    if (required) {
>+      switch (mType) {
>+        case NS_FORM_INPUT_CHECKBOX:
>+        case NS_FORM_INPUT_RADIO:
>+        { // In braces to contain local variables.
>+          if (GET_BOOLBIT(mBitField, BF_CHECKED)) {
>+            // We're good.
>+            mValidity->SetValueMissing(PR_FALSE);
>+            break;
>+          }

What happenes if required is changed on the fly? That doesn't seem to update the validitystate as needed? What you instead should do is to make the validitystate query the element inside validitystate::GetValueMissing.

Same thing applies to all other validiystate bits, so i'll review the rest of this function when there's a new patch.


>Index: content/html/content/src/nsHTMLTextAreaElement.cpp

The comments to nsHTMLInputElement also applies to this file.
Comment 12 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-09-06 16:45:41 PDT
Comment on attachment 231275 [details] [diff] [review]
Supplementary layout module patch

Nothing of this should be needed.
Comment 13 Alex Vincent [:WeirdAl] 2006-09-06 23:23:55 PDT
(In reply to comment #11)
> Validation may still need its own interface though. Or you could maybe add
> methods to nsIFormControl.

The ValidityState interface is clearly defined in WF2, so I must implement it.  I'll therefore stick with nsDOMWF2ValidityState.  That said, I realized an edge case on the radio/checkbox valueMissing issue where I must anticipate form controls with the same name but not radio/checkbox types:

<input type="radio" name="foo" required="required"/>
<input type="text" name="foo" value="Hello World"/><!-- stupid authors -->
<select name="foo"/><!-- equally stupid authors -->

Since I now (after fixing review comments) implement GetValidity on nsIDOMNSHTMLInputElement and nsIDOMHTMLTextAreaElement, I'll want to try getting a validity state for other form control types as well.  I'm thinking 

nsIDOMWF2ValidityState GetValidityState();

on nsIFormControl.  The caller of GetValidityState() will have to do the OOM check.  What do you think?

> There's no readonly check. You should test for HIDDEN along with the other
> types.

It's not my fault if the spec changes three weeks after I posted the patch for review.  :)

> Please don't leave comments like this in the code. You're
> writing the code so you should find out. Ask Hixie.

Clarification requested via WHATWG.
Comment 14 Alex Vincent [:WeirdAl] 2006-09-10 00:57:51 PDT
Created attachment 237590 [details] [diff] [review]
patch, v2
Comment 15 Mark Pilgrim (inactive) 2006-09-14 13:38:17 PDT
This needs to tie into the accessibility code so we expose the required property correctly to assistive technologies.
Comment 16 Alex Vincent [:WeirdAl] 2006-09-14 13:50:29 PDT
Mark: that's planned as a follow-up bug (one of very many).  I didn't want to file it until a patch for this bug received r+/sr+.
Comment 17 Alex Vincent [:WeirdAl] 2006-09-17 23:55:20 PDT
Comment on attachment 237590 [details] [diff] [review]
patch, v2

Bug 347165 has just obsoleted my patch.
Comment 18 Alex Vincent [:WeirdAl] 2006-09-20 00:24:18 PDT
Created attachment 239321 [details]
perf testcase
Comment 19 Alex Vincent [:WeirdAl] 2006-09-20 00:35:24 PDT
Created attachment 239325 [details] [diff] [review]
patch, v2.1

I will not seek reviews for this patch.  I had a bad feeling that I was letting a performance loophole slip by in my code today, and unfortunately I was right.

Thanks to bz for suggesting this testcase, although he suggested sets of 100, 1,000 and 10,000 radio inputs.  The time spent on 100 and 1,000 radio inputs shows this is unnecessary (unless you want to wait 200+ minutes).

The problem is this:

<form action="javascript:void()">
<input type="radio" name="foo" required="required">
<input type="radio" name="foo" required="required">
</form>

In this form, each radio input has to check all the others.  So it's a squared increase in processing time just for validity checking.  10 times as many radio inputs means 100 times as long in processing.  Unacceptable.

Suggestions?  I'm thinking of a cache of the valid controls at the form level, started when a checkValidity() operation (either form or input) begins, and stopped when the same operation ends.  Unless someone changes an input value during an invalid event, this should suffice.
Comment 20 alexander :surkov 2006-09-20 06:07:39 PDT
(In reply to comment #19)

> Suggestions?  I'm thinking of a cache of the valid controls at the form level,
> started when a checkValidity() operation (either form or input) begins, and
> stopped when the same operation ends.  Unless someone changes an input value
> during an invalid event, this should suffice.
> 

Probably, do you need something like xforms MDG (http://lxr.mozilla.org/mozilla/source/extensions/xforms/nsXFormsMDGEngine.h)? It holds all states of and relations between instance nodes. Though I don't know much about wf2 and I can't say how xforms mdg will fit for wf.
Comment 21 Boris Zbarsky [:bz] 2006-09-22 13:21:36 PDT
The form has a name-to-list hashtable, no?  You could keep, for each list, also a count of how many items in it are valid or whatever.  That would be O(1) to update on mutations (increment or decrement a counter) and O(1) (well, O(log N) if hashtable collisions are bad) to check.

But maybe I'm misunderstanding the problem you're trying to solve.
Comment 22 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-09-22 17:25:16 PDT
My question is, how long did it take for 100 radios and for a 1000 radios? How long did it take before the patch?
Comment 23 Alex Vincent [:WeirdAl] 2006-09-22 20:27:21 PDT
The question isn't entirely relevant at this stage; with this patch, validity checking on the required attribute is not yet tied into form processing at any point.  I was saving that for another bug yet to be filed.  So I can't test a "before" case.

I am now beginning to regret my optimism of comment 0... :)

sicking asked me how long it took to submit the form, and how long it took to do the checkValidity test.

Test results on calling form.checkValidity:
15 milliseconds elapsed (test 1, form form_1).
16 milliseconds elapsed (test 2, form form_1).
15 milliseconds elapsed (test 3, form form_1).
15 milliseconds elapsed (test 4, form form_1).
32 milliseconds elapsed (test 5, form form_1).
15 milliseconds elapsed (test 6, form form_1).
16 milliseconds elapsed (test 7, form form_1).
16 milliseconds elapsed (test 8, form form_1).
15 milliseconds elapsed (test 9, form form_1).
16 milliseconds elapsed (test 10, form form_1).
1079 milliseconds elapsed (test 1, form form_2).
1078 milliseconds elapsed (test 2, form form_2).
1078 milliseconds elapsed (test 3, form form_2).
1078 milliseconds elapsed (test 4, form form_2).
1078 milliseconds elapsed (test 5, form form_2).
1094 milliseconds elapsed (test 6, form form_2).
1094 milliseconds elapsed (test 7, form form_2).
1078 milliseconds elapsed (test 8, form form_2).
1078 milliseconds elapsed (test 9, form form_2).
1079 milliseconds elapsed (test 10, form form_2).
142672 milliseconds elapsed (test 1, form form_3).
182438 milliseconds elapsed (test 2, form form_3).
188219 milliseconds elapsed (test 3, form form_3).
183797 milliseconds elapsed (test 4, form form_3).
180031 milliseconds elapsed (test 5, form form_3).
180219 milliseconds elapsed (test 6, form form_3).
179375 milliseconds elapsed (test 7, form form_3).
178984 milliseconds elapsed (test 8, form form_3).
178562 milliseconds elapsed (test 9, form form_3).
180203 milliseconds elapsed (test 10, form form_3).

On average, it takes three minutes to check validity on the form with 10,000 identical radio inputs.  It takes maybe a second to submit the form...
Comment 24 Christian Schmidt 2006-09-23 13:39:41 PDT
FWIW these are the results when running the testcase in Opera 9.01 on Linux on a three years old AMD Athlon 2500+ 1800 MHz:

25 milliseconds elapsed (test 1, form form_1).
103 milliseconds elapsed (test 2, form form_1).
40 milliseconds elapsed (test 3, form form_1).
10 milliseconds elapsed (test 4, form form_1).
11 milliseconds elapsed (test 5, form form_1).
11 milliseconds elapsed (test 6, form form_1).
14 milliseconds elapsed (test 7, form form_1).
11 milliseconds elapsed (test 8, form form_1).
10 milliseconds elapsed (test 9, form form_1).
11 milliseconds elapsed (test 10, form form_1).
42 milliseconds elapsed (test 1, form form_2).
38 milliseconds elapsed (test 2, form form_2).
53 milliseconds elapsed (test 3, form form_2).
36 milliseconds elapsed (test 4, form form_2).
37 milliseconds elapsed (test 5, form form_2).
39 milliseconds elapsed (test 6, form form_2).
82 milliseconds elapsed (test 7, form form_2).
37 milliseconds elapsed (test 8, form form_2).
44 milliseconds elapsed (test 9, form form_2).
59 milliseconds elapsed (test 10, form form_2).

AFAICS Opera fully supports the "required" attribute.
Comment 25 Alex Vincent [:WeirdAl] 2006-09-26 13:01:03 PDT
Created attachment 240199 [details]
perf testcase, checkboxes
Comment 26 Alex Vincent [:WeirdAl] 2006-09-26 14:38:47 PDT
Created attachment 240209 [details] [diff] [review]
patch, v2.2

Well, with bz's help, I tackled that perf problem.  Radio and checkbox validity checking now happens in well under a second, even with ten thousand identically named controls.

A couple observations:

(1) I'm not really pleased with nsDOMWF2ValidityState::GetValueMissing at this time.  It's repeating a lot more code than I feel comfortable with.  I could optimize this by adding more methods to nsIFormControl, but the question is, which ones?

(2) The perf testcases here uncheck all the inputs before starting the timer and validation.  An interesting side effect is that GetValueMissing operates measurably faster when at least one control is checked than when none are checked, in both the radio and checkbox cases.  I don't know why.
Comment 27 Alex Vincent [:WeirdAl] 2006-09-26 18:38:47 PDT
Comment on attachment 240209 [details] [diff] [review]
patch, v2.2

<bz>	well
	what I mean is that we should be using internal COM-less code
	imo
	So nsHTMLInputElement::CheckValidity shouldn't create this "validity state" crap
<WeirdAl>	bz: well, someone's gotta create it.
<bz>	why?
	Why do you need a validity state object to return a boolean?
<WeirdAl>	the validity checking happens in the validity state object.
	are you suggesting it shouldn't?
<bz>	yes
	I'm suggesting it should happen in a non-COM function
	called by the state object and by nsHTMLInputElement::CheckValidity
	So that the latter doesn't have to create the former
	And in particular so form submission doesn't have to create all those nasty objects
	in the 99.999% case of the form needing no validation whatsoever
	I should note that then you could avoid the COM form of GetRequired too, maybe
* bz	mutters about a non-COM version of GetWillValidate too

...

<bz>	Or put some of it on nsIFormControl
	For example,
	+ nsCOMPtr<nsIDOMNSHTMLTextAreaElement> input = do_QueryInterface(mOwnerElement);
	+ NS_ASSERTION(input, "No html:textarea element?");
	+
	+ PRBool required;
	+ nsresult rv = input->GetRequired(&required);
	That should be a method on nsIFormControl returning a boolean
	<WeirdAl>	yeah, I thought a fair bit could go on nsIFormControl
	<biesi>	bz, or a static method on this validity state class
	<bz>	or what biesi said
	that last might be best, in fact.
	<WeirdAl>	whoa, wait a sec...
	nsIFoo.h can have methods that don't return nsresult?
	<bz>	yes
Comment 28 Alex Vincent [:WeirdAl] 2006-09-30 10:37:34 PDT
Created attachment 240733 [details] [diff] [review]
patch, v3

Okay, this gets rid of all those "nasty" object creations.

I'm going to attach a HTML table in a moment outlining performance numbers showing what I saw in comment 26 #2.  I still see that result (zero checked taking significantly longer than at least one checked).
Comment 29 Alex Vincent [:WeirdAl] 2006-09-30 10:50:49 PDT
Created attachment 240734 [details]
perf testcase results
Comment 30 jpl24 2006-10-05 21:45:05 PDT
Drive by review comments, I'm of course not an official reviewer.
 
NS_IMETHODIMP
+nsHTMLFormElement::AddCheckboxCheck(nsAString& aName)
+{
+  PRUint32 count = GetCheckedCount(aName);
+
+  count++;
+  PRBool valueSet = mCheckedBoxes.Put(aName, count);
+  return valueSet ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
+}
+
+NS_IMETHODIMP
+nsHTMLFormElement::RemoveCheckboxCheck(nsAString& aName)
+{
+  PRUint32 count = GetCheckedCount(aName);
+
+  count--;
+  PRBool valueSet = mCheckedBoxes.Put(aName, count);
+  return valueSet ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
+}

It's unfortunate that you have to do two lookups to increment or decrement a value. But looking at the nsDataHashTable/nsBaseHashTable interfaces there doesn't seem to be a way to get the entry. I'm not sure how performance sensitive this code is, but you might want to avoid the virtual call for GetCheckedCount. You could inline the code, or make a helper function(or hope nsIForm gets removed).

I don't think mCheckedBoxes.Put can fail when decrementing a count, as there shouldn't be any allocation going on.

+  nsCOMPtr<nsIFormControl> control;
+
+  PRUint32 len = sortedControls.Length();
+  for (PRUint32 i = 0; i < len; ++i) {
+    control = sortedControls[i];

Do you need the extra reference to the form control? Hmmm, maybe you do because the invalid event can be fired from within the loop.
Comment 31 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-11-21 17:01:57 PST
Comment on attachment 240733 [details] [diff] [review]
patch, v3

The whole AddCheckboxCheck/RemoveCheckboxCheck thing doesn't work. If a checkbox is moved from one form to another things stop working. Also it wastes a lot of memory.

I can think of two solutions:
Either do what bz suggested in comment 21 and store the count of checked checkboxes for a given name in the same hash that we currently use to map name->list-of-controls. You would basically have to adjust the AddCheckboxCheck/RemoveCheckboxCheck functions to use that hash, and make sure to also update the hash when checkboxes are moved in and out of a form. You would also have to deal with controls changing into and from being checkboxes.

Alternatively, you could let nsHTMLFormElement::CheckValidity create a hash on the stack and pass that has into the calls to the individual form controls when asking if they are valid or not. A checkbox would look up the result in that hash and return the result if it's already in there. If it's not in the hash the checkbox would ask the form for a list of all controls with the same name and walk that list to see if there are any checked checkboxes and then store the result in the hash before returning it.


What's the purpose of having both IsValid() and CheckValidity() on nsIFormControl?

DispatchInvalidEvent seems like it belongs in nsContentUtils rather than nsIFormControl.


>Index: content/html/content/public/nsDOMWF2ValidityState.h

>+class nsDOMWF2ValidityState : public nsIDOMWF2ValidityState
>+{
>+  public:
>+    NS_DECL_ISUPPORTS
>+    NS_DECL_NSIDOMWF2VALIDITYSTATE
>+
>+    nsDOMWF2ValidityState(nsIFormControl * aOwnerElement);
>+
>+    virtual ~nsDOMWF2ValidityState();
>+
>+  private:
>+    PRBool mValueMissing;

What is the purpose of this member?

>+    nsIFormControl* mOwnerElement;

This can become dangling if the element is destroyed before the validitystate is.

>+  protected:
>+    /* additional members */

Why this comment?

>+#define NS_VALIDITYSTATE_CID                        \
>+ { /* 215793da-b1b2-4784-9f2d-f6368a90302c */       \
>+   0x215793da, 0xb1b2, 0x4784,                      \
>+  {0x9f, 0x2d, 0xf6, 0x36, 0x8a, 0x90, 0x30, 0x2c} }
>+#define NS_VALIDITYSTATE_CONTRACTID \
>+"@mozilla.org/webforms2-validity-state;1"

No need for this. It's only needed if you expect that this object needs to be created through XPCOM.



>Index: content/html/content/src/nsDOMWF2ValidityState.cpp
>+nsDOMWF2ValidityState::nsDOMWF2ValidityState(nsIFormControl *aOwnerElement) :
>+  mValueMissing(PR_FALSE),
>+  mOwnerElement(aOwnerElement)
>+{
>+  /* member initializers and constructor code */
>+}

No need for this comment. It's clear that that is the constructor.

>+
>+nsDOMWF2ValidityState::~nsDOMWF2ValidityState()
>+{
>+  /* destructor code */
>+}

No need for this function at all, just use the default constructed one.

>+nsDOMWF2ValidityState::GetValid(PRBool *aValid)
>+{
>+  *aValid = PR_FALSE;
>+  PRBool invalid;
>+  nsresult rv = GetValueMissing(&invalid);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (invalid) {
>+    return NS_OK;
>+  }
>+
>+  *aValid = PR_TRUE;

Just do |*aValid = !invalid|. That also removes the need to init it.

>Index: content/html/content/src/nsGenericHTMLElement.cpp

>+NS_IMETHODIMP
>+nsGenericHTMLFormElement::GetWillValidate(PRBool *aWillValidate)
>+{
>+  return NS_ERROR_NOT_IMPLEMENTED;
>+}
>+
>+NS_IMETHODIMP
>+nsGenericHTMLFormElement::CheckValidity(PRBool *aWillValidate)
>+{
>+  return NS_ERROR_NOT_IMPLEMENTED;
>+}

I would probably make these functions act as if the control is valid since that's what the following two functions do. Aren't they used by things like <object> and <legend>?

>+PRBool
>+nsGenericHTMLFormElement::IsValueMissing() {
>+  return PR_FALSE;
>+}

Put '{' on its own line

>+PRBool
>+nsGenericHTMLFormElement::IsValid() {
>+  return PR_TRUE;
>+}

Put '{' on its own line

>+nsresult
>+nsGenericHTMLFormElement::DispatchInvalidEvent() {
>+  /* XXX ajvincent Per Web Forms 2.0, this is supposed to be in the XML
>+     Events namespace, but we don't support namespaced events yet.
>+   */
>+  // XXX ajvincent Is this needed?  Reference bug 329509.
>+  nsCOMPtr<nsPresContext> presContext = GetPresContext();

Did you check with anyone in that bug if this is needed or not?

>+  nsCOMPtr<nsIDOMDocumentEvent> de = do_QueryInterface(GetOwnerDoc());
>+  NS_ENSURE_TRUE(de, NS_ERROR_UNEXPECTED);
>+  nsCOMPtr<nsIDOMEvent> event;
>+  nsresult rv = de->CreateEvent(NS_LITERAL_STRING("HTMLEvents"),
>+                                getter_AddRefs(event));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = event->InitEvent(NS_LITERAL_STRING("invalid"), PR_TRUE, PR_TRUE);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = nsEventDispatcher::DispatchDOMEvent(NS_STATIC_CAST(nsINode*, this),
>+                                           nsnull, event, presContext, nsnull);
>+  NS_ENSURE_SUCCESS(rv, rv); // To give one last warning

Could you use nsContentUtils::DispatchTrustedEvent or some other helper on nsContentUtils to do all this?

>Index: content/html/content/src/nsGenericHTMLElement.h
>+  NS_IMETHOD GetWillValidate(PRBool *aWillValidate);
>+  NS_IMETHOD CheckValidity(PRBool *_retval);
>+  PRBool IsValueMissing();
>+  PRBool IsValid();
>+  virtual nsresult DispatchInvalidEvent();

Put these with the other nsIFormControl functions.

Ok, I won't go any further given that you may want to change a fair amount of the validation stuff.
Comment 32 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-11-21 17:16:28 PST
Btw, I would recommend the second option since I think that'll be simpler than keeping track of all the mutations that can affect the set of currently checked checkboxes.

Though the first solution does deal better with checkboxes being checked and unchecked during the validation process. Not sure what the expected behavior is there. Maybe you should ask Hixie.
Comment 33 Alex Vincent [:WeirdAl] 2007-02-01 12:38:06 PST
sicking: can you clearly state what the precise question is for Hixie to answer from comment 32?

Also, I added the "helpwanted" keyword because I have no idea how to implement the second proposed solution in comment 31 (third paragraph).
Comment 34 Hixie (not reading bugmail) 2007-02-01 12:41:59 PST
WeirdAl tried to ask me, but he wasn't sure what he was asking. If you could ask me directly, I could work out what the question is and answer it. :-) I'm afraid I don't understand the problem from the comments above.
Comment 35 Alex Vincent [:WeirdAl] 2007-02-14 13:15:44 PST
sicking says I can choose either option 1 or option 2 from comment 31; I think I'll try option 1 (which refers ultimately back to comment 21).
Comment 36 Alex Vincent [:WeirdAl] 2009-08-08 10:28:59 PDT
I'm starting to think about picking up work on this again (though if anyone else has free cycles and wants to work on it, be my guest).  I'm wondering if I should merge in the patch from bug 345624 - the two patches have to land together.
Comment 37 Marco Zehe (:MarcoZ) 2009-08-10 03:29:45 PDT
Alex, when you do get back to this, please include the accessibility part. On the element, STATE_REQUIRED needs to be set whenever this attribute is present, similar to what we do for WAI-ARIA. Let me know if you need any help with this!
Comment 38 Mounir Lamouri (:mounir) 2010-04-13 10:42:18 PDT
Like for bug 345624, I'm taking the ownership of this bug because Alex has not enough time to dedicate to it.
Patches following.
Comment 39 Mounir Lamouri (:mounir) 2010-04-13 10:44:12 PDT
Created attachment 438791 [details] [diff] [review]
Tests v1
Comment 40 Mounir Lamouri (:mounir) 2010-04-13 10:45:02 PDT
Created attachment 438792 [details] [diff] [review]
Patch v1
Comment 41 :Ehsan Akhgari 2010-04-13 11:03:39 PDT
Comment on attachment 438792 [details] [diff] [review]
Patch v1

Rev the IIDs, please?  :-)
Comment 42 Mounir Lamouri (:mounir) 2010-04-13 13:58:10 PDT
Created attachment 438839 [details] [diff] [review]
Patch v1.1

Argh, damn IIDs ! :)
Thanks again for the reminder, Ehsan ;)
Comment 43 Mounir Lamouri (:mounir) 2010-04-13 14:03:43 PDT
By the way, this patch has a small difference from the specifications: when the input element is in file, checkbox or radio state, it is checking if the element is mutable and for the radio state, it is checking if another radio button is checked _and_ mutable.

I've reported this to the working group:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=9505

If this is not accepted, it will be quite easy to remove it.
Comment 44 Alex Vincent [:WeirdAl] 2010-04-13 14:46:19 PDT
Have you run your patch against the perf testcases in this bug (attachment 239321 [details] and attachment 240199 [details] ) ?
Comment 45 alexander :surkov 2010-04-14 01:19:01 PDT
I field bug 559275 for accessibility support of @required attribute.
Comment 46 Mounir Lamouri (:mounir) 2010-04-14 09:32:42 PDT
The radio button test results:
60 milliseconds elapsed (test 1, form form_1).
31 milliseconds elapsed (test 2, form form_1).
31 milliseconds elapsed (test 3, form form_1).
29 milliseconds elapsed (test 4, form form_1).
39 milliseconds elapsed (test 5, form form_1).
30 milliseconds elapsed (test 6, form form_1).
31 milliseconds elapsed (test 7, form form_1).
29 milliseconds elapsed (test 8, form form_1).
31 milliseconds elapsed (test 9, form form_1).
39 milliseconds elapsed (test 10, form form_1).
423 milliseconds elapsed (test 1, form form_2).
362 milliseconds elapsed (test 2, form form_2).
303 milliseconds elapsed (test 3, form form_2).
300 milliseconds elapsed (test 4, form form_2).
313 milliseconds elapsed (test 5, form form_2).
301 milliseconds elapsed (test 6, form form_2).
297 milliseconds elapsed (test 7, form form_2).
298 milliseconds elapsed (test 8, form form_2).
298 milliseconds elapsed (test 9, form form_2).
388 milliseconds elapsed (test 10, form form_2).
Test completed.

I had to change the test because checkValidity() is not implemented in the form at the moment so that is the result when calling checkValidity() for each form elements.

I don't think the checkbox performance test is needed because AFAICT, a required checkbox means the checkbox has to be checked. And creating 10'000 checkboxs on load is just killing my firefox.

The performance test are linear, so I suppose it's okay. Except if there is some issue related to form checking I'm missing but in this case, it will have to be fixed will implementing the form validation.
Comment 47 Mounir Lamouri (:mounir) 2010-04-14 10:45:17 PDT
Created attachment 439024 [details] [diff] [review]
Tests v1.1
Comment 48 Mounir Lamouri (:mounir) 2010-04-14 10:46:04 PDT
Created attachment 439025 [details] [diff] [review]
Patch v1.2
Comment 49 Mounir Lamouri (:mounir) 2010-04-14 10:49:26 PDT
I've removed stuff that are not in the current specifications in these new patches because my request have been rejected.
Even if I've reopen it, that makes me realize it would be better to open a follow-up or update the patch than reverting a change even more for such minor modification.
Comment 50 Alex Vincent [:WeirdAl] 2010-04-14 11:08:21 PDT
(In reply to comment #46)
> I don't think the checkbox performance test is needed because AFAICT, a
> required checkbox means the checkbox has to be checked. And creating 10'000
> checkboxs on load is just killing my firefox.

I *really, really* do not like hearing this.  Why would creating 10K checkboxes on load seriously hurt your Firefox?  I'm running FF3.5.9 stock, and it takes a couple seconds.

I recommend someone look into that in a separate bug.

Also, have you tried selecting a radio or checkbox in each test, then running it?  Attachment 240734 [details] shows why I was concerned about perf:  in a situation with lots of radio buttons in the same radio group, and none of them checked, my code was pretty slow.  But with at least one checked, it was decently fast.  I'm just hoping you didn't fall into the same trap I did.

I don't know.  It's been a few years since I wrote those tests, and I'm not aware of any real-world situation where any form had a thousand radio buttons or a thousand check boxes.  But when it takes 500+ milliseconds to run that through checkValidity, and checkValidity could be required for form submission, it makes me think twice.

It's entirely possible I'm being overconservative and asking for optimization where it doesn't matter.  In any case, I'll be taking a look at your patch this weekend.
Comment 51 Mounir Lamouri (:mounir) 2010-04-14 11:20:51 PDT
(In reply to comment #50)
> (In reply to comment #46)
> > I don't think the checkbox performance test is needed because AFAICT, a
> > required checkbox means the checkbox has to be checked. And creating 10'000
> > checkboxs on load is just killing my firefox.
> 
> I *really, really* do not like hearing this.  Why would creating 10K checkboxes
> on load seriously hurt your Firefox?  I'm running FF3.5.9 stock, and it takes a
> couple seconds.
> 
> I recommend someone look into that in a separate bug.

On my 'stable' Firefox too. But on trunk it's quite slow. This version is compiled with debug symbol and without optimizations so I suppose it may be linked.
For the same reason, the time spent to run the tests should not be considered, only the relation between the different tests is important.

> Also, have you tried selecting a radio or checkbox in each test, then running
> it?  Attachment 240734 [details] shows why I was concerned about perf:  in a situation
> with lots of radio buttons in the same radio group, and none of them checked,
> my code was pretty slow.  But with at least one checked, it was decently fast. 
> I'm just hoping you didn't fall into the same trap I did.

I'm using |GetCurrentRadioButton()| which is in O(1) so I don't think we could fall in a performance issue here.

> I don't know.  It's been a few years since I wrote those tests, and I'm not
> aware of any real-world situation where any form had a thousand radio buttons
> or a thousand check boxes.  But when it takes 500+ milliseconds to run that
> through checkValidity, and checkValidity could be required for form submission,
> it makes me think twice.
> 
> It's entirely possible I'm being overconservative and asking for optimization
> where it doesn't matter.  In any case, I'll be taking a look at your patch this
> weekend.
Comment 52 Alex Vincent [:WeirdAl] 2010-04-15 00:07:13 PDT
Comment on attachment 439025 [details] [diff] [review]
Patch v1.2

This has been on my mind most of the day, so I think I should say it now for content peer opinions.

>+PRBool
>+nsHTMLInputElement::DoesReadOnlyApply() const
>+{
>+  // List the types for which the attribute does not apply.
>+  // TODO: add NS_FORM_INPUT_RANGE and NS_FORM_INPUT_COLOR when implemented.
>+  return mType != NS_FORM_INPUT_HIDDEN && mType != NS_FORM_INPUT_BUTTON &&
>+         mType != NS_FORM_INPUT_IMAGE  && mType != NS_FORM_INPUT_RESET &&
>+         mType != NS_FORM_INPUT_SUBMIT && mType != NS_FORM_INPUT_RADIO &&
>+         mType != NS_FORM_INPUT_FILE   && mType != NS_FORM_INPUT_CHECKBOX;
>+}
>+
>+PRBool
>+nsHTMLInputElement::DoesRequiredApply() const
>+{
>+  // List the types for which the attribute does not apply.
>+  // TODO: add NS_FORM_INPUT_RANGE and NS_FORM_INPUT_COLOR when implemented.
>+  return mType != NS_FORM_INPUT_HIDDEN && mType != NS_FORM_INPUT_BUTTON &&
>+         mType != NS_FORM_INPUT_IMAGE  && mType != NS_FORM_INPUT_RESET &&
>+         mType != NS_FORM_INPUT_SUBMIT;
>+}
>+

As written, these are fine, but I feel for forwards-compatibility (HTML 6, anyone?), there's something missing.

Here's what I'm thinking:
PRBool
nsHTMLInputElement::DoesReadOnlyApply() const
{
  switch (mType) {
  case NS_FORM_INPUT_HIDDEN:
  case NS_FORM_INPUT_BUTTON:
  case NS_FORM_INPUT_IMAGE:
  case NS_FORM_INPUT_RESET:
  case NS_FORM_INPUT_SUBMIT:
  case NS_FORM_INPUT_RADIO:
  case NS_FORM_INPUT_FILE:
  case NS_FORM_INPUT_CHECKBOX:
    return PR_FALSE;

#ifdef DEBUG
  case NS_FORM_INPUT_TEXT:
  // All other known input types
    break;

  default:
    NS_NOTYETIMPLEMENTED("Unexpected input type in DoesReadOnlyApply");
#endif
  } // switch
  return PR_TRUE;
}

I know this isn't the code style used in content code normally.  What I'm thinking is, when we add new input types, the assertion will force us to explicitly recognize them in functions like DoesReadOnlyApply or DoesRequiredApply.

Also, when --disable-debug, this results in logic identical to your form above.

What do you think about this, smaug, sicking?

volkmar:  Don't feel compelled to do this.  This is my opinion only, and might be overkill.
Comment 53 Mounir Lamouri (:mounir) 2010-04-15 02:47:08 PDT
In my opinion, that's fine. Quite verbose but reasonable.
In general, nsHTMLInputElement should be improved to have a better management of types. At the moment, it's really messy and it is going to be worst with all the new types coming with HTML5.
Comment 54 Alex Vincent [:WeirdAl] 2010-04-17 10:22:54 PDT
Comment on attachment 439025 [details] [diff] [review]
Patch v1.2

First off, jst-review simulacrum likes this patch - its only complaint is about long text lines.

http://beaufour.dk/jst-review/

>diff -r 31902e0dc035 content/html/content/src/nsConstraintValidation.cpp

Obviously this file hasn't been landed yet on mozilla-central.  :)  The change here looks good.

>diff -r 31902e0dc035 content/html/content/src/nsHTMLInputElement.cpp
> protected:
>+  enum ValueModeType
>+  {
>+    VALUE_MODE_DEFAULT,
>+    VALUE_MODE_VALUE,
>+    VALUE_MODE_DEFAULT_ON,
>+    VALUE_MODE_FILENAME
>+  };

I have no idea what a "value mode type" is.  This needs to be explained in comments:

protected:
  /* A value mode type is a ... */
  enum ValueModeType
  {
    VALUE_MODE_DEFAULT,    // Normal operations
    VALUE_MODE_VALUE,      // something else where value matters
    VALUE_MODE_DEFAULT_ON, // something where "on" matters
    VALUE_MODE_FILENAME    // something different for file inputs
  };

+
+  /**
+   * Get the mutable state of the element.
+   */
+  PRBool IsMutable() const;

Again, not quite clear.  How about:

/**
 * If the element is disabled or readonly in practice (having the attribute 
 * alone might not be enough), then the node is not mutable.  Otherwise, the
 * node is considered mutable.  Get this mutable state of the element.
 */
PRBool IsMutable() const;

>@@ -1340,17 +1369,19 @@ nsHTMLInputElement::RadioSetChecked(PRBo
>-  if (container && nameExists) {
>+  nsCOMPtr<nsIRadioGroupContainer> container = GetRadioGroupContainer();

A quick glance at RadioSetChecked says you've already got this defined in the same function (and I think, the same scope) :

nsCOMPtr<nsIRadioGroupContainer> container = GetRadioGroupContainer();

http://hg.mozilla.org/mozilla-central/annotate/ffafbee8b4b4/content/html/content/src/nsHTMLInputElement.cpp#l1289

>+already_AddRefed<nsIDOMHTMLInputElement>
>+nsHTMLInputElement::GetSelectedRadioButton()
>+{
>+  nsIDOMHTMLInputElement* selected;
>+  nsCOMPtr<nsIRadioGroupContainer> container = GetRadioGroupContainer();
>+
>+  if (!container) {
>+    return nsnull;
>+  }
>+
>+  nsAutoString name;
>+  if (!GetNameIfExists(name)) {
>+    return nsnull;
>+  }
>+
>+  container->GetCurrentRadioButton(name, &selected);
>+  return selected;
>+}

Are you ABSOLUTELY SURE that selected is already addrefed?  I went to look up the implementation of GetCurrentRadioButton, and found two references.

nsDocument::GetCurrentRadioButton does an addref.

nsHTMLFormElement::GetCurrentRadioButton, I'm not sure about.  Maybe it does, per http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsInterfaceHashtable.h#54 .  It'd be nice if a comment in GetCurrentRadioButton said so.

Both nsDocument and nsHTMLFormElement claim to implement nsIRadioGroupContainer.

>+nsHTMLInputElement::ValueModeType
>+nsHTMLInputElement::GetValueMode() const
>+{
>+  switch (mType)
>+  {
>+    case NS_FORM_INPUT_HIDDEN:
>+    case NS_FORM_INPUT_SUBMIT:
>+    case NS_FORM_INPUT_BUTTON:
>+    case NS_FORM_INPUT_RESET:
>+    case NS_FORM_INPUT_IMAGE:
>+      return VALUE_MODE_DEFAULT;
>+    case NS_FORM_INPUT_CHECKBOX:
>+    case NS_FORM_INPUT_RADIO:
>+      return VALUE_MODE_DEFAULT_ON;
>+    case NS_FORM_INPUT_FILE:
>+      return VALUE_MODE_FILENAME;
>+    default:
>+      return VALUE_MODE_VALUE;
>+  }
>+}

The same comment I made about debug assertions for undefined types applies here too.  Do what you think is best and will get review+.

>+nsHTMLInputElement::IsValueMissing()
>+{
>+  if (GetValueMode() == VALUE_MODE_VALUE) {
...
>+  }
>+
>+  if (mType == NS_FORM_INPUT_CHECKBOX) {
>+    return !GetChecked();
>+  }
>+
>+  if (mType == NS_FORM_INPUT_RADIO) {
...
>+  }
>+
>+  if (mType == NS_FORM_INPUT_FILE) {
...
>+  }
>+
>+  return PR_FALSE;
>+}

*sigh* I'm not sure what to say about a future-proofing debug select here.  GetValueMode() is clearly a shortcut for a number of known types already.  My gut instinct says this code is fine as-is, especially if you do future-proof GetValueMode().

>@@ -3163,16 +3294,44 @@ nsHTMLInputElement::GetValidationMessage
>+    case VALIDATION_MESSAGE_VALUE_MISSING:
>+      {
>+        nsXPIDLString message;
>+        switch (mType)
>+        {
>+          case NS_FORM_INPUT_FILE:
>+            rv = nsContentUtils::GetLocalizedString(nsContentUtils::eDOM_PROPERTIES,
>+                                                    "FileElementSuffersFromBeingMissing",
>+                                                    message);
>+            break;
>+          case NS_FORM_INPUT_CHECKBOX:
>+            rv = nsContentUtils::GetLocalizedString(nsContentUtils::eDOM_PROPERTIES,
>+                                                    "CheckboxElementSuffersFromBeingMissing",
>+                                                    message);
>+            break;
>+          case NS_FORM_INPUT_RADIO:
>+            rv = nsContentUtils::GetLocalizedString(nsContentUtils::eDOM_PROPERTIES,
>+                                                    "RadioElementSuffersFromBeingMissing",
>+                                                    message);
>+            break;
>+          default:
>+            rv = nsContentUtils::GetLocalizedString(nsContentUtils::eDOM_PROPERTIES,
>+                                                    "TextElementSuffersFromBeingMissing",
>+                                                    message);
>+        }
>+        aValidationMessage = message;
>+      }
>+      break;

I don't like this section.  I think it needs a little refactoring.

Try this (or some variant that would actually compile).

   case VALIDATION_MESSAGE_VALUE_MISSING:
   {
     nsXPIDLString message;
     nsCString key;
     switch (mType) {
     case NS_FORM_INPUT_FILE:
       key.Assign("FileElementSuffersFromBeingMissing");
       break;
     // ..
     }

     rv = nsContentUtils::GetLocalizedString(nsContentUtils::eDOM_PROPERTIES, key, message);
     NS_ENSURE_SUCCESS(rv, rv);
     aValidationMessage = message;
     break;
   }

I don't think I need to look over the textarea changes; they're similar enough to the input changes.

>diff -r 31902e0dc035 dom/locales/en-US/chrome/dom/dom.properties
>+TextElementSuffersFromBeingMissing=This field is mandatory, you have to fill it.
>+CheckboxElementSuffersFromBeingMissing=This checkbox is mandatory, you have to check it.
>+RadioElementSuffersFromBeingMissing=You have to select one of these options.
>+FileElementSuffersFromBeingMissing=You have to select a file.

Grammar nit:  the first two are run-on sentences.

Also, they might send the wrong message.  I'm wondering if we can find some better wording, especially since required checkboxes typically indicate "Terms and conditions" the user must accept before proceeding.

"This field is mandatory.  To proceed, you must enter a value here."
"The form will not be accepted unless this checkbox is checked."

feedback+ with all nits at least examined.
Comment 55 Alex Vincent [:WeirdAl] 2010-04-17 10:25:28 PDT
(In reply to comment #54)
> A quick glance at RadioSetChecked says you've already got this defined in the
> same function (and I think, the same scope) :

D'oh!  I missed that you'd removed the first reference in the patch.  Ignore this comment.
Comment 56 Alex Vincent [:WeirdAl] 2010-04-17 10:29:30 PDT
(In reply to comment #54)
>      rv = nsContentUtils::GetLocalizedString(nsContentUtils::eDOM_PROPERTIES,
> key, message);

I meant key.get().
Comment 57 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-04-19 02:48:56 PDT
(In reply to comment #51)
> On my 'stable' Firefox too. But on trunk it's quite slow. This version is
> compiled with debug symbol and without optimizations so I suppose it may be
> linked.

Never ever do performance testing on a debug build. It is just useless.
We run a lot more code, and even non-optimized code, when running debug builds.
Comment 58 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-04-19 02:49:32 PDT
Could you perhaps update the patch to include Alex' comments.
Comment 59 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-04-19 02:58:08 PDT
Comment on attachment 439025 [details] [diff] [review]
Patch v1.2

Waiting for an updated patch.
Comment 60 Boris Zbarsky [:bz] 2010-04-19 06:26:58 PDT
> We run a lot more code, and even non-optimized code

And more to the point we perform debug-only checks which can easily change the algorithmic complexity of code from O(N) to O(N^2) or O(N^3), for example.
Comment 61 Mounir Lamouri (:mounir) 2010-04-19 17:27:00 PDT
Created attachment 440091 [details] [diff] [review]
Patch v1.3

|GetValueModeType|, |DoesReadOnlyApply| and |DoesRequiredApply| are now future-proof as suggested by Alex in comment 52.

For the addref issue, I think |nsDocument::GetCurrentRadioButtion| is called so it does an addref.

I don't get what you meant by "Grammar nit:  the first two are run-on sentences."
For the messages, I did not changed them because they are more or less the same IMO. As I'm not a native english speaker, I can understand I do not feel the difference so if anyone around confirm that the proposed sentences are better, I will change them.

All other points have been fixed in this updated patch.
Comment 62 Alex Vincent [:WeirdAl] 2010-04-19 17:29:35 PDT
Ummm... I think you just reuploaded the previous patch.
Comment 63 Mounir Lamouri (:mounir) 2010-04-19 17:42:30 PDT
Created attachment 440093 [details] [diff] [review]
Tests v1.2

r=smaug

I've added 'search' and 'tel' types in the test because the patch now checks for them.
Comment 64 Mounir Lamouri (:mounir) 2010-04-19 17:44:58 PDT
Created attachment 440094 [details] [diff] [review]
Patch v1.4

Thank you Alex ;)
Comment 65 Mounir Lamouri (:mounir) 2010-04-19 17:54:10 PDT
Created attachment 440098 [details] [diff] [review]
Patch v1.5

Sorry for the noise, I found a nit to fix.
Comment 66 Mounir Lamouri (:mounir) 2010-04-20 05:01:09 PDT
Created attachment 440186 [details] [diff] [review]
Tests v1.3

r=smaug

This is fixing minor stuff (like using a global variable by mistake).
Comment 67 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-04-26 06:00:24 PDT
Did you test the performance on opt build?
Comment 68 Mounir Lamouri (:mounir) 2010-04-26 12:27:43 PDT
I sent the patch in to the try server which is building an opt build.

Results with radio butons:
12 milliseconds elapsed (test 1, form form_1).
10 milliseconds elapsed (test 2, form form_1).
9 milliseconds elapsed (test 3, form form_1).
9 milliseconds elapsed (test 4, form form_1).
9 milliseconds elapsed (test 5, form form_1).
11 milliseconds elapsed (test 6, form form_1).
9 milliseconds elapsed (test 7, form form_1).
11 milliseconds elapsed (test 8, form form_1).
9 milliseconds elapsed (test 9, form form_1).
10 milliseconds elapsed (test 10, form form_1).
110 milliseconds elapsed (test 1, form form_2).
90 milliseconds elapsed (test 2, form form_2).
93 milliseconds elapsed (test 3, form form_2).
97 milliseconds elapsed (test 4, form form_2).
96 milliseconds elapsed (test 5, form form_2).
91 milliseconds elapsed (test 6, form form_2).
88 milliseconds elapsed (test 7, form form_2).
94 milliseconds elapsed (test 8, form form_2).
91 milliseconds elapsed (test 9, form form_2).
92 milliseconds elapsed (test 10, form form_2).

With checkboxes:
10 milliseconds elapsed (test 1, form form_1).
10 milliseconds elapsed (test 2, form form_1).
9 milliseconds elapsed (test 3, form form_1).
8 milliseconds elapsed (test 4, form form_1).
8 milliseconds elapsed (test 5, form form_1).
9 milliseconds elapsed (test 6, form form_1).
8 milliseconds elapsed (test 7, form form_1).
8 milliseconds elapsed (test 8, form form_1).
7 milliseconds elapsed (test 9, form form_1).
9 milliseconds elapsed (test 10, form form_1).
80 milliseconds elapsed (test 1, form form_2).
83 milliseconds elapsed (test 2, form form_2).
75 milliseconds elapsed (test 3, form form_2).
87 milliseconds elapsed (test 4, form form_2).
74 milliseconds elapsed (test 5, form form_2).
76 milliseconds elapsed (test 6, form form_2).
83 milliseconds elapsed (test 7, form form_2).
70 milliseconds elapsed (test 8, form form_2).
79 milliseconds elapsed (test 9, form form_2).
83 milliseconds elapsed (test 10, form form_2).
821 milliseconds elapsed (test 1, form form_3).
713 milliseconds elapsed (test 2, form form_3).
731 milliseconds elapsed (test 3, form form_3).
780 milliseconds elapsed (test 4, form form_3).
753 milliseconds elapsed (test 5, form form_3).
724 milliseconds elapsed (test 6, form form_3).
723 milliseconds elapsed (test 7, form form_3).
770 milliseconds elapsed (test 8, form form_3).
803 milliseconds elapsed (test 9, form form_3).
712 milliseconds elapsed (test 10, form form_3).

As expected, my issue was only related to my debug build.
Comment 69 Mounir Lamouri (:mounir) 2010-04-30 03:49:00 PDT
I'm wondering why select elements can't suffer from being missing. I've open a bug and will open a follow-up if it is accepted.

http://www.w3.org/Bugs/Public/show_bug.cgi?id=9626
Comment 70 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-05-03 13:12:52 PDT
Comment on attachment 440098 [details] [diff] [review]
Patch v1.5

The description for IsMutable was somewhat uninformative. Please add something like:

"Returns false if element is readonly or disabled"
Comment 71 Mounir Lamouri (:mounir) 2010-05-03 17:17:40 PDT
(In reply to comment #70)
> (From update of attachment 440098 [details] [diff] [review])
> The description for IsMutable was somewhat uninformative. Please add something
> like:
> 
> "Returns false if element is readonly or disabled"

Isn't that a good way to have an outdated comment someday ? I think the comment let the reader know what means "mutable" and if he/she wants to know _when_ the element is mutable, he/she has to read the code.
Comment 72 Jeff Walden [:Waldo] (remove +bmo to email) 2010-05-03 17:23:40 PDT
For what it's worth, I was going to comment objecting to such a terse, undescriptive name, maybe suggesting IsReadonlyOrDisabled, but then I looked at the patch/spec and saw it used that precise term.  It's unfortunate the term's so vague, but as long as it's what the spec uses, and as long as the comment links directly to the term's definition, I tend to think IsMutable is the right choice and that the comment is as good as it can be.
Comment 73 Alex Vincent [:WeirdAl] 2010-05-03 17:50:16 PDT
I did object to it, and I agree with Jonas.  See comment 54.

As for the comment being outdated:  Sorry, I don't buy that as a major problem.

For the spec being vague, ping Hixie.  That's our job as implementers of the spec, to point out deficiencies in it.  I do think leaving a link to the specification in the comment is a good idea.
Comment 74 Mounir Lamouri (:mounir) 2010-07-28 06:10:46 PDT
Created attachment 460851 [details] [diff] [review]
Patch v1.6

Updated patch to apply against current tip.
Merged with test patch.
Comment 75 Mounir Lamouri (:mounir) 2010-08-19 14:19:02 PDT
This is now in the tree.
rev: http://hg.mozilla.org/mozilla-central/rev/3281fd2f0005
Comment 76 Mike Beltzner [:beltzner, not reading bugmail] 2010-09-01 11:52:29 PDT
Can webdevelopers override the red border we add to required fields using CSS?
Comment 77 Eric Shepherd [:sheppy] 2010-09-01 11:55:09 PDT
Yes, you use the :required pseudo-class:

https://developer.mozilla.org/en/CSS/%3arequired
Comment 78 :Ms2ger (⌚ UTC+1/+2) 2010-09-01 12:04:44 PDT
(In reply to comment #76)
> Can webdevelopers override the red border we add to required fields using CSS?

input:required { -moz-box-shadow: none; }
Comment 79 Mounir Lamouri (:mounir) 2010-09-01 12:05:13 PDT
:required will not solve their problems.
This red borders is the default style for invalid elements. This can be overridden with:
:invalid { -moz-box-shadow: none; box-shadow: none; }
Comment 80 Mike Beltzner [:beltzner, not reading bugmail] 2010-09-01 14:34:09 PDT
OK, it looks pretty crappy for things like https://support.mozilla.com/en-US/questions/749409
Comment 81 Mounir Lamouri (:mounir) 2010-09-01 14:48:03 PDT
(In reply to comment #80)
> OK, it looks pretty crappy for things like
> https://support.mozilla.com/en-US/questions/749409

Indeed. But websites can just override this default setting.
We need a default style for invalid elements because form submission will be blocked when invalid (that's the plan for beta6). See bug 561636 for more information.
Comment 82 Doug Wright 2010-09-29 06:35:34 PDT
This bug only seems to have covered implementing @required for <input> elements - but per spec it also applies to <select>.

Is there another bug on file for that?
Comment 83 gadjo 2010-09-29 06:40:37 PDT
yes, bug 596511
Comment 84 Boris Zbarsky [:bz] 2010-09-29 07:12:51 PDT
> - but per spec it also applies to <select>.

That was a spec change several weeks after this bug was fixed, for what it's worth.

Note You need to log in before you can comment on or make changes to this bug.