Closed Bug 558788 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b5
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mounir, Assigned: mounir)

References

()

Details

(Keywords: css3, dev-doc-complete, html5)

Attachments

(2 files, 10 obsolete files)

When the constraint validation API will be implemented (bug 345624), we need to use :invalid and :valid pseudo-classes according to HTML5/CSS3 specifications.
No longer blocks: 345624
Depends on: 345624
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....
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.
Summary: Implement CSS3 pseudo-classes :valid and :invalid → Valid/Invalid form elements should change there state to use CSS3 pseudo-classes :valid and :invalid
Summary: Valid/Invalid form elements should change there state to use CSS3 pseudo-classes :valid and :invalid → Valid/Invalid form elements should change their state to use CSS3 pseudo-classes :valid and :invalid
Blocks: 566045
Attached patch WIP Patch (obsolete) — Splinter Review
This is WIP and not really working:
- it works for <select>, <fieldset> and <button>
- but it breaks <input> and <textarea> constraint validation
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attached patch WIP Patch (obsolete) — Splinter Review
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.
Attachment #451982 - Attachment is obsolete: true
Attachment #454260 - Flags: feedback?(ehsan)
Depends on: 345822, 345512, 555559, 344615
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?
Attachment #454260 - Flags: feedback?(ehsan) → feedback+
Attached patch WIP Patch (obsolete) — Splinter Review
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...
Attachment #454260 - Attachment is obsolete: true
Attachment #457604 - Flags: feedback?(jonas)
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #457604 - Attachment is obsolete: true
Attachment #457801 - Flags: superreview?(jst)
Attachment #457801 - Flags: review?(jonas)
Attachment #457604 - Flags: feedback?(jonas)
CCing David and Marco for a11y.
Attached patch Patch v1.1 (obsolete) — Splinter Review
Small changes before attaching a patch is definitely the best way to attach a patch that does not compile :)
Attachment #457801 - Attachment is obsolete: true
Attachment #457827 - Flags: superreview?(jst)
Attachment #457827 - Flags: review?(jonas)
Attachment #457801 - Flags: superreview?(jst)
Attachment #457801 - Flags: review?(jonas)
Blocks: 561635
Attached patch Patch v1.2 (obsolete) — Splinter Review
Sorry for the bugmail spam...
Attachment #457827 - Attachment is obsolete: true
Attachment #457853 - Flags: superreview?(jst)
Attachment #457853 - Flags: review?(jonas)
Attachment #457827 - Flags: superreview?(jst)
Attachment #457827 - Flags: review?(jonas)
(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.
blocking2.0: --- → betaN+
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.
Attachment #457853 - Flags: superreview?(jst) → superreview+
Blocks: 344614
Attached patch Patch v1.3 (obsolete) — Splinter Review
sr=jst

Updated to apply against current tip.
Applied jst's comments.
Attachment #457853 - Attachment is obsolete: true
Attachment #461164 - Flags: review?(jonas)
Attachment #457853 - Flags: review?(jonas)
Adding bug 561640 in the dependency list because this patch is modifying the tests created by the patch in bug 561640.
Depends on: 561640
Attached patch Patch v1.4 (obsolete) — Splinter Review
Fixes following a discussion with Boris.
Attachment #461164 - Attachment is obsolete: true
Attachment #462103 - Flags: review?(jonas)
Attachment #461164 - Flags: review?(jonas)
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...
Attachment #462103 - Flags: review?(dbaron)
Attached patch Patch v1.5 (obsolete) — Splinter Review
Small changes: I kept IsTooLong/IsValueMissing/... methods because I was needing them for another patch (so better to keep that than re-adding them...).
Attachment #462103 - Attachment is obsolete: true
Attachment #462409 - Flags: review?(jonas)
Attachment #462103 - Flags: review?(jonas)
Attachment #462103 - Flags: review?(dbaron)
Attachment #462409 - Flags: review?(dbaron)
Mounir: Since you asked both me and dbaron for review, are there any particular parts you want me vs. david to look at?
(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 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
Attachment #462409 - Flags: review?(jonas) → review+
Attached patch Patch v1.6 (obsolete) — Splinter Review
Attachment #462409 - Attachment is obsolete: true
Attachment #464817 - Flags: review?(dbaron)
Attachment #462409 - Flags: review?(dbaron)
(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?
Attached patch Patch v1.7Splinter Review
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.
Attachment #464817 - Attachment is obsolete: true
Attachment #464846 - Flags: review?(dbaron)
Attachment #464817 - Flags: review?(dbaron)
> > 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.
Attached patch More testsSplinter Review
Some more tests just for you Jonas :)
Attachment #464846 - Attachment is obsolete: true
Attachment #465610 - Flags: review?(jonas)
Attachment #464846 - Flags: review?(dbaron)
Comment on attachment 464846 [details] [diff] [review]
Patch v1.7

Gasp, this patch isn't obsolete at all. Bad habits...
Attachment #464846 - Attachment is obsolete: false
Attachment #464846 - Flags: review?(dbaron)
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
Attachment #464846 - Flags: review?(dbaron) → review+
Landed with the requested change:
http://hg.mozilla.org/mozilla-central/rev/751da00c7b53
http://hg.mozilla.org/mozilla-central/rev/b6e9c69a2b37
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
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?
(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.
Depends on: 589686
(In reply to comment #8)
> CCing David and Marco for a11y.

We have bug 555728 for this I think.
You need to log in before you can comment on or make changes to this bug.