Bug 46868 - Input Method inserts conversion candidates unexpectedly
Summary: Input Method inserts conversion candidates unexpectedly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-29 19:53 PDT by Kenichi Ishibashi
Modified: 2010-10-26 13:27 PDT (History)
4 users (show)

See Also:


Attachments
screenshot (28.85 KB, image/png)
2010-09-29 19:53 PDT, Kenichi Ishibashi
no flags Details
HTML file to reproduce this issue (414 bytes, text/html)
2010-09-29 20:00 PDT, Kenichi Ishibashi
no flags Details
A trial patch V0 (1.59 KB, patch)
2010-09-29 20:14 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
A trial patch V1 (8.75 KB, patch)
2010-10-26 02:57 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
A trial patch V2 (9.31 KB, patch)
2010-10-26 03:51 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenichi Ishibashi 2010-09-29 19:53:43 PDT
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)
Comment 1 Kenichi Ishibashi 2010-09-29 19:58:16 PDT
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.
Comment 2 Kenichi Ishibashi 2010-09-29 20:00:07 PDT
Created attachment 69302 [details]
HTML file to reproduce this issue
Comment 3 Kenichi Ishibashi 2010-09-29 20:14:47 PDT
Created attachment 69303 [details]
A trial patch V0
Comment 4 Kenichi Ishibashi 2010-09-29 20:15:56 PDT
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,
Comment 5 Hironori Bono 2010-09-30 03:17:04 PDT
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
Comment 6 Kenichi Ishibashi 2010-09-30 03:44:08 PDT
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.
Comment 7 Kenichi Ishibashi 2010-10-04 03:48:01 PDT
Someone who can review or give a suggestion for this issue? I'd like to fix this issue as soon as possible.
Comment 8 Kent Tamura 2010-10-12 00:55:38 PDT
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.
Comment 9 Kenichi Ishibashi 2010-10-18 00:49:32 PDT
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.
Comment 10 Kenichi Ishibashi 2010-10-26 02:57:50 PDT
Created attachment 71857 [details]
A trial patch V1
Comment 11 Kenichi Ishibashi 2010-10-26 03:00:18 PDT
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,
Comment 12 Kent Tamura 2010-10-26 03:07:51 PDT
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?
Comment 13 Kenichi Ishibashi 2010-10-26 03:50:46 PDT
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.
Comment 14 Kenichi Ishibashi 2010-10-26 03:51:16 PDT
Created attachment 71859 [details]
A trial patch V2
Comment 15 Kent Tamura 2010-10-26 03:59:53 PDT
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.
Comment 16 Kenichi Ishibashi 2010-10-26 04:18:53 PDT
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,
Comment 17 WebKit Commit Bot 2010-10-26 12:25:52 PDT
Comment on attachment 71859 [details]
A trial patch V2

Clearing flags on attachment: 71859

Committed r70555: <http://trac.webkit.org/changeset/70555>
Comment 18 WebKit Commit Bot 2010-10-26 12:25:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Dimitri Glazkov (Google) 2010-10-26 13:27:07 PDT
(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.