Skip to content

fix(expansion-panel): emitting events twice on some browsers #13600

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 19, 2018
Merged

fix(expansion-panel): emitting events twice on some browsers #13600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/lib/expansion/expansion-panel.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<div class="mat-expansion-panel-content"
role="region"
[@bodyExpansion]="_getExpandedState()"
(@bodyExpansion.done)="_bodyAnimation($event)"
(@bodyExpansion.done)="_bodyAnimationDone.next($event)"
[attr.aria-labelledby]="_headerId"
[id]="id"
#body>
Expand Down
31 changes: 19 additions & 12 deletions src/lib/expansion/expansion-panel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import {
import {DOCUMENT} from '@angular/common';
import {ANIMATION_MODULE_TYPE} from '@angular/platform-browser/animations';
import {Subject} from 'rxjs';
import {filter, startWith, take} from 'rxjs/operators';
import {filter, startWith, take, distinctUntilChanged} from 'rxjs/operators';
import {matExpansionAnimations} from './expansion-animations';
import {MatExpansionPanelContent} from './expansion-panel-content';
import {MAT_ACCORDION, MatAccordionBase} from './accordion-base';
Expand Down Expand Up @@ -119,6 +119,9 @@ export class MatExpansionPanel extends CdkAccordionItem implements AfterContentI
/** ID for the associated header element. Used for a11y labelling. */
_headerId = `mat-expansion-panel-header-${uniqueId++}`;

/** Stream of body animation done events. */
_bodyAnimationDone = new Subject<AnimationEvent>();

constructor(@Optional() @SkipSelf() @Inject(MAT_ACCORDION) accordion: MatAccordionBase,
_changeDetectorRef: ChangeDetectorRef,
_uniqueSelectionDispatcher: UniqueSelectionDispatcher,
Expand All @@ -129,6 +132,20 @@ export class MatExpansionPanel extends CdkAccordionItem implements AfterContentI
super(accordion, _changeDetectorRef, _uniqueSelectionDispatcher);
this.accordion = accordion;
this._document = _document;

// We need a Subject with distinctUntilChanged, because the `done` event
// fires twice on some browsers. See https://github.com/angular/angular/issues/24084
this._bodyAnimationDone.pipe(distinctUntilChanged((x, y) => {
return x.fromState === y.fromState && x.toState === y.toState;
})).subscribe(event => {
if (event.fromState !== 'void') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of removing the _bodyAnimation method, can we call it here since an upstream fix to this from angular/angular will likely have us essentially reverting this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind re-adding, but I don't see why it's an issue, considering that it's a private API. Once the upstream fix gets merged, we can turn it back into a private method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, its fine to leave it as is. I just wanted us to consider it.

if (event.toState === 'expanded') {
this.afterExpand.emit();
} else if (event.toState === 'collapsed') {
this.afterCollapse.emit();
}
}
});
}

/** Determines whether the expansion panel should have spacing between it and its siblings. */
Expand Down Expand Up @@ -166,20 +183,10 @@ export class MatExpansionPanel extends CdkAccordionItem implements AfterContentI

ngOnDestroy() {
super.ngOnDestroy();
this._bodyAnimationDone.complete();
this._inputChanges.complete();
}

_bodyAnimation(event: AnimationEvent) {
const {phaseName, toState, fromState} = event;

if (phaseName === 'done' && toState === 'expanded' && fromState !== 'void') {
this.afterExpand.emit();
}
if (phaseName === 'done' && toState === 'collapsed' && fromState !== 'void') {
this.afterCollapse.emit();
}
}

/** Checks whether the expansion panel's content contains the currently-focused element. */
_containsFocus(): boolean {
if (this._body && this._document) {
Expand Down