Skip to content
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

Jump to bottom

refactor(material/table): remove use of zone onStable for style scheduling #28782

Merged
merged 2 commits into from
Mar 27, 2024
Merged

refactor(material/table): remove use of zone onStable for style scheduling #28782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
78b819f
refactor(material/table): remove use of zone onStable for style sched…
mmalerba Mar 26, 2024
6c5883a
test: fix tests
mmalerba Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 16 additions & 16 deletions src/cdk-experimental/table-scroll-container/table-scroll-container.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {CollectionViewer, DataSource} from '@angular/cdk/collections';
import {Component, Type, ViewChild} from '@angular/core';
import {ComponentFixture, fakeAsync, flushMicrotasks, TestBed} from '@angular/core/testing';
import {ComponentFixture, TestBed, waitForAsync} from '@angular/core/testing';
import {BehaviorSubject, combineLatest} from 'rxjs';
import {map} from 'rxjs/operators';
import {CdkTable, CdkTableModule} from '@angular/cdk/table';
Expand Down Expand Up @@ -48,10 +48,10 @@ describe('CdkTableScrollContainer', () => {
dataRows = getRows(tableElement);
});

it('sets scrollbar track margin for sticky headers', fakeAsync(() => {
it('sets scrollbar track margin for sticky headers', waitForAsync(async () => {
component.stickyHeaders = ['header-1', 'header-3'];
fixture.detectChanges();
flushMicrotasks();
await new Promise(r => setTimeout(r));

if (platform.FIREFOX) {
// ::-webkit-scrollbar-track is not recognized by Firefox.
Expand All @@ -66,18 +66,18 @@ describe('CdkTableScrollContainer', () => {

component.stickyHeaders = [];
fixture.detectChanges();
flushMicrotasks();
await new Promise(r => setTimeout(r));

expect(scrollerStyle.getPropertyValue('margin-top')).toBe('0px');
expect(scrollerStyle.getPropertyValue('margin-right')).toBe('0px');
expect(scrollerStyle.getPropertyValue('margin-bottom')).toBe('0px');
expect(scrollerStyle.getPropertyValue('margin-left')).toBe('0px');
}));

it('sets scrollbar track margin for sticky footers', fakeAsync(() => {
it('sets scrollbar track margin for sticky footers', waitForAsync(async () => {
component.stickyFooters = ['footer-1', 'footer-3'];
fixture.detectChanges();
flushMicrotasks();
await new Promise(r => setTimeout(r));

if (platform.FIREFOX) {
// ::-webkit-scrollbar-track is not recognized by Firefox.
Expand All @@ -92,18 +92,18 @@ describe('CdkTableScrollContainer', () => {

component.stickyFooters = [];
fixture.detectChanges();
flushMicrotasks();
await new Promise(r => setTimeout(r));

expect(scrollerStyle.getPropertyValue('margin-top')).toBe('0px');
expect(scrollerStyle.getPropertyValue('margin-right')).toBe('0px');
expect(scrollerStyle.getPropertyValue('margin-bottom')).toBe('0px');
expect(scrollerStyle.getPropertyValue('margin-left')).toBe('0px');
}));

it('sets scrollbar track margin for sticky start columns', fakeAsync(() => {
it('sets scrollbar track margin for sticky start columns', waitForAsync(async () => {
component.stickyStartColumns = ['column-1', 'column-3'];
fixture.detectChanges();
flushMicrotasks();
await new Promise(r => setTimeout(r));

if (platform.FIREFOX) {
// ::-webkit-scrollbar-track is not recognized by Firefox.
Expand All @@ -120,18 +120,18 @@ describe('CdkTableScrollContainer', () => {

component.stickyStartColumns = [];
fixture.detectChanges();
flushMicrotasks();
await new Promise(r => setTimeout(r));

expect(scrollerStyle.getPropertyValue('margin-top')).toBe('0px');
expect(scrollerStyle.getPropertyValue('margin-right')).toBe('0px');
expect(scrollerStyle.getPropertyValue('margin-bottom')).toBe('0px');
expect(scrollerStyle.getPropertyValue('margin-left')).toBe('0px');
}));

it('sets scrollbar track margin for sticky end columns', fakeAsync(() => {
it('sets scrollbar track margin for sticky end columns', waitForAsync(async () => {
component.stickyEndColumns = ['column-4', 'column-6'];
fixture.detectChanges();
flushMicrotasks();
await new Promise(r => setTimeout(r));

if (platform.FIREFOX) {
// ::-webkit-scrollbar-track is not recognized by Firefox.
Expand All @@ -148,21 +148,21 @@ describe('CdkTableScrollContainer', () => {

component.stickyEndColumns = [];
fixture.detectChanges();
flushMicrotasks();
await new Promise(r => setTimeout(r));

expect(scrollerStyle.getPropertyValue('margin-top')).toBe('0px');
expect(scrollerStyle.getPropertyValue('margin-right')).toBe('0px');
expect(scrollerStyle.getPropertyValue('margin-bottom')).toBe('0px');
expect(scrollerStyle.getPropertyValue('margin-left')).toBe('0px');
}));

it('sets scrollbar track margin for a combination of sticky rows and columns', fakeAsync(() => {
it('sets scrollbar track margin for a combination of sticky rows and columns', waitForAsync(async () => {
component.stickyHeaders = ['header-1'];
component.stickyFooters = ['footer-3'];
component.stickyStartColumns = ['column-1'];
component.stickyEndColumns = ['column-6'];
fixture.detectChanges();
flushMicrotasks();
await new Promise(r => setTimeout(r));

if (platform.FIREFOX) {
// ::-webkit-scrollbar-track is not recognized by Firefox.
Expand All @@ -184,7 +184,7 @@ describe('CdkTableScrollContainer', () => {
component.stickyStartColumns = [];
component.stickyEndColumns = [];
fixture.detectChanges();
flushMicrotasks();
await new Promise(r => setTimeout(r));

expect(scrollerStyle.getPropertyValue('margin-top')).toBe('0px');
expect(scrollerStyle.getPropertyValue('margin-right')).toBe('0px');
Expand Down
36 changes: 12 additions & 24 deletions src/cdk/table/coalesced-style-scheduler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Injectable, NgZone, OnDestroy, InjectionToken} from '@angular/core';
import {from, Subject} from 'rxjs';
import {take, takeUntil} from 'rxjs/operators';
import {Injectable, InjectionToken, NgZone, inject} from '@angular/core';

/**
* @docs-private
Expand All @@ -31,11 +29,11 @@ export const _COALESCED_STYLE_SCHEDULER = new InjectionToken<_CoalescedStyleSche
* @docs-private
*/
@Injectable()
export class _CoalescedStyleScheduler implements OnDestroy {
export class _CoalescedStyleScheduler {
private _currentSchedule: _Schedule | null = null;
private readonly _destroyed = new Subject<void>();
private _ngZone = inject(NgZone);

constructor(private readonly _ngZone: NgZone) {}
constructor(_unusedNgZone?: NgZone) {}

/**
* Schedules the specified task to run at the end of the current VM turn.
Expand All @@ -56,22 +54,19 @@ export class _CoalescedStyleScheduler implements OnDestroy {
this._currentSchedule!.endTasks.push(task);
}

/** Prevent any further tasks from running. */
ngOnDestroy() {
this._destroyed.next();
this._destroyed.complete();
}

private _createScheduleIfNeeded() {
if (this._currentSchedule) {
return;
}

this._currentSchedule = new _Schedule();

this._getScheduleObservable()
.pipe(takeUntil(this._destroyed))
.subscribe(() => {
this._ngZone.runOutsideAngular(() =>
// TODO(mmalerba): Scheduling this using something that runs less frequently
// (e.g. requestAnimationFrame, setTimeout, etc.) causes noticeable jank with the column
// resizer. We should audit the usages of schedule / scheduleEnd in that component and see
// if we can refactor it so that we don't need to flush the tasks quite so frequently.
queueMicrotask(() => {
while (this._currentSchedule!.tasks.length || this._currentSchedule!.endTasks.length) {
const schedule = this._currentSchedule!;

Expand All @@ -88,14 +83,7 @@ export class _CoalescedStyleScheduler implements OnDestroy {
}

this._currentSchedule = null;
});
}

private _getScheduleObservable() {
// Use onStable when in the context of an ongoing change detection cycle so that we
// do not accidentally trigger additional cycles.
return this._ngZone.isStable
? from(Promise.resolve(undefined))
: this._ngZone.onStable.pipe(take(1));
}),
);
}
}