Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(material/datepicker): update active date on focus #24172

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/material/datepicker/calendar-body.html
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
[attr.aria-selected]="_isSelected(item.compareValue)"
[attr.aria-current]="todayValue === item.compareValue ? 'date' : null"
(click)="_cellClicked(item, $event)"
(focus)="_cellFocused(item, $event)"
[style.width]="_cellWidth"
[style.paddingTop]="_cellPadding"
[style.paddingBottom]="_cellPadding">
Expand Down
10 changes: 10 additions & 0 deletions src/material/datepicker/calendar-body.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ export class MatCalendarBody implements OnChanges, OnDestroy {
/** Emits when a new value is selected. */
@Output() readonly selectedValueChange = new EventEmitter<MatCalendarUserEvent<number>>();

/** Emits when a new date becomes active. */
@Output() readonly activeValueChange = new EventEmitter<MatCalendarUserEvent<number>>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: might be more accurate to call this activeDateChange to align with the events on the other calendar views.


/** Emits when the preview has changed as a result of a user action. */
@Output() readonly previewChange = new EventEmitter<
MatCalendarUserEvent<MatCalendarCell | null>
Expand Down Expand Up @@ -153,6 +156,13 @@ export class MatCalendarBody implements OnChanges, OnDestroy {
}
}

/** Called when a cell is focused. */
_cellFocused(cell: MatCalendarCell, event: FocusEvent): void {
if (cell.enabled) {
this.activeValueChange.emit({value: cell.value, event});
}
}

/** Returns whether a cell should be marked as selected. */
_isSelected(value: number) {
return this.startValue === value || this.endValue === value;
Expand Down
1 change: 1 addition & 0 deletions src/material/datepicker/month-view.html
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
[labelMinRequiredCells]="3"
[activeCell]="_dateAdapter.getDate(activeDate) - 1"
(selectedValueChange)="_dateSelected($event)"
(activeValueChange)="_dateBecomesActive($event)"
(previewChange)="_previewChanged($event)"
(keyup)="_handleCalendarBodyKeyup($event)"
(keydown)="_handleCalendarBodyKeydown($event)">
Expand Down
18 changes: 18 additions & 0 deletions src/material/datepicker/month-view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,17 @@ export class MatMonthView<D> implements AfterContentInit, OnChanges, OnDestroy {
this._changeDetectorRef.markForCheck();
}

_dateBecomesActive(event: MatCalendarUserEvent<number>) {
const date = event.value;
const activeYear = this._dateAdapter.getYear(this.activeDate);
const activeMonth = this._dateAdapter.getMonth(this.activeDate);
const activeDate = this._dateAdapter.createDate(activeYear, activeMonth, date);

if (!this._dateAdapter.sameDate(activeDate, this._activeDate)) {
this.activeDateChange.emit(activeDate);
}
}

/** Handles keydown events on the calendar body when calendar is in month view. */
_handleCalendarBodyKeydown(event: KeyboardEvent): void {
// TODO(mmalerba): We currently allow keyboard navigation to disabled dates, but just prevent
Expand Down Expand Up @@ -329,7 +340,14 @@ export class MatMonthView<D> implements AfterContentInit, OnChanges, OnDestroy {
this.activeDateChange.emit(this.activeDate);
}

if (!event.isTrusted) {
// Manually triggered events in unit tests do not trigger change detection.
this._changeDetectorRef.detectChanges();
}
Comment on lines +343 to +346
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't really the right approach to this issue. The unit test should directly call detectChanges- library code (this code) shouldn't add additional behaviors explosively to make unit tests pass. Having the tests call detectChanges in tandem with emitting the fake event is the normal/standard way of handling this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, calling detectChanges in the unit test doesn't work in this situation because the microtask queue is flushing before change detection runs. the child component (MatCalendarBody) needs to received the updated data from this.activeDate before it can focus on the correct cell in this method

_focusActiveCell(movePreview = true) {
.

Here's an explanation I wrote up earlier which seems to have gotten buried in the PR comments.

I just pushed a change to only call detectChanges if the event is not trusted. This is only needed for the tests because fake keyboard events do not trigger change detection. I'm generally not a fan of having code that's only intended to run in the tests, and I'm curious how we usually handle this situation.

Calling detectChanges in the test code doesn't work because by that time the calendar body will have already triggered the activeDateChange event with a stale value. This wasn't a problem until now, because before focusing on a stale date wouldn't trigger the activeDateChange event to update the active date. Calling detectChanges in the test code would case the calendar body to focus again but on a updated value.

Here's a rough timeline of what happens in the unit tests if we don't call detectChanges when handling the keydown event.
0. (active Date is Jan 12 2021)

  1. trigger a fake keyboard event with the right arrow key
  2. MatMonthView sets this.activeDate to Jan 13 2021
  3. MatMonthView calls _focusActiveDate
  4. MatCalendarBody calls .focus on Jan 12, 2021 date then triggers an activeDateChange Event with Jan 12, 2021
  5. test code calls detectChanges
  6. month-view receives sets the active date to Jan 12, 2021
  7. (active date is Jan 12, but it should be Jan 13, 2021).

There is two approaches I can think of, but both required changing the library code (this code)

  1. call detect changes in library code (this code) only if the event is not trusted
  2. add a timeOut to _focusActiveCell so that the microtask queue does not get flushed until we manually flush it in the test code.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is too difficult to handle testing these in unit tests, I would suggest just test these cases in our e2e tests with real keyboard events


// Focuses the active cell after the child component recieves the updated data from `this.activeDate`.
this._focusActiveCell();

// Prevent unexpected default actions such as form submission.
event.preventDefault();
}
Expand Down
1 change: 1 addition & 0 deletions src/material/datepicker/multi-year-view.html
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
[cellAspectRatio]="4 / 7"
[activeCell]="_getActiveCell()"
(selectedValueChange)="_yearSelected($event)"
(activeValueChange)="_yearBecomesActive($event)"
(keyup)="_handleCalendarBodyKeyup($event)"
(keydown)="_handleCalendarBodyKeydown($event)">
</tbody>
Expand Down
21 changes: 21 additions & 0 deletions src/material/datepicker/multi-year-view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,21 @@ export class MatMultiYearView<D> implements AfterContentInit, OnDestroy {
);
}

_yearBecomesActive(event: MatCalendarUserEvent<number>) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nearly identical to the _yearSelected method above. I'm open to suggestions on how to refactor this, or if we should refactor it to share code.

const year = event.value;
let month = this._dateAdapter.getMonth(this.activeDate);
let daysInMonth = this._dateAdapter.getNumDaysInMonth(
this._dateAdapter.createDate(year, month, 1),
);
this.activeDateChange.emit(
this._dateAdapter.createDate(
year,
month,
Math.min(this._dateAdapter.getDate(this.activeDate), daysInMonth),
),
);
}

