Closed Bug 358899 Opened 13 years ago Closed 13 years ago
[Cocoa] Improve ns
IKBState Control implementation
We should implement: nsIKBStateControl::SetIMEOpenState nsIKBStateControl::GetIMEOpenState nsIKBStateControl::SetIMEEnabled nsIKBStateControl::GetIMEEnabled for improve the management of TSM. The patch will be coming soon.
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.
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
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
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...
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
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.
Comment on attachment 259119 [details] [diff] [review] Patch rv1.0 This patch has a bug...
Attachment #259119 - Flags: review?(joshmoz) → 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.
fix a nit, sorry for the spam.
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.
(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 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
13 years ago
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
Comment on attachment 259711 [details] [diff] [review] Patch rv2.0.1 And I need the review of roc.
updating for Josh's review.
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...
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.
Sorry. The previous patch changes the logic of GTK2 code. We should not change it.
Oops, sorry. I attached the wrong patch. Sorry for the spam.
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... :(
+ 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.
(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".
Thank you, roc.
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 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.
Oops, I forgot to rename the misspelled variable. Sorry.
Attachment #261356 - Flags: review?(emaijala)
Attachment #261356 - Flags: review?(masaki.katakai)
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: 13 years ago
Resolution: --- → FIXED
Depends on: 480277
You need to log in before you can comment on or make changes to this bug.