Bug 46868 – Input Method inserts conversion candidates unexpectedly

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Kenichi Ishibashi]

Created attachment 69300 [details]
screenshot

Chromium bug: http://code.google.com/p/chromium/issues/detail?id=56759

Steps to repro:
1. Open a chat window in Gmail
2. Type a Japanese character sequence "てすと" [tesuto] with IME enabled.
3. Hit spaces a couple of times to go through conversion candidates.

Expected result:
Conversion candidates should not be input to the textbox while going through them, until hitting an Enter. The expected result is just "テスト" or "てすと" or similar.

Actual result:
A conversion candidate is input to the textbox every time you hit a space, resulting in an unexpectedly long string like "testtestテストてすと" (as shown in the attached image)

[Kenichi Ishibashi]

I've investigated deeply this issue. This issue will happen on the textarea that has an event listener which invokes style recalculation.

When IME composition changes, WebKit calls the event listener at first and style recalculation is scheduled. Then, the IME composition is processed by Editor::setComposition(). In the typical sequence, it uses CompositionEditCommand::replaceTextInNode() to set the current IME composition into the text node which is on the textarea node. The replaceTextInNode() function is composed of DeleteFromTextNodeCommand and InsertIntoTextNodeCommand, and after invoking DeleteFromTextNodeCommand, it invokes style recalculation if it needed (and it's needed in this case).

At this point, a state mismatch occurred. Although the content of the text node has changed by deletion command, the textarea node still keeps previous text and the m_valueMatchesRenderer flag, which indicates whether the text is up-to-date, does not update yet. As the result, when WebKit recalculation the style of the textarea, it inserts the IME candidate text into the text node unexpectedly.

I wrote a repro for this issue and I'll attach the file.

I'll post a trial patch to fix this issue and would like to ask some advice.

[Kenichi Ishibashi]

Created attachment 69302 [details]
HTML file to reproduce this issue

[Kenichi Ishibashi]

Created attachment 69303 [details]
A trial patch V0

[Kenichi Ishibashi]

This patch attempts to recalculate style before setting composition into the textarea. I'm not sure this is preferable way to fix this issue so please let me know if you have ideas for fixing the issue in more desirable way.

FYI, as the patch implies, when the selection is different from the previous selection, that is the case of IME composition has multiple segments, this issue will not appear.

Thanks,

[Hironori Bono]

Even though I'm not a WebKit reviewer (or a committer), I would encourage you to write a layout test with textInputController.setComposition(). Also, if this problem happens only when we use an IME, it may be more reasonable to call updateStyleIfNeeded() in Editor::setComposition().

Regards,

Hironori Bono

[Kenichi Ishibashi]

Hi Bono-san,

> Even though I'm not a WebKit reviewer (or a committer), I would encourage you to write a layout test with textInputController.setComposition(). 

Thank you for your suggestions. I didn't know textInputController.setComposition() and just wondering how we can write test for this issue. I'll check it could use to validate the issue.

> Also, if this problem happens only when we use an IME, it may be more reasonable to call updateStyleIfNeeded() in Editor::setComposition().

Yes, this problem happens only when we use an IME. However it is not always needed because it happens when an event listener which involves style recalculation. So I just put it at the place we actually need to do it. I'd like to ask further opinions including overall idea of fixing the problem.

[Kenichi Ishibashi]

Someone who can review or give a suggestion for this issue? I'd like to fix this issue as soon as possible.

[Kent Tamura]

The change will affect all platforms.  So we should not add "[Chromium]" to the bug title.

Is this issue Chromium-Mac specific? Does it occur on non-Mac or non-Chromium? Why?

(In reply to comment #1)
> When IME composition changes, WebKit calls the event listener at first and style recalculation is scheduled. Then, the IME composition is processed by Editor::setComposition(). In the typical sequence, it uses CompositionEditCommand::replaceTextInNode() to set the current IME composition into the text node which is on the textarea node. The replaceTextInNode() function is composed of DeleteFromTextNodeCommand and InsertIntoTextNodeCommand, and after invoking DeleteFromTextNodeCommand, it invokes style recalculation if it needed (and it's needed in this case).
> 
> At this point, a state mismatch occurred. Although the content of the text node has changed by deletion command, the textarea node still keeps previous text and the m_valueMatchesRenderer flag, which indicates whether the text is up-to-date, does not update yet. As the result, when WebKit recalculation the style of the textarea, it inserts the IME candidate text into the text node unexpectedly.

This explanation should be in ChangeLog.

[Kenichi Ishibashi]

Kent-san,

Thank you for taking a look at this.

> The change will affect all platforms.  So we should not add "[Chromium]" to the bug title.

Done.

> Is this issue Chromium-Mac specific? Does it occur on non-Mac or non-Chromium? Why?

As I investigated roughly, for Safari, it fires event listeners after the IME composition text is updated so the mismatch of the text node doesn't occurred. I'm not sure about Chromium on Windows because I don't have build environment on Chromium on Windows for now. I'm preparing build environment but it will take some time.

> This explanation should be in ChangeLog.

I'll add the explanation in ChangeLog in the next patch.

I'm also trying to write a test for this issue, but I couldn't find the way to input key event to input method in DRT by JavaScript. Does anyone know the way to do this? Am I supposed to implement such function into DRT (and is it possible?)

I sought the API for dispatching key events to input method and found WKSendKeyEventToTSM() in WebKitSystemInterface.h, but it was disabled on SnowLeopard by changeset 41679. So it seems that there is no official way to dispatch key events to input method via DRT (I also try to use eventSender.keyDown() but it doesn't pass the event to input method). Anyway, I'll continue to seek the way to test for this issue.

[Kenichi Ishibashi]

Created attachment 71857 [details]
A trial patch V1

[Kenichi Ishibashi]

Hi,

I've updated the patch following bono-san's suggestion and including a test. To add a test, I introduced setComposition() to TextInputController class in DRT chromium port. I'm not sure this is a good way, so please let me know if I do something wrong.

Regards,

[Kent Tamura]

Comment on attachment 71857 [details]
A trial patch V1

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

> LayoutTests/platform/chromium/fast/text/chromium-mac-duplicate-ime-composition-expected.txt:1
> +This page tests that the composition text of an input method does not insert oddly into a textarea which have an event handler that changes the style of the textarea, as reported in the issue 46868. When you input text into the following textarea with an input method, the composition text should not insert oddly.

I think the first sentence is the test description and the last sentence is an instruction of manual test when we open the test with a browser.
"oddly" is ok for the description, but it's not ok for the instruction.  Please write what result is wrong concretely.

> WebKitTools/DumpRenderTree/chromium/TextInputController.cpp:250
> +    // Sends a keydown event with key code = 0xE5 to emulate input method behavior.

> WebKitTools/DumpRenderTree/chromium/TextInputController.cpp:254
> +    keyDown.windowsKeyCode = 0x5E; // VKEY_PROCESSKEY

Which is correct; 0xE5 or 0x5E?

[Kenichi Ishibashi]

Comment on attachment 71857 [details]
A trial patch V1

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

Hi kent-san,

Thank you very much your rapid response. I'll post revised patch in a minute.

>> LayoutTests/platform/chromium/fast/text/chromium-mac-duplicate-ime-composition-expected.txt:1
>> +This page tests that the composition text of an input method does not insert oddly into a textarea which have an event handler that changes the style of the textarea, as reported in the issue 46868. When you input text into the following textarea with an input method, the composition text should not insert oddly.
> 
> I think the first sentence is the test description and the last sentence is an instruction of manual test when we open the test with a browser.
> "oddly" is ok for the description, but it's not ok for the instruction.  Please write what result is wrong concretely.

Revised the description to include wrong and correct result.

>> WebKitTools/DumpRenderTree/chromium/TextInputController.cpp:254
>> +    keyDown.windowsKeyCode = 0x5E; // VKEY_PROCESSKEY
> 
> Which is correct; 0xE5 or 0x5E?

0xE5 is correct. Sorry for confusion.

[Kenichi Ishibashi]

Created attachment 71859 [details]
A trial patch V2

[Kent Tamura]

Comment on attachment 71859 [details]
A trial patch V2

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

- The test doesn't pass with Chromium test_shell.  So we had better add the test to LayoutTest/platform/chromium/test_expectations.txt.

I still have some comments on the patch.  However, I think it's ok to commit this.

> LayoutTests/platform/chromium/fast/text/chromium-mac-duplicate-ime-composition.html:5
> +      var console = document.getElementById('console');

nit: We usually use 4-space indent for JavaScript code.

> WebKitTools/DumpRenderTree/chromium/TextInputController.cpp:254
> +    keyDown.windowsKeyCode = 0xE5; // VKEY_PROCESSKEY

We should add VKEY_PROCESSKEY to webkit/support/webkit_support.h of Chromium.

[Kenichi Ishibashi]

Kent-san,

Thank you for reviewing!

(In reply to comment #15)
> nit: We usually use 4-space indent for JavaScript code.

Thank you letting me know that. I'll use 4-space indent next time.

> > WebKitTools/DumpRenderTree/chromium/TextInputController.cpp:254
> > +    keyDown.windowsKeyCode = 0xE5; // VKEY_PROCESSKEY
> 
> We should add VKEY_PROCESSKEY to webkit/support/webkit_support.h of Chromium.

I'll add it to Chromium soon.

Regards,

[WebKit Commit Bot]

Comment on attachment 71859 [details]
A trial patch V2

Clearing flags on attachment: 71859

Committed r70555: <http://trac.webkit.org/changeset/70555>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dimitri Glazkov (Google)]

(In reply to comment #16)
> Kent-san,
> 
> Thank you for reviewing!
> 
> (In reply to comment #15)
> > nit: We usually use 4-space indent for JavaScript code.
> 
> Thank you letting me know that. I'll use 4-space indent next time.
> 
> > > WebKitTools/DumpRenderTree/chromium/TextInputController.cpp:254
> > > +    keyDown.windowsKeyCode = 0xE5; // VKEY_PROCESSKEY
> > 
> > We should add VKEY_PROCESSKEY to webkit/support/webkit_support.h of Chromium.
> 
> I'll add it to Chromium soon.
> 
> Regards,

You've neglected to add the test to test expectations.
http://trac.webkit.org/changeset/70563.