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

VERIFIED FIXED in 2.1 S7 (24Oct)

Status

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: bzumwalt, Assigned: rudyl)

Tracking

({regression})

unspecified
2.1 S7 (24Oct)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.1 unaffected, b2g-v2.2 verified)

Details

(Whiteboard: [2.2-Daily-Testing], [p=1])

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Posted image 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
(Reporter)

Comment 1

5 years ago
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.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
Keywords: regression
Whiteboard: [2.2-Daily-Testing]
(Reporter)

Comment 2

5 years ago
Posted file Logcat
Hi Tim,

does this sound familiar to you?

Thanks!
Flags: needinfo?(timdream)
Should be a recent regression. (keeping needinfo)
blocking-b2g: --- → 2.2?
Component: Gaia::Contacts → Gaia::Keyboard
requesting a window.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
QA Contact: pcheng
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.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Nikhil - this window only has a single item in the pushlog which was authored by you, can you take a look?
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell) → needinfo?(nsm.nikhil)
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.
Flags: needinfo?(nsm.nikhil)
We are indeed using promise to load assets required to switch layouts in Keyboard code. I will look deeper to see what is involved.
Assignee: nobody → timdream
Blocks: 1062323
Status: NEW → ASSIGNED
Flags: needinfo?(timdream)
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.
Attachment #8503910 - Flags: feedback?(jlu)
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?
Attachment #8503910 - Flags: feedback?(jlu) → feedback+
(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.
Posted file debug-log
debug log from keyboard app
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().
Posted file 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.
Attachment #8503941 - Flags: feedback?(timdream)
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.
Attachment #8503941 - Flags: feedback?(timdream) → feedback+
Change the assignee.
Assignee: timdream → rlu
Comment on attachment 8503941 [details] [review]
Patch V1

Patch updated to include the modified test case.

Tim, please help review it.
Thanks.
Attachment #8503941 - Attachment description: WIP → Patch V1
Attachment #8503941 - Flags: review?(timdream)
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.
Attachment #8503941 - Flags: review?(timdream)
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [2.2-Daily-Testing] → [2.2-Daily-Testing], [p=1]
Target Milestone: --- → 2.1 S7 (24Oct)
Pete, please verify that this issue is fixed.
Flags: needinfo?(psiphantong)
Keywords: verifyme
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(psiphantong) → needinfo?(ktucker)
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Blocking 2.2+ for all fixed regressions.
blocking-b2g: 2.2? → 2.2+
You need to log in before you can comment on or make changes to this bug.