From 9ea9d1e491c390710abc679897e94e22d3e748ac Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Thu, 14 Jan 2021 17:54:25 +0100 Subject: [PATCH] fix(material-experimental/mdc-chips): decouple removal from animation Currently the MDC chips automatically trigger an animation that collapses the chip to 0x0 dimensions, but it still keeps it in the DOM unless the consumer updates their data source. This is misleading and there's no way for the consumer to see it. These changes bring the chip in line with our current behavior where we only dispatch the remove event and then it's up to the consumer to remove the chip. Making these changes also revealed some tests that were passing by accident and weren't actually testing the things they were supposed to, because the `animationend` event wasn't being dispatched. I've tweaked them in order to get them to work as expected. Fixes #21561. --- .../mdc-chips/chip-grid.spec.ts | 18 ++--------- .../mdc-chips/chip-grid.ts | 2 +- .../mdc-chips/chip-remove.spec.ts | 21 ------------- .../mdc-chips/chip-row.spec.ts | 25 --------------- .../mdc-chips/chip-row.ts | 22 ++++++++++--- .../mdc-chips/chip.spec.ts | 16 ---------- src/material-experimental/mdc-chips/chip.ts | 31 +++++-------------- .../mdc-chips/chips.scss | 8 +---- .../mdc-chips/testing/chip-harness.ts | 3 -- 9 files changed, 29 insertions(+), 117 deletions(-) diff --git a/src/material-experimental/mdc-chips/chip-grid.spec.ts b/src/material-experimental/mdc-chips/chip-grid.spec.ts index d51014bf5f60..2f06effcd7a8 100644 --- a/src/material-experimental/mdc-chips/chip-grid.spec.ts +++ b/src/material-experimental/mdc-chips/chip-grid.spec.ts @@ -12,7 +12,6 @@ import { TAB } from '@angular/cdk/keycodes'; import { - createFakeEvent, createKeyboardEvent, dispatchEvent, dispatchFakeEvent, @@ -216,7 +215,7 @@ describe('MDC-based MatChipGrid', () => { expect(chipGridInstance.focus).toHaveBeenCalled(); }); - it('should move focus to the last chip when the focused chip was deleted inside a' + + it('should move focus to the last chip when the focused chip was deleted inside a ' + 'component with animations', fakeAsync(() => { fixture.destroy(); TestBed.resetTestingModule(); @@ -602,7 +601,6 @@ describe('MDC-based MatChipGrid', () => { describe('with chip remove', () => { let chipGrid: MatChipGrid; - let chipElements: DebugElement[]; let chipRemoveDebugElements: DebugElement[]; beforeEach(() => { @@ -610,7 +608,6 @@ describe('MDC-based MatChipGrid', () => { fixture.detectChanges(); chipGrid = fixture.debugElement.query(By.directive(MatChipGrid))!.componentInstance; - chipElements = fixture.debugElement.queryAll(By.directive(MatChipRow)); chipRemoveDebugElements = fixture.debugElement.queryAll(By.directive(MatChipRemove)); chips = chipGrid._chips; }); @@ -623,12 +620,6 @@ describe('MDC-based MatChipGrid', () => { dispatchMouseEvent(chipRemoveDebugElements[2].nativeElement, 'click'); fixture.detectChanges(); - const fakeEvent = createFakeEvent('transitionend'); - (fakeEvent as any).propertyName = 'width'; - chipElements[2].nativeElement.dispatchEvent(fakeEvent); - - fixture.detectChanges(); - expect(chips.toArray()[2].value).not.toBe(2, 'Expected the third chip to be removed.'); expect(chipGrid._keyManager.activeRowIndex).toBe(2); }); @@ -771,9 +762,10 @@ describe('MDC-based MatChipGrid', () => { dispatchFakeEvent(nativeChips[0], 'focusout'); fixture.detectChanges(); + tick(); + fixture.detectChanges(); zone.simulateZoneExit(); fixture.detectChanges(); - tick(); expect(formField.classList).not.toContain('mat-focused'); })); @@ -787,10 +779,6 @@ describe('MDC-based MatChipGrid', () => { chip.focus(); dispatchKeyboardEvent(chip, 'keydown', BACKSPACE); fixture.detectChanges(); - const fakeEvent = createFakeEvent('transitionend'); - (fakeEvent as any).propertyName = 'width'; - chip.dispatchEvent(fakeEvent); - fixture.detectChanges(); tick(); }); diff --git a/src/material-experimental/mdc-chips/chip-grid.ts b/src/material-experimental/mdc-chips/chip-grid.ts index 17b3298df261..dac73e8aee22 100644 --- a/src/material-experimental/mdc-chips/chip-grid.ts +++ b/src/material-experimental/mdc-chips/chip-grid.ts @@ -505,7 +505,7 @@ export class MatChipGrid extends _MatChipGridMixinBase implements AfterContentIn const newChipIndex = Math.min(this._lastDestroyedChipIndex, this._chips.length - 1); this._keyManager.setActiveCell({ row: newChipIndex, - column: this._keyManager.activeColumnIndex + column: Math.max(this._keyManager.activeColumnIndex, 0) }); } else { this.focus(); diff --git a/src/material-experimental/mdc-chips/chip-remove.spec.ts b/src/material-experimental/mdc-chips/chip-remove.spec.ts index d0b23f23eaba..8c68eb6f3895 100644 --- a/src/material-experimental/mdc-chips/chip-remove.spec.ts +++ b/src/material-experimental/mdc-chips/chip-remove.spec.ts @@ -1,5 +1,4 @@ import { - createFakeEvent, dispatchKeyboardEvent, createKeyboardEvent, dispatchEvent, @@ -55,18 +54,6 @@ describe('MDC-based Chip Remove', () => { expect(buttonElement.hasAttribute('type')).toBe(false); }); - it('should start MDC exit animation on click', () => { - let buttonElement = chipNativeElement.querySelector('button')!; - - testChip.removable = true; - fixture.detectChanges(); - - buttonElement.click(); - fixture.detectChanges(); - - expect(chipNativeElement.classList.contains('mdc-chip--exit')).toBe(true); - }); - it('should emit (removed) event when exit animation is complete', () => { let buttonElement = chipNativeElement.querySelector('button')!; @@ -77,10 +64,6 @@ describe('MDC-based Chip Remove', () => { buttonElement.click(); fixture.detectChanges(); - const fakeEvent = createFakeEvent('transitionend'); - (fakeEvent as any).propertyName = 'width'; - chipNativeElement.dispatchEvent(fakeEvent); - expect(testChip.didRemove).toHaveBeenCalled(); }); @@ -164,10 +147,6 @@ describe('MDC-based Chip Remove', () => { dispatchKeyboardEvent(buttonElement, 'keydown', TAB); fixture.detectChanges(); - const fakeEvent = createFakeEvent('transitionend'); - (fakeEvent as any).propertyName = 'width'; - chipNativeElement.dispatchEvent(fakeEvent); - expect(testChip.didRemove).not.toHaveBeenCalled(); }); diff --git a/src/material-experimental/mdc-chips/chip-row.spec.ts b/src/material-experimental/mdc-chips/chip-row.spec.ts index 6c3230ab56c2..17b878de165e 100644 --- a/src/material-experimental/mdc-chips/chip-row.spec.ts +++ b/src/material-experimental/mdc-chips/chip-row.spec.ts @@ -2,7 +2,6 @@ import {Directionality} from '@angular/cdk/bidi'; import {BACKSPACE, DELETE, RIGHT_ARROW, ENTER} from '@angular/cdk/keycodes'; import { createKeyboardEvent, - createFakeEvent, dispatchEvent, dispatchFakeEvent, } from '@angular/cdk/testing/private'; @@ -97,10 +96,6 @@ describe('MDC-based Row Chips', () => { chipInstance.remove(); fixture.detectChanges(); - const fakeEvent = createFakeEvent('transitionend'); - (fakeEvent as any).propertyName = 'width'; - chipNativeElement.dispatchEvent(fakeEvent); - expect(testComponent.chipRemove).toHaveBeenCalledWith({chip: chipInstance}); }); @@ -127,10 +122,6 @@ describe('MDC-based Row Chips', () => { chipInstance._keydown(DELETE_EVENT); fixture.detectChanges(); - const fakeEvent = createFakeEvent('transitionend'); - (fakeEvent as any).propertyName = 'width'; - chipNativeElement.dispatchEvent(fakeEvent); - expect(testComponent.chipRemove).toHaveBeenCalled(); }); @@ -142,10 +133,6 @@ describe('MDC-based Row Chips', () => { chipInstance._keydown(BACKSPACE_EVENT); fixture.detectChanges(); - const fakeEvent = createFakeEvent('transitionend'); - (fakeEvent as any).propertyName = 'width'; - chipNativeElement.dispatchEvent(fakeEvent); - expect(testComponent.chipRemove).toHaveBeenCalled(); }); @@ -157,10 +144,6 @@ describe('MDC-based Row Chips', () => { removeIconInstance.interaction.next(ARROW_KEY_EVENT); fixture.detectChanges(); - const fakeEvent = createFakeEvent('transitionend'); - (fakeEvent as any).propertyName = 'width'; - chipNativeElement.dispatchEvent(fakeEvent); - expect(testComponent.chipRemove).not.toHaveBeenCalled(); }); }); @@ -179,10 +162,6 @@ describe('MDC-based Row Chips', () => { chipInstance._keydown(DELETE_EVENT); fixture.detectChanges(); - const fakeEvent = createFakeEvent('transitionend'); - (fakeEvent as any).propertyName = 'width'; - chipNativeElement.dispatchEvent(fakeEvent); - expect(testComponent.chipRemove).not.toHaveBeenCalled(); }); @@ -195,10 +174,6 @@ describe('MDC-based Row Chips', () => { chipInstance._keydown(BACKSPACE_EVENT); fixture.detectChanges(); - const fakeEvent = createFakeEvent('transitionend'); - (fakeEvent as any).propertyName = 'width'; - chipNativeElement.dispatchEvent(fakeEvent); - expect(testComponent.chipRemove).not.toHaveBeenCalled(); }); }); diff --git a/src/material-experimental/mdc-chips/chip-row.ts b/src/material-experimental/mdc-chips/chip-row.ts index cdf6ab0a096a..0e1d22a8a53d 100644 --- a/src/material-experimental/mdc-chips/chip-row.ts +++ b/src/material-experimental/mdc-chips/chip-row.ts @@ -99,6 +99,12 @@ export class MatChipRow extends MatChip implements AfterContentInit, AfterViewIn /** The focusable grid cells for this row. Implemented as part of GridKeyManagerRow. */ cells!: HTMLElement[]; + /** + * Timeout used to give some time between `focusin` and `focusout` + * in order to determine whether focus has left the chip. + */ + private _focusoutTimeout: number | null; + constructor( @Inject(DOCUMENT) private readonly _document: any, changeDetectorRef: ChangeDetectorRef, @@ -153,12 +159,13 @@ export class MatChipRow extends MatChip implements AfterContentInit, AfterViewIn * to the other gridcell. */ _focusout(event: FocusEvent) { - this._hasFocusInternal = false; + if (this._focusoutTimeout) { + clearTimeout(this._focusoutTimeout); + } + // Wait to see if focus moves to the other gridcell - setTimeout(() => { - if (this._hasFocus()) { - return; - } + this._focusoutTimeout = setTimeout(() => { + this._hasFocusInternal = false; this._onBlur.next({chip: this}); this._handleInteraction(event); }); @@ -166,6 +173,11 @@ export class MatChipRow extends MatChip implements AfterContentInit, AfterViewIn /** Records that the chip has focus when one of the gridcells is focused. */ _focusin(event: FocusEvent) { + if (this._focusoutTimeout) { + clearTimeout(this._focusoutTimeout); + this._focusoutTimeout = null; + } + this._hasFocusInternal = true; this._handleInteraction(event); } diff --git a/src/material-experimental/mdc-chips/chip.spec.ts b/src/material-experimental/mdc-chips/chip.spec.ts index 3d3075f174c3..613ec7e8b69a 100644 --- a/src/material-experimental/mdc-chips/chip.spec.ts +++ b/src/material-experimental/mdc-chips/chip.spec.ts @@ -1,5 +1,4 @@ import {Directionality} from '@angular/cdk/bidi'; -import {createFakeEvent} from '@angular/cdk/testing/private'; import {Component, DebugElement, ViewChild} from '@angular/core'; import {waitForAsync, ComponentFixture, TestBed} from '@angular/core/testing'; import {MatRipple} from '@angular/material-experimental/mdc-core'; @@ -135,24 +134,9 @@ describe('MDC-based MatChip', () => { chipInstance.remove(); fixture.detectChanges(); - const fakeEvent = createFakeEvent('transitionend'); - (fakeEvent as any).propertyName = 'width'; - chipNativeElement.dispatchEvent(fakeEvent); - expect(testComponent.chipRemove).toHaveBeenCalledWith({chip: chipInstance}); }); - it('should make the chip non-focusable when it is removed', () => { - chipInstance.remove(); - fixture.detectChanges(); - - const fakeEvent = createFakeEvent('transitionend'); - (fakeEvent as any).propertyName = 'width'; - chipNativeElement.dispatchEvent(fakeEvent); - - expect(chipNativeElement.style.display).toBe('none'); - }); - it('should be able to disable ripples with the `[rippleDisabled]` input', () => { expect(chipRippleInstance.disabled).toBe(false, 'Expected chip ripples to be enabled.'); diff --git a/src/material-experimental/mdc-chips/chip.ts b/src/material-experimental/mdc-chips/chip.ts index b007b0924cf0..24d8cb875208 100644 --- a/src/material-experimental/mdc-chips/chip.ts +++ b/src/material-experimental/mdc-chips/chip.ts @@ -231,9 +231,6 @@ export class MatChip extends _MatChipMixinBase implements AfterContentInit, Afte /** The unstyled chip selector for this component. */ protected basicChipAttrName = 'mat-basic-chip'; - /** Subject that emits when the component has been destroyed. */ - protected _destroyed = new Subject(); - /** The chip's leading icon. */ @ContentChild(MAT_CHIP_AVATAR) leadingIcon: MatChipAvatar; @@ -278,15 +275,7 @@ export class MatChip extends _MatChipMixinBase implements AfterContentInit, Afte notifyNavigation: () => this._notifyNavigation(), notifyTrailingIconInteraction: () => this.removeIconInteraction.emit(this.id), - notifyRemoval: - () => { - this.removed.emit({chip: this}); - - // When MDC removes a chip it just transitions it to `width: 0px` - // which means that it's still in the DOM and it's still focusable. - // Make it `display: none` so users can't tab into it. - this._elementRef.nativeElement.style.display = 'none'; - }, + notifyRemoval: () => this.remove(), notifyEditStart: () => { this._onEditStart(); @@ -375,24 +364,17 @@ export class MatChip extends _MatChipMixinBase implements AfterContentInit, Afte ngOnDestroy() { this.destroyed.emit({chip: this}); - this._destroyed.next(); - this._destroyed.complete(); this._chipFoundation.destroy(); } /** Sets up the remove icon chip foundation, and subscribes to remove icon events. */ - _initRemoveIcon() { + private _initRemoveIcon() { if (this.removeIcon) { this._chipFoundation.setShouldRemoveOnTrailingIconClick(true); - this._listenToRemoveIconInteraction(); this.removeIcon.disabled = this.disabled; - } - } - /** Handles interaction with the remove icon. */ - _listenToRemoveIconInteraction() { - this.removeIcon.interaction - .pipe(takeUntil(this._destroyed)) + this.removeIcon.interaction + .pipe(takeUntil(this.destroyed)) .subscribe(event => { // The MDC chip foundation calls stopPropagation() for any trailing icon interaction // event, even ones it doesn't handle, so we want to avoid passing it keyboard events @@ -405,7 +387,7 @@ export class MatChip extends _MatChipMixinBase implements AfterContentInit, Afte return; } - this._chipFoundation.handleTrailingActionInteraction(); + this.remove(); if (isKeyboardEvent && !hasModifierKey(event as KeyboardEvent)) { const keyCode = (event as KeyboardEvent).keyCode; @@ -416,6 +398,7 @@ export class MatChip extends _MatChipMixinBase implements AfterContentInit, Afte } } }); + } } /** @@ -425,7 +408,7 @@ export class MatChip extends _MatChipMixinBase implements AfterContentInit, Afte */ remove(): void { if (this.removable) { - this._chipFoundation.beginExit(); + this.removed.emit({chip: this}); } } diff --git a/src/material-experimental/mdc-chips/chips.scss b/src/material-experimental/mdc-chips/chips.scss index d6914a8db7cc..3d5dfd618f9e 100644 --- a/src/material-experimental/mdc-chips/chips.scss +++ b/src/material-experimental/mdc-chips/chips.scss @@ -11,14 +11,8 @@ cursor: default; &._mat-animation-noopable { - // MDC's chip removal works by toggling a class on the chip, waiting for its transitions - // to finish and emitting the remove event at the end. The problem is that if our animations - // were disabled via the `NoopAnimationsModule`, the element won't have a transition and - // `transitionend` won't fire. We work around the issue by assigning a very short transition. - transition-duration: 1ms; - - // Disables the chip enter animation. animation: none; + transition: none; .mdc-chip__checkmark-svg { transition: none; diff --git a/src/material-experimental/mdc-chips/testing/chip-harness.ts b/src/material-experimental/mdc-chips/testing/chip-harness.ts index cf6fbe52406a..1b5c7f3b85a7 100644 --- a/src/material-experimental/mdc-chips/testing/chip-harness.ts +++ b/src/material-experimental/mdc-chips/testing/chip-harness.ts @@ -43,9 +43,6 @@ export class MatChipHarness extends ComponentHarness { async remove(): Promise { const hostEl = await this.host(); await hostEl.sendKeys(TestKey.DELETE); - - // @breaking-change 12.0.0 Remove non-null assertion from `dispatchEvent`. - await hostEl.dispatchEvent!('transitionend', {propertyName: 'width'}); } /**