Skip to content

virtual-scroll: use detectChanges rather than markForCheck in CdkVirtualScrollViewport #12158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 12, 2018
Merged

virtual-scroll: use detectChanges rather than markForCheck in CdkVirtualScrollViewport #12158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8438ba8
virtual-scroll: use `detectChanges` rather than `markForCheck` in
mmalerba Jul 11, 2018
38afb9e
fix lint and tests
mmalerba Jul 11, 2018
138e2be
address comments
mmalerba Jul 11, 2018
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
37 changes: 36 additions & 1 deletion src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ describe('CdkVirtualScrollViewport', () => {
viewport.setRenderedContentOffset(
'arbitrary string as offset' as any, 'arbitrary string as to' as any);
fixture.detectChanges();
flush();

expect((viewport._renderedContentTransform as any).changingThisBreaksApplicationSecurity)
.toBe('translateY(NaNpx)');
Expand Down Expand Up @@ -58,11 +59,13 @@ describe('CdkVirtualScrollViewport', () => {
it('should update viewport size', fakeAsync(() => {
testComponent.viewportSize = 300;
fixture.detectChanges();
flush();
viewport.checkViewportSize();
expect(viewport.getViewportSize()).toBe(300);

testComponent.viewportSize = 500;
fixture.detectChanges();
flush();
viewport.checkViewportSize();
expect(viewport.getViewportSize()).toBe(500);
}));
Expand All @@ -78,6 +81,7 @@ describe('CdkVirtualScrollViewport', () => {
finishInit(fixture);
triggerScroll(viewport, testComponent.itemSize + 5);
fixture.detectChanges();
flush();

expect(viewport.getOffsetToRenderedContentStart()).toBe(testComponent.itemSize,
'should have 50px offset since first 50px item is not rendered');
Expand All @@ -87,6 +91,7 @@ describe('CdkVirtualScrollViewport', () => {
finishInit(fixture);
triggerScroll(viewport, testComponent.itemSize + 5);
fixture.detectChanges();
flush();

expect(viewport.measureScrollOffset()).toBe(testComponent.itemSize + 5);
}));
Expand All @@ -110,6 +115,7 @@ describe('CdkVirtualScrollViewport', () => {
finishInit(fixture);
viewport.setTotalContentSize(10000);
fixture.detectChanges();
flush();

expect(viewport.elementRef.nativeElement.scrollHeight).toBe(10000);
}));
Expand All @@ -118,6 +124,7 @@ describe('CdkVirtualScrollViewport', () => {
finishInit(fixture);
viewport.setRenderedRange({start: 2, end: 3});
fixture.detectChanges();
flush();

const items = fixture.elementRef.nativeElement.querySelectorAll('.item');
expect(items.length).toBe(1, 'Expected 1 item to be rendered');
Expand All @@ -128,6 +135,7 @@ describe('CdkVirtualScrollViewport', () => {
finishInit(fixture);
viewport.setRenderedContentOffset(10, 'to-start');
fixture.detectChanges();
flush();

expect(viewport.getOffsetToRenderedContentStart()).toBe(10);
}));
Expand All @@ -140,6 +148,7 @@ describe('CdkVirtualScrollViewport', () => {

viewport.setRenderedContentOffset(contentSize + 10, 'to-end');
fixture.detectChanges();
flush();

expect(viewport.getOffsetToRenderedContentStart()).toBe(10);
}));
Expand All @@ -148,8 +157,11 @@ describe('CdkVirtualScrollViewport', () => {
finishInit(fixture);
viewport.setScrollOffset(testComponent.itemSize * 2);
fixture.detectChanges();
flush();

triggerScroll(viewport);
fixture.detectChanges();
flush();

expect(viewport.elementRef.nativeElement.scrollTop).toBe(testComponent.itemSize * 2);
expect(viewport.getRenderedRange()).toEqual({start: 2, end: 6});
Expand All @@ -163,6 +175,7 @@ describe('CdkVirtualScrollViewport', () => {
for (let offset = 0; offset <= maxOffset; offset += 10) {
triggerScroll(viewport, offset);
fixture.detectChanges();
flush();

const expectedRange = {
start: Math.floor(offset / testComponent.itemSize),
Expand All @@ -188,6 +201,7 @@ describe('CdkVirtualScrollViewport', () => {
for (let offset = maxOffset; offset >= 0; offset -= 10) {
triggerScroll(viewport, offset);
fixture.detectChanges();
flush();

const expectedRange = {
start: Math.floor(offset / testComponent.itemSize),
Expand Down Expand Up @@ -220,6 +234,7 @@ describe('CdkVirtualScrollViewport', () => {
finishInit(fixture);
triggerScroll(viewport, testComponent.itemSize * 2);
fixture.detectChanges();
flush();

expect(viewport.getRenderedRange()).toEqual({start: 1, end: 7},
'should render 6 50px items to fill 200px space, plus one buffer element at the' +
Expand All @@ -231,6 +246,7 @@ describe('CdkVirtualScrollViewport', () => {
finishInit(fixture);
triggerScroll(viewport, testComponent.itemSize * 6);
fixture.detectChanges();
flush();

expect(viewport.getRenderedRange()).toEqual({start: 5, end: 10},
'should render the last 5 50px items to fill 200px space, plus one buffer element at' +
Expand All @@ -241,12 +257,14 @@ describe('CdkVirtualScrollViewport', () => {
finishInit(fixture);
triggerScroll(viewport, testComponent.itemSize * 2);
fixture.detectChanges();
flush();

expect(viewport.getRenderedRange())
.toEqual({start: 2, end: 6}, 'should render 4 50px items to fill 200px space');

testComponent.itemSize *= 2;
fixture.detectChanges();
flush();

expect(viewport.getRenderedRange())
.toEqual({start: 1, end: 3}, 'should render 2 100px items to fill 200px space');
Expand All @@ -256,12 +274,14 @@ describe('CdkVirtualScrollViewport', () => {
finishInit(fixture);
triggerScroll(viewport, testComponent.itemSize * 2);
fixture.detectChanges();
flush();

expect(viewport.getRenderedRange())
.toEqual({start: 2, end: 6}, 'should render 4 50px items to fill 200px space');

testComponent.bufferSize = 1;
fixture.detectChanges();
flush();

expect(viewport.getRenderedRange())
.toEqual({start: 1, end: 7}, 'should expand to 1 buffer element on each side');
Expand All @@ -271,14 +291,18 @@ describe('CdkVirtualScrollViewport', () => {
finishInit(fixture);
triggerScroll(viewport, testComponent.itemSize * 6);
fixture.detectChanges();
flush();

expect(viewport.getOffsetToRenderedContentStart())
.toBe(testComponent.itemSize * 6, 'should be scrolled to bottom of 10 item list');

testComponent.items = Array(5).fill(0);
fixture.detectChanges();
flush();

triggerScroll(viewport);
fixture.detectChanges();
flush();

expect(viewport.getOffsetToRenderedContentStart())
.toBe(testComponent.itemSize, 'should be scrolled to bottom of 5 item list');
Expand All @@ -293,6 +317,7 @@ describe('CdkVirtualScrollViewport', () => {
for (let offset = 0; offset <= maxOffset; offset += 10) {
triggerScroll(viewport, offset);
fixture.detectChanges();
flush();

const expectedRange = {
start: Math.floor(offset / testComponent.itemSize),
Expand All @@ -319,6 +344,7 @@ describe('CdkVirtualScrollViewport', () => {
for (let offset = maxOffset; offset >= 0; offset -= 10) {
triggerScroll(viewport, offset);
fixture.detectChanges();
flush();

const expectedRange = {
start: Math.floor(offset / testComponent.itemSize),
Expand Down Expand Up @@ -346,6 +372,7 @@ describe('CdkVirtualScrollViewport', () => {

data.next([1, 2, 3]);
fixture.detectChanges();
flush();

expect(viewport.getRenderedRange())
.toEqual({start: 0, end: 3}, 'newly emitted items should be rendered');
Expand All @@ -355,7 +382,6 @@ describe('CdkVirtualScrollViewport', () => {
const data = new Subject<number[]>();
testComponent.items = new ArrayDataSource(data) as any;
finishInit(fixture);
flush();

expect(viewport.getRenderedRange())
.toEqual({start: 0, end: 0}, 'no items should be rendered');
Expand All @@ -375,11 +401,13 @@ describe('CdkVirtualScrollViewport', () => {

testComponent.items = [0];
fixture.detectChanges();
flush();

expect(testComponent.virtualForViewContainer.detach).not.toHaveBeenCalled();

testComponent.items = [1];
fixture.detectChanges();
flush();

expect(testComponent.virtualForViewContainer.detach).toHaveBeenCalled();
}));
Expand All @@ -392,11 +420,13 @@ describe('CdkVirtualScrollViewport', () => {

testComponent.items = [0];
fixture.detectChanges();
flush();

expect(testComponent.virtualForViewContainer.detach).not.toHaveBeenCalled();

testComponent.items = [1];
fixture.detectChanges();
flush();

expect(testComponent.virtualForViewContainer.detach).not.toHaveBeenCalled();
}));
Expand All @@ -413,6 +443,7 @@ describe('CdkVirtualScrollViewport', () => {
spy.calls.reset();
triggerScroll(viewport, 10);
fixture.detectChanges();
flush();

// As we first start to scroll we need to create one more item. This is because the first item
// is still partially on screen and therefore can't be removed yet. At the same time a new
Expand All @@ -425,6 +456,7 @@ describe('CdkVirtualScrollViewport', () => {
for (let offset = 10; offset <= maxOffset; offset += 10) {
triggerScroll(viewport, offset);
fixture.detectChanges();
flush();
}

// As we scroll through the rest of the items, no new views should be created, our existing 5
Expand All @@ -445,6 +477,7 @@ describe('CdkVirtualScrollViewport', () => {
spy.calls.reset();
triggerScroll(viewport, 10);
fixture.detectChanges();
flush();

// As we first start to scroll we need to create one more item. This is because the first item
// is still partially on screen and therefore can't be removed yet. At the same time a new
Expand All @@ -457,6 +490,7 @@ describe('CdkVirtualScrollViewport', () => {
for (let offset = 10; offset <= maxOffset; offset += 10) {
triggerScroll(viewport, offset);
fixture.detectChanges();
flush();
}

// Since our template cache size is 0, as we scroll through the rest of the items, we need to
Expand Down Expand Up @@ -514,6 +548,7 @@ function finishInit(fixture: ComponentFixture<any>) {

// On the second cycle we render the items.
fixture.detectChanges();
flush();
}

/** Trigger a scroll event on the viewport (optionally setting a new scroll offset). */
Expand Down
Loading