Skip to content

Commit

Permalink
refactor(multiple): replace fromEvent usages (angular#30201)
Browse files Browse the repository at this point in the history
Replaces all of our usages of the rxjs `fromEvent` with direct event listeners going through the renderer. This has a few benefits:
* In most cases it made our simpler and easier to follow.
* By going through the renderer, other tooling can hook into it (e.g. the tracing service).
* It reduces our reliance on rxjs.

I also ended up cleaning up the fragile testing setup in `cdk/menu` which would've broken any time we introduce a new `inject` call.
  • Loading branch information
crisbeto authored Dec 18, 2024
1 parent 8921d07 commit ad70e87
Show file tree
Hide file tree
Showing 21 changed files with 362 additions and 420 deletions.
42 changes: 21 additions & 21 deletions src/cdk-experimental/column-resize/column-resize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,11 @@ import {
Input,
NgZone,
OnDestroy,
Renderer2,
} from '@angular/core';
import {_IdGenerator} from '@angular/cdk/a11y';
import {fromEvent, merge, Subject} from 'rxjs';
import {filter, map, mapTo, pairwise, startWith, take, takeUntil} from 'rxjs/operators';
import {merge, Subject} from 'rxjs';
import {mapTo, pairwise, startWith, take, takeUntil} from 'rxjs/operators';

import {_closest} from '@angular/cdk-experimental/popover-edit';

Expand All @@ -44,6 +45,8 @@ export const COLUMN_RESIZE_OPTIONS = new InjectionToken<ColumnResizeOptions>(
*/
@Directive()
export abstract class ColumnResize implements AfterViewInit, OnDestroy {
private _renderer = inject(Renderer2);
private _eventCleanups: (() => void)[] | undefined;
protected readonly destroyed = new Subject<void>();

/* Publicly accessible interface for triggering and being notified of resizes. */
Expand Down Expand Up @@ -78,6 +81,7 @@ export abstract class ColumnResize implements AfterViewInit, OnDestroy {
}

ngOnDestroy() {
this._eventCleanups?.forEach(cleanup => cleanup());
this.destroyed.next();
this.destroyed.complete();
}
Expand All @@ -99,25 +103,21 @@ export abstract class ColumnResize implements AfterViewInit, OnDestroy {

private _listenForRowHoverEvents() {
this.ngZone.runOutsideAngular(() => {
const element = this.elementRef.nativeElement!;

fromEvent<MouseEvent>(element, 'mouseover')
.pipe(
map(event => _closest(event.target, HEADER_CELL_SELECTOR)),
takeUntil(this.destroyed),
)
.subscribe(this.eventDispatcher.headerCellHovered);
fromEvent<MouseEvent>(element, 'mouseleave')
.pipe(
filter(
event =>
!!event.relatedTarget &&
!(event.relatedTarget as Element).matches(RESIZE_OVERLAY_SELECTOR),
),
mapTo(null),
takeUntil(this.destroyed),
)
.subscribe(this.eventDispatcher.headerCellHovered);
const element = this.elementRef.nativeElement;

this._eventCleanups = [
this._renderer.listen(element, 'mouseover', (event: MouseEvent) => {
this.eventDispatcher.headerCellHovered.next(_closest(event.target, HEADER_CELL_SELECTOR));
}),
this._renderer.listen(element, 'mouseleave', (event: MouseEvent) => {
if (
event.relatedTarget &&
!(event.relatedTarget as Element).matches(RESIZE_OVERLAY_SELECTOR)
) {
this.eventDispatcher.headerCellHovered.next(null);
}
}),
];
});
}

Expand Down
36 changes: 28 additions & 8 deletions src/cdk-experimental/column-resize/overlay-handle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,20 @@
* found in the LICENSE file at https://angular.dev/license
*/

import {AfterViewInit, Directive, ElementRef, OnDestroy, NgZone} from '@angular/core';
import {
AfterViewInit,
Directive,
ElementRef,
OnDestroy,
NgZone,
Renderer2,
inject,
} from '@angular/core';
import {coerceCssPixelValue} from '@angular/cdk/coercion';
import {Directionality} from '@angular/cdk/bidi';
import {ESCAPE} from '@angular/cdk/keycodes';
import {CdkColumnDef, _CoalescedStyleScheduler} from '@angular/cdk/table';
import {fromEvent, Subject, merge} from 'rxjs';
import {Subject, merge, Observable} from 'rxjs';
import {
distinctUntilChanged,
filter,
Expand All @@ -37,6 +45,7 @@ import {ResizeRef} from './resize-ref';
*/
@Directive()
export abstract class ResizeOverlayHandle implements AfterViewInit, OnDestroy {
private _renderer = inject(Renderer2);
protected readonly destroyed = new Subject<void>();

protected abstract readonly columnDef: CdkColumnDef;
Expand All @@ -62,11 +71,11 @@ export abstract class ResizeOverlayHandle implements AfterViewInit, OnDestroy {

private _listenForMouseEvents() {
this.ngZone.runOutsideAngular(() => {
fromEvent<MouseEvent>(this.elementRef.nativeElement!, 'mouseenter')
this._observableFromEvent<MouseEvent>(this.elementRef.nativeElement!, 'mouseenter')
.pipe(mapTo(this.resizeRef.origin.nativeElement!), takeUntil(this.destroyed))
.subscribe(cell => this.eventDispatcher.headerCellHovered.next(cell));

fromEvent<MouseEvent>(this.elementRef.nativeElement!, 'mouseleave')
this._observableFromEvent<MouseEvent>(this.elementRef.nativeElement!, 'mouseleave')
.pipe(
map(
event =>
Expand All @@ -76,7 +85,7 @@ export abstract class ResizeOverlayHandle implements AfterViewInit, OnDestroy {
)
.subscribe(cell => this.eventDispatcher.headerCellHovered.next(cell));

fromEvent<MouseEvent>(this.elementRef.nativeElement!, 'mousedown')
this._observableFromEvent<MouseEvent>(this.elementRef.nativeElement!, 'mousedown')
.pipe(takeUntil(this.destroyed))
.subscribe(mousedownEvent => {
this._dragStarted(mousedownEvent);
Expand All @@ -90,9 +99,9 @@ export abstract class ResizeOverlayHandle implements AfterViewInit, OnDestroy {
return;
}

const mouseup = fromEvent<MouseEvent>(this.document, 'mouseup');
const mousemove = fromEvent<MouseEvent>(this.document, 'mousemove');
const escape = fromEvent<KeyboardEvent>(this.document, 'keyup').pipe(
const mouseup = this._observableFromEvent<MouseEvent>(this.document, 'mouseup');
const mousemove = this._observableFromEvent<MouseEvent>(this.document, 'mousemove');
const escape = this._observableFromEvent<KeyboardEvent>(this.document, 'keyup').pipe(
filter(event => event.keyCode === ESCAPE),
);

Expand Down Expand Up @@ -233,4 +242,15 @@ export abstract class ResizeOverlayHandle implements AfterViewInit, OnDestroy {
}
});
}

private _observableFromEvent<T extends Event>(element: Element | Document, name: string) {
return new Observable<T>(subscriber => {
const handler = (event: T) => subscriber.next(event);
const cleanup = this._renderer.listen(element, name, handler);
return () => {
cleanup();
subscriber.complete();
};
});
}
}
34 changes: 15 additions & 19 deletions src/cdk/listbox/listbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,11 @@ import {
OnDestroy,
Output,
QueryList,
Renderer2,
signal,
} from '@angular/core';
import {ControlValueAccessor, NG_VALUE_ACCESSOR} from '@angular/forms';
import {defer, fromEvent, merge, Observable, Subject} from 'rxjs';
import {defer, merge, Observable, Subject} from 'rxjs';
import {filter, map, startWith, switchMap, takeUntil} from 'rxjs/operators';

/**
Expand Down Expand Up @@ -256,6 +257,8 @@ export class CdkOption<T = unknown> implements ListKeyManagerOption, Highlightab
],
})
export class CdkListbox<T = unknown> implements AfterContentInit, OnDestroy, ControlValueAccessor {
private _cleanupWindowBlur: (() => void) | undefined;

/** The id of the option's host element. */
@Input()
get id() {
Expand Down Expand Up @@ -439,7 +442,16 @@ export class CdkListbox<T = unknown> implements AfterContentInit, OnDestroy, Con

constructor() {
if (this._isBrowser) {
this._setPreviousActiveOptionAsActiveOptionOnWindowBlur();
const renderer = inject(Renderer2);

this._cleanupWindowBlur = this.ngZone.runOutsideAngular(() => {
return renderer.listen('window', 'blur', () => {
if (this.element.contains(document.activeElement) && this._previousActiveOption) {
this._setActiveOption(this._previousActiveOption);
this._previousActiveOption = null;
}
});
});
}
}

Expand All @@ -465,6 +477,7 @@ export class CdkListbox<T = unknown> implements AfterContentInit, OnDestroy, Con
}

ngOnDestroy() {
this._cleanupWindowBlur?.();
this.listKeyManager?.destroy();
this.destroyed.next();
this.destroyed.complete();
Expand Down Expand Up @@ -1035,23 +1048,6 @@ export class CdkListbox<T = unknown> implements AfterContentInit, OnDestroy, Con
const index = this.options.toArray().indexOf(this._lastTriggered!);
return index === -1 ? null : index;
}

/**
* Set previous active option as active option on window blur.
* This ensures that the `activeOption` matches the actual focused element when the user returns to the document.
*/
private _setPreviousActiveOptionAsActiveOptionOnWindowBlur() {
this.ngZone.runOutsideAngular(() => {
fromEvent(window, 'blur')
.pipe(takeUntil(this.destroyed))
.subscribe(() => {
if (this.element.contains(document.activeElement) && this._previousActiveOption) {
this._setActiveOption(this._previousActiveOption);
this._previousActiveOption = null;
}
});
});
}
}

/** Change event that is fired whenever the value of the listbox changes. */
Expand Down
35 changes: 23 additions & 12 deletions src/cdk/menu/menu-aim.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,16 @@
* found in the LICENSE file at https://angular.dev/license
*/

import {Directive, inject, Injectable, InjectionToken, NgZone, OnDestroy} from '@angular/core';
import {fromEvent, Subject} from 'rxjs';
import {filter, takeUntil} from 'rxjs/operators';
import {
Directive,
inject,
Injectable,
InjectionToken,
NgZone,
OnDestroy,
RendererFactory2,
} from '@angular/core';
import {Subject} from 'rxjs';
import {FocusableElement, PointerFocusTracker} from './pointer-focus-tracker';
import {Menu} from './menu-interface';
import {throwMissingMenuReference, throwMissingPointerFocusTracker} from './menu-errors';
Expand Down Expand Up @@ -102,8 +109,9 @@ function isWithinSubmenu(submenuPoints: DOMRect, m: number, b: number) {
*/
@Injectable()
export class TargetMenuAim implements MenuAim, OnDestroy {
/** The Angular zone. */
private readonly _ngZone = inject(NgZone);
private readonly _renderer = inject(RendererFactory2).createRenderer(null, null);
private _cleanupMousemove: (() => void) | undefined;

/** The last NUM_POINTS mouse move events. */
private readonly _points: Point[] = [];
Expand All @@ -121,6 +129,7 @@ export class TargetMenuAim implements MenuAim, OnDestroy {
private readonly _destroyed: Subject<void> = new Subject();

ngOnDestroy() {
this._cleanupMousemove?.();
this._destroyed.next();
this._destroyed.complete();
}
Expand Down Expand Up @@ -231,18 +240,20 @@ export class TargetMenuAim implements MenuAim, OnDestroy {

/** Subscribe to the root menus mouse move events and update the tracked mouse points. */
private _subscribeToMouseMoves() {
this._ngZone.runOutsideAngular(() => {
fromEvent<MouseEvent>(this._menu.nativeElement, 'mousemove')
.pipe(
filter((_: MouseEvent, index: number) => index % MOUSE_MOVE_SAMPLE_FREQUENCY === 0),
takeUntil(this._destroyed),
)
.subscribe((event: MouseEvent) => {
this._cleanupMousemove?.();

this._cleanupMousemove = this._ngZone.runOutsideAngular(() => {
let eventIndex = 0;

return this._renderer.listen(this._menu.nativeElement, 'mousemove', (event: MouseEvent) => {
if (eventIndex % MOUSE_MOVE_SAMPLE_FREQUENCY === 0) {
this._points.push({x: event.clientX, y: event.clientY});
if (this._points.length > NUM_POINTS) {
this._points.shift();
}
});
}
eventIndex++;
});
});
}
}
Expand Down
9 changes: 5 additions & 4 deletions src/cdk/menu/menu-base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
NgZone,
OnDestroy,
QueryList,
Renderer2,
computed,
inject,
signal,
Expand Down Expand Up @@ -51,12 +52,12 @@ export abstract class CdkMenuBase
extends CdkMenuGroup
implements Menu, AfterContentInit, OnDestroy
{
protected ngZone = inject(NgZone);
private _renderer = inject(Renderer2);

/** The menu's native DOM host element. */
readonly nativeElement: HTMLElement = inject(ElementRef).nativeElement;

/** The Angular zone. */
protected ngZone = inject(NgZone);

/** The stack of menus this menu belongs to. */
readonly menuStack: MenuStack = inject(MENU_STACK);

Expand Down Expand Up @@ -225,7 +226,7 @@ export abstract class CdkMenuBase
private _setUpPointerTracker() {
if (this.menuAim) {
this.ngZone.runOutsideAngular(() => {
this.pointerTracker = new PointerFocusTracker(this.items);
this.pointerTracker = new PointerFocusTracker(this._renderer, this.items);
});
this.menuAim.initialize(this, this.pointerTracker!);
}
Expand Down
26 changes: 8 additions & 18 deletions src/cdk/menu/menu-item-checkbox.spec.ts
Original file line number Diff line number Diff line change
@@ -1,30 +1,16 @@
import {Component, ElementRef} from '@angular/core';
import {ComponentFixture, TestBed, waitForAsync} from '@angular/core/testing';
import {Component} from '@angular/core';
import {ComponentFixture, TestBed} from '@angular/core/testing';
import {By} from '@angular/platform-browser';
import {CdkMenuModule} from './menu-module';
import {CdkMenuItemCheckbox} from './menu-item-checkbox';
import {CDK_MENU} from './menu-interface';
import {CdkMenu} from './menu';
import {MENU_STACK, MenuStack} from './menu-stack';

describe('MenuItemCheckbox', () => {
let fixture: ComponentFixture<SingleCheckboxButton>;
let checkbox: CdkMenuItemCheckbox;
let checkboxElement: HTMLButtonElement;

beforeEach(waitForAsync(() => {
TestBed.configureTestingModule({
imports: [CdkMenuModule, SingleCheckboxButton],
providers: [
{provide: CDK_MENU, useClass: CdkMenu},
{provide: MENU_STACK, useClass: MenuStack},
// View engine can't figure out the ElementRef to inject so we need to provide a fake
{provide: ElementRef, useValue: new ElementRef<null>(null)},
],
});
}));

beforeEach(() => {
TestBed.configureTestingModule({imports: [CdkMenuModule, SingleCheckboxButton]});
fixture = TestBed.createComponent(SingleCheckboxButton);
fixture.detectChanges();

Expand Down Expand Up @@ -99,7 +85,11 @@ describe('MenuItemCheckbox', () => {
});

@Component({
template: `<button cdkMenuItemCheckbox>Click me!</button>`,
template: `
<div cdkMenu>
<button cdkMenuItemCheckbox>Click me!</button>
</div>
`,
imports: [CdkMenuModule],
})
class SingleCheckboxButton {}
Loading

0 comments on commit ad70e87

Please sign in to comment.