828237 - Change the OS language should change the keyboard layout (or ask the user if he/she wants it)

Status: RESOLVED FIXED

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

Comment 1

[:Margaret Leibovic]

Is this just a problem when the user switches languages while the phone is running, or is the keyboard layout always english unless the user actively switches it?

I'm worried that it's probably the latter because the default keyboard.layouts.* settings are specified in gaia/build/settings.py, and only english is set to True.

Pike or Stas, are these settings modified for localized builds?

Comment 2

[Axel Hecht [:Pike]]

I wouldn't know if, or where to start looking. I don't know of any infrastructure in gaia that would do something like localized prefs, as we have in gecko.

Comment 3

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I think + should have been for b2g18.  I agree with vivien for comment 828912, I think in some sense that if this is fixed then 828912 wouldn't be an issue.  ie. I think 828912 is a dup of this bug.

Comment 4

[Fabien Cazenave [:kaze]]

No emergency imho but we need UX input here. Josh?

Comment 5

[Josh Carpenter [:jcarpenter]]

I'm not familiar with the issue, to be quite honest, being a solo english speaker, so I defer to what our multilingual team members feel is the right approach. 

AFAIK, switching languages in iOS automatically adds the associated keyboard to the "quick pick" button on the keyboard layout, enabling the user to use the new language's KB without having to manually select it from Settings, while also allowing them quick access to the previous language's KB. It's an automatic, elegant approach.

Comment 6

[mlevin]

Issue repro's on:
gecko: cf1b9d27345e70daf242514733f848e76241ea1d
gaia: 7e54ca673277b20b1d91d18477dc44d6ad226761
Build ID: 20130207070202

Issue: 
1. During FTU choose Spanish as the language. 
2. After FTU go to settings - language and change it to English. 
3. Go to settings - keyboard and notice that Spanish is checked and not English.

Comment 8

[Delphine Lebédel [:delphine - Use needinfo]]

FWIW: if I were for example an Arabic speaker, and I change the language from let's say English to Arabic in my phone's settings, I'd probably expect my keyboard to change into Arabic as well (and not stay in Latin Alphabet format).

Comment 9

[mlevin]

This comment appears in bug 839604.

Josh Carpenter [:jcarpenter] 2013-02-11 09:36:11 PST
This is a bug. Quoting the specs:

> Changing the language also adds that language to the user's 
> keyboard list automatically.

Specs here, page 21:

https://www.dropbox.com/s/yg8hwdocs3jr92z/R1_Personalization_v3.pdf

Comment 10

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Attachment: Pull Request #9073 - Switch to new keyboard if language changed

Based on the current selected language from the Settings app, this patch adds the new keyboard layout group to the user's enabled keyboards and switches the current keyboard ('keyboard.current') to the language-associated keyboard.

I also corrected some typos in layout.js and updated the 'language.current' observer in the FTU app to not conflict with the one I introduced in keyboard.js.

Comment 11

[Fernando Campo (:fcampo)]

For what is worth, changes on FTU have my blessing ;)

