Skip to content

Commit

Permalink
fix(datepicker): don't revalidate if new date object for same date is…
Browse files Browse the repository at this point in the history
… passed through input (#20362)

If a function is used to provide a new date object for the `min` or `max` of a datepicker input, we currently trigger a revalidation on each change detection, because the input value is technically different. Historically this is something that we haven't fixed on our end, because it masks an anti-pattern on the user's end, however in this case it breaks in a non-obvious way and detecting it would be more code than just handling it.

These changes add checks in several places so that we don't trigger validators or extra change detections if a new date object for the same date is provided.

These changes also clean up the date range input which had both a `stateChanges` and `_stateChanges` streams which are required by two separate interfaces. Now there's a single `stateChanges`.

Fixes #19907.

(cherry picked from commit 6296da2)
  • Loading branch information
crisbeto authored and mmalerba committed Sep 5, 2020
1 parent a307cf1 commit 5dca925
Show file tree
Hide file tree
Showing 8 changed files with 307 additions and 40 deletions.
123 changes: 119 additions & 4 deletions src/material/datepicker/date-range-input.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
import {Type, Component, ViewChild, ElementRef} from '@angular/core';
import {Type, Component, ViewChild, ElementRef, Directive} from '@angular/core';
import {ComponentFixture, TestBed, inject, fakeAsync, tick} from '@angular/core/testing';
import {FormsModule, ReactiveFormsModule, FormGroup, FormControl} from '@angular/forms';
import {
FormsModule,
ReactiveFormsModule,
FormGroup,
FormControl,
NG_VALIDATORS,
Validator,
} from '@angular/forms';
import {NoopAnimationsModule} from '@angular/platform-browser/animations';
import {OverlayContainer} from '@angular/cdk/overlay';
import {MatNativeDateModule} from '@angular/material/core';
Expand All @@ -15,7 +22,9 @@ import {MatDateRangePicker} from './date-range-picker';
import {MatStartDate, MatEndDate} from './date-range-input-parts';

describe('MatDateRangeInput', () => {
function createComponent<T>(component: Type<T>): ComponentFixture<T> {
function createComponent<T>(
component: Type<T>,
declarations: Type<any>[] = []): ComponentFixture<T> {
TestBed.configureTestingModule({
imports: [
FormsModule,
Expand All @@ -26,7 +35,7 @@ describe('MatDateRangeInput', () => {
ReactiveFormsModule,
MatNativeDateModule,
],
declarations: [component],
declarations: [component, ...declarations],
});

return TestBed.createComponent(component);
Expand Down Expand Up @@ -626,6 +635,78 @@ describe('MatDateRangeInput', () => {
endSubscription.unsubscribe();
});

it('should not trigger validators if new date object for same date is set for `min`', () => {
const fixture = createComponent(RangePickerWithCustomValidator, [CustomValidator]);
fixture.detectChanges();
const minDate = new Date(2019, 0, 1);
const validator = fixture.componentInstance.validator;

validator.validate.calls.reset();
fixture.componentInstance.min = minDate;
fixture.detectChanges();
expect(validator.validate).toHaveBeenCalledTimes(1);

fixture.componentInstance.min = new Date(minDate);
fixture.detectChanges();

expect(validator.validate).toHaveBeenCalledTimes(1);
});

it('should not trigger validators if new date object for same date is set for `max`', () => {
const fixture = createComponent(RangePickerWithCustomValidator, [CustomValidator]);
fixture.detectChanges();
const maxDate = new Date(2120, 0, 1);
const validator = fixture.componentInstance.validator;

validator.validate.calls.reset();
fixture.componentInstance.max = maxDate;
fixture.detectChanges();
expect(validator.validate).toHaveBeenCalledTimes(1);

fixture.componentInstance.max = new Date(maxDate);
fixture.detectChanges();

expect(validator.validate).toHaveBeenCalledTimes(1);
});

it('should not emit to `stateChanges` if new date object for same date is set for `min`', () => {
const fixture = createComponent(StandardRangePicker);
fixture.detectChanges();

const minDate = new Date(2019, 0, 1);
const spy = jasmine.createSpy('stateChanges spy');
const subscription = fixture.componentInstance.rangeInput.stateChanges.subscribe(spy);

fixture.componentInstance.minDate = minDate;
fixture.detectChanges();
expect(spy).toHaveBeenCalledTimes(1);

fixture.componentInstance.minDate = new Date(minDate);
fixture.detectChanges();
expect(spy).toHaveBeenCalledTimes(1);

subscription.unsubscribe();
});

it('should not emit to `stateChanges` if new date object for same date is set for `max`', () => {
const fixture = createComponent(StandardRangePicker);
fixture.detectChanges();

const maxDate = new Date(2120, 0, 1);
const spy = jasmine.createSpy('stateChanges spy');
const subscription = fixture.componentInstance.rangeInput.stateChanges.subscribe(spy);

fixture.componentInstance.maxDate = maxDate;
fixture.detectChanges();
expect(spy).toHaveBeenCalledTimes(1);

fixture.componentInstance.maxDate = new Date(maxDate);
fixture.detectChanges();
expect(spy).toHaveBeenCalledTimes(1);

subscription.unsubscribe();
});

});

@Component({
Expand Down Expand Up @@ -736,3 +817,37 @@ class RangePickerNoLabel {
@ViewChild('start') start: ElementRef<HTMLInputElement>;
@ViewChild('end') end: ElementRef<HTMLInputElement>;
}


@Directive({
selector: '[customValidator]',
providers: [{
provide: NG_VALIDATORS,
useExisting: CustomValidator,
multi: true
}]
})
class CustomValidator implements Validator {
validate = jasmine.createSpy('validate spy').and.returnValue(null);
}


@Component({
template: `
<mat-form-field>
<mat-date-range-input [rangePicker]="rangePicker" [min]="min" [max]="max">
<input matStartDate [(ngModel)]="start" customValidator/>
<input matEndDate [(ngModel)]="end" customValidator/>
</mat-date-range-input>
<mat-date-range-picker #rangePicker></mat-date-range-picker>
</mat-form-field>
`
})
class RangePickerWithCustomValidator {
@ViewChild(CustomValidator) validator: CustomValidator;
start: Date | null = null;
end: Date | null = null;
min: Date;
max: Date;
}
37 changes: 22 additions & 15 deletions src/material/datepicker/date-range-input.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
ElementRef,
Inject,
OnChanges,
SimpleChanges,
} from '@angular/core';
import {MatFormFieldControl, MatFormField, MAT_FORM_FIELD} from '@angular/material/form-field';
import {ThemePalette, DateAdapter} from '@angular/material/core';
Expand All @@ -34,7 +35,7 @@ import {
} from './date-range-input-parts';
import {MatDatepickerControl} from './datepicker-base';
import {createMissingDateImplError} from './datepicker-errors';
import {DateFilterFn} from './datepicker-input-base';
import {DateFilterFn, dateInputsHaveChanged} from './datepicker-input-base';
import {MatDateRangePicker, MatDateRangePickerInput} from './date-range-picker';
import {DateRange, MatDateSelectionModel} from './date-selection-model';

Expand Down Expand Up @@ -72,9 +73,6 @@ export class MatDateRangeInput<D> implements MatFormFieldControl<DateRange<D>>,
return this._model ? this._model.selection : null;
}

/** Emits when the input's state has changed. */
stateChanges = new Subject<void>();

/** Unique ID for the input. */
id = `mat-date-range-input-${nextUniqueId++}`;

Expand Down Expand Up @@ -133,17 +131,25 @@ export class MatDateRangeInput<D> implements MatFormFieldControl<DateRange<D>>,
@Input()
get min(): D | null { return this._min; }
set min(value: D | null) {
this._min = this._dateAdapter.getValidDateOrNull(this._dateAdapter.deserialize(value));
this._revalidate();
const validValue = this._dateAdapter.getValidDateOrNull(this._dateAdapter.deserialize(value));

if (!this._dateAdapter.sameDate(validValue, this._min)) {
this._min = validValue;
this._revalidate();
}
}
private _min: D | null;

/** The maximum valid date. */
@Input()
get max(): D | null { return this._max; }
set max(value: D | null) {
this._max = this._dateAdapter.getValidDateOrNull(this._dateAdapter.deserialize(value));
this._revalidate();
const validValue = this._dateAdapter.getValidDateOrNull(this._dateAdapter.deserialize(value));

if (!this._dateAdapter.sameDate(validValue, this._max)) {
this._max = validValue;
this._revalidate();
}
}
private _max: D | null;

Expand All @@ -159,7 +165,7 @@ export class MatDateRangeInput<D> implements MatFormFieldControl<DateRange<D>>,

if (newValue !== this._groupDisabled) {
this._groupDisabled = newValue;
this._stateChanges.next(undefined);
this.stateChanges.next(undefined);
}
}
_groupDisabled = false;
Expand Down Expand Up @@ -205,8 +211,8 @@ export class MatDateRangeInput<D> implements MatFormFieldControl<DateRange<D>>,
*/
ngControl: NgControl | null;

/** Emits when the input's state changes. */
_stateChanges = new Subject<void>();
/** Emits when the input's state has changed. */
stateChanges = new Subject<void>();

constructor(
private _changeDetectorRef: ChangeDetectorRef,
Expand Down Expand Up @@ -263,17 +269,18 @@ export class MatDateRangeInput<D> implements MatFormFieldControl<DateRange<D>>,
// We don't need to unsubscribe from this, because we
// know that the input streams will be completed on destroy.
merge(this._startInput.stateChanges, this._endInput.stateChanges).subscribe(() => {
this._stateChanges.next(undefined);
this.stateChanges.next(undefined);
});
}

ngOnChanges() {
this._stateChanges.next(undefined);
ngOnChanges(changes: SimpleChanges) {
if (dateInputsHaveChanged(changes, this._dateAdapter)) {
this.stateChanges.next(undefined);
}
}

ngOnDestroy() {
this.stateChanges.complete();
this._stateChanges.unsubscribe();
}

/** Gets the date at which the calendar should start. */
Expand Down
4 changes: 2 additions & 2 deletions src/material/datepicker/datepicker-base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ export interface MatDatepickerControl<D> {
disabled: boolean;
dateFilter: DateFilterFn<D>;
getConnectedOverlayOrigin(): ElementRef;
_stateChanges: Observable<void>;
stateChanges: Observable<void>;
}

/** Base class for a datepicker. */
Expand Down Expand Up @@ -442,7 +442,7 @@ export abstract class MatDatepickerBase<C extends MatDatepickerControl<D>, S,
this._inputStateChanges.unsubscribe();
this._datepickerInput = input;
this._inputStateChanges =
input._stateChanges.subscribe(() => this._stateChanges.next(undefined));
input.stateChanges.subscribe(() => this._stateChanges.next(undefined));
return this._model;
}

Expand Down
37 changes: 32 additions & 5 deletions src/material/datepicker/datepicker-input-base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
Output,
AfterViewInit,
OnChanges,
SimpleChanges,
} from '@angular/core';
import {
AbstractControl,
Expand Down Expand Up @@ -97,7 +98,7 @@ export abstract class MatDatepickerInputBase<S, D = ExtractDateTypeFromSelection

if (this._disabled !== newValue) {
this._disabled = newValue;
this._stateChanges.next(undefined);
this.stateChanges.next(undefined);
}

// We need to null check the `blur` method, because it's undefined during SSR.
Expand Down Expand Up @@ -125,7 +126,7 @@ export abstract class MatDatepickerInputBase<S, D = ExtractDateTypeFromSelection
_valueChange = new EventEmitter<D | null>();

/** Emits when the internal state has changed */
_stateChanges = new Subject<void>();
stateChanges = new Subject<void>();

_onTouched = () => {};
_validatorOnChange = () => {};
Expand Down Expand Up @@ -270,15 +271,17 @@ export abstract class MatDatepickerInputBase<S, D = ExtractDateTypeFromSelection
this._isInitialized = true;
}

ngOnChanges() {
this._stateChanges.next(undefined);
ngOnChanges(changes: SimpleChanges) {
if (dateInputsHaveChanged(changes, this._dateAdapter)) {
this.stateChanges.next(undefined);
}
}

ngOnDestroy() {
this._valueChangesSubscription.unsubscribe();
this._localeSubscription.unsubscribe();
this._valueChange.complete();
this._stateChanges.complete();
this.stateChanges.complete();
}

/** @docs-private */
Expand Down Expand Up @@ -394,3 +397,27 @@ export abstract class MatDatepickerInputBase<S, D = ExtractDateTypeFromSelection
static ngAcceptInputType_value: any;
static ngAcceptInputType_disabled: BooleanInput;
}

/**
* Checks whether the `SimpleChanges` object from an `ngOnChanges`
* callback has any changes, accounting for date objects.
*/
export function dateInputsHaveChanged(
changes: SimpleChanges,
adapter: DateAdapter<unknown>): boolean {
const keys = Object.keys(changes);

for (let key of keys) {
const {previousValue, currentValue} = changes[key];

if (adapter.isDateInstance(previousValue) && adapter.isDateInstance(currentValue)) {
if (!adapter.sameDate(previousValue, currentValue)) {
return true;
}
} else {
return true;
}
}

return false;
}
16 changes: 12 additions & 4 deletions src/material/datepicker/datepicker-input.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,17 +87,25 @@ export class MatDatepickerInput<D> extends MatDatepickerInputBase<D | null, D>
@Input()
get min(): D | null { return this._min; }
set min(value: D | null) {
this._min = this._dateAdapter.getValidDateOrNull(this._dateAdapter.deserialize(value));
this._validatorOnChange();
const validValue = this._dateAdapter.getValidDateOrNull(this._dateAdapter.deserialize(value));

if (!this._dateAdapter.sameDate(validValue, this._min)) {
this._min = validValue;
this._validatorOnChange();
}
}
private _min: D | null;

/** The maximum valid date. */
@Input()
get max(): D | null { return this._max; }
set max(value: D | null) {
this._max = this._dateAdapter.getValidDateOrNull(this._dateAdapter.deserialize(value));
this._validatorOnChange();
const validValue = this._dateAdapter.getValidDateOrNull(this._dateAdapter.deserialize(value));

if (!this._dateAdapter.sameDate(validValue, this._max)) {
this._max = validValue;
this._validatorOnChange();
}
}
private _max: D | null;

Expand Down
2 changes: 1 addition & 1 deletion src/material/datepicker/datepicker-toggle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ export class MatDatepickerToggle<D> implements AfterContentInit, OnChanges, OnDe
private _watchStateChanges() {
const datepickerStateChanged = this.datepicker ? this.datepicker._stateChanges : observableOf();
const inputStateChanged = this.datepicker && this.datepicker._datepickerInput ?
this.datepicker._datepickerInput._stateChanges : observableOf();
this.datepicker._datepickerInput.stateChanges : observableOf();
const datepickerToggled = this.datepicker ?
merge(this.datepicker.openedStream, this.datepicker.closedStream) :
observableOf();
Expand Down

0 comments on commit 5dca925

Please sign in to comment.