Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Binding input fields to and setting elements of a flat array behaves unexpectedly #3859

Closed
gkjohnson opened this issue Aug 12, 2016 · 3 comments
Labels

Comments

@gkjohnson
Copy link

When trying to create a list of input fields to directly edit the elements of an array, the incorrect elements of the array get changed. Other times, both the correct element will get set, as well as a second one.

Here's an example starting with an empty array with 10 elements, using a dom-repeat element to bind input fields to the array element values:
https://jsfiddle.net/na4cjwru/10/

Case 1

Case 2

  • Place cursor in the 3rd index input field and type "100"
    we expect just the value of the 3rd index to change
  • Instead, both the 0th and the 3rd array values change, and the path "arr.add g-selection and g-selector components #9" is fired to the change observer
  • Place cursor in the 4th index input field and type "500", the same thing happens
  • Place back in the 3rd index input field and change the value
  • Changing that field no longer updates the 0th value, but now the change function is receiving the invalid path of "arr."

And here is an example where we start off with a fully initialized array filled with values:
https://jsfiddle.net/na4cjwru/9/

Case 3

  • Delete the value in the 3rd index input field
  • Delete the value in the 6th index input field
  • Type "123" into the 3rd index input field
  • Both the 3rd and 6th index fields and values change, but only "arr.various changes to enable g-overlay #6" is fired to the change observer

Can anyone shed a little light on this? Only of the only work arounds at the moment seems to be to make an array of objects that point to the data I actually care about and convert it to a flat array later. This seems like the kind of thing that should work, though.

@gkjohnson
Copy link
Author

I've tracked the bug down a bit, and the source of the issue seems to be in how Polymer.Collection handles mapping arrays of non-objects:

https://github.com/Polymer/polymer/blob/master/src/lib/collection.html#L37

    initMap: function() {
      var omap = this.omap = new WeakMap();
      var pmap = this.pmap = {};
      var s = this.store;
      for (var i=0; i<s.length; i++) {
        var item = s[i];
        if (item && typeof item == 'object') {
          omap.set(item, i);
        } else {

          // This line keeps track of the value -> keys when dealing
          // with non-objects, but clearly if any of the values in the array
          // are the same, then the previously stored value->key will be
          // overwritten with the new one
          pmap[item] = i;

        }
      }
    },

Making these work-around changes in notify-path "set" function and dom-repeat "_applyFullRefresh" function fixes the issue, but it's not thorough:

https://github.com/Polymer/polymer/blob/master/src/standard/notify-path.html#L198

// here we guarantee the key is correct by manually
// creating it instead of going through the Collection's
// key map to get it (which uses the pmap object noted
// above), so now the correct index is updated when 
// "setItem" is called

// key = coll.getKey(old);
key = "#" + last;

and

https://github.com/Polymer/polymer/blob/master/src/lib/template/dom-repeat.html#L494

// Dom-repeat caches the keys in pmap along with
// each content instance created, so here we force
// the correct keys to be assigned to each instance
// which fixes two fields being updated when
// the last field is editted

// keys.push(c.getKey(items[i]));
keys.push("#" + i);

These couple changes fix the issues demonstrated in the jsfiddle, but ultimately it seems like the Collection's implementation of pmap should be changed to fix the issue generally or not be used at all.

@gkjohnson
Copy link
Author

It sounds like this issue is related:
#3062

@tjsavage tjsavage added the 1.x label Sep 8, 2016
@TimvdLippe
Copy link
Contributor

#3062 has been closed and it seems that this issue has been fixed as well in Polymer 2: http://jsbin.com/tacujibola/edit?html,console,output

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants