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

RESOLVED FIXED in Future

Status

()

P1
major
RESOLVED FIXED
19 years ago
12 years ago

People

(Reporter: erik, Assigned: masayuki)

Tracking

({helpwanted, intl})

Trunk
Future
helpwanted, intl
Points:
---
Bug Flags:
blocking-aviary1.0PR -
blocking-aviary1.0 -
blocking-aviary1.0mac -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(13 attachments, 14 obsolete attachments)

948 bytes, patch
Details | Diff | Splinter Review
9.94 KB, patch
Details | Diff | Splinter Review
1.76 KB, patch
dveditz
: review+
Details | Diff | Splinter Review
1.76 KB, patch
amardare
: review+
bryner
: superreview+
Details | Diff | Splinter Review
1.77 KB, patch
mikepinkerton
: review+
bryner
: superreview+
Details | Diff | Splinter Review
1.89 KB, patch
blizzard
: review+
bryner
: superreview+
Details | Diff | Splinter Review
1.71 KB, patch
mikepinkerton
: review+
bryner
: superreview+
Details | Diff | Splinter Review
1.59 KB, patch
roc
: review+
Details | Diff | Splinter Review
8.41 KB, patch
emaijala+moz
: review+
bryner
: superreview+
Details | Diff | Splinter Review
12.08 KB, patch
sfraser_bugs
: review+
dveditz
: superreview+
Details | Diff | Splinter Review
1.77 KB, patch
masayuki
: review+
masayuki
: superreview+
Details | Diff | Splinter Review
12.72 KB, patch
masayuki
: review+
masayuki
: superreview+
Details | Diff | Splinter Review
17.12 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

19 years ago
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.

Updated

19 years ago
Component: XP Toolkit/Widgets → HTML Form Controls

Comment 1

19 years ago
cc buster@netscape.com since he did work on the password input control.
Change component to HTML form controls.

Updated

19 years ago
Status: NEW → ASSIGNED
Target Milestone: M12

Comment 2

19 years ago
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

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

Updated

19 years ago
Target Milestone: M12 → M14

Updated

19 years ago
Target Milestone: M14 → M13

Comment 4

19 years ago
Move to M13.

Updated

19 years ago
Priority: P3 → P1

Updated

19 years ago
QA Contact: claudius → ckritzer

Updated

19 years ago
Target Milestone: M13 → M14

Comment 5

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

Updated

19 years ago
QA Contact: ckritzer → teruko
Summary: need an XP API to disable input methods for password fields → [BETA] need an XP API to disable input methods for password fields

Comment 6

19 years ago
Added [BETA] in summary and changed QA contact to teruko@netscape.com.

Comment 7

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

Comment 8

19 years ago
I don't think we can make it for BETA. move it to M16
Summary: [BETA] need an XP API to disable input methods for password fields → need an XP API to disable input methods for password fields
Target Milestone: M14 → M16

Comment 9

19 years ago
Created attachment 6000 [details] [diff] [review]
call PasswordFieldInit when editor is password field

Comment 10

19 years ago
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

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

Updated

19 years ago
Keywords: beta2
Whiteboard: estimate- 2-3 days of work total on 3 platform

Comment 12

19 years ago
info- 4.x Window / Linux disable IME on password while Mac don't
Move to M20 and mark as "Help wanted"
Keywords: beta2
Whiteboard: estimate- 2-3 days of work total on 3 platform → [Help Wanted]
Target Milestone: M16 → M20
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

19 years ago
Taking on to modify editor APIs to make this easier.
Assignee: ftang → sfraser
Status: ASSIGNED → NEW

Updated

19 years ago
Target Milestone: M20 → M17
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?
Keywords: helpwanted
Summary: need an XP API to disable input methods for password fields → [FEATURE] need an XP API to disable input methods for password fields
Whiteboard: [Help Wanted]
Target Milestone: M17 → Future

Comment 16

19 years ago
Created attachment 9427 [details] [diff] [review]
patch but code base is 24 Apr

Comment 17

19 years ago
I had discussed the solution with sfraser@netscape.com and
jfrancis@netscape.com and I had posted a patch to
mozilla-i18n@mozilla.org, mozilla-editor@mozilla.org
on 24 Apr 2000. But I didn't get any answers yet.

