1094122 - Keyboard remembered language regressed to default

Status: VERIFIED FIXED

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

Description

[:gerard-majax]

In bug 1048228 we made keyboard aboe to remember the last used language. Since a couple of days on my devices (Nexus S, Xperia ZR, Flame), I have noticed that after a reboot, my keyboard now goes again back from fr to en.

Comment 1

[:gerard-majax]

Jan, Rudy, any idea?

Comment 2

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

Adding qawanted for branch checks.

Comment 3

[Cody Roesch [:croesch]]

Tested with Shallow Flash on 319mb using Engineering builds

This bug repro's on Flame KK builds: Flame 2.2 KK, Flame 2.1 KK, Flame 2.0 KK, Flame 2.0 Base

Actual Results: Language keeps reverting back to English keyboard from French keyboard after a reboot takes place. Also tested other language keyboards and system languages with the same result.

Repro Rate: 7/7

Environmental Variables:
Device: Flame 2.2 KK
BuildID: 20141105043145
Gaia: bc843fbf8973c8e124e5668bb2752f5d025fb488
Gecko: 72ae882fce1a
Version: 36.0a1 (2.2) 
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
-----------------------------------------------------------------
Environmental Variables:
Device: Flame 2.1 KK
BuildID: 20141105063632
Gaia: 445656e36d14054d46a8f201dc68d76da8cfa281
Gecko: e7efb38a6477
Version: 34.0 (2.1) 
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
-----------------------------------------------------------------
Environmental Variables:
Device: Flame 2.0 KK
BuildID: 20141105063630
Gaia: e19ba2a049f192a560c6e63a0cc213b0353f1a02
Gecko: 2c2d1f552ff6
Version: 32.0 (2.0) 
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
-----------------------------------------------------------------
Environmental Variables:
Device: Flame 2.0 Base
BuildID: 20141021162107
Gaia: 8c5c956ee6909408e29f375cc7d843a03d92f3d8
Version: 32.0 (2.0)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 4

[:gerard-majax]

I'm a bit dubious of your branch checking, we have had this being fixed a while ago. I'd prefer that you check on master back to early september at most.

Comment 5

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

Since John is involved with input management recently more than me.
I'd like John's advice help on this.

John, any clue about this issue?
Thanks.

Comment 6

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

Yeah I've observed that recently too. I'll take a look when I have time, keeping NI for reminding now.

Comment 7

[Joshua Mitchell (Inactive)]

