From ec8c3878ccc6732ae26974540b13c77aac9f79be Mon Sep 17 00:00:00 2001 From: crisbeto Date: Mon, 24 Aug 2020 21:34:51 +0200 Subject: [PATCH] fix(datepicker): don't revalidate if new date object for same date is passed through input 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. --- .../datepicker/date-range-input.spec.ts | 123 ++++++++++++++++- src/material/datepicker/date-range-input.ts | 37 +++--- src/material/datepicker/datepicker-base.ts | 4 +- .../datepicker/datepicker-input-base.ts | 37 +++++- src/material/datepicker/datepicker-input.ts | 16 ++- src/material/datepicker/datepicker-toggle.ts | 2 +- src/material/datepicker/datepicker.spec.ts | 125 +++++++++++++++++- .../public_api_guard/material/datepicker.d.ts | 3 +- 8 files changed, 307 insertions(+), 40 deletions(-) diff --git a/src/material/datepicker/date-range-input.spec.ts b/src/material/datepicker/date-range-input.spec.ts index 726d1817b504..29626d23ce7a 100644 --- a/src/material/datepicker/date-range-input.spec.ts +++ b/src/material/datepicker/date-range-input.spec.ts @@ -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'; @@ -15,7 +22,9 @@ import {MatDateRangePicker} from './date-range-picker'; import {MatStartDate, MatEndDate} from './date-range-input-parts'; describe('MatDateRangeInput', () => { - function createComponent(component: Type): ComponentFixture { + function createComponent( + component: Type, + declarations: Type[] = []): ComponentFixture { TestBed.configureTestingModule({ imports: [ FormsModule, @@ -26,7 +35,7 @@ describe('MatDateRangeInput', () => { ReactiveFormsModule, MatNativeDateModule, ], - declarations: [component], + declarations: [component, ...declarations], }); return TestBed.createComponent(component); @@ -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({ @@ -736,3 +817,37 @@ class RangePickerNoLabel { @ViewChild('start') start: ElementRef; @ViewChild('end') end: ElementRef; } + + +@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: ` + + + + + + + + + ` +}) +class RangePickerWithCustomValidator { + @ViewChild(CustomValidator) validator: CustomValidator; + start: Date | null = null; + end: Date | null = null; + min: Date; + max: Date; +} diff --git a/src/material/datepicker/date-range-input.ts b/src/material/datepicker/date-range-input.ts index e8d575e9830c..290d67ff7a07 100644 --- a/src/material/datepicker/date-range-input.ts +++ b/src/material/datepicker/date-range-input.ts @@ -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'; @@ -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'; @@ -72,9 +73,6 @@ export class MatDateRangeInput implements MatFormFieldControl>, return this._model ? this._model.selection : null; } - /** Emits when the input's state has changed. */ - stateChanges = new Subject(); - /** Unique ID for the input. */ id = `mat-date-range-input-${nextUniqueId++}`; @@ -133,8 +131,12 @@ export class MatDateRangeInput implements MatFormFieldControl>, @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; @@ -142,8 +144,12 @@ export class MatDateRangeInput implements MatFormFieldControl>, @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; @@ -159,7 +165,7 @@ export class MatDateRangeInput implements MatFormFieldControl>, if (newValue !== this._groupDisabled) { this._groupDisabled = newValue; - this._stateChanges.next(undefined); + this.stateChanges.next(undefined); } } _groupDisabled = false; @@ -205,8 +211,8 @@ export class MatDateRangeInput implements MatFormFieldControl>, */ ngControl: NgControl | null; - /** Emits when the input's state changes. */ - _stateChanges = new Subject(); + /** Emits when the input's state has changed. */ + stateChanges = new Subject(); constructor( private _changeDetectorRef: ChangeDetectorRef, @@ -261,17 +267,18 @@ export class MatDateRangeInput implements MatFormFieldControl>, // 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. */ diff --git a/src/material/datepicker/datepicker-base.ts b/src/material/datepicker/datepicker-base.ts index 4d11a869b348..dc118b6dba87 100644 --- a/src/material/datepicker/datepicker-base.ts +++ b/src/material/datepicker/datepicker-base.ts @@ -232,7 +232,7 @@ export interface MatDatepickerControl { disabled: boolean; dateFilter: DateFilterFn; getConnectedOverlayOrigin(): ElementRef; - _stateChanges: Observable; + stateChanges: Observable; } /** Base class for a datepicker. */ @@ -440,7 +440,7 @@ export abstract class MatDatepickerBase, 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; } diff --git a/src/material/datepicker/datepicker-input-base.ts b/src/material/datepicker/datepicker-input-base.ts index 0d5974de9043..8f6c4821ff5f 100644 --- a/src/material/datepicker/datepicker-input-base.ts +++ b/src/material/datepicker/datepicker-input-base.ts @@ -19,6 +19,7 @@ import { Output, AfterViewInit, OnChanges, + SimpleChanges, } from '@angular/core'; import { AbstractControl, @@ -97,7 +98,7 @@ export abstract class MatDatepickerInputBase(); /** Emits when the internal state has changed */ - _stateChanges = new Subject(); + stateChanges = new Subject(); _onTouched = () => {}; _validatorOnChange = () => {}; @@ -267,15 +268,17 @@ export abstract class MatDatepickerInputBase): 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; +} diff --git a/src/material/datepicker/datepicker-input.ts b/src/material/datepicker/datepicker-input.ts index cff393165974..d0c48a3cb77e 100644 --- a/src/material/datepicker/datepicker-input.ts +++ b/src/material/datepicker/datepicker-input.ts @@ -87,8 +87,12 @@ export class MatDatepickerInput extends MatDatepickerInputBase @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; @@ -96,8 +100,12 @@ export class MatDatepickerInput extends MatDatepickerInputBase @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; diff --git a/src/material/datepicker/datepicker-toggle.ts b/src/material/datepicker/datepicker-toggle.ts index 70c125288a2a..b50a37493826 100644 --- a/src/material/datepicker/datepicker-toggle.ts +++ b/src/material/datepicker/datepicker-toggle.ts @@ -120,7 +120,7 @@ export class MatDatepickerToggle 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(); diff --git a/src/material/datepicker/datepicker.spec.ts b/src/material/datepicker/datepicker.spec.ts index 5881017ffe1c..c8c82d5c3f5e 100644 --- a/src/material/datepicker/datepicker.spec.ts +++ b/src/material/datepicker/datepicker.spec.ts @@ -9,9 +9,16 @@ import { dispatchKeyboardEvent, dispatchMouseEvent, } from '@angular/cdk/testing/private'; -import {Component, Type, ViewChild, Provider} from '@angular/core'; +import {Component, Type, ViewChild, Provider, Directive} from '@angular/core'; import {ComponentFixture, fakeAsync, flush, inject, TestBed, tick} from '@angular/core/testing'; -import {FormControl, FormsModule, NgModel, ReactiveFormsModule} from '@angular/forms'; +import { + FormControl, + FormsModule, + NgModel, + ReactiveFormsModule, + Validator, + NG_VALIDATORS, +} from '@angular/forms'; import { MAT_DATE_LOCALE, MatNativeDateModule, @@ -40,11 +47,12 @@ describe('MatDatepicker', () => { const SUPPORTS_INTL = typeof Intl != 'undefined'; // Creates a test component fixture. - function createComponent( - component: Type, + function createComponent( + component: Type, imports: Type[] = [], providers: Provider[] = [], - entryComponents: Type[] = []): ComponentFixture { + entryComponents: Type[] = [], + declarations: Type[] = []): ComponentFixture { TestBed.configureTestingModule({ imports: [ @@ -57,7 +65,7 @@ describe('MatDatepicker', () => { ...imports ], providers, - declarations: [component, ...entryComponents], + declarations: [component, ...declarations, ...entryComponents], }); TestBed.overrideModule(BrowserDynamicTestingModule, { @@ -1955,6 +1963,80 @@ describe('MatDatepicker', () => { expect(overlay.style.minWidth).toBeFalsy(); }); + it('should not trigger validators if new date object for same date is set for `min`', () => { + const fixture = createComponent(DatepickerInputWithCustomValidator, + [MatNativeDateModule], undefined, undefined, [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(DatepickerInputWithCustomValidator, + [MatNativeDateModule], undefined, undefined, [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(StandardDatepicker, [MatNativeDateModule]); + fixture.detectChanges(); + + const minDate = new Date(2019, 0, 1); + const spy = jasmine.createSpy('stateChanges spy'); + const subscription = fixture.componentInstance.datepickerInput.stateChanges.subscribe(spy); + + fixture.componentInstance.min = minDate; + fixture.detectChanges(); + expect(spy).toHaveBeenCalledTimes(1); + + fixture.componentInstance.min = 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(StandardDatepicker, [MatNativeDateModule]); + fixture.detectChanges(); + + const maxDate = new Date(2120, 0, 1); + const spy = jasmine.createSpy('stateChanges spy'); + const subscription = fixture.componentInstance.datepickerInput.stateChanges.subscribe(spy); + + fixture.componentInstance.max = maxDate; + fixture.detectChanges(); + expect(spy).toHaveBeenCalledTimes(1); + + fixture.componentInstance.max = new Date(maxDate); + fixture.detectChanges(); + expect(spy).toHaveBeenCalledTimes(1); + + subscription.unsubscribe(); + }); + }); /** @@ -1975,7 +2057,7 @@ const inputFixedWidthStyles = ` @Component({ template: ` - + ; @ViewChild(MatDatepickerInput) datepickerInput: MatDatepickerInput; xPosition: DatepickerDropdownPositionX; @@ -2289,3 +2373,30 @@ class DatepickerToggleWithNoDatepicker {} `, }) class DatepickerInputWithNoDatepicker {} + + +@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: ` + + + ` +}) +class DatepickerInputWithCustomValidator { + @ViewChild(CustomValidator) validator: CustomValidator; + value: Date; + min: Date; + max: Date; +} diff --git a/tools/public_api_guard/material/datepicker.d.ts b/tools/public_api_guard/material/datepicker.d.ts index e708ffcf6d67..b2547244d41e 100644 --- a/tools/public_api_guard/material/datepicker.d.ts +++ b/tools/public_api_guard/material/datepicker.d.ts @@ -292,7 +292,6 @@ export declare class MatDateRangeInput implements MatFormFieldControl; _groupDisabled: boolean; _startInput: MatStartDate; - _stateChanges: Subject; comparisonEnd: D | null; comparisonStart: D | null; controlType: string; @@ -329,7 +328,7 @@ export declare class MatDateRangeInput implements MatFormFieldControl