605124 - Add :-moz-ui-invalid pseudo-class

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mounir Lamouri (:mounir)]

This pseudo-class should follow rules from bug 561636 comment 35.

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Add :-moz-ui-invalid pseudo-class

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Make :-moz-ui-invalid follow rules for :invalid

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 487631 [details] [diff] [review]
Part 1 - Add :-moz-ui-invalid pseudo-class

More patches like this, please!

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 487632 [details] [diff] [review]
Part 2 - Make :-moz-ui-invalid follow rules for :invalid

r=me

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - :-moz-ui-invalid doesn't apply if the element hasn't been modified

So, the value has to be modified to have :-moz-ui-invalid applying. Except if the element is suffering from a custom error. It's up to the authors to do whatever they want in that case.

Note that when |SetValueChanged| is called, I'm calling ContentStatesChanged regardless of aNotify. It is already the case for NS_EVENT_STATE_MOZ_PLACEHOLDER. To change that, we would have to change an interface and that would be very annoying given that interfaces freeze is behind us :(

Finally, I realize that radio and required is a mess. I should think about something better and raise a spec issue. I hope we could have that fixed for Firefox 4.

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: Part 4 - :-moz-ui-invalid applies if the user tried to submit the form in an invalid state

IOW, :-moz-ui-invalid applies on <input required> if the user tried to submit the form while invalid.

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Part 5 - :-moz-ui-invalid should not apply while typing if it was not applied on focus

I think that's the last part.
I will ask for a UI review in bug 609017.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 488065 [details] [diff] [review]
Part 3 - :-moz-ui-invalid doesn't apply if the element hasn't been modified

> I'm calling ContentStatesChanged regardless of aNotify.

That's ok, I think.  Not notifying if aNotify is false is largely a performance optimization at this point, esp. for content states.  Are there even ways that SetValueChanged can happen when we shouldn't notify, though?

>+++ b/content/html/content/src/nsHTMLInputElement.cpp
> nsHTMLInputElement::SetValueChanged(PRBool aValueChanged)
>+  if (previousValue != GET_BOOLBIT(mBitField, BF_VALUE_CHANGED)) {

  if (previousValue != aValueChanged) ?

Also, maybe call |previousValue| something like |valueChangedBefore|?

>+nsHTMLInputElement::SetCheckedChangedInternal(PRBool aCheckedChanged,
>+                                              PRBool aNotify)

How can this get called with aNotify false?  Looks like the only callers when GetCurrentDoc() is non-null are AddedToRadioGroup and DoneCreatingElement, which does this weird thing with calling DoSetChecked and telling it to set the checked as changed, and then resetting it back to not changed...  File a followup bug on just removing that gunk, adding some asserts to DoSetChecked, and DoSetCheckedChanged maybe, and removing this aNotify parameter?  Or just roll that in here?  I don't think it's needed.

>+  if (aNotify && previousValue != GetCheckedChanged()) {

Why not |previousValue == aCheckedChanged|?

>@@ -3296,18 +3320,34 @@ nsHTMLInputElement::IntrinsicState() con
>+      // NS_EVENT_STATE_MOZ_UI_INVALID always apply.

"applies".

>+      // For VALUE_MODE_DEFAULT case, value being modified has no sense.

  For the VALUE_MODE_DEFAULT case, there is no concept of the value being modified,
  since value == defaultValue in that case.

>+           GET_BOOLBIT(mBitField, BF_VALUE_CHANGED))) {

Add an inline GetValueChanged() helper, please?

>+++ b/content/html/content/src/nsHTMLTextAreaElement.cpp
>@@ -1001,18 +1006,27 @@ nsHTMLTextAreaElement::IntrinsicState() 
>+      // NS_EVENT_STATE_MOZ_UI_INVALID always apply if the element suffers from

"applies".

I didn't really review the tests.  r=me with the above nits.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 488172 [details] [diff] [review]
Part 4 - :-moz-ui-invalid applies if the user tried to submit the form in an invalid state

>+++ b/content/html/content/src/nsHTMLFormElement.cpp
>+        // When a form control loose it's form owner,

 "loses its"

>+        // In addition, submit control shouldn't have

"controls"

>@@ -1703,20 +1712,49 @@ nsHTMLFormElement::CheckValidFormSubmiss

>+      // For the first invalid submission, we should update elements states.

I'd say "element states".


>+++ b/content/html/content/src/nsHTMLInputElement.cpp
>+          (valueMode == VALUE_MODE_DEFAULT ||

You don't need the open paren before "valueMode" here, nor the corresponding close paren, imo.  The code would be clearer without them.

>+++ b/content/html/content/src/nsHTMLTextAreaElement.cpp
>@@ -1013,17 +1014,20 @@ nsHTMLTextAreaElement::IntrinsicState() 
>+      if ((mForm && mForm->HasEverTriedInvalidSubmit()) ||
>+          (mValueChanged || GetValidityState(VALIDITY_STATE_CUSTOM_ERROR))) {

Extra parens here too.

r=me with those nits fixed.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 488185 [details] [diff] [review]
Part 5 - :-moz-ui-invalid should not apply while typing if it was not applied on focus

> +     * - the element can't has its value changed.

"the element has had its value changed"?

> +    switch (GetValueMode()) {

Are there no other modes?

If so, can you add a |default: NS_NOTREACHED(.....)| to the switch?

> +  bool mCanShowInvalidUI;

Why not pack it in with the existing packed boools?

r=me with those nits.

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to comment #8)
> Comment on attachment 488065 [details] [diff] [review]
> Part 3 - :-moz-ui-invalid doesn't apply if the element hasn't been modified
> 
> > I'm calling ContentStatesChanged regardless of aNotify.
> 
> That's ok, I think.  Not notifying if aNotify is false is largely a performance
> optimization at this point, esp. for content states.  Are there even ways that
> SetValueChanged can happen when we shouldn't notify, though?

With bug 609417 fixed, I don't think there are.

> >+nsHTMLInputElement::SetCheckedChangedInternal(PRBool aCheckedChanged,
> >+                                              PRBool aNotify)
> 
> How can this get called with aNotify false?  Looks like the only callers when
> GetCurrentDoc() is non-null are AddedToRadioGroup and DoneCreatingElement,
> which does this weird thing with calling DoSetChecked and telling it to set the
> checked as changed, and then resetting it back to not changed...  File a
> followup bug on just removing that gunk, adding some asserts to DoSetChecked,
> and DoSetCheckedChanged maybe, and removing this aNotify parameter?  Or just
> roll that in here?  I don't think it's needed.

bug 610427

> Add an inline GetValueChanged() helper, please?

bug 610436

Everything else has been fixed locally.

Comment 12

[Benjamin Smedberg]

a=me for the patch series

Comment 13

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/80c5a5c971b9
http://hg.mozilla.org/mozilla-central/rev/f88a334e605e
http://hg.mozilla.org/mozilla-central/rev/15b5a12e17ac
http://hg.mozilla.org/mozilla-central/rev/5be85a60b133
http://hg.mozilla.org/mozilla-central/rev/253651b1e9c2

Comment 14

[Janet Swisher]

Assigning to me for doc.

Comment 15

[Janet Swisher]

Documented at: https://developer.mozilla.org/en/CSS/:-moz-ui-invalid