-
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(datepicker): unable to close calendar when opened on focus in IE11 #8918
fix(datepicker): unable to close calendar when opened on focus in IE11 #8918
Conversation
this._focusedElementBeforeOpen.focus(); | ||
this._focusedElementBeforeOpen = null; | ||
setTimeout(completeClose); |
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.
Note: I'm not a fan of having to do this timeout, however it's probably the least-breaking option. The alternatives are:
- Adding a one-time
focus
event to the_focusedElementBeforeOpen
that completes the closing logic. This works as well as the timeout, however it makes it a lot harder to flush out during unit tests, because you'd need to know thedocument.activeElement
and you'd need to dispatch a fakefocus
event to it. - Adding a
blur
event on the currently-focused element so we know when focus was moved. Has the same issues as 1. - Scoping the timeout only to IE. The problem is that it makes unit tests inconsistent. Some
fakeAsync
tests would run fine on all browsers, but throw anX timer(s) still in queue
error in IE.
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.
is Promise.resolve().then
too quick for this?
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.
I don't remember trying it, but it should work. I went with the timeout since it would be more obvious what you need to do if your tests start failing. We can switch to the Promise.resolve
if we want to make the sync easier.
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.
I prefer to use it when possible since it executes quicker. I think adding a flush
to your test will fix it in either case
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.
It will, but the Promise.resolve
won't throw the X timers in queue
error which makes it a little harder to track down.
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.
really? so what happens if it never gets flushed, does the test just pass?
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.
Whatever is supposed to execute in the callback will happen after the test. In our case the opened
property on the datepicker would always stay at false.
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.
Oh, that sucks, would have expected fakeAsync
to catch that. Fine with leaving it as setTimeout
then
4f48754
to
e636e02
Compare
Fixes not being able to close a datepicker's calendar in IE11, if the datepicker's trigger opens it on focus. The issue comes down to the fact that all browsers focus elements synchronously, whereas IE does so asynchronously. Since our logic depends on everything firing in sequence, when IE focuses at a later point, the datepicker is already considered as closed which causes the logic that restores focus to the trigger to reopen the calendar. Fixes angular#8914.
e636e02
to
d166b24
Compare
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fixes not being able to close a datepicker's calendar in IE11, if the datepicker's trigger opens it on focus. The issue comes down to the fact that all browsers focus elements synchronously, whereas IE does so asynchronously. Since our logic depends on everything firing in sequence, when IE focuses at a later point, the datepicker is already considered as closed which causes the logic that restores focus to the trigger to reopen the calendar.
Fixes #8914.