1. Go to http://www.google.com/advanced_search?hl=en 2. Enter 가 in the "all these words:" <input> element (still preedit state) 3. Move focus to the "this exact wording or phrase:" <input> element by mouse down 4. Composition string is also entered to the "this exact wording or phrase:" <input> element When we are in the middle of Hangul composition in the current focused <input> element, if we select another <input> element, the composition string is also entered to the newly focused <input> element. Currently, IME commit signal is emitted after a mouse down signal so we need to filter IME commit signal when the focused node is changed.
Created attachment 58545 [details] demo_video
Created attachment 58546 [details] Proposed Patch This patch filters IME commit signal for not entering composition string into the newly focused input element. The composition string can be only entered in the previous input element before the user clicks another input element.
Created attachment 61900 [details] Proposed patch2 It fixes two problems: * A composition character can be entered at the previous editing position and a new editing position. * A composition can not be confirmed after a mouse press fires.
Created attachment 61944 [details] Korean Hangul composition problem This video shows the problems of Korean Hangul composition I mentioned. Chrome browser also has a similar composition problem.
I filed the same bug of chromium for this issue as follows, http://code.google.com/p/chromium/issues/detail?id=50485
IMO, it's not necessary to modify WebCore. All logic should be inside WebKit/gtk. You probably want to move the mouse press related logic into WebKitWebViewClass's button press handler.
And, mouse press may not always cause input method commit, so it's necessary to reset m_preventContextCommit somewhere.
Created attachment 63137 [details] Proposed Patch3 All logic was moved inside WebKit/gtk.
Comment on attachment 63137 [details] Proposed Patch3 Nice patch! I think this is very close, but I have a couple suggestions. > + // If this signal fires during a mousepress event when we are in the middle > + // of a composition, skip this 'commit' because the composition is already confirmed. > + if (client->preventContextCommitWhenComposition()) { > + client->clearPreventContextCommit(); > + return; > + } As discussed in IRC, maybe it would be safer to avoid resetting the value here and doing it in handleInputMethodKeydown. That way if, for some reason, the IM context doesn't do anything during the mouse event things will still work out okay. > + // When a mouse press fires, the commit signal happens during a composition. > + // In this case, if the focusd node is changed, the commit signal happens in a diffrent node. > + // We need to confirm the current compositon before handling the commit signal. Spelling nit: focusd -> focused The comment should probably be more explicit about the fate of the next commit signal too: "We need to confirm the current compositon before handling the commit signal." should be "We need to confirm the current compositon and ignore the next commit signal." > + GOwnPtr<gchar> newPreedit(0); > + gtk_im_context_get_preedit_string(priv->imContext, &newPreedit.outPtr(), 0, 0); > + > + if (g_utf8_strlen(newPreedit.get(), -1)) { > + m_preventContextCommit = true; > + targetFrame->editor()->confirmComposition(); > + gtk_im_context_reset(priv->imContext); > + } > +} Doesn't it make more sense to use targetFrame->editor()->hasComposition() here?I may be wrong about that, but if not, please use that instead. WebKit style isto use early returns as well. So perhaps something like: if (!targetFrame->editor()->hasComposition()) return; targetFrame->editor()->confirmComposition(); m_preventNextCompositionCommit = true; gtk_im_context_reset(priv->imContext); > bool treatContextCommitAsKeyEvent() { return m_treatContextCommitAsKeyEvent; } > + bool preventContextCommitWhenComposition() { return m_preventContextCommit; } > + void clearPreventContextCommit() { m_preventContextCommit = false; } > + bool m_preventContextCommit; The naming here is a little bit off. Perhaps m_preventNextCompositionCommit, preventNextCompositionCommit(), and acceptNextCompositionCommit()? > gboolean result = frame->eventHandler()->handleMousePressEvent(platformEvent); > + // Handle the IM context when a mouse press fires > + WebKit::EditorClient* client = static_cast<WebKit::EditorClient*>(core(webView)->editorClient()); > + client->handleInputMethodMousePress(); There's no need to store this value, so just do: static_cast<WebKit::EditorClient*>(core(webView)->editorClient())->handleInputMethodMousePress();
Created attachment 63811 [details] Proposed Patch4 I updated the patch according to Martin's review except applying the code below. if (!targetFrame->editor()->hasComposition()) return; Because, the above code can't check whether a composition completes when a mouse press fires at a different editing position.
In addition, I have tested other IMEs such as SCIM 1.4.9, nabi 0.99.3 in Ubuntu9.10. They are working basically without this patch. However, * no compositionend, textInput event fired when a mouse press fires during a composition. * can't move the mouse cursor in the text box during a composition. i.e. If we try to move the mouse cursor ahead from the composition character, the composition is just finished without moving the mouse cursor. Therefore, I think this patch is also useful for other IMEs. In case of iBus, it seems a bug of iBus, so we need to avoid it through this patch.
> In case of iBus, it seems a bug of iBus, so we need to avoid it through this patch. I'm a little confused by this statement. Do you mean that this patch fixes iBus or doesn't work with iBus. iBus is meant as the replacement, for SCIM, so I think we can't ignore it. :)
(In reply to comment #12) > > In case of iBus, it seems a bug of iBus, so we need to avoid it through this patch. > > I'm a little confused by this statement. Do you mean that this patch fixes > iBus or doesn't work with iBus. iBus is meant as the replacement, for SCIM, > so I think we can't ignore it. :) Yes, this patch can fix the iBus bug, and it also allows WebKitGtk to confirm the IME composition when a user clicks the composition text.
This patch looks good to me, except that m_preventNextCompositionCommit = false; should probably happen at the beginning of EditorClient::handleInputMethodKeydown(KeyboardEvent* event). Many keystrokes result in a composition commit during time they are passed to the IM context (such as ones for simple). You should be able to see this bug if you prevent the next composition and then switch to the context to simple and type any letter.
Created attachment 64121 [details] Proposed Patch5 I noticed that IME Commit signal is emitted in the middle of calling handleInputMethodKeydown(). Therefore, it seems more safe to move "m_preventNextCompositionCommit = false" at the beginning of EditorClient::handleInputMethodKeydown().
(In reply to comment #14) > This patch looks good to me, except that m_preventNextCompositionCommit = false; should probably happen at the beginning of EditorClient::handleInputMethodKeydown(KeyboardEvent* event). Many keystrokes result in a composition commit during time they are passed to the IM context (such as ones for simple). You should be able to see this bug if you prevent the next composition and then switch to the context to simple and type any letter. I couldn't see the bug you mentioned. Does "Many keystrokes" mean "typing fast" or "keeping pressing"? Could you explain more specific?
(In reply to comment #16) > I couldn't see the bug you mentioned. Does "Many keystrokes" > mean "typing fast" or "keeping pressing"? Could you explain > more specific? I actually meant exactly what you said in comment #15. It looks like you've fixed it with the most recent version of your patch too. This patch looks good to me now, so r+. Thanks for the revisions!
Comment on attachment 64121 [details] Proposed Patch5 Rejecting patch 64121 from review queue. mrobinson@webkit.org does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). Due to bug 30084 the commit-queue will require a restart after your change. Please contact eseidel@chromium.org to request a commit-queue restart. After restart the commit-queue will correctly respect your reviewer rights.
I think we just need to wait for the commit-queue to restart (supposed to happen every 10 cycles).
(In reply to comment #17) > (In reply to comment #16) > > I couldn't see the bug you mentioned. Does "Many keystrokes" > > mean "typing fast" or "keeping pressing"? Could you explain > > more specific? > > I actually meant exactly what you said in comment #15. It looks like you've fixed it with the most recent version of your patch too. This patch looks good to me now, so r+. Thanks for the revisions! Thanks, Martine, James for your review!! Many Koreans would like this fix. :-)
Comment on attachment 64121 [details] Proposed Patch5 Trying the r+ one more time!
Comment on attachment 64121 [details] Proposed Patch5 Clearing flags on attachment: 64121 Committed r65209: <http://trac.webkit.org/changeset/65209>
All reviewed patches have been landed. Closing bug.