Scoping CSS keyframes in emulated view encapsulation #42608

dario-piotrowicz · 2021-06-20T16:16:44Z

Keyframes are not scoped for Emulated view encapsulated component, practically making them global and causing css leakage

note: this does not happen with the shadowDom view encapsulation, there there is no leakage

Since keyframes are not hierarchical I don't believe this issue can be solved without modifying the keyframes names, so the only thing possible I believe is to actually rename the keyframes using the component's scope selector,

The way I did it is by scanning the css twice, in the first scan I modified/scoped the keyframes names in the way mentioned above and in the second I updated animation rules using such keyframes

Note: animations using keyframes not defined in the file are not modified, still allowing global keyframes to be defined

I understand that this is changing the actual names of the keyframes so it may not be what we actually want here, but I figured that opening a PR and starting a discussion would not hurt

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)
Docs have been added / updated (for bug fixes / features)

PR Type

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: #33885

Currently keyframes are not scoped so, if I create two different keyframe animations in two different components but with the same name, those will clash and only one of them will be applied to both components

You can see it in this example I created: demo code

What happens is that I have two different components each with a keyframes called box-animation which colors the component's background, as you can see the application does not behave as expected:

What is the new behavior?

Keyframes are scoped so that they include the component's scope selector like so:

and this allows the keyframes to remain isolated:

Does this PR introduce a breaking change?

