1.3.0-beta.17 still has issues with SVG and directives

[benlesh]

I'll leave this to @IgorMinar or @tbosch to break down, but here are the issues I see with the current implementation of SVG handling with directives:

Transclusion is a problem

Transcluded content doesn't always behave nicely when switching namespaces. This is because it relies heavily on .cloneNode which, in the case of custom elements inside of SVG is going to clone "broken" elements that SVG doesn't know what to do with. The only reliable way to transclude arbitrary HTML/SVG fragments that may or may not include custom elements is to use a wrapMap.

Attribute binding is a problem in directive templates

When evaluating a directive template, attributes bound with the {{x}} syntax cause browser errors when rendering the element (in console: Error: Invalid value for <circle> attribute cx="{{x}}"). SVG Elements are pretty strict about what they allow. The following throw errors: <path d=""> <rect x="{{foo}}">, or pretty much anything with an attribute equal to {{bindThing}} that happens to be an svg string attribute. The case of path[d] is an interesting and unique one, because while I'm sure it won't be often bound to, it expects at least valid path data of M0,0.

Here's a JSBin that demonstrates all of the behaviors mentioned.

I've sent an email with some ideas and discussion around this issue to @IgorMinar, and I'm happy to help in any way I can with this effort. There has been some discussion around re-creating the Ember-based composable graphing components I've created for Netflix in Angular for use in other existing Angular projects at Netflix, however the problems listed above would make it extremely difficult to do.

Edit: looks like ng-attr- fixes most of the attribute issues.

path="" breaking is still a unique one, but probably not worth a special work around.

[caitp]

Hi ben --- so there are a few different issues here. Most of this stuff is fixable (as far as I can tell, I might not be seeing the right results from this): http://jsbin.com/jitemola/1/edit

So, relatively recently, we've chanved ng-attr-* to use brians allOrNothing interpolation feature --- which means that the non-ng-attr-'d attribute won't be added to the DOM unless the interpolation is fully filled out (no undefined values). The type: 'svg' fixes most of the other problems you have there, as far as I can see.

The thing that you're having problems with mostly, is merging attributes with replace: true. Merging r="10" into r="{{r}}" gives you r="{{r}} 10", which is clearly not what you want.

So it takes a bit of care to avoid causing issues when merging attributes, but from a quick glance that seems to be the only real issue with this jsbin.

I'm probably missing something, so let me know. SVG is kind of quirky and there are a lot of problems, but I think this is pretty much working here.

[benlesh]

@caitp, nice, so it looks like the only issue is the tranclusion of known SVGElements that were "born" inside of a custom element. (The second lime-colored circle that isn't showing up)

This, as I've stated above, is the result of cloning a broken SVG element. Broken because it was created inside of a custom element which is unknown to the svg namespace.

FWIW: The email I sent @IgorMinar was the idea that an svg directive could be used to set the current namepace and cloning procedure $compile should use when doing it's thing. In the case of SVG, a wrapMap will probably have to be used as the clone procedure. This could work for MathML too, I suppose, but I'm not personally concerned with that.

[caitp]

yeah, that is still an issue because it would be an HTMLUnknownElement rather than an SVG*Element to begin with.

We could fix that with something like ng-svg or something to ensure it's cloned as an SVG element. (but I'm not sure it's really worth fixing that when you can just use real svg containers instead)

[benlesh]

It's more evil than that.... It's still an SVGCircleElement, it just doesn't work... which makes it harder to detect than HTMLUnknownElement

http://jsbin.com/vuwesi/1/

[benlesh]

I'm still of the school of thought that you're best off tracking the namespace you're currently in while $compiling the HTML. Since Angular is traversing the DOM tree to find directives anyhow, it should be a pretty straight-forward approach. That would provide a property namespace to use with document.createElementNS, as well as reliable cloning methods for mixed content (which in SVG's case is still doing a wrap map with , unfortunately)

[caitp]

what do you do when you're calling $compile within a directive? we don't have any clue which namespace we're in at that point.

It would be a cute strategy, but it's really finicky, simpler to just deal with the limitations of SVG I think.

@IgorMinar what do you think, should the compiler try to keep track of this?

[caitp]

Might be worth asking PDR or someone involved with SVG about why SVG elements created inside of non-SVG elements don't work as expected (but I expect you wouldn't like the answer)

[benlesh]

@caitp Sorry... when I'm talking $compile, I'm talking about what $compile is doing internally.

So when a view is compiled, it's traversing the DOM, finding directives and wiring them up. Part of wiring them up is the act of transclusion, which is currently cloning whatever children the directive's original custom element has. In the case of SVG content, it's cloning broken nodes.

So, I propose that an svg directive is added, that simply notifies the compilation process that it's context has changed from an HTML namespace to an SVG namespace, and uses the appropriate logic to perform transclusion. I also think this might enable Angular to ditch the type: 'svg' flag on directives entirely, since the compilation process itself would know what namespace it was dealing with.

So (in peudocode):

directive('svg', function($compilationContext, $svgNamespace){
  return function(){
    $compilationContext.namespace = $svgNamespace;
  };
});

/* and somewhere deep inside the compile logic where the transclusion is done: */

var transcludedClone = $compliationContext.namespace.clone(customElement.childNodes);

In the (very rough) example above, $compilationContext.namespace would default to $htmlNamespace which would use jqLiteClone() internally. $svgNamespace would have a clone() function that used an svg-specific wrapMap to handle cloning shenanigans.

Likewise, anywhere you were inserting html within the directive architecture you needed to wrapMap, type: 'svg' would become unnecessary, because something a $compileContext.namespace.wrap could be used to always make sure it was handled properly, since you'd already know what namespace you were dealing with.

Make sense?

[caitp]

I'm talking about what $compile is doing internally.

Yeah I understood you =)

So when a view is compiled, it's traversing the DOM, finding directives and wiring them up. Part of wiring them up is the act of transclusion, which is currently cloning whatever children the directive's original custom element has. In the case of SVG content, it's cloning broken nodes.

I actually understand what you're asking it to do --- however, I think this might not be worth it due to the added complexity.

$svgNamespace would have a clone() function that used an svg-specific wrapMap to handle cloning shenanigans.

Does jQuery-superDOM or any of the others provide such a map? Because I'm not sure anyone on the angular team is really an SVG expert, myself included. This is stuff that jQuery doesn't have in core, and it could be argued that it doesn't really need to live in angular core (I'm not making an argument in any direction, currently).

There's probably a smarter way to do this, like giving the jQuery constructor an optional context to use when cloning nodes, and using that everywhere in the compiler.

Anyways, I don't have strong opinions on this, I can see how it could be made to work, but I'd like to hear what other folks think about fixing that in core