1077124 - [Contacts] Number pad keyboard remains visible when switching from phone number field to name field

Status: VERIFIED FIXED

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

Description

[Brogan Zumwalt [Inactive]]

Attachment: Screenshot

Description:
While creating or editing contact tapping phone number field, then tapping any alphanumeric field (e.g. Name, Carrier, Email, etc.) the number pad keyboard remains on screen and never switches to expected QWERTY keyboard. Numpad is interactive and will enter characters into selected field. Tapping a second alphanumberic field brings up the QWERTY keyboard. Likewise, moving from an alphanumeric field to a number only field works as expected.
   
Repro Steps:
1) Update a Flame device to BuildID: 20141002093155
2) Launch Contacts app
3) Tap "+" icon to create new contact
4) Select phone number field
5) Select name field
  
Actual:
Number pad keyboard remains visible and QWERTY keyboard does not appear.
  
Expected: 
QWERTY keyboard replaces number pad.
  
Environmental Variables:
Device: Flame 2.2 Master KK (319mb) (Full Flash)
BuildID: 20141002093155
Gaia: 191d805f4911628d37a8a90a1e23a6013995138f
Gecko: 5d6ec4dddf14
Gonk: 2c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0
  
Repro frequency: 14/15, 93%
See attached: Screenshot & logcat

Comment 1

[Brogan Zumwalt [Inactive]]

Issue does NOT occur on Flame 2.1 KK (319mb) (Full Flash)

Flame 2.1 KitKat Base (319mb)(Full Flash)
Device: Flame 2.1
BuildID: 20141002000202
Gaia: 94dcc25f2e34a4900ea58310c26be52bcb089161
Gecko: baaa0c3ab8fd
Gonk: 2c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Actual Results: QWERTY keyboard replaces number pad.

Comment 2

[Brogan Zumwalt [Inactive]]

Attachment: Logcat

Comment 3

[Francisco Jordano [:arcturus] [:francisco]]

Hi Tim,

does this sound familiar to you?

Thanks!

Comment 4

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

Should be a recent regression. (keeping needinfo)

Comment 5

[Peter Bylenga [:PBylenga]]

requesting a window.

Comment 6

[Pi Wei Cheng [:piwei] (inactive)]

mozilla-inbound regression window:

Last Working Environmental Variables:
Device: Flame
BuildID: 20140926213838
Gaia: 2834baf4c7e34fe6ef335f0469f6d0f593c5922b
Gecko: 48311169d140
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

First Broken Environmental Variables:
Device: Flame
BuildID: 20140926233938
Gaia: 2834baf4c7e34fe6ef335f0469f6d0f593c5922b
Gecko: c84cee4a85b6
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Gaia is the same so it's a Gecko issue.

Gecko pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=48311169d140&tochange=c84cee4a85b6

There's only one bug in the pushlog which is Bug 1062323, though we're unsure what that commit is about. I've triple checked the window and it's the correct window.

Comment 7

[Joshua Mitchell (Inactive)]

Nikhil - this window only has a single item in the pushlog which was authored by you, can you take a look?

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

The code seems to rely on Promise resolution ordering being a certain way, and Bug 1062323 fixed that to be spec compliant. The logcat output also says something about Promise. Somebody who knows the keyboard code should look at the Promise ordering used since that may be causing the keyboard to not change.

Comment 9

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

We are indeed using promise to load assets required to switch layouts in Keyboard code. I will look deeper to see what is involved.

Comment 10

