Bug 27454

From 4.10.14.5 I read: "Constraint validation: If an element has a maximum allowed value length, and its dirty value flag is false, and the code-point length of the element's value is greater than the element's maximum allowed value length, then the element is suffering from being too long." This means that if the attribute is specified and its value is a non-negative number AND if value's length ("code-point length") is greater than that attribute's value AND the dirty flag is "FALSE"... it suffers from being too long. Specs clarify how the dirty value flag shall be dealt with, from http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#concept-input-value-dirty-flag I read: "Each input element has a boolean dirty value flag. When it is true, the element is said to have a dirty value. The dirty value flag must be initially set to false when the element is created, and must be set to true whenever the user interacts with the control in a way that changes the value." So, the tooLong flag would be set ONLY if element's value has never been modified by users (and if it is greater than maxlength). Is that right? Another issue for me is to understand the way it should react when maxlength=0: could text form controls (such as some input types and textareas) be forced to be left blank?

(In reply to comment #1) > So, the tooLong flag would be set ONLY if element's value has never been > modified by users (and if it is greater than maxlength). Is that right? I think you stuck in an extra "never". It can only be tooLong if it _has_ been modified. > Another issue for me is to understand the way it should react when maxlength=0: > could text form controls (such as some input types and textareas) be forced to > be left blank? Yes.

(In reply to comment #1) > From 4.10.14.5 I read: > > "Constraint validation: If an element has a maximum allowed value length, and > its dirty value flag is false, and the code-point length of the element's value > is greater than the element's maximum allowed value length, then the element is > suffering from being too long." Ian confirmed that it's a typo.:) Thank you both. Constraint validation: If an element has a maximum allowed value length, and its dirty value flag is *TRUE*, and the code-point length of the element's value is greater than the element's maximum allowed value length, then the element is suffering from being too long.

As commented WebCore already support a "maxlength" attribute but it seems to have a slight different behavior. Its interface is the following: attribute long maxLength; HTML5 specs state the following interface instead: attribute unsigned long maxLength; Moreover, at this time WebCore supports this attribute only in HTMLInputElement along with its data structure (InputElementData); this should be supported in HTMLTextAreaElement too. These are the changes I believe are necessary: 1) modify HTMLInputElement's idl to match the one declared in specs (unsigned long, instead of long); 2) modify "int maxlength()" getter to "unsigned maxlength()", same for its setter; both will be pulled up into HTMLFormControlElement. 3) InputElement::parseMaxLengthAttribute shall be refactored and pulled up into an higher level class (HTMLFormControlElement??)

(In reply to comment #4) > 1) modify HTMLInputElement's idl to match the one declared in specs (unsigned > long, instead of long); This is only needed if there's some way to detect the difference from a test case. Is there any difference? > 2) modify "int maxlength()" getter to "unsigned maxlength()", same for its > setter; both will be pulled up into HTMLFormControlElement. That seems wrong. We don't want to have a maxlength for other kinds of form control elements and moving this into the base class doesn't sound like a good idea to me. > 3) InputElement::parseMaxLengthAttribute shall be refactored and pulled up into an higher level class (HTMLFormControlElement??) These two maxlength attributes can be separate and you shouldn't try to factor them and move the functions into a base class unless there's some obvious clear benefit to doing so.

(In reply to comment #5) > > 2) modify "int maxlength()" getter to "unsigned maxlength()", same for its > > setter; both will be pulled up into HTMLFormControlElement. > That seems wrong. We don't want to have a maxlength for other kinds of form > control elements and moving this into the base class doesn't sound like a good > idea to me. maxlength applies to TextArea as well. http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#dom-textarea-maxlength > > 3) InputElement::parseMaxLengthAttribute shall be refactored and pulled up into an higher level class (HTMLFormControlElement??) > These two maxlength attributes can be separate and you shouldn't try to factor > them and move the functions into a base class unless there's some obvious clear > benefit to doing so. Ditto. WebCore's "maxlength" attribute seems to have a slight different behavior than HTML5's, how do you usually solve such situation?

Since Adele originally implemented maxLength on bug 6987 her insight is probably welcome here. I am confused like Michelangelo is by comment 5. Darin, are you proposing implementing an "HTML5 maxLength" that would live in parallel with the existing maxLength and behave differently? It seems at a glance like hoisting maxLength and modifying it to comply with the HTML5 spec would be the right course of action...

The intent in HTML5 is to define what WebKit should do, including for the existing maxlength support. It isn't supposed to be different. If there are differences that you are concerned will result in compatibility issues, please let me know.

(In reply to comment #6) > (In reply to comment #5) > > > > 2) modify "int maxlength()" getter to "unsigned maxlength()", same for its > > > setter; both will be pulled up into HTMLFormControlElement. > > That seems wrong. We don't want to have a maxlength for other kinds of form > > control elements and moving this into the base class doesn't sound like a good > > idea to me. > > maxlength applies to TextArea as well. > http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#dom-textarea-maxlength Sure, but that doesn't mean they have to share the same base class. I'm just talking about the factoring. > > > 3) InputElement::parseMaxLengthAttribute shall be refactored and pulled up into an higher level class (HTMLFormControlElement??) > > These two maxlength attributes can be separate and you shouldn't try to factor > > them and move the functions into a base class unless there's some obvious clear > > benefit to doing so. > > Ditto. WebCore's "maxlength" attribute seems to have a slight different > behavior than HTML5's, how do you usually solve such situation? We can fix the bugs. Make test cases and show what's wrong. I am arguing with some specifics about your proposal of where to put the code, not with the concept of fixing bugs.

(In reply to comment #9) > (In reply to comment #6) > > maxlength applies to TextArea as well. > > http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#dom-textarea-maxlength > > Sure, but that doesn't mean they have to share the same base class. I'm just > talking about the factoring. OK. I haven't looked at the existing WebCore maxLength implementation. Perhaps it's so simple it should be copy-and-pasted between the classes that need it, or there is an obvious third place for a shared implementation to go. I've asked Michelangelo to first write a patch that converts the existing maxLength algorithm to match the HTML5 spec, along with a testcase for each change made (so there should be a test for the long-versus-unsigned-long change, for example). After we get that agreed-on and landed, a patch that implements maxLength on all elements where HTML5 specs it should be simpler and easier to review. Sound OK?

> I've asked Michelangelo to first write a patch that converts the existing > maxLength algorithm to match the HTML5 spec, along with a testcase for each > change made (so there should be a test for the long-versus-unsigned-long > change, for example). After we get that agreed-on and landed, a patch that > implements maxLength on all elements where HTML5 specs it should be simpler and > easier to review. Sound OK? Sounds good. You could even do the tests first, landing failing and then patching the test results as we fix various things.

Actually, HTMLInputElement(s) never suffers of being "tooLong": each value longer than its "maxlength" attribute is trimmed so "validity.tooLong" is always false. About this specs state: "User agents may prevent the user from causing the element's value to be set to a value whose code-point length is greater than the element's maximum allowed value length." which seems very reasonable to me. If no objection is raised I'm gonna get a patch to make HTMLTextArea match the same behaviour, even if this means tooLong will always be false. Now, what would be an acceptable "default maxlength" for TextAreas?

(In reply to comment #12) > Actually, HTMLInputElement(s) never suffers of being "tooLong": each value > longer than its "maxlength" attribute is trimmed so "validity.tooLong" is > always false. I am concerned about apparent spec self-inconsistency here. Section 4.10.4.2.7 reads: "If the input element has a maximum allowed value length, then the code-point length of the value of the element's value attribute must be equal to or less than the element's maximum allowed value length." Section 4.10.14.5 reads: "If an element has a maximum allowed value length, and its dirty value flag is true, and the code-point length of the element's value is greater than the element's maximum allowed value length, then the element is suffering from being too long. "User agents may prevent the user from causing the element's value to be set to a value whose code-point length is greater than the element's maximum allowed value length." The issue here is that the second reference above makes clamping the value length to the maxLength optional, but the first makes it required. Or am I misreading somehow? In the abstract, it'd be easier to implement just flagging long values as "tooLong" and not actually clamping the values. If we do want to clamp, it seems like all applicable elements should clamp, and should disallow more typing when the value hits the limit. But perhaps there's some legacy expectations around the behavior here...

(In reply to comment #13) > I am concerned about apparent spec self-inconsistency here. I share you concerns; at this time, perhaps, even if a bit harder, I believe that making TextArea behave like Inputs do is better: having two separate behaviours for the same attribute would be most confusing and quite a pain. It's just an opinion of mine, for what it's worth. > In the abstract, it'd be easier to implement just flagging long values as > "tooLong" and not actually clamping the values. If we do want to clamp, it > seems like all applicable elements should clamp, and should disallow more > typing when the value hits the limit. But perhaps there's some legacy > expectations around the behavior here... Yes: much of the "clamp" logic is in InputElement::constrainValue.

This: "If the input element has a maximum allowed value length, then the code-point length of the value of the element's value attribute must be equal to or less than the element's maximum allowed value length." ...is a requirement on authors, not implementors.

(In reply to comment #15) > This: > "If the input element has a maximum allowed value length, then the code-point > length of the value of the element's value attribute must be equal to or less > than the element's maximum allowed value length." > ...is a requirement on authors, not implementors. So we can ignore that entirely? I'm not sure what that requires authors to do... not set an initial value that's too long? If the UA doesn't trim the value to the maxLength, what does this even mean?

Yes, you can ignore it entirely (unless you write an editor). It requires authors not to set an initial value that's longer than maxlength. I don't understand your last question. It has nothing to do with UA behaviour.

OK, here's the definitive summary from my conversation with Hixie on #webkit: The spec requires the following: * If a field's initial value is longer than maxLength, but the field has not had any value set via the DOM, and the user hasn't modified the field, tooLong is false. * If a field's value is longer than maxLength and either the value has been set via the DOM (even to the same value as the initial value!), or the user has modified the field, tooLong is true. Consistency with other UAs' behavior requires the following additional pieces: * If a field's initial value is longer than maxLength, the whole value should be displayed, but the user cannot modify or add to it except to delete it (until it becomes short enough). I have filed bug 28001 about this and written more complete details there. * If the user attempts to make a value longer than maxLength, e.g. by typing or pasting, the value is clamped to maxLength characters. * The value may be set to something longer than maxLength programmatically via the DOM, in which case all of it is displayed (like the first bullet). If you combine all these constraints, what you find is that tooLong can only ever be true in the following two cases: * The initial value is longer than maxLength, and the user deletes something from the value but doesn't delete enough of it to bring the result down to maxLength. * The value is set to something longer than maxLength via the DOM, and not subsequently pared back by the user (or script). Again, note that having the initial value be "foo" and then setting the element's value to "foo" by the DOM _is_ considered to set the dirty value flag and thus make the element potentially tooLong. The upshot is that tooLong will rarely be true.

I already have the code. I'll ask to review it after Bug#29292 is landed.

You might want to go ahead an post the code anyway, even if it's not up for review. I do that often so that patches I've worked on are never lost, even if my computer dies or I stop working in that area.

Created attachment 40214 [details] Proposed patch

Comment on attachment 40214 [details] Proposed patch > + switch (inputType()) { > + case EMAIL: > + case PASSWORD: > + case SEARCH: > + case TELEPHONE: > + case TEXT: > + case URL: { > + bool userEditted = !m_data.value().isNull(); // tooLong must be false for the default value. > + if (userEditted && hasAttribute(maxlengthAttr)) { > + // It's safe to cast maxLength() to unsigned because maxLength() can never be negative. > + return value().length() > static_cast<unsigned>(maxLength()); > + } > + break; It's spelled "edited", not "editted". I think the comment is hard to read. You say "tooLong must be false for the default value" but what I think a better comment would be: // Return false for the default value even if it is longer than maxLength. Also, better to have the comment on the line before. It's harder to read it on the end of the line. Should do return false here instead of break, so we can ASSERT_NOT_REACHED outside the switch. We normally prefer early-return style, so the return should be for the case where there is no length check. > + } > + case BUTTON: > + case CHECKBOX: > + case COLOR: > + case FILE: > + case HIDDEN: > + case IMAGE: > + case ISINDEX: > + case NUMBER: > + case RADIO: > + case RANGE: > + case RESET: > + case SUBMIT: > + break; Should do return false here instead of break, so we can ASSERT_NOT_REACHED outside the switch. > + return false; Should ASSERT_NOT_REACHED here. > + const_cast<HTMLTextAreaElement*>(this)->updateValue(); As a rule, we should not use const_cast. Instead we should use mutable. > +bool HTMLTextAreaElement::tooLong() const > +{ > + if (m_isDirty) { > + bool ok; > + unsigned maxLength = getAttribute(maxlengthAttr).string().toUInt(&ok); > + if (ok) > + return value().length() > maxLength; > + } > + return false; > +} Our normal approach is early return. So we'd do: if (!m_isDirty) return false; if (!ok) return false; Instead of the way the logic is structured above. r=me as is, but pleaseconsider my comments.

Comment on attachment 40214 [details] Proposed patch Given that Darin asked for comments above, I don't think this is ready for automated commit. cq-. You're welcome to make similar fixes as you land this by hand, or to post a new patch with corrections that can be cq+'d. Your call.

Thank you for the comments. I'll update the patch after Bug#29796 is landed.

Comment on attachment 40214 [details] Proposed patch Removing Darin Adler's r+ from this obsolete patch (so that it doesn't show up in our list of patches to commit).

(In reply to comment #22) > > + const_cast<HTMLTextAreaElement*>(this)->updateValue(); > > As a rule, we should not use const_cast. Instead we should use mutable. I can revert updateValue() to const by adding "mutable" to m_isDirty, but the current updateValue() has const_cast for setFormControlValueMatchesRenderer(). Do you think the const_cast in the current code is reasonable? Should we make setFormControlValueMatchesRender() const, or make value() non-const?

Created attachment 40343 [details] Proposed patch (rev.2) - Follow Darin's comments - Rename parameter names of setMaxLength() An existing const_cast in updateValue() remains.

(In reply to comment #26) > the > current updateValue() has const_cast for setFormControlValueMatchesRenderer(). > Do you think the const_cast in the current code is reasonable? Should we make > setFormControlValueMatchesRender() const, or make value() non-const? The right way to do this is to: 1) Decide which functions are "conceptually const" at a high level; which ones run without changing the state of the object. 2) Mark those functions as const. 3) Find cases where these const functions need to call non-const ones, and figure out why. Resolve this either by changing the caller to be non-const, or the called function to be const. 4) Make data members mutable as needed. But there's no need to resolve this for existing code just to write new code. For your specific question, I think that we should probably make setFormControlValueMatchesRender() const.

Comment on attachment 40343 [details] Proposed patch (rev.2) I think later we might find an even better name than m_isDirty since that's a bit vague -- dirty with respect to what? But I think it's already a pretty good. r=me

(In reply to comment #29) > (From update of attachment 40343 [details]) > I think later we might find an even better name than m_isDirty since that's a > bit vague -- dirty with respect to what? But I think it's already a pretty > good. m_isDirty" was taken from "dirty value flag" in the HTML5 spec. http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#concept-textarea-dirty I think it is very clear for the spec readers, and m_edited or m_userEdited might be better for others.

Comment on attachment 40343 [details] Proposed patch (rev.2) Clearing flags on attachment: 40343 Committed r48959: <http://trac.webkit.org/changeset/48959>

All reviewed patches have been landed. Closing bug.