Directive with replace fails to merge its attributes with its template's attributes (e.g. ngClass, style etc)

[dariocravero]

Say I have the a directive called myDirective which has the following template:

<div class="my-directive" ng-class="{'something': something}" ng-transclude><div>

Say I want to add more conditions to ng-class:

<my-directive ng-class="{'else': else}">my-directive</my-directive>

The expected result would be:

<div class="my-directive" ng-class="{'something': something, 'else': else}"><div>

However, Angular is producing this:

<div class="my-directive" ng-class="{'something': something} {'else': else}"><div>

Which of course causes an error since it isn't a valid expression.

My question is: should a directive that has an ng-class attribute merge it with whatever the caller send on its ng-class attribute?

I asked on IRC and @caitp suggested it might be an issue on the way Expressions are resolved and that I should wrote this issue. Thanks! :)

[caitp]

oh no don't credit me, what if it turns out to be a terrible idea! but it's something that we might be able to improve if people want it.

To summarize, when merging the attributes from the compiled node into the template node, this can screw up ngClass or other directives expecting expressions.

I'm not totally sure how to fix this (I don't think it would be a super trivial fix), but down the road it might be good to try

[dariocravero]

Didn't mean it that way, sorry.

BTW, here's how I'm solving it right now (fiddle):

compile: function compile(tElement, tAttrs, transclude) {
  tAttrs.ngClass = tAttrs.ngClass.replace(/}\s*{/g, ', ');
  // ...
}

angular.module('app', []).directive('myDirective', ['$templateCache', function($templateCache) {
        return {
            restrict: 'E',
            transclude: true,
            replace: true,
            compile: function compile(tElement, tAttrs, transclude) {
                tAttrs.ngClass = tAttrs.ngClass.replace(/}\s*{/g, ', ');

                return {
                    post: function postLink(scope, iElement, iAttrs) {
                        scope.something = true;
                        scope.else = true;
                    }
                };
            },
            template: $templateCache.get('template.html')
        };
    }]);

It's hacky but it does the trick for now.

[gsklee]

Interesting issue, subscribing.

[jenslukowski]

I have a (failing) test case reproducing this issue but I wonder how to merge the attribute values in other cases. What would happen when you have a variable and an object?

[caitp]

