fix(core): ensure ngFor only inserts/moves/removes elements when necessary (#10287)

Closes #9960
Closes #7239
Closes #9672
Closes #9454
Closes #10287
This commit is contained in:
Matias Niemelä 2016-08-01 11:09:52 -07:00 committed by GitHub
parent 4df7b1cfbc
commit e18626b7a2
10 changed files with 428 additions and 91 deletions

View File

@ -117,24 +117,23 @@ export class NgFor implements DoCheck, OnChanges {
}
private _applyChanges(changes: DefaultIterableDiffer) {
// TODO(rado): check if change detection can produce a change record that is
// easier to consume than current.
const recordViewTuples: RecordViewTuple[] = [];
changes.forEachRemovedItem(
(removedRecord: CollectionChangeRecord) =>
recordViewTuples.push(new RecordViewTuple(removedRecord, null)));
changes.forEachMovedItem(
(movedRecord: CollectionChangeRecord) =>
recordViewTuples.push(new RecordViewTuple(movedRecord, null)));
const insertTuples = this._bulkRemove(recordViewTuples);
changes.forEachAddedItem(
(addedRecord: CollectionChangeRecord) =>
insertTuples.push(new RecordViewTuple(addedRecord, null)));
this._bulkInsert(insertTuples);
const insertTuples: RecordViewTuple[] = [];
changes.forEachOperation(
(item: CollectionChangeRecord, adjustedPreviousIndex: number, currentIndex: number) => {
if (item.previousIndex == null) {
let view = this._viewContainer.createEmbeddedView(
this._templateRef, new NgForRow(null, null, null), currentIndex);
let tuple = new RecordViewTuple(item, view);
insertTuples.push(tuple);
} else if (currentIndex == null) {
this._viewContainer.remove(adjustedPreviousIndex);
} else {
let view = this._viewContainer.get(adjustedPreviousIndex);
this._viewContainer.move(view, currentIndex);
let tuple = new RecordViewTuple(item, <EmbeddedViewRef<NgForRow>>view);
insertTuples.push(tuple);
}
});
for (let i = 0; i < insertTuples.length; i++) {
this._perViewChange(insertTuples[i].view, insertTuples[i].record);
@ -155,39 +154,6 @@ export class NgFor implements DoCheck, OnChanges {
private _perViewChange(view: EmbeddedViewRef<NgForRow>, record: CollectionChangeRecord) {
view.context.$implicit = record.item;
}
private _bulkRemove(tuples: RecordViewTuple[]): RecordViewTuple[] {
tuples.sort(
(a: RecordViewTuple, b: RecordViewTuple) =>
a.record.previousIndex - b.record.previousIndex);
const movedTuples: RecordViewTuple[] = [];
for (let i = tuples.length - 1; i >= 0; i--) {
const tuple = tuples[i];
// separate moved views from removed views.
if (isPresent(tuple.record.currentIndex)) {
tuple.view =
<EmbeddedViewRef<NgForRow>>this._viewContainer.detach(tuple.record.previousIndex);
movedTuples.push(tuple);
} else {
this._viewContainer.remove(tuple.record.previousIndex);
}
}
return movedTuples;
}
private _bulkInsert(tuples: RecordViewTuple[]): RecordViewTuple[] {
tuples.sort((a, b) => a.record.currentIndex - b.record.currentIndex);
for (let i = 0; i < tuples.length; i++) {
var tuple = tuples[i];
if (isPresent(tuple.view)) {
this._viewContainer.insert(tuple.view, tuple.record.currentIndex);
} else {
tuple.view = this._viewContainer.createEmbeddedView(
this._templateRef, new NgForRow(null, null, null), tuple.record.currentIndex);
}
}
return tuples;
}
}
class RecordViewTuple {

View File

@ -63,6 +63,56 @@ export class DefaultIterableDiffer implements IterableDiffer {
}
}
forEachOperation(
fn: (item: CollectionChangeRecord, previousIndex: number, currentIndex: number) => void) {
var nextIt = this._itHead;
var nextRemove = this._removalsHead;
var addRemoveOffset = 0;
var moveOffsets: number[] = null;
while (nextIt || nextRemove) {
// Figure out which is the next record to process
// Order: remove, add, move
let record = !nextRemove ||
nextIt &&
nextIt.currentIndex < getPreviousIndex(nextRemove, addRemoveOffset, moveOffsets) ?
nextIt :
nextRemove;
var adjPreviousIndex = getPreviousIndex(record, addRemoveOffset, moveOffsets);
var currentIndex = record.currentIndex;
// consume the item, and adjust the addRemoveOffset and update moveDistance if necessary
if (record === nextRemove) {
addRemoveOffset--;
nextRemove = nextRemove._nextRemoved;
} else {
nextIt = nextIt._next;
if (record.previousIndex == null) {
addRemoveOffset++;
} else {
// INVARIANT: currentIndex < previousIndex
if (!moveOffsets) moveOffsets = [];
let localMovePreviousIndex = adjPreviousIndex - addRemoveOffset;
let localCurrentIndex = currentIndex - addRemoveOffset;
if (localMovePreviousIndex != localCurrentIndex) {
for (var i = 0; i < localMovePreviousIndex; i++) {
var offset = i < moveOffsets.length ? moveOffsets[i] : (moveOffsets[i] = 0);
var index = offset + i;
if (localCurrentIndex <= index && index < localMovePreviousIndex) {
moveOffsets[i] = offset + 1;
}
}
var previousIndex = record.previousIndex;
moveOffsets[previousIndex] = localCurrentIndex - localMovePreviousIndex;
}
}
}
if (adjPreviousIndex !== currentIndex) {
fn(record, adjPreviousIndex, currentIndex);
}
}
}
forEachPreviousItem(fn: Function) {
var record: CollectionChangeRecord;
for (record = this._previousItHead; record !== null; record = record._nextPrevious) {
@ -700,3 +750,13 @@ class _DuplicateMap {
toString(): string { return '_DuplicateMap(' + stringify(this.map) + ')'; }
}
function getPreviousIndex(item: any, addRemoveOffset: number, moveOffsets: number[]): number {
var previousIndex = item.previousIndex;
if (previousIndex === null) return previousIndex;
var moveOffset = 0;
if (moveOffsets && previousIndex < moveOffsets.length) {
moveOffset = moveOffsets[previousIndex];
}
return previousIndex + addRemoveOffset + moveOffset;
}

View File

@ -60,6 +60,30 @@ export class AppElement {
return result;
}
moveView(view: AppView<any>, currentIndex: number) {
var previousIndex = this.nestedViews.indexOf(view);
if (view.type === ViewType.COMPONENT) {
throw new BaseException(`Component views can't be moved!`);
}
var nestedViews = this.nestedViews;
if (nestedViews == null) {
nestedViews = [];
this.nestedViews = nestedViews;
}
ListWrapper.removeAt(nestedViews, previousIndex);
ListWrapper.insert(nestedViews, currentIndex, view);
var refRenderNode: any /** TODO #9100 */;
if (currentIndex > 0) {
var prevView = nestedViews[currentIndex - 1];
refRenderNode = prevView.lastRootNode;
} else {
refRenderNode = this.nativeElement;
}
if (isPresent(refRenderNode)) {
view.renderer.attachViewAfter(refRenderNode, view.flatRootNodes);
}
view.markContentChildAsMoved(this);
}
attachView(view: AppView<any>, viewIndex: number) {
if (view.type === ViewType.COMPONENT) {

View File

@ -288,6 +288,8 @@ export abstract class AppView<T> {
}
}
markContentChildAsMoved(renderAppElement: AppElement): void { this.dirtyParentQueriesInternal(); }
addToContentChildren(renderAppElement: AppElement): void {
renderAppElement.parentView.contentChildren.push(this);
this.viewContainerElement = renderAppElement;

View File

@ -99,6 +99,13 @@ export abstract class ViewContainerRef {
*/
abstract insert(viewRef: ViewRef, index?: number): ViewRef;
/**
* Moves a View identified by a {@link ViewRef} into the container at the specified `index`.
*
* Returns the inserted {@link ViewRef}.
*/
abstract move(viewRef: ViewRef, currentIndex: number): ViewRef;
/**
* Returns the index of the View, specified via {@link ViewRef}, within the current container or
* `-1` if this container doesn't contain the View.
@ -170,6 +177,14 @@ export class ViewContainerRef_ implements ViewContainerRef {
return wtfLeave(s, viewRef_);
}
move(viewRef: ViewRef, currentIndex: number): ViewRef {
var s = this._insertScope();
if (currentIndex == -1) return;
var viewRef_ = <ViewRef_<any>>viewRef;
this._element.moveView(viewRef_.internalView, currentIndex);
return wtfLeave(s, viewRef_);
}
indexOf(viewRef: ViewRef): number {
return ListWrapper.indexOf(this._element.nestedViews, (<ViewRef_<any>>viewRef).internalView);
}

View File

@ -749,6 +749,135 @@ function declareTests({useJit}: {useJit: boolean}) {
})));
});
describe('ng directives', () => {
describe('*ngFor', () => {
let tpl = '<div *ngFor="let item of items" @trigger>{{ item }}</div>';
let getText =
(node: any) => { return node.innerHTML ? node.innerHTML : node.children[0].data; };
let assertParentChildContents = (parent: any, content: string) => {
var values: string[] = [];
for (var i = 0; i < parent.childNodes.length; i++) {
let child = parent.childNodes[i];
if (child['nodeType'] == 1) {
values.push(getText(child).trim());
}
}
var value = values.join(' -> ');
expect(value).toEqual(content);
};
it('should animate when items are inserted into the list at different points',
inject(
[TestComponentBuilder, AnimationDriver],
fakeAsync((tcb: TestComponentBuilder, driver: MockAnimationDriver) => {
makeAnimationCmp(
tcb, tpl,
[
trigger('trigger', [transition('void => *', [animate(1000)])]),
],
(fixture: any /** TODO #9100 */) => {
var cmp = fixture.debugElement.componentInstance;
var parent = fixture.debugElement.nativeElement;
cmp.items = [0, 2, 4, 6, 8];
fixture.detectChanges();
flushMicrotasks();
expect(driver.log.length).toEqual(5);
assertParentChildContents(parent, '0 -> 2 -> 4 -> 6 -> 8');
driver.log = [];
cmp.items = [0, 1, 2, 3, 4, 5, 6, 7, 8];
fixture.detectChanges();
flushMicrotasks();
expect(driver.log.length).toEqual(4);
assertParentChildContents(
parent, '0 -> 1 -> 2 -> 3 -> 4 -> 5 -> 6 -> 7 -> 8');
});
})));
it('should animate when items are removed + moved into the list at different points and retain DOM ordering during the animation',
inject(
[TestComponentBuilder, AnimationDriver],
fakeAsync((tcb: TestComponentBuilder, driver: MockAnimationDriver) => {
makeAnimationCmp(
tcb, tpl,
[
trigger('trigger', [transition('* => *', [animate(1000)])]),
],
(fixture: any /** TODO #9100 */) => {
var cmp = fixture.debugElement.componentInstance;
var parent = fixture.debugElement.nativeElement;
cmp.items = [0, 1, 2, 3, 4];
fixture.detectChanges();
flushMicrotasks();
expect(driver.log.length).toEqual(5);
driver.log = [];
cmp.items = [3, 4, 0, 9];
fixture.detectChanges();
flushMicrotasks();
// TODO (matsko): update comment below once move animations are a thing
// there are only three animations since we do
// not yet support move-based animations
expect(driver.log.length).toEqual(3);
// move(~), add(+), remove(-)
// -1, -2, ~3, ~4, ~0, +9
var rm0 = driver.log.shift();
var rm1 = driver.log.shift();
var in0 = driver.log.shift();
// we want to assert that the DOM chain is still preserved
// until the animations are closed
assertParentChildContents(parent, '3 -> 4 -> 0 -> 9 -> 1 -> 2');
rm0['player'].finish();
assertParentChildContents(parent, '3 -> 4 -> 0 -> 9 -> 2');
rm1['player'].finish();
assertParentChildContents(parent, '3 -> 4 -> 0 -> 9');
});
})));
});
describe('[ngClass]', () => {
it('should persist ngClass class values when a remove element animation is active',
inject(
[TestComponentBuilder, AnimationDriver],
fakeAsync(
(tcb: TestComponentBuilder, driver: InnerContentTrackingAnimationDriver) => {
makeAnimationCmp(
tcb, `<div [ngClass]="exp2" *ngIf="exp" @trigger></div>`,
[
trigger('trigger', [transition('* => void', [animate(1000)])]),
],
(fixture: any /** TODO #9100 */) => {
var cmp = fixture.debugElement.componentInstance;
cmp.exp = true;
cmp.exp2 = 'blue';
fixture.detectChanges();
flushMicrotasks();
expect(driver.log.length).toEqual(0);
cmp.exp = false;
fixture.detectChanges();
flushMicrotasks();
var animation = driver.log.pop();
var element = animation['element'];
(<any>expect(element)).toHaveCssClass('blue');
});
})));
});
});
describe('DOM order tracking', () => {
if (!getDOM().supportsDOMEvents()) return;
@ -875,39 +1004,6 @@ function declareTests({useJit}: {useJit: boolean}) {
})));
});
describe('ng directives', () => {
describe('[ngClass]', () => {
it('should persist ngClass class values when a remove element animation is active',
inject(
[TestComponentBuilder, AnimationDriver],
fakeAsync(
(tcb: TestComponentBuilder, driver: InnerContentTrackingAnimationDriver) => {
makeAnimationCmp(
tcb, `<div [ngClass]="exp2" *ngIf="exp" @trigger></div>`,
[
trigger('trigger', [transition('* => void', [animate(1000)])]),
],
(fixture: any /** TODO #9100 */) => {
var cmp = fixture.debugElement.componentInstance;
cmp.exp = true;
cmp.exp2 = 'blue';
fixture.detectChanges();
flushMicrotasks();
expect(driver.log.length).toEqual(0);
cmp.exp = false;
fixture.detectChanges();
flushMicrotasks();
var animation = driver.log.pop();
var element = animation['element'];
(<any>expect(element)).toHaveCssClass('blue');
});
})));
});
});
describe('animation states', () => {
it('should throw an error when an animation is referenced that isn\'t defined within the component annotation',
inject(
@ -1273,6 +1369,7 @@ class InnerContentTrackingAnimationPlayer extends MockAnimationPlayer {
class DummyIfCmp {
exp = false;
exp2 = false;
items = [0, 1, 2, 3, 4];
}
@Component({

View File

@ -298,6 +298,165 @@ export function main() {
}));
});
describe('forEachOperation', () => {
function stringifyItemChange(record: any, p: number, c: number, originalIndex: number) {
var suffix = originalIndex == null ? '' : ' [o=' + originalIndex + ']';
var value = record.item;
if (record.currentIndex == null) {
return `REMOVE ${value} (${p} -> VOID)${suffix}`;
} else if (record.previousIndex == null) {
return `INSERT ${value} (VOID -> ${c})${suffix}`;
} else {
return `MOVE ${value} (${p} -> ${c})${suffix}`;
}
}
function modifyArrayUsingOperation(
arr: number[], endData: any[], prev: number, next: number) {
var value: number = null;
if (prev == null) {
value = endData[next];
arr.splice(next, 0, value);
} else if (next == null) {
value = arr[prev];
arr.splice(prev, 1);
} else {
value = arr[prev];
arr.splice(prev, 1);
arr.splice(next, 0, value);
}
return value;
}
it('should trigger a series of insert/move/remove changes for inputs that have been diffed',
() => {
var startData = [0, 1, 2, 3, 4, 5];
var endData = [6, 2, 7, 0, 4, 8];
differ = differ.diff(startData);
differ = differ.diff(endData);
var operations: string[] = [];
differ.forEachOperation((item: any, prev: number, next: number) => {
var value = modifyArrayUsingOperation(startData, endData, prev, next);
operations.push(stringifyItemChange(item, prev, next, item.previousIndex));
});
expect(operations).toEqual([
'INSERT 6 (VOID -> 0)', 'MOVE 2 (3 -> 1) [o=2]', 'INSERT 7 (VOID -> 2)',
'REMOVE 1 (4 -> VOID) [o=1]', 'REMOVE 3 (4 -> VOID) [o=3]',
'REMOVE 5 (5 -> VOID) [o=5]', 'INSERT 8 (VOID -> 5)'
]);
expect(startData).toEqual(endData);
});
it('should consider inserting/removing/moving items with respect to items that have not moved at all',
() => {
var startData = [0, 1, 2, 3];
var endData = [2, 1];
differ = differ.diff(startData);
differ = differ.diff(endData);
var operations: string[] = [];
differ.forEachOperation((item: any, prev: number, next: number) => {
var value = modifyArrayUsingOperation(startData, endData, prev, next);
operations.push(stringifyItemChange(item, prev, next, item.previousIndex));
});
expect(operations).toEqual([
'REMOVE 0 (0 -> VOID) [o=0]', 'MOVE 2 (1 -> 0) [o=2]', 'REMOVE 3 (2 -> VOID) [o=3]'
]);
expect(startData).toEqual(endData);
});
it('should be able to manage operations within a criss/cross of move operations', () => {
var startData = [1, 2, 3, 4, 5, 6];
var endData = [3, 6, 4, 9, 1, 2];
differ = differ.diff(startData);
differ = differ.diff(endData);
var operations: string[] = [];
differ.forEachOperation((item: any, prev: number, next: number) => {
var value = modifyArrayUsingOperation(startData, endData, prev, next);
operations.push(stringifyItemChange(item, prev, next, item.previousIndex));
});
expect(operations).toEqual([
'MOVE 3 (2 -> 0) [o=2]', 'MOVE 6 (5 -> 1) [o=5]', 'MOVE 4 (4 -> 2) [o=3]',
'INSERT 9 (VOID -> 3)', 'REMOVE 5 (6 -> VOID) [o=4]'
]);
expect(startData).toEqual(endData);
});
it('should skip moves for multiple nodes that have not moved', () => {
var startData = [0, 1, 2, 3, 4];
var endData = [4, 1, 2, 3, 0, 5];
differ = differ.diff(startData);
differ = differ.diff(endData);
var operations: string[] = [];
differ.forEachOperation((item: any, prev: number, next: number) => {
var value = modifyArrayUsingOperation(startData, endData, prev, next);
operations.push(stringifyItemChange(item, prev, next, item.previousIndex));
});
expect(operations).toEqual([
'MOVE 4 (4 -> 0) [o=4]', 'MOVE 1 (2 -> 1) [o=1]', 'MOVE 2 (3 -> 2) [o=2]',
'MOVE 3 (4 -> 3) [o=3]', 'INSERT 5 (VOID -> 5)'
]);
expect(startData).toEqual(endData);
});
it('should not fail', () => {
var startData = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11];
var endData = [10, 11, 1, 5, 7, 8, 0, 5, 3, 6];
differ = differ.diff(startData);
differ = differ.diff(endData);
var operations: string[] = [];
differ.forEachOperation((item: any, prev: number, next: number) => {
var value = modifyArrayUsingOperation(startData, endData, prev, next);
operations.push(stringifyItemChange(item, prev, next, item.previousIndex));
});
expect(operations).toEqual([
'MOVE 10 (10 -> 0) [o=10]', 'MOVE 11 (11 -> 1) [o=11]', 'MOVE 1 (3 -> 2) [o=1]',
'MOVE 5 (7 -> 3) [o=5]', 'MOVE 7 (9 -> 4) [o=7]', 'MOVE 8 (10 -> 5) [o=8]',
'REMOVE 2 (7 -> VOID) [o=2]', 'INSERT 5 (VOID -> 7)', 'REMOVE 4 (9 -> VOID) [o=4]',
'REMOVE 9 (10 -> VOID) [o=9]'
]);
expect(startData).toEqual(endData);
});
it('should trigger nothing when the list is completely full of replaced items that are tracked by the index',
() => {
differ = new DefaultIterableDiffer((index: number) => index);
var startData = [1, 2, 3, 4];
var endData = [5, 6, 7, 8];
differ = differ.diff(startData);
differ = differ.diff(endData);
var operations: string[] = [];
differ.forEachOperation((item: any, prev: number, next: number) => {
var value = modifyArrayUsingOperation(startData, endData, prev, next);
operations.push(stringifyItemChange(item, prev, next, item.previousIndex));
});
expect(operations).toEqual([]);
});
});
describe('diff', () => {
it('should return self when there is a change', () => {
expect(differ.diff(['a', 'b'])).toBe(differ);

View File

@ -29,14 +29,13 @@ import {
<button (click)="state='active'">Active State</button>
|
<button (click)="state='void'">Void State</button>
<button (click)="reorderAndRemove()">Scramble!</button>
<button (click)="state='default'">Unhandled (default) State</button>
<button style="float:right" (click)="bgStatus='blur'">Blur Page (Host)</button>
<hr />
<div *ngFor="let item of items" class="box" [@boxAnimation]="state">
{{ item }}
<div *ngIf="true">
something inside
</div>
<div *ngFor="let item of items; let i=index" class="box" [@boxAnimation]="state">
{{ item }} - {{ i }}
<button (click)="remove(item)">x</button>
</div>
`,
animations: [
@ -77,6 +76,20 @@ export class AnimateApp {
public bgStatus = 'focus';
remove(item: any) {
var index = this.items.indexOf(item);
if (index >= 0) {
this.items.splice(index, 1);
}
}
reorderAndRemove() {
this.items = this.items.sort((a: any,b: any) => Math.random() - 0.5);
this.items.splice(Math.floor(Math.random() * this.items.length), 1);
this.items.splice(Math.floor(Math.random() * this.items.length), 1);
this.items[Math.floor(Math.random() * this.items.length)] = 99;
}
get state() { return this._state; }
set state(s) {
this._state = s;

View File

@ -7,10 +7,9 @@ button {
}
.box {
font-size:50px;
font-size:40px;
border:10px solid black;
width:200px;
font-size:80px;
line-height:100px;
height:100px;
display:inline-block;

View File

@ -463,6 +463,7 @@ export declare class DefaultIterableDiffer implements IterableDiffer {
forEachIdentityChange(fn: Function): void;
forEachItem(fn: Function): void;
forEachMovedItem(fn: Function): void;
forEachOperation(fn: (item: CollectionChangeRecord, previousIndex: number, currentIndex: number) => void): void;
forEachPreviousItem(fn: Function): void;
forEachRemovedItem(fn: Function): void;
onDestroy(): void;
@ -1370,6 +1371,7 @@ export declare abstract class ViewContainerRef {
abstract get(index: number): ViewRef;
abstract indexOf(viewRef: ViewRef): number;
abstract insert(viewRef: ViewRef, index?: number): ViewRef;
abstract move(viewRef: ViewRef, currentIndex: number): ViewRef;
abstract remove(index?: number): void;
}