(In reply to Alexandre LISSY :gerard-majax from comment #4)
> I'm a bit dubious of your branch checking, we have had this being fixed a
> while ago. I'd prefer that you check on master back to early september at
> most.

Nothing dubious here; it is always standard to do a branch check on the latest builds. As an additional task you can tag qa-wanted to check specific areas like you mentioned.

After checking the First KK Master Branch build I see that this does NOT repro there, meaning that it has regressed within the branches themselves. I'll tag regression-window-wanted and we will find the cause.

Comment 8

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

Heads-up: It seems that the issue will NOT repro if you have SIM PIN enabled. I'm still investigating.

Comment 9

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

Attachment: Patch (PR @ GitHub)

Well, it appears that now we receive onerror at [1] that takes place before retrieving currentActiveLayout from settings. As such, setKeyboardToShow would be launched with index = undefined in the code that follows, which would unfortunately write a default layout setting, overwriting what we have already set in last keyboard switching.

[1] https://github.com/mnjul/gaia/blob/bug_1094122_layout_not_remembered/apps/system/js/keyboard_manager.js#L137

Chances are that results from a Gecko change so I'll leave bisecting to QA.

Still, I think we should probably not write currentActiveLayout setting in setKeyboardToShow if it's being called with index = undefined, so here's the patch doing that. Rudy, Jan, how do you think?

Comment 10

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

We implemented this issue with bug 1048228 and it only landed to v2.2.
Will look into this in more detail after we have regression window identified.

Comment 11

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

(In reply to Rudy Lu [:rudyl] from comment #10)
> We implemented this issue with bug 1048228 and it only landed to v2.2.

implemented this *feature*, i.e. to remember the last active layout.

> Will look into this in more detail after we have regression window
> identified.

Comment 12

[:gerard-majax]

(In reply to John Lu [:mnjul] [MoCoTPE] from comment #9)
> Created attachment 8519693 [details] [review]
> Patch (PR @ GitHub)
> 
> Well, it appears that now we receive onerror at [1] that takes place before
> retrieving currentActiveLayout from settings. As such, setKeyboardToShow
> would be launched with index = undefined in the code that follows, which
> would unfortunately write a default layout setting, overwriting what we have
> already set in last keyboard switching.
> 
> [1]
> https://github.com/mnjul/gaia/blob/bug_1094122_layout_not_remembered/apps/
> system/js/keyboard_manager.js#L137
> 
> Chances are that results from a Gecko change so I'll leave bisecting to QA.
> 
> Still, I think we should probably not write currentActiveLayout setting in
> setKeyboardToShow if it's being called with index = undefined, so here's the
> patch doing that. Rudy, Jan, how do you think?

We did a lot of work around settings in Gecko recently, it's possible there are still edge cases to polish :).

I added some debugging to the code you pointed:
> 11-10 10:33:09.905   256   256 D GeckoConsole: Content JS DEBUG: LAUNCH_ON_BOOT_KEY: err=[object Event] 
> 11-10 10:33:09.905   256   256 D GeckoConsole:     at km_launchOnBoot/req.onerror< (app://system.gaiamobile.org/js/keyboard_manager.js:139:8)
> 11-10 10:33:09.905   256   256 D GeckoConsole: Content JS DEBUG: LAUNCH_ON_BOOT_KEY: JSON(err)={"isTrusted":true} 
> 11-10 10:33:09.905   256   256 D GeckoConsole:     at km_launchOnBoot/req.onerror< (app://system.gaiamobile.org/js/keyboard_manager.js:140:0)
> 11-10 10:33:10.035   256   256 D GeckoConsole: Content JS DEBUG: LAUNCH_ON_BOOT_KEY: err=[object Event] 
> 11-10 10:33:10.035   256   256 D GeckoConsole:     at km_launchOnBoot/req.onerror< (app://system.gaiamobile.org/js/keyboard_manager.js:139:8)
> 11-10 10:33:10.035   256   256 D GeckoConsole: Content JS DEBUG: LAUNCH_ON_BOOT_KEY: JSON(err)={"isTrusted":true} 
> 11-10 10:33:10.035   256   256 D GeckoConsole:     at km_launchOnBoot/req.onerror< (app://system.gaiamobile.org/js/keyboard_manager.js:140:0)

Comment 13

[:gerard-majax]

Attachment: dumping all error props

Comment 14

[:gerard-majax]

Settings API debugging turned on:

> 11-10 11:08:05.044   265   265 I Gecko   : -*- SettingsRequestManager: Making get request for keyboard.launch-on-boot
> 11-10 11:08:05.054   265   265 I Gecko   : -*- SettingsRequestManager: Request for 'keyboard.launch-on-boot' successful. Record count: 1
> 11-10 11:08:05.054   265   265 I Gecko   : -*- SettingsRequestManager: keyboard.launch-on-boot: undefined, true

We successfully read a valid user-defined value "undefined". I'm not sure that we are really getting into the error handler.

Comment 15

[:gerard-majax]

Using a differentiated onerror/onsuccess callback I clearly don't get into the error handler but rather in the success handler, with a value |undefined| which, according to the underlying API, is a user-defined value and not the default which is |true|.

Comment 16

[:gerard-majax]

(In reply to John Lu [:mnjul] [MoCoTPE] from comment #8)
> Heads-up: It seems that the issue will NOT repro if you have SIM PIN
> enabled. I'm still investigating.

That's not true: my devices have SIM cards with PIN code defined, and I do reproduce the issue.

Comment 17

[Joshua Mitchell (Inactive)]

B2G-Inbound Regression Window:

Last Working:
Device: Flame 2.2
Build ID: 20141103030734
Gaia: 7a8c0fe5bd812d51cf9dd336232b803151780171
Gecko: 95edbe6764d0
Gonk: Could not pull gonk.  Did you shallow Flash?
Version: 36.0a1 (2.2)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First Broken:
Device: Flame 2.2
Build ID: 20141103033733
Gaia: 82c3899a7fd73cb3d7c406696c67f12ed461547c
Gecko: abce2fad9485
Gonk: Could not pull gonk.  Did you shallow Flash?
Version: 36.0a1 (2.2)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Gaia/Gecko Swap
Last Working Gaia First Broken Gecko: Issue does NOT reproduce
Gaia: 7a8c0fe5bd812d51cf9dd336232b803151780171
Gecko: abce2fad9485
First Broken Gaia Last Working Gecko: Issue DOES reproduce
Gaia: 82c3899a7fd73cb3d7c406696c67f12ed461547c
Gecko: 95edbe6764d0

Gaia pushlog: https://github.com/mozilla-b2g/gaia/compare/7a8c0fe5bd812d51cf9dd336232b803151780171...82c3899a7fd73cb3d7c406696c67f12ed461547c

------------------------------------------------------------------------------------------------------------------

This was broken by Bug 1075306 - patch author is already assigned, so no NI here.

Comment 18

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

So, it's my bug anyway. I'll take a look. :/

Comment 19

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

and I need to keep that NI to remind myself. :/

Comment 20

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

Alex, first I'd like to thank you for your time with this bug. Yep, it turned out that the issue on onsuccess/onerror by itself was irrelevant and Gecko was not involved.

The regression analysis was correct: My patch at 1075653 omitted the early return that had prevented unintentional saving [1].

So, my patch at comment 9 kinda addressed the regression, but I have modified the patch a little bit to rely on the correct argument, restoring the behavior omitted by bug 1075653.

[1] https://github.com/mozilla-b2g/gaia/pull/25249/files?diff=unified#diff-869edc18f465b7f5cdda81d81cd58a71L363

Rudy, could you check the review? Thanks.

Comment 21

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

Comment on attachment 8519693 [details] [review]
Patch (PR @ GitHub)

Since this is just my regression, we don't really need to ask Jan to provide input :)

Comment 22

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

Comment on attachment 8519693 [details] [review]
Patch (PR @ GitHub)

r=me.

Thanks for figuring this out.

Comment 23

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

Guess we should try to work out a integration test on this.
So set flag for tracking.

Comment 24

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

Master: https://github.com/mozilla-b2g/gaia/commit/dd4e56b6bb98a5609903a0342a9f3e5887124183

Comment 25

[:gerard-majax]

Will this fix on devices where the bad save has already been done ?

Comment 26

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

With this patch, you still need to re-launch one of the keyboard layouts and then it will be remembered.
The stored "undefined" value is not able to be recovered, since it means nothing to the input management.

Comment 27

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

This is because the “active layout” settings is overwritten with undefined value.
(To be precise, the setting entry is "keyboard.current-active-layouts")
So, you have to focus on an input field to launch the keyboard again and the new value will be saved for the next boot.

Hope this is clear enough.

Comment 28

[:gerard-majax]

(In reply to Rudy Lu [:rudyl] from comment #27)
> This is because the “active layout” settings is overwritten with undefined
> value.
> (To be precise, the setting entry is "keyboard.current-active-layouts")
> So, you have to focus on an input field to launch the keyboard again and the
> new value will be saved for the next boot.
> 
> Hope this is clear enough.

This is not working. I've updated my Nexus S, made sure that Gaia contains the patch from this, and it is still defaulting to "en".

STR:
 0. Boot
 1. Focus input field
 2. Set keyboard to French
 3. Reboot

Expected:
 I have french keyboard when focusing in an input.

Actual:
 English keyboard is displayed.

Comment 29

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

Well, I can't reproduce the bug with your STR current master. Do you have more detailed STR/configurations, and on Flame maybe?

Rudy, could you check?

Comment 30

[:gerard-majax]

(In reply to John Lu [:mnjul] [MoCoTPE] from comment #29)
> Well, I can't reproduce the bug with your STR current master. Do you have
> more detailed STR/configurations, and on Flame maybe?

That's not related to the device.

Comment 31

[:gerard-majax]

And that's probably not because of your patch: after adding debug and reinstalling system app, it does work. I fear bug 941969 is at fault here.

Comment 32

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

Glad it goes well as I have verified this patch should work during review.
Thanks for the re-confirmation.

Comment 33

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

Blocking 2.2+ for all fixed regressions.

Comment 34

[sarsenyev]

Verified the issue is fixed on 2.2 Flame

When a different language is chosen and the device is rebooted, the keyboard for the selected language is remembered after reboot

Flame 2.2 (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141124100136
Gaia: aad40f6d6eb8f626c6a20db55b9f00d2e832f113
Gecko: be4ba3d5ca9a
Gonk: 
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0