596515 - Add a possibility of styling form:invalid

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[gadjo]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv:1.9.2.9) Gecko/20100824 Firefox/3.6.9
Build Identifier: 

There should be a possibility of styling the form or fieldset which is invalid, now we can style only :invalid form elements or :-moz-submit-invalid input. Styling -moz-any(form,fieldset):invalid would be much more flexible and more intuitive than the -moz-submit-invalid, and we still could style submit by writing

form:invalid input[type="submit"] 



Reproducible: Always

Comment 1

[Boris Zbarsky [:bzbarsky]]

> and we still could style submit by writing
> form:invalid input[type="submit"] 

No, you couldn't.  That does the wrong thing when the relevant submit is not a descendant of the form in the DOM, for example.

Have you considered raising your suggestion with the whawg or html working group, by the way?

Comment 2

[Mounir Lamouri (:mounir)]

I think the idea is interesting and it will be quite easy to do.

Comment 3

[Mounir Lamouri (:mounir)]

A few hints if someone wants to jump in:
For form:invalid, the file to touch is content/html/content/src/nsHTMLInputElement.cpp and that would be as easy as using mInvalidElementsCount.
When nsHTMLFormElement::UpdateValidity is called, you will have to update the element state with ::UpdateState() and you will have to create a ::IntrinsicState() method (like in nsHTMLInputElement.cpp) that will have something like:
if (mInvalidElementsCount) {
  state |= NS_EVENT_STATE_INVALID;
} else {
  state |= NS_EVENT_STATE_INVALID;
}

We might want to use :-moz-invalid instead of :invalid actually. In addition, some tests will be necessary.

For fieldset:invalid, that will be a bit more complex but maybe doing something similar to what is done with nsHTMLFormElement:UpdateValidity() would be the way to go.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Mounir meant content/html/content/src/nsHTMLFormElement.cpp

Comment 5

[Mounir Lamouri (:mounir)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #3)
> if (mInvalidElementsCount) {
>   state |= NS_EVENT_STATE_INVALID;
> } else {
>   state |= NS_EVENT_STATE_INVALID;
> }

And here I meant NS_EVENT_STATE_VALID for the second "state |= [...]".

Comment 6

[Mounir Lamouri (:mounir)]

FWIW, :invalid/:valid in <form> is now part of HTML5 spec, see:
http://html5.org/tools/web-apps-tracker?from=6887&to=6888

Also, I would be okay to have this bug split in two. In other words, implementing :valid/:invalid for <form> here and opening another bug for <fieldset>.

Comment 7

[gadjo]

Bug 717181 for fieldset:invalid

Comment 8

[Jignesh Kakadiya [:jhk]]

Attachment: patch

Comment 9

[Mounir Lamouri (:mounir)]

Comment on attachment 588833 [details] [diff] [review]
patch

Review of attachment 588833 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

For the tests, you can try to use the same as in layout/reftests/css-submit-invalid/input-submit/, with some adaptations.
You should put them in:
layout/reftests/css-invalid/form/ and layout/reftests/css-valid/form/
Feel free to merge some tests in one file.

I'm kind of a test maniac and this is the kind of features that should have a good test coverage so feel free to write a lot of them :)

::: content/html/content/src/nsHTMLFormElement.cpp
@@ +1222,5 @@
>      nsCOMPtr<nsIConstraintValidation> cvElmt = do_QueryObject(aChild);
>      if (cvElmt &&
>          cvElmt->IsCandidateForConstraintValidation() && !cvElmt->IsValid()) {
>        UpdateValidity(false);
> +      UpdateState(true);

I would have preferred to have the call in UpdateValidity().

@@ +2144,5 @@
> +  if (mInvalidElementsCount) {
> +    state |= NS_EVENT_STATE_INVALID;
> +  } else {
> +      state |= NS_EVENT_STATE_VALID;
> +    }

nit: indentation seems to be wrong here

Comment 10

[Jignesh Kakadiya [:jhk]]

Attachment: test+patch

Comment 11

[Mounir Lamouri (:mounir)]

Comment on attachment 590224 [details] [diff] [review]
test+patch

Review of attachment 590224 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for the code
f- for the tests

You should tests basically situations like these:
- form with only invalid elements
- form with only valid elements
- form with invalid and valid elements
- form with one invalid element and the element is dynamically removed
- form with no element and one invalid one is added dynamically
- form with no element and one valid one is added dynamically
- form with one valid element which is made valid dynamically
- form with one invalid element which is made invalid dynamically
- form with one invalid element and another invalid one is added dynamically
- form with one invalid element and another valid one is added dynamically
- form with one valid element and a valid one is added dynamically
- form with one valid element and a invalid one is added dynamically
- form with one invalid element and a barred for constraint validation element
- form with one invalid element and a barred for constraint validation element then you remove the second element
- form with one valid element and a barred for constraint validation element
- form with one valid element and a barred for constraint validation element then you remove the second element
- form with one valid element and you dynamically add a barred constraint validation element
- form with one invalid element and you dynamically add a barred constraint validation element

Feel free to merge some of those tests in the same file. Feel free to add some too ;)

And sorry, I told you I was a test maniac :)

