Bug 25253 - REGRESSION: 'maxLength' of input text field doesn't work for CJK characters
Summary: REGRESSION: 'maxLength' of input text field doesn't work for CJK characters
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P1 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-04-16 16:52 PDT by xlyuan
Modified: 2009-09-03 02:29 PDT (History)
6 users (show)

See Also:


Attachments
Test file (176 bytes, text/html)
2009-04-16 16:53 PDT, xlyuan
no flags Details
Proposed patch (6.12 KB, patch)
2009-08-27 01:10 PDT, Kent Tamura
tkent: review-
Details | Formatted Diff | Diff
Proposed patch (rev.2) (11.43 KB, patch)
2009-09-01 21:54 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.3) (11.43 KB, patch)
2009-09-01 22:23 PDT, Kent Tamura
eric: review-
Details | Formatted Diff | Diff
Proposed patch (rev.4) (11.41 KB, patch)
2009-09-03 02:02 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description xlyuan 2009-04-16 16:52:39 PDT
Build: WebKit-r42377 2009-04-15
   OS: Windows, not test on Mac

Other Browsers:
Chromium: Fail
Firefox3: Fail
     IE8: OK

Steps:
1. Launch Safari
2. Open the attached test file
   >>It includes <input type="text" maxLength="10" />
3. Try to type double byte characters, any CCJK language is fine
4. Observe

Result:
There is no limitation of the numbers when typing double byte characters

Expected:
Should only allow typing 10 characters

Notes:
When you copy/paste in this case, the length limits in 10 characters correctly
Comment 1 xlyuan 2009-04-16 16:53:32 PDT
Created attachment 29562 [details]
Test file
Comment 2 xlyuan 2009-04-16 16:54:43 PDT
This is also tracked in Chromium as crbug.com/10612
Comment 3 Ojan Vafai 2009-04-29 17:40:17 PDT
This works in Safari 3.2. I loaded the following page (has an input with maxLength=3):
http://www.plexode.com/cgi-bin/eval3.py#ht=%3Cinput%20type%3D%22text%22%20maxLength%3D%223%22%20%2F%3E

In Safari 3.2, I can type three Chinese characters. Then further typing brings up the IME, but hitting enter doesn't insert a character.

In today's nightly, I can type as many Chinese characters as I want. :)
Comment 4 Kent Tamura 2009-08-25 02:31:35 PDT
I'm investigating this.
Comment 5 Kent Tamura 2009-08-25 04:18:09 PDT
Bono-san gave me some information.
InputElement::handleBeforeTextInsertedEvent() makes this problem.

void InputElement::handleBeforeTextInsertedEvent(InputElementData& data, InputElement* inputElement, Document* document, Event* event)
{
    ASSERT(event->isBeforeTextInsertedEvent());

    // Make sure that the text to be inserted will not violate the maxLength.
    int oldLength = numGraphemeClusters(inputElement->value().impl());
    ASSERT(oldLength <= data.maxLength());
    int selectionLength = numGraphemeClusters(plainText(document->frame()->selection()->selection().toNormalizedRange().get()).impl());
    ASSERT(oldLength >= selectionLength);
    int maxNewLength = data.maxLength() - (oldLength - selectionLength);

    // Truncate the inserted text to avoid violating the maxLength and other constraints.
    BeforeTextInsertedEvent* textEvent = static_cast<BeforeTextInsertedEvent*>(event);
    textEvent->setText(constrainValue(inputElement, textEvent->text(), maxNewLength));
}

selectionLength represents the length to be removed by the next text insertion.  However, frame()->selection()->selection() can be IME's pre-edit text, which is outside of the current inputElement->value().  So maxNewLength can be larger than expectation.
Comment 6 Kent Tamura 2009-08-27 00:34:19 PDT
A pre-edit text can be outside the InputElment::value() because IME inputs change subtree without firing BeforeTextInsertedEvents.  RenderTextControlSingleLine::subtreeHasChanged() calls input->constrainValue(text()).  It makes a mismatch of InputElement::value() and visible text.
Comment 7 Kent Tamura 2009-08-27 01:10:58 PDT
Created attachment 38662 [details]
Proposed patch

