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

RESOLVED FIXED in mozilla2.0b9

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla2.0b9
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

7 years ago
For the moment, :-moz-ui-invalid applies when the form has @novalidate is set which might be confusing.
(Assignee)

Comment 1

7 years ago
Created attachment 493579 [details] [diff] [review]
Patch v1
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-
(Assignee)

Comment 4

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

Comment 6

7 years ago
Created attachment 497618 [details] [diff] [review]
Patch v2

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?
(Assignee)

Comment 7

7 years ago
Created attachment 497619 [details] [diff] [review]
Inter diff
(Assignee)

Updated

7 years ago
Whiteboard: [needs-review]
(Assignee)

Updated

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

Updated

7 years ago
Whiteboard: [needs-review][passed-try] → [needs-landing][passed-try]
(Assignee)

Comment 9

7 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/cd6aa4d8c7bd
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs-landing][passed-try]
Target Milestone: --- → mozilla2.0b9
(Assignee)

Updated

7 years ago
Depends on: 622597
You need to log in before you can comment on or make changes to this bug.