::: content/html/content/src/nsHTMLFormElement.cpp
@@ +2143,5 @@
> +
> +  if (mInvalidElementsCount) {
> +    state |= NS_EVENT_STATE_INVALID;
> +  } 
> +  else {

nit: coding style says you should do "} else {"

Comment 12

[Jignesh Kakadiya [:jhk]]

Attachment: test + Patch

Comment 13

[Mounir Lamouri (:mounir)]

Comment on attachment 593810 [details] [diff] [review]
test + Patch

Review of attachment 593810 [details] [diff] [review]:
-----------------------------------------------------------------

Except the two comments I put, everything looks good. Some files are not present but I assume you haven't written them yet, write?

Also, a general comment: it's usually nice to put a comment in the tested file to say what should actually happen. It can be simple like:
<!--
  The form should be valid.
-->

Thank you very much for writing those tests!

::: layout/reftests/css-invalid/form/form-invalid-ref.html
@@ +2,5 @@
> +<html>
> +  <body>
> +    <table id='t'>
> +      <tr><td><input required></td></tr>
> +   </table>

You want a real form here, not a table.
This said, I doubt you want this file. Looks like your tests are assuming invalid forms are not displayed no about:blank should be enough.

::: layout/reftests/css-invalid/form/reftest.list
@@ +1,4 @@
> +== form-static-valid.html form-valid-ref.html
> +== form-dynamic-valid.html form-valid-ref.html
> +== form-remove-invalid-element.html form-valid-ref-2.html
> +== form-remove-control.html form-valid-ref-3.html

form-remove-control.html seems to be invalid.

Comment 14

[Jignesh Kakadiya [:jhk]]

> ::: layout/reftests/css-invalid/form/reftest.list
> @@ +1,4 @@
> > +== form-static-valid.html form-valid-ref.html
> > +== form-dynamic-valid.html form-valid-ref.html
> > +== form-remove-invalid-element.html form-valid-ref-2.html
> > +== form-remove-control.html form-valid-ref-3.html
> 
> form-remove-control.html seems to be invalid.
What do you mean by Invalid? This file shouldn't be included in tests? or It shouldn't be compared with form-valid-ref-3.html?

Comment 15

[Jignesh Kakadiya [:jhk]]

Attachment: Added remaining tests.

Comment 16

[Mounir Lamouri (:mounir)]

Comment on attachment 594077 [details] [diff] [review]
Added remaining tests.

Review of attachment 594077 [details] [diff] [review]:
-----------------------------------------------------------------

Looks generally pretty good but in addition of the few comments I did, I think you are missing those tests:
- form with invalid and valid elements
- form with no element and one invalid element is added dynamically
- form with one invalid element and a barred for constraint validation element
- form with one invalid element and a barred for constraint validation element then you remove the second element
- form with one valid element and a barred for constraint validation element
- form with one valid element and a barred for constraint validation element then you remove the second element
- form with one valid element and you dynamically add a barred constraint validation element
- form with one invalid element and you dynamically add a barred constraint validation element

Plus those two that are not done correctly (mostly because I made a mistake in the description)
~ form with one valid element which is made invalid dynamically
~ form with one invalid element which is made valid dynamically

In the other hand, you wrote some tests I didn't really ask for and that's awesome! :)

Sorry again for being picky with the tests but that way we guarantee we are not going to regress [easily] this feature.

::: layout/reftests/css-invalid/form/form-add-control.html
@@ +1,2 @@
> +<!DOCTYPE html>
> +<!--form with one invalid element and another invalid one is added dynamically -->

nit: you meant "another *valid* one"

::: layout/reftests/css-invalid/form/form-add-valid-with-invalid-element.html
@@ +1,2 @@
> +<!DOCTYPE html>
> +<!--form with one invalid element and another valid one is added dynamically -->

Seems similar to form-add-control.html, isn't it?

