Implement :valid and :invalid pseudoclass for <form>

Implement :valid and :invalid pseudoclass for <form>

BUG=360466
TEST=added form-pseudo-valid-style.html layout test

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183185

[keishi, 2014-09-29 13:20:43]

keishi@chromium.org changed reviewers:
+ keishi@chromium.org

[keishi, 2014-09-29 13:20:44]

In blink we always add a test if the behavior changes.
You'll need to add a test to LayoutTests/fast/forms.
This file might help.
LayoutTests/fast/forms/radio/radio-live-validation-style.html

https://codereview.chromium.org/616443002/diff/1/Source/core/html/HTMLFormCon...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/616443002/diff/1/Source/core/html/HTMLFormCon...
Source/core/html/HTMLFormControlElement.cpp:497:
formOwner()->setNeedsStyleRecalc(SubtreeStyleChange);
Form controls can use form attribute to get associated with a non ancestor form,
so they are not always descendants of the owner. I think this element and its
owner form both need style recalc.

[Bartek Nowierski, 2014-10-01 08:22:23]

On 2014/09/29 13:20:44, keishi wrote:
> In blink we always add a test if the behavior changes.
I couldn't find any tests related to control validity, so I just matched the
test coverage here haha ;)

I just added some simple validity tests for both the controls and form.

[Bartek Nowierski, 2014-10-01 08:23:01]

https://codereview.chromium.org/616443002/diff/1/Source/core/html/HTMLFormCon...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/616443002/diff/1/Source/core/html/HTMLFormCon...
Source/core/html/HTMLFormControlElement.cpp:497:
formOwner()->setNeedsStyleRecalc(SubtreeStyleChange);
On 2014/09/29 13:20:43, keishi wrote:
> Form controls can use form attribute to get associated with a non ancestor
form,
> so they are not always descendants of the owner. I think this element and its
> owner form both need style recalc.

Done.

[keishi, 2014-10-01 09:25:28]

https://codereview.chromium.org/616443002/diff/60001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/616443002/diff/60001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.cpp:497: // other form elemetns, so
recaluclate the entire form's subtree.
nit: /elemetns/elements/
nit: /recaluclate/recalculate/

https://codereview.chromium.org/616443002/diff/60001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormControlElementTest.cpp (right):

https://codereview.chromium.org/616443002/diff/60001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElementTest.cpp:79:
EXPECT_TRUE(input2->isValidElement());
We don't need this test because this should already be covered by existing tests
like
LayoutTests/fast/forms/checkValidity-002.html

https://codereview.chromium.org/616443002/diff/60001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormElementTest.cpp (right):

