Bug 821646 - [Keyboard] Don't allow 0 selected keyboard layouts #9032

Merged
merged 1 commit into from May 22, 2013

Projects

None yet

4 participants

@mihai
Mozilla-B2G member

No description provided.

@davidflanagan davidflanagan commented on an outdated diff Apr 8, 2013
apps/keyboard/js/keyboard.js
@@ -458,12 +454,6 @@ function handleNewKeyboards() {
keyboardGroups[group]);
}
- // If no keyboards were selected, use a default
- if (enabledKeyboardNames.length === 0)
- Array.prototype.push.apply(enabledKeyboardNames,
- defaultKeyboardNames);
-
@davidflanagan
davidflanagan Apr 8, 2013

I'd prefer not to take this default code out of keyboard.js. It just seems safer to leave it in.

@evanxd evanxd and 2 others commented on an outdated diff May 8, 2013
apps/settings/js/settings.js
@@ -831,6 +909,22 @@ window.addEventListener('localized', function showLanguages() {
languages[navigator.mozL10n.language.code];
});
Settings.updateLanguagePanel();
+
+ // update the keyboard layouts list by resetting the top pinned element,
+ // since it displays the previous language setting
+ var kbLayoutsList = document.getElementById('keyboard-layouts');
@evanxd
evanxd May 8, 2013

Hi Mihai,

When Settings App startup, the keyboard section is in the comment tag.
So we couldn't get the "keyboard-layouts" element.

And the following code couldn't work at first time.
We should handle this.

Thanks. :)

@crh0716
crh0716 May 9, 2013

If users never enter the keyboard panel, kbLayoutsList is null. Please add a null check here.

@mihai
mihai May 9, 2013

Good point! I will add a check for kbLayoutsList here.