(In reply to Mihai Cirlanaru [:mcirlanaru] from comment #10)
> Created attachment 735471 [details] [review]
> Pull Request #9073 - Switch to new keyboard if language changed
> [...] updated the 'language.current'
> observer in the FTU app to not conflict with the one I introduced in
> keyboard.js.

Comment 12

[David Flanagan [:djf]]

Mihai,

According to https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.gaia/E1xniLLxPoc this bug doesn't have high enough priority for me to review the patch right now.  Sorry!

Fixing this does seem important. And I think this is related to the issue in bug 821646. If we can get Josh to approve the approach I suggested in that bug, then I think that would fix this one as well.

When the user switches from English to Arabic, we obviously want to enable an Arabic keyboard layout. But what is not clear to me is whether we would also then disable the English layout or leave that setting alone. What if the user then switches from Arabic to Spanish? We enable Spanish, but do we disable Arabic, or leave it enabled?

If we enable an appropriate default layout for the current language in the keyboard app, then we can let the settings app be just for optional additional layouts.

Comment 13

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Hi David,

Thanks for looking into this,

> Fixing this does seem important. And I think this is related to the issue in
> bug 821646. If we can get Josh to approve the approach I suggested in that
> bug, then I think that would fix this one as well.

Yes that is true, however, from what I understand from the specs referenced in comment 9, the desired behavior is to add the keyboard associated with the current language to the list of enabled keyboards.

The only time when only the selected language default keyboard is enabled is in the FTU app. My changes to FTU (that Fernando agrees with in comment 11), enforce this by disabling all other enabled keyboards -- behavior that was not present there before (only the previous keyboard layout was disabled).
 
> When the user switches from English to Arabic, we obviously want to enable
> an Arabic keyboard layout. But what is not clear to me is whether we would
> also then disable the English layout or leave that setting alone. What if
> the user then switches from Arabic to Spanish? We enable Spanish, but do we
> disable Arabic, or leave it enabled?

The specs referenced in comment 9 (page 21 of pdf) mention (quote):
> Changing the language also adds that language to the user's keyboard list
> automatically.

From this, what I understand is that any switch of language would simply add the specific keyboard to the list of enabled keyboards (which can be consulted/changed in the Keyboard section of the Settings app). One scenario comes into my mind that highlights this behavior as desired: user sets language to Portuguese (pt-BR) in FTU app, and then decides to switch the language to English (en-US), however, being located in a Portuguese speaking country, most of his communication (SMS, Email, Social Networks, etc.) is done in Portuguese and thus the pt-BR keyboard would be needed (or at least frequently used) -- leaving pt-BR in the list of enabled keyboards is thus important.

Josh, maybe you can give us a comment on this too? Thanks!

> 
> If we enable an appropriate default layout for the current language in the
> keyboard app, then we can let the settings app be just for optional
> additional layouts.

That is true, however, how will multiple language-specific keyboards be enabled in this case (since you can't select more languages from the Language section of the Settings app)?

Comment 14

[Josh Carpenter [:jcarpenter]]

Here's the breakdown, from my POV. 

Scenario: "User changes device language from Spanish to Italian..."

FTU:

* Enable Italian keyboard and disable all others. Binary.

Normal:

* Enable Italian keyboard, and leave Spanish also enabled. Additive.

Have I missed anything?

Comment 15

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

(In reply to Josh Carpenter [:jcarpenter] from comment #14)
> Here's the breakdown, from my POV. 
> 
> Scenario: "User changes device language from Spanish to Italian..."
> 
> FTU:
> 
> * Enable Italian keyboard and disable all others. Binary.
> 
> Normal:
> 
> * Enable Italian keyboard, and leave Spanish also enabled. Additive.
> 
> Have I missed anything?

Thanks for the concise reply, Josh, this is exactly the way I implemented the patch.

David, do you think that, taking comment 14 into consideration, this would be relevant for leo? I'm nominating this bug as blocking so that it gets the needed priority.

The current behavior for the scenario that Josh highlights in comment 14 is:
* FTU: enable current language keyboard and disable previous one (i.e. not all others) -- which my patch fixes (cf. comment 11)
* Normal: the current language associated keyboard is not enabled, nor switched to -- basically, nothing happens with the keyboard when the current language is changed.

Comment 16

[Josh Carpenter [:jcarpenter]]

> * Normal: the current language associated keyboard is not enabled, nor switched to -- basically, nothing happens with the keyboard when the current language is changed.

This is the part I missed in comment 14 (I knew there was something...): when we switch languages and enable the associated keyboard, do we also activate it? In my opinion, yes, we do. We do not want to create unnecessary confusion for users who may need to change their device's language, but never have the keyboard Settings interface. Better to err on the side of helping novice users here, IMO. Experienced users who want to use an English keyboard while their device is in Spanish (for example), can always make that change relatively easily.

So to ammend comment 14:

Scenario: "User changes device language from Spanish to Italian..."

FTU:

* Enable Italian keyboard and disable all others. Binary.

Normal:

* Enable Italian keyboard, activate Italian keyboard, and leave Spanish also enabled. Additive.

Definition of terms:

"Enabled" — Keyboard is "checked" in Settings > Keyboard, which makes it available from the "globe" key of the keyboard interface.
"Active" — Keyboard appears when the user taps a text field. It is "active", or "default" (choose your own verbiage :)

Make sense?

Comment 17

[David Flanagan [:djf]]

Mihai,

Yes, I agree that this should be fixed for Leo. Ping me if it gets approval and I'll make it a priority to review.

Comment 18

[David Flanagan [:djf]]

Comment on attachment 735471 [details] [review]
Pull Request #9073 - Switch to new keyboard if language changed

r- because I think that most of this code belongs in the settings app rather than the keyboard app.

This is all about changing the enabled layouts settings when the language setting changes, so please just do it in the settings app. The settings app already reads /shared/resources/languages.json, so it is the app that should read the other json file as well.

I think you'll need to add code to the keyboard app so that when a new layout is enabled it becomes the current layout, but I think that is the only modification you should have to make to the keyboard.

Comment 19

[David Flanagan [:djf]]

Here's another question: why is the change to FTU required? When would FTU be run with more than one keyboard layout enabled? If it really is the first time, presumably there is only one keyboard group enabled by default.  If the user has done a 'reset to factory defaults', presumably that reset the enabled keyboards.  And if the user just wants to re-run FTU because they want to take the tour again or something, then maybe they don't want to reset any layouts they previously requested.

So are you certain that this change to FTU is necessary and appropriate?  Was there any specific bug filed about the current FTU behavior?

Comment 20

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

(In reply to David Flanagan [:djf] from comment #18)
> Comment on attachment 735471 [details] [review]
> Pull Request #9073 - Switch to new keyboard if language changed
> 
> r- because I think that most of this code belongs in the settings app rather
> than the keyboard app.
> 
> This is all about changing the enabled layouts settings when the language
> setting changes, so please just do it in the settings app. The settings app
> already reads /shared/resources/languages.json, so it is the app that should
> read the other json file as well.
> 
> I think you'll need to add code to the keyboard app so that when a new
> layout is enabled it becomes the current layout, but I think that is the
> only modification you should have to make to the keyboard.

Thanks for the feedback, David! I updated the pull request with the new implementation in the Settings app, as you suggested. I'm also adding :evelyn for review on the settings.js changes.

The changes to the FTU are motivated by the intended UX (i.e. "Enable <language> keyboard and disable all others", cf. comment 14), which is not as it was implemented -- of course, the case you highlight in comment 19 makes sense and the current implementation works just fine for it, however, it does not "disable all others"... :fcampo agrees with my changes in the FTU (cf. comment 11), let me know if I should remove them.

From what I know there is no bug about this behavior since it is not observable with existing use cases (i.e. only one keyboard is enabled when a phone is flashed) -- only checking the source code you notice this.

Comment 21

[David Flanagan [:djf]]

Comment on attachment 735471 [details] [review]
Pull Request #9073 - Switch to new keyboard if language changed

This is much better, but still not quite the right way to do it, I think. See my comments on github.

Comment 22

[Evelyn Hung]

Comment on attachment 735471 [details] [review]
Pull Request #9073 - Switch to new keyboard if language changed

clear review request, please address the comments from :djf.

Comment 23

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Comment on attachment 735471 [details] [review]
Pull Request #9073 - Switch to new keyboard if language changed

Thanks for the feedback, David! I addressed your comments on GitHub, please have a look at my input (including on the outdated diff) and let me know how should I go about it.

Comment 24

[David Flanagan [:djf]]

Comment on attachment 735471 [details] [review]
Pull Request #9073 - Switch to new keyboard if language changed

This is getting much closer, but there are still some issues with how the keyboard.current setting is handled in the keyboard app. My full set of comments is on github, but as you can see from those comments, I'm pretty confused about the right way to handle this. 

Here is my current thinking, though: 

1) We should modify shared/resources/keyboard_layouts.json so that it maps from language codes to keyboard layout codes. It should not mention keyboard layout groups at all.  This file will be like your keyboardAliases object in keyboard.js

2) Ideally, we should change it from a .json file that has to be read with XHR into an ordinary .js file that apps can just include.  Unless there is some vendor configurability reason that we have to use JSON.

3) In FTU and Settings, when the language changes, you should update keyboard.current to match the language change, based on the data in the keyboard_layouts file.

4) The Settings app will allow the user to enable and disable additional keyboard layouts but that should be completely independent of the keyboard.current setting

