fix(typeahead): Typeahead uses ngSanitize when present #3463

fernando-sendMail · 2015-03-29T16:22:35Z

Hey there!

This pull request addresses the issue #2884 which mentions the use of the attribute of bind-html-unsafe attribute in the default template/typeahead/typeahead-match.html.

So basically, I changed a little the testing process, considering what @chrisirhc said about the fix, here's the implemented solution:

Added the angular-sanitize.js dependency to the misc/test-lib/ folder (the angular-sanitize.js is the same version as the angular.js included).
Changed the karma.conf.js file to add this dependency.
Added a line in the demo file to ship it with the dist folder.
Changed the typeaheadHilghlight filter:
- Checks if $sanitize is available to use, if not then a warning is printed to the console using the $log service, this will happen when the filer is requested by angular. The ngSanitize can be imported as part of the module that's using the typeahead directive. The reason of why the ngSanitize module is not imported directly on the ui.bootstrap.typeahead module is because it will add another dependency to the project, needless to say it will not work if not present.
- When $sanitize is present it will use it to sanitize the string itself if it contains any sequence of < followed by any combination of chars, and an ending >, if not, then the string is wrapped in an object that ng-bind-html need to be displayed with the $sce.trustAsHtml method.
- And finally the <strong> tags are added to the matched characters.
On the file src/typeahead/test/typeahead-highlight.spec.js the returned value of the filter need to be unwrapped.
Created another spec file (src/typeahead/test/typeahead-highlight-ngsanitize.spec.js) that will include the ngSanitize module and test that tags and attributes are being escaped.

And that's it, I think that's all.

I'm looking forward to your feedback and comments. Thanks!

chrisirhc · 2015-03-31T16:54:06Z

