-
Notifications
You must be signed in to change notification settings - Fork 27k
fix(compiler): fix CSS animation rule scope #56800
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(compiler): fix CSS animation rule scope #56800
Conversation
3e4e220 to
dcb91fd
Compare
JoostK
left a comment
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.
Thanks, LGTM
|
Marking as TGP candidate as such fixes can affect existing code in surprising ways that are hard to track down. I might even go as far as suggesting to sync this one on its own. |
It is valid CSS to list keyframe names in an animation declaration only separating the names with a comma and no whitespace. This is typical of production builds. Updated a couple of regexes and added a couple of tests to account for this scenario. Fixes angular#53038
dcb91fd to
1f6db2a
Compare
|
caretaker note: it is advised to sync this one on its own, assuming TGP ends up green |
|
The deflaked TGP came back "green". I'm going to merge and sync this one separately on Monday. |
|
This PR was merged into the repository by commit 387e1cb. The changes were merged into the following branches: main, 18.0.x, 18.1.x |
|
@JoostK @devversion I posted in #49068 but I believe this PR also fixed #49068. Issue pertained to an animation declaration with two or more keyframe names where not all of the keyframe names were preceded by 1 or more whitespace (could be caused by a prod build stripping whitespace). In my testing, the issue is resolved once this PR makes its way to an NPM release. Maybe #49068 can be closed as well. Issue CSS from #49068 : @keyframes foo { ... }
@keyframes bar { ... }
#my-div {
/* prettier-ignore */
animation: bar 1s infinite,foo 1s infinite; /* THIS BUG: foo is not processed */
animation: bar 1s infinite, foo 1s infinite; /* OK: foo is processed */
animation: 1s infinite bar,1s infinite foo; /* OK: foo is processed */
} |
|
I think this PR may have also fixed #49384 I posted on that issue but on In the screenshot provided in #49384 the animations in the list were not separated by one or more whitespace characters which this PR addressed (by supporting animations not separated by one or more whitespace characters). Worth someone else taking a look and potentially closing as resolved. |
|
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. |
The replacement function is executed for each match. In production builds the regex prevented two or more matches. I updated the regex used to support values which use CONTENT_ATTR
_ngcontent-${COMPONENT_VARIABLE}and COMPONENT_VARIABLE%COMP%as the selector for previous matches.I also ran
yarn run lintafter running tests.I tested against the repo in #53038 and
ng buildproduced the following CSS:Fixes #53038
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?
Issue number #53038
What is the new behavior?
Current behavior for production builds would produce CSS like the following:
Does this PR introduce a breaking change?
Other information
Will support change requests as needed.