5) When the user switches layouts from the keyboard, the keyboard app should set the keyboard.current setting so that the user's preference is remembered across reboots, and so that the preference gets passed on to interested apps like e.me.

6) The Keyboard app must be modified to always offer the user a choice among these keyboards:  a) the default keyboard for the current language b) the keyboard specified by keyboard.current c) all keyboards in all enabled keyboard layout groups.

Does this make sense?  As you can probably tell I'm not entirely confident about the right way to handle this.  So feel free to argue it with me.

Comment 25

[David Flanagan [:djf]]

Evelyn: what do you think the right way to handle this bug is? Do you think comment 24 makes sense?

Comment 26

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

David, thanks for checking my latest updates to the patch, I'm adding below my thoughts and comments on your suggestions:

(In reply to David Flanagan [:djf] from comment #24)
> Comment on attachment 735471 [details] [review]
> Pull Request #9073 - Switch to new keyboard if language changed
> 1) We should modify shared/resources/keyboard_layouts.json so that it maps
> from language codes to keyboard layout codes. It should not mention keyboard
> layout groups at all.  This file will be like your keyboardAliases object in
> keyboard.js

This would be a relatively major change in the keyboard layout groups logic, the FTU app uses this shared file to enable the appropriate (language-specific) keyboard group. If we remove this, we might as well consider removing the whole keyboard groups idea -- I think we were discussing this on GitHub too.

