860546 - [keyboard] JS changes to a textfield while keyboard is displayed do not get passed to keyboard

Status: RESOLVED FIXED

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

Description

[David Flanagan [:djf]]

When the user taps on a text field, magic stuff in b2g/chrome/content/forms.js passes the content of the text field and the cursor position to the gaia keyboard.  The keyboard sends key events that end up adding and removing characters from the text field. In order to do word suggestions, auto punctuation and auto correct, the keyboard has to keep its internal state in sync with what is actually displayed in the text field.

The current system works fine as long as the app does not use JS to change the contents of the text field while the keyboard is displayed.

Unfortunately, one of the building blocks that we use in apps like contacts and email adds a little circled X icon to input fields. Tapping on the X clears the text field. And it clears the text field without taking focus away from the field (I seem to recall a bug about that). Since focus doesn't leave the text field, the keyboard doesn't go away and come back, which means the keyboard is never notified of the new state of the input field and word suggestions, etc., are all messed up.

Steps to reproduce:
 - turn on word suggestions in the settings app

 - Open contacts app

 - click + to add a new contact.
 
 - tap in Name field

 - Type "th" and notice the suggestions

 - click the X button to clear the field

 - Type "i" and notice that the keyboard now suggests "This" because it thinks the text field contains the letters "Thi" even though it actually only contains "i".  Also notice that they keyboard did not capitalize the i because it does not think that it is the first character in the field.

I'm not sure if we can fix this in forms.js or not. We need to detect change events that are not caused by key events from the keyboard.

Comment 1

[David Flanagan [:djf]]

Cc'ing Tim and Margaret because they have both worked on forms.js and might be able to work on this.

Nominating this for Leo because it seems like something we ought to fix if we can.

Comment 2

[bhavana bajaj [:bajaj]]

Adding qawanted to get testeing around the scenario's for the word suggestions turned on . Basically doing a few basic checks if backspacing etc work fine and this is a problem only when a user is trying to " click the X button to clear the field" as mentioned in STR here.

Comment 3

[Wayne Chang [:wchang]]

Triage 4/12 - Leo+ as reproducible on my Leo device running 20130411070205.

Comment 4

[David Flanagan [:djf]]

Adding Jed, who is also working on the keyboard.

Comment 5

[William Hsu [:whsu]]

Hi, all,

As I reproduced, this case only happened on (X) button.
I used backspace button to delete the characters, the word suggestion works as usual.
By the wasy, doing the same test on message application, this case didn't happen.
Attach the screenshot later.

* Reproduction build: (Mozilla-b2g18_v1_0_1/2013-04-11-23-02-04)
  - Mercurial-Information
    + Gecko revision="ada03bf95314"
    + Gaia revision="c11d500cd9e5"
  - Git-Information:
    + Gecko revision="b0123a5e6f0e3f815c543d94e9f779ee53e78044"
    + Gaia revision="63890fe9845c13613e9de6017f742a28ea4ee24d"

Best Regards,
William

Comment 6

[William Hsu [:whsu]]

Attachment: Use (X) button to cancel characters

Comment 7

[William Hsu [:whsu]]

Attachment: Use (<-) backapace to delete characters

Comment 8

[William Hsu [:whsu]]

Sorry for the late information.
This might be a legacy issue since I reproduce it on v1.0.1.
I still didn't have Leo device. I will try to reproduce this case if I have.
Thanks!

Comment 9

[Shelly Lin [:shelly]]

Hi :yxl, your proposal in bug 861665 looks very good, but are we not notifying the keyboard when a text content is changed? Thanks.

http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/forms.js#328

Comment 10

[Shelly Lin [:shelly]]

After looking closely, seems like we lack a notification of text changed for IME, forms.js only deals with focused element.

Comment 11

[William Hsu [:whsu]]

Hi, all,

Just did a quick replication/verification.
This problem also happened on V1.1.0(leo).
So, it was a legacy issue.

* Test build:(Mozilla-b2g18-leo/2013-04-08-23-02-06)
  - Mercurial-Information:
    + Gecko revision="08de0253274c"
    + Gaia revision="453b0eee07f9"
  - Git-Information:
    + Gecko revision="8aa66dedde1a7b20159836b1d9250e826567d2d4"
    + Gaia revision="48e6e68da32cb725d5182206c7bbd3490dda05a8"

Thanks. Have a nice day!

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Shelly Lin [:shelly] from comment #9)
> Hi :yxl, your proposal in bug 861665 looks very good, but are we not
> notifying the keyboard when a text content is changed? Thanks.
> 
> http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/forms.js#328

This is bad. I thought 'input' event will be triggered when web content sets the value of the input.

yxl, is there any chrome-only event available to forms.js for monitoring such situation?

