Pasvaz / bindonce

Zero watches binding for AngularJs
2.73k stars 278 forks source link

bo-html when using jQuery instead of jqLite #78

Open naorye opened 10 years ago

naorye commented 10 years ago

bo-html does nothing when using jQuery instead of jqLite. Here is a plunkr: http://plnkr.co/edit/tjCvvxu2gG75m3WiSJAB?p=preview

Try to remove the reference to jQuery and everything will work.

Related issues: https://github.com/Pasvaz/bindonce/issues/68 https://github.com/Pasvaz/bindonce/issues/39

naorye commented 10 years ago

Look on the following implementation of angular.js (1.2.14):

... (line 17559)
var ngBindHtmlDirective = ['$sce', '$parse', function($sce, $parse) {
  return function(scope, element, attr) {
    element.addClass('ng-binding').data('$binding', attr.ngBindHtml);
    var parsed = $parse(attr.ngBindHtml);
    function getStringValue() { return (parsed(scope) || '').toString(); }
    scope.$watch(getStringValue, function ngBindHtmlWatchAction(value) {
      element.html($sce.getTrustedHtml(parsed(scope)) || '');
    });
  };
}];
...

The following line uses $sce.getTrustedHtml which you are not using. Adding it to line 183 of bindonce.js will solve this issue. Angular has default implementation to $sce even when the user didn't include it as a dependency.

Pasvaz commented 10 years ago

The problem is that previous versions of Angular don't support $sce. I'm going to push a fix that will work also with AngularJs < 1.2

naorye commented 10 years ago

I think you can use the $injector.has() method to check for dependency. This way you can distinguish between $sce and $sanitize. But it is added only on 1.1.5. Maybe you can use try-catch with $injector.get(). What do you think?

Pasvaz commented 10 years ago

Yes, that's what I did:

function getProvider(name) {
  try {
    return $injector.get(name);
  } catch (e) {
    return false;
  }
}

plus, in the coming version there is a bindonce provider that let you configure some parameters, one of these is the opportunity to turn sanitization on/off for html. I'm not totally sure it is the best approach, I'm considering adding an html-unsafe directive, what do you think?

Pasvaz commented 10 years ago

I think the html-unsafe is a better choice because gives you the possibility to apply sanitization per element rather than per application, let's see if someone else wants to add his own opinion while I finish the development.

mikestopcontinues commented 10 years ago

It would be really useful to have html-unsafe around. Though isn't bo-html is currently unsafe even with ngSantize? So maybe html-safe?

naorye commented 10 years ago

Hi Pasvaz, I really don't think that html-unsafe directive is a good thing to add for two reasons:

  1. Using unsafe html is not a good practice.
  2. It is really easy to make using a filter:
app.filter('unsafe', function($sce) {
    return function(val) {
        return $sce.trustAsHtml(val);
    };
});

And then:

<ANY ng-bind-html="value | unsafe"></ANY>

NaorYe

mikestopcontinues commented 10 years ago

Currently, we use a "safe" filter with bo-html, but you shouldn't need to do either. Unsafe html is only bad practice when it's user input. We use it for internationalization. That's hardly a fringe use case.

Pasvaz commented 10 years ago

Hi @naorye what's the difference between creating your own filter or having a directive for unsafe html? If you consider it a bad practice also having a filter is a bad practice and should be avoided. In the other hand if you found yourself in the need to create a custom filter, you could also get some benefits from the bo-html-unsafe directive like working with both $sce and $sanitize modules.

naorye commented 10 years ago

@Pasvaz, It is more easier to use a built in directive than write a custom filter. Bad practice is something I don't want to allow easily in my plugin. Therefor I would not add such a directive in it. But this is of course only a point of view and this issue is not so important. @mikestopcontinues, unsafe html is a bad practice when the bounded data come from an input,but I don't consider it as a fringe use case.

Pasvaz commented 10 years ago

@naorye I see your point, however ng-bind-html-unsafe was present as core directive in the previous version of Angular and it's a common use case so I think we can mark it as not recomended in the readme and include it.

naorye commented 10 years ago

@Pasvaz Sounds fine. Do you need help in something?

Pasvaz commented 10 years ago

Hi @naorye I'm working on this issue right now and I had the chance to dig into your plunker. In order to dissipate any doubts I want to add my considerations about it: the reason why your custom filter doesn't work when jquery is present is because myFilter return the wrapped trusted object and while angular is aware of it, jquery's html() doesn't know how to use it. For instance if you modify your filter to return the value of the wrapper it works again with both angular and jquery:

return $sce.trustAsHtml(html).valueOf()

In this case, however, you cannot use this filter also for ng-bind-html. Anyway if you just to use unsafe html with the current version of bindonce, remove the filter for bo-html and it will be fine also with jquery.

Also, $sce.getTrustedHtml itself doesn't fix the issue, it relies on $sanitize (if present) to sanitize the html, when $sanitize is not present it basically unwrap the value wrapped by $sce.trustAsHtml, that's why it worked on your example, but if $sce.getTrustedHtml is used directly on the html it just throw an exception.

The solution I'm going to push fixes both the use cases but it will not allow anymore unsafe html by default, so it'll be a breaking change. For this purpose it'll be available another directive, the above mentioned bo-html-unsafe

naorye commented 10 years ago

@Pasvaz You are right. Totally agreed.

scotthong commented 10 years ago

Hi, I am having issues making bindonce.js passing "jshint" that it complains about "Don't make functions within a loop." at the following two places: case 'boSwitch': ... transclude(selectedTransclude, binder.scope.$new()); case 'attr': ... newValue = binder.scope.$eval(binder.attrs[attrKey]); binder.element.attr(newAttr, newValue);

Could you suggest a workaround?

Thanks!