fix(animations): allow animations on elements in the shadow DOM #40134

jeripeierSBB · 2020-12-15T16:44:26Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: #25672

What is the new behavior?

enhanced containsElement method to not only use the native node contains method but to walk up the DOM tree by also considering shadow elements. Fixing this allows animations to be played if they are placed inside a shadow DOM.

Does this PR introduce a breaking change?

Yes
No

LinboLen · 2021-02-24T12:34:42Z

ping... , does this pull request can be merged soon?. animation with ng-content or shadowdom won't work for a while .

IonelLupu · 2021-03-18T11:34:43Z

hey. How can we help with this PR? Maybe we can have it in the next Angular release. It's blocking some other animation related issues

petebacondarwin · 2021-04-02T14:52:31Z

I am aware that I have still not reviewed this PR. I will prioritise it for next week.

petebacondarwin

OK, so I finally got round to taking a look at this PR.
Thank you @jeripeierSBB for putting this together, and for your patience, everyone.
I think it is in pretty good shape. Just a couple of suggestions.

Also, since the commit message will appear in the CHANGELOG, can we reword it slightly...

fix(animations): allow animations on elements in the shadow DOM

When determining whether to run an animation, the `TransiationAnimationPlayer`
checks to see if a DOM element is attached to the document. This is done by
checking to see if the element is "contained" by the document body node.

Previously, if the element was inside a shadow DOM, the engine would
determine that the element was not attached, even if the shadow DOM's
host was attached to the document. This commit updates the `containsElement()`
method on `AnimationDriver` implementations to also include shadow DOM
elements as being contained if their shadow host element is contained.

Further, when using CSS keyframes to trigger animations, the styling
was always added to the `head` element of the document, even for
animations on elements within a shadow DOM. This meant that those
elements never receive those styles and the animation would not run.
This commit updates the insertion of these styles so that they are added,
to the element's "root node", which is the nearest shadow DOM host, or the
`head` of the document if the element is not in a shadow DOM.

Closes #25672

packages/animations/browser/src/render/css_keyframes/css_keyframes_driver.ts

packages/animations/browser/src/render/shared.ts

packages/animations/browser/test/render/web_animations/web_animations_driver_spec.ts

packages/animations/browser/test/render/css_keyframes/css_keyframes_driver_spec.ts

jeripeierSBB · 2021-04-06T19:19:31Z

@petebacondarwin thanks a lot for your review. I will update the PR in the next days.

petebacondarwin

LGTM! Thanks @jeripeierSBB

Reviewed-for: size-tracking

We will need to get further reviews

packages/animations/browser/test/render/css_keyframes/css_keyframes_driver_spec.ts

goldens/size-tracking/aio-payloads.json

petebacondarwin · 2021-04-07T15:57:35Z

goldens/size-tracking/aio-payloads.json

