Bug 34924 – Add checks if setNeedsWillValidateCheck() and setNeedsValidityCheck() are called correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

https://bugs.webkit.org/show_bug.cgi?id=31716#c20
> It almost seems that despite our bad experiences, caching the old value would
> be helpful, not for optimizing the result of the functions to fetch the
> validity state, but for getting the state changes correct. It's quite fragile
> to both have functions to answer "is this valid" and then scattered code to
> invalidate when those constraints change. How would someone changing the
> functions know to go update the call sites where those states could change.
> 
> How would notice if we forgot to call one of these state change functions?
> Maybe we need to devise a debugging feature to catch mistakes.
> 
> If we do cache a value I think it should be a single three-state concept: "will
> not validate", "valid", "invalid". Treating these as independent makes the code
> more complicated in places where they overlap. I know the DOM API for this
> treats them separately, but the style code would be much simpler if it used a
> single three-state concept.

[Kent Tamura]

Created attachment 48721 [details]
Proposed patch

[Kent Tamura]

(In reply to comment #0)
> > If we do cache a value I think it should be a single three-state concept: "will
> > not validate", "valid", "invalid". Treating these as independent makes the code
> > more complicated in places where they overlap. I know the DOM API for this
> > treats them separately, but the style code would be much simpler if it used a
> > single three-state concept.

I couldn't apply the single three-state concept.
According to the HTML5, the validity state should work even if willValidate is false. So the patch introduces two boolean members.

[Adam Barth]

Comment on attachment 48721 [details]
Proposed patch

in HTMLInputElement::setValue, it looks like

+ setNeedsValidityCheck();

can be lifted above the "inputType() == FILE" if-clause because its called on both branches.

This looks reasonable.  I'm not an expert on validity checks, but this patch has been sitting around for a month and seems to prove the codebase.

[Kent Tamura]

Thank you for reviewing.

(In reply to comment #3)
> + setNeedsValidityCheck();
> 
> can be lifted above the "inputType() == FILE" if-clause because its called on
> both branches.

setNeedsValidityCehck() should be called
  after updating the value && before style recalc.
So this code is the best.

I'm adding a comment about it, and landing.

[Kent Tamura]

Landed as r56385.