MD Settings: Allow the user to order the list of languages.

MD Settings: Allow the user to order the list of languages.

BUG=614382
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/04d3fdc643cd42e4734e59584b0c75daa4481cb5
Cr-Commit-Position: refs/heads/master@{#402464}

[Moe, 2016-06-17 19:38:44]

Description was changed from

==========
MD Settings: Allow the user to order the list of languages.

BUG=614382
==========

to

==========
MD Settings: Allow the user to order the list of languages.

BUG=614382
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Moe, 2016-06-17 19:39:29]

mahmadi@chromium.org changed reviewers:
+ michaelpg@chromium.org

[Moe, 2016-06-17 19:39:30]

Hi Michael, Please review this CL.

[Moe, 2016-06-20 17:44:50]

On 2016/06/17 19:39:30, Moe wrote:
> Hi Michael, Please review this CL.

Fixed the broken tests.

[Moe, 2016-06-20 17:44:50]

On 2016/06/17 19:39:30, Moe wrote:
> Hi Michael, Please review this CL.

Fixed the broken tests.

[michaelpg, 2016-06-21 00:34:43]

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages.js (right):

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages.js:568:
this.languageSettingsPrivate.setLanguageList(languageCodes);
there's a bug(?) here:
https://cs.chromium.org/chromium/src/components/translate/core/browser/transl...

which will turn this:
  ['en-US', 'es', 'en']
into this:
  ['en-US', 'en', 'es']
(which seems like undesired behavior, thus a bug)

test this by having these languages enabled:
  * English (United States
  * English
  * Spanish

and trying to move Spanish up one.

apparently, old Options doesn't quite update until you refresh the page, so
reordering *seems* to work but doesn't (try it).

in Settings, the result is that nothing happens (we never get a notification
from prefs, because the pref doesn't change). for some reason, we aren't closing
the menu unless we re-create the list (because the items change), so the effect
is that nothing happens and the menu stays open.

do you agree the old Options behavior is buggy? seems like something we should
address in a separate CL, but it makes for a weird bug in Settings.

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages_page.html
(right):

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.html:85:
<iron-dropdown opened="{{item.optionsMenuOpened}}"
I noticed some issues here, but they exist elsewhere in MD Settings, so I opened
crbug.com/621731. We'll update this if we find solutions for that.

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.html:88:
<paper-item on-tap="onMoveUpTap_" id="moveUp"
an id in a dom-repeat doesn't make sense (it'll be duplicated, right?)

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.html:93:
<paper-item on-tap="onMoveDownTap_" id="moveDown"
id?

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.html:95:
languages.enabled.*)]]">
align as on 90

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.html:98:
<paper-item id="languageDetail"
id?

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages_page.js (right):

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.js:105: *    
gets called whenever languages.enabled.* changes).
doesn't |index| change when the list changes?

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.js:119:
onMoveUpTap_: function(e) {
nit: put this above onMoveDownTap_

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.js:136: * @param
{!Object} change Polymer change object (provided in the HTML so this
meh, feel free to omit this param altogether, if not though, no need for this
parenthetical.

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:644:
base::ASCIIToUTF16(chrome::kLanguageSettingsLearnMoreUrl));
fyi, filed crbug.com/621723

[Moe, 2016-06-21 20:10:20]

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages.js (right):

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages.js:568:
this.languageSettingsPrivate.setLanguageList(languageCodes);
On 2016/06/21 00:34:42, michaelpg wrote:
> there's a bug(?) here:
>
https://cs.chromium.org/chromium/src/components/translate/core/browser/transl...
> 
> which will turn this:
>   ['en-US', 'es', 'en']
> into this:
>   ['en-US', 'en', 'es']
> (which seems like undesired behavior, thus a bug)
> 
> test this by having these languages enabled:
>   * English (United States
>   * English
>   * Spanish
> 
> and trying to move Spanish up one.
> 
> apparently, old Options doesn't quite update until you refresh the page, so
> reordering *seems* to work but doesn't (try it).
> 
> in Settings, the result is that nothing happens (we never get a notification
> from prefs, because the pref doesn't change). for some reason, we aren't
closing
> the menu unless we re-create the list (because the items change), so the
effect
> is that nothing happens and the menu stays open.
> 
> do you agree the old Options behavior is buggy? seems like something we should
> address in a separate CL, but it makes for a weird bug in Settings.

I confirm this behavior. We may need to keep expanding the language codes after
all though. Is it possible to show the cros preferred languages pref as the list
of languages on desktop too? that one get saved without modification.

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages_page.html
(right):

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.html:85:
<iron-dropdown opened="{{item.optionsMenuOpened}}"
On 2016/06/21 00:34:42, michaelpg wrote:
> I noticed some issues here, but they exist elsewhere in MD Settings, so I
opened
> crbug.com/621731. We'll update this if we find solutions for that.

Acknowledged. I've seen the gray focus ring in MD User Manager too. I CC'ed
myself to the bug to stay tuned.

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.html:88:
<paper-item on-tap="onMoveUpTap_" id="moveUp"
On 2016/06/21 00:34:42, michaelpg wrote:
> an id in a dom-repeat doesn't make sense (it'll be duplicated, right?)

Yep. this is wrong. Changed it to "moveUp-[[item.language.code]]"

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.html:93:
<paper-item on-tap="onMoveDownTap_" id="moveDown"
On 2016/06/21 00:34:42, michaelpg wrote:
> id?

Done.

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.html:95:
languages.enabled.*)]]">
On 2016/06/21 00:34:42, michaelpg wrote:
> align as on 90

