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 1 commit
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
28 changes: 16 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,22 @@ 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
if (!dst[key]) {
Copy link
Member

@gkalpak gkalpak Jun 8, 2016

Choose a reason for hiding this comment

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

Without thinking much about it, using !dst.hasOwnProperty(key) feels safer (and should be quicker).


if (key === 'class') {
safeAddClass($element, value);
dst['class'] = (dst['class'] ? dst['class'] + ' ' : '') + value;
Copy link
Member

@gkalpak gkalpak Jun 8, 2016

Choose a reason for hiding this comment

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

We have already checked that dst[key] is falsy, so no need for this check.

} 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)) {
Copy link
Member

@gkalpak gkalpak Jun 8, 2016

Choose a reason for hiding this comment

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

We could replace the !dst[key] check above with !dst.hasOwnProperty(key) and remove the check from here.

dst[key] = value;
dstAttr[key] = srcAttr[key];
}
}
});
}
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

@gkalpak gkalpak Jun 8, 2016

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

@Narretz Narretz Jun 9, 2016

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

@gkalpak gkalpak Jun 8, 2016

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

@Narretz Narretz Jun 9, 2016

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

@gkalpak gkalpak Jun 9, 2016

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