867433 - Compositing Korean letter can be committed twice with iBus

Status: NEW

(Core :: DOM: UI Events & Focus Handling, defect, P2)

Description

[Joone Hur]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130307122351

Steps to reproduce:

 Launch Firefox turnk on Ubuntu 12.10 
2. Go to https://www.google.com/advanced_search
3. Turn on Korean IME (iBus1.4.1)
4. Set focus at "all these words:" text input field
5. Type "rksk" (가나)
6. Mouse press at the end of text or at "this exact wording or phrase:" text input field


Actual results:

The last letter "나" is entered again at the focused position.


Expected results:

"가나" should be entered at the current text input field.

Comment 1

[Joone Hur]

Attachment: Proposed Patch

Comment 2

[Channy Yun [:channy]]

I confirmed this bug as a Korean l10n owner. Please review it as an aspect of GTK2 widget code.

Comment 3

[Karl Tomlinson (:karlt)]

Joone, are you able to attach stack traces at the two times when the composition is committed, please?

Comment 4

[Joone Hur]

Here is the stack trace when the second commit signal is triggered.
http://paste.fedoraproject.org/11317/

Comment 5

[Karl Tomlinson (:karlt)]

That stack shows an org.freedesktop.IBus CommitText signal causing a GtkIMContext commit signal handled by OnCommitCompositionNative().  That all seems reasonable.

What I'd like to know is what else is causing the text to be input (i.e. the first time) and why this problem doesn't happen with other GTK+ apps.

Comment 6

[Joone Hur]

Chromium had the same problem. This is the reason why IBus sends a commit signal again:
https://code.google.com/p/chromium/issues/detail?id=50485#c9

Comment 7

[Joone Hur]

@karl Do you have any other suggestions?

Comment 8

[Karl Tomlinson (:karlt)]

I think I understand what is happening now thanks:

  Some input methods don't commit the current preedit string during
  gtk_im_context_reset() and so Gecko chooses to commit the string itself.

  For those input methods that do commit the preedit string during _reset(),
  Gecko uses mIgnoreNativeCompositionEvent to try to suppress the input
  method's commit during _reset().

  With ibus, however, the commit from the reset happens asynchronously after
  _reset() has returned.  When the commit is received,
  mIgnoreNativeCompositionEvent has already been set to false.

Can the setting of mIgnoreNativeCompositionEvent to false be delayed to some
later point?

It is already being reset to false in preedit-start and I assume we want to
keep that.  Can one of the other places that set it to false be removed or
changed?
Chrome also resets suppress_next_commit before calling
gtk_im_context_filter_keypress().  I don't know whether that is necessary or
not.

What is the order of events received from iBus?
Is preedit-end received before or after commit?

Comment 9

[Joone Hur]

