feat(tooltip): tooltip now accepts and sanitizes HTML content

[joshdmiller]

The tooltip uses the $sanitize service on the content passed to the tooltip attribute and then binds that content in the template using ng-bind-html-unsafe. The reason I used the service instead of the ng-bind-html attribute was to be able to design the tests to run exclusively off the scope, rather than requiring DOM knowledge, which would simply break with any template customization by the user.

The ngSanitize module is now included during testing and a note was added to the requirements section of the docs to specify that module is required, but we may need to make it more clear.

Closes #142.

[joshdmiller]

Sorry for the second commit. I always have to change the browser section of the Testacular config so I've gotten used to just ignoring it in git status. But of course I had to push a legitimate change this time...

On a different note, I would like to push a few more tooltip and popover changes this week, but I'll wait until this is approved by y'all so as to avoid any merge-related funk.

[pkozlowski-opensource]

LGTM, just would like to clarify one thing based on my comment. Great stuff, you've figured out all the places to change!

I guess we will update README as well with more explicit instructions as I'm sure that people will keep forgetting to include ngSanitize :-(

[joshdmiller]

I did actually choose $sanitize intentionally, but for a cheating reason: I wanted to be able to write the spec without referencing the DOM. So instead of having to test element.find('.inner-tooltip').html() for the right content, I can simply check the scope var. This way the test won't break when someone changes the template.

Doh! I did forget to change the README. It's always something...

[ajoslin]

I dunno about the requirement of ngSanitize. I know it's technically angular core, but we will get sooo many people asking about module 'ngSanitize' not found errors 😕

I think it would be better to just not sanitize it. Even though that's slightly dangerous, isn't it the user of the library's role to check the values he puts into the title?

[joshdmiller]

@ajoslin - That's a valid point and there will be many angry posts. But I would look at it from the perspective of least work. We can call $sanitize once in the tooltip, or the user can do some manipulation using forEach, which can be more tough if the tooltip content is nested in a complex data structure. Seems simpler this way to me, but I don't feel strongly about it.

[ajoslin]

What do you mean by "the user can do some manipulation with forEach"?

Can't you just use ng-bind-html-unsafe skipping the current $sanitize step?

[joshdmiller]

Yeah, that was pretty vague. :-)

My assumption was that most of the time, any content populated from the server that would end up in a tooltip was probably part of some data set; if it's complex, then the user would have to do something like this in the controller:

var $scope.itemsCopy = []; // so as not to overwrite the server data
angular.forEach( 'items', function ( item, idx ) { 
  item.prop.subprop = $sanitize( item.prop.subprop );
  $scope.itemsCopy.push( item );
});

So I meant that we could include ngSanitize and have one line of code in the tooltip directive, or the user could have to write a (potentially) complex routine in their controller to get the data sanitized themselves.

But at the end of the hangout earlier, we just decided not to include ngSanitize at all, and to offer two directives: tooltip and tooltip-html-unsafe; the former would render as text (i.e. current functionality) and the latter would use ng-bind-html-unsafe. Because we're factoring out most of the tooltip logic (because we re-used it in the popover), the two directives will be super thin, so it'll be pretty easy to have two of them.

So actually, I'm just going to close this PR. :-)