Implement required attribute

RESOLVED FIXED in mozilla2.0b5

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
11 years ago
3 years ago

People

(Reporter: WeirdAl, Assigned: mounir)

Tracking

(Blocks: 1 bug, {access, dev-doc-complete, testcase})

Trunk
mozilla2.0b5
access, dev-doc-complete, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 17 obsolete attachments)

2.31 KB, text/html
Details
2.90 KB, text/html
Details
9.41 KB, text/html
Details
35.02 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
The required attribute is simple to implement.  Patch coming right up.
(Reporter)

Comment 1

11 years ago
Created attachment 230556 [details] [diff] [review]
patch for <html:input/> (almost ready for reviews)
(Reporter)

Comment 2

11 years ago
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.
Status: NEW → ASSIGNED
(Reporter)

Comment 3

11 years ago
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.
(Reporter)

Comment 4

11 years ago
Comment on attachment 230556 [details] [diff] [review]
patch for <html:input/> (almost ready for reviews)

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

Whoops - (!(*_retval))
(Reporter)

Comment 5

11 years ago
Events also appear to be problematic.  :(
(Reporter)

Comment 6

11 years ago
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.
(Reporter)

Comment 7

11 years ago
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.
Attachment #230556 - Attachment is obsolete: true
Attachment #230689 - Flags: review?(bugmail)
(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)
(Reporter)

Comment 9

11 years ago
Note: I forgot to add an entry to allmakefiles.sh for dom/public/idl/web-forms/Makefile.  (Doh!)
(Reporter)

Comment 10

11 years ago
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/>).
Attachment #231275 - Flags: review?(bugmail)
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.
Attachment #230689 - Flags: review?(bugmail) → review-
Comment on attachment 231275 [details] [diff] [review]
Supplementary layout module patch

Nothing of this should be needed.
Attachment #231275 - Flags: review?(bugmail) → review-
(Reporter)

Comment 13

11 years ago
(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.
(Reporter)

Comment 14

11 years ago
Created attachment 237590 [details] [diff] [review]
patch, v2
Attachment #230689 - Attachment is obsolete: true
Attachment #231275 - Attachment is obsolete: true
(Reporter)

Updated

11 years ago
Attachment #237590 - Flags: review?(bugmail)
This needs to tie into the accessibility code so we expose the required property correctly to assistive technologies.
Keywords: access
(Reporter)

Comment 16

11 years ago
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+.
(Reporter)

Comment 17

11 years ago
Comment on attachment 237590 [details] [diff] [review]
patch, v2

Bug 347165 has just obsoleted my patch.
Attachment #237590 - Attachment is obsolete: true
Attachment #237590 - Flags: review?(bugmail)
(Reporter)

Comment 18

11 years ago
Created attachment 239321 [details]
perf testcase
(Reporter)

Comment 19

11 years ago
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

11 years ago
(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.
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.
My question is, how long did it take for 100 radios and for a 1000 radios? How long did it take before the patch?
(Reporter)

Comment 23

11 years ago
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

11 years ago
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.
(Reporter)

Comment 25

11 years ago
Created attachment 240199 [details]
perf testcase, checkboxes
(Reporter)

Comment 26

11 years ago
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.
Attachment #239325 - Attachment is obsolete: true
Attachment #240209 - Flags: review?(bugmail)
(Reporter)

Comment 27

11 years ago
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
Attachment #240209 - Flags: review?(bugmail)
(Reporter)

Comment 28

11 years ago
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).
Attachment #240209 - Attachment is obsolete: true
Attachment #240733 - Flags: review?(bugmail)
(Reporter)

Comment 29

11 years ago
Created attachment 240734 [details]
perf testcase results

Comment 30

11 years ago
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 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.
Attachment #240733 - Flags: review?(bugmail) → review-
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.
(Reporter)

Updated

10 years ago
Keywords: helpwanted
(Reporter)

Comment 33

10 years ago
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).
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.
(Reporter)

Comment 35

10 years ago
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).

Updated

9 years ago
Component: DOM: HTML → DOM: Core & HTML
QA Contact: ian → general
(Reporter)

Updated

8 years ago
(Reporter)

Comment 36

8 years ago
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.
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!
(Reporter)

Updated

8 years ago
Blocks: 506554
(Assignee)

Comment 38

7 years ago
Like for bug 345624, I'm taking the ownership of this bug because Alex has not enough time to dedicate to it.
Patches following.
(Assignee)

Updated

7 years ago
Assignee: ajvincent → mounir.lamouri
Keywords: helpwanted
OS: Windows XP → All
Hardware: x86 → All
Summary: Implement required attribute for <input type="text">, <textarea> → Implement required attribute
(Assignee)

Comment 39

7 years ago
Created attachment 438791 [details] [diff] [review]
Tests v1
Attachment #438791 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 40

7 years ago
Created attachment 438792 [details] [diff] [review]
Patch v1
Attachment #438792 - Flags: review?(Olli.Pettay)
Attachment #438792 - Flags: feedback?
(Assignee)

Updated

7 years ago
Attachment #438792 - Flags: feedback? → feedback?(ajvincent)
Comment on attachment 438792 [details] [diff] [review]
Patch v1

Rev the IIDs, please?  :-)
(Assignee)

Comment 42

7 years ago
Created attachment 438839 [details] [diff] [review]
Patch v1.1

