Maxlength limits value

[Christian Pedersen]

An input field defined like this:
input type="text" value="Enter zipcode" maxlength="5"
will only show "Enter" when loaded in a Webkit browser. It seems that the maxlength attribute limits the value.

w3c defines maxlength as "the maximum number of characters the *user* may enter."
See http://www.w3.org/TR/1998/REC-html40-19980424/interact/forms.html#adef-maxlength

MS describes it like this: "The maxLength property limits the number of characters the user can enter. The property does not limit programmatic assignments to the value property."
See http://msdn.microsoft.com/en-us/library/ms534157(VS.85).aspx

This bug is a showstopper on sites where the value of the field is compared to the initial value.

[Christian Pedersen]

Created attachment 23980 [details]
Demo of problem

[Mark Rowe (bdash)]

<rdar://problem/6261976>

[Rob Buis]

Created attachment 24091 [details]
Work in progress

So this patch is done based on the bug report and the behaviour of FF3 and Opera. I do wonder if there is also work in Radar for this?

Anyway, if this behaviour/patch is accepted, here is how I think some existing tests will have to change:

input-text-maxlength.html should be done using user input
input-implicit-length-limit.html should be done using user input
max-length.html should be done using user input
input-appearance-maxlength.html first test can go, second test is fine
input-text-paste-maxlength.html look at each seperate case

I am posting the patch early in case I am doing something that is not desired, I am doing duplicated work that is done in radar and to get some idea of how to test and change tests. I'll try tomorrow to rework above tests as I described and compare to FF3 and Opera, I'll post a new patch then.
Cheers,

Rob.

[Darin Adler]

