From e2663f62b0fbb8b9ce2e706b821a135e0bc7e885 Mon Sep 17 00:00:00 2001
From: Dhruv Manek <dmanek@gmail.com>
Date: Fri, 4 Nov 2011 17:13:56 -0700
Subject: [PATCH] feat(ng:style): compatibility + perf improvements

- better compatibility with 3rd party code - we clober 3rd party
  style only if it direcrtly collides with 3rd party styles
- better perf since it doesn't execute stuff on every digest
- lots of tests
---
 src/directives.js      | 20 +++--------
 test/directivesSpec.js | 79 +++++++++++++++++++++++++++++++++---------
 2 files changed, 68 insertions(+), 31 deletions(-)

diff --git a/src/directives.js b/src/directives.js
index f37cdde6c4d5..a806e9285b00 100644
--- a/src/directives.js
+++ b/src/directives.js
@@ -802,21 +802,11 @@ angularDirective("ng:hide", function(expression, element){
      </doc:scenario>
    </doc:example>
  */
-angularDirective("ng:style", function(expression, element){
-  // TODO(i): this is inefficient (runs on every $digest) and obtrusive (overrides 3rd part css)
-  //   we should change it in a similar way as I changed ng:class
-  return function(element){
-    var resetStyle = getStyle(element);
-    this.$watch(function(scope){
-      var style = scope.$eval(expression) || {}, key, mergedStyle = {};
-      for(key in style) {
-        if (resetStyle[key] === undefined) resetStyle[key] = '';
-        mergedStyle[key] = style[key];
-      }
-      for(key in resetStyle) {
-        mergedStyle[key] = mergedStyle[key] || resetStyle[key];
-      }
-      element.css(mergedStyle);
+angularDirective("ng:style", function(expression, element) {
+  return function(element) {
+    this.$watch(expression, function(scope, newStyles, oldStyles) {
+      if (oldStyles) forEach(oldStyles, function(val, style) { element.css(style, '');});
+      if (newStyles) element.css(newStyles);
     });
   };
 });
diff --git a/test/directivesSpec.js b/test/directivesSpec.js
index 8c07cf70a183..e92cb719a488 100644
--- a/test/directivesSpec.js
+++ b/test/directivesSpec.js
@@ -363,35 +363,82 @@ describe("directive", function() {
 
 
   describe('ng:style', function() {
+
     it('should set', function() {
       var scope = compile('<div ng:style="{height: \'40px\'}"></div>');
       scope.$digest();
       expect(element.css('height')).toEqual('40px');
     });
 
+
     it('should silently ignore undefined style', function() {
       var scope = compile('<div ng:style="myStyle"></div>');
       scope.$digest();
       expect(element.hasClass('ng-exception')).toBeFalsy();
     });
 
-    it('should preserve and remove previous style', function() {
-      var scope = compile('<div style="height: 10px;" ng:style="myStyle"></div>');
-      scope.$digest();
-      expect(getStyle(element)).toEqual({height: '10px'});
-      scope.myStyle = {height: '20px', width: '10px'};
-      scope.$digest();
-      expect(getStyle(element)).toEqual({height: '20px', width: '10px'});
-      scope.myStyle = {};
-      scope.$digest();
-      expect(getStyle(element)).toEqual({height: '10px'});
-    });
-  });
 
-  it('should silently ignore undefined ng:style', function() {
-    var scope = compile('<div ng:style="myStyle"></div>');
-    scope.$digest();
-    expect(element.hasClass('ng-exception')).toBeFalsy();
+    describe('preserving styles set before and after compilation', function() {
+      var scope, preCompStyle, preCompVal, postCompStyle, postCompVal;
+
+      beforeEach(function() {
+        preCompStyle = 'width';
+        preCompVal = '300px';
+        postCompStyle = 'height';
+        postCompVal = '100px';
+        element = jqLite('<div ng:style="styleObj"></div>');
+        element.css(preCompStyle, preCompVal);
+        jqLite(document.body).append(element);
+        scope = compile(element);
+        scope.styleObj = {'margin-top': '44px'};
+        scope.$apply();
+        element.css(postCompStyle, postCompVal);
+      });
+
+      afterEach(function() {
+        element.remove();
+      });
+
+
+      it('should not mess up stuff after compilation', function() {
+        element.css('margin', '44px');
+        expect(element.css(preCompStyle)).toBe(preCompVal);
+        expect(element.css('margin-top')).toBe('44px');
+        expect(element.css(postCompStyle)).toBe(postCompVal);
+      });
+
+
+      it('should not mess up stuff after $apply with no model changes', function() {
+        element.css('padding-top', '33px');
+        scope.$apply();
+        expect(element.css(preCompStyle)).toBe(preCompVal);
+        expect(element.css('margin-top')).toBe('44px');
+        expect(element.css(postCompStyle)).toBe(postCompVal);
+        expect(element.css('padding-top')).toBe('33px');
+      });
+
+
+      it('should not mess up stuff after $apply with non-colliding model changes', function() {
+        scope.styleObj = {'padding-top': '99px'};
+        scope.$apply();
+        expect(element.css(preCompStyle)).toBe(preCompVal);
+        expect(element.css('margin-top')).not.toBe('44px');
+        expect(element.css('padding-top')).toBe('99px');
+        expect(element.css(postCompStyle)).toBe(postCompVal);
+      });
+
+
+      it('should overwrite original styles after a colliding model change', function() {
+        scope.styleObj = {'height': '99px', 'width': '88px'};
+        scope.$apply();
+        expect(element.css(preCompStyle)).toBe('88px');
+        expect(element.css(postCompStyle)).toBe('99px');
+        scope.styleObj = {};
+        scope.$apply();
+        expect(element.css(preCompStyle)).not.toBe('88px');
+        expect(element.css(postCompStyle)).not.toBe('99px');
+      }); 
+    });
   });