Bug 19479 – Special characters can be inserted in text field having reached maxlength

• Component: HTML Editing
• Status: RESOLVED FIXED

I can add as many ô or ñ as i  want to a textfield with maxlength set to 1.
Same for the ^ and ~ as single character.

[Julien Chaffraix]

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.

[Viktor Kelemen]

It's the same with Japanse characters.
I can insert as many Japanese characters as I want regardless of the maxlength.

[Alexey Proskuryakov]

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.

[Viktor Kelemen]

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

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

Created attachment 94276 [details]
proposed fix

[Ryosuke Niwa]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

The failing test seems wrong. I'll update the patch to include corrected results.

[Alexey Proskuryakov]

Created attachment 94282 [details]
proposed fix

[Kent Tamura]

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

[Kent Tamura]

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.

[Alexey Proskuryakov]

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!

[WebKit Commit Bot]

Comment on attachment 94282 [details]
proposed fix

Clearing flags on attachment: 94282

Committed r87008: <http://trac.webkit.org/changeset/87008>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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

[Kent Tamura]

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

[Alexey Proskuryakov]

Skipped it for WK2 in r87212.