Yes
No
(at least I don't think so)

Other information

As I mentioned this will cause all the keyframes names in components with the Emulated view encapsulation to be modified, this is definitely not too nice/elegant but I think it's the only way to scope the keyframes and I believe that it is still better to leave them global as they are now (as that produces unwanted css leakage + it is inconsistent with what happens with ShadowDom)

packages/compiler/src/shadow_css.ts

petebacondarwin

I like this idea!

In general I think it would provide a positive benefit to the emulated encapsulation mode.

Because of the change to the keyframes name, I think we would have to land this in a major release, since these names could be referenced in JS code - for example in an animation event handler.

We can wait for this timeframe, IMO, since the workaround is quite straightforward - even if identifying the problem may not be!

packages/compiler/src/shadow_css.ts

petebacondarwin · 2021-06-23T16:30:32Z

For other reviewers, I checked through the Angular animations code, which also makes use of @keyframes but this does not seem to be affected by this change, since it always generates its own keyframe names, and does not reference ones in the component definition.

packages/compiler/src/shadow_css.ts

dario-piotrowicz · 2021-06-23T21:26:01Z

@petebacondarwin all done as you suggested, thanks a lot 😃

The only remaining thing are the quoted keyframes,I am waiting to hear from you on those 🙂

packages/compiler/src/shadow_css.ts

packages/compiler/test/shadow_css_spec.ts

dario-piotrowicz · 2022-09-12T22:25:08Z

Ok, last time around this was soooo close. We had to roll it back because of a single backslid target. We could try again with a fresh TGP for 15?

Thanks @dylhunn !!! 😍

Please let me know if I can help in any way 😃

jessicajaniuk · 2022-09-23T21:42:34Z

Hey @dario-piotrowicz. We did find a bug here. In the case where the css is all minified into a single string, the regex does not work properly and results in bugs. Here is a test you can use to see the failure:


  it('should scope animations using local keyframes identifiers', () => {
    const css = `:host{display:block}component-a{margin-bottom:10px}.class-b{display:-webkit-box;display:-webkit-flex;display:-moz-box;display:-ms-flexbox;display:flex;gap:8px;width:100%}key-comp-a{-webkit-box-flex:1;-webkit-flex-grow:1;-moz-box-flex:1;-ms-flex-positive:1;flex-grow:1}key-comp-b{-webkit-animation:slide-left .3s .05s forwards;animation:slide-left .3s .05s forwards;-webkit-animation-timing-function:cubic-bezier(.785,.135,.15,.86);animation-timing-function:cubic-bezier(.785,.135,.15,.86);-webkit-flex-shrink:0;-ms-flex-negative:0;flex-shrink:0;opacity:0}@-webkit-keyframes slide-left{0%{opacity:0;-webkit-transform:translateX(10px);transform:translateX(10px)}to{opacity:1;-webkit-transform:translateX(0);transform:translateX(0)}}@keyframes slide-left{0%{opacity:0;-webkit-transform:translateX(10px);transform:translateX(10px)}to{opacity:1;-webkit-transform:translateX(0);transform:translateX(0)}}`;
    const result = s(css, 'host-a');
    expect(result).toContain('-webkit-animation: host-a_slide-left .3s .05s forwards;');
    expect(result).toContain('animation: host-a_slide-left .3s .05s forwards;');
  });

dario-piotrowicz · 2022-09-24T12:44:10Z

Hi @jessicajaniuk thanks for the test 🙂

And thank you so very much for looking into this ❤️

but, are you completely sure that this test highlights the issue?

I've started looking into it and it seems like the test can be easily fixed by removing an extra space in the checks:

-    expect(result).toContain('-webkit-animation: host-a_slide-left .3s .05s forwards;');
-    expect(result).toContain('animation: host-a_slide-left .3s .05s forwards;');
+    expect(result).toContain('-webkit-animation:host-a_slide-left .3s .05s forwards;');
+    expect(result).toContain('animation:host-a_slide-left .3s .05s forwards;');

(which to me seems correct as you can see that this is the expected behaviour in the should maintain the spacing when handling (scoping or not) keyframes and animations test)

Also by running the thing locally the keyframe animation seems to work (and be scoped) as expected:

AndrewKushnir · 2022-09-24T20:54:42Z

@dario-piotrowicz quick question: did you run the test as is or you've formatted the input string? We've noticed that the issue is reproducible with the styles written in 1 line (as we have in a test), but things seem to be working correctly when the input is formatted (which potentially hints at the fact that the RegExp added in this PR might match different things in those 2 cases).

dario-piotrowicz · 2022-09-24T21:44:03Z

@AndrewKushnir I literally just copy-pasted the exact text Jessica posted in her comment, the the only difference being the removal of the spaces in the toContain calls:

I've also rebased to the latest main to make sure that there can't be anything else different somewhere that could produce a different output

😟

jessicajaniuk · 2022-09-24T22:48:42Z

I would suggest looking at the actual output rather than the expect line. We were able to see in the processed css result that not everything was scoped when the string was formatted as a single line with no spaces. I say this because that contains line could pass since '-webkit-animation:...' would also contain 'animation:'. So it's not a safe expect.

…

On Sat, Sep 24, 2022, 2:44 PM Dario Piotrowicz ***@***.***> wrote: @AndrewKushnir <https://github.com/AndrewKushnir> I literally just copy-pasted the exact text Jessica posted in her comment, the the only difference being the removal of the spaces in the toContain calls: [image: Screenshot at 2022-09-24 22-38-27] <https://user-images.githubusercontent.com/61631103/192119652-6f71631b-4663-450c-bb25-7a98882038c1.png> I've also rebased to the latest main to make sure that there can't be anything else different somewhere that could produce a different output 😟 — Reply to this email directly, view it on GitHub <#42608 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ARLFZ2DPHCT6TCRSJ22YSEDV75Y3BANCNFSM47AGWO2Q> . You are receiving this because you were mentioned.Message ID: ***@***.***>

dario-piotrowicz · 2022-09-25T21:02:18Z

@AndrewKushnir @jessicajaniuk sorry I actually found the issue there 😅

I've compared the result before and after:

the second animation doesn't seem to be scoped! 😓

and the tests pass because: animation:host-a_slide-left .3s .05s forwards;') is actually contained in the result (but with the webkit prefix)

sorry for the confusion 🙇 😢

I now know what to look at, thanks a lot! 🙂

dario-piotrowicz · 2022-09-25T21:56:35Z

I've just rebased to the latest main and squashed all the fixup commits, I hope you don't mind 🙂

dario-piotrowicz · 2022-09-25T22:21:32Z

@jessicajaniuk @AndrewKushnir it was a very very small issue 😅 , the regex accepted the animation to only be the first property defined or have spaces prefixing it, I've added that ; is also acceptable, so now it should be all good 🙂

(as you can see in my latest fixup commit)

Ensure that keyframes rules, defined within components with emulated view encapsulation, are scoped to avoid collisions with keyframes in other components. This is achieved by renaming these keyframes to add a prefix that makes them unique across the application. In order to enable the handling of keyframes names defined as strings the previous strategy of replacing quoted css content with `%QUOTED%` (introduced in commit 7f689a2) has been removed and in its place now only specific characters inside quotes are being replaced with placeholder text (those are `;`, `:` and `,`, more can be added in the future if the need arises). Closes angular#33885 BREAKING CHANGE: Keyframes names are now prefixed with the component's "scope name". For example, the following keyframes rule in a component definition, whose "scope name" is host-my-cmp: @Keyframes foo { ... } will become: @Keyframes host-my-cmp_foo { ... } Any TypeScript/JavaScript code which relied on the names of keyframes rules will no longer match. The recommended solutions in this case are to either: - change the component's view encapsulation to the `None` or `ShadowDom` - define keyframes rules in global stylesheets (e.g styles.css) - define keyframes rules programmatically in code.

JoostK

Amazing work, as always! 👏

dario-piotrowicz · 2022-09-27T12:14:41Z

Amazing work, as always! clap

Thanks so much!!! 😊

jessicajaniuk · 2022-09-27T21:50:23Z

TGP is green

AndrewKushnir · 2022-09-29T22:49:53Z

This PR was merged into the repository by commit 051f756.

…gular#42608) Ensure that keyframes rules, defined within components with emulated view encapsulation, are scoped to avoid collisions with keyframes in other components. This is achieved by renaming these keyframes to add a prefix that makes them unique across the application. In order to enable the handling of keyframes names defined as strings the previous strategy of replacing quoted css content with `%QUOTED%` (introduced in commit 7f689a2) has been removed and in its place now only specific characters inside quotes are being replaced with placeholder text (those are `;`, `:` and `,`, more can be added in the future if the need arises). Closes angular#33885 BREAKING CHANGE: Keyframes names are now prefixed with the component's "scope name". For example, the following keyframes rule in a component definition, whose "scope name" is host-my-cmp: @Keyframes foo { ... } will become: @Keyframes host-my-cmp_foo { ... } Any TypeScript/JavaScript code which relied on the names of keyframes rules will no longer match. The recommended solutions in this case are to either: - change the component's view encapsulation to the `None` or `ShadowDom` - define keyframes rules in global stylesheets (e.g styles.css) - define keyframes rules programmatically in code. PR Close angular#42608

angular-automatic-lock-bot · 2022-10-30T16:06:27Z

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.}

