MD Settings: Migrating add/edit address dropdowns to native select.

MD Settings: Migrating add/edit address dropdowns to native select.

BUG=651513
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/6472a3585610c0ae147d9f4e5b5210fa170abecb
Cr-Commit-Position: refs/heads/master@{#422854}

[dpapad, 2016-09-30 01:37:27]

Description was changed from

==========
MD Settings: Migrating add/edit address dropdowns to native select.

BUG=651513
==========

to

==========
MD Settings: Migrating add/edit address dropdowns to native select.

BUG=651513
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-09-30 22:13:07]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 22:13:44]

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

[dpapad, 2016-09-30 22:16:29]

dpapad@chromium.org changed reviewers:
+ dbeam@chromium.org

[dpapad, 2016-09-30 22:16:30]

Before/after screenshots at http://imgur.com/a/Tv4p7.

Notes:
1) Before the "Country/Region" text was a floating placeholder inside the
dropdown. After, it is just a label just like all the other address columns, but
I did not replicate the effect where it gets blue when the dropdown is focused.
Let me know if there is a CSS only way of replicating this.

2) You will notice in the screenshots a slight difference in the left padding
from before/after. This can only be fixed if "-webkit-appearance: none" is used,
so I prefer to address this along with the modification of the arrow which also
requires removing webkit appearance.

[commit-bot: I haz the power, 2016-10-01 00:24:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-01 00:24:08]

Dry run: This issue passed the CQ dry run.

[dpapad, 2016-10-03 17:22:25]

On 2016/09/30 at 22:16:30, dpapad wrote:
> Before/after screenshots at http://imgur.com/a/Tv4p7.
> 
> Notes:
> 1) Before the "Country/Region" text was a floating placeholder inside the
dropdown. After, it is just a label just like all the other address columns, but
I did not replicate the effect where it gets blue when the dropdown is focused.
Let me know if there is a CSS only way of replicating this.
> 
> 2) You will notice in the screenshots a slight difference in the left padding
from before/after. This can only be fixed if "-webkit-appearance: none" is used,
so I prefer to address this along with the modification of the arrow which also
requires removing webkit appearance.

Friendly ping.

[Dan Beam, 2016-10-03 18:27:14]

https://codereview.chromium.org/2382943002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.html
(right):

https://codereview.chromium.org/2382943002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.html:23:
select.address-column {
why does this need the "select" part?

https://codereview.chromium.org/2382943002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.html:32:
#select-row > div {
optional nit: can we just name this like #country-label or something?

"> div" is mildly a bummer because it's enforcing hierarchy and tag name

https://codereview.chromium.org/2382943002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js
(right):

https://codereview.chromium.org/2382943002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:71:
Polymer.dom.flush();
i still don't like that we have to flush() stuff to avoid a bug, that's 2 bad
things

[dpapad, 2016-10-03 21:49:53]

Addressed comments. PTAL

https://codereview.chromium.org/2382943002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.html
(right):

https://codereview.chromium.org/2382943002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.html:23:
select.address-column {
On 2016/10/03 at 18:27:14, Dan Beam wrote:
> why does this need the "select" part?

This is undoing what the rule right above is doing, only for <select>. So it
needs the "select" to only have affect on the select itself. I could drop the
".address-column" part, but I thought it was clearer to keep it.

https://codereview.chromium.org/2382943002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.html:32:
#select-row > div {
On 2016/10/03 at 18:27:14, Dan Beam wrote:
> optional nit: can we just name this like #country-label or something?
> 
> "> div" is mildly a bummer because it's enforcing hierarchy and tag name

Done.

https://codereview.chromium.org/2382943002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js
(right):

https://codereview.chromium.org/2382943002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:71:
Polymer.dom.flush();
On 2016/10/03 at 18:27:14, Dan Beam wrote:
> i still don't like that we have to flush() stuff to avoid a bug, that's 2 bad
things

Changed to this.async(). BTW, this file is already calling Polymer.dom.flush()
at line 105, perhaps this can be removed too (in a future cleanup).

[Dan Beam, 2016-10-03 22:01:57]

lgtm

https://codereview.chromium.org/2382943002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js
(right):

https://codereview.chromium.org/2382943002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:192:
this.countryCode_ = countrySelect.selectedOptions[0].value;
can't you just ask for countrySelect.value?
https://jsfiddle.net/d1pg525k/

[dpapad, 2016-10-03 22:15:56]

https://codereview.chromium.org/2382943002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js
(right):

https://codereview.chromium.org/2382943002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:192:
this.countryCode_ = countrySelect.selectedOptions[0].value;
On 2016/10/03 at 22:01:57, Dan Beam wrote:
> can't you just ask for countrySelect.value?
> https://jsfiddle.net/d1pg525k/

Done. I just think this is counter-intuitive
select#value is bound to countryCode_ in HTML (one-way binding direction
countryCode_ => select#value). Now in JS we assign select#value to countryCode_,
which is the reverse direction (select#value => countryCode_). It still works,
but confusing IMO (and yes, the confusion originally comes from the fact that
two-way binding does not work for select#value).

[dpapad, 2016-10-03 23:42:02]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-10-03 23:42:02]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2382943002/#ps100001
(title: "Addressing comment.")

[commit-bot: I haz the power, 2016-10-03 23:42:55]

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-10-04 02:38:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-04 02:38:57]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dpapad, 2016-10-04 17:03:19]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-10-04 17:03:41]

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-10-04 17:47:49]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-04 17:51:11]

Description was changed from

==========
MD Settings: Migrating add/edit address dropdowns to native select.

BUG=651513
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Migrating add/edit address dropdowns to native select.

BUG=651513
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/6472a3585610c0ae147d9f4e5b5210fa170abecb
Cr-Commit-Position: refs/heads/master@{#422854}
==========

[commit-bot: I haz the power, 2016-10-04 17:51:12]

Patchset 6 (id:??) landed as
https://crrev.com/6472a3585610c0ae147d9f4e5b5210fa170abecb
Cr-Commit-Position: refs/heads/master@{#422854}