Updated

18 years ago
Keywords: intl

Comment 18

15 years ago
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.
Created attachment 153591 [details] [diff] [review]
Patch for nsEditor
Created attachment 153592 [details] [diff] [review]
patch for nsIKBStateControl
Created attachment 153593 [details] [diff] [review]
Patch for Windows
Created attachment 153594 [details] [diff] [review]
Patch for Other OS(non-implemented)
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.
(Assignee)

Updated

15 years ago
Attachment #153593 - Flags: review?(ere)
(Assignee)

Updated

15 years ago
Attachment #153591 - Flags: review?(daniel)
(Assignee)

Updated

15 years ago
Attachment #153592 - Flags: review?(roc)
(Assignee)

Updated

15 years ago
Attachment #153595 - Flags: review?(alecf)
Created attachment 153651 [details] [diff] [review]
Patch for Photon(not implemented)
Attachment #153594 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #153651 - Flags: review?(amardare)
Created attachment 153652 [details] [diff] [review]
Patch for Cocoa(not implemented)
Created attachment 153653 [details] [diff] [review]
Patch for Mac(not implemented)
(Assignee)

Updated

15 years ago
Attachment #153653 - Flags: review?(pinkerton)
Created attachment 153654 [details] [diff] [review]
Patch for gtk(not implemented)
(Assignee)

Updated

15 years ago
Attachment #153654 - Flags: review?(pavlov)
Is mozilla/widget/src/cocoa not maintained?

Comment 32

15 years ago
it's maintained. camino uses it.
(Assignee)

Updated

15 years ago
Attachment #153652 - Flags: review?(pinkerton)
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 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.
Attachment #153652 - Flags: review?(pinkerton) → review-
Comment on attachment 153653 [details] [diff] [review]
Patch for Mac(not implemented)

r=pink, but who is going to implement these?
Attachment #153653 - Flags: review?(pinkerton) → review+
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.
Adding reviewers to CC.
Created attachment 153684 [details] [diff] [review]
Patch for Cocoa(not implemented) rv. 2
Attachment #153652 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #153684 - Flags: review?(pinkerton)
Comment on attachment 153684 [details] [diff] [review]
Patch for Cocoa(not implemented) rv. 2

r=pink
Attachment #153684 - Flags: review?(pinkerton) → review+

Comment 40

15 years ago
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;|
Attachment #153593 - Flags: review?(ere) → review-
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?
# 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);
Created attachment 153733 [details] [diff] [review]
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.
(Assignee)

Updated

15 years ago
Attachment #153593 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #153733 - Flags: review?(ere)
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".
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.
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
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".
# 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'.
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'.
Created attachment 153904 [details] [diff] [review]
Patch for nsIKBStateControl

Thank you for you wrote the comment.
(Assignee)

Updated

15 years ago
Attachment #153592 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #153904 - Flags: review?(roc)
(Assignee)

Updated

15 years ago
Attachment #153592 - Flags: review?(roc) → review-
Attachment #153904 - Flags: superreview+
Attachment #153904 - Flags: review?(roc)
Attachment #153904 - Flags: review+
Thank you! Robert.

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

Updated

15 years ago
Attachment #153651 - Flags: review?(amardare) → review+
Adrian:
Thank you for the review.

Comment 53

15 years ago
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.
Attachment #153733 - Flags: review?(ere) → review-
Created attachment 153929 [details] [diff] [review]
Patch for Windows rv. 1.2
Attachment #153733 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #153929 - Flags: review?(ere)

Comment 55

15 years ago
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).
Attachment #153929 - Flags: review?(ere) → review-
Created attachment 153962 [details] [diff] [review]
Patch for Windows rv. 1.3
Attachment #153929 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #153962 - Flags: review?(ere)
Simon: 

Do you can review 'Patch for nsEditor'?
I sent mail to Daniel, But no reply...
Comment on attachment 153654 [details] [diff] [review]
Patch for gtk(not implemented)

I change reviewer to Blizzard.

