settings-dropdown-menu must set <select>#value after its options change

settings-dropdown-menu must set <select>#value after its options change

The |value| property of an HTMLSelectElement is a getter/setter, not a POD
variable. Setting it performs an algorithm which is something like:

  1. Let O be the first <option> in my children whose "property" attribute
     equals |value|.
  2. If O exists, add the "selected" attribute to O.

As a result, a vanilla Polymer binding is not safe if the contents are
dynamically generated:

  <select value="[[myValue]]">
    <template is="dom-repeat" items="[[myItems]]">
      <option value="[[item.value]]">[[item.name]]</option>
    </template>
  </select>

because if |myValue| already has a particular value, and then |myItems| is
set, |myValue| has the correct value but the <select>'s |value| "setter"
is not called. Thus the option is never actually selected.

Fix this by setting |value| directly instead of in a data binding.

Also, ensure "Custom" can't be selected when hidden via type-ahead.

BUG=658075
R=dpapad@chromium.org
TEST=CrSettingsDropdownMenu browser test
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/b02d4b3efb575bb85e49404940a84cc616a48388
Cr-Commit-Position: refs/heads/master@{#427792}

[michaelpg, 2016-10-25 00:24:24]

Description was changed from

==========
settings-dropdown-menu must set <select>#value after its options change

The |value| property of an HTMLSelectElement is a getter/setter, not a POD
variable. Setting it performs an algorithm which is something like:

  1. Let O be the first <option> in my children whose "property" attribute
     equals |value|.
  2. If O exists, add the "selected" attribute to O.

As a result, a vanilla Polymer binding is not safe if the contents are
dynamically generated:

  <select value="[[myValue]]">
    <template is="dom-repeat" items="[[myItems]]">
      <option value="[[item.value]]">[[item.name]]</option>
    </template>
  </select>

because if |myValue| already has a particular value, and then |myItems| is
set, |myValue| has the correct value but the <select>'s |value| "setter"
is not called. Thus the option is never actually selected.

Fix this by setting |value| directly instead of in a data binding.

BUG=658075
R=dpapad@chromium.org
TEST=CrSettingsDropdownMenu browser test
==========

to

==========
settings-dropdown-menu must set <select>#value after its options change

The |value| property of an HTMLSelectElement is a getter/setter, not a POD
variable. Setting it performs an algorithm which is something like:

  1. Let O be the first <option> in my children whose "property" attribute
     equals |value|.
  2. If O exists, add the "selected" attribute to O.

As a result, a vanilla Polymer binding is not safe if the contents are
dynamically generated:

  <select value="[[myValue]]">
    <template is="dom-repeat" items="[[myItems]]">
      <option value="[[item.value]]">[[item.name]]</option>
    </template>
  </select>

because if |myValue| already has a particular value, and then |myItems| is
set, |myValue| has the correct value but the <select>'s |value| "setter"
is not called. Thus the option is never actually selected.

Fix this by setting |value| directly instead of in a data binding.

BUG=658075
R=dpapad@chromium.org
TEST=CrSettingsDropdownMenu browser test
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-10-25 00:27:32]

Description was changed from

==========
settings-dropdown-menu must set <select>#value after its options change

The |value| property of an HTMLSelectElement is a getter/setter, not a POD
variable. Setting it performs an algorithm which is something like:

  1. Let O be the first <option> in my children whose "property" attribute
     equals |value|.
  2. If O exists, add the "selected" attribute to O.

As a result, a vanilla Polymer binding is not safe if the contents are
dynamically generated:

  <select value="[[myValue]]">
    <template is="dom-repeat" items="[[myItems]]">
      <option value="[[item.value]]">[[item.name]]</option>
    </template>
  </select>

because if |myValue| already has a particular value, and then |myItems| is
set, |myValue| has the correct value but the <select>'s |value| "setter"
is not called. Thus the option is never actually selected.

Fix this by setting |value| directly instead of in a data binding.

BUG=658075
R=dpapad@chromium.org
TEST=CrSettingsDropdownMenu browser test
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
settings-dropdown-menu must set <select>#value after its options change

