821646 - [Keyboard] Should not allow 0 keyboard layouts

Status: VERIFIED FIXED

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

Description

[Walter Chen[:ypwalter][:wachen]]

*Environment:
Phone - Unagi
https://releases.mozilla.com/b2g/    20121213 build 

*How to reproduce:
  0. turn on the phone
  1. go to Settings app
  2. go to keyboard settings
  3. unselect all keyboard layout

*Expected Result:
  You should no be able to do so.

*Actual Result:
  You see zero selected keyboard layout.

Comment 1

[David Scravaglieri [:scravag]]

Not V1 Blocker but we would tak a patch.

Comment 2

[:Margaret Leibovic]

I think the UI for this would need to change if we're going to require that one checkbox is always checked, because this would mean that the user would have to be sure to select a new keyboard layout before being able to unselect the current one.

By the way, in keyboard.js we have a fallback to a default keyboard layout, so this doesn't actually break the keyboard (although right now the default is hardcoded to 'en', see bug 828912).

Comment 3

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

Attachment: Pull Request #9032 - Don't allow no keyboard layouts selected

Comment 4

[David Flanagan [:djf]]

Comment on attachment 734448 [details]
Pull Request #9032 - Don't allow no keyboard layouts selected

I'd prefer not to take the fallback default out of keyboard.js.

The changes in settings.js seem okay, though I'm not really familiar with that app.

I'm concerned about the UX change, and am not willing to give an r+ unless we have explicit buy-in from Casey or Josh.  I'm not going to give r- either at this point.  Just resetting the flag.

Once we have clear direction from UX, please ask for review again.

I think it might be better to enhance keyboard.js with a mapping from current language to default keyboard layout. Let's treat the settings app as a way to specify additional non-default layouts. In that case, it is fine if no boxes are checked.  And keyboard.js will always use the default layout for the current language if no other layout is specified.

Comment 5

[David Flanagan [:djf]]

Josh (or Casey),

The attached patch modifies the settings app so that there is always at least one keyboard layout selected. I'm nervous about it, however, because it does this by not allowing you to de-select the last one. So if you want to switch from english to spanish, you have to first select spanish before you can de-select english. A user who did not know that might be frustrated by a button that won't deselect.

As noted in comment 2, we currently fallback on the English layout if no other layouts are selected. My proposal in comment 4 is to have a language-specific fallback.  (Or, on second thought, we could have a default layout setting that was controled by an option menu instead of a set of check boxes. Then the checkbox items become additional enabled layouts, but there would always be the default primary layout.)

What do you think?

Comment 6

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

Thanks for looking into my patch, David, I added some comments below based on my research of the problem:

(In reply to David Flanagan [:djf] from comment #4)
> I'd prefer not to take the fallback default out of keyboard.js.

This fallback is discussed in Bug 828912, however no clear direction about approaching it is described. The changes I added to the settings app will make those extra checks redundant and that was my reasoning to remove them.

I have updated the patch to not take the fallback default out of keyboard.js

> I think it might be better to enhance keyboard.js with a mapping from
> current language to default keyboard layout. Let's treat the settings app as
> a way to specify additional non-default layouts. In that case, it is fine if
> no boxes are checked.  And keyboard.js will always use the default layout
> for the current language if no other layout is specified.

If this will be the approach opted for, I will update the patch accordingly :) thanks!

Comment 7

[Josh Carpenter [:jcarpenter]]

Hey folks, let's update as follows:

* Keyboard associated with active language is always enabled. User cannot disable.
* User can add additional keyboards (per comment 14 in bug #828237).

Will attach a screen cap of the updated layout in a few minutes.

Comment 8

[Josh Carpenter [:jcarpenter]]

Attachment: Settings > Keyboard updated layout

The attached shows a modified Settings > Keyboard layout, wherein the keyboad associated with the active language is pinned to the top of the "Keyboard Layouts" list, without a enable/disable checkbox.

Comment 9

[Josh Carpenter [:jcarpenter]]

We will revisit this once we tackle third party keyboards, so this is really just a stop gap solution.

Comment 10

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

Comment on attachment 734448 [details]
Pull Request #9032 - Don't allow no keyboard layouts selected

Initially asked for review from David, but since the changes mainly concentrate on the Settings app, I redirected the review request to Fabien.

In short, my updated patch refactors the shared resource reading code to accommodate reading new relevant resources (i.e. keyboard_layouts.json), and adds a similar behavior to the one present in the Languages panel for updating the Keyboard Layouts list.

Comment 11

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

Comment on attachment 734448 [details]
Pull Request #9032 - Don't allow no keyboard layouts selected

Redirect review request to :evelyn since :kaze seems to be away until early May.

Comment 12

[Evelyn Hung]

Comment on attachment 734448 [details]
Pull Request #9032 - Don't allow no keyboard layouts selected

redirect to Arthur because I really can't find my time to review this, sorry about that. :(

Comment 13

[Evan Tseng [:evanxd]]

Hi Mihai,

I left two comments in GitHub.
You could check that.

Thanks. :)

