ractivejs / ractive

Next-generation DOM manipulation
http://ractive.js.org
MIT License
5.94k stars 397 forks source link

CDATA block in the template causes Ractive to stop with error message #558

Closed laszlothewiz closed 10 years ago

laszlothewiz commented 10 years ago

If a CDATA block is in the template then you get an error message:

Error: Unexpected character ! (expected "/")

Here's the fiddle to demonstrate: http://jsfiddle.net/DUB8K/3/

I looked in the code and there's no mention of CDATA in it. I am, however, not familiar with the inside works of Ractive to add the missing parsing code to handle this.

laszlothewiz commented 10 years ago

No response yet -- It looks like a tough one.

I'm willing to add the feature if someone could point me to right direction as to where is the code that needs to be amended.

Thanks!

Rich-Harris commented 10 years ago

Funnily enough I was just in the process of replying.

CDATA isn't currently supported (to be honest I didn't really understand what it was for until I read the wikipedia article just now...) - this is something we'll have to look into, as I'm not totally clear on what the expected behaviour would be if a CDATA section occurred inside regular markup. From my brief experiments just now - http://jsfiddle.net/rich_harris/2b4Qb/ - it appears that HTML parsers don't know what to do with CDATA, but SVG (being a subset of XML) can accommodate CDATA. So the parser would need a level of context awareness that it doesn't currently have to bother with.

Given that Ractive aims to support SVG to the same degree that it supports HTML I think it makes sense to allow CDATA. The best starting point would probably be to look at how comments are identified and treated - getComment.js/_Tokenizer.js - since CDATA would behave similarly. (Fair warning: here be dragons. The parser code is overdue for a bit of a spring clean!)

laszlothewiz commented 10 years ago

Thanks. I seems like that the text inside the CDATA block is converted into a simple text node. At least that's what the DOM inspector shows in Firebug.

I'll check out the Tokenizer.js and see if I can figure out what to change. I saw the handling on the comments briefly, I'll study it a bit deeper.

Rich-Harris commented 10 years ago

Here's a question (not a rhetorical question, I'm genuinely unsure as to the answer) - what should happen to mustaches inside CDATA blocks? Should they be interpolated and data-bound, or should <![CDATA[this is a {{mustache}}]]> literally render as this is a {{mustache}}?

laszlothewiz commented 10 years ago

Good point. I think the mustaches should be data-bound. CDATA is basically there to prevent misinterpretation of XML code. It basically just tells the parser that: don't try to interpret anything inside here as a tag but put it in the DOM as it is, character-by-character. In essence CDATA in XML is the same as the XMP tag in HTML. Because mustaches are data-bound in the text nodes then following that logic they should be data-bound inside CDATA. But nothing else in the CDATA should be parsed further.

Also here's the fiddle that confirms that CDATA will simply need to be converted to a text node: http://jsfiddle.net/k34AH/1/

laszlothewiz commented 10 years ago

I'm looking at the code inside the tokenizer and my thought is that CDATA and XMP requires a new type. It is because inside these blocks the only further tokenization that needs to be done is the mustaches. Or maybe, it could be handled as a special 'text' type.

Also, somehow the tokenizer needs to be made context aware so that it knows whether it needs to follow XML syntax or HTML. There are subtle differences between the two. For example the CDATA block, if used in HTML gets converted to a comment block, in XML it gets converted to a text node. Isn't it something like a namespace awereness already in place? I thought I've seen in the code that the node insertion uses that to determine whether to use createElement or createElementNS (a previous bug fix). Is that available during parsing?

Do you have a diagram or some documentation on how the parser in Ractive operates? I think I could add the new type (or modify the 'text' type) if I could understand how the parsing works.

martypdx commented 10 years ago

