55751 - Mnemonic doesn't work when IME is on

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P1)

Description

[Koike Kazuhiko]

When IME is on, [Alt-F] - [O] doesn't work. [Alt-F] opens
[File] menu, but [O] is sent to a text field. It should open
Open File dialog.

Win32 build 2000100520 on Win98

Comment 1

[nhottanscp]

I can reprodce this using today's trunk build on WinNT4-J.

Comment 2

[nhottanscp]

Reassign to yokoyama, cc to ftang, shanjian.

Comment 3

[nhottanscp]

Put rtm to keyword.

Comment 4

[Masaki Katakai]

same problem happens on Linux

Comment 5

[Teruko Kobayashi]

Changed QA contact to [e-mail].

Comment 6

[Roy Yokoyama]

Updating the target milestone.

Comment 7

[Jungshik Shin]

The same is true of shortcuts in the context menu. 

Comment 8

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

Can this bug be fixed?
WinIE has same problem in the Editor(IE is disabling IME on other of Editor).
I think the way of to fix this bug is very difficult way.
We should disable IME on other of Editor.
This is same approach of WinIE.

But we have FAYT. If we disable IME on other of editor, we cannot use IME at FAYT.
Therefore, we should implement Findbar on Suite too.
And FAYT's editor of Firefox and Suite should tread to input for FAYT.

This solution is related Bug 113187.

Comment 9

[Jungshik Shin]

(In reply to comment #8)
> Can this bug be fixed?
> WinIE has same problem in the Editor(IE is disabling IME on other of Editor).

Does MS IE have the same problem? I can't reproduce it with MS IE (and Korean
IME in textarea like this bugzilla comment input area). BTW, can you explain
what  you meant by 'other of Editor'? 

Comment 10

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

IE has same problem.
On WinXP-ja SP2 + ATOK17, if the focus is set in editor and IME is active, Alt +
F does not work.(I can hear error sound.)

The 'other of editor' is the non-editor widget.
e.g., button, checkbox, menu-window and the canvas of the page.

Comment 11

[Jungshik Shin]

(In reply to comment #10)
> IE has same problem.
> On WinXP-ja SP2 + ATOK17, if the focus is set in editor and IME is active, Alt +
> F does not work.(I can hear error sound.)

See comment #0. What's reported was that 'Alt-F' works (brings up the file
submenu) but 'O' (for file open) doesn't work. Anyway, MS IE doesn't have the
same problem with the standard Korean IME on Win 2k although it seems to have
the same problem with ATOK17. 

Comment 12

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

> What's reported was that 'Alt-F' works (brings up the file
> submenu) but 'O' (for file open) doesn't work.

Oh, you are right, however I think that these problem are same.
Because the cause of both problem is 'eating key events' by IME.
Therefore if non-editor widget disable IME, this problem cannot be reproduced on
non-editor widget.

Comment 13

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

Oops...

I found same problem on IE.
Execute the next steps.

1. Set focus editor(but exclude password field).
2. Turn on IME.
3. _Click_ "Favorites" by mouse.
4. Type 'A' or 'O'.

The favorites menu is not windows native menu.
This situation is same as Mozilla.

And I found new pattern. Following pattern is reproduced on Mozilla and IE.

1. Turn on IME.
2. Type Alt key.
3. Type F key.

The file menu is not opened by 'F' key.

Comment 14

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

Attachment: Patch rv1.0

Comment 15

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

Attachment: Patch rv1.0 content/

Comment 16

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

Attachment: Patch rv1.0 editor/, modules/libpref/

Comment 17

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

Attachment: Patch rv1.0 widget/

Comment 18

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

My patch makes IE like IME control on Windows(Other OSs are not implemented XP
IME API). On non-editor widgets, we don't use IME, so, these widgets should
disable IME if it has focus.

My patch hooks on focus content event. The focus content event has IME status
that's default value is 'Disabled'. When editor widget gets the focus, the
editor set 'Enable' status to the focus content event reply. ESM is setting IME
status on current widget according to event reply's IME status.

Comment 19

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

This patch fixes bug 113187 too.

Comment 20

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

Attachment: Patch rv1.1 content/

Comment 21

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

Attachment: Patch rv1.1

Comment 22

[timeless]

