875674 - Implement NSTextInputClient protocol on Mac

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

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

Starting Mac OS X 10.5, there is a new protocol NSTextInputClient which replaces NSTextInput.

Since we don't support new methods of NSTextInputClient, some functions of IME don't work. So, we should implement this ASAP.

Comment 1

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

NSTextInputClient:
https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/NSTextInputClient_Protocol/Reference/Reference.html

NSTextInput:
http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/ApplicationKit/Protocols/NSTextInput_Protocol/Reference/Reference.html

changed methods:

– setMarkedText:selectedRange:  required method
 → – setMarkedText:selectedRange:replacementRange:  required method

– attributedSubstringFromRange:  required method
 → – attributedSubstringForProposedRange:actualRange:  required method

– insertText:  required method
 → – insertText:replacementRange:  required method

– firstRectForCharacterRange:  required method
 → – firstRectForCharacterRange:actualRange:  required method

new methods:

– attributedString                              * won't support in this bug
– fractionOfDistanceThroughGlyphForPoint:       * won't support in this bug
– baselineDeltaForCharacterAtIndex:             * won't support in this bug
– windowLevel
– drawsVerticallyForCharacterAtIndex:           * won't support in this bug

Comment 2

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

There is a problem. The document of setMarkedText:selectedRange:replacementRange: is completely broken.

The summary is, "Replaces a specified range in the receiver’s text storage with the given string and sets the selection. (required)".

And the definition of replacementRange is, "The range to replace, computed from the beginning of the *marked* text."

Additionally, the discussion says that "If there is no marked text, the current selection is replaced. If there is no selection, the string is inserted at the insertion point."

So, the summary and discussion say really different things. Additionally, replacementRange's purpose isn't clear with this explanation.

