16940 - [FEATURE] need an XP API to disable input methods for password fields

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P1)

Description

[Erik van der Poel]

Frank, since you are currently looking after XP input method issues, I've
initially assigned this bug to you. We need an XP API to turn off input methods
for example for password fields where we should not be seeing the IM text.

After designing and checking in the XP API, we need the owners of each platform
(Unix, Windows, Mac, etc) to implement for their platform.

Comment 1

[Kathleen :Brade]

cc [e-mail] since he did work on the password input control.
Change component to HTML form controls.

Comment 2

[buster]

could this be as simple (and XP?) as turning off IME-specific DOM events based
on a flag passed to the editor and/or IME system?

Comment 3

[Frank Tang]

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

Comment 4

[Frank Tang]

Move to M13.

Comment 5

[Frank Tang]

I don't think I can make it M13. Move to M14.

Comment 6

[Teruko Kobayashi]

Added [BETA] in summary and changed QA contact to [e-mail].

Comment 7

[Frank Tang]

Move this to M16. This is a new feature (compare to NS 4.0) Make it post Beta 1

Comment 8

[Frank Tang]

I don't think we can make it for BETA. move it to M16

Comment 9

[Hidetoshi Tajima]

Attachment: call PasswordFieldInit when editor is password field

Comment 10

[Hidetoshi Tajima]

Frank, please review the patch in the attachment,
and have it tested on Windows and Mac, also. We've
tested it on Unix, i386 Linux and SPARC Solaris.

Comment 11

[Masaki Katakai]

Attached new version of patch in 17419 for nsEditor.cpp.
Please review the codes and my comment in the bug report. Thanks.

Comment 12

[Frank Tang]

info- 4.x Window / Linux disable IME on password while Mac don't
Move to M20 and mark as "Help wanted"

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I read this as "it is impossible for passwords to have anything other than 
ASCII, so the IME is not necessary." Is that correct? It seems odd that non-
English systems would require English/Latin-1 passwords--Japanese people 
wouldn't be able to type Japanese passwords for use on remote Japanese sites, 
for example. It seems that instead of disabling the input method, you really 
just want to disable the DISPLAY of the characters. Otherwise, how could my 
password be しのはら (Shinohara). I run Microsoft Japanese IME on Engish Win32 
systems, so I could test this out if you are looking for help.

Comment 14

[Simon Fraser [no longer active]]

Taking on to modify editor APIs to make this easier.

Comment 15

[ekrock's old account (dead)]

Brian, I lived five years in Japan and all the passwords I ever saw were ASCII. 
Reason: you wouldn't want to reveal characters of your password by bringing up 
an IME editor window to convert the ASCII characters into 
hiragana/katakana/kanji. If behavior has changed in last 3.5 years, someone 
please let me know, but I doubt it has. So support for non-ASCII passwords 
shouldn't be considered a product requirement for Japanese at least. Expect the 
same is true for other languages. 

So Win/Linux automatically disable IME on password fields, but Mac doesn't? Do 
we have to explicitly disable this on the Mac? Is this therefore a 
Mac-specific issue now? What problems occur if we don't do this?

Resetting to FUTURE. Can anyone show a product /user need for this 
functionality?

Comment 16

[Masaki Katakai]

Attachment: patch but code base is 24 Apr

Comment 17

[Masaki Katakai]

I had discussed the solution with [e-mail] and
[e-mail] and I had posted a patch to
[e-mail], [e-mail]
on 24 Apr 2000. But I didn't get any answers yet.

Comment 18

[Ryuichi KUBUKI]

I totally agree with comment #15. You can't tell it is ASCII or non-ASCII when
your own input is masked. But it's still reasonable that Mozilla accepts
non-ASCII password input with IME on if it can by setting about:config option
true, or by pasting non-ASCII string on password input box.

That said, why this bug and its proposed patch had been forgotten 3 years?
I had this bug on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b)
Gecko/20030822 Mozilla Firebird/0.6.1+ and possibly on the nightly of Mozilla of
the same date. Not sure about Linux and Mac, but on Windows XP it doesn't
disable IME now.

Comment 19

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

Attachment: Patch for nsEditor

Comment 20

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

Attachment: patch for nsIKBStateControl

Comment 21

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

Attachment: Patch for Windows

Comment 22

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

Attachment: Patch for Other OS(non-implemented)

Comment 23

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

