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

RESOLVED FIXED in Firefox 35

Status

()

Core
Widget: Gtk
--
major
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({inputmethod, regression})

Trunk
mozilla36
x86_64
Linux
inputmethod, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox34 unaffected, firefox35+ fixed, firefox36+ fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Okay, I cannot reproduce this bug with SCIM. That means that we have a bug at asynchronous commit.
Keywords: regressionwindow-wanted
Created attachment 8505532 [details] [diff] [review]
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.
Attachment #8505532 - Flags: review?(karlt)
Comment on attachment 8505532 [details] [diff] [review]
Patch

Oops, sorry, this patch includes obsoleted approach.
Attachment #8505532 - Attachment is obsolete: true
Attachment #8505532 - Flags: review?(karlt)
Created attachment 8505537 [details] [diff] [review]
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.
Attachment #8505537 - Flags: review?(karlt)
Created attachment 8505544 [details] [diff] [review]
Patch

Ugh. I'm very sorry for the spam. This is the right patch!
Attachment #8505537 - Attachment is obsolete: true
Attachment #8505537 - Flags: review?(karlt)
Attachment #8505544 - Flags: review?(karlt)
[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.
Blocks: 1065835
status-firefox34: --- → unaffected
status-firefox35: --- → affected
status-firefox36: --- → affected
tracking-firefox35: --- → ?
Keywords: regressionwindow-wanted
karlt: ping
Flags: needinfo?(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?
Flags: needinfo?(karlt) → needinfo?(masayuki)
(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.
Flags: needinfo?(masayuki)
tracking-firefox35: ? → +
tracking-firefox36: --- → +
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.
Attachment #8505544 - Flags: review?(karlt) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/d783563ff4c3
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox36: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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.
Attachment #8505544 - Flags: approval-mozilla-aurora?
Attachment #8505544 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.