Skip to content

refactor(multiple): use renderer for manual event bindings #30179

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.

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 1 commit into from
Dec 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 6 additions & 4 deletions src/cdk/dialog/dialog-container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
Injector,
NgZone,
OnDestroy,
Renderer2,
ViewChild,
ViewEncapsulation,
afterNextRender,
Expand Down Expand Up @@ -79,6 +80,7 @@ export class CdkDialogContainer<C extends DialogConfig = DialogConfig>
protected _ngZone = inject(NgZone);
private _overlayRef = inject(OverlayRef);
private _focusMonitor = inject(FocusMonitor);
private _renderer = inject(Renderer2);

private _platform = inject(Platform);
protected _document = inject(DOCUMENT, {optional: true})!;
Expand Down Expand Up @@ -223,13 +225,13 @@ export class CdkDialogContainer<C extends DialogConfig = DialogConfig>
// The tabindex attribute should be removed to avoid navigating to that element again
this._ngZone.runOutsideAngular(() => {
const callback = () => {
element.removeEventListener('blur', callback);
element.removeEventListener('mousedown', callback);
deregisterBlur();
deregisterMousedown();
element.removeAttribute('tabindex');
};

element.addEventListener('blur', callback);
element.addEventListener('mousedown', callback);
const deregisterBlur = this._renderer.listen(element, 'blur', callback);
const deregisterMousedown = this._renderer.listen(element, 'mousedown', callback);
});
}
element.focus(options);
Expand Down
4 changes: 3 additions & 1 deletion src/cdk/drag-drop/drag-drop.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.dev/license
*/

import {Injectable, NgZone, ElementRef, inject} from '@angular/core';
import {Injectable, NgZone, ElementRef, inject, RendererFactory2} from '@angular/core';
import {DOCUMENT} from '@angular/common';
import {ViewportRuler} from '@angular/cdk/scrolling';
import {DragRef, DragRefConfig} from './drag-ref';
Expand All @@ -28,6 +28,7 @@ export class DragDrop {
private _ngZone = inject(NgZone);
private _viewportRuler = inject(ViewportRuler);
private _dragDropRegistry = inject(DragDropRegistry);
private _renderer = inject(RendererFactory2).createRenderer(null, null);
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that the renderer can't be injected in services so we have to go through the factory. Even though the method is called createRenderer, the service will return the same renderer every time: https://github.com/angular/angular/blob/main/packages/platform-browser/src/dom/dom_renderer.ts#L117

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a little unfortunate since it relies on the implementation in the framework. Would be nice (and out of scope) if Angular had some better supported API for the case of a servicing wanting the default renderer. As it reads, passing null, null is confusing and doesnt even seem like it should be valid


constructor(...args: unknown[]);
constructor() {}
Expand All @@ -48,6 +49,7 @@ export class DragDrop {
this._ngZone,
this._viewportRuler,
this._dragDropRegistry,
this._renderer,
);
}

