Skip to content

wip: fix(virtual-scroll): make horizontal scrolling work in RTL #12512

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 8 commits into from
Aug 23, 2018
Merged

wip: fix(virtual-scroll): make horizontal scrolling work in RTL #12512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7da621d
fix(virtual-scroll): make horizontal scrolling work in RTL
mmalerba Aug 2, 2018
4c25e86
address comments
mmalerba Aug 3, 2018
d2ce70d
wip: scrollable
mmalerba Aug 8, 2018
df672ee
fix rebase
mmalerba Aug 13, 2018
cb132c8
refactor to use CdkScrollable
mmalerba Aug 14, 2018
d9fa498
fix lint and tests
mmalerba Aug 20, 2018
6aedbcf
fix safari
mmalerba Aug 21, 2018
c018e6f
address comments
mmalerba Aug 22, 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
20 changes: 7 additions & 13 deletions src/cdk-experimental/scrolling/auto-size-virtual-scroll.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ export class AutoSizeVirtualScrollStrategy implements VirtualScrollStrategy {
attach(viewport: CdkVirtualScrollViewport) {
this._averager.reset();
this._viewport = viewport;
this._setScrollOffset();
this._renderContentForCurrentOffset();
}

/** Detaches this scroll strategy from the currently attached viewport. */
Expand All @@ -143,7 +143,7 @@ export class AutoSizeVirtualScrollStrategy implements VirtualScrollStrategy {
/** @docs-private Implemented as part of VirtualScrollStrategy. */
onDataLengthChanged() {
if (this._viewport) {
this._setScrollOffset();
this._renderContentForCurrentOffset();
this._checkRenderedContentSize();
}
}
Expand Down Expand Up @@ -241,7 +241,7 @@ export class AutoSizeVirtualScrollStrategy implements VirtualScrollStrategy {
// 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();
this._renderContentForCurrentOffset();
} 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
Expand Down Expand Up @@ -346,18 +346,12 @@ export class AutoSizeVirtualScrollStrategy implements VirtualScrollStrategy {
}

/**
* 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.
* Recalculates the rendered content based on our estimate of what should be shown at the current
* scroll offset.
*/
private _setScrollOffset(scrollOffset?: number) {
private _renderContentForCurrentOffset() {
const viewport = this._viewport!;
if (scrollOffset == null) {
scrollOffset = viewport.measureScrollOffset();
} else {
viewport.setScrollOffset(scrollOffset);
}
const scrollOffset = viewport.measureScrollOffset();
this._lastScrollOffset = scrollOffset;
this._removalFailures = 0;

Expand Down
28 changes: 14 additions & 14 deletions src/cdk/scrolling/scrollable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,21 +48,21 @@ export class CdkScrollable implements OnInit, OnDestroy {
private _destroyed = new Subject();

private _elementScrolled: Observable<Event> = Observable.create(observer =>
this._ngZone.runOutsideAngular(() =>
fromEvent(this._elementRef.nativeElement, 'scroll').pipe(takeUntil(this._destroyed))
this.ngZone.runOutsideAngular(() =>
fromEvent(this.elementRef.nativeElement, 'scroll').pipe(takeUntil(this._destroyed))
.subscribe(observer)));

constructor(private _elementRef: ElementRef<HTMLElement>,
private _scroll: ScrollDispatcher,
private _ngZone: NgZone,
@Optional() private _dir?: Directionality) {}
constructor(protected elementRef: ElementRef<HTMLElement>,
protected scrollDispatcher: ScrollDispatcher,
protected ngZone: NgZone,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't all of the protected properties be underscored?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want people to extend their own components from this if they act as a viewport. Therefore the protected properties are kind of a part of the public API and should not be underscored

@Optional() protected dir?: Directionality) {}

ngOnInit() {
this._scroll.register(this);
this.scrollDispatcher.register(this);
}

ngOnDestroy() {
this._scroll.deregister(this);
this.scrollDispatcher.deregister(this);
this._destroyed.next();
this._destroyed.complete();
}
Expand All @@ -74,7 +74,7 @@ export class CdkScrollable implements OnInit, OnDestroy {

/** Gets the ElementRef for the viewport. */
getElementRef(): ElementRef<HTMLElement> {
return this._elementRef;
return this.elementRef;
}

/**
Expand All @@ -86,8 +86,8 @@ export class CdkScrollable implements OnInit, OnDestroy {
* @param options specified the offsets to scroll to.
*/
scrollTo(options: ExtendedScrollToOptions): void {
const el = this._elementRef.nativeElement;
const isRtl = this._dir && this._dir.value == 'rtl';
const el = this.elementRef.nativeElement;
const isRtl = this.dir && this.dir.value == 'rtl';

// Rewrite start & end offsets as right or left offsets.
options.left = options.left == null ? (isRtl ? options.end : options.start) : options.left;
Expand Down Expand Up @@ -119,7 +119,7 @@ export class CdkScrollable implements OnInit, OnDestroy {
}

private _applyScrollToOptions(options: ScrollToOptions): void {
const el = this._elementRef.nativeElement;
const el = this.elementRef.nativeElement;

if (supportsScrollBehavior()) {
el.scrollTo(options);
Expand All @@ -145,7 +145,7 @@ export class CdkScrollable implements OnInit, OnDestroy {
measureScrollOffset(from: 'top' | 'left' | 'right' | 'bottom' | 'start' | 'end'): number {
const LEFT = 'left';
const RIGHT = 'right';
const el = this._elementRef.nativeElement;
const el = this.elementRef.nativeElement;
if (from == 'top') {
return el.scrollTop;
}
Expand All @@ -154,7 +154,7 @@ export class CdkScrollable implements OnInit, OnDestroy {
}

// Rewrite start & end as left or right offsets.
const isRtl = this._dir && this._dir.value == 'rtl';
const isRtl = this.dir && this.dir.value == 'rtl';
if (from == 'start') {
from = isRtl ? RIGHT : LEFT;
} else if (from == 'end') {
Expand Down
2 changes: 1 addition & 1 deletion src/cdk/scrolling/scrolling-module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {CdkVirtualForOf} from './virtual-for-of';
import {CdkVirtualScrollViewport} from './virtual-scroll-viewport';

@NgModule({
imports: [PlatformModule, BidiModule],
imports: [BidiModule, PlatformModule],
exports: [
BidiModule,
CdkFixedSizeVirtualScroll,
Expand Down
14 changes: 12 additions & 2 deletions src/cdk/scrolling/virtual-scroll-viewport.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ cdk-virtual-scroll-viewport {
contain: strict;
transform: translateZ(0);
will-change: scroll-position;
-webkit-overflow-scrolling: touch;
}

// Wrapper element for the rendered content. This element will be transformed to push the rendered
Expand All @@ -44,7 +45,14 @@ cdk-virtual-scroll-viewport {
top: 0;
left: 0;
contain: content;
will-change: transform;

// Note: We can't put `will-change: transform;` here because it causes Safari to not update the
// viewport's `scrollHeight` when the spacer's transform changes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: might sound a little weird, but we could even have a unit test for this if we want to be super sure that it never gets reintroduced. E.g. the test would just be expect(getComputedStyle(element).willChange).toBe('none').

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually did add some unit tests that verify the scrollHeight / scrollWidth are correct after updating the spacer transform. They would fail if someone tried to add this back


[dir='rtl'] & {
right: 0;
left: auto;
}
}

.cdk-virtual-scroll-orientation-horizontal .cdk-virtual-scroll-content-wrapper {
Expand All @@ -67,7 +75,9 @@ cdk-virtual-scroll-viewport {
height: 1px;
width: 1px;
transform-origin: 0 0;
will-change: transform;

// Note: We can't put `will-change: transform;` here because it causes Safari to not update the
// viewport's `scrollHeight` when the spacer's transform changes.

[dir='rtl'] & {
right: 0;
Expand Down
Loading