580575 - Implement -moz-submit-invalid pseudo-class for submit controls on invalid forms

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mounir Lamouri (:mounir)]

When a form is invalid (following the constraint validation system introduced by HTML5), the form will not be submittable. We want to help the user understand there is an problem in the current form so the submit(s) button(s) in the form could be styled in a specific way with -moz-submit-invalid.

The default style from limi's mockup in bug 561636 would be something like:
-moz-box-shadow: 0 0 2px red;

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 459623 [details] [diff] [review]
Patch v1

>+#define NS_EVENT_STATE_MOZ_SUBMITINVALID (1 << 28)

Please put this at the end of the list, and make it (1 << 30).

>+++ b/content/html/content/src/nsHTMLFormElement.cpp
>+    // An element became valid,

Period at the end there, right?

>+    // if any element is invald, that means nothing changed
>+    // otherwise that means the form just became valid.

Couldn't it have been valid before too, if all the invalid elements were not candidates for constraint validation?  Or can that not happen?

(And fix the capitalization and spelling issues here, please.)

>+    // We do not call CheckValidity() for optimization purpose.

I don't follow this comment.  What does it mean?

>+    PRUint32 length = mControls->mElements.Length();
>+    for (PRUint32 i = 0; i < length; ++i) {

Are we worried about script changing lots of elements all at once giving us O(N^2) behavior?  Would it make sense to track the count of invalid elements in the form, maybe?

>+      nsCOMPtr<nsIConstraintValidation> cvElmt =
>+        do_QueryInterface((nsGenericHTMLElement*)mControls->mElements[i]);

Nix the cast and use do_QueryObject.

>+      if (cvElmt && cvElmt->IsCandidateForConstraintValidation() &&
>+        !cvElmt->IsValid()) {

Fix the indent, please.

>+  // Cache the validity state. Used by GetValidity().
>+  if (aElementValidityState) {
>+    mValidity = PR_TRUE;
>+  } else {
>+    mValidity = PR_FALSE;
>+  }

  mValidity = aElementValidityState;

>+  {
>+    PRUint32 length = mControls->mElements.Length();
>+    for (PRUint32 i = 0; i < length; ++i) {

Nix the outer scope and do:

  for (PRUint32 i = 0, length = mControls->mElements.Length(); 
       i < length; ++i) 

>+          doc->ContentStatesChanged(mControls->mElements[i], nsnull,
>+                                    NS_EVENT_STATE_MOZ_SUBMITINVALID);

You need to have an update batch around this stuff, no?

>+  // Because of backward compatibility, <input type='image'> is not in elements
>+  // so we have to check for controls not in elements. (sic)

Not sure what the "sic" is for here...

>+          doc->ContentStatesChanged(mControls->mNotInElements[i], nsnull,
>+                                    NS_EVENT_STATE_MOZ_SUBMITINVALID);

Again, update batch.

Should we be maintaining a separate array or hashtable of submit controls?  Might be a good idea...

>+++ b/content/html/content/src/nsHTMLFormElement.h
>+   * This method will update the form validity thus updating the state of the
>+   * submit controls (with -moz-submit-invalid pseudo-class).

Why is the part starting with "thus" needed?

>+   * This method has to be called by form elements whenever there validity state
>+   * changes.

s/there/their/

>+++ b/content/html/content/src/nsIConstraintValidation.h

Are we ok with adding all that inlined gunk for SetValidityState?

I didn't review the tests very carefully.

Comment 3

[Mounir Lamouri (:mounir)]

(In reply to comment #2)
> Comment on attachment 459623 [details] [diff] [review]
> Patch v1
> 
> >+#define NS_EVENT_STATE_MOZ_SUBMITINVALID (1 << 28)
> 
> Please put this at the end of the list, and make it (1 << 30).

When I wrote the patch they were no 28 and 29. They have been added yesterday (depending on TZ). I've refreshed my queue list yesterday but it looks like it did that a little bit too early... :(
It would take me some time to update everything now so I will probably attach another patch with 28 but considering the patch will not apply on tip with 28, you can be sure that is going to be fixed.

> >+    // if any element is invald, that means nothing changed
> >+    // otherwise that means the form just became valid.
> 
> Couldn't it have been valid before too, if all the invalid elements were not
> candidates for constraint validation?  Or can that not happen?

Oh... You bring an interesting point here: the validity can change but an invalid element not candidate for constraint validation wouldn't make the form invalid. I didn't thought about that.

I think if we try to keep track on constraint validation changes it's going to be messy (conditions depends on the elements). I think the best way would be to keep track on invalid elements then check if they are barred from constraint validation. Considering valid elements barred from constraint validation does not change anything.

I'm going to update the patch to implement this. Let me know ASAP if you think it's a bad idea/have a better idea.

> >+    // We do not call CheckValidity() for optimization purpose.
> 
> I don't follow this comment.  What does it mean?

CheckValidity() is doing exactly what we want (returns the form validity) but it's always O(n) (min, max and average) contrarily to this loop. Maybe I should remove the comment if it's that confusing.

> >+    PRUint32 length = mControls->mElements.Length();
> >+    for (PRUint32 i = 0; i < length; ++i) {
> 
> Are we worried about script changing lots of elements all at once giving us
> O(N^2) behavior?  Would it make sense to track the count of invalid elements in
> the form, maybe?

I will update the patch to track invalid elements. That will be prevent this problem.

>   for (PRUint32 i = 0, length = mControls->mElements.Length(); 
>        i < length; ++i) 
> 
> >+          doc->ContentStatesChanged(mControls->mElements[i], nsnull,
> >+                                    NS_EVENT_STATE_MOZ_SUBMITINVALID);
> 
> You need to have an update batch around this stuff, no?

I guess that means I need an update batch. I don't know why that's needed (I mean that's working without it) but I've seen some elements do that before changing their states.

> >+  // Because of backward compatibility, <input type='image'> is not in elements
> >+  // so we have to check for controls not in elements. (sic)
> 
> Not sure what the "sic" is for here...

That forces us to look at _all_ form elements just because input type image isn't in the right list. That's sad.

> Should we be maintaining a separate array or hashtable of submit controls? 
> Might be a good idea...

I think that would be a good idea indeed. However, I would prefer to do that in a follow-up.

> >+++ b/content/html/content/src/nsHTMLFormElement.h
> >+   * This method will update the form validity thus updating the state of the
> >+   * submit controls (with -moz-submit-invalid pseudo-class).
> 
> Why is the part starting with "thus" needed?

That's because it's the only reason why this function exists. It's not meant to optimize CheckValidity(). At least, not now. I will update the comment to make that more clear.

> >+++ b/content/html/content/src/nsIConstraintValidation.h
> 
> Are we ok with adding all that inlined gunk for SetValidityState?

What do you mean?

Comment 4

[Mounir Lamouri (:mounir)]

Actually, I realize that tracking the invalid elements can help to know quickly the form validity. However, a submit control will not be informed that the form validity has changed (therefore the pseudo-classes will not match). But, they would know it if they check it (form.GetValidity() would check mInvalidElements).
Unfortunately, I guess having a delay between form validity change and submit control style change isn't acceptable. So maybe I will have to inform the form as soon as an element is barred for constraint validation which doesn't really seem lean and clean :(

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

I've fixed the issue Boris pointed with new tests to cover that.

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.1

I forgot hg add... sorry for the noise :(

Comment 7

[Boris Zbarsky [:bzbarsky]]

> When I wrote the patch they were no 28 and 29.

There sort of were, but in 0x... form.  Your latest patch still has this issue; please fix.

> Let me know ASAP if you think it's a bad idea/have a better idea.

Please cc me on bugs if you ask me a question?  ;)

> Maybe I should remove the comment if it's that confusing.

I think I just misread the comment...   If the loop does something that CheckValidity does as well, it's worth documenting why we do it this way instead.  Just don't aim for a terse comment; explain at length if needed.

> I guess that means I need an update batch. I don't know why that's needed

Because that's what the API calls for.  Right this second and for state changes specifically I _think_ nothing relies on it happening, but your code shouldn't depend on the assumption that this will continue to be the case.

> That forces us to look at _all_ form elements just because input type image
> isn't in the right list. 

99.9% of form controls are in mElements anyway, though...  So looking at the other list is not exactly a hardship.

> However, I would prefer to do that in a follow-up.

OK.  Please file and note in a comment in the code.

> What do you mean?

SetValidityState is inlined.  With your changes it does a good bit of work.  Is it inlined in lots of places?  If not, this is fine.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 461554 [details] [diff] [review]
Patch v2.1

>+++ b/content/events/public/nsIEventStateManager.h
>+#define NS_EVENT_STATE_MOZ_SUBMITINVALID (1 << 28)

Still need to fix this.

>+++ b/content/html/content/src/nsConstraintValidation.cpp
>+  // By default, all elements are not barred from constraint validation.
>+  , mBarredFromConstraintValidation(PR_FALSE)

I'd write that comment as:

  // By default, all elements are subject to constraint validation

>+    nsCOMPtr<nsIFormControl> formCtrl = do_QueryInterface(this);
>+    NS_ASSERTION(formCtrl, "This interface should be used by form elements!");
>+
>+    nsHTMLFormElement* form =
>+      static_cast<nsHTMLFormElement*>(formCtrl->GetFormElement());

It's worth thinking about how we can maybe change our APIs here to make this suck less.... but that would be followups.

>+nsConstraintValidation::SetBarredFromConstraintValidation(PRBool aBarred)
>+      // If the element is going to be barred from constraint validation,
>+      // we can inform the form that we are now valid.
>+      // Otherwise, we are now invalid.
>+      form->UpdateValidity(aBarred);

!aBarred, right?  Add tests?

>+  mBarredFromConstraintValidation = aBarred;

This doesn't look like it correctly handles dynamic changes to mBarredFromConstraintValidation.  So if I have an input that is invalid and I set @disabled on it, it would continue to match :invalid, as far as I can tell.  This used to be handled for you because the barred state was completely determined by the attr value, but now that it's out-of-band state that doesn't change during the normal attr change process you have to notify the change yourself.  Which of course means you need an aNotify parameter here or something...

Add tests?

We really need to move to a setup where state changes are harder to get wrong.  :(

>+++ b/content/html/content/src/nsConstraintValidation.h
>   PRBool GetValidityState(ValidityStateType mState) const {
>+           return mValidityBitField & mState;
>          }

I know this was already here... but this code is wrong.  You need a != 0 or something.

>+++ b/content/html/content/src/nsHTMLButtonElement.cpp
> nsHTMLButtonElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>+        mType = kButtonDefaultType->value;
>+      }
>+      UpdateBarredFromConstraintValidation();

This will mess up dynamic changes to @type affecting validity for buttons.  Then again, that was already broken before this patch, no?

Specifically, changing the type of a button doesn't look like it will update whether :valid and :invalid match....  or am I missing something?  Don't we need to notify this state change?

And now you also need to notify a possible SUBMITINVALID state change here, I'd think.

Also, with your new code changes to @disabled also affect validity only starting here, not when the stored attr value changes, so those changes need to be notified too.

Please double-check the state stuff here and in the underlying patches, per our IRC discussion.  And add tests for all the things you find, and whatever else along these lines I mention below.

> nsHTMLButtonElement::IntrinsicState() const
>+  if (mForm && !mForm->GetValidity() && IsSubmitControl()) {
>+    state |= NS_EVENT_STATE_MOZ_SUBMITINVALID;

So when mForm changes to/from null we also need to notify on the submitinvalid state change, right?

>+++ b/content/html/content/src/nsHTMLFormElement.cpp
>+nsHTMLFormElement::UpdateValidity(PRBool aElementValidity)

>+  aElementValidity ? --mInvalidElementsCount : ++mInvalidElementsCount;

I'd really prefer this as an if/else.

>+  // The form validity has just changed if:
>+  // - there is no more invalid elements ;

"are no more"

>+  // - or, there is one invalid elmement and an element just became invalid.

Nix the comma, please.

>+  if (!(!mInvalidElementsCount ||
>+        (mInvalidElementsCount == 1 && !aElementValidity))) {

  if (mInvalidElementsCount &&
      (mInvalidElementsCount != 1 || aElementValidity)) {

seems more readable to me.  And perhaps document as:

  // If we have invalid elements and we used to before as well, do nothing.

though the existing comment is ok too.

>+      nsIDocument* doc = mControls->mElements[i]->GetCurrentDoc();
>+      if (doc) {
>+        MOZ_AUTO_DOC_UPDATE(doc, UPDATE_CONTENT_STATE, PR_TRUE);

So.  Since we're talking about _current_ docs, the current doc of the control will be the current doc of the form.  Furthermore, you want to wrap a single batch around all your changes.  So I'd do this like so:

  nsIDocument* doc = GetCurrentDoc();
  if (!doc) {
    return;
  }

  MOZ_AUTO_DOC_UPDATE(doc, UPDATE_CONTENT_STATE, PR_TRUE);

and then do your loops.  Always doing the batch should be safe enough perf-wise, since it's very rare for a form to not have any submit controls, I'd think.

Where do we call UpdateValidity() when elements are removed from the form and added to it?  I don't see anything relevant in this patch.  I know it's an edge case...

>+++ b/content/html/content/src/nsHTMLInputElement.cpp
>@@ -455,17 +455,19 @@ nsHTMLInputElement::AfterSetAttr(PRInt32
>+      UpdateBarredFromConstraintValidation();
>       // If we are changing type from File/Text/Tel/Passwd to other input types

I think this will mess up if an <input type="submit"> changes both valid state and moz-submit-invalid state due to a single type change, since you'll end up notifying the two changes separately, right?  You probably want to add SUBMITINVALID to the list of things a type change notifies below here...

>@@ -503,16 +505,19 @@ nsHTMLInputElement::AfterSetAttr(PRInt32
>+      if (aName == nsGkAtoms::readonly || aName == nsGkAtoms::disabled) {
>+        UpdateBarredFromConstraintValidation();
>+      }

Similar here, I think.

>@@ -2770,16 +2775,20 @@ nsHTMLInputElement::IntrinsicState() con
>+  if (mForm && !mForm->GetValidity() && IsSubmitControl()) {
>+    state |= NS_EVENT_STATE_MOZ_SUBMITINVALID;

Again, need to notify when removed from or added to form, right?

>+++ b/content/html/content/src/nsHTMLSelectElement.cpp
>+nsHTMLSelectElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>+    SetBarredFromConstraintValidation(!!aValue);

Need to notify somewhere here, right?

>+++ b/content/html/content/src/nsHTMLTextAreaElement.cpp
> nsHTMLTextAreaElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>+        UpdateBarredFromConstraintValidation();

And here.

Comment 9

[Mounir Lamouri (:mounir)]

(In reply to comment #8)
> >+    nsCOMPtr<nsIFormControl> formCtrl = do_QueryInterface(this);
> >+    NS_ASSERTION(formCtrl, "This interface should be used by form elements!");
> >+
> >+    nsHTMLFormElement* form =
> >+      static_cast<nsHTMLFormElement*>(formCtrl->GetFormElement());
> 
> It's worth thinking about how we can maybe change our APIs here to make this
> suck less.... but that would be followups.

Maybe having nsHTMLFormElement* nsIFormControl::GetForm(); ?

> >+nsConstraintValidation::SetBarredFromConstraintValidation(PRBool aBarred)
> >+      // If the element is going to be barred from constraint validation,
> >+      // we can inform the form that we are now valid.
> >+      // Otherwise, we are now invalid.
> >+      form->UpdateValidity(aBarred);
> 
> !aBarred, right?  Add tests?

No. If the element becomes barred, it is valid. Otherwise, if we are here that means it is invalid. So, |aBarred|.

> >+  mBarredFromConstraintValidation = aBarred;
> 
> This doesn't look like it correctly handles dynamic changes to
> mBarredFromConstraintValidation.  So if I have an input that is invalid and I
> set @disabled on it, it would continue to match :invalid, as far as I can tell.
>  This used to be handled for you because the barred state was completely
> determined by the attr value, but now that it's out-of-band state that doesn't
> change during the normal attr change process you have to notify the change
> yourself.  Which of course means you need an aNotify parameter here or
> something...
> 
> Add tests?

Actually, I've added a lot of tests in bug 558788 (:valid/:invalid) and now this patch is raising some errors like this one.

> >+++ b/content/html/content/src/nsConstraintValidation.h
> >   PRBool GetValidityState(ValidityStateType mState) const {
> >+           return mValidityBitField & mState;
> >          }
> 
> I know this was already here... but this code is wrong.  You need a != 0 or
> something.

I've updated bug 558788 to return a bool.

> >+++ b/content/html/content/src/nsHTMLButtonElement.cpp
> > nsHTMLButtonElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
> >+        mType = kButtonDefaultType->value;
> >+      }
> >+      UpdateBarredFromConstraintValidation();
> 
> This will mess up dynamic changes to @type affecting validity for buttons. 
> Then again, that was already broken before this patch, no?

Fixed in bug 558788.

> Specifically, changing the type of a button doesn't look like it will update
> whether :valid and :invalid match....  or am I missing something?  Don't we
> need to notify this state change?
> 
> And now you also need to notify a possible SUBMITINVALID state change here, I'd
> think.
> 
> Also, with your new code changes to @disabled also affect validity only
> starting here, not when the stored attr value changes, so those changes need to
> be notified too.
> 
> Please double-check the state stuff here and in the underlying patches, per our
> IRC discussion.  And add tests for all the things you find, and whatever else
> along these lines I mention below.

Tests will be added for @type change and :-moz-submit-invalid update.

> > nsHTMLButtonElement::IntrinsicState() const
> >+  if (mForm && !mForm->GetValidity() && IsSubmitControl()) {
> >+    state |= NS_EVENT_STATE_MOZ_SUBMITINVALID;
> 
> So when mForm changes to/from null we also need to notify on the submitinvalid
> state change, right?

Yes.

I'm going to add a aNotify parameter to nsHTMLFormElement::UpdateValidity() so we don't call ContentStatesChanged when aNotify = PR_FALSE. I thought you mentioned that on IRC but your comment wasn't mentioning it. Could you confirm it's needed?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Maybe having nsHTMLFormElement* nsIFormControl::GetForm(); ?

For example, if the consumers are all in gklayout.

> No. If the element becomes barred, it is valid. Otherwise, if we are here that
> means it is invalid. So, |aBarred|.

Ah, indeed.  I'm not sure how I confused myself there...

> Could you confirm it's needed?

Yep.  It is.

Comment 11

[Mounir Lamouri (:mounir)]

Attachment: Patch v3

Yet another try to pass the review :) (this should fix everything mentioned in bugzilla and IRC)

By the way, I was wondering if it would be okay (regarding what is usually done in the tree) to have:
#ifdef DEBUG
PRInt32 mInvalidElementsCount;
#else
PRUint16 mInvalidElementsCount;
#endif // DEBUG
instead of:
PRInt32 mInvalidElementsCount;

So we can assert on negative values in debug builds but we don't waste memory otherwise.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I don't think saving a word per <form> is worth worrying about.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 462461 [details] [diff] [review]
Patch v3

I assume IsValid() is cheap?  If not, there's an extraneous call to it in SetValidityState that should go away.

I'm not sure I follow the last part of the AddElement changes.  Same for RemoveElement?

Bailing out of UpdateValidity is aNotify is false looks wrong to me.

That's about where I stopped for now...  I'd like the last two issues above sorted out before I spend more time on this.

Comment 14

[Mounir Lamouri (:mounir)]

(In reply to comment #13)
> Comment on attachment 462461 [details] [diff] [review]
> Patch v3
> 
> I assume IsValid() is cheap?  If not, there's an extraneous call to it in
> SetValidityState that should go away.

It is quite cheap: return mValidityBitField == 0;

> I'm not sure I follow the last part of the AddElement changes.  Same for
> RemoveElement?

You mean:
>+  // Submit controls now have a form, they need to be updated.
>+  if (aNotify && aChild->IsSubmitControl()) {
>+    nsIDocument* doc = aChild->GetCurrentDoc();
>+    if (doc) {
>+      doc->ContentStatesChanged(aChild, nsnull,
>+                                NS_EVENT_STATE_MOZ_SUBMITINVALID);
>+    }
>+  }

It's in case we add or remove a submit control to a form. When I add or remove a submit control to a form it needs to get the correct state (because there is |if mForm && mForm->GetValidity [...]| in IntrinsicState().

> Bailing out of UpdateValidity is aNotify is false looks wrong to me.

I don't get it... Are you talking about nsHTMLFormElement::UpdateValidity which don't call ContentStatesChanged() if !aNotify?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> When I add or remove a submit control to a form it needs to get the correct
> state

Yes, but for adding isn't that handled by either BindToTree (no state change needed) or the attribute change code?  How else can AddElement happen?

For removals, those can happen without the element changing in any way (due to the form being removed from the document), but it seems like notifying in that code might be better than in RemoveElement.

> I don't get it... Are you talking about nsHTMLFormElement::UpdateValidity
> which don't call ContentStatesChanged() if !aNotify?

Yes.  That doesn't look correct, since aNotify means "for the node involved", whereas this is skipping notifications on _all_ nodes.

Comment 16

[Mounir Lamouri (:mounir)]

(In reply to comment #15)
> > When I add or remove a submit control to a form it needs to get the correct
> > state
> 
> Yes, but for adding isn't that handled by either BindToTree (no state change
> needed) or the attribute change code?  How else can AddElement happen?

I will check and fix.

> For removals, those can happen without the element changing in any way (due to
> the form being removed from the document), but it seems like notifying in that
> code might be better than in RemoveElement.

I'm not sure I follow. What "that" refers to?

> > I don't get it... Are you talking about nsHTMLFormElement::UpdateValidity
> > which don't call ContentStatesChanged() if !aNotify?
> 
> Yes.  That doesn't look correct, since aNotify means "for the node involved",
> whereas this is skipping notifications on _all_ nodes.

Ouch :-/ But I can't know if these elements have to be updated when UpdateValidity is called, can I?

Comment 17

[Boris Zbarsky [:bzbarsky]]

> I'm not sure I follow. What "that" refers to?

The code that calls RemoveElement in the one case when it's nothing about the element itself that causes the removal.

> But I can't know if these elements have to be updated when UpdateValidity is
> called, can I?

I'm not sure I follow the question....

Comment 18

[Mounir Lamouri (:mounir)]

Attachment: Patch v3.1

This is fixing issues mentioned in previous comments.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 465649 [details] [diff] [review]
Patch v3.1

I'd probably name UpdateValidity something more descriptive, like InvalidElementCountChanged(aNowValid) or some such.

>+nsHTMLFormElement::UpdateValidity(PRBool aElementValidity)
>+{
>+  if (aElementValidity) {
>+    --mInvalidElementsCount;
>+  } else {
>+    ++mInvalidElementsCount;
>+  }
>+
>+  NS_ASSERTION(mInvalidElementsCount >= 0, "Something went seriously wrong!");
>+
>+  // The form validity has just changed if:
>+  // - there are no more invalid elements ;
>+  // - or there is one invalid elmement and an element just became invalid.
>+  // If we have invalid elements and we used to before as well, do nothing.
>+  if (mInvalidElementsCount &&
>+      (mInvalidElementsCount != 1 || aElementValidity)) {
>+    return;
>+  }

Nit (feel free to ignore): How about something like:

NS_PRECONDITION(aElementValidity || mInvalidElementsCount > 0);

PRBool wasInvalid = mInvalidElementsCount > 0;

if (aElementValidity) {
  --mInvalidElementsCount;
} else {
  ++mInvalidElementsCount;
}

if (wasInvalid == (mInvalidElementsCount > 0)) {
  return;
}


I had to do a double-take to understand what your code was doing.

sr=me

Comment 20

[Benjamin Smedberg]

According to blizzard, all the HTML5 form bits are opportunistic; we're not going to block on them.

Comment 21

[Mounir Lamouri (:mounir)]

This should really be a beta6 blocker otherwise we may to not have HTML5 Forms validation feature even if we are very close to...

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 465649 [details] [diff] [review]
Patch v3.1

>+++ b/content/events/public/nsIEventStateManager.h
>+// Content is a submit control and the form isn't valid.
>+#define NS_EVENT_STATE_MOZ_SUBMITINVALID (1 << 30)

This would need to be 1<<31 now, but event states are signed all over.  They need to be switched to unsigned as a preliminary.  I pointed this out in bug 457801 back in mid-August...

>+++ b/content/html/content/src/nsHTMLButtonElement.cpp
> nsHTMLButtonElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>+  PRInt32 states = 0;

So this would need to be PRUint32.

>     if (aNotify) {

"&& states", right?  Otherwise, why spend the time on it?
 
I'm not going to be able to sanely review the rest of this tonight at this point (big patch, no good tracking of what changed from my next review :( ); I'll see if I can sneak some time off tomorrow, but I'm technically off, so no guarantees.  :(

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 465649 [details] [diff] [review]
Patch v3.1

>+++ b/content/html/content/src/nsHTMLFormElement.cpp
>@@ -494,16 +496,25 @@ CollectOrphans(nsINode* aRemovalRoot, ns
>+            doc->ContentStatesChanged(node, nsnull,
>+                                      NS_EVENT_STATE_MOZ_SUBMITINVALID);

Update batch?

>@@ -1171,16 +1182,25 @@ nsHTMLFormElement::AddElement(nsGenericH
> +  // If the element is subject of constraint validaton and invalid, we need

"is subject to constraint validation and is invalid" (several grammar/spelling nits there).

>+    do_QueryInterface((nsGenericHTMLElement*)aChild);

Static cast, please?

>@@ -1238,16 +1258,25 @@ nsHTMLFormElement::RemoveElement(nsGener
>+  // If the element was subject of constraint validaton and invalid, we need
>+  // to update our internal counter.

Again, fix the grammar/spelling.

>+    do_QueryInterface((nsGenericHTMLElement*)aChild);

static_cast.

>+++ b/content/html/content/src/nsHTMLSelectElement.cpp
>+        doc->ContentStatesChanged(this, nsnull, NS_EVENT_STATE_VALID |
>+                                                NS_EVENT_STATE_INVALID);

Update batch?

>+++ b/content/html/content/src/nsIConstraintValidation.cpp
>+  // By default, all elements are subjects to constraint validation.

s/subjects/subject/

>+nsIConstraintValidation::SetValidityState(ValidityStateType aState,
>+                                         PRBool aValue)

Fix the indent?

r=bzbarsky with those nits and the stuff from the previous comment fixed.

Comment 24

[Mounir Lamouri (:mounir)]

Boris, would that be fine with you if I land this bug for beta6 then I fix bug 595036 (for nsEventState) ?

Comment 25

[Boris Zbarsky [:bzbarsky]]

> Boris, would that be fine with you if I land this bug for beta6 then I fix bug
> 595036 (for nsEventState) ?

You can't land this bug without changing the type passed around, because you can't use (1<<31) as a PRInt32 and have things work sanely, last I checked...

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

BTW shouldn't this pseudo-class be added to a spec somewhere?

Comment 27

[Mounir Lamouri (:mounir)]

I've added dev-doc-needed. And I will send a message to whatwg about that too.

Comment 28

[[e-mail]]

(In reply to comment #23)
>(From update of attachment 465649 [details] [diff] [review])
>>+    do_QueryInterface((nsGenericHTMLElement*)aChild);
>Static cast, please?
do_QueryObject(aChild) would probably work.

Comment 29

[Mounir Lamouri (:mounir)]

Unfortunately, it has been pushed before you post your recommandation, Neil.
Feel free to open a follow-up if you think that worth being changed to do_QueryObject.

Changeset:
http://hg.mozilla.org/mozilla-central/rev/e5fd1e37beb4

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It should NOT be do_QueryObject.  do_QueryObject should be used for getting to things that are not interfaces.  (We should fix do_QueryInterface so you don't need that cast.)

Comment 31

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/CSS/%3a-moz-submit-invalid

And mentioned on Firefox 4 for developers.