558788 - Valid/Invalid form elements should change their state to use CSS3 pseudo-classes :valid and :invalid

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mounir Lamouri (:mounir)]

When the constraint validation API will be implemented (bug 345624), we need to use :invalid and :valid pseudo-classes according to HTML5/CSS3 specifications.

Comment 1

[Boris Zbarsky [:bzbarsky]]

By "implement" I assume you mean just change the HTML nodes in question to set the right state bits?  The rest of it all works fine, and :valid/:invalid work with existing XForms stuff and such....

Comment 2

[Mounir Lamouri (:mounir)]

Indeed, the title wasn't really explicit. Actually, the real goal/problem of this bug is going to be the "real time" aspect. I mean, at the moment, the validity of an element is checked when requested but these pseudo-classes should be changed probably more often.
While the user is typing or when he/she leave the field, that are the two options I think. Actually, Webkit and Opera are changing the pseudo-class while the user is typing so I suppose we should do the same but I will look at the specifications to know what is really requested.

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: WIP Patch

This is WIP and not really working:
- it works for <select>, <fieldset> and <button>
- but it breaks <input> and <textarea> constraint validation

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: WIP Patch

This is now working but the patch may need some clean-up and tests.

Ehsan, could I have your feedback about nsTextEditorState changes and the tests ? I think some other (more dynamic) tests could be added using mochitests. If it's possible to know which CSS pseudo-classes apply to an element in JS, it should be easy (I think it is but really not sure). Otherwise, using "screenshots" in mochitests.

Comment 5

[(no longer active)]

Comment on attachment 454260 [details] [diff] [review]
WIP Patch

The nsTextEditorState changes look good, but having to add a new method to nsITextControlElement makes me kind of sad.  The existing tests also look very good.  I don't know what other tests we're going to need though.

TO answer your question, let's say you have an input element and you want to test which pseudo element selectors have been applied to it.  You can have the following in a style element inside the page:

input { margin-top: 0px; }
input:valid { margin-top: 1px; }
input:invalid { margin-top: 2px; }

And then get the computed style for margin-top and infer which of those rules have been applied to it using window.getComputedStyle.

If several of these can be applied simultaneously, you can use two different properties (such as margin-left and margin-right) for :valid and :invalid, for example.

Does that answer your question?

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: WIP Patch

f=ehsan

This is working for every elements and types as far as tested.
I would like to add tests simulating user inputs and some other tests for type changes.
I also think the code needs some clean-up. Jonas, your feedback about that would be appreciated :)

Sorry, the patch is big and modifying files that are not in the tree...

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

I think the patch is now ready with a (I hope) strong test suite.

I'm not really satisfied with some nsHTMLInputElement changes:
- The radio group visitor is calling |OnValueChanged| which is not really meant for that but I've the feeling adding a method to nsIRadioControlElement just for that is too much.
- I've the feeling there is a way to not have |OnValueChanged| called in two places in nsTextEditorState. It has to be called in |EditAction| but I've the feeling the call in |SetValue| can be removed. However, I was not able to have something working without the call in |SetValue|. Let me know if you think it worth to be changed (at least investigate more) for this patch or in a follow-up.

Comment 8

[Mounir Lamouri (:mounir)]

CCing David and Marco for a11y.

Comment 9

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

Small changes before attaching a patch is definitely the best way to attach a patch that does not compile :)

Comment 10

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.2

Sorry for the bugmail spam...

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to comment #7)
> - The radio group visitor is calling |OnValueChanged| which is not really meant
> for that but I've the feeling adding a method to nsIRadioControlElement just
> for that is too much.

As jst pointed on IRC, bug 578570 has actually removed nsIRadioControlElement.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 457853 [details] [diff] [review]
Patch v1.2

- In content/html/content/src/nsConstraintValidation.h:

+  void   SetValidityState(ValidityStateType mState, PRBool mValue) {
+           mValue ? mValidityBitField |= mState : mValidityBitField &= ~mState;
+         }

I think this would be more readable if written as an if statement instead.

- In nsHTMLInputElement::AfterSetAttr():

