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.

Sign up for GitHub

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

fix(compiler): handle case-sensitive CSS custom properties #41380

Closed
wants to merge 2 commits into from
Closed

fix(compiler): handle case-sensitive CSS custom properties #41380

wants to merge 2 commits into from

Conversation

crisbeto
Copy link
Member

Currently we normalize all CSS property names in the StylingBuilder which breaks custom properties, because they're case-sensitive. These changes add a check so that custom properties aren't normalized.

Fixes #41364.

@crisbeto
fix(compiler): handle case-sensitive CSS custom properties
21c557e 
Currently we normalize all CSS property names in the `StylingBuilder` which breaks custom properties, because they're case-sensitive. These changes add a check so that custom properties aren't normalized.

Fixes angular#41364.
@google-cla google-cla bot added the cla: yes label Mar 30, 2021
@crisbeto crisbeto marked this pull request as ready for review March 30, 2021 15:29
@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: compiler Issues related to `ngc`, Angular's template compiler target: major This PR is targeted for the next major release labels Mar 30, 2021
@ngbot ngbot bot modified the milestone: Backlog Mar 30, 2021
AndrewKushnir
AndrewKushnir approved these changes Mar 30, 2021
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @crisbeto 👍 I've just left a few minor comments.

I've also noticed that you've applied "target: major" label, just curious if you see any potential risks with this change that might be worth mentioning in commit message or here in PR description?

Thank you.

packages/compiler/src/render3/view/styling_builder.ts Outdated Show resolved Hide resolved
packages/compiler/src/render3/view/styling_builder.ts Outdated Show resolved Hide resolved
packages/core/test/acceptance/styling_spec.ts Show resolved Hide resolved
@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 30, 2021
@AndrewKushnir
Copy link
Contributor

Presubmit.

@crisbeto
Copy link
Member Author

Regarding the targeting, I was just confused whether targeting patch now would go out in the first patch after v12 or now. I don't think it's particularly risky.

@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Mar 30, 2021
@AndrewKushnir AndrewKushnir added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: presubmit The PR is in need of a google3 presubmit labels Mar 30, 2021
@AndrewKushnir
Copy link
Contributor

@crisbeto FYI presubmits are successful for the changes in this PR. I'm adding the "cleanup" label for now to see if you want to change the "target" label (if not, just remove the "cleanup" label). Thank you.

@crisbeto crisbeto added target: patch This PR is targeted for the next patch release and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: major This PR is targeted for the next major release labels Mar 30, 2021
alxhub pushed a commit that referenced this pull request Mar 30, 2021
@crisbeto @alxhub
fix(compiler): handle case-sensitive CSS custom properties (#41380)
f9da272 
Currently we normalize all CSS property names in the `StylingBuilder` which breaks custom properties, because they're case-sensitive. These changes add a check so that custom properties aren't normalized.

Fixes #41364.

PR Close #41380
@alxhub alxhub closed this in e112e32 Mar 30, 2021
TeriGlover pushed a commit to TeriGlover/angular that referenced this pull request Apr 5, 2021
@crisbeto @TeriGlover
fix(compiler): handle case-sensitive CSS custom properties (angular#4…
c3ea7dd 
…1380)

Currently we normalize all CSS property names in the `StylingBuilder` which breaks custom properties, because they're case-sensitive. These changes add a check so that custom properties aren't normalized.

Fixes angular#41364.

PR Close angular#41380
@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 Apr 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

Custom CSS Properties case sensitivity not respected with HostBinding
2 participants
@crisbeto @AndrewKushnir