Argh, damn IIDs ! :)
Thanks again for the reminder, Ehsan ;)
Attachment #438792 - Attachment is obsolete: true
Attachment #438839 - Flags: review?(Olli.Pettay)
Attachment #438839 - Flags: feedback?(ajvincent)
Attachment #438792 - Flags: review?(Olli.Pettay)
Attachment #438792 - Flags: feedback?(ajvincent)
(Assignee)

Comment 43

7 years ago
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.
(Reporter)

Comment 44

7 years ago
Have you run your patch against the perf testcases in this bug (attachment 239321 [details] and attachment 240199 [details] ) ?

Updated

7 years ago
Blocks: 559275

Comment 45

7 years ago
I field bug 559275 for accessibility support of @required attribute.
(Assignee)

Comment 46

7 years ago
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.
(Assignee)

Comment 47

7 years ago
Created attachment 439024 [details] [diff] [review]
Tests v1.1
Attachment #438791 - Attachment is obsolete: true
Attachment #439024 - Flags: review?(Olli.Pettay)
Attachment #438791 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 48

7 years ago
Created attachment 439025 [details] [diff] [review]
Patch v1.2
Attachment #438839 - Attachment is obsolete: true
Attachment #439025 - Flags: review?(Olli.Pettay)
Attachment #439025 - Flags: feedback?(ajvincent)
Attachment #438839 - Flags: review?(Olli.Pettay)
Attachment #438839 - Flags: feedback?(ajvincent)
(Assignee)

Comment 49

7 years ago
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.
(Reporter)

Comment 50

7 years ago
(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.
(Assignee)

Comment 51

7 years ago
(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.
(Reporter)

Comment 52

7 years ago
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.
(Assignee)

Comment 53

7 years ago
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.
(Reporter)

Updated

7 years ago
Attachment #439025 - Flags: feedback?(ajvincent) → feedback+
(Reporter)

Comment 54

7 years ago
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.
(Reporter)

Comment 55

7 years ago
(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.
(Reporter)

Comment 56

7 years ago
(In reply to comment #54)
>      rv = nsContentUtils::GetLocalizedString(nsContentUtils::eDOM_PROPERTIES,
> key, message);

I meant key.get().
(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.
Could you perhaps update the patch to include Alex' comments.

Updated

7 years ago
Attachment #439024 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 439025 [details] [diff] [review]
Patch v1.2

Waiting for an updated patch.
Attachment #439025 - Flags: review?(Olli.Pettay)
> 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.
(Assignee)

Comment 61

7 years ago
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.
Attachment #439025 - Attachment is obsolete: true
Attachment #440091 - Flags: review?(Olli.Pettay)
(Reporter)

Comment 62

7 years ago
Ummm... I think you just reuploaded the previous patch.
(Assignee)

Comment 63

7 years ago
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.
Attachment #439024 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Depends on: 456229, 557620
(Assignee)

Comment 64

7 years ago
Created attachment 440094 [details] [diff] [review]
Patch v1.4

Thank you Alex ;)
Attachment #440091 - Attachment is obsolete: true
Attachment #440094 - Flags: review?(Olli.Pettay)
Attachment #440091 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 65

7 years ago
Created attachment 440098 [details] [diff] [review]
Patch v1.5

Sorry for the noise, I found a nit to fix.
Attachment #440094 - Attachment is obsolete: true
Attachment #440098 - Flags: review?(Olli.Pettay)
Attachment #440094 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 66

7 years ago
Created attachment 440186 [details] [diff] [review]
Tests v1.3

r=smaug

This is fixing minor stuff (like using a global variable by mistake).
Attachment #440093 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Blocks: 555559
(Assignee)

Updated

7 years ago
Blocks: 344615
Did you test the performance on opt build?
(Assignee)

Comment 68

7 years ago
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.
(Assignee)

Comment 69

7 years ago
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

Updated

7 years ago
Attachment #440098 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Updated

7 years ago
Attachment #440098 - Flags: superreview?(jonas)
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"
Attachment #440098 - Flags: superreview?(jonas) → superreview+
(Assignee)

Comment 71

7 years ago
(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.
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.
(Reporter)

Comment 73

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

Updated

7 years ago
Blocks: 566348
Keywords: dev-doc-needed
(Assignee)

Updated

7 years ago
Blocks: 558788
(Assignee)

Updated

7 years ago
Blocks: 561635
(Assignee)

Comment 74

7 years ago
Created attachment 460851 [details] [diff] [review]
Patch v1.6

Updated patch to apply against current tip.
Merged with test patch.
Attachment #240733 - Attachment is obsolete: true
Attachment #440098 - Attachment is obsolete: true
Attachment #440186 - Attachment is obsolete: true
(Assignee)

Comment 75

7 years ago
This is now in the tree.
rev: http://hg.mozilla.org/mozilla-central/rev/3281fd2f0005
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
Keywords: dev-doc-needed → dev-doc-complete
Can webdevelopers override the red border we add to required fields using CSS?
Yes, you use the :required pseudo-class:

https://developer.mozilla.org/en/CSS/%3arequired
(In reply to comment #76)
> Can webdevelopers override the red border we add to required fields using CSS?

input:required { -moz-box-shadow: none; }
(Assignee)

Comment 79

7 years ago
: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; }
OK, it looks pretty crappy for things like https://support.mozilla.com/en-US/questions/749409
(Assignee)

Comment 81

7 years ago
(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

7 years ago
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

7 years ago
yes, bug 596511
> - 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.
Blocks: 1015305
You need to log in before you can comment on or make changes to this bug.