The |value| property of an HTMLSelectElement is a getter/setter, not a POD
variable. Setting it performs an algorithm which is something like:

  1. Let O be the first <option> in my children whose "property" attribute
     equals |value|.
  2. If O exists, add the "selected" attribute to O.

As a result, a vanilla Polymer binding is not safe if the contents are
dynamically generated:

  <select value="[[myValue]]">
    <template is="dom-repeat" items="[[myItems]]">
      <option value="[[item.value]]">[[item.name]]</option>
    </template>
  </select>

because if |myValue| already has a particular value, and then |myItems| is
set, |myValue| has the correct value but the <select>'s |value| "setter"
is not called. Thus the option is never actually selected.

Fix this by setting |value| directly instead of in a data binding.

Also, ensure "Custom" can't be selected when hidden via type-ahead.

BUG=658075
R=dpapad@chromium.org
TEST=CrSettingsDropdownMenu browser test
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-10-25 00:31:32]

PTAL.

[dpapad, 2016-10-25 01:02:38]

https://codereview.chromium.org/2446003002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/settings_dropdown_menu.html
(right):

https://codereview.chromium.org/2446003002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:18:
hidden$="[[!showNotFoundValue_(menuOptions, pref.value)]]"
showNotFoundValue is performing an O(n) search, for every element in the array,
which means O(n^2). It is also called twice per element so basically O(2*n^2).
Can we do something smarter than Array#find(). Some lists are pretty big, for
example the fonts lists.

https://codereview.chromium.org/2446003002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/settings_dropdown_menu.js
(right):

https://codereview.chromium.org/2446003002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/settings_dropdown_menu.js:99:
this.$.dropdownMenu.value = option == undefined ?
Instead of removing the 1-way binding and directly assigning to |value|, could
we do a trick as described at
https://www.polymer-project.org/1.0/docs/devguide/model-data#override-dirty-c...
Essentially force the 1-way binding to fire even if the |selected_| value did
not actually change?

[dpapad, 2016-10-25 01:13:32]

https://codereview.chromium.org/2446003002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/settings_dropdown_menu.html
(right):

https://codereview.chromium.org/2446003002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:18:
hidden$="[[!showNotFoundValue_(menuOptions, pref.value)]]"
On 2016/10/25 at 01:02:38, dpapad wrote:
> showNotFoundValue is performing an O(n) search, for every element in the
array, which means O(n^2). It is also called twice per element so basically
O(2*n^2). Can we do something smarter than Array#find(). Some lists are pretty
big, for example the fonts lists.

Ah, just noticing, this <option> is outside of the dom-repeat, so disregard my
comment about n^2.

[michaelpg, 2016-10-25 01:14:37]

https://codereview.chromium.org/2446003002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/settings_dropdown_menu.html
(right):

https://codereview.chromium.org/2446003002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:18:
hidden$="[[!showNotFoundValue_(menuOptions, pref.value)]]"
On 2016/10/25 01:02:38, dpapad wrote:
> showNotFoundValue is performing an O(n) search, for every element in the
array,
> which means O(n^2). It is also called twice per element so basically O(2*n^2).
> Can we do something smarter than Array#find(). Some lists are pretty big, for
> example the fonts lists.

The search is O(n) and happens:
  twice whenever menuOptions is set (once for hidden, once for disabled)
  twice whenever pref.value is set (once for hidden, once for disabled)

Is the dom-repeat above this option throwing you off, or am I missing something?

I'm afraid that smarter things, like using selectedIndex, risk hitting edge
cases when menuOptions changes.

https://codereview.chromium.org/2446003002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/settings_dropdown_menu.js
(right):

https://codereview.chromium.org/2446003002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/settings_dropdown_menu.js:99:
this.$.dropdownMenu.value = option == undefined ?
On 2016/10/25 01:02:38, dpapad wrote:
> Instead of removing the 1-way binding and directly assigning to |value|, could
> we do a trick as described at
>
https://www.polymer-project.org/1.0/docs/devguide/model-data#override-dirty-c...
> Essentially force the 1-way binding to fire even if the |selected_| value did
> not actually change?