Blizzard:
Please review the patch.
Attachment #153654 - Flags: review?(pavlov) → review?(blizzard)
Attachment #153654 - Flags: review?(blizzard) → review+
Comment on attachment 153591 [details] [diff] [review]
Patch for nsEditor

I change reviwer to Simon.

Blizzard:
Thank you for the review.
Attachment #153591 - Flags: review?(daniel) → review?(sfraser)

Comment 60

15 years ago
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.
Attachment #153591 - Flags: review?(sfraser) → review-
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?
Attachment #153595 - Flags: review?(alecf) → review?

Updated

15 years ago
Attachment #153962 - Flags: review?(ere) → review+
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.
Attachment #153595 - Flags: review? → review?(sfraser)

Updated

15 years ago
Attachment #153595 - Flags: superreview?(neil.parkwaycc.co.uk)

Updated

15 years ago
Attachment #153595 - Flags: review?(sfraser) → review?(dveditz)
Created attachment 154075 [details] [diff] [review]
Patch for nsEditor rv. 2
Attachment #153591 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #154075 - Flags: review?(sfraser)
Who can superreview for other patch? Blizzard? (intl and widget)
(Assignee)

Updated

15 years ago
Attachment #153651 - Flags: superreview?(blizzard)
(Assignee)

Updated

15 years ago
Attachment #153653 - Flags: superreview?(blizzard)
(Assignee)

Updated

15 years ago
Attachment #153654 - Flags: superreview?(blizzard)
(Assignee)

Updated

15 years ago
Attachment #153684 - Flags: superreview?(blizzard)
(Assignee)

Updated

15 years ago
Attachment #153962 - Flags: superreview?(blizzard)
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

15 years ago
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?
(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".

dveditz:

See comment 65 before review.

Comment 69

15 years ago
> 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.
O.K.
Please wait.
(Assignee)

Updated

15 years ago
Attachment #154075 - Flags: review?(sfraser) → review-
Created attachment 154130 [details] [diff] [review]
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.
Attachment #154075 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #154130 - Flags: review?(sfraser)

Comment 72

15 years ago
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

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

sr=me once the pref names are finalized ;-)
Attachment #153595 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #153595 - Flags: superreview+
Created attachment 154167 [details] [diff] [review]
Patch for nsEditor rv. 3.1

Thank you neil.

PRPackedBool -> PRBool in global variables.
Attachment #154130 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #154167 - Flags: review?(sfraser)
(Assignee)

Updated

15 years ago
Attachment #154130 - Flags: review?(sfraser) → review-
(Assignee)

Updated

15 years ago
Flags: blocking-aviary1.0RC1?
Created attachment 154175 [details] [diff] [review]
Patch for nsEditor rv. 3.2

I rewrote |ForceCompositionEnd| with |GetKBStateControl|.
Attachment #154167 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #154175 - Flags: review?(sfraser)
(Assignee)

Updated

15 years ago
Attachment #154167 - Flags: review?(sfraser)

Comment 76

14 years ago
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
Attachment #154175 - Flags: review?(sfraser) → review+
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.
(Assignee)

Updated

14 years ago
Attachment #154175 - Flags: superreview?(kinmoz)
timeless:

I sent the mail to dveditz@cruzio.com.
But no reply.

Can you contact to dveditz@cruzio.com? or Do you know another reviewer?
Comment on attachment 154175 [details] [diff] [review]
Patch for nsEditor rv. 3.2

I change superreviewer to Simon.

Simon: 
Please super-review.
Attachment #154175 - Flags: superreview?(kinmoz) → superreview?(sfraser)
Daniel Veditz, Christopher Blizzard, Simon Fraser:

Please review or superreview.
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
Attachment #153595 - Flags: review?(dveditz) → review+
Attachment #154175 - Flags: superreview?(sfraser) → superreview?(dveditz)

Updated

14 years ago
Flags: blocking-aviary1.0PR? → blocking-aviary1.0PR-

Updated

14 years ago
Assignee: sfraser → masayuki
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

14 years ago
Attachment #153651 - Flags: superreview?(blizzard) → superreview?(bryner)
(Assignee)

Updated

