refactor: $animate - add $animateClassToggler to stop race conditions

ajoslin · ajoslin · commit e75e20dc9d6f · 2014-04-10T10:35:36.000-06:00
Addresses #1100. Fixes race conditions where $animate.{removeClass,hideClass} are called simultaneously, in addition fixes any blinking that coulld be caused by showing an element just attached to the dom.
diff --git a/js/ext/angular/src/service/animateClassToggler.js b/js/ext/angular/src/service/animateClassToggler.js
@@ -0,0 +1,55 @@
+/**
+ * Stop race conditions with toggling an ngAnimate on an element rapidly,
+ * due to the asynchronous nature of addClass and removeClass
+ *
+ * Additionally, only toggle classes on requestAnimationFrame, to stop flickers on
+ * iOS and older android for elements that were just attached to the DOM.
+ */
+angular.module('ionic')
+.factory('$animateClassToggler', [
+  '$animate',
+  '$rootScope',
+function($animate, $rootScope) {
+
+  return function(element, className) {
+
+    var self = {
+      _nextOperation: null,
+      _animating: false,
+      _toggle: toggle,
+      _animate: animate,
+      _animationDone: animationDone,
+      addClass: function() { self._toggle(true); },
+      removeClass: function() { self._toggle(false); }
+    };
+
+    return self;
+
+    function toggle(hasClass) {
+      var operation = hasClass ? 'addClass' : 'removeClass';
+      if (self._animating) {
+        self._nextOperation = operation;
+      } else {
+        self._animate(operation);
+      }
+    }
+
+    function animate(operation) {
+      self._animating = true;
+      ionic.requestAnimationFrame(function() {
+        $rootScope.$evalAsync(function() {
+          $animate[operation](element, className, self._animationDone);
+        });
+      });
+    }
+
+    function animationDone() {
+      self._animating = false;
+      if (self._nextOperation) {
+        self._animate(self._nextOperation);
+        self._nextOperation = null;
+      }
+    }
+  };
+}]);
+
diff --git a/js/ext/angular/src/service/ionicBackdrop.js b/js/ext/angular/src/service/ionicBackdrop.js
@@ -6,32 +6,30 @@ angular.module('ionic')
 .factory('$ionicBackdrop', [
   '$animate',
   '$document',
-function($animate, $document) {
+  '$animateClassToggler',
+function($animate, $document, $animateClassToggler) {
 
-  var el;
+  var el = angular.element('<div class="backdrop ng-hide">');
   var backdropHolds = 0;
+  var toggler = $animateClassToggler(el, 'ng-hide');
+
+  $document[0].body.appendChild(el[0]);
 
   return {
     retain: retain,
     release: release,
-    _getElement: getElement
+    // exposed for testing
+    _element: el
   };
 
-  function getElement() {
-    if (!el) {
-      el = angular.element('<div class="backdrop ng-hide">');
-      $document[0].body.appendChild(el[0]);
-    }
-    return el;
-  }
   function retain() {
     if ( (++backdropHolds) === 1 ) {
-      $animate.removeClass(getElement(), 'ng-hide');
+      toggler.removeClass();
     }
   }
   function release() {
     if ( (--backdropHolds) === 0 ) {
-      $animate.addClass(getElement(), 'ng-hide');
+      toggler.addClass();
     }
   }
 }]);
diff --git a/js/ext/angular/src/service/ionicLoading.js b/js/ext/angular/src/service/ionicLoading.js
@@ -41,7 +41,8 @@ angular.module('ionic.service.loading', [])
   '$q',
   '$log',
   '$compile',
-function($animate, $document, $ionicTemplateLoader, $ionicBackdrop, $timeout, $q, $log, $compile) {
+  '$animateClassToggler',
+function($animate, $document, $ionicTemplateLoader, $ionicBackdrop, $timeout, $q, $log, $compile, $animateClassToggler) {
 
   var loaderInstance;
   //default value
@@ -81,13 +82,17 @@ function($animate, $document, $ionicTemplateLoader, $ionicBackdrop, $timeout, $q
         appendTo: $document[0].body
       })
       .then(function(loader) {
+
+        var toggler = $animateClassToggler(loader.element, 'ng-hide');
+
         loader.show = function(options) {
           var self = this;
           var templatePromise = options.templateUrl ?
             $ionicTemplateLoader.load(options.templateUrl) :
             //options.content: deprecated
             $q.when(options.template || options.content || '');
 
+
           if (!this.isShown) {
             //options.showBackdrop: deprecated
             this.hasBackdrop = !options.noBackdrop && options.showBackdrop !== false;
@@ -111,26 +116,16 @@ function($animate, $document, $ionicTemplateLoader, $ionicBackdrop, $timeout, $q
             }
           });
 
+          toggler.removeClass();
+          ionic.DomUtil.centerElementByMarginTwice(this.element[0]);
           this.isShown = true;
-          ionic.requestAnimationFrame(function() {
-            if (self.isShown) {
-              $animate.removeClass(self.element, 'ng-hide');
-              //Fix for ios: if we center the element twice, it always gets
-              //position right. Otherwise, it doesn't
-              ionic.DomUtil.centerElementByMargin(self.element[0]);
-              //One frame after it's visible, position it
-              ionic.requestAnimationFrame(function() {
-                ionic.DomUtil.centerElementByMargin(self.element[0]);
-              });
-            }
-          });
         };
         loader.hide = function() {
           if (this.isShown) {
             if (this.hasBackdrop) {
               $ionicBackdrop.release();
             }
-            $animate.addClass(this.element, 'ng-hide');
+            toggler.addClass();
           }
           $timeout.cancel(this.durationTimeout);
           this.isShown = false;
diff --git a/js/ext/angular/src/service/ionicPopup.js b/js/ext/angular/src/service/ionicPopup.js
@@ -327,14 +327,8 @@ function($animate, $ionicTemplateLoader, $ionicBackdrop, $log, $q, $timeout, $ro
 
           self.element.removeClass('popup-hidden');
           self.element.addClass('popup-showing active');
+          ionic.DomUtil.centerElementByMarginTwice(self.element[0]);
           focusLastButton(self.element);
-          //Fix for ios: if we center the element twice, it always gets
-          //position right. Otherwise, it doesn't
-          ionic.DomUtil.centerElementByMargin(self.element[0]);
-          //One frame after it's visible, position it
-          ionic.requestAnimationFrame(function() {
-            ionic.DomUtil.centerElementByMargin(self.element[0]);
-          });
         });
       };
       self.hide = function(callback) {
diff --git a/js/ext/angular/test/directive/ionicBackdrop.unit.js b/js/ext/angular/test/directive/ionicBackdrop.unit.js
@@ -1,38 +1,34 @@
 describe('$ionicBackdrop service', function() {
   beforeEach(module('ionic'));
 
-  beforeEach(function() {
+  beforeEach(inject(function($animate) {
     ionic.requestAnimationFrame = function(cb) { cb(); };
-  });
-
-  it('should create backdrop first time then reuse', inject(function($ionicBackdrop) {
-    var el = $ionicBackdrop._getElement();
-    var el2 = $ionicBackdrop._getElement();
-    expect(el.hasClass('backdrop')).toBe(true);
-    expect(el.hasClass('ng-hide')).toBe(true);
-    expect(el[0]).toBe(el2[0]);
   }));
 
-  it('should remove ngHide on retain', inject(function($ionicBackdrop) {
-    var el = $ionicBackdrop._getElement();
+  it('should remove ngHide on retain', inject(function($ionicBackdrop, $timeout) {
+    var el = $ionicBackdrop._element;
     expect(el.hasClass('ng-hide')).toBe(true);
     $ionicBackdrop.retain();
+    $timeout.flush();
     expect(el.hasClass('ng-hide')).toBe(false);
   }));
 
-  it('should add ngHide on retain', inject(function($ionicBackdrop) {
-    var el = $ionicBackdrop._getElement();
+  it('should add ngHide on retain', inject(function($ionicBackdrop, $timeout) {
+    var el = $ionicBackdrop._element;
     expect(el.hasClass('ng-hide')).toBe(true);
     $ionicBackdrop.retain();
+    $timeout.flush();
     expect(el.hasClass('ng-hide')).toBe(false);
     $ionicBackdrop.release();
+    $timeout.flush();
     expect(el.hasClass('ng-hide')).toBe(true);
   }));
 
-  it('should require equal releases and retains', inject(function($ionicBackdrop) {
-    var el = $ionicBackdrop._getElement();
+  it('should require equal releases and retains', inject(function($ionicBackdrop, $timeout) {
+    var el = $ionicBackdrop._element;
     expect(el.hasClass('ng-hide')).toBe(true);
     $ionicBackdrop.retain();
+    $timeout.flush();
     expect(el.hasClass('ng-hide')).toBe(false);
     $ionicBackdrop.retain();
     expect(el.hasClass('ng-hide')).toBe(false);
@@ -43,6 +39,7 @@ describe('$ionicBackdrop service', function() {
     $ionicBackdrop.release();
     expect(el.hasClass('ng-hide')).toBe(false);
     $ionicBackdrop.release();
+    $timeout.flush();
     expect(el.hasClass('ng-hide')).toBe(true);
   }));
 });
diff --git a/js/ext/angular/test/service/animateClassToggler.unit.js b/js/ext/angular/test/service/animateClassToggler.unit.js
@@ -0,0 +1,66 @@
+describe('$animateClassToggler service', function() {
+  beforeEach(module('ionic'));
+
+  var toggler, el;
+  beforeEach(inject(function($animateClassToggler) {
+    el = angular.element('<div>');
+    toggler = $animateClassToggler(el, 'foo');
+    ionic.requestAnimationFrame = function(cb) { cb(); };
+  }));
+
+  function flush() {
+    inject(function($timeout) {
+      $timeout.flush();
+    });
+  }
+
+  it('addClass should go to toggle', function() {
+    spyOn(toggler, '_toggle');
+    toggler.addClass();
+    expect(toggler._toggle).toHaveBeenCalledWith(true);
+  });
+  it('removeClass should go to toggle', function() {
+    spyOn(toggler, '_toggle');
+    toggler.removeClass();
+    expect(toggler._toggle).toHaveBeenCalledWith(false);
+  });
+  it('toggle should add class and remove class', function() {
+    toggler._toggle(true);
+    flush();
+    expect(el.hasClass('foo')).toBe(true);
+    toggler._toggle(false);
+    flush();
+    expect(el.hasClass('foo')).toBe(false);
+  });
+
+  it('toggle should set nextOperation if animating', function() {
+    toggler._animating = true;
+    toggler._toggle(true);
+    expect(toggler._nextOperation).toBe('addClass');
+    toggler._toggle(false);
+    expect(toggler._nextOperation).toBe('removeClass');
+  });
+
+  it('animate should set animating to true and animationDone when done', inject(function($rootScope, $animate) {
+    spyOn($rootScope, '$evalAsync').andCallThrough();
+    spyOn($animate, 'addClass');
+    toggler._animate('addClass');
+    expect(toggler._animating).toBe(true);
+    expect($rootScope.$evalAsync).toHaveBeenCalled();
+    flush();
+    expect($animate.addClass).toHaveBeenCalledWith(el, 'foo', toggler._animationDone);
+  }));
+
+  it('animationDone should use nextOperation and unset animating if no more', function() {
+    spyOn(toggler, '_animate').andCallThrough();
+    toggler._animating = true;
+    toggler._nextOperation = 'addClass';
+    toggler._animationDone();
+    expect(toggler._nextOperation).toBeFalsy();
+    expect(toggler._animate).toHaveBeenCalled();
+    flush();
+    expect(el.hasClass('foo')).toBe(true);
+    expect(toggler._animating).toBe(false);
+  });
+
+});
diff --git a/js/ext/angular/test/service/ionicLoading.unit.js b/js/ext/angular/test/service/ionicLoading.unit.js
@@ -41,11 +41,12 @@ describe('$ionicLoading service', function() {
         expect(loader.durationTimeout).toBeTruthy();
         expect(loader.durationTimeout.$$timeoutId).toBeTruthy();
       }));
-      it('should remove ng-hide after raf', inject(function($ionicLoading) {
+      it('should remove ng-hide', inject(function($ionicLoading, $timeout) {
         var loader = TestUtil.unwrapPromise($ionicLoading._getLoader());
         ionic.requestAnimationFrame = function(cb) { cb(); };
         expect(loader.element.hasClass('ng-hide')).toBe(true);
         loader.show({});
+        $timeout.flush();
         expect(loader.element.hasClass('ng-hide')).toBe(false);
       }));
       it('should isShown = true', inject(function($ionicLoading) {
@@ -137,7 +138,7 @@ describe('$ionicLoading service', function() {
     it('show should only removeClass after raf is still isShown', inject(function($ionicLoading) {
       var loader = TestUtil.unwrapPromise($ionicLoading._getLoader());
       var rafCallback;
-      ionic.requestAnimationFrame = function(cb) { 
+      ionic.requestAnimationFrame = function(cb) {
         rafCallback = cb;
       };
       loader.show({});
diff --git a/js/utils/dom.js b/js/utils/dom.js
@@ -187,6 +187,16 @@
       el.style.marginLeft = (-el.offsetWidth) / 2 + 'px';
       el.style.marginTop = (-el.offsetHeight) / 2 + 'px';
     },
+    //Center twice, after raf, to fix a bug with ios and showing elements
+    //that have just been attached to the DOM.
+    centerElementByMarginTwice: function(el) {
+      ionic.requestAnimationFrame(function() {
+        ionic.DomUtil.centerElementByMargin(el);
+        ionic.requestAnimationFrame(function() {
+          ionic.DomUtil.centerElementByMargin(el);
+        });
+      });
+    },
 
     /**
      * @ngdoc method
