The following two methods are required to implement to support composition. /* * Set current composing text. This method will start composition or update composition if it * has started. The composition will be started right before the cursor position and any * selected text will be replaced by the composing text. When the composition is started, * calling this method can update the text and move cursor winthin the range of the composing * text. * @param text The new composition text to show. * @param cursor The new cursor position relative to the start of the composition text. The cursor should * be positioned within the composition text. This means the value should be >= 0 and <= the length of * composition text. Defaults to the lenght of composition text, i.e., the cursor will be positioned after * the composition text. * * The composing text, which is shown with underlined style to distinguish from the existing text, is used * to compose non-ASCII characters from keystrokes, e.g. Pinyin or Hiragana. The composing text is the * intermediate text to help input complex character and is not committed to current input field. Therefore * if any text operation other than composition is performed, the composition will be automatically * canceled and the composing text will be cleared. Same apply when the inputContext is lost during a * unfinished composition session. * * To finish composition and commit text to current input field, an IME should call |endComposition|. */ Promise<boolean> setComposition(DOMString text, [optional] long cursor); /* * End composition, clear the composing text and commit given text to current input field. The text will * be committed before the cursor position. * @param text The text to commited before cursor position. If empty string is given, no text will be * committed. * * Note that composition always ends automatically with nothing to commit if the composition does not * explicitly end by calling |endComposition|, but is interrupted by |sendKey|, |setSelectionRange|, * |replaceSurroundingText|, |deleteSurroundingText|, user moving the cursor, changing the focus, etc. */ Promise<boolean> endComposition(DOMString text);

Hi Jan, Are you still working on this or need us to help? We would need this to accomplish a user story in v1.2 to support simplified Chinese. Thanks.

I'll take this bug from Jan.:-)

:rudyl, yeah I mailed Yuan about it but not the rest of team :-)

Hi, I notify Japanese IME vendor of this bug. Somebody might feedback for the interface design.

@masayuki, thanks. For v1.2, we expect to support essential features similar to that of android and iOS. And don't make the interface too complex.

@Fabrice, please review the input method API changes. @Masayuki, please review the usage of composition. Thanks.

Sorry for the delay. Perhaps, I'll review this in this week.

Hi Masayuki, thanks. If you don't have time, please feel free to pass it to other person. ;-) I wrapped the logic of composition in the CompositionManager of forms.js. You check the usage CompositionManager only.

Comment on attachment 796086 [details] [diff] [review] Implement setComposition and endComposition. Masayuki, I'd like to check in this patch so as to unblock Bug 906617. But I'm still looking forward to your feedback of the patch. Please take your time and I'll create a follow up bug to improve this patch based on your feedback if needed.

