904530 - Implement composition methods for InputContext API

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Description

[Yuan Xulei [:yxl] (Leave from May 18)]

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

Comment 2

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

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.

Comment 3

[Yuan Xulei [:yxl] (Leave from May 18)]

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

Comment 4

[Jan Jongboom [:janjongboom] (inactive)]

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

Comment 5

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

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

Comment 6

[Yuan Xulei [:yxl] (Leave from May 18)]

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

Comment 7

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Implement setComposition and endComposition.

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

Thanks.

Comment 8

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

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

Comment 9

[Yuan Xulei [:yxl] (Leave from May 18)]

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 10

[Yuan Xulei [:yxl] (Leave from May 18)]

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 11

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

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.

Comment 12

[Yuan Xulei [:yxl] (Leave from May 18)]

@Masayuki, really appreciate your help.

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

Comment 13

[Yuan Xulei [:yxl] (Leave from May 18)]

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

Comment 14

[Yuan Xulei [:yxl] (Leave from May 18)]

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.

Comment 15

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Implement setComposition and endComposition.

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.

Comment 16

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: diff to help review(don't check in)

Hi Masayuki, 

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

Comment 17

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

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

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

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.

Comment 19

[Yuan Xulei [:yxl] (Leave from May 18)]

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;

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Are there tests for this on the Gaia side?

Comment 21

[Ed Morley [:emorley]]

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

Comment 22

[Yuan Xulei [:yxl] (Leave from May 18)]

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

Comment 23

[dgod.osa]

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

Comment 24

[Yuan Xulei [:yxl] (Leave from May 18)]

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

Comment 25

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: part2 - enComposition should commit text before cursor.

Comment 26

[Yuan Xulei [:yxl] (Leave from May 18)]

Thanks Nakano san:)

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 28

[Yuan Xulei [:yxl] (Leave from May 18)]

I got it and thank you.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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