PolymerElements / iron-selector

Manages a list of elements that can be selected
32 stars 55 forks source link

On initial load, `selectedItem` not set when `selected` change observer fires. #95

Closed zerodevx closed 8 years ago

zerodevx commented 8 years ago

Hello!

If the selected property is set up to be observed, when the first change fires, the selectedItem property is undefined when we expect it to contain the selected node.

observers: ["_selectedChanged(selected)"],

This issue is introduced in v1.0.8. Essentially it's a race condition - the property-change observer fires between created() and ready() lifecycles, but since v1.0.8, the selectedItem property is only updated on the attached() event through the _updateItems() method.

This unfortunately breaks neon-animated-pages's animate-initial-selection feature.

Repro (working in v1.0.7): http://jsbin.com/nuhumokuwu/edit?html,console,output Repro (not working in v1.0.8): http://jsbin.com/qonifikeku/1/edit?html,console,output

sandro-k commented 8 years ago

+1 despite that (more-routing)[https://github.com/PolymerLabs/more-routing] is not maintained I am still using it until something else comes out. After updating more-routing, iron-pages and IronSelectableBehavior the routing doesn't work any more, as IronSelectableBehavior.items is an empty array.

cdata commented 8 years ago

Hi, the selectedItem seems to be set at the right time. Can you re-open this issue if you are still experiencing a problem? If you do, please also provide the expected console output of your test case, in case I am misunderstanding something.

zerodevx commented 8 years ago

@cdata sorry, i re-checked the test cases. There is an error with my v1.0.8 jsbin - you'll see that I selected Polymer v1.2.3 together with iron-selector v1.0.8 here: http://jsbin.com/ledoceroku/edit?html,console,output

The expected output is selectedItem should be set when I query it after the selected-changed event fires. This behavior works before, but breaks in v1.0.8.

I am the original committer for the animate-initial-selection feature in neon-animated-pages, and this issue is the reason why https://github.com/PolymerElements/neon-animation/issues/105 and https://github.com/PolymerElements/neon-animation/issues/112 happens.

zerodevx commented 8 years ago

@cdata

Can you confirm if this is an issue - if it is, this issue needs to be re-opened so that it's in the radar.

Basically, selectedItem is not set when the first selected-changed observer fires. I thought this is very self-explanatory - I'm not sure how much better I can demonstrate this.

Behavior in v1.0.7: http://jsbin.com/nuhumokuwu/edit?html,console,output Behavior in v1.0.8: http://jsbin.com/ledoceroku/edit?html,console,output

MeTaNoV commented 8 years ago

I think this is fixed with the current release which is v1.2.0! :)

zerodevx commented 8 years ago

@MeTaNoV I see you everywhere :stuck_out_tongue_closed_eyes:, don't you have to work on "secret project"?

Actually, this is still a bug in v1.2.0. According to the docs:

  /**
   * Returns the currently selected item.
   *
   * @type {?Object}
   */
  selectedItem: {
    type: Object,
    readOnly: true,
    notify: true
  },

Consider this snippet:

<x-test selected="0">
  <div>ONE</div>
  <div>TWO</div>
</x-test>

and:

Polymer({
  is: 'x-test',
  behaviors: [Polymer.IronSelectableBehavior],
  observers: ["_selectedChanged(selected)"],
  _selectedChanged: function () {
    if (this.selectedItem) { console.log("selectedItem is set"); }
    else { console.log("selectedItem not set"); }
  }
});

You'll expect to see "selectedItem is set". However, this fails from v1.0.8 onwards.

Repro v1.0.7 (working): http://jsbin.com/nuhumokuwu/edit?html,console,output Repro v1.0.8 (fails): http://jsbin.com/ledoceroku/edit?html,console,output Repro v1.2.0 (fails): http://jsbin.com/gelozoxega/edit?html,console,output

@cdata, @bicknellr Am I misunderstanding something?

MeTaNoV commented 8 years ago

try this and tell me if this works:

Polymer({
  is: 'x-test',
  behaviors: [Polymer.IronSelectableBehavior],
  observers: ["_selectedChanged(selected)"],
  _selectedChanged: function () {
    this.async(function() {
      if (this.selectedItem) { console.log("selectedItem is set"); }
      else { console.log("selectedItem not set"); }
    });
  }
});
zerodevx commented 8 years ago

Wrapping the call in async() will work, but this behavior change from v1.0.7 to date causes other components (that relies on selectedItem being set on initialization when selected-changed observer fires) to break.

For example: https://github.com/PolymerElements/neon-animation/issues/112

For me, I feel that when selected-changed fires, it should be guaranteed that when I query selectedItem, I am returned the selected node, without requiring me to push this task back to the end of the microtask queue.

bicknellr commented 8 years ago

My guess is that your selected observer is fired before the one in IronSelectableBehavior which eventually would initialize selectedItem. This kind of feels weird but I suppose it's not completely wrong given that the observer doesn't say it depends on selectedItem. If you add selectedItem it seems to work fine; check out this section of the docs for more details.

MeTaNoV commented 8 years ago

Interesting indeed, but I find it a specific workaround to the use-case. My question being, when having several observers on the same property (at different level, in an element, outside the element), what is the order of execution from the associated callback? I don't find anything in the documentation, and I fear, that it is unknown?

sandro-k commented 8 years ago

@MeTaNoV I don't think wrapping listener in async is a solution. I have set up de demo http://sandro-k.github.io/selector-demo/ that binds to iron-selector selected-item="{{selectedItem}}" and observes the changes to selectedItem in the host element. The listener gets called twice for each change made to the selectedItem. First the it is set to undefined and then to the selected item.

Source

MeTaNoV commented 8 years ago

@sandro-k this is not a proper solution, and there is not proper solution if you have multiple observers since we don't know the order of execution...