Closed Bug 358899 Opened 14 years ago Closed 14 years ago

[Cocoa] Improve nsIKBStateControl implementation

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: jp-critical)

Attachments

(1 file, 14 obsolete files)

45.73 KB, patch
masayuki
: review+
emaijala+moz
: review+
masaki.katakai
: review+
masayuki
: superreview+
Details | Diff | Splinter Review
We should implement:
nsIKBStateControl::SetIMEOpenState
nsIKBStateControl::GetIMEOpenState
nsIKBStateControl::SetIMEEnabled
nsIKBStateControl::GetIMEEnabled

for improve the management of TSM.
The patch will be coming soon.
Attached patch Patch rc1 (obsolete) — Splinter Review
The original author is KATO Kazuyoshi-san(kzys@...).

This patch suppresses to work the IME if it is disabled by gecko.

If we cannot find the regression, I will request the review.
Attached patch Patch rc3 (obsolete) — Splinter Review
This is simpler.

But I found a problem, on Mac, the dead key inputtings are same behavior as IME. So, we land this patch, the dead keys cannot use in password field, is it OK??? (e.g., Spanish.)
Attachment #259011 - Attachment is obsolete: true
Attached patch Patch rc4 (obsolete) — Splinter Review
The dead key inputs are allowed under disabled the IME.

>   // We should send the event to super class. It is normal process.
>   // But if IME was disabled, we should not send it. If we don't send it,
>   // we can suppress the IME composition behavior. But in that time, the
>   // insertText method is never called, so, if we don't use super class,
>   // we need to dispatch text event ourselves. Note that we need to keep the
>   // dead key behavior even if the IME was disabled. Therefore, even if the
>   // IME was disabled, we need to send the event to super class:
>   //   1. that is a input of a dead key
>   //   2. under composing process, it means the previous is dead key
>   //   3. non-charactered input (e.g., arrow keys, delete)
>   if (nsTSMManager::IsIMEEnabled() || nsTSMManager::IsComposing() ||
>       !nonDeadKeyPress || !geckoEvent.isChar)
>     [super interpretKeyEvents:[NSArray arrayWithObject:theEvent]];
>   else
>     [self insertText:[theEvent characters]];
Attachment #259066 - Attachment is obsolete: true
Attached patch Patch rc4.1 (obsolete) — Splinter Review
fix some nits in my comment.
Attachment #259071 - Attachment is obsolete: true
Sigh, it doesn't work fine with Chinese or Korean or Tamil IME...
Attached patch Patch rc5 (obsolete) — Splinter Review
This patch's behavior is similar to safari.

If the focus is on the content but it is not on editor, the IME is 'disabled'. But users can select any keyboard layouts. Therefore, the keypress events may have non-ASCII character, e.g., if the user select the Korean keyboard layout, the events have Hangul characters. But I don't generate the keypress event for dead keys. Because on safari, it is generated as charCode == 0 and keyCode == 0. I think that this events are not good events...(and if we should generate the events, we need more code. But I don't think that it is a valuable code.)

If the focus is on password field, the keyboard layouts are limited by KeyScript(-23). That means non-Roman keyboard layouts are disabled. So, if non-Roman keyboard layout is selected, the password field changes the 'previous' Roman keyboard layout. The spec depends on the OS X.

By this change, we can fix some bugs ans compatibility issue for safari. But there is an important issue. The non-Roman characters cannot use in password field. I.e., Greek and Russian users may have trouble by this change. But safari cannot input these characters too. (We can get same behavior by this patch!) And the guide line of Apple preferred this behavior. (Now the document is not found...)

