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

Add raw html bind annotation #3362

Closed
wants to merge 4 commits into from
Closed

Add raw html bind annotation #3362

wants to merge 4 commits into from

Conversation

shawncplus
Copy link

This adds a new {# #} annotation which will bind to innerHTML instead of textContent. By design it does not work on compound bindings for some reason I can't really justify. After the 5th or so request in the Polymer slack on advice on how to bind a value with html I threw this together. Please don't ban me for this insanity.

Usage:

<dom-module id="x-foo">
  <template>
    <p class="message">{#messageBody#}</p>
  </template>
  <script>
  Polymer({
    is: 'x-foo',
    properties: {
      messageBody: {
        value: '<b>Some html string here</b>'
      }
    }
  });
  </script>
</dom-module>

@miztroh-zz
Copy link

Doesn't this already work with <div inner-h-t-m-l="{{html}}"></div>?

@shawncplus
Copy link
Author

Yes, except I consider that an absolutely horrendous hack which is not documented and is simply an artifact of how bindings to attributes work

@BLamy
Copy link

BLamy commented Feb 3, 2016

If it's a do not use unless you know what you are doing feature, it makes sense for it to be hidden behind a horrendous hack.

@miztroh-zz
Copy link

Say what you will, but it least it doesn't add new syntax.

@arthurevans
Copy link

This says "safe," but I don't think it actually does anything to ensure the bound text doesn't do Bad Things any more than inner-h-t-m-l.

@shawncplus
Copy link
Author

@arthurevans True, as I mentioned to @BLamy in slack I'm not actually expecting this to be accepted, more start a conversation since binding HTML isn't exactly a niche problem. And right now the only answer is happening upon some github issue somewhere where someone said "inner-h-t-m-l worked for me." Without making Polymer opinionated to Caja or something it would rely on the user sanitizing their HTML. Which they should be doing anyway but "You should have done it right" isn't the best design philosophy, I fully admit. And by "safe" it's meant to be "assumed safe"; naming isn't my forte

@shawncplus shawncplus changed the title Add safe html bind annotation Add html bind annotation Feb 3, 2016
@jonvuri
Copy link

jonvuri commented Feb 3, 2016

Without knowing a whole ton about it, it seems like it would be prohibitively expensive (both in the library size and in runtime compute) to have a syntax that enforces safety, since it would involve processing and ensuring the sanitization of the HTML. However, there are legitimate uses for inserting HTML declaratively. For instance, in my own case, I need to insert a string dynamically based on a Polymer property, but I also need to wrap a portion of the string with HTML tags dynamically to style incremental search highlights. It is easy to style things with CSS when only the overall presentation is changing, but when the tags and the text content need to change orientation dynamically in response to logic, such as in this case, injecting HTML is the only way to accomplish it. And it would be nice to have a clean declarative way to do that with Polymer and not have to set innerHTML.

It might be even nicer to have bindings within the injected HTML, akin to this.injectBoundHTML from 0.5, but that's a different story.

@miztroh-zz
Copy link

To update my previous example:

<div inner-h-t-m-l="{{sanitize(html)}}"></div>

The current syntax and functionality allows us to do all that's being asked (aside from data bindings in the dynamic HTML). Just find some JS HTML sanitizer (I found this one from a quick Google search: https://github.com/punkave/sanitize-html) and incorporate it into your custom element as a function, then use the function to sanitize your dynamic HTML on-the-fly.

@shawncplus
Copy link
Author

A real annotation has a few advantages over inner-h-t-m-l:

  • consistency: why does binding html content require an attribute when binding text uses an annotation?
  • expressiveness: at a glance you can tell "This is binding raw html", it's easy to miss inner-h-t-m-l as just another bound attribute then try to use an annotation to that element's content and have it not work inexplicably
    • <div id="mydiv" hidden$="[[!selectMode]]" inner-h-t-m-l="{{foo}}" class="baz">{{bar}}</div>
    • <div id="mydiv" hidden$="[[!selectMode]]" class="baz">{#bar#}</div>
  • No "undefined behavior": with the previous example why does inner-h-t-m-l take precedence over the {{bar}} annotation? You have no idea why and it's not even remotely documented. When you have an annotation you can't mix the two, you can only use one at a time

And you could do the same sanitization with annotation {#sanitize(myHtml)#}

Also to just preempt any bikeshedding, I completely don't care at all what {# #} actually is, could be @@ @@ for all I care

@shawncplus shawncplus changed the title Add html bind annotation Add raw html bind annotation Feb 3, 2016
@rictic
Copy link
Contributor

rictic commented Feb 4, 2016

Hey, this PR is totally safe! As long as no one makes any mistakes!

torch juggling gone wrong

More srsly, this is totally a common request, but I think this is a use case where you want syntactic vinegar rather than a sugary sweet and easy to use footgun in the toolbox. Given that there's no simple way to safely support this, I suspect that implementing a caja-html element is gonna be the best option here.

If someone really and truly knows what they're doing, they could always write their own <unsafe-html> element, but I don't think it's a good idea to ship this functionality, especially not in the base Polymer library.

@sorvell
Copy link
Contributor

sorvell commented Feb 4, 2016

As @rictic noted, because of the security concerns here, we cannot add additional sugar around binding to raw html to the core library. We plan to allow users to hook into the binding system more directly such that we don't need to arbitrate these types of issues at the library level.

@sorvell sorvell closed this Feb 4, 2016
@shawncplus shawncplus deleted the safe-html-bind-annotation branch February 4, 2016 04:29
@jonvuri
Copy link

jonvuri commented Feb 5, 2016

@sorvell I can't really tell from the issue you linked - does that mean that it would be possible to write a custom element that would allow injecting dynamic data-bound HTML at some point?

@sorvell
Copy link
Contributor

sorvell commented Feb 5, 2016

No, that will be covered when we address #1303.

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

Successfully merging this pull request may close these issues.

8 participants