861515 - Keyboard should be able to modify the text of the input field directly

Status: RESOLVED FIXED

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

Description

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

Current Latin keyboard uses key events to modify the input field content. To add or delete a character, we need send 3 key events. When doing auto-correction and suggestion, we need much more cost to add and remove a string from the input field.

In order to improve the performance, I suggest to implement part of the draft keyboard API(https://wiki.mozilla.org/WebAPI/KeboardIME) to allow keyboard to directly modify the input filed without key events:

  /*
   * Insert the given text at the cursor position. Replace the selected text if
   * any.
   */
  void commitText(in DOMString text);

  /*
   * Delete around the beginning of the current selection range of the
   * editable text.
   *
   * @param beforeLength The number of characters to be deleted before the
   * beginning of the current selection range.
   * @param afterLength The number of characters to be deleted after the
   * beginning of the current selection range.
   */
  void deleteSurroundingText(in long beforeLength, in long afterLength);

Comment 1

[David Flanagan [:djf]]

It would be big performance win if we could do this. Cc'ing Jed because he has been working on keyboard performance.

I never made the time to review this proposed keyboard API before, sorry. Do we really need two separate methods for this? Couldn't we have a single replaceText() method that could be used for both insertions and deletions?

In the app that contains the text field, what events will be generated?  Will calling commitText() still generate a series of key events in that app?  Or will event handlers running in the app see this kind of insertion as if the user had just done a cut-and-paste?

Finally, if we're implementing more of the mozKeyboard API, can we also implement the parts we'd need to solve bug 860546?  The IM needs to be notified when the content of the text field is changed by JS code. Would that cause a selectionchange event?  Or do we need some other event?

And, if we fix this bug will we be willing to uplift it to v1.1?

Comment 2

[David Flanagan [:djf]]

Bug 835404 is the keyboard performance meta-bug.

Comment 3

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

cc Jonas and Salvador to discuss the API.

To combine commitText with deleteSurroundingText, I propose the following replaceText:

  /*
   * Replace text around the beginning of the current selection range of the
   * editable text.
   *
   * @param text The string to be replaced with. 
   * @param beforeLength The number of characters to be deleted before the
   * beginning of the current selection range. Defaults to 0.
   * @param afterLength The number of characters to be deleted after the
   * beginning of the current selection range. Defaults to 0.
   */
  void replaceSurroundingText(in DOMString text, [optional] in long beforeLength, 
                   [optional] in long afterLength);

We could also use replaceSurroundingText to add and delete string around the cursor position:
  replaceSurroundingText('something to add'); // add a string.
  replaceSurroundingText('', 5); // delete 5 characters before the cursor position.

When keyboard calls replaceSurroundingText(or commitText and  deleteSurroundingText), the app see this kind of insertion as if the user had just done a cut-and-paste.

Comment 4

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

(In reply to David Flanagan [:djf] from comment #1)
> It would be big performance win if we could do this. Cc'ing Jed because he
> has been working on keyboard performance.
> 
> I never made the time to review this proposed keyboard API before, sorry. Do
> we really need two separate methods for this? Couldn't we have a single
> replaceText() method that could be used for both insertions and deletions?
> 
> In the app that contains the text field, what events will be generated? 
> Will calling commitText() still generate a series of key events in that app?
> Or will event handlers running in the app see this kind of insertion as if
> the user had just done a cut-and-paste?
> 
> Finally, if we're implementing more of the mozKeyboard API, can we also
> implement the parts we'd need to solve bug 860546?  The IM needs to be
> notified when the content of the text field is changed by JS code. Would
> that cause a selectionchange event?  Or do we need some other event?
Yes, we can implement the parts for bug 860546. Actually I hope to implement other parts as well. 

When content of the text field is changed, it may not cause the selectionchange event if the cursor position isn't changed. But now even the cursor position is changed after the content of the text field is modified by JS code, the selectionchange event won't be fired. It's a bug of the selectionchange event.

We need a new textchange event to solve bug 860546.

> 
> And, if we fix this bug will we be willing to uplift it to v1.1?
Yes, I hope so though I don't have the right to uplift. :-)

Comment 5

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

Attachment: replaceSurroundingText of mozKeyboard

Implement replaceSurroundingText for mozKeyboard.

  /*
   * Replace text around the beginning of the current selection range of the
   * editable text.
   *
   * @param text The string to be replaced with. 
   * @param beforeLength The number of characters to be deleted before the
   * beginning of the current selection range. Defaults to 0.
   * @param afterLength The number of characters to be deleted after the
   * beginning of the current selection range. Defaults to 0.
   */
  void replaceSurroundingText(in DOMString text, [optional] in long beforeLength, 
                   [optional] in long afterLength);

The patch uses nsIPlaintextEditor to modify the input field text.

Comment 6

[[:fabrice] Fabrice Desré]

Comment on attachment 737205 [details] [diff] [review]
replaceSurroundingText of mozKeyboard

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

::: b2g/chrome/content/forms.js
@@ +237,5 @@
>        element.addEventListener('mouseup', this);
>        if (isContentEditable(element)) {
>          this._documentEncoder = getDocumentEncoder(element);
>        }
> +      this._editor = getPlaintextEditor(element);

Maybe we could do that lazily at first use?

@@ +746,5 @@
> +  if (element instanceof HTMLInputElement ||
> +      element instanceof HTMLTextAreaElement) {
> +    // Get from the <input> and <textarea> elements
> +    element.QueryInterface(Ci.nsIDOMNSEditableElement);
> +    editor = element.editor;

nit: editor = element.QueryInterface(Ci.nsIDOMNSEditableElement).editor;

@@ +754,5 @@
> +    let editingSession = win.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                  .getInterface(Ci.nsIWebNavigation)
> +                                  .QueryInterface(Ci.nsIInterfaceRequestor)
> +                                  .getInterface(Ci.nsIEditingSession);
> +    editor = editingSession.getEditorForWindow(win);

is editingSession guaranteed to be not null?

@@ +790,5 @@
> +  if (start != end) {
> +    // Delete the selected text
> +    editor.deleteSelection(Ci.nsIEditor.ePrevious, Ci.nsIEditor.eStrip);
> +  }
> +  

Nit: trailing whitespace

::: b2g/components/MozKeyboard.js
@@ +143,5 @@
>      return this._focusHandler;
>    },
>  
> +  replaceSurroundingText: function mozKeyboardReplaceSurroundingText(text,
> +      beforeLength, afterLength) {

Nit: the alignment is strange.

@@ +146,5 @@
> +  replaceSurroundingText: function mozKeyboardReplaceSurroundingText(text,
> +      beforeLength, afterLength) {
> +    let before = 0;
> +    let after = 0;
> +    if (typeof beforeLength !== 'undefined') {

I would rather check that this is actually a number.

@@ +149,5 @@
> +    let after = 0;
> +    if (typeof beforeLength !== 'undefined') {
> +      before = beforeLength;
> +    }
> +    if (typeof afterLength !== 'undefined') {

Same here

@@ +152,5 @@
> +    }
> +    if (typeof afterLength !== 'undefined') {
> +      after = afterLength;
> +    }
> +    cpmm.sendAsyncMessage('Keyboard:ReplaceSurroundingText', {

What do we do if text is null or undefined? Do we want text || "" ?

::: b2g/components/b2g.idl
@@ +65,5 @@
> +  /*
> +   * Replace text around the beginning of the current selection range of the
> +   * editable text.
> +   *
> +   * @param text The string to be replaced with. 

Nit: trailing whitespace

@@ +72,5 @@
> +   * @param afterLength The number of characters to be deleted after the
> +   * beginning of the current selection range. Defaults to 0.
> +   */
> +  void replaceSurroundingText(in DOMString text, [optional] in long beforeLength, 
> +                   [optional] in long afterLength);

You need to change the interface UUID

Comment 7

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

Attachment: replaceSurroundingText

Fix all as comment 6 mentioned.

One thing I'm not sure is about the alignment. I searched some source code of mozilla, but didn't find a direct answer of the conventional way to wrap the line. It seems I should use 2 spaces instead of 4 to indent, right?

::: b2g/components/MozKeyboard.js
@@ +143,5 @@
>      return this._focusHandler;
>    },
>  
> +  replaceSurroundingText: function mozKeyboardReplaceSurroundingText(text,
> +      beforeLength, afterLength) {

Nit: the alignment is strange.

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 737259 [details] [diff] [review]
replaceSurroundingText

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

We're almost there, but I'd like to see another version.

::: b2g/chrome/content/forms.js
@@ +246,5 @@
>    get documentEncoder() {
>      return this._documentEncoder;
>    },
>  
> +  // Get the nsIPlaintextEditor object of current input field.

Nit: end the sentence with a '.' (do that for all other comments as well)

@@ +253,5 @@
> +      let element = this.focusedElement;
> +      if (element) {
> +        this._editor = getPlaintextEditor(element);
> +      }
> +    }

What about:
if (!this._editor && this.focusedElement) {
  this._editor = getPlainTextEditor(this.focusedElement);
}

::: b2g/components/MozKeyboard.js
@@ +44,5 @@
>                 .testExactPermissionFromPrincipal(principal, "keyboard");
>      if (perm != Ci.nsIPermissionManager.ALLOW_ACTION) {
>        dump("No permission to use the keyboard API for " +
>             principal.origin + "\n");
> +      return;

Why did you change that?

@@ +146,5 @@
> +  replaceSurroundingText: function mozKeyboardReplaceSurroundingText(
> +    text, beforeLength, afterLength) {
> +    let before = 0;
> +    let after = 0;
> +    if (typeof beforeLength !== 'number') {

You want (typeof beforeLength == 'number'), right?

@@ +149,5 @@
> +    let after = 0;
> +    if (typeof beforeLength !== 'number') {
> +      before = beforeLength;
> +    }
> +    if (typeof afterLength !== 'number') {

same here.

::: b2g/components/b2g.idl
@@ +71,5 @@
> +   * beginning of the current selection range. Defaults to 0.
> +   * @param afterLength The number of characters to be deleted after the
> +   * beginning of the current selection range. Defaults to 0.
> +   */
> +  void replaceSurroundingText(in DOMString text, [optional] in long beforeLength, 

Not: trailing whitespace

Comment 9

[David Flanagan [:djf]]

Adding bug 797170 to the list of bugs this blocks. Auto-correction is painful with the current API where we have to send backspaces and individual characters.

Are you still working on the Yuan, or have you been pulled off to higher-priority bugs?

Comment 10

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

Yes, I'm still working on it. 

The bug has been pulled off as we are still discussing the kebyoard API in the webapi mail list. There will be some changes to replaceSurroundingText method after we get a consensus on the keyboard API. 

Should we have the bug land now for a temporary workaround or wait for the kebyoard API?

Comment 11

[Walter Chen[:ypwalter][:wachen]]

Any news on this bug?

Comment 12

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

Still discussing the API, but I would like to fix it in advance and create a new bug to make it fit for the final API.

Comment 13

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

Attachment: replaceSurroundingText of mozKeyboard

Nit and fix all as comment 8 mentioned:

1. Make each sentence of the comment end with a '.'
2. Leave MozKeyboard.ini unchanged.
3. Fix `typeof beforeLength == 'number'`

Note: we don't need to create and hold nsIPlaintextEditor any more as it is already created by other patches.

Fabrice, thanks in advance :-)

Comment 15

[[:fabrice] Fabrice Desré]

Comment on attachment 754736 [details] [diff] [review]
replaceSurroundingText of mozKeyboard

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

r=me with nits addressed

::: b2g/components/MozKeyboard.js
@@ +151,5 @@
> +      before = beforeLength;
> +    }
> +    if (typeof afterLength === 'number') {
> +      after = afterLength;
> +    }

That could be simplified to:
let before = typeof beforeLength === 'number' ? beforeLength : 0

and a similar one for after.

@@ +155,5 @@
> +    }
> +    cpmm.sendAsyncMessage('Keyboard:ReplaceSurroundingText', {
> +      'text': text || '',
> +      'beforeLength': before,
> +      'afterLength': after

you can even inline before and after here.

Comment 16

[[:fabrice] Fabrice Desré]

Comment on attachment 754736 [details] [diff] [review]
replaceSurroundingText of mozKeyboard

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

r=me with nits addressed

::: b2g/components/MozKeyboard.js
@@ +151,5 @@
> +      before = beforeLength;
> +    }
> +    if (typeof afterLength === 'number') {
> +      after = afterLength;
> +    }

That could be simplified to:
let before = typeof beforeLength === 'number' ? beforeLength : 0

and a similar one for after.

@@ +155,5 @@
> +    }
> +    cpmm.sendAsyncMessage('Keyboard:ReplaceSurroundingText', {
> +      'text': text || '',
> +      'beforeLength': before,
> +      'afterLength': after

you can even inline before and after here.

Comment 17

[David Flanagan [:djf]]

\o/

We have tentative agreement in bug 873934 to uplift keyboard auto-correction to v1-train (assuming I can finish the feature and that QA does not turn up major issues).  

If we could uplift this patch in addition, we'd have keyboard nirvana! The fact that autocorrections are visibly slow is probably the biggest current problem with them.

Comment 18

[Andreas Gal :gal]

The correction might be difficult to fix. We have to send individual key events to the page since those events are expected to fire, which means we are sending a bunch of backspace followed by new characters, and we probably reflow in between. We might need to postpone that reflow if we can.

Comment 19

[Andreas Gal :gal]

To make matters worse we actually send 3 events for each key:

    send("keydown");
    send("keypress");
    send("keyup");

Comment 20

[Andreas Gal :gal]

What ensures that we still fire onkey* event listeners if the keyboard uses this?

Comment 21

[David Flanagan [:djf]]

I was assuming that to any scripts paying attention to events would see an autocorrect as something like a paste. I think that generates an input event but no key events, or something.  I wonder what Android does?

I've also filed 878387 as a step in the right direction if we can't use this patch for auto-corrections: at least it minimizes the number of events.

Andreas: per comment 18, how would we postpone reflow?  Any ideas?

Comment 22

[Andreas Gal :gal]

djf, you are right! We can do a single paste event after a single cut event. I have no objects to that. Andreas

Comment 23

[Wayne Chang [:wchang]]

Triage - adding QE2 milestone to match bug 873934  and for partner to decide if this should be taken on 1.1.

Comment 24

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

Attachment: replaceSurroundingText of mozKeyboard

r=fabrice.
fix nits as Comment 16 mentioned to inline arguments.

Comment 25

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

FYI, the current patch will generate input events instead of key events.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/5b571c577bdc

Comment 27

[Andreas Gal :gal]

Yuan, please try to come up with a test for this

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Jan Jongboom [:janjongboom] (inactive)]

Great work guys! We've already incorporated it in the autocorrect in Gaia. See bug 878387.

Comment 30

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

(In reply to Andreas Gal :gal from comment #27)
> Yuan, please try to come up with a test for this

I have no idea about how to write a unit test for this, as the test will interact between the two apps(the keyboard app and the user app receiving input). 

@djf do you have any idea?

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Needs a branch-specific patch for b2g18 uplift.

Comment 32

[Tony Chung [:tchung]]

Please add a testcase for this bug to moztrap for 1.1 testsuite.  If yes, mark this in-moztrap+ when completed.  If not, mark this in-moztrap-.

Comment 33

[David Flanagan [:djf]]

Yuan: are you able to create the branch-specific patch so this can be uplifted?

About testing... I don't understand our testing frameworks very well. We could add a case to the UITests app, that at least displays the input events received when the user does auto-correction.

The automation team is able to run some tests automatically. Maybe they could write a test that would perform a simple auto-correction and verify that the right event was delivered to the text field?  I think they're using marionette for that?

Comment 34

[:CSchmoeckel]

Modifying text with the keyboard and the speed of the process is covered in Keyboard Suite Test Case #8452 - [Keyboard] The Auto Correct feature replaces characters in a word even when a word is spelled correctly

Comment 35

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

Attachment: branch-specific patch for b2g18

This patch is specified for b2g18 branch. It added the methods pertinent to `nsIPlaintextEditor` and `setSelectionRange` which are required but didn't exists in b2g18 branch.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/185f30abad64

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/185f30abad64

Comment 38

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

Attachment: Gaia patch

Thanks for your guys' help!

We fixed gecko, but Gaia v1-train exists a tiny bug after this bug was fixed on b2g18 branch.

David, I get a |variable not defined| error, and Gaia v1-train could be fixed by this patch.

I don't know if I need file a new bug for gaia or you will do it.

Comment 39

[David Flanagan [:djf]]

This is the cause of bugs 882197 and 882327, and I'd just diagnosed it myself. I wish I'd looked at your patch here, first!  I'll fix this in those two bugs

Comment 40

[Jason Smith [:jsmith]]

(In reply to :CSchmoeckel from comment #34)
> Modifying text with the keyboard and the speed of the process is covered in
> Keyboard Suite Test Case #8452 - [Keyboard] The Auto Correct feature
> replaces characters in a word even when a word is spelled correctly

If there's already coverage here, then please + this. If coverage doesn't make sense here, then set it to -.

Comment 41

[Jeni]

Verified fixed on 

Build ID: 20130807071207
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/11bb1b0eefff
Gaia: 60ca81600a080dae33058b0692ecaa213556c926
Platform Version: 18.1