Closed Bug 605124 Opened 14 years ago Closed 14 years ago

Add :-moz-ui-invalid pseudo-class

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b8

People

(Reporter: mounir, Assigned: mounir)

References

Details

(Keywords: dev-doc-complete, html5)

Attachments

(5 files)

This pseudo-class should follow rules from bug 561636 comment 35.
Severity: blocker → normal
OS: Linux → All
Depends on: 609016
Blocks: 609017
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #487631 - Flags: review?(bzbarsky)
Attachment #487631 - Flags: review?(bzbarsky) → review?(jonas)
Attachment #487632 - Flags: review?(bzbarsky) → review?(jonas)
Attachment #487631 - Flags: review?(jonas) → review?(bzbarsky)
Attachment #487632 - Flags: review?(jonas) → review?(bzbarsky)
Comment on attachment 487631 [details] [diff] [review]
Part 1 - Add :-moz-ui-invalid pseudo-class

More patches like this, please!
Attachment #487631 - Flags: review?(bzbarsky) → review+
Comment on attachment 487632 [details] [diff] [review]
Part 2 - Make :-moz-ui-invalid follow rules for :invalid

r=me
Attachment #487632 - Flags: review?(bzbarsky) → review+
Depends on: 609417
Depends on: 609162
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.
Attachment #488065 - Flags: review?(bzbarsky)
IOW, :-moz-ui-invalid applies on <input required> if the user tried to submit the form while invalid.
Attachment #488172 - Flags: review?(bzbarsky)
I think that's the last part.
I will ask for a UI review in bug 609017.
Attachment #488185 - Flags: review?(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.
Attachment #488065 - Flags: review?(bzbarsky) → review+
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.
Attachment #488172 - Flags: review?(bzbarsky) → review+
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.
Attachment #488185 - Flags: review?(bzbarsky) → review+
Attachment #487631 - Flags: approval2.0?
Attachment #487632 - Flags: approval2.0?
Attachment #488065 - Flags: approval2.0?
Attachment #488172 - Flags: approval2.0?
Attachment #488185 - Flags: approval2.0?
Depends on: 610415
(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.
a=me for the patch series
Attachment #487631 - Flags: approval2.0? → approval2.0+
Depends on: 610427
Attachment #487632 - Flags: approval2.0?
Attachment #488065 - Flags: approval2.0?
Attachment #488172 - Flags: approval2.0?
Attachment #488185 - Flags: approval2.0?
Depends on: 612730
Depends on: 613249
Assigning to me for doc.
Assignee: mounir.lamouri → jswisher
Assignee: jswisher → mounir.lamouri
Depends on: 619278
Depends on: 622558
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: