Walter Chen[:ypwalter][:wachen] Reporter
	Description • 12 years ago

*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.

	David Scravaglieri [:scravag]
	Comment 1 • 12 years ago

Not V1 Blocker but we would tak a patch.

	:Margaret Leibovic
	Comment 2 • 12 years ago

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).

	David Flanagan [:djf]
	Comment 4 • 12 years ago

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.

	David Flanagan [:djf]
	Comment 5 • 12 years ago

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?

	Mihai Cirlanaru [:mihai][:mcirlanaru] Assignee
	Comment 6 • 12 years ago

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!

	Josh Carpenter [:jcarpenter]
	Comment 7 • 12 years ago

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.

	Josh Carpenter [:jcarpenter]
	Comment 8 • 12 years ago

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.

	Josh Carpenter [:jcarpenter]
	Comment 9 • 12 years ago

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

	Mihai Cirlanaru [:mihai][:mcirlanaru] Assignee
	Comment 10 • 12 years ago

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.

	Mihai Cirlanaru [:mihai][:mcirlanaru] Assignee
	Comment 11 • 12 years ago

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.

	Evelyn Hung
	Comment 12 • 12 years ago

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. :(

	Evan Tseng [:evanxd]
	Comment 13 • 12 years ago

Hi Mihai,

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

Thanks. :)

	Arthur Chen [:arthurcc][inactive after 6/30]
	Comment 14 • 12 years ago

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.

	Mihai Cirlanaru [:mihai][:mcirlanaru] Assignee
	Comment 15 • 12 years ago

(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.

	Mihai Cirlanaru [:mihai][:mcirlanaru] Assignee
	Comment 16 • 12 years ago

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?

	Josh Carpenter [:jcarpenter]
	Comment 17 • 12 years ago

(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 :)

	Mihai Cirlanaru [:mihai][:mcirlanaru] Assignee
	Comment 18 • 12 years ago

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.

	Arthur Chen [:arthurcc][inactive after 6/30]
	Comment 19 • 12 years ago

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!

	Mihai Cirlanaru [:mihai][:mcirlanaru] Assignee
	Comment 20 • 12 years ago

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.

	Arthur Chen [:arthurcc][inactive after 6/30]
	Comment 21 • 12 years ago

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!

	Mihai Cirlanaru [:mihai][:mcirlanaru] Assignee
	Comment 22 • 12 years ago

(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.

	Arthur Chen [:arthurcc][inactive after 6/30]
	Comment 23 • 12 years ago

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

r=me. Thank you Mihai!.

	Mihai Cirlanaru [:mihai][:mcirlanaru] Assignee
	Comment 24 • 12 years ago

Thanks for taking the time to review this, Arthur!

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

	Mihai Cirlanaru [:mihai][:mcirlanaru] Assignee
	Comment 25 • 11 years ago

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

	Alex Keybl [:akeybl]
	Comment 26 • 11 years ago

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.

	Walter Chen[:ypwalter][:wachen] Reporter
	Comment 27 • 11 years ago

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

	Walter Chen[:ypwalter][:wachen] Reporter
	Comment 28 • 11 years ago

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

	Montoya Clemmons (MClemmons)
	Comment 29 • 11 years ago

New Test Case created to cover bug

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