Closed Bug 1094122 Opened 5 years ago Closed 5 years ago

Keyboard remembered language regressed to default

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

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

VERIFIED FIXED
2.1 S9 (21Nov)
blocking-b2g 2.2+
Tracking Status
b2g-v2.0 --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.2 --- verified

People

(Reporter: gerard-majax, Assigned: mnjul)

References

Details

(Keywords: regression, Whiteboard: [p=1])

Attachments

(2 files)

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.
Jan, Rudy, any idea?
Blocks: 1048228
Flags: needinfo?(rlu)
Flags: needinfo?(janjongboom)
Adding qawanted for branch checks.
Keywords: qawanted
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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Keywords: qawanted
QA Contact: croesch
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
Keywords: regression
QA Contact: croesch
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.
Flags: needinfo?(croesch)
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.
Flags: needinfo?(rlu) → needinfo?(jlu)
Yeah I've observed that recently too. I'll take a look when I have time, keeping NI for reminding now.
(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.
QA Whiteboard: [QAnalyst-Triage+]
Flags: needinfo?(croesch)
QA Contact: jmitchell
Heads-up: It seems that the issue will NOT repro if you have SIM PIN enabled. I'm still investigating.
Attached file 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?
Assignee: nobody → jlu
Flags: needinfo?(jlu)
Attachment #8519693 - Flags: review?(rlu)
Attachment #8519693 - Flags: feedback?(janjongboom)
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.
(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.
(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)
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.
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|.
Flags: needinfo?(jlu)
(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.
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.
QA Whiteboard: [QAnalyst-Triage+]
QA Contact: jmitchell
So, it's my bug anyway. I'll take a look. :/
Blocks: 1075306
Flags: needinfo?(jlu)
and I need to keep that NI to remind myself. :/
Flags: needinfo?(jlu)
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.
Flags: needinfo?(jlu)
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 :)
Flags: needinfo?(janjongboom)
Attachment #8519693 - Flags: feedback?(janjongboom)
Comment on attachment 8519693 [details] [review]
Patch (PR @ GitHub)

r=me.

Thanks for figuring this out.
Attachment #8519693 - Flags: review?(rlu) → review+
Guess we should try to work out a integration test on this.
So set flag for tracking.
Flags: in-testsuite?
Master: https://github.com/mozilla-b2g/gaia/commit/dd4e56b6bb98a5609903a0342a9f3e5887124183
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [p=1]
Target Milestone: --- → 2.1 S9 (21Nov)
Will this fix on devices where the bad save has already been done ?
Flags: needinfo?(rlu)
Flags: needinfo?(jlu)
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.
Flags: needinfo?(rlu)
Flags: needinfo?(jlu)
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.
(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.
Status: RESOLVED → REOPENED
Flags: needinfo?(rlu)
Flags: needinfo?(jlu)
Resolution: FIXED → ---
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?
Flags: needinfo?(jlu)
(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.
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.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Glad it goes well as I have verified this patch should work during review.
Thanks for the re-confirmation.
Flags: needinfo?(rlu)
Blocking 2.2+ for all fixed regressions.
blocking-b2g: 2.2? → 2.2+
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.