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

XPath error with style attribute matcher #352

Closed
oliverklee opened this issue Nov 9, 2016 · 7 comments
Closed

XPath error with style attribute matcher #352

oliverklee opened this issue Nov 9, 2016 · 7 comments
Assignees
Labels
Milestone

Comments

@oliverklee
Copy link
Contributor

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:

    div[style*="margin: 16px 0"] {
        margin:0 !important;
    }

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:

    style*="margin: 1

Hope that helps to solve this issue.

@Khaleel
Copy link

Khaleel commented Nov 10, 2017

Was this fixed?

@oliverklee
Copy link
Contributor Author

No, this is not fixed yet.

@oliverklee oliverklee modified the milestones: 2.0.0, 2.1.0 Jan 4, 2018
@oliverklee oliverklee changed the title XPath error with colon in selector XPath error with style attribute matcher Jan 7, 2018
@oliverklee
Copy link
Contributor Author

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?

@zoliszabo
Copy link
Contributor

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.

@jjriv
Copy link
Contributor

jjriv commented Jan 8, 2018

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.

@oliverklee
Copy link
Contributor Author

@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.

@zoliszabo
Copy link
Contributor

zoliszabo commented Jan 10, 2018

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.

@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 p[style*="margin: 16px 0"], but it actually shouldn't.

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

No branches or pull requests

4 participants