> 2) Ideally, we should change it from a .json file that has to be read with
> XHR into an ordinary .js file that apps can just include.  Unless there is
> some vendor configurability reason that we have to use JSON.

If we stick to a 1-to-1 mapping between language code and keyboard, as mentioned in point 1), I totally agree with this.

> 3) In FTU and Settings, when the language changes, you should update
> keyboard.current to match the language change, based on the data in the
> keyboard_layouts file.

The way I approached this in my patch is that, when the language changes, the keyboard.current is set and the language associated keyboard group is enabled (using keyboard_layouts.json) -- all this happens in the FTU and the Settings apps independently (i.e. both apps read keyboard_layouts.json and have their own implementation for updating mozSettings).

> 4) The Settings app will allow the user to enable and disable additional
> keyboard layouts but that should be completely independent of the
> keyboard.current setting

This is how it happens now: (in Settings > Keyboard) enabling new keyboard layout groups does not affect the keyboard.current setting, but simply adds the relevant keyboard.layout._ to mozSettings.

> 5) When the user switches layouts from the keyboard, the keyboard app should
> set the keyboard.current setting so that the user's preference is remembered
> across reboots, and so that the preference gets passed on to interested apps
> like e.me.

This logic is already present in keyboard.js, keyboard.current is set when renderKeyboard is called.

> 6) The Keyboard app must be modified to always offer the user a choice among
> these keyboards:  a) the default keyboard for the current language b) the
> keyboard specified by keyboard.current c) all keyboards in all enabled
> keyboard layout groups.

For a) my understanding is that keyboard.current has this exact purpose: to provide the user with the default keyboard for the current language.

Bug 821646, for which I also worked, seems to also be relevant to these points.

Comment 27

[David Flanagan [:djf]]

I've asked people who have worked on the settings and keyboard apps for longer than me what the purpose of the keyboard groups is and whether we can get rid of them.

Comment 28

[David Flanagan [:djf]]

Mihai,

