Closed Bug 187839 Opened 19 years ago Closed 19 years ago

Caret disappears when turning on IME(JA)

Categories

(Core :: Internationalization, defect)

x86
Windows XP
defect
Not set
minor

Tracking

()

VERIFIED FIXED

People

(Reporter: kazhik, Assigned: aaronlev)

References

Details

(Keywords: inputmethod, intl, regression)

Attachments

(1 file, 1 obsolete file)

Caret disappears when you turn on IME in input field.

Reproduced with 2003010508/WinXP.

Original report in Bugzilla-jp:
http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=2904
Confirm on 01-03 trunk build on both WinME-JA and WinXP-SC when switch to
Japanese IME on text field, but not with Chinese IME/WinXP-SC.

This is a regression - not reproducible on N7.01(1.02branch).
Keywords: intl, regression
Summary: Caret disappears when turning on IME → Caret disappears when turning on IME(JA)
I can reproduce when I downed alt key.

Messages      wParam     lParam
WM_IME_NOTIFY 0x0000000E 0x00000001
WM_SYSKEYDOWN 0x00000012 0x21380001
*** Bug 188457 has been marked as a duplicate of this bug. ***
I am annoyed by this bug every days!
This is not a minor bug for Japanese.

If 1.3b is released befor this bug is fixed,
I can't use it.
Flags: blocking1.3b?
This is a bad regression -> nsbeta1.
Assignee: smontagu → ftang
Keywords: nsbeta1
Can someone identify between which builds this bug started appearing?

Does this affect all Japanese IME users, or just some of them?  (What about
Chinese IME or others?)  I'm marking as blocking1.3b+ on the assumption that
it's all, although if it's only a small proportion I wouldn't maintain that.
Flags: blocking1.3b? → blocking1.3b+
2003010108-trunk OK
2003010408-trunk NG
Comment #1 says 01-03 trunk is NG

I think this is Windows specific and more than half 
of Japanese Windows users are affected by this bug.

I am binding right-ALT key to IME-key on WindowsXP.
When I type right-ALT key, then caret disappeared.
I can recall caret by clicking outside the textarea and 
clicking again inside the textarea.

When I type left-ALT key, which is not bound to any function,
then caret move to menu bar. And typing ESC or left-ALT or 
other keys, caret back to the textarea.
Who can help us here? is ftang still working on Mozilla?
Aaron, it's been suggested that your fix could be responsible for this problem.
It's blocking 1.3beta. Can you take a look at see if you can help us out here?
Thanks. 
Assignee: ftang → aaronl
Yes, that's my code. 
The caret disappears when the alt key is pressed, because the alt key activates
the main menu.

So we're behaving like Internet Explorer and other programs. When the menubar is
focused, the caret disappears. Does this IME caret problem not occur with other
applications?
When I tested with native windows applications...
alt key down ....... No function
alt key up   ....... Menu has focus, and hide caret
esc key down ....... before focused control has focus, and show caret

but, Mozilla is...
alt key down ....... Menu has focus, and hide caret
alt key up   ....... No function
esc key down ....... before focused control has focus(I can't show caret)
I find 2 kinds of behavior in Windows Apps (IE, notepad, other text editors, etc.).

When ALT key is pressed, menu bar activated and:
1. the caret disappears.
2. the caret stop to blink.
When menubar is deactivated, the caret appears and starts to blink again.

I don't find this problem in these applications.
> When I tested with native windows applications...
> alt key down ....... No function
> alt key up   ....... Menu has focus, and hide caret

That's strange, this is the behavior I am getting on win2k mozilla.

What is the key combo used to turn on Japanese IME?
Is it Alt+Shift, or just Alt by itself?
Also, is this bug happening in win2k? If it is, I'd like to try and repro the
bug on my win2k system -- but where can I download Japanese IME for win2k?
We can reproduce with Win98,Win2k,WinMe and WinXP.
We trun on IME Alt + Hankaku/Zenkaku(Kanji) with 106 keyboard.

I don't know the case of that with 101 keyboard.

Adding Cc: momoi-san.
> What is the key combo used to turn on Japanese IME?
> Is it Alt+Shift, or just Alt by itself?
I'll answer my own question :)  I now see you metioned it is configured to use
right alt.

So ... I wonder why Mozilla's menu bar is activating when your right alt key
goes down rather than when it come up. On my win2k machine it waits for alt to
come back up.
Attached patch patch v1 (obsolete) — Splinter Review
I found Mozilla's menu bar calls |SetActive()| from somewhere
 even if the key combination is not assigned to menu bar.
(only in WIndows?)
This patch checks if the activity of menu bar is changed or not.

In this patch, direct assignment of |mIsActive| was replaced to |SetActive()|
call.
And the |SetActive()| has |FireDOMEvent()| call.
Does this cause some side-effects to DOMEvent?
Attachment #113188 - Flags: review?(aaronl)
aaronl: start>settings>control panel>keyboard>input locales>add>japanese, japanese.

yes i'm running w/ the japanese ime (i'm trying to cut ime bloat out of
nsWindow). My tree is currently frozen from pre darin and i'm working on other
things. if this isn't fixed before i merge to tip i'll play with it.
oh, you'll want to run one of these:
conime.exe
phime.exe
cjime.exe
imejpmgr.exe <- this one
pyime.exe
from your system32 directory.
Comment on attachment 113188 [details] [diff] [review]
patch v1

