feat(query): view query is properly updated when dom changes.

Fixes a bug in view manager util where sibling injector is not correctly
calculated.

ViewQuery no longer includes the view's initiating component injector.

Includes some refactoring of view methods and a removal of a polymorphic
map call.

Closes #3033

Closes #3439
This commit is contained in:
Rado Kirov 2015-07-29 16:16:45 -07:00 committed by Rado Kirov
parent 720a3c8edd
commit 2150a8f9d1
5 changed files with 110 additions and 72 deletions

View File

@ -3,6 +3,7 @@ import {CONST} from 'angular2/src/facade/lang';
import {Locals} from './parser/locals'; import {Locals} from './parser/locals';
import {BindingRecord} from './binding_record'; import {BindingRecord} from './binding_record';
import {DirectiveIndex, DirectiveRecord} from './directive_record'; import {DirectiveIndex, DirectiveRecord} from './directive_record';
import {ChangeDetectorRef} from './change_detector_ref';
/** /**
* Interface used by Angular to control the change detection strategy for an application. * Interface used by Angular to control the change detection strategy for an application.
@ -50,6 +51,7 @@ export interface ChangeDispatcher {
export interface ChangeDetector { export interface ChangeDetector {
parent: ChangeDetector; parent: ChangeDetector;
mode: string; mode: string;
ref: ChangeDetectorRef;
addChild(cd: ChangeDetector): void; addChild(cd: ChangeDetector): void;
addShadowDomChild(cd: ChangeDetector): void; addShadowDomChild(cd: ChangeDetector): void;

View File

@ -421,7 +421,7 @@ class _Context {
export class ElementInjector extends TreeNode<ElementInjector> implements DependencyProvider { export class ElementInjector extends TreeNode<ElementInjector> implements DependencyProvider {
private _host: ElementInjector; private _host: ElementInjector;
private _preBuiltObjects = null; private _preBuiltObjects: PreBuiltObjects = null;
// Queries are added during construction or linking with a new parent. // Queries are added during construction or linking with a new parent.
// They are removed only through unlinking. // They are removed only through unlinking.
@ -477,19 +477,35 @@ export class ElementInjector extends TreeNode<ElementInjector> implements Depend
this._host = host; this._host = host;
this._preBuiltObjects = preBuiltObjects; this._preBuiltObjects = preBuiltObjects;
this._reattachInjectors(imperativelyCreatedInjector);
this._strategy.hydrate();
if (isPresent(host)) { if (isPresent(host)) {
this._addViewQueries(host); this._addViewQueries(host);
} }
this._reattachInjectors(imperativelyCreatedInjector);
this._strategy.hydrate();
this._addDirectivesToQueries(); this._addDirectivesToQueries();
this._addVarBindingsToQueries(); this._addVarBindingsToQueries();
// TODO(rado): optimize this call, if view queries are not moved around,
// simply appending to the query list is faster than updating.
this._updateViewQueries();
this.hydrated = true; this.hydrated = true;
} }
private _updateViewQueries() {
if (isPresent(this._query0) && this._query0.isViewQuery) {
this._query0.update();
}
if (isPresent(this._query1) && this._query1.isViewQuery) {
this._query1.update();
}
if (isPresent(this._query2) && this._query2.isViewQuery) {
this._query2.update();
}
}
private _debugContext(): any { private _debugContext(): any {
var p = this._preBuiltObjects; var p = this._preBuiltObjects;
var index = p.elementRef.boundElementIndex - p.view.elementOffset; var index = p.elementRef.boundElementIndex - p.view.elementOffset;
@ -651,23 +667,20 @@ export class ElementInjector extends TreeNode<ElementInjector> implements Depend
} }
private _addViewQueries(host: ElementInjector): void { private _addViewQueries(host: ElementInjector): void {
if (isPresent(host._query0) && host._query0.originator == host && this._addViewQuery(host._query0, host);
host._query0.query.isViewQuery) this._addViewQuery(host._query1, host);
this._addViewQuery(host._query0); this._addViewQuery(host._query2, host);
if (isPresent(host._query1) && host._query1.originator == host &&
host._query1.query.isViewQuery)
this._addViewQuery(host._query1);
if (isPresent(host._query2) && host._query2.originator == host &&
host._query2.query.isViewQuery)
this._addViewQuery(host._query2);
} }
private _addViewQuery(queryRef: QueryRef): void { private _addViewQuery(queryRef: QueryRef, host: ElementInjector): void {
if (isBlank(queryRef) || !queryRef.isViewQuery || this._hasQuery(queryRef)) return;
if (host._query0.originator == host) {
// TODO(rado): Replace this.parent check with distanceToParent = 1 when // TODO(rado): Replace this.parent check with distanceToParent = 1 when
// https://github.com/angular/angular/issues/2707 is fixed. // https://github.com/angular/angular/issues/2707 is fixed.
if (!queryRef.query.descendants && isPresent(this.parent)) return; if (!queryRef.query.descendants && isPresent(this.parent)) return;
this._assignQueryRef(queryRef); this._assignQueryRef(queryRef);
} }
}
private _addVarBindingsToQueries(): void { private _addVarBindingsToQueries(): void {
this._addVarBindingsToQuery(this._query0); this._addVarBindingsToQuery(this._query0);
@ -694,6 +707,7 @@ export class ElementInjector extends TreeNode<ElementInjector> implements Depend
private _addDirectivesToQuery(queryRef: QueryRef): void { private _addDirectivesToQuery(queryRef: QueryRef): void {
if (isBlank(queryRef) || queryRef.query.isVarBindingQuery) return; if (isBlank(queryRef) || queryRef.query.isVarBindingQuery) return;
if (queryRef.isViewQuery && queryRef.originator == this) return;
var matched = []; var matched = [];
this.addDirectivesMatchingQuery(queryRef.query, matched); this.addDirectivesMatchingQuery(queryRef.query, matched);
@ -754,42 +768,37 @@ export class ElementInjector extends TreeNode<ElementInjector> implements Depend
this._addParentQueries(); this._addParentQueries();
} }
unlink(): void {
var parent = this.parent;
this.remove();
this._removeParentQueries(parent);
}
private _addParentQueries(): void { private _addParentQueries(): void {
if (isBlank(this.parent)) return; if (isBlank(this.parent)) return;
if (isPresent(this.parent._query0) && !this.parent._query0.query.isViewQuery) { this._addParentQuery(this.parent._query0);
this._addQueryToTree(this.parent._query0); this._addParentQuery(this.parent._query1);
if (this.hydrated) this.parent._query0.update(); this._addParentQuery(this.parent._query2);
} }
if (isPresent(this.parent._query1) && !this.parent._query1.query.isViewQuery) {
this._addQueryToTree(this.parent._query1); private _addParentQuery(query): void {
if (this.hydrated) this.parent._query1.update(); if (isPresent(query) && !this._hasQuery(query)) {
} this._addQueryToTree(query);
if (isPresent(this.parent._query2) && !this.parent._query2.query.isViewQuery) { if (this.hydrated) query.update();
this._addQueryToTree(this.parent._query2);
if (this.hydrated) this.parent._query2.update();
} }
} }
unlink(): void { private _removeParentQueries(parent: ElementInjector): void {
var queriesToUpdate = []; this._removeParentQuery(parent._query0);
if (isPresent(this.parent._query0)) { this._removeParentQuery(parent._query1);
this._pruneQueryFromTree(this.parent._query0); this._removeParentQuery(parent._query2);
queriesToUpdate.push(this.parent._query0);
}
if (isPresent(this.parent._query1)) {
this._pruneQueryFromTree(this.parent._query1);
queriesToUpdate.push(this.parent._query1);
}
if (isPresent(this.parent._query2)) {
this._pruneQueryFromTree(this.parent._query2);
queriesToUpdate.push(this.parent._query2);
} }
this.remove(); private _removeParentQuery(query: QueryRef) {
// TODO(rado): update should work on view queries too, however currently it if (isPresent(query)) {
// is not implemented, so we filter to non-view queries. this._pruneQueryFromTree(query);
var nonViewQueries = ListWrapper.filter(queriesToUpdate, (q) => !q.query.isViewQuery); query.update();
ListWrapper.forEach(nonViewQueries, (q) => q.update()); }
} }
private _pruneQueryFromTree(query: QueryRef): void { private _pruneQueryFromTree(query: QueryRef): void {
@ -852,6 +861,12 @@ export class ElementInjector extends TreeNode<ElementInjector> implements Depend
getHost(): ElementInjector { return this._host; } getHost(): ElementInjector { return this._host; }
getBoundElementIndex(): number { return this._proto.index; } getBoundElementIndex(): number { return this._proto.index; }
getRootViewInjectors(): ElementInjector[] {
var view = this._preBuiltObjects.view;
return view.getNestedView(view.elementOffset + this.getBoundElementIndex())
.rootElementInjectors;
}
} }
interface _ElementInjectorStrategy { interface _ElementInjectorStrategy {
@ -1133,9 +1148,19 @@ export class QueryRef {
constructor(public query: Query, public list: QueryList<any>, constructor(public query: Query, public list: QueryList<any>,
public originator: ElementInjector) {} public originator: ElementInjector) {}
get isViewQuery(): boolean { return this.query.isViewQuery; }
update(): void { update(): void {
var aggregator = []; var aggregator = [];
if (this.query.isViewQuery) {
// intentionally skipping originator for view queries.
var rootViewInjectors = this.originator.getRootViewInjectors();
for (var i = 0; i < rootViewInjectors.length; i++) {
this.visit(rootViewInjectors[i], aggregator);
}
} else {
this.visit(this.originator, aggregator); this.visit(this.originator, aggregator);
}
this.list.reset(aggregator); this.list.reset(aggregator);
} }

View File

@ -120,13 +120,14 @@ export class AppViewManagerUtils {
parentView.viewContainers[boundElementIndex] = viewContainer; parentView.viewContainers[boundElementIndex] = viewContainer;
} }
ListWrapper.insert(viewContainer.views, atIndex, view); ListWrapper.insert(viewContainer.views, atIndex, view);
var elementInjector = contextView.elementInjectors[contextBoundElementIndex];
var sibling; var sibling;
if (atIndex == 0) { if (atIndex == 0) {
sibling = null; sibling = elementInjector;
} else { } else {
sibling = ListWrapper.last(viewContainer.views[atIndex - 1].rootElementInjectors); sibling = ListWrapper.last(viewContainer.views[atIndex - 1].rootElementInjectors);
} }
var elementInjector = contextView.elementInjectors[contextBoundElementIndex];
for (var i = view.rootElementInjectors.length - 1; i >= 0; i--) { for (var i = view.rootElementInjectors.length - 1; i >= 0; i--) {
if (isPresent(elementInjector.parent)) { if (isPresent(elementInjector.parent)) {
view.rootElementInjectors[i].linkAfter(elementInjector.parent, sibling); view.rootElementInjectors[i].linkAfter(elementInjector.parent, sibling);

View File

@ -355,7 +355,7 @@ export function main() {
view.detectChanges(); view.detectChanges();
expect(q.query.map((d: TextDirective) => d.text)).toEqual(["self", "1", "2", "3"]); expect(q.query.map((d: TextDirective) => d.text)).toEqual(["1", "2", "3"]);
async.done(); async.done();
}); });
@ -407,26 +407,29 @@ export function main() {
}); });
})); }));
/* TODO(rado): fix and reenable.
it('should maintain directives in pre-order depth-first DOM order after dynamic insertion', it('should maintain directives in pre-order depth-first DOM order after dynamic insertion',
inject([TestComponentBuilder, AsyncTestCompleter], (tcb:TestComponentBuilder, async) => { inject([TestComponentBuilder, AsyncTestCompleter], (tcb: TestComponentBuilder, async) => {
var template = '<needs-view-query-order #q text="self"></needs-view-query-order>'; var template = '<needs-view-query-order #q></needs-view-query-order>';
tcb.overrideTemplate(MyComp, template) tcb.overrideTemplate(MyComp, template)
.createAsync(MyComp) .createAsync(MyComp)
.then((view) => { .then((view) => {
var q:NeedsViewQueryOrder = view.componentViewChildren[0].getLocal("q"); var q: NeedsViewQueryOrder = view.componentViewChildren[0].getLocal("q");
view.detectChanges(); view.detectChanges();
expect(q.query.length).toBe(4); expect(q.query.map((d: TextDirective) => d.text)).toEqual(["1", "2", "3", "4"]);
expect(q.query.first.text).toEqual("1");
expect(q.query.first.text).toEqual("4"); q.list = ["-3", "2"];
view.detectChanges();
expect(q.query.map((d: TextDirective) => d.text)).toEqual(["1", "-3", "2", "4"]);
async.done(); async.done();
}); });
}));*/ }));
}); });
}); });
} }
@ -551,17 +554,23 @@ class NeedsViewQueryNestedIf {
} }
// TODO(rado): once https://github.com/angular/angular/issues/3438 is resolved
// duplicate the test without InertDirective.
@Component({selector: 'needs-view-query-order'}) @Component({selector: 'needs-view-query-order'})
@View({ @View({
directives: [NgFor, TextDirective], directives: [NgFor, TextDirective, InertDirective],
template: '<div text="1">' + template: '<div dir><div text="1"></div>' +
'<div *ng-for="var i of [\'2\', \'3\']" [text]="i"></div>' + '<div *ng-for="var i of list" [text]="i"></div>' +
'<div text="4">' '<div text="4"></div></div>'
}) })
@Injectable() @Injectable()
class NeedsViewQueryOrder { class NeedsViewQueryOrder {
query: QueryList<TextDirective>; query: QueryList<TextDirective>;
constructor(@ViewQuery(TextDirective) query: QueryList<TextDirective>) { this.query = query; } list: string[];
constructor(@ViewQuery(TextDirective, {descendants: true}) query: QueryList<TextDirective>) {
this.query = query;
this.list = ['2', '3'];
}
} }
@Component({selector: 'needs-tpl'}) @Component({selector: 'needs-tpl'})

View File

@ -171,7 +171,8 @@ export function main() {
createViews(2); createViews(2);
utils.attachViewInContainer(parentView, 0, contextView, 1, 0, childView); utils.attachViewInContainer(parentView, 0, contextView, 1, 0, childView);
expect(childView.rootElementInjectors[0].spy('linkAfter')) expect(childView.rootElementInjectors[0].spy('linkAfter'))
.toHaveBeenCalledWith(contextView.elementInjectors[0], null); .toHaveBeenCalledWith(contextView.elementInjectors[0],
contextView.elementInjectors[1]);
}); });
}); });