Bug 25551 (H5F-RequiredAttr)

Created attachment 30679 [details] Early, test-free and incomplete patch Waiting for comments on this early patch which is still test-free. Obviously no r? asked.

I'm not a reviewer, but here are my drive-by comments. * Lines should not have only whitespace in them (some added lines have spaces in them). Use a linter. * I wouldn't bother with FIXMEs and TODOs for "add support for more input types" in these functions as whenever more input types are added it will be clear they need to be handled everywhere the existing types are. Since we don't yet have the enum values for these this just decreases clarity. * "Leads us to understand whether or not a form control could potentially be a required form control." is incredibly verbose and vague at the same time. Either don't comment or say something like "True if this control supports the 'required' attribute for form validation." * Add tests (as you noted).

Created attachment 31139 [details] Final patch Added "semi"-final patch, waiting for comments to ask for review.

The patch doesn't actually include the new tests, just the ChangeLog for them. TELEPHONE is already in the codebase? Wow. If you include the tests I think you should go ahead and r? your patch.

Created attachment 31141 [details] Final patch, corrected

(In reply to comment #4) > The patch doesn't actually include the new tests, just the ChangeLog for them. LOL, mistake.:)

Comment on attachment 31141 [details] Final patch, corrected I'm concerned by the absence of tests for validity (i realise they're dependent on bug #19562, but i think in that case we probably want to make this bug depend on that one). I actually believe that hasMissingValue is wrong for CHECKBOX and RADIO as it appears to imply that a checked radio button or checkbox is missing its value.

(In reply to comment #7) > I'm concerned by the absence of tests for validity (i realise they're dependent > on bug #19562, but i think in that case we probably want to make this bug > depend on that one). I actually believe that hasMissingValue is wrong for I definitely agree on that. Updated and clean patch for #19562 in on its way.

#19562 has been landed; working to adapt required attribute code to ValidityState.

Created attachment 32803 [details] Propopsed patch, rev. 2 This patch include support for the valueMissing flag as well as new tests for the ValidityState object. Waiting for comments.

Comment on attachment 32803 [details] Propopsed patch, rev. 2 This patch lacks the support for ":required" and ":optional" CSS pseudoclasses, I'm working to include it.

I must point out conditions for these pseudoclasses to be applied to elements are quite dark to me, from http://www.w3.org/TR/html5/interactive-elements.html: *** :required The :required pseudo-class must match the following elements: input elements that are required textarea elements that have a required attribute *** This means that INPUT elements, potentially suffering of being missed (= NO disabled, NO readonly, NO button, etc..) can have this pseudoclasses on them (this means I can use the validity().isControlRequired() checke found in the latest patch of mine.) On the other hand, textareas just have to be marked as required, what about readonly attribute on such elements? This seems quite inconsistent to me. Anyone quite clearer ideas? Same deal with the optional pseudoclass: *** :optional The :optional pseudo-class must match the following elements: button elements input elements that are not required select elements textarea elements that do not have a required attribute ***

(In reply to comment #12) > I must point out conditions for these pseudoclasses to be applied to elements > are quite dark to me I'm not clear what your objection is. Do you worry about the practical effect on users of having an element that is both required and readonly (and given no initial value)? If you think that's a case the spec should cover you should bring that up on the whatwg mailing list, or at least ask Hixie in IRC. Otherwise it seems pretty clear to me that an element matches :required iff it is marked "required", without regard to whether it's possible for a user to edit the value therein.

(In reply to comment #13) > Otherwise it seems pretty clear to me that an element matches :required iff it > is marked "required", without regard to whether it's possible for a user to > edit the value therein. It seemed quite weird to me because the required attribute applies and it's "honourable" only on mutable inputs and mutable textareas. Anyway, I've written the code that adds these new pseudoclasses and I've been writing another TS to check'em out. In the meantime, except for the CSS stuff, how does the code look? What are your opinions?

(In reply to comment #14) > It seemed quite weird to me because the required attribute applies and it's > "honourable" only on mutable inputs and mutable textareas. A careful reading of 4.10.4.2.5 seems to make this clear: "The required attribute is a boolean attribute. When specified, the element is required. Constraint validation: If the element is required, and its value DOM attribute applies and is in the mode value, and the element is mutable, and the element's value is the empty string, then the element is suffering from being missing." When combined with the relevant bit on :required: "The :required pseudo-class must match the following elements: * input elements that are required * textarea elements that have a required attribute" The result is: :required matches input elements that are required. Input elements are required if they have the boolean "required" attribute specified. Input element only fail constraint validation if they're required _and_ they're mutable _and_ they're empty and some other stuff. Thus, it's possible for an element to be required (and thus be matched by :required) and yet not fail constraint validation when empty.

Almost. Input elements are required if they have the boolean "required" attribute specified *and the attribute applies*. So for example, :required won't match this element: <input required type=submit> This is because of the following text: # These attributes only apply to an input element if its type attribute # is in a state whose definition declares that the attribute applies. # When an attribute doesn't apply to an input element, user agents # must ignore the attribute. -- http://www.whatwg.org/specs/web-apps/current-work/#common-input-element-attributes But you are correct in that :required will match this element, despite this element not preventing form submission: <input required type=text disabled>

(In reply to comment #14) > In the meantime, except for the CSS stuff, how does the code look? What are > your opinions? Based on Hixie's and my comments, I think if you move some things like disabled/readonly checking from isControlRequired() into the valueMissing() code, then you can use isControlRequired() to implement :required. Some of the layout tests' language is a little unclear, but perhaps that's not a problem.

(In reply to comment #15) > Thus, it's possible for an element to be required (and thus be matched by > :required) and yet not fail constraint validation when empty. Uhmmm, I'm quite doubtful about the semantic of the specs: input elements that are required -> inputs that have requiredAttr set AND required applies (mutable) textarea elements that have a required attribute -> textareas that have requiredAttr set, whenever or not they are mutable. In my opinion (for what it's worth:)), if that spec meant something such as ":required applies only to inputs and textareas with requiredAttr set, whatever state they're in", they should have stated in another manner: input elements that have a required ATTRIBUTE <- textarea elements that have a required attribute This may sound academic, I know.:)

(In reply to comment #18) > Uhmmm, I'm quite doubtful about the semantic of the specs: > > input elements that are required -> inputs that have requiredAttr set AND > required applies (mutable) No, you've mistaken what "required applies" means. "mutable" is a separate precondition for constraint validation than "required applies". "required applies" means "input element is a type that supports required". To fail constraint validation, the element must be required, required must apply, the element must be mutable, and the element must be empty; those are all separate points.

Created attachment 32898 [details] Proposed patch, rev.3

Comment on attachment 32898 [details] Proposed patch, rev.3 Looks good. Two questions. > + case CSSSelector::PseudoOptional: { > + if (!e || !e->isFormControlElement()) > + return false; > + > + if (e->hasTagName(buttonTag) || e->hasTagName(selectTag)) > + return true; > + else if (e->hasTagName(inputTag)) > + return !static_cast<HTMLInputElement*>(e)->validity()->isControlRequired(); > + else if (e->hasTagName(textareaTag)) > + return !static_cast<HTMLTextAreaElement*>(e)->required(); > + > + break; > + } Can we add a virtual function for this instead of all this per-element code in the style selector? > + case CSSSelector::PseudoRequired: { > + if (!e || !e->isFormControlElement()) > + return false; > + > + if (e->hasTagName(inputTag)) > + return static_cast<HTMLInputElement*>(e)->validity()->isControlRequired(); > + else if (e->hasTagName(textareaTag)) > + return static_cast<HTMLTextAreaElement*>(e)->required(); > + > + break; > + } Ditto. > + virtual bool required() const; Why is this virtual? Any polymorphic callers?

(In reply to comment #21) > Can we add a virtual function for this instead of all this per-element code in > the style selector? Good observation, I'll move validity() definition up to Element.

Created attachment 32945 [details] Proposed patch, rev. 4 Modified according to observations: + virtual isOptionalFormControl + virtual isRequiredFormControl

Created attachment 32950 [details] Proposed patch, rev. 4b In Rev.4 there was an unecessary inclusion of ValidityState.h in CSSStyleSelector.cpp, cleaned up.

Comment on attachment 32950 [details] Proposed patch, rev. 4b > + case CSSSelector::PseudoOptional: { > + if (!e || !e->isFormControlElement()) > + return false; > + > + return e->isOptionalFormControl(); > + } No braces needed here. Since you put isOptionalFormControl into Element, you don't need so much code. Just: case CSSSelector::PseudoOptional: return e && e->isOptionalFormControl(); Same for PseudoRequired. > + virtual bool isOptionalFormControl() const { return true; } I prefer to make such virtual function overrides in the private: section of a class. If anyone calls this on a button element it means they made a programming mistake and I think it's best if the code fails to compile in such cases that can easily be caught. Making the virtual function override private accomplishes that. > +bool HTMLInputElement::isRequiredFormControl() const > +{ > + if (!required()) > + return false; > + > + switch (inputType()) { > + case HTMLInputElement::HIDDEN: > + case HTMLInputElement::RANGE: > + case HTMLInputElement::SUBMIT: > + case HTMLInputElement::IMAGE: > + case HTMLInputElement::RESET: > + case HTMLInputElement::BUTTON: > + return false; > + default: > + return true; > + } > + > + return false; > +} There is no need to have that "return false" statement there if there is no exit from the switch statement, and in fact I believe this will fail to compile with some compilers due to the unreachable code. The enum values don't need to be qualified by the HTMLInputElement class name, because this is already a member function. For maintenance I prefer to have all the values of the enum listed. If you do that and omit the default from the switch, then the gcc compiler will catch you if you add a new enum value without modifying this function. If you remove the default, then you will need the return after the switch, though, although you can do an ASSERT_NOT_REACHED. The saveForMControlState function uses the preferred style. > + virtual bool isOptionalFormControl() const { return !isRequiredFormControl(); } > + virtual bool isRequiredFormControl() const; Same thought about making these private. > + virtual bool isOptionalFormControl() const { return true; } Ditto. > + virtual bool isOptionalFormControl() const { return !isRequiredFormControl(); } > + virtual bool isRequiredFormControl() const { return required(); } Ditto. > + switch (i->inputType()) { > + case HTMLInputElement::TEXT: > + case HTMLInputElement::SEARCH: > + case HTMLInputElement::URL: > + case HTMLInputElement::TELEPHONE: > + case HTMLInputElement::EMAIL: > + case HTMLInputElement::PASSWORD: > + case HTMLInputElement::NUMBER: > + return i->value().isEmpty(); > + case HTMLInputElement::FILE: > + return i->files()->isEmpty(); > + case HTMLInputElement::CHECKBOX: > + return !i->checked(); > + case HTMLInputElement::RADIO: > + return !i->document()->checkedRadioButtons().checkedButtonForGroup(i->name()); > + default: > + return false; Same thought about omitting the default case from this. Again, would this be done better with code in the various element classes and a virtual function rather than all this code specific per-type here in the ValidityState class? I think those are enough comments that I'll say review- and give you a chance to consider the comments. I think all the comments I made are optional, so please do use your own best judgement.

(In reply to comment #25) > For maintenance I prefer to have all > the values of the enum listed. If you do that and omit the default from the > switch, then the gcc compiler will catch you if you add a new enum value > without modifying this function. FWIW (which is not very much), I prefer the "use-default" route as the code is now, because it's more compact (and thus more readable IMO), and it makes adding future values easier when the default is well-chosen. However, Darin's points are reasonable, so use your judgment. > If you remove the default, then you will need the return after the switch, > though, although you can do an ASSERT_NOT_REACHED. Personally I would s/can/should/.

(In reply to comment #26) > (In reply to comment #25) > > For maintenance I prefer to have all > > the values of the enum listed. If you do that and omit the default from the > > switch, then the gcc compiler will catch you if you add a new enum value > > without modifying this function. > > FWIW (which is not very much), I prefer the "use-default" route as the code is > now, because it's more compact (and thus more readable IMO), and it makes > adding future values easier when the default is well-chosen. This is predicated on a “well-chosen default”, but what would that be in a function like the one added here? If the new value is a variant on type=text we'd need one default, but if it’s a variant on type=button we’d need another.

(In reply to comment #25) <...big snip...> > > + virtual bool isOptionalFormControl() const { return true; } > I prefer to make such virtual function overrides in the private: section of a > class. If anyone calls this on a button element it means they made a > programming mistake and I think it's best if the code fails to compile in such > cases that can easily be caught. Making the virtual function override private > accomplishes that. Uhmmm, I honestly fail to understand this: speaking conceptually an HTMLButtonElement is an optional form control element so I wonder what could be wrong in calling something such as myHTMLButtonIstance->isOptionalFormControl() [always true] or someChildOfElement->isRequiredFormControl(); [false]. [ValidityState::valueMissing(), checks on real name] > Again, would this be done better with code in the various element classes and a > virtual function rather than all this code specific per-type here in the > ValidityState class? IMO, checks for validation are not elements' responsability, I mean: a given element could be required or optional (it's its own property/status), but validation checks are made through external routines that are conceptually separated from element's abstraction: it's not priceless, indeed (those two ifs/casts). Always IMO: it's someElement.validity.valueMissing, not someElement.valueMissing. Obviously your wise commens are most welcome, it's just some opinion of mine; you have the final word on those.:)

(In reply to comment #28) > Uhmmm, I honestly fail to understand this: speaking conceptually an > HTMLButtonElement is an optional form control element so I wonder what could be > wrong in calling something such as myHTMLButtonIstance->isOptionalFormControl() > [always true] or someChildOfElement->isRequiredFormControl(); [false]. It's inefficient to make a virtual function call for something that's known at compile time. That's all. And in the past I usually find that when someone calls one of these it's because they made a programming mistake. It's not a conceptual error to ask if a <button> is optional, but it's a clue that the programmer may be confused, since every <button> is so there's no need to make a function call. > [ValidityState::valueMissing(), checks on real name] > > Again, would this be done better with code in the various element classes and a > > virtual function rather than all this code specific per-type here in the > > ValidityState class? > > IMO, checks for validation are not elements' responsibility OK.

(In reply to comment #29) > It's inefficient to make a virtual function call for something that's known at > compile time. That's all. And in the past I usually find that when someone > calls one of these it's because they made a programming mistake. The principle is to start everything as private as possible, and only make things public as needed, and also to not make an override public just because it's public in the base class.

(In reply to comment #29) > It's inefficient to make a virtual function call for something that's known at > compile time. That's all. And in the past I usually find that when someone > calls one of these it's because they made a programming mistake. > > It's not a conceptual error to ask if a <button> is optional, but it's a clue > that the programmer may be confused, since every <button> is so there's no need > to make a function call. True, but that seems a little less obvious on the other cases, where the implementation calls another function rather than simply returning true. And it seems less future-proof if we think there would ever be a reason we'd make a simple "return true" depend on anything else. But those are minor. I think your comment about making things as private as possible is probably a clearer argument. Although I still don't really see any harm in making these public either :) > > [ValidityState::valueMissing(), checks on real name] > > > Again, would this be done better with code in the various element classes and a > > > virtual function rather than all this code specific per-type here in the > > > ValidityState class? > > > > IMO, checks for validation are not elements' responsibility > > OK. FWIW, I agree with Darin's original comment here. I think it is clearer and better to implement these checks on the elements themselves. Just because "validity" is some collection of attributes does not mean that the actual implementation of those attributes is somehow divorced from the element they're on.

(In reply to comment #31) > FWIW, I agree with Darin's original comment here. I think it is clearer and ...it's worth, it's worth...:-) Ok, changes in progress: - moving isRequired/OptionalFormControl() in private section of Element's subclasses; - moving validation code to form control elements. > better to implement these checks on the elements themselves. Just because > "validity" is some collection of attributes does not mean that the actual > implementation of those attributes is somehow divorced from the element they're > on. I can't say I totally agree but you made your point.:-)

Created attachment 32975 [details] Proposed patch, rev.5 Modified according to comments from Darin and Peter.

Comment on attachment 32975 [details] Proposed patch, rev.5 > + switch (inputType()) { > + case TEXT: > + case SEARCH: > + case URL: > + case TELEPHONE: > + case EMAIL: > + case PASSWORD: > + case NUMBER: > + case FILE: > + return value().isEmpty(); > + case CHECKBOX: > + return !checked(); > + case RADIO: > + return !document()->checkedRadioButtons().checkedButtonForGroup(name()); > + case HIDDEN: > + case RANGE: > + case SUBMIT: > + case IMAGE: > + case RESET: > + case BUTTON: > + case ISINDEX: > + ASSERT_NOT_REACHED(); > + } > + > + return false; Could also use an assertion outside the switch statement, since that should not be reached either. I suggest replacing the ASSERT_NOT_REACHED with a break and moving the ASSERT_NOT_REACHED outside the switch. r=me

Created attachment 32979 [details] Proposed patch, rev. 5b Done.

Landed patch in r46062.

Associated radar: <rdar://problem/119655018>

Does anyone know whether or not this worked on safari at one point? Because it definitely doesn't now...

It didn't.

(In reply to comment #38) > Does anyone know whether or not this worked on safari at one point? > Because it definitely doesn't now... I haven't tried Safari 6 yet, but I guess it support :required. e.g. <input required value=""> matches :invalid CSS selector. Safari 6 might have no interactive validation feature. See Bug 59019.