Bug 21271

Summary: Maxlength limits value
Product: WebKit Reporter: Christian Pedersen <christian>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, eric, pkasting, tkent
Priority: P2 Keywords: InRadar
Version: 525.x (Safari 3.1)   
Hardware: All   
OS: All   
URL: http://software.hixie.ch/utilities/js/live-dom-viewer/saved/190
Bug Depends on:    
Bug Blocks: 27454    
Attachments:
Description Flags
Demo of problem
none
Work in progress
darin: review-
Further work in progress
none
Another proposed patch
darin: review+, tkent: commit-queue-
Another proposed patch (rev.2)
darin: review+, commit-queue: commit-queue-
Another proposed patch (rev.3)
none
Another proposed patch (rev.4) eric: review+, eric: commit-queue-

Description Christian Pedersen 2008-10-01 02:51:28 PDT
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.
Comment 1 Christian Pedersen 2008-10-01 04:40:53 PDT
Created attachment 23980 [details]
Demo of problem
Comment 2 Mark Rowe (bdash) 2008-10-01 14:15:13 PDT
<rdar://problem/6261976>
Comment 3 Rob Buis 2008-10-04 13:17:00 PDT
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.
Comment 4 Darin Adler 2008-10-04 13:29:06 PDT
(In reply to comment #3)
> I do wonder if there is also work in Radar for this?

No, there's not.
Comment 5 Darin Adler 2008-10-04 13:29:43 PDT
You mention Firefox 3 and Opera. But what about MSIE?
Comment 6 Rob Buis 2008-10-05 08:48:45 PDT
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.
Comment 7 Darin Adler 2008-10-11 14:45:01 PDT
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?
Comment 8 Rob Buis 2008-10-26 11:43:58 PDT
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.
Comment 9 Rob Buis 2008-10-26 11:49:58 PDT
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
Comment 10 Peter Kasting 2009-08-25 09:55:04 PDT
*** Bug 28001 has been marked as a duplicate of this bug. ***
Comment 11 Peter Kasting 2009-08-25 09:56:32 PDT
The duped bug 28001 contains in its first two comments a fairly exhaustive description of the desired behavior.
Comment 12 Kent Tamura 2009-09-15 04:02:59 PDT
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
Comment 13 Darin Adler 2009-09-15 09:10:15 PDT
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.
Comment 14 Kent Tamura 2009-09-15 18:33:21 PDT
Comment on attachment 39594 [details]
Another proposed patch

I'll update the patch.
Comment 15 Kent Tamura 2009-09-15 20:17:26 PDT
Created attachment 39632 [details]
Another proposed patch (rev.2)
Comment 16 Kent Tamura 2009-09-15 20:24:22 PDT
(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.
Comment 17 Eric Seidel (no email) 2009-09-16 15:42:30 PDT
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.
Comment 18 WebKit Commit Bot 2009-09-16 16:48:04 PDT
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.
Comment 19 Eric Seidel (no email) 2009-09-16 16:51:50 PDT
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. :)
Comment 20 Kent Tamura 2009-09-16 17:15:17 PDT
Created attachment 39668 [details]
Another proposed patch (rev.3)


---
 29 files changed, 423 insertions(+), 1624 deletions(-)
Comment 21 Kent Tamura 2009-09-16 17:19:53 PDT
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.
Comment 22 Eric Seidel (no email) 2009-09-16 17:22:57 PDT
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.
Comment 23 Kent Tamura 2009-09-16 17:28:38 PDT
Created attachment 39670 [details]
Another proposed patch (rev.4)

remade by "git format-patch"
Comment 24 Eric Seidel (no email) 2009-09-16 17:35:01 PDT
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.
Comment 25 WebKit Commit Bot 2009-09-16 19:13:58 PDT
Committed r48449: <http://trac.webkit.org/changeset/48449>
Comment 26 Eric Seidel (no email) 2009-09-16 19:27:35 PDT
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.
Comment 27 Eric Seidel (no email) 2009-09-16 19:28:11 PDT
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.