Skip to content

fix(animations): enable shadowElements to leave when their parent does #46459

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

Closed
dario-piotrowicz wants to merge 1 commit into from
Closed

fix(animations): enable shadowElements to leave when their parent does #46459

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
10 changes: 7 additions & 3 deletions packages/animations/browser/src/render/transition_animation_engine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,11 +314,13 @@ export class AnimationTransitionNamespace {

private _signalRemovalForInnerTriggers(rootElement: any, context: any) {
const elements = this._engine.driver.query(rootElement, NG_TRIGGER_SELECTOR, true);

const shadowElements = rootElement.shadowRoot ?
Copy link
Copy Markdown
Member

@crisbeto crisbeto Jun 22, 2022

Choose a reason for hiding this comment

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

A bit of an edge case, but this won't handle nested shadow roots.

Copy link
Copy Markdown
Contributor Author

@dario-piotrowicz dario-piotrowicz Jun 22, 2022

Choose a reason for hiding this comment

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

ah yeah, you're totally right, as I mentioned in the PR's description I already saw that querying does not work with shadowDom, I'll look into improving the query function to also take shadowDom into account, so I think that should fix the case you're mentioning

So if you prefer I extend this PR with the query improvement or we can try to merge it now and look into the querying issue as a separate task, please let me know whatever works best for you 🙂

(I think I'd go with extending the query function as a separate PR, but I'll need to make extra sure that this piece of code is cleaned up)

Copy link
Copy Markdown
Member

@crisbeto crisbeto Jun 22, 2022

Choose a reason for hiding this comment

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

I'm fine with merging it as it is, but I wanted to flag it as a potential issue.

Copy link
Copy Markdown
Contributor Author

@dario-piotrowicz dario-piotrowicz Jun 22, 2022

Choose a reason for hiding this comment

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

awesome thanks 🙂 , I'll make sure to address it! 👍

this._engine.driver.query(rootElement.shadowRoot, NG_TRIGGER_SELECTOR, true) :
[];
// emulate a leave animation for all inner nodes within this node.
// If there are no animations found for any of the nodes then clear the cache
// for the element.
elements.forEach(elm => {
[...elements, ...shadowElements].forEach(elm => {
// this means that an inner remove() operation has already kicked off
// the animation on this element...
if (elm[REMOVAL_FLAG]) return;
Expand Down Expand Up @@ -402,7 +404,9 @@ export class AnimationTransitionNamespace {

removeNode(element: any, context: any): void {
const engine = this._engine;
if (element.childElementCount) {
const elementHasChildren = !!element.childElementCount;
const elementHasShadowChildren = !!(element.shadowRoot && element.shadowRoot.childElementCount);
if (elementHasChildren || elementHasShadowChildren) {
this._signalRemovalForInnerTriggers(element, context);
}

Expand Down
62 changes: 60 additions & 2 deletions packages/core/test/animation/animation_query_integration_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@
* 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 {animate, animateChild, AnimationPlayer, AUTO_STYLE, group, query, sequence, stagger, state, style, transition, trigger, ɵAnimationGroupPlayer as AnimationGroupPlayer} from '@angular/animations';
import {animate, animateChild, AnimationEvent, AnimationPlayer, AUTO_STYLE, group, query, sequence, stagger, state, style, transition, trigger, ɵAnimationGroupPlayer as AnimationGroupPlayer} from '@angular/animations';
import {AnimationDriver, ɵAnimationEngine, ɵnormalizeKeyframes as normalizeKeyframes} from '@angular/animations/browser';
import {TransitionAnimationPlayer} from '@angular/animations/browser/src/render/transition_animation_engine';
import {ENTER_CLASSNAME, LEAVE_CLASSNAME} from '@angular/animations/browser/src/util';
import {MockAnimationDriver, MockAnimationPlayer} from '@angular/animations/browser/testing';
import {CommonModule} from '@angular/common';
import {Component, HostBinding, ViewChild} from '@angular/core';
import {Component, HostBinding, ViewChild, ViewEncapsulation} from '@angular/core';
import {fakeAsync, flushMicrotasks, TestBed} from '@angular/core/testing';
import {BrowserAnimationsModule} from '@angular/platform-browser/animations';

Expand Down Expand Up @@ -2832,6 +2832,64 @@ describe('animation query tests', function() {
]);
}));

it(`should emulate a leave animation on a child elements when a parent component using shadowDom leaves the DOM`,
Copy link
Copy Markdown
Contributor Author

@dario-piotrowicz dario-piotrowicz Jun 22, 2022

Choose a reason for hiding this comment

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

seems like a too straightforward test but it does indeed not pass without the changes 🙂
Screenshot at 2022-06-22 21-12-23

fakeAsync(() => {
let childLeaveLog = 0;

@Component({
selector: 'ani-host',
template: `<ani-parent *ngIf="exp"></ani-parent>`,
})
class HostCmp {
public exp: boolean = false;
}

@Component({
selector: 'ani-parent',
encapsulation: ViewEncapsulation.ShadowDom,
template: `
<div @childAnimation (@childAnimation.start)="logChildLeave($event)"></div>
`,
animations: [
trigger(
'childAnimation',
[
transition(':leave', []),
]),
]
})
class ParentCmp {
logChildLeave(event: AnimationEvent) {
if (event.toState === 'void') {
childLeaveLog++;
}
}
}

TestBed.configureTestingModule({declarations: [ParentCmp, HostCmp]});

const fixture = TestBed.createComponent(HostCmp);
const cmp = fixture.componentInstance;

const updateExpAndFlush = (value: boolean) => {
cmp.exp = value;
fixture.detectChanges();
flushMicrotasks();
};

updateExpAndFlush(true);
expect(childLeaveLog).toEqual(0);

updateExpAndFlush(false);
expect(childLeaveLog).toEqual(1);

updateExpAndFlush(true);
expect(childLeaveLog).toEqual(1);

updateExpAndFlush(false);
expect(childLeaveLog).toEqual(2);
}));

it('should build, but not run sub triggers when a parent animation is scheduled', () => {
@Component({
selector: 'parent-cmp',
Expand Down