I think this fix might work -- you tested it right? It might be a pretty big
change for this stage. Another possible fix might be to not activate the menu
bar when we receive a WM_IME_NOTIFY with the alt key.


>+  PRBool isChangedActivity = (mIsActive != aActiveFlag) ? PR_TRUE : PR_FALSE;
+  PRBool isChangedActivity = (mIsActive != aActiveFlag);

>   mIsActive = aActiveFlag;
>   if (mIsActive) {
>     InstallKeyboardNavigator();
>@@ -239,14 +240,16 @@
>     if (!selCon)
>       break;
> 
>-    if (mIsActive) {// store whether caret was visible so that we can restore that state when menu is closed
>+    if (mIsActive && isChangedActivity) {// store whether caret was visible so that we can restore that state when menu is closed
>       PRBool isCaretVisible;
>       selCon->GetCaretEnabled(&isCaretVisible);
>       mCaretWasVisible |= isCaretVisible;
>     }
>-    selCon->SetCaretEnabled(!mIsActive && mCaretWasVisible);
>-    if (!mIsActive) {
>-      mCaretWasVisible = PR_FALSE;
>+    if (isChangedActivity) {
>+      selCon->SetCaretEnabled(!mIsActive && mCaretWasVisible);
>+      if (!mIsActive) {
>+        mCaretWasVisible = PR_FALSE;
>+      }
You don't need to test isChangedActivity twice. You can wrap the 2 if blocks
that use it in if (isChangedActivity) so that it has 1 less test.

>     }
>   } while (0);
> 
>@@ -379,7 +382,7 @@
>   if (result) {
>     // We got one!
>     aHandledFlag = PR_TRUE;
>-    mIsActive = PR_TRUE;
>+    SetActive(PR_TRUE);
>     SetCurrentMenuItem(result);
>     result->OpenMenu(PR_TRUE);
>     result->SelectFirstItem();
>@@ -627,14 +630,10 @@
>     return NS_OK;
>   }
> 
>-  // It's us. Just set our active flag to false.
>-  mIsActive = PR_FALSE;
>-
>   // Clear our current menu item if we've got one.
>   SetCurrentMenuItem(nsnull);
> 
>-  // Remove our keyboard navigator
>-  RemoveKeyboardNavigator();
>+  SetActive(PR_FALSE);
> 
>   // Clear out our dismissal listener
>   if (nsMenuFrame::sDismissalListener)

Firing the DOM event only has side effects when accessibility APIs are being
used -- the DOM event is used to tell assistive technology that the content
area now does or doesn't have focus. So this side effect is desirable whenever
the menubar active state changes. So, that part of the change should be
non-risky, and good for accessibility.

Should we even be activating the menu bar when the alt key is being used for
IME?
Attached patch patch v2Splinter Review
If we remove  |SetActive(PR_FALSE)| from |nsMenuBarFrame::DismissChain()|,
we can suppress original reporter's bug.
But there is another bug.
Clicking a few menu items sequentially, the caret disappears.
This is because |SetActive()| is triggered multiple time in same activity.

It seems to me that checking changes of the activity is reasonable.
Attachment #113188 - Attachment is obsolete: true
Attachment #113295 - Flags: review?(aaronl)
Looks reasonable to me. One question:

Do we really need to set the menus to active state in ShortcutNavigation()?
Wouldn't they already be active? What happens if we just remove |mActive =
PR_TRUE;| -- it is it unneccessary?

@@ -379,7 +383,7 @@
   if (result) {
     // We got one!
     aHandledFlag = PR_TRUE;
-    mIsActive = PR_TRUE;
+    SetActive(PR_TRUE);
     SetCurrentMenuItem(result);
     result->OpenMenu(PR_TRUE);
     result->SelectFirstItem();

This patch seems like the way to go, although I still don't see why the menubar
should activate at all when IME is being activated, or why the menubar is
getting activated when alt is onkeydown.

I'd like to see what bryner or hyatt think of the patch.
Comment on attachment 113295 [details] [diff] [review]
patch v2

I'll answer my own question about needing to set the menubar to the active
state in ShortcutNavigation(). We do need that, because the menu bar is not yet
active when the user type Alt+letter from content, to access the menubar.

This patch seems like the right fix to me.
Attachment #113295 - Flags: review?(aaronl) → review+
Attachment #113295 - Flags: superreview?(bryner)
Attachment #113295 - Flags: superreview?(bryner) → superreview+
Attachment #113295 - Flags: approval1.3b?
Attachment #113295 - Flags: approval1.3b? → approval1.3b+
checked in -- thanks Shotaro!
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Verified fixed in 02-04 trunk build / WinME and WinXP.
Status: RESOLVED → VERIFIED
please to be removing the 'printf("\nIs active?...' that was accidentally checked 
in and now shows up every time you open a menu in opt. release builds.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Sorry about the printf!

Fixed again, without the printf this time :-)
Status: REOPENED → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
back to verif.
Status: RESOLVED → VERIFIED
Comment on attachment 113188 [details] [diff] [review]
patch v1

(clearing review request from obsolete patch)
Attachment #113188 - Flags: review?(aaronl)
You need to log in before you can comment on or make changes to this bug.