search

showdownjs / ng-showdown

Angular integration for Showdown
BSD 3-Clause "New" or "Revised" License
105 stars 33 forks source link

Fix markdown directive #15

Closed phw closed 9 years ago

phw commented 9 years ago

This is an attempt to fix #14 . The markdownToHtmlDirective is currently broken in multiple ways.

  1. $sce is not used properly ($sce.trustAsHtml returns an object. not a string)
  2. The deprecated sdModelToHtml attribute name is hardcoded in the directive

I fixed the second issue by adding a builder function that constructs the final directive function, making the directive name configurable. A much easier and cleaner option would be of course to just ditch sdModelToHtml altogether. Given the broken state of the current release that's probably best.

phw commented 9 years ago

Tests are locally still failing:

AssertionError: expected '<h1>Title</h1>' to equal '<h1 id="title">Title</h1>'

Well, that has to be expected, $sce (and also $sanitize) will strip some stuff deemed unsafe for web applications. Not sure how you would like to handle this, but my suggestion would be to keep the code as is and change the tests. You could also provide a markdownToHtmlUnsafe attribute (then not using $sce).

phw commented 9 years ago

I have updated the test for the directive. Travis is of course still failing, but that's due to #12 (and I also don't quite understand why)

SyntaxRules commented 9 years ago

$sce.trustAsHtml and $sce.getTrustedHtml do very different things. One accepts, passing it to privileged directives and the other sanitizes inputs.

If you want any html the showdown output to be displayed, you would use $sce.trustAsHtml.

So the question is, should ng-showdown display any html output from showdown, or should it sanitize the output?

phw commented 9 years ago

$sce.trustAsHtml and $sce.getTrustedHtml do very different things. One accepts, passing it to privileged directives and the other sanitizes inputs.

Exactly, that's the whole point here ;) But 701930d45310af43119bd73c46e10438108353f1 treated both as interchangeable. So you either want to sanitize or you don't (which makes perfect sense here), but in the later case you don't need that whole $sce monkey dance at all since the HTML will get written directly to the DOM and not passed around.

phw commented 9 years ago

Closing this in favor of #17