Closed Bug 861515 Opened 7 years ago Closed 7 years ago

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

Categories

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

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:leo+, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18 verified, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

RESOLVED FIXED
1.1 QE2 (6jun)
blocking-b2g leo+
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
b2g18 --- verified
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: xyuan, Assigned: xyuan)

References

Details

Attachments

(3 files, 3 obsolete files)

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);
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?
Bug 835404 is the keyboard performance meta-bug.
Blocks: 835404
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.
(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. :-)
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.
Attachment #737205 - Flags: review?(dflanagan)
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
Attached patch replaceSurroundingText (obsolete) — Splinter Review
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.
Attachment #737205 - Attachment is obsolete: true
Attachment #737205 - Flags: review?(dflanagan)
Attachment #737259 - Flags: review?(fabrice)
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
Attachment #737259 - Flags: review?(fabrice) → review-
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?
Blocks: 797170
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?
Blocks: 873934
Any news on this bug?
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.
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 :-)
Attachment #754736 - Flags: review?(fabrice)
Duplicate of this bug: 876672
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 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.
Attachment #754736 - Flags: review?(fabrice) → review+
\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.
blocking-b2g: --- → leo?
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.
To make matters worse we actually send 3 events for each key:

    send("keydown");
    send("keypress");
    send("keyup");
What ensures that we still fire onkey* event listeners if the keyboard uses this?
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?
Flags: needinfo?(gal)
djf, you are right! We can do a single paste event after a single cut event. I have no objects to that. Andreas
Flags: needinfo?(gal)
Triage - adding QE2 milestone to match bug 873934  and for partner to decide if this should be taken on 1.1.
Target Milestone: --- → 1.1 QE2 (6jun)
r=fabrice.
fix nits as Comment 16 mentioned to inline arguments.
Attachment #737259 - Attachment is obsolete: true
Attachment #754736 - Attachment is obsolete: true
Attachment #757356 - Flags: review+
FYI, the current patch will generate input events instead of key events.
Yuan, please try to come up with a test for this
https://hg.mozilla.org/mozilla-central/rev/5b571c577bdc
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Great work guys! We've already incorporated it in the autocorrect in Gaia. See bug 878387.
(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?
blocking-b2g: leo? → leo+
Needs a branch-specific patch for b2g18 uplift.
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-.
Flags: in-moztrap?(cschmoeckel)
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?
Flags: needinfo?(xyuan)
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
Flags: in-moztrap?(cschmoeckel) → in-moztrap-
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.
Attachment #759673 - Flags: review?(fabrice)
Flags: needinfo?(xyuan)
Attachment #759673 - Flags: review?(fabrice) → review+
Attached patch Gaia patchSplinter Review
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.
Flags: needinfo?(dflanagan)
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
Flags: needinfo?(dflanagan)
(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 -.
Flags: in-moztrap- → in-moztrap+
Verified fixed on 

Build ID: 20130807071207
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/11bb1b0eefff
Gaia: 60ca81600a080dae33058b0692ecaa213556c926
Platform Version: 18.1
You need to log in before you can comment on or make changes to this bug.