/** Handles keydown events on the calendar body when calendar is in multi-year view. */
_handleCalendarBodyKeydown(event: KeyboardEvent): void {
const oldActiveDate = this._activeDate;
Expand Down Expand Up @@ -278,6 +293,12 @@ export class MatMultiYearView<D> implements AfterContentInit, OnDestroy {
this.activeDateChange.emit(this.activeDate);
}

if (!event.isTrusted) {
// Manually triggered events in unit tests do not trigger change detection.
this._changeDetectorRef.detectChanges();
}

// Focuses the active cell after the child component recieves the updated data from `this.activeDate`.
this._focusActiveCell();
// Prevent unexpected default actions such as form submission.
event.preventDefault();
Expand Down
1 change: 1 addition & 0 deletions src/material/datepicker/year-view.html
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
[cellAspectRatio]="4 / 7"
[activeCell]="_dateAdapter.getMonth(activeDate)"
(selectedValueChange)="_monthSelected($event)"
(activeValueChange)="_monthBecomesActive($event)"
(keyup)="_handleCalendarBodyKeyup($event)"
(keydown)="_handleCalendarBodyKeydown($event)">
</tbody>
Expand Down
25 changes: 25 additions & 0 deletions src/material/datepicker/year-view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,25 @@ export class MatYearView<D> implements AfterContentInit, OnDestroy {
);
}

/** Handles when a new month becomes active. */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nearly identical to the _monthSelected method above. I'm open to suggestions on how to refactor this, or if we should refactor it to share code.

