513952 - Refactor nsTSMManager

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

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

nsTSMManager uses some legacy APIs which cannot be used for 64bit build. They can be replaced by TIS or IMK.

Comment 1

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

Ugh... Maybe, we cannot access Input Method Kit APIs... We should use TIS APIs, NSInputManager and NSTextInputContext.

Perhaps, we should remove NSTextInput protocol. Instead of it, we should implement NSTextInputClient.
http://developer.apple.com/mac/library/documentation/Cocoa/Reference/NSTextInputClient_Protocol/Reference/Reference.html

And also we need to keep to use NSInputManager on 10.5. However, on 10.6, we should use NSTextInputContext because NSInputManager was deprecated...

Comment 2

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

I think nsTSMManager isn't useful in the new architecture. Now, we need the new managers on each nsChildView.

And I've got a plan for IME code separation from other code because they are sometimes broken by some developers who don't know IME. If we separate it to another file, the developers may request to review or cc me or smichaud.

And the current nsChildView.mm has over 6k lines, we can reduce it.

Comment 3

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

Attachment: Patch v1.0

Comment 4

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

Attachment: Patch v1.1

Comment 5

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

Attachment: Patch v1.2

Comment 6

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

Attachment: Patch v1.3

Comment 7

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

Attachment: Patch v2.0

Comment 8

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

Attachment: Patch v3.0

Comment 9

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

Attachment: Patch v3.1

Comment 10

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

Attachment: Patch v4.0

Comment 11

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

Attachment: Patch v4.1

Comment 12

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

Comment on attachment 403718 [details] [diff] [review]
Patch v4.1

This patch works for me, and passed to the tests on tryserver.

I'll post the detail of this patch in next comment.

Comment 13

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

This patch add two classes:

1. nsCocoaIMEHandler
2. nsCocoaTextInputHandler

The nsCocoaTextInputHandler inherits the nsCocoaIMEHandler. The reason of the separation is to protect the IME related code from some developers who are not non-IME users.

nsCocoaTextInputHandler is empty class currently. I'm planing that I'll move the NSTextInput protocol implementation to this class. It makes the nsChildView simply. And it will help the next API changes on 10.6 (NSInputManager is obsolete, we'll use NSInputContext after we drop 10.5 support).

nsCocoaTextInputHandler is created in nsChildView instance. The old nsTSMManager was static class, so, I change the architecture of the IME management. Because nsTSMManager approach is wrong. I misunderstand the Mac's IME management design by the legacy KeyScript API and GetScriptManagerVariable. On current Mac OS X, the IME context is created by every NSView, probably. When an NSView has focus, the results of [NSInputManager currentInputManager] and ::TSMGetActiveDocument() are always same. However, when another NSView has focus, the results are different.

I tried to cache the NSView's NSInputManager and TSMDocumentID, however, it failed. E.g., NSInputManager doesn't work fine when the related NSView is deactive. So, we can access to IMEs only when the NSView is active.

When an NSView is deactive but there is a request to commit the composition, nsCocoaIMEHandler commits the composition string internally. I.e., it sends the NS_TEXT_TEXT event with the latest composition string. And when the NSView is focused, nsCocoaIMEHandler requests IME to discard the composition string. When the NSView is deactive, nsCocoaIMEHandler ignores the composition events. By these design, I fix a bug: When I click another editor which is on another nsChildView, the composition string are doubled when the focus comes back.

Additionally, the IME open/close implementation is changed by this patch. nsTSMManager emulated the Cmd+Space key combination behavior by using KeyScript API. However, such function isn't there in new APIs. And also the implementation switches between ASCII capable keyboard layout and non-ASCII capable keyboard layout. This isn't preferred behavior.

The new APIs can check whether a Text Input Source is an IME mode or not. Therefore, we can only open/close the IME. When I'll close IME, we can just use TISCopyCurrentASCIICapableKeyboardInputSource API which returns the last selected ASCII capable keyboard layout. Otherwise, we change to the last used IME mode if we know. We are now tracking the keyboard layout changes. So, if the user use an IME mode after we are launched, we can switch back the keyboard layout to it. However, if the user hasn't used any IME modes at that time, we will open an IME mode forcedly from the system languages which is prioritized by the system settings. If there are no available IME modes in the language lists, we don't do anything. Note that the IME open/close request is come only from IE's CSS "ime-mode" property. The request is rare, therefore, in most cases, the user activate an IME mode before the request.

Even if we land this patch, there are two bugs I found:
1. When switching the active window from our window to another our window, the composition string isn't committed correctly. This is a bug of nsPresShell. I'll try to fix it after this bug.
2. We need to use the nsITimer hack at focus event because when OnFocusChangeInGecko is called, the focus isn't moved to new content completely. The reason is simple, nsFocusManager notifies the focus changing before it finishes the all processing. I think it can be fixed, but I'm not sure how to do it. I'll try to fix it, but it's low priority.

Comment 14

[Josh Aas]

Comment on attachment 403718 [details] [diff] [review]
Patch v4.1

+#if defined(MAC_OS_X_VERSION_10_5) && (MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_5)
+#define NS_LEOPARD_AND_LATER 1
+#endif

Please define this in "nsCocoaUtils.h".

+  // XXX If the current input source is a mode of IME, we should trun on it,

"trun" -> "turn"

Thanks a lot Masayuki. Please be sure to file bugs on the 2 problems you mentioned in comment 13 and cc me on them.

Comment 15

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

Attachment: Patch v4.2

Comment 16

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

http://hg.mozilla.org/mozilla-central/rev/7d4dcb4ed7ee