New issue
Advanced search Search tips

Issue 658075 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug

Blocking:
issue 614588
issue 614589



Sign in to add a comment

MD Settings: settings-dropdown-menu selection issues

Project Member Reported by michae...@chromium.org, Oct 21 2016

Issue description

Some problems with the way the <select> in <settings-dropdown-menu> is handled,
because we are abusing the <select>#value property:

1. Changing menuOptions may reset the <select> to the first <option> in its DOM.

   If menuOptions changes, the first <option> becomes selected. We asynchronously
   try to set the <select>#value property with data binding, but this does not work
   if the Polymer property hasn't changed.

   In other words, Polymer doesn't care that <select>'s DOM changed, and <select>
   is required to select the first option in the list when its DOM is set anew.

2. The "Custom" option can always be selected, even if it's hidden.

   We use the "hidden" attribute (display: none) to show/hide the Custom option.
   This does not invalidate the option; it can still be chosen from the <select>,
   e.g. by typing "Cus" or just hitting "C" a few times to cycle through options.

Source: https://www.w3.org/TR/html5/forms.html#dom-select-value

I already have fixes as part of the Time Zone CL; I'll upload that part separately
for review.
 

Comment 1 by dpa...@chromium.org, Oct 21 2016

Michael, I think you are discovering issues I've already discovered with select, as I migrated multiple cases of paper-dropdown-menu. Let me rephrase to confirm if we are talking about the same issues.

a) 2-way binding does not work with <select>. When the select's value changes, Polymer does not get notified. For this reason we don't use 2-way bindings in any <select>

b) When the value of a <select> is explicitly set, it is ignored if there is no <option> that has the same |value| attribute. This causes a problem when the <option> instances are generated via a dom-repeat template. For this reason, whenever the array bound to a dom-repeat is changed, we yield with this.async() to let the <option>s get rendered, before updating |value| (either directly or via a 1-way binding.

Both of a and b AFAIK are handled in our UI such that they don't cause any user facing bugs. Regarding  issue 2  in your original report, I've not encountered it, perhaps a fiddle would be helpful. Off the top of my head, making it disabled instead of hidden would probably fix the issue.
a) and b) are correct, but not related to this bug.


> a) 2-way binding does not work with <select>. When the select's value
> changes, Polymer does not get notified. For this reason we don't use
> 2-way bindings in any <select>


Right. This bug isn't about 2-way binding. (though 2-way binding is possible)

> b) When the value of a <select> is explicitly set, it is ignored if there
> is no <option> that has the same |value| attribute.

Yes. Setting <select>#value is either a no-op, or sets the <option>#selected
attribute for an option with a matching value. |value| is like a getter/setter,
not a property that holds a boolean state.

> This causes a problem when the <option> instances are generated via a
> dom-repeat template.

More specifically it can cause a problem *whenever* <option> elements are created
dynamically.

> For this reason, whenever the array bound to a dom-repeat is changed, we
> yield with this.async() to let the <option>s get rendered, before
> updating |value| (either directly or via a 1-way binding.
 
The bug is that we *won't* always update |value| via 1-way binding.

Setting a Polymer property to its current value does not count as a change, and
does not trigger any Polymer effects, including data binding effects.

For example, suppose |ourValue| is one-way bound to a <paper-input>#value. If we
set |ourValue| to "Hello", the <paper-input> will show "Hello". If we type " World",
the <paper-input>#value will be "Hello World" but |ourValue| is still "Hello".

Now if we set |ourValue| to "Hello" -- again -- nothing happens, because Polymer
didn't see anything change. Demo: https://jsfiddle.net/hfh5jsby/


In our <select>, the option we want to select will only become selected if Polymer
sets <select>#value after that option has been added.


So here's the bug: If <settings-dropdown-menu> has a menuOptions item which
matches the pref, that option is selected in the <select>; if |menuOptions| is
then set again (even to an identical array) the <select> never selects the new
option matching the pref, so we see the 1st menu option and <select>#value
doesn't match the pref even though |selectedValue_| does.

You can see this today in md-settings by selecting a settings-dropdown-menu that
is set to a pref value and doing:

    $0.menuOptions = JSON.parse(JSON.stringify($0.menuOptions));

This became an actual problem for me (because I wanted to lazily fetch the
menu options for the time zone dropdown instead of fetching them at startup).

Demo: https://jsfiddle.net/oo2sszv5/



> Both of a and b AFAIK are handled in our UI such that they don't cause any
> user facing bugs. Regarding  issue 2  in your original report, I've not
> encountered it, perhaps a fiddle would be helpful.

Fiddle: https://jsfiddle.net/smtgwe8k/

"Custom" option is hidden, but you can still select Custom by focusing the
<select> and typing "Custom" (or just "cu"). It is also included in the
first-letter cycle: tap "c" to select "Cat", wait a second, tap "c" again and
"Custom" becomes selected.

> Off the top of my head, making it disabled instead of hidden would probably
> fix the issue.

Cool, that works: we can set disabled whenever it's not hidden.

Comment 3 by dpa...@chromium.org, Oct 21 2016

>In our <select>, the option we want to select will only become selected if Polymer
>sets <select>#value after that option has been added.

I understand the bug you are describing now. There are three possible solutions I can imagine.
1) Bypass the Polymer's behavior of not triggering observers when an value is assigned to the already
   existing value, by assigning a dummy value first (maybe selected_ = '' would work, but have not tried).
2) Instead of relying on the 1-way data binding when the list updates dynamically, just set the
   select#value directly (after a call to this.async() to allow the list to render first.
3) Use a selected$= attribute binding on the <option>, as we do at
   https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/search_page/search_page.html?l=22.
   This is guaranteed to work, but more expensive since the binding executes N times.
#2 seems the most reasonable to me.
Cc: derat@chromium.org
My last sentence in #2 is wrong: we should set the custom option's |disabled| whenever it IS |hidden|.

Patch uploaded: https://codereview.chromium.org/2446003002
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 26 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b02d4b3efb575bb85e49404940a84cc616a48388

commit b02d4b3efb575bb85e49404940a84cc616a48388
Author: michaelpg <michaelpg@chromium.org>
Date: Wed Oct 26 21:13:44 2016

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

Review-Url: https://codereview.chromium.org/2446003002
Cr-Commit-Position: refs/heads/master@{#427792}

[modify] https://crrev.com/b02d4b3efb575bb85e49404940a84cc616a48388/chrome/browser/resources/settings/controls/settings_dropdown_menu.html
[modify] https://crrev.com/b02d4b3efb575bb85e49404940a84cc616a48388/chrome/browser/resources/settings/controls/settings_dropdown_menu.js
[modify] https://crrev.com/b02d4b3efb575bb85e49404940a84cc616a48388/chrome/test/data/webui/settings/dropdown_menu_tests.js

Status: Fixed (was: Started)
Status: Verified (was: Fixed)
Code change. Closing bug and marking verified.

Sign in to add a comment