Bug 40518 - [GTK] Last Hangul letter is typed again when a composition is finished with mouse press
Summary: [GTK] Last Hangul letter is typed again when a composition is finished with m...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL: http://www.google.com/advanced_search...
Keywords:
Depends on: 43020
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-12 01:10 PDT by Joone Hur
Modified: 2010-08-11 19:09 PDT (History)
8 users (show)

See Also:


Attachments
demo_video (253.53 KB, application/octet-stream)
2010-06-12 01:33 PDT, Joone Hur
no flags Details
Proposed Patch (4.40 KB, patch)
2010-06-12 02:21 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
Proposed patch2 (8.38 KB, patch)
2010-07-17 21:04 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
Korean Hangul composition problem (491.81 KB, video/ogg)
2010-07-19 06:48 PDT, Joone Hur
no flags Details
Proposed Patch3 (5.62 KB, patch)
2010-07-30 19:37 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
Proposed Patch4 (5.58 KB, patch)
2010-08-07 02:08 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
Proposed Patch5 (5.82 KB, patch)
2010-08-11 09:19 PDT, Joone Hur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joone Hur 2010-06-12 01:10:37 PDT
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.
Comment 1 Joone Hur 2010-06-12 01:33:07 PDT
Created attachment 58545 [details]
demo_video
Comment 2 Joone Hur 2010-06-12 02:21:48 PDT
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.
Comment 3 Joone Hur 2010-07-17 21:04:23 PDT
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.
Comment 4 Joone Hur 2010-07-19 06:48:27 PDT
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.
Comment 5 Joone Hur 2010-07-28 00:02:58 PDT
I filed the same bug of chromium for this issue as follows,
http://code.google.com/p/chromium/issues/detail?id=50485
Comment 6 James Su 2010-07-29 11:54:39 PDT
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.
Comment 7 James Su 2010-07-29 11:59:33 PDT
And, mouse press may not always cause input method commit, so it's necessary to reset m_preventContextCommit somewhere.
Comment 8 Joone Hur 2010-07-30 19:37:43 PDT
Created attachment 63137 [details]
Proposed Patch3

All logic was moved inside WebKit/gtk.
Comment 9 Martin Robinson 2010-08-05 16:58:59 PDT
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();
Comment 10 Joone Hur 2010-08-07 02:08:53 PDT
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.
Comment 11 Joone Hur 2010-08-07 03:29:34 PDT
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.
Comment 12 Martin Robinson 2010-08-09 08:47:37 PDT
> 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. :)
Comment 13 Joone Hur 2010-08-10 08:21:44 PDT
(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.
Comment 14 Martin Robinson 2010-08-10 08:33:38 PDT
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.
Comment 15 Joone Hur 2010-08-11 09:19:44 PDT
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().
Comment 16 Joone Hur 2010-08-11 09:39:01 PDT
(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?
Comment 17 Martin Robinson 2010-08-11 15:31:35 PDT
(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 18 WebKit Commit Bot 2010-08-11 15:35:14 PDT
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.
Comment 19 Martin Robinson 2010-08-11 15:50:50 PDT
I think we just need to wait for the commit-queue to restart (supposed to happen every 10 cycles).
Comment 20 Joone Hur 2010-08-11 18:15:29 PDT
(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 21 Martin Robinson 2010-08-11 18:17:39 PDT
Comment on attachment 64121 [details]
Proposed Patch5

Trying the r+ one more time!
Comment 22 WebKit Commit Bot 2010-08-11 19:09:49 PDT
Comment on attachment 64121 [details]
Proposed Patch5

Clearing flags on attachment: 64121

Committed r65209: <http://trac.webkit.org/changeset/65209>
Comment 23 WebKit Commit Bot 2010-08-11 19:09:56 PDT
All reviewed patches have been landed.  Closing bug.