Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Description • 10 years ago

STR:

1. Go to any app, say Contact -> New Contact, and focus on an input field, e.g. Phone number.
2. Tap on the Rocket bar ("Search the web") to switch the focus to the rocket bar.
3. Type something

Expected:

1. The input caret moved to rocket bar, and the layout switch to Search w/o auto suggestions.

Actual:

1. Sometimes the caret remained in the Contact app frame, sometimes the caret does moved to search field but the layout is not switched, sometimes everything visually correct but you can't type anything.

Note:

I suspect this is a regression of bug 1057898, bug 1067264, and bug 1067266, but it's worthy to check if InputMethod API is previously shown this behavior already. qawanted for v2.1 check.

I have a patch (see bug 1077124 comment 11) that would bring back the race protection in the keyboard app (the delay removed in bug 1067264). I will file a bug blocks this one to land it.

We can decide whether or not we need to revert the behavior of input mgmt later, as the visual effect (have the keyboard start slide away in midway and slide back instantly) can be fixed in CSS with bug 1075306.

I would still like to keep bug 1057898 in the tree for switching inputs between the same frame.

	Cody Roesch [:croesch]
	Comment 1 • 10 years ago

Tim, I think i'm seeing your bug here in Flame 2.1 KK. I've not experienced all your different scenarios but i often get the keyboard to not show up when I switch between the phone field and the rocketbar. The carat is visible in the rocketbar but no keyboard pops up so obviously I can't type. I can get this frequently if the phone field is tapped then the rocketbar is tapped shortly after as the numerical keyboard is popping up.

I tried this in other apps and found the same behavior in Messages app. Tapping in the contact field then into rocketbar produced this.

***Let me know if you think this is not your bug. Hard to tell since you describe several bahaviors.

Repro Rate: 5/10

Environmental Variables:
Device: Flame 2.1 KK
BuildID: 20141015143144
Gaia: 477a9e61c3edf12f32a62a19d329cd277202cc6b
Gecko: 67573e422a0f
Version: 34.0 (2.1) 
Firmware Version: L1TC10011800
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 2 • 10 years ago