@evanxd evanxd and 1 other commented on an outdated diff May 8, 2013
apps/settings/js/settings.js
+ var langKeyboard = keyboards[currentLang];
+
+ var kbLayout = kbLayoutsList.children;
+ for (var i = kbLayout.length - 1; i >= 0; i--) {
+ var li = kbLayout[i];
+ var selector = 'input[name^="keyboard.layouts."]';
+ var inputNode = li.querySelector(selector);
+
+ if (inputNode) {
+ var layout = inputNode.name;
+ var keyboardLayoutName =
+ layout.slice(layout.lastIndexOf('.') + 1);
+ // If the current element in the Keyboard Layouts list matches the
+ // current language, remove it from its position in the list and
+ // pin it at the top
+ if (keyboardLayoutName == langKeyboard) {
@evanxd
evanxd May 8, 2013

Hi Mihai,

We should handle the language of keyboard wasn't supported in the Keyboard settings.
If we didn't do that, user still could disable all keyboard layouts.

For example, user change the system language to zh-TW(Chinese), and user can disable all Keyboard layouts.
Because there is NO settings for the zh-TW(zhuyin) keyboard layout in the Keyboard section of Settings App.

So we should make sure the UX design of this case to know how to handle it.
Maybe we could always change to English layout when the language is not supported in the Keyboard section of Settings App. Just maybe. :p

Thanks. :)

@mihai
mihai May 9, 2013

Hi Evan,

Thanks for pointing this out! I just checked the particular case you were pointing out and, although the zh-TW(zhuyin) keyboard layout exist, it is indeed not in the Keyboard section of the Settings App.

I think for such cases a fallback to the default keyboard (i.e. English) should be sufficient. I will nevertheless ask Josh from UX to confirm this and update accordingly.

@mihai
mihai May 17, 2013

Josh just confirmed this so I updated the patch accordingly.

@crh0716 crh0716 commented on an outdated diff May 17, 2013
apps/settings/js/settings.js
+
+ updateKeyboardPanel: function settings_updateKeyboardPanel() {
+ var panel = document.getElementById('keyboard');
+ // Update the keyboard layouts list from the Keyboard panel
+ if (panel) {
+ this.getSupportedKbLayouts(function updateKbList(keyboards) {
+ var kbLayoutsList = panel.querySelector('#keyboard-layouts');
+ var langKeyboardNode = document.createElement('li');
+
+ // Get the current language and its associate keyboard layout
+ var currentLang = document.documentElement.lang;
+ var langKeyboard = keyboards[currentLang];
+
+ var kbLayout = kbLayoutsList.children;
+ var foundKbLayout = false;
+ for (var i = kbLayout.length - 1; i >= 0 && !foundKbLayout; i--) {
@crh0716
crh0716 May 17, 2013

What the for loop does here could be replaced by a simple css query. (i.e. querySelector('input[name="keyboard.layouts.' + langKeyboard + '"]') ) If nothing found by this query, use keyboard.layouts.english to query again. Once the input node is found, you can then pin the new keyboardNode to the top and hide the existing one, which also avoid some code dups.

@crh0716 crh0716 commented on an outdated diff May 17, 2013
apps/settings/js/settings.js
+ liChildren.forEach(function(node) {
+ if (node.nodeName !== 'LABEL')
+ langKeyboardNode.appendChild(node);
+ });
+ // Remove the entry from the list since it will be pinned on top
+ // of the Keyboard Layouts list
+ li.classList.add('hidden');
+ kbLayoutsList.insertBefore(langKeyboardNode,
+ kbLayoutsList.firstChild);
+ foundKbLayout = true;
+ }
+ }
+ }
+ // If the current language does not have an associated keyboard,
+ // fallback to the default keyboard: 'en'
+ if (!foundKbLayout) {
@crh0716
crh0716 May 17, 2013

There is a code dup here. As described above.

@crh0716 crh0716 and 1 other commented on an outdated diff May 17, 2013
apps/settings/js/settings.js
+ var foundKbLayout = false;
+ for (var i = kbLayout.length - 1; i >= 0 && !foundKbLayout; i--) {
+ var li = kbLayout[i];
+ var selector = 'input[name^="keyboard.layouts."]';
+ var inputNode = li.querySelector(selector);
+
+ if (inputNode) {
+ var layout = inputNode.name;
+ var keyboardLayoutName =
+ layout.slice(layout.lastIndexOf('.') + 1);
+ // If the current element in the Keyboard Layouts list matches the
+ // current language, remove it from its position in the list and
+ // pin it at the top
+ if (keyboardLayoutName == langKeyboard) {
+ var liChildren = Array.prototype.slice.call(li.children);
+ liChildren.forEach(function(node) {
@crh0716
crh0716 May 17, 2013

I would suggest simply create a fixed li in index.html for displaying the pinned layout. Then we can get rid of expensive DOM manipulations here.

@mihai
mihai May 17, 2013

That is indeed a simpler approach :) I don't know why I jumped directly in this solution. I will update accordingly, thanks!

@crh0716 crh0716 and 1 other commented on an outdated diff May 17, 2013
apps/settings/js/settings.js
+ if (inputNode) {
+ var layout = inputNode.name;
+ var keyboardLayoutName =
+ layout.slice(layout.lastIndexOf('.') + 1);
+ // If the current element in the Keyboard Layouts list matches the
+ // current language, remove it from its position in the list and
+ // pin it at the top
+ if (keyboardLayoutName == langKeyboard) {
+ var liChildren = Array.prototype.slice.call(li.children);
+ liChildren.forEach(function(node) {
+ if (node.nodeName !== 'LABEL')
+ langKeyboardNode.appendChild(node);
+ });
+ // Remove the entry from the list since it will be pinned on top
+ // of the Keyboard Layouts list
+ li.classList.add('hidden');
@crh0716
crh0716 May 17, 2013

Please use li.hidden = true;, then we can avoid adding a new style ul li.hidden.

@mihai
mihai May 17, 2013

I had to use this workaround with CSS classes instead of hidden because of performing DOM manipulations: when showing the previously hidden list elements the labels were not there anymore (couldn't figure why, but the use of CSS classes fixed it)

@crh0716
crh0716 May 20, 2013

Do you mean the list elements are not in the DOM tree? The elements should be able to be queried using li[hidden] selector.

@crh0716 crh0716 commented on an outdated diff May 17, 2013
apps/settings/style/lists.css
@@ -47,6 +47,10 @@ ul li.disabled > a {
pointer-events: none;
}
+ul li.hidden {
@crh0716
crh0716 May 17, 2013

As the comment above, the style is not needed.

@crh0716

I checked the commit history and found the keyboard related codes were committed recently. IMHO I think these code should not be in settings.js because they are not executed until users enter the keyboard subpanel. I would suggest create a new keyboard.js loaded when users enter the keyboard panel, and move all related code (including those commited earlier) into this file.

Sorry I did not notice this when I review this patch last time, would you mind refine the patch again? Thanks!

@mihai
Mozilla-B2G member

@crh0716, the language selection panel also has quite a bit of code that is in settings.js, that is why I included this code and previous keyboard/language settings changes there.

I think with the code simplifications you are suggesting in the diff the amount of keyboard settings code will not be that much. Nevertheless, if you think a separate keyboard_settings.js and/or language_settings.js would still be recommended, feel free to file a bug on it and assign it to me. I would be happy to take it. Thanks!

@crh0716

Please request a review once you finish the refinement. Let's land this patch at first. I will file a bug for the refinement. Thank you for the effort!

@crh0716 crh0716 commented on an outdated diff May 21, 2013
apps/settings/index.html
@@ -1472,7 +1472,11 @@ <h1 data-l10n-id="keyboard"> Keyboard </h1>
<header>
<h2 data-l10n-id="keyboardLayouts">Layouts</h2>
</header>
- <ul>
+ <ul id="keyboard-layouts">
+ <li id="language-keyboard">
@crh0716
crh0716 May 21, 2013

nit: The indent is incorrect.

@crh0716 crh0716 commented on an outdated diff May 21, 2013
apps/settings/js/settings.js
@@ -603,6 +603,54 @@ var Settings = {
function callback() {
self._panelStylesheetsLoaded = true;
});
+ },
+
+ updateKeyboardPanel: function settings_updateKeyboardPanel() {
+ var panel = document.getElementById('keyboard');
+ // Update the keyboard layouts list from the Keyboard panel
+ if (panel) {
+ this.getSupportedKbLayouts(function updateKbList(keyboards) {
+ var kbLayoutsList = panel.querySelector('#keyboard-layouts');
@crh0716
crh0716 May 21, 2013

Please use getElementById if the selector contains only an ID.

@crh0716 crh0716 commented on an outdated diff May 21, 2013
apps/settings/js/settings.js
@@ -603,6 +603,54 @@ var Settings = {
function callback() {
self._panelStylesheetsLoaded = true;
});
+ },
+
+ updateKeyboardPanel: function settings_updateKeyboardPanel() {
+ var panel = document.getElementById('keyboard');
+ // Update the keyboard layouts list from the Keyboard panel
+ if (panel) {
+ this.getSupportedKbLayouts(function updateKbList(keyboards) {
+ var kbLayoutsList = panel.querySelector('#keyboard-layouts');
+ // Get pointers to the top list entry and its labels which are used to
+ // pin the language associated keyboard at the top of the keyboards list
+ var pinnedKb = panel.querySelector('#language-keyboard');
@crh0716 crh0716 commented on an outdated diff May 21, 2013
apps/settings/js/settings.js
+
+ var label = kbListEntry.querySelector('a');
+ var sub = kbListEntry.querySelector('small');
+ pinnedKbLabel.dataset.l10nId = label.dataset.l10nId;
+ pinnedKbLabel.textContent = label.textContent;
+ if (sub) {
+ pinnedKbSubLabel.dataset.l10nId = sub.dataset.l10nId;
+ pinnedKbSubLabel.textContent = sub.textContent;
+ }
+ } else {
+ // If the current language does not have an associated keyboard,
+ // fallback to the default keyboard: 'en'
+ // XXX update this if the list order in index.html changes
+ var englishEntry = kbLayoutsList.children[1];
+ englishEntry.hidden = true;
+ //englishEntry.classList.add('hidden');
@crh0716
crh0716 May 21, 2013

Is this a real comment?

@mihai mihai Bug 821646 - [Settings] Pin the language associated keyboard at the t…
…op of the keyboards list. r=arthurcc

Make the keyboard associated with the current language always enabled by pinning
it at the top of the Keyboard Layouts list. If the current language doesn't have
a keyboard layout in the list from settings, the English keyboard is pinned.
3405557
@mihai mihai merged commit 6152ab2 into mozilla-b2g:master May 22, 2013

1 check failed

Details default The Travis CI build failed
@mihai mihai deleted the mihai:bug_821646 branch Jun 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment