Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Description • 11 years ago

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

	Rudy Lu [:rudyl] (inactive after 2015/7/3)
	Comment 2 • 11 years ago

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.

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 3 • 11 years ago

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

	Jan Jongboom [:janjongboom] (inactive)
	Comment 4 • 11 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 5 • 11 years ago

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

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 6 • 11 years ago

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

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 7 • 11 years ago

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

Thanks.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 8 • 11 years ago

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

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 9 • 11 years ago

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.

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 10 • 11 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 11 • 11 years ago

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.

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 12 • 11 years ago

@Masayuki, really appreciate your help.

Clear the check-in needed flag and I'll fix those addressed in your comments soon. :-)

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 13 • 11 years ago

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

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 14 • 11 years ago

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.

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 15 • 11 years ago

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.

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 16 • 11 years ago

Hi Masayuki, 

This is the diff with previous patch. It shows the changes with the previous patch.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 17 • 11 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 18 • 11 years ago

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.

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 19 • 11 years ago

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;

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 11 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/5f2ab76773e0

Are there tests for this on the Gaia side?

	Ed Morley [:emorley]
	Comment 21 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/5f2ab76773e0

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 22 • 11 years ago

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

	dgod.osa
	Comment 23 • 11 years ago

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

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 24 • 11 years ago

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

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 26 • 11 years ago

Thanks Nakano san:)

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 11 years ago

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.

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 28 • 11 years ago

I got it and thank you.

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/c45e24c1798b