Add :-moz-ui-invalid pseudo-class

RESOLVED FIXED in mozilla2.0b8

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Blocks: 1 bug, {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

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

Updated

7 years ago
Severity: blocker → normal
(Assignee)

Updated

7 years ago
OS: Linux → All
(Assignee)

Updated

7 years ago
Depends on: 609016
(Assignee)

Updated

7 years ago
Blocks: 609017
(Assignee)

Comment 1

7 years ago
Created attachment 487631 [details] [diff] [review]
Part 1 - Add :-moz-ui-invalid pseudo-class
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #487631 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

7 years ago
Created attachment 487632 [details] [diff] [review]
Part 2 - Make :-moz-ui-invalid follow rules for :invalid
Attachment #487632 - Flags: review?(bzbarsky)
(Assignee)

Updated

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

Updated

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

Updated

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

Updated

7 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

7 years ago
Depends on: 609417
(Assignee)

Updated

7 years ago
Depends on: 609162
(Assignee)

Comment 5

7 years ago
Created attachment 488065 [details] [diff] [review]
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.
Attachment #488065 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

7 years ago
Created attachment 488172 [details] [diff] [review]
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.
Attachment #488172 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

7 years ago
Created attachment 488185 [details] [diff] [review]
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.
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

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

7 years ago
Depends on: 610415
(Assignee)

Comment 11

7 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
Attachment #487631 - Flags: approval2.0? → approval2.0+
(Assignee)

Updated

7 years ago
Depends on: 610427
(Assignee)

Updated

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

Updated

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

Updated

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

Updated

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

Updated

7 years ago
Depends on: 612730
(Assignee)

Updated

7 years ago
Depends on: 613249
(Assignee)

Comment 13

7 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8

Comment 14

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

Comment 15

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

Updated

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

Updated

7 years ago
Depends on: 619278
(Assignee)

Updated

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