> "Basics of Localizability" 4.3 Password
> http://developer.apple.com/intl/resources/LocalizabilityTNv3.0.pdf
> 
> The recommended solution is to switch the keyboard to Roman in this field and
> lock it, so users cannot switch back to the native script, thus forcing them to
> use an ASCII password. This is the most secure solution (although some users
> don't like it). 
> In Cocoa, this is the default behavior of the NSSecureTextField. 
> In Carbon, to switch and lock the keyboard, you use the KeyScript routine, first
> with the script code smKeyRoman and then again with the smKeyDisableKeybdSwitch
> verb.
Attachment #259073 - Attachment is obsolete: true
Attached patch Patch rv1.0 (obsolete) — Splinter Review
fix some nits.
Attachment #259111 - Attachment is obsolete: true
Comment on attachment 259119 [details] [diff] [review]
Patch rv1.0

First, I need the review by Josh.

Josh, see comment 6 for the detail of this patch.

This patch has a serious issue for Greek and Russian users. But it is a Cocoa standard behavior, I believe that this behavior is better for compatibility for safari and other cocoa applications. (If the users use Greek or Cyrillic characters, they cannot use safari for the page.) There is a workaround for them, safari cannot paste the non-ASCII characters in password field, but Fx can do it. So, the users cannot the input these characters directly but they can *use* the characters.
Attachment #259119 - Attachment description: Patch rv5.1 → Patch rv1.0
Attachment #259119 - Flags: review?(joshmoz)
Comment on attachment 259119 [details] [diff] [review]
Patch rv1.0

This patch has a bug...
Attachment #259119 - Flags: review?(joshmoz) → review-
Flags: blocking1.9?
Attached patch Patch rv2.0 (obsolete) — Splinter Review
Sorry for the delay.

This fixes the bug that is the non-character input events call the insertText.

And this minimizes the changing in nsIKBStateControl.
Attachment #259119 - Attachment is obsolete: true
Attachment #259708 - Flags: review?(joshmoz)
Attached patch Patch rv2.0.1 (obsolete) — Splinter Review
fix a nit, sorry for the spam.
Attachment #259708 - Attachment is obsolete: true
Attachment #259711 - Flags: review?(joshmoz)
Attachment #259708 - Flags: review?(joshmoz)
Comment #8 concerns me - can you explain that in more detail? I don't think I understand.
My patch emulate the behavior of NSSecureTextField of cocoa if the focused widget is password editor.

We can separate the all keyboard layouts into two groups:
1. it can input ASCII characters directly.
  this is called as roman keyboard layouts in apple documents. E.g., they are U.S., German and Spanish. Please note that they can input non-ASCII characters too. E.g.,  ä, ü and ö in German keyboard layout.

2. it cannot input non-ASCII characters.
  this group has other keyboard layouts from 1. E.g., they are Greek, Russian and CJK keyboard layouts. If we type the keys with these keyboard layouts, we can type non-ASCII characters. I.e., Greek, Cyrillic and CJKT characters.

Apple document said the password editors should disable the group 2 at getting the focus. And the password editors of WebKit do same behavior (See comment 6).

So, we can type the Greek, Cyrillic and CJKT characters in current password editors of Gecko. I.e., our users can use non-accessible password from safari. However, I don't know whether the actual users use these characters for password. In Japan, most users may use ASCII characters only.

I think that we have these issues in this patch:
1. If Greek, Cyrillic and CKT users use these keyboard layouts for password editors, they cannot type the password directly. (But they can use these characters by paste.)

2. On Linux and Windows, the users can use Greek and Cyrillic keyboard layouts in password editors. This behavior is same as them native password editors. (But they cannot use IME, so CJKT characters cannot use directry.) So, if they use these characters, they cannot type the password from Gecko for Mac.

So, we have an accessibility issue. The users may not be able to type the same passwords in *all* platforms/browsers. But I think that we should choice same behavior in each platforms. Because there is the problem since Netscape Navigator...

Josh: can you help this comment?
Comment on attachment 259711 [details] [diff] [review]
Patch rv2.0.1

It would be great if smfr could take a look at this at the same time I do. The IME changes in cocoa widgets are pretty extensive it seems.
Attachment #259711 - Flags: review?(sfraser_bugs)
Yeah, the patch has high risk. I'm working for improve the IME behavior on all platforms in 1.9 cycle. I have finished the work for Win and Linux. We should fix this bug in the alpha stage.
Masayuki: how critical is this for Japanese users?
(In reply to comment #16)
> Masayuki: how critical is this for Japanese users?

this is critical. this also fixes bug 113187, bug 279246 and bug 16940 on Mac.
Blocks: 16940, 113187, 279246
(In reply to comment #17)
> (In reply to comment #16)
> > Masayuki: how critical is this for Japanese users?
> 
> this is critical. this also fixes bug 113187, bug 279246 and bug 16940 on Mac.
> 

bug 279264 is not fixed by this, sorry. (but the bug needs this change.)
Flags: blocking1.9? → blocking1.9+
Comment on attachment 259711 [details] [diff] [review]
Patch rv2.0.1

So long as you have the nsTSMManager class in the nsChildView header file can you put the implementation near the bottom of the nsChildView.mm file? Right above the accessibility section please.

This looks good to me otherwise, but I am not a great reviewer for this code.
Attachment #259711 - Flags: review?(joshmoz) → review+
Thank you, Josh.

The patch still needs the review of sfraser? I sent the mail to him, but I don't get the reply yet...
Status: NEW → ASSIGNED
Attachment #259711 - Flags: review?(sfraser_bugs) → superreview?(pavlov)
Comment on attachment 259711 [details] [diff] [review]
Patch rv2.0.1

I accidentally requested sr as ben... sorry for the bugspam
Attachment #259711 - Flags: superreview?(pavlov)
Attachment #259711 - Flags: superreview?(pavlov)
Comment on attachment 259711 [details] [diff] [review]
Patch rv2.0.1

And I need the review of roc.
Attachment #259711 - Flags: review?(roc)
Attached patch Patch rv2.1 (obsolete) — Splinter Review
updating for Josh's review.
Attachment #259711 - Attachment is obsolete: true
Attachment #260995 - Flags: superreview?(pavlov)
Attachment #260995 - Flags: review+
Attachment #259711 - Flags: superreview?(pavlov)
Attachment #259711 - Flags: review?(roc)
IME_STAUTS_DISABLED_FOR_PASSWORD is misspelled everywhere.

+#if defined(XP_MACOSX)
+      IME_STAUTS_DISABLED_FOR_PASSWORD = 2
+#else
+      IME_STAUTS_DISABLED_FOR_PASSWORD = IME_STATUS_DISABLED
+#endif

Can we call this just IME_STATUS_PASSWORD and make it its own value on all platforms? Windows and Linux can just treat it the same as DISABLED (say by converting it in SetIMEEnabled).

We could have a helper function (in nsContentUtils?) which converts from an nsIKBStateControl IME_STATUS to nsIContent IME_STATUS flags.

-    NS_IMETHOD SetIMEEnabled(PRBool aState) = 0;
+    NS_IMETHOD SetIMEEnabled(PRUint32 aState) = 0;

While we're doing this, how about deCOMtaminating Set/GetIMEEnabled?
(In reply to comment #24)
> While we're doing this, how about deCOMtaminating Set/GetIMEEnabled?

Do create the new class for deCOMtaminating? On Win and Linux, the methods depend on nsWindow...
Attached patch Patch rv2.2 (obsolete) — Splinter Review
Attachment #260995 - Attachment is obsolete: true
Attachment #261133 - Flags: review+
Attachment #260995 - Flags: superreview?(pavlov)
Attachment #260995 - Flags: review?(roc)
Comment on attachment 261133 [details] [diff] [review]
Patch rv2.2

I think that we don't need the review by Stuart. But this changes Win/Linux code, so, we need the additional reviews by theirs reviewers.
Attachment #261133 - Flags: superreview?(roc)
Attachment #261133 - Flags: review?(roc)
Attached patch Patch rv2.2.1 (obsolete) — Splinter Review
Sorry. The previous patch changes the logic of GTK2 code. We should not change it.
Attachment #261133 - Attachment is obsolete: true
Attachment #261135 - Flags: review+
Attachment #261133 - Flags: superreview?(roc)
Attachment #261133 - Flags: review?(roc)
Attached patch Patch rv2.2.1 (obsolete) — Splinter Review
Oops, sorry. I attached the wrong patch. Sorry for the spam.
Attachment #261135 - Attachment is obsolete: true
Attachment #261136 - Flags: review+
Attachment #261135 - Flags: superreview?(roc)
Attachment #261135 - Flags: review?(roc)
Comment on attachment 261136 [details] [diff] [review]
Patch rv2.2.1

Ah, it doesn't have the diff from 2.2. Looks like that it is a bug of the diff tool of bugzilla... :(
Attachment #261136 - Flags: superreview?(roc)
Attachment #261136 - Flags: review?(roc)
+  static PRUint32 GetIMEStatusFromKBStateControlStatus(PRUint32 aState);

Sorry I think I made a mistake. If you reverse this, you can use it in nsIMEStateManager::SetIMEState and nsIMEStateManager::OnChangeFocus.
If I reverse it, there are no valid value for the result in some cases (e.g., nsIContent::IME_STATUS_NONE). How about merge the Set(Get)IMEEnabled and Set(Get)IMEOpenState to Set(Get)IMEState?
Don't worry about merging them, then. Just undo the merge I asked for. Sorry.

+  // We should send the event to the super class. It is a normal process.
+  // But if IME was disabled, we want to suppress the IME composing behavior,
+  // By we don't send it, we can do it. In this time, the insertText method
+  // is never called, so, if we don't use the super class, we need to dispatch
+  // the text event ourselves.

I don't understand this comment. Josh, can you suggest better text for the comment?

Otherwise, it's fine.
Attached patch Patch rv2.2.2 (obsolete) — Splinter Review
Attachment #261136 - Attachment is obsolete: true
Attachment #261261 - Flags: review+
Attachment #261136 - Flags: superreview?(roc)
Attachment #261136 - Flags: review?(roc)
(In reply to comment #33)
> +  // We should send the event to the super class. It is a normal process.
> +  // But if IME was disabled, we want to suppress the IME composing behavior,
> +  // By we don't send it, we can do it. In this time, the insertText method
> +  // is never called, so, if we don't use the super class, we need to dispatch
> +  // the text event ourselves.
> 
> I don't understand this comment. Josh, can you suggest better text for the
> comment?

How about this?

// We should send this event to the super class if IME is enabled.
// Otherwise, we need to suppress the IME composing. We can do it by the way
// which is we don't send this event to the super class. But at the time,
// we need to call the insertText ourselves for dispatching the text event.
Comment on attachment 261261 [details] [diff] [review]
Patch rv2.2.2

OK, thanks, now I understand. Then I think we can go with this comment:

// We should send this event to the superclass if IME is enabled.
// Otherwise, we need to suppress IME composition. We can do it by
// not sending this event to the superclass. But in that case,
// we need to call insertText ourselves.

Also, "dispached" should be "dispatched".
Attachment #261261 - Flags: superreview?(roc)
Attachment #261261 - Flags: superreview+
Attachment #261261 - Flags: review?(roc)
Attachment #261261 - Flags: review+
Attached patch Patch rv2.2.3 (obsolete) — Splinter Review
Thank you, roc.
Attachment #261261 - Attachment is obsolete: true
Attachment #261352 - Flags: superreview+
Attachment #261352 - Flags: review+
Comment on attachment 261352 [details] [diff] [review]
Patch rv2.2.3

Ere:

Would you check the win32 part?
This patch adds an IME enabled state. Which is 'password' special state. But on Win32, it is same as 'disabled'. So, the actual behavior should not be changed on Win32.
Attachment #261352 - Flags: review?(emaijala)
Comment on attachment 261352 [details] [diff] [review]
Patch rv2.2.3

Katakai-san:

Would you check the gtk2 part? See previous my comment for the details for this patch. The actual behavior should not be changed on gtk2 too.
Attachment #261352 - Flags: review?(masaki.katakai)
Attached patch Patch rv2.2.4Splinter Review
Oops, I forgot to rename the misspelled variable. Sorry.
Attachment #261352 - Attachment is obsolete: true
Attachment #261356 - Flags: superreview+
Attachment #261356 - Flags: review+
Attachment #261352 - Flags: review?(masaki.katakai)
Attachment #261352 - Flags: review?(emaijala)
Comment on attachment 261356 [details] [diff] [review]
Patch rv2.2.4

Looks ok to me.
Attachment #261356 - Flags: review?(emaijala) → review+
Comment on attachment 261356 [details] [diff] [review]
Patch rv2.2.4

looks OK for me.
Attachment #261356 - Flags: review?(masaki.katakai) → review+
checked-in. Thank you for the reviewers!
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.