[GTK] IMContextWrapper shouldn't dispatch redundant "keydown" and "keyup" events

RESOLVED FIXED in Firefox 61

Status

()

enhancement
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({inputmethod})

Trunk
mozilla61
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 wontfix, firefox61 fixed)

Details

Attachments

(2 attachments)

As far as I've tested, Linux's IME behaves odd in these days.

I tested on Ubuntu 17.10 with both ibus and fcitx.

With both cases, we may receive two pairs of GDK_KEY_PRESS and GDK_KEY_RELEASE events. Then, gtk_im_context_filter_keypress() returns TRUE for first pair, but does nothing actually. The second pair is handled as usual. It seems that the preceding pair is used for checking something. E.g., whether application handles key events before IME or something.

Anyway, with current implementation, we dispatch redundant eKeyDown event and eKeyUp event for the preceding dummy key event pair and mark them as "processed by IME" since gtk_im_context_filter_keypress() returns TRUE.

Oddly, the event order could be both of the following:

1. GDK_KEY_PRESS
2. GDK_KEY_PRESS
3. GDK_KEY_RELEASE
4. GDK_KEY_RELEASE

vs.

1. GDK_KEY_PRESS
2. GDK_KEY_RELEASE
3. GDK_KEY_PRESS
4. GDK_KEY_RELEASE

I usually see the former order. However, sometimes I saw the latter order.

And first GDK_KEY_PRESS may be consumed by IME as usual. So, simply ignoring the first key event is wrong approach.

Actual logs are here:
> I/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900 OnKeyEvent(aCaller=0x0x7fb4e202c800, aEvent(0x0x7fb4bd977ac0): { type=GDK_KEY_PRESS, keyval=a, unicode=0x61 }, aKeyboardEventWasDispatched=false), mMaybeInDeadKeySequence=false, mCompositionState=NotComposing, current context=0x0x7fb4d9e43e20, active context=0x0x7fb4d9e43e20, 
> I/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900 SetCursorPosition(aContext=0x0x7fb4d9e43e20), mCompositionTargetRange={ mOffset=4294967295, mLength=4294967295 }mSelection={ mOffset=4294967295, Length()=0, mWritingMode=Horizontal }
> E/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900   SetCursorPosition(), FAILED, mCompositionTargetRange and mSelection are invalid
> D/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900   MaybeDispatchKeyEventAsProcessedByIME(), keydown or keyup event is dispatched
> D/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900   OnKeyEvent(), succeeded, filterThisEvent=true (isFiltered=true, mFallbackToKeyEvent=false), mCompositionState=NotComposing, mMaybeInDeadKeySequence=false
> I/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900 OnKeyEvent(aCaller=0x0x7fb4e202c800, aEvent(0x0x7fb4bd977ac0): { type=GDK_KEY_PRESS, keyval=a, unicode=0x61 }, aKeyboardEventWasDispatched=false), mMaybeInDeadKeySequence=false, mCompositionState=NotComposing, current context=0x0x7fb4d9e43e20, active context=0x0x7fb4d9e43e20, 
> I/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900 OnCommitCompositionNative(aContext=0x0x7fb4d9e43e20), current context=0x0x7fb4d9e43e20, active context=0x0x7fb4d9e43e20, commitString="a", mProcessingKeyEvent=0x0x7fb4bd977ac0, IsComposingOn(aContext)=false
> I/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900   OnCommitCompositionNative(), we'll send normal key event
> D/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900   OnKeyEvent(), succeeded, filterThisEvent=false (isFiltered=true, mFallbackToKeyEvent=true), mCompositionState=NotComposing, mMaybeInDeadKeySequence=false
> I/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900 OnSelectionChange(aCaller=0x0x7fb4e202c800, aIMENotification={ mSelectionChangeData={ mOffset=1, Length()=0, mReversed=false, mWritingMode=Horizontal, mCausedByComposition=false, mCausedBySelectionEvent=false, mOccurredDuringComposition=false } }), mCompositionState=NotComposing, mIsDeletingSurrounding=false, mRetrieveSurroundingSignalReceived=false
> I/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900 OnKeyEvent(aCaller=0x0x7fb4e202c800, aEvent(0x0x7fb4bd977ac0): { type=GDK_KEY_RELEASE, keyval=a, unicode=0x61 }, aKeyboardEventWasDispatched=false), mMaybeInDeadKeySequence=false, mCompositionState=NotComposing, current context=0x0x7fb4d9e43e20, active context=0x0x7fb4d9e43e20, 
> I/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900 SetCursorPosition(aContext=0x0x7fb4d9e43e20), mCompositionTargetRange={ mOffset=4294967295, mLength=4294967295 }mSelection={ mOffset=1, Length()=0, mWritingMode=Horizontal }
> D/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900   MaybeDispatchKeyEventAsProcessedByIME(), keydown or keyup event is dispatched
> D/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900   OnKeyEvent(), succeeded, filterThisEvent=true (isFiltered=true, mFallbackToKeyEvent=false), mCompositionState=NotComposing, mMaybeInDeadKeySequence=false
> I/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900 OnKeyEvent(aCaller=0x0x7fb4e202c800, aEvent(0x0x7fb4bd977ac0): { type=GDK_KEY_RELEASE, keyval=a, unicode=0x61 }, aKeyboardEventWasDispatched=false), mMaybeInDeadKeySequence=false, mCompositionState=NotComposing, current context=0x0x7fb4d9e43e20, active context=0x0x7fb4d9e43e20, 
> D/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900   OnKeyEvent(), succeeded, filterThisEvent=false (isFiltered=false, mFallbackToKeyEvent=false), mCompositionState=NotComposing, mMaybeInDeadKeySequence=false
But important and good hint is, all native key events are always same instance. Although I'll investigate what happens when I press another key while pressing a key.
According to the ibus's source code, ibus may post key event to engine process at first time. Then, returns true for preventing application handles the key event. After that, if the engine doesn't process the key event, same key event will be sent to the application again (held by gdk_event_copy(), therefore, the address is always same). However, it seems that this is ibus specific issue. Can we distinguish if active IM is ibus?
Summary: [GTK] IMContextWrapper shouldn't dispatch redundant "keydown" and "keyup" events during composition → [GTK] IMContextWrapper shouldn't dispatch redundant "keydown" and "keyup" events
This is not ibus specific bug. Fcitx also posts GdkEventKey to different process.

