fix(ng-bind-html): deep watch the actual $sce value, not its toString.

[mprobst]

Custom $sce implementations might not have a toString(), or it might be
compiled away in non-debug mode. Deep watching the object is the correct fix
with Angular's regular watch semantics.

The performance of this should be equivalent - toString() on objects usually
touches all fields, so a deep object watch will be in the same ballpark, except
that it avoids the (potentially big) string allocation.

Fixes #14526.

[mprobst]

@rjamet FYI

[IgorMinar]

update the comment please

[mprobst]

fixed.

[IgorMinar]

lgtm.

@gkalpak, @petebacondarwin can you please help to merge this in?

[gkalpak]

FWIW, I don't think performance will be the same (in all cases). Running angular.equals() on every digest is more expensive that comparing strings. But ngBindHtml shouldn't be so common, so the overall impact won't make any practical difference.

I wonder whether it would be worth it to use toString() if the built-in $sceDelegate is used or to always use $sce.valueOf() (with swallow $watch) instead of toString() ?

Anyway, I'll merge this once Travis is green and we can make further changes in subsequent PRs.

[gkalpak]

The failures are legitimate. Basically, due to how equals() works (and how $sceDelegate builds trusted types), any vaues returned by $sce.trustAsHtml('...') are considered identical.

Here is a failing testcase:

inject(function($compile, $rootScope, $sce) {
  $rootScope.getHtml = function() { return $sce.trustAsHtml($rootScope.snippet); };
  element = $compile('<div ng-bind-html="getHtml()"></div>')($rootScope);

  $rootScope.$apply('snippet = 1');
  expect(element.text()).toBe('1');

  $rootScope.$apply('snippet = 2');
  expect(element.text()).toBe('2');
      // It is still '1', because
      // `angular.equals($sce.trustAsHtml('1'), $sce.trustAsHtml('2')) === true` 
});

[gkalpak]

We can fix this by exposing the trusted value as a public property on $sceDelegate's wrapper type (so that angular.equals detects them as different), but this change might still break existing implementations of custom $sceDelegates (the same way it broke the built-in one).

[petebacondarwin]

@mprobst can you take a look at @gkalpak's comments?

[rjamet]

Exposing the internal trusted value is what $sce.valueOf does, and it returns non-sce types as-is. It's part of the public interface of the $sce, so I think it's the right tool here. (code: https://github.com/angular/angular.js/blob/master/src/ng/sce.js#L330)

[mprobst]

@gkalpak you're right re performance, assuming most of the time the string return is reference identical and we don't need to compare a (potentially long) string, we should be fine. But it should be a wash indeed, and the updated PR should have the same characteristics as before.

@rjamet thanks for the tip with valueOf, that's indeed the right approach. I've amended the PR. I also added a unit test for ngBindHtml updating values on digest loops, i.e. the that the specific watch getter that it uses actually fires.

[gkalpak]

LGTM

[gkalpak]

I tweaked the commit message a bit (because it was describing the previous approach with deep-watching) and merged.

[mprobst]

Thanks @gkalpak!