fix(material/core): ripples persisting when container is removed from DOM while fading-in

.github/CODEOWNERS

@@ -265,6 +265,7 @@
 /src/e2e-app/progress-bar/**                       @andrewseguin @crisbeto
 /src/e2e-app/progress-spinner/**                   @andrewseguin @crisbeto
 /src/e2e-app/radio/**                              @andrewseguin @devversion
+/src/e2e-app/select/**                             @crisbeto
 /src/e2e-app/sidenav/**                            @mmalerba
 /src/e2e-app/slide-toggle/**                       @devversion
 /src/e2e-app/stepper/**                            @mmalerba

src/e2e-app/BUILD.bazel

@@ -70,6 +70,7 @@ ng_module(
         "//src/material/progress-bar",
         "//src/material/progress-spinner",
         "//src/material/radio",
+        "//src/material/select",
         "//src/material/sidenav",
         "//src/material/slide-toggle",
         "//src/material/tabs",

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

@@ -27,6 +27,7 @@ export class E2eAppLayout {
     {path: 'progress-bar', title: 'Progress bar'},
     {path: 'progress-spinner', title: 'Progress Spinner'},
     {path: 'radio', title: 'Radios'},
+    {path: 'select', title: 'Select'},
     {path: 'sidenav', title: 'Sidenav'},
     {path: 'slide-toggle', title: 'Slide Toggle'},
     {path: 'stepper', title: 'Stepper'},

src/e2e-app/main-module.ts

@@ -1,6 +1,6 @@
 import {NgModule} from '@angular/core';
 import {BrowserModule} from '@angular/platform-browser';
-import {NoopAnimationsModule} from '@angular/platform-browser/animations';
+import {BrowserAnimationsModule} from '@angular/platform-browser/animations';
 import {RouterModule} from '@angular/router';
 import {BlockScrollStrategyE2eModule} from './block-scroll-strategy/block-scroll-strategy-e2e-module';
 import {ButtonToggleE2eModule} from './button-toggle/button-toggle-e2e-module';
@@ -41,11 +41,14 @@ import {VirtualScrollE2eModule} from './virtual-scroll/virtual-scroll-e2e-module
 import {MdcProgressBarE2eModule} from './mdc-progress-bar/mdc-progress-bar-e2e-module';
 import {MdcProgressSpinnerE2eModule} from './mdc-progress-spinner/mdc-progress-spinner-module';
 
+/** We allow for animations to be explicitly enabled in certain e2e tests. */
+const enableAnimations = window.location.search.includes('animations=true');
+
 @NgModule({
   imports: [
     BrowserModule,
     E2eAppModule,
-    NoopAnimationsModule,
+    BrowserAnimationsModule.withConfig({disableAnimations: !enableAnimations}),
     RouterModule.forRoot(E2E_APP_ROUTES),
 
     // E2E demos

src/e2e-app/routes.ts

@@ -36,6 +36,7 @@ import {BasicTabs} from './tabs/tabs-e2e';
 import {ToolbarE2e} from './toolbar/toolbar-e2e';
 import {VirtualScrollE2E} from './virtual-scroll/virtual-scroll-e2e';
 import {Home} from './e2e-app/e2e-app-layout';
+import {SelectE2e} from './select/select-e2e';
 
 export const E2E_APP_ROUTES: Routes = [
   {path: '', component: Home},
@@ -70,6 +71,7 @@ export const E2E_APP_ROUTES: Routes = [
   {path: 'progress-spinner', component: ProgressSpinnerE2E},
   {path: 'radio', component: SimpleRadioButtons},
   {path: 'sidenav', component: SidenavE2E},
+  {path: 'select', component: SelectE2e},
   {path: 'slide-toggle', component: SlideToggleE2E},
   {path: 'stepper', component: StepperE2e},
   {path: 'tabs', component: BasicTabs},

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

@@ -0,0 +1,17 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {NgModule} from '@angular/core';
+import {ExampleViewerModule} from '../example-viewer/example-viewer-module';
+import {SelectE2e} from './select-e2e';
+
+@NgModule({
+  imports: [ExampleViewerModule],
+  declarations: [SelectE2e],
+})
+export class SelectE2eModule {}

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

@@ -0,0 +1,17 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {Component} from '@angular/core';
+
+@Component({
+  selector: 'select-demo',
+  template: `<example-list-viewer [ids]="examples"></example-list-viewer>`,
+})
+export class SelectE2e {
+  examples = ['select-overview'];
+}

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

@@ -176,6 +176,10 @@ export class RippleRenderer implements EventListenerObject {
     if (!animationForciblyDisabledThroughCss && (enterDuration || animationConfig.exitDuration)) {
       this._ngZone.runOutsideAngular(() => {
         ripple.addEventListener('transitionend', () => this._finishRippleTransition(rippleRef));
+        // If the transition is cancelled (e.g. due to DOM removal), we destroy the ripple
+        // directly as otherwise we would keep it part of the ripple container forever.
+        // https://www.w3.org/TR/css-transitions-1/#:~:text=no%20longer%20in%20the%20document.
+        ripple.addEventListener('transitioncancel', () => this._destroyRipple(rippleRef));
       });
     }
 
@@ -190,19 +194,8 @@ export class RippleRenderer implements EventListenerObject {
 
   /** Fades out a ripple reference. */
   fadeOutRipple(rippleRef: RippleRef) {
-    const wasActive = this._activeRipples.delete(rippleRef);
-
-    if (rippleRef === this._mostRecentTransientRipple) {
-      this._mostRecentTransientRipple = null;
-    }
-
-    // Clear out the cached bounding rect if we have no more ripples.
-    if (!this._activeRipples.size) {
-      this._containerRect = null;
-    }
-
-    // For ripples that are not active anymore, don't re-run the fade-out animation.
-    if (!wasActive) {
+    // For ripples already fading out or hidden, this should be a noop.
+    if (rippleRef.state === RippleState.FADING_OUT || rippleRef.state === RippleState.HIDDEN) {
       return;
     }
 
@@ -303,6 +296,19 @@ export class RippleRenderer implements EventListenerObject {
 
   /** Destroys the given ripple by removing it from the DOM and updating its state. */
   private _destroyRipple(rippleRef: RippleRef) {
+    this._activeRipples.delete(rippleRef);
+
+    // Clear out the cached bounding rect if we have no more ripples.
+    if (!this._activeRipples.size) {
+      this._containerRect = null;
+    }
+
+    // If the current ref is the most recent transient ripple, unset it
+    // avoid memory leaks.
+    if (rippleRef === this._mostRecentTransientRipple) {
+      this._mostRecentTransientRipple = null;
+    }
+
     rippleRef.state = RippleState.HIDDEN;
     rippleRef.element.remove();
   }

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

@@ -45,6 +45,7 @@ describe('MatRipple', () => {
         RippleContainerWithNgIf,
         RippleCssTransitionNone,
         RippleCssTransitionDurationZero,
+        RippleWithDomRemovalOnClick,
       ],
     });
   });
@@ -802,6 +803,33 @@ describe('MatRipple', () => {
       dispatchMouseEvent(rippleTarget, 'mouseup');
       expect(rippleTarget.querySelectorAll('.mat-ripple-element').length).toBe(0);
     });
+
+    it('should destroy the ripple if the transition is being canceled due to DOM removal', async () => {
+      fixture = TestBed.createComponent(RippleWithDomRemovalOnClick);
+      fixture.detectChanges();
+
+      rippleTarget = fixture.nativeElement.querySelector('.mat-ripple');
+
+      dispatchMouseEvent(rippleTarget, 'mousedown');
+      dispatchMouseEvent(rippleTarget, 'mouseup');
+      dispatchMouseEvent(rippleTarget, 'click');
+
+      const fadingRipple = rippleTarget.querySelector('.mat-ripple-element');
+      expect(fadingRipple).not.toBe(null);
+
+      // The ripple animation is still on-going but the element is now removed from DOM as
+      // part of the change detecton (given `show` being set to `false` on click)
+      fixture.detectChanges();
+
+      // The `transitioncancel` event is emitted when a CSS transition is canceled due
+      // to e.g. DOM removal. More details in the CSS transitions spec:
+      // https://www.w3.org/TR/css-transitions-1/#:~:text=no%20longer%20in%20the%20document.
+      dispatchFakeEvent(fadingRipple!, 'transitioncancel');
+
+      // There should be no ripple element anymore because the fading-in ripple from
+      // before had its animation canceled due the DOM removal.
+      expect(rippleTarget.querySelector('.mat-ripple-element')).toBeNull();
+    });
   });
 });
 
@@ -866,3 +894,14 @@ class RippleCssTransitionNone {}
   encapsulation: ViewEncapsulation.None,
 })
 class RippleCssTransitionDurationZero {}
+
+@Component({
+  template: `
+    <div *ngIf="show" (click)="show = false" matRipple>
+      Click to remove this element.
+    </div>
+  `,
+})
+class RippleWithDomRemovalOnClick {
+  show = true;
+}

src/material/select/BUILD.bazel

@@ -1,6 +1,8 @@
+load("//src/e2e-app:test_suite.bzl", "e2e_test_suite")
 load(
     "//tools:defaults.bzl",
     "markdown_to_html",
+    "ng_e2e_test_library",
     "ng_module",
     "ng_test_library",
     "ng_web_test_suite",
@@ -78,6 +80,19 @@ ng_web_test_suite(
     deps = [":unit_test_sources"],
 )
 
+ng_e2e_test_library(
+    name = "e2e_test_sources",
+    srcs = glob(["**/*.e2e.spec.ts"]),
+    deps = [
+        "//src/cdk/testing/private/e2e",
+    ],
+)
+
+e2e_test_suite(
+    name = "e2e_tests",
+    deps = [":e2e_test_sources"],
+)
+
 markdown_to_html(
     name = "overview",
     srcs = [":select.md"],

src/material/select/select.e2e.spec.ts

@@ -0,0 +1,36 @@
+import {browser, by, element, ExpectedConditions} from 'protractor';
+import {getElement} from '../../cdk/testing/private/e2e';
+
+const presenceOf = ExpectedConditions.presenceOf;
+const not = ExpectedConditions.not;
+
+describe('select', () => {
+  beforeEach(async () => await browser.get('/select?animations=true'));
+
+  // Regression test which ensures that ripples within the select are not persisted
+  // accidentally. This could happen because the select panel is removed from DOM
+  // immediately when an option is clicked. Usually ripples still fade-in at that point.
+  it('should not accidentally persist ripples', async () => {
+    const select = getElement('.mat-select');
+    const options = element.all(by.css('.mat-option'));
+    const ripples = element.all(by.css('.mat-ripple-element'));
+
+    // Wait for select to be rendered.
+    await browser.wait(presenceOf(select));
+
+    // Opent the select and wait for options to be displayed.
+    await select.click();
+    await browser.wait(presenceOf(options.get(0)));
+
+    // Click the first option and wait for the select to be closed.
+    await options.get(0).click();
+    await browser.wait(not(presenceOf(options.get(0))));
+
+    // Re-open the select and wait for it to be rendered.
+    await select.click();
+    await browser.wait(presenceOf(options.get(0)));
+
+    // Expect no ripples to be showing up without an option click.
+    expect(await ripples.isPresent()).toBe(false);
+  });
+});