fix(ng:repeat) with array ignore properties not representing array elements

Along the way I also changed the repeater impl to use for loop instead
of for in loop.

Iteration over objects is handled by creating an array of keys, which is
sorted and this array then determines the order of iteration over an
element. This makes repeating over objects deterministic and
cross-browser compatible.

2 files changed, 77 insertions, 45 deletions

diff --git a/src/widgets.js b/src/widgets.js
index c70e9efa..11d9a2f0 100644
--- a/src/widgets.js
+++ b/src/widgets.js
@@ -361,7 +361,7 @@ angularWidget('@ng:repeat', function(expression, element){
     // We expect this to be a rare case.
     var lastOrder = new HashQueueMap();
     this.$watch(function(scope){
-      var index = 0,
+      var index, length,
           collection = scope.$eval(rhs),
           collectionLength = size(collection, true),
           childScope,
@@ -372,52 +372,64 @@ angularWidget('@ng:repeat', function(expression, element){
           array, last,       // last object information {scope, element, index}
           cursor = iterStartElement;     // current position of the node
 
-      for (key in collection) {
-        if (collection.hasOwnProperty(key) && key.charAt(0) != '$') {
-          last = lastOrder.shift(value = collection[key]);
-          if (last) {
-            // if we have already seen this object, then we need to reuse the
-            // associated scope/element
-            childScope = last.scope;
-            nextOrder.push(value, last);
-
-            if (index === last.index) {
-              // do nothing
-              cursor = last.element;
-            } else {
-              // existing item which got moved
-              last.index = index;
-              // This may be a noop, if the element is next, but I don't know of a good way to
-              // figure this out,  since it would require extra DOM access, so let's just hope that
-              // the browsers realizes that it is noop, and treats it as such.
-              cursor.after(last.element);
-              cursor = last.element;
-            }
-          } else {
-            // new item which we don't know about
-            childScope = parentScope.$new();
+      if (!isArray(collection)) {
+        // if object, extract keys, sort them and use to determine order of iteration over obj props
+        array = [];
+        for(key in collection) {
+          if (collection.hasOwnProperty(key) && key.charAt(0) != '$') {
+            array.push(key);
           }
+        }
+        array.sort();
+      } else {
+        array = collection || [];
+      }
 
-          childScope[valueIdent] = collection[key];
-          if (keyIdent) childScope[keyIdent] = key;
-          childScope.$index = index;
-          childScope.$position = index == 0
-              ? 'first'
-              : (index == collectionLength - 1 ? 'last' : 'middle');
-
-          if (!last) {
-            linker(childScope, function(clone){
-              cursor.after(clone);
-              last = {
-                  scope: childScope,
-                  element: (cursor = clone),
-                  index: index
-                };
-              nextOrder.push(value, last);
-            });
+      // we are not using forEach for perf reasons (trying to avoid #call)
+      for (index = 0, length = array.length; index < length; index++) {
+        key = (collection === array) ? index : array[index];
+        value = collection[key];
+        last = lastOrder.shift(value);
+        if (last) {
+          // if we have already seen this object, then we need to reuse the
+          // associated scope/element
+          childScope = last.scope;
+          nextOrder.push(value, last);
+
+          if (index === last.index) {
+            // do nothing
+            cursor = last.element;
+          } else {
+            // existing item which got moved
+            last.index = index;
+            // This may be a noop, if the element is next, but I don't know of a good way to
+            // figure this out,  since it would require extra DOM access, so let's just hope that
+            // the browsers realizes that it is noop, and treats it as such.
+            cursor.after(last.element);
+            cursor = last.element;
           }
+        } else {
+          // new item which we don't know about
+          childScope = parentScope.$new();
+        }
 
-          index ++;
+        childScope[valueIdent] = value;
+        if (keyIdent) childScope[keyIdent] = key;
+        childScope.$index = index;
+        childScope.$position = index == 0
+            ? 'first'
+            : (index == collectionLength - 1 ? 'last' : 'middle');
+
+        if (!last) {
+          linker(childScope, function(clone){
+            cursor.after(clone);
+            last = {
+                scope: childScope,
+                element: (cursor = clone),
+                index: index
+              };
+            nextOrder.push(value, last);
+          });
         }
       }
 
diff --git a/test/widgetsSpec.js b/test/widgetsSpec.js
index 4aa36428..b93d698d 100644
--- a/test/widgetsSpec.js
+++ b/test/widgetsSpec.js
@@ -254,7 +254,7 @@ describe("widget", function() {
                                   'ng:bind="key + \':\' + val + $index + \'|\'"></li></ul>');
       scope.items = {'misko':'m', 'shyam':'s', 'frodo':'f'};
       scope.$digest();
-      expect(element.text()).toEqual('misko:m0|shyam:s1|frodo:f2|');
+      expect(element.text()).toEqual('frodo:f0|misko:m1|shyam:s2|');
     });
 
     it('should expose iterator position as $position when iterating over arrays', function() {
@@ -282,7 +282,7 @@ describe("widget", function() {
         '</ul>');
       scope.items = {'misko':'m', 'shyam':'s', 'doug':'d', 'frodo':'f'};
       scope.$digest();
-      expect(element.text()).toEqual('misko:m:first|shyam:s:middle|doug:d:middle|frodo:f:last|');
+      expect(element.text()).toEqual('doug:d:first|frodo:f:middle|misko:m:middle|shyam:s:last|');
 
       delete scope.items.doug;
       delete scope.items.frodo;
@@ -312,6 +312,26 @@ describe("widget", function() {
       expect(element.text()).toEqual('a|b|Xc|d|X');
     });
 
+    it('should ignore non-array element properties when iterating over an array', function() {
+      var scope = compile('<ul><li ng:repeat="item in array">{{item}}|</li></ul>');
+      scope.array = ['a', 'b', 'c'];
+      scope.array.foo = '23';
+      scope.array.bar = function() {};
+      scope.$digest();
+
+      expect(element.text()).toBe('a|b|c|');
+    });
+
+    it('should iterate over non-existent elements of a sparse array', function() {
+      var scope = compile('<ul><li ng:repeat="item in array">{{item}}|</li></ul>');
+      scope.array = ['a', 'b'];
+      scope.array[4] = 'c';
+      scope.array[6] = 'd';
+      scope.$digest();
+
+      expect(element.text()).toBe('a|b|||c||d|');
+    });
+
 
     describe('stability', function() {
       var a, b, c, d, scope, lis;