861665 - Allow IME to get notification when text field content is changed

Status: RESOLVED WONTFIX

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

Description

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

As parts of our draft keyboard API(https://wiki.mozilla.org/WebAPI/KeboardIME), we need to allow IME to get notification through mozKeyboard when the content of current input field has changed. This feature is required by Latin keyboard to make suggestion and auto-correction, see Bug 860546(https://bugzilla.mozilla.org/show_bug.cgi?id=860546) for details.

Thus I suggest implementing the following functions:

    // Get the text value of the current input field asynchronously.
    void getText(in long start, in long end, in Function callback);

    // Length of the content
    readonly attribute long textLength;

    // Listener to get noftified when the content of the current input field has changed.
    attribute Function ontextchange;

Examples:
    navigator.mozKeyboard.ontextchange = function() {
      navigator.mozKeyboard.getText(0, navigator.mozKeyboard.textLength - 1, function(text) {
        console.log(text);
      }
    }

Comment 1

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

Attachment: Support getText, textLength and ontextchange for mozKeyboard

The patch implements the following methods and properties:

  // Listener to get noftified when the content of the current input field has
  // changed.
  attribute nsIDOMEventListener ontextchange;

  /*
   * Get a substring of the text content of the current input field.
   * @param start The start index of the substring.
   * @param end The end index of the substring. The character at the end index
   * is not included.
   */
  void getText(in long start, in long end, in nsIKeyboardGetTextCallback callback);

  // Length of the text content of the current input field.
  readonly attribute long textLength;

Comment 2

[David Flanagan [:djf]]

Comment on attachment 737378 [details] [diff] [review]
Support getText, textLength and ontextchange for mozKeyboard

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

I'm very excited that you're working on this, but I'm giving an r- for now.

My main concern is that when the keyboard opens, I think the current implementation sends a textchange event even when the text has not changed.

But I'm also worried about race conditions when using the API as it is designed. If I get a textchange event and have to make an async call to find out what the new text is, there might be other changes by the time I get a response. I'm thinking about things like an etherpad app with asynchronous edits happening over the network. We can't avoid race conditions entirely for an app like that.  But I'd really like to have the current text sent with each textchange event.  Or a synchronous property (like textLength) for querying the text.

Also, we should think about how this new API is going to be used.  Currently the keyboard does its best to track the state of the input field based on the edits it makes.  When it calls sendKey(), it updates its own internal representation of the text and cursor position.  As it stands, it only needs to receive textchange events when edits happen that it does not know about.  In that case, it would be fine if forms.js just sent another Form:Input event like it does when the user taps to move the cursor.

But this patch generates a textchange event for every edit, including those that result from sendKey (or from the newer insert/delete/replace function, I think).  With this patch landed, I could modify the keyboard so that it did not track its own state and just called sendKey() and responded to textchange events.  But then I couldn't start predicting the next word (auto correct and word suggestions) until I received the textchange event. So the keyboard will probably keep tracking its own changes and will have to discard and ignore most textchange events.

I think overall it would be better if textchange was only sent for edits that did not originate through the mozKeyboard API.

Finally, note that I am not qualified to review the Gecko implementation details of this patch. I don't have much experience with chrome code at all, so you should probably have someone like Fabrice look at it also.

::: b2g/chrome/content/forms.js
@@ +242,4 @@
>        if (isContentEditable(element)) {
>          this._documentEncoder = getDocumentEncoder(element);
>        }
> +      this._editor = getPlaintextEditor(element);

Now that you're using the plaintext editor, can you get rid of the document encoder in this file? (Or at least file a bug to remove it in the future if it is not necessary)

@@ +264,5 @@
> +
> +  // Implments nsIEditorObserver to get notification when the text content of
> +  // current input field has changed.
> +  EditAction: function fa_editAction() {
> +    this.updateTextContent();

Is this going to trigger a textchange event every time the keyboard uses the mozKeyboard API to send a key?  Do we want that?  Or should it only trigger when the text is alterned independently of the keyboard?

@@ +291,5 @@
>  
>          if (this.isFocusableElement(target)) {
>            this.showKeyboard(target);
>            this.updateSelection();
> +          this.updateTextContent();

the call to showKeyboard() calls showKeyboarState() which calls getJSON() which gets the full text and sends it to the keyboard.

Then updateTextContent() gets the text again, a different way. But because getJSON didn't set _text, it generates a textchanged event even though the text may not have changed.

Maybe we don't need updateTextContent to be called here and above.  Or maybe you need to integrate the Forms:Input event with the Forms:TextChange event so that they work more closely together.

@@ +525,5 @@
> +    let element = this.focusedElement;
> +    let text = element.value || "";
> +    if (isContentEditable(element)) {
> +      text = getContentEditableText(element);
> +    }

You have two conditionals in a row here. How about an if/else instead of an || followed by an if?

@@ +529,5 @@
> +    }
> +    if (text !== this._text) {
> +      this._text = text;
> +      sendAsyncMessage("Forms:TextChange", {
> +        textLength: text.length

Can we pass other information with the event?  The new cursor position at least?

It seems inefficient to receive a textchanged event and then have to make an async query to retrieve the new text. Can we just pass the entire text content?  What are the performance implications if the user is editing an etherpad or something. Is transfering multiple kilobytes on each change too much?  Can we at least pass the text around the cursor to the nearest whitespace on each side maybe?  Including the whitespace.

I'm trying to think of what the keyboard needs to handle this event without having to immediately query the chnaged text.

Actually, instead of sending the entire text, in the event, could we send a diff?  Specify a range of characters that have been removed and a string of characters that have been inserted.  For edits that are more complicated as that, encode them as deleting everything and inserting an entirely new string. That would be efficient and useful to the keyboard code, I think.

That probably only works if we send a textchange for everything, including changes made through the keyboard API.  

The race conditions here are really hard!

::: b2g/components/MozKeyboard.js
@@ +159,5 @@
> +  get ontextchange() {
> +    return this._textHandler;
> +  },
> +
> +  getText: function mozKeyboardGetText(start, end, callback) {

Can we allow the user to omit the end argument like we can for String.substring()?  

Can we allow negative arguments like we can for String.slice()?

@@ +175,5 @@
> +    this._getTextCallbacks.push(callback);
> +    cpmm.sendAsyncMessage('Keyboard:GetText', {
> +      'start': start,
> +      'end': end
> +    });

If you pass at least some of the text around the cursor whenever the text changes, then we could cache it here, and you'd only have to make an asynchronous query if the keyboard asked for text that was outside of the cached range.

So if you don't want to pass the text context in the event, you could at least avoid async round trips by caching the text here.

I think that passing the text with the textchange event is the cleanest way to avoid race conditions, however.

@@ +220,5 @@
>        handler.handleEvent(evt);
> +    } else if (msg.name == "Keyboard:TextChange") {
> +      this._textLength = msg.json.textLength;
> +
> +      let handler = this._textHandler;

Do we not support addEventListener() for mozKeyboard?  It seems like we should at least have a bug filed to add support for that eventually.  Probably out of scope for this bug, though.

@@ +225,5 @@
> +      if (!handler || !(handler instanceof Ci.nsIDOMEventListener))
> +        return;
> +
> +      let evt = new this._window.CustomEvent("textchange",
> +          ObjectWrapper.wrap({}, this._window));

It would be great if we could pass the text length, the cursor position and the text itself (or at least the text around the cursor as part of this event).

@@ +231,5 @@
> +    } else if (msg.name == "Keyboard:GetTextCallback") {
> +      let text = msg.json.text;
> +      let callback = this._getTextCallbacks.shift();
> +      if (callback) {
> +        callback.handleEvent(text);

Shouldn't you protect this with a try/catch?  Or is that not necessary in chrome code?

::: b2g/components/b2g.idl
@@ +67,5 @@
>     * To move the cursor, set the start and end position to the same value.
>     */
>    void setSelectionRange(in long start, in long end);
> +
> +  // Listener to get noftified when the content of the current input field has

s/noftified/notified/

Comment 3

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

Thanks for your review.

There are many things to do with the text change event.

We're discussing text change event in dev-webapi mail list with the title "Proposal: mozKeyboard API for extending built-in keyboard and supporting 3rd-party Keyboard app"(https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.webapi/A7dIBaR3lpU).

Can you send your comments to [e-mail] as well? I think it will be a great help to the API implementation.

Comment 4

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Add ontextchange event to keyboard API

This patch tries to address the concerns raised a couple of months ago.

Major point: we no longer expose an async getText() method but rather add event data to the ontextchange event. This event data consists of the length of the current text field, plus it contains a diff that can be used to keep track of internal state, this way we avoid that a lot of data flows over the line. This patch will generate an event on every change in the active text field, instead of only when re-focussing like in the last patch because omitting some events will cause an unmaintainable state. F.e. typing in hardware keyboard was not updating the state, nor was copying pasting. The patch is generated through a modified version of https://github.com/kpdecker/jsdiff (which is BSD licensed, so if there's something wrong with that please tell me), with a patch format that can be parsed on the frontend like:

```
// o contains the original string
var bas = o.split('');
var pos = 0;
diff.forEach(function(r) {
  if (r[0] === 1) {
    bas.splice.apply(bas, [pos, 0].concat(r[1].split('')));
    pos += r[1].length;
  }
  else if (r[0] === 0) {
    pos += r[1];
  }
  else if (r[0] === -1) {
    bas.splice(pos, r[1]);
  }
})
console.log(bas.join(''))
```

So TL;DR:

* Synchronous code, state comes in together with the event (I also patched the selection code which suffered from the same problem)
* Diffing instead of sending all text

Comment 5

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

Jan, I'm glad that you would like to work on it. It is a good implementation to resolve the performance issue of sending the whole text content with ontextchange event. But diff data is more complicated for developers to use, the keyboard app needs extra effort to decode the diff data and maintain the internal state. Additionally, in one case(see Bug 888076 for details), if user switches from one IME to another, the internal state cannot be passed and the keyboard would fail to parse the text content only by diff data.

One simple solution for this bug we proposed is to send part of the text around the cursor position with the textchange event, because when handling text content, in most cases the keyboard only cares about the text before or after the cursor and the text far from the cursor is ignored. I think it is at least true for auto-correction and suggestion features of the latin keyboard. So we suggest implement the following method instead:

    /*
     * This event is sent when the text around the cursor is changed, due to either text
     * editing or cursor movement. The text length is limited to 100 characters for each
     * back and forth direction.
     *
     * The event handler function is specified as:
     * @param beforeString Text before and including cursor position.
     * @param afterString Text after and excluing cursor position.
     * function(long contextId, DOMString beforeText, DOMString afterText) {
     * ...
     *  }
     */
     attribute EventHandler onsurroundingtextchange;

Comment 6

[Jan Jongboom [:janjongboom] (inactive)]

I don't agree on the 'harder to parse', the diff format is extremely easy and can be applied in 10 LOCs as seen in comment 4. Getting part of the text that is subject to it's own rules is in a way also a diff mechanism but with it's own quirks. A lot more data will go over the channel, and we have to do manual parsing to keep state there anyway but now with an additional ruleset because we only get a fragment. Applying the diff as we have it now would be easier.

For switching IMEs we'd need a `getValue()` function or something that will get the full current text and then take any approach chosen in this bug, so I don't think we should optimize for that already.

Comment 7

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

I would like to keep the API simple and straightforward. If we should allow the keyboard to get the full text, can we hide the implementation details? 

The text content can be tracked in MozKeyboard.js and sync with the input field by diffs. When MozKeyboard sends textchange event to keyboard app, the whole text will be parsed and sent. The keyboard app does not need to parse the diff data again.

Comment 8

[Jan Jongboom [:janjongboom] (inactive)]

How big is the overhead if passing data from MozKeyboard.js -> consuming code? Is it going to affect performance if we send 100 KB of data in such an event? If there's little to no overhead there I'm fine with such an approach.

Comment 9

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

Hi Jan, Yuan,

Thanks for working on this part.

1. I somehow share the concern with Yuan that "diff data" seems a little too complicated IMHO, I don't remember I have seen any APIs designed with diff data passed as event.detail.
   
2. I was wondering if we could do a simple profiling about comment 8, so that we can this simpler API as a try to tackle this issue.

Thanks.

Comment 10

[Jan Jongboom [:janjongboom] (inactive)]

Here's a new approach...

1. The most important state data is the wordBeforeCursor, because that's used for completion, and mainly why we're holding state at the moment
2. We're having two state events currently onselectionchange and ontextchange
3. We're replicating state data in latin.js that we already have on the gecko side

So my proposal is:

1. Have one state event, onselectionchange would do (or maybe call it onstatechange)
2. onselectionchange event data will contain wordBeforeCursor and wordAfterCursor (if needed)
3. latin.js in gaia relies on onselectionchange to render suggestions etc. and doesn't maintain full state there
4. All keyboard interactions that change state will kick off an event thus relying on just the event will always be accurate (ontextchange already does this in current patch)


Additionally: Switching IMEs will be fine because no internal state is required in the IME so one onselectionchange event is enough.

Feedback?

Comment 11

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

(In reply to Jan Jongboom [:janjongboom] from comment #10)
> Here's a new approach...
> 
> 1. The most important state data is the wordBeforeCursor, because that's
> used for completion, and mainly why we're holding state at the moment
> 2. We're having two state events currently onselectionchange and ontextchange
> 3. We're replicating state data in latin.js that we already have on the

I think not only wordBeforeCursor is needed, we also need to wordAfter to know if the cursor is in the middle of a word. In this case, the keyboard app might have different behavior of suggestion/auto-correction.

> gecko side
> 
> So my proposal is:
> 
> 1. Have one state event, onselectionchange would do (or maybe call it
> onstatechange)
> 2. onselectionchange event data will contain wordBeforeCursor and
> wordAfterCursor (if needed)
> 3. latin.js in gaia relies on onselectionchange to render suggestions etc.
> and doesn't maintain full state there
> 4. All keyboard interactions that change state will kick off an event thus
> relying on just the event will always be accurate (ontextchange already does
> this in current patch)
> 
> 
> Additionally: Switching IMEs will be fine because no internal state is
> required in the IME so one onselectionchange event is enough.
> 
> Feedback?

Yes, I guess that will be onsurroundingtextchange in our new WebAPI of ime, which will combine both onselectionchange and ontextchange.

Comment 12

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

Attachment: parameter_passing_test

(In reply to Jan Jongboom [:janjongboom] (PTO until 7/28) from comment #8)
> How big is the overhead if passing data from MozKeyboard.js -> consuming
> code? Is it going to affect performance if we send 100 KB of data in such an
> event? If there's little to no overhead there I'm fine with such an approach.

I create a test with the attachment patch that passes data via `focuschange` event. The test result shows the size of data passed has nothing to do with the performance.

I guess the data is passed by reference, and size of the data doesn't affect the size of the reference. As a result, passing large data won't increase the overhead.

The following is the test result.

Text size: 0.001KB, Time: 0.000013s
Text size: 0.01KB, Time: 0.000012s
Text size: 0.1KB, Time: 0.000012s
Text size: 1KB, Time: 0.000013s
Text size: 10KB, Time: 0.000012s
Text size: 100KB, Time: 0.000012s
Text size: 1000KB, Time: 0.000013s

Comment 13

[Jan Jongboom [:janjongboom] (inactive)]

Good, but the overhead will then by in the IPC layer (child -> keyboard process) so it's an interesting topic.

Comment 14

[Jan Jongboom [:janjongboom] (inactive)]

Superseeded by bug 899999. (Which is an amazing number)