Closed Bug 610415 Opened 10 years ago Closed 10 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b9

People

(Reporter: mounir, Assigned: mounir)

References

Details

Attachments

(2 files, 1 obsolete file)

For the moment, :-moz-ui-invalid applies when the form has @novalidate is set which might be confusing.
Attached patch Patch v1 (obsolete) — Splinter Review
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #493579 - Flags: review?(jonas)
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.
Attachment #493579 - Flags: review?(jonas) → review+
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
Attachment #493579 - Flags: review+ → review-
(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?
> 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?
Attached patch Patch v2Splinter Review
Same patch without the aNotify check.

Thank you for your comment Boris.
Attachment #493579 - Attachment is obsolete: true
Attachment #497618 - Flags: review?(jonas)
Attachment #497618 - Flags: approval2.0?
Attached patch Inter diffSplinter Review
Whiteboard: [needs-review]
Whiteboard: [needs-review] → [needs-review][passed-try]
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.
Attachment #497618 - Flags: review?(jonas)
Attachment #497618 - Flags: review+
Attachment #497618 - Flags: approval2.0?
Attachment #497618 - Flags: approval2.0+
Whiteboard: [needs-review][passed-try] → [needs-landing][passed-try]
Pushed:
http://hg.mozilla.org/mozilla-central/rev/cd6aa4d8c7bd
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs-landing][passed-try]
Target Milestone: --- → mozilla2.0b9
Depends on: 622597
You need to log in before you can comment on or make changes to this bug.