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.

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

@-webkit-keyframes animationName not supported #886

Closed
janmyszkier opened this issue Jun 13, 2020 · 2 comments · Fixed by #899
Closed

@-webkit-keyframes animationName not supported #886

janmyszkier opened this issue Jun 13, 2020 · 2 comments · Fixed by #899
Assignees
Labels
Milestone

Comments

@janmyszkier
Copy link

created a branch with a test that fails:
https://github.com/janmyszkier/emogrifier/tree/jan/2_2_0_css_animation_test

done based on 2.2.0 (which seems to be currently used by magento/magento2 repository):
https://github.com/magento/magento2/blob/2.3/composer.lock#L3670

please provide a fix or doc information on how to avoid this error with emogrifier, so we can alter our code. (mentioning the animation in the readme would also be helpful if not going to be supported)

@JakeQZ
Copy link
Contributor

JakeQZ commented Jun 14, 2020

Hi @janmyszkier,

Thanks for reporting, and in particular for providing a test that fails.

I can confirm that your test also fails with the latest Emogirifer version, 4.0.0.

However, if I add $subject->setDebug(false); just after $subject = $this->buildDebugSubject(...);, the test passes.

I've also played around with adding some CSS before and after the @-webkit-keyframes rule, and this appears to be processed and inlined correctly (when 'debug' is false).

Could you try your test with the 2.2.0 release but 'debug' set to false and see if that makes a difference? (EDIT: PS. If it does, in your Magento setup, can 'debug' be set to false?)

