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

[benlesh]

severity:inconvenient? To that I say "Ha!", and "💩!"

It's a almost a non-starter for redeveloping the components I've created at Netflix as Angular directives.

There is a high demand for data visualization integration with single page applications, to be sure. Currently, dealing with SVG ends up being monolithic D3 implementations wrapped in directives. This is absolute yuck.

jQuery is all kinds of broken when it comes to SVG, for some the same issues really: A wrapMap that doesn't include mappings for SVG elements, addClass and removeClass rely on building a string and setting className, which doesn't work with SVGElements because className on them isn't a string, it's an SVGAnimatedString. The workaround for that is to use classList or set the class attribute directly (which is what jqLite does).

Last I looked at Angular's compile code, it was actually using Angular's jqLite clone function explicitly, presumably to resolve some other issue.

I'd submit that with the approach I'm proposing, you could actually reduce complexity in the compile code, by removing nested if statements that check things in the directive processing like if(type === 'svg') { /* do magic */ } etc. Giving the compile process contextual handling of DOM manipulation based on the current namespace would isolate the handling of different namespaces.

SVG is treated as a second-class citizen by nearly every framework.

To date, Ember is the only framework to provide solid handling of SVG, but it honestly wasn't until I submitted a PR to expose Metamorph's wrapMap, and created a script to patch it. My understanding is that HTMLBars will use an approach that's probably similar to what I'm proposing at least in the aspect of tracking the namespace as it's processing it's templates.

ReactJS does have some handling for most of the basic svg components via a very simple wrap map that only covers the basic SVG elements people use.

Polymer, on the other hand, is hopelessly broken when it comes to handling of nesting SVG with <template> and <content> tags. The same namespacing issues that common Web Components face, really.

[benlesh]

Equal rights for SVG! 🇺🇸

;)

[caitp]

I hear you =) It would be nice to make this work well for all use cases, but I want to hear a green light from from folks before diving into that, because there is a set of things which really need to be fixed before shipping 1.3, and as far as I know, this isn't really on that list yet (although maybe it should be). --- /cc @IgorMinar @btford @matsko @jeffbcross

---