I have no idea how to write automated tests for this...
Comment 8 Eric Seidel (no email) 2009-08-27 14:06:41 PDT
I thought that we had some fake IME tests somewhere... Ojan, do you remember?
Comment 9 Ojan Vafai 2009-08-27 14:13:53 PDT
The support for fake IME is only available on the Mac, so look at the tests in platform/mac/editing/input for examples. I'm not 100% sure what's there will actually work for this case since it really fakes out IME. It does do marked text, but when you accept a composition, it just puts in the plain latin characters that you typed. It never actually does the conversion to characters.
Comment 10 Hironori Bono 2009-08-27 17:40:52 PDT
(In reply to comment #9)
> The support for fake IME is only available on the Mac, so look at the tests in
> platform/mac/editing/input for examples. I'm not 100% sure what's there will
> actually work for this case since it really fakes out IME. It does do marked
> text, but when you accept a composition, it just puts in the plain latin
> characters that you typed. It never actually does the conversion to characters.

Right. We can call IME functions of WebKit through TextInputController on Mac and I assume we can write an automated text for this issue. (I'm not sure whether or not it becomes a text test or a pixel test, though.) Let me talk with him about the test.

By the way, do we need to port the above IME test feature to the test_shell of Chrome? If so, I'm happy to implement it. (Sorry for my off-the-wall question in advance.)

Regards,
Comment 11 Kent Tamura 2009-08-27 18:44:30 PDT
Thanks.
LayoutTests/platform/mac/editing/input/text-control-ime-input.html seems very helpful.

BTW, my fix strongly depends on the current behavior that IME input modifies DOM before text insertion events, and it is the root cause of this regression.
bug#25119 might change this behavior.  So we shouldn't commit this patch for now.
Comment 12 Ojan Vafai 2009-08-28 09:46:03 PDT
(In reply to comment #11)
> Thanks.
> LayoutTests/platform/mac/editing/input/text-control-ime-input.html seems very
> helpful.
> 
> BTW, my fix strongly depends on the current behavior that IME input modifies
> DOM before text insertion events, and it is the root cause of this regression.
> bug#25119 might change this behavior.  So we shouldn't commit this patch for
> now.

Bug 25119 is only about key events, not text insertion events. Also, that bug is not actively being worked on. I don't think you should block this bug on that one. As long as this patch comes with a layout test, then it will be the responsibility of whoever fixes bug 25119 to keep the layout test passing.
Comment 13 Kent Tamura 2009-09-01 21:52:24 PDT
(In reply to comment #12)

Ok, I understand.  I'll upload the patch with tests.
Comment 14 Kent Tamura 2009-09-01 21:54:07 PDT
Created attachment 38909 [details]
Proposed patch (rev.2)

run-webkit-tests and check-webkit-style passed.
Comment 15 David Levin 2009-09-01 22:19:21 PDT
I think there are better people to review this patch but I'll note that this is a typo that would be nice to fix "RednerTextControlSingleLine::subtreeHasChanged()"
Comment 16 Kent Tamura 2009-09-01 22:23:42 PDT
Created attachment 38910 [details]
Proposed patch (rev.3)

- Fix a typo in a comment
- Use numGraphemeClusters() instead of .length()
Comment 17 Eric Seidel (no email) 2009-09-03 00:52:02 PDT
Comment on attachment 38910 [details]
Proposed patch (rev.3)

These tests look like they can be dumpAsText() tests.  That's much preferred over render tree dump tests.

Why do we need to pass Document* now that we're passing Element*?  you can get element->document(). :)
Comment 18 Kent Tamura 2009-09-03 02:02:26 PDT
Created attachment 38974 [details]
Proposed patch (rev.4)

> These tests look like they can be dumpAsText() tests.  That's much preferred
> over render tree dump tests.

dumpAsText() didn't show text in <input>.
Bono-san gave me an advice of using document.getSelection().  I have updated the tests for document.getSelection().

> Why do we need to pass Document* now that we're passing Element*?  you can get
> element->document(). :)

That's right.  I removed the Document* parameter.
Comment 19 Eric Seidel (no email) 2009-09-03 02:18:26 PDT
Comment on attachment 38974 [details]
Proposed patch (rev.4)

This looks fine, but it seems input.textLength or input.value would have been simpler than doing a selectAll. :)

LGTM.
Comment 20 Kent Tamura 2009-09-03 02:27:41 PDT
(In reply to comment #19)
> This looks fine, but it seems input.textLength or input.value would have been
> simpler than doing a selectAll. :)

Actually DOM properties have been working well even without this patch.  This bug is a kind of mismatch of a DOM value and a renderer value.

Thank you for reviewing.
Comment 21 Eric Seidel (no email) 2009-09-03 02:29:45 PDT
Comment on attachment 38974 [details]
Proposed patch (rev.4)

Clearing flags on attachment: 38974

Committed r48011: <http://trac.webkit.org/changeset/48011>
Comment 22 Eric Seidel (no email) 2009-09-03 02:29:51 PDT
All reviewed patches have been landed.  Closing bug.