Closed Bug 899999 Opened 7 years ago Closed 7 years ago

Implement new InputContext API

Categories

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

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: janjongboom, Assigned: janjongboom)

References

Details

Attachments

(1 file, 5 obsolete files)

Implement the API for InputContext as specified in https://wiki.mozilla.org/WebAPI/KeboardIME
The InputMethodManager is implemented in bug 885692. This bug supersedes bug 861665.
Assignee: nobody → janjongboom
Depends on: keyboard-api
No longer depends on: keyboard-api
Attached patch 899999_v1.txt (obsolete) — Splinter Review
This is inputcontext API, most things are implemented but some aren't (sendKey, composition). But I'd like to get feedback about this before continueing.

Questions/remarks from my end:
* As we aren't in WebIDL land I implemented this with DOMRequests and nsIDOMEventListener
* `Otherwise false if the input context becomes void.`: So the DOMRequest should throw if the inputcontext changes? But probably the event will still have reached its intended target and have done something so don't know.
* I think we should add `textBeforeCursor` and `textAfterCursor` to inputcontext as we also have selectionStart and selectionEnd
* I made the events a bit more elaborate than described in spec (also goes for the return value of the DOMRequests)
* I didnt implement onselectionchange because I think the use case of having another event (as described by :yxl) is extremely uncommon but requires monitoring two separate events to keep state
* The inputcontext and inputcontextchange events are now created on mozKeyboard because I didn't want to break existing keyboard spec. It will be moved to InputMethod when the InputMethodManager patch is landed. So the mozKeyboard code will all go.

Anyway, let me know what you think.
Attachment #784349 - Flags: review?
Attachment #784349 - Flags: feedback?(xyuan)
Flags: needinfo?(fabrice)
(In reply to Jan Jongboom [:janjongboom] from comment #2)
> Created attachment 784349 [details] [diff] [review]
> 899999_v1.txt
> 
> This is inputcontext API, most things are implemented but some aren't
> (sendKey, composition). But I'd like to get feedback about this before
> continueing.
> 
> Questions/remarks from my end:
> * As we aren't in WebIDL land I implemented this with DOMRequests and
> nsIDOMEventListener
`Promise` is able to used without WebIDL. You can continue to implement with DOMRequests,
 but I expect we will switch to `Promise` later.

> * `Otherwise false if the input context becomes void.`: So the DOMRequest
> should throw if the inputcontext changes? But probably the event will still
> have reached its intended target and have done something so don't know.
For v1.2, the keyboard app will be OOPed and we cannot send key event directly to the app receiving user's input. We need to change its implementation to that of `replaceSurroundingText`, i.e. sending a message to forms.js and then generating key events there. If so we can block invalid `sendKey` as `replaceSurroundingText`.

> * I think we should add `textBeforeCursor` and `textAfterCursor` to
> inputcontext as we also have selectionStart and selectionEnd
Agree. Would there exists any performance issue with `textBeforeCursor` and `textAfterCursor`?

> * I made the events a bit more elaborate than described in spec (also goes
> for the return value of the DOMRequests)
As we can access select range and text around cursor info from inputContext, do the events and return value need to take the same info?

> * I didnt implement onselectionchange because I think the use case of having
> another event (as described by :yxl) is extremely uncommon but requires
> monitoring two separate events to keep state
I'm not a big fan of onselectionchange, but keeping it will make the API much clear if the IME just wants to get cursor position without surrounding text.

> * The inputcontext and inputcontextchange events are now created on
> mozKeyboard because I didn't want to break existing keyboard spec. It will
> be moved to InputMethod when the InputMethodManager patch is landed. So the
> mozKeyboard code will all go.
That's OK.

> 
> Anyway, let me know what you think.
Attachment #784349 - Attachment is patch: true
(In reply to Yuan Xulei [:yxl] from comment #3)
> > * I didnt implement onselectionchange because I think the use case of having
> > another event (as described by :yxl) is extremely uncommon but requires
> > monitoring two separate events to keep state
> I'm not a big fan of onselectionchange, but keeping it will make the API
> much clear if the IME just wants to get cursor position without surrounding
> text.
The `onsurroundingtextchange` event does both. If you want to get cursor position, rely on that, it will fire for all changes. You just get surrounding text for free.
Maybe we can just name it `onselectionchange` and ditch `onsurroundingtextchange`. But just have the before and after word added to the event. (Ergo: renaming the event :-))
The following are other reasons I want to keep two separate events.

`onsurroundingtextchange` has overlap with `onselectionchange`, but they don't cover each other.

cases that `onsurroundingtextchange` is sent but `onselectionchange` isn't:
1) append text after cursor;
2) remove text after cursor;
3) replace the text around cursor with different text of the same length.

