Closed Bug 860546 Opened 7 years ago Closed 6 years ago

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

Categories

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

x86
macOS
defect
Not set

Tracking

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

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

People

(Reporter: djf, Assigned: xyuan)

References

Details

Attachments

(6 files, 3 obsolete files)

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.
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.
blocking-b2g: --- → leo?
Keywords: qawanted
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.
Triage 4/12 - Leo+ as reproducible on my Leo device running 20130411070205.
blocking-b2g: leo? → leo+
Adding Jed, who is also working on the keyboard.
QA Contact: 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
Keywords: qawanted
Depends on: 861665
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!
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
After looking closely, seems like we lack a notification of text changed for IME, forms.js only deals with focused element.
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!
(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?
Flags: needinfo?(xyuan)
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.
Assignee: nobody → xyuan
Flags: needinfo?(xyuan)
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.
Attachment #738997 - Flags: review?(fabrice)
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.
Attachment #738997 - Flags: review?(fabrice) → review-
(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.
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.
Attachment #738997 - Attachment is obsolete: true
Attachment #742781 - Flags: review?(fabrice)
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.
Attachment #742781 - Flags: review?(fabrice) → review+
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.
Status: NEW → ASSIGNED
Flags: needinfo?
Fix a nit: missing full stop at the end of the comment.

Thanks for your help.
Attachment #742781 - Attachment is obsolete: true
Attachment #744976 - Flags: review?(fabrice)
Flags: needinfo?
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.
Attachment #744976 - Flags: review?(fabrice) → review+
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?
Keywords: checkin-needed
Don't need to reland it. Thanks.
https://hg.mozilla.org/mozilla-central/rev/5565fd265415
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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!
Attached video Flashing UI
The solution of this bug has these two side effects only on phone, but doesn't on b2g-desktop. I'll find the cause.
This will need a b2g18-specific patch for uplift.
William - I'd file new bugs for the issue you've found.
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.
Attachment #746499 - Flags: review?(fabrice)
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!
I filed Bug 871752 too that's maybe related.
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.
And the three bugs in the blocks field should be moved so that they're dependents of the new bug not this one.
Blocks: 871883
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.
No longer blocks: 869877, 869880, 871752, 871883
Blocks: 871883
Attachment #746499 - Attachment is obsolete: true
Attachment #746499 - Flags: review?(fabrice)
Depends on: 874754
Does something need doing here for b2g18 still? I don't see that comment 29 was ever addressed.
Flags: needinfo?(xyuan)
No, I'm sorry that I forgot about the b2g18-specific patch. It'll be ready soon.
Flags: needinfo?(xyuan)
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.
Attachment #762105 - Flags: review?(fabrice)
Attachment #762105 - Flags: review?(fabrice) → review+
Fabrice, thanks for your help.

Ryan, please don't uplift this patch to b2g18 and hold off for a while.
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.
Depends on: 811679
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!
It seems that this change caused a regression, Bug 889138.
Setting the dependency here.
Depends on: 889138
No longer depends on: 889138
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.
You need to log in before you can comment on or make changes to this bug.