Comment on attachment 193822 [details] [diff] [review]
Patch rv1.1 content/

>Index: content/events/public/nsIPrivateFocusEvent.h
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 1998

this is wrong

Comment 23

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

Attachment: Patch rv1.2 content/

Comment 24

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

Attachment: Patch rv1.2

Comment 25

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

Attachment: Patch rv1.3 content/

Comment 26

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

Attachment: Patch rv1.3 widget/

Comment 27

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

Attachment: Patch rv1.3

Comment 28

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

If this bug is fixed by current patch, there are two regressions.
But these regressions will be fixed easy. Don't worry.

regressions:
1. Cannot use IME in FAYT of Firefox.
2. Cannot use IME in FAYT of Suite.

Comment 29

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

Attachment: Patch rv1.4 content/

Comment 30

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

Attachment: Patch rv1.4 widget/

Comment 31

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

Attachment: Patch rv1.4

Comment 32

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

Attachment: testcase

These cases are cleared by Patch rv1.4. But Firefox's FAYT problem still
happen.

Comment 33

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

Is there a reason why you need to fix the regression in a separate patch?

Comment 34

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

(In reply to comment #33)
> Is there a reason why you need to fix the regression in a separate patch?

I think that if I separate to the bugs, we can get clearer comments and logs.
If I fix all regressions in this bug, if someone find similar regressions, it
may not be easy to find his/her needed comments in this many comments.

And simply, regressions are not exists in current patch. These will be fixed in
other components that are toolkit/components and xpfe(not core. it is fixed in
FAYT components). I think that reviews for these components need many days(these
reviewers current main works are for 1.8 branch, not trunk). I hope that other
regressions should be found by many nightly testers in this term.

Comment 35

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

> And simply, regressions are not exists in current patch.

I'm not sure what you mean, can you rephrase that?

Comment 36

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

(In reply to comment #35)
> > And simply, regressions are not exists in current patch.
> 
> I'm not sure what you mean, can you rephrase that?

I need change XPFE's FAYT code and Toolkit's widget code for fixing regressions.
Those files is not included in current patch.

Comment 37

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

This patch isn't really so big. I want to fix any known regressions in this
patch before landing. It's ultimately simpler and less work this way.

Comment 38

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

O.K. I will be adding new patches.

Comment 39

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

Comment on attachment 194192 [details] [diff] [review]
Patch rv1.4 widget/

clearing based on comment.

Just put it all in one big patch. I'll review the whole thing.

Comment 40

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

Attachment: Patch rv1.9(work in progress)

Firstshot of new patch.
This can be built on Windows only. And this patch's IME controling for FAYT of
seamonkey does not work fine.

Comment 41

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

*** Bug 310760 has been marked as a duplicate of this bug. ***

Comment 42

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

Attachment: Patch rv1.9.1(work in progress)

Comment 43

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

Attachment: Patch rv2.0

This fix this bug. And I cannot find regression in my test.

Spec of this patch:
1. If focus is set to OBJECT or Editor, it enables IME.
   Otherwise, it disables IME.
2. If FAYT is stated on Seamonkey, it enables IME.
3. If FAYT is finished on Seamonkey, it disables IME.

Comment 44

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

This is a lot of code and the FAYT special-case doesn't make me happy.

Maybe a better approach is to implement some sort of general feature to control whether IME should be actived when an element is focused ... e.g., a CSS property like IE's ime-mode:
http://www.blooberry.com/indexdot/css/properties/intl/imemode.htm
or maybe related to XForms' inputmode attribute, which has additional features:
http://www.w3.org/TR/2005/PER-xforms-20051006/sliceE.html
(I thought we might already have a bug on this, but I can't find it)

Then we could just check the content property on a focus change to determine whether to enable or disable IME. The default would be to disable IME; we'd set the property for editors and FAYT, but users/web authors would be able to control it in their own content too. (I heard requests for this while I was at IBM.) What do you think?

Comment 45

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

I have a plan that is to implement '-moz-ime-mode' CSS property in bug 279246.
I think we can implement this property by current patch's mechanism.
So, we will change the result of |nsEditor::GetDesirableIMEStatus|.

But non-Editor widgets should not enable IME.
Because there is no merit for usability.

Comment 46

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

(In reply to comment #45)
> I have a plan that is to implement '-moz-ime-mode' CSS property in bug 279246.
> I think we can implement this property by current patch's mechanism.
> So, we will change the result of |nsEditor::GetDesirableIMEStatus|.
> 
> But non-Editor widgets should not enable IME.
> Because there is no merit for usability.
 
It's possible to create your own DHTML widget where you trap key events and you want to receive IME input. That's what FAYT is, but web authors could build their own as well. For example, a list that you can search by typing characters that's implemented using DHTML rather than using a standard <select>. (BTW does this patch disable IME in select drop-downs?)

Also, the current patch makes a number of changes to FAYT. We really need to avoid that by at least making it easy to turn IME on/off for content in our own code. FAYT code shouldn't be aware of IME except for one method or property that says "I need IME when focused".

Comment 47

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

(In reply to comment #46)
> It's possible to create your own DHTML widget where you trap key events and you
> want to receive IME input. That's what FAYT is, but web authors could build
> their own as well. For example, a list that you can search by typing characters
> that's implemented using DHTML rather than using a standard <select>. (BTW does
> this patch disable IME in select drop-downs?)

I don't think so, IME composition string rendering and IME candidate list controling is very difficult. Maybe, it cannot implement by DHTML.
# especially, we cannot control IME candidate list from JS code.
If it is possible, the DHTML content cannot use on WinIE.
(WinIE disable IME on its non-Editor widgets having focus.)

> Also, the current patch makes a number of changes to FAYT. We really need to
> avoid that by at least making it easy to turn IME on/off for content in our own
> code. FAYT code shouldn't be aware of IME except for one method or property
> that says "I need IME when focused".
This is difficult issue. I don't have more simple idea for this...
Currently patch enable IME at starting FAYT input, and disable IME at finished FAYT input. Is this complicated? 

Comment 48

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

http://lxr.mozilla.org/mozilla/source/editor/libeditor/text/nsEditorEventListeners.cpp#438
438 nsTextEditorTextListener::HandleText(nsIDOMEvent* aTextEvent)
439 {

443    nsCOMPtr<nsIPrivateTextEvent> textEvent = do_QueryInterface(aTextEvent);
449    nsAutoString            composedText;
450    nsresult                result;
451    nsIPrivateTextRangeList *textRangeList;
452    nsTextEventReply        *textEventReply;

454    textEvent->GetText(composedText);
455    textEvent->GetInputRange(&textRangeList);
456    textEvent->GetEventReply(&textEventReply);

Roc:
See this code. We cannot process IME composition string in JS code.
We should use native code for IME processing.
So, we *cannot* create IME usable editor by DHTML.

For IME users, composition string and candidate list is most important UI. Without these, we cannot use IME.

FYI:
Current SeaMonkey's IME implementation is not good. Because the composition string doesn't render correctly, and candidate list is not positioned correct position. I think that SeaMonkey should port Firefox's Find Toolbar. But this is not scope of this bug. It is bug 97023.

Comment 49

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

> We really need to
> avoid that by at least making it easy to turn IME on/off for content in our own
> code. FAYT code shouldn't be aware of IME except for one method or property
> that says "I need IME when focused".

I have one more comment.

The SeaMonkey's FAYT codes *know* the IME.
Because it is handling the IME event for the Brwoser Window and it treats the Composition String itself.
So, FAYT code is controling the IME transactions itself already. 

Comment 50

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

I think that:

Currently, any scripts cannot process and control the IME on Gecko.
Your IME usable editor by DHTML is very interesting idea for me. However, we cannot implement it on current Gecko. Because we need to expand the API for IME transaction. But it's not scope of this bug. We don't have any reason for enabling IME on the non-Editor widgets(and non-Object element).

Comment 51

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

So these FAYT changes are actually just porting code over from Firefox to Seamonkey?

Should FAYT really be using an editor?

This patch is still a lot of code and new interfaces for something that doesn't seem on the surface to be very complicated. For example, can you explain why we need FocusContent events and the reply?

Comment 52

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

(In reply to comment #51)
> Should FAYT really be using an editor?

Right. We cannot process and control all IME features on current Gecko.

> For example, can you explain why we
> need FocusContent events and the reply?

If we don't use the reply of FocusContent events, we need to write IME enabling/disabling code in the FocusContent event on the *all* elements.
This is not useful for developers. Maybe, all developers will not be disabling IME on FocusContent events on new elements that is created by them. If so, I will need to fix to IME bugs on all new elements :-(

The reply system of thie patch, other developers don't need to care IME status. Because they doesn't change the reply value, the default value is disabling IME.

Comment 53

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

> (In reply to comment #51)
>> Should FAYT really be using an editor?
> 
> Right. We cannot process and control all IME features on current Gecko.

Right. We cannot process and control all IME features *on non-Editor widgets* on current Gecko.

Comment 54

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

After each focus change, can't we just check where the focus is and determine whether the IME should be disabled or not, rather than mess around with replies to the event?

Comment 55

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

I have no idea. Because HTML/PlainText editor is handling the focus event itself.
By HTML editor, almost elements can become Editor widgets also. Of course, they can become non-Editor widget too, in many cases.

Comment 56

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

And I think that if we can check the current focus is in Editor or not, we need many processes.

for Editor:
1. need to check that the current focus is set to Editor or not.
2. need to get the editor interface.
3. need to check that the Editor is password field or not.
4. (after bug 279246) need to get the -moz-ime-mode style.

for Object:
1. need to check that the current focus is set to Object or not.
2. need to check that the Object is using plug-in or not.

We need to check these on every focus changing times.
This is not reasonable. My approch is only changing the reply of FocusContent events if it should enable IME. This is very reasonable. There are no futility costs. And I think that my approach is simple for IME status controling.

Comment 57

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

Attachment: Patch rv2.1

Updating to current trunk.

Comment 58

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

Why can't we add a method to nsIContent alongside IsFocusable(), something like GetDesiredIMEState(), returning "disabled" as the default implementation in nsGenericElement? nsHTMLObjectElement, nsHTMLInputElement could override it and choose the result the same way they choose their focusability. We could call it and update the IME state from nsEventManager::ShiftFocusInternal. ShiftFocusInternal would also need to check if we're in an editable document and take that into account. Is this approach really hard? It doesn't seem so to me.

Comment 59

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

Fmm... I'll try to create new test patch.

Comment 60

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

Attachment: Patch rv3.0

This patch is more simple than previous one.
But this patch also has many minnor issues on FAYT of SeaMonkey(on Firefox, all green!).

I think that for FAYT module of SeaMonkey, we need to refact it. Because it has many bugs of the transaction managiment.(inputting transaction, finding transaction). If I fix all minor issues without refactoring, the code will be spaghetti.

Roc, I hope that we should fix this bug by this patch. And I should file a new bug for FAYT refactoring with Neil and Aaron.

Comment 61

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

Attachment: Patch rv3.1

Sorry, I fogot to change IID of nsIEventStateManager.

Comment 62

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

This looks really good!

+  // Suppress/Unsuppress IME state controlling for FAYT of SeaMonkey.
+  NS_IMETHOD NotifyStartFAYTInput(PRUint32* aIMEState) = 0;
+  NS_IMETHOD NotifyFinishFAYTInput() = 0;

except this looks really bad, putting in special FAYT code in the eventmanager. Will the refactoring of Seamonkey FAYT make this go away?

If so, I wonder if it would be better to just disable IME in Seamonkey FAYT until it can be fixed, instead of checking in special hack code. Without the Seamonkey FAYT support this patch would be even simpler. If you don't agree,  you should at least mark with comments the code that you expect to be removed by the Seamonkey FAYT work.

I'll have more comments tomorrow so you needn't revise the patch just yet.

Comment 63

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

> Will the refactoring of Seamonkey FAYT make this go away?

No, we need these methods after refactoring.
# If we will port the find toolbar to SeaMonkey, we can remove these methods.
# But I think that it is not time for porting. Because we have many bugs in
# find toolbar. I'm working on these bugs in Trunk. It is no good existing same
# modules in different modules.

In this patch, the IME controller is ESM. On FAYT of SeaMonkey, we must override the controlloring(always enable IME) by ESM. Don't you like the names of the methods? If so, we use following names instead:
nsIEventStateManager::FixIMEState(PRUint32 aUseState, PRUint32 *aCurrentState);
nsIEventStateManager::EnableIMEStateControl();

If not so, do you have any idea?

I have an idea but I don't like this. In the ESM, we make a pref for disable/enable IME controlling. And the default value is enable in all product.
But if the users likes FAYT more than this bug, they can disable IME controlling by pref themselves.

Comment 64

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

> I have an idea but I don't like this. In the ESM, we make a pref for
> disable/enable IME controlling. And the default value is enable in all product.
> But if the users likes FAYT more than this bug, they can disable IME
> controlling by pref themselves.

This means we can remove these methods and all changes in nsTypeAheadFind.*

Comment 65

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

I don't really like the idea of a pref either. And I don't want to check in some nasty code just to keep Seamonkey FAYT working if we have no clear plan to remove it.

I'm leaning towards disabling IME for Seamonkey FAYT for now. It is then Seamonkey people's problem to fix it to work like the Firefox find toolbar, or port the Firefox find toolbar. Hopefully they can do that within the 1.9 timeframe (the next year).

Comment 66

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

More comments:

+  /*
+   * Get desired IME state for the content.
+   *
+   * @return The desired IME status for the content.
+   *         That is combination of IME_STATUS_*.
+   */

You need to say what combinations of flags are allowed. E.g., I assume IME_STATUS_ENABLE | IME_STATUS_DISABLE is not allowed. You also need to say it means when a flag is not set (e.g., IME_STATUS_NONE) ... I assume it means "leave IME in its current state". And does DISABLE also force IME to close or should people set both?

"ControlIMEState" is probably better named "UpdateIMEState".

+  // On Print Preview, we don't need IME.
+  if (aPresContext->Type() == nsPresContext::eContext_PrintPreview) {

Printing too. We really need a boolean method to check whether the context is interactive, but you don't need to do that here.

+  NS_IMETHOD GetKBStateControl(nsPresContext* aPresContext,
+                               nsIKBStateControl** aResult);

I think we don't need this if we disable IME in Seamonkey FAYT, but if we do keep it, it doesn't need to be virtual, right?

+  [noscript] void getDesirableIMEStatus(out unsigned long aState);

Better name "getPreferredIMEState"?

I like how this simplifies editor code!

Comment 67

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

(In reply to comment #66)
> More comments:
> 
> +  /*
> +   * Get desired IME state for the content.
> +   *
> +   * @return The desired IME status for the content.
> +   *         That is combination of IME_STATUS_*.
> +   */
> 
> You need to say what combinations of flags are allowed. E.g., I assume
> IME_STATUS_ENABLE | IME_STATUS_DISABLE is not allowed.
Yes.

> You also need to say it
> means when a flag is not set (e.g., IME_STATUS_NONE) ... I assume it means
> "leave IME in its current state".
right.

> And does DISABLE also force IME to close or
> should people set both?
Sorry, I cannot understand this. What you mean?

> +  // On Print Preview, we don't need IME.
> +  if (aPresContext->Type() == nsPresContext::eContext_PrintPreview) {
> 
> Printing too. We really need a boolean method to check whether the context is
> interactive, but you don't need to do that here.
Did you mean that we need a new function, but I don't need create it in this time?

> +  NS_IMETHOD GetKBStateControl(nsPresContext* aPresContext,
> +                               nsIKBStateControl** aResult);
> 
> I think we don't need this if we disable IME in Seamonkey FAYT, but if we do
> keep it, it doesn't need to be virtual, right?
Right. Should I remove this too? we don't need this if we disable IME in FAYT.

thanks,

Comment 68

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

>> +  NS_IMETHOD GetKBStateControl(nsPresContext* aPresContext,
>> +                               nsIKBStateControl** aResult);
>> 
>> I think we don't need this if we disable IME in Seamonkey FAYT, but if we do
>> keep it, it doesn't need to be virtual, right?
> Right. Should I remove this too? we don't need this if we disable IME in FAYT.

Oops. We need this. for SetIMEState/GetIMEState. not related FAYT.

Comment 69

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

> > And does DISABLE also force IME to close or
> > should people set both?
> Sorry, I cannot understand this. What you mean?

If I set DISABLE, will that close an open IME, or should I set DISABLE | CLOSE?

> > Printing too. We really need a boolean method to check whether the context is
> > interactive, but you don't need to do that here.
> Did you mean that we need a new function, but I don't need create it in this
> time?

Yes

(In reply to comment #68)
> >> +  NS_IMETHOD GetKBStateControl(nsPresContext* aPresContext,
> >> +                               nsIKBStateControl** aResult);
> >> 
> >> I think we don't need this if we disable IME in Seamonkey FAYT, but if we do
> >> keep it, it doesn't need to be virtual, right?
> > Right. Should I remove this too? we don't need this if we disable IME in FAYT.
> 
> Oops. We need this. for SetIMEState/GetIMEState. not related FAYT.

Ah yes, you need GetKBStateControl(nsPresContext* aPresContext, nsIKBStateControl** aResult) for SetIMEState (and it should be nonvirtual). However, you don't need GetIMEState.

Comment 70

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

(In reply to comment #69)
> > > And does DISABLE also force IME to close or
> > > should people set both?
> > Sorry, I cannot understand this. What you mean?
> 
> If I set DISABLE, will that close an open IME, or should I set DISABLE | CLOSE?
We don't need CLOSE for disable. Because when enabled, the open state is recovered. If CLOSE is set too, the open state is CLOSED when enabled.

> > Oops. We need this. for SetIMEState/GetIMEState. not related FAYT.
> 
> However, you don't need GetIMEState.
Oh, you are right. 

Comment 71

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

Attachment: Patch rv4.0

O.K. Let's go.

Comment 72

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

Comment on attachment 203028 [details] [diff] [review]
Patch rv4.0

It's good.

+  if (aPresContext->Type() == nsPresContext::eContext_PrintPreview) {
+    SetIMEState(aPresContext, nsIContent::IME_STATUS_DISABLE);
+    return;
+  }

Please also check for printing here.

I'll give you some better text for the comment.

Comment 73

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

  * @return The desired IME status for the content.
  *         This is a combination of IME_STATUS_* flags,
  *         controlling what happens to IME when the content takes focus.
  *         If this is IME_STATUS_NONE, IME remains in its current state.
  *         IME_STATUS_ENABLE and IME_STATUS_DISABLE must not be set
  *         together; likewise IME_STATUS_OPEN and IME_STATUS_CLOSE must
  *         not be set together.
  *         If you return IME_STATUS_DISABLE, you should not set the
  *         OPEN or CLOSE flag; that way, when IME is next enabled,
  *         the previous OPEN/CLOSE state will be restored (unless the newly
  *         focused content specifies the OPEN/CLOSE state by setting the OPEN
  *         or CLOSE flag with the ENABLE flag).

Thanks for being patient. This is really good work.

Comment 74

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

Attachment: Patch rv4.1(for check-in)

Comment 75

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

Checked-in!

(In reply to comment #73)
> Thanks for being patient. This is really good work.

You too! Thank you very much!
# This and bug 113187 was disliked by IME users.
# But this fixed on XP code and Win code only. We need to work for Mac and Linux.

Comment 76

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

backed out the patch. Because with VC6, failed to build.

> ../../dist/include/content\nsIContent.h(582) : error C2252: 'IME_STATUS_NONE' : pure specifier can only be specified for functions
> ../../dist/include/content\nsIContent.h(583) : error C2258: illegal pure syntax, must be '= 0'
> ../../dist/include/content\nsIContent.h(583) : error C2252: 'IME_STATUS_ENABLE' : pure specifier can only be specified for functions
> ../../dist/include/content\nsIContent.h(584) : error C2258: illegal pure syntax, must be '= 0'
> ../../dist/include/content\nsIContent.h(584) : error C2252: 'IME_STATUS_DISABLE' : pure specifier can only be specified for functions
> ../../dist/include/content\nsIContent.h(585) : error C2258: illegal pure syntax, must be '= 0'
> ../../dist/include/content\nsIContent.h(585) : error C2252: 'IME_STATUS_OPEN' : pure specifier can only be specified for functions
> ../../dist/include/content\nsIContent.h(586) : error C2258: illegal pure syntax, must be '= 0'
> ../../dist/include/content\nsIContent.h(586) : error C2252: 'IME_STATUS_CLOSE' : pure specifier can only be specified for functions
> ../../dist/include/content\nsIContent.h(588) : error C2065: 'IME_STATUS_ENABLE' : undeclared identifier
> ../../dist/include/content\nsIContent.h(588) : error C2065: 'IME_STATUS_DISABLE' : undeclared identifier
> ../../dist/include/content\nsIContent.h(588) : error C2258: illegal pure syntax, must be '= 0'
> ../../dist/include/content\nsIContent.h(588) : error C2252: 'IME_STATUS_MASK_ENABLED' : pure specifier can only be specified for functions
> ../../dist/include/content\nsIContent.h(590) : error C2065: 'IME_STATUS_OPEN' : undeclared identifier
> ../../dist/include/content\nsIContent.h(590) : error C2065: 'IME_STATUS_CLOSE' : undeclared identifier
> ../../dist/include/content\nsIContent.h(590) : error C2258: illegal pure syntax, must be '= 0'
> ../../dist/include/content\nsIContent.h(590) : error C2252: 'IME_STATUS_MASK_OPENED' : pure specifier can only be specified for functions

Comment 77

[jag (Peter Annema)]

To make VC6 happy you'll have to initialize those PRUint32s outside the class.

Try using an enum instead:

  static enum IME_Status
  {
    IME_STATUS_NONE    = 0,
    IME_STATUS_ENABLE  = 1 << 0,
    IME_STATUS_DISABLE = 1 << 1,
    IME_STATUS_OPEN    = 1 << 2,
    IME_STATUS_CLOSE   = 1 << 3,
    IME_STATUS_MASK_ENABLED = IME_STATUS_ENABLE | IME_STATUS_DISABLE,
    IME_STATUS_MASK_OPENED  = IME_STATUS_OPEN | IME_STATUS_CLOSE
  }

and adjust your methods to take and return the enum instead of a bare PRUint32.

Comment 78

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

Thank you, I'm testing with following code:

  enum {
    IME_STATUS_NONE    = 0x0000,
    IME_STATUS_ENABLE  = 0x0001,
    IME_STATUS_DISABLE = 0x0002,
    IME_STATUS_OPEN    = 0x0004,
    IME_STATUS_CLOSE   = 0x0008
  };
  enum {
    IME_STATUS_MASK_ENABLED = IME_STATUS_ENABLE | IME_STATUS_DISABLE,
    IME_STATUS_MASK_OPENED  = IME_STATUS_OPEN | IME_STATUS_CLOSE
  };

If I will confirm this is O.K., I'll check-in without review.
Thanks.(Now, testing with VC6 on VMWare, So, I need an hour.)

Comment 79

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

Attachment: Patch rv4.2(for re-check-in)

Comment 80

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

checked-in.

Comment 81

[Olli Pettay [:smaug][[e-mail]]]

I'm now seeing this all the time:
WARNING: NS_ENSURE_TRUE(kb) failed, file nsEventStateManager.cpp, line 5557

Comment 82

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

Ah, do you using GTK2 or OS/2 or BeOS?

Comment 83

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

I filed a new bug for the comment 81. See bug 316805.

Comment 84

[chris hofmann]

see: https://bugzilla.mozilla.org/show_bug.cgi?id=339470

Comment 85

[Jungshik Shin]

Mnemonic still does not work even in the trunk. With the IME on and the focus in text area, I can invoke File menu by pressing Alt-F, but pressing 'o' with the file menu  does not open up a file picker. Instead, 'o' is processed by the IME in text area.  The very same symptom described in comment #0. 

Comment 86

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

right. but it should be separated to new bug. because we need to implement another mechanism for the editors. I have tried to it on Windows, but I failed it and I cannot find what way can fix this bug...

Comment 87

[Jungshik Shin]

Hmm... Why was this bug resolved as fixed when the symptom reported in comment #0 hadn't been fixed? Either this bug should be reopened or the bug summary should be changed to match what WAS fixed here?  If we do the latter, we should file a new bug about the original problem (have you filed one already?). 

Comment 88

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

When the editors doesn't have a focus, this bug was fixed. But there are still some issues, it is also right. Therefore, I marked to fixed.

I think that the bug on editor is a *new* bug of my approach in this bug. The xul menu windows cannot have focus, but they have the actual keyboard focus. Therefore, the patch doesn't work fine. I think that we should _suspend_ the composing temporarily at popping up the XUL menu and resume the composing at closing the *all* XUL menus.

# I didn't file a new bug.