https://codereview.chromium.org/616443002/diff/60001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormElementTest.cpp:17: class HTMLFormElementTest : public
::testing::Test {
In blink we try to write layout tests if possible and only use gtest when we
need to test something that is not exposed to the web. For example the test
in HTMLFormControlElementTest.cpp is testing the C++ method
findCustomValidationMessageTextDirection.

Also simple test for isValidElement is already covered by tests like
fast/forms/checkValidity-select-form.html and fast/forms/checkValidity-004.html

This CL adds :valid/:invalid support so I recommend adding a test using
Element.matches(":valid") like the test I mentioned in my previous comment.

Your new test should be added to LayoutTests/fast/forms.

If you want to test if the correct style is applied (for example to test that
the
shared style issue you had won't happen again), you could also write a reftest.
http://trac.webkit.org/wiki/Writing%20Reftests

A reftest is a type of layout test that captures a screenshot of the test html
and the expected html and
compares them.
Your test html could be something like:
<style>form:invalid {background-color:red}</style>
<form><input required></form>

And your -expected html would be:
<form style="background-color: red"><input></form>

[Bartek Nowierski, 2014-10-02 14:26:43]

We discussed caching validity in the form for performance. If you don't mind,
I'd like to do it in another CL. This one is already quite complex

https://codereview.chromium.org/616443002/diff/60001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/616443002/diff/60001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.cpp:497: // other form elemetns, so
recaluclate the entire form's subtree.
On 2014/10/01 09:25:27, keishi wrote:
> nit: /elemetns/elements/
> nit: /recaluclate/recalculate/

Done.

https://codereview.chromium.org/616443002/diff/60001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormControlElementTest.cpp (right):

https://codereview.chromium.org/616443002/diff/60001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElementTest.cpp:79:
EXPECT_TRUE(input2->isValidElement());
On 2014/10/01 09:25:27, keishi wrote:
> We don't need this test because this should already be covered by existing
tests
> like
> LayoutTests/fast/forms/checkValidity-002.html

Should I remove it? Or keep it since I already wrote it?

https://codereview.chromium.org/616443002/diff/60001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormElementTest.cpp (right):

https://codereview.chromium.org/616443002/diff/60001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormElementTest.cpp:17: class HTMLFormElementTest : public
::testing::Test {
On 2014/10/01 09:25:28, keishi wrote:
> In blink we try to write layout tests if possible and only use gtest when we
> need to test something that is not exposed to the web. For example the test
> in HTMLFormControlElementTest.cpp is testing the C++ method
> findCustomValidationMessageTextDirection.
> 
> Also simple test for isValidElement is already covered by tests like
> fast/forms/checkValidity-select-form.html and
fast/forms/checkValidity-004.html
> 
> This CL adds :valid/:invalid support so I recommend adding a test using
> Element.matches(":valid") like the test I mentioned in my previous comment.
> 
> Your new test should be added to LayoutTests/fast/forms.
> 
> If you want to test if the correct style is applied (for example to test that
> the
> shared style issue you had won't happen again), you could also write a
reftest.
> http://trac.webkit.org/wiki/Writing%20Reftests
> 
> A reftest is a type of layout test that captures a screenshot of the test html
> and the expected html and
> compares them.
> Your test html could be something like:
> <style>form:invalid {background-color:red}</style>
> <form><input required></form>
> 
> And your -expected html would be:
> <form style="background-color: red"><input></form>

Added a LayoutTests/fast/forms/form-pseudo-valid-style.html
It also covers the case of multiple forms sharing styles.

[keishi, 2014-10-03 03:44:14]

Okay you can add the caching in another CL.

https://codereview.chromium.org/616443002/diff/60001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormControlElementTest.cpp (right):

https://codereview.chromium.org/616443002/diff/60001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElementTest.cpp:79:
EXPECT_TRUE(input2->isValidElement());
On 2014/10/02 14:26:43, Bartek Nowierski wrote:
> On 2014/10/01 09:25:27, keishi wrote:
> > We don't need this test because this should already be covered by existing
> tests
> > like
> > LayoutTests/fast/forms/checkValidity-002.html
> 
> Should I remove it? Or keep it since I already wrote it?

I don't think this adds any value, I think we should remove it.

https://codereview.chromium.org/616443002/diff/80001/LayoutTests/fast/forms/f...
File LayoutTests/fast/forms/form-pseudo-valid-style.html (right):

https://codereview.chromium.org/616443002/diff/80001/LayoutTests/fast/forms/f...
LayoutTests/fast/forms/form-pseudo-valid-style.html:50: debug('Adding a required
text input indirectly to a form:');
nit: 'indirectly' is hard to understand. Maybe 'from JS' would be better.

https://codereview.chromium.org/616443002/diff/80001/LayoutTests/fast/forms/f...
LayoutTests/fast/forms/form-pseudo-valid-style.html:54: var dev1 =
document.createElement('dev');
/dev/div/?

https://codereview.chromium.org/616443002/diff/80001/Source/core/css/resolver...
File Source/core/css/resolver/SharedStyleFinder.cpp (right):

https://codereview.chromium.org/616443002/diff/80001/Source/core/css/resolver...
Source/core/css/resolver/SharedStyleFinder.cpp:245: if
(candidate.isValidElement() != element().isValidElement())
I think we should check document().containsValidityStyleRules().

https://codereview.chromium.org/616443002/diff/80001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/616443002/diff/80001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.cpp:249: void
HTMLFormControlElement::setFormOwnerNeedsStyleRecalc()
I think this should be done by the HTMLFormElement in a method like
HTMLFormElement::setNeedsValidityCheck(). We have some exceptions but I think
elements should decide if it needs style recalc for itself not for other
elements. For example in this case, if you implement caching HTMLFormElement
could tell when the validity hasn't changed and avoid a style recalc.

https://codereview.chromium.org/616443002/diff/80001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.cpp:264: // Element addition can change
validity of the form.
I think you'll find that Blink is light on comments. People often complain about
it, but it is a tradition carried over from WebKit. It is so we try to make the
code describe what we are doing so we don't need comments.

If we use HTMLFormElement::setNeedsValidityCheck() here, I think it will be more
clear that we are expecting  a validity change on the form and we won't need
this comment.

[Bartek Nowierski, 2014-10-03 06:01:33]

https://codereview.chromium.org/616443002/diff/60001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormControlElementTest.cpp (right):

https://codereview.chromium.org/616443002/diff/60001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElementTest.cpp:79:
EXPECT_TRUE(input2->isValidElement());
On 2014/10/03 03:44:13, keishi wrote:
> On 2014/10/02 14:26:43, Bartek Nowierski wrote:
> > On 2014/10/01 09:25:27, keishi wrote:
> > > We don't need this test because this should already be covered by existing
> > tests
> > > like
> > > LayoutTests/fast/forms/checkValidity-002.html
> > 
> > Should I remove it? Or keep it since I already wrote it?
> 
> I don't think this adds any value, I think we should remove it.

Ok, removing this one and removing HTMLFormElementTest.cpp completely

https://codereview.chromium.org/616443002/diff/80001/LayoutTests/fast/forms/f...
File LayoutTests/fast/forms/form-pseudo-valid-style.html (right):

https://codereview.chromium.org/616443002/diff/80001/LayoutTests/fast/forms/f...
LayoutTests/fast/forms/form-pseudo-valid-style.html:50: debug('Adding a required
text input indirectly to a form:');
On 2014/10/03 03:44:13, keishi wrote:
> nit: 'indirectly' is hard to understand. Maybe 'from JS' would be better.
Oh, I meant that it isn't a direct child of <form>. I updated the text

https://codereview.chromium.org/616443002/diff/80001/LayoutTests/fast/forms/f...
LayoutTests/fast/forms/form-pseudo-valid-style.html:54: var dev1 =
document.createElement('dev');
On 2014/10/03 03:44:13, keishi wrote:
> /dev/div/?

Done.

https://codereview.chromium.org/616443002/diff/80001/Source/core/css/resolver...
File Source/core/css/resolver/SharedStyleFinder.cpp (right):

https://codereview.chromium.org/616443002/diff/80001/Source/core/css/resolver...
Source/core/css/resolver/SharedStyleFinder.cpp:245: if
(candidate.isValidElement() != element().isValidElement())
On 2014/10/03 03:44:13, keishi wrote:
> I think we should check document().containsValidityStyleRules().

Done.
And I moved this check to the bottom

https://codereview.chromium.org/616443002/diff/80001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/616443002/diff/80001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.cpp:249: void
HTMLFormControlElement::setFormOwnerNeedsStyleRecalc()
On 2014/10/03 03:44:13, keishi wrote:
> I think this should be done by the HTMLFormElement in a method like
> HTMLFormElement::setNeedsValidityCheck(). We have some exceptions but I think
> elements should decide if it needs style recalc for itself not for other
> elements. For example in this case, if you implement caching HTMLFormElement
> could tell when the validity hasn't changed and avoid a style recalc.

Done.

https://codereview.chromium.org/616443002/diff/80001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.cpp:264: // Element addition can change
validity of the form.
On 2014/10/03 03:44:13, keishi wrote:
> I think you'll find that Blink is light on comments. People often complain
about
> it, but it is a tradition carried over from WebKit. It is so we try to make
the
> code describe what we are doing so we don't need comments.
> 
> If we use HTMLFormElement::setNeedsValidityCheck() here, I think it will be
more
> clear that we are expecting  a validity change on the form and we won't need
> this comment.

Done.

[keishi, 2014-10-03 06:34:41]

https://codereview.chromium.org/616443002/diff/100001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/form-pseudo-valid-style.html (right):

https://codereview.chromium.org/616443002/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/form-pseudo-valid-style.html:56:
input1.setAttribute('type', 'text')
nit: semicolon at end

https://codereview.chromium.org/616443002/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/form-pseudo-valid-style.html:57:
input1.setAttribute('required', '')
Ditto.

https://codereview.chromium.org/616443002/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/form-pseudo-valid-style.html:60: debug('')
Ditto.

https://codereview.chromium.org/616443002/diff/100001/Source/core/html/HTMLFo...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/616443002/diff/100001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.cpp:264:
formOwnerSetNeedsValidityCheck();
I'm sorry. I told you to put it here yesterday but I just realized that you
should be override willChangeForm/didChangeForm and call it from there.

Like this example, the form validity could change because of the form attribute.
http://jsfiddle.net/zxg3go7o/2/

[Bartek Nowierski, 2014-10-03 07:16:58]

https://codereview.chromium.org/616443002/diff/100001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/form-pseudo-valid-style.html (right):

https://codereview.chromium.org/616443002/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/form-pseudo-valid-style.html:56:
input1.setAttribute('type', 'text')
On 2014/10/03 06:34:41, keishi wrote:
> nit: semicolon at end

Done.

https://codereview.chromium.org/616443002/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/form-pseudo-valid-style.html:57:
input1.setAttribute('required', '')
On 2014/10/03 06:34:41, keishi wrote:
> Ditto.

Done.

https://codereview.chromium.org/616443002/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/form-pseudo-valid-style.html:60: debug('')
On 2014/10/03 06:34:41, keishi wrote:
> Ditto.

Done.

https://codereview.chromium.org/616443002/diff/100001/Source/core/html/HTMLFo...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/616443002/diff/100001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.cpp:264:
formOwnerSetNeedsValidityCheck();
On 2014/10/03 06:34:41, keishi wrote:
> I'm sorry. I told you to put it here yesterday but I just realized that you
> should be override willChangeForm/didChangeForm and call it from there.
> 
> Like this example, the form validity could change because of the form
attribute.
> http://jsfiddle.net/zxg3go7o/2/

That makes sense! I added a test for that too.

[keishi, 2014-10-03 08:01:58]

keishi@chromium.org changed reviewers:
+ tkent@chromium.org

[keishi, 2014-10-03 08:01:59]

LGTM. +tkent

You should fill out the CL description.
BUG= should be the crbug.com number and TEST= should say that you added
form-pseudo-valid-style.html.

https://codereview.chromium.org/616443002/diff/100001/Source/core/html/HTMLFo...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/616443002/diff/100001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.cpp:264:
formOwnerSetNeedsValidityCheck();
On 2014/10/03 07:16:57, Bartek Nowierski wrote:
> On 2014/10/03 06:34:41, keishi wrote:
> > I'm sorry. I told you to put it here yesterday but I just realized that you
> > should be override willChangeForm/didChangeForm and call it from there.
> > 
> > Like this example, the form validity could change because of the form
> attribute.
> > http://jsfiddle.net/zxg3go7o/2/
> 
> That makes sense! I added a test for that too.

Thanks!

[tkent, 2014-10-03 08:17:23]

lgtm.
Please work on :valid/:invalid support for <fieldset> too.

[tkent, 2014-10-03 08:18:16]

> Please work on :valid/:invalid support for <fieldset> too.

By another CL, I meant.

[Bartek Nowierski, 2014-10-03 09:12:39]

The CQ bit was checked by bartekn@chromium.org

[commit-bot: I haz the power, 2014-10-03 09:13:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/616443002/140001

[commit-bot: I haz the power, 2014-10-03 10:21:48]

Committed patchset #8 (id:140001) as 183185

[Bartek Nowierski, 2014-10-07 00:18:30]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/628133004/ by bartekn@chromium.org.

The reason for reverting is:
https://code.google.com/p/chromium/issues/detail?id=420450 "Heap-use-after-free
in blink::RenderBlock::willBeDestroyed".