Skip to content

Commit

Permalink
fix(slider, drawer): unsubscribe from directionaly change subject (#6907
Browse files Browse the repository at this point in the history
)

If a slider component will be destroyed, the `ngOnDestroy` hook calls `_directionality.change.unsubscribe()`, which means that the `unsubscribe` method is called on the `EventEmitter` instead of the actual `Subscription`. This causes the `EventEmitter` to be kind of "completed", which then means that there will be errors if `emit()` is called again (https://github.com/ReactiveX/rxjs/blob/master/src/Subject.ts#L96).

Also fixes that the drawer never unsubscribes from the `_dir.change` `EventEmitter`. This means that even if the component is destroyed, whenever the directionality changes, the drawer calls `validateDrawers()`.

Fixes #6892. Fixes #6903.
  • Loading branch information
devversion authored and tinayuangao committed Sep 8, 2017
1 parent bef6271 commit a7ce31e
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 9 deletions.
13 changes: 11 additions & 2 deletions src/cdk/bidi/bidi.md
Expand Up @@ -6,16 +6,25 @@ text direction (RTL or LTR);
#### Example
```ts
@Component({ ... })
export class MyWidget {
export class MyWidget implements OnDestroy {

/** Whether the widget is in RTL mode or not. */
private isRtl: boolean;

/** Subscription to the Directionality change EventEmitter. */
private _dirChangeSubscription = Subscription.EMPTY;

constructor(dir: Directionality) {
this.isRtl = dir.value === 'rtl';

dir.change.subscribe(() => {
_dirChangeSubscription = dir.change.subscribe(() => {
this.flipDirection();
});
}

ngOnDestroy() {
this._dirChangeSubscription.unsubscribe();
}
}
```

12 changes: 10 additions & 2 deletions src/lib/sidenav/drawer.ts
Expand Up @@ -31,6 +31,7 @@ import {ESCAPE} from '../core/keyboard/keycodes';
import {first, takeUntil, startWith} from '../core/rxjs/index';
import {DOCUMENT} from '@angular/platform-browser';
import {merge} from 'rxjs/observable/merge';
import {Subscription} from 'rxjs/Subscription';


/** Throws an exception when two MdDrawer are matching the same position. */
Expand Down Expand Up @@ -313,7 +314,7 @@ export class MdDrawer implements AfterContentInit, OnDestroy {
changeDetection: ChangeDetectionStrategy.OnPush,
encapsulation: ViewEncapsulation.None,
})
export class MdDrawerContainer implements AfterContentInit {
export class MdDrawerContainer implements AfterContentInit, OnDestroy {
@ContentChildren(MdDrawer) _drawers: QueryList<MdDrawer>;

/** The drawer child with the `start` position. */
Expand All @@ -338,6 +339,9 @@ export class MdDrawerContainer implements AfterContentInit {
private _left: MdDrawer | null;
private _right: MdDrawer | null;

/** Subscription to the Directionality change EventEmitter. */
private _dirChangeSubscription = Subscription.EMPTY;

/** Inline styles to be applied to the container. */
_styles: { marginLeft: string; marginRight: string; transform: string; };

Expand All @@ -347,7 +351,7 @@ export class MdDrawerContainer implements AfterContentInit {
// If a `Dir` directive exists up the tree, listen direction changes and update the left/right
// properties to point to the proper start/end.
if (_dir != null) {
_dir.change.subscribe(() => this._validateDrawers());
this._dirChangeSubscription = _dir.change.subscribe(() => this._validateDrawers());
}
}

Expand All @@ -361,6 +365,10 @@ export class MdDrawerContainer implements AfterContentInit {
});
}

ngOnDestroy() {
this._dirChangeSubscription.unsubscribe();
}

/** Calls `open` of both start and end drawers */
open(): void {
this._drawers.forEach(drawer => drawer.open());
Expand Down
13 changes: 8 additions & 5 deletions src/lib/slider/slider.ts
Expand Up @@ -38,7 +38,7 @@ import {HammerInput} from '../core';
import {FocusOrigin, FocusOriginMonitor} from '../core/style/focus-origin-monitor';
import {CanDisable, mixinDisabled} from '../core/common-behaviors/disabled';
import {CanColor, mixinColor} from '../core/common-behaviors/color';

import {Subscription} from 'rxjs/Subscription';

/**
* Visually, a 30px separation between tick marks looks best. This is very subjective but it is
Expand Down Expand Up @@ -385,6 +385,9 @@ export class MdSlider extends _MdSliderMixinBase
/** Decimal places to round to, based on the step amount. */
private _roundLabelTo: number;

/** Subscription to the Directionality change EventEmitter. */
private _dirChangeSubscription = Subscription.EMPTY;

/** The value of the slider when the slide start event fires. */
private _valueOnSlideStart: number | null;

Expand Down Expand Up @@ -420,15 +423,15 @@ export class MdSlider extends _MdSliderMixinBase
this._changeDetectorRef.detectChanges();
});
if (this._dir) {
this._dir.change.subscribe(() => this._changeDetectorRef.markForCheck());
this._dirChangeSubscription = this._dir.change.subscribe(() => {
this._changeDetectorRef.markForCheck();
});
}
}

ngOnDestroy() {
this._focusOriginMonitor.stopMonitoring(this._elementRef.nativeElement);
if (this._dir) {
this._dir.change.unsubscribe();
}
this._dirChangeSubscription.unsubscribe();
}

_onMouseenter() {
Expand Down

0 comments on commit a7ce31e

Please sign in to comment.