Attachment: Patch for all.js

Comment 24

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

I created above patch.

Spec of the patch.
1. On getting focus, if the editor is password field, IME is closed.
2. On lost focus, if the editor has closed when getting focus, IME is reopened.
3. If IME is used on password field, the editor don't care for IME when lost focus.
4. These behavior can be killed by pref.js.

Note:
I cannot test to build on Linux and Mac and others.
So, attachment 153594 [details] [diff] [review] is not tested.

Comment 25

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

Who can test and review attachment 153594 [details] [diff] [review]?

Comment 26

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

test case
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2327&action=view

Comment 27

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

Attachment: Patch for Photon(not implemented)

Comment 28

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

Attachment: Patch for Cocoa(not implemented)

Comment 29

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

Attachment: Patch for Mac(not implemented)

Comment 30

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

Attachment: Patch for gtk(not implemented)

Comment 31

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

Is mozilla/widget/src/cocoa not maintained?

Comment 32

[timeless]

it's maintained. camino uses it.

Comment 33

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

I hope that this bug is fixed early.
Because I want to fix the other IME bugs before 1.8 final.
The other IME bugs need to create alike patch of this bug's patch.

Please help me for fixing this bug. 

Comment 34

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 153652 [details] [diff] [review]
Patch for Cocoa(not implemented)

these need an implementation, don't they? the mac version has a stub impl.

Comment 35

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 153653 [details] [diff] [review]
Patch for Mac(not implemented)

r=pink, but who is going to implement these?

Comment 36

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

Mike:

Thank you.

> these need an implementation, don't they? the mac version has a stub impl.

I think it.
The method implement in nsChildView.mm?
# nsCocoaWindow has IKBStateControl's method, but the interface is not inherited.

> but who is going to implement these?

It is most difficult issue.
But the necessary code is simple.
I recruit hackers who hack the code for non-Windows platforms in Bugzilla-jp.

Comment 37

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

Adding reviewers to CC.

Comment 38

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

Attachment: Patch for Cocoa(not implemented) rv. 2

Comment 39

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 153684 [details] [diff] [review]
Patch for Cocoa(not implemented) rv. 2

r=pink

Comment 40

[Ere Maijala (slow)]

Comment on attachment 153593 [details] [diff] [review]
Patch for Windows

> +      bRtn = (BOOL)theIMM.GetOpenStatus(hIMC);  \

bRtn is PRBool, right? I'd prefer 
*bRtn = theIMM.GetOpenStatus(hIMC) ? PR_TRUE : PR_FALSE;
(note that I'd also move * to the define as it is in the others -- it looks
more like a proper function call)
Same with the setter when converting from PRBool to BOOL.

> +      return (mGetOpenStatus) ? mGetOpenStatus(h) : 0L;
and 
> +      return (mSetOpenStatus) ? mSetOpenStatus(h,b) : 0L;

That should be |: FALSE;|

Comment 41

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

Ere:

Thank you for review.

> bRtn is PRBool, right? I'd prefer 
> *bRtn = theIMM.GetOpenStatus(hIMC) ? PR_TRUE : PR_FALSE;

It is my mistake.
But I think that should |bRtn| be Bool?
And should converting Bool to PRBool is outer macro?

Comment 42

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

