610415 - Make :-moz-ui-invalid and :-moz-ui-valid not applying when the form element has @novalidate set

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

For the moment, :-moz-ui-invalid applies when the form has @novalidate is set which might be confusing.

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 2

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

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

>diff --git a/content/html/content/src/nsHTMLInputElement.h b/content/html/content/src/nsHTMLInputElement.h
...
>   bool ShouldShowInvalidUI() const {
...
>+    if (mForm) {
>+      if (mForm->HasEverTriedInvalidSubmit()) {
>+        return true;
>+      }
>+      if (mForm->HasAttr(kNameSpaceID_None, nsGkAtoms::novalidate)) {
>+        return false;
>+      }

Shouldn't these checks be done the other way around. Consider the case when someone sets @novalidate after a submission has been attempted. At that point we don't want to show the invalid UI, right?


>   bool ShouldShowValidUI() const {
>-    if (mForm && mForm->HasEverTriedInvalidSubmit()) {
>-      return true;
>+    if (mForm) {
>+      if (mForm->HasEverTriedInvalidSubmit()) {
>+        return true;
>+      }
>+      if (mForm->HasAttr(kNameSpaceID_None, nsGkAtoms::novalidate)) {
>+        return false;
>+      }

Same here. And in a bunch of other places in the patch.

r=me with that fixed.

Comment 3

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

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

>+nsHTMLFormElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>+                                const nsAString* aValue, PRBool aNotify)
>+{
>+  if (aName == nsGkAtoms::novalidate && aNameSpaceID == kNameSpaceID_None &&
>+      aNotify) {
>+    // Update all form elements states because they might be [no longer]
>+    // affected by :-moz-ui-valid or :-moz-ui-invalid.
>+    nsIDocument* doc = GetCurrentDoc();
>+    if (doc) {
>+      MOZ_AUTO_DOC_UPDATE(doc, UPDATE_CONTENT_STATE, PR_TRUE);
>+
>+      for (PRUint32 i = 0, length = mControls->mElements.Length();
>+           i < length; ++i) {
>+        doc->ContentStatesChanged(mControls->mElements[i], nsnull,
>+                                  NS_EVENT_STATE_MOZ_UI_VALID |
>+                                  NS_EVENT_STATE_MOZ_UI_INVALID);
>+      }
...

Hmm.. though don't we want to do this even if aNotify is false, now that we generally only let aNotify affect notifications for the element on which SetAttr is called.

r- for now

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to comment #3)
> Comment on attachment 493579 [details] [diff] [review]
> Patch v1
> 
> >+nsHTMLFormElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
> >+                                const nsAString* aValue, PRBool aNotify)
> >+{
> >+  if (aName == nsGkAtoms::novalidate && aNameSpaceID == kNameSpaceID_None &&
> >+      aNotify) {
> >+    // Update all form elements states because they might be [no longer]
> >+    // affected by :-moz-ui-valid or :-moz-ui-invalid.
> >+    nsIDocument* doc = GetCurrentDoc();
> >+    if (doc) {
> >+      MOZ_AUTO_DOC_UPDATE(doc, UPDATE_CONTENT_STATE, PR_TRUE);
> >+
> >+      for (PRUint32 i = 0, length = mControls->mElements.Length();
> >+           i < length; ++i) {
> >+        doc->ContentStatesChanged(mControls->mElements[i], nsnull,
> >+                                  NS_EVENT_STATE_MOZ_UI_VALID |
> >+                                  NS_EVENT_STATE_MOZ_UI_INVALID);
> >+      }
> ...
> 
> Hmm.. though don't we want to do this even if aNotify is false, now that we
> generally only let aNotify affect notifications for the element on which
> SetAttr is called.
> 
> r- for now

I was assuming that aNotify=PR_FALSE here means we are parsing. Can't we assume that?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I was assuming that aNotify=PR_FALSE here means we are parsing

That's probably a reasonable assumption... but why do you need to make it?  Seems like if we're just parsing the <form> tag then mElements.Length() will be 0 (can check that explicitly before starting the update.  If it's not 0, we need to notify them, right?

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Same patch without the aNotify check.

Thank you for your comment Boris.

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Inter diff

Comment 8

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

Comment on attachment 497618 [details] [diff] [review]
Patch v2

>+    if (mForm) {
>+      if (mForm->HasEverTriedInvalidSubmit()) {
>+        return true;
>+      }
>+      if (mForm->HasAttr(kNameSpaceID_None, nsGkAtoms::novalidate)) {
>+        return false;
>+      }
>+    }

These still need to be the other way around. Same in a lot of places in the patch.

r=me with all of these fixed.

Comment 9

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/cd6aa4d8c7bd