Add :-moz-ui-invalid pseudo-class

RESOLVED FIXED in mozilla2.0b8

Status

()

RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

({dev-doc-complete, html5})

Trunk
mozilla2.0b8
dev-doc-complete, html5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Assignee)

Description

9 years ago
This pseudo-class should follow rules from bug 561636 comment 35.
(Assignee)

Updated

9 years ago
Severity: blocker → normal
(Assignee)

Updated

9 years ago
OS: Linux → All
(Assignee)

Updated

9 years ago
Depends on: 609016
(Assignee)

Updated

9 years ago
Blocks: 609017
(Assignee)

Comment 1

9 years ago
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #487631 - Flags: review?(bzbarsky)
(Assignee)

Updated

9 years ago
Attachment #487631 - Flags: review?(bzbarsky) → review?(jonas)
(Assignee)

Updated

9 years ago
Attachment #487632 - Flags: review?(bzbarsky) → review?(jonas)
(Assignee)

Updated

9 years ago
Attachment #487631 - Flags: review?(jonas) → review?(bzbarsky)
(Assignee)

Updated

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

Updated

9 years ago
Depends on: 609417
(Assignee)

Updated

9 years ago
Depends on: 609162
(Assignee)

Comment 5

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

Comment 6

9 years ago
IOW, :-moz-ui-invalid applies on <input required> if the user tried to submit the form while invalid.
Attachment #488172 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

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

Updated

9 years ago
Attachment #487631 - Flags: approval2.0?
(Assignee)

Updated

9 years ago
Attachment #487632 - Flags: approval2.0?
(Assignee)

Updated

9 years ago
Attachment #488065 - Flags: approval2.0?
(Assignee)

Updated

9 years ago
Attachment #488172 - Flags: approval2.0?
(Assignee)

Updated

9 years ago
Attachment #488185 - Flags: approval2.0?
(Assignee)

Updated

9 years ago
Depends on: 610415
(Assignee)

Comment 11

9 years ago
(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

Updated

9 years ago
Attachment #487631 - Flags: approval2.0? → approval2.0+
(Assignee)

Updated

9 years ago
Depends on: 610427
(Assignee)

Updated

9 years ago
Attachment #487632 - Flags: approval2.0?
(Assignee)

Updated

9 years ago
Attachment #488065 - Flags: approval2.0?
(Assignee)

Updated

9 years ago
Attachment #488172 - Flags: approval2.0?
(Assignee)

Updated

9 years ago
Attachment #488185 - Flags: approval2.0?
(Assignee)

Updated

9 years ago
Depends on: 612730
(Assignee)

Updated

9 years ago
Depends on: 613249

Comment 14

8 years ago
Assigning to me for doc.
Assignee: mounir.lamouri → jswisher

Comment 15

8 years ago
Documented at: https://developer.mozilla.org/en/CSS/:-moz-ui-invalid
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Updated

8 years ago
Assignee: jswisher → mounir.lamouri
(Assignee)

Updated

8 years ago
Depends on: 619278
(Assignee)

Updated

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