417315 - Cannot use IME menus during IME transaction

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

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

Attachment: Patch v0.1

We cannot use IME menus (keyboard layout switching menu, I don't know the official name in English) during IME transaction.

Because current trunk build commit the IME transaction at losing focus. This is safe and simple fix for some bugs. But the menu gets the focus at opened. Therefore, we cannot use the menu.

The menu has some features of IMEs and some users might use them. However, I'm not sure that I can fix Gecko1.9 final.

I attach a simple patch. That removes the force commit code at loosing the focus. But when switch between 2 Gecko windows, there is a serious bug (in system (in other words, in IME), the transaction was committed, but Gecko still continues the transaction). We cannot fix this bug until all regression bugs being gone.

Comment 1

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

Attachment: Patch v0.2

This fixes the found issues. I need more testing for this.

Comment 2

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

Attachment: Patch v1.0

o.k. I cannot found any regression on Mac.

However, this patch cannot fix  this bug on search bar of browser window. We need to change the toolkit code and editor code for it. I'll create the patch, but it should be in other review process, because they are not related.

Comment 3

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

Comment on attachment 303270 [details] [diff] [review]
Patch v1.0

We should not define NS_KBSC_USE_SHARED_CONTEXT on Mac. And we can remove nsTSMManager::CommitIME() at childview gets/loses the focus.

When window is changed between a Gecko application, nsIKBStateControl::ResetInputState is called, and it calls nsTSMManaager::CommitIME, finally, it fires the IME composition end event. However, it is sent to rood element, not last focused content, therefore, we need to change nsPresShell.

Comment 4

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

Maybe, I can fix this bug soon. This should block the release.

Comment 5

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

Attachment: Patch for toolkit and editor v1.0

The textbox of toolkit with clickSelectsAll always calls selectAll when that gets focus. However, if the editor has IME transaction, it should not call selectAll. Because it commits the IME transaction. Therefore, we need to get whether the editor has IME transaction, and then, textbox should not call selectAll.

First, I need editor part's review.

Comment 6

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

NS_SHOULD_EVENT_TARGET_IS_LAST_FOCUSED_CONTENT isn't a very good name. Also, it should be a static function with #ifdefs in its body, and it should be always defined. How about calling it TargetUnfocusedEventToLastFocusedContent?

Instead of #if0'ing  NS_KBSC_USE_SHARED_CONTEXT, just remove it completely.

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 303310 [details] [diff] [review]
Patch for toolkit and editor v1.0

Please get an additional review from a toolkit peer on the toolkit part.

Comment 8

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

Comment on attachment 303310 [details] [diff] [review]
Patch for toolkit and editor v1.0

Neil:

See comment 5 for this patch.

Comment 9

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

Attachment: Patch v1.1

Comment 10

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

Attachment: Patch v1.1

Oops, sorry for the spam.

Comment 11

[Neil Deakin]

Comment on attachment 303310 [details] [diff] [review]
Patch for toolkit and editor v1.0

>+            else if (this.clickSelectsAll) {
>+              const nsIEditorIMESupport =
>+                      Components.interfaces.nsIEditorIMESupport;
>+              imeEditor = this.editor.QueryInterface(nsIEditorIMESupport);

You need to declare imeEditor. (var or let)

>+              if (!imeEditor || !imeEditor.composing)
>+                this.editor.selectAll();
>+            }

Also, even though this was a problem before, the code block for if (this.clickSelectsAll) should really be in try/catch

Comment 12

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

Attachment: Patch for toolkit and editor v1.1

Comment 13

[Neil Deakin]

Comment on attachment 303552 [details] [diff] [review]
Patch for toolkit and editor v1.1

Actually, it's the 'this.editor' and 'selectAll' calls that will cause an exception (a textbox in a non-chrome document for instance), so I don't think the additional selectAll call within the catch block is necessary.

Comment 14

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

Attachment: Patch for toolkit and editor v1.2

Neil:

I misunderstand why you need the try/catch. It seems that this is your wanted patch. If you don't like this, please deny this. (I don't set the review flag, because the previous is granted.)

Comment 15

[Neil Deakin]

Whe  you(In reply to comment #14)
> Created an attachment (id=303568) [details]
> Patch for toolkit and editor v1.2
> 
> Neil:
> 
> I misunderstand why you need the try/catch. 

Mainly because it causes an exception when used from non-chrome. But the code had that problem already so it isn't a big deal if you checked in either the original patch (with the var change) or the latest one.

Comment 16

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

Comment on attachment 303480 [details] [diff] [review]
Patch v1.1

+        if (NS_TargetUnfocusedEvenToLastFocusedContent(aEvent)) {

Missing a "t" in "Event"

+#endif // defined(MOZ_X11) || defined(XP_MACOSX)

The // comment is incorrect. Just remove it.

Comment 17

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

Comment on attachment 303480 [details] [diff] [review]
Patch v1.1

oops

Comment 18

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

Attachment: Patch v1.2

This should be landed now. This bug is major feature non-working bug.
The risk is middle. Because some behavior is depended on IMEs. However, it's same on other applications. So, if there is some problem, it might be IME bugs.

Comment 19

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

Comment on attachment 303568 [details] [diff] [review]
Patch for toolkit and editor v1.2

This is major feature bug. And this patch's risk is low.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 303568 [details] [diff] [review]
Patch for toolkit and editor v1.2

a=beltzner for 1.9

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 304268 [details] [diff] [review]
Patch v1.2

a=beltzner for 1.9

Comment 22

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

checked-in, thanks!

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

This patch added a whole slew of warnings to my build: ../../../../dist/include/widget/nsGUIEvent.h:1250: warning: ‘PRBool NS_TargetUnfocusedEventToLastFocusedContent(nsEvent*)’ defined but not used

because NS_TargetUnfocusedEventToLastFocusedContent is declared static in a wildly-included header file. Perhaps you meant it to be 'inline' instead of 'static'?