fix(ng-bind-html): watch the unwrapped value using $sce.valueOf() (instead of toString())

mprobst · gkalpak · commit 1c1c9b27cbb5 · 2016-04-28T20:25:13.000+03:00
Custom `$sce` implementations might not provide a `toString()` method on the wrapped object, or it might be compiled away in non-debug mode. Watching the unwrapped value (retrieved using `$sce.valueOf()`) fixes the problem. The performance of this should be equivalent - `toString()` on objects usually touches all fields, plus we will also avoid the (potentially big) string allocation. Fixes angular#14526 Closes angular#14527
diff --git a/src/ng/directive/ngBind.js b/src/ng/directive/ngBind.js
@@ -188,18 +188,19 @@ var ngBindHtmlDirective = ['$sce', '$parse', '$compile', function($sce, $parse,
     restrict: 'A',
     compile: function ngBindHtmlCompile(tElement, tAttrs) {
       var ngBindHtmlGetter = $parse(tAttrs.ngBindHtml);
-      var ngBindHtmlWatch = $parse(tAttrs.ngBindHtml, function getStringValue(value) {
-        return (value || '').toString();
+      var ngBindHtmlWatch = $parse(tAttrs.ngBindHtml, function sceValueOf(val) {
+        // Unwrap the value to compare the actual inner safe value, not the wrapper object.
+        return $sce.valueOf(val);
       });
       $compile.$$addBindingClass(tElement);
 
       return function ngBindHtmlLink(scope, element, attr) {
         $compile.$$addBindingInfo(element, attr.ngBindHtml);
 
         scope.$watch(ngBindHtmlWatch, function ngBindHtmlWatchAction() {
-          // we re-evaluate the expr because we want a TrustedValueHolderType
-          // for $sce, not a string
-          element.html($sce.getTrustedHtml(ngBindHtmlGetter(scope)) || '');
+          // The watched value is the unwrapped value. To avoid re-escaping, use the direct getter.
+          var value = ngBindHtmlGetter(scope);
+          element.html($sce.getTrustedHtml(value) || '');
         });
       };
     }
diff --git a/test/ng/directive/ngBindSpec.js b/test/ng/directive/ngBindSpec.js
@@ -142,6 +142,16 @@ describe('ngBind*', function() {
         expect(angular.lowercase(element.html())).toEqual('<div onclick="">hello</div>');
       }));
 
+      it('should update html', inject(function($rootScope, $compile, $sce) {
+        element = $compile('<div ng-bind-html="html"></div>')($rootScope);
+        $rootScope.html = 'hello';
+        $rootScope.$digest();
+        expect(angular.lowercase(element.html())).toEqual('hello');
+        $rootScope.html = 'goodbye';
+        $rootScope.$digest();
+        expect(angular.lowercase(element.html())).toEqual('goodbye');
+      }));
+
       it('should one-time bind if the expression starts with two colons', inject(function($rootScope, $compile) {
         element = $compile('<div ng-bind-html="::html"></div>')($rootScope);
         $rootScope.html = '<div onclick="">hello</div>';
@@ -176,7 +186,18 @@ describe('ngBind*', function() {
         expect(angular.lowercase(element.html())).toEqual('<div onclick="">hello</div>');
       }));
 
-      it('should watch the string value to avoid infinite recursion', inject(function($rootScope, $compile, $sce) {
+      it('should update html', inject(function($rootScope, $compile, $sce) {
+        element = $compile('<div ng-bind-html="html"></div>')($rootScope);
+        $rootScope.html = $sce.trustAsHtml('hello');
+        $rootScope.$digest();
+        expect(angular.lowercase(element.html())).toEqual('hello');
+        $rootScope.html = $sce.trustAsHtml('goodbye');
+        $rootScope.$digest();
+        expect(angular.lowercase(element.html())).toEqual('goodbye');
+      }));
+
+      it('should not cause infinite recursion for trustAsHtml object watches',
+          inject(function($rootScope, $compile, $sce) {
         // Ref: https://github.com/angular/angular.js/issues/3932
         // If the binding is a function that creates a new value on every call via trustAs, we'll
         // trigger an infinite digest if we don't take care of it.
@@ -188,6 +209,33 @@ describe('ngBind*', function() {
         expect(angular.lowercase(element.html())).toEqual('<div onclick="">hello</div>');
       }));
 
+      it('should handle custom $sce objects', function() {
+        function MySafeHtml(val) { this.val = val; }
+
+        module(function($provide) {
+          $provide.decorator('$sce', function($delegate) {
+            $delegate.trustAsHtml = function(html) { return new MySafeHtml(html); };
+            $delegate.getTrustedHtml = function(mySafeHtml) { return mySafeHtml.val; };
+            $delegate.valueOf = function(v) { return v instanceof MySafeHtml ? v.val : v; };
+            return $delegate;
+          });
+        });
+
+        inject(function($rootScope, $compile, $sce) {
+          // Ref: https://github.com/angular/angular.js/issues/14526
+          // Previous code used toString for change detection, which fails for custom objects
+          // that don't override toString.
+          element = $compile('<div ng-bind-html="getHtml()"></div>')($rootScope);
+          var html = 'hello';
+          $rootScope.getHtml = function() { return $sce.trustAsHtml(html); };
+          $rootScope.$digest();
+          expect(angular.lowercase(element.html())).toEqual('hello');
+          html = 'goodbye';
+          $rootScope.$digest();
+          expect(angular.lowercase(element.html())).toEqual('goodbye');
+        });
+      });
+
       describe('when $sanitize is available', function() {
         beforeEach(function() { module('ngSanitize'); });
 