+    if (aName == nsGkAtoms::disabled && aNotify) {
+      nsIDocument* doc = GetCurrentDoc();
+      if (doc) {
+        doc->ContentStatesChanged(this, nsnull,
+                                  NS_EVENT_STATE_VALID |
+                                  NS_EVENT_STATE_INVALID);
+      }
+    }
+
+    if (aName == nsGkAtoms::required || aName == nsGkAtoms::disabled ||
+        aName == nsGkAtoms::readonly) {
+      UpdateValueMissingValidityState();
+
+      if (aNotify) {
         nsIDocument* doc = GetCurrentDoc();
         if (doc) {
           doc->ContentStatesChanged(this, nsnull,
                                     NS_EVENT_STATE_REQUIRED |
-                                    NS_EVENT_STATE_OPTIONAL);
+                                    NS_EVENT_STATE_OPTIONAL |
+                                    NS_EVENT_STATE_VALID |
+                                    NS_EVENT_STATE_INVALID);

If "disabled" is changed, we'll end up calling ContentStatesChanged() twice for this element, no? Seems it'd be more efficient to only do one call here, or am I missing something here? Also, the chain of if (aName == nsGkAtoms::...) could maybe be if () {} else if () {} else if () {}... to avoid having to compare the atom pointers more than necessary. Might be less code too if you just set a state variable in the if checks, and at the end have one place that calls ContentStatesChanged() on the document, instead of having several calls to that method in this code, and maybe move the aNotify check further out too to avoid atom compares if we're not notifying, if possible?

sr=jst with that, and assuming Jonas is ok with this. And this still needs to be updated to deal with the removal of nsIRadioControlElement.

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.3

sr=jst

Updated to apply against current tip.
Applied jst's comments.

Comment 14

[Mounir Lamouri (:mounir)]

Adding bug 561640 in the dependency list because this patch is modifying the tests created by the patch in bug 561640.

Comment 15

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.4

Fixes following a discussion with Boris.

Comment 16

[Mounir Lamouri (:mounir)]

Comment on attachment 462103 [details] [diff] [review]
Patch v1.4

David, could you review the states change? I'm particularly concerned about ContentStatesChanged calls without aNotify. Boris told me I should prevent that but I wasn't able to do get a aNotify in every situations...

Comment 17

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.5

Small changes: I kept IsTooLong/IsValueMissing/... methods because I was needing them for another patch (so better to keep that than re-adding them...).

Comment 18

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

Mounir: Since you asked both me and dbaron for review, are there any particular parts you want me vs. david to look at?

Comment 19

[Mounir Lamouri (:mounir)]

(In reply to comment #18)
> Mounir: Since you asked both me and dbaron for review, are there any particular
> parts you want me vs. david to look at?

I would like to have David review the states management (usage of |ContentStatesChange|).
So, could you review the entire patch without worrying too much about usage of ContentStatesChange. Even if I think having two pair of eyes looking at that wouldn't hurt ;)

Comment 20

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

Comment on attachment 462409 [details] [diff] [review]
Patch v1.5

>@@ -488,51 +484,57 @@ nsHTMLInputElement::AfterSetAttr(PRInt32
>         // We just got switched to be an image input; we should see
>         // whether we have an image to load;
>         nsAutoString src;
>         if (GetAttr(kNameSpaceID_None, nsGkAtoms::src, src)) {
>           LoadImage(src, PR_FALSE, aNotify);
>         }
>       }
> 
>-      if (aNotify && document) {
>-        // Changing type affects the applicability of some states.  Just notify
>-        // on them all now, just in case.  Note that we can't rely on the
>-        // notifications LoadImage or CancelImageRequests might have sent,
>-        // because those didn't include all the possibly-changed states in the
>-        // mask.  We have to do this here because we just updated mType, so the
>-        // code in nsGenericElement::SetAttrAndNotify didn't see the new
>-        // states.
>-        document->ContentStatesChanged(this, nsnull,
>-                                       NS_EVENT_STATE_CHECKED |
>-                                       NS_EVENT_STATE_DEFAULT |
>-                                       NS_EVENT_STATE_BROKEN |
>-                                       NS_EVENT_STATE_USERDISABLED |
>-                                       NS_EVENT_STATE_SUPPRESSED |
>-                                       NS_EVENT_STATE_LOADING |
>-                                       NS_EVENT_STATE_INDETERMINATE |
>-                                       NS_EVENT_STATE_MOZ_READONLY |
>-                                       NS_EVENT_STATE_MOZ_READWRITE |
>-                                       NS_EVENT_STATE_REQUIRED |
>-                                       NS_EVENT_STATE_OPTIONAL);
>+      // Changing type affects the applicability of some states.  Just notify
>+      // on them all now, just in case.  Note that we can't rely on the
>+      // notifications LoadImage or CancelImageRequests might have sent, because
>+      // those didn't include all the possibly-changed states in the mask. We
>+      // have to do this here because we just updated mType, so the code in
>+      // nsGenericElement::SetAttrAndNotify didn't see the new states.
>+      states |= NS_EVENT_STATE_CHECKED | NS_EVENT_STATE_DEFAULT |
>+                NS_EVENT_STATE_BROKEN | NS_EVENT_STATE_USERDISABLED |
>+                NS_EVENT_STATE_SUPPRESSED | NS_EVENT_STATE_LOADING |
>+                NS_EVENT_STATE_MOZ_READONLY | NS_EVENT_STATE_MOZ_READWRITE |
>+                NS_EVENT_STATE_REQUIRED | NS_EVENT_STATE_OPTIONAL |
>+                NS_EVENT_STATE_VALID | NS_EVENT_STATE_INVALID |
>+                NS_EVENT_STATE_INDETERMINATE;

IMHO it's actually easier to read this with one value per line. It also makes patches that add or remove values easier to read. And it prevents someone from having to rewrap the whole list if the NS_EVENT_STATE_DEFAULT value is removed :)

>+class nsRadioUpdateValueMissingVisitor: public nsRadioVisitor {

space before ':'

>+public:
>+  nsRadioUpdateValueMissingVisitor(PRBool aNotify)
>+    : nsRadioVisitor()
>+    , mNotify(aNotify)
>+    { }
>+
>+  virtual ~nsRadioUpdateValueMissingVisitor() { };
>+
>+  NS_IMETHOD Visit(nsIFormControl* aRadio, PRBool* aStop)
>+  {
>+    /**
>+     * The simplest way to update the value missing validity state is to do a
>+     * global update of the validity state by simulationg a value change.
>+     * OnValueChanged() is declared into nsITextControlElement. That may sound
>+     * to be a weird way to update the validity states for radio controls but
>+     * they are also implementing nsITextControlElement interface.
>+     */
>+    nsCOMPtr<nsITextControlElement> textCtl(do_QueryInterface(aRadio));
>+    NS_ASSERTION(textCtl, "Visit() passed a null or non-radio pointer");
>+    textCtl->OnValueChanged(mNotify);
>+    return NS_OK;
>+  }

Please document how OnValueChanged is called on radio controls. I would not have guessed that they were called in this way so please document in nsITextControlElement.h

I.e. something like "This is called on radio buttons whenever any radio button in the same radio group is checked or unchecked" or some such.

>+NS_GetRadioUpdateValueMissingVisitor(nsIRadioVisitor** aVisitor, PRBool aNotify)
>+{
>+  *aVisitor = new nsRadioUpdateValueMissingVisitor(aNotify);
>+  NS_ADDREF(*aVisitor);
>+  return NS_OK;
>+}

I know we use this code pattern elsewhere, but there's really no reason to. Simply do |new nsRadioUpdateValueMissingVisitor(...)| whereever you need to use this class.

Btw, another way to check for selector matching is to use node.querySelectorAll. You should probably do that in addition to the reftests you have.

r=me

Comment 21

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.6

Comment 22

[Mounir Lamouri (:mounir)]

(In reply to comment #20)
> >+public:
> >+  nsRadioUpdateValueMissingVisitor(PRBool aNotify)
> >+    : nsRadioVisitor()
> >+    , mNotify(aNotify)
> >+    { }
> >+
> >+  virtual ~nsRadioUpdateValueMissingVisitor() { };
> >+
> >+  NS_IMETHOD Visit(nsIFormControl* aRadio, PRBool* aStop)
> >+  {
> >+    /**
> >+     * The simplest way to update the value missing validity state is to do a
> >+     * global update of the validity state by simulationg a value change.
> >+     * OnValueChanged() is declared into nsITextControlElement. That may sound
> >+     * to be a weird way to update the validity states for radio controls but
> >+     * they are also implementing nsITextControlElement interface.
> >+     */
> >+    nsCOMPtr<nsITextControlElement> textCtl(do_QueryInterface(aRadio));
> >+    NS_ASSERTION(textCtl, "Visit() passed a null or non-radio pointer");
> >+    textCtl->OnValueChanged(mNotify);
> >+    return NS_OK;
> >+  }
> 
> Please document how OnValueChanged is called on radio controls. I would not
> have guessed that they were called in this way so please document in
> nsITextControlElement.h

I've added a comment in the radio visitor code. Adding one about radio controls in nsITextControlElement.h sounded weird.

> >+NS_GetRadioUpdateValueMissingVisitor(nsIRadioVisitor** aVisitor, PRBool aNotify)
> >+{
> >+  *aVisitor = new nsRadioUpdateValueMissingVisitor(aNotify);
> >+  NS_ADDREF(*aVisitor);
> >+  return NS_OK;
> >+}
> 
> I know we use this code pattern elsewhere, but there's really no reason to.
> Simply do |new nsRadioUpdateValueMissingVisitor(...)| whereever you need to use
> this class.

I completely agree. Changed.

> Btw, another way to check for selector matching is to use
> node.querySelectorAll. You should probably do that in addition to the reftests
> you have.

I've added some checks in mochitests in addition of the reftests. They are not using querySelector (didn't know that) but I do not think it worths changing them to use querySelector as long as they are doing more or less the same checks.
Otherwise you want me to check that querySelector is working correctly with :valid and :invalid?

Comment 23

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.7

I had to recreate a NS_GetRadioUpdateValueMissingVisitor method because radio visitors are declared and defined at the end of nsHTMLInputElement.cpp and it would have been weird to move only my new class at the top of the file to be able to use it directly (NS_GetRadio* are kind of factories so I don't need to know the class when calling them).

I've open bug 586298 to clean that mess because we have factories for no good reasons.

Comment 24

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

> > Btw, another way to check for selector matching is to use
> > node.querySelectorAll. You should probably do that in addition to the reftests
> > you have.
> 
> I've added some checks in mochitests in addition of the reftests. They are not
> using querySelector (didn't know that) but I do not think it worths changing
> them to use querySelector as long as they are doing more or less the same
> checks.
> Otherwise you want me to check that querySelector is working correctly with
> :valid and :invalid?

Yes, I think we want at least some basic tests that querySelector(All) works with :valid and :invalid. No need to change any existing tests, but would be great if you could add some simple tests for this too.

Comment 25

[Mounir Lamouri (:mounir)]

Attachment: More tests

Some more tests just for you Jonas :)

Comment 26

[Mounir Lamouri (:mounir)]

Comment on attachment 464846 [details] [diff] [review]
Patch v1.7

Gasp, this patch isn't obsolete at all. Bad habits...

Comment 27

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

Comment on attachment 464846 [details] [diff] [review]
Patch v1.7

I think all the SetCustomValidity methods (quite a few of them) should call ContentStatesChanged *after* calling nsConstraintValidation::SetCustomValidity.  Otherwise they'd call ContentStatesChanged before the state actually changes, which is incorrect.

r=dbaron on the state management parts with that fixed

Comment 28

[Mounir Lamouri (:mounir)]

Landed with the requested change:
http://hg.mozilla.org/mozilla-central/rev/751da00c7b53
http://hg.mozilla.org/mozilla-central/rev/b6e9c69a2b37

Comment 29

[Boris Zbarsky [:bzbarsky]]

In that first patch:

1)  There are several places where we call ContentStatesChanged without an update
    batch.  Why?
2)  There are several places where we start/end and update batch but never call
    ContentStatesChanged.  Again, why?  Why not move that auto batch into the
    innermost scope where we actually do the update?

Comment 30

[Mounir Lamouri (:mounir)]

(In reply to comment #29)
> In that first patch:
> 
> 1)  There are several places where we call ContentStatesChanged without an
> update batch.  Why?

You told me it was needed for -moz-submit-invalid patch and I didn't know that when I wrote this one. I guess I didn't check correctly when I fixed this patch after your explanations :(

> 2)  There are several places where we start/end and update batch but never call
>     ContentStatesChanged.  Again, why?  Why not move that auto batch into the
>     innermost scope where we actually do the update?

Hmm, I thought UpdateEditableState needed to be in the update batch scope. Don't ask me why...

I'm going to open a follow-up bug to fix these issues.

Comment 31

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> CCing David and Marco for a11y.

We have bug 555728 for this I think.

Comment 32

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/CSS/:valid
https://developer.mozilla.org/en/CSS/:invalid

Also added mentions to:

http://developer.mozilla.org/en/HTML/Element/input

And to the Firefox 4 for developers page.