From c0208c59806116eb8c9af078a832db0670a4b062 Mon Sep 17 00:00:00 2001 From: Miles Malerba Date: Fri, 9 Mar 2018 14:07:39 -0800 Subject: [PATCH 1/4] virtual-scroll: add incremental scroll logic in `AutosizeVirtualScrollStrategy`. This still has a couple issues that need to be ironed out and it doesn't have the code for correcting the error between the predicted and actual scroll position. (See various TODOs for additional things that need work). --- .../scrolling/auto-size-virtual-scroll.ts | 146 +++++++++++++- .../scrolling/virtual-for-of.ts | 66 +++---- .../scrolling/virtual-scroll-viewport.ts | 178 ++++++++++++------ src/demo-app/demo-app/demo-module.ts | 5 +- 4 files changed, 292 insertions(+), 103 deletions(-) diff --git a/src/cdk-experimental/scrolling/auto-size-virtual-scroll.ts b/src/cdk-experimental/scrolling/auto-size-virtual-scroll.ts index b1f8694e710a..c06fa4df7edd 100644 --- a/src/cdk-experimental/scrolling/auto-size-virtual-scroll.ts +++ b/src/cdk-experimental/scrolling/auto-size-virtual-scroll.ts @@ -23,8 +23,12 @@ export class ItemSizeAverager { /** The current average item size. */ private _averageItemSize: number; + /** The default size to use for items when no data is available. */ + private _defaultItemSize: number; + /** @param defaultItemSize The default size to use for items when no data is available. */ constructor(defaultItemSize = 50) { + this._defaultItemSize = defaultItemSize; this._averageItemSize = defaultItemSize; } @@ -49,6 +53,12 @@ export class ItemSizeAverager { } } } + + /** Resets the averager. */ + reset() { + this._averageItemSize = this._defaultItemSize; + this._totalWeight = 0; + } } @@ -65,6 +75,15 @@ export class AutoSizeVirtualScrollStrategy implements VirtualScrollStrategy { /** The estimator used to estimate the size of unseen items. */ private _averager: ItemSizeAverager; + + /** The last measured scroll offset of the viewport. */ + private _lastScrollOffset: number; + + /** The last measured size of the rendered content in the viewport. */ + private _lastRenderedContentSize: number; + + /** The last measured size of the rendered content in the viewport. */ + private _lastRenderedContentOffset: number; /** * @param minBufferPx The minimum amount of buffer rendered beyond the viewport (in pixels). @@ -85,8 +104,9 @@ export class AutoSizeVirtualScrollStrategy implements VirtualScrollStrategy { * @param viewport The viewport to attach this strategy to. */ attach(viewport: CdkVirtualScrollViewport) { + this._averager.reset(); this._viewport = viewport; - this._renderContentForOffset(this._viewport.measureScrollOffset()); + this._setScrollOffset(); } /** Detaches this scroll strategy from the currently attached viewport. */ @@ -97,14 +117,15 @@ export class AutoSizeVirtualScrollStrategy implements VirtualScrollStrategy { /** Implemented as part of VirtualScrollStrategy. */ onContentScrolled() { if (this._viewport) { - this._renderContentForOffset(this._viewport.measureScrollOffset()); + this._updateRenderedContentAfterScroll(); } } /** Implemented as part of VirtualScrollStrategy. */ onDataLengthChanged() { if (this._viewport) { - this._renderContentForOffset(this._viewport.measureScrollOffset()); + // TODO(mmalebra): Do something smarter here. + this._setScrollOffset(); } } @@ -126,23 +147,130 @@ export class AutoSizeVirtualScrollStrategy implements VirtualScrollStrategy { this._addBufferPx = addBufferPx; } + /** Update the rendered content after the user scrolls. */ + private _updateRenderedContentAfterScroll() { + const viewport = this._viewport!; + + // The current scroll offset. + const scrollOffset = viewport.measureScrollOffset(); + // The delta between the current scroll offset and the previously recorded scroll offset. + const scrollDelta = scrollOffset - this._lastScrollOffset; + // The magnitude of the scroll delta. + const scrollMagnitude = Math.abs(scrollDelta); + + // TODO(mmalerba): Record error between actual scroll offset and predicted scroll offset given + // the index of the first rendered element. Fudge the scroll delta to slowly eliminate the error + // as the user scrolls. + + // The current amount of buffer past the start of the viewport. + const startBuffer = this._lastScrollOffset - this._lastRenderedContentOffset; + // The current amount of buffer past the end of the viewport. + const endBuffer = (this._lastRenderedContentOffset + this._lastRenderedContentSize) - + (this._lastScrollOffset + viewport.getViewportSize()); + // The amount of unfilled space that should be filled on the side the user is scrolling toward + // in order to safely absorb the scroll delta. + const underscan = scrollMagnitude + this._minBufferPx - + (scrollDelta < 0 ? startBuffer : endBuffer); + + // Check if there's unfilled space that we need to render new elements to fill. + if (underscan > 0) { + // Check if the scroll magnitude was larger than the viewport size. In this case the user + // won't notice a discontinuity if we just jump to the new estimated position in the list. + // However, if the scroll magnitude is smaller than the viewport the user might notice some + // jitteriness if we just jump to the estimated position. Instead we make sure to scroll by + // the same number of pixels as the scroll magnitude. + if (scrollMagnitude >= viewport.getViewportSize()) { + this._setScrollOffset(); + + } else { + // The number of new items to render on the side the user is scrolling towards. Rather than + // just filling the underscan space, we actually fill enough to have a buffer size of + // `addBufferPx`. This gives us a little wiggle room in case our item size estimate is off. + const addItems = Math.max(0, Math.ceil((underscan - this._minBufferPx + this._addBufferPx) / + this._averager.getAverageItemSize())); + // The amount of filled space beyond what is necessary on the side the user is scrolling + // away from. + const overscan = (scrollDelta < 0 ? endBuffer : startBuffer) - this._minBufferPx + + scrollMagnitude; + // The number of currently rendered items to remove on the side the user is scrolling away + // from. + const removeItems = Math.max(0, Math.floor(overscan / this._averager.getAverageItemSize())); + + // The currently rendered range. + const renderedRange = viewport.getRenderedRange(); + // The new range we will tell the viewport to render. We first expand it to include the new + // items we want rendered, we then contract the opposite side to remove items we no longer + // want rendered. + const range = this._expandRange( + renderedRange, scrollDelta < 0 ? addItems : 0, scrollDelta > 0 ? addItems : 0); + if (scrollDelta < 0) { + range.end = Math.max(range.start + 1, range.end - removeItems); + } else { + range.start = Math.min(range.end - 1, range.start + removeItems); + } + + // The new offset we want to set on the rendered content. To determine this we measure the + // number of pixels we removed and then adjust the offset to the start of the rendered + // content or to the end of the rendered content accordingly (whichever one doesn't require + // that the newly added items to be rendered to calculate.) + let contentOffset: {offset: number, to: 'to-start' | 'to-end'}; + if (scrollDelta < 0) { + const removedSize = viewport.measureRangeSize({ + start: range.end, + end: renderedRange.end, + }); + contentOffset = { + offset: this._lastRenderedContentOffset + this._lastRenderedContentSize - removedSize, + to: 'to-end', + }; + } else { + const removedSize = viewport.measureRangeSize({ + start: renderedRange.start, + end: range.start, + }); + contentOffset = { + offset: this._lastRenderedContentOffset + removedSize, + to: 'to-start', + }; + } + + // Set the range and offset we calculated above. + viewport.setRenderedRange(range); + viewport.setRenderedContentOffset(contentOffset.offset, contentOffset.to); + } + } + + // Save the scroll offset to be compared to the new value on the next scroll event. + this._lastScrollOffset = scrollOffset; + } + /** * Checks the size of the currently rendered content and uses it to update the estimated item size * and estimated total content size. */ private _checkRenderedContentSize() { const viewport = this._viewport!; - const renderedContentSize = viewport.measureRenderedContentSize(); - this._averager.addSample(viewport.getRenderedRange(), renderedContentSize); - this._updateTotalContentSize(renderedContentSize); + this._lastRenderedContentOffset = viewport.measureRenderedContentOffset(); + this._lastRenderedContentSize = viewport.measureRenderedContentSize(); + this._averager.addSample(viewport.getRenderedRange(), this._lastRenderedContentSize); + this._updateTotalContentSize(this._lastRenderedContentSize); } /** - * Render the content that we estimate should be shown for the given scroll offset. - * Note: must not be called if `this._viewport` is null + * Sets the scroll offset and renders the content we estimate should be shown at that point. + * @param scrollOffset The offset to jump to. If not specified the scroll offset will not be + * changed, but the rendered content will be recalculated based on our estimate of what should + * be shown at the current scroll offset. */ - private _renderContentForOffset(scrollOffset: number) { + private _setScrollOffset(scrollOffset?: number) { const viewport = this._viewport!; + if (scrollOffset == null) { + scrollOffset = viewport.measureScrollOffset(); + } else { + viewport.setScrollOffset(scrollOffset); + } + this._lastScrollOffset = scrollOffset; + const itemSize = this._averager.getAverageItemSize(); const firstVisibleIndex = Math.min(viewport.getDataLength() - 1, Math.floor(scrollOffset / itemSize)); diff --git a/src/cdk-experimental/scrolling/virtual-for-of.ts b/src/cdk-experimental/scrolling/virtual-for-of.ts index 20ceb0b87c0a..1ccd8589a00b 100644 --- a/src/cdk-experimental/scrolling/virtual-for-of.ts +++ b/src/cdk-experimental/scrolling/virtual-for-of.ts @@ -151,47 +151,39 @@ export class CdkVirtualForOf implements CollectionViewer, DoCheck, OnDestroy } /** - * Get the client rect for the given index. - * @param index The index of the data element whose client rect we want to measure. - * @return The combined client rect for all DOM elements rendered as part of the given index. - * Or null if no DOM elements are rendered for the given index. - * @throws If the given index is not in the rendered range. + * Measures the combined size (width for horizontal orientation, height for vertical) of all items + * in the specified range. Throws an error if the range includes items that are not currently + * rendered. */ - measureClientRect(index: number): ClientRect | null { - if (index < this._renderedRange.start || index >= this._renderedRange.end) { - throw Error(`Error: attempted to measure an element that isn't rendered.`); + measureRangeSize(range: ListRange, orientation: 'horizontal' | 'vertical'): number { + if (range.start >= range.end) { + return 0; + } + if (range.start < this._renderedRange.start || range.end > this._renderedRange.end) { + throw Error(`Error: attempted to measure an item that isn't rendered.`); } - const renderedIndex = index - this._renderedRange.start; - let view = this._viewContainerRef.get(renderedIndex) as - EmbeddedViewRef> | null; - if (view && view.rootNodes.length) { - // There may be multiple root DOM elements for a single data element, so we merge their rects. - // These variables keep track of the minimum top and left as well as maximum bottom and right - // that we have encoutnered on any rectangle, so that we can merge the results into the - // smallest possible rect that contains all of the root rects. - let minTop = Infinity; - let minLeft = Infinity; - let maxBottom = -Infinity; - let maxRight = -Infinity; - - for (let i = view.rootNodes.length - 1; i >= 0 ; i--) { - let rect = (view.rootNodes[i] as Element).getBoundingClientRect(); - minTop = Math.min(minTop, rect.top); - minLeft = Math.min(minLeft, rect.left); - maxBottom = Math.max(maxBottom, rect.bottom); - maxRight = Math.max(maxRight, rect.right); - } - return { - top: minTop, - left: minLeft, - bottom: maxBottom, - right: maxRight, - height: maxBottom - minTop, - width: maxRight - minLeft - }; + // Helper to extract size from a ClientRect. + const getSize = rect => orientation == 'horizontal' ? rect.width : rect.height; + // The index into the list of rendered views for the first item in the range. + const renderedStartIndex = range.start - this._renderedRange.start; + // The length of the range we're measuring. + const rangeLen = range.end - range.start; + + // Loop over all root nodes for all items in the range and sum up their size. + // TODO(mmalerba): Make this work with non-element nodes. + let totalSize = 0; + let i = rangeLen; + while(i--) { + const view = this._viewContainerRef.get(i + renderedStartIndex) as + EmbeddedViewRef> | null; + let j = view ? view.rootNodes.length : 0; + while (j--) { + totalSize += getSize((view!.rootNodes[j] as Element).getBoundingClientRect()); + } } - return null; + + return totalSize; } ngDoCheck() { diff --git a/src/cdk-experimental/scrolling/virtual-scroll-viewport.ts b/src/cdk-experimental/scrolling/virtual-scroll-viewport.ts index fdabd276ec88..85e93ce2b06c 100644 --- a/src/cdk-experimental/scrolling/virtual-scroll-viewport.ts +++ b/src/cdk-experimental/scrolling/virtual-scroll-viewport.ts @@ -11,6 +11,7 @@ import { ChangeDetectionStrategy, ChangeDetectorRef, Component, + DoCheck, ElementRef, Inject, Input, @@ -20,6 +21,7 @@ import { ViewChild, ViewEncapsulation, } from '@angular/core'; +import {DomSanitizer, SafeStyle} from '@angular/platform-browser'; import {Observable} from 'rxjs/Observable'; import {fromEvent} from 'rxjs/observable/fromEvent'; import {sampleTime} from 'rxjs/operators/sampleTime'; @@ -50,7 +52,7 @@ function rangesEqual(r1: ListRange, r2: ListRange): boolean { changeDetection: ChangeDetectionStrategy.OnPush, preserveWhitespaces: false, }) -export class CdkVirtualScrollViewport implements OnInit, OnDestroy { +export class CdkVirtualScrollViewport implements DoCheck, OnInit, OnDestroy { /** Emits when the viewport is detached from a CdkVirtualForOf. */ private _detachedSubject = new Subject(); @@ -72,7 +74,7 @@ export class CdkVirtualScrollViewport implements OnInit, OnDestroy { _totalContentSize = 0; /** The transform used to offset the rendered content wrapper element. */ - _renderedContentTransform: string; + _renderedContentTransform: SafeStyle; /** The currently rendered range of indices. */ private _renderedRange: ListRange = {start: 0, end: 0}; @@ -83,13 +85,75 @@ export class CdkVirtualScrollViewport implements OnInit, OnDestroy { /** The size of the viewport (in pixels). */ private _viewportSize = 0; - /** Whether this viewport is attached to a CdkVirtualForOf. */ - private _isAttached = false; + /** The pending scroll offset to be applied during the next change detection cycle. */ + private _pendingScrollOffset: number | null; + + /** the currently attached CdkVirtualForOf. */ + private _forOf: CdkVirtualForOf | null; constructor(public elementRef: ElementRef, private _changeDetectorRef: ChangeDetectorRef, - private _ngZone: NgZone, + private _ngZone: NgZone, private _sanitizer: DomSanitizer, @Inject(VIRTUAL_SCROLL_STRATEGY) private _scrollStrategy: VirtualScrollStrategy) {} + ngOnInit() { + Promise.resolve().then(() => { + this._viewportSize = this.orientation === 'horizontal' ? + this.elementRef.nativeElement.clientWidth : this.elementRef.nativeElement.clientHeight; + this._scrollStrategy.attach(this); + + this._ngZone.runOutsideAngular(() => { + fromEvent(this.elementRef.nativeElement, 'scroll') + // Sample the scroll stream at every animation frame. This way if there are multiple + // scroll events in the same frame we only need to recheck our layout once. + .pipe(sampleTime(0, animationFrame)) + .subscribe(() => this._scrollStrategy.onContentScrolled()); + }); + }); + } + + ngDoCheck() { + if (this._pendingScrollOffset != null) { + if (this.orientation === 'horizontal') { + this.elementRef.nativeElement.offsetLeft = this._pendingScrollOffset; + } else { + this.elementRef.nativeElement.offsetTop = this._pendingScrollOffset; + } + } + } + + ngOnDestroy() { + this.detach(); + this._scrollStrategy.detach(); + + // Complete all subjects + this._detachedSubject.complete(); + this._renderedRangeSubject.complete(); + } + + /** Attaches a `CdkVirtualForOf` to this viewport. */ + attach(forOf: CdkVirtualForOf) { + if (this._forOf) { + throw Error('CdkVirtualScrollViewport is already attached.'); + } + this._forOf = forOf; + + // Subscribe to the data stream of the CdkVirtualForOf to keep track of when the data length + // changes. + this._forOf.dataStream.pipe(takeUntil(this._detachedSubject)).subscribe(data => { + const len = data.length; + if (len != this._dataLength) { + this._dataLength = len; + this._scrollStrategy.onDataLengthChanged(); + } + }); + } + + /** Detaches the current `CdkVirtualForOf`. */ + detach() { + this._forOf = null; + this._detachedSubject.next(); + } + /** Gets the length of the data bound to this viewport (in number of items). */ getDataLength(): number { return this._dataLength; @@ -100,6 +164,11 @@ export class CdkVirtualScrollViewport implements OnInit, OnDestroy { return this._viewportSize; } + // TODO(mmalerba): This is technically out of sync with what's really rendered until a render + // cycle happens. I'm being careful to only call it after the render cycle is complete and before + // setting it to something else, but its error prone and should probably be split into + // `pendingRange` and `renderedRange`, the latter reflecting whats actually in the DOM. + /** Get the current rendered range of items. */ getRenderedRange(): ListRange { return this._renderedRange; @@ -129,83 +198,84 @@ export class CdkVirtualScrollViewport implements OnInit, OnDestroy { this._renderedRangeSubject.next(this._renderedRange = range); this._changeDetectorRef.markForCheck(); this._ngZone.runOutsideAngular(() => this._ngZone.onStable.pipe(take(1)).subscribe(() => { - this._scrollStrategy.onContentRendered(); + // Queue this up in a `Promise.resolve()` so that if the user makes a series of calls + // like: + // + // viewport.setRenderedRange(...); + // viewport.setTotalContentSize(...); + // viewport.setRenderedContentOffset(...); + // + // The call to `onContentRendered` will happen after all of the updates have been applied. + Promise.resolve().then(() => this._scrollStrategy.onContentRendered()); })); }); } } /** Sets the offset of the rendered portion of the data from the start (in pixels). */ - setRenderedContentOffset(offset: number) { - const transform = - this.orientation === 'horizontal' ? `translateX(${offset}px)` : `translateY(${offset}px)`; + setRenderedContentOffset(offset: number, to: 'to-start' | 'to-end' = 'to-start') { + const axis = this.orientation === 'horizontal' ? 'X' : 'Y'; + let transform = `translate${axis}(${Number(offset)}px)`; + if (to === 'to-end') { + // TODO(mmalerba): The viewport should rewrite this as a `to-start` offset on the next render + // cycle. Otherwise elements will appear to expand in the wrong direction (e.g. + // `mat-expansion-panel` would expand upward). + transform += ` translate${axis}(-100%)`; + } if (this._renderedContentTransform != transform) { // Re-enter the Angular zone so we can mark for change detection. this._ngZone.run(() => { - this._renderedContentTransform = transform; + // We know this value is safe because we parse `offset` with `Number()` before passing it + // into the string. + this._renderedContentTransform = this._sanitizer.bypassSecurityTrustStyle(transform); this._changeDetectorRef.markForCheck(); }); } } - /** Attaches a `CdkVirtualForOf` to this viewport. */ - attach(forOf: CdkVirtualForOf) { - if (this._isAttached) { - throw Error('CdkVirtualScrollViewport is already attached.'); - } - - this._isAttached = true; - // Subscribe to the data stream of the CdkVirtualForOf to keep track of when the data length - // changes. - forOf.dataStream.pipe(takeUntil(this._detachedSubject)).subscribe(data => { - const len = data.length; - if (len != this._dataLength) { - this._dataLength = len; - this._scrollStrategy.onDataLengthChanged(); - } + /** Sets the scroll offset on the viewport. */ + setScrollOffset(offset: number) { + this._ngZone.run(() => { + this._pendingScrollOffset = offset; + this._changeDetectorRef.markForCheck(); }); } - /** Detaches the current `CdkVirtualForOf`. */ - detach() { - this._isAttached = false; - this._detachedSubject.next(); - } - /** Gets the current scroll offset of the viewport (in pixels). */ - measureScrollOffset() { + measureScrollOffset(): number { return this.orientation === 'horizontal' ? this.elementRef.nativeElement.scrollLeft : this.elementRef.nativeElement.scrollTop; } /** Measure the combined size of all of the rendered items. */ - measureRenderedContentSize() { + measureRenderedContentSize(): number { const contentEl = this._contentWrapper.nativeElement; return this.orientation === 'horizontal' ? contentEl.offsetWidth : contentEl.offsetHeight; } - ngOnInit() { - Promise.resolve().then(() => { - this._viewportSize = this.orientation === 'horizontal' ? - this.elementRef.nativeElement.clientWidth : this.elementRef.nativeElement.clientHeight; - this._scrollStrategy.attach(this); - - this._ngZone.runOutsideAngular(() => { - fromEvent(this.elementRef.nativeElement, 'scroll') - // Sample the scroll stream at every animation frame. This way if there are multiple - // scroll events in the same frame we only need to recheck our layout once - .pipe(sampleTime(0, animationFrame)) - .subscribe(() => this._scrollStrategy.onContentScrolled()); - }); - }); + // TODO(mmalerba): Try to do this in a way that's less bad for performance. (The bad part here is + // that we have to measure the viewport which is not absolutely positioned.) + /** Measure the offset from the start of the viewport to the start of the rendered content. */ + measureRenderedContentOffset(): number { + const viewportEl = this.elementRef.nativeElement; + const contentEl = this._contentWrapper.nativeElement; + if (this.orientation === 'horizontal') { + return contentEl.getBoundingClientRect().left + viewportEl.scrollLeft - + viewportEl.getBoundingClientRect().left - viewportEl.clientLeft; + } else { + return contentEl.getBoundingClientRect().top + viewportEl.scrollTop - + viewportEl.getBoundingClientRect().top - viewportEl.clientTop; + } } - ngOnDestroy() { - this.detach(); - this._scrollStrategy.detach(); - - // Complete all subjects - this._detachedSubject.complete(); - this._renderedRangeSubject.complete(); + /** + * Measure the total combined size of the given range. Throws if the range includes items that are + * not rendered. + */ + measureRangeSize(range: ListRange): number { + if (!this._forOf) { + return 0; + } + return this._forOf.measureRangeSize(range, this.orientation); } } diff --git a/src/demo-app/demo-app/demo-module.ts b/src/demo-app/demo-app/demo-module.ts index fb1fc77a7662..21f324744777 100644 --- a/src/demo-app/demo-app/demo-module.ts +++ b/src/demo-app/demo-app/demo-module.ts @@ -21,6 +21,7 @@ import {ButtonDemo} from '../button/button-demo'; import {CardDemo} from '../card/card-demo'; import {CheckboxDemo, MatCheckboxDemoNestedChecklist} from '../checkbox/checkbox-demo'; import {ChipsDemo} from '../chips/chips-demo'; +import {ConnectedOverlayDemo, DemoOverlay} from '../connected-overlay/connected-overlay-demo'; import {DatepickerDemo} from '../datepicker/datepicker-demo'; import {DemoMaterialModule} from '../demo-material-module'; import {ContentElementDialog, DialogDemo, IFrameDialog, JazzDialog} from '../dialog/dialog-demo'; @@ -63,13 +64,11 @@ import { } from '../tabs/tabs-demo'; import {ToolbarDemo} from '../toolbar/toolbar-demo'; import {TooltipDemo} from '../tooltip/tooltip-demo'; +import {TreeDemoModule} from '../tree/tree-demo-module'; import {TypographyDemo} from '../typography/typography-demo'; import {VirtualScrollDemo} from '../virtual-scroll/virtual-scroll-demo'; import {DemoApp, Home} from './demo-app'; import {DEMO_APP_ROUTES} from './routes'; -import {TableDemoModule} from '../table/table-demo-module'; -import {TreeDemoModule} from '../tree/tree-demo-module'; -import {ConnectedOverlayDemo, DemoOverlay} from '../connected-overlay/connected-overlay-demo'; @NgModule({ imports: [ From fbc0ed3aaa6be466124be141d4fbd7b0ffb13964 Mon Sep 17 00:00:00 2001 From: Miles Malerba Date: Tue, 20 Mar 2018 15:46:15 -0700 Subject: [PATCH 2/4] fix lint --- src/cdk-experimental/scrolling/auto-size-virtual-scroll.ts | 2 +- src/cdk-experimental/scrolling/virtual-for-of.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cdk-experimental/scrolling/auto-size-virtual-scroll.ts b/src/cdk-experimental/scrolling/auto-size-virtual-scroll.ts index c06fa4df7edd..941a49adb781 100644 --- a/src/cdk-experimental/scrolling/auto-size-virtual-scroll.ts +++ b/src/cdk-experimental/scrolling/auto-size-virtual-scroll.ts @@ -75,7 +75,7 @@ export class AutoSizeVirtualScrollStrategy implements VirtualScrollStrategy { /** The estimator used to estimate the size of unseen items. */ private _averager: ItemSizeAverager; - + /** The last measured scroll offset of the viewport. */ private _lastScrollOffset: number; diff --git a/src/cdk-experimental/scrolling/virtual-for-of.ts b/src/cdk-experimental/scrolling/virtual-for-of.ts index 1ccd8589a00b..3be069f92955 100644 --- a/src/cdk-experimental/scrolling/virtual-for-of.ts +++ b/src/cdk-experimental/scrolling/virtual-for-of.ts @@ -174,7 +174,7 @@ export class CdkVirtualForOf implements CollectionViewer, DoCheck, OnDestroy // TODO(mmalerba): Make this work with non-element nodes. let totalSize = 0; let i = rangeLen; - while(i--) { + while (i--) { const view = this._viewContainerRef.get(i + renderedStartIndex) as EmbeddedViewRef> | null; let j = view ? view.rootNodes.length : 0; From f98b2deabf909594d726fbe9995e6c3faa10816f Mon Sep 17 00:00:00 2001 From: Miles Malerba Date: Thu, 22 Mar 2018 12:08:15 -0700 Subject: [PATCH 3/4] address comments --- .../scrolling/auto-size-virtual-scroll.ts | 21 ++++++++----------- .../scrolling/virtual-for-of.ts | 10 ++++++--- .../scrolling/virtual-scroll-viewport.ts | 11 +++++----- 3 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/cdk-experimental/scrolling/auto-size-virtual-scroll.ts b/src/cdk-experimental/scrolling/auto-size-virtual-scroll.ts index 941a49adb781..47b49604780b 100644 --- a/src/cdk-experimental/scrolling/auto-size-virtual-scroll.ts +++ b/src/cdk-experimental/scrolling/auto-size-virtual-scroll.ts @@ -166,7 +166,7 @@ export class AutoSizeVirtualScrollStrategy implements VirtualScrollStrategy { const startBuffer = this._lastScrollOffset - this._lastRenderedContentOffset; // The current amount of buffer past the end of the viewport. const endBuffer = (this._lastRenderedContentOffset + this._lastRenderedContentSize) - - (this._lastScrollOffset + viewport.getViewportSize()); + (this._lastScrollOffset + viewport.getViewportSize()); // The amount of unfilled space that should be filled on the side the user is scrolling toward // in order to safely absorb the scroll delta. const underscan = scrollMagnitude + this._minBufferPx - @@ -181,7 +181,6 @@ export class AutoSizeVirtualScrollStrategy implements VirtualScrollStrategy { // the same number of pixels as the scroll magnitude. if (scrollMagnitude >= viewport.getViewportSize()) { this._setScrollOffset(); - } else { // The number of new items to render on the side the user is scrolling towards. Rather than // just filling the underscan space, we actually fill enough to have a buffer size of @@ -213,30 +212,28 @@ export class AutoSizeVirtualScrollStrategy implements VirtualScrollStrategy { // number of pixels we removed and then adjust the offset to the start of the rendered // content or to the end of the rendered content accordingly (whichever one doesn't require // that the newly added items to be rendered to calculate.) - let contentOffset: {offset: number, to: 'to-start' | 'to-end'}; + let contentOffset: number; + let contentOffsetTo: 'to-start' | 'to-end'; if (scrollDelta < 0) { const removedSize = viewport.measureRangeSize({ start: range.end, end: renderedRange.end, }); - contentOffset = { - offset: this._lastRenderedContentOffset + this._lastRenderedContentSize - removedSize, - to: 'to-end', - }; + contentOffset = + this._lastRenderedContentOffset + this._lastRenderedContentSize - removedSize; + contentOffsetTo = 'to-end'; } else { const removedSize = viewport.measureRangeSize({ start: renderedRange.start, end: range.start, }); - contentOffset = { - offset: this._lastRenderedContentOffset + removedSize, - to: 'to-start', - }; + contentOffset = this._lastRenderedContentOffset + removedSize; + contentOffsetTo = 'to-start'; } // Set the range and offset we calculated above. viewport.setRenderedRange(range); - viewport.setRenderedContentOffset(contentOffset.offset, contentOffset.to); + viewport.setRenderedContentOffset(contentOffset, contentOffsetTo); } } diff --git a/src/cdk-experimental/scrolling/virtual-for-of.ts b/src/cdk-experimental/scrolling/virtual-for-of.ts index 3be069f92955..1820ac5f9cce 100644 --- a/src/cdk-experimental/scrolling/virtual-for-of.ts +++ b/src/cdk-experimental/scrolling/virtual-for-of.ts @@ -45,6 +45,12 @@ export type CdkVirtualForOfContext = { }; +/** Helper to extract size from a ClientRect. **/ +function getSize(orientation: 'horizontal' | 'vertical', rect: ClientRect): number { + return orientation == 'horizontal' ? rect.width : rect.height; +} + + /** * A directive similar to `ngForOf` to be used for rendering data inside a virtual scrolling * container. @@ -163,8 +169,6 @@ export class CdkVirtualForOf implements CollectionViewer, DoCheck, OnDestroy throw Error(`Error: attempted to measure an item that isn't rendered.`); } - // Helper to extract size from a ClientRect. - const getSize = rect => orientation == 'horizontal' ? rect.width : rect.height; // The index into the list of rendered views for the first item in the range. const renderedStartIndex = range.start - this._renderedRange.start; // The length of the range we're measuring. @@ -179,7 +183,7 @@ export class CdkVirtualForOf implements CollectionViewer, DoCheck, OnDestroy EmbeddedViewRef> | null; let j = view ? view.rootNodes.length : 0; while (j--) { - totalSize += getSize((view!.rootNodes[j] as Element).getBoundingClientRect()); + totalSize += getSize(orientation, (view!.rootNodes[j] as Element).getBoundingClientRect()); } } diff --git a/src/cdk-experimental/scrolling/virtual-scroll-viewport.ts b/src/cdk-experimental/scrolling/virtual-scroll-viewport.ts index 85e93ce2b06c..4141e041b259 100644 --- a/src/cdk-experimental/scrolling/virtual-scroll-viewport.ts +++ b/src/cdk-experimental/scrolling/virtual-scroll-viewport.ts @@ -96,13 +96,14 @@ export class CdkVirtualScrollViewport implements DoCheck, OnInit, OnDestroy { @Inject(VIRTUAL_SCROLL_STRATEGY) private _scrollStrategy: VirtualScrollStrategy) {} ngOnInit() { + const viewportEl = this.elementRef.nativeElement; Promise.resolve().then(() => { this._viewportSize = this.orientation === 'horizontal' ? - this.elementRef.nativeElement.clientWidth : this.elementRef.nativeElement.clientHeight; + viewportEl.clientWidth : viewportEl.clientHeight; this._scrollStrategy.attach(this); this._ngZone.runOutsideAngular(() => { - fromEvent(this.elementRef.nativeElement, 'scroll') + fromEvent(viewportEl, 'scroll') // Sample the scroll stream at every animation frame. This way if there are multiple // scroll events in the same frame we only need to recheck our layout once. .pipe(sampleTime(0, animationFrame)) @@ -114,9 +115,9 @@ export class CdkVirtualScrollViewport implements DoCheck, OnInit, OnDestroy { ngDoCheck() { if (this._pendingScrollOffset != null) { if (this.orientation === 'horizontal') { - this.elementRef.nativeElement.offsetLeft = this._pendingScrollOffset; + this.elementRef.nativeElement.scrollLeft = this._pendingScrollOffset; } else { - this.elementRef.nativeElement.offsetTop = this._pendingScrollOffset; + this.elementRef.nativeElement.scrollTop = this._pendingScrollOffset; } } } @@ -126,8 +127,8 @@ export class CdkVirtualScrollViewport implements DoCheck, OnInit, OnDestroy { this._scrollStrategy.detach(); // Complete all subjects - this._detachedSubject.complete(); this._renderedRangeSubject.complete(); + this._detachedSubject.complete(); } /** Attaches a `CdkVirtualForOf` to this viewport. */ From 7456686679b89a79ec95bc774f5d2fcb82fc4ece Mon Sep 17 00:00:00 2001 From: Miles Malerba Date: Thu, 5 Apr 2018 12:06:54 -0700 Subject: [PATCH 4/4] address comments --- .../scrolling/virtual-scroll-viewport.spec.ts | 46 +++++++++++++++++++ .../scrolling/virtual-scroll-viewport.ts | 4 ++ 2 files changed, 50 insertions(+) create mode 100644 src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts diff --git a/src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts b/src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts new file mode 100644 index 000000000000..a84330d870a4 --- /dev/null +++ b/src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts @@ -0,0 +1,46 @@ +import {Component, ViewChild} from '@angular/core'; +import {ComponentFixture, fakeAsync, flush, TestBed} from '@angular/core/testing'; +import {ScrollingModule} from './scrolling-module'; +import {CdkVirtualScrollViewport} from './virtual-scroll-viewport'; + +describe('Basic CdkVirtualScrollViewport', () => { + let fixture: ComponentFixture; + let viewport: CdkVirtualScrollViewport; + + beforeEach(() => { + TestBed.configureTestingModule({ + imports: [ScrollingModule], + declarations: [BasicViewport], + }).compileComponents(); + + fixture = TestBed.createComponent(BasicViewport); + viewport = fixture.componentInstance.viewport; + }); + + it('should sanitize transform inputs', fakeAsync(() => { + fixture.detectChanges(); + flush(); + + viewport.orientation = 'arbitrary string as orientation' as any; + viewport.setRenderedContentOffset( + 'arbitrary string as offset' as any, 'arbitrary string as to' as any); + fixture.detectChanges(); + flush(); + + expect((viewport._renderedContentTransform as any).changingThisBreaksApplicationSecurity) + .toEqual('translateY(NaNpx)'); + })); +}); + +@Component({ + template: ` + + {{item}} + + ` +}) +class BasicViewport { + @ViewChild(CdkVirtualScrollViewport) viewport; + + items = Array(10).fill(0); +} diff --git a/src/cdk-experimental/scrolling/virtual-scroll-viewport.ts b/src/cdk-experimental/scrolling/virtual-scroll-viewport.ts index 4141e041b259..20cc5c48db75 100644 --- a/src/cdk-experimental/scrolling/virtual-scroll-viewport.ts +++ b/src/cdk-experimental/scrolling/virtual-scroll-viewport.ts @@ -113,6 +113,8 @@ export class CdkVirtualScrollViewport implements DoCheck, OnInit, OnDestroy { } ngDoCheck() { + // In order to batch setting the scroll offset together with other DOM writes, we wait until a + // change detection cycle to actually apply it. if (this._pendingScrollOffset != null) { if (this.orientation === 'horizontal') { this.elementRef.nativeElement.scrollLeft = this._pendingScrollOffset; @@ -236,6 +238,8 @@ export class CdkVirtualScrollViewport implements DoCheck, OnInit, OnDestroy { /** Sets the scroll offset on the viewport. */ setScrollOffset(offset: number) { + // Rather than setting the offset immediately, we batch it up to be applied along with other DOM + // writes during the next change detection cycle. this._ngZone.run(() => { this._pendingScrollOffset = offset; this._changeDetectorRef.markForCheck();