fix($compile): don't add replaced attributes twice to $attrs #14737
Conversation
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
Narretz
force-pushed
the
fix-attribute-duplicates
branch
from
05d56de to
724273a
Compare
| }) | ||
| }); | ||
|
|
||
| inject(function($compile, $rootScope) { |
There was a problem hiding this comment.
Aren't $compile and $rootScope already available inside the top-level `describe's scope ?
Why is everyone injecting it ? 😛
There was a problem hiding this comment.
I just tried removing it - injection seems necessary
|
I just thought about this a bit more, and with this change we now have two different strategies of updating the element attributes for class & style. Before this change, classes and styles from "src" (new element) would always be added with safeAddClass() and $element.attr('style') respectively. Now we only add them with dst.$set() if they are also in dst, but use the other technique when they are only in src. And there's also the question why dst attrs are using attr.$set, but src attributes are not. |
|
The whole mergeTemplateAttributes fn is a crazy patchwork of commits. |
|
Oh, look, you can replace the whole special-casing for the src attributes! |
| // You will get an "InvalidCharacterError: DOM Exception 5" error if you | ||
| // have an attribute like "has-own-property" or "data-has-own-property", etc. | ||
| if (!dst.hasOwnProperty(key) && key.charAt(0) !== '$') { | ||
| dst.$set(key, value, true, srcAttr[key]); |
There was a problem hiding this comment.
Interesting. This will also do some more validation/sanitizing (e.g. for src attributes etc). I guess it's a good thing, but maybe also a breaking change.
I wasn't able to find where in the previous code were these new dst attributes (the ones copied from src) added to the actual element 😞 - we might duplicating some work (not that it's a big deal).
There was a problem hiding this comment.
Oh ... interesting. The attributes are previously added with the replaceWith function, which basically overwrites the dst element with everything from the src element. mergeTemplateAttributes is then really just for merging, not plain adding. So using dst.$set actually does work we already did in replaceWith(), and since replaceWith adds all template attributes that don't need merging, we should be able to remove $element.attr('style', value) and safeAddClass($element, value).
replaceWith() already adds all attributes from the template, which means if no merging took place, we don't have to set the attributes again
|
Holy Cow! Now that is a neat bit of refactoring. |
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
| dst[key] = value; | ||
| dstAttr[key] = srcAttr[key]; | ||
|
|
||
| if (key !== 'class' && key !== 'style') { |
There was a problem hiding this comment.
I think this is a breaking (and unnecessary) change 😃
Why not set dstAttr[key] for class and style ?
There was a problem hiding this comment.
@gkalpak - I don't believe this is a BC. The previous behaviour was not to set distAttr for class and style, so doing so would actually be a BC.
There was a problem hiding this comment.
Wow, you are right, it didn't. Any idea why?
It seems like an oversight (bug?)...
Anyway, this is consistent, so no harm 😃
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
bugfix
What is the current behavior? (You can also link to an open issue here)
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.
Does this PR introduce a breaking change?
No
Please check if the PR fulfills these requirements
Other information:
@gkalpak I fixed this while working on another attribute-related PR.
Fixes #8159
Closes #5597