also, it's marked as inconvenient because really you shouldn't run into this issue if you use attribute directives instead of element directives (I know that's not as pretty as Polymer or whatever, but it should work)

[benlesh]

Yeah, my use case calls for composition of custom elements. Decorators might work, but they ugly up my API/DSL/whatever, where elements more clearly convey structure and intent. I think I might need to drive up and give you guys a demo to help make my case.

[IgorMinar]

I like the idea of keeping track of the svg context within the compiler.

In order to make it work (and be performant), I think it will need to be done within compiler rather than via some kind of directive.

I'm sad to see that you are making a (valid) case for replace: true which, I've been trying to kill for a while now. Isn't there any other way to compose these directives without replace: true?

In the past you mentioned that mixing html elements with svg elements within the svg context works quite well. I don't have extensive svg experience, but when I played with this scenario I couldn't get it to work well. Am I missing something?

besides cloning (fixable via keeping track of context) and merging attributes (fixable - the current behavior works well only for class attribute and we should just special case it), is there anything else to be fixed?

[benlesh]

@IgorMinar: Yeah, I was just looking at that now. And I agree, it should probably be done within compile. It would make it easier to manage the namespace differences using functional composition rather than having to switch in and out somehow with directives. I don't think a directive would even work that well for it now that I look at it.

Side note: $element.removeClass, $element.addClass and safeAddClass have some issues:

1. jqLite.addClass should work fine with SVG, but this might just be luck. It works because it's setting the class attribute, rather than trying to set className.
2. If jQuery is present, addClass (and removeClass) will suddenly break, because jQuery, even 2.X for "modern" browsers is still building a string of classes and putting them in element.className, which doesn't work on SVGElements.
3. safeAddClass found here has a funny comment about className being "readonly". It's not readonly, it's an SVGAnimatedString, on which you need to set baseVal. (e.g. circleElement.className.baseVal = 'myClass')

The safe methods for setting classNames are:

1. Use className on HTML namespace element, and className.baseVal on SVG elements.
2. Use setAttribute('class', 'whatever')
3. User classList.add() and classList.remove()

Method #3 is probably the most efficient option, but it doesn't exist in every browser you're targeting.

[benlesh]

@IgorMinar: On the "mixing HTML and SVG" front...

You can mix HTML into SVG using the <foreignObject> tag. It's weird, but it does work well:

<svg width="400" height="400">
  <foreignObject width="100" height="100">
    <h1>Hello, Igor</h1>
  </foreignObject>
</svg>

[benlesh]

Another issue with wrapMap might come up: two types of <a> tags

Keeping track of a context becomes useful when you need to use a wrapMap as well... because both HTML and SVG have an <a> tag. Which means you can't always find the proper wrap without knowing the context first.

[benlesh]

The polymer and web component workarounds for issues with <context> and SVG namespace have used <foreignObject> to embed <svg> inside an <svg> ... That solution is really, really gross. It results in some really strange DOM trees and it's really what most developers would expect from a framework, IMO. More importantly, you're forced to create properly sized <foreignObject> tags and manage that, which sucks. Also, CSS selectors for styling purposes might become weird.

Angular 2.0 concerns

1. <content> - As I've mentioned before. Not in the SVG namespace, will cause issues.
2. No createShadowRoot() functionality on SVG elements. The method exists, but it will fail with a message "shadow roots are disabled for this element".

Other than those two things, the other issues will be similar to what I'm discussing in Angular 1.0.

[benlesh]

... I forgot (Angular 2.X):

\3. <template> is an HTML namespace element, so you can't directly declare SVG elements underneath them and expect them to work. (e.g. <template id="myThing"><circle cx="2" cy="2" r="1"></circle></template>, will create an HTMLUnknownElement inside the template)

\4. document.registerElement doesn't work on SVGElements, because they're missing some expected properties (Error: the tag name specified in extends isn't valid)

[caitp]

These sound like good concerns to raise with the SVG folks and hixie, I mean if SVG is going to cause problems with custom elements, that's probably not something we want? Maybe it would be better if custom elements were smarter about that, or if XML were less dumb when living in an HTML document =)

[benlesh]

An example of "Composable Components"

For the record, here is very, very simplified version of what I'm doing with Ember at Netflix, as I would have to implement it in Angular:

http://jsbin.com/siwoyu/3/edit?html,js,output

The idea is a bl-graph container that manages scale for the bl-line children. I put some tables below so you can manipulate the data and see the behavior.

The ideal api:

<bl-graph width="300" height="300" min-x="0" max-x="10" min-y="0" max-y="40">
  <bl-line line-data="apples" prop-x="x" prop-y="y"/>
  <bl-line line-data="oranges" prop-x="x" prop-y="y"/>
  <bl-line line-data="bananas" prop-x="x" prop-y="y"/>
</bl-graph>

what I'm stuck with:

<svg bl-graph width="300" height="300" min-x="0" max-x="10" min-y="0" max-y="40">
  <path bl-line line-data="apples" prop-x="x" prop-y="y"/>
  <path bl-line line-data="oranges" prop-x="x" prop-y="y"/>
  <path bl-line line-data="bananas" prop-x="x" prop-y="y"/>
</svg>

So, you're right, it's probably just "inconvient" for most, but it does have an effect on the readability of my components in use, IMO. Which is even more exacerbated by color schemes in different IDEs.

If someone were to try to use this in JADE/HAML, it would definitely look ugly.

[benlesh]

I spent some time poking around at this last night.

I was able to track the namespace within the compileNodes and applyDirectivesToNode functions. This means I can also carry that namepace information into the compileTemplateUrl function and wrapTemplate function. This will render the type: 'svg' flag on directives obsolete, or at least unnecessary.

From here it's just a matter of identifying what needs to be done differently for each namespace and adding a method to what I'm calling the namespaceContext to handle those things.

I should have a proof of concept very soon (or as time allows, lol).

[IgorMinar]

The fix for the transclusion issue has landed via #8716. The remaining stuff is either fixed already or obscure corner-cases that should be tracked via separate bugs when people actually come across them.

I'm going to close this issue.

PS: Equal rights for SVG!

[benlesh]

Ooo... and all five tests pass?

[IgorMinar]

Yes and we added some more.
On Aug 23, 2014 5:00 PM, "Ben Lesh" notifications@github.com wrote:

Ooo... and all five tests pass?

[image: Very nice]
https://camo.githubusercontent.com/611813b763f7a56eb65654f6a569df3e7bca8e48/687474703a2f2f692e696d6775722e636f6d2f41515777532e676966

—
Reply to this email directly or view it on GitHub
#8494 (comment).

[benlesh]

... beta.18 then?

[jeffbcross]

Landed in beta.19. 💯