(In reply to Karl Tomlinson (:karlt) from comment #8)
> I think I understand what is happening now thanks:
> 
>   Some input methods don't commit the current preedit string during
>   gtk_im_context_reset() and so Gecko chooses to commit the string itself.
> 
>   For those input methods that do commit the preedit string during _reset(),
>   Gecko uses mIgnoreNativeCompositionEvent to try to suppress the input
>   method's commit during _reset().
> 
>   With ibus, however, the commit from the reset happens asynchronously after
>   _reset() has returned.  When the commit is received,
>   mIgnoreNativeCompositionEvent has already been set to false.
> 
> Can the setting of mIgnoreNativeCompositionEvent to false be delayed to some
> later point?

Yes, it can. As you know, we set mIgnoreNativeCompositionEvent to false in nsGtkIMModule::OnFocusChangeInGecko and nsGtkIMModule::OnEndCompositionNative.
But, instead of setting false in two methods, if we set mIgnoreNativeCompositionEvent to false in nsGtkIMModule::OnCommitCompositionNative, it works like my patch. 

However, it seems to be required to set mIgnoreNativeCompositionEvent to false in OnFocusChangeInGecko and OnEndCompositionNative according to the comments.

> 
> It is already being reset to false in preedit-start and I assume we want to
> keep that.  Can one of the other places that set it to false be removed or
> changed?

nsGtkIMModule::OnCommitCompositionNative is good for setting it to false as follows:

void
nsGtkIMModule::OnCommitCompositionNative(GtkIMContext *aContext,
                                         const gchar *aUTF8Char)
{
...
    if (ShouldIgnoreNativeCompositionEvent()) {
        mIgnoreNativeCompositionEvent = false; // added
        return;
    }


> Chrome also resets suppress_next_commit before calling
> gtk_im_context_filter_keypress().  I don't know whether that is necessary or
> not.
> 
> What is the order of events received from iBus?
> Is preedit-end received before or after commit?

This is a call stack:
OnStartCompositionCallback preedit_start 
DispatchCompositionStart
OnChangeCompositionCallback preedit_change 
OnChangeCompositionCallback preedit_change 
ResetIME
OnFocusChangeInGecko
OnFocusChangeInGecko
OnChangeCompositionCallback preedit_change 
OnEndCompositionCallback preedit_end 
OnEndCompositionNative 
OnCommitCompositionCallback commit 

As you can see, preedit_end is received before commit.

Anyway, if we do not need to set mIgnoreNativeCompositionEvent to false in OnFocusChangeInGecko and OnEndCompositionNative, the patch will be simpler.

Comment 10

[Joone Hur]

Attachment: Simple Patch

This patch allows not to set mIgnoreNativeCompositionEvent to false in OnFocusChangeInGecko and OnEndCompositionNative methods.

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 756997 [details] [diff] [review]
Simple Patch

>@@ -965,6 +953,7 @@ nsGtkIMModule::OnCommitCompositionNative(GtkIMContext *aContext,
>     }
> 
>     if (ShouldIgnoreNativeCompositionEvent()) {
>+        mIgnoreNativeCompositionEvent = false;
>         return;
>     }

I can imagine that this works fine with iBus, because it will dispatch a "commit", but some other IMEs don't dispatch a commit, and I don't know whether mIgnoreNativeCompositionEvent will be reset to false in time for them.

I assume that you needed to add this line to make iBus work as expected?
As this code won't be run for some IMEs, this suggests that mIgnoreNativeCompositionEvent still needs to be reset at some other point in the code.

I'm guessing that there is an appropriate place to reset mIgnoreNativeCompositionEvent that will work for all IMEs.

Comment 12

[Joone Hur]

(In reply to Karl Tomlinson (:karlt) from comment #11)
> Comment on attachment 756997 [details] [diff] [review]
> Simple Patch
> 
> >@@ -965,6 +953,7 @@ nsGtkIMModule::OnCommitCompositionNative(GtkIMContext *aContext,
> >     }
> > 
> >     if (ShouldIgnoreNativeCompositionEvent()) {
> >+        mIgnoreNativeCompositionEvent = false;
> >         return;
> >     }
> 
> I can imagine that this works fine with iBus, because it will dispatch a
> "commit", but some other IMEs don't dispatch a commit, and I don't know
> whether mIgnoreNativeCompositionEvent will be reset to false in time for
> them.
> 
> I assume that you needed to add this line to make iBus work as expected?

Yes, the line is required to make iBus work.

> As this code won't be run for some IMEs, this suggests that
> mIgnoreNativeCompositionEvent still needs to be reset at some other point in
> the code.
>
> I'm guessing that there is an appropriate place to reset
> mIgnoreNativeCompositionEvent that will work for all IMEs.

Yes, the simple patch may only work for iBus, so I will try to find an appropriate place to reset mIgnoreNativeCompositionEvent.

By the way, what IMEs should be supported?

Comment 13

[Karl Tomlinson (:karlt)]

Ideally we'd support all IMEs that use GtkIMContext as intended.
Masayuki can you suggest some IMEs to test with, please?

Comment 14

[Hiroyuki Ikezoe (:hiro)]

I'd say iBus, uim and Fcitx.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Sorry for the delay, I'm very busy until next week's weekend.

I'm afraid to change the management of mIgnoreNativeCompositionEvent because it's hacky and it's been tested with a lot of environment.

I should explain the background why we need to do it.

First of all, our editor (nsEditor) includes composition string as its content and doesn't support modifying its another part while there is composition string. Therefore, when nsEditor receives an action (operation) which will cause modifying its content, it needs to commit the current composition *synchronously*.

However, some IMEs doesn't commit composition string at calling gtk_im_context_reset() as karlt mentioned. Some commit the composition string asynchronously, some others never commit the composition string.

So, ideally, nsEditor should support modifying content even there is composition string. I have a plan for such improvement. But unfortunately, I'll work on it late this year or later.

So, we need hacky fix for *now*, indeed.

IIRC, some IMEs commit composition string when the IM context loses focus. Isn't it usable for this case? I mean that if we can fix by calling gtk_im_context_focus_out() and gtk_im_context_focus_in() after calling gtk_im_context_reset() at nsGtkIMModule::ResetIME(), probably, it's the safest way for now.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Anyway, we should redesign the management of composition using structure or nested class line nsTextStore, though.

Comment 17

[Joone Hur]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (Offline 6/11 - 6/17) from comment #15)
> 
> IIRC, some IMEs commit composition string when the IM context loses focus.
> Isn't it usable for this case? I mean that if we can fix by calling
> gtk_im_context_focus_out() and gtk_im_context_focus_in() after calling
> gtk_im_context_reset() at nsGtkIMModule::ResetIME(), probably, it's the
> safest way for now.

This problem is not fixed by calling gtk_im_context_focus_out() and gtk_im_context_focus_in() at ResetIME().

Comment 18

[Joone Hur]

Attachment: compromise patch

There seems no appropriate place to reset mIgnoreNativeCompositionEvent,so mIgnoreCompositionCommit is used to ignore the commit signal.

Comment 19

[Karl Tomlinson (:karlt)]

Chrome resets its suppress_next_commit before calling gtk_im_context_filter_keypress().  Is there a reason why that approach does not work in Gecko?

I fear that there may not be a preedit-start signal from all IMEs, as some may keep the preedit string after gtk_im_context_reset().

Comment 20

[Joone Hur]

(In reply to Karl Tomlinson (:karlt) from comment #19)
> Chrome resets its suppress_next_commit before calling
> gtk_im_context_filter_keypress().  Is there a reason why that approach does
> not work in Gecko?

It works in Gecko, so I will update the patch soon.

Comment 21

[Joone Hur]

Attachment: updated patch

This patch allows to set mIgnoreCompositionCommit to false in nsGtkIMModule::OnKeyEvent.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

I worry about:

1. How does hand-writing system or something which are not related to keyboard work with IME?
2. Just adding new flag mess the code.

For #2, I think that we should make a nested class of nsGtkIMModule which manages flags for gtk_im_context_reset().

For example:

class ContextResetState
{
public:
    ResetState() :
        mIgnoreUpdate(false), mIgnoreCommit(false)
    {
    }

    void WillResetContext()
    {
        mIgnoreUpdate = true;
        mIgnoreCommit = true;
    }

    void OnFocusChangeInGecko(bool aFocus)
    {
        // If new editor gets focus but the last composition hasn't been
        // committed by resetting context, the composition should be available
        // on the new focused editor.
        if (!aFocus || !mIgnoreUpdate) {
            return;
        }
        mIgnoreUpdate = false;
        // TODO: Start composition here.
    }

    void OnKeyPress()
    {
        mIgnoreUpdate = mIgnoreCommit = false;
    }

    void OnStartComposition()
    {
        mIgnoreUpdate = mIgnoreCommit = false;
    }

    bool OnUpdateComposition()
    {
        bool accept = !mIgnoreUpdate;
        mIgnoreUpdate = mIgnoreCommit = false;
        return accept;
    }

    bool OnEndComposition()
    {
        bool accept = !mIgnoreCommit;
        mIgnoreUpdate = mIgnoreCommit = false;
        return accept;
    }

private:
    bool mIgnoreUpdate;
    bool mIgnoreCommit;
};

Of course, each method should be implemented in .cpp actually with PR_LOG().

Additionally, please use -U8 for making diff file. See https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 23

[Karl Tomlinson (:karlt)]

Isn't the asynchrous commit signal a problem if it can arrive after a
gtk_im_context_reset() call on change in cursor position?  How does this work
in other GTK apps (or in the native GTK file save dialog)?  Wouldn't the
string get inserted at the new cursor position (instead of the intended old
cursor position)?

Perhaps there is a way to force iBus to wait for pending commits?

Chromium seems to manage with just one suppress_next_commit variable, but I don't know what bugs, if any, result from that.

Comment 24

[Joone Hur]

(In reply to Karl Tomlinson (:karlt) from comment #23)
> Isn't the asynchrous commit signal a problem if it can arrive after a
> gtk_im_context_reset() call on change in cursor position? 

Yes, that is the problem.

 How does this work
> in other GTK apps (or in the native GTK file save dialog)?  Wouldn't the
> string get inserted at the new cursor position (instead of the intended old
> cursor position)?

Native Gtk applications have a similar problem with iBus.

> Perhaps there is a way to force iBus to wait for pending commits?

I heard that this is a iBus bug.

> 
> Chromium seems to manage with just one suppress_next_commit variable, but I
> don't know what bugs, if any, result from that.

suppress_next_commit variable was added to fix this bug in Chromium.
Here is the bug: https://code.google.com/p/chromium/issues/detail?id=50485

Comment 25

[Joone Hur]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (Offline 6/11 - 6/17) from comment #22)
> I worry about:
> 
> 1. How does hand-writing system or something which are not related to
> keyboard work with IME?

"This issue may happen even without mouse click, for example when the input focus is reset programmatically (by JavaScript code)" (from the Chromium bug comment)

So we need to check it.

> 2. Just adding new flag mess the code.
> 
> For #2, I think that we should make a nested class of nsGtkIMModule which
> manages flags for gtk_im_context_reset().

That's a good idea. I will add this.

Comment 26

[Karl Tomlinson (:karlt)]

(In reply to Joone Hur from comment #24)
> (In reply to Karl Tomlinson (:karlt) from comment #23)
> > How does this work
> > in other GTK apps (or in the native GTK file save dialog)?  Wouldn't the
> > string get inserted at the new cursor position (instead of the intended old
> > cursor position)?
> 
> Native Gtk applications have a similar problem with iBus.

I'm sorry.  I mis-understood this to be a bug due to Gecko's special handling during reset.

If iBus has similar problems even in native GTK apps, then this is an iBus bug.
I don't want to try to work around it in Gecko because there seems to be a high risk of causing problems with IMEs that use GtkIMContext as intended.

Comment 27

[Joone Hur]

Actually, this bug only happens with typing Korean letters, because typing Chinese and Japanese needs an enter in order to complete the current composition.

I'm not sure if iBus maintainers could fix this problem, because this is an old bug unresolved. Other browser engines such as Chromium, WebKitGtk+ already fixed this bug using the same workaround way.

I hope that Firefox would fix this problem soon. All Korean Linux users have had the same problem with Firefox for a long time.

Comment 28

[Karl Tomlinson (:karlt)]

Is iBus the primary input method used for Korean on Linux?

If we workaround the bug in Gecko:

1.  The variable should not be reset during the commit message.  If it needs to be reset here then there is a problem because other IMs won't send the commit message.  Note also that chromium no longer resets during commit.
https://src.chromium.org/viewvc/chrome?revision=195916&view=revision

2.  I would like to see a solution with only one variable.
Chromium seems to have similar behaviour to Gecko, and uses only one variable.
If this is not possible in Gecko, then I'd like to know why not.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (Offline 6/11 - 6/17) from comment #15)

> I'm afraid to change the management of mIgnoreNativeCompositionEvent because
> it's hacky and it's been tested with a lot of environment.

We can change the behavior if the situations that required the addition of that variable are tested.

Comment 29

[fujiwara]

I think this is a bug in both firefox and chrome.
Some IMEs likes Hangul need to commit the preedit with mouse click but other not. E.g. ibus-anthy has three options of commit, clear and keep.
But firefox and chrome forcibly commit the preedit. It should be chosen by each IME.
And the committing text with reset signal causes problems.

Now I'll override the firefox behavior with IBus clients and this problem also will be fixed:
https://github.com/ibus/ibus/issues/1980#issuecomment-414588836

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Moving all open keyboard/IME handling bugs to DOM: UI Events & Focus Handling component.