Comment 13

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

Yes, nsIEditorObserver can be used to monitoring all the content change events of a text field. I'm thinking of a workaround to fix this urgent bug before bug 861665 lands.

Comment 14

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

Attachment: Notify keyboard by focus change event when the text content is changed by external actions

To work around this bug, when the text field is changed by external actions, such as JS changing the text content, a focus change event will be sent to the keyboard.

The patch uses nsIEditorObserver to monitor edit actions applied to the text field and see FormAssistant.EditAction in forms.js for details.

In order to distinguish sendKey action from external actions, the patch moves the event generating function from MozKeyboard.js to forms.js.

Comment 15

[[:fabrice] Fabrice Desré]

Comment on attachment 738997 [details] [diff] [review]
Notify keyboard by focus change event when the text content is changed by external actions

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

I'd like to see a new version with comments addressed.

::: b2g/chrome/content/forms.js
@@ +256,5 @@
>    get documentEncoder() {
>      return this._documentEncoder;
>    },
>  
> +  // Implments nsIEditorObserver get notification when the text content of

Nit: Implements

@@ +422,5 @@
> +
> +      case "Forms:SendKey": {
> +        let keyCode = json.keyCode;
> +        let charCode = json.charCode;
> +        this.sendKey(keyCode, charCode);

please add break; there. also, no need to the local variables.

@@ +432,5 @@
> +  },
> +
> +  sendKey:function fa_sendKey(keyCode, charCode) {
> +    ['keydown', 'keypress', 'keyup'].forEach(function sendKey(type){
> +      domWindowUtils.sendKeyEvent(type, keyCode, charCode, null);

We had similar code in Keyboard.jsm and changed it to async dispatch to help with responsiveness. Please do the same here (see https://mxr.mozilla.org/mozilla-central/source/b2g/components/MozKeyboard.js#76)

::: b2g/components/MozKeyboard.js
@@ +73,5 @@
>      charCode = (charCode == undefined) ? keyCode : charCode;
> +    cpmm.sendAsyncMessage('Keyboard:SendKey', {
> +      'keyCode': keyCode,
> +      'charCode': charCode
> +    });

I'm a bit worried about the performance impact here. We go from MozKeyboard.js -> Keyboard.jsm -> Forms.js
Please do some perf measurement since it's critical to not slow down the keyboard.

Comment 16

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

(In reply to Fabrice Desré [:fabrice] from comment #15)
> ...
> I'm a bit worried about the performance impact here. We go from
> MozKeyboard.js -> Keyboard.jsm -> Forms.js
> Please do some perf measurement since it's critical to not slow down the
> keyboard.

I made a simple test with the Email and Contacts app. The performance of SendKey will be severely degraded(it consumes morn than 20 times of time), but the degradation does not result from sending message. For an out-of-process app, the time of exchanging asynchronous messages with system app is negligible, while the time of synthesizing key events by nsIDOMWindowUtils.sendKeyEvent in the app is 20 times more than that in system app. I don't know the reason. 

In order not to impact the performance, I need to find other method to resolve the SendKey issue.

Comment 17

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

Attachment: Notify keyboard when the text of current input field is changed by JS

If the content of current input field is changed directly by JS, the patch will fire a focus change event to notify the keyboard through mozKeyboard API. 

But it will not cover the case that the input field is changed by key events not sending from keyboard.

Comment 18

[[:fabrice] Fabrice Desré]

Comment on attachment 742781 [details] [diff] [review]
Notify keyboard when the text of current input field is changed by JS

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

r=me

Yuan, can you file a followup to investigate why nsIDOMWindowUtils.sendKeyEvent is so much slower in content processes? thanks!

::: b2g/chrome/content/forms.js
@@ +358,2 @@
>          // We use 'setTimeout' to wait until the input element accomplishes the
> +        // change in selection range or text content

Nit: missing full stop at the end of the comment.

Comment 19

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

Bug 868292(https://bugzilla.mozilla.org/show_bug.cgi?id=868292) was filed to investigate why nsIDOMWindowUtils.sendKeyEvent is so much slower in content processes.

Comment 20

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

Attachment: Notify keyboard when the text of current input field is changed by JS

Fix a nit: missing full stop at the end of the comment.

Thanks for your help.

Comment 21

[[:fabrice] Fabrice Desré]

Comment on attachment 744976 [details] [diff] [review]
Notify keyboard when the text of current input field is changed by JS

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

Thanks! you don't even have to ask for review when just fixing nits.

Comment 22

[Jacek Caban]

I landed this, but the hg import couldn't handle From header, so the author name is broken in the commit:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5565fd265415

Should I reland it?

Comment 23

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

Don't need to reland it. Thanks.

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5565fd265415

Comment 25

[William Hsu [:whsu]]

Hi, all,

Thanks your help on this case.
I verified this case on the latest build. The solution fix the problem.
* Mozilla-central-unagi/20130506031043
  + Mercurial-Information
    - Gecko revision="b109e2dbf03b"
    - Gaia revision=""
  + Git-information
    - Gecko revision="61322a468cf3b98bb01bf58843ae47754f747f50"
    - Gaia revision="5b50627a6da022258593cecc05dd8e0302f93a6f"

But, there had two different behaviors between new and old build.
I think these behaviors/bugs are side effect.

1) UI still shows other suggestion words after you chose a correct one.
   Please refer to the attached video -"Incorrect_Word_Suggestion.3gp"
2) The UI of word suggestion field flashed abnormally after you type a word.
   This may be a minor bug but we still need to know why it flashes.
   Please refer to the attached video -"Flashing_UI.3gp"

If you don't think these are side effects, I will create other cases to trace it.
Thanks!

Comment 26

[William Hsu [:whsu]]

Attachment: Incorrect word suggestion

Comment 27

[William Hsu [:whsu]]

Attachment: Flashing UI

Comment 28

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

The solution of this bug has these two side effects only on phone, but doesn't on b2g-desktop. I'll find the cause.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

This will need a b2g18-specific patch for uplift.

Comment 30

[Jason Smith [:jsmith]]

William - I'd file new bugs for the issue you've found.

Comment 31

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

Attachment: Ignore content changes caused by key events

The side effects of flashing UI and incorrect work suggestion are caused by the focus change events sending after keydown events. 

On the phone, the focus change event will be sent every time after the keydown event. 

To resolve the issue, the patch ensures that the focus change event won't be sent if the input field content is changed by keydown event.

Comment 32

[William Hsu [:whsu]]

Hi, Jason,

Thanks for your reminder.
I submit other bugs to trace side effects.
Bug 869877 [B2G][Mozilla-central][Keyboard] UI still shows other suggested words after you chose a correct one.
Bug 869880 [B2G][Mozilla-central][Keyboard] The UI of word suggestion field flashed abnormally after you type a word.
Thanks!

Comment 33

[Julien Wajsberg [:julienw]]

I filed Bug 871752 too that's maybe related.

Comment 34

[[:fabrice] Fabrice Desré]

Yuan, can you move the last patch to a new bug? This one is closed, and this is why it was not in my review queue at all.

Comment 35

[David Flanagan [:djf]]

And the three bugs in the blocks field should be moved so that they're dependents of the new bug not this one.

Comment 36

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

Fabrice and David, thanks for reminding.

Bug 871883 was filed to track the side effect and moved the dependency of the three related bugs to Bug 871883 as well.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Does something need doing here for b2g18 still? I don't see that comment 29 was ever addressed.

Comment 38

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

No, I'm sorry that I forgot about the b2g18-specific patch. It'll be ready soon.

Comment 39

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

Attachment: b2g18-specific patch

b2g18-specific patch for uplift.
This patch fixed the side effect addressed in the follow up Bug 871883 as well, so we don't need to uplift the patch of Bug 871883 after this patch applied.

Comment 40

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

Fabrice, thanks for your help.

Ryan, please don't uplift this patch to b2g18 and hold off for a while.

Comment 41

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

This bug depends on Bug 811679(Add nsIEditorObserver back), which has been fixed on geck 19, but not b2g18. We need to fix the Bug 811679 before uplift.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/0b9d507dc9d6

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/0b9d507dc9d6

Comment 44

[William Hsu [:whsu]]

just kindly remind you that I verified comment 32 mentioned bugs.(Bug 869877 and Bug 869880) 
I cannot reproduce these bugs with latest v1-train build.
Test build:
* Mozilla-b2g18-unagi-eng/2013-06-20-23-02-08
  + Mercurial-Information:
    - Gecko revision: "a34f6d62cb05"
    - Gaia revision:
  + Git-information
    - Gecko revision: "243a03062a9e6cfa2e315298ed5b45352ae829e8"
    - Gaia revision: "de211a86e1df621415942e8b02acea77efafd864"

Thanks for your help!

Comment 45

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

It seems that this change caused a regression, Bug 889138.
Setting the dependency here.

Comment 46

[dkumar]

Varified,fixed on LEO 1.1 Mozilla RIL
Environmental Variables
Build ID: 20130716070204
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/629020cf576b
Gaia: fb9362d34260771d4a00b9a0e10a6bbad397bd3b
Platform Version: 18.1
RIL Version: 01.01.00.019.158 

Varified that typing "i" suggests "I" "In" "Is" Also noticed "i"is capitalized and "I" is considered as the first character in the field.