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

fix($compile): attribute bindings should not break due to terminal directives #4649

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
56 changes: 30 additions & 26 deletions src/ng/compile.js
Original file line number Diff line number Diff line change
Expand Up @@ -1717,33 +1717,37 @@ function $CompileProvider($provide) {
}

directives.push({
priority: -100,
compile: valueFn(function attrInterpolateLinkFn(scope, element, attr) {
var $$observers = (attr.$$observers || (attr.$$observers = {}));

if (EVENT_HANDLER_ATTR_REGEXP.test(name)) {
throw $compileMinErr('nodomevents',
"Interpolations for HTML DOM event attributes are disallowed. Please use the " +
"ng- versions (such as ng-click instead of onclick) instead.");
}
priority: 100,
compile: function() {
return {
pre: function attrInterpolatePreLinkFn(scope, element, attr) {
var $$observers = (attr.$$observers || (attr.$$observers = {}));

if (EVENT_HANDLER_ATTR_REGEXP.test(name)) {
throw $compileMinErr('nodomevents',
"Interpolations for HTML DOM event attributes are disallowed. Please use the " +
"ng- versions (such as ng-click instead of onclick) instead.");
}

// we need to interpolate again, in case the attribute value has been updated
// (e.g. by another directive's compile function)
interpolateFn = $interpolate(attr[name], true, getTrustedContext(node, name));

// if attribute was updated so that there is no interpolation going on we don't want to
// register any observers
if (!interpolateFn) return;

// TODO(i): this should likely be attr.$set(name, iterpolateFn(scope) so that we reset the
// actual attr value
attr[name] = interpolateFn(scope);
($$observers[name] || ($$observers[name] = [])).$$inter = true;
(attr.$$observers && attr.$$observers[name].$$scope || scope).
$watch(interpolateFn, function interpolateFnWatchAction(value) {
attr.$set(name, value);
});
})
// we need to interpolate again, in case the attribute value has been updated
Copy link
Member

Choose a reason for hiding this comment

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

This comment worries me. That we have to interpolate twice seems wrong - this seems to catch if another directive changes an attribute that previously had an interpolation but what happens if another directive adds an interpolation to an attribute that didn't have interpolation before? Won't it be missed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it will be. I don't think that there is a way around it.

if an attribute doesn't have interpolation at compile time then we won't establish a databinding for that attribute.

// (e.g. by another directive's compile function)
interpolateFn = $interpolate(attr[name], true, getTrustedContext(node, name));

// if attribute was updated so that there is no interpolation going on we don't want to
// register any observers
if (!interpolateFn) return;

// TODO(i): this should likely be attr.$set(name, iterpolateFn(scope) so that we reset the
// actual attr value
attr[name] = interpolateFn(scope);
($$observers[name] || ($$observers[name] = [])).$$inter = true;
(attr.$$observers && attr.$$observers[name].$$scope || scope).
$watch(interpolateFn, function interpolateFnWatchAction(value) {
attr.$set(name, value);
});
}
};
}
});
}

Expand Down
52 changes: 46 additions & 6 deletions test/ng/compileSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1591,7 +1591,7 @@ describe('$compile', function() {
);


it('should process attribute interpolation at the beginning of the post-linking phase', function() {
it('should process attribute interpolation in pre-linking phase at priority 100', function() {
module(function() {
directive('attrLog', function(log) {
return {
Expand All @@ -1600,22 +1600,36 @@ describe('$compile', function() {

return {
pre: function($scope, $element, $attrs) {
log('preLink=' + $attrs.myName);
log('preLinkP0=' + $attrs.myName);
},
post: function($scope, $element) {
post: function($scope, $element, $attrs) {
log('postLink=' + $attrs.myName);
}
}
}
}
})
});
});
module(function() {
directive('attrLogHighPriority', function(log) {
return {
priority: 101,
compile: function() {
return {
pre: function($scope, $element, $attrs) {
log('preLinkP101=' + $attrs.myName);
}
};
}
}
});
});
inject(function($rootScope, $compile, log) {
element = $compile('<div attr-log my-name="{{name}}"></div>')($rootScope);
element = $compile('<div attr-log-high-priority attr-log my-name="{{name}}"></div>')($rootScope);
$rootScope.name = 'angular';
$rootScope.$apply();
log('digest=' + element.attr('my-name'));
expect(log).toEqual('compile={{name}}; preLink={{name}}; postLink=; digest=angular');
expect(log).toEqual('compile={{name}}; preLinkP101={{name}}; preLinkP0=; postLink=; digest=angular');
});
});

Expand Down Expand Up @@ -1758,6 +1772,32 @@ describe('$compile', function() {
expect(element.text()).toBe('AHOJ|ahoj|AHOJ');
});
});


it('should make attributes observable for terminal directives', function() {
module(function() {
directive('myAttr', function(log) {
return {
terminal: true,
link: function(scope, element, attrs) {
attrs.$observe('myAttr', function(val) {
log(val);
});
}
}
});
});

inject(function($compile, $rootScope, log) {
element = $compile('<div my-attr="{{myVal}}"></div>')($rootScope);
expect(log).toEqual([]);

$rootScope.myVal = 'carrot';
$rootScope.$digest();

expect(log).toEqual(['carrot']);
});
})
});


Expand Down