Skip to content

Commit

Permalink
fix(datepicker): focus handling performance regression
Browse files Browse the repository at this point in the history
Removes unnecessary model updates and change detections when focus moves
inside the datepicker
  • Loading branch information
maxokorokov committed Dec 13, 2018
1 parent d033ead commit 1d9a84e
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 21 deletions.
13 changes: 13 additions & 0 deletions src/datepicker/datepicker-service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,19 @@ describe('ngb-datepicker-service', () => {
expect(mock.onNext).toHaveBeenCalledTimes(1);
});

it(`should not rebuild months when focus visibility changes`, () => {
service.focus(new NgbDate(2017, 5, 1));
expect(model.focusVisible).toEqual(false);
expect(model.months.length).toBe(1);
const month = model.months[0];
const date = month.weeks[0].days[0].date;

service.focusVisible = true;
expect(model.focusVisible).toEqual(true);
expect(model.months[0]).toBe(month);
expect(getDay(0).date).toBe(date);
});

});

describe(`navigation`, () => {
Expand Down
5 changes: 5 additions & 0 deletions src/datepicker/datepicker-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,11 @@ export class NgbDatepickerService {
startDate = state.selectedDate;
}

// terminate early if only focus visibility was changed
if ('focusVisible' in patch) {
return state;
}

// focus date changed
if ('focusDate' in patch) {
state.focusDate = checkDateInRange(state.focusDate, state.minDate, state.maxDate);
Expand Down
2 changes: 1 addition & 1 deletion src/datepicker/datepicker.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1133,7 +1133,7 @@ describe('ngb-datepicker', () => {
});

it('should initialize inputs with provided config as provider', () => {
const fixture = createGenericTestComponent('', NgbDatepicker);
const fixture = TestBed.createComponent(NgbDatepicker);

const datepicker = fixture.componentInstance;
expectSameValues(datepicker, config);
Expand Down
55 changes: 35 additions & 20 deletions src/datepicker/datepicker.ts
Original file line number Diff line number Diff line change
@@ -1,23 +1,24 @@
import {Subscription} from 'rxjs';
import {take} from 'rxjs/operators';
import {fromEvent, merge, Subject} from 'rxjs';
import {filter, take, takeUntil} from 'rxjs/operators';
import {
ChangeDetectionStrategy,
ChangeDetectorRef,
Component,
ElementRef,
EventEmitter,
forwardRef,
Input,
NgZone,
OnChanges,
TemplateRef,
forwardRef,
OnDestroy,
OnInit,
SimpleChanges,
EventEmitter,
Output,
OnDestroy,
ElementRef,
NgZone,
SimpleChanges,
TemplateRef,
ViewChild,
ViewEncapsulation
} from '@angular/core';
import {NG_VALUE_ACCESSOR, ControlValueAccessor} from '@angular/forms';
import {ControlValueAccessor, NG_VALUE_ACCESSOR} from '@angular/forms';
import {NgbCalendar} from './ngb-calendar';
import {NgbDate} from './ngb-date';
import {NgbDatepickerService} from './datepicker-service';
Expand All @@ -29,6 +30,7 @@ import {NgbDateAdapter} from './adapters/ngb-date-adapter';
import {NgbDateStruct} from './ngb-date-struct';
import {NgbDatepickerI18n} from './datepicker-i18n';
import {isChangedDate} from './datepicker-tools';
import {hasClassName} from '../util/util';

const NGB_DATEPICKER_VALUE_ACCESSOR = {
provide: NG_VALUE_ACCESSOR,
Expand Down Expand Up @@ -85,7 +87,7 @@ export interface NgbDatepickerNavigateEvent {
</ngb-datepicker-navigation>
</div>
<div class="ngb-dp-months" (keydown)="onKeyDown($event)" (focusin)="showFocus(true)" (focusout)="showFocus(false)">
<div #months class="ngb-dp-months" (keydown)="onKeyDown($event)">
<ng-template ngFor let-month [ngForOf]="model.months" let-i="index">
<div class="ngb-dp-month">
<div *ngIf="navigation === 'none' || (displayMonths > 1 && navigation === 'select')"
Expand All @@ -111,9 +113,10 @@ export class NgbDatepicker implements OnDestroy,
OnChanges, OnInit, ControlValueAccessor {
model: DatepickerViewModel;

@ViewChild('months') private _monthsEl: ElementRef<HTMLElement>;
private _controlValue: NgbDate;
private _subscription: Subscription;
private _selectSubscription: Subscription;
private _destroyed$ = new Subject<void>();

/**
* Reference for the custom template for the day display
*/
Expand Down Expand Up @@ -214,9 +217,9 @@ export class NgbDatepicker implements OnDestroy,
'maxDate', 'navigation', 'outsideDays', 'showWeekdays', 'showWeekNumbers', 'startDate']
.forEach(input => this[input] = config[input]);

this._selectSubscription = _service.select$.subscribe(date => { this.select.emit(date); });
_service.select$.pipe(takeUntil(this._destroyed$)).subscribe(date => { this.select.emit(date); });

this._subscription = _service.model$.subscribe(model => {
_service.model$.pipe(takeUntil(this._destroyed$)).subscribe(model => {
const newDate = model.firstDate;
const oldDate = this.model ? this.model.firstDate : null;
const newSelectedDate = model.selectedDate;
Expand Down Expand Up @@ -271,11 +274,25 @@ export class NgbDatepicker implements OnDestroy,
this._service.open(NgbDate.from(date ? date.day ? date as NgbDateStruct : {...date, day: 1} : null));
}

ngOnDestroy() {
this._subscription.unsubscribe();
this._selectSubscription.unsubscribe();
ngAfterContentInit() {
this._ngZone.runOutsideAngular(() => {
const focusIns$ = fromEvent<FocusEvent>(this._monthsEl.nativeElement, 'focusin');
const focusOuts$ = fromEvent<FocusEvent>(this._monthsEl.nativeElement, 'focusout');

// we're changing 'focusVisible' only when entering or leaving months view
// and ignoring all focus events where both 'target' and 'related' target are day cells
merge(focusIns$, focusOuts$)
.pipe(
filter(
({target, relatedTarget}) =>
!(hasClassName(target, 'ngb-dp-day') && hasClassName(relatedTarget, 'ngb-dp-day'))),
takeUntil(this._destroyed$))
.subscribe(({type}) => this._ngZone.run(() => this._service.focusVisible = type === 'focusin'));
});
}

ngOnDestroy() { this._destroyed$.next(); }

ngOnInit() {
if (this.model === undefined) {
['dayTemplateData', 'displayMonths', 'markDisabled', 'firstDayOfWeek', 'navigation', 'minDate', 'maxDate',
Expand Down Expand Up @@ -322,8 +339,6 @@ export class NgbDatepicker implements OnDestroy,

setDisabledState(isDisabled: boolean) { this._service.disabled = isDisabled; }

showFocus(focusVisible: boolean) { this._service.focusVisible = focusVisible; }

writeValue(value) {
this._controlValue = NgbDate.from(this._ngbDateAdapter.fromModel(value));
this._service.select(this._controlValue);
Expand Down

0 comments on commit 1d9a84e

Please sign in to comment.