In the medium-term, I think Emogrifier should handle the various 'unsupported' at-rules as it now does @font-face (see #870) so that they end up in the <style> element added back in to the resultant HTML. (There is a possibility that the ordering could be important, e.g. for @supports, but a solution to that would be more involved - see #544.)

@JakeQZ JakeQZ added the bug label Jun 14, 2020
@JakeQZ JakeQZ added this to the 5.0.0 milestone Jun 14, 2020
JakeQZ added a commit that referenced this issue Jun 14, 2020
All at-rules that have no special handling by Emogrifier will now by copied to a
`<style>` element.  Previously, Emogrifier would attempt to process them as
regular CSS rules, causing an exception in `CssSelectorConverter::toXPath()`,
which in 'debug' mode would be propagated; in either case they would be
discarded.

The rationale is that if any such rules are in the supplied CSS, they are
desired in the resulting HTML.  Emogrifier can treat them as a black box it
knows nothing about, simply to be passed on.

For at-rules which are containers for regular CSS rules (e.g. `@supports`), the
ordering could be important.  For now, these rules will be placed at the start
of the `<style>` element, so it is possible that they will be overridden by
other uninlinable rules that are placed after them, but were originally placed
before.  However, there should be no regression because, by being placed first
(and in their own original order), they would still never override a rule in the
`<style>` element that they would not have in the original CSS.

The ordering improvement can be addressed separately, e.g. by handling
containers, like `@supports`, more generally, in a similarly way to how `@media`
is currently processed.  (Addressing #544 first may also help.)

Fixes #886.
JakeQZ added a commit that referenced this issue Jun 14, 2020
All at-rules that have no special handling by Emogrifier will now by copied to a
`<style>` element.  Previously, Emogrifier would attempt to process them as
regular CSS rules, causing an exception in `CssSelectorConverter::toXPath()`,
which in 'debug' mode would be propagated; in either case they would be
discarded.

The rationale is that if any such rules are in the supplied CSS, they are
desired in the resulting HTML.  Emogrifier can treat them as a black box it
knows nothing about, simply to be passed on.

For at-rules which are containers for regular CSS rules (e.g. `@supports`), the
ordering could be important.  For now, these rules will be placed at the start
of the `<style>` element, so it is possible that they will be overridden by
other uninlinable rules that are placed after them, but were originally placed
before.  However, there should be no regression because, by being placed first
(and in their own original order), they would still never override a rule in the
`<style>` element that they would not have in the original CSS.

The ordering improvement can be addressed separately, e.g. by handling container
at-rules, like `@supports`, more generally, in a similarly way to how `@media`
rules are currently processed.  (Addressing #544 first may also help.)

Fixes #886.
JakeQZ added a commit that referenced this issue Jun 14, 2020
All at-rules that have no special handling by Emogrifier will now by copied to a
`<style>` element.  Previously, Emogrifier would attempt to process them as
regular CSS rules, causing an exception in `CssSelectorConverter::toXPath()`,
which in 'debug' mode would be propagated; in either case they would be
discarded.

The rationale is that if any such rules are in the supplied CSS, they are
desired in the resulting HTML.  Emogrifier can treat them as a black box it
knows nothing about, simply to be passed on.

For at-rules which are containers for regular CSS rules (e.g. `@supports`), the
ordering could be important.  For now, these rules will be placed at the start
of the `<style>` element, so it is possible that they will be overridden by
other uninlinable rules that are placed after them, but were originally placed
before.  However, there should be no regression because, by being placed first
(and in their own original order), they would still never override a rule in the
`<style>` element that they would not have in the original CSS.

The ordering improvement can be addressed separately, e.g. by handling container
at-rules, like `@supports`, more generally, in a similarly way to how `@media`
rules are currently processed.  (Addressing #544 first may also help.)

Fixes #886.
@JakeQZ JakeQZ self-assigned this Jun 14, 2020
@JakeQZ
Copy link
Contributor

JakeQZ commented Jun 14, 2020

@janmyszkier, FYI, this issue should be fixed by #899 which will preserve @keyframes and other at-rules, and will be closed when that is merged, although that fix won't be immediately avaible in a release. That said, it appears you should be able to turn 'debug' off and/or upgrade to 4.0.0 to avoid the exception.

JakeQZ added a commit that referenced this issue Jun 14, 2020
All at-rules that have no special handling by Emogrifier will now by copied to a
`<style>` element.  Previously, Emogrifier would attempt to process them as
regular CSS rules, causing an exception in `CssSelectorConverter::toXPath()`,
which in 'debug' mode would be propagated; in either case they would be
discarded.

The rationale is that if any such rules are in the supplied CSS, they are
desired in the resulting HTML.  Emogrifier can treat them as a black box it
knows nothing about, simply to be passed on.

For at-rules which are containers for regular CSS rules (e.g. `@supports`), the
ordering could be important.  For now, these rules will be placed at the start
of the `<style>` element, so it is possible that they will be overridden by
other uninlinable rules that are placed after them, but were originally placed
before.  However, there should be no regression because, by being placed first
(and in their own original order), they would still never override a rule in the
`<style>` element that they would not have in the original CSS.

The ordering improvement can be addressed separately, e.g. by handling container
at-rules, like `@supports`, more generally, in a similar way to how `@media`
rules are currently processed.  (Addressing #544 first may also help.)

Fixes #886.
JakeQZ added a commit that referenced this issue Jun 15, 2020
All at-rules that have no special handling by Emogrifier will now by copied to a
`<style>` element.  Previously, Emogrifier would attempt to process them as
regular CSS rules, causing an exception in `CssSelectorConverter::toXPath()`,
which in 'debug' mode would be propagated; in either case they would be
discarded.

The rationale is that if any such rules are in the supplied CSS, they are
desired in the resulting HTML.  Emogrifier can treat them as a black box it
knows nothing about, simply to be passed on.

For at-rules which are containers for regular CSS rules (e.g. `@supports`), the
ordering could be important.  For now, these rules will be placed at the start
of the `<style>` element, so it is possible that they will be overridden by
other uninlinable rules that are placed after them, but were originally placed
before.  However, there should be no regression because, by being placed first
(and in their own original order), they would still never override a rule in the
`<style>` element that they would not have in the original CSS.

The ordering improvement can be addressed separately, e.g. by handling container
at-rules, like `@supports`, more generally, in a similar way to how `@media`
rules are currently processed.  (Addressing #544 first may also help.)

Fixes #886.
JakeQZ added a commit that referenced this issue Jun 15, 2020
All at-rules that have no special handling by Emogrifier will now by copied to a
`<style>` element.  Previously, Emogrifier would attempt to process them as
regular CSS rules, causing an exception in `CssSelectorConverter::toXPath()`,
which in 'debug' mode would be propagated; in either case they would be
discarded.

The rationale is that if any such rules are in the supplied CSS, they are
desired in the resulting HTML.  Emogrifier can treat them as a black box it
knows nothing about, simply to be passed on.

For at-rules which are containers for regular CSS rules (e.g. `@supports`), the
ordering could be important.  For now, these rules will be placed at the start
of the `<style>` element, so it is possible that they will be overridden by
other uninlinable rules that are placed after them, but were originally placed
before.  However, there should be no regression because, by being placed first
(and in their own original order), they would still never override a rule in the
`<style>` element that they would not have in the original CSS.

The ordering improvement can be addressed separately, e.g. by handling container
at-rules, like `@supports`, more generally, in a similar way to how `@media`
rules are currently processed.  (Addressing #544 first may also help.)

Fixes #886.
JakeQZ added a commit that referenced this issue Jun 15, 2020
All at-rules that have no special handling by Emogrifier will now by copied to a
`<style>` element.  Previously, Emogrifier would attempt to process them as
regular CSS rules, causing an exception in `CssSelectorConverter::toXPath()`,
which in 'debug' mode would be propagated; in either case they would be
discarded.

The rationale is that if any such rules are in the supplied CSS, they are
desired in the resulting HTML.  Emogrifier can treat them as a black box it
knows nothing about, simply to be passed on.

For at-rules which are containers for regular CSS rules (e.g. `@supports`), the
ordering could be important.  For now, these rules will be placed at the start
of the `<style>` element, so it is possible that they will be overridden by
other uninlinable rules that are placed after them, but were originally placed
before.  However, there should be no regression because, by being placed first
(and in their own original order), they would still never override a rule in the
`<style>` element that they would not have in the original CSS.

The ordering improvement can be addressed separately, e.g. by handling container
at-rules, like `@supports`, more generally, in a similar way to how `@media`
rules are currently processed.  (Addressing #544 first may also help.)

Fixes #886.
JakeQZ added a commit that referenced this issue Jun 15, 2020
All at-rules that have no special handling by Emogrifier will now by copied to a
`<style>` element.  Previously, Emogrifier would attempt to process them as
regular CSS rules, causing an exception in `CssSelectorConverter::toXPath()`,
which in 'debug' mode would be propagated; in either case they would be
discarded.

The rationale is that if any such rules are in the supplied CSS, they are
desired in the resulting HTML.  Emogrifier can treat them as a black box it
knows nothing about, simply to be passed on.

For at-rules which are containers for regular CSS rules (e.g. `@supports`), the
ordering could be important.  For now, these rules will be placed at the start
of the `<style>` element, so it is possible that they will be overridden by
other uninlinable rules that are placed after them, but were originally placed
before.  However, there should be no regression because, by being placed first
(and in their own original order), they would still never override a rule in the
`<style>` element that they would not have in the original CSS.

The ordering improvement can be addressed separately, e.g. by handling container
at-rules, like `@supports`, more generally, in a similar way to how `@media`
rules are currently processed.  (Addressing #544 first may also help.)

Fixes #886.
oliverklee pushed a commit that referenced this issue Jun 15, 2020
All at-rules that have no special handling by Emogrifier will now by copied to a
`<style>` element.  Previously, Emogrifier would attempt to process them as
regular CSS rules, causing an exception in `CssSelectorConverter::toXPath()`,
which in 'debug' mode would be propagated; in either case they would be
discarded.

The rationale is that if any such rules are in the supplied CSS, they are
desired in the resulting HTML.  Emogrifier can treat them as a black box it
knows nothing about, simply to be passed on.

For at-rules which are containers for regular CSS rules (e.g. `@supports`), the
ordering could be important.  For now, these rules will be placed at the start
of the `<style>` element, so it is possible that they will be overridden by
other uninlinable rules that are placed after them, but were originally placed
before.  However, there should be no regression because, by being placed first
(and in their own original order), they would still never override a rule in the
`<style>` element that they would not have in the original CSS.

The ordering improvement can be addressed separately, e.g. by handling container
at-rules, like `@supports`, more generally, in a similar way to how `@media`
rules are currently processed.  (Addressing #544 first may also help.)

Fixes #886.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants