peterbe / premailer

Turns CSS blocks into style attributes
https://premailer.io
BSD 3-Clause "New" or "Revised" License
1.06k stars 188 forks source link

Style sorting breaking proper precedence #260

Closed jack-guy closed 2 years ago

jack-guy commented 2 years ago

For the following styles:

.foo {
  padding-left: 6px;
  padding-right: 6px;
  padding-top: 8px;
  padding-bottom: 8px;
  padding: 4px;
}

The browser should see the padding rules as all having the same precedence. As a tie-breaker, whichever appears last in the list applies (in this case padding: 4px).

Premailer appears to be sorting styles alphabetically in csstext_to_pairs. This causes the padding rule to appear before the others and breaks the precedence.

Is there any reason to be sorting styles in this way? Would you be open to a PR removing this sort?

> /virtualenv_run/lib/python3.7/site-packages/premailer/premailer.py(473)transform()
-> for item in items:
(Pdb) style
'font-family:"Open Sans", "Helvetica Neue", Helvetica, Arial, sans-serif;font-weight:normal;font-size:14px;line-height:20px;color:rgba(43, 39, 60, 1);text-align:left;margin-bottom:16px;padding-left:6px;padding-right:6px;padding-top:8px;padding-bottom:8px;padding:5px'
(Pdb) processed_style
[('color', 'rgba(43, 39, 60, 1)'), ('font-family', '"Open Sans", "Helvetica Neue", Helvetica, Arial, sans-serif'), ('font-size', '14px'), ('font-weight', 'normal'), ('line-height', '20px'), ('margin-bottom', '16px'), ('padding', '5px'), ('padding-bottom', '8px'), ('padding-left', '6px'), ('padding-right', '6px'), ('padding-top', '8px'), ('text-align', 'left')]
peterbe commented 2 years ago

I have no idea/memory why it sorts them. Seems odd. Try removing it and see if the tests break. Also, perhaps you should add a test that uses that great example of padding-left: ....; padding: ... to assert that output.find('padding:') > output.find('padding-left:') or something. Go for it!

jack-guy commented 2 years ago

@peterbe thanks for the quick response! Opened https://github.com/peterbe/premailer/pull/261 - if you're able to merge / release this change that would be a big help to me!

Cheers

peterbe commented 2 years ago

if you're able to merge / release this change that would be a big help to me!

I'll say it now so I don't forget. Please remind me to kick off a release once this is merged. Just in case I forget.