In your CDATA convert to text node example (btw - you're missing the the first bracket on the CDATA) it's not really the same thing because it is not wrapped in CDATA (you can't see in Chrome dev tools, but I checked via instanceof).

One difficulty here is that arbitrary text like Here is a {{foo}} you should {{bar}} can be modelled as a fragment that contains Text nodes, meaning there's a 1:1 relationship between the parts of the fragment and the DOM nodes. However, a CDATA "Section" actually derives from TextNode, meaning it does NOT contain child nodes.

One approach would be to treat the content of the CDATA as an Interpolator (with a KeypathExpression mustache), and then adjust the value of the CDATA accordingly as a single value.

Which got me thinking, why not just create an expression inside a triple with the CDATA directive?

<style>{{{ ('<![CDATA[ text { font-size: ' + size + 'px } ]]>') }}}></style>

See http://jsfiddle.net/6W9uP/1/

martypdx commented 10 years ago

Okay, so I realize for svg it would be desirable to have it work inline w/o the expression.

I think what it is closest to is a StringFragment. Instead of an attribute value, it would be CDATA value. So if you looked at how attributes are parsed, you could follow that model.

laszlothewiz commented 10 years ago

Thank you. I like your fiddle with the slider ;-)

The triple mustache approach only works in Firefox. Both Chrome and IE ignores it.

Now, I made an experiment on those browsers and the following code does add the style sheet using jQuery: $("svg style").text("text {font-size:20px}"); So jQuery's text method does something differently from Ractive's renderer.

I also found that if I just leave out the CDATA block in the template then it works in FF but ignored in Chrome and IE. So there's something going on during the render phase in Ractive that doesn't put the text node inside the style sheet properly. It works inside any other tag in SVG as far as I could tell. That might worth looking into.

I'll do some more experiments and look into the parser. Is there any other documentation on how the parser in Ractive works than going through the source code? (eg: a diagram)

martypdx commented 10 years ago

Take a look at http://jsfiddle.net/k34AH/6/.

First, if I create an xml document with that same svg:

<?xml version="1.0" standalone="no"?>
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN"
  "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
<svg xmlns="http://www.w3.org/2000/svg" version="1.1"
     width="10cm" height="5cm" viewBox="0 0 1000 1000">
  <defs>
    <style id="svg-style" type="text/css"><![CDATA[
      rect {
        fill: red;
        stroke: blue;
        stroke-width: 3
      }
    ]]></style>
    <style id="other-style" type="text/css">
      g > rect {
        fill: blue;
        stroke: blue;
        stroke-width: 3
      }
    </style>
  </defs>
  <rect x="200" y="100" width="600" height="300"/>
  <g>
  <rect x="200" y="500" width="600" height="300"/>
  </g>
  <script>
      var cd = document.createCDATASection('hello world')
      console.log(cd)
  </script>
</svg>

Chrome does show a CDATA surrounding the text. However, in the fiddle notice that the CDATA is stripped (or at least not shown by Chrome dev tools).

Second, notice the error in the console when trying to call document.createCDATASection. Unless the document is an xml document, you cannot create a CDATA section.

Third, notice that despite the whole raison d'etre of wrapping css > in CDATA, no xml parser is throwing an error in either case.

Inline svg should be parsed as xml according to the spec. But there's no way to add in a CDATA section.

Maybe the answer is just to have the parser ignore CDATA directives as anything other than text?

laszlothewiz commented 10 years ago

Everything points to the conclusion that the CDATA section will simply needs to be converted to a text node as a whole, in other words, no further parsing, other than the mustaches inside.

I was able to make the style sheet work if I simply added it without the CDATA wrapping. It works as long as there are no characters in it that would throw off the parser (eg: & or <).

I needed to fix a bug though to do that, because it would work in FF but not in Chrome or IE. I proposed the fix in #569 which has been working fine in all browsers and didn't seem to break anything else.

martypdx commented 10 years ago

@laszlothewiz Can we close this issue because of changes we did for #569?

laszlothewiz commented 10 years ago

Yes. I think so. With that I was able to insert the SVG code when needed.

I basically abandoned using SVG in my project because the inconsistencies in supporting some features between browsers. I do the graphical portion with jCanvas now, but everything else on the page is using Ractive.

martypdx commented 10 years ago

Interesting. What svg features caused you pain?

laszlothewiz commented 10 years ago

It was mainly creating a text block with word wrapping. <TSPAN>s were too complicated to implement. <textArea> is an SVGT 1.2 feature not supported by any of the major browsers <foreignObject> would have been perfect but not supported by IE, & I wanted my page to working in IE 9+.