Closed Bug 1097450 Opened 6 years ago Closed 5 years ago

Regressed bug 942309: Keyboard language again changes for input type=url and type=email, from type=text

Categories

(Firefox OS Graveyard :: Gaia::System::Input Mgmt, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

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

VERIFIED FIXED
2.2 S1 (5dec)
blocking-b2g 2.2+
Tracking Status
b2g-v2.2 --- verified

People

(Reporter: mnjul, Assigned: mnjul)

References

Details

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

Attachments

(1 file)

48 bytes, text/x-github-pull-request
timdream
: review+
timdream
: feedback+
johnhu
: feedback+
Details | Review
It appears that bug 942309 got regressed somehow.

STR:
1. Reset-gaia, select English language at FTU.
2. In Settings, enable some keyboard layouts such as German and French.
3. Go to UI tests app's Keyboard page. For convenience, scroll to the Others area.
4. Focus "text" input. Suppose you're at En layout. Switch to De layout.
5. Focus "url" input below. You'll be at De layout.
6. Focus "text" again, and you're at De layout. Switch to Fr layout.
7. Focus "url" input below.

Actual result:
At step 7, the layout activated is De layout.

Expected result:
At step 7, the layout activated is Fr layout, as specified by 942309.

Gaia: 65ce58778f65c5b39d9e558e02417c9954d55d6e
Gecko flashed from PBT Build-ID: 20141110160207
Alright, here's the cause: 

At _activateKeyboard() [1], the currentActiveLayout is always retrieved from Settings, even though |inputLayouts.setGroupsActiveLayout()| may have been called in previous setKeyboardToShow() and we have a more preferable layout to show.

Ideally we should not be looking up the Settings if we already have some activeLayout defined for a particular input group.

With that said, we need to change the schema of activeLayout: an uninitialized value should be |undefined|, different from 0, which signifies that the active layout is the first layout in the array.

This is really lame for me as the author of bug 942309's patch -- the tests I was supposed to write for that bug should have busted bug 1048228. Oh well.

[1] https://github.com/mozilla-b2g/gaia/blob/69ca5a030d42fff8eeb65de1012fd984cc1ab833/apps/system/js/keyboard_manager.js#L189-L202
Attached file Patch (PR @ GitHub)
(In reply to John Lu [:mnjul] [MoCoTPE] from comment #1)
> Ideally we should not be looking up the Settings if we already have some
> activeLayout defined for a particular input group.
> 
> With that said, we need to change the schema of activeLayout: an
> uninitialized value should be |undefined|, different from 0, which signifies
> that the active layout is the first layout in the array.

Aye, that's not enough by itself. We still need to save the settings when we do |this.inputLayouts.setGroupsActiveLayout(layout)| -- for each group supported by that layout, we need to save the setting of the active layout for that group. Or things still fail after reboot.

Tim & Rudy, please see if the proposed fix looks good for you.

I created |saveCurrentActiveLayoutMulti| in KeyboardHelper (!) as we probably don't want a for-loop that calls |saveCurrentActiveLayout| consecutively, locking mozSettings in each iteration. The old |saveCurrentActiveLayout| is being kept there for now because I haven't touched tv_apps (of course, I will do that in final patch). I'm planning that in the final revision the non-multi version will be replaced by the multi version, which will use the old name.
Assignee: nobody → jlu
Attachment #8522115 - Flags: feedback?(timdream)
Attachment #8522115 - Flags: feedback?(rlu)
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Whiteboard: [p=1]
Target Milestone: --- → 2.1 S9 (21Nov)
Comment on attachment 8522115 [details] [review]
Patch (PR @ GitHub)

Let's not put more things into KeyboardHelper.
Attachment #8522115 - Flags: feedback?(timdream)
Comment on attachment 8522115 [details] [review]
Patch (PR @ GitHub)

Discussed with Tim and we'll try removing functions form KeyboardHelper to new modules as a part of efforts toward the goal to disentangle KeyboardHelper and to facilitate bug 1094559.

For this bug, it directly means anything related to currentActiveLayout (down to settings retrieval/modification) can be extracted and live only in "Service" App. I will try to make it happen.
Attachment #8522115 - Flags: feedback?(rlu)
Comment on attachment 8522115 [details] [review]
Patch (PR @ GitHub)

Alright, Tim, could you check again?

I actually think we could use some more refactoring about InputLayout's storing "a group's active layout" -- currently we're having IL._currentActiveLayouts (mapping to Settings) and we're also having IL.layouts[group].activeLayout (accessed by KeyboardManager). We probably can consolidate that -- but I need to think about how KeyboardManager reasons about layouts[group].activeLayout. I think that can get into a follow-up? (Since this bug is actually a fix for regression, I don't want to put too many things in it.)

Will also fix things for tv-apps and flag JoHu for feedback in the next round.
Attachment #8522115 - Flags: feedback?(timdream)
Comment on attachment 8522115 [details] [review]
Patch (PR @ GitHub)

See comments. Please make sure we have explicit protection so that keyboard is only loaded *after* we have the settings. Preferably if the getGroupCurrentActiveLayoutIndex method can be a async promise (instead of returning a sync result) it would be clearer.
Attachment #8522115 - Flags: feedback?(timdream) → feedback+
Comment on attachment 8522115 [details] [review]
Patch (PR @ GitHub)

Tim, I'd like to ask you to check the async design before I write tests and ask for review. Thanks!
Attachment #8522115 - Flags: feedback+ → feedback?(timdream)
Comment on attachment 8522115 [details] [review]
Patch (PR @ GitHub)

It's getting close but we should be able to reduce the logic more with Promise.
Attachment #8522115 - Flags: feedback?(timdream) → feedback+
I'm pushing back the Target Milestone as I'll probably need to study how to mess around with MockDOMRequest and/or MockPromise for writing the unit tests.
Target Milestone: 2.1 S9 (21Nov) → 2.2 S1 (5dec)
Uhm....Gip doesn't seem to like my patch. Let me see what I can do.
Comment on attachment 8522115 [details] [review]
Patch (PR @ GitHub)

(In reply to John Lu [:mnjul] [MoCoTPE] from comment #10)
> Uhm....Gip doesn't seem to like my patch. Let me see what I can do.

It turned out that subsequent retriggers didn't have the failure and I couldn't reproduce it locally either..

Anyway, I have amended the patch and also fixed/added unit tests.

JohnHu, since we're modifying the shared keyboard_helper.js, I had to touch keyboard_manager.js and input_layouts.js in tv_apps. Patch for input_layouts.js could be directly applied but not for keyboard_manager.js, so you might want to take a look. Thanks!
Attachment #8522115 - Attachment description: Proposed WIP Patch (PR @ GitHub) → Patch (PR @ GitHub)
Attachment #8522115 - Flags: review?(timdream)
Attachment #8522115 - Flags: feedback?(im)
Whiteboard: [p=1] → [p=2]
Comment on attachment 8522115 [details] [review]
Patch (PR @ GitHub)

Thanks for this patch.

We don't use mozSettings.get to retrieve record anymore because the cost is high, about 500ms for each. We have a boot-up time limitation. Would you mind to change the code to use |SettingsCache.get(callback)| which is also used at Settings app.
Attachment #8522115 - Flags: feedback?(im) → feedback+
Attachment #8522115 - Flags: review?(timdream) → review+
Comment on attachment 8522115 [details] [review]
Patch (PR @ GitHub)

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #12)
> Comment on attachment 8522115 [details] [review]
> Patch (PR @ GitHub)
> 
> Thanks for this patch.
> 
> We don't use mozSettings.get to retrieve record anymore because the cost is
> high, about 500ms for each. We have a boot-up time limitation. Would you
> mind to change the code to use |SettingsCache.get(callback)| which is also
> used at Settings app.

JohnHu, I made a modification on that, could you make a quick check again (since this is my first time poking into tv_apps)? Thanks!
Attachment #8522115 - Flags: feedback+ → feedback?(im)
Comment on attachment 8522115 [details] [review]
Patch (PR @ GitHub)

Thanks. This looks good.
Attachment #8522115 - Flags: feedback?(im) → feedback+
Master: https://github.com/mozilla-b2g/gaia/commit/64c2be93efa05a5586be2fdb21809d901e873a52
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Tested repro steps on the engineering build- verified as fixed in Flame 2.2.

Actual Result: At step 7, the Fr layout remains. Keyboard layouts remain as long as the user desires.

Environmental Variables:
----------------------------------------
Device: Flame 2.2  (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141124160209
Gaia: e5d666d6f62480ced56c6d9352f5e12befb5a862
Gecko: 5c4d07e2199e
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?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
This patch made depends on Gecko 36 (because of mozSettings.get API change). We're currently stuck with Gecko 34 (see bug 1089710) and it makes testing integration of the System app on MacOS at least impossible.
Zibi, is there anything I can do on my side? (Also, if needed, feel free to find me in person if you're in Portland too)
Flags: needinfo?(gandalf)
I dont thunk so. We just need to fix the bug with b2g 36 :)
Flags: needinfo?(gandalf)
2.2+ all fixed regressions -- reset if disagree.
blocking-b2g: 2.2? → 2.2+
You need to log in before you can comment on or make changes to this bug.