fix(scope): fix edge case for $digest & $broadcast scope traversal

- fixed traversal originating on a scope with with a right sibling
- unified code for both $broadcast and $digest

2 files changed, 81 insertions, 59 deletions

diff --git a/src/Scope.js b/src/Scope.js
index badfc3f5..e037cb06 100644
--- a/src/Scope.js
+++ b/src/Scope.js
@@ -321,31 +321,31 @@ Scope.prototype = {
    *
    */
   $digest: function() {
-    var watch, value, last, next,
+    var watch, value, last,
         watchers,
         asyncQueue,
         length,
         dirty, ttl = 100,
-        scope;
+        next, current, target = this;
 
-    if (this.$$phase) {
-      throw Error(this.$$phase + ' already in progress');
+    if (target.$$phase) {
+      throw Error(target.$$phase + ' already in progress');
     }
     do {
 
       dirty = false;
-      scope = this;
+      current = target;
       do {
-        scope.$$phase = '$digest';
-        asyncQueue = scope.$$asyncQueue;
+        current.$$phase = '$digest';
+        asyncQueue = current.$$asyncQueue;
         while(asyncQueue.length) {
           try {
-            scope.$eval(asyncQueue.shift());
+            current.$eval(asyncQueue.shift());
           } catch (e) {
-            scope.$service('$exceptionHandler')(e);
+            current.$service('$exceptionHandler')(e);
           }
         }
-        if ((watchers = scope.$$watchers)) {
+        if ((watchers = current.$$watchers)) {
           // process our watches
           length = watchers.length;
           while (length--) {
@@ -353,28 +353,27 @@ Scope.prototype = {
               watch = watchers[length];
               // Most common watches are on primitives, in which case we can short
               // circuit it with === operator, only when === fails do we use .equals
-              if ((value = watch.get(scope)) !== (last = watch.last) && !equals(value, last)) {
+              if ((value = watch.get(current)) !== (last = watch.last) && !equals(value, last)) {
                 dirty = true;
-                watch.fn(scope, watch.last = copy(value), last);
+                watch.fn(current, watch.last = copy(value), last);
               }
             } catch (e) {
-              scope.$service('$exceptionHandler')(e);
+              current.$service('$exceptionHandler')(e);
             }
           }
         }
 
+        current.$$phase = null;
 
-        scope.$$phase = null;
-        // find the next scope in traversal.
-        if (!(next = scope.$$childHead || scope.$$nextSibling) && scope !== this) {
-          do {
-            scope = scope.$parent;
-            if (scope == this || (next = scope.$$nextSibling)) {
-              break;
-            }
-          } while (scope !== this);
+        // Insanity Warning: scope depth-first traversal
+        // yes, this code is a bit crazy, but it works and we have tests to prove it!
+        // this piece should be kept in sync with the traversal in $broadcast
+        if (!(next = (current.$$childHead || (current !== target && current.$$nextSibling)))) {
+          while(current !== target && !(next = current.$$nextSibling)) {
+            current = current.$parent;
+          }
         }
-      } while ((scope = next));
+      } while ((current = next));
 
       if(!(ttl--)) {
         throw Error('100 $digest() iterations reached. Aborting!');
@@ -651,44 +650,34 @@ Scope.prototype = {
    * @param {...*} args Optional set of arguments which will be passed onto the event listeners.
    */
   $broadcast: function(name, args) {
-    var targetScope = this,
-        currentScope = targetScope,
-        nextScope = targetScope,
+    var target = this,
+        current = target,
+        next = target,
         event = { name: name,
-                  targetScope: targetScope },
+                  targetScope: target },
         listenerArgs = concat([event], arguments, 1);
 
     //down while you can, then up and next sibling or up and next sibling until back at root
     do {
-      currentScope = nextScope;
-      event.currentScope = currentScope;
-      forEach(currentScope.$$listeners[name], function(listener) {
+      current = next;
+      event.currentScope = current;
+      forEach(current.$$listeners[name], function(listener) {
         try {
           listener.apply(null, listenerArgs);
         } catch(e) {
-          currentScope.$service('$exceptionHandler')(e);
+          current.$service('$exceptionHandler')(e);
         }
       });
 
-      // down or to the right!
-      nextScope = currentScope.$$childHead || currentScope.$$nextSibling;
-
-      if (nextScope) {
-        // found child or sibling
-        continue;
-      }
-
-      // we have to restore nextScope and go up!
-      nextScope = currentScope;
-
-      while (!nextScope.$$nextSibling && (nextScope != targetScope)) {
-        nextScope = nextScope.$parent;
-      }
-
-      if (nextScope != targetScope) {
-        nextScope = nextScope.$$nextSibling;
+      // Insanity Warning: scope depth-first traversal
+      // yes, this code is a bit crazy, but it works and we have tests to prove it!
+      // this piece should be kept in sync with the traversal in $digest
+      if (!(next = (current.$$childHead || (current !== target && current.$$nextSibling)))) {
+        while(current !== target && !(next = current.$$nextSibling)) {
+          current = current.$parent;
+        }
       }
-    } while (nextScope != targetScope);
+    } while ((current = next));
   }
 };
 
diff --git a/test/ScopeSpec.js b/test/ScopeSpec.js
index 4f6293d0..4e21537c 100644
--- a/test/ScopeSpec.js
+++ b/test/ScopeSpec.js
@@ -153,7 +153,7 @@ describe('Scope', function() {
     });
 
 
-    it('should delegate $digest to children in addition order', function() {
+    it('should call child $watchers in addition order', function() {
       // this is not an external guarantee, just our own sanity
       var log = '';
       var childA = root.$new();
@@ -168,6 +168,30 @@ describe('Scope', function() {
     });
 
 
+    it('should allow $digest on a child scope with and without a right sibling', function() {
+      // tests a traversal edge case which we originally missed
+      var log = '',
+          childA = root.$new(),
+          childB = root.$new();
+
+      root.$watch(function() { log += 'r'; });
+      childA.$watch(function() { log += 'a'; });
+      childB.$watch(function() { log += 'b'; });
+
+      // init
+      root.$digest();
+      expect(log).toBe('rabrab');
+
+      log = '';
+      childA.$digest();
+      expect(log).toBe('a');
+
+      log = '';
+      childB.$digest();
+      expect(log).toBe('b');
+    });
+
+
     it('should repeat watch cycle while model changes are identified', function() {
       var log = '';
       root.$watch('c', function(self, v){self.d = v; log+='c'; });
@@ -498,7 +522,7 @@ describe('Scope', function() {
 
     describe('$broadcast', function() {
       describe('event propagation', function() {
-        var log, child1, child2, child3, grandChild11, grandChild21, grandChild22,
+        var log, child1, child2, child3, grandChild11, grandChild21, grandChild22, grandChild23,
             greatGrandChild211;
 
         function logger(event) {
@@ -513,6 +537,7 @@ describe('Scope', function() {
           grandChild11 = child1.$new();
           grandChild21 = child2.$new();
           grandChild22 = child2.$new();
+          grandChild23 = child2.$new();
           greatGrandChild211 = grandChild21.$new();
 
           root.id = 0;
@@ -522,6 +547,7 @@ describe('Scope', function() {
           grandChild11.id = 11;
           grandChild21.id = 21;
           grandChild22.id = 22;
+          grandChild23.id = 23;
           greatGrandChild211.id = 211;
 
           root.$on('myEvent', logger);
@@ -531,13 +557,14 @@ describe('Scope', function() {
           grandChild11.$on('myEvent', logger);
           grandChild21.$on('myEvent', logger);
           grandChild22.$on('myEvent', logger);
+          grandChild23.$on('myEvent', logger);
           greatGrandChild211.$on('myEvent', logger);
 
-          //         R
-          //       / |  \
-          //     1   2   3
-          //    /   / \
-          //   11  21  22
+          //          R
+          //       /  |   \
+          //     1    2    3
+          //    /   / | \
+          //   11  21 22 23
           //       |
           //      211
         });
@@ -545,22 +572,28 @@ describe('Scope', function() {
 
         it('should broadcast an event from the root scope', function() {
           root.$broadcast('myEvent');
-          expect(log).toBe('0>1>11>2>21>211>22>3>');
+          expect(log).toBe('0>1>11>2>21>211>22>23>3>');
         });
 
 
         it('should broadcast an event from a child scope', function() {
           child2.$broadcast('myEvent');
-          expect(log).toBe('2>21>211>22>');
+          expect(log).toBe('2>21>211>22>23>');
         });
 
 
-        it('should broadcast an event from a leaf scope', function() {
+        it('should broadcast an event from a leaf scope with a sibling', function() {
           grandChild22.$broadcast('myEvent');
           expect(log).toBe('22>');
         });
 
 
+        it('should broadcast an event from a leaf scope without a sibling', function() {
+          grandChild23.$broadcast('myEvent');
+          expect(log).toBe('23>');
+        });
+
+
         it('should not not fire any listeners for other events', function() {
           root.$broadcast('fooEvent');
           expect(log).toBe('');