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

Directive with 'replace: true' and template duplicates classes in the attrs parameter of the link fn #8159

Closed
jstrength opened this issue Jul 11, 2014 · 10 comments

Comments

@jstrength
Copy link

When I create a directive with a template that has a class attribute and replace: true. The attrs parameter in the link function duplicates the classes. They are also duplicated when I look at the element in the HTML.

This appears to only happen when the tag that the directive is replacing also has a class attribute.

Here is some example code, along with the output I see in the console.

<test class="btn-block"></test>
directive("test", function () {
        return {
            restrict: 'E',
            replace: 'true',
            link: function(scope, element, attrs) {
                console.log(attrs);
            },
            template: '<div class="center-block"></div>'
        };
    })

screen shot 2014-07-11 at 3 57 28 pm

@caitp
Copy link
Contributor

caitp commented Jul 11, 2014

replace: true will merge attribute values from the compiled node into the root of the replaced node.

If this is not what you want, you need to manually remove the merged class.

There is currently no way around this. Technically replace has been deprecated, so I wouldn't expect to see new changes to the directive API which allow you to prevent certain attributes from being merged.

So, just to sum up, this is basically doing what it's expected to do, even if it's inconvenient, and we probably can't change it in 1.x (this will be totally different in 2.0.0). But you can work around this by manually adjusting attributes.

@IgorMinar is this a WONTFIX or do you think it would make sense to make some changes to this? As a classic example of the problems with merging attributes, SVG will often throw when we do that, so maybe we should offer some way to avoid merging. I am not really expecting us to, but I can see how it would be useful.

@jeffbcross jeffbcross self-assigned this Jul 21, 2014
@jeffbcross
Copy link
Contributor

@caitp, @jstrength is referring to the fact that the "center-block" class is repeated twice on the class attribute, where it should only be there once.

@jeffbcross jeffbcross added this to the 1.3.0 milestone Jul 21, 2014
@jeffbcross jeffbcross removed their assignment Jul 21, 2014
@caitp
Copy link
Contributor

caitp commented Jul 21, 2014

@jeffbcross I understood that, it's a byproduct of the way our attribute merging works. If we detect that the attribute is there, we append to it with a preceding space. So, if the attribute is both in the directive template and the calling template, it shows up twice.

There are a lot of other silly things that happen because of this (There was one where using id in the template and caller caused the replaced ID to be like id="foo: " (would be id="foo: {{id}}" except id was not in scope). I can't remember the issue # for that one, but I think it's been closed.

I think this is just one of those silly things with repeat: true. We can't really do much about it unless it's okay to make huge breaking changes. and there is a limit to how much we can actually do about it (eg, replacing class=... / ng-class=... or SVG attributes or style=...... always problematic)

@caitp
Copy link
Contributor

caitp commented Jul 22, 2014

#6979 #6239, there are probably more of these open (including this one), but there are too many to go through right now while I make dinner!

I can't recall the number or title of the one I'm thinking of, but yeah this is basically the main issue people run into with replace: true, the way caller attributes are merged into the newly created node.

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

reneras commented Oct 21, 2014

I've made a plunker to validate it still exists in 1.3.0 and it does. I will create a PR in a lil bit.

@reneras
Copy link

reneras commented Oct 21, 2014

@jstrength seems like the element itself doesn't have the duplicate classes ( also according the element.classList ). The duplicates only show up in the console.
How does it manifest itself in your application ?

@jeffbcross jeffbcross modified the milestones: Purgatory, 1.3.x Oct 21, 2014
@jstrength
Copy link
Author

It wasn't manifesting itself in my application in any way that I could tell. Just something I came across when I was debugging my code and thought it should be fixed.

@Narretz Narretz removed the PRs plz! label Sep 28, 2015
@gkalpak
Copy link
Member

gkalpak commented May 31, 2016

Although it shouldn't really affect much (and replace is indeed deprecated for quite some time), I believe this is a bug.

More specifically, in mergeTemplateAttributes(), we are first merging the source attributes with the destination attributes, joining values if both have the same key. Right after that we are copying the keys in source that are not present in destination, but for some reason we are special-casing class and style and always re-appending the source value (even if the key is present in destination, meaning the source value has already been appended in the previous loop).

@Narretz
Copy link
Contributor

Narretz commented Jun 3, 2016

So this only duplicates the attributes internally. It'll probably be an easy fix, but without much impact.

@Narretz Narretz changed the title Directive with 'replace: true' and template Directive with 'replace: true' and template duplicates classes in the attrs parameter of the link fn Jun 3, 2016
@gkalpak gkalpak modified the milestones: Backlog, Purgatory Jun 3, 2016
@gkalpak
Copy link
Member

gkalpak commented Jun 3, 2016

Exactly. I'm putting this in the backlog, but since replace is deprecated this is really low priority.
PRs are welcome though 😃

@Narretz Narretz self-assigned this Jun 8, 2016
Narretz added a commit to Narretz/angular.js that referenced this issue Jun 8, 2016
Narretz added a commit to Narretz/angular.js that referenced this issue Jun 8, 2016
Narretz added a commit to Narretz/angular.js that referenced this issue Jun 8, 2016
In replace directives, attribute values from the template are added twice to the replaced element when both the replaced element and the template element have the attribute. This does not affect the DOM, as it normalizes duplicate values. The values are however duplicated in the $attrs object that is available to directive link functions.

Fixes angular#8159
Closes angular#14737
petebacondarwin pushed a commit that referenced this issue Jun 9, 2016
In replace directives, attribute values from the template are added twice
to the replaced element when both the replaced element and the template
element have the attribute. This does not affect the DOM, as it normalizes
duplicate values. The values are however duplicated in the $attrs object
that is available to directive link functions.

Fixes #8159
Closes #14737
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.