Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

ngInclude doesn't work for SVG in Safari or Firefox (5263) #7538

Closed
ghost opened this issue May 21, 2014 · 22 comments
Closed

ngInclude doesn't work for SVG in Safari or Firefox (5263) #7538

ghost opened this issue May 21, 2014 · 22 comments

Comments

@ghost
Copy link

ghost commented May 21, 2014

"Invalid" bug #5263 is still a problem (1.2.16).

The example plunkr at http://plnkr.co/edit/Igvyeb1g4sWHoeOgAHTz?p=preview only works in Chrome.

@tbosch tbosch self-assigned this May 21, 2014
@tbosch
Copy link
Contributor

tbosch commented May 21, 2014

Also in chrome, it outputs a lot of errors in the console, as the interpolation symbols are not valid for svg attributes. However, I don't know a way of preventing those errors. They even show up when putting those svg elements inside of a template tag.

@tbosch tbosch added this to the Backlog milestone May 21, 2014
@tbosch tbosch removed their assignment May 21, 2014
@tbosch
Copy link
Contributor

tbosch commented May 21, 2014

@caitp and @IgorMinar FYI. I think this is legit, although I am not sure how well we would like to support interpolation in svg (e.g. using {{}} in attribute values) as that gives errors in the browser console.

@ghost
Copy link
Author

ghost commented May 21, 2014

Ignore the interpolation errors; the original plunk author should've used "ng-attr-[attribute]" (e.g.: ng-attr-dx) for those. Here's one with the proper attributes: http://plnkr.co/edit/6L7cv6sjSaROJsEx0qAT?p=preview

@caitp
Copy link
Contributor

caitp commented May 21, 2014

it's true though, it still doesn't work. I'm looking into it =)

@caitp caitp self-assigned this May 21, 2014
@caitp
Copy link
Contributor

caitp commented May 21, 2014

I'm not totally sure exactly what is happening here, but the element which is receiving the interpolated attribute values, seems to be HTMLUnknownElement in Gecko (and possibly Safari too, but the devtools in safari keep crashing on me so it's hard to verify =() --- so, these should still be added to the DOM, but that doesn't seem to be happening, and I haven't sorted that out yet.

I think turning this into a failing unit test would make this easier to sort out, but the question is, what's the priority like on this --- because there are some other issues I'd like to take care of this week. Thoughts?

@avowkind
Copy link

avowkind commented Jun 4, 2014

This may be related. see http://embed.plnkr.co/FMgx03i4Vkc6rDeAeFaT/preview
In this very short example the text colour is normally set by the class on the text element, when a value is bound to the element - the text string the class is corrupted on safari so the red from the parent element class shows through.

debug shows the element as
Text in SVG

I have a project with a fair bit of this type of code and would love a fix or at least to understand what is going on.

My work-around is to only set styles on the parent elements but there are some circumstances where this is difficult.

@ghost
Copy link
Author

ghost commented Jun 4, 2014

That class bug isn't in 1.2.16; you should consider upgrading

@IgorMinar
Copy link
Contributor

the reason for this not working in FF/Safari as opposed to Chrome is that the behavior of innerHTML in Chrome is different from that of FF/Safari. (we use innerHTML in ngInclude)

In chrome when you innerHTML into an svg element, the inlined DOM will be created within the svg namespace (check namespaceURI), however in firefox it is created in html/xhtml namespace.

@caitp is checking which browser is doing the wrong thing here.

@caitp
Copy link
Contributor

caitp commented Jul 9, 2014

asking PDR about this, but the reproduction in angular was a bit too complicated for him to assess. I've put together a vanilla reproduction of this, but it's not quite a faithful reproduction of what's happening in angular (but interestingly, does sort of reproduce the issue!)

Igor can you take a look at it and see if that looks sort of faithful to you? http://jsfiddle.net/j4pWZ/ / http://jsfiddle.net/82f6S/ (with logging)

@caitp
Copy link
Contributor

caitp commented Jul 9, 2014

It looks like it is the same issue to me, HTMLUnknownElement instead of SVGGElement

