Skip to content

Commit

Permalink
fix(select): handle async changes to the option label
Browse files Browse the repository at this point in the history
Currently `mat-select` doesn't react to changes in the label of its options, which is problematic, because the option label might be populated at a later point by a pipe or it might have changed while the select is closed. These changes add a `Subject` to the `MatOption` that will emit whenever the label changes and allows the select to react accordingly.

Fixes angular#7923.
  • Loading branch information
crisbeto committed Dec 29, 2017
1 parent c3d7cd9 commit 8e9cc0e
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 11 deletions.
24 changes: 23 additions & 1 deletion src/lib/core/option/option.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import {coerceBooleanProperty} from '@angular/cdk/coercion';
import {ENTER, SPACE} from '@angular/cdk/keycodes';
import {Subject} from 'rxjs/Subject';
import {
ChangeDetectionStrategy,
ChangeDetectorRef,
Expand All @@ -21,6 +22,7 @@ import {
ViewEncapsulation,
InjectionToken,
Inject,
AfterViewChecked,
} from '@angular/core';
import {MatOptgroup} from './optgroup';

Expand Down Expand Up @@ -81,11 +83,12 @@ export const MAT_OPTION_PARENT_COMPONENT =
preserveWhitespaces: false,
changeDetection: ChangeDetectionStrategy.OnPush,
})
export class MatOption {
export class MatOption implements AfterViewChecked {
private _selected = false;
private _active = false;
private _disabled = false;
private _id = `mat-option-${_uniqueIdCounter++}`;
private _oldViewValue = '';

/** Whether the wrapping component is in multiple selection mode. */
get multiple() { return this._parent && this._parent.multiple; }
Expand All @@ -110,6 +113,9 @@ export class MatOption {
/** Event emitted when the option is selected or deselected. */
@Output() onSelectionChange = new EventEmitter<MatOptionSelectionChange>();

/** Emits when the state of the option changes and any parents have to be notified. */
_stateChanges = new Subject<void>();

constructor(
private _element: ElementRef,
private _changeDetectorRef: ChangeDetectorRef,
Expand Down Expand Up @@ -219,6 +225,22 @@ export class MatOption {
return this._element.nativeElement;
}

ngAfterViewChecked() {
// Since parent components could be using the option's label to display the selected values
// (e.g. `mat-select`) and they don't have a way of knowing if the option's label has changed
// we have to check for changes in the DOM ourselves and dispatch an event. These checks are
// relatively cheap, however we still limit them only to selected options in order to avoid
// hitting the DOM too often.
if (this._selected) {
const viewValue = this.viewValue;

if (viewValue !== this._oldViewValue) {
this._oldViewValue = viewValue;
this._stateChanges.next();
}
}
}

/** Emits the selection change event. */
private _emitSelectionChangeEvent(isUserInput = false): void {
this.onSelectionChange.emit(new MatOptionSelectionChange(this, isUserInput));
Expand Down
12 changes: 12 additions & 0 deletions src/lib/select/select.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -997,6 +997,18 @@ describe('MatSelect', () => {
.toBe(fixture.componentInstance.options.last);
}));

it('should update the trigger when the selected option label is changed', fakeAsync(() => {
fixture.componentInstance.control.setValue('pizza-1');
fixture.detectChanges();

expect(trigger.textContent!.trim()).toBe('Pizza');

fixture.componentInstance.foods[1].viewValue = 'Calzone';
fixture.detectChanges();

expect(trigger.textContent!.trim()).toBe('Calzone');
}));

it('should not select disabled options', fakeAsync(() => {
trigger.click();
fixture.detectChanges();
Expand Down
28 changes: 18 additions & 10 deletions src/lib/select/select.ts
Original file line number Diff line number Diff line change
Expand Up @@ -829,16 +829,24 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,

/** Drops current option subscriptions and IDs and resets from scratch. */
private _resetOptions(): void {
this.optionSelectionChanges.pipe(
takeUntil(merge(this._destroy, this.options.changes)),
filter(event => event.isUserInput)
).subscribe(event => {
this._onSelect(event.source);

if (!this.multiple) {
this.close();
}
});
const changedOrDestroyed = merge(this.options.changes, this._destroy);

this.optionSelectionChanges
.pipe(takeUntil(changedOrDestroyed), filter(event => event.isUserInput))
.subscribe(event => {
this._onSelect(event.source);

if (!this.multiple) {
this.close();
}
});

merge(...this.options.map(option => option._stateChanges))
.pipe(takeUntil(changedOrDestroyed))
.subscribe(() => {
this._changeDetectorRef.markForCheck();
this.stateChanges.next();
});

this._setOptionIds();
}
Expand Down

0 comments on commit 8e9cc0e

Please sign in to comment.