I've asked around, and it sounds like there is no good reason for the keyboard groups stuff to exist. So we can remove it if we want. However, I've also learned that there are plans to redo all of the keyboard settings code for version 1.2, so at this point, it might just make sense to land a simpler patch (like what you've got) and fix this leo+ bug quickly.

I didn't realize that renderKeyboard() already updated keyboard.current, so that addresses one of my objections to your patch.  And I'm willing to give up on the keyboardAlias object: I now understand why you need that in keyboard.js.  So maybe we can just land the code you've got.

I forget whether any of my github comments were substantive.  If you think no changes are needed, just set the r? flag again, and I'll review the patch again. This time, with the knowledge that we're going to redo it all soon anyway, I won't be such a stickler about getting it right.

Thanks for your patience with me on this one.

Clearing the needinfo request for Evelyn since we've exchanged email.

Comment 29

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Comment on attachment 735471 [details] [review]
Pull Request #9073 - Switch to new keyboard if language changed

Thanks for all the info, David! I addressed all your comments on GitHub and made the relevant updates, feel free to look over the patch and merge it if it looks good (already added r=you in the commit message).

Regarding the new changes to the Keyboard App that are planned for the future, I would be more than happy to help out since I became quite familiar with all the inner workings of its implementation. Count me in!

Comment 30

[David Flanagan [:djf]]

Comment on attachment 735471 [details] [review]
Pull Request #9073 - Switch to new keyboard if language changed

I have only two new comments on Github.

r+ if you remove the FTU changes and address the issue of the unnecessary call to handleNewKeyboards().

If you really think we need the changes to the FTU app, let me know (maybe with a needinfo here or by email) and we'll discuss that.

Thank you for your work on this. I may well take you up on your offer to help with the refactoring when we start working on a new version of the keyboard!

Comment 31

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Thanks for all the feedback, David! Updated the patch to take into account your latest suggestions.

Landed on master:
https://github.com/mozilla-b2g/gaia/commit/74e31735fc78bf550db1f6e2f00ba5e907357180

Comment 32

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 74e31735fc78bf550db1f6e2f00ba5e907357180 to:
v1-train: 1ad04796ce64a79c084a6fef1617fd49e43c3bff

Comment 33

[David Flanagan [:djf]]

Backed out of master: https://github.com/mozilla-b2g/gaia/commit/8c7623bcc3fc6192d8bd7a84d83388421ed71d4a

Comment 34

[David Flanagan [:djf]]

Backed out because it turns out that this patch completely cripples the latin input method.  I don't know why I didn't catch that in review.

Comment 35

[David Flanagan [:djf]]

Backed out of v1-train also. 

Reopening the bug.

Setting needinfo for jhford to check whether I've done the backout correctly.

Also taking the bug from Mihai to fix myself in the morning.

Mihai: I backed this out when I realized that it had broken the auto-correct and all the other features provided by the latin input method. I don't know why neither of us noticed that during testing.

The bug here is a very subtle one. I had trouble with it out months ago, but then didn't notice that you'd re-introduced it. Input methods are dynamically (asynchronously) loaded when the keyboard is first initialized. So we can't call setKeyboardName() from getKeyboardSettings(). If we do that, the input method hasn't loaded yet and isn't ready, so we end up with a keyboard name and the default input method.

That's why the existing code had the call to setKeyboardName() in showKeyboard() rather than in the initialization code.

Comment 36

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

(In reply to David Flanagan [:djf] from comment #35)
> Mihai: I backed this out when I realized that it had broken the auto-correct
> and all the other features provided by the latin input method. I don't know
> why neither of us noticed that during testing.
> 
> The bug here is a very subtle one. I had trouble with it out months ago, but
> then didn't notice that you'd re-introduced it. Input methods are
> dynamically (asynchronously) loaded when the keyboard is first initialized.
> So we can't call setKeyboardName() from getKeyboardSettings(). If we do
> that, the input method hasn't loaded yet and isn't ready, so we end up with
> a keyboard name and the default input method.
> 
> That's why the existing code had the call to setKeyboardName() in
> showKeyboard() rather than in the initialization code.

Didn't realize this, the reason I called setKeyboardName() from getKeyboardSettings() was to initialize the keyboard with the value of 'keyboard.current' (upon reboot).

If I can help in any way fixing this let me know, feel free to build on top of my previous patch if you're taking it.

Comment 37

[David Flanagan [:djf]]

(In reply to Mihai Cirlanaru [:mcirlanaru] from comment #36)
> Didn't realize this, the reason I called setKeyboardName() from
> getKeyboardSettings() was to initialize the keyboard with the value of
> 'keyboard.current' (upon reboot).
> 

Understood. I'll figure out a way to keep the correct initialization, but just do it later, once the IM has loaded.

> If I can help in any way fixing this let me know, feel free to build on top
> of my previous patch if you're taking it.

Yes. My hope is to make just a small modification to your patch.

Comment 38

[David Flanagan [:djf]]

Attachment: link to patch on github

Rudy,

Mihai and I messed the first version of this Leo+ patch up, and had to back it out. So now I need a new reviewer to take a look.

The attachment is a new pull request with two commits. The first commit is Mihai's original patch and the second commit is my modification of it to initialize things correctly so that the input method works.

I'll squash them before landing but wanted to keep them separate for now because I've already given r+ to Mihai's commit, including the changes to the settings app. So it is the second commit that I need you to review.

The primary point of this bug is to change the default keyboard layout when the user switches languages in the settings app. Secondarily, we also now remember the current layout and restore it after a reboot.

My commit changes when setKeyboardName() is called.  If we call it too early, or call it right after a settings change it may not work (because the IM may not be initialized or because the desired keyboard might not be enabled yet). So I've moved the call to setKeyboardName() into showKeyboard().

Also, we used to be setting keyboard.current from renderKeyboard() which meant that we were updating every time the user switched to a symbol layout.  I've changed that so now we only update the setting when the user actually switches primary layouts.

Comment 39

[John Ford [:jhford] CET/CEST Berlin Time]

Without going through it line by line, the backout looks reasonable.

Comment 40

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

Comment on attachment 748149 [details]
link to patch on github

r=me, with some suggestions listed on pull request.
Thanks for working on this.

Comment 41

[David Flanagan [:djf]]

Landed to master: https://github.com/mozilla-b2g/gaia/commit/a1cf4de967a4a8231c4dfd34d5da118047100b31

Comment 42

[David Flanagan [:djf]]

Uplifted to v1-train: https://github.com/mozilla-b2g/gaia/commit/fbb9a3febbc81dc4c290b6a3bc095495fa19484c

Comment 43

[Angela Hubenya]

ICID: ftesetup-003