Bug 21271

Created attachment 23980 [details] Demo of problem

<rdar://problem/6261976>

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.

(In reply to comment #3) > I do wonder if there is also work in Radar for this? No, there's not.

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

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 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?

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.

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

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

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

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 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 on attachment 39594 [details] Another proposed patch I'll update the patch.

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

(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 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 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.

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. :)

Created attachment 39668 [details] Another proposed patch (rev.3) --- 29 files changed, 423 insertions(+), 1624 deletions(-)

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.

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.

Created attachment 39670 [details] Another proposed patch (rev.4) remade by "git format-patch"

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.

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

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.

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.