# Additional above comment.
i.e.,

  if (hIMC) {
    Bool isOpen;
    NS_IMM_GETOPENSTATUS(hIMC, isOpen);
    *aState = isOpen ? PR_TRUE : PR_FALSE;
    NS_IMM_RELEASECONTEXT(mWnd, hIMC);

Comment 43

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

Attachment: Patch for Windows rv. 1.1

I think that NS_IMM_GETOPENSTATUS is simple wrapper of Win32API.
Therefore the |bRtn| is BOOL.
For nsIKBStateContrl, the value is converted outer macro.

Comment 44

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

Comment on attachment 153592 [details] [diff] [review]
patch for nsIKBStateControl

I think the comments need a bit more information. "Open or Close" is not quite
descriptive enough. Should SetImeOpenState *force* the IME to be enabled or
disabled? If so I think you should rename it "SetIMEEnabled".

Comment 45

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

Robert:

> I think the comments need a bit more information. "Open or Close" is not quite
> descriptive enough. 

O.K. I rewrite comment.

> Should SetImeOpenState *force* the IME to be enabled or
> disabled? If so I think you should rename it "SetIMEEnabled".

No! These methods are changing Open or Close. not Enabled or Disabled.
On Mozilla's widget, IME is always Enabled.
I cannot rename it.
Because we may need the name for other bugs.

# If IME is Disabled in the window, open state is always 'closed'.
# And the state is not able to be changed.

Comment 46

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

By the way, what would be *really* cool is if (after checking in this patch)
nsIKBStateControl was extended to support "inputmode":

http://www.whatwg.org/specs/web-forms/2004-06-27-call-for-comments/#the-inputmode

Comment 47

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

Alright, then can you say in the comment exactly what "open" and "closed" means?
I assume it means something like "In the 'Closed' state, no user input is
visible on the screen".

Comment 48

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

# Sorry, my English is not enough...

                    | 'Opened'                        | 'Closed'
--------------------+---------------------------------+--------------------------
IME Eanbled Window  | User can input any character.   | User can input the ASCII
                    | i.e., Users can input Japanese  | characters only. This 
                    | character and others.           | state same as non IME
                    | User can change to 'Closed'.    | users environment.
                    |                                 | User can change to     
                
                    |                                 | 'Opend'.
--------------------+---------------------------------+--------------------------
IME Disabled Window | N/A                             | Same as when 'Enabled'.
                    |                                 | But user cannot change to
                    |                                 | 'Opened'.

Comment 49

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

Okay, so put in the comment:

'Opened' means the user can input any character. I.e., users can input Japanese  
and other characters. The user can change the state to 'Closed'.
'Closed' means the user can input ASCII characters only. This is the same as a
non-IME environment. The user can change the state to 'Opened'.

Comment 50

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

Attachment: Patch for nsIKBStateControl

Thank you for you wrote the comment.

Comment 51

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

Thank you! Robert.

Daniel, Alec, Adrian, Stuart, Ere:
Please review other patchs.

Comment 52

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

Adrian:
Thank you for the review.

Comment 53

[Ere Maijala (slow)]

Comment on attachment 153733 [details] [diff] [review]
Patch for Windows rv. 1.1

> +    NS_IMM_SETOPENSTATUS(hIMC, aState);

You should have a conversion to BOOL here. Otherwise it looks ok. I'll withdraw
my previous notes about the parameter style as it's consistent now.

Comment 54

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

Attachment: Patch for Windows rv. 1.2

Comment 55

[Ere Maijala (slow)]

Comment on attachment 153929 [details] [diff] [review]
Patch for Windows rv. 1.2

Sorry, I should've been more precise. I'd like to have it read

+    NS_IMM_SETOPENSTATUS(hIMC, aState ? TRUE : FALSE);

This is to avoid possible problems in different types. To be frank, it wouldn't
matter in this case, but it's a good practise (it would be a real problem for
example when converting to VARIANT_BOOL where VARIANT_TRUE is 0xFFFF).

Comment 56

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

Attachment: Patch for Windows rv. 1.3

Comment 57

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

Simon: 

Do you can review 'Patch for nsEditor'?
I sent mail to Daniel, But no reply...

Comment 58

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

Comment on attachment 153654 [details] [diff] [review]
Patch for gtk(not implemented)

I change reviewer to Blizzard.

Blizzard:
Please review the patch.

Comment 59

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

Comment on attachment 153591 [details] [diff] [review]
Patch for nsEditor

I change reviwer to Simon.

Blizzard:
Thank you for the review.

Comment 60

[Simon Fraser [no longer active]]

Comment on attachment 153591 [details] [diff] [review]
Patch for nsEditor

>Index: idl/nsIEditorIMESupport.idl
>===================================================================

>+  
>+  /**
>+   * Care for IME when the editor got focus.
>+   */
>+
>+  [noscript] void CareForImeOnFocus();
>+  
>+  /**
>+   * Care for IME when the editor lost focus.
>+   */
>+
>+  [noscript] void CareForImeOnBlur();
>+

These methods need better names. I'm not sure what the "CareFor"
implies.

Perhaps "NotifyImeOnBlur()" or "OnImeOnBlue()" ?

>Index: libeditor/base/nsEditor.cpp
>===================================================================

>+  // If IME state changeing on the field,

"changing"

I think this comment needs a little more detail; it's not obvious
to me why this flag is required.

