378752 - Mnemonic of Menu doesn't work if an editor has focus and IME is on

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

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

This is a remaining issue of bug 55751.

In bug 55751, I have fixed the bug on most cases. The way is the IME doesn't work on non-editable widgets. Therefore, the bug still in the editors.

I think that we should notify for nsIWidget and/or nsIMEStateManager when the menus install/uninstall the keyboard navigation. And nsIWidget should suspend the IME composing even if it is during the composing.

Note that we don't need to fix this on Mac, because Mac doesn't have Mnemonic in the menus. We need to fix only on Win/Linux.

Comment 1

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

Attachment: Patch rv1.0

I think that we should add new IME state for menu loop, and the native widgets should use the each platform rules.

On win32, the behavior is not same as native behavior, but I don't know whether we can implement same behavior. However, the IME state is same as 'Disabled' by this patch, therefore, the menu mnemonics works fine. The native behavior is to keep the transaction after menu loop.

On Linux, the behavior is not same as native behavior too. Only with Tran-Chinese IME, we can open the menus during composing the string. The native behavior is to keep the composing transaction. But the patch commit the transaction at a menu is opened. However, the Japanese and the Simp-Chnese IMEs cannot open the menu in the composing transaction, and Korean IME commits the composing at Alt key is downed. The patch disables the IME at the menu loop too.

On Mac, I don't change the behavior for the menu loop. Because Mac doesn't have the menu mnemonics.


Thus, I cannot simulate the same behavior for native. But I think that the patch's behavior is enough for many users. Because I believe that the many users don't use the menus at composing.

Comment 2

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

+    if (sOldIMEState != nsIKBStateControl::IME_STATUS_UNDEFINED) {
+      // XXX If the context menu was already opened and reopening the context
+      // menu from the keyboard, it's comes here.
+      return NS_OK;
+    }

What does this mean?

Do we need to pass the prescontext associated with the menu that's opening? You're assuming it's the prescontext with focus, but I think that might not be true.

When the menu opens, you're resetting the IME state to the old state. But what if focus has changed while the menu was open (e.g. by script)?

Instead of adding the new MENU state, could you modify
nsIMEStateManager::GetNewIMEState to return STATUS_DISABLED when a menu is open (for non-Mac)? And then call OnChangeFocus (or some other method) when menus open and close?

Comment 3

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

(In reply to comment #2)
> +    if (sOldIMEState != nsIKBStateControl::IME_STATUS_UNDEFINED) {
> +      // XXX If the context menu was already opened and reopening the context
> +      // menu from the keyboard, it's comes here.
> +      return NS_OK;
> +    }
> 
> What does this mean?
> 
> Do we need to pass the prescontext associated with the menu that's opening?
> You're assuming it's the prescontext with focus, but I think that might not be
> true.

If I press the context menu key on keyboard two or more times, the context menu is reopening. In this time, the code is run.

> When the menu opens, you're resetting the IME state to the old state. But what
> if focus has changed while the menu was open (e.g. by script)?
> 
> Instead of adding the new MENU state, could you modify
> nsIMEStateManager::GetNewIMEState to return STATUS_DISABLED when a menu is open
> (for non-Mac)? And then call OnChangeFocus (or some other method) when menus
> open and close?

it sounds like good idea, I rewrite the patch.

Comment 4

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

Attachment: Patch rv2.0

Comment 5

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

NotifyOnMenuLoop should have a better name, say NotifyInstalledMenuKeyboardListener. Similarly, change OnMenuLoop to OnInstalledMenuKeyboardListener, sInstalledMenuKeyboardListener.

+#ifndef XP_MACOSX
+#define MENU_HAS_MNEMONIC 1
+#endif

Do we really need this? Seems to me that Mac is not going to install the menu keyboard listener so this code just isn't going to be called and we don't need any #ifdefs.

+  if (sInMenuLoop == aEnter)
+    return NS_OK;

I don't think we need this optimization. This code will not be called on any performance-sensitive path.

Otherwise, I think this is a great solution.

Comment 6

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

Attachment: Patch rv2.1

The patch becomes simple! thanks.

Comment 7

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

Comment on attachment 263437 [details] [diff] [review]
Patch rv2.1

+nsresult
+nsContentUtils::NotifyInstalledMenuKeyboardListener(PRBool aInstalling)

Make this return void.

+nsresult
+nsIMEStateManager::OnInstalledMenuKeyboardListener(PRBool aInstalling)

Make this return void as well.

Comment 8

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

Attachment: Patch rv2.2

Thank you roc.

Comment 9

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

checked-in, thank you.