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.
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
fix(animations): ensure consistent transition namespace ordering #19854
Conversation
I looked into the performance implications of this change and it might be worthwhile to use |
@matsko Can you look into this? |
@JoostK thank you for finding and fixing this issue. I'm just waiting for the tests to pass (the AIO tests are a bit flaky, but they need to be green). Once that's good then I'll have this flagged as mergeable. |
625884c
to
4311e4d
Compare
@matsko Rebased onto master, this is still an issue as of today. I migrated the reproduction project to Angular 6 in Stackblitz. |
4311e4d
to
88b9257
Compare
88b9257
to
f019f3b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Just a quick comment: we are re-running global presubmit to make sure there are no breakages in Google's codebase. I will keep this thread updated. Thank you. |
Update: there is one failing tests in Google's codebase that requires further investigation. Reached out to the team that owns the test to get some help. Will keep this thread updated. Thank you. |
FYI, no updates from g3 team yet, so I'm adding "blocked" label for now. |
Just a quick note: I haven't received any additional info from an internal team yet. I will rebase this PR and try to run another global presubmit to see if things have changed. |
When including a component in a template, the component's host element is immediately appended as child of the parent node upon creation. Hence, `hostElement.parentNode` will be a valid reference. However, if the parent component is being inserted as an embedded view—through `ngIf` for example—then the parent's node itself will not have been inserted yet. This means that we cannot properly determine the position of the transition namespace, as any `containsElement` check will return false given that the partial DOM tree has not been inserted yet, even though it will be contained within an existing transition namespace once the partial tree is attached. This commit fixes the issue by not just looking at the existence of a parent node, but rather a more extensive check using the driver's `containsElement` method.
f019f3b
to
cbfe44f
Compare
Global presubmit indicated that there is 1 failing target (a different one now), I will perform further investigation and keep this thread updated. |
@JoostK @AndrewKushnir What's the status of this PR? Looks like it was in Presubmit limbo. |
@jessicajaniuk I've restarted TGP a couple days ago and reached out to an internal team which owns failing target. Will update this PR once I get any news. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
FYI, the code in Google's codebase was updated to work correctly with this change, so I'm removing the "blocked" label and marking this PR as ready for merge. |
) When including a component in a template, the component's host element is immediately appended as child of the parent node upon creation. Hence, `hostElement.parentNode` will be a valid reference. However, if the parent component is being inserted as an embedded view—through `ngIf` for example—then the parent's node itself will not have been inserted yet. This means that we cannot properly determine the position of the transition namespace, as any `containsElement` check will return false given that the partial DOM tree has not been inserted yet, even though it will be contained within an existing transition namespace once the partial tree is attached. This commit fixes the issue by not just looking at the existence of a parent node, but rather a more extensive check using the driver's `containsElement` method. PR Close #19854
) When including a component in a template, the component's host element is immediately appended as child of the parent node upon creation. Hence, `hostElement.parentNode` will be a valid reference. However, if the parent component is being inserted as an embedded view—through `ngIf` for example—then the parent's node itself will not have been inserted yet. This means that we cannot properly determine the position of the transition namespace, as any `containsElement` check will return false given that the partial DOM tree has not been inserted yet, even though it will be contained within an existing transition namespace once the partial tree is attached. This commit fixes the issue by not just looking at the existence of a parent node, but rather a more extensive check using the driver's `containsElement` method. PR Close #19854
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
In #19006 a fix was implemented to properly delay the removal of nested animations in distinct components if they are queried in a parent's leave animation using
animateChild
. Upon testing the new behavior I found that the problem occurs once again for deeper nested components. The problem can be observed in this Plunker.After an intense debugging session I discovered that this is caused by the fact that the inner animation is not considered as child animation of the parent's leave transition but as a root animation in itself. As such, the parent's leave transition does not wait for the supposed child animation to complete and causes the node to be removed immediately.
What is the new behavior?
Queried elements in a parent leave animation are now always considered as sub-animation of that parent's animation by assuring proper ordering of transition namespaces.
Does this PR introduce a breaking change?
Other information
When including a component in a template, the component's host element is immediately appended as child of the parent node upon creation. Hence,
hostElement.parentNode
will be a valid reference. However, if the parent component is being inserted as an embedded view---throughngIf
for example---then the parent's node itself will not have been insterted yet. This means that we cannot properly determine the position of the transition namespace, as anycontainsElement
check will return false given that the partial DOM tree has not been inserted yet, even though it will be contained within an existing transition namespace once the partial tree is attached. This PR ensures that the registration of the transition namespace is queued if the element is not yet attached with the body node.As a consequence of the incorrect ordering of transition namespaces, the leave transition on the parent using
animateChild
was unable to select the child transition as it had not been processed yet. In the end, this causes the child animation not be considered as nested transition within the parent player, but as a separate root player of its own.In the test, it is required to inspect the players of the
AnimationEngine
and not those fromgetLog
. The latter only returns the actual animation players, of which the parent leave animation is not part of given that it does not have animation instructions of its own.I have tried a fix where the registration of transition namespaces is always queued, however that causes several tests regarding
:enter
transitions to fail, along with two computed style tests for pre- and post animation styles. I did not look into why exactly this happens but opted to go with the current solution.