Both of them store original event instance with using gdk_event_copy() (i.e., grabbing it with increasing its refcount) before dispatching the event to another process. Then, both of them adds a bit flag, 1 << 24 (defined as IBUS_HANDLED_MASK and FcitxKeyState_HandledMask), to GdkEventKey::state and return true for gtk_im_context_filter_keypress(). Unfortunately, even if it's handled synchronously, the bit flag is set by both of them. So, we cannot know if the event is dispatched to different process.  If we can know the refcount of GdkEventKey, we can know that, though.
Comment on attachment 8957759 [details]
Bug 1443421 - part 1: Make IMContextWrapper not dispatch eKeyDown and eKeyUp event if the native key event is being handled by other IME process

https://reviewboard.mozilla.org/r/226736/#review232702

::: widget/gtk/IMContextWrapper.h:105
(Diff revision 2)
> +    // TODO: Typically, new IM comes every several years.  And now, our code
> +    //       becomes really IM behavior dependent.  So, perhaps, we need prefs
> +    //       to control related flags for IM developers.
> +    enum class IMContextID : uint8_t
> +    {
> +      eFcitx,

nits: indent is 4

::: widget/gtk/IMContextWrapper.h:116
(Diff revision 2)
> +
> +    static const char* GetIMContextIDName(IMContextID aIMContextID)
> +    {
> +        switch (aIMContextID) {
> +            case IMContextID::eFcitx:
> +              return "eFcitx";

nits: indent is 4

::: widget/gtk/IMContextWrapper.cpp:280
(Diff revision 2)
> +    // https://github.com/ibus/ibus/blob/86963f2f94d1e4fc213b01c2bc2ba9dcf4b22219/client/gtk2/ibusimcontext.c#L520-L537
> +    if (!env) {
> +        return false;
> +    }
> +    if (!env[0] ||
> +        !nsCRT::strcmp(env, "0") ||

Since you already check whether env is null, you should use strcmp instead of nsCRT::strcmp.  (ex. bug 798840)

Also, you might have to nsDependentString instead of PL_str or str\*.  (If possible, we shouldn't use str* functions due to unsafe.)

```
nsDependentString envStr(env)
if (envStr.IsEmpty() ||
    envStr.EqualLiteral("0") ||
...
```

::: widget/gtk/IMContextWrapper.cpp:299
(Diff revision 2)
> +    const char* env = PR_GetEnv(aEnv);
> +    if (!env) {
> +        return false;
> +    }
> +    if (!env[0] ||
> +        !nsCRT::strcmp(env, "0") ||

Use strcmp or nsDepedentString since env isn't always null
Attachment #8957759 - Flags: review?(m_kato) → review+
Comment on attachment 8957760 [details]
Bug 1443421 - part 2: IMContextWrapper should dispatch eKeyDown or eKeyUp event as "processed by IME" even when IM sent some signals without sending key event again

https://reviewboard.mozilla.org/r/226738/#review232744
Attachment #8957760 - Flags: review?(m_kato) → review+
Added eScim to IMContextID because I succeeded to create test environment with Debian. (But looks like that we don't need to support it aggressively since it's too old and not supported by mozc on Debian.)
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/edc1455e7082
part 1: Make IMContextWrapper not dispatch eKeyDown and eKeyUp event if the native key event is being handled by other IME process r=m_kato
https://hg.mozilla.org/integration/autoland/rev/6afa399e604a
part 2: IMContextWrapper should dispatch eKeyDown or eKeyUp event as "processed by IME" even when IM sent some signals without sending key event again r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/b93dcde52457
part 1: Make IMContextWrapper not dispatch eKeyDown and eKeyUp event if the native key event is being handled by other IME process r=m_kato
https://hg.mozilla.org/integration/autoland/rev/11c1e1afaaca
part 2: IMContextWrapper should dispatch eKeyDown or eKeyUp event as "processed by IME" even when IM sent some signals without sending key event again r=m_kato
https://hg.mozilla.org/mozilla-central/rev/b93dcde52457
https://hg.mozilla.org/mozilla-central/rev/11c1e1afaaca
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Can this ride the trains to 61 or is there something we need to uplift?
Flags: needinfo?(masayuki)
It's not necessary since we don't dispatch keydown nor keyup event during composition in default settings yet.
Flags: needinfo?(masayuki)
You need to log in before you can comment on or make changes to this bug.