fix(dialog): multiple dialogs not opening with disabled animations #6736
Conversation
devversion
requested review from
crisbeto,
jelbourn and
mmalerba
as code owners
googlebot
added
the
cla: yes
devversion
added
in progress
| it('setting selected should update input and close calendar', () => { | ||
| expect(document.querySelector('md-dialog-container')).toBeNull(); | ||
|
|
||
| // The fake async zone expects all active timers to be done at the end of the test. |
There was a problem hiding this comment.
I'm not sure that this kind of comment is necessary for every tick call.
There was a problem hiding this comment.
Yeah totally see what you mean. It's just that it can be confusing if we place a tick(500) after every test.
| // Note: as of Angular 4.3, the animations module seems to fire the `start` callback before | ||
| // the end if animations are disabled. Make this call async to ensure that it still fires | ||
| // at the appropriate time. | ||
| Promise.resolve().then(() => { |
There was a problem hiding this comment.
Add a test for this fix? The current tests are using the NoopAnimationsModule and they didn't catch the issue.
There was a problem hiding this comment.
I looked into a way to reproduce that issue inside of the TestBed, but it looks like the animation callbacks fire correctly there.
I tried to look into the NoopAnimationPlayer from @angular/animations and I assume its related to some timing issues with zone.js
devversion
force-pushed
the
fix/dialog-noop-animation-secondary-dialog
branch
2 times, most recently
from
c09683f
to
9744725
Compare
devversion
added
pr: needs review
and removed
in progress
crisbeto
added
pr: lgtm
action: merge
devversion
added
the
P1
devversion
force-pushed
the
fix/dialog-noop-animation-secondary-dialog
branch
2 times, most recently
from
01fae7a
to
1a49212
Compare
|
Why hasn't this PR been merged and added to the last version ? The PR has been merge-ready for a while.. |
jelbourn
added
the
presubmit failures
|
@mmalerba is working on resolving presubmit failures |
|
The only presubmit issue left is a screenshot test that shows a second copy of the same dialog opened on top of itself. Do we know if this PR is still necessary or if it can be fixed by a change in animations? |
devversion
force-pushed
the
fix/dialog-noop-animation-secondary-dialog
branch
from
1a49212
to
d542bb4
Compare
With SHA 36f708c, second dialogs can be only opened if the first dialog finished animating. Due to the recent update to Angular 4.3, the animation trigger callbacks are firing in a wrong order (done callback fires before start), which causes the _isAnimating property to be set to true while the animation already finished. @crisbeto Not too happy how the tests ended up. Fixes angular#6719
devversion
force-pushed
the
fix/dialog-noop-animation-secondary-dialog
branch
from
d542bb4
to
b13ceca
Compare
|
@mmalerba I juste tested with angular 4.4.4 and angular-material beta 11 and the issue is still present. |
|
Any progress on this issue? It really is blocking our product dev work for IE/Edge browsers. |
|
I'm facing the same issue on Safari (Desktop & Mobile). It's a big issue ! |
jelbourn
added
P2
|
We're going to resolve this issue with #8051 instead |
jelbourn
removed
pr: lgtm
action: merge
|
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. |
With SHA 36f708c, second dialogs can be only opened if the first dialog finished animating.
Due to the recent update to Angular 4.3, the animation trigger callbacks are firing in a wrong order (
donecallback fires beforestart), which causes the_isAnimatingproperty to be set totruewhile the animation already finished.@crisbeto Not too happy how the tests ended up.
Fixes #6719