Unfortunately, Chromium always ignores the replacementRange :(

Comment 3

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

Looks like this implementation is same as the reference:
http://code.ohloh.net/file?fid=5NFrxQemcxRJptFbYnVVagHlY5c&cid=ntsbFCp8FJY&s=setMarkedText&pp=0&fl=ObjectiveC&ff=1&filterChecked=true&mp=1&ml=0&me=1&md=1&browser=Default#L338

However, some other OSS product treat replacement range is a range in text storage. Additionally, a lot of OSS products ignore the replacementRange...

Comment 4

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

Oh, I found an official sample code of NSTextInputClient implementation. In this, the replacementRange is a range in the text storage. I believe that the explanation in the document is wrong.
https://developer.apple.com/library/mac/#samplecode/TextInputView/Listings/FadingTextView_m.html

Comment 5

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

Attachment: part.1 nsIMEUpdatePreference should store wanted updates per notification

For performance reason, Cocoa's widget::TextInputHandler wants to store current selection while an editor has focus because some new methods of NSTextInputClinet needs to compare specified |replacementRange| with current selection (if they are different and there is composition, they need to commit composition temporarily).

If it stores current selection, it needs to be notified the selection change. However, currently, text change is also notified with very expensive performance cost. It's bad for Mac users since the expensive text change notification isn't necessary for Mac users.

For solving this issue, this patch makes widget be able to enable notifications particularly.

Comment 6

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

Attachment: part.2 Cache selection for improving the performance

This patch makes TextInputHandler stores current selection if an editor has focus. This improves later patches' performance.

Comment 7

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

Attachment: part.3 Implement windowLevel of NSTextInputClient

Implements windowLevel of NSTextInputClient protocol. This isn't required for minimum implementation. However, this helps a11y of IME's windows on top-most XUL <panel>. See also part.8 patch.

Comment 8

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

Attachment: part.4 Implement firstRectForCharacterRange:actualRange: of NSTextInputClient

Implements firstRectForCharacterRange:actualRange: of NSTextInputClient.

For returning actualRange which is used for computing the rect, NS_QUERY_TEXT_RECT and NS_QUERY_CARET_RECT need to return the offset after checking the cluster boundary. So, nsQueryContentEvent::mReply::mOffset should store it.

Comment 9

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

Attachment: part.5 Implement attributedSubstringForProposedRange:actualRange: of NSTextInputClient

Implements attributedSubstringForProposedRange:actualRange: of NSTextInputClient.

Similar to part.4, NS_QUERY_TEXT_CONTENT needs to return the actual offset of the return text.

Comment 10

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

Attachment: part.6 Implement insertText:replacementRange: of NSTextInputClient

Implements insertText:replacementRange: of NSTextInputClient.

By the replacementRange, caller can specify "current selection". I don't find actual cases that the range's offset isn't NSNotFound. So, actually, I don't test the new feature.

Even if the string is empty, the selected string should be deleted. Looks like the sample code of Apple does so.
https://developer.apple.com/library/mac/#samplecode/TextInputView/Listings/FadingTextView_m.html

This patch uses NS_CONTENT_COMMAND_DELETE for this.

The other important point of this patch is, if replacementRange indicates different range from current marked range (i.e., the range of composition string), InsertTextAsCommittingComposition() commits current composition with the latest composition string. Then, setting selection for replacementRange and handle normally.

Comment 11

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

Attachment: part.7 Implement setMarkedText:selectedRange:replacementRange: of NSTextInputClient

Implements setMarkedText:selectedRange:replacementRange: of NSTextInputClient.

Similar to part.6 what this patch does.

See comment 2 and comment 4. I ignore the document of NSTextInputClient. I believe that the document is wrong.

Actually, some Japanese IME uses replacementRange of setMarkedText for setting current selection at reconverting committed text and this patch works fine with such IME functions.

Comment 12

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

Attachment: part.8 Notify IME of focus change in Gecko for resetting IME stored window level

According to the actual behavior on 10.6 and 10.7, when native focus is changed, windowLevel is called and the value seems to be stored (not sure if by system or by IME).

Looks like that the focus change can be emulated by calling deactivate of NSTextInputContext and activate of NSTextInputContext. According to the document of NSTextInputContext, these methods shouldn't call directly. But I think that it's safe for this purpose.
http://developer.apple.com/library/mac/#documentation/cocoa/reference/NSTextInputContext_Class/Reference/Reference.html

This patch gets rid of the code using legacy API!

Comment 13

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

Note that only part.6 isn't tested. I confirmed others work fine actually.

Comment 14

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

The patched build will be here:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=280cb89f753b

Note that the tryserver build's bookmark panel (opened by clicking star button) is top-most XUL <panel>. You can test such case with it.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 764645 [details] [diff] [review]
part.1 nsIMEUpdatePreference should store wanted updates per notification

Drive by:

+    // add selection change listener
+    nsCOMPtr<nsISelectionPrivate> selPrivate(do_QueryInterface(mSel));
+    NS_ENSURE_TRUE_VOID(selPrivate);
+    nsresult rv = selPrivate->AddSelectionListener(this);
+    NS_ENSURE_SUCCESS_VOID(rv);
+    rv = selPrivate->AddSelectionListener(this);
+    NS_ENSURE_SUCCESS_VOID(rv);

Here you call selPrivate->AddSelectionListener(this) twice in a row.  Did you intend this?  Is it really necessary?

The duplicate call was actually introduced by Part 3 of your patch for bug 806996.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Masayuki, have you tried to reverse engineer how Safari uses the NSTextInputClient protocol?  Particularly setMarkedText:selectedRange:replacementRange: and insertText:replacementRange: (where you've observed behavior that doesn't match Apple's docs)?

If not, it might be worth trying -- at least for the two methods I mentioned.

I have a lot of experience with this (I use interpose libraries and the Hopper disassembler).  But I need testcases to play with.  If you think this might be useful, please give me a few.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 764650 [details] [diff] [review]
part.3 Implement windowLevel of NSTextInputClient

Looks fine to me, but I have a couple of nits:

+  NSInteger windowLevel = [[editorView window] level];
...
+  return [[editorView window] level];

Why not make the second line "return windowLevel;"?

+  // When an <input> element on a XUL <panel>, the actual focused view is the
+  // panel's parent view (mView). But the editor is displayed on the popuped
+  // widget's view (editorView).  So, their window level may be different.

This comment is a bit garbled.  How about changing it to something like this?

"When an <input> element on a XUL <panel> is focused, the actual focused view is the panel's parent view (mView).  But the editor is displayed on the popped-up widget's view (editorView).  We want the latter's window level."

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 764669 [details] [diff] [review]
part.8 Notify IME of focus change in Gecko for resetting IME stored window level

Looks fine to me, though I didn't test it (or any of the other patches here).  

But the following puzzles me:

+  [inputContext deactivate];
+  [inputContext activate];

...

+  // ... Most IMEs stores windowLevel value of
+  // NSTextInputClient at the server activated.  Therefore, we do it.

The comment appears to explain why the first block is necessary (the one that deactivates and activates mView's input context).  But I don't understand the explanation.

In any case you *do* need to explain why you deactivate and activate mView's input context, and the comment needs to be clearer.

Comment 19

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

(In reply to Steven Michaud from comment #15)
> Comment on attachment 764645 [details] [diff] [review]
> part.1 nsIMEUpdatePreference should store wanted updates per notification
> 
> Drive by:
> 
> +    // add selection change listener
> +    nsCOMPtr<nsISelectionPrivate> selPrivate(do_QueryInterface(mSel));
> +    NS_ENSURE_TRUE_VOID(selPrivate);
> +    nsresult rv = selPrivate->AddSelectionListener(this);
> +    NS_ENSURE_SUCCESS_VOID(rv);
> +    rv = selPrivate->AddSelectionListener(this);
> +    NS_ENSURE_SUCCESS_VOID(rv);
> 
> Here you call selPrivate->AddSelectionListener(this) twice in a row.  Did
> you intend this?  Is it really necessary?
> 
> The duplicate call was actually introduced by Part 3 of your patch for bug
> 806996.

Oh... I'll fix it in the patch, thanks!!

Comment 20

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

(In reply to Steven Michaud from comment #16)
> Masayuki, have you tried to reverse engineer how Safari uses the
> NSTextInputClient protocol?  Particularly
> setMarkedText:selectedRange:replacementRange: and
> insertText:replacementRange: (where you've observed behavior that doesn't
> match Apple's docs)?
> 
> If not, it might be worth trying -- at least for the two methods I mentioned.
> 
> I have a lot of experience with this (I use interpose libraries and the
> Hopper disassembler).  But I need testcases to play with.  If you think this
> might be useful, please give me a few.

I never do it since I don't know how to do it. With Kotoeri, reconvert with selected text uses setMarkedText:selectedRange:replacementRange: but I don't know other cases of they used.

Comment 21

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

(In reply to Steven Michaud from comment #18)
> Comment on attachment 764669 [details] [diff] [review]
> part.8 Notify IME of focus change in Gecko for resetting IME stored window
> level
> 
> Looks fine to me, though I didn't test it (or any of the other patches
> here).  
> 
> But the following puzzles me:
> 
> +  [inputContext deactivate];
> +  [inputContext activate];
> 
> ...
> 
> +  // ... Most IMEs stores windowLevel value of
> +  // NSTextInputClient at the server activated.  Therefore, we do it.
> 
> The comment appears to explain why the first block is necessary (the one
> that deactivates and activates mView's input context).  But I don't
> understand the explanation.
> 
> In any case you *do* need to explain why you deactivate and activate mView's
> input context, and the comment needs to be clearer.

Okay, I'll land the patch with this comment:

+  // When an <input> element on a XUL <panel> element gets focus from an <input>
+  // element on the opener window of the <panel> element, the owner window
+  // still has native focus.  Therefore, IMEs may store the opener window's
+  // level at this time because they don't know the actual focus is moved to
+  // different window.  If IMEs try to get the newest window level after the
+  // focus change, we return the window level of the XUL <panel>'s widget.
+  // Therefore, let's emulate the native focus change.  Then, IMEs can refresh
+  // the stored window level.
+  [inputContext deactivate];
+  [inputContext activate];

Comment 22

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba887369ea3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/680e0bd7f71f
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b4ecd224366
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fd6977113ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/61959adde761
https://hg.mozilla.org/integration/mozilla-inbound/rev/71c683c5f7d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/2323d0f079f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/86a041d63d65

If you want to modify the new comment, I'll do it with additional patch.

Comment 23

[Steven Michaud [:smichaud] (Retired)]

> If you want to modify the new comment, I'll do it with additional patch.

Your comment looks fine to me.  Thanks!

Comment 24

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #20)

I'll try to spend a few hours on this ... though it may be a while before I can get to it.

> With Kotoeri, reconvert with selected text uses
> setMarkedText:selectedRange:replacementRange:

Is switching proposed input between kanji and kana one example of "reconvert with selected text"?

Comment 25

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

(In reply to Steven Michaud from comment #24)
> (In reply to comment #20)
> 
> I'll try to spend a few hours on this ... though it may be a while before I
> can get to it.
> 
> > With Kotoeri, reconvert with selected text uses
> > setMarkedText:selectedRange:replacementRange:
> 
> Is switching proposed input between kanji and kana one example of "reconvert
> with selected text"?

Er, sorry, I wrote wrong behavior since I forgot the detail :-(

The right behavior is "reinput alphabet instead of previous committed text".

For example:

1. change to Japanese input mode.
2. type 'kana', then, you see 'かな'.
3. press "Eisu" key of Japanese Mac keyboard (or Control + Shift + ;) twice, then 'かな' is replaced with 'kana'.

So, insertText:replacementRange: is used, probably.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ba887369ea3e
https://hg.mozilla.org/mozilla-central/rev/680e0bd7f71f
https://hg.mozilla.org/mozilla-central/rev/4b4ecd224366
https://hg.mozilla.org/mozilla-central/rev/9fd6977113ae
https://hg.mozilla.org/mozilla-central/rev/61959adde761
https://hg.mozilla.org/mozilla-central/rev/71c683c5f7d7
https://hg.mozilla.org/mozilla-central/rev/2323d0f079f9
https://hg.mozilla.org/mozilla-central/rev/86a041d63d65

Comment 27

[Steven Michaud [:smichaud] (Retired)]

Attachment: Interpose library for reverse-engineering use of replacementRange: in apps like Safari

I had some time, so I wrote an interpose library.  I know too little about how IME works to make much use of it.  But you, Masayuki, may find it more helpful.

You should be able to build it using 'make'.  Then follow the instructions in interpose.mm to get other apps (like Safari) to load it.

For complicated reasons, Safari on OS X 10.8 won't allow interpose libraries to load.  But Safari has no problems with them on OS X 10.7.

Let me know if you find anything interesting.