1083098 - Composition string doesn't appear after forcibly commit composition by moving focus on Linux

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

1. Load data:text/html,<input>
2. Type a character with IME (don't commit it)
3. Click outside of the <input>
4. Click the <input>
5. Type a character with IME

Actual result:
New composition string doesn't appear.

Expected result:
New composition string should appear.

I guess that this is a regression of bug 1065835, but I've not confirmed it yet because I cannot use IME with mozregression on Ubuntu.

My environment:
Ubuntu 14.04 (64bit), iBus.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, I cannot reproduce this bug with SCIM. That means that we have a bug at asynchronous commit.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

For a hack for bug 472635, we're checking a given IM context is same as ours when our native IME signal handlers are called. This causes this bug.

If IME is iBus, a call of gtk_im_context_reset() causes asynchronous commit of composition. Therefore, if it's called at moving focus from an <input> element to another element or its document whose IME enable state isn't "enabled", signals to commit composition come after moving focus. Then, GetContext() returns one of different context from its result before moving focus. This causes our signal handlers ignoring the commit events because we believe that the signals are buggy. Then, nsGtkIMModule loses a chance to end storing composition. So, it will be confused by following composing signals because it already has a composition.

So, we shouldn't ignore the delayed signals and end composition normally.

For solving this, we should check if a giving IM context matches *one* of our own IM context. If it matches one, we nsGtkIMModule should handle the signals and dispatch composition events. Then, in XP part, TextComposition instance handles/consumes the event and kills itself.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8505532 [details] [diff] [review]
Patch

Oops, sorry, this patch includes obsoleted approach.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

This is the right patch.

And I should've note that this patch is not good for maintenance because each method of nsGtkIMModule expects GetContext() always returns an expected IM context for current IM enable state. However, this is wrong at least for handling composition change and composition end in this case. Therefore, I think that each method should take GtkIMContext as its first argument even if it currently doesn't use it.

However, I guess that this patch should be uplift to Aurora. Therefore, I want to keep this patch small and simple as far as possible. So, my plan is, I'll create the refactoring patch only for m-c in another bug.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

Ugh. I'm very sorry for the spam. This is the right patch!

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

[Tracking Requested - why for this release]:

I confirmed that this is a regression of bug 1065835. The fix is simple and backing out the patches is not safer than fixing this.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

karlt: ping

Comment 8

[Karl Tomlinson (:karlt)]

I won't have time to review this today, but I should be able to try harder to understand sometime this week.

Which patch in bug 1065835 caused the change in behavior, and how did it cause the change in behavior?

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Karl Tomlinson (:karlt) from comment #8)
> Which patch in bug 1065835 caused the change in behavior, and how did it
> cause the change in behavior?

By the part.1 (attachment 8492119 [details] [diff] [review]), it makes synchronous commit or cancel composition for a request guaranteed in XP level. At the old behavior, it was guaranteed by nsGtkIMModule for Linux but this was removed by part.5 (attachment 8492125 [details] [diff] [review]).

It was not problem nsGtkIMModule not to handle _delayed_ native commit or cancel events because ShouldIgnoreNativeCompositionEvent() ignores the delayed events *after* synchronous commit was performed.

However, now, nsGtkIMModule does *not* perform synchronous commit at a request to commit or cancel composition. Therefore, even after handling a request, nsGtkIMModule keeps having _composing_ state. Therefore, it now needs to receive all delayed events for resetting its state. I.e., it should *not* ignore the delayed events, instead, it *needs* to handle them.

FYI: the delayed events should cause composition events. The events will be handled only by TextComposition. Then, TextComposition will kill itself. So, it's not okay to reset composition state at nsGtkIMModule handling a request.

The problem of this bug is that the delayed events may be fired *after* focus in Gecko is moved. I.e., nsGtkIMModule::GetContext() may return different content if the focus move causes different IME enabled state (e.g., "enabled" -> "disabled", "enabled" -> "password").

In the STR of comment 0, this is what occurs:

1. When <input> has focus, the IME state is "enabled" and GetContext() returns mContext
2. Before moving focus, nsGtkIMContext receives a request to commit composition and notifies native IME of it.
3. Gecko moves focus actually. Then, the IME state becomes "disabled". This makes GetContext() returns mDummyContext.
4. iBus sends some signals to _cancel_ composition. At this time, iBus passes a context which is mContext because the composition is handled on it.
5. Our signal receivers compares given context (same as mContext) and the result of GetContext() (mDummyContext). Then, this mismatch makes our handlers to ignore the signals.
6. nsGtkIMModule keeps having composition which is already finished in iBus already (at #4-5)

FYI: This is what occurred in old build:

1. When <input> has focus, the IME state is "enabled" and GetContext() returns mContext
2. Before moving focus, nsGtkIMContext receives a request to commit composition and notifies native IME of it _and commit composition internally_.
3. Gecko moves focus actually. Then, the IME state becomes "disabled". This makes GetContext() returns mDummyContext.
4. iBus sends some signals to _cancel_ composition. At this time, iBus passes a context which is mContext because the composition is handled on it.
5. Our signal receivers compares given context (same as mContext) and the result of GetContext() (mDummyContext). Then, this mismatch makes our handlers to ignore the signals but this is okay since the signals will be ignored by ShouldIgnoreNativeCompositionEvent().
6. nsGtkIMModule _has not_ kept having composition because at #2, it's been reset.

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 8505544 [details] [diff] [review]
Patch

Thanks for the explanations.  The details on the series of events are helpful.
All the changes here seem good, and I don't know what was happening in bug 472635, so I wonder whether we still even need that workaround.

However, there are still some things that I don't understand, so I wonder whether this is really the fix that is needed.

If an IME chooses to commit on reset, then what prevents an already internally committed string being committed again?

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> FYI: the delayed events should cause composition events. The events will be
> handled only by TextComposition. Then, TextComposition will kill itself. So,
> it's not okay to reset composition state at nsGtkIMModule handling a request.

Hasn't the composition already been committed at XP level?
Why is another composition event now necessary?

> 5. Our signal receivers compares given context (same as mContext) and the
> result of GetContext() (mDummyContext). Then, this mismatch makes our
> handlers to ignore the signals but this is okay since the signals will be
> ignored by ShouldIgnoreNativeCompositionEvent().

Ignoring some signals made it OK to ignore some other signals, but I don't know which signals or why.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Karl Tomlinson (:karlt) from comment #10)
> Comment on attachment 8505544 [details] [diff] [review]
> Patch
> 
> Thanks for the explanations.

Thank you for your review too ;-)

> If an IME chooses to commit on reset, then what prevents an already
> internally committed string being committed again?
> 
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> > FYI: the delayed events should cause composition events. The events will be
> > handled only by TextComposition. Then, TextComposition will kill itself. So,
> > it's not okay to reset composition state at nsGtkIMModule handling a request.
> 
> Hasn't the composition already been committed at XP level?
> Why is another composition event now necessary?

Now, in XP level, TextComposition class handles WidgetCompositionEvent before dispatching the event into a DOM tree. If TextComposition emulates to commit composition itself, it waits async commit events coming from widget. When it receives delayed compositionend event, it kills its instance itself and doesn't dispatch the delayed events into the tree.
http://mxr.mozilla.org/mozilla-central/source/dom/events/TextComposition.cpp?rev=52273860a2cd#245
https://developer.mozilla.org/en-US/docs/Mozilla/IME_handling_guide#Requests_to_IME

In other words, if widget doesn't send NS_COMPOSITION_END properly, a TextComposition instance is not destroyed nor released until following native composition causes NS_COMPOSITION_END event.

> > 5. Our signal receivers compares given context (same as mContext) and the
> > result of GetContext() (mDummyContext). Then, this mismatch makes our
> > handlers to ignore the signals but this is okay since the signals will be
> > ignored by ShouldIgnoreNativeCompositionEvent().
> 
> Ignoring some signals made it OK to ignore some other signals, but I don't
> know which signals or why.

We can ignore signals only when a signal gives strange context. Otherwise, we shouldn't do it.

The reason why I don't make the other signal handlers use IsValidContext() is that I have no idea of such cases. Because the cause of this bug is the over between commit signals and a call of SetInputContext(). So, when we receive the other signals, aContext should match with mContext always.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d783563ff4c3

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d783563ff4c3

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8505544 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: bug 1065835
[User impact if declined]: If user moves focus by a click during IME composition to a non-editable element (e.g., <body>), user cannot use IME until user commits or cancels next composition. I.e., if this bug occurs, there is a hidden composition string which will be never inputted actually.
[Describe test coverage new/current, TBPL]: This cannot be tested with automated tests. I tested this manually.
[Risks and why]: This touches the if conditions which were added by bug 472635. It's a hack for ignoring strange IME signals which are fired on another IME context (we're not sure the owner) and come from RDP application or something. We cannot test it because nobody of us reproduced it. This patch makes nsGtkIMModule handle all signals which are fired on *our* owning IME contexts. So, I believe this is right approach.
[String/UUID change made/needed]: Nothing.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6459604b7ccc