-
Notifications
You must be signed in to change notification settings - Fork 27.3k
fix(ngInclude): correctly add non-html namespaced template content #8981
Conversation
|
hmm, jenkins doesn't like it :( |
test/ng/directive/ngIncludeSpec.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is a terrible test name, will come up with something better
|
It's hard to debug the failures unfortunately, |
28dbcbe to
a2922ec
Compare
src/ng/compile.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!futureParentElement && !cloneConnectFn
|
The approach seems ok! |
ec59785 to
8d5e11d
Compare
|
So there's a problem, Safari seems to have problems with this --- it doesn't create childNodes at all when setting innerHTML of an SVG element =( (it's possible that it's not parsing immediately or something) (repro at http://jsfiddle.net/hp2rm4yc/) |
|
There's a bug filed on this already for webkit https://bugs.webkit.org/show_bug.cgi?id=135698 --- I don't mind fixing it in the browser, so after my little conference talk I can spend a few hours on that maybe. But unfortunately that doesn't do much good for fixing this in angular for the legacy Webkit browsers |
|
I agree. overall it looks good. |
|
As a short-term solution to the safari/webkit problem, we could also abandon the test |
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
This reverts commit 8d5e11d.
src/ng/directive/ngInclude.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a blank line above this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we simplify the check to !$element[0].childNodes.length && ctrl.template.length
|
The fix as-is doesn't really work well enough anyways (works for Safari now, but not for Chrome/Firefox) --- I think we want to take the SVG path for both chrome and FF --- so we need to tell the directive that it's supposed to be an SVG node. I guess a quick SVG test for the toString()'d name of the template would be good enough |
src/ng/directive/ngInclude.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT we don't need to call jqLite here.
e336b80 to
0471aa8
Compare
|
with the latest fix, (I think) this works correctly --- it's not quite right yet, I'll try to get it ready to land tonight and merge before release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the jqLiteParseHTML addition doesn't matter much, but I don't think it's bad to have it
This makes sure that the linked element is empty before adding the content.
|
From local tests, the current iteration is working well on Safari, FF31 and Chrome --- I think it's ready to squash and land --- @IgorMinar can you take a last look at it (the diff is smaller due to the removal of revert of petes commit) |
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.
/CC @tbosch / @IgorMinar
Closes #7538