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

fix($compile): be more careful about shadowing $attrs values #12151

Closed
wants to merge 1 commit into from
+94 −27
Closed

fix($compile): be more careful about shadowing $attrs values #12151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 12 additions & 15 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2566,36 +2566,32 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
lastValue,
parentGet, parentSet, compare;

if (!hasOwnProperty.call(attrs, attrName)) {
// In the case of user defined a binding with the same name as a method in Object.prototype but didn't set
// the corresponding attribute. We need to make sure subsequent code won't access to the prototype function
attrs[attrName] = undefined;
}

switch (mode) {

case '@':
if (!attrs[attrName] && !optional) {
destination[scopeName] = undefined;
if (!optional && !hasOwnProperty.call(attrs, attrName)) {
destination[scopeName] = attrs[attrName] = void 0;
}

attrs.$observe(attrName, function(value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Slightly unrelated to this fix, but should we be bothering to call $observe at all if the attribute doesn't exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might exist later, I think that's the reasoning behind this

Copy link
Member

Choose a reason for hiding this comment

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

The problem is that if you have a one-time binding and the value is sometimes expected to be not defined then in many cases you basically have to write attribute="::null". This is very unintuitive and it's very easy to create lots of excessive bindings.

Changing that might be a breaking change, though. But it might be worth doing in 1.5?

destination[scopeName] = value;
if (isString(value)) {
destination[scopeName] = value;
}
});
attrs.$$observers[attrName].$$scope = scope;
if (attrs[attrName]) {
if (isString(attrs[attrName])) {
// If the attribute has been provided then we trigger an interpolation to ensure
// the value is there for use in the link fn
destination[scopeName] = $interpolate(attrs[attrName])(scope);
}
break;

case '=':
if (optional && !attrs[attrName]) {
return;
if (!hasOwnProperty.call(attrs, attrName)) {
if (optional) break;
attrs[attrName] = void 0;
}
parentGet = $parse(attrs[attrName]);

parentGet = $parse(attrs[attrName]);
if (parentGet.literal) {
compare = equals;
} else {
Expand Down Expand Up @@ -2634,7 +2630,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
break;

case '&':
parentGet = $parse(attrs[attrName]);
// Don't assign Object.prototype method to scope
parentGet = attrs.hasOwnProperty(attrName) ? $parse(attrs[attrName]) : noop;

// Don't assign noop to destination if expression is not valid
if (parentGet === noop && optional) break;
Expand Down
94 changes: 82 additions & 12 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2521,10 +2521,17 @@ describe('$compile', function() {
};

expect(func).not.toThrow();
expect(element.find('span').scope()).toBe(element.isolateScope());
expect(element.isolateScope()).not.toBe($rootScope);
expect(element.isolateScope()['constructor']).toBe($rootScope.constructor);
expect(element.isolateScope()['valueOf']).toBeUndefined();
var scope = element.isolateScope();
expect(element.find('span').scope()).toBe(scope);
expect(scope).not.toBe($rootScope);

// Not shadowed because optional
expect(scope.constructor).toBe($rootScope.constructor);
expect(scope.hasOwnProperty('constructor')).toBe(false);

// Shadowed with undefined because not optional
expect(scope.valueOf).toBeUndefined();
expect(scope.hasOwnProperty('valueOf')).toBe(true);
})
);

Expand All @@ -2539,10 +2546,13 @@ describe('$compile', function() {
};

expect(func).not.toThrow();
expect(element.find('span').scope()).toBe(element.isolateScope());
expect(element.isolateScope()).not.toBe($rootScope);
expect(element.isolateScope()['constructor']).toBe('constructor');
expect(element.isolateScope()['valueOf']).toBe('valueOf');
var scope = element.isolateScope();
expect(element.find('span').scope()).toBe(scope);
expect(scope).not.toBe($rootScope);
expect(scope.constructor).toBe('constructor');
expect(scope.hasOwnProperty('constructor')).toBe(true);
expect(scope.valueOf).toBe('valueOf');
expect(scope.hasOwnProperty('valueOf')).toBe(true);
})
);

Expand All @@ -2553,10 +2563,17 @@ describe('$compile', function() {
};

expect(func).not.toThrow();
expect(element.find('span').scope()).toBe(element.isolateScope());
expect(element.isolateScope()).not.toBe($rootScope);
expect(element.isolateScope()['constructor']).toBe($rootScope.constructor);
expect(element.isolateScope()['valueOf']).toBeUndefined();
var scope = element.isolateScope();
expect(element.find('span').scope()).toBe(scope);
expect(scope).not.toBe($rootScope);

// Does not shadow value because optional
expect(scope.constructor).toBe($rootScope.constructor);
expect(scope.hasOwnProperty('constructor')).toBe(false);

// Shadows value because not optional
expect(scope.valueOf).toBeUndefined();
expect(scope.hasOwnProperty('valueOf')).toBe(true);
})
);

Expand Down Expand Up @@ -3554,6 +3571,31 @@ describe('$compile', function() {
}));


it('should not overwrite @-bound property each digest when not present', function() {
module(function($compileProvider) {
$compileProvider.directive('testDir', valueFn({
scope: {prop: '@'},
controller: function($scope) {
$scope.prop = $scope.prop || 'default';
this.getProp = function() {
return $scope.prop;
};
},
controllerAs: 'ctrl',
template: '<p></p>'
}));
});
inject(function($compile, $rootScope) {
element = $compile('<div test-dir></div>')($rootScope);
var scope = element.isolateScope();
expect(scope.ctrl.getProp()).toBe('default');

$rootScope.$digest();
expect(scope.ctrl.getProp()).toBe('default');
});
});


describe('bind-once', function() {

function countWatches(scope) {
Expand Down Expand Up @@ -4415,6 +4457,34 @@ describe('$compile', function() {
childScope.theCtrl.test();
});
});

it('should not overwrite @-bound property each digest when not present', function() {
module(function($compileProvider) {
$compileProvider.directive('testDir', valueFn({
scope: {},
bindToController: {
prop: '@'
},
controller: function() {
var self = this;
this.prop = this.prop || 'default';
this.getProp = function() {
return self.prop;
};
},
controllerAs: 'ctrl',
template: '<p></p>'
}));
});
inject(function($compile, $rootScope) {
element = $compile('<div test-dir></div>')($rootScope);
var scope = element.isolateScope();
expect(scope.ctrl.getProp()).toBe('default');

$rootScope.$digest();
expect(scope.ctrl.getProp()).toBe('default');
});
});
});


Expand Down