Skip to content

Commit 612e446

Browse files
committed
fix(material/core): ripples persisting when container is removed from DOM while fading-in
Follow-up to 65fb5f4. We recently changed how animations are handled in the ripple. As part of this change we accidentally broke the select <> ripple interaction. The select is special because the option panel is removed from the DOM as soon as an option got clicked. An option click will trigger a ripple that will usually still fade-in when the panel is removed. Currently such ripples never will complete because the `transitionend` event does not fire once removed from DOM. We should destroy the ripples when the transition is canceled (e.g. through DOM removal).
1 parent 8a12da7 commit 612e446

File tree

10 files changed

+137
-2
lines changed

10 files changed

+137
-2
lines changed

src/e2e-app/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ ng_module(
7070
"//src/material/progress-bar",
7171
"//src/material/progress-spinner",
7272
"//src/material/radio",
73+
"//src/material/select",
7374
"//src/material/sidenav",
7475
"//src/material/slide-toggle",
7576
"//src/material/tabs",

src/e2e-app/e2e-app/e2e-app-layout.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ export class E2eAppLayout {
2727
{path: 'progress-bar', title: 'Progress bar'},
2828
{path: 'progress-spinner', title: 'Progress Spinner'},
2929
{path: 'radio', title: 'Radios'},
30+
{path: 'select', title: 'Select'},
3031
{path: 'sidenav', title: 'Sidenav'},
3132
{path: 'slide-toggle', title: 'Slide Toggle'},
3233
{path: 'stepper', title: 'Stepper'},

src/e2e-app/main-module.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import {NgModule} from '@angular/core';
22
import {BrowserModule} from '@angular/platform-browser';
3-
import {NoopAnimationsModule} from '@angular/platform-browser/animations';
3+
import {BrowserAnimationsModule} from '@angular/platform-browser/animations';
44
import {RouterModule} from '@angular/router';
55
import {BlockScrollStrategyE2eModule} from './block-scroll-strategy/block-scroll-strategy-e2e-module';
66
import {ButtonToggleE2eModule} from './button-toggle/button-toggle-e2e-module';
@@ -41,11 +41,14 @@ import {VirtualScrollE2eModule} from './virtual-scroll/virtual-scroll-e2e-module
4141
import {MdcProgressBarE2eModule} from './mdc-progress-bar/mdc-progress-bar-e2e-module';
4242
import {MdcProgressSpinnerE2eModule} from './mdc-progress-spinner/mdc-progress-spinner-module';
4343

44+
/** We allow for animations to be explicitly enabled in certain e2e tests. */
45+
const enableAnimations = window.location.search.includes('animations=true');
46+
4447
@NgModule({
4548
imports: [
4649
BrowserModule,
4750
E2eAppModule,
48-
NoopAnimationsModule,
51+
BrowserAnimationsModule.withConfig({disableAnimations: !enableAnimations}),
4952
RouterModule.forRoot(E2E_APP_ROUTES),
5053

5154
// E2E demos

src/e2e-app/routes.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ import {BasicTabs} from './tabs/tabs-e2e';
3636
import {ToolbarE2e} from './toolbar/toolbar-e2e';
3737
import {VirtualScrollE2E} from './virtual-scroll/virtual-scroll-e2e';
3838
import {Home} from './e2e-app/e2e-app-layout';
39+
import {SelectE2e} from './select/select-e2e';
3940

4041
export const E2E_APP_ROUTES: Routes = [
4142
{path: '', component: Home},
@@ -70,6 +71,7 @@ export const E2E_APP_ROUTES: Routes = [
7071
{path: 'progress-spinner', component: ProgressSpinnerE2E},
7172
{path: 'radio', component: SimpleRadioButtons},
7273
{path: 'sidenav', component: SidenavE2E},
74+
{path: 'select', component: SelectE2e},
7375
{path: 'slide-toggle', component: SlideToggleE2E},
7476
{path: 'stepper', component: StepperE2e},
7577
{path: 'tabs', component: BasicTabs},
Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import {NgModule} from '@angular/core';
10+
import {ExampleViewerModule} from '../example-viewer/example-viewer-module';
11+
import {SelectE2e} from './select-e2e';
12+
13+
@NgModule({
14+
imports: [ExampleViewerModule],
15+
declarations: [SelectE2e],
16+
})
17+
export class SelectE2eModule {}

src/e2e-app/select/select-e2e.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import {Component} from '@angular/core';
10+
11+
@Component({
12+
selector: 'select-demo',
13+
template: `<example-list-viewer [ids]="examples"></example-list-viewer>`,
14+
})
15+
export class SelectE2e {
16+
examples = ['select-overview'];
17+
}

src/material/core/ripple/ripple-renderer.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,10 @@ export class RippleRenderer implements EventListenerObject {
176176
if (!animationForciblyDisabledThroughCss && (enterDuration || animationConfig.exitDuration)) {
177177
this._ngZone.runOutsideAngular(() => {
178178
ripple.addEventListener('transitionend', () => this._finishRippleTransition(rippleRef));
179+
// If the transition is cancelled (e.g. due to DOM removal), we destroy the ripple
180+
// directly as otherwise we would keep it part of the ripple container forever.
181+
// https://www.w3.org/TR/css-transitions-1/#:~:text=no%20longer%20in%20the%20document.
182+
ripple.addEventListener('transitioncancel', () => this._destroyRipple(rippleRef));
179183
});
180184
}
181185

src/material/core/ripple/ripple.spec.ts

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ describe('MatRipple', () => {
4545
RippleContainerWithNgIf,
4646
RippleCssTransitionNone,
4747
RippleCssTransitionDurationZero,
48+
RippleWithDomRemovalOnClick,
4849
],
4950
});
5051
});
@@ -802,6 +803,33 @@ describe('MatRipple', () => {
802803
dispatchMouseEvent(rippleTarget, 'mouseup');
803804
expect(rippleTarget.querySelectorAll('.mat-ripple-element').length).toBe(0);
804805
});
806+
807+
it('should destroy the ripple if the transition is being canceled due to DOM removal', async () => {
808+
fixture = TestBed.createComponent(RippleWithDomRemovalOnClick);
809+
fixture.detectChanges();
810+
811+
rippleTarget = fixture.nativeElement.querySelector('.mat-ripple');
812+
813+
dispatchMouseEvent(rippleTarget, 'mousedown');
814+
dispatchMouseEvent(rippleTarget, 'mouseup');
815+
dispatchMouseEvent(rippleTarget, 'click');
816+
817+
const fadingRipple = rippleTarget.querySelector('.mat-ripple-element');
818+
expect(fadingRipple).not.toBe(null);
819+
820+
// The ripple animation is still on-going but the element is now removed from DOM as
821+
// part of the change detecton (given `show` being set to `false` on click)
822+
fixture.detectChanges();
823+
824+
// The `transitioncancel` event is emitted when a CSS transition is canceled due
825+
// to e.g. DOM removal. More details in the CSS transitions spec:
826+
// https://www.w3.org/TR/css-transitions-1/#:~:text=no%20longer%20in%20the%20document.
827+
dispatchFakeEvent(fadingRipple!, 'transitioncancel');
828+
829+
// There should be no ripple element anymore because the fading-in ripple from
830+
// before had its animation canceled due the DOM removal.
831+
expect(rippleTarget.querySelector('.mat-ripple-element')).toBeNull();
832+
});
805833
});
806834
});
807835

@@ -866,3 +894,14 @@ class RippleCssTransitionNone {}
866894
encapsulation: ViewEncapsulation.None,
867895
})
868896
class RippleCssTransitionDurationZero {}
897+
898+
@Component({
899+
template: `
900+
<div *ngIf="show" (click)="show = false" matRipple>
901+
Click to remove this element.
902+
</div>
903+
`,
904+
})
905+
class RippleWithDomRemovalOnClick {
906+
show = true;
907+
}

