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

fix($compile): don't add replaced attributes twice to $attrs #14737

Closed
wants to merge 4 commits into from
Closed

fix($compile): don't add replaced attributes twice to $attrs #14737

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
724273a
fix($compile): don't add merged attributes twice to $attrs
Narretz Jun 8, 2016
3dff191
refactor checks
Narretz Jun 9, 2016
362b36c
try to simplify
Narretz Jun 9, 2016
5c1dc45
better simplify
Narretz Jun 9, 2016
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
18 changes: 6 additions & 12 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2761,18 +2761,12 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

// copy the new attributes on the old attrs object
forEach(src, function(value, key) {
if (key === 'class') {
safeAddClass($element, value);
dst['class'] = (dst['class'] ? dst['class'] + ' ' : '') + value;
} else if (key === 'style') {
$element.attr('style', $element.attr('style') + ';' + value);
dst['style'] = (dst['style'] ? dst['style'] + ';' : '') + value;
// `dst` will never contain hasOwnProperty as DOM parser won't let it.
// You will get an "InvalidCharacterError: DOM Exception 5" error if you
// have an attribute like "has-own-property" or "data-has-own-property", etc.
} else if (key.charAt(0) !== '$' && !dst.hasOwnProperty(key)) {
dst[key] = value;
dstAttr[key] = srcAttr[key];
// Check if we already set this attribute in the loop above.
// `dst` will never contain hasOwnProperty as DOM parser won't let it.
// 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]);
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

}
});
}
Expand Down
23 changes: 23 additions & 0 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -980,6 +980,29 @@ describe('$compile', function() {
}));


it('should not set merged attributes twice in $attrs', function() {
var attrs;

module(function() {
directive('logAttrs', function() {
return {
link: function($scope, $element, $attrs) {
attrs = $attrs;
}
};
});
});

inject(function($compile, $rootScope) {
Copy link
Member

Choose a reason for hiding this comment

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

Aren't $compile and $rootScope already available inside the top-level `describe's scope ?
Why is everyone injecting 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.

I just tried removing it - injection seems necessary

element = $compile(
'<div><div log-attrs replace class="myLog"></div><div>')($rootScope);
var div = element.find('div');
expect(div.attr('class')).toBe('myLog log');
expect(attrs.class).toBe('myLog log');
});
});
Copy link
Member

Choose a reason for hiding this comment

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

I would add a test for style as well, since it is a different code path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really? Both are handled by the if (!dst[key]) { before the src loop.

Copy link
Member

Choose a reason for hiding this comment

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

There are different if/else branhes for each. Most importantly, the one uses safeAddClass(...) and the other $element.attr('style', ...). We need to make sure that whatever these code paths do (which are skipped when dst[key] is trythy, has already been done in the previous loop (where we use dst.$set(...)).



it('should prevent multiple templates per element', inject(function($compile) {
try {
$compile('<div><span replace class="replace"></span></div>');
Expand Down