Bug 34733 - :invalid only works when input is in a form
Summary: :invalid only works when input is in a form
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords: HasReduction
Depends on: 20101 27357
Blocks:
  Show dependency treegraph
 
Reported: 2010-02-08 16:56 PST by Erik Arvidsson
Modified: 2010-04-08 08:38 PDT (History)
8 users (show)

See Also:


Attachments
Test case (432 bytes, text/html)
2010-02-08 16:56 PST, Erik Arvidsson
no flags Details
Proposed patch (26.28 KB, patch)
2010-02-09 22:09 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.2) (26.27 KB, patch)
2010-02-09 22:55 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.3) (29.53 KB, patch)
2010-04-06 10:08 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Arvidsson 2010-02-08 16:56:26 PST
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.
Comment 1 Kent Tamura 2010-02-08 19:24:51 PST
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>.
Comment 2 Michelangelo De Simone 2010-02-09 10:46:52 PST
(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
Comment 3 Kent Tamura 2010-02-09 22:02:11 PST
> 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().
Comment 4 Kent Tamura 2010-02-09 22:09:38 PST
Created attachment 48464 [details]
Proposed patch
Comment 5 Kent Tamura 2010-02-09 22:55:38 PST
Created attachment 48465 [details]
Proposed patch (rev.2)
Comment 6 Erik Arvidsson 2010-02-10 09:30:14 PST
Comment on attachment 48465 [details]
Proposed patch (rev.2)

Thanks and LGTM

A reviewer still has to review this though.
Comment 7 Kent Tamura 2010-02-12 02:13:55 PST
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.
Comment 8 Kent Tamura 2010-04-06 06:36:05 PDT
The specification has been changed so that existens of owner form and non-empty name attribute are ignorable.
Comment 9 Kent Tamura 2010-04-06 10:08:50 PDT
Created attachment 52648 [details]
Proposed patch (rev.3)
Comment 10 Darin Adler 2010-04-06 10:34:32 PDT
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
Comment 11 Eric Seidel (no email) 2010-04-06 23:46:46 PDT
Attachment 52648 [details] was posted by a committer and has review+, assigning to Kent Tamura for commit.
Comment 12 Kent Tamura 2010-04-08 07:05:14 PDT
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.
Comment 13 Kent Tamura 2010-04-08 07:12:44 PDT
Landed as r57274.
Comment 14 WebKit Review Bot 2010-04-08 07:35:40 PDT
http://trac.webkit.org/changeset/57274 might have broken SnowLeopard Intel Release (Tests)
Comment 16 Kent Tamura 2010-04-08 07:52:43 PDT
(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.
Comment 17 Kent Tamura 2010-04-08 07:55:42 PDT
(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.
Comment 18 Kent Tamura 2010-04-08 08:01:56 PDT
r57279 fixes the test failure.
Comment 19 Adam Barth 2010-04-08 08:38:56 PDT
Thanks!