fix(ripple): Always remove ripple after a certain period #1915
Conversation
googlebot
added
the
cla: yes
tinayuangao
added
pr: needs review
and removed
cla: yes
| @@ -143,6 +143,8 @@ export class RippleRenderer { | |||
|
|
|||
| rippleDiv.addEventListener('transitionend', | |||
| (event: TransitionEvent) => transitionEndCallback(ripple, event)); | |||
| // Remove the ripple after transitions (fade-in, transform, fade-out) | |||
| setTimeout(() => this.removeRippleFromDom(ripple.rippleElement), fadeInSeconds * 3 * 1000); | |||
There was a problem hiding this comment.
This will require all related tests to be async, right? @jelbourn Should we run this outside angular zone?
| @@ -143,6 +143,8 @@ export class RippleRenderer { | |||
|
|
|||
| rippleDiv.addEventListener('transitionend', | |||
| (event: TransitionEvent) => transitionEndCallback(ripple, event)); | |||
| // Remove the ripple after transitions (fade-in, transform, fade-out) | |||
| setTimeout(() => this.removeRippleFromDom(ripple.rippleElement), fadeInSeconds * 3 * 1000); | |||
There was a problem hiding this comment.
Would it solve the problem to remove the ripple element in ngOnDestroy instead of using a timeout?
There was a problem hiding this comment.
Sometimes the element is only hidden, or display: none. In this case ngOnDestroy is never called.
If we use *ngIf to remove the element, the ripple will be removed when it's removed from the DOM tree.
googlebot
added
the
cla: yes
| @@ -143,6 +143,8 @@ export class RippleRenderer { | |||
|
|
|||
| rippleDiv.addEventListener('transitionend', | |||
| (event: TransitionEvent) => transitionEndCallback(ripple, event)); | |||
| // Remove the ripple after transitions (fade-in, transform, fade-out) | |||
| setTimeout(() => this.removeRippleFromDom(ripple.rippleElement), fadeInSeconds * 3 * 1000); | |||
There was a problem hiding this comment.
// Ensure that ripples are always removed, even when transitionend doesn't fire.
// Run this outside the Angular zone because there's nothing that Angular cares about.
// If it were to run inside the Angular zone, every test that used ripples would have to be
// either async or fakeAsync.
this._ngZone.runOutsideAngular(() => {
// The ripple lasts a time equal to the sum of fade-in, transform, and fade-out (3 * fade-in time).
let rippleDuration = fadeInSeconds * 3 * 1000;
setTimeout(() => this.removeRippleFromDom(ripple.rippleElement), rippleDuration);
});There was a problem hiding this comment.
Done. PTAL.
Thanks for review!
tinayuangao
force-pushed
the
ripple-detach
branch
from
908862e
to
559fdb6
Compare
tinayuangao
force-pushed
the
ripple-detach
branch
from
559fdb6
to
ab78c14
Compare
tinayuangao
force-pushed
the
ripple-detach
branch
from
ab78c14
to
a0c30cb
Compare
| rippleElement.click(); | ||
| expect(rippleElement.querySelectorAll('.md-ripple-foreground').length).toBe(1); | ||
|
|
||
| setTimeout(() => { |
There was a problem hiding this comment.
Add a comment like
// Use a real timeout because the ripple's timeout runs outside of the angular zone.
|
Still seems to be failing on IE11... I'll try to take a look today. |
|
Fixed IE11 |
|
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. |
The ripple foreground is not removed if transition is interrupt (e.g. change parent visibility, hidden).
Fixes #1818
R: @kara @jelbourn