pullapprove bot requested a review from alxhub June 20, 2021 16:16

google-cla bot added the cla: yes label Jun 20, 2021

dario-piotrowicz marked this pull request as draft June 20, 2021 16:24

dario-piotrowicz commented Jun 20, 2021

View reviewed changes

packages/compiler/src/shadow_css.ts Outdated Show resolved Hide resolved

dario-piotrowicz force-pushed the keyframes-leak branch from 2b1e6fa to 45cd603 Compare June 20, 2021 16:51

dylhunn added the area: animations label Jun 21, 2021

ngbot bot added this to the Backlog milestone Jun 21, 2021

dario-piotrowicz force-pushed the keyframes-leak branch 3 times, most recently from 051d2ec to 988116d Compare June 22, 2021 22:20

dario-piotrowicz marked this pull request as ready for review June 22, 2021 22:22

AndrewKushnir requested a review from petebacondarwin June 23, 2021 01:42

petebacondarwin requested changes Jun 23, 2021

View reviewed changes

packages/compiler/src/shadow_css.ts Show resolved Hide resolved

packages/compiler/src/shadow_css.ts Outdated Show resolved Hide resolved

packages/compiler/src/shadow_css.ts Outdated Show resolved Hide resolved

dario-piotrowicz force-pushed the keyframes-leak branch from 988116d to 8dda4a5 Compare June 23, 2021 21:18

dario-piotrowicz commented Jun 23, 2021

View reviewed changes

packages/compiler/src/shadow_css.ts Outdated Show resolved Hide resolved

dario-piotrowicz commented Jun 23, 2021

View reviewed changes

packages/compiler/src/shadow_css.ts Outdated Show resolved Hide resolved

dario-piotrowicz requested a review from petebacondarwin June 23, 2021 21:24

jessicajaniuk reviewed Jun 23, 2021

View reviewed changes