14 years ago
Attachment #153653 - Flags: superreview?(blizzard) → superreview?(bryner)
(Assignee)

Updated

14 years ago
Attachment #153654 - Flags: superreview?(blizzard) → superreview?(bryner)
(Assignee)

Updated

14 years ago
Attachment #153684 - Flags: superreview?(blizzard) → superreview?(bryner)
(Assignee)

Updated

14 years ago
Attachment #153962 - Flags: superreview?(blizzard) → superreview?(bryner)
I changed superreviewer.
-> bryner@brianryner.com
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.
Attachment #153962 - Flags: superreview?(bryner) → superreview+
Attachment #153651 - Flags: superreview?(bryner) → superreview+
Attachment #153653 - Flags: superreview?(bryner) → superreview+
Attachment #153654 - Flags: superreview?(bryner) → superreview+
Attachment #153684 - Flags: superreview?(bryner) → superreview+
Brian:

Thank you for superreview!

> Try to be consistent with indentation.

It is fixed by bug 253035.
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?
Simon and Daniel:

Should I rename the name of new functions on nsEditor too?
e.x.,
NotifyImeOnFocus -> NotifyIMEOnFocus
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
Attachment #154175 - Flags: superreview?(dveditz) → superreview+
Created attachment 158314 [details] [diff] [review]
Patch of widget/ for check in
(Assignee)

Updated

14 years ago
Attachment #158314 - Attachment is obsolete: true
Created attachment 158315 [details] [diff] [review]
Patch of widget/ for check in
Created attachment 158316 [details] [diff] [review]
Patch of editor/ for check in
Created attachment 158317 [details] [diff] [review]
Patch of modules/libpref/src/init/ for check in
(Assignee)

Updated

14 years ago
Attachment #158315 - Flags: superreview+
Attachment #158315 - Flags: review+
(Assignee)

Updated

14 years ago
Attachment #158316 - Flags: superreview+
Attachment #158316 - Flags: review+
(Assignee)

Updated

14 years ago
Attachment #158317 - Flags: superreview+
Attachment #158317 - Flags: review+
Comment on attachment 158317 [details] [diff] [review]
Patch of modules/libpref/src/init/ for check in

Daniel:

Please check in.
Attachment #158317 - Attachment description: Patch of modules/libpref/src/init → Patch of modules/libpref/src/init/ for check in
Created attachment 158338 [details] [diff] [review]
Patch of editor/ for check in
Attachment #158316 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #158338 - Flags: superreview+
Attachment #158338 - Flags: review+
(Assignee)

Updated

14 years ago
Attachment #158316 - Flags: superreview+
Attachment #158316 - Flags: review+
Who can check in?
Patches checked in. Masayuki-san, can you please double check that the checkin
included everything necessary?
> Masayuki-san, can you please double check that the checkin
> included everything necessary?

Sorry. I cannot understand. What's "double check"?
(In reply to comment #96)

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

Can you verify that the checkin is not missing something?
backed out because of build bustage.
Umm..

I don't have Linx, Mac, and others.
That should be corrected, how...
Created attachment 158520 [details] [diff] [review]
Patch of widget/ ( not completed)

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

Updated

14 years ago
Attachment #158315 - Attachment is obsolete: true
Created attachment 158521 [details] [diff] [review]
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.
Attachment #158520 - Attachment is obsolete: true

Comment 102

14 years ago
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)
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.
Oops...

Jungshik Shin:

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

Comment 105

14 years ago
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? 
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.
In bugzilla-jp, Mac build went through fine.
Thank you.

Comment 108

14 years ago
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. 
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 111

14 years ago
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

14 years ago
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?
Flags: blocking-aviary1.0mac?
Flags: blocking-aviary1.0?
This bug was fixed on Windows only.
Nobody made the patch for new IME API for Mac.
Flags: blocking-aviary1.0mac? → blocking-aviary1.0mac-

Comment 114

14 years ago
If the patch author would like to request that this be landed for aviary, he can
request approval in the patch manager. 
Flags: blocking-aviary1.0? → blocking-aviary1.0-
*** Bug 68382 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

12 years ago
Depends on: 358899
You need to log in before you can comment on or make changes to this bug.