Done.

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.html:98:
<paper-item id="languageDetail"
On 2016/06/21 00:34:42, michaelpg wrote:
> id?

Done.

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages_page.js (right):

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.js:105: *    
gets called whenever languages.enabled.* changes).
On 2016/06/21 00:34:42, michaelpg wrote:
> doesn't |index| change when the list changes?

Not really. It may be a Polymer bug. It seems like functions bound to |index|
get called only once when an item is added (or at the initial construction of
the repeater). the documentation also hints that index gets updated when the
array is sorted or filtered. It doesn't say anything about the index getting
updated when the array changes:
https://www.polymer-project.org/1.0/docs/devguide/templates

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.js:119:
onMoveUpTap_: function(e) {
On 2016/06/21 00:34:42, michaelpg wrote:
> nit: put this above onMoveDownTap_

Done.

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.js:136: * @param
{!Object} change Polymer change object (provided in the HTML so this
On 2016/06/21 00:34:42, michaelpg wrote:
> meh, feel free to omit this param altogether, if not though, no need for this
> parenthetical.

Done.

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:644:
base::ASCIIToUTF16(chrome::kLanguageSettingsLearnMoreUrl));
On 2016/06/21 00:34:42, michaelpg wrote:
> fyi, filed crbug.com/621723

Acknowledged.

[michaelpg, 2016-06-22 01:06:27]

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages_page.html
(right):

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.html:88:
<paper-item on-tap="onMoveUpTap_" id="moveUp"
On 2016/06/21 20:10:19, Moe wrote:
> On 2016/06/21 00:34:42, michaelpg wrote:
> > an id in a dom-repeat doesn't make sense (it'll be duplicated, right?)
> 
> Yep. this is wrong. Changed it to "moveUp-[[item.language.code]]"

Looks like we only use these IDs for testing, and even there we only use
"#languageDetail-en-US". Seems unnecessary to dynamically generate all of these
in that case. Can we remove these and just put a "languageDetailItem" class on
the third <paper-item>?

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages_page.js (right):

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.js:105: *    
gets called whenever languages.enabled.* changes).
On 2016/06/21 20:10:19, Moe wrote:
> On 2016/06/21 00:34:42, michaelpg wrote:
> > doesn't |index| change when the list changes?
> 
> Not really. It may be a Polymer bug. It seems like functions bound to |index|
> get called only once when an item is added (or at the initial construction of
> the repeater). the documentation also hints that index gets updated when the
> array is sorted or filtered. It doesn't say anything about the index getting
> updated when the array changes:
> https://www.polymer-project.org/1.0/docs/devguide/templates

TIL, thanks.

[Moe, 2016-06-27 15:00:06]

sorry for the late reply. I was OOO.

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages_page.html
(right):

https://codereview.chromium.org/2077063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.html:88:
<paper-item on-tap="onMoveUpTap_" id="moveUp"
On 2016/06/22 01:06:27, michaelpg wrote:
> On 2016/06/21 20:10:19, Moe wrote:
> > On 2016/06/21 00:34:42, michaelpg wrote:
> > > an id in a dom-repeat doesn't make sense (it'll be duplicated, right?)
> > 
> > Yep. this is wrong. Changed it to "moveUp-[[item.language.code]]"
> 
> Looks like we only use these IDs for testing, and even there we only use
> "#languageDetail-en-US". Seems unnecessary to dynamically generate all of
these
> in that case. Can we remove these and just put a "languageDetailItem" class on
> the third <paper-item>?

Done.

[michaelpg, 2016-06-27 22:24:01]

lgtm

https://codereview.chromium.org/2077063002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/languages_page_browsertest.js (right):

https://codereview.chromium.org/2077063002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/languages_page_browsertest.js:86: var
languageDetailMenuItem = languagesCollapse.querySelectorAll(
querySelector(...) is equivalent to querySelectorAll(...)[0] when there's at
least one item to find

[Moe, 2016-06-28 14:28:23]

Thank you.

https://codereview.chromium.org/2077063002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/languages_page_browsertest.js (right):

https://codereview.chromium.org/2077063002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/languages_page_browsertest.js:86: var
languageDetailMenuItem = languagesCollapse.querySelectorAll(
On 2016/06/27 22:24:01, michaelpg wrote:
> querySelector(...) is equivalent to querySelectorAll(...)[0] when there's at
> least one item to find

yes. I meant to make it clear to readers of these tests that there are multiple
items to choose from. It wasn't very clear to me when I first read the test.

[Moe, 2016-06-28 14:28:34]

The CQ bit was checked by mahmadi@chromium.org

[commit-bot: I haz the power, 2016-06-28 14:29:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 14:43:43]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-28 14:45:27]

Description was changed from

==========
MD Settings: Allow the user to order the list of languages.

BUG=614382
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Allow the user to order the list of languages.

BUG=614382
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/04d3fdc643cd42e4734e59584b0c75daa4481cb5
Cr-Commit-Position: refs/heads/master@{#402464}
==========

[commit-bot: I haz the power, 2016-06-28 14:45:29]

Patchset 5 (id:??) landed as
https://crrev.com/04d3fdc643cd42e4734e59584b0c75daa4481cb5
Cr-Commit-Position: refs/heads/master@{#402464}