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

fix($compile): use the correct namespace for transcluded svg elements #8716

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 62 additions & 27 deletions src/ng/compile.js
Original file line number Diff line number Diff line change
@@ -185,9 +185,18 @@
* * `$scope` - Current scope associated with the element
* * `$element` - Current element
* * `$attrs` - Current attributes object for the element
* * `$transclude` - A transclude linking function pre-bound to the correct transclusion scope.
* The scope can be overridden by an optional first argument.
* `function([scope], cloneLinkingFn)`.
* * `$transclude` - A transclude linking function pre-bound to the correct transclusion scope:
* `function([scope], cloneLinkingFn, futureParentElement)`.
* * `scope`: optional argument to override the scope.
* * `cloneLinkingFn`: optional argument to create clones of the original translcuded content.
* * `futureParentElement`:
* * defines the parent to which the `cloneLinkingFn` will add the cloned elements.
* * default: `$element.parent()` resp. `$element` for `transclude:'element'` resp. `transclude:true`.
* * only needed for transcludes that are allowed to contain non html elements (e.g. SVG elements)
* and when the `cloneLinkinFn` is passed,
* as those elements need to created and cloned in a special way when they are defined outside their
* usual containers (e.g. like `<svg>`).
* * See also the `directive.templateNamespace` property.
*
*
* #### `require`
@@ -219,18 +228,17 @@
* * `M` - Comment: `<!-- directive: my-directive exp -->`
*
*
* #### `type`
* String representing the document type used by the markup. This is useful for templates where the root
* node is non-HTML content (such as SVG or MathML). The default value is "html".
* #### `templateNamespace`
* String representing the document type used by the markup in the template.
* AngularJS needs this information as those elements need to be created and cloned
* in a special way when they are defined outside their usual containers like `<svg>` and `<math>`.
*
* * `html` - All root template nodes are HTML, and don't need to be wrapped. Root nodes may also be
* * `html` - All root nodes in the template are HTML. Root nodes may also be
* top-level elements such as `<svg>` or `<math>`.
* * `svg` - The template contains only SVG content, and must be wrapped in an `<svg>` node prior to
* processing.
* * `math` - The template contains only MathML content, and must be wrapped in an `<math>` node prior to
* processing.
* * `svg` - The root nodes in the template are SVG elements (excluding `<math>`).
* * `math` - The root nodes in the template are MathML elements (excluding `<svg>`).
*
* If no `type` is specified, then the type is considered to be html.
* If no `templateNamespace` is specified, then the namespace is considered to be `html`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe templateNamespace is a better name for this, but it's weird --- A) because these values are not actually namespaces, and B) because it's a breaking change for not much of a real reason given A).

If we're going to call it namespace, the values should probably be proper XML namespaces, but we don't do this because people are going to have a much easier time remembering "svg" than "http://www.w3.org/2000/svg"

IMO we should change the symbol name within $compile --- so that the compiler is easier to understand, but the public api doesn't have an unneeded change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your are right, but I chatted with Igor to leave it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't have a better name than "namespace"... (type is really too general)

