Skip to content

Commit

Permalink
fix(scrolling): virtual scroll throw off if directive injects ViewCon…
Browse files Browse the repository at this point in the history
…tainerRef (#16137)

Fixes the virtual scrolling being thrown off and rendering the items in a wrong sequence, if there's a directive on the items that injects `ViewContainerRef`. The issue is due to the way we insert new views in a particular index. Rather than inserting the item at the index directly, we insert it and then move it into place. It looks like this behavior, coupled with Angular adding an extra comment node if something injects the `ViewContainerRef` causes the insertion order to be messed up.

Fixes #16130.
  • Loading branch information
crisbeto authored and josephperrott committed Jun 7, 2019
1 parent 8c4f25f commit 8daaf4d
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 15 deletions.
28 changes: 14 additions & 14 deletions src/cdk/scrolling/virtual-for-of.ts
Original file line number Diff line number Diff line change
Expand Up @@ -357,20 +357,20 @@ export class CdkVirtualForOf<T> implements CollectionViewer, DoCheck, OnDestroy

/** Creates a new embedded view and moves it to the given index */
private _createEmbeddedViewAt(index: number): EmbeddedViewRef<CdkVirtualForOfContext<T>> {
const view = this._viewContainerRef.createEmbeddedView(this._template, {
$implicit: null!,
cdkVirtualForOf: this._cdkVirtualForOf,
index: -1,
count: -1,
first: false,
last: false,
odd: false,
even: false
});
if (index < this._viewContainerRef.length) {
this._viewContainerRef.move(view, index);
}
return view;
// Note that it's important that we insert the item directly at the proper index,
// rather than inserting it and the moving it in place, because if there's a directive
// on the same node that injects the `ViewContainerRef`, Angular will insert another
// comment node which can throw off the move when it's being repeated for all items.
return this._viewContainerRef.createEmbeddedView(this._template, {
$implicit: null!,
cdkVirtualForOf: this._cdkVirtualForOf,
index: -1,
count: -1,
first: false,
last: false,
odd: false,
even: false
}, index);
}

/** Inserts a recycled view from the cache at the given index. */
Expand Down
73 changes: 72 additions & 1 deletion src/cdk/scrolling/virtual-scroll-viewport.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ import {
NgZone,
TrackByFunction,
ViewChild,
ViewEncapsulation
ViewEncapsulation,
Directive,
ViewContainerRef
} from '@angular/core';
import {async, ComponentFixture, fakeAsync, flush, inject, TestBed} from '@angular/core/testing';
import {animationFrameScheduler, Subject} from 'rxjs';
Expand Down Expand Up @@ -797,6 +799,36 @@ describe('CdkVirtualScrollViewport', () => {
'Error: cdk-virtual-scroll-viewport requires the "itemSize" property to be set.');
}));
});

describe('with item that injects ViewContainerRef', () => {
let fixture: ComponentFixture<VirtualScrollWithItemInjectingViewContainer>;
let testComponent: VirtualScrollWithItemInjectingViewContainer;
let viewport: CdkVirtualScrollViewport;

beforeEach(async(() => {
TestBed.configureTestingModule({
imports: [ScrollingModule],
declarations: [VirtualScrollWithItemInjectingViewContainer, InjectsViewContainer],
}).compileComponents();
}));

beforeEach(() => {
fixture = TestBed.createComponent(VirtualScrollWithItemInjectingViewContainer);
testComponent = fixture.componentInstance;
viewport = testComponent.viewport;
});

it('should render the values in the correct sequence when an item is ' +
'injecting ViewContainerRef', fakeAsync(() => {
finishInit(fixture);

const contentWrapper =
viewport.elementRef.nativeElement.querySelector('.cdk-virtual-scroll-content-wrapper')!;

expect(Array.from(contentWrapper.children).map(child => child.textContent!.trim()))
.toEqual(['0', '1', '2', '3', '4', '5', '6', '7']);
}));
});
});


Expand Down Expand Up @@ -938,3 +970,42 @@ class FixedSizeVirtualScrollWithRtlDirection {
class VirtualScrollWithNoStrategy {
items = [];
}

@Directive({
selector: '[injects-view-container]'
})
class InjectsViewContainer {
constructor(public viewContainerRef: ViewContainerRef) {
}
}

@Component({
template: `
<cdk-virtual-scroll-viewport itemSize="50">
<div injects-view-container class="item" *cdkVirtualFor="let item of items">{{item}}</div>
</cdk-virtual-scroll-viewport>
`,
styles: [`
.cdk-virtual-scroll-content-wrapper {
display: flex;
flex-direction: column;
}
.cdk-virtual-scroll-viewport {
width: 200px;
height: 200px;
}
.item {
width: 100%;
height: 50px;
}
`],
encapsulation: ViewEncapsulation.None
})
class VirtualScrollWithItemInjectingViewContainer {
@ViewChild(CdkVirtualScrollViewport, {static: true}) viewport: CdkVirtualScrollViewport;
itemSize = 50;
items = Array(20000).fill(0).map((_, i) => i);
}

0 comments on commit 8daaf4d

Please sign in to comment.