fix(dialog): backdrop not being removed if it doesn't have transitions #1716
Conversation
googlebot
added
the
cla: yes
| backdropToDetach.classList.remove('md-overlay-backdrop-showing'); | ||
| backdropToDetach.classList.remove(this._state.backdropClass); | ||
| backdropToDetach.addEventListener('transitionend', () => { | ||
| let styles = getComputedStyle(backdropToDetach); |
There was a problem hiding this comment.
Instead of using getComputedStyle, let's just set pointer-events: none on the backdrop and dismiss it after whichever comes frist: transitionend or a short wait (say, 300ms). This will ensure that the backdrop doesn't get in the way of doing anything and that it always gets removed.
There was a problem hiding this comment.
Will do, although from my testing the getComputedStyle only takes 1-3ms on IE11 and Edge (it's even faster on other browsers). It may be different on a more complex page, though.
crisbeto
force-pushed
the
1607/backdrop-transition-none
branch
from
e593a8d
to
0b8b5dc
Compare
|
Updated @jelbourn. I went with a 500ms timeout, because the default transition is 400ms.
|
crisbeto
force-pushed
the
1607/backdrop-transition-none
branch
from
0b8b5dc
to
c68ea6e
Compare
Fixes the dialog's backdrop not being removed if it's transition have been disabled. Fixes angular#1607.
crisbeto
force-pushed
the
1607/backdrop-transition-none
branch
from
c68ea6e
to
714a91a
Compare
| // If the backdrop doesn't have a transition, the `transitionend` event won't fire. | ||
| // In this case we make it unclickable and we try to remove it after a delay. | ||
| backdropToDetach.style.pointerEvents = 'none'; | ||
| setTimeout(onTransitionEnd, 500); |
There was a problem hiding this comment.
I'm not sure if timeouts are reliable. What if at some point the transition timing is changed?
There was a problem hiding this comment.
When I initially opened the PR, it was using getComputedStyle, however it can cause performance issues in some slower browsers.
There was a problem hiding this comment.
Ah, forgot to mention that it would probably make more sense to switch this over to Angular's animation API. This fix should suffice for now, though.
There was a problem hiding this comment.
Just wanted to point out that there might be a better way of handling this, but if this can make it to the next alpha I guess it works 👍
| backdropToDetach.classList.remove(this._state.backdropClass); | ||
| backdropToDetach.addEventListener('transitionend', () => { | ||
| backdropToDetach.parentNode.removeChild(backdropToDetach); | ||
| let onTransitionEnd = () => { |
There was a problem hiding this comment.
Since this can be used in two ways now, we should rename it to something like finishDetach
| }; | ||
|
|
||
| backdropToDetach.classList.remove('md-overlay-backdrop-showing'); | ||
| backdropToDetach.classList.remove(this._state.backdropClass); |
There was a problem hiding this comment.
Do we need to remove the backdropClass? Wouldn't this potentially cause the backdrop to appear in cases where we use the class to hide it?
There was a problem hiding this comment.
That's the custom class that can be added to the backdrop. It has been handled like that since 0b54668, I only moved it around.
|
|
||
| // If the backdrop doesn't have a transition, the `transitionend` event won't fire. | ||
| // In this case we make it unclickable and we try to remove it after a delay. | ||
| backdropToDetach.style.pointerEvents = 'none'; |
There was a problem hiding this comment.
Need unit test to assert that pointerEvents is set.
|
Updated with a unit test for the |
|
LGTM |
jelbourn
added
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. |
Fixes the dialog's backdrop not being removed if it's transition have been disabled.
Fixes #1607.