Skip to content

Commit

Permalink
refactor(material/table): remove use of zone onStable for style sched…
Browse files Browse the repository at this point in the history 
…uling (#28782)

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

Roll forward with fix for column resizer

This reverts commit b01f682.

* test: fix tests
  • Loading branch information
@mmalerba
mmalerba committed Mar 27, 2024
1 parent 00fe374 commit 436301f
Show file tree
Hide file tree
Showing 6 changed files with 77 additions and 87 deletions.
32 changes: 16 additions & 16 deletions src/cdk-experimental/table-scroll-container/table-scroll-container.spec.ts
View file
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
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));
}),
);
}
}

0 comments on commit 436301f

Please sign in to comment.