(In reply to Cody Roesch [:croesch] from comment #1)
> ***Let me know if you think this is not your bug. Hard to tell since you
> describe several bahaviors.
> 
> Repro Rate: 5/10

Thank you. That is indeed this bug is about. So this is not a regression to the mentioned bug after all but a defect existed in InputMethod API.

Let me try to isolate the cause in API after finishing bug 1083638.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 3 • 10 years ago

The same STR can result two different results and this patch address one of them. The Rockbar search field comes is placed in System app (inproc), and when the focus moves from the focused element on remote process to there, the blur message from remote process might sent after the focus message from chrome process.

This patch address the bug where the actual result is "keyboard non popping up for search field".

Unfortunately I am not sure how to write a mochitest for this -- I have a test script ready that could switch focus between two inputs in two frames, but I can't seem to be able to push the two mozbrowser frames to remote.

The second result of this bug is, for some reason, when the rocketbar overlay is shown, instead of focus moving from the app to rocketbar, the focus was moved back to the frame. This can be due to a Gaia bug and I would need to dig deeper.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 4 • 10 years ago

Comment on attachment 8507651 [details] [diff] [review]
Patch v1.0

This patch does not work for the situation I described. Need to look deeper.

We probably need to land bug 1083638 first coz keyboard app itself is a bit flicky too.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 5 • 10 years ago

I try to find the root cause by annotating forms.js and update omni.ja, however for some reason the forms.js in chrome b2g process is loaded from somewhere else and I can't update it.

Will try to build the entire Gecko Monday :'(.

	[:fabrice] Fabrice Desré
	Comment 6 • 10 years ago

Hm, there's a single forms.js in gecko Tim. So something else is going on, but repackaging omni.ja should really work.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 7 • 10 years ago

Update on the alternative approach: try to reproduce this bug with mochitest.

So, I modified inputmothod_common.js and added the necessary perfs to push iframes to remote, but the tests timed out. Further annotating forms.js found that it is indeed being loaded but it does not receive any focus event. It might be related to bug 1085217 but neither Kanru and I can sure.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 8 • 10 years ago

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #5)
> Will try to build the entire Gecko Monday :'(.

Here is the log from built Gecko, when the bug happens:

Tap on the Phone # field in Contacts:

I/Gecko   ( 9891 contacts): XXXXX 407 blur event
I/Gecko   ( 9891 contacts): XXXXX 380 focus event
I/Gecko   ( 9891 contacts): XXXXX 675 fa_handleFocus
I/Gecko   ( 9891 contacts): XXXXX 681 fa_handleFocus
I/Gecko   ( 9891 contacts): XXXXX 685 fa_handleFocusSync
I/Gecko   ( 9891 contacts): XXXXX 690 focus true
I/Gecko   ( 9569 keyboard): XXXXX MozKeyboard.js inputcontext {"contextId":1,"type":"tel","choices":null,"value":"","inputmode":"","selectionStart":0,"selectionEnd":0,"max":"","min":"","lang":"","textBeforeCursor":"","textAfterCursor":""}

Tap on Rocket bar:

I/Gecko   ( 9235 system  ): XXXXX 407 blur event
I/Gecko   ( 9235 system  ): XXXXX 380 focus event
I/Gecko   ( 9235 system  ): XXXXX 675 fa_handleFocus
I/Gecko   ( 9235 system  ): XXXXX 681 fa_handleFocus
I/Gecko   ( 9891 contacts): XXXXX 407 blur event
I/Gecko   ( 9891 contacts): XXXXX 408 blur event
I/Gecko   ( 9891 contacts): XXXXX 696 fa_unhandleFocus
I/Gecko   ( 9891 contacts): XXXXX 407 blur event
I/Gecko   ( 9891 contacts): XXXXX 407 blur event
I/Gecko   ( 9235 system  ): XXXXX 685 fa_handleFocusSync
I/Gecko   ( 9235 system  ): XXXXX 690 focus true
I/Gecko   ( 9569 keyboard): XXXXX MozKeyboard.js inputcontext {"contextId":3,"type":"search","choices":null,"value":"","inputmode":"verbatim","selectionStart":0,"selectionEnd":0,"max":"","min":"","lang":"","textBeforeCursor":"","textAfterCursor":""}
I/Gecko   ( 9891 contacts): XXXXX 703 fa_unhandleFocusSync
I/Gecko   ( 9891 contacts): XXXXX 710 blur
I/Gecko   ( 9569 keyboard): XXXXX MozKeyboard.js inputcontext null


This indeed confirms comment 3 but I don't know why my patch doesn't work. Will look deeper tomorrow.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 9 • 10 years ago

Here is a more detailed log and annotation, which confirms comment 3 is indeed correct but we have the same race in System app input management as well:

When the focus changes, focus message reaches the keyboard sooner than the blur message from oop frame:

I/Gecko   ( 2058): XXXXX 407 blur event
I/Gecko   ( 2058): XXXXX 380 focus event
I/Gecko   ( 2622): XXXXX 407 blur event
I/Gecko   ( 2622): XXXXX 408 blur event
I/Gecko   ( 2622): XXXXX 696 fa_unhandleFocus
I/Gecko   ( 2058): XXXXX 675 fa_handleFocus
I/Gecko   ( 2058): XXXXX 681 fa_handleFocus
I/Gecko   ( 2622): XXXXX 407 blur event
I/Gecko   ( 2622): XXXXX 407 blur event
I/Gecko   ( 2058): XXXXX 685 fa_handleFocusSync
I/Gecko   ( 2622): XXXXX 703 fa_unhandleFocusSync
I/Gecko   ( 2058): XXXXX 690 focus true
I/Gecko   ( 2622): XXXXX 710 blur
I/Gecko   ( 2058): XXXXX 252 forward event Keyboard:FocusChange{"target":{},"name":"Forms:Input","sync":false,"json":{"contextId":7,"type":"search","choices":null,"value":"","inputmode":"verbatim","selectionStart":0,"selectionEnd":0,"max":"","min":"","lang":"","textBeforeCursor":"","textAfterCursor":""},"data":{"contextId":7,"type":"search","choices":null,"value":"","inputmode":"verbatim","selectionStart":0,"selectionEnd":0,"max":"","min":"","lang":"","textBeforeCursor":"","textAfterCursor":""},"objects":{}}
I/Gecko   ( 2058): XXXXX 267 send
I/Gecko   ( 2387): XXXXX MozKeyboard 225
I/Gecko   ( 2387): XXXXX MozKeyboard.js inputcontext {"contextId":7,"type":"search","choices":null,"value":"","inputmode":"verbatim","selectionStart":0,"selectionEnd":0,"max":"","min":"","lang":"","textBeforeCursor":"","textAfterCursor":""}
I/Gecko   ( 2058): XXXXX 869 browser elememt true
I/Gecko   ( 2387): XXXXX 1123 browser element child true
I/Gecko   ( 2058): XXXXX 252 forward event Keyboard:FocusChange{"target":{},"name":"Forms:Input","sync":false,"json":{"type":"blur"},"data":{"type":"blur"},"objects":{}}
I/Gecko   ( 2058): XXXXX 260 skip

... and the patch in comment 3 will work and block the blur message

I/Gecko   ( 2058): XXXXX 252 forward event Keyboard:SelectionChange{"target":{},"name":"Forms:SelectionChange","sync":false,"json":{"selectionStart":0,"selectionEnd":0,"textBeforeCursor":"","textAfterCursor":"","changed":true},"data":{"selectionStart":0,"selectionEnd":0,"textBeforeCursor":"","textAfterCursor":"","changed":true},"objects":{}}
I/Gecko   ( 2058): XXXXX 267 send
I/Gecko   ( 2058): XXXXX 252 forward event Keyboard:GetText:Result:OK{"target":{},"name":"Forms:GetText:Result:OK","sync":false,"json":{"requestId":"id{7d4657eb-e83c-4435-88c8-b7b1dd8f4a7d}","text":""},"data":{"requestId":"id{7d4657eb-e83c-4435-88c8-b7b1dd8f4a7d}","text":""},"objects":{}}
I/Gecko   ( 2058): XXXXX 267 send
I/Gecko   ( 2058): XXXXX 252 forward event Keyboard:GetText:Result:OK{"target":{},"name":"Forms:GetText:Result:OK","sync":false,"json":{"requestId":"id{3f1e9262-1230-4c23-b558-56fe59b25d1b}","text":""},"data":{"requestId":"id{3f1e9262-1230-4c23-b558-56fe59b25d1b}","text":""},"objects":{}}
I/Gecko   ( 2058): XXXXX 267 send

... however, we'll receive an setInputMethodActive(false) from input mgmt.

I/Gecko   ( 2058): XXXXX 869 browser elememt false
I/Gecko   ( 2387): XXXXX 1123 browser element child false
I/Gecko   ( 2387): XXXXX MozKeyboard 327
I/Gecko   ( 2387): XXXXX MozKeyboard.js inputcontext null

I will clone a bug and fix this on the input management side.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 10 • 10 years ago

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #9)
> I will clone a bug and fix this on the input management side.

bug 1090034.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 11 • 10 years ago

Comment on attachment 8507651 [details] [diff] [review]
Patch v1.0

I decide to set this for review.

-- I can't write any test with mochitest because of the problem documented in bug 1090032
-- We can't manually verify the fix unless input mgmt is also fixed, in bug 1090034

Let's attack the problem one at the time.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4e2193516ce8

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 12 • 10 years ago

After re-read the code again I realize my judgement in comment 9 is not correct. My patch did not stop chrome event from being sent out to input mgmt which caused input mgmt to call setInputMethodActive(false).

This is the correct the patch. However the reason for me not to write a mochitest main valid.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 13 • 10 years ago

Try run of patch v1.1
 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=917828d454ac

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 15 • 10 years ago

[Blocking Requested - why for this release]: bug 1087556 was nominated.

	Yuan Xulei [:yxl] (Leave from May 18)
	Comment 16 • 10 years ago

Comment on attachment 8512454 [details] [diff] [review]
Patch v1.1

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

::: dom/inputmethod/Keyboard.jsm
@@ +248,5 @@
>      }
>    },
>  
>    forwardEvent: function keyboardForwardEvent(newEventName, msg) {
> +    var mm = msg.target.QueryInterface(Ci.nsIFrameLoaderOwner)

nit: use |let| instead of |var|

@@ +273,5 @@
>  
>    handleFocusChange: function keyboardHandleFocusChange(msg) {
> +    var sent = this.forwardEvent('Keyboard:FocusChange', msg);
> +
> +    if (!sent) {

nit: The |sent| variable is not necessary and you can combine the above two lines into one.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 17 • 10 years ago

(In reply to Yuan Xulei [:yxl] from comment #16)
> @@ +273,5 @@
> >  
> >    handleFocusChange: function keyboardHandleFocusChange(msg) {
> > +    var sent = this.forwardEvent('Keyboard:FocusChange', msg);
> > +
> > +    if (!sent) {
> 
> nit: The |sent| variable is not necessary and you can combine the above two
> lines into one.

I think this will reduce the readability of the code, I will keep this and simply do s/var/let/. Is that good to you?

	Yuan Xulei [:yxl] (Leave from May 18)
	Comment 19 • 10 years ago

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #17)
> (In reply to Yuan Xulei [:yxl] from comment #16)
> > @@ +273,5 @@
> > >  
> > >    handleFocusChange: function keyboardHandleFocusChange(msg) {
> > > +    var sent = this.forwardEvent('Keyboard:FocusChange', msg);
> > > +
> > > +    if (!sent) {
> > 
> > nit: The |sent| variable is not necessary and you can combine the above two
> > lines into one.
> 
> I think this will reduce the readability of the code, I will keep this and
> simply do s/var/let/. Is that good to you?
It's fine for me. Personally speaking, I don't see any readability issue without |sent| variable :)

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/52cdf66f6666

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/52cdf66f6666

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 22 • 10 years ago

Will nominate for uplift after seeing the patch on daily OTA.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 23 • 10 years ago

Comment on attachment 8513269 [details] [diff] [review]
Patch for commit

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This is an old issue since the creating of the API, but commonly trigger-able since the introduction of the rocket bar.
User impact if declined: The STR will fail, creating confusing and annoy user experience.
Testing completed: tested manually on m-c and locally
Risk to taking this patch (and alternatives if risky): There is no alternative. This is the minimal patch possible to workaround the bug.
String or UUID changes made by this patch: None.

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 10 years ago

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f848ec429805

	Yeojin Chung [:YeojinC]
	Comment 25 • 10 years ago

Tim, I was trying to verify the fix for this bug, but found another issue. After STR #2 on Comment 0, the keyboard did not pop up until the user taps the rocket bar. I'm not sure if this is a separate or related issue. This only happens on Flame 2.1, and once the user taps the rocket bar, the keyboard is fully functional. 

Here's the video:
http://youtu.be/ptDfAp--v-Y

Please let me know if this issue prevents verifying the original bug. Thanks!

Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141103001220
Gaia: 027a7de0c95320cea0579bfd1a4ceef3e9038f34
Gecko: ffecb2be228b
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1) 
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 26 • 10 years ago

Yeojin,

That is not related to this bug I think. I don't think it would always happen either -- you should be able to verify this bug.

	Yeojin Chung [:YeojinC]
	Comment 27 • 10 years ago

This issue is verified fixed on Flame 2.1.

Result: The keyboard is displayed and fully functional when switched from an app to rocket bar.

Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141103001220
Gaia: 027a7de0c95320cea0579bfd1a4ceef3e9038f34
Gecko: ffecb2be228b
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1) 
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
=========================================

This issue still reproduces on Flame 2.2.

STR:
1. Open Contacts > Add contact.
2. Tap on the "Phone" field, and type a few numbers.
3. Tap on the rocket bar.

Result: The keyboard does not show up, even after the user taps the search field.
Repro rate: 3/5

Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141104040207
Gaia: 3c50520982560ccba301474d1ac43706138fc851
Gecko: 54d05732f29b
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 36.0a1 (2.2) 
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

	Yeojin Chung [:YeojinC]
	Comment 28 • 10 years ago

(In reply to Yeojin Chung [:YeojinC] from comment #27)

> This issue still reproduces on Flame 2.2.
> 
> STR:
> 1. Open Contacts > Add contact.
> 2. Tap on the "Phone" field, and type a few numbers.
> 3. Tap on the rocket bar.
> 
> Result: The keyboard does not show up, even after the user taps the search
> field.
> Repro rate: 3/5
> 
> Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
> BuildID: 20141104040207
> Gaia: 3c50520982560ccba301474d1ac43706138fc851
> Gecko: 54d05732f29b
> Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
> Version: 36.0a1 (2.2) 
> Firmware Version: v188
> User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Filed a new bug regarding the issue on Flame 2.2. Bug 1097936