-
Notifications
You must be signed in to change notification settings - Fork 27.4k
fix($compile): don't add replaced attributes twice to $attrs #14737
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aren't There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'); | ||
}); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would add a test for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Really? Both are handled by the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are different |
||
|
||
|
||
it('should prevent multiple templates per element', inject(function($compile) { | ||
try { | ||
$compile('<div><span replace class="replace"></span></div>'); | ||
|
There was a problem hiding this comment.
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 fromsrc
) added to the actual element 😞 - we might duplicating some work (not that it's a big deal).There was a problem hiding this comment.
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)
andsafeAddClass($element, value)
.