Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bo-html when using jQuery instead of jqLite #78

Open
naorye opened this issue Apr 19, 2014 · 15 comments
Open

bo-html when using jQuery instead of jqLite #78

naorye opened this issue Apr 19, 2014 · 15 comments

Comments

@naorye
Copy link

naorye commented Apr 19, 2014

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:
#68
#39

@naorye
Copy link
Author

naorye commented Apr 19, 2014

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
Copy link
Owner

Pasvaz commented Apr 19, 2014

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
Copy link
Author

naorye commented Apr 19, 2014

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
Copy link
Owner

Pasvaz commented Apr 20, 2014

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
Copy link
Owner

Pasvaz commented Apr 20, 2014

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
Copy link

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
Copy link
Author

naorye commented Apr 23, 2014

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
Copy link

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
Copy link
Owner

Pasvaz commented Apr 24, 2014

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
Copy link
Author

naorye commented Apr 24, 2014

@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
Copy link
Owner

Pasvaz commented Apr 24, 2014

@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
Copy link
Author

naorye commented Apr 24, 2014

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

albertov pushed a commit to albertov/bindonce that referenced this issue May 9, 2014
@Pasvaz
Copy link
Owner

Pasvaz commented May 9, 2014

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
Copy link
Author

naorye commented May 9, 2014

@Pasvaz You are right. Totally agreed.

@scotthong
Copy link

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants