angular / angular.js

AngularJS - HTML enhanced for web apps!
https://angularjs.org
MIT License
58.85k stars 27.52k forks source link

ngInclude doesn't seem to work on svg elements #5263

Closed wmertens closed 10 years ago

wmertens commented 10 years ago

Plunker: http://plnkr.co/edit/JF8O1NMbdNQLSNYkCNyG?p=preview

Basically, I'd like to use ngInclude to do recursive svg inclusions but that doesn't seem to work.

caitp commented 10 years ago

I might dig into this today if the issue doesn't get resolved/closed before this evening. There seem to be a few issues regarding SVG + angular compilation, so this might be related, but I haven't really dug into it in much detail just yet

IgorMinar commented 10 years ago

@wmertens you were missing quotes around the template id: <g ng-include="'svgOrg'"></g>

with quotes ng include does work in svg http://plnkr.co/edit/Igvyeb1g4sWHoeOgAHTz?p=preview

wmertens commented 10 years ago

@IgorMinar You're correct, I forgot the single quotes in the plunker but even then it doesn't work for me. What browser are you using? The second section still doesn't show the org in your plunker on Safari and Chrome (stable) for me. It's supposed to look like the third section.

wmertens commented 10 years ago

@caitp did you have a chance to take a look at it? I'd like to get this issue reopened because it's not fixed for me. Can you verify if it works for you on http://plnkr.co/edit/Igvyeb1g4sWHoeOgAHTz?p=preview ?

caitp commented 10 years ago

The W3 validator complains if I use even data-ng-include or data-ng-init in SVG elements.... So I suppose it's not really possible to do it that way (I mean unless this is a different problem, but it does say it's invalid)

Also, it complains about the viewport attribute, which seems to be the case looking at the SVG spec and MDN doesn't mention it either.

The ngInclude directive isn't being compiled or linked, and I don't think we have any special logic to ignore SVG tags (although it's possible that we do), so it is likely that we just don't see the SVG nodes... I'll see if there's anything I can do about that

edit okay, chrome devtools are acting up a bit in plnkr, it seems ngInclude is being compiled/linked, and the template is compiled as well.hmmm

Issue seems to be at

              currentElement.html(response);
              $animate.enter(currentElement, null, $element, afterAnimation);
              $compile(currentElement.contents())(currentScope);
              currentScope.$emit('$includeContentLoaded');

(https://github.com/angular/angular.js/blob/master/src/ng/directive/ngInclude.js#L203)

The innerHTML gets set correctly, but childNodes are never populated. This isn't specific to jqLite, as it affects jquery 2.0.3 as well


So... I guess theoretically it might be possible to make this work if using a strategy similar to https://github.com/angular/angular.js/pull/5235, but it would be really gross and require changes to ngInclude too.

I guess it could be done if there's a serious demand for it, but seen as though it makes markup invalid, that might be an indicator that it's something we shouldn't do

wmertens commented 10 years ago

Actually there's a ton of things being used on SVG that probably make markup invalid. ng-repeat works fine in the example and that's not a valid SVG attribute either.

It would be great to fix this, d3.js is awesome but it's very hard to set up the graphing, and being able to ngInclude templates allows you to use d3 for the layout calculations and angular for the binding elements to content. It's much more natural to define a graph in html with bindings than having to do it piecewise in code with the d3.enter().append() etc idiom. To see this illustrated suberbly, watch the http://worrydream.com/#!/DrawingDynamicVisualizationsTalk . Angular isn't a drag and drop thing (yet), but it's sure a lot nicer than the d3 code.

caitp commented 10 years ago

ng-repeat and ng-include don't strictly work the same way (ng-include simply does element.html(theTemplateHTML), and attempts to compile the contents without ensuring that the contents are actually permitted. ng-repeat uses the transclude function instead, which may do a better job of collecting the SVG content...)

Although as far as I can tell, the current $compile/transclude implementation shouldn't be producing valid SVG fragments from SVG content templates, it is more likely to work than the document fragment being generated by jqLite(svgContent)

Interestingly from testing here, http://jsbin.com/UhIrEtES/1/, they seem to both produce valid SVG document fragments


Hmm, actually it generates document fragments, but the document fragments are not SVG document fragments, unless the <svg> tag is at the root of the template. Which is basically what I assumed was happening. I'm not totally sure what the solution to that would be (since there are a lot more svg elements than table elements, a solution like the one I linked above would be harder and nastier to implement)

wmertens commented 10 years ago

@caitp I tried taking a page from your book, adding the tag to the svgOrg template as a workaround, but that doesn't seem to make it into an svg fragment. What am I doing wrong, or will this not work?

See http://plnkr.co/edit/is9tr9cPdR3LmbI9scd4?p=preview .

caitp commented 10 years ago

Well @wmertens, there is one fix available: https://github.com/angular/angular.js/pull/5508.

Unfortunately it's not a very "friendly" fix, but it's a work-around at least, which is proven to solve the issue when dealing with SVG nodes.

I've previously written a patch designed to do the same thing, working with table nodes, and if that gets merged, then it shouldn't be impossible to do the same sort of thing for SVG and MathML nodes. That's at https://github.com/angular/angular.js/pull/5235

I guess I should clarify, they aren't really "available" in the sense of being merged, just available in the sense that solutions exist, and will perhaps be merged in the future

shock01 commented 10 years ago

This can be fixed by not doing a angular.element().html() in the $compileProvider but instead use:

That way all svg will be parse properly and interpretted by the browser.

ghost commented 10 years ago

http://plnkr.co/edit/Igvyeb1g4sWHoeOgAHTz?p=preview Does not work in Safari 7.0.3.