-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -252,6 +252,17 @@ export class MatMonthView<D> implements AfterContentInit, OnChanges, OnDestroy { | |||
this._changeDetectorRef.markForCheck(); | ||||
} | ||||
|
||||
_dateBecomesActive(event: MatCalendarUserEvent<number>) { | ||||
zarend marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
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); | ||||
zarend marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
|
||||
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 | ||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, calling
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.
There is two approaches I can think of, but both required changing the library code (this code)
WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||||
} | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -218,6 +218,21 @@ export class MatMultiYearView<D> implements AfterContentInit, OnDestroy { | |
); | ||
} | ||
|
||
_yearBecomesActive(event: MatCalendarUserEvent<number>) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is nearly identical to the |
||
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; | ||
|
@@ -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(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -198,6 +198,25 @@ export class MatYearView<D> implements AfterContentInit, OnDestroy { | |
); | ||
} | ||
|
||
/** Handles when a new month becomes active. */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is nearly identical to the |
||
_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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would the |
||
} | ||
/** 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 | ||
|
@@ -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(); | ||
|
There was a problem hiding this comment.
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.