Skip to content

Commit 3437d56

Browse files
vsavkinvsavkin
committed
feat(core): made directives shadow native element properties
BREAKING CHANGE Previously, if an element had a property, Angular would update that property even if there was a directive placed on the same element with the same property. Now, the directive would have to explicitly update the native elmement by either using hostProperties or the renderer.
1 parent 3c58878 commit 3437d56

3 files changed

Lines changed: 107 additions & 39 deletions

File tree

‎modules/angular2/src/render/dom/view/proto_view_builder.ts‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -330,20 +330,27 @@ const STYLE_PREFIX = 'style';
330330

331331
function buildElementPropertyBindings(
332332
schemaRegistry: ElementSchemaRegistry, protoElement: /*element*/ any, isNgComponent: boolean,
333-
bindingsInTemplate: Map<string, ASTWithSource>, directiveTempaltePropertyNames: Set<string>):
333+
bindingsInTemplate: Map<string, ASTWithSource>, directiveTemplatePropertyNames: Set<string>):
334334
List<api.ElementPropertyBinding> {
335335
var propertyBindings = [];
336+
336337
MapWrapper.forEach(bindingsInTemplate, (ast, propertyNameInTemplate) => {
337338
var propertyBinding = createElementPropertyBinding(schemaRegistry, ast, propertyNameInTemplate);
338-
if (isValidElementPropertyBinding(schemaRegistry, protoElement, isNgComponent,
339-
propertyBinding)) {
339+
340+
if (isPresent(directiveTemplatePropertyNames) &&
341+
SetWrapper.has(directiveTemplatePropertyNames, propertyNameInTemplate)) {
342+
// We do nothing because directives shadow native elements properties.
343+
344+
} else if (isValidElementPropertyBinding(schemaRegistry, protoElement, isNgComponent,
345+
propertyBinding)) {
340346
propertyBindings.push(propertyBinding);
341-
} else if (!isPresent(directiveTempaltePropertyNames) ||
342-
!SetWrapper.has(directiveTempaltePropertyNames, propertyNameInTemplate)) {
343-
// directiveTempaltePropertyNames is null for host property bindings
347+
348+
} else {
344349
var exMsg =
345350
`Can't bind to '${propertyNameInTemplate}' since it isn't a known property of the '<${DOM.tagName(protoElement).toLowerCase()}>' element`;
346-
if (isPresent(directiveTempaltePropertyNames)) {
351+
352+
// directiveTemplatePropertyNames is null for host property bindings
353+
if (isPresent(directiveTemplatePropertyNames)) {
347354
exMsg += ' and there are no matching directives with a corresponding property';
348355
}
349356
throw new BaseException(exMsg);

‎modules/angular2/test/core/compiler/integration_spec.ts‎

Lines changed: 54 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import {
3131
isJsObject,
3232
global,
3333
stringify,
34+
isBlank,
3435
CONST,
3536
CONST_EXPR
3637
} from 'angular2/src/facade/lang';
@@ -1359,8 +1360,8 @@ export function main() {
13591360
});
13601361
}));
13611362

1362-
if (!IS_DARTIUM) {
1363-
describe('Missing property bindings', () => {
1363+
describe('Property bindings', () => {
1364+
if (!IS_DARTIUM) {
13641365
it('should throw on bindings to unknown properties',
13651366
inject([TestComponentBuilder, AsyncTestCompleter], (tcb: TestComponentBuilder,
13661367
async) => {
@@ -1386,8 +1387,47 @@ export function main() {
13861387
.createAsync(MyComp)
13871388
.then((val) => { async.done(); });
13881389
}));
1389-
});
1390-
}
1390+
}
1391+
1392+
it('should not be created when there is a directive with the same property',
1393+
inject([TestComponentBuilder, AsyncTestCompleter], (tcb: TestComponentBuilder, async) => {
1394+
tcb.overrideView(MyComp, new viewAnn.View({
1395+
template: '<span [title]="ctxProp"></span>',
1396+
directives: [DirectiveWithTitle]
1397+
}))
1398+
.createAsync(MyComp)
1399+
.then((rootTC) => {
1400+
rootTC.componentInstance.ctxProp = "TITLE";
1401+
rootTC.detectChanges();
1402+
1403+
var el = DOM.querySelector(rootTC.nativeElement, "span");
1404+
expect(isBlank(el.title) || el.title == '').toBeTruthy();
1405+
1406+
async.done();
1407+
1408+
});
1409+
}));
1410+
1411+
it('should work when a directive uses hostProperty to update the DOM element',
1412+
inject([TestComponentBuilder, AsyncTestCompleter], (tcb: TestComponentBuilder, async) => {
1413+
tcb.overrideView(MyComp, new viewAnn.View({
1414+
template: '<span [title]="ctxProp"></span>',
1415+
directives: [DirectiveWithTitleAndHostProperty]
1416+
}))
1417+
.createAsync(MyComp)
1418+
.then((rootTC) => {
1419+
rootTC.componentInstance.ctxProp = "TITLE";
1420+
rootTC.detectChanges();
1421+
1422+
var el = DOM.querySelector(rootTC.nativeElement, "span");
1423+
expect(el.title).toEqual("TITLE");
1424+
1425+
async.done();
1426+
1427+
});
1428+
}));
1429+
});
1430+
13911431

13921432
describe('different proto view storages', () => {
13931433
function runWithMode(mode: string) {
@@ -1505,6 +1545,16 @@ class MyDir {
15051545
constructor() { this.dirProp = ''; }
15061546
}
15071547

1548+
@Directive({selector: '[title]', properties: ['title']})
1549+
class DirectiveWithTitle {
1550+
title: string;
1551+
}
1552+
1553+
@Directive({selector: '[title]', properties: ['title'], host: {'[title]': 'title'}})
1554+
class DirectiveWithTitleAndHostProperty {
1555+
title: string;
1556+
}
1557+
15081558
@Component({selector: 'push-cmp', properties: ['prop'], changeDetection: ON_PUSH})
15091559
@View({template: '{{field}}'})
15101560
@Injectable()

‎modules/angular2/test/render/dom/view/proto_view_builder_spec.ts‎

Lines changed: 39 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -104,42 +104,53 @@ export function main() {
104104
var pv = builder.build(new DomElementSchemaRegistry(), templateCloner);
105105
expect(pv.elementBinders[0].propertyBindings[0].property).toEqual('readOnly');
106106
});
107-
108107
});
109108

110-
describe('property binding types', () => {
111-
it('should detect property names', () => {
112-
builder.bindElement(el('<div/>')).bindProperty('tabindex', emptyExpr());
113-
var pv = builder.build(new DomElementSchemaRegistry(), templateCloner);
114-
expect(pv.elementBinders[0].propertyBindings[0].type).toEqual(PropertyBindingType.PROPERTY);
115-
});
109+
describe('property binding', () => {
110+
describe('types', () => {
111+
it('should detect property names', () => {
112+
builder.bindElement(el('<div/>')).bindProperty('tabindex', emptyExpr());
113+
var pv = builder.build(new DomElementSchemaRegistry(), templateCloner);
114+
expect(pv.elementBinders[0].propertyBindings[0].type)
115+
.toEqual(PropertyBindingType.PROPERTY);
116+
});
116117

117-
it('should detect attribute names', () => {
118-
builder.bindElement(el('<div/>')).bindProperty('attr.someName', emptyExpr());
119-
var pv = builder.build(new DomElementSchemaRegistry(), templateCloner);
120-
expect(pv.elementBinders[0].propertyBindings[0].type)
121-
.toEqual(PropertyBindingType.ATTRIBUTE);
122-
});
118+
it('should detect attribute names', () => {
119+
builder.bindElement(el('<div/>')).bindProperty('attr.someName', emptyExpr());
120+
var pv = builder.build(new DomElementSchemaRegistry(), templateCloner);
121+
expect(pv.elementBinders[0].propertyBindings[0].type)
122+
.toEqual(PropertyBindingType.ATTRIBUTE);
123+
});
123124

124-
it('should detect class names', () => {
125-
builder.bindElement(el('<div/>')).bindProperty('class.someName', emptyExpr());
126-
var pv = builder.build(new DomElementSchemaRegistry(), templateCloner);
127-
expect(pv.elementBinders[0].propertyBindings[0].type).toEqual(PropertyBindingType.CLASS);
128-
});
125+
it('should detect class names', () => {
126+
builder.bindElement(el('<div/>')).bindProperty('class.someName', emptyExpr());
127+
var pv = builder.build(new DomElementSchemaRegistry(), templateCloner);
128+
expect(pv.elementBinders[0].propertyBindings[0].type).toEqual(PropertyBindingType.CLASS);
129+
});
129130

130-
it('should detect style names', () => {
131-
builder.bindElement(el('<div/>')).bindProperty('style.someName', emptyExpr());
132-
var pv = builder.build(new DomElementSchemaRegistry(), templateCloner);
133-
expect(pv.elementBinders[0].propertyBindings[0].type).toEqual(PropertyBindingType.STYLE);
134-
});
131+
it('should detect style names', () => {
132+
builder.bindElement(el('<div/>')).bindProperty('style.someName', emptyExpr());
133+
var pv = builder.build(new DomElementSchemaRegistry(), templateCloner);
134+
expect(pv.elementBinders[0].propertyBindings[0].type).toEqual(PropertyBindingType.STYLE);
135+
});
135136

136-
it('should detect style units', () => {
137-
builder.bindElement(el('<div/>')).bindProperty('style.someName.someUnit', emptyExpr());
138-
var pv = builder.build(new DomElementSchemaRegistry(), templateCloner);
139-
expect(pv.elementBinders[0].propertyBindings[0].unit).toEqual('someUnit');
137+
it('should detect style units', () => {
138+
builder.bindElement(el('<div/>')).bindProperty('style.someName.someUnit', emptyExpr());
139+
var pv = builder.build(new DomElementSchemaRegistry(), templateCloner);
140+
expect(pv.elementBinders[0].propertyBindings[0].unit).toEqual('someUnit');
141+
});
140142
});
141-
});
142143

144+
it('should not create a property binding when there is already same directive property binding',
145+
() => {
146+
var binder = builder.bindElement(el('<div/>'));
143147

148+
binder.bindProperty('tabindex', emptyExpr());
149+
binder.bindDirective(0).bindProperty('tabindex', emptyExpr(), 'tabindex');
150+
151+
var pv = builder.build(new DomElementSchemaRegistry(), templateCloner);
152+
expect(pv.elementBinders[0].propertyBindings.length).toEqual(0);
153+
});
154+
});
144155
});
145156
}

0 commit comments

Comments
 (0)