cases that `onselectionchange` is sent but `onsurroundingtextchange` isn't:
1) cursor doesn't move, but selection range has been changed.
2) cursor move to a position where the text is the same as the previous.

Also the two events can be used in different user cases:
1) for auto-correction, it needs the info of surrounding text, but doesn't care about the selection range, so it will listen to `onsurroundingtextchange` only.
2) but for swipe selection, it needs the selection range value, but not the surrounding text, and thus `onselectionchange` is better.
Comment on attachment 784349 [details] [diff] [review]
899999_v1.txt

Review of attachment 784349 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/components/MozKeyboard.js
@@ +163,5 @@
>         if (msgJson.type != "blur") {
>           this._selectionStart = msgJson.selectionStart;
>           this._selectionEnd = msgJson.selectionEnd;
> +
> +         this.setInputContext(msgJson);

We need create a new MozInputContext here. I think IME should get different MozInputContext objects with different input fields. If so, we can prevent IME app from sending message to forms.js when inputContext becomes void or is out of date.
Attachment #784349 - Flags: feedback?(xyuan) → feedback-
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #784349 - Attachment is obsolete: true
Attachment #784349 - Flags: review?
Attachment #785715 - Attachment is patch: true
Attached patch Patch v4 (obsolete) — Splinter Review
Implemented everything except for composition, added it under mozInputMethod. Can remove the whole mozKeyboard code now.
Attachment #785715 - Attachment is obsolete: true
Attachment #785720 - Flags: review?(xyuan)
Comment on attachment 785720 [details] [diff] [review]
Patch v4

Review of attachment 785720 [details] [diff] [review]:
-----------------------------------------------------------------

Good job and you just need a polish.

::: b2g/app/b2g.js
@@ +745,5 @@
>  // Enable the disk space watcher
>  pref("disk_space_watcher.enabled", true);
>  
>  // Enable promise
> +pref("dom.promise.enabled", true);

Don't change the setting of `Promise` unless we need.

::: b2g/chrome/content/forms.js
@@ +406,5 @@
>        return;
>      }
>  
>      this._editing = true;
>      let json = msg.json;

`json` is redefined.

