intercellular / cell

A self-driving web app framework
https://www.celljs.org
MIT License
1.5k stars 93 forks source link

New $html special attribute. #13

Closed fprijate closed 7 years ago

fprijate commented 7 years ago

Current implementation processes $text value as innerHTML: Here is a code:

if(key === "$text"){
   if(typeof val === "function") val = Phenotype.multiline(val);
   if(val.toString().length > 0) {
        $node.innerHTML = val;
  }
} 

My proposal is to use new $html attribute for this purpose:

if(key === "$html"){
  if(typeof val === "function") val = Phenotype.multiline(val);
  if(val.toString().length > 0) {
        $node.innerHTML = val;
  }
} 

and in case of $text, we append text content to node.:

if(key === "$text"){
   . . .
   var textnode=document.createTextNode(val);
   $node.appendChild(textnode);           
}
gliechtenstein commented 7 years ago

Thanks for the suggestion, I just realized I forgot to mention how to create text nodes on the tutorial. Just added it. https://github.com/intercellular/tutorial/commit/d7ec6e15e8bd99014204305df5a8b236ab291db2

But I'll share it again here just for reference:

To create plain text nodes, you use $type: "text". For example, if you wanted to express something like this:

<div class='row'>
  <img src="http://localhost/img.jpg"> Plain text
</div>

You would write

Row = {
  class: "row",
  $components: [{
    $type: "img",
    src: "http://localhost/img.jpg"
  }, {
    $type: "text",
    $text: "Plain Text"
  }]
}

Now, as for why the attribute name is $text instead of $html (even though internally it uses innerHTML): It was because I actually don't want think it's good to use this for HTML.

Of course no one is stopping one from using HTML expressions inside (and sometimes it might be a nifty feature) but if we actually name the attribute $html, we're basically promoting that "you're supposed to use HTML instead of plain text expressions here", which is not what we want.

Hope this makes sense. Let me know if you have further questions or arguments. Love to discuss further

fprijate commented 7 years ago

Thanks for response. I understood this before (from examples and my own code).

My intention is to get from:

Row = {
  $type: "div",
  class: "row",
  $text: " <span> test </span>" 
}
<div class='row'>
     &lt; span &gt; test &lt;/span&gt;
</div>

instead of

<div class='row'>
    <span> test </span>
</div>

Of course I can always make something like this:

Row = {
  $type: "div",
  class: "row",
  $text:  " <span> test </span>".replace(/</g, "&lt;").replace(/>/g, "&gt;");
}

or call jQuery $().html() function.

IMHO $text name is little misleading. Also it's only used with plain text in documentation. If compatibility is a problem, then we should have $text , $plaintext attributes instead.

By the way , I forgot to say you, that cell.js is brilliant. It's simple and powerful at the same time. Thanks

fprijate commented 7 years ago

Typical use of HTML expression in text is:

. . .
$text: "this is  <i> italic </i> text. "
. . .
gliechtenstein commented 7 years ago

Wait, I feel like I'm misunderstanding you. Why do you need to this:

<div class='row'>
     &lt; span &gt; test &lt;/span&gt;
</div>

instead of:

<div class='row'>
    <span> test </span>
</div>
fprijate commented 7 years ago

We are doing it all the time when we put a program code into some div or pre element Iwant to do this

{
   $cell: true,
   $type: "pre",
   $text: "this is a span example: <span> test </span>"
}
Caffeinix commented 7 years ago

What @fprijate is asking for sounds to me like protection from XSS vulnerabilities. Speaking for myself, I would not expect HTML content added to $text to be active; that sounds like a pretty big security hole actually. In fact, if you do add an $html, I would recommend naming it something like $unsafeHtml instead to make it very clear that it must never be used to insert anything that comes from user content.

codehz commented 7 years ago

Why not use innertext for $text and innerHTML for $html?

piranna commented 7 years ago

Can this be clossed? Seems to be fixed in master...

devsnek commented 7 years ago

@fprijate this issue has been addressed in #114

gliechtenstein commented 7 years ago

Thanks to @devsnek now we have $html. I wrote a quick demo to test this. It takes a Handlebars template, parses it, and integrates it into Cell. Check it out: https://play.celljs.org/items/1mQfq1/edit

I think this feature is way cooler than I first expected!