@@ -21,7 +21,7 @@
    "master": {
      "uncompressed": {
        "runtime-es2015": 3153,
-        "main-es2015": 437306,
+        "main-es2015": 437924,


Actual size on master was 437712. So this only adds 212 bytes.

Similarly this one can be 437844.

Sorry I did not really get the size tracking thing.. Thanks for pointing me in the right direction ;)

@petebacondarwin I tried several ways to locally get the right size to enter here, but failed. What is the correct way to get the bundle file size which should be entered here (not waiting for ci to fail)?

atscott

Reviewed-for: size-tracking

packages/animations/browser/src/render/shared.ts

When determining whether to run an animation, the `TransitionAnimationPlayer` checks to see if a DOM element is attached to the document. This is done by checking to see if the element is "contained" by the document body node. Previously, if the element was inside a shadow DOM, the engine would determine that the element was not attached, even if the shadow DOM's host was attached to the document. This commit updates the `containsElement()` method on `AnimationDriver` implementations to also include shadow DOM elements as being contained if their shadow host element is contained. Further, when using CSS keyframes to trigger animations, the styling was always added to the `head` element of the document, even for animations on elements within a shadow DOM. This meant that those elements never receive those styles and the animation would not run. This commit updates the insertion of these styles so that they are added, to the element's "root node", which is the nearest shadow DOM host, or the `head` of the document if the element is not in a shadow DOM. Closes angular#25672

AndrewKushnir · 2021-04-13T18:57:26Z

Presubmit + Global Presubmit.

AndrewKushnir · 2021-04-14T06:18:40Z

@petebacondarwin FYI, presubmits (including the global one) are successful for the changes in this PR.

petebacondarwin · 2021-04-14T09:15:18Z

Thanks @AndrewKushnir - let's merge it!

When determining whether to run an animation, the `TransitionAnimationPlayer` checks to see if a DOM element is attached to the document. This is done by checking to see if the element is "contained" by the document body node. Previously, if the element was inside a shadow DOM, the engine would determine that the element was not attached, even if the shadow DOM's host was attached to the document. This commit updates the `containsElement()` method on `AnimationDriver` implementations to also include shadow DOM elements as being contained if their shadow host element is contained. Further, when using CSS keyframes to trigger animations, the styling was always added to the `head` element of the document, even for animations on elements within a shadow DOM. This meant that those elements never receive those styles and the animation would not run. This commit updates the insertion of these styles so that they are added, to the element's "root node", which is the nearest shadow DOM host, or the `head` of the document if the element is not in a shadow DOM. Closes #25672 PR Close #40134

AndrewKushnir · 2021-04-15T19:47:40Z

FYI, this change was merged into the master and 12.0.x (RC) branches, but there was a conflict while merging into 11.2.x branch. If the change should also be included into the 11.2.x branch, please create a new PR targeting 11.2.x branch. Thank you.

lubos-bistak · 2021-04-22T10:06:27Z

Is it possible to apply this fix also to Angular 9?

petebacondarwin · 2021-04-22T15:46:34Z

Since v9 is in LTS, it will not receive this bug fix, sorry.

angular-automatic-lock-bot · 2021-05-23T16:08:19Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

google-cla bot added the cla: yes label Dec 15, 2020

jeripeierSBB force-pushed the support-shadow-dom-animations branch 4 times, most recently from 2a7bea7 to bfe9555 Compare December 17, 2020 09:48

jeripeierSBB marked this pull request as ready for review December 17, 2020 10:14

pullapprove bot requested a review from IgorMinar December 17, 2020 10:14

pullapprove bot added the area: animations label Dec 17, 2020

ngbot bot added this to the Backlog milestone Dec 17, 2020

pipeBuitragoFiduciariaBogota approved these changes Dec 29, 2020

View reviewed changes

jeripeierSBB force-pushed the support-shadow-dom-animations branch from bfe9555 to 9939ccc Compare January 19, 2021 09:41

LinboLen mentioned this pull request Feb 23, 2021

bug happened when animations work with ng-content #24041

Open

petebacondarwin self-assigned this Mar 23, 2021

petebacondarwin self-requested a review March 23, 2021 13:34

petebacondarwin requested changes Apr 6, 2021

View reviewed changes

petebacondarwin added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release labels Apr 6, 2021

jeripeierSBB force-pushed the support-shadow-dom-animations branch from 3afe416 to e0bb3b1 Compare April 7, 2021 07:04

pullapprove bot requested a review from alxhub April 7, 2021 07:04

jeripeierSBB force-pushed the support-shadow-dom-animations branch from e0bb3b1 to f9b783d Compare April 7, 2021 07:37

jeripeierSBB requested a review from petebacondarwin April 7, 2021 08:08

jeripeierSBB changed the title ~~fix(animations): allow animations in shadow DOM~~ fix(animations): allow animations on elements in the shadow DOM Apr 7, 2021

petebacondarwin approved these changes Apr 7, 2021

View reviewed changes

alxhub approved these changes Apr 9, 2021

View reviewed changes

pullapprove bot requested review from alxhub and atscott April 9, 2021 13:41

atscott approved these changes Apr 9, 2021

View reviewed changes

AndrewKushnir approved these changes Apr 9, 2021

View reviewed changes

packages/animations/browser/src/render/shared.ts Outdated Show resolved Hide resolved

jeripeierSBB added 3 commits April 13, 2021 08:22

jeripeierSBB force-pushed the support-shadow-dom-animations branch from 9fb0b28 to d554735 Compare April 13, 2021 07:22

jeripeierSBB force-pushed the support-shadow-dom-animations branch from d554735 to f20ff83 Compare April 13, 2021 07:51

AndrewKushnir removed request for IgorMinar and alxhub April 13, 2021 18:56

AndrewKushnir removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Apr 13, 2021

AndrewKushnir removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Apr 13, 2021

AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Apr 14, 2021

petebacondarwin added the action: merge The PR is ready for merge by the caretaker label Apr 14, 2021

AndrewKushnir added target: rc This PR is targeted for the next release-candidate and removed target: patch This PR is targeted for the next patch release labels Apr 15, 2021

AndrewKushnir closed this in a99aa29 Apr 15, 2021

crisbeto mentioned this pull request May 16, 2021

using "ViewEncapsulation.Native" breaks down Angular 2 animations #14340

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators May 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(animations): allow animations on elements in the shadow DOM #40134

fix(animations): allow animations on elements in the shadow DOM #40134

jeripeierSBB commented Dec 15, 2020 •

edited

LinboLen commented Feb 24, 2021 •

edited

IonelLupu commented Mar 18, 2021

petebacondarwin commented Apr 2, 2021

petebacondarwin left a comment

jeripeierSBB commented Apr 6, 2021

petebacondarwin left a comment

petebacondarwin Apr 7, 2021

petebacondarwin Apr 9, 2021

jeripeierSBB Apr 9, 2021

jeripeierSBB Apr 13, 2021

atscott left a comment

AndrewKushnir commented Apr 13, 2021 •

edited

AndrewKushnir commented Apr 14, 2021

petebacondarwin commented Apr 14, 2021

AndrewKushnir commented Apr 15, 2021

lubos-bistak commented Apr 22, 2021

petebacondarwin commented Apr 22, 2021

angular-automatic-lock-bot bot commented May 23, 2021

fix(animations): allow animations on elements in the shadow DOM #40134

fix(animations): allow animations on elements in the shadow DOM #40134

Conversation

jeripeierSBB commented Dec 15, 2020 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

LinboLen commented Feb 24, 2021 • edited

IonelLupu commented Mar 18, 2021

petebacondarwin commented Apr 2, 2021

petebacondarwin left a comment

Choose a reason for hiding this comment

jeripeierSBB commented Apr 6, 2021

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin Apr 7, 2021

Choose a reason for hiding this comment

petebacondarwin Apr 9, 2021

Choose a reason for hiding this comment

jeripeierSBB Apr 9, 2021

Choose a reason for hiding this comment

jeripeierSBB Apr 13, 2021

Choose a reason for hiding this comment

atscott left a comment

Choose a reason for hiding this comment

AndrewKushnir commented Apr 13, 2021 • edited

AndrewKushnir commented Apr 14, 2021

petebacondarwin commented Apr 14, 2021

AndrewKushnir commented Apr 15, 2021

lubos-bistak commented Apr 22, 2021

petebacondarwin commented Apr 22, 2021

angular-automatic-lock-bot bot commented May 23, 2021

jeripeierSBB commented Dec 15, 2020 •

edited

LinboLen commented Feb 24, 2021 •

edited

AndrewKushnir commented Apr 13, 2021 •

edited