Summary: | Special characters can be inserted in text field having reached maxlength | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | macservo | ||||||||
Component: | HTML Editing | Assignee: | 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
macservo
2008-06-11 08:01:47 PDT
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. It's the same with Japanse characters. I can insert as many Japanese characters as I want regardless of the maxlength. 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. 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 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. Created attachment 94276 [details]
proposed fix
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 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 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
> 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.
The failing test seems wrong. I'll update the patch to include corrected results. Created attachment 94282 [details]
proposed fix
(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 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 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 on attachment 94282 [details] proposed fix Clearing flags on attachment: 94282 Committed r87008: <http://trac.webkit.org/changeset/87008> All reviewed patches have been landed. Closing bug. 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 (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. Skipped it for WK2 in r87212. |