>+  // don't recover the IME open state.
>+  if (mNeedRecoverIMEOpenState)
>+    mNeedRecoverIMEOpenState = PR_FALSE;
>+
>   return ret;
> }

> NS_IMETHODIMP
>+nsEditor::CareForImeOnFocus()
>+{
>+  mNeedRecoverIMEOpenState = PR_FALSE;
>+
>+  nsresult result;
>+  PRBool dontCareForIme;
>+  nsCOMPtr<nsIPrefBranch> prefBranch = 
>+      do_GetService(NS_PREFSERVICE_CONTRACTID, &result);
>+  if (NS_SUCCEEDED(result) && prefBranch) {
>+    result = prefBranch->GetBoolPref("ime.password.onFocus.dontCare", &dontCareForIme);
>+    if (NS_FAILED(result))
>+      return result;
>+  }

Do we really have to fetch a service and read a pref every time we unfocus a
text field?
I think it would be much better to get the pref once and cache it.

>+NS_IMETHODIMP
>+nsEditor::CareForImeOnBlur()

Most of this method looke the same as the previous method. Can you factor the
code into one shared private method?


>Index: libeditor/base/nsEditor.h

>   /* ------------ nsIEditorIMESupport methods -------------- */
>   
>   NS_IMETHOD BeginComposition(nsTextEventReply* aReply);
>   NS_IMETHOD QueryComposition(nsTextEventReply* aReply);
>   NS_IMETHOD SetCompositionString(const nsAString& aCompositionString, nsIPrivateTextRangeList* aTextRangeList,nsTextEventReply* aReply);
>   NS_IMETHOD EndComposition(void);
>   NS_IMETHOD ForceCompositionEnd(void);
>   NS_IMETHOD GetReconversionString(nsReconversionEventReply *aReply);
>+  NS_IMETHOD CareForImeOnFocus();
>+  NS_IMETHOD CareForImeOnBlur();

Can we replace this block with the IDL-generated macro
NS_DECL_NSIEDITORIMESUPPORT?

>   // data necessary to build IME transactions
>   PRBool                        mInIMEMode;        // are we inside an IME composition?
>   nsIPrivateTextRangeList*      mIMETextRangeList; // IME special selection ranges
>   nsCOMPtr<nsIDOMCharacterData> mIMETextNode;      // current IME text node
>   PRUint32                      mIMETextOffset;    // offset in text node where IME comp string begins
>   PRUint32                      mIMEBufferLength;  // current length of IME comp string
>   PRBool                        mIsIMEComposing;   // is IME in composition state?
>                                                    // This is different from mInIMEMode. see Bug 98434.
>+  PRBool                        mNeedRecoverIMEOpenState;   // Need IME open state 

Please use PRPackedBool for mIsIMEComposing and mNeedRecoverIMEOpenState to
save 2 bytes.

Comment 61

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

Comment on attachment 153595 [details] [diff] [review]
Patch for all.js

Simon:

Thank you for the review.

I have a problem.
Alec Flett said "Please find another reviewer".
But nobody are modules/libpref's owner.
Who can review it?

Comment 62

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

Comment on attachment 153595 [details] [diff] [review]
Patch for all.js

Simon:

Please review this patch.
Nobady is the module owner, I think the alternate reviwer should be the owner
of module that useing new pref item.

Comment 63

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

Attachment: Patch for nsEditor rv. 2

Comment 64

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

Who can superreview for other patch? Blizzard? (intl and widget)

Comment 65

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

Blizzard:

Please super review the patch of widget.

dveditz:

Please review the patch for 'all.js'.
I'm adding to the generic name.
The reason is 'editor' and 'intl' is not suitable.
The IME bugs have three faces.
1. IME's bug is internationalization bug.
2. IME is very important UI for CJK.
3. IME's bugs are very important for accessibility.

I will fix some IME's bug after fixed this bug.
In the case, I will add many prefs for IME accessibility.
# These prefs may be very important for IME users.

Therefore I think that the prefs should be grouped by "ime".
Those prefs should not be grouped by "intl" or "editor".

I'm adding two prefs for this bug.

1. "ime.password.onFocus.dontCare"
This is "care for IME" is enabled or disabled when the password field
got focus.

2. "ime.password.onBlur.dontCare"
Same to above. But when the password field lost focus.

Comment 66

[Simon Fraser [no longer active]]

Comment on attachment 154075 [details] [diff] [review]
Patch for nsEditor rv. 2

>Index: libeditor/base/nsEditor.cpp
>===================================================================

>+  nsresult result;
>+  nsCOMPtr<nsIPrefBranchInternal> prefBranch = 
>+      do_GetService(NS_PREFSERVICE_CONTRACTID, &result);
>+  if (NS_SUCCEEDED(result) && prefBranch) {
>+    prefBranch->AddObserver(NS_PREF_IME_ON_FOCUS_PASSWORD, this, PR_FALSE);
>+    prefBranch->AddObserver(NS_PREF_IME_ON_BLUR_PASSWORD, this, PR_FALSE);
>+  }

Are pref listeners really required? When are these prefs going to change at
runtime?

Comment 67

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

(In reply to comment #66)
> Are pref listeners really required? When are these prefs going to change at
> runtime?

Why?
If these listners is none, it cannot be changed directry from "about:config".

Comment 68

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

dveditz:

See comment 65 before review.

Comment 69

[Simon Fraser [no longer active]]

> it cannot be changed directry from "about:config".
I'm not convinced that this justifies the code bloat. If you make a new browser
window, the new settings will apply there. If you quit and restart, the new
settings will apply.

Comment 70

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

O.K.
Please wait.

Comment 71

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

Attachment: Patch for nsEditor rv. 3

The prefs cache move to global variable.
Because when the pref is changed and new editor is created, 
the changing is influence to old editors.

Comment 72

[[e-mail]]

Comment on attachment 154130 [details] [diff] [review]
Patch for nsEditor rv. 3

Random comment: no point packing globals, as you're only saving bytes once.

Comment 73

[[e-mail]]

Comment on attachment 153595 [details] [diff] [review]
Patch for all.js

sr=me once the pref names are finalized ;-)

Comment 74

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

Attachment: Patch for nsEditor rv. 3.1

Thank you neil.

PRPackedBool -> PRBool in global variables.

Comment 75

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

Attachment: Patch for nsEditor rv. 3.2

I rewrote |ForceCompositionEnd| with |GetKBStateControl|.

Comment 76

[Simon Fraser [no longer active]]

Comment on attachment 154175 [details] [diff] [review]
Patch for nsEditor rv. 3.2

Final question: would anyone ever want different values for the two prefs?

r=sfraser

Comment 77

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

I don't know.
But our(bugizlla-jp) opinion are separated, which behavior is good.

OnFoucus = false, OnBlur = false
IE, Nestcape4.x compatible(it is not correctly, these UA are "disabled" in the
password field.)

OnFocus = true, OnBlur = false
Opera compatible

OnFocus = true, OnBlur = true
Current Mozilla compatible.

Comment 78

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

timeless:

I sent the mail to [e-mail].
But no reply.

Can you contact to [e-mail]? or Do you know another reviewer?

Comment 79

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

Comment on attachment 154175 [details] [diff] [review]
Patch for nsEditor rv. 3.2

I change superreviewer to Simon.

Simon: 
Please super-review.

Comment 80

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

Daniel Veditz, Christopher Blizzard, Simon Fraser:

Please review or superreview.

Comment 81

[Daniel Veditz [:dveditz]]

Comment on attachment 153595 [details] [diff] [review]
Patch for all.js

Don't see why these couldn't be intl.ime.foo to help battle the
overproliferation of toplevel pref branches, but OK. r=dveditz

Comment 82

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

I changed superreviewer.
-> [e-mail]

Comment 83

[Brian Ryner (not reading)]

Comment on attachment 153962 [details] [diff] [review]
Patch for Windows rv. 1.3

>--- nsWindow.cpp	18 Jul 2004 17:36:03 -0000	3.508
>+++ nsWindow.cpp	22 Jul 2004 04:17:29 -0000
>+//==========================================================================
>+NS_IMETHODIMP nsWindow::SetImeOpenState(PRBool aState)

Capitalize IME to be consistent with other methods on this class.


>--- nsWindow.h	25 May 2004 14:33:50 -0000	3.177
>+++ nsWindow.h	22 Jul 2004 04:17:30 -0000
>@@ -162,16 +165,22 @@ public:
> 	    mSetCompositionWindow=(mInstance) ? (SetCompWindowPtr)GetProcAddress(mInstance,"ImmSetCompositionWindow") : 0;
> 	    NS_ASSERTION(mSetCompositionWindow!=NULL,"nsIMM.ImmSetCompositionWindow failed.");
> 
> 	    mGetProperty=(mInstance) ? (GetPropertyPtr)GetProcAddress(mInstance,"ImmGetProperty") : 0;
> 	    NS_ASSERTION(mGetProperty!=NULL,"nsIMM.ImmGetProperty failed.");
> 
> 	    mGetDefaultIMEWnd=(mInstance) ? (GetDefaultIMEWndPtr)GetProcAddress(mInstance,"ImmGetDefaultIMEWnd") : 0;
> 	    NS_ASSERTION(mGetDefaultIMEWnd!=NULL,"nsIMM.ImmGetDefaultIMEWnd failed.");
>+
>+      mGetOpenStatus=(mInstance) ? (GetOpenStatusPtr)GetProcAddress(mInstance,"ImmGetOpenStatus") : 0;
>+      NS_ASSERTION(mGetOpenStatus!=NULL,"nsIMM.ImmGetOpenStatus failed.");
>+
>+      mSetOpenStatus=(mInstance) ? (SetOpenStatusPtr)GetProcAddress(mInstance,"ImmSetOpenStatus") : 0;
>+      NS_ASSERTION(mSetOpenStatus!=NULL,"nsIMM.ImmSetOpenStatus failed.");

Try to be consistent with indentation.

sr=bryner with those fixed.

Comment 84

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

Brian:

Thank you for superreview!

> Try to be consistent with indentation.

It is fixed by bug 253035.

Comment 85

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

Robert O'Callahan:

Brian wrote:
>>--- nsWindow.cpp	18 Jul 2004 17:36:03 -0000	3.508
>>+++ nsWindow.cpp	22 Jul 2004 04:17:29 -0000
>>+//==========================================================================
>>+NS_IMETHODIMP nsWindow::SetImeOpenState(PRBool aState)
> 
> Capitalize IME to be consistent with other methods on this class.

This method is inherited from nsIKBStateControl.
Do you allow to capitalize?

Comment 86

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

Simon and Daniel:

Should I rename the name of new functions on nsEditor too?
e.x.,
NotifyImeOnFocus -> NotifyIMEOnFocus

Comment 87

[Daniel Veditz [:dveditz]]

Comment on attachment 154175 [details] [diff] [review]
Patch for nsEditor rv. 3.2

>Index: idl/nsIEditorIMESupport.idl
>===================================================================
>+  [noscript] void NotifyImeOnFocus();
>+  [noscript] void NotifyImeOnBlur();

Please use all caps on "IME" here.

Since you're changing an interface you ought to modify the uuid as well.


>Index: libeditor/base/nsEditor.cpp
>===================================================================
>+// Value of "ime.password.onFocus.dontCare"
>+static PRBool gDontCareForImeOnFocusPassword = PR_FALSE;
>+// Value of "ime.password.onBlur.dontCare"
>+static PRBool gDontCareForImeOnBlurPassword  = PR_FALSE;

Capitalize IME here, too (the variables, not the pref names).

With those changes sr=dveditz

Comment 88

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

Attachment: Patch of widget/ for check in

Comment 89

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

Attachment: Patch of widget/ for check in

Comment 90

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

Attachment: Patch of editor/ for check in

Comment 91

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

Attachment: Patch of modules/libpref/src/init/ for check in

Comment 92

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

Comment on attachment 158317 [details] [diff] [review]
Patch of modules/libpref/src/init/ for check in

Daniel:

Please check in.

Comment 93

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

Attachment: Patch of editor/ for check in

Comment 94

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

Who can check in?

Comment 95

[Simon Montagu :smontagu]

Patches checked in. Masayuki-san, can you please double check that the checkin
included everything necessary?

Comment 96

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

> Masayuki-san, can you please double check that the checkin
> included everything necessary?

Sorry. I cannot understand. What's "double check"?

Comment 97

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to comment #96)

> Sorry. I cannot understand. What's "double check"?

Can you verify that the checkin is not missing something?

Comment 98

[Simon Montagu :smontagu]

backed out because of build bustage.

Comment 99

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

Umm..

I don't have Linx, Mac, and others.
That should be corrected, how...

Comment 100

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

Attachment: Patch of widget/ ( not completed)

I think this patch can be build on Linux(gtk), but no Mac.
Who can test it?

Comment 101

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

Attachment: Patch of widget/

O.K. 

On Linux, I mistaked the implmentation class.
I changed to implement nsWindow -> nsWidget.

On Mac and Photon, I forgot class name.
I changed 'NS_IMETHODIMP GetIMEOpenState' 
 -> 'NS_IMETHODIMP nsWidget::GetIMEOpenState'.

Please help the build test or re-check in.

Comment 102

[Jungshik Shin]

With attachment 158521 [details] [diff] [review] and attacahement 158338, my Linux build went through, but
with attachment 158521 [details] [diff] [review] and attachment 158338 [details] [diff] [review], my Mac build failed. 

/Users/jungshik/prj/moz/src/mozilla/editor/libeditor/base/nsEditor.cpp: In 
   member function `virtual nsresult nsEditor::NotifyImeOnFocus()':
/Users/jungshik/prj/moz/src/mozilla/editor/libeditor/base/nsEditor.cpp:2118: err
or: `
   GetImeOpenState' undeclared (first use this function)
/Users/jungshik/prj/moz/src/mozilla/editor/libeditor/base/nsEditor.cpp:2118: err
or: (Each
   undeclared identifier is reported only once for each function it appears 
   in.)
/Users/jungshik/prj/moz/src/mozilla/editor/libeditor/base/nsEditor.cpp:2123: err
or: `
   SetImeOpenState' undeclared (first use this function)
/Users/jungshik/prj/moz/src/mozilla/editor/libeditor/base/nsEditor.cpp: In 
   member function `virtual nsresult nsEditor::NotifyImeOnBlur()':
/Users/jungshik/prj/moz/src/mozilla/editor/libeditor/base/nsEditor.cpp:2151: err
or: `
   SetImeOpenState' undeclared (first use this function)

Comment 103

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

Jungshik Shin:

Thank you for the test.

But, on Mac, Do you mistake the patches?
In attachment 158338 [details] [diff] [review], 'Ime' is not existing, but 'IME' is existing.

Comment 104

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

Oops...

Jungshik Shin:

Do you build Linux with gtk? or gtk2?
The patch is for gtk, not gtk2.

Comment 105

[Jungshik Shin]

Can you tell me exactly which set of patches to apply on Mac and Linux,
respectively? It's rather confusing. 

My Linux build is gtk2, but it just went through fine. Should I test it with gtk
as well? 

Comment 106

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

The set of attachment 158317 [details] [diff] [review], attachment 158338 [details] [diff] [review], attachment 158521 [details] [diff] [review] is correctly.
But, these patch don't have 'Ime' for name of method.
However your error message has 'Ime'.
'Ime' is deprecated. 'IME' is, now.

> Should I test it with gtk as well? 
Would you please test this for me.

Comment 107

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

In bugzilla-jp, Mac build went through fine.
Thank you.

Comment 108

[Jungshik Shin]

I've just built a Mac build as well. (I must have mixed up patches in my first
try as you wrote). So Mac part is all right. I really can't make a gtk build at
the moment. I'll  see if I can do it on another machine. 

Comment 109

[Jungshik Shin]

Linux-gtk is all right, too. attachment 158317 [details] [diff] [review], attachment 158338 [details] [diff] [review], attachment
158521 [details] [diff] [review] were checked in. 

Comment 110

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

Thank you Jungshik Shin and all reviewers and superreviewers.

I close this bug.
Because this bug was fixed on XP source code.

On non-Windows, this bug is the problem of to implement of nsIKBStateControl.
So, this bug should be separated to each OS bugs.

-> FIXED

thank you.

Comment 111

[Jungshik Shin]

Perhaps, it's too late (and possibly risky) to add this to aviary-1.0, but I
just wonder what others think about that. This bug has been reported multiple
times at www.mozilla.or.kr

Comment 112

[Gashu]

to comment 111
It is probably much pointed on Netscape7 with Ja IME as well. and definitely
annoying thing for every input method users. It should be FIXED on aviary-1.0 if
possible so I mark flags as "?".

Can anyone clarify it is possible of too risky?

# should I reopen this for aviary-1.0?

Comment 113

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

This bug was fixed on Windows only.
Nobody made the patch for new IME API for Mac.

Comment 114

[Asa Dotzler [:asa]]

If the patch author would like to request that this be landed for aviary, he can
request approval in the patch manager. 

Comment 115

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

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