Bug 40518 – [GTK] Last Hangul letter is typed again when a composition is finished with mouse press

• Component: Platform
• Status: RESOLVED FIXED

[Joone Hur]

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.

[Joone Hur]

Created attachment 58545 [details]
demo_video

[Joone Hur]

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.

[Joone Hur]

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.

[Joone Hur]

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.

[Joone Hur]

I filed the same bug of chromium for this issue as follows,
http://code.google.com/p/chromium/issues/detail?id=50485

[James Su]

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.

[James Su]

And, mouse press may not always cause input method commit, so it's necessary to reset m_preventContextCommit somewhere.

[Joone Hur]

Created attachment 63137 [details]
Proposed Patch3

All logic was moved inside WebKit/gtk.

[Martin Robinson]

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

[Joone Hur]

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.

[Joone Hur]

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.

[Martin Robinson]

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

[Joone Hur]

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

[Martin Robinson]

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.

[Joone Hur]

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

[Joone Hur]

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

[Martin Robinson]

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

[WebKit Commit Bot]

Comment on attachment 64121 [details]
Proposed Patch5

Rejecting patch 64121 from review queue.

[e-mail] 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 [e-mail] to request a commit-queue restart.  After restart the commit-queue will correctly respect your reviewer rights.

[Martin Robinson]

I think we just need to wait for the commit-queue to restart (supposed to happen every 10 cycles).

[Joone Hur]

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

[Martin Robinson]

Comment on attachment 64121 [details]
Proposed Patch5

Trying the r+ one more time!

[WebKit Commit Bot]

Comment on attachment 64121 [details]
Proposed Patch5

Clearing flags on attachment: 64121

Committed r65209: <http://trac.webkit.org/changeset/65209>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.