*
* #### `template`
* HTML markup that may:
@@ -266,6 +274,10 @@
* one. See the {@link guide/directive#creating-custom-directives_creating-directives_template-expanding-directive
* Directives Guide} for an example.
*
* There very few scenarios were element replacement is required for the application function,
* the main one being reusable custom components that are used within SVG contexts
* (because SVG doesn't work with custom elements in the DOM tree).
*
* #### `transclude`
* compile the content of the element and make it available to the directive.
* Typically used with {@link ng.directive:ngTransclude
@@ -360,10 +372,9 @@
* the directives to use the controllers as a communication channel.
*
* * `transcludeFn` - A transclude linking function pre-bound to the correct transclusion scope.
* The scope can be overridden by an optional first argument. This is the same as the `$transclude`
* parameter of directive controllers.
* `function([scope], cloneLinkingFn)`.
*
* This is the same as the `$transclude`
* parameter of directive controllers, see there for details.
* `function([scope], cloneLinkingFn, futureParentElement)`.
*
* #### Pre-linking function
*
@@ -880,8 +891,18 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
compileNodes($compileNodes, transcludeFn, $compileNodes,
maxPriority, ignoreDirective, previousCompileContext);
safeAddClass($compileNodes, 'ng-scope');
return function publicLinkFn(scope, cloneConnectFn, transcludeControllers, parentBoundTranscludeFn){
var namespace = null;
return function publicLinkFn(scope, cloneConnectFn, transcludeControllers, parentBoundTranscludeFn, futureParentElement){
assertArg(scope, 'scope');
if (!namespace) {
namespace = detectNamespaceForChildElements(futureParentElement);
if (namespace !== 'html') {
$compileNodes = jqLite(
wrapTemplate(namespace, jqLite('<div>').append($compileNodes).html())
);
}
}

// important!!: we must call our jqLite.clone() since the jQuery one is trying to be smart
// and sometimes changes the structure of the DOM.
var $linkNode = cloneConnectFn
@@ -902,6 +923,16 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
};
}

function detectNamespaceForChildElements(parentElement) {
// TODO: Make this detect MathML as well...
var node = parentElement && parentElement[0];
if (!node) {
return 'html';
} else {
return nodeName_(node) !== 'foreignobject' && node.toString().match(/SVG/) ? 'svg': 'html';
}
}

function safeAddClass($element, className) {
try {
$element.addClass(className);
@@ -1025,7 +1056,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

function createBoundTranscludeFn(scope, transcludeFn, previousBoundTranscludeFn, elementTransclusion) {

var boundTranscludeFn = function(transcludedScope, cloneFn, controllers) {
var boundTranscludeFn = function(transcludedScope, cloneFn, controllers, futureParentElement) {
var scopeCreated = false;

if (!transcludedScope) {
@@ -1034,7 +1065,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
scopeCreated = true;
}

var clone = transcludeFn(transcludedScope, cloneFn, controllers, previousBoundTranscludeFn);
var clone = transcludeFn(transcludedScope, cloneFn, controllers, previousBoundTranscludeFn, futureParentElement);
if (scopeCreated && !elementTransclusion) {
clone.on('$destroy', function() { transcludedScope.$destroy(); });
}
@@ -1339,7 +1370,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
if (jqLiteIsTextNode(directiveValue)) {
$template = [];
} else {
$template = jqLite(wrapTemplate(directive.type, trim(directiveValue)));
$template = jqLite(wrapTemplate(directive.templateNamespace, trim(directiveValue)));
}
compileNode = $template[0];

@@ -1646,20 +1677,24 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}

// This is the function that is injected as `$transclude`.
function controllersBoundTransclude(scope, cloneAttachFn) {
// Note: all arguments are optional!
function controllersBoundTransclude(scope, cloneAttachFn, futureParentElement) {
var transcludeControllers;

// no scope passed
if (!cloneAttachFn) {
// No scope passed in:
if (!isScope(scope)) {
futureParentElement = cloneAttachFn;
cloneAttachFn = scope;
scope = undefined;
}

if (hasElementTranscludeDirective) {
transcludeControllers = elementControllers;
}

return boundTranscludeFn(scope, cloneAttachFn, transcludeControllers);
if (!futureParentElement) {
futureParentElement = hasElementTranscludeDirective ? $element.parent() : $element;
}
return boundTranscludeFn(scope, cloneAttachFn, transcludeControllers, futureParentElement);
}
}
}
@@ -1786,7 +1821,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
templateUrl = (isFunction(origAsyncDirective.templateUrl))
? origAsyncDirective.templateUrl($compileNode, tAttrs)
: origAsyncDirective.templateUrl,
type = origAsyncDirective.type;
templateNamespace = origAsyncDirective.templateNamespace;

$compileNode.empty();

@@ -1800,7 +1835,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
if (jqLiteIsTextNode(content)) {
$template = [];
} else {
$template = jqLite(wrapTemplate(type, trim(content)));
$template = jqLite(wrapTemplate(templateNamespace, trim(content)));
}
compileNode = $template[0];

Loading