Closed
Bug 596515
Opened 14 years ago
Closed 13 years ago
Add a possibility of styling form:invalid
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: jk1700, Assigned: mounir)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [mentor=volkmar][lang=c++])
Attachments
(1 file, 5 obsolete files)
24.15 KB,
patch
|
mounir
:
checkin+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
> 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?
Assignee | ||
Comment 2•13 years ago
|
||
I think the idea is interesting and it will be quite easy to do.
Assignee: nobody → mounir.lamouri
Blocks: 344614
Status: UNCONFIRMED → NEW
Component: General → DOM: Core & HTML
Ever confirmed: true
QA Contact: general → general
Assignee | ||
Comment 3•13 years ago
|
||
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.
Whiteboard: [mentor=volkmar]
![]() |
||
Comment 4•13 years ago
|
||
Mounir meant content/html/content/src/nsHTMLFormElement.cpp
Assignee | ||
Comment 5•13 years ago
|
||
(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 |= [...]".
Assignee | ||
Comment 6•13 years ago
|
||
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>.
Summary: Add a possibility of styling form:invalid, fieldset:invalid → Add a possibility of styling form:invalid
Bug 717181 for fieldset:invalid
Updated•13 years ago
|
Keywords: dev-doc-needed
Comment 8•13 years ago
|
||
Attachment #588833 -
Flags: feedback?(mounir)
Assignee | ||
Comment 9•13 years ago
|
||
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
Attachment #588833 -
Flags: feedback?(mounir) → feedback+
Assignee | ||
Updated•13 years ago
|
Whiteboard: [mentor=volkmar] → [mentor=volkmar][lang=c++]
Comment 10•13 years ago
|
||
Attachment #588833 -
Attachment is obsolete: true
Attachment #590224 -
Flags: feedback?(mounir)
Assignee | ||
Comment 11•13 years ago
|
||
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 {"
Attachment #590224 -
Flags: feedback?(mounir)
Comment 12•13 years ago
|
||
Attachment #590224 -
Attachment is obsolete: true
Attachment #593810 -
Flags: feedback?(mounir)
Assignee | ||
Comment 13•13 years ago
|
||
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.
Attachment #593810 -
Flags: feedback?(mounir) → feedback+
Comment 14•13 years ago
|
||
> ::: 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•13 years ago
|
||
Attachment #593810 -
Attachment is obsolete: true
Attachment #594077 -
Flags: review?(mounir)
Assignee | ||
Comment 16•13 years ago
|
||
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.
Attachment #594077 -
Flags: review?(mounir) → feedback+
Comment 17•13 years ago
|
||
Hope I didn't miss any test.:)
Attachment #594077 -
Attachment is obsolete: true
Attachment #594161 -
Flags: review?(mounir)
Assignee | ||
Comment 18•13 years ago
|
||
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'
Attachment #594161 -
Flags: review?(mounir) → review+
Comment 19•13 years ago
|
||
Attachment #594161 -
Attachment is obsolete: true
Assignee | ||
Comment 20•13 years ago
|
||
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=2973c92a2bdb
Assignee | ||
Updated•13 years ago
|
Attachment #594185 -
Flags: checkin+
Comment 21•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7fa9d642fe74
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Comment 22•13 years ago
|
||
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•