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): preserve @page rules in encapsulated styles #41915
Conversation
class SomeClass3 {} | ||
class SomeClass4 {} | ||
class SomeClass5 {} | ||
@Directive() |
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.
These changes aren't necessary for the fix, but they prevent focused tests in shadow_css_spec.ts
from failing when run with the Ivy compiler.
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.
I think one could argue that @page
has similar requirements to @font-face
. As such it should be processed in the same way. See da6ed15
I don't mind changing it, although since the spec doesn't allow for
|
Yeah exactly. They should not be encapsulated. But there is an issue if you happen to import a file with such rules from within a rule that contains |
Currently the compiler treats `@page` rules in the same way as `@media`, however that is incorrect and it results in invalid CSS, because `@page` allows style declarations at the root (e.g. `@page (margin: 50%) {}`) and it only allows a limited set of at-rules to be nested into it. Given these restrictions, we can't really encapsulate the styles since they apply at the document level when the user tries to print. These changes make it so that `@page` rules are preserved so that we don't break the user's CSS. More information: https://www.w3.org/TR/css-page-3 Fixes angular#26269.
acedaf8
to
004d086
Compare
Sorry for the delay @petebacondarwin, I've addressed the feedback. I also managed to remove a bit of logic from the code that handles |
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.
LGTM
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.
Looks great, thanks @crisbeto 👍 Just left a comment on adding more docs (based on the info from PR description) into the code.
Currently the compiler treats `@page` rules in the same way as `@media`, however that is incorrect and it results in invalid CSS, because `@page` allows style declarations at the root (e.g. `@page (margin: 50%) {}`) and it only allows a limited set of at-rules to be nested into it. Given these restrictions, we can't really encapsulate the styles since they apply at the document level when the user tries to print. These changes make it so that `@page` rules are preserved so that we don't break the user's CSS. More information: https://www.w3.org/TR/css-page-3 Fixes #26269. PR Close #41915
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. |
Currently the compiler treats
@page
rules in the same way as@media
, however that is incorrect and it results in invalid CSS, because@page
allows style declarations at the root (e.g.@page { margin: 50%; }
) and it only allows a limited set of at-rules to be nested into it. Given these restrictions, we can't really encapsulate the styles since they apply at the document level when the user tries to print.These changes make it so that
@page
rules are preserved so that we don't break the user's CSS.More information: https://www.w3.org/TR/css-page-3
Fixes #26269.