src/typeahead/typeahead.js

    }

    return function(matchItem, query) {
-      return query ? ('' + matchItem).replace(new RegExp(escapeRegexp(query), 'gi'), '<strong>$&</strong>') : matchItem;
+      if($sanitize) { // Is the $sanitize service available?
+        if(/<.*>/g.test(matchItem)){


I think this check isn't required. It's safe to $sanitize something even if it doesn't contain any HTML tags.

chrisirhc · 2015-03-31T17:00:04Z

Did a review. Good work here. Also, PR needs rebasing.

fernando-sendMail · 2015-04-03T01:57:07Z

Hi @chrisirhc
Thanks for your feedback, I polished the code according to your comments. I already pushed the commit.

karianna · 2015-04-03T16:07:22Z

Can't be merged at present

fernando-sendMail · 2015-04-03T20:53:59Z

@karianna I'm sorry, I'm kind of new to this, can I or should I do something about that?

karianna · 2015-04-04T14:34:25Z

@fernando-sendMail you'll need to rebase, e.g.

do a git pull angular-ui/bootstrap master (I'm assuming you've aliased the angular-ui bootstrap remote)
resolve any conflicts
git push into your remote branch again.

YMMV here, others rebase in different ways :-).

fernando-sendMail · 2015-04-05T17:03:29Z

@karianna Thanks for the info!

chrisirhc · 2015-04-05T17:33:03Z

karma.conf.js

@@ -18,6 +18,7 @@ module.exports = function(config) {
      'misc/test-lib/jquery-1.8.2.min.js',
      'node_modules/angular/angular.js',
      'node_modules/angular-mocks/angular-mocks.js',
+      'misc/test-lib/angular-sanitize.js',


Should change this to use node_modules/angular-sanitize/angular-sanitize.js and modify package.json to include angular-sanitize as a module since we changed this in 5536e96 so that we don't need to include the angular files in this repository.

chrisirhc · 2015-04-05T17:56:57Z

Thanks @fernando-sendMail for your work on this, I did some further review.

chrisirhc · 2015-04-06T05:58:14Z

Also note, should remove typeahead's dependency on ui.bootstrap.bindHtml module as it's no longer used after this change ( 👍 hooray!)

fernando-sendMail · 2015-04-07T04:43:17Z

@chrisirhc Thanks for the review, I'll be working on this over the week.

fernando-sendMail · 2015-04-10T03:29:55Z

Hey @chrisirhc I committed the latest stuff with the fixes and modifications to the test.

fernando-sendMail · 2015-08-02T18:50:36Z

@wesleycho Sure, I'll work on that.

fernando-sendMail · 2015-08-09T20:23:26Z

Hey @wesleycho I think I resolved all the conflicts, the PR is ready for review again

wesleycho · 2015-08-09T20:39:02Z

Can you squash into one commit @fernando-sendMail ?

wesleycho · 2015-08-09T20:39:49Z

package.json

@@ -1,38 +1,37 @@
 {
  "author": "https://github.com/angular-ui/bootstrap/graphs/contributors",
  "name": "angular-ui-bootstrap",
-  "version": "0.13.4-SNAPSHOT",
+  "version": "0.13.0-SNAPSHOT",


Change this back to 4

Revert all changes to this file excepting the addition of angular-sanitize

wesleycho · 2015-08-09T20:43:05Z

With these minor reverts/changes, this should be good to merge. Squashing the commits makes it easier to replay the change on top of master when we merge it, which is very useful for us.

fernando-sendMail · 2015-08-09T21:58:03Z

Hey @wesleycho I tried using git rebase -i <commit to squash> but that didn't worked for me, it created an aditional commit instead, am I doing something wrong?

wesleycho · 2015-08-09T22:08:23Z

You can do git rebase -i HEAD~[number of commits without brackets]

wesleycho · 2015-08-09T22:10:21Z

Keep in mind once you do this, you will have to force push to the remote to overwrite the history with the new version.

fernando-sendMail · 2015-08-17T04:59:17Z

@wesleycho Finally I've made it, it's ready for review again.

wesleycho · 2015-08-17T14:17:41Z

Thanks a lot for sticking with it through the time, this is good work!

fernando-sendMail · 2015-08-17T18:05:06Z

No problem, thanks a lot for the support guys!
On Mon, Aug 17, 2015 at 7:18 AM Wesley Cho [email protected] wrote:

Thanks a lot for sticking with it through the time, this is good work!

—
Reply to this email directly or view it on GitHub
#3463 (comment)
.

karianna added component: typeahead type: tests needs: review labels Mar 30, 2015

karianna assigned chrisirhc Mar 30, 2015

chrisirhc added type: enhancement and removed type: tests labels Mar 31, 2015

chrisirhc reviewed Mar 31, 2015
View reviewed changes

chrisirhc added needs: work and removed needs: review labels Mar 31, 2015

chrisirhc added this to the Backlog milestone Mar 31, 2015

chrisirhc reviewed Apr 5, 2015
View reviewed changes

karianna added needs: review and removed needs: work labels Apr 10, 2015

This was referenced Aug 2, 2015

feat(typeahead): *BREAKING CHANGE* switch to ngBindHtml #4074

Closed

Switch from bind-html-unsafe to ng-bind-html in typeahead #4073

Closed

fernando-sendMail force-pushed the issue-2884 branch from be28195 to 69246f8 Compare August 9, 2015 20:02

wesleycho added needs: squash and removed needs: work labels Aug 9, 2015

wesleycho reviewed Aug 9, 2015
View reviewed changes

wesleycho added the needs: work label Aug 9, 2015

fernando-sendMail force-pushed the issue-2884 branch from c220da5 to 844d376 Compare August 9, 2015 21:26

fernando-sendMail force-pushed the issue-2884 branch from ad6dd18 to 4a3ccf7 Compare August 9, 2015 22:18

fernando-sendMail force-pushed the issue-2884 branch 2 times, most recently from 8a66fd9 to 12292bb Compare August 17, 2015 04:22

fix(typeahead): Typeahead uses ngSanitize when present

5a41d48

fernando-sendMail force-pushed the issue-2884 branch from 08a18f3 to 5a41d48 Compare August 17, 2015 04:52

wesleycho modified the milestones: 0.13.4 (Performance), 0.13.x Aug 17, 2015

wesleycho removed needs: squash needs: work labels Aug 17, 2015

wesleycho closed this in bb9fa1a Aug 17, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(typeahead): Typeahead uses ngSanitize when present #3463

fix(typeahead): Typeahead uses ngSanitize when present #3463

fernando-sendMail commented Mar 29, 2015

chrisirhc Mar 31, 2015

chrisirhc commented Mar 31, 2015

fernando-sendMail commented Apr 3, 2015

karianna commented Apr 3, 2015

fernando-sendMail commented Apr 3, 2015

karianna commented Apr 4, 2015

fernando-sendMail commented Apr 5, 2015

chrisirhc Apr 5, 2015

chrisirhc commented Apr 5, 2015

chrisirhc commented Apr 6, 2015

fernando-sendMail commented Apr 7, 2015

fernando-sendMail commented Apr 10, 2015

fernando-sendMail commented Aug 2, 2015

fernando-sendMail commented Aug 9, 2015

wesleycho commented Aug 9, 2015

wesleycho Aug 9, 2015

wesleycho Aug 9, 2015

wesleycho commented Aug 9, 2015

fernando-sendMail commented Aug 9, 2015

wesleycho commented Aug 9, 2015

wesleycho commented Aug 9, 2015

fernando-sendMail commented Aug 17, 2015

wesleycho commented Aug 17, 2015

fernando-sendMail commented Aug 17, 2015

fix(typeahead): Typeahead uses ngSanitize when present #3463

fix(typeahead): Typeahead uses ngSanitize when present #3463

Conversation

fernando-sendMail commented Mar 29, 2015

chrisirhc Mar 31, 2015

Choose a reason for hiding this comment

chrisirhc commented Mar 31, 2015

fernando-sendMail commented Apr 3, 2015

karianna commented Apr 3, 2015

fernando-sendMail commented Apr 3, 2015

karianna commented Apr 4, 2015

fernando-sendMail commented Apr 5, 2015

chrisirhc Apr 5, 2015

Choose a reason for hiding this comment

chrisirhc commented Apr 5, 2015

chrisirhc commented Apr 6, 2015

fernando-sendMail commented Apr 7, 2015

fernando-sendMail commented Apr 10, 2015

fernando-sendMail commented Aug 2, 2015

fernando-sendMail commented Aug 9, 2015

wesleycho commented Aug 9, 2015

wesleycho Aug 9, 2015

Choose a reason for hiding this comment

wesleycho Aug 9, 2015

Choose a reason for hiding this comment

wesleycho commented Aug 9, 2015

fernando-sendMail commented Aug 9, 2015

wesleycho commented Aug 9, 2015

wesleycho commented Aug 9, 2015

fernando-sendMail commented Aug 17, 2015

wesleycho commented Aug 17, 2015

fernando-sendMail commented Aug 17, 2015