::: layout/reftests/css-invalid/form/form-change-type-control.html
@@ +7,5 @@
> +  </head>
> +  <script>
> +    function onloadHandler()
> +    {
> +      document.getElementById('i').type = 'form';

I don't understand what you are trying to do in that test.

::: layout/reftests/css-invalid/form/form-change-type-not-control.html
@@ +7,5 @@
> +  </head>
> +  <script>
> +    function onloadHandler()
> +    {
> +      document.getElementById('i').type = 'submit';

ditto

::: layout/reftests/css-invalid/form/form-dynamic-invalid-barred-2.html
@@ +10,5 @@
> +  </head>
> +  <script>
> +    function onloadHandler()
> +    {
> +      document.getElementById('i').value = '';

ditto
Do you want to make the element invalid? If so, you want to remove readonly too.

::: layout/reftests/css-invalid/form/form-dynamic-invalid-barred.html
@@ +1,2 @@
> +<!DOCTYPE html>
> +<!--form with one invalid element and a barred for constraint validation element -->

The description doesn't really match what the test does.
Otherwise, the test look ok.

::: layout/reftests/css-invalid/form/form-dynamic-invalid.html
@@ +1,2 @@
> +<!DOCTYPE html>
> +<!-- form with one invalid element which is made invalid dynamically -->

My bad, my list of tests was wrong, you want to make it *valid* dynamically.
So, value='foo' here for example.

::: layout/reftests/css-invalid/form/form-dynamic-valid.html
@@ +1,2 @@
> +<!DOCTYPE html>
> +<!--  form with one valid element which is made valid dynamically -->

That would be like the previous test. But you something *invalid* that is made *valid*.

::: layout/reftests/css-invalid/form/form-valid-ref-4.html
@@ +2,5 @@
> +<html>
> +  <body>
> +    <form>
> +      <input type='text' value='foo'>
> +      <input type = 'text' value='bar'>

nit: type='text' (remove the spaces)

::: layout/reftests/css-invalid/form/reftest.list
@@ +3,5 @@
> +== form-remove-invalid-element.html form-valid-ref-2.html
> +== form-change-type-not-control.html form-invalid-ref.html
> +== form-static-invalid.html form-invalid-ref.html
> +== form-dynamic-invalid.html form-invalid-ref.html
> +== form-add-control.html form-invalid-ref.html

You shouldn't campare to form-invalid-ref.html but about:blank.
The reason is you are defining form:invalid { dispaly: none; } in the tested page style and you are comparing this to an HTML page with some data. Luckily, all elements do not show any content so invalid-ref.html is equivalent to about:blank but about:blank will be just fine here.
Alternatively, you can keep form-invalid-ref.html and remove <form></form> from it. As you prefer.

Comment 17

[Jignesh Kakadiya [:jhk]]

Attachment: Added More Tests.

Hope I didn't miss any test.:)

Comment 18

[Mounir Lamouri (:mounir)]

Comment on attachment 594161 [details] [diff] [review]
Added More Tests.

Review of attachment 594161 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome! I think you wrote more tests than I asked you to :)

r=me with the nits fixed. No need to re-ask for a review after changing the nits.

Could you send that patch to the try server and ask for checkin if you have a green run? I will be glad to push it for you.

::: content/html/content/src/nsHTMLFormElement.cpp
@@ +2144,5 @@
> +  if (mInvalidElementsCount) {
> +    state |= NS_EVENT_STATE_INVALID;
> +  } 
> +  else {
> +      state |= NS_EVENT_STATE_VALID;

nit: indentation

::: layout/reftests/css-invalid/form/form-dynamic-invalid-barred-2.html
@@ +1,3 @@
> +<!DOCTYPE html>
> +<!-- form with one barred for constraint validation element then you remove
> +the element -->

nit: the description isn't correct: you make the element not barred and invalid.
I'm ok with the test doing what it's currently doing, you should just update the comment (and maybe the file name).

@@ +17,5 @@
> +    }
> +  </script>
> +  <body onload='onloadHandler();'>
> +    <form>
> +      <input id='i' value='foo' readonly='readonly' required>

nit: readonly alone is ok, no need for readonly='readonly'

Comment 19

[Jignesh Kakadiya [:jhk]]

Attachment: fixed suggested changes.

Comment 20

[Mounir Lamouri (:mounir)]

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=2973c92a2bdb

Comment 21

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/7fa9d642fe74

Comment 22

[Jean-Yves Perrier [:teoli]]

I updated: https://developer.mozilla.org/en/CSS/:invalid
and
https://developer.mozilla.org/en/HTML/Element/form


https://developer.mozilla.org/en/Firefox_13_for_developers has been updated by sheppy.