@caitp
Copy link
Contributor

caitp commented Jul 9, 2014

Based on http://jsfiddle.net/3LktM/, setting innerHTML in non-blink browsers seems to be the main culprit, not cloneNode

@IgorMinar
Copy link
Contributor

Correct. As I mentioned before it's innerHTML that is causing these issues
in non-blink browsers.

I haven't looked into cloneNode myself but I heard that it was problematic
with svg as well.
On Jul 8, 2014 6:38 PM, "Caitlin Potter" notifications@github.com wrote:

Based on http://jsfiddle.net/3LktM/, setting innerHTML in non-blink
browsers seems to be the main culprit, not cloneNode


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

@btford btford removed the gh: issue label Aug 20, 2014
@antoinepairet
Copy link

Hi all,
What's the status of this?

Using Angular 1.3-rc.0 and svg directives, ng-include only works in Chrome. Safari and FF are broken.
Here is a minimal plunkr: http://plnkr.co/edit/pjVIYLkUHp0jwT9S0n6v

FF reports:

__proto__: HTMLUnknownElementPrototype { ELEMENT_NODE=1, ATTRIBUTE_NODE=2, TEXT_NODE=3, more...}

@caitp, as this bug report is related to 1.2.16, should I create a new issue?

Thanks in advance

@caitp
Copy link
Contributor

caitp commented Sep 8, 2014

well ngInclude doesn't do our templateNamespace hackery... to support ngIncluding SVG files we'd probably need to add another attribute to tell ngInclude how it should wrap the template, or something. @IgorMinar what do you think? We could also have a quick and dirty ngIncludeSvg or something, heh.

@tbosch
Copy link
Contributor

tbosch commented Sep 8, 2014

As ngInclude loads/clones the template in link phase and not in compile phase (in contrast to directive.templateUrl / template) we should be able to detect whether we are in an svg context or not using the same trick we already do for transcludes. Via that we don't need an additional attribute / new directive.

@tbosch
Copy link
Contributor

tbosch commented Sep 8, 2014

@caitp
Copy link
Contributor

caitp commented Sep 8, 2014

It doesn't really transclude content --- it transcludes the root node, but it just raw $compiles() the template content, which is the problem here --- we can detect that the root is an SVG node, but we can't detect that any of the children loaded from the template are SVG nodes this way

@tbosch
Copy link
Contributor

tbosch commented Sep 8, 2014

Yes, and we need some way know whether to use the special way of creating the svg nodes. You proposed an additional attribute, but I say we should already know this information when ngInclude calls $compile immediately. Will create a PR soon...

@tbosch
Copy link
Contributor

tbosch commented Sep 8, 2014

Or better: could you do it?

@caitp
Copy link
Contributor

caitp commented Sep 8, 2014

I'll work on something in a moment

@caitp
Copy link
Contributor

caitp commented Sep 8, 2014

I have a fix for this, uploading in a sec

caitp added a commit to caitp/angular.js that referenced this issue Sep 8, 2014
It is now possible for ngInclude to correctly load SVG content in non-blink browsers, which do not
sort out the namespace when parsing HTML.

Closes angular#7538
caitp added a commit to caitp/angular.js that referenced this issue Sep 8, 2014
It is now possible for ngInclude to correctly load SVG content in non-blink browsers, which do not
sort out the namespace when parsing HTML.

Closes angular#7538
caitp added a commit to caitp/angular.js that referenced this issue Sep 8, 2014
It is now possible for ngInclude to correctly load SVG content in non-blink browsers, which do not
sort out the namespace when parsing HTML.

Closes angular#7538
caitp added a commit to caitp/angular.js that referenced this issue Sep 9, 2014
It is now possible for ngInclude to correctly load SVG content in non-blink browsers, which do not
sort out the namespace when parsing HTML.

Closes angular#7538
@caitp caitp closed this as completed in 6639ca9 Sep 9, 2014
@tbosch
Copy link
Contributor

tbosch commented Sep 9, 2014

+1!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.