Comment on attachment 796086 [details] [diff] [review] Implement setComposition and endComposition. Sorry for the delay. >diff --git a/b2g/chrome/content/forms.js b/b2g/chrome/content/forms.js >--- a/b2g/chrome/content/forms.js >+++ b/b2g/chrome/content/forms.js >@@ -362,26 +364,30 @@ let FormAssistant = { > } > break; > > case 'mousedown': > if (!this.focusedElement) { > break; > } > >+ CompositionManager.endComposition(''); Why do you need this? Gecko notifies nsWidget instance when Gecko wants IME to commit composition forcibly. So, basically, widget shouldn't commit itself for consistent behavior between all platforms. > case 'mouseup': > if (!this.focusedElement) { > break; > } > >+ CompositionManager.endComposition(''); Same. >@@ -417,31 +423,35 @@ let FormAssistant = { > } > break; > > case "keydown": > if (!this.focusedElement) { > break; > } > >+ CompositionManager.endComposition(''); I think that this is okay because other widgets do same thing at key event handling. > case "keyup": > if (!this.focusedElement) { > break; > } > >+ CompositionManager.endComposition(''); This is also okay. >@@ -469,25 +479,29 @@ let FormAssistant = { > break; > } > return; > } > > this._editing = true; > switch (msg.name) { > case "Forms:Input:Value": { >+ CompositionManager.endComposition(''); I don't understand what's this case? > case "Forms:Input:SendKey": >+ CompositionManager.endComposition(''); At dispatching key event, it's good to commit composition forcibly. >@@ -522,31 +536,35 @@ let FormAssistant = { > break; > > case "Forms:Select:Blur": { > this.setFocusedElement(null); > break; > } > > case "Forms:SetSelectionRange": { >+ CompositionManager.endComposition(''); I'm not sure if this is okay for South-Asian languages. However, I guess that if it's not, we will get bug reports. We should fix it later. > case "Forms:ReplaceSurroundingText": { >+ CompositionManager.endComposition(''); Same. >@@ -577,36 +595,56 @@ let FormAssistant = { > break; > } > > case "Forms:GetContext": { > let obj = getJSON(target, this._focusCounter); > sendAsyncMessage("Forms:GetContext:Result:OK", obj); > break; > } >+ >+ case "Forms:SetComposition": { >+ CompositionManager.setComposition(target, json.text, json.cursor); >+ sendAsyncMessage("Forms:SetComposition:Result:OK", { >+ requestId: json.requestId, >+ }); >+ break; >+ } >+ >+ case "Forms:EndComposition": { >+ CompositionManager.endComposition(json.text); >+ sendAsyncMessage("Forms:EndComposition:Result:OK", { >+ requestId: json.requestId, >+ }); >+ break; >+ } > } > this._editing = false; > > }, I don't understand well here. I guess fabrice checked here. > showKeyboard: function fa_showKeyboard(target) { >+ CompositionManager.endComposition(''); I'm not sure if this is actually needed. Are you sure? > hideKeyboard: function fa_hideKeyboard() { >+ CompositionManager.endComposition(''); Also This. >@@ -1030,8 +1068,53 @@ function replaceSurroundingText(element, > if (text) { > // We don't use CR but LF > // see https://bugzilla.mozilla.org/show_bug.cgi?id=902847 > text = text.replace(/\r/g, '\n'); > // Insert the text to be replaced with. > editor.insertText(text); > } > } >+ >+let CompositionManager = { >+ _isStarted: false, >+ _text: '', >+ >+ setComposition: function cm_setComposition(element, text, cursor) { I discussed about this API with Japanese IME vendor this morning. In conclusion, Japanese IME needs support of two or more clauses. So, it's okay to improve in another bug, but I strongly suggest that we should change this API as: setComposition(element, text, clauses, cursor); "clauses" must be null or an array of object which has "selectionType" and "length". Then, in first implementation, 4th or lager clause information should be ignored because nsIDOMWindowUtils::SendTextEvent() supports only 3 clauses. I'll file a bug and post a patch for that it can receive 4 or more clauses. The important things of this change is, 1.2 should have this argument even though it cannot support 4 or more clauses. Then, non-Japanese IME which don't need multiple clause support won't need to change the calls of this API. >+ // Check parameters. >+ if (!element) { >+ return; >+ } >+ let len = text.length; >+ if (cursor < 0) { >+ cursor = 0; >+ } else if (cursor > len) { >+ cursor = len; >+ } >+ >+ // Start composition if need to. >+ if (!this._isStarted) { >+ this._isStarted = true; >+ domWindowUtils.sendCompositionEvent('compositionstart', '', ''); >+ this._text = ''; >+ } >+ >+ // Update the composing text. >+ if (this._text !== text) { I think that this is correct for dispatching compositionupdate event. However, not for sendTextEvent(). The caller might want to update only clauses information and/or cursor information. So, sendTextEvent() should be called always. >+ this._text = text; >+ domWindowUtils.sendCompositionEvent('compositionupdate', text, ''); >+ // Only support one clause. >+ domWindowUtils.sendTextEvent(text, len, >+ domWindowUtils.COMPOSITION_ATTR_RAWINPUT, >+ 0, 0, 0, 0, cursor, 0); >+ } >+ }, >+ >+ endComposition: function cm_endComposition(text) { >+ if (!this._isStarted) { >+ return; >+ } >+ domWindowUtils.sendTextEvent(text, 0, 0, 0, 0, 0, 0, 0, 0); This is wrong. If this._text and text are different, you should dispatch compositionupdate event before sendTextEvent(). >+ domWindowUtils.sendCompositionEvent('compositionend', text, ''); >+ this._text = ''; >+ this._isStarted = false; >+ } >+}; Looks like the others are not may area. feedback=masayuki, if you will improve the setComposition() API for multiple clause support.

@Masayuki, really appreciate your help. Clear the check-in needed flag and I'll fix those addressed in your comments soon. :-)

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #11) > Comment on attachment 796086 [details] [diff] [review] > > >@@ -469,25 +479,29 @@ let FormAssistant = { > > break; > > } > > return; > > } > > > > this._editing = true; > > switch (msg.name) { > > case "Forms:Input:Value": { > >+ CompositionManager.endComposition(''); > > I don't understand what's this case? "Forms:Input:Value" is used by mozKeyboard.setValue to set the value of the current focused element. When the keyboard tries to change the text content of the current focused element, we'll end composition forcibly. > >+ > >+ case "Forms:SetComposition": { > >+ CompositionManager.setComposition(target, json.text, json.cursor); > >+ sendAsyncMessage("Forms:SetComposition:Result:OK", { > >+ requestId: json.requestId, > >+ }); > >+ break; > >+ } > >+ > >+ case "Forms:EndComposition": { > >+ CompositionManager.endComposition(json.text); > >+ sendAsyncMessage("Forms:EndComposition:Result:OK", { > >+ requestId: json.requestId, > >+ }); > >+ break; > >+ } > > } > > this._editing = false; > > > > }, > > I don't understand well here. I guess fabrice checked here. These are the entries of mozInputMethod.setComposition and mozInputMethod.endComposition.

For below comments, I'll add a compositionend lister to CompositionManager. If the composition ends without an explicit calling CompositionManager.endComposition, CompositionManager can be notified and reset its states. (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #11) > > > > case 'mousedown': > > if (!this.focusedElement) { > > break; > > } > > > >+ CompositionManager.endComposition(''); > > Why do you need this? Gecko notifies nsWidget instance when Gecko wants IME > to commit composition forcibly. So, basically, widget shouldn't commit > itself for consistent behavior between all platforms. > > > case 'mouseup': > > if (!this.focusedElement) { > > break; > > } > > > >+ CompositionManager.endComposition(''); > > Same. > > > > showKeyboard: function fa_showKeyboard(target) { > >+ CompositionManager.endComposition(''); > > I'm not sure if this is actually needed. Are you sure? > > > hideKeyboard: function fa_hideKeyboard() { > >+ CompositionManager.endComposition(''); > > Also This.

This patch addressed all the items mentioned in Comment 11. It extends setComposition to support multiple clauses with the following signature: Promise<void> setComposition(DOMString text, optional long cursor, optional sequence<CompositionClauseParameters> clauses); The API change of `setComposition` is temporary, we should discuss it over webapi mail list and refine it.

Hi Masayuki, This is the diff with previous patch. It shows the changes with the previous patch.

(In reply to Yuan Xulei [:yxl] from comment #13) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #11) > > Comment on attachment 796086 [details] [diff] [review] > > > > >@@ -469,25 +479,29 @@ let FormAssistant = { > > > break; > > > } > > > return; > > > } > > > > > > this._editing = true; > > > switch (msg.name) { > > > case "Forms:Input:Value": { > > >+ CompositionManager.endComposition(''); > > > > I don't understand what's this case? > "Forms:Input:Value" is used by mozKeyboard.setValue to set the value of the > current focused element. When the keyboard tries to change the text content > of the current focused element, we'll end composition forcibly. Did you confirm the behavior? While setting a value to input element, script blocker is running. At this time, composition events are ignored: http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsTextEditorState.cpp#1810 http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#5983 And also see bug 549674. > > >+ case "Forms:SetComposition": { > > >+ CompositionManager.setComposition(target, json.text, json.cursor); > > >+ sendAsyncMessage("Forms:SetComposition:Result:OK", { > > >+ requestId: json.requestId, > > >+ }); > > >+ break; > > >+ } > > >+ > > >+ case "Forms:EndComposition": { > > >+ CompositionManager.endComposition(json.text); > > >+ sendAsyncMessage("Forms:EndComposition:Result:OK", { > > >+ requestId: json.requestId, > > >+ }); > > >+ break; > > >+ } > > > } > > > this._editing = false; > > > > > > }, > > > > I don't understand well here. I guess fabrice checked here. > These are the entries of mozInputMethod.setComposition and > mozInputMethod.endComposition. Ah, I see.

Comment on attachment 797384 [details] [diff] [review] Implement setComposition and endComposition. Thank you!! I agree with discussing it, please CC me to it. However, looks very nice to me about the "clauses" implementation. Please check "Forms:Input:Value" case behavior if you can. If your code doesn't work well, I recommend you remove the adding line. Forcibly commit should be performed from Gecko itself as far as possible because it makes more consistent behavior between platforms. Otherwise, keep current change. Then, r=masayuki FYI: Also cursor should be able to specify the length of caret. However, nsEditor currently doesn't support "wide" caret. So, I believe that we don't need to worry about this. In the future, we can change "cursor" argument as an object without API change since JS can check the type of argument.

Hi Masayuki, thank you and it help me a lot:-) I checked the hehaviors of case "Forms:Input:Value", "Forms:SetSelectionRange" and "Forms:ReplaceSurroundingText". They worked well. But if I removed 'CompositionManager.endComposition' for them with the following patch, I got erratic results. So I will keep current change. diff --git a/b2g/chrome/content/forms.js b/b2g/chrome/content/forms.js index 31ea9c1..d82c364 100644 --- a/b2g/chrome/content/forms.js +++ b/b2g/chrome/content/forms.js @@ -490,8 +490,6 @@ let FormAssistant = { this._editing = true; switch (msg.name) { case "Forms:Input:Value": { - CompositionManager.endComposition(''); - target.value = json.value; let event = target.ownerDocument.createEvent('HTMLEvents'); @@ -547,8 +545,6 @@ let FormAssistant = { } case "Forms:SetSelectionRange": { - CompositionManager.endComposition(''); - let start = json.selectionStart; let end = json.selectionEnd; setSelectionRange(target, start, end); @@ -564,8 +560,6 @@ let FormAssistant = { } case "Forms:ReplaceSurroundingText": { - CompositionManager.endComposition(''); - let text = json.text; let beforeLength = json.beforeLength; let afterLength = json.afterLength;

https://hg.mozilla.org/integration/b2g-inbound/rev/5f2ab76773e0 Are there tests for this on the Gaia side?

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #20) > https://hg.mozilla.org/integration/b2g-inbound/rev/5f2ab76773e0 > > Are there tests for this on the Gaia side? Not yet.

in nightly b2g build, endComposition commit text after the current cursor, it sould be "The text will be committed before the cursor position".

(In reply to dgod.osa from comment #23) > in nightly b2g build, endComposition commit text after the current cursor, > it sould be "The text will be committed before the cursor position". Thanks. We need a tiny fix for this issue:-).

Thanks Nakano san:)

https://hg.mozilla.org/integration/b2g-inbound/rev/c45e24c1798b Please don't reopen bugs for follow-up issues unless the original patch is being backed out. It complicates the history and makes them harder to follow. File new bugs blocking the original bug for follow-up work.

I got it and thank you.