src/material/select/BUILD.bazel

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
1+
load("//src/e2e-app:test_suite.bzl", "e2e_test_suite")
12
load(
23
"//tools:defaults.bzl",
34
"markdown_to_html",
5+
"ng_e2e_test_library",
46
"ng_module",
57
"ng_test_library",
68
"ng_web_test_suite",
@@ -78,6 +80,19 @@ ng_web_test_suite(
7880
deps = [":unit_test_sources"],
7981
)
8082

83+
ng_e2e_test_library(
84+
name = "e2e_test_sources",
85+
srcs = glob(["**/*.e2e.spec.ts"]),
86+
deps = [
87+
"//src/cdk/testing/private/e2e",
88+
],
89+
)
90+
91+
e2e_test_suite(
92+
name = "e2e_tests",
93+
deps = [":e2e_test_sources"],
94+
)
95+
8196
markdown_to_html(
8297
name = "overview",
8398
srcs = [":select.md"],
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
import {browser, by, element, ExpectedConditions} from 'protractor';
2+
import {getElement} from '../../cdk/testing/private/e2e';
3+
4+
const presenceOf = ExpectedConditions.presenceOf;
5+
const not = ExpectedConditions.not;
6+
7+
describe('select', () => {
8+
beforeEach(async () => await browser.get('/select?animations=true'));
9+
10+
// Regression test which ensures that ripples within the select are not persisted
11+
// accidentally. This could happen because the select panel is removed from DOM
12+
// immediately when an option is clicked. Usually ripples still fade-in at that point.
13+
it('should not accidentally persist ripples', async () => {
14+
const select = getElement('.mat-select');
15+
const options = element.all(by.css('.mat-option'));
16+
const ripples = element.all(by.css('.mat-ripple-element'));
17+
18+
// Wait for select to be rendered.
19+
await browser.wait(presenceOf(select));
20+
21+
// Opent the select and wait for options to be displayed.
22+
await select.click();
23+
await browser.wait(presenceOf(options.get(0)));
24+
25+
// Click the first option and wait for the select to be closed.
26+
await options.get(0).click();
27+
await browser.wait(not(presenceOf(options.get(0))));
28+
29+
// Re-open the select and wait for it to be rendered.
30+
await select.click();
31+
await browser.wait(presenceOf(options.get(0)));
32+
33+
// Expect no ripples to be showing up without an option click.
34+
expect(await ripples.isPresent()).toBe(false);
35+
});
36+
});

0 commit comments

Comments
 (0)