We could, but I'm not sure why we would. Either way, we need to set something to
the value of this expression; with the dirty check trick, we have to do another
set, and we have to bind that value to the <select> -- it doesn't gain us
anything if we have to enforce the binding manually anyhow.

I also prefer saying "Binding to <select> is dangerous, don't do it" rather than
"Binding to <select> is dangerous, so make sure you use this trick when you do
it."

[dpapad, 2016-10-25 01:18:09]

https://codereview.chromium.org/2446003002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/settings_dropdown_menu.html
(right):

https://codereview.chromium.org/2446003002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:19:
disabled="[[!showNotFoundValue_(menuOptions, pref.value)]]">
So to avoid calculating the same boolean twice, can we use CSS for this? See
example at https://jsfiddle.net/5bt02py5/1/

[dpapad, 2016-10-25 01:30:48]

LGTM with minor nit about not calculating the same boolean twice.

https://codereview.chromium.org/2446003002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/settings_dropdown_menu.js
(right):

https://codereview.chromium.org/2446003002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/settings_dropdown_menu.js:99:
this.$.dropdownMenu.value = option == undefined ?
On 2016/10/25 at 01:14:37, michaelpg wrote:
> On 2016/10/25 01:02:38, dpapad wrote:
> > Instead of removing the 1-way binding and directly assigning to |value|,
could
> > we do a trick as described at
> >
https://www.polymer-project.org/1.0/docs/devguide/model-data#override-dirty-c...
> > Essentially force the 1-way binding to fire even if the |selected_| value
did
> > not actually change?
> 
> We could, but I'm not sure why we would. Either way, we need to set something
to the value of this expression; with the dirty check trick, we have to do
another set, and we have to bind that value to the <select> -- it doesn't gain
us anything if we have to enforce the binding manually anyhow.
> 
> I also prefer saying "Binding to <select> is dangerous, don't do it" rather
than "Binding to <select> is dangerous, so make sure you use this trick when you
do it."

Ok, that's a fair point.

[michaelpg, 2016-10-26 01:18:47]

Patchset #3 (id:40001) has been deleted

[michaelpg, 2016-10-26 01:23:10]

Done in CSS. Thanks!

https://codereview.chromium.org/2446003002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/settings_dropdown_menu.html
(right):

https://codereview.chromium.org/2446003002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:19:
disabled="[[!showNotFoundValue_(menuOptions, pref.value)]]">
On 2016/10/25 01:18:09, dpapad wrote:
> So to avoid calculating the same boolean twice, can we use CSS for this? See
> example at https://jsfiddle.net/5bt02py5/1/

Nice, done.

[michaelpg, 2016-10-26 01:23:14]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2016-10-26 01:23:15]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2446003002/#ps60001
(title: "Hide custom option via CSS")

[commit-bot: I haz the power, 2016-10-26 01:23:39]

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-26 02:17:20]

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

[commit-bot: I haz the power, 2016-10-26 02:17:21]

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

[michaelpg, 2016-10-26 02:20:46]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2016-10-26 02:21:11]

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-26 02:28:24]

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

[commit-bot: I haz the power, 2016-10-26 02:28:24]

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

[michaelpg, 2016-10-26 05:26:34]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2016-10-26 05:26:54]

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-26 07:09:02]

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

[commit-bot: I haz the power, 2016-10-26 07:09:03]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[michaelpg, 2016-10-26 18:31:56]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2016-10-26 18:33:31]

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-26 21:14:10]

Description was changed from

==========
settings-dropdown-menu must set <select>#value after its options change

The |value| property of an HTMLSelectElement is a getter/setter, not a POD
variable. Setting it performs an algorithm which is something like:

  1. Let O be the first <option> in my children whose "property" attribute
     equals |value|.
  2. If O exists, add the "selected" attribute to O.