(In reply to comment #3)
> I do wonder if there is also work in Radar for this?

No, there's not.

[Darin Adler]

You mention Firefox 3 and Opera. But what about MSIE?

[Rob Buis]

Hi Darin,

(In reply to comment #5)
> You mention Firefox 3 and Opera. But what about MSIE?
> 

I tried IE6, it reacts the same on the testcase. I tried setting the value from javascript and it does not take into account maxLength either, so to me that confirms that constraining only happens on user input.
Cheers,

Rob.

[Darin Adler]

Comment on attachment 24091 [details]
Work in progress

The patch looks OK.

Note that constrainValue does more than just limit length. It also cuts off strings at the first control character (characters in the range 0-8 or 9-31). We'll need some kind of test about that too to ensure that it's OK to remove that behavior. I'm worried that we will now allow control characters in a field that weren't allowed before. Maybe these characters could even be used to submit misleading forms?

I'm not sure that defaultEventHandler code does the right thing when we already have a string that's too long. Do the other browsers allow you to overtype characters if the string is too long? It's a little strange that typing over a letter won't behave the same as deletion of that letter followed by typing a new letter. We should make test cases that cover edge cases like that.

WIth the reduced use of the constrainValue function, we might want to rename it and remove the overload that doesn't take a maximum length argument. It's not really a generic "constrain value" function any more if it's really just enforcing maximum length after typing. Maybe the entire body of the isBeforeTextInsertedEvent() if statement should be moved into a function and that could be merged with constrainValue?

[Rob Buis]

Hi Darin,

(In reply to comment #7)
> (From update of attachment 24091 [details] [edit])
> I'm not sure that defaultEventHandler code does the right thing when we already
> have a string that's too long. Do the other browsers allow you to overtype
> characters if the string is too long? It's a little strange that typing over a
> letter won't behave the same as deletion of that letter followed by typing a
> new letter. We should make test cases that cover edge cases like that.

Oops, I just checked and overtyping is not allowed, it is allowed to use backspace to reduce text length even if after that operation it is still too long for maxlength. I'll update my patch to reflect this.
Cheers,

Rob.

[Rob Buis]

Created attachment 24683 [details]
Further work in progress

This one does not allow increasing text length of input text that is already beyond maxlength (due to value attribute or setValue call).
Cheers,

Rob

[Peter Kasting]

*** Bug 28001 has been marked as a duplicate of this bug. ***

[Peter Kasting]

The duped bug 28001 contains in its first two comments a fairly exhaustive description of the desired behavior.

[Kent Tamura]

Created attachment 39594 [details]
Another proposed patch

This patch was made by "git diff" without "--binary" because a patch with "--binary" made Internal Server Error of bugs.webkit.org

[Darin Adler]

Comment on attachment 39594 [details]
Another proposed patch

I don't think the names sanitizeValue and constrainValue are expressive enough, especially without comments. These names do not make it clear that constrainValue does everything sanitizeValue does, but also enforces a maximum length based on the maxlength property.

Better names would be ones that make it clear what the difference is between these two closely related functions. One is for user input and another is for values that don't come from user input. Perhaps sanitizeValue and sanitizeUserInputValue? Or constrainValue and constrainUserInputValue? These are not as elegant as your names, but they are clearer, and I think they would make the code easier to understand.

> -// large. However, due to http://bugs.webkit.org/show_bugs.cgi?id=14536 things
> +// large. However, due to http://bugs.webkit.org/show_bug.cgi?id=14536 things

If you're going to fix the URL I also suggest making it https since http just redirects.

>  void InputElement::setValueFromRenderer(InputElementData& data, InputElement* inputElement, Element* element, const String& value)
>  {
> -    // Renderer and our event handler are responsible for constraining values.
> -    ASSERT(value == inputElement->constrainValue(value) || inputElement->constrainValue(value).isEmpty());

I'm a bit concerned about eliminating this assertion. Your change log doesn't give any explanation for why it's correct to remove it. Should we change the assertion to use sanitizeValue instead of constrainValue? Also, under what circumstances do we get a string that violates maxlength from the renderer?

> +String HTMLInputElement::sanitizeValue(const String& proposedValue) const
> +{
> +    if (isTextField())
> +        return InputElement::sanitizeValue(this, proposedValue);
> +    return proposedValue;
> +}

Since InputElement::constrainValue already checks isTextField and does nothing if it is false, I do not think we need an additional check here. Lets keep this consistent with the existing constrainValue function, which does not add an isTextField check.

> -    input->setValueFromRenderer(input->constrainValue(text()));
> +    // We don't need to apply constrainValue() or sanitizeValue() here.
> +    // InputElement::handleBeforeTextInsertedEvent() does enough job.
> +    input->setValueFromRenderer(text());

The phrase "does enough job" is not grammatical and not specific enough. I would write:

    // We don't need to call InputElement's constrainValue or sanitizeValue
    // function here because InputElement::handleBeforeTextInsertedEvent has
    // already called constrainValue.

I'd also like to understand if that's really true. Is there no code path where that is violated?

I'm going to say review+ because I do think this is OK as-is, but I do have a few concerns. Feel free to clear the review flag if you plan to do a new patch.

[Kent Tamura]

Comment on attachment 39594 [details]
Another proposed patch

I'll update the patch.

[Kent Tamura]

Created attachment 39632 [details]
Another proposed patch (rev.2)

[Kent Tamura]

(In reply to comment #13)
> Better names would be ones that make it clear what the difference is between
> these two closely related functions. One is for user input and another is for
> values that don't come from user input. Perhaps sanitizeValue and
> sanitizeUserInputValue? Or constrainValue and constrainUserInputValue? These
> are not as elegant as your names, but they are clearer, and I think they would
> make the code easier to understand.

I renamed constrainValue() to sanitizeUserInputValue().

> > -// large. However, due to http://bugs.webkit.org/show_bugs.cgi?id=14536 things
> > +// large. However, due to http://bugs.webkit.org/show_bug.cgi?id=14536 things
> 
> If you're going to fix the URL I also suggest making it https since http just
> redirects.

Done.

> > -    // Renderer and our event handler are responsible for constraining values.
> > -    ASSERT(value == inputElement->constrainValue(value) || inputElement->constrainValue(value).isEmpty());
> 
> I'm a bit concerned about eliminating this assertion. Your change log doesn't
> give any explanation for why it's correct to remove it. Should we change the
> assertion to use sanitizeValue instead of constrainValue? Also, under what
> circumstances do we get a string that violates maxlength from the renderer?

Yeah, we should have assertion for sanitizeValue(), and RenderTextControlSingleLine.subtreeHasChanged() should call sanitizeValue().

> > +String HTMLInputElement::sanitizeValue(const String& proposedValue) const
> > +{
> > +    if (isTextField())
> > +        return InputElement::sanitizeValue(this, proposedValue);
> > +    return proposedValue;
> > +}
> 
> Since InputElement::constrainValue already checks isTextField and does nothing
> if it is false, I do not think we need an additional check here. Lets keep this
> consistent with the existing constrainValue function, which does not add an
> isTextField check.

Right.  This is redundant for now.  I added it because we need to implement sanitizing rules for non-text types.

[Eric Seidel (no email)]

Comment on attachment 39632 [details]
Another proposed patch (rev.2)

I think tkent is a committer these days, but given his cq=? request, I'm happy to let the commit-queue handle this too.  tkent if you're a committer you can add yourself to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py and set cq+ yourself whenever you want to.  You also of course can land patches yourself.  Completely up to you.

[WebKit Commit Bot]

Comment on attachment 39632 [details]
Another proposed patch (rev.2)

Rejecting patch 39632 from commit-queue.

Patch https://bugs.webkit.org/attachment.cgi?id=39632 from bug 21271 failed to download and apply.

[Eric Seidel (no email)]

Unfortunately svn-apply isn't smart enough to handle this patch (bug 28456).  We would need either an svn-create-patch patch, bug 28456 fixed, or for you to land this yourself. :)

[Kent Tamura]

Created attachment 39668 [details]
Another proposed patch (rev.3)


---
 29 files changed, 423 insertions(+), 1624 deletions(-)

[Kent Tamura]

Eric, I'm not a committer.

svn-create-patch doesn't work for git, right?
I reposted the patch via bugzilla-tool though I'm not sure if it makes difference from the previous patch.

[Eric Seidel (no email)]

Yeah, bugzilla-tool won't help us here, it just uses git diff under the covers too.  We have to fix bug 28456, or make a patch from an svn repository. :(  Someone (possibly me) could land this manually, although we might need a "git format-patch" patch for that.

[Kent Tamura]

Created attachment 39670 [details]
Another proposed patch (rev.4)

remade by "git format-patch"

[Eric Seidel (no email)]

Comment on attachment 39670 [details]
Another proposed patch (rev.4)

LGTM based largely on Darin's previous approval.  I can land this by hand using "git apply" for you in a minute.

[WebKit Commit Bot]

Committed r48449: <http://trac.webkit.org/changeset/48449>

[Eric Seidel (no email)]

Hum... Sorry for the confusing comment.  This wasn't actually committed by the bot.  I just used the bot's checkout to do the commit.

[Eric Seidel (no email)]

I manually used "git am --3way" to apply the git patch and did the commit using bugzilla-tool land-diff from the bot's checkout.