MD Settings: settings-dropdown-menu selection issues |
||||
Issue descriptionSome 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.
,
Oct 21 2016
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.
,
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.
,
Oct 22 2016
#2 seems the most reasonable to me.
,
Oct 24 2016
,
Oct 25 2016
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
,
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
,
Oct 26 2016
,
Nov 23 2016
Code change. Closing bug and marking verified. |
||||
►
Sign in to add a comment |
||||
Comment 1 by dpa...@chromium.org
, Oct 21 2016