Fix bug where form/fieldset :valid/:invalid won't be recalculated upon control's willValidate change

Fix a problem where a control's willValidate change wouldn't trigger the containing form's or fieldset's style change. Cache form validity while at it to avoid style recalculation

BUG=360466
TEST=Added cases to layout tests that toggle controls disabled and readonly in form and fieldset

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

[Bartek Nowierski, 2014-11-04 04:19:38]

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

[Bartek Nowierski, 2014-11-04 04:20:39]

This is derived from https://codereview.chromium.org/672163002/ after pulling
and resolving

[tkent, 2014-11-06 04:44:57]

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

https://codereview.chromium.org/703473003/diff/1/Source/core/html/HTMLFormCon...
Source/core/html/HTMLFormControlElement.h:146: void
setNeedsWillValidateCheck(bool = true);
I couldn't find setNeedsWillValidateCheck callsites with false argument.

[Bartek Nowierski, 2014-11-06 07:15:44]

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

https://codereview.chromium.org/703473003/diff/1/Source/core/html/HTMLFormCon...
Source/core/html/HTMLFormControlElement.h:146: void
setNeedsWillValidateCheck(bool = true);
On 2014/11/06 04:44:57, tkent wrote:
> I couldn't find setNeedsWillValidateCheck callsites with false argument.

Good catch! In one place where I'm calling with true I should be calling with
false. Fixed.

[tkent, 2014-11-07 00:15:19]

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

https://codereview.chromium.org/703473003/diff/1/Source/core/html/HTMLFormCon...
Source/core/html/HTMLFormControlElement.h:146: void
setNeedsWillValidateCheck(bool = true);
On 2014/11/06 07:15:44, Bartek Nowierski wrote:
> On 2014/11/06 04:44:57, tkent wrote:
> > I couldn't find setNeedsWillValidateCheck callsites with false argument.
> 
> Good catch! In one place where I'm calling with true I should be calling with
> false. Fixed.

I see.
nit: Because we don't like bool arguments (#10 of
http://www.chromium.org/blink/coding-style#TOC-Names), I recommend to have two
functions like setNeedsWillValidateCheck() and
setNeedsWillValidateCheckAndHideValidationMessage().

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

https://codereview.chromium.org/703473003/diff/20001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.cpp:410:
const_cast<HTMLFormControlElement*>(this)->setNeedsWillValidateCheck(false);
Why do we need to pass |false| here?  I think validation message is not visible
in this case, and calling hideVisibleValidationMessage() in
setNeedsWillValidateCheck() is harmless.

[Bartek Nowierski, 2014-11-12 22:23:57]

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

https://codereview.chromium.org/703473003/diff/1/Source/core/html/HTMLFormCon...
Source/core/html/HTMLFormControlElement.h:146: void
setNeedsWillValidateCheck(bool = true);
On 2014/11/07 00:15:19, tkent wrote:
> On 2014/11/06 07:15:44, Bartek Nowierski wrote:
> > On 2014/11/06 04:44:57, tkent wrote:
> > > I couldn't find setNeedsWillValidateCheck callsites with false argument.
> > 
> > Good catch! In one place where I'm calling with true I should be calling
with
> > false. Fixed.
> 
> I see.
> nit: Because we don't like bool arguments (#10 of
> http://www.chromium.org/blink/coding-style#TOC-Names), I recommend to have two
> functions like setNeedsWillValidateCheck() and
> setNeedsWillValidateCheckAndHideValidationMessage().

We don't like bool arguments more than we don't like code duplication??

Anyway, I removed the parameter per your other comment, so problem solved :)

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

https://codereview.chromium.org/703473003/diff/20001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.cpp:410:
const_cast<HTMLFormControlElement*>(this)->setNeedsWillValidateCheck(false);
On 2014/11/07 00:15:19, tkent wrote:
> Why do we need to pass |false| here?  I think validation message is not
visible
> in this case, and calling hideVisibleValidationMessage() in
> setNeedsWillValidateCheck() is harmless.

