The Wayback Machine - https://web.archive.org/web/20220610051717/https://github.com/angular/angular/pull/42608
Skip to content
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

Jump to bottom

Scoping CSS keyframes in emulated view encapsulation #42608

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Scoping CSS keyframes in emulated view encapsulation #42608

wants to merge 14 commits into from

Conversation

dario-piotrowicz
Copy link
Contributor

@dario-piotrowicz dario-piotrowicz commented Jun 20, 2021

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:

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:
Screenshot at 2021-06-20 16-58-37

What is the new behavior?

Keyframes are scoped so that they include the component's scope selector like so:
Screenshot at 2021-06-20 16-55-49

and this allows the keyframes to remain isolated:
Screenshot at 2021-06-20 16-59-02

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)

@pullapprove pullapprove bot requested a review from alxhub Jun 20, 2021
@google-cla google-cla bot added the cla: yes label Jun 20, 2021
@dario-piotrowicz dario-piotrowicz marked this pull request as draft Jun 20, 2021
@ngbot ngbot bot added this to the Backlog milestone Jun 21, 2021
@dario-piotrowicz dario-piotrowicz force-pushed the keyframes-leak branch 3 times, most recently from 051d2ec to 988116d Compare Jun 22, 2021
@dario-piotrowicz dario-piotrowicz marked this pull request as ready for review Jun 22, 2021
petebacondarwin
petebacondarwin requested changes Jun 23, 2021
View changes
Copy link
Member

@petebacondarwin petebacondarwin left a comment

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 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
@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Jun 23, 2021

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.

@dario-piotrowicz
Copy link
Contributor Author

@dario-piotrowicz dario-piotrowicz commented Jun 23, 2021

@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 🙂

jessicajaniuk
jessicajaniuk reviewed Jun 23, 2021
View 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 dario-piotrowicz force-pushed the keyframes-leak branch 2 times, most recently from e54c6a4 to a99a2e0 Compare Jun 24, 2021
@dylhunn dylhunn added action: cleanup and removed action: merge labels Mar 29, 2022
josmar-crwdstffng pushed a commit to josmar-crwdstffng/angular that referenced this issue Apr 8, 2022
@dario-piotrowicz @josmar-crwdstffng
fix(compiler): scope css keyframes in emulated view encapsulation (an…
a26524e 
…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
josmar-crwdstffng pushed a commit to josmar-crwdstffng/angular that referenced this issue Apr 8, 2022
@dylhunn @josmar-crwdstffng
Revert "fix(compiler): scope css keyframes in emulated view encapsula…
606ef67 
…tion (angular#42608)" (angular#45466)

This reverts commit f03e313.

PR Close angular#45466
@dylhunn
Copy link
Contributor

@dylhunn dylhunn commented Apr 11, 2022 •
edited

A status update: this caused one test inside google to fail, which was depending on the keyframe leaking across encapsulation boundaries. The team proposed a fix using ng-deep, which didn't work as this PR doesn't support ng-deep. We will discuss further.

@dylhunn dylhunn added action: merge and removed action: cleanup labels Apr 27, 2022
@dylhunn
Copy link
Contributor

@dylhunn dylhunn commented Apr 27, 2022

The broken g3 test was fixed as per b/230486028. This can now be merged.

@atscott
Copy link
Contributor

@atscott atscott commented Apr 27, 2022

This PR was merged into the repository by commit 4d6a1d6.

dylhunn added a commit to dylhunn/angular that referenced this issue Apr 27, 2022
@dylhunn
Revert "fix(compiler): scope css keyframes in emulated view encapsula…
7857acc 
…tion (angular#42608)"

This reverts commit 4d6a1d6.
@dylhunn dylhunn mentioned this pull request Apr 27, 2022
Closed
14 tasks
dylhunn added a commit that referenced this issue Apr 27, 2022
@dylhunn
Revert "fix(compiler): scope css keyframes in emulated view encapsula…
58e8f4b 
…tion (#42608)" (#45786)

This reverts commit 4d6a1d6.

PR Close #45786
@dylhunn dylhunn reopened this Apr 28, 2022
@dylhunn
Copy link
Contributor

@dylhunn dylhunn commented Apr 28, 2022

This caused a bit of a meltdown in g3. I got paged and had to roll it back. Unclear why the TGP didn't catch all the issues -- I will follow up on what happened, but unfortunately I think we're likely to miss the v14 RC. :/

@dylhunn dylhunn added state: blocked and removed action: merge labels Apr 28, 2022
@dylhunn
Copy link
Contributor

@dylhunn dylhunn commented May 2, 2022 •
edited

So looking back at this, it seems that we broke code that was not tested as part of the TGP (i.e. Guitar tests). Additionally, it was not test-only breakage; it was real pages that are used in production. Nominally we don't support rolling back for non-TGP tests, but I got enough messages from GCP folks that in practice the rollback was inevitable. I suggested considering ViewEncapsulation.None as a band-aid, but that suggestion was not accepted.

Unfortunately, this has probably missed 14, which closes on Wednesday. I still think this is a really valuable change, but we probably need a flag. (Maybe a new ViewEncapsulation option, or something along those lines?) Then it won't be a breaking change, and we can land it in 14.1.

I'm sorry about this @dario-piotrowicz, we could not have foreseen the additional failures. :(

@dylhunn dylhunn removed this from the v14-candidates milestone May 2, 2022
@dylhunn dylhunn added this to the Backlog milestone May 2, 2022
@dylhunn dylhunn dismissed stale reviews from JoostK, gkalpak, petebacondarwin, and jessicajaniuk via 5423f14 May 2, 2022
@dario-piotrowicz
Copy link
Contributor Author

@dario-piotrowicz dario-piotrowicz commented May 2, 2022

@dylhunn I am the one sorry for causing all these issues (I hope you didn't get in trouble because of me!!! 😓)

If the flag is the only option I am totally fine with it, but I believe it still needs to be in the Emulated view encapsulation since it is actually a bug in the emulation and not something different (I checked and the ShadowDom version does provide encapsulation for keyframes, and as far as I know the emulated version should work as closely as possible like the shadow dom one)

@dylhunn
Copy link
Contributor

@dylhunn dylhunn commented May 2, 2022

Ok, thanks for the clarification, we will need to discuss further the exact strategy. And no worries, it happens sometimes!

@Brandinga Brandinga mentioned this pull request Jun 6, 2022
Closed
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petebacondarwin petebacondarwin

@JoostK JoostK

@gkalpak gkalpak

@jessicajaniuk jessicajaniuk

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
breaking changes cla: yes comp: core core: CSS encapsulation flag: breaking change state: blocked target: major
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

css @keyframes leak between components
8 participants
@dario-piotrowicz @petebacondarwin @gkalpak @AndrewKushnir @jessicajaniuk @dylhunn @atscott @JoostK