Last Comment Bug 596515 - Add a possibility of styling form:invalid
: Add a possibility of styling form:invalid
Status: RESOLVED FIXED
[mentor=volkmar][lang=c++]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla13
Assigned To: Mounir Lamouri (:mounir)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 724839
Blocks: html5forms
  Show dependency treegraph
 
Reported: 2010-09-15 01:32 PDT by gadjo
Modified: 2013-04-15 05:02 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (4.13 KB, patch)
2012-01-16 02:40 PST, Jignesh Kakadiya [:jhk]
mounir: feedback+
Details | Diff | Splinter Review
test+patch (5.30 KB, patch)
2012-01-20 09:08 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
test + Patch (8.83 KB, patch)
2012-02-02 06:08 PST, Jignesh Kakadiya [:jhk]
mounir: feedback+
Details | Diff | Splinter Review
Added remaining tests. (19.55 KB, patch)
2012-02-02 23:52 PST, Jignesh Kakadiya [:jhk]
mounir: feedback+
Details | Diff | Splinter Review
Added More Tests. (25.04 KB, patch)
2012-02-03 06:31 PST, Jignesh Kakadiya [:jhk]
mounir: review+
Details | Diff | Splinter Review
fixed suggested changes. (24.15 KB, patch)
2012-02-03 07:51 PST, Jignesh Kakadiya [:jhk]
mounir: checkin+
Details | Diff | Splinter Review

Description gadjo 2010-09-15 01:32:49 PDT
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 [:bz] (still a bit busy) 2010-09-15 01:40:08 PDT
> 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) 2011-06-15 10:15:26 PDT
I think the idea is interesting and it will be quite easy to do.
Comment 3 Mounir Lamouri (:mounir) 2011-09-15 11:27:26 PDT
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 [:bz] (still a bit busy) 2011-09-15 11:32:09 PDT
Mounir meant content/html/content/src/nsHTMLFormElement.cpp
Comment 5 Mounir Lamouri (:mounir) 2011-09-15 11:35:43 PDT
(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) 2012-01-10 16:28:43 PST
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 2012-01-11 00:36:22 PST
Bug 717181 for fieldset:invalid
Comment 8 Jignesh Kakadiya [:jhk] 2012-01-16 02:40:14 PST
Created attachment 588833 [details] [diff] [review]
patch
Comment 9 Mounir Lamouri (:mounir) 2012-01-16 03:26:10 PST
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] 2012-01-20 09:08:44 PST
Created attachment 590224 [details] [diff] [review]
test+patch
Comment 11 Mounir Lamouri (:mounir) 2012-01-24 06:17:47 PST
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] 2012-02-02 06:08:53 PST
Created attachment 593810 [details] [diff] [review]
test + Patch
Comment 13 Mounir Lamouri (:mounir) 2012-02-02 06:56:36 PST
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] 2012-02-02 12:11:47 PST
> ::: 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] 2012-02-02 23:52:41 PST
Created attachment 594077 [details] [diff] [review]
Added remaining tests.
Comment 16 Mounir Lamouri (:mounir) 2012-02-03 02:56:34 PST
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] 2012-02-03 06:31:00 PST
Created attachment 594161 [details] [diff] [review]
Added More Tests.

Hope I didn't miss any test.:)
Comment 18 Mounir Lamouri (:mounir) 2012-02-03 06:51:02 PST
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] 2012-02-03 07:51:08 PST
Created attachment 594185 [details] [diff] [review]
fixed suggested changes.
Comment 20 Mounir Lamouri (:mounir) 2012-02-03 09:13:06 PST
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=2973c92a2bdb
Comment 21 Marco Bonardo [::mak] 2012-02-07 15:06:26 PST
https://hg.mozilla.org/mozilla-central/rev/7fa9d642fe74

Note You need to log in before you can comment on or make changes to this bug.