_monthBecomesActive(event: MatCalendarUserEvent<number>) {
const month = event.value;
const normalizedDate = this._dateAdapter.createDate(
this._dateAdapter.getYear(this.activeDate),
month,
1,
);

const daysInMonth = this._dateAdapter.getNumDaysInMonth(normalizedDate);

this.activeDateChange.emit(
this._dateAdapter.createDate(
this._dateAdapter.getYear(this.activeDate),
month,
Math.min(this._dateAdapter.getDate(this.activeDate), daysInMonth),
),
);
Comment on lines +212 to +218
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the clampDate method on DateAdapter be a simpler way of implementing this?

}
/** Handles keydown events on the calendar body when calendar is in year view. */
_handleCalendarBodyKeydown(event: KeyboardEvent): void {
// TODO(mmalerba): We currently allow keyboard navigation to disabled dates, but just prevent
Expand Down Expand Up @@ -261,6 +280,12 @@ export class MatYearView<D> implements AfterContentInit, OnDestroy {
this.activeDateChange.emit(this.activeDate);
}

if (!event.isTrusted) {
// Manually triggered events in unit tests do not trigger change detection.
this._changeDetectorRef.detectChanges();
}

// Focuses the active cell after the child component recieves the updated data from `this.activeDate`.
this._focusActiveCell();
// Prevent unexpected default actions such as form submission.
event.preventDefault();
Expand Down
9 changes: 8 additions & 1 deletion tools/public_api_guard/material/datepicker.md
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,10 @@ export class MatCalendar<D> implements AfterContentInit, AfterViewChecked, OnDes
export class MatCalendarBody implements OnChanges, OnDestroy {
constructor(_elementRef: ElementRef<HTMLElement>, _ngZone: NgZone);
activeCell: number;
readonly activeValueChange: EventEmitter<MatCalendarUserEvent<number>>;
cellAspectRatio: number;
_cellClicked(cell: MatCalendarCell, event: MouseEvent): void;
_cellFocused(cell: MatCalendarCell, event: FocusEvent): void;
_cellPadding: string;
_cellWidth: string;
comparisonEnd: number | null;
Expand Down Expand Up @@ -239,7 +241,7 @@ export class MatCalendarBody implements OnChanges, OnDestroy {
startValue: number;
todayValue: number;
// (undocumented)
static ɵcmp: i0.ɵɵComponentDeclaration<MatCalendarBody, "[mat-calendar-body]", ["matCalendarBody"], { "label": "label"; "rows": "rows"; "todayValue": "todayValue"; "startValue": "startValue"; "endValue": "endValue"; "labelMinRequiredCells": "labelMinRequiredCells"; "numCols": "numCols"; "activeCell": "activeCell"; "isRange": "isRange"; "cellAspectRatio": "cellAspectRatio"; "comparisonStart": "comparisonStart"; "comparisonEnd": "comparisonEnd"; "previewStart": "previewStart"; "previewEnd": "previewEnd"; }, { "selectedValueChange": "selectedValueChange"; "previewChange": "previewChange"; }, never, never>;
static ɵcmp: i0.ɵɵComponentDeclaration<MatCalendarBody, "[mat-calendar-body]", ["matCalendarBody"], { "label": "label"; "rows": "rows"; "todayValue": "todayValue"; "startValue": "startValue"; "endValue": "endValue"; "labelMinRequiredCells": "labelMinRequiredCells"; "numCols": "numCols"; "activeCell": "activeCell"; "isRange": "isRange"; "cellAspectRatio": "cellAspectRatio"; "comparisonStart": "comparisonStart"; "comparisonEnd": "comparisonEnd"; "previewStart": "previewStart"; "previewEnd": "previewEnd"; }, { "selectedValueChange": "selectedValueChange"; "activeValueChange": "activeValueChange"; "previewChange": "previewChange"; }, never, never>;
// (undocumented)
static ɵfac: i0.ɵɵFactoryDeclaration<MatCalendarBody, never>;
}
Expand Down Expand Up @@ -759,6 +761,8 @@ export class MatMonthView<D> implements AfterContentInit, OnChanges, OnDestroy {
comparisonStart: D | null;
// (undocumented)
_dateAdapter: DateAdapter<D>;
// (undocumented)
_dateBecomesActive(event: MatCalendarUserEvent<number>): void;
dateClass: MatCalendarCellClassFunction<D>;
dateFilter: (date: D) => boolean;
_dateSelected(event: MatCalendarUserEvent<number>): void;
Expand Down Expand Up @@ -831,6 +835,8 @@ export class MatMultiYearView<D> implements AfterContentInit, OnDestroy {
readonly selectedChange: EventEmitter<D>;
_selectedYear: number | null;
_todayYear: number;
// (undocumented)
_yearBecomesActive(event: MatCalendarUserEvent<number>): void;
_years: MatCalendarCell[][];
readonly yearSelected: EventEmitter<D>;
_yearSelected(event: MatCalendarUserEvent<number>): void;
Expand Down Expand Up @@ -907,6 +913,7 @@ export class MatYearView<D> implements AfterContentInit, OnDestroy {
set maxDate(value: D | null);
get minDate(): D | null;
set minDate(value: D | null);
_monthBecomesActive(event: MatCalendarUserEvent<number>): void;
_months: MatCalendarCell[][];
readonly monthSelected: EventEmitter<D>;
_monthSelected(event: MatCalendarUserEvent<number>): void;
Expand Down