packages/compiler/src/shadow_css.ts Outdated Show resolved Hide resolved

packages/compiler/src/shadow_css.ts Outdated Show resolved Hide resolved

dario-piotrowicz commented Jun 23, 2021

View reviewed changes

packages/compiler/src/shadow_css.ts Outdated Show resolved Hide resolved

dario-piotrowicz commented Jun 23, 2021

View reviewed changes

packages/compiler/test/shadow_css_spec.ts Outdated Show resolved Hide resolved

petebacondarwin reviewed Jun 24, 2021

View reviewed changes

packages/compiler/test/shadow_css_spec.ts Outdated Show resolved Hide resolved

dario-piotrowicz force-pushed the keyframes-leak branch 2 times, most recently from e54c6a4 to a99a2e0 Compare June 24, 2021 18:30

dario-piotrowicz commented Jun 24, 2021

View reviewed changes

packages/compiler/test/shadow_css_spec.ts Outdated Show resolved Hide resolved

dario-piotrowicz force-pushed the keyframes-leak branch from 5423f14 to f2528d1 Compare September 25, 2022 21:54

dario-piotrowicz force-pushed the keyframes-leak branch 2 times, most recently from 745e92a to 11dc22a Compare September 26, 2022 19:15

dario-piotrowicz added 2 commits September 26, 2022 21:57

fixup! fix(compiler): scope css keyframes in emulated view encapsulation

5a6f6c9

dario-piotrowicz force-pushed the keyframes-leak branch from 11dc22a to 5a6f6c9 Compare September 26, 2022 20:57

jessicajaniuk removed the state: blocked label Sep 26, 2022

jessicajaniuk approved these changes Sep 26, 2022

View reviewed changes

JoostK approved these changes Sep 27, 2022

View reviewed changes

jessicajaniuk added action: merge The PR is ready for merge by the caretaker and removed action: global presubmit The PR is in need of a google3 global presubmit labels Sep 27, 2022

alxhub modified the milestones: v15 candidates, v15 feature freeze blockers Sep 29, 2022

AndrewKushnir closed this in 051f756 Sep 29, 2022

angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scoping CSS keyframes in emulated view encapsulation #42608

Scoping CSS keyframes in emulated view encapsulation #42608

dario-piotrowicz commented Jun 20, 2021

petebacondarwin left a comment

petebacondarwin commented Jun 23, 2021

dario-piotrowicz commented Jun 23, 2021

dario-piotrowicz commented Sep 12, 2022

jessicajaniuk commented Sep 23, 2022

dario-piotrowicz commented Sep 24, 2022

AndrewKushnir commented Sep 24, 2022 •

edited

dario-piotrowicz commented Sep 24, 2022

jessicajaniuk commented Sep 24, 2022 via email

dario-piotrowicz commented Sep 25, 2022

dario-piotrowicz commented Sep 25, 2022

dario-piotrowicz commented Sep 25, 2022

JoostK left a comment

dario-piotrowicz commented Sep 27, 2022

jessicajaniuk commented Sep 27, 2022

AndrewKushnir commented Sep 29, 2022

angular-automatic-lock-bot bot commented Oct 30, 2022

Scoping CSS keyframes in emulated view encapsulation #42608

Scoping CSS keyframes in emulated view encapsulation #42608

Conversation

dario-piotrowicz commented Jun 20, 2021

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin commented Jun 23, 2021

dario-piotrowicz commented Jun 23, 2021

dario-piotrowicz commented Sep 12, 2022

jessicajaniuk commented Sep 23, 2022

dario-piotrowicz commented Sep 24, 2022

AndrewKushnir commented Sep 24, 2022 • edited

dario-piotrowicz commented Sep 24, 2022

jessicajaniuk commented Sep 24, 2022 via email

dario-piotrowicz commented Sep 25, 2022

dario-piotrowicz commented Sep 25, 2022

dario-piotrowicz commented Sep 25, 2022

JoostK left a comment

Choose a reason for hiding this comment

dario-piotrowicz commented Sep 27, 2022

jessicajaniuk commented Sep 27, 2022

AndrewKushnir commented Sep 29, 2022

angular-automatic-lock-bot bot commented Oct 30, 2022

AndrewKushnir commented Sep 24, 2022 •

edited