yeah, unfortunately this is not a super easy thing :(

[jeffwhelpley]

Ah, @caitp this touches on the issue I ran into earlier. Understood that it is complex but hopefully someone can implement a solution for this.

[plumpNation]

@dariocravero Thanks for the quick fix suggestion.

[teropa]

There's an additional complication here, which is that if the directive uses isolate scopes or scope inheritance, the ng-class entries defined in the original element won't behave as expected if the expression is merged and then evaluated in the isolated directive scope.

Since replace is being deprecated, this probably isn't worth the trouble?

[caitp]

it's not worth the trouble --- you can't really merge the ng-class attributes correctly, we can't special case certain attributes because it could apply to any attribute/directive that takes an expression --- there's basically no real fix, other than finding a way to link the ngClass directive twice for the same element. (and we can't really generalize that to other directives --- at least not without extending the directive API :()

[KamBha]

@teropa, can you clarify the comment about replace being deprecated? What should we be using instead?

[jkjustjoshing]

@KamBha right now you should be using replace: false in all your directives (or just omitting it - that's the default value) to avoid issues of attribute merging (and to prepare for the future when replace: true won't be an option).

[LeleDev]

Even if replace will be deprecated in the future, a solution (even temporary) would be really appreciated

[btm1]

yeahhhhh this is supper annoying .... I'm running up against not being able to use ng-class on accordion right now. Any way we can fix this?

[btm1]

@LeleDev you'll probably just need to make a custom directive for now that you use in a special case like this... that's what i did:

.directive('lClass', [function () {
    return {
        restrict: 'A',
        link: function (scope, elem, attrs) {
            if(!attrs.lClass) return;

            var w = scope.$watch(function(){
                var cls = scope.$eval(attrs.lClass);
                if(!angular.isObject(cls)) {
                    w();
                    return;
                }

                angular.forEach(cls,function(condition,cl){
                    if(condition) {
                        elem.addClass(cl);
                    } else {
                        elem.removeClass(cl);
                    }
                });
            });
        }
    };
}]);

[LeleDev]

@btm1 : Thank you! I ended up using the class attribute at the moment:

class="static-class-1 {{ condition1 ? 'dynamic-class-1' : '' }} {{ condition2 ? 'dynamic-class-2' : '' }}"

Don't know which of the two workarounds is better (performance-wise)

[btm1]

@LeleDev np ... I doubt there's much of a difference performance wise between the two: Both use a watch expression and are tied to the digest cycle so either way it's happen although with the class method each {{}} represents a separate watch expression, where as mine only uses one and evaluates each property in a supplied object just like ng-class.

[surdu]

The same applies to ng-disabled, probably other ng-attributes as well ... :disappointed:

[skmasq]

I'm self-transcluding an element right now and this is quite interesting that I can't seamlessly merge both ng-classes the original and the one I need....

What is an ETA on this?

[gkalpak]

Considering that:

1. replace: true is deprecated, so we don't really allocate resource on fixing broken edgecases. (No matter how many we fix, there are constantly new ones popping up anyway :stuck_out_tongue:)
2. It's probably not even possible to fix this in a reasonable way (see https://github.com/angular/angular.js/issues/5695#issuecomment-59945290).
3. There are viable workarounds (e.g. https://github.com/angular/angular.js/issues/5695#issuecomment-132960698)

I wouldn't hold my breath :wink:

PRs are always welcome though, if someone feels brave :grin:

[HermanBovens]

replace: true being deprecated is an issue by itself IMO because:

1. You really need it when your directive generates one or more <li> or <tr> elements to be included directly under the respective <ul> or <tbody> element outside the directive.
2. You want it in order to keep your CSS stable when refactoring a component that has grown too large into smaller sub-components.

[thany]

If replace: true is deprecated, what's the most viable alternative?

Because as stated, it's a totally neccesary construct. For elements that may not contain arbitrary/custom other elements, and for svg.

[gkalpak]

@thany, depedning on the exact usecase, there might be different work-arounds/solutions available.

[thany]

The use-case is simple: I don't need those directive-element in my DOM because they mess up styling and they are not always allowed in places where they appear. On top of that, I like a clean DOM - a clean DOM is easy to understand for a CSS'er, provides a cleaner accesibility tree, and is easier to traverse in case when tracking down problems.

So, what's the recommended thing to do instead of replace: true? Transplanting all attributes and whatnot, and replacing the directive element manually? Here be dragons.

Actually, let's turn this question around: Why would you want/like/need those <my-custom-directive> elements in your DOM?

[gkalpak]

The use-case is simple: I don't need those directive-element in my DOM because they mess up styling

That is usually an issue with styling. Unfortunately, Bootstrap is notorious for tightly coupling its styling with DOM structure in some cases. Possible work-arounds:

1. Using the expected elements and enhance them with attribute directives
2. Having a directive on parent or wrapper element generate the necessary elements.
3. Using comment directives (:-1:)

and they are not always allowed in places where they appear.

replace: true won't help you much here, because the browser moves the original elements around, before Angular's compiler can get its hands on them.

On top of that, I like a clean DOM - a clean DOM is easy to understand for a CSS'er, provides a cleaner accesibility tree, and is easier to traverse in case when tracking down problems.

Sound subjective (especially the first and last arguments). Having a <my-component></my-component> element seems much more clear to me instead of having a just the templates. It gives the DOM some structure (knowing when each component starts/ends) and is much closer to the Web Components mentality.

Why would you want/like/need those elements in your DOM?

As mentioned above, I like these because they keep the resulting HTML much more declarative and structured (which is even more useful when troubleshooting). But besides that, a main reason for deprecating replace: true is that there are too many moving parts and corner cases that is just not possible to make it work together correctly with the current implementation of $compile (things like merging attributes between elements and combining transclusion, structural directives, templateUrl etc).

[thany]

That is usually an issue with styling. Unfortunately, Bootstrap is notorious for tightly coupling its styling with DOM structure in some cases. Possible work-arounds:

I'd expect a framework as well-maintained and well-proven to solve those problems, so I don't need to workaround stuff like this. I'm not using Bootstrap, or any other bloated CSS framework, but I do want sensible CSS. so things like ul > li must work.

Using comment directives (:-1:)

Never heard of that! It sounds fantastic, so why the :-1:?

replace: true won't help you much here, because the browser moves the original elements around, before Angular's compiler can get its hands on them.

That's a shortcoming of Angular in general then: it parses the templates after the browser has seen them. Iow, it relies on the browser to parse templates, which is why Angular cannot divergence from html syntax too much, and this is why we need strange workarounds like ng-attr-* in some cases. It's also the reason why replace:true exists in the first place.

I guess this is where React shines. But React has too many other shortcomings for me ;)

Sound subjective (especially the first and last arguments). Having a element seems much more clear to me instead of having a just the templates. It gives the DOM some structure (knowing when each component starts/ends) and is much closer to the Web Components mentality.

That's also subjective. So we have differing opinions. That's ok. I can see your points, hopefully you can see mine.

[gkalpak]

That is usually an issue with styling. Unfortunately, Bootstrap is notorious for tightly coupling its styling with DOM structure in some cases. Possible work-arounds:

I'd expect a framework as well-maintained and well-proven to solve those problems, so I don't need to workaround stuff like this. I'm not using Bootstrap, or any other bloated CSS framework, but I do want sensible CSS. so things like ul > li must work.

¯(ツ)/¯

Using comment directives (:-1:)

Never heard of that! It sounds fantastic, so why the :-1:?

It is generally considered a good practice to prefer tag name or attribute directives over comment and class names, as it makes it easier to determine what directives a given element matches. Also, avoid comment and class name directives allows you to turn on some optimization in $compile (this feature might not actually be released yet). But in cases where it is unavoidable, comment directives might be a good idea. So, in retrospect I was overreacting with the :-1:.

replace: true won't help you much here, because the browser moves the original elements around, before Angular's compiler can get its hands on them.

That's a shortcoming of Angular in general then: it parses the templates after the browser has seen them. [...]

True - nobody said Angular 1 is perfect :smiley:
FWIW, Angular 2 doesn't suffer by that as it has its own HTML parser, but things are not likely to change in Angular 1.

Sound subjective (especially the first and last arguments). Having a element seems much more clear to me instead of having a just the templates. It gives the DOM some structure (knowing when each component starts/ends) and is much closer to the Web Components mentality.

That's also subjective. So we have differing opinions. That's ok. I can see your points, hopefully you can see mine.

Exactly! I was just pointing out the subjectiveness :smiley: