Bug 34733 – :invalid only works when input is in a form

• Component: CSS
• Status: RESOLVED FIXED

[Erik Arvidsson]

Created attachment 48377 [details]
Test case

If I have an input in invalid state (validity.valid === false) I would expect the :invalid rule to apply to it. However, the CSS rule is only applied if the input is inside a form. The validity state works correctly outside forms.

[Kent Tamura]

I checked the current HTML5 draft, and I think we don't need to restrict validation to form controls inside a <form> element.

Opera 10.10 updates :valid/:invalid for form controls without <form>.

[Michelangelo De Simone]

(In reply to comment #1)
> I checked the current HTML5 draft, and I think we don't need to restrict
> validation to form controls inside a <form> element.

I may be wrong but as far as I remember those two pseudoclasses apply only to "validable" elements; to be validable an element should be bound to a form, otherwise is de-facto non-validable (and, thus, can't be neither :valid or :invalid).

CC'ing Hixie

[Kent Tamura]

> I may be wrong but as far as I remember those two pseudoclasses apply only to
> "validable" elements; to be validable an element should be bound to a form,
> otherwise is de-facto non-validable (and, thus, can't be neither :valid or
> :invalid).

I didn't find any clues for requirements of form element and name attribute.  So I'll remove them from HTMLFormControlElement::willValidate().

[Kent Tamura]

Created attachment 48464 [details]
Proposed patch

[Kent Tamura]

Created attachment 48465 [details]
Proposed patch (rev.2)

[Erik Arvidsson]

Comment on attachment 48465 [details]
Proposed patch (rev.2)

Thanks and LGTM

A reviewer still has to review this though.

[Kent Tamura]

I found.

http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#association-of-controls-and-forms
> Constraint validation: If an element has no form owner, it is barred
> from constraint validation.

http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#naming-form-controls
> Constraint validation: If an element does not have a name attribute
> specified, or its name attribute's value is the empty string, then
> it is barred from constraint validation.

http://www.whatwg.org/specs/web-apps/current-work/multipage/links.html#pseudo-classes
> :valid
> The :valid pseudo-class must match all elements that are candidates for
> constraint validation and that satisfy their constraints.
>
> :invalid
> The :invalid pseudo-class must match all elements that are candidates
> for constraint validation but that do not satisfy their constraints.

http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#candidate-for-constraint-validation
> A listed form-associated element is a candidate for constraint
> validation except when a condition has barred the element from
> constraint validation.

So, the current implementation conforms to the specification.

Erik, if we want to change the behavior, we need to propose the change to WHATWG.

[Kent Tamura]

The specification has been changed so that existens of owner form and non-empty name attribute are ignorable.

[Kent Tamura]

Created attachment 52648 [details]
Proposed patch (rev.3)

[Darin Adler]

Comment on attachment 52648 [details]
Proposed patch (rev.3)

> -    , m_willValidate(false)
> +    , m_willValidateInitialized(false)
> +    , m_willValidate(true)

I am not sure that "initialized" is the right word here. Since you called the function "recalc" maybe you should reverse the sense and call it "m_willValidateNeedsRecalc" or "m_needsWillValidateRecalc"? Or maybe this is needed only during construction. Maybe we can make m_willValidateInitialized be a debug-only concept?

>      // FIXME: Check if the control does not have a datalist element as an ancestor.
> -    return m_form && m_hasName && !m_disabled && !m_readOnly;
> +    return !m_disabled && !m_readOnly;

It would be good to make that existing FIXME clearer and more specific. Check that, and do what? Why check it?

>      bool newWillValidate = recalcWillValidate();
> -    if (m_willValidate == newWillValidate)
> +    if (m_willValidateInitialized && m_willValidate == newWillValidate)
>          return;
> +    m_willValidateInitialized = true;

This seems a little strange. Do we really need to recalc immediately here? There at least needs to be a comment explaining why this doesn't just set m_willValidateInitialized to false and return.

r=me

[Eric Seidel (no email)]

Attachment 52648 [details] was posted by a committer and has review+, assigning to Kent Tamura for commit.

[Kent Tamura]

Thank you for reviewing.

(In reply to comment #10)
> (From update of attachment 52648 [details])
> > -    , m_willValidate(false)
> > +    , m_willValidateInitialized(false)
> > +    , m_willValidate(true)
> 
> I am not sure that "initialized" is the right word here. Since you called the
> function "recalc" maybe you should reverse the sense and call it
> "m_willValidateNeedsRecalc" or "m_needsWillValidateRecalc"? Or maybe this is
> needed only during construction. Maybe we can make m_willValidateInitialized be
> a debug-only concept?

This is needed only during construction.  So I think m_willValidateNeedsRecalc and m_needsWillValidateRecalc are not suitable.

> >      // FIXME: Check if the control does not have a datalist element as an ancestor.
> > -    return m_form && m_hasName && !m_disabled && !m_readOnly;
> > +    return !m_disabled && !m_readOnly;
> 
> It would be good to make that existing FIXME clearer and more specific. Check
> that, and do what? Why check it?

Ok, I added what we should do and the reason.

> >      bool newWillValidate = recalcWillValidate();
> > -    if (m_willValidate == newWillValidate)
> > +    if (m_willValidateInitialized && m_willValidate == newWillValidate)
> >          return;
> > +    m_willValidateInitialized = true;
> 
> This seems a little strange. Do we really need to recalc immediately here?

Yes. if willValidate value is changed, we need to add or remove :valid :invalid pseudo selectors. I added a comment.

I'll commit this change with additional comments.

[Kent Tamura]

Landed as r57274.

[WebKit Review Bot]

http://trac.webkit.org/changeset/57274 might have broken SnowLeopard Intel Release (Tests)

[Adam Barth]

Looks like it might be a real breakage:
http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r57274%20(5752)/fast/css/pseudo-invalid-novalidate-001-pretty-diff.html

[Kent Tamura]

(In reply to comment #15)
> Looks like it might be a real breakage:
> http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r57274%20(5752)/fast/css/pseudo-invalid-novalidate-001-pretty-diff.html

I'm investigating.

[Kent Tamura]

(In reply to comment #16)
> (In reply to comment #15)
> > Looks like it might be a real breakage:
> > http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r57274%20(5752)/fast/css/pseudo-invalid-novalidate-001-pretty-diff.html
> 
> I'm investigating.

Ok, I have understood. I'll fix this soon.

[Kent Tamura]

r57279 fixes the test failure.

[Adam Barth]

Thanks!