From a2b5fb92cc4d3394096deb46b140d299524e604f Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Tue, 20 Feb 2024 09:20:05 +0100 Subject: [PATCH 1/2] fix(cdk/overlay): only emit positionChanges when position is different Currently we emit the `positionChanges` event whenever a position is recalculcated which can be on each scroll event. These changes switch to doing so only if either the actual position or the scrolled state has changed. --- .../flexible-connected-position-strategy.ts | 39 ++++++++++++++++--- 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/src/cdk/overlay/position/flexible-connected-position-strategy.ts b/src/cdk/overlay/position/flexible-connected-position-strategy.ts index 61d909881fe8..f4fbd8e3bbe7 100644 --- a/src/cdk/overlay/position/flexible-connected-position-strategy.ts +++ b/src/cdk/overlay/position/flexible-connected-position-strategy.ts @@ -115,6 +115,9 @@ export class FlexibleConnectedPositionStrategy implements PositionStrategy { /** The last position to have been calculated as the best fit position. */ private _lastPosition: ConnectedPosition | null; + /** The last calculated scroll visibility. Only tracked */ + private _lastScrollVisibility: ScrollingVisibility | null; + /** Subject that emits whenever the position changes. */ private readonly _positionChanges = new Subject(); @@ -710,18 +713,28 @@ export class FlexibleConnectedPositionStrategy implements PositionStrategy { this._addPanelClasses(position.panelClass); } - // Save the last connected position in case the position needs to be re-calculated. - this._lastPosition = position; - // Notify that the position has been changed along with its change properties. // We only emit if we've got any subscriptions, because the scroll visibility // calculations can be somewhat expensive. if (this._positionChanges.observers.length) { - const scrollableViewProperties = this._getScrollVisibility(); - const changeEvent = new ConnectedOverlayPositionChange(position, scrollableViewProperties); - this._positionChanges.next(changeEvent); + const scrollVisibility = this._getScrollVisibility(); + + // We're recalculating on scroll, but we only want to emit if anything + // changed since downstream code might be hitting the `NgZone`. + if ( + position !== this._lastPosition || + !this._lastScrollVisibility || + !compareScrollVisibility(this._lastScrollVisibility, scrollVisibility) + ) { + const changeEvent = new ConnectedOverlayPositionChange(position, scrollVisibility); + this._positionChanges.next(changeEvent); + } + + this._lastScrollVisibility = scrollVisibility; } + // Save the last connected position in case the position needs to be re-calculated. + this._lastPosition = position; this._isInitialRender = false; } @@ -1289,6 +1302,20 @@ function getRoundedBoundingClientRect(clientRect: Dimensions): Dimensions { }; } +/** Returns whether two `ScrollingVisibility` objects are identical. */ +function compareScrollVisibility(a: ScrollingVisibility, b: ScrollingVisibility): boolean { + if (a === b) { + return true; + } + + return ( + a.isOriginClipped === b.isOriginClipped && + a.isOriginOutsideView === b.isOriginOutsideView && + a.isOverlayClipped === b.isOverlayClipped && + a.isOverlayOutsideView === b.isOverlayOutsideView + ); +} + export const STANDARD_DROPDOWN_BELOW_POSITIONS: ConnectedPosition[] = [ {originX: 'start', originY: 'bottom', overlayX: 'start', overlayY: 'top'}, {originX: 'start', originY: 'top', overlayX: 'start', overlayY: 'bottom'}, From eaa5ac777a7a1fafea7cb678aebf5dc826681405 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Tue, 20 Feb 2024 09:26:14 +0100 Subject: [PATCH 2/2] fix(cdk/overlay): run positionChange event inside the zone Fixes that the `positionChange` event of the `CdkConnectedOverlay` directive was running outside the zone. Fixes #28568. --- src/cdk/overlay/overlay-directives.spec.ts | 14 +++++++++++++- src/cdk/overlay/overlay-directives.ts | 4 +++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/cdk/overlay/overlay-directives.spec.ts b/src/cdk/overlay/overlay-directives.spec.ts index eb086ace797e..0aea4c46403d 100644 --- a/src/cdk/overlay/overlay-directives.spec.ts +++ b/src/cdk/overlay/overlay-directives.spec.ts @@ -1,4 +1,4 @@ -import {Component, ElementRef, ViewChild} from '@angular/core'; +import {Component, ElementRef, NgZone, ViewChild} from '@angular/core'; import {By} from '@angular/platform-browser'; import { ComponentFixture, @@ -618,6 +618,18 @@ describe('Overlay directives', () => { .toBe(true); }); + it('should emit the position change handler inside the zone', () => { + let callsInZone: boolean[] = []; + + fixture.componentInstance.positionChangeHandler.and.callFake(() => { + callsInZone.push(NgZone.isInAngularZone()); + }); + fixture.componentInstance.isOpen = true; + fixture.detectChanges(); + + expect(callsInZone).toEqual([true]); + }); + it('should emit when attached', () => { expect(fixture.componentInstance.attachHandler).not.toHaveBeenCalled(); fixture.componentInstance.isOpen = true; diff --git a/src/cdk/overlay/overlay-directives.ts b/src/cdk/overlay/overlay-directives.ts index 4b560a2baff9..4775bfe6f7c3 100644 --- a/src/cdk/overlay/overlay-directives.ts +++ b/src/cdk/overlay/overlay-directives.ts @@ -16,6 +16,7 @@ import { Inject, InjectionToken, Input, + NgZone, OnChanges, OnDestroy, Optional, @@ -116,6 +117,7 @@ export class CdkConnectedOverlay implements OnDestroy, OnChanges { private _position: FlexibleConnectedPositionStrategy; private _scrollStrategyFactory: () => ScrollStrategy; private _disposeOnNavigation = false; + private _ngZone = inject(NgZone); /** Origin for the connected overlay. */ @Input('cdkConnectedOverlayOrigin') @@ -421,7 +423,7 @@ export class CdkConnectedOverlay implements OnDestroy, OnChanges { this._positionSubscription = this._position.positionChanges .pipe(takeWhile(() => this.positionChange.observers.length > 0)) .subscribe(position => { - this.positionChange.emit(position); + this._ngZone.run(() => this.positionChange.emit(position)); if (this.positionChange.observers.length === 0) { this._positionSubscription.unsubscribe();