PolymerElements / paper-dropdown-menu

A Material Design browser select element
https://www.webcomponents.org/element/PolymerElements/paper-dropdown-menu
61 stars 107 forks source link

Selected value label does not update when underlying array updates. #197

Open freshp86 opened 7 years ago

freshp86 commented 7 years ago

Repro at https://jsfiddle.net/j4rfjasf/10/

Steps:

  1. Expand dropdown menu, everything looks fine
  2. Click "change dropdown options"

Expected: The displayed "selected" value should change from "hello" to "hola" Actual: The displayed value does not change, and it does not match any of the dropdown contents. This should not be possible.

image

This is affecting Chrome's new Settings page, https://bugs.chromium.org/p/chromium/issues/detail?id=640496.

danbeam commented 7 years ago

/cc @cdata

danbeam commented 7 years ago

@tjsavage

JanMiksovsky commented 7 years ago

@freshp86 Just to clarify your expectation: the reason the selected value should change to "hola" is because that's the first (default) item, not because it corresponds to the position of the previously-selected value. That is, the desired behavior here should mirror what <select> does.

freshp86 commented 7 years ago

My expectation is because it corresponds to the position of the previously selected value, and AFAIU this is how <select> works, see updated example at https://jsfiddle.net/j4rfjasf/20/. Try selecting the 2nd or 3rd value before clicking the button.

JanMiksovsky commented 7 years ago

The persistence of the selection position seems to result from the use of Polymer's data binding.

A plain <select> instance whose children change resets the selection: http://jsbin.com/qimehu/2/edit?html,output. That's what I would expect. While maintaining selection position might be helpful, in other cases it could induce users to commit data values they didn't expect. To me, it feels like a safer default behavior is to reset the selection when items change. But perhaps the expectations are different in the context of data binding.

So the bug here would be to ensure that a data-bound paper-dropdown-menu works the same as a data-bound select, even though the data-bound select would exhibit different behavior than a plain select populated by changing child items. Is that correct?

freshp86 commented 7 years ago

A plain <select> instance whose children change resets the selection

Not really. Your example is not entirely equivalent to what data-binding does. You are creating new <option> instances, by assigning the innerHTML proprety, and discarding the old <option> instances. Yes selection is reset in that case, that's fine.

But if you simply get a reference to the selected <option> instance and change its text value, the <select> updates accordingly. That does not happen for the paper-dropdown-menu-light case. See example at https://jsfiddle.net/1uav3d4v/

JanMiksovsky commented 7 years ago

We're in agreement, I was just trying to get a handle on the expectations here.

I'll investigate.

JanMiksovsky commented 7 years ago

The core problem here seems to lie in iron-selectable/iron-multi-selectable. The latter is the behavior used by paper-listbox here, but the former also likely has the same problem.

First, it’s worth noting that the only expectations that paper-dropdown-menu has on its dropdown content element (which could be anything, not necessarily a paper-listbox) is that the dropdown content element raise iron-select and iron-deselect events. That’s the whole contract. So any fix here has to leverage those events if we want to avoid coming up with a new means for the dropdown menu and its content to communicate.

The core surprise seems to be that when the items of an iron-selectable/iron-multi-selectable element change, the iron-select event is not raised again. If item n of the items array is selected, and then the content of that item n change (e.g., because that item was re-bound to new data), it feels like the iron-select event should be raised again. Even though the selected index will still be n, the content/children of the selectedItem can be different, so it’s reasonable to expect another iron-select event.

I believe if that were fixed in iron-selectable/iron-multi-selectable, that would resolve this issue cleanly, and might resolve similar issues in others uses of those behaviors.

@cdata I'd like to pursue a fix in this general direction, but wanted to flag this in case you had other suggestions.

JanMiksovsky commented 7 years ago

On further investigation, it seems unlikely this can be properly fixed without a substantial change in the semantics of iron-selectable with regard to data binding.

As it stands, an iron-selectable element like paper-listbox does have a basic MutationObserver to detect changes in direct children (via childList). So changes to the list box's items collection that force a change in the number of children will be detected as expected. But changes that simply change textContent (or attributes, or subtrees) of existing items will not be detected. This is the exact situation that occurs with data binding. If the items collection is replaced with a new array of the same length, the existing direct children will be reused. The change will not be detected by the MutationObserver, the list box's _updateItems method will not be invoked, and its iron-items-changed event will not fire.

Given that an iron-selectable doesn’t know when its own items are changing, there’s no practical way for it to re-raise an iron-select event here to fix the original issue.

Fundamentally, the current semantics of item change detection in iron-selectable are weak. Strengthening them would entail a fairly substantial rewrite, and involve either creating a custom MutationObserver, or rewriting Polymer's own observeNodes to optionally detect characterData and subtree mutations. Changing these semantics would be a non-trivial change in the component.

Unless there's real interest in fixing that, the only answer here is to have creators of components that combine paper-dropdown-menu with paper-listbox handle this situation explicitly. I think it’s reasonable to at least provide a reference implementation for how to do that, so I’ve taken a shot here: http://jsbin.com/fuqoye/edit?html,output.

It’s unfortunate to not fix the underlying problem, but would such a workaround be acceptable?

JanMiksovsky commented 7 years ago

@freshp86 Thoughts?

freshp86 commented 7 years ago

@JanMiksovsky: Thank you for providing a workaround. We are going to experiment with native <select> for the first launch of Chrome's settings UI. Unfortunately there are more dropdown related issues affecting us.

This issue is still worth fixing for the remaining of Polymer users, but it is now a lower priority for us. We are interested in moving back to Polymer's dropdowns again in the future.

dominik0711 commented 7 years ago

@JanMiksovsky Be careful with the provided workaround! In my case I'm using a paper-listbox with the attribute attr-for-selected which maps to something different then the index of the array! For that the provided workaround above won't be a proper general implementation. If you need any help please let me know

mindon commented 7 years ago

need to call this to update the label, when item changed event not fired in this case.

Polymer.Async.microTask.run(()=>{
      this.$.theDropdownMenu._selectedItemChanged(this.$.theListBox.selectedItem);
})