Comment 14

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 734448 [details]
Pull Request #9032 - Don't allow no keyboard layouts selected

Mihai, thank you for the work. Please check the comments from Evan and me in github. Also, I've noticed that some of the code in this patch has been merged to gaia master already. Please update this patch accordingly.

Comment 15

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

(In reply to Arthur Chen [:arthurcc] from comment #14)
> Comment on attachment 734448 [details]
> Pull Request #9032 - Don't allow no keyboard layouts selected
> 
> Mihai, thank you for the work. Please check the comments from Evan and me in
> github. Also, I've noticed that some of the code in this patch has been
> merged to gaia master already. Please update this patch accordingly.

Evan, Arthur, thanks for the suggestions and feedback! I am currently rebasing this patch on master since some of its changes are part of the already landed patch for bug 828237, and will integrate your suggestions.

Comment 16

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

Josh, there is one case that I was made aware of during the review process which does not fit with the desired UX you highlighted in comment 7: if the currently selected language does not have a keyboard layout in the Keyboard section of the Settings app, nothing will be pinned on top of the list (cf. https://github.com/mozilla-b2g/gaia/pull/9032#discussion_r4132202)

One suggestion for addressing this case would be to pin the default keyboard (i.e. English). What do you think about this?

Comment 17

[Josh Carpenter [:jcarpenter]]

(In reply to Mihai Cirlanaru [:mcirlanaru] from comment #16)
> Josh, there is one case that I was made aware of during the review process
> which does not fit with the desired UX you highlighted in comment 7: if the
> currently selected language does not have a keyboard layout in the Keyboard
> section of the Settings app, nothing will be pinned on top of the list (cf.
> https://github.com/mozilla-b2g/gaia/pull/9032#discussion_r4132202)
> 
> One suggestion for addressing this case would be to pin the default keyboard
> (i.e. English). What do you think about this?

That makes sense to me :)

Comment 18

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

Comment on attachment 734448 [details]
Pull Request #9032 - Don't allow no keyboard layouts selected

Updated the patch based on UX feedback and discussion on GitHub.

Comment 19

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 734448 [details]
Pull Request #9032 - Don't allow no keyboard layouts selected

Mihai, thank you for the patch! It works well, however, I have concerns and would like you to make some refinements. Details please refer to github, thanks!

Comment 20

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

Comment on attachment 734448 [details]
Pull Request #9032 - Don't allow no keyboard layouts selected

Thanks for the feedback, Arthur! I refactored the solution as you suggested, let me know if it looks good now.

Comment 21

[Arthur Chen [:arthurcc][inactive after 6/30]]

Looks much better! It also works well. However, there are some nit picks need to be addressed before I can r+ it. Please check github, thanks!

Comment 22

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

(In reply to Arthur Chen [:arthurcc] from comment #21)
> Looks much better! It also works well. However, there are some nit picks
> need to be addressed before I can r+ it. Please check github, thanks!

Thanks for pointing those things out :) I made the necessary correction, have a look.

Comment 23

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 734448 [details]
Pull Request #9032 - Don't allow no keyboard layouts selected

r=me. Thank you Mihai!.

Comment 24

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

Thanks for taking the time to review this, Arthur!

Landed on master:
https://github.com/mozilla-b2g/gaia/commit/6152ab22b517a5af8639156f6174a6d96b16d61e

Comment 25

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

Comment on attachment 734448 [details]
Pull Request #9032 - Don't allow no keyboard layouts selected

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Settings > Keyboard
User impact if declined: Low (user will be able to unselect all active keyboards)
Testing completed: Yes
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: No

Comment 26

[Alex Keybl [:akeybl]]

Comment on attachment 734448 [details]
Pull Request #9032 - Don't allow no keyboard layouts selected

We'll take this fix in v1.2 for the first time - we're mostly taking blockers at this point to mitigate risk.

Comment 27

[Walter Chen[:ypwalter][:wachen]]

I think we should open a MozTrap test cases for this bug.

Comment 28

[Walter Chen[:ypwalter][:wachen]]

Verified in 2013.06.16 pvt master build in unagi
  <!-- Mercurial-Information: <project name="mozilla-central" path="gecko" remote="hgmozillaorg" revision="36da3cb92193"/> -->
  <project name="gecko.git" path="gecko" remote="mozillaorg" revision="161d0d64f7baa57bd52220ade7f189fd4e4f0dea"/>
  <project name="gaia.git" path="gaia" remote="mozillaorg" revision="141ed6da339f477523dcc932217fc0ba7a0ea68d"/>

It's interesting that we forced to have English keyboard layout on now. :P

Comment 29

[Montoya Clemmons (MClemmons)]

New Test Case created to cover bug

https://moztrap.mozilla.org/manage/cases/?filter-id=9366