327003 - Don't commit the IME composition string when Mozilla is deactivated

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

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

Now, we commit the IME composition string at the editor lost focus.
# In nsTextEditorFocusListener::Blur
http://lxr.mozilla.org/mozilla/source/editor/libeditor/text/nsEditorEventListeners.cpp#1078

But this is not good for some WM of the Linux. Because on some WM, the IME candidate list window steals the focus from Mozilla.

Therefore, we need to move the commiting code from editor to ESM. We should commit the IME only when the another Mozilla widget steals the focus.

Comment 1

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

Attachment: Patch rv1.0

This patch has |nsIMEStateManager| that is separating the IME managed code from ESM for readability.

roc:
If you grant to this approach, I'll request additional review to timeless(editor) and Masaki Katakai-san(gtk2).

Comment 2

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

roc:

> If you grant to this approach,

"If you agree to this approach,"

Comment 3

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

This patch also fixes bug 278802.

Comment 4

[Hiro]

(In reply to comment #1)
> Created an attachment (id=216228) [edit]
> Patch rv1.0

It reject by the following files.
(bug330710)

patching file content/events/public/Makefile.inpatching file content/events/public/nsIIMEStateManager.h
patching file content/events/src/Makefile.in
patching file content/events/src/nsEventStateManager.cpp
patching file content/events/src/nsEventStateManager.h
patching file content/events/src/nsIMEStateManager.cpp
patching file content/events/src/nsIMEStateManager.h
patching file editor/libeditor/base/nsEditor.cpp
patching file editor/libeditor/text/nsEditorEventListeners.cpp
Hunk #2 FAILED at 1088.
1 out of 2 hunks FAILED -- saving rejects to file editor/libeditor/text/nsEditorEventListeners.cpp.rej
patching file widget/src/gtk2/nsWindow.cpp
patching file widget/src/gtk2/nsWindow.h

Comment 5

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

Attachment: Patch rv1.1

updating.

Comment 6

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

Attachment: Patch rv1.1

Sorry, new files dropped.

Comment 7

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

Attachment: Patch rv1.2

maybe, this fixes the crash on soralis.

Comment 8

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

Attachment: Patch rv1.3

Sorry, this is better.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  nsIViewManager* GetViewManager() {
+    return GetPresShell() ? GetPresShell()->GetViewManager() : nsnull;
+  }

I'd prefer that you did not do this. I assume you were crashing during presshell/prescontext destruction. Can you find a way to get around the crash without making this change?

+  static nsIMEStateManager sISM;

That's a cute trick but it's not really needed here. The static instance could be a regular static variable and it's probably slightly more efficient to make it one.

I think we don't need nsIIMEStateManager. Just use nsIMEStateManager.h directly. Does that need to be in EXPORTS? It's not used outside content/events/src, is it? nsIMEStateManager doesn't need to have an IID or inherit from nsISupports.

Comment 10

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

roc:

Thank you for your review. I'll recreate a new patch. But I have another problem that is nsPresContext of background tab in active window returns true on nsIMEStateManager::IsActive. This is not good. We need to check the focus controller's active flag. But the flag is not correct on activate/deactivate...
If you have a good idea for this problem, please tell me.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You might get some help by QI'ing the presshell to nsIViewObserver and calling IsVisible(). This is set to false on background tabs.

Comment 12

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

Attachment: Patch rv2.0

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  virtual ~nsIMEStateManager();
+
+  NS_IMETHOD OnDestroyPresContext(nsPresContext* aPresContext);
+  NS_IMETHOD OnRemoveContent(nsPresContext* aPresContext, nsIContent* aContent);
+  NS_IMETHOD OnChangeFocus(nsPresContext* aPresContext, nsIContent* aContent);
+  NS_IMETHOD OnActivate(nsPresContext* aPresContext);
+  NS_IMETHOD OnDeactivate(nsPresContext* aPresContext);

These don't need to be virtual. In fact, they can be static, and the fields of this class can be static too. Then you don't need an instance of nsIMEStateManager at all, and you don't need constructors and destructors.

+            nsCOMPtr<nsPresContext> context = shell->GetPresContext();

This doesn't need to be a strong reference since we're already holding a strong ref to the presshell, and the presshell strongly references the prescontext.

Comment 14

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

Attachment: Patch rv3.0

changing to static.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

GetKBStateControl can just return a non-addrefed pointer directly. There is no need to return an addrefed pointer here; you're not doing anything that could destroy the widget, right? Return nsnull if there's an error.

Why do we need nsEventStateManager to have the new NotifyDestroyPresContext method? Is it too late to do that stuff when SetPresContext(nsnull) gets called?

Comment 16

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

(In reply to comment #15)
> Why do we need nsEventStateManager to have the new NotifyDestroyPresContext
> method? Is it too late to do that stuff when SetPresContext(nsnull) gets
> called?

Maybe, 'Yes'. It's too late. Because the view manager of the presShell is released after here, but before SetPresContext(nsnull). Do you have better idea? I don't have better idea.(I don't like this approach too.)

Comment 17

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

Attachment: Patch rv3.1

And see previous my comment for |NotifyDestroyPresContext|.

Comment 18

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

Comment on attachment 217356 [details] [diff] [review]
Patch rv3.1

timeless:

Would you review the editor code?
I removed to call |ForceCompositionEnd()| from |nsEditor::PostCreate()| this is added by bug 17419. But I think that this code is not needed for the bug. But If it's needed for that bug, maybe it is wrong. We should find another way.

Comment 19

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

Comment on attachment 217356 [details] [diff] [review]
Patch rv3.1

Katakai-san:

Would you review gtk2 widget code?

Comment 20

[Masaki Katakai]

Comment on attachment 217356 [details] [diff] [review]
Patch rv3.1

>     if (focusedIm && focusedIm == window->mIMEData->mContext) {
>         // Release current IME focus if IME is enabled.
>-        if (window->mIMEData->mEnabled)
>+        if (window->mIMEData->mEnabled) {
>+            focusedWin->ResetInputState();
>             focusedWin->IMELoseFocus();
>+        }

Hi Nakako-san,

focusedWin->ResetInputState() was not in the patch for bug 321468, but it's newly added for this patch.
Any reason?

Comment 21

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

(In reply to comment #20)
> (From update of attachment 217356 [details] [diff] [review] [edit])
> >     if (focusedIm && focusedIm == window->mIMEData->mContext) {
> >         // Release current IME focus if IME is enabled.
> >-        if (window->mIMEData->mEnabled)
> >+        if (window->mIMEData->mEnabled) {
> >+            focusedWin->ResetInputState();
> >             focusedWin->IMELoseFocus();
> >+        }
> 
> Hi Nakako-san,
> 
> focusedWin->ResetInputState() was not in the patch for bug 321468, but it's
> newly added for this patch.
> Any reason?

It was removed by your advice, because it commited at the window is activating. But it's necessary and this patch removes the problem.(at the window is activating, |SetIMEEnabled| is not called by this patch.)

Comment 22

[Masaki Katakai]

> It was removed by your advice, because it commited at the window is activating.
> But it's necessary and this patch removes the problem.(at the window is
> activating, |SetIMEEnabled| is not called by this patch.)

Which bug can be fixed by that line? I need a reason why this line is necessary, any bugid or example step?

Comment 23

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

(In reply to comment #22)
> > It was removed by your advice, because it commited at the window is activating.
> > But it's necessary and this patch removes the problem.(at the window is
> > activating, |SetIMEEnabled| is not called by this patch.)
> 
> Which bug can be fixed by that line? I need a reason why this line is
> necessary, any bugid or example step?
> 

There is no bug. But I think that we should ensure that there is not IM transaction while IM is disabled on the widget. If there is a IM transaction but the IM is disabled on the widget, user cannot commit the IM transaction, because the widget doesn't send key events to IM while IM is disabled.

Comment 24

[Masaki Katakai]

Comment on attachment 217356 [details] [diff] [review]
Patch rv3.1

Thank you Nakano-san for clarification. I understand now.

Comment 25

[timeless]

Comment on attachment 217356 [details] [diff] [review]
Patch rv3.1

>Index: content/events/public/nsIEventStateManager.h
> class nsIEventStateManager : public nsISupports {
>+  NS_IMETHOD NotifyDestroyPresContext(nsPresContext* aPresContext) = 0;
>   NS_IMETHOD SetPresContext(nsPresContext* aPresContext) = 0;

normally we add methods to the end, but since you're bumping the iid i suppose it doesn't matter.
but that's not my part of the code area :).

changes seem fine.

Comment 26

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

checked-in, thank you.

Comment 27

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

Attachment: Patch for check-in