610829 - Japanese IME: The textbox may move to a different window prior to commit

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: Screenshot

1. use Japanese version of Windows XP
2. use Microsoft Japanese IME 
3. launch Firefox and set to Hiragana
4. bookmark the first page and tag it with "tag"
5. open a new browser
6. open the bookmarks
7. change the tag to "nihongo" and double tap the space 
8. click in the other browser's text field

Expected: the IME box does not change location
Actual: the IME box does switch over to the other browser

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: screenshot

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Mozilla/5.0 (Windows NT 5.1; rv:2.0b7) Gecko/20100101 Firefox/4.0b7

Comment 3

[Alice0775 White]

Confirmed WindowsXP+IME2003
*It does not happens ATOK2006
*It does not happens on Firefox3.6.13pre

Comment 4

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

maybe, this is a regression of bug 569023.

Comment 5

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

(In reply to comment #3)
> *It does not happens ATOK2006

Alice-san, are you sure? Cannot you reproduce with ATOK too? I can reproduce this bug with ATOK 2010 and ATOK 2011 on Win7 with following STR:

1. Launch Firefox.
2. Open a new window.
3. Set focus to the textbox for search in one of the windows.
4. Turn on IME.
5. Input something and press space key and ensure the candidate window to be visible.
6. Click the other window's textbox for search.

Comment 6

[Alice0775 White]

(In reply to comment #5)
> (In reply to comment #3)
> > *It does not happens ATOK2006
> 
> Alice-san, are you sure? Cannot you reproduce with ATOK too? I can reproduce
> this bug with ATOK 2010 and ATOK 2011 on Win7 with following STR:
> 
> 1. Launch Firefox.
> 2. Open a new window.
> 3. Set focus to the textbox for search in one of the windows.
> 4. Turn on IME.
> 5. Input something and press space key and ensure the candidate window to be
> visible.
> 6. Click the other window's textbox for search.

umm, the candidate window close immediately when I click the other window's textbox. I'm not sure, but it is  ATOK2006 and I use "明鏡国語辞典" optional dictionary.

Comment 7

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

Cannot you reopen the candidate window by pressing space key again?

Comment 8

[Alice0775 White]

(In reply to comment #7)
> Cannot you reopen the candidate window by pressing space key again?
No I cannot. Autocomplete menu pops up when I press space key.

Comment 9

[Alice0775 White]

(In reply to comment #4)
> maybe, this is a regression of bug 569023.

In my investigation with Microsoft IME, the regression window is as follows.
Works:
http://hg.mozilla.org/mozilla-central/rev/01fa971e62ee
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5pre) Gecko/20100827 Firefox/4.0b5pre ID:20100827190212
Fails:
http://hg.mozilla.org/mozilla-central/rev/0886ad6e6aaa
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5pre) Gecko/20100827 Firefox/4.0b5pre ID:20100827190555
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=01fa971e62ee&tochange=0886ad6e6aaa

In localbuild:
build from 687b70fea4d0  :works
build from 7804b5cf4313 : fails

Comment 10

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

Thanks, it makes sense widget removable caused this. I'm creating patch but I'm fighting with some complicated issue...

I'll check the ATOK2006 behavior before review.

Comment 11

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

Attachment: Patch v1.0

If an IM context has been disassociated and deactivated at committing or canceling composition, IME does nothing. Therefore, we should associate IM context with the window before doing that and disassociate IM from the window if we associated temporarily.

And now, we can use ImmAssociateContextEx() instead of ImmAssociateContext(). Therefore, we can get rid of mOldIMC from nsWindow.

Note that with ATOK 2011, you can see unnecessary candidate window on new focused editor. This should be a bug of ATOK 2011. I tried to avoid this issue by using NI_CLOSECANDIDATE but failed. I'll contact ATOK team later.

Comment 12

[Masatoshi Kimura [:emk]]

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

> +  PRBool associated = aWindow->AssociateDefaultIMC(PR_TRUE);
> +  PR_LOG(gIMM32Log, PR_LOG_ALWAYS,
> +    ("IMM32: CommitComposition, associated=%s\n",
> +     associated ? "YES" : "NO"));
> +
>    nsIMEContext IMEContext(aWindow->GetWindowHandle());
>    if (IMEContext.IsValid()) {
>      ::ImmNotifyIME(IMEContext.get(), NI_COMPOSITIONSTR, CPS_COMPLETE, 0);
>      ::ImmNotifyIME(IMEContext.get(), NI_COMPOSITIONSTR, CPS_CANCEL, 0);
> +    if (associated) {
> +      aWindow->AssociateDefaultIMC(PR_FALSE);
> +    }
>    }
Is it Ok to skip disassociating when IMEContext.IsValid() returns false?

> +PRBool nsWindow::AssociateDefaultIMC(PRBool aAssociate)
This function do not share the code between associating and disassociating.
Could you split the function for those two cases?

r+ assuming the patch passes the try. Is it difficult to add a regression test?

Comment 13

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

(In reply to comment #12)
> Is it Ok to skip disassociating when IMEContext.IsValid() returns false?

Ah, it's Okay for now. But in logically, I shouldn't do so. I'll change it thanks.

> > +PRBool nsWindow::AssociateDefaultIMC(PRBool aAssociate)
> This function do not share the code between associating and disassociating.
> Could you split the function for those two cases?

I wrote so, but see the nsWindow::SetInputMode(). It makes the caller simple if we can change the behavior by param.

> r+ assuming the patch passes the try. Is it difficult to add a regression
> test?

Yes, we need to create IMM emulator layer for testing.

Comment 14

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/cceff8f75d6a

landed on inbound except the design issue of nsWindow::AssociateDefaultIMC() because no reply for comment 13 and it's just a coding style issue.

Kimura-san, if you don't like it and I should fix it, please file a new bug.

Comment 15

[Phil Ringnalda (:philor)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/4aaf3b77a6f3 - see your assertion firing in http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1309840444.1309844926.29590.gz&fulltext=1

Comment 16

[Masatoshi Kimura [:emk]]

(In reply to comment #14)
> landed on inbound except the design issue of nsWindow::AssociateDefaultIMC()
> because no reply for comment 13 and it's just a coding style issue.
That's fine. Sorry to forget a reply.

Comment 17

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

(In reply to comment #15)
> Backed out in
> http://hg.mozilla.org/integration/mozilla-inbound/rev/4aaf3b77a6f3 - see
> your assertion firing in
> http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1309840444.
> 1309844926.29590.gz&fulltext=1

Ah... probably the cause is that IMM isn't installed in English Windows.

Comment 18

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

Attachment: Patch v1.1

I'd like to print the API failure. However, we shouldn't do it on non-IMM installed environment.

This patch adds nsIMM32Handler::IsIMEAvailable() for debug build. It returns the result of ::ImmIsIME() for current keyboard layout. And only when it returns TRUE, this patch checks the result of ::ImmAssociateContextEx(). Note that probably, it's not needed for disassociating because the context should be always NULL on non-IMM installed environment.

Comment 19

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/78a414587aa6

Comment 20

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/78a414587aa6