@@ +495,5 @@
>          break;
>        }
> +
> +      case "Forms:GetText": {
> +        let value = this.isTextInputElement(target) ?

`this.isTextInputElement(target)` should be replaced by `isPlainTextField`, where
`isPlainTextField = element instanceof HTMLInputElement || element instanceof HTMLTextAreaElement`.

@@ +593,5 @@
>    },
>  
> +  getSelectionInfo: function fa_getSelectionInfo() {
> +    let element = this.focusedElement;
> +    let range =  getSelectionRange(this.focusedElement);

Don't need to retrieve focused element again.
`let range = getSelectionRange(element);`

@@ +622,3 @@
>    // Notify when the selection range changes
>    updateSelection: function fa_updateSelection() {
> +    sendAsyncMessage("Forms:SelectionChange", this.getSelectionInfo());

Send message only if the selection range or the surrounding text has changed.

::: b2g/components/MozKeyboard.js
@@ +318,5 @@
> +  },
> +
> +  setInputContext: function mozKeyboardContextChange(data) {
> +    if (data) {
> +      this._inputcontext = new MozInputContext(data);

Maybe you need uninitialize `this._inputcontext` before assigning new value.

@@ +344,5 @@
> + * InputContext
> + * ==============================================
> + */
> +function MozInputContext(ctx) {
> +  this._context = {

An issue that needs to consider is how to prevent IME app from sending messages to forms.js when inputContext becomes void or is out of date.

I think one way to do this is to give a unique ID to each InputContext. Anytime the current input context is changed, we generate a new context ID. Every message sent to forms.js contains a context ID. forms.js should check the context ID before modifying the input field.

@@ +367,5 @@
> +  classID: Components.ID("{1e38633d-d08b-4867-9944-afa5c648adb6}"),
> +
> +  QueryInterface: XPCOMUtils.generateQI([
> +    Ci.nsIB2GInputContext,
> +    Ci.nsIDOMGlobalPropertyInitializer,

Ci.nsIDOMGlobalPropertyInitializer is not needed. As we don't expose InuputContext as navigator attribute.

@@ +383,5 @@
> +  init: function ic_init(win) {
> +    let principal = win.document.nodePrincipal;
> +    let perm = Services.perms
> +               .testExactPermissionFromPrincipal(principal, "keyboard");
> +    if (perm != Ci.nsIPermissionManager.ALLOW_ACTION) {

As the permission is checked in its parent of MozInputMethod, don't need check it again.

@@ +393,5 @@
> +    this._window = win;
> +    this._utils = win.QueryInterface(Ci.nsIInterfaceRequestor)
> +                     .getInterface(Ci.nsIDOMWindowUtils);
> +
> +    this.initDOMRequestHelper(win,

To avoid creating and destorying DOMRequestHelper many times, you can keep DOMRequestHelper in MozInputMethod.

And we also need to call destroyDOMRequestHelper to free DOMRequestHelper when don't need it.

@@ +401,5 @@
> +       "Keyboard:ReplaceSurroundingText:Result:OK",
> +       "Keyboard:SendKey:Result:OK"]);
> +  },
> +
> +  uninit: function ic_uninit() {

Before destorying complete destroy InputContext. We need to deal with all pending dom requests. Maybe call fireError for each dom request.

@@ +424,5 @@
> +      case "Keyboard:ReplaceSurroundingText:Result:OK":
> +        Services.DOMRequest.fireSuccess(request,
> +          ObjectWrapper.wrap(json.selectioninfo, this._window));
> +        break;
> +    }

Call DOMRequestIpcHelper#removeRequest to remove useless requtest.

@@ +436,5 @@
> +    let selectionDirty = this._context.selectionStart !== ctx.selectionStart ||
> +          this._context.selectionEnd !== ctx.selectionEnd;
> +    let surroundDirty = this._context.textBeforeCursor !== ctx.textBeforeCursor ||
> +          this._context.textAfterCursor !== ctx.textAfterCursor;
> +

Udpate values of selectionStart, selectionEnd, textBeforeCursor and textAfterCursor only if they are diry.

::: b2g/components/b2g.idl
@@ +20,5 @@
> +  readonly attribute DOMString type;
> +
> +  // The type of the input field, which is enum of text, number, password, url, search, email, and so on.
> +  // See http://www.whatwg.org/specs/web-apps/current-work/multipage/states-of-the-type-attribute.html#states-of-the-type-attribute
> +  readonly attribute DOMString inputtype;

We just made a slight change to the API specification to make the names of these attributes camel-cased.

Plese change the name to `inputType`.

@@ +26,5 @@
> +  /*
> +   * The inputmode string, representing the input mode.
> +   * See http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#input-modalities:-the-inputmode-attribute
> +   */
> +  readonly attribute DOMString inputmode;

change this to `inputMode`.

@@ +218,4 @@
>    void hide();
>  };
>  
>  [scriptable, uuid(5c7f4ce1-a946-4adc-89e6-c908226341a0)]

Set a new uuid when the interface changes.

@@ +223,5 @@
>  {
>    // Input Method Manager contain a few global methods expose to apps
>    readonly attribute nsIInputMethodManager mgmt;
> +
> +   // inputcontext glue...

If possible, leave more comments about the attribute.
Attachment #785720 - Flags: review?(xyuan) → review-
Attached patch 899999_v6.diff (obsolete) — Splinter Review
Hi Yuan, Thanks for your elaborate comments. I've changed the PR accordingly.

The behavior for now is that when you try to use an inputcontext that has been invalidated, calling any function will throw and all attributes are set to NULL. All pending requests will be rejected. Because we rely on standard DOMRequestHelper functionality here we don't need to keep a counter ourselves. Does that sound reasonable?
Attachment #785720 - Attachment is obsolete: true
Attachment #786203 - Flags: review?(xyuan)
Comment on attachment 786203 [details] [diff] [review]
899999_v6.diff

Review of attachment 786203 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to give r=+, but there exists a special case that the patch doesn't cover.

Sine messages between InputContext and forms.js are asynchronous, when the focused element is changed, there is a short delay for the old InputContext to get notified and become invalid. During such short interval of time, the IME may interact with the old InputContext and send message to forms.js. So I believe we need kind of `id` to bind a InputContext and its messages to a specific input element.

::: b2g/chrome/content/forms.js
@@ +497,5 @@
>        }
> +
> +      case "Forms:GetText": {
> +        let isPlainTextField = target instanceof HTMLInputElement ||
> +                      target instanceof HTMLTextAreaElement;

nit: indentation.

::: b2g/components/MozKeyboard.js
@@ +475,5 @@
> +  _fireEvent: function ic_fireEvent(handler, eventName, detail) {
> +    if (!handler || !(handler instanceof Ci.nsIDOMEventListener))
> +      return;
> +
> +    let detail = {

The local variable has the same name with that of the argument. We'd better choose a different name.
Attachment #786203 - Flags: review?(xyuan) → review-
Comment on attachment 786203 [details] [diff] [review]
899999_v6.diff

Review of attachment 786203 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/components/MozKeyboard.js
@@ +246,5 @@
>  
>  MozInputMethod.prototype = {
> +  _inputcontext: null,
> +
> +  classID: Components.ID("{4607330d-e7d2-40a4-9eb8-43967eae0142}"),

no need to change the class ID.

@@ +292,5 @@
> +        if (json.type !== 'blur')
> +          this.setInputContext(json);
> +        else
> +          this.setInputContext(null);
> +        break;

the js coding style is to use {} even for single line if/else. Please change that here and in all other places.

@@ +394,5 @@
> +  destroy: function ic_destroy() {
> +    let self = this;
> +
> +    // I think there is a flaw in the DOMRequestHelper
> +    // _requests is used as an object rather than an array, so should be object

Did you file a bug on this?

@@ +417,5 @@
> +  },
> +
> +  receiveMessage: function ic_receiveMessage(msg) {
> +    let json = msg.json;
> +    let request = json.requestId ? this.takeRequest(json.requestId) : null;

You must guard against a null request.

@@ +475,5 @@
> +  _fireEvent: function ic_fireEvent(handler, eventName, detail) {
> +    if (!handler || !(handler instanceof Ci.nsIDOMEventListener))
> +      return;
> +
> +    let detail = {

like using aName for arguments ;)

@@ +589,5 @@
> +    return request;
> +  },
> +
> +  setComposition: function ic_setComposition(text, cursor) {
> +    // @todo

Is a no-op acceptable here? You should at least print some unconditional error message, or throw (or both).

@@ +593,5 @@
> +    // @todo
> +  },
> +
> +  endComposition: function ic_endComposition(text) {
> +    // @todo

Idem.

@@ +598,5 @@
>    }
>  };
>  
>  this.NSGetFactory = XPCOMUtils.generateNSGetFactory(
> +  [MozKeyboard, MozInputMethodManager, MozInputMethod, MozInputContext]);

I think you don't need MozInputContext here since you never instanciate such objects from xpcom.
Flags: needinfo?(fabrice)
> >  
> >  MozInputMethod.prototype = {
> > +  _inputcontext: null,
> > +
> > +  classID: Components.ID("{4607330d-e7d2-40a4-9eb8-43967eae0142}"),
> 
> no need to change the class ID.
Ah, I thought this needed to be in sync with interface ID, but doesn't really make sense now I think of it.

> @@ +394,5 @@
> > +  destroy: function ic_destroy() {
> > +    let self = this;
> > +
> > +    // I think there is a flaw in the DOMRequestHelper
> > +    // _requests is used as an object rather than an array, so should be object
> 
> Did you file a bug on this?
https://bugzilla.mozilla.org/show_bug.cgi?id=902362
Attached patch 899999_v7.diff (obsolete) — Splinter Review
Thanks Yuan and Fabrice for the reviews so far. I've added a contextId to requests from inputcontext->forms.js so we can verify in forms.js that the incoming message is still up to date. I've addressed the nits.
Attachment #786203 - Attachment is obsolete: true
Attachment #786831 - Flags: review?(xyuan)
Comment on attachment 786831 [details] [diff] [review]
899999_v7.diff

Review of attachment 786831 [details] [diff] [review]:
-----------------------------------------------------------------

We're almost there!

::: b2g/chrome/content/forms.js
@@ +400,5 @@
> +
> +    // To not break mozKeyboard contextId is optional
> +    if ('contextId' in json &&
> +          json.contextId !== this._focusCounter &&
> +          json.requestId) {

nit: indentation is strange and reduce 2 spaces.

@@ +405,5 @@
> +      // Ignore messages that are meant for a previously focused element
> +      sendAsyncMessage("Forms:SequenceError", {
> +        requestId: json.requestId,
> +        error: "Expected contextId " + this._focusCounter +
> +                " but was " + json.contextId

nit: indentaion.

::: b2g/components/MozKeyboard.js
@@ +276,4 @@
>      this._mgmt = new MozInputMethodManager();
> +
> +    cpmm.addMessageListener('Keyboard:FocusChange', this);
> +    cpmm.addMessageListener('Keyboard:SelectionChange', this);

I'm sorry that one thing I missed with the previous patches is that we need add an "inner-window-destroyed" observer to get notified when the window is destroyed so that we can call `unint` to remove these message listeners. 

Please refer to MozKeyboard for details, including:
 1) Implement nsIObserver. Allow QueryInterface to get nsIObserver and define the function `observe: function(subject, topic, data) `.
 2) Call `uninit` to remove message listeners if "inner-window-destroyed" notification is received.
Attachment #786831 - Flags: review?(xyuan) → review-
After discussed with the gaia keyboard devs in Taipei, an extra requirement we need is to create InputContext when the keyboard app launches.

For v1.2 we're going to support 3rd party keyboard apps, there may be more than one keyboard apss installed. The system app is responsible for listening the `focuschange` event and launching the corresponding keyboard app. Thus a keyboard app may be loaded after the `focuschange` event is sent and then can't get `inputcontextchange` event either.

To sove the problem, the keyboard app should actively get the input context information for time it launches. That is to say, InputMethod needs to send a message to forms.js to build the input context if exists when initializing.
Attached patch 899999_v8.diffSplinter Review
This patch adds inner-window roundtrip, and tries to retrieve the context when being first used. It will also trigger an event so you can reliably use that.
Attachment #786831 - Attachment is obsolete: true
Attachment #786876 - Flags: review?(xyuan)
Attachment #786876 - Flags: review?(xyuan) → review+
Can we add tests before landing this patch?
Yeah but I'll need some help with that. Are you online tomorrow during the CET day?
It isn't easy to create test in gecko for this now, as we need to simulate the interaction between a keyboard app and a normal user app. I guess that is why we don't have gecko tests for the original keyboard API. 

So I suggest create a new bug to add some gaia tests instead. We'll check in this patch. Then the gaia keyboard devs can help to test the new API and give feedback for us to improve it.
Keywords: checkin-needed
We should be able to create a test that runs an app with the mozkeyboard permission and exercises the api. Since this is not blocking any release, let's do that properly.
Fair enough. I create bug 902562 for testing the keyboard API. Not only this bug, but also other parts of the API should be properly tested.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Duplicate of this bug: 845171
Depends on: 1090883
You need to log in before you can comment on or make changes to this bug.