Skip to content

Conversation

@puckowski
Copy link
Contributor

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 lint after running tests.

I tested against the repo in #53038 and ng build produced the following CSS:

.test[_ngcontent-ng-c3737221485]{
  animation:_ngcontent-ng-c3737221485_my-anim 1s,_ngcontent-ng-c3737221485_my-anim2 2s
}
@keyframes _ngcontent-ng-c3737221485_my-anim{
  0%{
    color:red
  }
  to{
    color:#00f
  }
}
@keyframes _ngcontent-ng-c3737221485_my-anim2{
  0%{
    font-size:1em
  }
  to{
    font-size:1.2em
  }
}

Fixes #53038

PR Checklist

Please check if your PR fulfills the following requirements:

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 #53038

What is the new behavior?

Current behavior for production builds would produce CSS like the following:

.test[_ngcontent-ng-c3737221485]{
  animation:_ngcontent-ng-c3737221485_my-anim 1s,my-anim2 2s
}

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Will support change requests as needed.

@pullapprove pullapprove bot requested a review from devversion July 1, 2024 23:37
@angular-robot angular-robot bot added the area: compiler Issues related to `ngc`, Angular's template compiler label Jul 1, 2024
@ngbot ngbot bot added this to the Backlog milestone Jul 1, 2024
@JoostK JoostK 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 core: CSS encapsulation labels Jul 2, 2024
@puckowski puckowski force-pushed the fix-css-prod-animation-rule-scope branch 5 times, most recently from 3e4e220 to dcb91fd Compare July 4, 2024 13:10
Copy link
Member

@JoostK JoostK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM

@JoostK JoostK added action: global presubmit The PR is in need of a google3 global presubmit and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jul 4, 2024
@JoostK
Copy link
Member

JoostK commented Jul 4, 2024

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
@puckowski puckowski force-pushed the fix-css-prod-animation-rule-scope branch from dcb91fd to 1f6db2a Compare July 4, 2024 16:07
@JoostK JoostK added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Jul 4, 2024
@JoostK
Copy link
Member

JoostK commented Jul 4, 2024

caretaker note: it is advised to sync this one on its own, assuming TGP ends up green

@pkozlowski-opensource
Copy link
Member

The deflaked TGP came back "green". I'm going to merge and sync this one separately on Monday.

@pkozlowski-opensource
Copy link
Member

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

pkozlowski-opensource pushed a commit that referenced this pull request Jul 9, 2024
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 #53038

PR Close #56800
pkozlowski-opensource pushed a commit that referenced this pull request Jul 9, 2024
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 #53038

PR Close #56800
@puckowski
Copy link
Contributor Author

@JoostK @devversion
Thank you for your support.

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 */
}

@puckowski
Copy link
Contributor Author

I think this PR may have also fixed #49384

I posted on that issue but on 18.2.0-next.0 the 5 animation names are properly scoped in dev/prod builds.

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.

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: global presubmit The PR is in need of a google3 global presubmit action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler core: CSS encapsulation merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multiple CSS Animations are not prefixed during build

4 participants