We added this to make the new code behave exactly like the old code. This way we
don't call an extra hideVisibleValidationMessage() if we don't need to. But I'll
be happy to remove it. It will simplify things :)

[tkent, 2014-11-13 01:15:03]

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

https://codereview.chromium.org/703473003/diff/1/Source/core/html/HTMLFormCon...
Source/core/html/HTMLFormControlElement.h:146: void
setNeedsWillValidateCheck(bool = true);
On 2014/11/12 22:23:56, Bartek Nowierski wrote:
> On 2014/11/07 00:15:19, tkent wrote:
> > On 2014/11/06 07:15:44, Bartek Nowierski wrote:
> > > On 2014/11/06 04:44:57, tkent wrote:
> > > > I couldn't find setNeedsWillValidateCheck callsites with false argument.
> > > 
> > > Good catch! In one place where I'm calling with true I should be calling
> with
> > > false. Fixed.
> > 
> > I see.
> > nit: Because we don't like bool arguments (#10 of
> > http://www.chromium.org/blink/coding-style#TOC-Names), I recommend to have
two
> > functions like setNeedsWillValidateCheck() and
> > setNeedsWillValidateCheckAndHideValidationMessage().
> 
> We don't like bool arguments more than we don't like code duplication??
> 
> Anyway, I removed the parameter per your other comment, so problem solved :)

I meant something like:

void setNeedsWillValidateCheck() { ...}
void setNeedsWillValidateCheckAndHideValidationMessage()
{
    setNeedsWillValidateCheck();
    hideVisibleValidationMessage();
}

[Bartek Nowierski, 2014-11-13 01:38:52]

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

https://codereview.chromium.org/703473003/diff/1/Source/core/html/HTMLFormCon...
Source/core/html/HTMLFormControlElement.h:146: void
setNeedsWillValidateCheck(bool = true);
On 2014/11/13 01:15:02, tkent wrote:
> On 2014/11/12 22:23:56, Bartek Nowierski wrote:
> > On 2014/11/07 00:15:19, tkent wrote:
> > > On 2014/11/06 07:15:44, Bartek Nowierski wrote:
> > > > On 2014/11/06 04:44:57, tkent wrote:
> > > > > I couldn't find setNeedsWillValidateCheck callsites with false
argument.
> > > > 
> > > > Good catch! In one place where I'm calling with true I should be calling
> > with
> > > > false. Fixed.
> > > 
> > > I see.
> > > nit: Because we don't like bool arguments (#10 of
> > > http://www.chromium.org/blink/coding-style#TOC-Names), I recommend to have
> two
> > > functions like setNeedsWillValidateCheck() and
> > > setNeedsWillValidateCheckAndHideValidationMessage().
> > 
> > We don't like bool arguments more than we don't like code duplication??
> > 
> > Anyway, I removed the parameter per your other comment, so problem solved :)
> 
> I meant something like:
> 
> void setNeedsWillValidateCheck() { ...}
> void setNeedsWillValidateCheckAndHideValidationMessage()
> {
>     setNeedsWillValidateCheck();
>     hideVisibleValidationMessage();
> }

Oh. But then we'll lose the m_willValidate check and we'd call
hideVisibleValidationMessage() when we shouldn't. We could work it around by
returning m_willValidate from setNeedsWillValidateCheck(), but I don't think
it's any cleaner than what I had.

[keishi, 2014-11-13 04:46:28]

LGTM

[Bartek Nowierski, 2014-11-14 03:18:19]

I had to update a css layout test. Please take a peek. I'm not sure if it's a
good test if it counts calls of certain type, which is really an implementation
detail

[tkent, 2014-11-14 03:30:13]

The CQ bit was checked by tkent@chromium.org

[tkent, 2014-11-14 03:30:13]

lgtm

[commit-bot: I haz the power, 2014-11-14 03:30:46]

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

[commit-bot: I haz the power, 2014-11-14 04:40:22]

Committed patchset #5 (id:80001) as 185347