358899 - [Cocoa] Improve nsIKBStateControl implementation

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

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

We should implement:
nsIKBStateControl::SetIMEOpenState
nsIKBStateControl::GetIMEOpenState
nsIKBStateControl::SetIMEEnabled
nsIKBStateControl::GetIMEEnabled

for improve the management of TSM.
The patch will be coming soon.

Comment 1

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

Attachment: Patch rc1

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.

Comment 2

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

Attachment: Patch rc3

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.)

Comment 3

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

Attachment: Patch rc4

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]];

Comment 4

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

Attachment: Patch rc4.1

fix some nits in my comment.

Comment 5

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

Sigh, it doesn't work fine with Chinese or Korean or Tamil IME...

Comment 6

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

Attachment: Patch rc5

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.

Comment 7

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

Attachment: Patch rv1.0

fix some nits.

Comment 8

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

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.

Comment 9

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

Comment on attachment 259119 [details] [diff] [review]
Patch rv1.0

This patch has a bug...

Comment 10

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

Attachment: Patch rv2.0

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.

Comment 11

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

Attachment: Patch rv2.0.1

fix a nit, sorry for the spam.

Comment 12

[Josh Aas]

Comment #8 concerns me - can you explain that in more detail? I don't think I understand.

Comment 13

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

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 14

[Josh Aas]

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.

Comment 15

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

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.

Comment 16

[Stuart Parmenter]

Masayuki: how critical is this for Japanese users?

Comment 17

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

(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.

Comment 18

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

(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.)

Comment 19

[Josh Aas]

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.

Comment 20

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

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...

Comment 21

[Josh Aas]

Comment on attachment 259711 [details] [diff] [review]
Patch rv2.0.1

I accidentally requested sr as ben... sorry for the bugspam

Comment 22

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

Comment on attachment 259711 [details] [diff] [review]
Patch rv2.0.1

And I need the review of roc.

Comment 23

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

Attachment: Patch rv2.1

updating for Josh's review.

Comment 24

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

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?

Comment 25

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

(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...

Comment 26

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

Attachment: Patch rv2.2

Comment 27

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

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.

Comment 28

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

Attachment: Patch rv2.2.1

Sorry. The previous patch changes the logic of GTK2 code. We should not change it.

Comment 29

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

Attachment: Patch rv2.2.1

Oops, sorry. I attached the wrong patch. Sorry for the spam.

Comment 30

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

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... :(

Comment 31

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

+  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.

Comment 32

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

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?

Comment 33

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

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.

Comment 34

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

Attachment: Patch rv2.2.2

Comment 35

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

(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 36

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

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".

Comment 37

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

Attachment: Patch rv2.2.3

Thank you, roc.

Comment 38

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

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.

Comment 39

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

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.

Comment 40

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

Attachment: Patch rv2.2.4

Oops, I forgot to rename the misspelled variable. Sorry.

Comment 41

[Ere Maijala (slow)]

Comment on attachment 261356 [details] [diff] [review]
Patch rv2.2.4

Looks ok to me.

Comment 42

[Masaki Katakai]

Comment on attachment 261356 [details] [diff] [review]
Patch rv2.2.4

looks OK for me.

Comment 43

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

checked-in. Thank you for the reviewers!