-
Notifications
You must be signed in to change notification settings - Fork 154
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
XPath error with style attribute matcher #352
Comments
Was this fixed? |
No, this is not fixed yet. |
This particular matcher works on the style attribute. The style attribute is a moving target during the Emogrifier processing, and any matchers against it will be highly unreliable. I propose to document that Emogrifier does not officially support matchers against style nodes, and that those should not be used. @jjriv @zoliszabo Thoughts on this? |
You are perfectly right with this expression being highly unreliable in Emogrifier's case. The only use-case I can imagine for such expressions is in media query blocks in order to implement responsiveness. On the other hand, wouldn't this be resolved with #449? This would ensure that such expressions are recognized (in media query blocks). Documenting the unreliability outside of media query blocks would be a good thing. |
I don't understand the issue well enough to offer my thoughts on this one. If you'd like, I can have one of our engineers here weigh in. But, what @zoliszabo says sounds good to me. I'm going to defer judgement to both of you on this one. |
@zoliszabo There are two things at play here: One is that Emogrifier currently cannot build the correct Xpath from this selector (which would be resolved by #449), and the other is that even with the correct Xpath, the Xpath might not match the at-that-moment state of the HTML as the attributes change while Emogrifier does its work. Closing this ticket for now. |
@oliverklee While inlining styles, the result of these kind of selectors is indeed unpredictable and we can/should document this. On the other side, when Emogrifier is parsing media query block selectors (in order to find out which ones to keep), the inline styles are already final and don't change anymore, so they could be matched. And this will work out-of-the-box after handling #449. Of course, there can be edge cases even here, for example: <style>
p {margin: 16px 0}
p.c {margin-top:10px}
@media screen and ... { p[style*="margin: 16px 0"] {...} }
</style>
<p class="c"></p> This would produce the following HTML: <p class="c" style="margin: 16px 0; margin-top: 10px"></p> Which then is matched by |
From #350 (comment) (comment by @Bigwebmaster):
I am also getting this same error with an email template view that I am passing through to emogrifier. I narrowed down the cause to the error to this exact CSS rule:
When I take that rule out, everything works with no errors.
To narrow it down further, if I leave the rule in above but just remove the colon after margin, then there is no fatal error. So it appears the colon in the selector is the cause of the bug:
Hope that helps to solve this issue.
The text was updated successfully, but these errors were encountered: