Bug 19479

Summary: Special characters can be inserted in text field having reached maxlength
Product: WebKit Reporter: macservo
Component: HTML EditingAssignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, dglazkov, jchaffraix, kelemen.viktor, rniwa, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
proposed fix
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-02
none
proposed fix none

Description macservo 2008-06-11 08:01:47 PDT
I can add as many ô or ñ as i  want to a textfield with maxlength set to 1.
Same for the ^ and ~ as single character.
Comment 1 Julien Chaffraix 2008-12-18 14:40:59 PST
Confirmed. Try typing in such a text field:

<input maxlength="5"/>

All characters seems to be taken into account for the maxlength but special characters like  ^, ~ or ¨ or accentuated character can go be inserted even if maxlength is reached.
Comment 2 Viktor Kelemen 2010-05-12 02:43:58 PDT
It's the same with Japanse characters.
I can insert as many Japanese characters as I want regardless of the maxlength.
Comment 3 Alexey Proskuryakov 2010-05-12 14:02:32 PDT
I'm not sure what the correct behavior would be though. Clearly, we can't limit the number of unconfirmed characters, because that would prevent reaching maxLength when typing Japanese. But one can submit a form with unconfirmed text, and magically shortening it at the time of submission would be wrong, too.
Comment 4 Viktor Kelemen 2010-05-12 18:46:31 PDT
Firefox 3.6.3 does not limit the unconfirmed characters but when you changed them to confirmed it cuts the characters which are out of the limit.
I recorded this behavior, http://www.youtube.com/watch?v=veQcD-_PG2o
Comment 5 Alexey Proskuryakov 2011-05-20 13:16:07 PDT
The behavior in Safari 5 (and ToT) is different from what I remember.

We now truncate the inline hole when confirming it, but only if the input has been under the limit prior to typing. If it was exactly at the limit, then we select the inline hole, but fail to delete it.
Comment 6 Alexey Proskuryakov 2011-05-20 15:07:11 PDT
Created attachment 94276 [details]
proposed fix
Comment 7 Ryosuke Niwa 2011-05-20 15:18:12 PDT
Comment on attachment 94276 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=94276&action=review

> Source/WebCore/ChangeLog:18
> +        (WebCore::CompositeEditCommand::replaceTextInNode):
> +        Avoid hitting an assertion below, now that we can get here with empty text.
> +
> +        * editing/TypingCommand.cpp: (WebCore::TypingCommand::insertText): There is still work to do
> +        even if beforetextinput removed all text from the event. At the very least, we should delete
> +        the current selection.

I don't quite understand how these changes fix the bug.  Could you explain in the change what caused the bug and how your change fixes it?

> LayoutTests/platform/mac/editing/input/maxlength.html:15
> +    var testInput = document.getElementById("test");
> +    testInput.focus();

I don't think we normally indent the entire script like this.
Comment 8 WebKit Review Bot 2011-05-20 15:31:39 PDT
Comment on attachment 94276 [details]
proposed fix

Attachment 94276 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8720450

New failing tests:
fast/forms/input-number-commit-valid-only.html
Comment 9 WebKit Review Bot 2011-05-20 15:31:43 PDT
Created attachment 94279 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 10 Alexey Proskuryakov 2011-05-20 15:38:04 PDT
> I don't quite understand how these changes fix the bug.  Could you explain in the change what caused the bug and how your change fixes it?

So here is what happens when inline input is confirmed by the user:
- Kotoeri calls -[WebHTMLView insertText:], which gets us in Editor::confirmComposition() when there is a composition.
- the text that used to be in inline hole gets unmarked and selected;
- Editor::insertTextForConfirmedComposition() is called, dispatching TextEventInputComposition;
- its default handler proceeds to insert text as normal;
- TypingCommand::insertText() dispatches BeforeTextInsertedEvent;
- when BeforeTextInsertedEvent is dispatched, its default handler makes sure that maxlength is honored, possibly shortening BeforeTextInsertedEvent::text();
- if the text is now empty, we used to bail out here;
- text insertion goes on otherwise, replacing selection with text from BeforeTextInsertedEvent.

So, the problem is that no replacement happened if there was nothing to insert after dispatching BeforeTextInsertedEvent.

The question for this review is whether it's fine to process with the full text insertion process here, only fixing the two places I fixed, or if we still want to short circuit it somehow. I think that this bug demonstrates that short circuiting is problematic.
Comment 11 Alexey Proskuryakov 2011-05-20 15:53:47 PDT
The failing test seems wrong. I'll update the patch to include corrected results.
Comment 12 Alexey Proskuryakov 2011-05-20 15:54:05 PDT
Created attachment 94282 [details]
proposed fix
Comment 13 Kent Tamura 2011-05-20 17:06:02 PDT
(In reply to comment #10)
> > I don't quite understand how these changes fix the bug.  Could you explain in the change what caused the bug and how your change fixes it?
> 
> So here is what happens when inline input is confirmed by the user:
> - Kotoeri calls -[WebHTMLView insertText:], which gets us in Editor::confirmComposition() when there is a composition.
> ...

This explanation is very helpful.  It would be great if ChangeLog contains it.
Comment 14 Kent Tamura 2011-05-20 17:06:50 PDT
Comment on attachment 94282 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=94282&action=review

> LayoutTests/fast/forms/script-tests/input-number-commit-valid-only.js:22
>  document.execCommand('SelectAll', false, null);
>  document.execCommand('InsertText', false, '');
>  input.blur();
> -shouldBe('input.value', '"512"');
> +shouldBe('input.value', '""');

Oh, it's right.  An empty string is valid string for type=number.
Comment 15 Alexey Proskuryakov 2011-05-20 17:50:58 PDT
Comment on attachment 94282 [details]
proposed fix

I'm not really happy with putting such a long explanation in ChangeLog - the bug is not going anywhere, so it will still be available here.

Thank you for the review!
Comment 16 WebKit Commit Bot 2011-05-20 18:15:56 PDT
Comment on attachment 94282 [details]
proposed fix

Clearing flags on attachment: 94282

Committed r87008: <http://trac.webkit.org/changeset/87008>
Comment 17 WebKit Commit Bot 2011-05-20 18:16:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Kent Tamura 2011-05-24 15:10:22 PDT
(In reply to comment #18)
> The test added by this patch is failing on WebKit2:
> http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(WebKit2%20Tests)/r87196%20(11943)/platform/mac/editing/input/maxlength-pretty-diff.html

Yeah, the test uses textInputController, which is not available in WebKit2.
Comment 20 Alexey Proskuryakov 2011-05-24 15:10:29 PDT
Skipped it for WK2 in r87212.