"_ngcontent" inserted into font-url, making it invalid #38587
Comments
sonukapoor
added
area: core
|
I dived into the code, debugged and i found the problem, but I am not 100% which part should be changed because i have 0 knowledge in the angular internal mechanisms. So i am describing my findings here. Maybe some of you have already a solution in mind. I very slightly adjusted the repo mention in the description: I changed the SCSS code of the app-component to the following: @import url('https://fonts.googleapis.com/css2?family=Roboto:wght@400;500;700&display=swap');
div {
color: red;
}FindingsThe problem lies in the function processRules(input, ruleCallback) {This stringreplace-function receives the full SCSS string and applies StringWithEscapedBlocks {
escapedString: '@import url("https://fonts.googleapis.com/css2?family=Roboto:wght@400;500;700&display=swap");\n' +
'div {%BLOCK%}\n',
blocks: [ '\n color: red;\n' ]
}So far the import-line is still correct. This regex splits the import-line wrongly. It generates the following pieces:
Yes, my first intuition was that the regex has some problem.
So my guess is that those are kind of exceptions so that those rules are not processed the same way as "normal" rules so they maybe don't get corrupted?
So my theory of having some pre-checking there is wrong i guess. So this would only leave my initial guess that it is correct that the regex is applied to every rule but there is some bug in the regex itself which breaks certain import-lines apart (the rest of the logic then treats each segment as css-rule and so makes wrong followup-errors based on those segments). I'm not good enough at regex to fix this sadly. Maybe one of the more angular-experienced devs can enlighten me here :) I hope that this finding is helpfull. Regarding the severity of this bug i was having thoughts, because it seems that this bug is existing since multiple major-versions and i have a gut feeling that there are some angular-applications out there where fonts can't be loaded because of this bug but the devs are not checking the network-log but only the console log and so maybe don't even realize that they are using slightly wrong fonts :/ |
…d semicolons The style encapsulation logic currently uses a regex to parse CSS style declarations out of a rule. The problem is that the current regex is a bit too broad in that it matches any valid characters preceeded by a selector and suffixed with a semicolon, which breaks down for `@import` statements that have a URL containing semicolons (e.g. the the URLs provided by Google Fonts like https://fonts.googleapis.com/css2?family=Roboto:wght@400;500&display=swap). These changes resolve the issue by making the regex a bit more narrow in what it's supposed to match. Fixes angular#38587.
…d semicolons
At a high level, the current shadow DOM shim logic works by escaping the content of a CSS rule
(e.g. `div {color: red;}` becomes `div {%BLOCK%}`), using a regex to parse out things like the
selector and the rule body, and then re-adding the content after the selector has been modified.
The problem is that the regex has to be very broad in order capture all of the different use cases,
which can cause it to match strings suffixed with a semi-colon in some places where it shouldn't,
like this URL from Google Fonts `https://fonts.googleapis.com/css2?family=Roboto:wght@400;500&display=swap`.
Most of the time this is fine, because the logic that escapes the rule content to `%BLOCK%` will
have converted it to something that won't be matched by the regex. However, it breaks down for rules
like `@import` which don't have a body, but can still have quoted content with characters that can
match the regex.
These changes resolve the issue by making a second pass over the escaped string and replacing all
of the remaining quoted content with `%QUOTED%` before parsing it with the regex. Once everything
has been processed, we make a final pass where we restore the quoted content.
In a previous iteration of this PR, I went with a shorter approach which narrowed down the
regex so that it doesn't capture rules without a body. It fixed the issue, but it also ended
up breaking some of the more contrived unit test cases. I decided not to pursue it further, because
we would've ended up with a very long and brittle regex that likely would've broken in even weirder
ways.
Fixes angular#38587.
…d semicolons
At a high level, the current shadow DOM shim logic works by escaping the content of a CSS rule
(e.g. `div {color: red;}` becomes `div {%BLOCK%}`), using a regex to parse out things like the
selector and the rule body, and then re-adding the content after the selector has been modified.
The problem is that the regex has to be very broad in order capture all of the different use cases,
which can cause it to match strings suffixed with a semi-colon in some places where it shouldn't,
like this URL from Google Fonts `https://fonts.googleapis.com/css2?family=Roboto:wght@400;500&display=swap`.
Most of the time this is fine, because the logic that escapes the rule content to `%BLOCK%` will
have converted it to something that won't be matched by the regex. However, it breaks down for rules
like `@import` which don't have a body, but can still have quoted content with characters that can
match the regex.
These changes resolve the issue by making a second pass over the escaped string and replacing all
of the remaining quoted content with `%QUOTED%` before parsing it with the regex. Once everything
has been processed, we make a final pass where we restore the quoted content.
In a previous iteration of this PR, I went with a shorter approach which narrowed down the
regex so that it doesn't capture rules without a body. It fixed the issue, but it also ended
up breaking some of the more contrived unit test cases. I decided not to pursue it further, because
we would've ended up with a very long and brittle regex that likely would've broken in even weirder
ways.
Fixes angular#38587.
…d semicolons (#38716) At a high level, the current shadow DOM shim logic works by escaping the content of a CSS rule (e.g. `div {color: red;}` becomes `div {%BLOCK%}`), using a regex to parse out things like the selector and the rule body, and then re-adding the content after the selector has been modified. The problem is that the regex has to be very broad in order capture all of the different use cases, which can cause it to match strings suffixed with a semi-colon in some places where it shouldn't, like this URL from Google Fonts `https://fonts.googleapis.com/css2?family=Roboto:wght@400;500&display=swap`. Most of the time this is fine, because the logic that escapes the rule content to `%BLOCK%` will have converted it to something that won't be matched by the regex. However, it breaks down for rules like `@import` which don't have a body, but can still have quoted content with characters that can match the regex. These changes resolve the issue by making a second pass over the escaped string and replacing all of the remaining quoted content with `%QUOTED%` before parsing it with the regex. Once everything has been processed, we make a final pass where we restore the quoted content. In a previous iteration of this PR, I went with a shorter approach which narrowed down the regex so that it doesn't capture rules without a body. It fixed the issue, but it also ended up breaking some of the more contrived unit test cases. I decided not to pursue it further, because we would've ended up with a very long and brittle regex that likely would've broken in even weirder ways. Fixes #38587. PR Close #38716
|
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. |
🐞 bug report
Affected Package
Seems to be related to SCSS-compiling but seems to be also related to angular itself.
Is this a regression?
It seems this bug is also existing in version 8.2.3 / 8.3.0, most likely in all versions inbetween.
Description
If an angular project uses SCSS and imports a font by url, in some cases angular wrongly inserts a
[_ngcontent-...]into the url so the font cannot be loaded.See reproduction section for details.
🔬 Minimal Reproduction
Created a reproduction repo: https://github.com/mhombach/angularBugReport1
But basically the one step to reproducer is to add the following line into your SCSS file:
If you load this app and look into the network inspector you will see a 400 error for loading the font and the reason is that the above import-url is changed to:
https://fonts.googleapis.com/css2?family=Roboto:wght@400;500[_ngcontent-kph-c11];700&display=swapThis url is btw the official url that google-fonts is giving you for embedding.
🔥 Exception or Error
There is no error other than the browser indication the 400.
🌍 Your Environment
Angular Version:
Anything else relevant?
I tested some patterns that don't produce this error.
Those lines are working:
I think this error is produced when there are more then 2 weight-numbers separated by a semicolon.
Update:
This import line here
produces this url
so this time the insert happens on a different place.
The text was updated successfully, but these errors were encountered: