Skip to content

Commit

Permalink
fix(material-experimental/mdc-slider): event handling fixes and clean…
Browse files Browse the repository at this point in the history
…up (#23063)

* Fixes that the `mouseenter` and `mouseleave` handlers on the slider were leaking. It was `Function.bind` returns a new function and we weren't passing the same function in when the component is destroyed.
* Fixes that slider was calling `unsubscribe` on subjects that belong to the slider thumbs. `Subject.unsubscribe` will drop all subscriptions to the observable, including ones that come from the outside. The correct thing to do is to either complete the observables or save a reference to the `Subscription` and call `unsubscribe` on it.
* Removes the `*Ctor` interfaces from the mixin class since it isn't necessary anymore.

(cherry picked from commit 91fa44e)
  • Loading branch information
crisbeto authored and amysorto committed Jun 28, 2021
1 parent 155cfea commit 174c99e
Showing 1 changed file with 20 additions and 23 deletions.
43 changes: 20 additions & 23 deletions src/material-experimental/mdc-slider/slider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,7 @@ import {
} from '@angular/core';
import {ControlValueAccessor, NG_VALUE_ACCESSOR} from '@angular/forms';
import {
CanColorCtor,
CanDisableRipple,
CanDisableRippleCtor,
MatRipple,
MAT_RIPPLE_GLOBAL_OPTIONS,
mixinColor,
Expand Down Expand Up @@ -132,30 +130,27 @@ export class MatSliderVisualThumb implements AfterViewInit, OnDestroy {
this._ripple.radius = 24;
this._sliderInput = this._slider._getInput(this.thumbPosition);

this._sliderInput.dragStart.subscribe((e: MatSliderDragEvent) => this._onDragStart(e));
this._sliderInput.dragEnd.subscribe((e: MatSliderDragEvent) => this._onDragEnd(e));
// Note that we don't unsubscribe from these, because they're complete on destroy.
this._sliderInput.dragStart.subscribe(event => this._onDragStart(event));
this._sliderInput.dragEnd.subscribe(event => this._onDragEnd(event));

this._sliderInput._focus.subscribe(() => this._onFocus());
this._sliderInput._blur.subscribe(() => this._onBlur());

// These two listeners don't update any data bindings so we bind them
// outside of the NgZone to pervent angular from needlessly running change detection.
// outside of the NgZone to prevent Angular from needlessly running change detection.
this._ngZone.runOutsideAngular(() => {
this._elementRef.nativeElement.addEventListener('mouseenter', this._onMouseEnter.bind(this));
this._elementRef.nativeElement.addEventListener('mouseleave', this._onMouseLeave.bind(this));
this._elementRef.nativeElement.addEventListener('mouseenter', this._onMouseEnter);
this._elementRef.nativeElement.addEventListener('mouseleave', this._onMouseLeave);
});
}

ngOnDestroy() {
this._sliderInput.dragStart.unsubscribe();
this._sliderInput.dragEnd.unsubscribe();
this._sliderInput._focus.unsubscribe();
this._sliderInput._blur.unsubscribe();
this._elementRef.nativeElement.removeEventListener('mouseenter', this._onMouseEnter);
this._elementRef.nativeElement.removeEventListener('mouseleave', this._onMouseLeave);
}

private _onMouseEnter(): void {
private _onMouseEnter = (): void => {
this._isHovered = true;
// We don't want to show the hover ripple on top of the focus ripple.
// This can happen if the user tabs to a thumb and then the user moves their cursor over it.
Expand All @@ -164,7 +159,7 @@ export class MatSliderVisualThumb implements AfterViewInit, OnDestroy {
}
}

private _onMouseLeave(): void {
private _onMouseLeave = (): void => {
this._isHovered = false;
this._hoverRippleRef?.fadeOut();
}
Expand Down Expand Up @@ -279,7 +274,7 @@ export class MatSliderVisualThumb implements AfterViewInit, OnDestroy {
multi: true
}],
})
export class MatSliderThumb implements AfterViewInit, ControlValueAccessor, OnInit {
export class MatSliderThumb implements AfterViewInit, ControlValueAccessor, OnInit, OnDestroy {

// ** IMPORTANT NOTE **
//
Expand Down Expand Up @@ -315,7 +310,7 @@ export class MatSliderThumb implements AfterViewInit, ControlValueAccessor, OnIn
* to facilitate the two-way binding for the `value` input.
* @docs-private
*/
@Output() readonly valueChange: EventEmitter<number> = new EventEmitter<number>();
@Output() readonly valueChange: EventEmitter<number> = new EventEmitter<number>();

/** Event emitted when the slider thumb starts being dragged. */
@Output() readonly dragStart: EventEmitter<MatSliderDragEvent>
Expand Down Expand Up @@ -386,6 +381,14 @@ export class MatSliderThumb implements AfterViewInit, ControlValueAccessor, OnIn
}
}

ngOnDestroy() {
this.dragStart.complete();
this.dragEnd.complete();
this._focus.complete();
this._blur.complete();
this.valueChange.complete();
}

_onBlur(): void {
this._onTouched();
this._blur.emit();
Expand Down Expand Up @@ -512,15 +515,9 @@ export class MatSliderThumb implements AfterViewInit, ControlValueAccessor, OnIn
}

// Boilerplate for applying mixins to MatSlider.
/** @docs-private */
class MatSliderBase {
const _MatSliderMixinBase = mixinColor(mixinDisableRipple(class {
constructor(public _elementRef: ElementRef<HTMLElement>) {}
}
const _MatSliderMixinBase:
CanColorCtor &
CanDisableRippleCtor &
typeof MatSliderBase =
mixinColor(mixinDisableRipple(MatSliderBase), 'primary');
}), 'primary');

/**
* Allows users to select from a range of values by moving the slider thumb. It is similar in
Expand Down

0 comments on commit 174c99e

Please sign in to comment.