As a result, a vanilla Polymer binding is not safe if the contents are
dynamically generated:

  <select value="[[myValue]]">
    <template is="dom-repeat" items="[[myItems]]">
      <option value="[[item.value]]">[[item.name]]</option>
    </template>
  </select>

because if |myValue| already has a particular value, and then |myItems| is
set, |myValue| has the correct value but the <select>'s |value| "setter"
is not called. Thus the option is never actually selected.

Fix this by setting |value| directly instead of in a data binding.

Also, ensure "Custom" can't be selected when hidden via type-ahead.

BUG=658075
R=dpapad@chromium.org
TEST=CrSettingsDropdownMenu browser test
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
settings-dropdown-menu must set <select>#value after its options change

The |value| property of an HTMLSelectElement is a getter/setter, not a POD
variable. Setting it performs an algorithm which is something like:

  1. Let O be the first <option> in my children whose "property" attribute
     equals |value|.
  2. If O exists, add the "selected" attribute to O.

As a result, a vanilla Polymer binding is not safe if the contents are
dynamically generated:

  <select value="[[myValue]]">
    <template is="dom-repeat" items="[[myItems]]">
      <option value="[[item.value]]">[[item.name]]</option>
    </template>
  </select>

because if |myValue| already has a particular value, and then |myItems| is
set, |myValue| has the correct value but the <select>'s |value| "setter"
is not called. Thus the option is never actually selected.

Fix this by setting |value| directly instead of in a data binding.

Also, ensure "Custom" can't be selected when hidden via type-ahead.

BUG=658075
R=dpapad@chromium.org
TEST=CrSettingsDropdownMenu browser test
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-10-26 21:14:11]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-10-26 21:17:47]

Description was changed from

==========
settings-dropdown-menu must set <select>#value after its options change

The |value| property of an HTMLSelectElement is a getter/setter, not a POD
variable. Setting it performs an algorithm which is something like:

  1. Let O be the first <option> in my children whose "property" attribute
     equals |value|.
  2. If O exists, add the "selected" attribute to O.

As a result, a vanilla Polymer binding is not safe if the contents are
dynamically generated:

  <select value="[[myValue]]">
    <template is="dom-repeat" items="[[myItems]]">
      <option value="[[item.value]]">[[item.name]]</option>
    </template>
  </select>

because if |myValue| already has a particular value, and then |myItems| is
set, |myValue| has the correct value but the <select>'s |value| "setter"
is not called. Thus the option is never actually selected.

Fix this by setting |value| directly instead of in a data binding.

Also, ensure "Custom" can't be selected when hidden via type-ahead.

BUG=658075
R=dpapad@chromium.org
TEST=CrSettingsDropdownMenu browser test
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
settings-dropdown-menu must set <select>#value after its options change

The |value| property of an HTMLSelectElement is a getter/setter, not a POD
variable. Setting it performs an algorithm which is something like:

  1. Let O be the first <option> in my children whose "property" attribute
     equals |value|.
  2. If O exists, add the "selected" attribute to O.

As a result, a vanilla Polymer binding is not safe if the contents are
dynamically generated:

  <select value="[[myValue]]">
    <template is="dom-repeat" items="[[myItems]]">
      <option value="[[item.value]]">[[item.name]]</option>
    </template>
  </select>

because if |myValue| already has a particular value, and then |myItems| is
set, |myValue| has the correct value but the <select>'s |value| "setter"
is not called. Thus the option is never actually selected.

Fix this by setting |value| directly instead of in a data binding.

Also, ensure "Custom" can't be selected when hidden via type-ahead.

BUG=658075
R=dpapad@chromium.org
TEST=CrSettingsDropdownMenu browser test
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/b02d4b3efb575bb85e49404940a84cc616a48388
Cr-Commit-Position: refs/heads/master@{#427792}
==========

[commit-bot: I haz the power, 2016-10-26 21:17:48]

Patchset 3 (id:??) landed as
https://crrev.com/b02d4b3efb575bb85e49404940a84cc616a48388
Cr-Commit-Position: refs/heads/master@{#427792}