REGRESSION: 'maxLength' of input text field doesn't work for CJK characters

• Component: Forms
• Status: RESOLVED FIXED

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

Created attachment 29562 [details]
Test file

This is also tracked in Chromium as crbug.com/10612

[Ojan Vafai]

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

[Kent Tamura]

I'm investigating this.

[Kent Tamura]

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.

[Kent Tamura]

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.

[Kent Tamura]

Created attachment 38662 [details]
Proposed patch

I have no idea how to write automated tests for this...

[Eric Seidel (no email)]

I thought that we had some fake IME tests somewhere... Ojan, do you remember?

[Ojan Vafai]

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.

[Hironori Bono]

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

[Kent Tamura]

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.

[Ojan Vafai]

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

[Kent Tamura]

(In reply to comment #12)

Ok, I understand.  I'll upload the patch with tests.

[Kent Tamura]

Created attachment 38909 [details]
Proposed patch (rev.2)

run-webkit-tests and check-webkit-style passed.

[David Levin]

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()"

[Kent Tamura]

Created attachment 38910 [details]
Proposed patch (rev.3)

- Fix a typo in a comment
- Use numGraphemeClusters() instead of .length()

[Eric Seidel (no email)]

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

[Kent Tamura]

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.

[Eric Seidel (no email)]

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.

[Kent Tamura]

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

[Eric Seidel (no email)]

Comment on attachment 38974 [details]
Proposed patch (rev.4)

Clearing flags on attachment: 38974

Committed r48011: <http://trac.webkit.org/changeset/48011>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.