Expand Down
11 changes: 7 additions & 4 deletions src/cdk/drag-drop/drag-ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
ElementRef,
EmbeddedViewRef,
NgZone,
Renderer2,
TemplateRef,
ViewContainerRef,
signal,
Expand Down Expand Up @@ -378,6 +379,7 @@ export class DragRef<T = any> {
private _ngZone: NgZone,
private _viewportRuler: ViewportRuler,
private _dragDropRegistry: DragDropRegistry,
private _renderer: Renderer2,
) {
this.withRootElement(element).withParent(_config.parentDragRef || null);
this._parentPositions = new ParentPositionTracker(_document);
Expand Down Expand Up @@ -853,6 +855,7 @@ export class DragRef<T = any> {
this._pickupPositionOnPage,
this._initialTransform,
this._config.zIndex || 1000,
this._renderer,
);
this._preview.attach(this._getPreviewInsertionPoint(parent, shadowRoot));

Expand Down Expand Up @@ -1106,24 +1109,24 @@ export class DragRef<T = any> {

return this._ngZone.runOutsideAngular(() => {
return new Promise(resolve => {
const handler = ((event: TransitionEvent) => {
const handler = (event: TransitionEvent) => {
if (
!event ||
(this._preview &&
_getEventTarget(event) === this._preview.element &&
event.propertyName === 'transform')
) {
this._preview?.removeEventListener('transitionend', handler);
cleanupListener();
resolve();
clearTimeout(timeout);
}
}) as EventListenerOrEventListenerObject;
};

// If a transition is short enough, the browser might not fire the `transitionend` event.
// Since we know how long it's supposed to take, add a timeout with a 50% buffer that'll
// fire if the transition hasn't completed when it was supposed to.
const timeout = setTimeout(handler as Function, duration * 1.5);
this._preview!.addEventListener('transitionend', handler);
const cleanupListener = this._preview!.addEventListener('transitionend', handler);
});
});
}
Expand Down
11 changes: 4 additions & 7 deletions src/cdk/drag-drop/preview-ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.dev/license
*/

import {EmbeddedViewRef, TemplateRef, ViewContainerRef} from '@angular/core';
import {EmbeddedViewRef, Renderer2, TemplateRef, ViewContainerRef} from '@angular/core';
import {Direction} from '@angular/cdk/bidi';
import {
extendStyles,
Expand Down Expand Up @@ -56,6 +56,7 @@ export class PreviewRef {
},
private _initialTransform: string | null,
private _zIndex: number,
private _renderer: Renderer2,
) {}

attach(parent: HTMLElement): void {
Expand Down Expand Up @@ -91,12 +92,8 @@ export class PreviewRef {
return getTransformTransitionDurationInMs(this._preview);
}

addEventListener(name: string, handler: EventListenerOrEventListenerObject) {
this._preview.addEventListener(name, handler);
}

removeEventListener(name: string, handler: EventListenerOrEventListenerObject) {
this._preview.removeEventListener(name, handler);
addEventListener(name: string, handler: (event: any) => void): () => void {
return this._renderer.listen(this._preview, name, handler);
}

private _createPreview(): HTMLElement {
Expand Down
15 changes: 10 additions & 5 deletions src/cdk/observers/private/shared-resize-observer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.dev/license
*/
import {inject, Injectable, NgZone, OnDestroy} from '@angular/core';
import {inject, Injectable, NgZone, OnDestroy, RendererFactory2} from '@angular/core';
import {Observable, Subject} from 'rxjs';
import {filter, shareReplay, takeUntil} from 'rxjs/operators';

Expand Down Expand Up @@ -98,6 +98,8 @@ class SingleBoxSharedResizeObserver {
providedIn: 'root',
})
export class SharedResizeObserver implements OnDestroy {
private _cleanupErrorListener: (() => void) | undefined;

/** Map of box type to shared resize observer. */
private _observers = new Map<ResizeObserverBoxOptions, SingleBoxSharedResizeObserver>();

Expand All @@ -107,7 +109,12 @@ export class SharedResizeObserver implements OnDestroy {
constructor() {
if (typeof ResizeObserver !== 'undefined' && (typeof ngDevMode === 'undefined' || ngDevMode)) {
this._ngZone.runOutsideAngular(() => {
window.addEventListener('error', loopLimitExceededErrorHandler);
const renderer = inject(RendererFactory2).createRenderer(null, null);
this._cleanupErrorListener = renderer.listen(
'window',
'error',
loopLimitExceededErrorHandler,
);
});
}
}
Expand All @@ -117,9 +124,7 @@ export class SharedResizeObserver implements OnDestroy {
observer.destroy();
}
this._observers.clear();
if (typeof ResizeObserver !== 'undefined' && (typeof ngDevMode === 'undefined' || ngDevMode)) {
window.removeEventListener('error', loopLimitExceededErrorHandler);
}
this._cleanupErrorListener?.();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,15 +138,17 @@ describe('OverlayKeyboardDispatcher', () => {
it('should dispose of the global keyboard event handler correctly', () => {
const overlayRef = overlay.create();
const body = document.body;

spyOn(body, 'addEventListener');
spyOn(body, 'removeEventListener');

keyboardDispatcher.add(overlayRef);
expect(body.addEventListener).toHaveBeenCalledWith('keydown', jasmine.any(Function));
expect(body.addEventListener).toHaveBeenCalledWith('keydown', jasmine.any(Function), false);

overlayRef.dispose();
expect(body.removeEventListener).toHaveBeenCalledWith('keydown', jasmine.any(Function));
expect(document.body.removeEventListener).toHaveBeenCalledWith(
'keydown',
jasmine.any(Function),
);
});

it('should skip overlays that do not have keydown event subscriptions', () => {
Expand Down
28 changes: 10 additions & 18 deletions src/cdk/overlay/dispatchers/overlay-keyboard-dispatcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.dev/license
*/

import {Injectable, NgZone, inject} from '@angular/core';
import {Injectable, NgZone, RendererFactory2, inject} from '@angular/core';
import {BaseOverlayDispatcher} from './base-overlay-dispatcher';
import type {OverlayRef} from '../overlay-ref';

Expand All @@ -17,30 +17,28 @@ import type {OverlayRef} from '../overlay-ref';
*/
@Injectable({providedIn: 'root'})
export class OverlayKeyboardDispatcher extends BaseOverlayDispatcher {
private _ngZone = inject(NgZone, {optional: true});
private _ngZone = inject(NgZone);
private _renderer = inject(RendererFactory2).createRenderer(null, null);
private _cleanupKeydown: (() => void) | undefined;

/** Add a new overlay to the list of attached overlay refs. */
override add(overlayRef: OverlayRef): void {
super.add(overlayRef);

// Lazily start dispatcher once first overlay is added
if (!this._isAttached) {
/** @breaking-change 14.0.0 _ngZone will be required. */
if (this._ngZone) {
this._ngZone.runOutsideAngular(() =>
this._document.body.addEventListener('keydown', this._keydownListener),
);
} else {
this._document.body.addEventListener('keydown', this._keydownListener);
}
this._ngZone.runOutsideAngular(() => {
this._cleanupKeydown = this._renderer.listen('body', 'keydown', this._keydownListener);
});

this._isAttached = true;
}
}

/** Detaches the global keyboard event listener. */
protected detach() {
if (this._isAttached) {
this._document.body.removeEventListener('keydown', this._keydownListener);
this._cleanupKeydown?.();
this._isAttached = false;
}
}
Expand All @@ -57,13 +55,7 @@ export class OverlayKeyboardDispatcher extends BaseOverlayDispatcher {
// because we don't want overlays that don't handle keyboard events to block the ones below
// them that do.
if (overlays[i]._keydownEvents.observers.length > 0) {
const keydownEvents = overlays[i]._keydownEvents;
/** @breaking-change 14.0.0 _ngZone will be required. */
if (this._ngZone) {
this._ngZone.run(() => keydownEvents.next(event));
} else {
keydownEvents.next(event);
}
this._ngZone.run(() => overlays[i]._keydownEvents.next(event));
break;
}
}
Expand Down
40 changes: 15 additions & 25 deletions src/cdk/overlay/fullscreen-overlay-container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.dev/license
*/

import {Injectable, OnDestroy} from '@angular/core';
import {inject, Injectable, OnDestroy, RendererFactory2} from '@angular/core';
import {OverlayContainer} from './overlay-container';

/**
Expand All @@ -18,8 +18,9 @@ import {OverlayContainer} from './overlay-container';
*/
@Injectable({providedIn: 'root'})
export class FullscreenOverlayContainer extends OverlayContainer implements OnDestroy {
private _renderer = inject(RendererFactory2).createRenderer(null, null);
private _fullScreenEventName: string | undefined;
private _fullScreenListener: () => void;
private _cleanupFullScreenListener: (() => void) | undefined;

constructor(...args: unknown[]);

Expand All @@ -29,38 +30,27 @@ export class FullscreenOverlayContainer extends OverlayContainer implements OnDe

override ngOnDestroy() {
super.ngOnDestroy();

if (this._fullScreenEventName && this._fullScreenListener) {
this._document.removeEventListener(this._fullScreenEventName, this._fullScreenListener);
}
this._cleanupFullScreenListener?.();
}

protected override _createContainer(): void {
const eventName = this._getEventName();
super._createContainer();
this._adjustParentForFullscreenChange();
this._addFullscreenChangeListener(() => this._adjustParentForFullscreenChange());
}

private _adjustParentForFullscreenChange(): void {
if (!this._containerElement) {
return;
if (eventName) {
this._cleanupFullScreenListener?.();
this._cleanupFullScreenListener = this._renderer.listen('document', eventName, () => {
this._adjustParentForFullscreenChange();
});
}

const fullscreenElement = this.getFullscreenElement();
const parent = fullscreenElement || this._document.body;
parent.appendChild(this._containerElement);
}

private _addFullscreenChangeListener(fn: () => void) {
const eventName = this._getEventName();

if (eventName) {
if (this._fullScreenListener) {
this._document.removeEventListener(eventName, this._fullScreenListener);
}

this._document.addEventListener(eventName, fn);
this._fullScreenListener = fn;
private _adjustParentForFullscreenChange(): void {
if (this._containerElement) {
const fullscreenElement = this.getFullscreenElement();
const parent = fullscreenElement || this._document.body;
parent.appendChild(this._containerElement);
}
}

Expand Down
Loading
Loading