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.

Sign up for GitHub

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

Jump to bottom

Capitalized elements with classes in CSS not parsing #590

Closed
bclarkau opened this issue Aug 6, 2018 · 6 comments · Fixed by #769
Closed

Capitalized elements with classes in CSS not parsing #590

bclarkau opened this issue Aug 6, 2018 · 6 comments · Fixed by #769
Assignees
@JakeQZ
Labels
bug unit tests needed
Milestone
3.0.0

Comments

@bclarkau
Copy link

bclarkau commented Aug 6, 2018

I'm having a bit of trouble with using Emogrifier with my HTML/CSS. The issue seems to be that the parser does not always pick up elements in the CSS if they are capitalized (eg. "A" instead of "a").

Here's a simplified test:

HTML

<html>
    <body id="email-view">
        <table>
            <tr id="email-body">
                <td>
                    <a href="#">Red</a>
                    <a class="green" href="#">Green</a>
                </td>
            </tr>
        </table>
    </body>
</html>

CSS (note the capitalized A's)

#email-view A {color:red;}
#email-view #email-body A.green {color:green;}

What I expected to see after inlining is that the first link is coloured red, and the second be coloured green. However what I'm seeing is that both links are red.

The above code does work if I change the "A" to lowercase on the second line of CSS:

#email-view #email-body a.green {color:green;}

(the weird thing is that the first capitalized A (red) always works, so the issue seems to be mainly related to capitalized elements with selectors attached eg. "A#green", "A.green", etc)

I had a look to see if I could fix it but couldn't find where the parser picks up these elements?

Thanks!
@oliverklee oliverklee added the bug label Aug 6, 2018
@JakeQZ
Copy link
Contributor

JakeQZ commented Aug 9, 2018

Thanks for the detail and analysis in your report, it does look like a possible bug.

Which version (s) are you testing against or are you using 'master'?

'master' has a change to use the Symfony selector-to-xpath calculator which could affect results since 2.0. There are a few places in the code with comments about, and tests for, allowing case insensitivity wherever it should be. Though the spec has been unclear, the general interpretation is that any reasonable input should be accepted and handled as best as can.

@bclarkau
Copy link
Author

bclarkau commented Aug 9, 2018 •
edited
Loading

No worries, I'm using master (files say version 2.0.0)

@bclarkau
Copy link
Author

I've also just tried v1.2.1 and am seeing the same issue with the test code above.

Thanks a lot for looking into this!

@oliverklee
Copy link
Contributor

@bclarkau Could you please test if the same problem occurs if you use the new CssInliner class?

@bclarkau
Copy link
Author

Thanks for following up.

I ran my above test code again using the CssInliner class (in v2.1.1) and it's working perfectly. I'm seeing the expected inlined output now. I'm also seeing CssInliner working in my actual application.

Looking forward to using it in v3!

@JakeQZ JakeQZ added this to the 3.0.0 milestone Sep 29, 2019
@JakeQZ
Copy link
Contributor

JakeQZ commented Sep 29, 2019

We could probably add a unit test to confirm this is fixed in 3.0 (which will be released soon, probably within a couple of weeks)...

@JakeQZ JakeQZ self-assigned this Oct 1, 2019
JakeQZ added a commit that referenced this issue Oct 1, 2019
@JakeQZ
[TASK] Test uppercase type with class/ID in selector
da5df9a 
Resolves #590.

(Also corrected a typewriter apostrophe in the CHANGELOG.)
@JakeQZ JakeQZ mentioned this issue Oct 1, 2019
Merged
oliverklee pushed a commit that referenced this issue Oct 1, 2019
@JakeQZ @oliverklee
[TASK] Test uppercase type with class/ID in selector (#769)
c8f7fe6 
Resolves #590.

(Also corrected a typewriter apostrophe in the CHANGELOG.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@JakeQZ JakeQZ

Labels
bug unit tests needed
Projects
None yet
Milestone
3.0.0
Development

Successfully merging a pull request may close this issue.

[TASK] Test uppercase type with class/ID in selector MyIntervals/emogrifier
3 participants
@oliverklee @JakeQZ @bclarkau