[Mozilla Shepherd (This account is used for automation and doesn't receive bugmail)]

Attachment: mozilla-b2g:master PR#25071

Comment 11

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

Comment on attachment 8503910 [details] [review]
mozilla-b2g:master PR#25071

It's not desirable to drop this amount of code in a regression bug, but I failed to find a band-aid solution since it's hard to identify what changed in the promise sequence that resulted this bug. 

Therefore I rolled out the complete solution here by ensuring nothing is run in parallel. I also removed other checks in place.

:mnjul, I am not sure this is the best way to implement a check to interrupt a queue. If you have better idea please share. I was also thinking about generalize the queue into a smaller module; but I ended up didn't do it.

Comment 12

[John Lu [:mnjul][Please NI or I'll miss]]

Comment on attachment 8503910 [details] [review]
mozilla-b2g:master PR#25071

The architecture of sequentializing those promises looks good enough, and it feels better that the sequentiality is now centrally controlled. It does appear that this lockstep-like mechanism is needed since we don't have any readily available interrupt construct in JS and/or API.

Please see github comments for some nits.

I think we might be able to abstract the p.then(actionIdCheck).then(someAction).then(actionIdCheck).then(someAction) such that manual insertion of .then(actionIdCheck) would not be required.

Side note: we always get an error message of the actionID-mismatch rejection when we try the STR of this bug, which is perfectly normal use case (so I don't think the error messages should be "error"). Is there anyway we can tweak the verbosity?

Comment 13

[John Lu [:mnjul][Please NI or I'll miss]]

(In reply to John Lu [:mnjul] [MoCoTPE] from comment #12)
> I think we might be able to abstract the
> p.then(actionIdCheck).then(someAction).then(actionIdCheck).then(someAction)
> such that manual insertion of .then(actionIdCheck) would not be required.

In a follow-up bug, for sure.

Comment 14

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

Attachment: debug-log

debug log from keyboard app

Comment 15

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

Comment on attachment 8503937 [details]
debug-log

I did some investigation into this issue, and I think this was caused by the following sequence,

 0. InputMethodManager.updateInputContextData would be called first, so this._inputContextData would hold the promise for getText().

 1. InputMethodManager.switchCurrentIMEngine() to default.
    after this call, this._inputContextData would be set to null.

 2. InputMethodManager.switchCurrentIMEngine() to latin.
    since this._inputContextData was set to null, cannot get the state (returned from inputContext.getText()) to do ime.activate().

Comment 16

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

Attachment: Patch V1

FWIW, I have a WIP patch that could avoid this issue, so please take it as an alternative solution if you think this makes sense.

Comment 17

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

Comment on attachment 8503941 [details] [review]
Patch V1

Your patch is better. Let's land it here in this bug and move my patch to a follow-up. Thanks for identifying the root cause.

Comment 18

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

Change the assignee.

Comment 19

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

Comment on attachment 8503941 [details] [review]
Patch V1

Patch updated to include the modified test case.

Tim, please help review it.
Thanks.

Comment 20

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

Comment on attachment 8503941 [details] [review]
Patch V1

Discussed offline.

Comment 21

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

Comment on attachment 8503941 [details] [review]
Patch V1

Patch updated so we don't check 'this._inputContextData' in InputMethodManager.prototype.updateInputContextData().

Tim, could you please review it again?
Thank you.

Comment 22

[Mozilla Shepherd (This account is used for automation and doesn't receive bugmail)]

Attachment: mozilla-b2g:master PR#25252

Comment 23

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

master,
https://github.com/mozilla-b2g/gaia/commit/7b7ccc4101e036e4c889dc32534aa890c7f3b3ec
--
CI with gij part 1,
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=3314f6a90be1

gji part2,
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=b88e85112801


--
Thanks for the review.

Comment 24

[KTucker [:KTucker][Inactive 3/4/2016]]

Pete, please verify that this issue is fixed.

Comment 25

[pete siphantong [:Petes]]

This bug is verified fixed on the Flame 2.2 (319mb)


Flame 2.2 Master KK (319mb) (Full Flash)

Environmental Variables:
Device: Flame Master
Build ID: 20141020040204
Gaia: dc496d04907dd314f9736ff78bab3bd27156f79a
Gecko: ca6b46cbca3b
Version: 36.0a1 (Master)
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0


Result: QWERTY keyboard replaces number pad when selecting a name field.

Comment 26

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

Blocking 2.2+ for all fixed regressions.